Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

commitBlobTransaction: fix nil pointer #23

Open
wants to merge 3 commits into
base: op-es
Choose a base branch
from
Open

commitBlobTransaction: fix nil pointer #23

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cc702b9
fix nil pointer
blockchaindevsh Dec 26, 2024
b2bc505
add comment
blockchaindevsh Dec 27, 2024
80a30e2
address comments
blockchaindevsh Dec 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 13 additions & 3 deletions miner/worker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -389,14 +389,24 @@ func (miner *Miner) commitTransaction(env *environment, tx *types.Transaction) e

func (miner *Miner) commitBlobTransaction(env *environment, tx *types.Transaction) error {
sc := tx.BlobTxSidecar()
if sc == nil && env.isEffectivelySequencing() /* we want to allow blob tx without blobs when it's deriving */ {
panic("blob transaction without blobs in miner")
var nblobs int
if sc == nil {
if env.isEffectivelySequencing() /* we want to allow blob tx without blobs when it's deriving */ {
panic("blob transaction without blobs in sequencing")
} else { // deriving, which does not have the sidecar
nblobs = len(tx.BlobHashes())
}
} else {
if !env.isEffectivelySequencing() {
panic("blob transaction with blobs in derivation")
}
nblobs = len(sc.Blobs)
}
// Checking against blob gas limit: It's kind of ugly to perform this check here, but there
// isn't really a better place right now. The blob gas limit is checked at block validation time
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@qizhou suggests addressing this case at line 392 to make the code self-explanatory and explicitly handle all possible scenarios.

Suggested change
// isn't really a better place right now. The blob gas limit is checked at block validation time
nblobs := 0
if sc == nil {
if env.isEffectivelySequencing() /* we want to allow blob tx without blobs when it's deriving */ {
panic("blob transaction without blobs in sequencing")
} else { // deriving, which does not have the sidecar
nblobs = len(tx.BlobHashes())
}
} else {
if !env.isEffectivelySequencing() {
panic("blob transaction with blobs in derivation")
}
nblobs = len(sc.Blobs)
}
// should we check tx.BlobHashes() matches that of sc.Blobs.BlobHashes()?
// Checking against blob gas limit: It's kind of ugly to perform this check here, but there
// isn't really a better place right now. The blob gas limit is checked at block validation time
// and not during execution. This means core.ApplyTransaction will not return an error if the
// tx has too many blobs. So we have to explicitly check it here.
if (env.blobs+nblobs)*params.BlobTxBlobGasPerBlob > params.MaxBlobGasPerBlock {
return errors.New("max data blobs reached")
}

Copy link
Collaborator Author

@blockchaindevsh blockchaindevsh Dec 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 80a30e2.

// and not during execution. This means core.ApplyTransaction will not return an error if the
// tx has too many blobs. So we have to explicitly check it here.
if (env.blobs+len(sc.Blobs))*params.BlobTxBlobGasPerBlob > params.MaxBlobGasPerBlock {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the issue is due to sc == nil, I prefer to handle sc directly with if sc != nil {}, as the code is self-explanatory and doesn’t need additional comments.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I don't think so, more safety checking is better than no checking.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean check ("handle" is misleading)sc directly (instead of tx.BlobHashes()).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we check whether len(tx.BlobHashes()) matches len(sc.Blobs), or is this validation handled elsewhere in the code?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When sc != nil, it must be sequencing. In this mode, the blob tx must be pulled from the tx pool, which has already done the validation. So it's not necessary to check it again here.

if (env.blobs+nblobs)*params.BlobTxBlobGasPerBlob > params.MaxBlobGasPerBlock {
return errors.New("max data blobs reached")
}
receipt, err := miner.applyTransaction(env, tx)
Expand Down