Skip to content

miner: fix 4844 max check #28028

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

Closed
wants to merge 1 commit into from
Closed

miner: fix 4844 max check #28028

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion miner/worker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -761,7 +761,7 @@ func (w *worker) commitBlobTransaction(env *environment, tx *types.Transaction)
// 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+len(sc.Blobs))*params.BlobTxBlobGasPerBlob > params.MaxBlobGasPerBlock {
if (env.blobs + len(sc.Blobs)) > params.MaxBlobGasPerBlock/params.BlobTxBlobGasPerBlob {
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, what's the difference though?

Btw, perhaps we can define a constant in params package stands for the maximum blob allowed in a single block.

Copy link
Member Author

Choose a reason for hiding this comment

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

In the original it was adding blob count to blob gas used by the side car, which is two different quantities.

Copy link
Member Author

Choose a reason for hiding this comment

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

It might be nice to have a MaxBlobsPerBlock value also though. I need to look and see how it else we use it. I'm also not sure how we'll want to deal with this value if it is going to change between forks.

Copy link
Contributor

Choose a reason for hiding this comment

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

I still don't see what changed. This is what I see:
1: if (a + b) * c > d
2: if (a + b) > d / c

Help

Copy link
Member

Choose a reason for hiding this comment

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

The only change I can see between the original and this one is that it's less prone to overflows since the multiplication over "untrusted" quantities is replaced by a division over known quantities. That said, the txpool should validate that a tx cannot have insanely many blobs for this overflow to be a relevant concern.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh wow you're right, I was missing the parentheses there. It was already correct.

return nil, errors.New("max data blobs reached")
}

Expand Down