Add blob schedule support #15272
Add blob schedule support #15272
Conversation
terencechain
force-pushed
the
blob-schedule
branch
2 times, most recently
from
815868e to
787a904
Compare
api/client/builder/types.go
Outdated
| if len(bb.BlobKzgCommitments) > params.BeaconConfig().MaxBlobsPerBlockByVersion(version.Electra) { | ||
| return nil, fmt.Errorf("blob commitment count %d exceeds the maximum %d", len(bb.BlobKzgCommitments), params.BeaconConfig().MaxBlobsPerBlockByVersion(version.Electra)) | ||
| if len(bb.BlobKzgCommitments) > params.BeaconConfig().MaxBlobsPerBlock(slot) { | ||
| return nil, fmt.Errorf("blob commitment count %d exceeds the maximum %d", len(bb.BlobKzgCommitments), params.BeaconConfig().MaxBlobsPerBlock(slot)) |
There was a problem hiding this comment.
Maybe can we set the result of params.BeaconConfig().MaxBlobsPerBlock into a variable to avoid double calls?
config/params/config.go
Outdated
| } | ||
|
|
||
| if v >= version.Electra { | ||
| case epoch >= b.ElectraForkEpoch: |
There was a problem hiding this comment.
What's the advantage to go from an if pattern to a switch pattern here?
As a personal taste, I find the if pattern clearer.
There was a problem hiding this comment.
Also, do we still need those Deneb/Electra cases if they are already overwritten in blob schedule?
https://github.com/GabrielAstieres/consensus-specs/blob/7c5b431f40b71bedc2cc5a07a16d8be45d8abfe1/configs/mainnet.yaml#L196-L202
There was a problem hiding this comment.
I think once there are more than two if statements, using a switch becomes cleaner. But I don't feel strongly about this nit, so feel free to overrule me if anyone has a strong preference.
I'm also not sure if we still need this, my only concern was someone running the latest Prysm version but loading an old mainnet YAML without a blob schedule, which could cause issues. This acts as a backup safety check that we can remove later if needed.
There was a problem hiding this comment.
but thinking more even if loads an old yaml, the latest prysm version should still have blob version schedule as default, I think we are fine. Removing them now
terencechain
force-pushed
the
blob-schedule
branch
3 times, most recently
from
4442b1f to
84ab56e
Compare
terencechain
force-pushed
the
blob-schedule
branch
2 times, most recently
from
51bdd19 to
da62dc6
Compare
james-prysm
left a comment
james-prysm
left a comment
There was a problem hiding this comment.
LGTM i think might want to wait for manu?
terencechain
force-pushed
the
blob-schedule
branch
from
914164d to
b78839f
Compare
terencechain
force-pushed
the
blob-schedule
branch
from
b78839f to
74300c2
Compare
config/params/config.go
Outdated
| if epoch >= b.FuluForkEpoch { | ||
| return b.DeprecatedMaxBlobsPerBlockFulu | ||
| if len(b.BlobSchedule) > 0 { | ||
| // Assume BlobSchedule is already sorted ascending by Epoch |
There was a problem hiding this comment.
How do you enforce this? That isn't a guarantee?
config/params/config.go
Outdated
| if epoch >= b.FuluForkEpoch { | ||
| return b.DeprecatedMaxBlobsPerBlockFulu | ||
| if len(b.BlobSchedule) > 0 { | ||
| // Assume BlobSchedule is already sorted ascending by Epoch |
There was a problem hiding this comment.
I think this assumption is bad when we could implement a struct or something to reliably return the correct data, sorted or unsorted.
There was a problem hiding this comment.
But that adds implementation complexity and comes at a cost.
My thinking is there's no real difference between using a config with completely wrong values and using one where the values are just in the wrong order. Either way, your code won’t work. If the blob schedule isn't sorted correctly, it’s just as bad as having invalid values, and the spec does require it to be sorted.
terencechain
force-pushed
the
blob-schedule
branch
from
74300c2 to
3cba0d4
Compare
terencechain
force-pushed
the
blob-schedule
branch
from
3cba0d4 to
c3966f1
Compare
* Add blob schedule support * Feedback * Fix e2e test * adding in temporary fix until web3signer is supported for blob schedule in config * Add fallback * Uncomment blob schedule from plcae holder fields for test * Log blob limit increase * Sort --------- Co-authored-by: james-prysm <[email protected]>
* Add blob schedule support * Feedback * Fix e2e test * adding in temporary fix until web3signer is supported for blob schedule in config * Add fallback * Uncomment blob schedule from plcae holder fields for test * Log blob limit increase * Sort --------- Co-authored-by: james-prysm <[email protected]>
This PR adds blob schedule support from ethereum/consensus-specs#4277
Note to the reviewer:
MaxBlobsPerBlockByVersionis no longer safe to use after BPO, so it's better to remove it. Its only usage is in mev-boost to check that the number of blobs doesn't exceed the limit, and in that case, we can just pass the slot directly into the function.