miner: fix 4844 max check #28028
miner: fix 4844 max check #28028
Conversation
| @@ -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 { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
In the original it was adding blob count to blob gas used by the side car, which is two different quantities.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Oh wow you're right, I was missing the parentheses there. It was already correct.
There was a small error in #27875 where I conflated blob count with blob gas used. This PR rectifies the issue.