cerc-io/lighthouse
16
0
Fork 0
Code Issues 1 Pull Requests Packages Projects Releases Wiki Activity

Fix parent_beacon_block_root during proposer prep (#4703)

Browse Source 
* Fix `parent_beacon_block_root` during prep/reorg

* Fix another bug and add tests

* Remove overzealous payload attributes check
This commit is contained in:
Michael Sproul 2023-09-07 15:43:23 +10:00 committed by GitHub
parent f9bea3c174
commit 8db44decb7
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
7 changed files with 202 additions and 187 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

20
beacon_node/beacon_chain/src/beacon_chain.rs
View File

@ -236,9 +236,12 @@ pub enum ProduceBlockVerification {
pub struct PrePayloadAttributes {
pub proposer_index: u64,
pub prev_randao: Hash256,
/// The block number of the block being built upon (same block as fcU `headBlockHash`).
///
/// The parent block number is not part of the payload attributes sent to the EL, but *is*
/// sent to builders via SSE.
pub parent_block_number: u64,
/// The block root of the block being built upon (same block as fcU `headBlockHash`).
pub parent_beacon_block_root: Hash256,
}
@ -4111,10 +4114,10 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
let proposal_epoch = proposal_slot.epoch(T::EthSpec::slots_per_epoch());
let head_block_root = cached_head.head_block_root();
let parent_beacon_block_root = cached_head.parent_block_root();
let head_parent_block_root = cached_head.parent_block_root();
// The proposer head must be equal to the canonical head or its parent.
if proposer_head != head_block_root && proposer_head != parent_beacon_block_root {
if proposer_head != head_block_root && proposer_head != head_parent_block_root {
warn!(
self.log,
"Unable to compute payload attributes";
@ -4193,7 +4196,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
// Get the `prev_randao` and parent block number.
let head_block_number = cached_head.head_block_number()?;
let (prev_randao, parent_block_number) = if proposer_head == parent_beacon_block_root {
let (prev_randao, parent_block_number) = if proposer_head == head_parent_block_root {
(
cached_head.parent_random()?,
head_block_number.saturating_sub(1),
@ -4206,7 +4209,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
proposer_index,
prev_randao,
parent_block_number,
parent_beacon_block_root,
parent_beacon_block_root: proposer_head,
}))
}
@ -4589,8 +4592,13 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
let prepare_payload_handle = match &state {
BeaconState::Base(_) | BeaconState::Altair(_) => None,
BeaconState::Merge(_) | BeaconState::Capella(_) | BeaconState::Deneb(_) => {
let prepare_payload_handle =
get_execution_payload(self.clone(), &state, proposer_index, builder_params)?;
let prepare_payload_handle = get_execution_payload(
self.clone(),
&state,
parent_root,
proposer_index,
builder_params,
)?;
Some(prepare_payload_handle)
}
};

7
beacon_node/beacon_chain/src/execution_payload.rs
View File

@ -404,6 +404,7 @@ pub fn get_execution_payload<
>(
chain: Arc<BeaconChain<T>>,
state: &BeaconState<T::EthSpec>,
parent_block_root: Hash256,
proposer_index: u64,
builder_params: BuilderParams,
) -> Result<PreparePayloadHandle<T::EthSpec, Payload>, BlockProductionError> {
@ -426,10 +427,10 @@ pub fn get_execution_payload<
&BeaconState::Base(_) | &BeaconState::Altair(_) => None,
};
let parent_beacon_block_root = match state {
&BeaconState::Deneb(_) => Some(state.latest_block_header().canonical_root()),
&BeaconState::Merge(_) | &BeaconState::Capella(_) => None,
BeaconState::Deneb(_) => Some(parent_block_root),
BeaconState::Merge(_) | BeaconState::Capella(_) => None,
// These shouldn't happen but they're here to make the pattern irrefutable
&BeaconState::Base(_) | &BeaconState::Altair(_) => None,
BeaconState::Base(_) | BeaconState::Altair(_) => None,
};
// Spawn a task to obtain the execution payload from the EL via a series of async calls. The

61
beacon_node/beacon_chain/src/test_utils.rs
View File

@ -890,32 +890,10 @@ where
| SignedBeaconBlock::Altair(_)
| SignedBeaconBlock::Merge(_)
| SignedBeaconBlock::Capella(_) => (signed_block, None),
SignedBeaconBlock::Deneb(_) => {
if let Some(blobs) = maybe_blob_sidecars {
let signed_blobs: SignedSidecarList<E, BlobSidecar<E>> = Vec::from(blobs)
.into_iter()
.map(|blob| {
blob.sign(
&self.validator_keypairs[proposer_index].sk,
&state.fork(),
state.genesis_validators_root(),
&self.spec,
)
})
.collect::<Vec<_>>()
.into();
let mut guard = self.blob_signature_cache.write();
for blob in &signed_blobs {
guard.insert(
BlobSignatureKey::new(blob.message.block_root, blob.message.index),
blob.signature.clone(),
);
}
(signed_block, Some(signed_blobs))
} else {
(signed_block, None)
}
}
SignedBeaconBlock::Deneb(_) => (
signed_block,
maybe_blob_sidecars.map(|blobs| self.sign_blobs(blobs, &state, proposer_index)),
),
};
(block_contents, state)
@ -1037,6 +1015,35 @@ where
)
}
/// Sign blobs, and cache their signatures.
pub fn sign_blobs(
&self,
blobs: BlobSidecarList<E>,
state: &BeaconState<E>,
proposer_index: usize,
) -> SignedSidecarList<E, BlobSidecar<E>> {
let signed_blobs: SignedSidecarList<E, BlobSidecar<E>> = Vec::from(blobs)
.into_iter()
.map(|blob| {
blob.sign(
&self.validator_keypairs[proposer_index].sk,
&state.fork(),
state.genesis_validators_root(),
&self.spec,
)
})
.collect::<Vec<_>>()
.into();
let mut guard = self.blob_signature_cache.write();
for blob in &signed_blobs {
guard.insert(
BlobSignatureKey::new(blob.message.block_root, blob.message.index),
blob.signature.clone(),
);
}
signed_blobs
}
/// Produces an "unaggregated" attestation for the given `slot` and `index` that attests to
/// `beacon_block_root`. The provided `state` should match the `block.state_root` for the
/// `block` identified by `beacon_block_root`.
@ -1940,7 +1947,7 @@ where
)
.await?
.try_into()
.unwrap();
.expect("block blobs are available");
self.chain.recompute_head_at_current_slot().await;
Ok(block_hash)
}

95
beacon_node/execution_layer/src/test_utils/execution_block_generator.rs
View File

@ -449,20 +449,18 @@ impl<T: EthSpec> ExecutionBlockGenerator<T> {
) -> Result<JsonForkchoiceUpdatedV1Response, String> {
// This is meant to cover starting post-merge transition at genesis. Useful for
// testing Capella forks and later.
let head_block_hash = forkchoice_state.head_block_hash;
if let Some(genesis_pow_block) = self.block_by_number(0) {
if genesis_pow_block.block_hash() == forkchoice_state.head_block_hash {
self.terminal_block_hash = forkchoice_state.head_block_hash;
if genesis_pow_block.block_hash() == head_block_hash {
self.terminal_block_hash = head_block_hash;
}
}
if let Some(payload) = self
.pending_payloads
.remove(&forkchoice_state.head_block_hash)
{
if let Some(payload) = self.pending_payloads.remove(&head_block_hash) {
self.insert_block(Block::PoS(payload))?;
}
let unknown_head_block_hash = !self.blocks.contains_key(&forkchoice_state.head_block_hash);
let unknown_head_block_hash = !self.blocks.contains_key(&head_block_hash);
let unknown_safe_block_hash = forkchoice_state.safe_block_hash
!= ExecutionBlockHash::zero()
&& !self.blocks.contains_key(&forkchoice_state.safe_block_hash);
@ -495,20 +493,52 @@ impl<T: EthSpec> ExecutionBlockGenerator<T> {
let parent = self
.blocks
.get(&forkchoice_state.head_block_hash)
.ok_or_else(|| {
format!(
"unknown parent block {:?}",
forkchoice_state.head_block_hash
)
})?;
.get(&head_block_hash)
.cloned()
.ok_or_else(|| format!("unknown parent block {head_block_hash:?}"))?;
let id = payload_id_from_u64(self.next_payload_id);
self.next_payload_id += 1;
let mut execution_payload = match &attributes {
let execution_payload =
self.build_new_execution_payload(head_block_hash, &parent, id, &attributes)?;
self.payload_ids.insert(id, execution_payload);
Some(id)
}
};
self.head_block = Some(
self.blocks
.get(&forkchoice_state.head_block_hash)
.unwrap()
.clone(),
);
if forkchoice_state.finalized_block_hash != ExecutionBlockHash::zero() {
self.finalized_block_hash = Some(forkchoice_state.finalized_block_hash);
}
Ok(JsonForkchoiceUpdatedV1Response {
payload_status: JsonPayloadStatusV1 {
status: JsonPayloadStatusV1Status::Valid,
latest_valid_hash: Some(forkchoice_state.head_block_hash),
validation_error: None,
},
payload_id: id.map(Into::into),
})
}
pub fn build_new_execution_payload(
&mut self,
head_block_hash: ExecutionBlockHash,
parent: &Block<T>,
id: PayloadId,
attributes: &PayloadAttributes,
) -> Result<ExecutionPayload<T>, String> {
let mut execution_payload = match attributes {
PayloadAttributes::V1(pa) => ExecutionPayload::Merge(ExecutionPayloadMerge {
parent_hash: forkchoice_state.head_block_hash,
parent_hash: head_block_hash,
fee_recipient: pa.suggested_fee_recipient,
receipts_root: Hash256::repeat_byte(42),
state_root: Hash256::repeat_byte(43),
@ -525,7 +555,7 @@ impl<T: EthSpec> ExecutionBlockGenerator<T> {
}),
PayloadAttributes::V2(pa) => match self.get_fork_at_timestamp(pa.timestamp) {
ForkName::Merge => ExecutionPayload::Merge(ExecutionPayloadMerge {
parent_hash: forkchoice_state.head_block_hash,
parent_hash: head_block_hash,
fee_recipient: pa.suggested_fee_recipient,
receipts_root: Hash256::repeat_byte(42),
state_root: Hash256::repeat_byte(43),
@ -541,7 +571,7 @@ impl<T: EthSpec> ExecutionBlockGenerator<T> {
transactions: vec![].into(),
}),
ForkName::Capella => ExecutionPayload::Capella(ExecutionPayloadCapella {
parent_hash: forkchoice_state.head_block_hash,
parent_hash: head_block_hash,
fee_recipient: pa.suggested_fee_recipient,
receipts_root: Hash256::repeat_byte(42),
state_root: Hash256::repeat_byte(43),
@ -560,7 +590,7 @@ impl<T: EthSpec> ExecutionBlockGenerator<T> {
_ => unreachable!(),
},
PayloadAttributes::V3(pa) => ExecutionPayload::Deneb(ExecutionPayloadDeneb {
parent_hash: forkchoice_state.head_block_hash,
parent_hash: head_block_hash,
fee_recipient: pa.suggested_fee_recipient,
receipts_root: Hash256::repeat_byte(42),
state_root: Hash256::repeat_byte(43),
@ -599,32 +629,7 @@ impl<T: EthSpec> ExecutionBlockGenerator<T> {
*execution_payload.block_hash_mut() =
ExecutionBlockHash::from_root(execution_payload.tree_hash_root());
self.payload_ids.insert(id, execution_payload);
Some(id)
}
};
self.head_block = Some(
self.blocks
.get(&forkchoice_state.head_block_hash)
.unwrap()
.clone(),
);
if forkchoice_state.finalized_block_hash != ExecutionBlockHash::zero() {
self.finalized_block_hash = Some(forkchoice_state.finalized_block_hash);
}
Ok(JsonForkchoiceUpdatedV1Response {
payload_status: JsonPayloadStatusV1 {
status: JsonPayloadStatusV1Status::Valid,
latest_valid_hash: Some(forkchoice_state.head_block_hash),
validation_error: None,
},
payload_id: id.map(Into::into),
})
Ok(execution_payload)
}
}

35
beacon_node/execution_layer/src/test_utils/mock_builder.rs
View File

@ -402,22 +402,35 @@ impl<E: EthSpec> mev_rs::BlindedBlockProvider for MockBuilder<E> {
let prev_randao = head_state
.get_randao_mix(head_state.current_epoch())
.map_err(convert_err)?;
let parent_root = head_state.latest_block_header().parent_root;
let expected_withdrawals = match fork {
ForkName::Base | ForkName::Altair | ForkName::Merge => None,
ForkName::Capella | ForkName::Deneb => Some(
self.beacon_client
.get_expected_withdrawals(&StateId::Head)
.await
.unwrap()
.data,
),
};
let payload_attributes = match fork {
ForkName::Merge => {
PayloadAttributes::new(timestamp, *prev_randao, fee_recipient, None, None)
}
// the withdrawals root is filled in by operations
ForkName::Capella => {
PayloadAttributes::new(timestamp, *prev_randao, fee_recipient, Some(vec![]), None)
}
// the withdrawals root is filled in by operations, but we supply the valid withdrawals
// first to avoid polluting the execution block generator with invalid payload attributes
// NOTE: this was part of an effort to add payload attribute uniqueness checks,
// which was abandoned because it broke too many tests in subtle ways.
ForkName::Merge | ForkName::Capella => PayloadAttributes::new(
timestamp,
*prev_randao,
fee_recipient,
expected_withdrawals,
None,
),
ForkName::Deneb => PayloadAttributes::new(
timestamp,
*prev_randao,
fee_recipient,
Some(vec![]),
Some(parent_root),
expected_withdrawals,
Some(head_block_root),
),
ForkName::Base | ForkName::Altair => {
return Err(MevError::InvalidFork);
@ -451,7 +464,7 @@ impl<E: EthSpec> mev_rs::BlindedBlockProvider for MockBuilder<E> {
.map_err(convert_err)?
.into();
let header: ExecutionPayloadHeader<E> = match payload {
let header = match payload {
ExecutionPayload::Merge(payload) => ExecutionPayloadHeader::Merge((&payload).into()),
ExecutionPayload::Capella(payload) => {
ExecutionPayloadHeader::Capella((&payload).into())

17
beacon_node/http_api/tests/interactive_tests.rs
View File

@ -391,8 +391,8 @@ pub async fn proposer_boost_re_org_test(
) {
assert!(head_slot > 0);
// Test using Capella so that we simulate conditions as similar to mainnet as possible.
let mut spec = ForkName::Capella.make_genesis_spec(E::default_spec());
// Test using the latest fork so that we simulate conditions as similar to mainnet as possible.
let mut spec = ForkName::latest().make_genesis_spec(E::default_spec());
spec.terminal_total_difficulty = 1.into();
// Ensure there are enough validators to have `attesters_per_slot`.
@ -623,7 +623,7 @@ pub async fn proposer_boost_re_org_test(
.await
.unwrap()
.data;
let unsigned_block_c = unsigned_block_contents_c.deconstruct().0;
let (unsigned_block_c, block_c_blobs) = unsigned_block_contents_c.deconstruct();
let block_c = harness.sign_beacon_block(unsigned_block_c, &state_b);
if should_re_org {
@ -634,9 +634,13 @@ pub async fn proposer_boost_re_org_test(
assert_eq!(block_c.parent_root(), block_b_root);
}
// Sign blobs.
let block_c_signed_blobs =
block_c_blobs.map(|blobs| harness.sign_blobs(blobs, &state_b, proposer_index));
// Applying block C should cause it to become head regardless (re-org or continuation).
let block_root_c = harness
.process_block_result((block_c.clone(), None))
.process_block_result((block_c.clone(), block_c_signed_blobs))
.await
.unwrap()
.into();
@ -699,6 +703,11 @@ pub async fn proposer_boost_re_org_test(
assert_ne!(expected_withdrawals, pre_advance_withdrawals);
}
// Check that the `parent_beacon_block_root` of the payload attributes are correct.
if let Ok(parent_beacon_block_root) = payload_attribs.parent_beacon_block_root() {
assert_eq!(parent_beacon_block_root, block_c.parent_root());
}
let lookahead = slot_clock
.start_of(slot_c)
.unwrap()

28
beacon_node/http_api/tests/tests.rs
View File

@ -3973,20 +3973,6 @@ impl ApiTester {
)));
let slot = self.chain.slot().unwrap();
let propose_state = self
.harness
.chain
.state_at_slot(slot, StateSkipConfig::WithoutStateRoots)
.unwrap();
let withdrawals = get_expected_withdrawals(&propose_state, &self.chain.spec).unwrap();
let withdrawals_root = withdrawals.tree_hash_root();
// Set withdrawals root for builder
self.mock_builder
.as_ref()
.unwrap()
.builder
.add_operation(Operation::WithdrawalsRoot(withdrawals_root));
let epoch = self.chain.epoch().unwrap();
let (_, randao_reveal) = self.get_test_randao(slot, epoch).await;
@ -4024,20 +4010,6 @@ impl ApiTester {
)));
let slot = self.chain.slot().unwrap();
let propose_state = self
.harness
.chain
.state_at_slot(slot, StateSkipConfig::WithoutStateRoots)
.unwrap();
let withdrawals = get_expected_withdrawals(&propose_state, &self.chain.spec).unwrap();
let withdrawals_root = withdrawals.tree_hash_root();
// Set withdrawals root for builder
self.mock_builder
.as_ref()
.unwrap()
.builder
.add_operation(Operation::WithdrawalsRoot(withdrawals_root));
let epoch = self.chain.epoch().unwrap();
let (_, randao_reveal) = self.get_test_randao(slot, epoch).await;