From 58e35bc96fab30e5f4085ac36f70db1f88ca5bb1 Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Mon, 15 Jun 2026 16:56:09 -0700 Subject: [PATCH] Gloas alpha spec 9 (#9393) Changes implemented Ensure bids are for a higher slot than their parent (https://github.com/ethereum/consensus-specs/pull/5302) Ignore PTC attestations for empty assigned slots (https://github.com/ethereum/consensus-specs/pull/5281) Limit should_build_on_full checks to the previous slot (https://github.com/ethereum/consensus-specs/pull/5309) Apply proposer boost if dependent roots match (https://github.com/ethereum/consensus-specs/pull/5306) Exclude slashed validators from proposing (EIP-8045) (https://github.com/ethereum/consensus-specs/pull/5115) Force the proposer to reorg late payloads (https://github.com/ethereum/consensus-specs/pull/5210) Remove support for old deposit mechanism in Fulu (https://github.com/ethereum/consensus-specs/pull/4704) Co-Authored-By: Eitan Seri-Levi Co-Authored-By: dapplion <35266934+dapplion@users.noreply.github.com> Co-Authored-By: Eitan Seri-Levi Co-Authored-By: Michael Sproul Co-Authored-By: Michael Sproul --- beacon_node/beacon_chain/src/beacon_chain.rs | 43 ------ .../src/block_production/gloas.rs | 2 +- .../gossip_verified_payload_attestation.rs | 16 ++- .../payload_attestation_verification/mod.rs | 12 ++ .../payload_attestation_verification/tests.rs | 96 +++++--------- .../gossip_verified_bid.rs | 12 +- .../src/payload_bid_verification/mod.rs | 2 + .../src/payload_bid_verification/tests.rs | 62 ++++++--- .../payload_envelope_verification/import.rs | 3 - .../beacon_chain/tests/op_verification.rs | 61 +++++++++ .../tests/payload_invalidation.rs | 1 - .../src/beacon/execution_payload_envelopes.rs | 46 ++++--- beacon_node/http_api/src/block_id.rs | 5 - .../gossip_methods.rs | 31 +++-- .../network_beacon_processor/sync_methods.rs | 4 + beacon_node/operation_pool/src/lib.rs | 103 ++++++++++++++- consensus/fork_choice/src/fork_choice.rs | 67 +++++++--- consensus/fork_choice/tests/tests.rs | 2 - .../src/fork_choice_test_definition.rs | 33 +++++ .../gloas_payload.rs | 90 +++++++++++++ consensus/proto_array/src/proto_array.rs | 18 ++- .../src/proto_array_fork_choice.rs | 3 +- .../process_operations.rs | 7 +- .../src/per_epoch_processing/single_pass.rs | 6 +- consensus/types/src/state/beacon_state.rs | 52 +++++--- consensus/types/src/state/slashings_cache.rs | 124 ++++++++++++++++++ testing/ef_tests/Makefile | 2 +- testing/ef_tests/check_all_files_accessed.py | 2 + .../ef_tests/src/cases/epoch_processing.rs | 3 + testing/ef_tests/src/cases/fork_choice.rs | 81 +++++++++--- testing/ef_tests/src/cases/operations.rs | 7 +- testing/ef_tests/src/handler.rs | 12 +- testing/ef_tests/tests/tests.rs | 24 ++-- 33 files changed, 785 insertions(+), 247 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index a7f1a7cfcd..5a521d18e6 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -4248,12 +4248,6 @@ impl BeaconChain { let cached_head = self.canonical_head.cached_head(); let old_head_slot = cached_head.head_slot(); - // Compute the expected proposer for `current_slot` on the canonical chain. This is used by - // `on_block` to gate proposer boost on the block's proposer matching the canonical proposer - // (per spec `update_proposer_boost_root` added in v1.7.0-alpha.5). - let canonical_head_proposer_index = - self.canonical_head_proposer_index(current_slot, &cached_head)?; - // Take an upgradable read lock on fork choice so we can check if this block has already // been imported. We don't want to repeat work importing a block that is already imported. let fork_choice_reader = self.canonical_head.fork_choice_upgradable_read_lock(); @@ -4285,7 +4279,6 @@ impl BeaconChain { block_delay, &state, payload_verification_status, - canonical_head_proposer_index, &self.spec, ) .map_err(|e| BlockError::BeaconChainError(Box::new(e.into())))?; @@ -5033,42 +5026,6 @@ impl BeaconChain { })) } - /// Compute the expected beacon proposer for `slot` on the canonical chain extending `cached_head`. - /// - /// Uses the beacon proposer cache to avoid recomputing the shuffling on every block import. - /// - /// This is used by `update_proposer_boost_root` to gate proposer boost on the block's proposer - /// matching the canonical proposer, per consensus-specs v1.7.0-alpha.5. - /// - /// This function should never error unless there is some corruption of the head state. If a - /// state advance is needed, it will be handled by the proposer cache. - pub fn canonical_head_proposer_index( - &self, - slot: Slot, - cached_head: &CachedHead, - ) -> Result { - let proposal_epoch = slot.epoch(T::EthSpec::slots_per_epoch()); - let head_block_root = cached_head.head_block_root(); - let head_state = &cached_head.snapshot.beacon_state; - - let shuffling_decision_root = head_state.proposer_shuffling_decision_root_at_epoch( - proposal_epoch, - head_block_root, - &self.spec, - )?; - - self.with_proposer_cache::<_, Error>( - shuffling_decision_root, - proposal_epoch, - |proposers| { - proposers - .get_slot::(slot) - .map(|p| p.index as u64) - }, - || Ok((cached_head.head_state_root(), head_state.clone())), - ) - } - pub fn get_expected_withdrawals( &self, forkchoice_update_params: &ForkchoiceUpdateParameters, diff --git a/beacon_node/beacon_chain/src/block_production/gloas.rs b/beacon_node/beacon_chain/src/block_production/gloas.rs index 82dad6f6ad..90fc60524a 100644 --- a/beacon_node/beacon_chain/src/block_production/gloas.rs +++ b/beacon_node/beacon_chain/src/block_production/gloas.rs @@ -163,7 +163,7 @@ impl BeaconChain { let should_build_on_full = self .canonical_head .fork_choice_read_lock() - .should_build_on_full(&parent_root, parent_payload_status) + .should_build_on_full(&parent_root, parent_payload_status, produce_at_slot) .map_err(|e| { BlockProductionError::BeaconChain(Box::new(BeaconChainError::ForkChoiceError(e))) })?; diff --git a/beacon_node/beacon_chain/src/payload_attestation_verification/gossip_verified_payload_attestation.rs b/beacon_node/beacon_chain/src/payload_attestation_verification/gossip_verified_payload_attestation.rs index 3e9f9e4b60..f8a1143b2e 100644 --- a/beacon_node/beacon_chain/src/payload_attestation_verification/gossip_verified_payload_attestation.rs +++ b/beacon_node/beacon_chain/src/payload_attestation_verification/gossip_verified_payload_attestation.rs @@ -70,13 +70,21 @@ impl VerifiedPayloadAttestationMessage { // 2. Blocks we've seen that are invalid (REJECT). // Presently both cases return IGNORE. let beacon_block_root = payload_attestation_message.data.beacon_block_root; - if ctx + let block = ctx .canonical_head .fork_choice_read_lock() .get_block(&beacon_block_root) - .is_none() - { - return Err(Error::UnknownHeadBlock { beacon_block_root }); + .ok_or(Error::UnknownHeadBlock { beacon_block_root })?; + + // [IGNORE] The block referenced by `data.beacon_block_root` is at slot `data.slot`, i.e. + // the block has `block.slot == data.slot`. A PTC member assigned to an empty slot must not + // attest, so ignore messages that reference an earlier block. + if block.slot != slot { + return Err(Error::BlockNotAtSlot { + beacon_block_root, + block_slot: block.slot, + data_slot: slot, + }); } let message_epoch = slot.epoch(T::EthSpec::slots_per_epoch()); diff --git a/beacon_node/beacon_chain/src/payload_attestation_verification/mod.rs b/beacon_node/beacon_chain/src/payload_attestation_verification/mod.rs index 89ae1bbbdd..d1fa7f52fb 100644 --- a/beacon_node/beacon_chain/src/payload_attestation_verification/mod.rs +++ b/beacon_node/beacon_chain/src/payload_attestation_verification/mod.rs @@ -60,6 +60,18 @@ pub enum Error { /// The attestation points to a block we have not yet imported. It's unclear if the /// attestation is valid or not. UnknownHeadBlock { beacon_block_root: Hash256 }, + /// The block referenced by `data.beacon_block_root` is not at slot `data.slot`, i.e. the + /// PTC member's assigned slot was likely empty. + /// + /// ## Peer scoring + /// + /// PTC members should not attest for empty slots, so we + /// ignore the message. + BlockNotAtSlot { + beacon_block_root: Hash256, + block_slot: Slot, + data_slot: Slot, + }, /// The validator index is not a member of the PTC for this slot. /// /// ## Peer scoring diff --git a/beacon_node/beacon_chain/src/payload_attestation_verification/tests.rs b/beacon_node/beacon_chain/src/payload_attestation_verification/tests.rs index d4b82c41fc..6c52e5ce2d 100644 --- a/beacon_node/beacon_chain/src/payload_attestation_verification/tests.rs +++ b/beacon_node/beacon_chain/src/payload_attestation_verification/tests.rs @@ -3,7 +3,6 @@ use std::time::Duration; use bls::Signature; use slot_clock::{SlotClock, TestingSlotClock}; -use state_processing::AllCaches; use types::{ Domain, Epoch, EthSpec, ForkName, Hash256, MinimalEthSpec, PayloadAttestationData, PayloadAttestationMessage, SignedRoot, Slot, @@ -167,6 +166,25 @@ fn unknown_head_block() { ); } +#[test] +fn block_not_at_slot() { + if !fork_name_from_env().is_some_and(|f| f.gloas_enabled()) { + return; + } + let ctx = TestContext::new(); + let gossip = ctx.gossip_ctx(); + + // The genesis block is at slot 0, but the message claims slot 1. A PTC member assigned to an + // empty slot must not attest, so this must be ignored (per consensus-specs #5281). + let msg = make_payload_attestation(Slot::new(1), 0, ctx.genesis_block_root); + let result = VerifiedPayloadAttestationMessage::new(msg, &gossip); + assert!( + matches!(result, Err(PayloadAttestationError::BlockNotAtSlot { .. })), + "expected BlockNotAtSlot, got: {:?}", + result + ); +} + #[test] fn not_in_ptc() { if !fork_name_from_env().is_some_and(|f| f.gloas_enabled()) { @@ -174,7 +192,7 @@ fn not_in_ptc() { } let ctx = TestContext::new(); let gossip = ctx.gossip_ctx(); - let slot = Slot::new(1); + let slot = Slot::new(0); let ptc_members = ctx.ptc_members(slot); let non_ptc_validator = (0..NUM_VALIDATORS as u64) @@ -196,7 +214,7 @@ fn invalid_signature() { } let ctx = TestContext::new(); let gossip = ctx.gossip_ctx(); - let slot = Slot::new(1); + let slot = Slot::new(0); let ptc_members = ctx.ptc_members(slot); let validator_index = ptc_members[0] as u64; @@ -216,7 +234,7 @@ fn valid_payload_attestation() { } let ctx = TestContext::new(); let gossip = ctx.gossip_ctx(); - let slot = Slot::new(1); + let slot = Slot::new(0); let ptc_members = ctx.ptc_members(slot); let validator_index = ptc_members[0] as u64; @@ -243,7 +261,7 @@ fn duplicate_after_valid() { } let ctx = TestContext::new(); let gossip = ctx.gossip_ctx(); - let slot = Slot::new(1); + let slot = Slot::new(0); let ptc_members = ctx.ptc_members(slot); let validator_index = ptc_members[0] as u64; @@ -300,10 +318,8 @@ async fn ptc_cache_is_primed_at_gloas_fork_boundary() { .mock_execution_layer() .build(); - harness.extend_to_slot(fork_boundary_slot).await; - for slot in test_slots { - harness.chain.slot_clock.set_slot(slot.as_u64()); + harness.extend_to_slot(slot).await; assert!( harness .chain @@ -350,10 +366,9 @@ async fn ptc_cache_is_primed_at_gloas_fork_boundary() { } } -/// Exercises payload attestation gossip verification when the message epoch is ahead of the -/// canonical head due to many missed slots. +/// Check that a payload attestation whose assigned slot is empty is ignored. #[tokio::test] -async fn stale_head_payload_attestation() { +async fn stale_head_empty_slot_payload_attestation_ignored() { if !fork_name_from_env().is_some_and(|f| f.gloas_enabled()) { return; } @@ -363,9 +378,9 @@ async fn stale_head_payload_attestation() { let head_slot = Slot::new(slots_per_epoch); let missed_epochs = 4; let target_slot = Slot::new(slots_per_epoch * (1 + missed_epochs)); - let target_epoch = target_slot.epoch(slots_per_epoch); - // GIVEN a chain with blocks through epoch 1 (so the store has states). + // Given a chain with blocks through epoch 1, then a slot clock advanced 4 epochs without + // producing blocks (simulating missed slots). let harness = BeaconChainHarness::builder(E::default()) .default_spec() .deterministic_keypairs(64) @@ -373,71 +388,30 @@ async fn stale_head_payload_attestation() { .mock_execution_layer() .build(); harness.extend_to_slot(head_slot).await; - - let head = harness.chain.canonical_head.cached_head(); - let head_epoch = head.snapshot.beacon_state.current_epoch(); - assert!( - target_epoch > head_epoch + harness.spec.min_seed_lookahead, - "precondition: message epoch must exceed head + min_seed_lookahead" - ); - - // GIVEN a slot clock advanced to epoch 5 without producing blocks - // (simulating missed slots during a liveness failure). harness.chain.slot_clock.set_slot(target_slot.as_u64()); - // Advance a reference state to compute the PTC at the target slot. - let mut reference_state = head.snapshot.beacon_state.clone(); - state_processing::state_advance::partial_state_advance( - &mut reference_state, - Some(head.snapshot.beacon_state_root()), - target_slot, - &harness.spec, - ) - .expect("should advance reference state"); - reference_state - .build_all_caches(&harness.spec) - .expect("should build caches"); + let head = harness.chain.canonical_head.cached_head(); - let ptc = reference_state - .get_ptc(target_slot, &harness.spec) - .expect("should get PTC from reference state"); - let validator_index = *ptc.0.first().expect("PTC should have at least one member") as u64; - - // WHEN a properly-signed payload attestation from a PTC member is verified. The signature - // domain should come from the spec fork schedule and genesis validators root, not a loaded - // state in the verifier. - let domain = harness.spec.get_domain( - target_epoch, - Domain::PTCAttester, - &reference_state.fork(), - reference_state.genesis_validators_root(), - ); + // When a payload attestation for empty target slot references a stale block root + // it is ignored because target_slot != block.slot let data = PayloadAttestationData { beacon_block_root: head.head_block_root(), slot: target_slot, payload_present: true, blob_data_available: true, }; - let message = data.signing_root(domain); - let signature = harness.validator_keypairs[validator_index as usize] - .sk - .sign(message); let msg = PayloadAttestationMessage { - validator_index, + validator_index: 0, data, - signature, + signature: Signature::empty(), }; - // THEN verification succeeds despite the head being 4 epochs stale. let result = harness .chain .verify_payload_attestation_message_for_gossip(msg); assert!( - result.is_ok(), - "expected Ok (head epoch {}, message epoch {}), got: {:?}", - head_epoch, - target_epoch, - result.unwrap_err() + matches!(result, Err(PayloadAttestationError::BlockNotAtSlot { .. })), + "expected BlockNotAtSlot, got: {result:?}" ); } diff --git a/beacon_node/beacon_chain/src/payload_bid_verification/gossip_verified_bid.rs b/beacon_node/beacon_chain/src/payload_bid_verification/gossip_verified_bid.rs index 354705b92c..1687760d25 100644 --- a/beacon_node/beacon_chain/src/payload_bid_verification/gossip_verified_bid.rs +++ b/beacon_node/beacon_chain/src/payload_bid_verification/gossip_verified_bid.rs @@ -144,9 +144,17 @@ impl GossipVerifiedPayloadBid { let fork_choice = ctx.canonical_head.fork_choice_read_lock(); // TODO(gloas) reprocess bids whose parent_block_root becomes known & canonical after a reorg? - if !fork_choice.contains_block(&bid_parent_block_root) { - return Err(PayloadBidError::ParentBlockRootUnknown { + let parent_block = fork_choice.get_block(&bid_parent_block_root).ok_or( + PayloadBidError::ParentBlockRootUnknown { parent_block_root: bid_parent_block_root, + }, + )?; + + // [REJECT] The bid is for a higher slot than its parent block. + if bid_slot <= parent_block.slot { + return Err(PayloadBidError::BidNotDescendantOfParent { + bid_slot, + parent_slot: parent_block.slot, }); } diff --git a/beacon_node/beacon_chain/src/payload_bid_verification/mod.rs b/beacon_node/beacon_chain/src/payload_bid_verification/mod.rs index a40fd14872..e23c537e18 100644 --- a/beacon_node/beacon_chain/src/payload_bid_verification/mod.rs +++ b/beacon_node/beacon_chain/src/payload_bid_verification/mod.rs @@ -37,6 +37,8 @@ pub enum PayloadBidError { }, /// The bids slot is not the current slot or the next slot. InvalidBidSlot { bid_slot: Slot }, + /// The bid's slot is not greater than the slot of its parent block. + BidNotDescendantOfParent { bid_slot: Slot, parent_slot: Slot }, /// The slot clock cannot be read. UnableToReadSlot, /// No proposer preferences for the current slot. diff --git a/beacon_node/beacon_chain/src/payload_bid_verification/tests.rs b/beacon_node/beacon_chain/src/payload_bid_verification/tests.rs index ccdf64d41d..04eb875bd9 100644 --- a/beacon_node/beacon_chain/src/payload_bid_verification/tests.rs +++ b/beacon_node/beacon_chain/src/payload_bid_verification/tests.rs @@ -310,7 +310,7 @@ fn builder_already_seen_for_slot() { } let ctx = TestContext::new(); let gossip = ctx.gossip_ctx(); - let slot = Slot::new(0); + let slot = Slot::new(1); seed_preferences(&ctx, slot, Address::ZERO, 30_000_000); let bid = make_signed_bid(slot, 42, Address::ZERO, 30_000_000, 100, Hash256::ZERO); @@ -336,7 +336,7 @@ fn bid_value_below_cached() { } let ctx = TestContext::new(); let gossip = ctx.gossip_ctx(); - let slot = Slot::new(0); + let slot = Slot::new(1); seed_preferences(&ctx, slot, Address::ZERO, 30_000_000); let high_bid = GossipVerifiedPayloadBid { @@ -384,7 +384,7 @@ fn fee_recipient_mismatch() { } let ctx = TestContext::new(); let gossip = ctx.gossip_ctx(); - let slot = Slot::new(0); + let slot = Slot::new(1); seed_preferences(&ctx, slot, Address::repeat_byte(0xaa), 30_000_000); let bid = make_signed_bid( @@ -406,7 +406,7 @@ fn gas_limit_mismatch() { } let ctx = TestContext::new(); let gossip = ctx.gossip_ctx(); - let slot = Slot::new(0); + let slot = Slot::new(1); seed_preferences(&ctx, slot, Address::ZERO, 30_000_000); let bid = make_signed_bid( @@ -428,7 +428,7 @@ fn execution_payment_nonzero() { } let ctx = TestContext::new(); let gossip = ctx.gossip_ctx(); - let slot = Slot::new(0); + let slot = Slot::new(1); seed_preferences(&ctx, slot, Address::ZERO, 30_000_000); let bid = Arc::new(SignedExecutionPayloadBid { @@ -455,7 +455,7 @@ fn unknown_builder_index() { } let ctx = TestContext::new(); let gossip = ctx.gossip_ctx(); - let slot = Slot::new(0); + let slot = Slot::new(1); seed_preferences(&ctx, slot, Address::ZERO, 30_000_000); // Use a builder_index that doesn't exist in the registry. @@ -483,7 +483,7 @@ fn inactive_builder() { } let ctx = TestContext::new(); let gossip = ctx.gossip_ctx(); - let slot = Slot::new(0); + let slot = Slot::new(1); seed_preferences(&ctx, slot, Address::ZERO, 30_000_000); let bid = make_signed_bid( @@ -508,7 +508,7 @@ fn builder_cant_cover_bid() { } let ctx = TestContext::new(); let gossip = ctx.gossip_ctx(); - let slot = Slot::new(0); + let slot = Slot::new(1); seed_preferences(&ctx, slot, Address::ZERO, 30_000_000); // Builder index 0 exists but bid value far exceeds their balance. @@ -534,7 +534,7 @@ fn parent_block_root_unknown() { } let ctx = TestContext::new(); let gossip = ctx.gossip_ctx(); - let slot = Slot::new(0); + let slot = Slot::new(1); seed_preferences(&ctx, slot, Address::ZERO, 30_000_000); // Parent block root not in fork choice. @@ -556,7 +556,9 @@ fn parent_block_root_not_canonical() { } let ctx = TestContext::new(); let gossip = ctx.gossip_ctx(); - let slot = Slot::new(0); + // The non-canonical fork block is at slot 1, so use slot 2 to satisfy the `bid.slot > parent + // block slot` rule and exercise the bid descendant from parent check specifically. + let slot = Slot::new(2); seed_preferences(&ctx, slot, Address::ZERO, 30_000_000); let fork_root = ctx.insert_non_canonical_block(); @@ -570,6 +572,36 @@ fn parent_block_root_not_canonical() { ); } +#[test] +fn bid_slot_not_after_parent() { + if !fork_name_from_env().is_some_and(|f| f.gloas_enabled()) { + return; + } + let ctx = TestContext::new(); + let gossip = ctx.gossip_ctx(); + // The genesis (parent) block is at slot 0, so a bid at slot 0 is not for a higher slot than + // its parent and must be rejected. + let slot = Slot::new(0); + seed_preferences(&ctx, slot, Address::ZERO, 30_000_000); + + let bid = make_signed_bid( + slot, + 0, + Address::ZERO, + 30_000_000, + 0, + ctx.genesis_block_root, + ); + let result = GossipVerifiedPayloadBid::new(bid, &gossip); + assert!( + matches!( + result, + Err(PayloadBidError::BidNotDescendantOfParent { .. }) + ), + "expected BidNotDescendantOfParent, got: {result:?}" + ); +} + #[test] fn invalid_blob_kzg_commitments() { if !fork_name_from_env().is_some_and(|f| f.gloas_enabled()) { @@ -577,7 +609,7 @@ fn invalid_blob_kzg_commitments() { } let ctx = TestContext::new(); let gossip = ctx.gossip_ctx(); - let slot = Slot::new(0); + let slot = Slot::new(1); seed_preferences(&ctx, slot, Address::ZERO, 30_000_000); let max_blobs = ctx @@ -614,7 +646,7 @@ fn bad_signature() { } let ctx = TestContext::new(); let gossip = ctx.gossip_ctx(); - let slot = Slot::new(0); + let slot = Slot::new(1); seed_preferences(&ctx, slot, Address::ZERO, 30_000_000); // All checks pass but signature is empty/invalid. @@ -643,7 +675,7 @@ fn valid_bid() { } let ctx = TestContext::new(); let gossip = ctx.gossip_ctx(); - let slot = Slot::new(0); + let slot = Slot::new(1); seed_preferences(&ctx, slot, Address::ZERO, 30_000_000); let bid = ctx.sign_bid(ExecutionPayloadBid { @@ -670,7 +702,7 @@ fn two_builders_coexist_in_cache() { } let ctx = TestContext::new(); let gossip = ctx.gossip_ctx(); - let slot = Slot::new(0); + let slot = Slot::new(1); seed_preferences(&ctx, slot, Address::ZERO, 30_000_000); let bid_0 = ctx.sign_bid(ExecutionPayloadBid { @@ -725,7 +757,7 @@ fn bid_equal_to_cached_value_rejected() { } let ctx = TestContext::new(); let gossip = ctx.gossip_ctx(); - let slot = Slot::new(0); + let slot = Slot::new(1); seed_preferences(&ctx, slot, Address::ZERO, 30_000_000); // Seed a cached bid with value 100. diff --git a/beacon_node/beacon_chain/src/payload_envelope_verification/import.rs b/beacon_node/beacon_chain/src/payload_envelope_verification/import.rs index 00806f0e17..90cdb4fe97 100644 --- a/beacon_node/beacon_chain/src/payload_envelope_verification/import.rs +++ b/beacon_node/beacon_chain/src/payload_envelope_verification/import.rs @@ -117,9 +117,6 @@ impl BeaconChain { ); // TODO(gloas) do we need to send a `PayloadImported` event to the reprocess queue? - // TODO(gloas) do we need to recompute head? - // should canonical_head return the block and the payload now? - self.recompute_head_at_current_slot().await; metrics::inc_counter(&metrics::ENVELOPE_PROCESSING_SUCCESSES); diff --git a/beacon_node/beacon_chain/tests/op_verification.rs b/beacon_node/beacon_chain/tests/op_verification.rs index adc14541a9..d1df72234f 100644 --- a/beacon_node/beacon_chain/tests/op_verification.rs +++ b/beacon_node/beacon_chain/tests/op_verification.rs @@ -299,6 +299,67 @@ async fn proposer_slashing_duplicate_in_state() { )); } +#[tokio::test] +async fn slashings_cache_matches_state_after_block_import() { + let db_path = tempdir().unwrap(); + let store = get_store(&db_path); + let harness = get_harness(store.clone(), VALIDATOR_COUNT); + + // Slash a spread of validators by importing proposer slashings into the op pool, exactly as + // they would arrive over gossip. + let slashed_validators = [0u64, 7, VALIDATOR_COUNT as u64 - 1]; + for &validator_index in &slashed_validators { + let slashing = harness.make_proposer_slashing(validator_index); + let ObservationOutcome::New(verified_slashing) = harness + .chain + .verify_proposer_slashing_for_gossip(slashing) + .unwrap() + else { + panic!("slashing should verify"); + }; + harness.chain.import_proposer_slashing(verified_slashing); + } + + // Produce and import a block that includes the slashings. This drives the production flow: + // `per_block_processing` -> `slash_validator` -> `SlashingsCache::record_validator_slashing`. + harness + .extend_chain( + 1, + BlockStrategy::OnCanonicalHead, + AttestationStrategy::AllValidators, + ) + .await; + + let state = harness.get_current_state(); + + // The block processing above should have left the slashings cache initialized for the head. + assert!( + state.slashings_cache_is_initialized(), + "slashings cache should be initialized after block import" + ); + + // The targeted validators must actually be slashed in the state (i.e. the slashings were + // included and applied, not silently dropped). + for &validator_index in &slashed_validators { + assert!( + state + .get_validator(validator_index as usize) + .unwrap() + .slashed, + "validator {validator_index} should be slashed in the state" + ); + } + + // The cache must agree with the `slashed` flag of *every* validator in the state. + for index in 0..state.validators().len() { + assert_eq!( + state.slashings_cache().is_slashed(index), + state.get_validator(index).unwrap().slashed, + "slashings cache disagrees with state at validator {index}" + ); + } +} + #[test] fn attester_slashing() { let db_path = tempdir().unwrap(); diff --git a/beacon_node/beacon_chain/tests/payload_invalidation.rs b/beacon_node/beacon_chain/tests/payload_invalidation.rs index 42a78d740f..c89be0f5dd 100644 --- a/beacon_node/beacon_chain/tests/payload_invalidation.rs +++ b/beacon_node/beacon_chain/tests/payload_invalidation.rs @@ -1077,7 +1077,6 @@ async fn invalid_parent() { Duration::from_secs(0), &state, PayloadVerificationStatus::Optimistic, - block.message().proposer_index(), &rig.harness.chain.spec, ), Err(ForkChoiceError::ProtoArrayStringError(message)) diff --git a/beacon_node/http_api/src/beacon/execution_payload_envelopes.rs b/beacon_node/http_api/src/beacon/execution_payload_envelopes.rs index f8ab8cddc8..b6b681e091 100644 --- a/beacon_node/http_api/src/beacon/execution_payload_envelopes.rs +++ b/beacon_node/http_api/src/beacon/execution_payload_envelopes.rs @@ -8,7 +8,9 @@ use crate::version::{ }; use beacon_chain::data_column_verification::{GossipDataColumnError, GossipVerifiedDataColumn}; use beacon_chain::payload_envelope_verification::EnvelopeError; -use beacon_chain::{BeaconChain, BeaconChainTypes, NotifyExecutionLayer}; +use beacon_chain::{ + AvailabilityProcessingStatus, BeaconChain, BeaconChainTypes, NotifyExecutionLayer, +}; use bytes::Bytes; use eth2::types as api_types; use lighthouse_network::PubsubMessage; @@ -160,12 +162,16 @@ pub async fn publish_execution_payload_envelope( ) .await; - if let Err(e) = import_result { - warn!(%slot, error = ?e, "Failed to import execution payload envelope"); - return Err(warp_utils::reject::custom_server_error(format!( - "envelope import failed: {e}" - ))); - } + let mut envelope_imported = match &import_result { + Ok(AvailabilityProcessingStatus::Imported(_)) => true, + Ok(AvailabilityProcessingStatus::MissingComponents(_, _)) => false, + Err(e) => { + warn!(%slot, error = ?e, "Failed to import execution payload envelope"); + return Err(warp_utils::reject::custom_server_error(format!( + "envelope import failed: {e}" + ))); + } + }; // From here on the envelope is on the wire. `take_blobs` already consumed the cache // entry, so a retry would not republish columns; returning Err would mislead the @@ -201,19 +207,27 @@ pub async fn publish_execution_payload_envelope( .collect::>(); // Local processing only — envelope already broadcast, so log and fall through. - if !sampling_columns.is_empty() - && let Err(e) = - Box::pin(chain.process_gossip_data_columns(sampling_columns, || Ok(()))).await - { - error!( - %slot, - error = ?e, - "Failed to process sampling data columns during envelope publication" - ); + if !sampling_columns.is_empty() { + match Box::pin(chain.process_gossip_data_columns(sampling_columns, || Ok(()))).await + { + Ok(AvailabilityProcessingStatus::Imported(_)) => envelope_imported = true, + Ok(AvailabilityProcessingStatus::MissingComponents(_, _)) => {} + Err(e) => { + error!( + %slot, + error = ?e, + "Failed to process sampling data columns during envelope publication" + ); + } + } } } } + if envelope_imported { + chain.recompute_head_at_current_slot().await; + } + Ok(warp::reply().into_response()) } diff --git a/beacon_node/http_api/src/block_id.rs b/beacon_node/http_api/src/block_id.rs index dce4713245..50d5c8d165 100644 --- a/beacon_node/http_api/src/block_id.rs +++ b/beacon_node/http_api/src/block_id.rs @@ -621,10 +621,6 @@ mod tests { .unwrap(); let current_slot = harness.get_current_slot(); - let cached_head = chain.canonical_head.cached_head(); - let canonical_head_proposer_index = chain - .canonical_head_proposer_index(current_slot, &cached_head) - .unwrap(); chain .canonical_head @@ -636,7 +632,6 @@ mod tests { Duration::ZERO, &post_state, PayloadVerificationStatus::Verified, - canonical_head_proposer_index, &chain.spec, ) .unwrap(); diff --git a/beacon_node/network/src/network_beacon_processor/gossip_methods.rs b/beacon_node/network/src/network_beacon_processor/gossip_methods.rs index 98c143eaeb..b52732000e 100644 --- a/beacon_node/network/src/network_beacon_processor/gossip_methods.rs +++ b/beacon_node/network/src/network_beacon_processor/gossip_methods.rs @@ -3795,13 +3795,19 @@ impl NetworkBeaconProcessor { // TODO(gloas) metrics // register_process_result_metrics(&result, metrics::BlockSource::Gossip, "envelope"); - if let Err(e) = &result { - debug!( - ?beacon_block_root, - %peer_id, - error = ?e, - "Execution payload envelope processing failed" - ); + match &result { + Ok(AvailabilityProcessingStatus::Imported(_)) => { + self.chain.recompute_head_at_current_slot().await; + } + Ok(AvailabilityProcessingStatus::MissingComponents(_, _)) => {} + Err(e) => { + debug!( + ?beacon_block_root, + %peer_id, + error = ?e, + "Execution payload envelope processing failed" + ); + } } } @@ -3829,7 +3835,8 @@ impl NetworkBeaconProcessor { | PayloadBidError::InvalidBuilder { .. } | PayloadBidError::InvalidFeeRecipient | PayloadBidError::ExecutionPaymentNonZero { .. } - | PayloadBidError::InvalidBlobKzgCommitments { .. }, + | PayloadBidError::InvalidBlobKzgCommitments { .. } + | PayloadBidError::BidNotDescendantOfParent { .. }, ) => { self.propagate_validation_result(message_id, peer_id, MessageAcceptance::Reject); self.gossip_penalize_peer( @@ -4008,6 +4015,14 @@ impl NetworkBeaconProcessor { *beacon_block_root, )) } + PayloadAttestationError::BlockNotAtSlot { .. } => { + debug!( + %peer_id, + %message_slot, + "Payload attestation references block at wrong slot" + ); + self.propagate_validation_result(message_id, peer_id, MessageAcceptance::Ignore); + } PayloadAttestationError::NotInPTC { .. } => { self.propagate_validation_result(message_id, peer_id, MessageAcceptance::Reject); self.gossip_penalize_peer( diff --git a/beacon_node/network/src/network_beacon_processor/sync_methods.rs b/beacon_node/network/src/network_beacon_processor/sync_methods.rs index e2226af094..caf718732b 100644 --- a/beacon_node/network/src/network_beacon_processor/sync_methods.rs +++ b/beacon_node/network/src/network_beacon_processor/sync_methods.rs @@ -376,6 +376,10 @@ impl NetworkBeaconProcessor { let result: Result = result.map_err(|e| BlockError::InternalError(format!("envelope: {e}"))); + if matches!(result, Ok(AvailabilityProcessingStatus::Imported(_))) { + self.chain.recompute_head_at_current_slot().await; + } + self.send_sync_message(SyncMessage::BlockComponentProcessed { process_type, result: result.into(), diff --git a/beacon_node/operation_pool/src/lib.rs b/beacon_node/operation_pool/src/lib.rs index a1789e3b19..45563c2ee9 100644 --- a/beacon_node/operation_pool/src/lib.rs +++ b/beacon_node/operation_pool/src/lib.rs @@ -546,9 +546,15 @@ impl OperationPool { } }); + let max_attester_slashings = if state.fork_name_unchecked().electra_enabled() { + E::max_attester_slashings_electra() + } else { + E::MaxAttesterSlashings::to_usize() + }; + maximum_cover( relevant_attester_slashings, - E::MaxAttesterSlashings::to_usize(), + max_attester_slashings, "attester_slashings", ) .into_iter() @@ -927,6 +933,29 @@ mod release_tests { harness } + /// The maximum number of attester slashings allowed in a block for the state's fork. + fn max_attester_slashings(state: &BeaconState) -> usize { + if state.fork_name_unchecked().electra_enabled() { + E::max_attester_slashings_electra() + } else { + E::MaxAttesterSlashings::to_usize() + } + } + + /// Given the candidate slashings ordered most-profitable first, return the prefix that a + /// block on the state's fork would actually include (i.e. the N most profitable, where N + /// is the per-block attester slashing limit). This keeps the max-cover assertions generic + /// across forks. + fn most_profitable_slashings( + state: &BeaconState, + ordered_by_profitability: Vec, + ) -> Vec { + ordered_by_profitability + .into_iter() + .take(max_attester_slashings(state)) + .collect() + } + /// Test state for attestation-related tests. fn attestation_test_state( num_committees: usize, @@ -1594,7 +1623,10 @@ mod release_tests { op_pool.insert_attester_slashing(slashing_4.clone().validate(&state, spec).unwrap()); let best_slashings = op_pool.get_slashings_and_exits(&state, &harness.spec); - assert_eq!(best_slashings.1, vec![slashing_4, slashing_3]); + assert_eq!( + best_slashings.1, + most_profitable_slashings(&state, vec![slashing_4, slashing_3]) + ); } // Check that we get maximum coverage for attester slashings with overlapping indices @@ -1616,7 +1648,10 @@ mod release_tests { op_pool.insert_attester_slashing(slashing_4.clone().validate(&state, spec).unwrap()); let best_slashings = op_pool.get_slashings_and_exits(&state, &harness.spec); - assert_eq!(best_slashings.1, vec![slashing_1, slashing_3]); + assert_eq!( + best_slashings.1, + most_profitable_slashings(&state, vec![slashing_1, slashing_3]) + ); } // Max coverage of attester slashings taking into account proposer slashings @@ -1638,7 +1673,10 @@ mod release_tests { op_pool.insert_attester_slashing(a_slashing_3.clone().validate(&state, spec).unwrap()); let best_slashings = op_pool.get_slashings_and_exits(&state, &harness.spec); - assert_eq!(best_slashings.1, vec![a_slashing_1, a_slashing_3]); + assert_eq!( + best_slashings.1, + most_profitable_slashings(&state, vec![a_slashing_1, a_slashing_3]) + ); } //Max coverage checking that non overlapping indices are still recognized for their value @@ -1661,7 +1699,10 @@ mod release_tests { op_pool.insert_attester_slashing(slashing_3.clone().validate(&state, spec).unwrap()); let best_slashings = op_pool.get_slashings_and_exits(&state, &harness.spec); - assert_eq!(best_slashings.1, vec![slashing_1, slashing_3]); + assert_eq!( + best_slashings.1, + most_profitable_slashings(&state, vec![slashing_1, slashing_3]) + ); } // Max coverage should be affected by the overall effective balances @@ -1684,7 +1725,10 @@ mod release_tests { op_pool.insert_attester_slashing(slashing_3.clone().validate(&state, spec).unwrap()); let best_slashings = op_pool.get_slashings_and_exits(&state, &harness.spec); - assert_eq!(best_slashings.1, vec![slashing_2, slashing_3]); + assert_eq!( + best_slashings.1, + most_profitable_slashings(&state, vec![slashing_2, slashing_3]) + ); } /// End-to-end test of basic sync contribution handling. @@ -2177,6 +2221,53 @@ mod release_tests { assert_eq!(op_pool.attester_slashings.read().len(), 1); } + /// Regression test to ensure that we are using the correct spec value for max attester slashings post-Electra. + #[tokio::test] + async fn attester_slashings_capped_at_electra_limit() { + let (harness, spec) = cross_fork_harness::(); + let slots_per_epoch = MainnetEthSpec::slots_per_epoch(); + let electra_fork_epoch = spec.electra_fork_epoch.unwrap(); + let deneb_fork_epoch = spec.deneb_fork_epoch.unwrap(); + + let op_pool = OperationPool::::new(); + + harness + .extend_to_slot(electra_fork_epoch.start_slot(slots_per_epoch)) + .await; + let electra_head = harness.chain.canonical_head.cached_head().snapshot; + assert!( + electra_head + .beacon_state + .fork_name_unchecked() + .electra_enabled() + ); + + // Create two slashings + for validators in [vec![0], vec![1]] { + let slashing = harness.make_attester_slashing_with_epochs( + validators, + Some(Epoch::new(0)), + Some(deneb_fork_epoch - 1), + Some(Epoch::new(0)), + Some(deneb_fork_epoch - 1), + ); + let verified = slashing + .validate(&electra_head.beacon_state, &harness.chain.spec) + .unwrap(); + op_pool.insert_attester_slashing(verified); + } + assert_eq!(op_pool.attester_slashings.read().len(), 2); + + // Despite two valid slashings being pending, only one may be extracted post-Electra. + let mut to_be_slashed = HashSet::new(); + let attester_slashings = + op_pool.get_attester_slashings(&electra_head.beacon_state, &mut to_be_slashed); + assert_eq!( + attester_slashings.len(), + MainnetEthSpec::max_attester_slashings_electra() + ); + } + fn make_payload_attestation_message( slot: Slot, validator_index: u64, diff --git a/consensus/fork_choice/src/fork_choice.rs b/consensus/fork_choice/src/fork_choice.rs index 7816fb4483..90f2bb9a67 100644 --- a/consensus/fork_choice/src/fork_choice.rs +++ b/consensus/fork_choice/src/fork_choice.rs @@ -542,6 +542,27 @@ where } } + /// Returns the dependent root for `block_root`, per the spec `get_dependent_root` helper. + fn get_dependent_root( + &self, + block_root: Hash256, + current_slot: Slot, + spec: &ChainSpec, + ) -> Result, Error> { + let epoch = current_slot.epoch(E::slots_per_epoch()); + + if epoch <= spec.min_seed_lookahead { + return Ok(Some(Hash256::zero())); + } + + let dependent_slot = epoch + .saturating_sub(spec.min_seed_lookahead) + .start_slot(E::slots_per_epoch()) + .saturating_sub(1_u64); + + self.get_ancestor(block_root, dependent_slot) + } + /// Run the fork choice rule to determine the head. /// /// ## Specification @@ -768,7 +789,6 @@ where block_delay: Duration, state: &BeaconState, payload_verification_status: PayloadVerificationStatus, - canonical_head_proposer_index: u64, spec: &ChainSpec, ) -> Result<(), Error> { let _timer = metrics::start_timer(&metrics::FORK_CHOICE_ON_BLOCK_TIMES); @@ -780,10 +800,17 @@ where return Ok(()); } - // Provide the slot (as per the system clock) to the `fc_store` and then return its view of - // the current slot. The `fc_store` will ensure that the `current_slot` is never - // decreasing, a property which we must maintain. - let current_slot = self.update_time(system_time_current_slot)?; + let head_root = if system_time_current_slot == self.fc_store.get_current_slot() { + // Fork choice has already run for the current slot, so we can safely use the cached + // head without recomputing it. + self.cached_fork_choice_view().head_block_root + } else { + // Fork choice hasn't run for the current slot yet: run it, updating the fork choice + // store's current slot in the process. + self.get_head(system_time_current_slot, spec)?.0 + }; + let current_slot = self.fc_store.get_current_slot(); + debug_assert_eq!(current_slot, system_time_current_slot); // Parent block must be known. let parent_block = self @@ -833,19 +860,24 @@ where let attestation_threshold = spec.get_attestation_due::(block.slot()); - // Add proposer score boost if the block is the first timely block for this slot and its - // proposer matches the expected proposer on the canonical chain (per spec - // `update_proposer_boost_root`, introduced in v1.7.0-alpha.5). + // Add proposer score boost if the block is the first timely block for this slot and it + // shares the same dependent root as the canonical chain head (per spec + // `update_proposer_boost_root`). let is_before_attesting_interval = block_delay < attestation_threshold; - + let is_timely = current_slot == block.slot() && is_before_attesting_interval; let is_first_block = self.fc_store.proposer_boost_root().is_zero(); - let is_canonical_proposer = block.proposer_index() == canonical_head_proposer_index; - if current_slot == block.slot() - && is_before_attesting_interval - && is_first_block - && is_canonical_proposer - { - self.fc_store.set_proposer_boost_root(block_root); + + if is_timely && is_first_block { + // The block isn't in fork choice so resolve its dependent root via its parent. + let block_dependent_root = + self.get_dependent_root(block.parent_root(), current_slot, spec)?; + let head_dependent_root = self.get_dependent_root(head_root, current_slot, spec)?; + + // Add proposer score boost if the block is timely, not conflicting with an + // existing block, with the same dependent root as the canonical chain head. + if block_dependent_root.is_some() && block_dependent_root == head_dependent_root { + self.fc_store.set_proposer_boost_root(block_root); + } } // Update store with checkpoints if necessary @@ -1581,9 +1613,10 @@ where &self, block_root: &Hash256, parent_payload_status: PayloadStatus, + current_slot: Slot, ) -> Result> { self.proto_array - .should_build_on_full::(block_root, parent_payload_status) + .should_build_on_full::(block_root, parent_payload_status, current_slot) .map_err(Error::ProtoArrayStringError) } diff --git a/consensus/fork_choice/tests/tests.rs b/consensus/fork_choice/tests/tests.rs index 848834b4d8..02229e6f33 100644 --- a/consensus/fork_choice/tests/tests.rs +++ b/consensus/fork_choice/tests/tests.rs @@ -316,7 +316,6 @@ impl ForkChoiceTest { Duration::from_secs(0), &state, PayloadVerificationStatus::Verified, - block.message().proposer_index(), &self.harness.chain.spec, ) .unwrap(); @@ -360,7 +359,6 @@ impl ForkChoiceTest { Duration::from_secs(0), &state, PayloadVerificationStatus::Verified, - block.message().proposer_index(), &self.harness.chain.spec, ) .expect_err("on_block did not return an error"); diff --git a/consensus/proto_array/src/fork_choice_test_definition.rs b/consensus/proto_array/src/fork_choice_test_definition.rs index 3dc5406212..7ffa763308 100644 --- a/consensus/proto_array/src/fork_choice_test_definition.rs +++ b/consensus/proto_array/src/fork_choice_test_definition.rs @@ -124,6 +124,15 @@ pub enum Operation { #[serde(default)] proposer_boost_root: Option, }, + /// Assert the result of `should_build_on_full` for the parent `block_root`, where + /// `parent_payload_status` is the status the proposer would build on and `proposal_slot` + /// is the slot being proposed. + AssertShouldBuildOnFull { + block_root: Hash256, + parent_payload_status: PayloadStatus, + proposal_slot: Slot, + expected: bool, + }, } #[derive(Debug, Clone, Serialize, Deserialize)] @@ -606,6 +615,30 @@ impl ForkChoiceTestDefinition { op_index ); } + Operation::AssertShouldBuildOnFull { + block_root, + parent_payload_status, + proposal_slot, + expected, + } => { + let actual = fork_choice + .should_build_on_full::( + &block_root, + parent_payload_status, + proposal_slot, + ) + .unwrap_or_else(|e| { + panic!( + "should_build_on_full op at index {} returned error: {}", + op_index, e + ) + }); + assert_eq!( + actual, expected, + "should_build_on_full mismatch at op index {}", + op_index + ); + } } } } diff --git a/consensus/proto_array/src/fork_choice_test_definition/gloas_payload.rs b/consensus/proto_array/src/fork_choice_test_definition/gloas_payload.rs index bf79a0170f..07262fb0d7 100644 --- a/consensus/proto_array/src/fork_choice_test_definition/gloas_payload.rs +++ b/consensus/proto_array/src/fork_choice_test_definition/gloas_payload.rs @@ -971,6 +971,90 @@ pub fn get_gloas_proposer_boost_flips_ancestor_test_definition() -> ForkChoiceTe } } +/// Tests the slot check in `should_build_on_full`. When the parent is from an earlier slot the +/// function returns `true` and ignores PTC data-availability votes. It only checks those votes +/// when the parent is from the immediately preceding slot. +pub fn get_gloas_should_build_on_full_test_definition() -> ForkChoiceTestDefinition { + let mut ops = vec![]; + + // Block 1 at slot 1, child of genesis. + ops.push(Operation::ProcessBlock { + slot: Slot::new(1), + root: get_root(1), + parent_root: get_root(0), + justified_checkpoint: get_checkpoint(0), + finalized_checkpoint: get_checkpoint(0), + execution_payload_parent_hash: Some(get_hash(0)), + execution_payload_block_hash: Some(get_hash(1)), + }); + + // PTC has voted the payload data unavailable. `is_timely` sets `payload_received` so the votes + // are consulted, and clearing the data-availability bits gives the "false" votes a majority. + ops.push(Operation::SetPayloadTiebreak { + block_root: get_root(1), + is_timely: true, + is_data_available: false, + }); + + // When the parent is `Empty` `should_build_on_full` returns `false`. This check runs before + // the slot check, so the result is `false` for both the previous-slot case (block slot 1, proposal slot 2) + // and an earlier-slot case (proposal slot 3). + ops.push(Operation::AssertShouldBuildOnFull { + block_root: get_root(1), + parent_payload_status: PayloadStatus::Empty, + proposal_slot: Slot::new(2), + expected: false, + }); + ops.push(Operation::AssertShouldBuildOnFull { + block_root: get_root(1), + parent_payload_status: PayloadStatus::Empty, + proposal_slot: Slot::new(3), + expected: false, + }); + + // `Full` parent from the immediately preceding slot (block slot 1, proposal slot 2). The PTC + // votes are consulted, and since data is unavailable the proposer does not build on full. + ops.push(Operation::AssertShouldBuildOnFull { + block_root: get_root(1), + parent_payload_status: PayloadStatus::Full, + proposal_slot: Slot::new(2), + expected: false, + }); + + // `Full` parent from an *earlier* slot (block slot 1, proposal slot 3). The slot check + // short-circuits to `true` without consulting the (unavailable) PTC votes. + ops.push(Operation::AssertShouldBuildOnFull { + block_root: get_root(1), + parent_payload_status: PayloadStatus::Full, + proposal_slot: Slot::new(3), + expected: true, + }); + + // Flip the PTC view to *available* and re-check the previous-slot case. The votes now permit + // building on full. + ops.push(Operation::SetPayloadTiebreak { + block_root: get_root(1), + is_timely: true, + is_data_available: true, + }); + ops.push(Operation::AssertShouldBuildOnFull { + block_root: get_root(1), + parent_payload_status: PayloadStatus::Full, + proposal_slot: Slot::new(2), + expected: true, + }); + + ForkChoiceTestDefinition { + finalized_block_slot: Slot::new(0), + justified_checkpoint: get_checkpoint(0), + finalized_checkpoint: get_checkpoint(0), + operations: ops, + execution_payload_parent_hash: Some(get_hash(42)), + execution_payload_block_hash: Some(get_hash(0)), + spec: Some(gloas_spec()), + } +} + #[cfg(test)] mod tests { use super::*; @@ -1160,6 +1244,12 @@ mod tests { test.run(); } + #[test] + fn should_build_on_full_slot_check() { + let test = get_gloas_should_build_on_full_test_definition(); + test.run(); + } + /// Test that execution payload invalidation propagates across the V17→V29 fork /// boundary: after invalidating a V17 parent, head must not select any descendant. /// diff --git a/consensus/proto_array/src/proto_array.rs b/consensus/proto_array/src/proto_array.rs index bd15bb4599..d45412c608 100644 --- a/consensus/proto_array/src/proto_array.rs +++ b/consensus/proto_array/src/proto_array.rs @@ -1569,10 +1569,13 @@ impl ProtoArray { /// Called by the proposer to decide whether to build on the full or empty /// parent pending node. Returns false if the PTC has voted the data as unavailable. + /// For a parent from an earlier slot the `Empty` or `Full` node has already been resolved + /// by attestation weight in `get_head`. pub fn should_build_on_full( &self, fc_node: &IndexedForkChoiceNode, proto_node: &ProtoNode, + current_slot: Slot, ) -> Result { if fc_node.payload_status == PayloadStatus::Pending { return Err(Error::InvalidPayloadStatus { @@ -1584,10 +1587,23 @@ impl ProtoArray { if fc_node.payload_status == PayloadStatus::Empty { return Ok(false); } + + if proto_node.slot().saturating_add(1u64) != current_slot { + return Ok(true); + } + // Check that false votes have not achieved an absolute majority. This allows the payload to be // considered available when either a majority have voted true or not enough votes have // been cast either way. - Ok(!proto_node.payload_data_availability::(false)?) + if proto_node.payload_data_availability::(false)? { + return Ok(false); + } + + if proto_node.payload_timeliness::(false)? { + return Ok(false); + } + + Ok(true) } pub fn should_extend_payload( diff --git a/consensus/proto_array/src/proto_array_fork_choice.rs b/consensus/proto_array/src/proto_array_fork_choice.rs index f0b599dbcd..69202486a7 100644 --- a/consensus/proto_array/src/proto_array_fork_choice.rs +++ b/consensus/proto_array/src/proto_array_fork_choice.rs @@ -968,6 +968,7 @@ impl ProtoArrayForkChoice { &self, block_root: &Hash256, parent_payload_status: PayloadStatus, + current_slot: Slot, ) -> Result { let block_index = self .proto_array @@ -985,7 +986,7 @@ impl ProtoArrayForkChoice { payload_status: parent_payload_status, }; self.proto_array - .should_build_on_full::(&fc_node, proto_node) + .should_build_on_full::(&fc_node, proto_node, current_slot) .map_err(|e| format!("{e:?}")) } diff --git a/consensus/state_processing/src/per_block_processing/process_operations.rs b/consensus/state_processing/src/per_block_processing/process_operations.rs index f88a325d4e..876e66d3af 100644 --- a/consensus/state_processing/src/per_block_processing/process_operations.rs +++ b/consensus/state_processing/src/per_block_processing/process_operations.rs @@ -880,8 +880,11 @@ pub fn process_deposit_requests_pre_gloas( spec: &ChainSpec, ) -> Result<(), BlockProcessingError> { for request in deposit_requests { - // Set deposit receipt start index - if state.deposit_requests_start_index()? == spec.unset_deposit_requests_start_index { + // Set deposit receipt start index if pre-Fulu. + // Support for the former Eth1 bridge deposit mechanism was removed in Fulu. + if !state.fork_name_unchecked().fulu_enabled() + && state.deposit_requests_start_index()? == spec.unset_deposit_requests_start_index + { *state.deposit_requests_start_index_mut()? = request.index } let slot = state.slot(); diff --git a/consensus/state_processing/src/per_epoch_processing/single_pass.rs b/consensus/state_processing/src/per_epoch_processing/single_pass.rs index 881e6bb16c..43d6606f07 100644 --- a/consensus/state_processing/src/per_epoch_processing/single_pass.rs +++ b/consensus/state_processing/src/per_epoch_processing/single_pass.rs @@ -1105,8 +1105,10 @@ impl PendingDepositsContext { let pending_deposits = state.pending_deposits()?; for deposit in pending_deposits.iter() { - // Do not process deposit requests if the Eth1 bridge deposits are not yet applied. - if deposit.slot > spec.genesis_slot + // Do not process deposit requests if pre-Fulu and the Eth1 bridge deposits are not yet applied. + // Support for the former Eth1 bridge deposit mechanism was removed in Fulu. + if !state.fork_name_unchecked().fulu_enabled() + && deposit.slot > spec.genesis_slot && state.eth1_deposit_index() < state.deposit_requests_start_index()? { break; diff --git a/consensus/types/src/state/beacon_state.rs b/consensus/types/src/state/beacon_state.rs index 027acfab7f..26f28eda45 100644 --- a/consensus/types/src/state/beacon_state.rs +++ b/consensus/types/src/state/beacon_state.rs @@ -1127,21 +1127,33 @@ impl BeaconState { // Post-Fulu we must never compute proposer indices using insufficient lookahead. This // would be very dangerous as it would lead to conflicts between the *true* proposer as // defined by `self.proposer_lookahead` and the output of this function. - // With MIN_SEED_LOOKAHEAD=1 (common config), this is equivalent to checking that the - // requested epoch is not the current epoch. // - // We do not run this check if this function is called from `upgrade_to_fulu`, - // which runs *after* the slot is incremented, and needs to compute the proposer - // shuffling for the epoch that was just transitioned into. - if self.fork_name_unchecked().fulu_enabled() - && epoch < current_epoch.safe_add(spec.min_seed_lookahead)? - { - return Err( - BeaconStateError::ComputeProposerIndicesInsufficientLookahead { - current_epoch, - request_epoch: epoch, - }, - ); + // Furthermore, post-Gloas, we must never compute proposers at any slot other than the + // dependent root slot itself, as slashings at subsequent slots have the ability to + // change the shuffling. + // + // For simplicity these two checks are combined into a single check on the dependent + // slot, which is safe for Fulu and Gloas. This function is always called from + // `get_beacon_proposer_indices`, which uses the cached lookahead for `current_epoch` and + // `next_epoch`. The only epoch's shuffling that should ordinarily be computed therefore + // is `next_epoch + 1`, which for Fulu and Gloas is computed during the epoch transition + // in the last slot of `current_epoch` (before the slot is incremented into + // `next_epoch`). + // + // The only case where computation of proposers in `current_epoch` and `next_epoch` is + // directly required is during the fork to Fulu itself + // (`upgrade_to_fulu`/`initialize_proposer_lookahead`), in which case the state is not + // yet the Fulu variant, and we omit the check. + if self.fork_name_unchecked().fulu_enabled() { + let dependent_slot = spec.proposer_shuffling_decision_slot::(epoch); + if self.slot() != dependent_slot { + return Err( + BeaconStateError::ComputeProposerIndicesInsufficientLookahead { + current_epoch, + request_epoch: epoch, + }, + ); + } } } else { // Pre-Fulu the situation is reversed, we *should not* compute proposer indices using @@ -1375,7 +1387,7 @@ impl BeaconState { spec: &ChainSpec, ) -> Result, BeaconStateError> { // This isn't in the spec, but we remove the footgun that is requesting the current epoch - // for a Fulu state. + // or next epoch for a Fulu state. if let Ok(proposer_lookahead) = self.proposer_lookahead() && epoch >= self.current_epoch() && epoch <= self.next_epoch()? @@ -1394,7 +1406,15 @@ impl BeaconState { } // Not using the cached validator indices since they are shuffled. - let indices = self.get_active_validator_indices(epoch, spec)?; + let mut indices = self.get_active_validator_indices(epoch, spec)?; + + // Post-Gloas, slashed validators are excluded from proposer selection + if self.fork_name_unchecked().gloas_enabled() { + let latest_block_slot = self.latest_block_header().slot; + let slashings_cache = self.slashings_cache(); + slashings_cache.check_initialized(latest_block_slot)?; + indices.retain(|&index| !slashings_cache.is_slashed(index)); + } let preimage = self.get_seed(epoch, Domain::BeaconProposer, spec)?; self.compute_proposer_indices(epoch, preimage.as_slice(), &indices, spec) diff --git a/consensus/types/src/state/slashings_cache.rs b/consensus/types/src/state/slashings_cache.rs index b6ed583df8..cdaa2c6c89 100644 --- a/consensus/types/src/state/slashings_cache.rs +++ b/consensus/types/src/state/slashings_cache.rs @@ -64,3 +64,127 @@ impl SlashingsCache { self.latest_block_slot = Some(latest_block_slot); } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::{Epoch, Hash256}; + use bls::PublicKeyBytes; + + /// Build a minimal validator with the given `slashed` flag. The other fields are irrelevant to + /// the slashings cache. + fn validator(slashed: bool) -> Validator { + Validator { + pubkey: PublicKeyBytes::empty(), + withdrawal_credentials: Hash256::ZERO, + effective_balance: 0, + slashed, + activation_eligibility_epoch: Epoch::new(0), + activation_epoch: Epoch::new(0), + exit_epoch: Epoch::new(0), + withdrawable_epoch: Epoch::new(0), + } + } + + /// Validators 1 and 3 are slashed, the rest are not. + fn validators() -> Vec { + vec![ + validator(false), + validator(true), + validator(false), + validator(true), + validator(false), + ] + } + + #[test] + fn new_captures_slashed_indices() { + let validators = validators(); + let cache = SlashingsCache::new(Slot::new(7), validators.iter()); + + // The cache is initialized at the block slot it was built for. + assert!(cache.is_initialized(Slot::new(7))); + assert!(!cache.is_initialized(Slot::new(8))); + + // Each index reports the same `slashed` status as the source validator. + for (index, validator) in validators.iter().enumerate() { + assert_eq!( + cache.is_slashed(index), + validator.slashed, + "validator {index} slashed status mismatch" + ); + } + + // An out-of-bounds index is not slashed. + assert!(!cache.is_slashed(validators.len())); + } + + #[test] + fn default_is_uninitialized() { + let cache = SlashingsCache::default(); + + // A default cache is not initialized at any slot. + assert!(!cache.is_initialized(Slot::new(0))); + assert_eq!( + cache.check_initialized(Slot::new(0)), + Err(BeaconStateError::SlashingsCacheUninitialized { + initialized_slot: None, + latest_block_slot: Slot::new(0), + }) + ); + + // It reports nothing as slashed. This is exactly why callers must check initialization + // before trusting `is_slashed`. + assert!(!cache.is_slashed(0)); + } + + #[test] + fn check_initialized_matches_block_slot() { + let cache = SlashingsCache::new(Slot::new(3), validators().iter()); + + assert_eq!(cache.check_initialized(Slot::new(3)), Ok(())); + assert_eq!( + cache.check_initialized(Slot::new(4)), + Err(BeaconStateError::SlashingsCacheUninitialized { + initialized_slot: Some(Slot::new(3)), + latest_block_slot: Slot::new(4), + }) + ); + } + + #[test] + fn record_validator_slashing_requires_matching_slot() { + let mut cache = SlashingsCache::new(Slot::new(3), validators().iter()); + + // Index 0 starts unslashed. + assert!(!cache.is_slashed(0)); + + // Recording at the initialized slot succeeds and marks the validator slashed. + cache.record_validator_slashing(Slot::new(3), 0).unwrap(); + assert!(cache.is_slashed(0)); + + // Recording at a slot the cache is not initialized for errors and leaves the set unchanged. + assert_eq!( + cache.record_validator_slashing(Slot::new(4), 2), + Err(BeaconStateError::SlashingsCacheUninitialized { + initialized_slot: Some(Slot::new(3)), + latest_block_slot: Slot::new(4), + }) + ); + assert!(!cache.is_slashed(2)); + } + + #[test] + fn update_latest_block_slot_preserves_slashed_set() { + let mut cache = SlashingsCache::new(Slot::new(3), validators().iter()); + + cache.update_latest_block_slot(Slot::new(4)); + + // The initialized slot moves forward without clearing the recorded slashings. + assert!(!cache.is_initialized(Slot::new(3))); + assert!(cache.is_initialized(Slot::new(4))); + assert!(cache.is_slashed(1)); + assert!(cache.is_slashed(3)); + assert!(!cache.is_slashed(0)); + } +} diff --git a/testing/ef_tests/Makefile b/testing/ef_tests/Makefile index f566a89ded..a497fdaeff 100644 --- a/testing/ef_tests/Makefile +++ b/testing/ef_tests/Makefile @@ -1,6 +1,6 @@ # To download/extract nightly tests, run: # CONSENSUS_SPECS_TEST_VERSION=nightly make -CONSENSUS_SPECS_TEST_VERSION ?= v1.7.0-alpha.8 +CONSENSUS_SPECS_TEST_VERSION ?= v1.7.0-alpha.10 REPO_NAME := consensus-spec-tests OUTPUT_DIR := ./$(REPO_NAME) diff --git a/testing/ef_tests/check_all_files_accessed.py b/testing/ef_tests/check_all_files_accessed.py index 723c5e7e9e..c81f94d8e9 100755 --- a/testing/ef_tests/check_all_files_accessed.py +++ b/testing/ef_tests/check_all_files_accessed.py @@ -84,6 +84,8 @@ excluded_paths = [ "tests/.*/.*/networking/gossip_sync_committee_message/.*", "tests/.*/.*/networking/gossip_sync_committee_contribution_and_proof/.*", "tests/.*/.*/networking/gossip_blob_sidecar/.*", + "tests/.*/.*/networking/gossip_data_column_sidecar/.*", + "tests/.*/.*/networking/gossip_partial_data_column_sidecar/.*", # TODO: fast confirmation rule not merged yet "tests/.*/.*/fast_confirmation", ] diff --git a/testing/ef_tests/src/cases/epoch_processing.rs b/testing/ef_tests/src/cases/epoch_processing.rs index ec243f05cc..b89a72ca77 100644 --- a/testing/ef_tests/src/cases/epoch_processing.rs +++ b/testing/ef_tests/src/cases/epoch_processing.rs @@ -423,6 +423,9 @@ impl> Case for EpochProcessing { // Processing requires the committee caches. pre_state.build_all_committee_caches(spec).unwrap(); + // Proposer index computation (e.g. proposer lookahead) requires the slashings cache post-Gloas + pre_state.build_slashings_cache().unwrap(); + let mut state = pre_state.clone(); let mut expected = self.post.clone(); diff --git a/testing/ef_tests/src/cases/fork_choice.rs b/testing/ef_tests/src/cases/fork_choice.rs index cf122dbd65..9edc5b85c8 100644 --- a/testing/ef_tests/src/cases/fork_choice.rs +++ b/testing/ef_tests/src/cases/fork_choice.rs @@ -53,6 +53,9 @@ pub struct PowBlock { pub struct Head { slot: Slot, root: Hash256, + // Post-gloas, the head check also asserts the payload status of the head block + #[serde(default)] + payload_status: Option, } #[derive(Debug, Clone, Copy, PartialEq, Deserialize)] @@ -132,6 +135,10 @@ pub enum Step< }, Attestation { attestation: TAttestation, + // Post-Gloas `on_attestation` tests can assert that an attestation is rejected (e.g. an + // invalid payload-present index). Defaults to `true` for the pre-Gloas tests that omit it. + #[serde(default = "default_true")] + valid: bool, }, AttesterSlashing { attester_slashing: TAttesterSlashing, @@ -169,8 +176,12 @@ fn default_true() -> bool { #[derive(Debug, Clone, Deserialize)] #[serde(deny_unknown_fields)] pub struct Meta { - #[serde(rename(deserialize = "description"))] - _description: String, + #[serde(default, rename(deserialize = "description"))] + _description: Option, + // Some Gloas fork choice tests carry a `bls_setting` instead of a description. We accept and + // ignore it: the value is always `1` (BLS required), which matches our default behaviour. + #[serde(default, rename(deserialize = "bls_setting"))] + _bls_setting: Option, } #[derive(Debug)] @@ -240,17 +251,19 @@ impl LoadCase for ForkChoiceTest { valid, }) } - Step::Attestation { attestation } => { + Step::Attestation { attestation, valid } => { if fork_name.electra_enabled() { ssz_decode_file(&path.join(format!("{}.ssz_snappy", attestation))).map( |attestation| Step::Attestation { attestation: Attestation::Electra(attestation), + valid, }, ) } else { ssz_decode_file(&path.join(format!("{}.ssz_snappy", attestation))).map( |attestation| Step::Attestation { attestation: Attestation::Base(attestation), + valid, }, ) } @@ -389,7 +402,9 @@ impl Case for ForkChoiceTest { proofs.clone(), *valid, )?, - Step::Attestation { attestation } => tester.process_attestation(attestation)?, + Step::Attestation { attestation, valid } => { + tester.process_attestation(attestation, *valid)? + } Step::AttesterSlashing { attester_slashing } => { tester.process_attester_slashing(attester_slashing.to_ref()) } @@ -673,7 +688,7 @@ impl Tester { if success { for attestation in block.message().body().attestations() { let att = attestation.clone_as_attestation(); - let _ = self.process_attestation(&att); + let _ = self.process_attestation(&att, true); } for attester_slashing in block.message().body().attester_slashings() { self.process_attester_slashing(attester_slashing); @@ -786,7 +801,7 @@ impl Tester { if success { for attestation in block.message().body().attestations() { let att = attestation.clone_as_attestation(); - let _ = self.process_attestation(&att); + let _ = self.process_attestation(&att, true); } for attester_slashing in block.message().body().attester_slashings() { self.process_attester_slashing(attester_slashing); @@ -848,7 +863,6 @@ impl Tester { block_delay, &state, PayloadVerificationStatus::Irrelevant, - block.message().proposer_index(), &self.harness.chain.spec, ); @@ -863,22 +877,41 @@ impl Tester { Ok(()) } - pub fn process_attestation(&self, attestation: &Attestation) -> Result<(), Error> { - let (indexed_attestation, _) = obtain_indexed_attestation_and_committees_per_slot( + pub fn process_attestation( + &self, + attestation: &Attestation, + valid: bool, + ) -> Result<(), Error> { + // Post-Gloas `on_attestation` tests can assert that an attestation is rejected (e.g. an + // invalid same-slot/payload-present index). Treat any failure in either indexing or fork + // choice application as a rejection so it can be compared against the expected `valid` flag. + let result = obtain_indexed_attestation_and_committees_per_slot( &self.harness.chain, attestation.to_ref(), ) - .map_err(|e| Error::InternalError(format!("attestation indexing failed with {:?}", e)))?; - let verified_attestation: ManuallyVerifiedAttestation> = - ManuallyVerifiedAttestation { - attestation, - indexed_attestation, - }; + .map_err(|e| format!("attestation indexing failed with {:?}", e)) + .and_then(|(indexed_attestation, _)| { + let verified_attestation: ManuallyVerifiedAttestation> = + ManuallyVerifiedAttestation { + attestation, + indexed_attestation, + }; - self.harness - .chain - .apply_attestation_to_fork_choice(&verified_attestation) - .map_err(|e| Error::InternalError(format!("attestation import failed with {:?}", e))) + self.harness + .chain + .apply_attestation_to_fork_choice(&verified_attestation) + .map_err(|e| format!("attestation import failed with {:?}", e)) + }); + + if valid { + result.map_err(Error::InternalError) + } else if result.is_ok() { + Err(Error::DidntFail( + "attestation was valid but the test expects it to be rejected".to_string(), + )) + } else { + Ok(()) + } } pub fn process_attester_slashing(&self, attester_slashing: AttesterSlashingRef) { @@ -909,9 +942,17 @@ impl Tester { let chain_head = Head { slot: head.head_slot(), root: head.head_block_root(), + // Compared separately below so the slot/root equality is not affected. + payload_status: expected_head.payload_status, }; - check_equal("head", chain_head, expected_head) + check_equal("head", chain_head, expected_head)?; + + if let Some(expected_status) = expected_head.payload_status { + self.check_head_payload_status(expected_status)?; + } + + Ok(()) } pub fn check_time(&self, expected_time: u64) -> Result<(), Error> { diff --git a/testing/ef_tests/src/cases/operations.rs b/testing/ef_tests/src/cases/operations.rs index f5c999920d..d851427a95 100644 --- a/testing/ef_tests/src/cases/operations.rs +++ b/testing/ef_tests/src/cases/operations.rs @@ -204,7 +204,12 @@ impl Operation for Deposit { ssz_decode_file(path) } - fn is_enabled_for_fork(_: ForkName) -> bool { + fn is_enabled_for_fork(fork_name: ForkName) -> bool { + // The standalone `deposit` operation tests were removed in Fulu (deposits are processed + // via `deposit_request` from Electra onwards). + if fork_name.fulu_enabled() { + return false; + } // Some deposit tests require signature verification but are not marked as such. cfg!(not(feature = "fake_crypto")) } diff --git a/testing/ef_tests/src/handler.rs b/testing/ef_tests/src/handler.rs index df1ece49dd..b45ea3a230 100644 --- a/testing/ef_tests/src/handler.rs +++ b/testing/ef_tests/src/handler.rs @@ -708,13 +708,6 @@ impl Handler for ForkChoiceHandler { return false; } - // No FCU override tests prior to bellatrix, and removed in Gloas. - if self.handler_name == "should_override_forkchoice_update" - && (!fork_name.bellatrix_enabled() || fork_name.gloas_enabled()) - { - return false; - } - // Deposit tests exist only for Electra and later. if self.handler_name == "deposit_with_reorg" && !fork_name.electra_enabled() { return false; @@ -725,9 +718,10 @@ impl Handler for ForkChoiceHandler { return false; } - // on_execution_payload_envelope, get_parent_payload_status, and + // on_attestation, on_execution_payload_envelope, get_parent_payload_status, and // on_payload_attestation_message tests exist only for Gloas and later. - if (self.handler_name == "on_execution_payload_envelope" + if (self.handler_name == "on_attestation" + || self.handler_name == "on_execution_payload_envelope" || self.handler_name == "get_parent_payload_status" || self.handler_name == "on_payload_attestation_message") && !fork_name.gloas_enabled() diff --git a/testing/ef_tests/tests/tests.rs b/testing/ef_tests/tests/tests.rs index 6e1c4fdc10..9af88e0201 100644 --- a/testing/ef_tests/tests/tests.rs +++ b/testing/ef_tests/tests/tests.rs @@ -708,14 +708,18 @@ mod ssz_static { #[test] fn blob_sidecar() { - SszStaticHandler::, MinimalEthSpec>::deneb_and_later().run(); - SszStaticHandler::, MainnetEthSpec>::deneb_and_later().run(); + SszStaticHandler::, MinimalEthSpec>::deneb_only().run(); + SszStaticHandler::, MainnetEthSpec>::deneb_only().run(); + SszStaticHandler::, MinimalEthSpec>::electra_only().run(); + SszStaticHandler::, MainnetEthSpec>::electra_only().run(); } #[test] fn blob_identifier() { - SszStaticHandler::::deneb_and_later().run(); - SszStaticHandler::::deneb_and_later().run(); + SszStaticHandler::::deneb_only().run(); + SszStaticHandler::::deneb_only().run(); + SszStaticHandler::::electra_only().run(); + SszStaticHandler::::electra_only().run(); } #[test] @@ -1025,6 +1029,12 @@ fn fork_choice_get_head() { ForkChoiceHandler::::new("get_head").run(); } +#[test] +fn fork_choice_on_attestation() { + ForkChoiceHandler::::new("on_attestation").run(); + ForkChoiceHandler::::new("on_attestation").run(); +} + #[test] fn fork_choice_on_block() { ForkChoiceHandler::::new("on_block").run(); @@ -1049,12 +1059,6 @@ fn fork_choice_withholding() { // There is no mainnet variant for this test. } -#[test] -fn fork_choice_should_override_forkchoice_update() { - ForkChoiceHandler::::new("should_override_forkchoice_update").run(); - ForkChoiceHandler::::new("should_override_forkchoice_update").run(); -} - #[test] fn fork_choice_get_proposer_head() { ForkChoiceHandler::::new("get_proposer_head").run();