-
Notifications
You must be signed in to change notification settings - Fork 998
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.
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
Update block's blob_kzg_commitments
size limit to MAX_BLOB_COMMITMENTS_PER_BLOCK (4096)
#3338
Conversation
This doesn't work because every validator has to download 128MB ( Currently the value is just 4*128KB = 512KB which is far more possible. |
This is a top limit with sharding in mind where clients are not downloading all blobs but doing DAS, but a super proposer can feed it into network The actual/effective limit of blobs in a block come from |
Also not super hungup on the |
It's true that 1024 is not high with sharding in mind, but this is not sharding, it's 4844. I think 512KB (4 blobs) is the highest limit that we can have. I'm not sure if 1MB (8 blobs) will work in the network and I think 2MB (16 blobs) is quite dangerous. Since the concern is in the CL network and independent from EL, this limit should be controlled not only by EL but also CL. Quotes from ethereum/execution-apis#404
If the EL wants to increase the data gas limit, I think it's better to let the EL increase it to only some extent. If the EL wants to increase more, it should wait for the CL hard fork. cc's network people |
My intuition is based on a fact that EL limiting a number of blobs per block is more natural than CL handling it. This intuition might be wrong if |
I understand the desire to leave the param with room to grow here so EL can be independently forked to increase the gas limit. That said, changing it from 4844 regime to DAS regime should certainly be done with substantial coordination between the layers. If we give room for buffer here, I would argue to just go to 2MB max ( |
sounds good, will make an update to 16 for 4844 regime |
presets/mainnet/deneb.yaml
Outdated
@@ -4,5 +4,5 @@ | |||
# --------------------------------------------------------------- | |||
# `uint64(4096)` | |||
FIELD_ELEMENTS_PER_BLOB: 4096 | |||
# `uint64(2**2)` (= 4) | |||
MAX_BLOBS_PER_BLOCK: 4 | |||
# `uint64(2**4)` (= 16) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given how sensitive this is should a note be made somewhere that capacity is actually limit in exec layer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added the note in p2p interface
I don't understand the argument here. Can't we just agree that the real limit is coded in the EL and this is purely the theoretical list limit? What would be the justification for "to change it up to 16, you only need to modify EL; but if you want to go above 16, both EL and CL need to be changed"? This makes no sense to me. Just having one constand that defines this in one place seems significantly less confusing. |
just a point to note: this would not really be theoretical in the sense that CL will subscribe to these many |
So lining up the problems -
Its current value (4) is derived from the numbers that we believe fall within network capacity limits (max size of each blob * number of blobs per block), ie we don't want the EL to frivolously increase them without involvement from the CL because the CL has to deal with the fallout of the change. The alternative is to "dimension" the CL for the full value from start, which we're not comfortable doing, else we'd be doing it already. We cannot re-dimension the CL post-fact without a HF (gossip topics, spam protection limits etc but also promises we make to users with regards to bandwidth consumption when they set up their rigs). Of all these constraints, the execution API is the least significant reason to increase the limit "just in case" - it can (likely) be done without a HF and users will have reasonable incentive to upgrade outside of the HF should this become a reality. I would complete disregard its upgrade path for the purposes of deciding on a value here. re I'd thus:
|
I think this is a great idea! |
I don't understand this one. With If so, I think |
I decoupled the gossipsub topics from |
no what it means is that But its a list to it will always be filled less than |
But what will it independently do w.r.t. So now we have three constants around the same:
I think, what you mean is 2nd only since EL already has its independent data gas mechanism of defining its limit (actual limit calculation on EL side would however be : |
I'm not sure if we have a 'target' staker, but many home stakers running a handful of validators on residential DSL plans already struggle from time to time on bandwidth, and adding to that 2MB of data to download, and more importantly upload, might exclude that class of validator from our network, as they won't be able to upload fast enough to participate any more... I'm hoping that at least for 4844 we can have a solution that doesn't exclude them... Admittedly this should improve with full sharding, but in the meantime we should continue to consider our sizings very carefully, or be very transparent that DSL is going to be severely limiting to the home staker... |
the real limit right now could be controlled by |
presets/mainnet/deneb.yaml
Outdated
# `uint64(2**2)` (= 4) | ||
MAX_BLOBS_PER_BLOCK: 4 | ||
# `uint64(2**4)` (= 16) | ||
MAX_BLOBS_PER_BLOCK: 16 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should remain 4: that already represents a significant increase in traffic on the gossipsub network
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rolledback
I don't feel we're ready for this just yet given that the block itself also can touch this size, totaling 4mb for a block - since the limit no longer has long-term consensus effects, all that's needed to change it is a coordination hard fork and we can gradually increase. For increasing the limit, we should also consider other bandwidth-saving techniques (lazy sending, potentially episub, better attestation subnet subscription handling etc) |
No, If As mentioned in #3346, the advantage of doing this is that we can change For example, if we want to change |
The reason why we shouldn't put the limit only in the EL is due to the philosophy of the layered protocol design. We should design the CL to work fine even with any silly EL (not only with the EL with the max data gas limiting). For example, let's say we have a silly EL that wants to publish 1024 blobs and the CL doesn't have a limit, the network will collapse. Why do we need to think about such the silly EL even if it will not really exist? Let me explain. All the layered protocols in the Internet have the contracts that the upper layer and the lower layer must satisfy and it will work fine with any silly upper and lower protocol, as long as they satisfy the contracts. The benefit of this principle is that it allows us to change the protocol at any layer as long as the new protocol satisfies the contracts between layers. For example, HTTP says that it will work fine as long as the lower protocol provides an in-order and reliable connection. Even if the lower protocol is silly, as long as it satisfies the contract, HTTP will work fine. That's why we can use HTTP not only over TCP, but also TLS, QUIC, and Unix Domain Socket, but not UDP, because UDP doesn't provide a reliable connection. Another example is TCP, TCP says that it will work fine as long as the upper protocol is connection-oriented, stream-oriented (doesn't care about the write boundary), and resilient to delay, so TCP will work fine with HTTP, etc., but not DNS and NTP because DNS is not stream-oriented and NTP is sensitive to delay. In conclusion, due to this principle, we should ensure that the CL will work fine as long as the upper layer doesn't publish more than The benefit of this principle is that it allows anyone to just use our CL and design their own EL which satisfies the contract that it will not publish more than Without this principle, the EL people will have to work very closely to the CL people, i.e. the EL people will have to look at the CL p2p network spec and decide what is the right value for If we think that the third-party EL is not an issue and decide to not follow this principle, I think it's okay, but the EL and CL will be really coupled and they are not really layers according to the norm definition. |
It is set to a reasonable number (4): the number of blobs that we're comfortable supporting - you're right that the fact that it coincides with the subnet count is an implementation detail on the CL side - it's merely offered as the reason why we're uncomfortable going higher: doing so has complex side effects on the CL side that we're working to resolve: once we've solved them, the number can go up. Re layers, you bring up an excellent example - take UDP: it is one of the layers - part of its "interface" to the next layer is the packet size which tends to hover in the 600:ish bytes range: it's a limit that you, when designing a protocol on top of UDP, have to take into account (just like the packet reordering and so on) - as long as you stay within this limit, you're fine. Layered protocols rely on artifacts like |
Yeah, I meant I'm against setting it to an unreasonable number. (like 1024)
I didn't know what to call it, so I called it a contract between layers. It probably should be called an interface instead.
As there is an question in this PR on why we shouldn't do the limit only in the EL, I brought up the layering thing to explain why we should also have a limit in the CL. |
I agree, but then Also its not really an independent CL variable because EL has to limit its blob production per block to the min of ( MAX_DATA_GAS_PER_BLOCK // DATA_GAS_PER_BLOB, may be we are just arguing over some hypothetical here since all the constants right now have to be in tandem together and will upgrade in tandem. The two fruitful outcomes of this discussion as per me are:
|
Strongly agree, but I think it should be in def process_blob_kzg_commitments(body: BeaconBlockBody) -> None:
assert len(body.blob_kzg_commitments) <= MAX_BLOBS_PER_BLOCK # Added
assert verify_kzg_commitments_against_transactions(body.execution_payload.transactions, body.blob_kzg_commitments) ** Please also don't forget to add the pyspec test for this. |
I think this function is going away since CL computed versioned hashes (from CL commitments) will now be verified as part of new_payload call, |
Right, the commitments aren't "operations". I was thinking about the precedent of checking some length constraints in It could either got in If we go with this suggestion here #3359 (comment), I'd recommend |
awesome will update the check in |
97a075e
to
8ccc257
Compare
have moved |
As we don't have a conclusion whether we want to follow #3359 (comment) and we don't know when #3359 will be merged, I think it's better to put the length check in
Yes, that's right. I agree with that. |
updated! 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would probably remove the reference to the execution layer constants.
Let's wait on #3359 and then move this into process_execution_payload
specs/deneb/beacon-chain.md
Outdated
@@ -68,7 +68,7 @@ This upgrade adds blobs to the beacon chain as part of Deneb. This is an extensi | |||
|
|||
| Name | Value | | |||
| - | - | | |||
| `MAX_BLOBS_PER_BLOCK` | `uint64(2**2)` (= 4) | | |||
| `MAX_BLOB_COMMITMENTS_PER_BLOCK` | `uint64(2**12)` (= 4096) | hardfork independent fixed theoretical limit same as `LIMIT_BLOBS_PER_TX` (see EIP 4844) | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| `MAX_BLOB_COMMITMENTS_PER_BLOCK` | `uint64(2**12)` (= 4096) | hardfork independent fixed theoretical limit same as `LIMIT_BLOBS_PER_TX` (see EIP 4844) | | |
| `MAX_BLOB_COMMITMENTS_PER_BLOCK` | `uint64(2**12)` (= 4096) | hardfork independent fixed theoretical limit same as `LIMIT_BLOBS_PER_TX` (see EIP 4844) | |
@@ -40,6 +40,8 @@ | |||
|
|||
This upgrade adds blobs to the beacon chain as part of Deneb. This is an extension of the Capella upgrade. | |||
|
|||
The blob transactions are packed into the execution payload by the EL/builder with their corresponding blobs being independently transmitted and are limited by `MAX_DATA_GAS_PER_BLOCK // DATA_GAS_PER_BLOB`. However the CL limit is independently defined by `MAX_BLOBS_PER_BLOCK`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I would put this reference to execution-layer constraints here.
Definitely don't need a reference to "builder", and there is no definition of EL in the consensus-layer specs at this point. If we want to reference that layer use ExecutionEngine
FYI I'm working on tests and pushing commits |
New validation: | ||
|
||
- _[REJECT]_ The length of KZG commitments is less than or equal to the limitation defined in Consensus Layer -- | ||
i.e. validate that `len(body.signed_beacon_block.message.blob_kzg_commitments) <= MAX_BLOBS_PER_BLOCK` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@djrtwo I added the p2p validation here. Feel free to open a post-merge PR to revise it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, makes sense!. definitely need the p2p limit given the type isn't protecting us from dos
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank @g11tech
LGTM
I'm going to merge it and run test gen. We can apply proofread suggestions later if need.
UPDATE:
MAX_BLOB_COMMITMENTS_PER_BLOCK
for setting list limit to beacon block'sblob_kzg_commitments
. (thanks @arnetheduck for bringing up the idea)MAX_BLOBS_PER_BLOCK
in p2p layer and elsewhere--------------------------------------Previously---------------------------------------
As per discussion in execution specs
it would be nice to have a higher
MAX_BLOBS_PER_BLOCK
limit formainnet
spec which can independently allow ELs to hardfork and increase data gas limit (and hence number of blobs per block)Suggesting a limit of
1024
withsharding
into account.cc @mkalinin