supernode: Use Denylist Output for "OptimisticOutputRoot"#19880
supernode: Use Denylist Output for "OptimisticOutputRoot"#19880axelKingsley wants to merge 6 commits intodevelopfrom
Conversation
…ot computation Made-with: Cursor
Made-with: Cursor
…root API OptimisticOutputAtTimestamp now checks the deny list before falling back to the local safe block. When a block at a given height has been invalidated, the original (pre-replacement) block's output is returned as the "optimistic" output — reflecting what the block would have been without verification. This gives superroot API callers a meaningful distinction between the verified and optimistic views. Adds DenyList.LastDeniedOutputV0(height) to retrieve the most recently denied OutputV0 at a height in a single DB read. Made-with: Cursor
axelKingsley
force-pushed
the
supernode/DeniedOutputAsOptimistic
branch
from
356b9a6 to
58d28df
Compare
|
|
||
| // LastDeniedOutputV0 returns the OutputV0 for the most recently denied block at the given height. | ||
| // Returns nil if no blocks are denied at that height. | ||
| // Note: supernode does not currently behave in well defined ways when there are multiple denied blocks at the same height. |
There was a problem hiding this comment.
Q: Are we going to add special handling for this edge case eventually? Or do we even expect this to be a possible failure mode in practice?
I'm trying to think/understand but I don't know how/if this could even happen in practice: the node's normal/optimistic derivation should always result in the same invalid block B at height H, which gets denied and replaced by the deposits-only block D that is always valid at that height. Having two invalid blocks at the same height would either require non-deterministic derivation (two possible invalid blocks B1 and B2 generated for the same height - I guess this is possible with a derivation or node/chain container bug?) or for a deposits-only replacement block to get denied (impossible).
I feel like I'm probably missing something though
(FWIW I think this PR is fine as-is, I'm just asking for clarifying my own understanding)
There was a problem hiding this comment.
derivation should always result in the same invalid block B at height H, which gets denied and replaced by the deposits-only block D that is always valid at that height
That's right. I don't think that it's possible to have two denied blocks at the same height. At least not with our current interop rules
There was a problem hiding this comment.
Both correct, I left this comment because I realized that our current Denylist storage is a little funny -- it expects you to provide the Number and Hash because it can store multiple denied blocks for a given height. As for why it can store multiple blocks for a given height... Notionally, it seemed more flexible to store multiple blocks, and in some future world with multiple verifiers, maybe there's some reason that is helpful.
HOWEVER, I agree that in protocol you'll never see more than one block get denied for a given height. This function is basically an admission of that, especially since this caller does not have the block hash -- if there were multiple denied blocks, this caller wouldn't know which one to pick.
We should consider pulling the YAGNI card and constraining the denylist storage to single outputs per height. That would allow for all callers to follow a simpler pattern, likely rip out ~500 lines of implementation and an equal amount of tests.
There was a problem hiding this comment.
It would be a very rare case, but I suspect if L1 re-orged and changed the batch that was included you could get two different, but both invalid L2 blocks at a specific height. The last denied one is probably the right one unless L1 reorged back. Do we drop invalidated blocks if there's an L1 reorg? they could actually become valid if that causes another chain to reorg and now include the required init message.
It's important we get this strictly right as op-challenger will be depending on this information being accurate in order to play games, including posting bonds. If it's wrong the challenger will lose, lose its bonds and the game will resolve incorrectly.
Unresolving this just for visibility - if we do drop invalid block on reorgs then we should be all good.
Just wanted to throw in my 2 cents that this naming is definitely confusing and has confused me before about exactly this behavior. I'm not sure if it's worth changing the name of |
I'm not sure exactly which operation is best, but my assumption is that the function we are not modifying needs to be updated to something like "LocalAt". For the sake of minimal scope updates I'd like to just modify the function super-roots uses. But to follow through on the others I just cut this ticket: #19902 . |
axelKingsley
force-pushed
the
supernode/DeniedOutputAsOptimistic
branch
from
bbd91c8 to
58d28df
Compare
The SuperRootAtTimestampResponse's OutputWithRequiredL1 carried a full OutputResponse (with L2BlockRef, Version, Status, etc.) when consumers only ever accessed BlockHash and OutputRoot. Replace with *OutputV0 which directly contains the three output preimage fields (StateRoot, MessagePasserStorageRoot, BlockHash), eliminating dead weight from the superroot API. Made-with: Cursor
…tputAsOptimistic Made-with: Cursor # Conflicts: # op-supernode/supernode/chain_container/invalidation.go # op-supernode/supernode/chain_container/invalidation_test.go
The only caller immediately converted the OutputResponse into an OutputV0. Return *OutputV0 from the interface and implementation, eliminating the intermediate OutputResponse construction. The deny list path returns the OutputV0 directly; the fallback path now delegates to OutputV0AtBlockNumber instead of going through the rollup RPC client. Made-with: Cursor
ajsutton
left a comment
ajsutton
left a comment
There was a problem hiding this comment.
Will defer to others on the details of the deny list tracking - I had a quick look and it seems ok but didn't have time to fully review it.
| next.OptimisticAtTimestamp[eth.ChainIDFromUInt64(1)] = eth.OutputWithRequiredL1{ | ||
| Output: ð.OutputResponse{ | ||
| OutputRoot: eth.Bytes32{0xad}, | ||
| BlockRef: eth.L2BlockRef{Hash: common.Hash{0xcd}}, | ||
| WithdrawalStorageRoot: common.Hash{0xde}, | ||
| StateRoot: common.Hash{0xdf}, | ||
| OutputRoot: ð.OutputV0{ | ||
| StateRoot: eth.Bytes32{0xdf}, |
There was a problem hiding this comment.
It's very tempting to have the OuptutV0 as the Output field and add a OutputRoot field that's the actual hash. Strictly you can calculate it but as a human trying to debug things it would be nice to have it already present.
|
|
||
| // LastDeniedOutputV0 returns the OutputV0 for the most recently denied block at the given height. | ||
| // Returns nil if no blocks are denied at that height. | ||
| // Note: supernode does not currently behave in well defined ways when there are multiple denied blocks at the same height. |
There was a problem hiding this comment.
It would be a very rare case, but I suspect if L1 re-orged and changed the batch that was included you could get two different, but both invalid L2 blocks at a specific height. The last denied one is probably the right one unless L1 reorged back. Do we drop invalidated blocks if there's an L1 reorg? they could actually become valid if that causes another chain to reorg and now include the required init message.
It's important we get this strictly right as op-challenger will be depending on this information being accurate in order to play games, including posting bonds. If it's wrong the challenger will lose, lose its bonds and the game will resolve incorrectly.
Unresolving this just for visibility - if we do drop invalid block on reorgs then we should be all good.
4 participants
Tool Use Notice
Generated.
What
Note
There is also
OptimisticAtwhich I chose not to update. I think the language here is unfortunately mirky, but that function likely means optimistic as-in the locally derived, including the replacement once it becomes canonical. I remember talking with @geoknee about this distinction, but in any case the function I've updated in this PR is directly used by super-root, which is the target of this update anyway.