diff --git a/.github/scripts/hive/expected_failures.yaml b/.github/scripts/hive/expected_failures.yaml index ff9605106b..5d29a99e56 100644 --- a/.github/scripts/hive/expected_failures.yaml +++ b/.github/scripts/hive/expected_failures.yaml @@ -20,9 +20,7 @@ engine-withdrawals: [ ] engine-api: [ ] -# no fix due to https://github.com/paradigmxyz/reth/issues/8732 -engine-cancun: - - Invalid PayloadAttributes, Missing BeaconRoot, Syncing=True (Cancun) (reth) +engine-cancun: [ ] sync: [ ] diff --git a/crates/rpc/rpc-engine-api/src/engine_api.rs b/crates/rpc/rpc-engine-api/src/engine_api.rs index 33ca04861e..30bd760074 100644 --- a/crates/rpc/rpc-engine-api/src/engine_api.rs +++ b/crates/rpc/rpc-engine-api/src/engine_api.rs @@ -759,17 +759,11 @@ where // MUST NOT begin a payload build process. In such an event, the forkchoiceState // update MUST NOT be rolled back. // - // NOTE: This will also apply to the validation result for the cancun or - // shanghai-specific fields provided in the payload attributes. - // - // To do this, we set the payload attrs to `None` if attribute validation failed, but - // we still apply the forkchoice update. + // NOTE: This also applies to cancun/shanghai-specific payload attributes. if let Err(err) = attr_validation_res { let fcu_res = self.inner.beacon_consensus.fork_choice_updated(state, None, version).await?; - // TODO: decide if we want this branch - the FCU INVALID response might be more - // useful than the payload attributes INVALID response - if fcu_res.is_invalid() { + if fcu_res.is_invalid() || fcu_res.payload_status.is_syncing() { return Ok(fcu_res) } return Err(err.into()) @@ -1344,10 +1338,13 @@ struct EngineApiInner { + assert!( + payload_attrs.is_none(), + "FCU for syncing state should be evaluated before payload attributes" + ); + tx + } + other => panic!("unexpected engine message: {other:?}"), + }; + + response_tx.send(Ok(OnForkChoiceUpdated::syncing())).expect("send syncing response"); + + let response = api_task + .await + .expect("api task should not panic") + .expect("forkchoiceUpdatedV3 should return a syncing response"); + assert!(response.payload_status.is_syncing()); + assert!(response.payload_id.is_none()); + } + + #[tokio::test] + async fn fcu_v3_valid_forkchoice_missing_beacon_root_returns_invalid_attributes() { + let (mut handle, api) = setup_engine_api(); + + let state = ForkchoiceState { + head_block_hash: B256::from([0x22; 32]), + safe_block_hash: B256::ZERO, + finalized_block_hash: B256::ZERO, + }; + let payload_attributes = PayloadAttributes { + timestamp: 1, + prev_randao: B256::ZERO, + suggested_fee_recipient: Address::ZERO, + withdrawals: Some(vec![]), + parent_beacon_block_root: None, + }; + + let api_task = tokio::spawn(async move { + api.fork_choice_updated_v3(state, Some(payload_attributes)).await + }); + + let request = + tokio::time::timeout(std::time::Duration::from_secs(1), handle.from_api.recv()) + .await + .expect("timed out waiting for forkchoiceUpdated request") + .expect("expected forkchoiceUpdated request"); + + let response_tx = match request { + BeaconEngineMessage::ForkchoiceUpdated { payload_attrs, tx, .. } => { + assert!( + payload_attrs.is_none(), + "when attrs are invalid, API should first evaluate forkchoice without attrs" + ); + tx + } + other => panic!("unexpected engine message: {other:?}"), + }; + + response_tx + .send(Ok(OnForkChoiceUpdated::valid(PayloadStatus::from_status( + PayloadStatusEnum::Valid, + )))) + .expect("send valid response"); + + let response = api_task.await.expect("api task should not panic"); + assert_matches!( + response, + Err(EngineApiError::EngineObjectValidationError( + reth_payload_primitives::EngineObjectValidationError::PayloadAttributes(_) + )) + ); + + match tokio::time::timeout(std::time::Duration::from_millis(100), handle.from_api.recv()) + .await + { + Err(_) | Ok(None) => {} + Ok(Some(BeaconEngineMessage::ForkchoiceUpdated { .. })) => { + panic!("no second forkchoiceUpdated call should be sent when attrs are invalid") + } + Ok(Some(other)) => panic!("unexpected engine message: {other:?}"), + } + } + // tests covering `engine_getPayloadBodiesByRange` and `engine_getPayloadBodiesByHash` mod get_payload_bodies { use super::*;