Fix external payload validation logic, resolve txs execution logic bug and add pre-exec validation #341
Conversation
sieniven
requested review from
0x416e746f6e,
SozinM,
avalonche and
karim-agha
as code owners
|
hi @sieniven thank you for the fixes, apologies for the slow response! are you able to share the steps you used to trigger the hash mismatch error? this feature is very experimental and has only been tested locally, not on any sort of production environment. it would be good to have the cases you used for testing documented. |
Hey @noot, thanks for taking the time to explain - this makes sense. The hash mismatch error occur in 2 scenarios. I'll comment directly on the code blocks for your reference. |
| let sender = tx | ||
| .recover_signer() | ||
| .wrap_err("failed to recover tx signer")?; | ||
| let tx_env = TxEnv::from_recovered_tx(&tx, sender); | ||
| let executable_tx = match tx { | ||
| OpTxEnvelope::Deposit(ref tx) => { | ||
| let deposit = DepositTransactionParts { | ||
| mint: Some(tx.mint), | ||
| source_hash: tx.source_hash, | ||
| is_system_transaction: tx.is_system_transaction, | ||
| }; | ||
| OpTransaction { | ||
| base: tx_env, | ||
| enveloped_tx: None, | ||
| deposit, | ||
| } | ||
| } | ||
| OpTxEnvelope::Legacy(_) => { | ||
| let mut tx = OpTransaction::new(tx_env); | ||
| tx.enveloped_tx = Some(vec![0x00].into()); | ||
| tx | ||
| } | ||
| OpTxEnvelope::Eip2930(_) => { | ||
| let mut tx = OpTransaction::new(tx_env); | ||
| tx.enveloped_tx = Some(vec![0x00].into()); | ||
| tx | ||
| } | ||
| OpTxEnvelope::Eip1559(_) => { | ||
| let mut tx = OpTransaction::new(tx_env); | ||
| tx.enveloped_tx = Some(vec![0x00].into()); | ||
| tx | ||
| } | ||
| OpTxEnvelope::Eip7702(_) => { | ||
| let mut tx = OpTransaction::new(tx_env); | ||
| tx.enveloped_tx = Some(vec![0x00].into()); | ||
| tx | ||
| } | ||
| }; |
There was a problem hiding this comment.
The first problematic area that the PR resolves is here - there shouldnt be a need to write local definition of convertion methods to alloy typed tx types, but instead pass the tx value itself directly into evm.transact, which allows the safe conversion into the tx env interface (for revm execution) using upstream conversion methods.
| .transpose() | ||
| .wrap_err("failed to get depositor nonce")?; | ||
|
|
||
| let ResultAndState { result, state } = match evm.transact_raw(executable_tx) { |
There was a problem hiding this comment.
Better to use the upstream's default pattern, evm.transact here which inherently calls IntoTxEnv on the concrete type for tx execution.
| let depositor_nonce = (is_regolith_active && tx.is_deposit()) | ||
| .then(|| { | ||
| evm.db_mut() | ||
| .load_cache_account(sender) | ||
| .map(|acc| acc.account_info().unwrap_or_default().nonce) | ||
| }) | ||
| .transpose() | ||
| .wrap_err("failed to get depositor nonce")?; |
There was a problem hiding this comment.
The second issue (and where the block hash mismatch is) is located here. The depositor nonce when bulding the receipt should be using the nonce prior to execution. However, the deposit tx receipt uses the depositor nonce after execution, which will cause a stateroot mismatch on the incorrect block data
| /// Validates the payload header and its relationship with the parent before execution. | ||
| /// This performs consensus rule validation including: | ||
| /// - Header field validation (timestamp, gas limit, etc.) | ||
| /// - Parent relationship validation (block number increment, timestamp progression) | ||
| fn validate_pre_execution( | ||
| payload: &OpBuiltPayload, | ||
| parent_header: &reth_primitives_traits::Header, | ||
| parent_hash: alloy_primitives::B256, | ||
| chain_spec: Arc<OpChainSpec>, | ||
| ) -> eyre::Result<()> { | ||
| use reth::consensus::HeaderValidator; | ||
|
|
||
| fn is_regolith_active(chain_spec: &OpChainSpec, timestamp: u64) -> bool { | ||
| use reth_optimism_chainspec::OpHardforks as _; | ||
| chain_spec.is_regolith_active_at_timestamp(timestamp) | ||
| let consensus = OpBeaconConsensus::new(chain_spec); | ||
| let parent_sealed = SealedHeader::new(parent_header.clone(), parent_hash); | ||
|
|
||
| // Validate incoming header | ||
| consensus | ||
| .validate_header(payload.block().sealed_header()) | ||
| .wrap_err("header validation failed")?; | ||
|
|
||
| // Validate incoming header against parent | ||
| consensus | ||
| .validate_header_against_parent(payload.block().sealed_header(), &parent_sealed) | ||
| .wrap_err("header validation against parent failed")?; | ||
|
|
||
| Ok(()) |
There was a problem hiding this comment.
The 3rd issue is that in the payload validation process on default reth, we should pre-validate the incoming payload first prior to execution. This includes validating incoming payload header first which is missing in this p2p builder validation logic - https://github.com/okx/reth/blob/upstream/dev/crates/engine/tree/src/tree/payload_validator.rs#L27
|
@sieniven thank you, the changes look good to me! in terms of what case you used to trigger the hash mismatch, i am assuming you used deposit transactions to cause the issue? i would also be curious to discuss how you are using this feature and if you are planning to use it in prod. we are not actively working on this feature at the moment and don't really have plans to do so, so if you are planning to use it, i would like to figure out how we can support that. |
|
@noot thanks for the review! Yeah the issue can easily be recreated by running any optimism stack based chain, which the mismatch will happen on the default system tx (deposit tx type). Regarding our use case - actually we use the p2p functionality in production because we directly integrate the flashblocks builder into our X Layer sequencer, which we have found to greatly optimize the throughput of the chain (due to some inherent design tradeoffs when separating the builder and proposal with rollup-boost running). As such, we rely on the p2p functionality to do 2 things:
Our op-rbuilder code differs abit here, I was intending to open another PR to you guys support this after this PR is merged. Maybe you guys can discuss internally if it makes sense to support this? 👍 Code ref: https://github.com/okx/op-rbuilder/blob/d35d855fc25458b846d0d80aabf47325eed4028e/crates/op-rbuilder/src/builders/flashblocks/payload_handler.rs#L108-L123 cc/ @SozinM |
3 participants
📝 Summary
This PR resolve issues found on the payload handler when handling received externally built flashblocks payload. It is found that when retrieving external payloads from peer builders, the state root calculation could mismatch due to errors on the payload validation execution (refer to Issue logs section for the example logs).
evm.transact()instead for execution of transactions and handling of state changes. We should pass the tx value itself directly into evm.transact, allowing the safe conversion into the tx env interface (for revm execution) using upstream conversion methodsIssue logs
✅ I have completed the following steps:
make lintmake test