Update EIP-7928: Amend storage-read gas-feasibility check - #12277
Conversation
|
✅ All reviewers have approved. |
|
The commit 16f47f0 (as a parent of cc1c9f4) contains errors. |
16f47f0 to
92fdaa9
Compare
|
I have given this some thought and in order for this EIP to be forward-compatible I agree with removing this. This "optimization" is somewhat implicit. However, I think we should alter this sentence in the Security Considerations to point the clients (now, but also in the future e.g. in 1 year, 3 years, 5 years) to note a client should create a mechanism to quickly invalidate BALs which are clearly not possible. I think that execution-specs benchmarks will flag this for clients, but I believe a pointer directly in this EIP would be helpful. Then (future) clients can think about this before running these benchmarks. (this is a condensed version of a conversation we had on Discord, @fselmo please add thoughts or comment if you think this statement is not entirely correct) |
Yeah good summary and agreed 👍🏼. The BAL benchmarks can be a good home for this heuristic and it may be good at some point to have red to green marks for clients for tests like these on a public dashboard. |
|
I was interpreting as G_remaining starting at the block gas limit. Then, with enough buffer by using 2000 instead of 2100, and having 87 items from pre and post system contract txs at max, we would never invalidate a valid block early. The intent here is to allow earlier validation for worst-cases. This isn't a optimization for the happy path. So, we can assume that the full block gas available is used (today e.g. 60M). |
The buffer argument works for user-paid reads but not for the system call reads. Every user At the final boundary of a full block I've been working on test fixtures in ethereum/execution-specs#3484 for exactly this boundary (a valid block with 983,999 gas unused and 492 pending system reads, plus a fully packed one). Clients can still invalidate early soundly, as Nethermind does by counting only non-system-contract reads against the declared Note that the tests in ethereum/execution-specs#3484 will fill and I believe will pass on all clients but that's because no one is implementing this check afaict, other than the Nethermind one that does not include system contract reads in ^ apologies if any of the above is repeating myself, just trying to clarify it 😅 . Got some data from the tests and from Claude reviews. |
@jochem-brouwer, any thoughts on what this text should be? Something like this perhaps, maybe shorter or less specific to reads? |
|
I think the section is good. I would only change "phantom" to "spurious", like we refer to such in other parts |
|
I wouldn't remove the section but just add some buffer for the system-contract storage reads. |
If we can keep a low complexity here in the suggestion that doesn't age, like adding a large enough buffer to account for system contract reads in a way that is future-proof, this is a good alternative. I couldn't think of a great way to do this where the check couldn't end up getting stale, but I think this could work if we can get there.
Sounds good. For now I will change the PR to use this, but open to ideas on the above suggestion ^ too 👀 |
6c9c25c to
f1e7dcd
Compare
|
EDIT: ignore this message, I realized later that this section is about the storage read part specifically, and I was thinking about this for the size of the entire BAL (so including writes). I will keep the text here though. OG textI just realized something which will directly be the reason why I signal that we should remove this section from the EIP and make clients (not this EIP) responsible for an approach to quickly reject invalid BALs. We should however note that clients should be aware of this and that they find their own strategy on how to quickly reject invalid BALs (in particular: BALs which are too large). The assumption for the formula used is that the min cost to add an item is 2000 (this is the Osaka value to add a storage slot to the BAL I believe). However, we have to realize that this is not true: we can add cheaper items by calling precompiles. These are already warm, and therefore the warmup cost does not have to be paid, so these are cheap items to be added to the BAL (per transction). The BAL size also has extra items per transaction per default (sender, receiver, coinbase), so the division formula also does not work because the transactions themselves also add at most 3 "free" items to the BAL.So, a "more correct" formula would be:
Here offset_empty_block accounts for the max-sized empty block in terms for BAL entries (the maximum amount of items have to be added in this empty block to get a correct offset). We can also cheaply add the precompiles, as these are warm. This can only be done once (one entry per address in the BAL), but this complicates the formula above, because these precompile entries are cheaper than the storage entries (100 gas to add precompile, 2100 gas to add storage slot, we are "saved" by the fact that we do not have thousands of precompiles but "only" 18). Not adding this to the formula would make it possible to create a block which has a BAL which by formula is invalid, but in practice is not. Finally, if there is any special logic at the fork block (irregular state transition) then this should also be accounted for in the formula (otherwise the formula at the fork block is not correct). What do you think EDIT: so I realized later that precompiles do not have to do with this. An interesting thing to check here is to see if our current notion of maximum sized BAL is correct: if it would be possible to also sneak in the precompiles then what we think the maximum sized BAL is, is then thus off by a few items (the precompiles). |
|
EDIT: this comment was added to removed comment text above (ignore it) OG textWait, the 3*len(transactions) I proposed is not true because we pay for it in EIP-2780, which is more expensive than the storage slots. Adding this offset without defining it (?) might work here? Does this offset cover everything? |
The problem I have with this is that adding this to the EIP also means this is part of the test coverage. Currently, this DoS vector is listed with this formula, and that would also mean from my perspective that So to restate: this security part should absolutely be kept in the EIP, but stating an explicit formula there makes me nervous (I know it is already there), because then I think this implicitly means that |
|
Wait, this formula is actually part of the spec, and it is not correct for the reasons now covered by the comments above (under the spoilers 😅 ) So this section: https://github.com/fselmo/EIPs/blob/f1e7dcdc37ca2f2ae421126caf8043c610deed43/EIPS/eip-7928.md#block-access-list-size-constraint I believe we should remove this section from the Specification section, because it is more complex than it currently specs. It currently states:
This is not true because we can add the precompiles to BAL for a warm address load gas (100). The formula itself should indeed also have this offset to account for system contract items and withdrawals (and maybe more). I think clients should figure out themselves an optimized way to reject invalid BALs quickly, and that we should not spec it. However, we should indeed state in the security section that especially these storage reads (which do not have an index) are a DoS vector which clients should mitigate. |
|
Imo this should be tested. If that means it must be part of EELS then I'd add it, but it shouldn't need that. So, I'm for restoring that section and at the same time keeping the new paragraph to clarify things around post-execution system contracts. |
|
I'm also happy to remove it from the EELS spec. I can see the argument that it shouldn't be there, as it's in the security considerations of the EIP and not the specifications. |
The Security Considerations section recommends that clients reject a block at any transaction boundary where:
G_remaining <= R_remaining × 2000The intent is to catch declared reads that no remaining gas could pay for. In its current form, though, the check can also reject valid blocks.
storage_readscarry noblock_access_index, so reads performed by the post-execution system calls (the EIP-7002, EIP-7251, and EIP-8282) are indistinguishable from reads that transactions have yet to perform. Those system calls read storage without consuming block gas, so their reads remain inR_remainingwhile contributing nothing toG_remaining. A valid block filled to its gas limit reaches its final boundary withG_remaining = 0and up to a few hundred such reads outstanding, and the invariant fails. With only the Prague contracts the threshold is exactly 58 × 2000 = 116,000 gas of leftover; with the EIP-8282 queues it is on the order of 1M gas.execution-specsfixtures reproduce this (draft PR here). I believe is there is only one client (Nethermind) to implement this, checking at every 8 txs, but they explicitly exclude reads from post-execution system contracts by address, which avoids rejecting valid blocks. But this is not something the EIP currently describes. Since the check is aSHOULDthat can lead to rejecting a valid block, clients that implement it may end up resolving this issue differently, or not at all. Either way, as written, it doesn't account for this.Phantom-read I/O is already bounded by the size constraint, which is exact and checked before execution begins. The feasibility check can still carry up to roughly unused_gas / 2000 phantom reads without ever failing it. This change removes the section. No client changes are required, and conformance test fixtures asserting that full blocks with full sys contract reads will remain as tests in
execution-specs.Happy to consider other alternatives but I think this is probably my favored solution from what I mulled through, given this is a
SHOULDand not aMUST.cc: @jochem-brouwer @nerolation