core: Make ExecuteStateless fully self-validating#34007
Conversation
| if crossStateRoot != payload.Block.Root() { | ||
| fmt.Fprintf(os.Stderr, "stateless self-validation root mismatch (cross: %x local: %x)\n", crossStateRoot, payload.Block.Root()) | ||
| os.Exit(11) | ||
| } | ||
| if crossReceiptRoot != payload.Block.ReceiptHash() { | ||
| fmt.Fprintf(os.Stderr, "stateless self-validation receipt root mismatch (cross: %x local: %x)\n", crossReceiptRoot, payload.Block.ReceiptHash()) | ||
| os.Exit(12) | ||
| } |
There was a problem hiding this comment.
Post state check (ValidateState) is now done inside of ExecuteStateless
| if v.config.IsOsaka(block.Number(), block.Time()) && block.Size() > params.MaxBlockSize { | ||
| return ErrBlockOversized | ||
| } | ||
| // Check whether the block is already imported. |
There was a problem hiding this comment.
The checks that we have moved from here are only needed if you have state and are safe guards to check if we should indeed insert this block into the chain. It has been moved into blockchain_insert so that ExecuteStateless can use this method as is for pre execution checks.
The checks that remain in this method are essentially checking the consistency between the block body and the header (pre-execution)
|
|
||
| // ValidateState validates the various changes that happen after a state transition, | ||
| // such as amount of used gas, the receipt roots and the state root itself. | ||
| func (v *BlockValidator) ValidateState(block *types.Block, statedb *state.StateDB, res *ProcessResult, stateless bool) error { |
There was a problem hiding this comment.
We no longer need to return early when in stateless mode because stateless mode does the same checks as stateful mode
| // Remove critical computed fields from the block to force true recalculation | ||
| context := block.Header() | ||
| context.Root = common.Hash{} | ||
| context.ReceiptHash = common.Hash{} | ||
|
|
||
| task := types.NewBlockWithHeader(context).WithBody(*block.Body()) | ||
|
|
There was a problem hiding this comment.
no longer need these because stateless mode takes in the unmodified block. I think it was a bit awkward before because the code was executing a block that has fields in its header that were intentionally zeroed out. As noted above, this led to some parts of the code returning early when in stateless mode or just skipping important checks like ValidateBody
| // Skip blocks we've already imported and fully processed. | ||
| if it.bc.HasBlockAndState(block.Hash(), block.NumberU64()) { | ||
| return block, ErrKnownBlock | ||
| } | ||
| // Verify uncle blocks against chain history (pre-mereg only) | ||
| if err := it.bc.engine.VerifyUncles(it.bc, block); err != nil { | ||
| return block, err | ||
| } | ||
| // Ensure the parent block is known and its state is available. | ||
| if !it.bc.HasBlockAndState(block.ParentHash(), block.NumberU64()-1) { | ||
| if !it.bc.HasBlock(block.ParentHash(), block.NumberU64()-1) { | ||
| return block, consensus.ErrUnknownAncestor | ||
| } | ||
| return block, consensus.ErrPrunedAncestor | ||
| } |
There was a problem hiding this comment.
Checks that were in ValidateBody have been moved here.
These are very much tied to the blockchain instance and not related to verifying a block against its header fields
| if err := validator.ValidateBody(block); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
new check added, so we now completely validate the block
| // Almost everything validated, but receipt and state root needs to be returned | ||
| receiptRoot := types.DeriveSha(res.Receipts, trie.NewStackTrie(nil)) | ||
| stateRoot := db.IntermediateRoot(config.IsEIP158(block.Number())) | ||
| return stateRoot, receiptRoot, nil |
There was a problem hiding this comment.
We no longer return receiptRoot and stateRoot, however if the block was valid, we can just call methods on Block to get them
| envelope.ExecutionPayload.StateRoot = common.Hash{} | ||
| envelope.ExecutionPayload.ReceiptsRoot = common.Hash{} | ||
|
|
There was a problem hiding this comment.
main change, we no longer zero out the state root and receipt root
| api.lastNewPayloadUpdate.Store(time.Now().Unix()) | ||
|
|
||
| log.Trace("Executing block statelessly", "number", block.Number(), "hash", params.BlockHash) | ||
| stateRoot, receiptRoot, err := core.ExecuteStateless(context.Background(), api.config(), vm.Config{}, block, witness) |
There was a problem hiding this comment.
-
I don't think anyone is using these cross validation methods?
-
Note: the change is non-breaking because the input is a valid block and its only internally that the blocks have their state root and receipt root zeroed out.
| // Header validity is known at this point. Here we verify that uncles, transactions | ||
| // and withdrawals given in the block body match the header. | ||
| header := block.Header() | ||
| if err := v.bc.engine.VerifyUncles(v.bc, block); err != nil { |
There was a problem hiding this comment.
This check is still required even after the Merge, ensuring the uncle list is empty. I believe it's also required in stateless mode.
// VerifyUncles verifies that the given block's uncles conform to the consensus
// rules of the Ethereum consensus engine.
func (beacon *Beacon) VerifyUncles(chain consensus.ChainReader, block *types.Block) error {
if !beacon.IsPoSHeader(block.Header()) {
return beacon.ethone.VerifyUncles(chain, block)
}
// Verify that there is no uncle block. It's explicitly disabled in the beacon
if len(block.Uncles()) > 0 {
return errTooManyUncles
}
return nil
}
| if it.bc.HasBlockAndState(block.Hash(), block.NumberU64()) { | ||
| return block, ErrKnownBlock | ||
| } | ||
| // Verify uncle blocks against chain history (pre-merge only) |
There was a problem hiding this comment.
Please restore it back to Body validation, it's still required after the merge.
There was a problem hiding this comment.
Ah I think I am missing something, I thought that it would be covered by verifyHeader and validateBody.
My thinking was that:
-
In verifyHeader, we check if uncleHash == EmptyUncleHash here
-
Then in validateBody, we check that the calculated UncleHash matches the one in the header here
Opened a PR here to check
There was a problem hiding this comment.
Maybe we have some duplicated checks here, honestly.
| // In stateless mode, return early because the receipt and state root are not | ||
| // provided through the witness, rather the cross validator needs to return it. | ||
| if stateless { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
this is designed for another use case and is still needed. I agree that, in your case, it should be called, but then it has to be via another flag.
There was a problem hiding this comment.
I think the original intention is that: the stateless mode produces all the hashes without the hints (such as the state root in block header) and these hashes are explicitly checked outside the stateless execution.
But I feel Kev is right here. The input for stateless execution is: (a) the block (b) the state (c) the state proofs.
The executor is responsible for verifying the states and making sure the produced hashes are not malformed.
There was a problem hiding this comment.
func ExecuteStateless(ctx context.Context, config *params.ChainConfig, vmconfig vm.Config, block *types.Block, witness *stateless.Witness) error {
// Create and populate the state database to serve as the stateless backend
memdb := witness.MakeHashDB()
db, err := state.New(witness.Root(), state.NewDatabase(triedb.NewDatabase(memdb, triedb.HashDefaults), nil))
if err != nil {
return err
}The pre-execution root is derived from the witness. I am not 100% sure it's correct. Ideally it should be the state root of the parent referred by the block.
There was a problem hiding this comment.
Ideally it should be the state root of the parent referred by the block.
Yep this is my understanding, so we would:
- Retrieve the purported parent header from the execution witness
- Check that it is the parent header
- Take the post state root from that header and use it as the pre-state root for this block
There was a problem hiding this comment.
I didn't look at the pipeline for this part of this part of the code, ie whether memdb , db etc are correctly formed. I was planning to look at that as a followup, but can also do it in this PR
There was a problem hiding this comment.
@rjl493456442 my point is that the other use case needs to be fixed before you delete this. Doing this would recompute the state root twice. You can't just delete that check.
There was a problem hiding this comment.
can you say what this other usecase is?
Can you say what breaks? Also interested to hear how this could possibly break BALs |
where does this PR add the pre-execution checks that are reportedly missing? |
reporting from an offline convo: |
|
The cross validation idea was deemed to be important. Feel free to cherry pick what makes sense to ensure it works with your usecase! |
|
No need to close it, we're just going to polish it a bit and merge it. We just don't want to jettison the cross-validation. |
Add header verification (VerifyHeader), body validation (ValidateBody), and state/receipt root checks directly inside ExecuteStateless, making it a complete self-contained block validator. This is a minimal, conservative alternative to ethereum#34007 that achieves the same goal without restructuring BlockValidator or insertIterator.
|
Turns out this branch can't be edited, creating a new PR and closing this one. |
With this change, the stateless codepath mirrors the stateful codepath.
Context
Previously the code was only doing partial validation, which means that it was missing the pre-execution checks done by
ValidateBodyand it was also skipping the post exection checks (receipts root, post state root and request hash) inValidateState.This meant that there were if statements in the code to cover corner cases such as returning early in
ValidateBodyand zeroing out the state root and receipt root before callingExecuteStateless.Reading the code, the motivation for this was to allow for cross validation between multiple stateless client. Where
ExecuteStatelesswould essentially return the computed roots and this would be used to cross-check with other stateless clients.Bugs Found
Since keeper is meant to be fully validate a block statelessly, it was not safe to call
ExecuteStatelessas is because it was missing pre-execution checks.Changes
Will comment inline