From bf4cbd3b0a11fdb810a9e0bc1c3c3ed0f8873689 Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Wed, 26 Jun 2024 16:53:53 -0700 Subject: [PATCH 01/23] Remove all batches related to a peer on disconnect (#5969) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Remove all batches related to a peer on disconnect * Cleanup map entries after disconnect * Allow lookups to continue in case of disconnections * Pretty response types * fmt * Fix lints * Remove lookup if it cannot progress * Fix tests * Remove poll_close on rpc behaviour * Remove redundant test * Fix issue raised by lion * Revert pretty response types * Cleanup * Fix test * Merge remote-tracking branch 'origin/release-v5.2.1' into rpc-error-on-disconnect-revert * Apply suggestions from joao Co-authored-by: João Oliveira * Fix log * update request status on no peers found * Do not remove lookup after peer disconnection * Add comments about expected event api * Update single_block_lookup.rs * Update mod.rs * Merge branch 'rpc-error-on-disconnect-revert' into 5969-review * Merge pull request #10 from dapplion/5969-review Add comments about expected event api --- .../lighthouse_network/src/rpc/handler.rs | 31 ------ .../lighthouse_network/tests/rpc_tests.rs | 94 +------------------ .../network/src/sync/backfill_sync/mod.rs | 38 +++++++- .../network/src/sync/block_lookups/mod.rs | 42 +++++---- .../sync/block_lookups/single_block_lookup.rs | 38 ++++++-- .../network/src/sync/block_lookups/tests.rs | 48 ++++++---- .../src/sync/block_sidecar_coupling.rs | 14 ++- beacon_node/network/src/sync/manager.rs | 27 +++++- .../network/src/sync/network_context.rs | 76 ++++++++++++++- .../src/sync/network_context/requests.rs | 13 ++- .../network/src/sync/range_sync/chain.rs | 26 ++++- .../network/src/sync/range_sync/range.rs | 5 +- 12 files changed, 270 insertions(+), 182 deletions(-) diff --git a/beacon_node/lighthouse_network/src/rpc/handler.rs b/beacon_node/lighthouse_network/src/rpc/handler.rs index b7166efc37..6f338ebc8b 100644 --- a/beacon_node/lighthouse_network/src/rpc/handler.rs +++ b/beacon_node/lighthouse_network/src/rpc/handler.rs @@ -352,37 +352,6 @@ where !matches!(self.state, HandlerState::Deactivated) } - // NOTE: This function gets polled to completion upon a connection close. - fn poll_close(&mut self, _: &mut Context<'_>) -> Poll> { - // Inform the network behaviour of any failed requests - - while let Some(substream_id) = self.outbound_substreams.keys().next().cloned() { - let outbound_info = self - .outbound_substreams - .remove(&substream_id) - .expect("The value must exist for a key"); - // If the state of the connection is closing, we do not need to report this case to - // the behaviour, as the connection has just closed non-gracefully - if matches!(outbound_info.state, OutboundSubstreamState::Closing(_)) { - continue; - } - - // Register this request as an RPC Error - return Poll::Ready(Some(HandlerEvent::Err(HandlerErr::Outbound { - error: RPCError::Disconnected, - proto: outbound_info.proto, - id: outbound_info.req_id, - }))); - } - - // Also handle any events that are awaiting to be sent to the behaviour - if !self.events_out.is_empty() { - return Poll::Ready(Some(self.events_out.remove(0))); - } - - Poll::Ready(None) - } - fn poll( &mut self, cx: &mut Context<'_>, diff --git a/beacon_node/lighthouse_network/tests/rpc_tests.rs b/beacon_node/lighthouse_network/tests/rpc_tests.rs index 8d29f5158b..527b853dc3 100644 --- a/beacon_node/lighthouse_network/tests/rpc_tests.rs +++ b/beacon_node/lighthouse_network/tests/rpc_tests.rs @@ -3,7 +3,7 @@ mod common; use common::Protocol; -use lighthouse_network::rpc::{methods::*, RPCError}; +use lighthouse_network::rpc::methods::*; use lighthouse_network::{rpc::max_rpc_size, NetworkEvent, ReportSource, Request, Response}; use slog::{debug, warn, Level}; use ssz::Encode; @@ -1012,98 +1012,6 @@ fn test_tcp_blocks_by_root_chunked_rpc_terminates_correctly() { }) } -#[test] -fn test_disconnect_triggers_rpc_error() { - // set up the logging. The level and enabled logging or not - let log_level = Level::Debug; - let enable_logging = false; - - let log = common::build_log(log_level, enable_logging); - let spec = E::default_spec(); - - let rt = Arc::new(Runtime::new().unwrap()); - // get sender/receiver - rt.block_on(async { - let (mut sender, mut receiver) = common::build_node_pair( - Arc::downgrade(&rt), - &log, - ForkName::Base, - &spec, - Protocol::Tcp, - ) - .await; - - // BlocksByRoot Request - let rpc_request = Request::BlocksByRoot(BlocksByRootRequest::new( - // Must have at least one root for the request to create a stream - vec![Hash256::from_low_u64_be(0)], - &spec, - )); - - // build the sender future - let sender_future = async { - loop { - match sender.next_event().await { - NetworkEvent::PeerConnectedOutgoing(peer_id) => { - // Send a STATUS message - debug!(log, "Sending RPC"); - sender - .send_request(peer_id, 42, rpc_request.clone()) - .unwrap(); - } - NetworkEvent::RPCFailed { error, id: 42, .. } => match error { - RPCError::Disconnected => return, - other => panic!("received unexpected error {:?}", other), - }, - other => { - warn!(log, "Ignoring other event {:?}", other); - } - } - } - }; - - // determine messages to send (PeerId, RequestId). If some, indicates we still need to send - // messages - let mut sending_peer = None; - let receiver_future = async { - loop { - // this future either drives the sending/receiving or times out allowing messages to be - // sent in the timeout - match futures::future::select( - Box::pin(receiver.next_event()), - Box::pin(tokio::time::sleep(Duration::from_secs(1))), - ) - .await - { - futures::future::Either::Left((ev, _)) => match ev { - NetworkEvent::RequestReceived { peer_id, .. } => { - sending_peer = Some(peer_id); - } - other => { - warn!(log, "Ignoring other event {:?}", other); - } - }, - futures::future::Either::Right((_, _)) => {} // The timeout hit, send messages if required - } - - // if we need to send messages send them here. This will happen after a delay - if let Some(peer_id) = sending_peer.take() { - warn!(log, "Receiver got request, disconnecting peer"); - receiver.__hard_disconnect_testing_only(peer_id); - } - } - }; - - tokio::select! { - _ = sender_future => {} - _ = receiver_future => {} - _ = sleep(Duration::from_secs(30)) => { - panic!("Future timed out"); - } - } - }) -} - /// Establishes a pair of nodes and disconnects the pair based on the selected protocol via an RPC /// Goodbye message. fn goodbye_test(log_level: Level, enable_logging: bool, protocol: Protocol) { diff --git a/beacon_node/network/src/sync/backfill_sync/mod.rs b/beacon_node/network/src/sync/backfill_sync/mod.rs index ce7d04ac0a..5431e1bcdc 100644 --- a/beacon_node/network/src/sync/backfill_sync/mod.rs +++ b/beacon_node/network/src/sync/backfill_sync/mod.rs @@ -307,7 +307,11 @@ impl BackFillSync { /// A peer has disconnected. /// If the peer has active batches, those are considered failed and re-requested. #[must_use = "A failure here indicates the backfill sync has failed and the global sync state should be updated"] - pub fn peer_disconnected(&mut self, peer_id: &PeerId) -> Result<(), BackFillError> { + pub fn peer_disconnected( + &mut self, + peer_id: &PeerId, + network: &mut SyncNetworkContext, + ) -> Result<(), BackFillError> { if matches!( self.state(), BackFillState::Failed | BackFillState::NotRequired @@ -315,7 +319,37 @@ impl BackFillSync { return Ok(()); } - self.active_requests.remove(peer_id); + if let Some(batch_ids) = self.active_requests.remove(peer_id) { + // fail the batches. + for id in batch_ids { + if let Some(batch) = self.batches.get_mut(&id) { + match batch.download_failed(false) { + Ok(BatchOperationOutcome::Failed { blacklist: _ }) => { + self.fail_sync(BackFillError::BatchDownloadFailed(id))?; + } + Ok(BatchOperationOutcome::Continue) => {} + Err(e) => { + self.fail_sync(BackFillError::BatchInvalidState(id, e.0))?; + } + } + // If we have run out of peers in which to retry this batch, the backfill state + // transitions to a paused state. + // We still need to reset the state for all the affected batches, so we should not + // short circuit early. + if self.retry_batch_download(network, id).is_err() { + debug!( + self.log, + "Batch could not be retried"; + "batch_id" => id, + "error" => "no synced peers" + ); + } + } else { + debug!(self.log, "Batch not found while removing peer"; + "peer" => %peer_id, "batch" => id) + } + } + } // Remove the peer from the participation list self.participating_peers.remove(peer_id); diff --git a/beacon_node/network/src/sync/block_lookups/mod.rs b/beacon_node/network/src/sync/block_lookups/mod.rs index f685b7e59d..0148a6548d 100644 --- a/beacon_node/network/src/sync/block_lookups/mod.rs +++ b/beacon_node/network/src/sync/block_lookups/mod.rs @@ -1,3 +1,25 @@ +//! Implements block lookup sync. +//! +//! Block lookup sync is triggered when a peer claims to have imported a block we don't know about. +//! For example, a peer attesting to a head block root that is not in our fork-choice. Lookup sync +//! is recursive in nature, as we may discover that this attested head block root has a parent that +//! is also unknown to us. +//! +//! Block lookup is implemented as an event-driven state machine. It sends events to the network and +//! beacon processor, and expects some set of events back. A discrepancy in the expected event API +//! will result in lookups getting "stuck". A lookup becomes stuck when there is no future event +//! that will trigger the lookup to make progress. There's a fallback mechanism that drops lookups +//! that live for too long, logging the line "Notify the devs a sync lookup is stuck". +//! +//! The expected event API is documented in the code paths that are making assumptions with the +//! comment prefix "Lookup sync event safety:" +//! +//! Block lookup sync attempts to not re-download or re-process data that we already have. Block +//! components are cached temporarily in multiple places before they are imported into fork-choice. +//! Therefore, block lookup sync must peek these caches correctly to decide when to skip a download +//! or consider a lookup complete. These caches are read from the `SyncNetworkContext` and its state +//! returned to this module as `LookupRequestResult` variants. + use self::parent_chain::{compute_parent_chains, NodeChain}; pub use self::single_block_lookup::DownloadResult; use self::single_block_lookup::{LookupRequestError, LookupResult, SingleBlockLookup}; @@ -410,21 +432,9 @@ impl BlockLookups { /* Error responses */ pub fn peer_disconnected(&mut self, peer_id: &PeerId) { - self.single_block_lookups.retain(|_, lookup| { + for (_, lookup) in self.single_block_lookups.iter_mut() { lookup.remove_peer(peer_id); - - // Note: this condition should be removed in the future. It's not strictly necessary to drop a - // lookup if there are no peers left. Lookup should only be dropped if it can not make progress - if lookup.has_no_peers() { - debug!(self.log, - "Dropping single lookup after peer disconnection"; - "block_root" => ?lookup.block_root() - ); - false - } else { - true - } - }); + } } /* Processing responses */ @@ -787,12 +797,12 @@ impl BlockLookups { }; if stuck_lookup.id == ancestor_stuck_lookup.id { - warn!(self.log, "Notify the devs, a sync lookup is stuck"; + warn!(self.log, "Notify the devs a sync lookup is stuck"; "block_root" => ?stuck_lookup.block_root(), "lookup" => ?stuck_lookup, ); } else { - warn!(self.log, "Notify the devs, a sync lookup is stuck"; + warn!(self.log, "Notify the devs a sync lookup is stuck"; "block_root" => ?stuck_lookup.block_root(), "lookup" => ?stuck_lookup, "ancestor_block_root" => ?ancestor_stuck_lookup.block_root(), diff --git a/beacon_node/network/src/sync/block_lookups/single_block_lookup.rs b/beacon_node/network/src/sync/block_lookups/single_block_lookup.rs index 13efd36ab7..e17991286a 100644 --- a/beacon_node/network/src/sync/block_lookups/single_block_lookup.rs +++ b/beacon_node/network/src/sync/block_lookups/single_block_lookup.rs @@ -197,21 +197,36 @@ impl SingleBlockLookup { } let Some(peer_id) = self.use_rand_available_peer() else { - // Allow lookup to not have any peers. In that case do nothing. If the lookup does - // not have peers for some time, it will be dropped. + // Allow lookup to not have any peers and do nothing. This is an optimization to not + // lose progress of lookups created from a block with unknown parent before we receive + // attestations for said block. + // Lookup sync event safety: If a lookup requires peers to make progress, and does + // not receive any new peers for some time it will be dropped. If it receives a new + // peer it must attempt to make progress. + R::request_state_mut(self) + .get_state_mut() + .update_awaiting_download_status("no peers"); return Ok(()); }; let request = R::request_state_mut(self); match request.make_request(id, peer_id, downloaded_block_expected_blobs, cx)? { LookupRequestResult::RequestSent(req_id) => { + // Lookup sync event safety: If make_request returns `RequestSent`, we are + // guaranteed that `BlockLookups::on_download_response` will be called exactly + // with this `req_id`. request.get_state_mut().on_download_start(req_id)? } LookupRequestResult::NoRequestNeeded => { + // Lookup sync event safety: Advances this request to the terminal `Processed` + // state. If all requests reach this state, the request is marked as completed + // in `Self::continue_requests`. request.get_state_mut().on_completed_request()? } // Sync will receive a future event to make progress on the request, do nothing now LookupRequestResult::Pending(reason) => { + // Lookup sync event safety: Refer to the code paths constructing + // `LookupRequestResult::Pending` request .get_state_mut() .update_awaiting_download_status(reason); @@ -222,16 +237,28 @@ impl SingleBlockLookup { // Otherwise, attempt to progress awaiting processing // If this request is awaiting a parent lookup to be processed, do not send for processing. // The request will be rejected with unknown parent error. + // + // TODO: The condition `block_is_processed || Block` can be dropped after checking for + // unknown parent root when import RPC blobs } else if !awaiting_parent && (block_is_processed || matches!(R::response_type(), ResponseType::Block)) { // maybe_start_processing returns Some if state == AwaitingProcess. This pattern is // useful to conditionally access the result data. if let Some(result) = request.get_state_mut().maybe_start_processing() { + // Lookup sync event safety: If `send_for_processing` returns Ok() we are guaranteed + // that `BlockLookups::on_processing_result` will be called exactly once with this + // lookup_id return R::send_for_processing(id, result, cx); } + // Lookup sync event safety: If the request is not in `AwaitingDownload` or + // `AwaitingProcessing` state it is guaranteed to receive some event to make progress. } + // Lookup sync event safety: If a lookup is awaiting a parent we are guaranteed to either: + // (1) attempt to make progress with `BlockLookups::continue_child_lookups` if the parent + // lookup completes, or (2) get dropped if the parent fails and is dropped. + Ok(()) } @@ -246,10 +273,9 @@ impl SingleBlockLookup { self.peers.insert(peer_id) } - /// Remove peer from available peers. Return true if there are no more available peers and all - /// requests are not expecting any future event (AwaitingDownload). - pub fn remove_peer(&mut self, peer_id: &PeerId) -> bool { - self.peers.remove(peer_id) + /// Remove peer from available peers. + pub fn remove_peer(&mut self, peer_id: &PeerId) { + self.peers.remove(peer_id); } /// Returns true if this lookup has zero peers diff --git a/beacon_node/network/src/sync/block_lookups/tests.rs b/beacon_node/network/src/sync/block_lookups/tests.rs index a607151bde..02b07fa43e 100644 --- a/beacon_node/network/src/sync/block_lookups/tests.rs +++ b/beacon_node/network/src/sync/block_lookups/tests.rs @@ -290,6 +290,7 @@ impl TestRig { .0 } + #[track_caller] fn expect_no_active_single_lookups(&self) { assert!( self.active_single_lookups().is_empty(), @@ -298,6 +299,7 @@ impl TestRig { ); } + #[track_caller] fn expect_no_active_lookups(&self) { self.expect_no_active_single_lookups(); } @@ -539,10 +541,6 @@ impl TestRig { }) } - fn peer_disconnected(&mut self, disconnected_peer_id: PeerId) { - self.send_sync_message(SyncMessage::Disconnect(disconnected_peer_id)); - } - /// Return RPCErrors for all active requests of peer fn rpc_error_all_active_requests(&mut self, disconnected_peer_id: PeerId) { self.drain_network_rx(); @@ -562,6 +560,10 @@ impl TestRig { } } + fn peer_disconnected(&mut self, peer_id: PeerId) { + self.send_sync_message(SyncMessage::Disconnect(peer_id)); + } + fn drain_network_rx(&mut self) { while let Ok(event) = self.network_rx.try_recv() { self.network_rx_queue.push(event); @@ -1026,6 +1028,28 @@ fn test_single_block_lookup_failure() { rig.expect_empty_network(); } +#[test] +fn test_single_block_lookup_peer_disconnected_then_rpc_error() { + let mut rig = TestRig::test_setup(); + + let block_hash = Hash256::random(); + let peer_id = rig.new_connected_peer(); + + // Trigger the request. + rig.trigger_unknown_block_from_attestation(block_hash, peer_id); + let id = rig.expect_block_lookup_request(block_hash); + + // The peer disconnect event reaches sync before the rpc error. + rig.peer_disconnected(peer_id); + // The lookup is not removed as it can still potentially make progress. + rig.assert_single_lookups_count(1); + // The request fails. + rig.single_lookup_failed(id, peer_id, RPCError::Disconnected); + rig.expect_block_lookup_request(block_hash); + // The request should be removed from the network context on disconnection. + rig.expect_empty_network(); +} + #[test] fn test_single_block_lookup_becomes_parent_request() { let mut rig = TestRig::test_setup(); @@ -1289,19 +1313,9 @@ fn test_lookup_peer_disconnected_no_peers_left_while_request() { rig.trigger_unknown_parent_block(peer_id, trigger_block.into()); rig.peer_disconnected(peer_id); rig.rpc_error_all_active_requests(peer_id); - rig.expect_no_active_lookups(); -} - -#[test] -fn test_lookup_peer_disconnected_no_peers_left_not_while_request() { - let mut rig = TestRig::test_setup(); - let peer_id = rig.new_connected_peer(); - let trigger_block = rig.rand_block(); - rig.trigger_unknown_parent_block(peer_id, trigger_block.into()); - rig.peer_disconnected(peer_id); - // Note: this test case may be removed in the future. It's not strictly necessary to drop a - // lookup if there are no peers left. Lookup should only be dropped if it can not make progress - rig.expect_no_active_lookups(); + // Erroring all rpc requests and disconnecting the peer shouldn't remove the requests + // from the lookups map as they can still progress. + rig.assert_single_lookups_count(2); } #[test] diff --git a/beacon_node/network/src/sync/block_sidecar_coupling.rs b/beacon_node/network/src/sync/block_sidecar_coupling.rs index d159733cbc..f31f2921ea 100644 --- a/beacon_node/network/src/sync/block_sidecar_coupling.rs +++ b/beacon_node/network/src/sync/block_sidecar_coupling.rs @@ -1,4 +1,5 @@ use beacon_chain::block_verification_types::RpcBlock; +use lighthouse_network::PeerId; use ssz_types::VariableList; use std::{collections::VecDeque, sync::Arc}; use types::{BlobSidecar, EthSpec, SignedBeaconBlock}; @@ -17,16 +18,19 @@ pub struct BlocksAndBlobsRequestInfo { is_sidecars_stream_terminated: bool, /// Used to determine if this accumulator should wait for a sidecars stream termination request_type: ByRangeRequestType, + /// The peer the request was made to. + pub(crate) peer_id: PeerId, } impl BlocksAndBlobsRequestInfo { - pub fn new(request_type: ByRangeRequestType) -> Self { + pub fn new(request_type: ByRangeRequestType, peer_id: PeerId) -> Self { Self { accumulated_blocks: <_>::default(), accumulated_sidecars: <_>::default(), is_blocks_stream_terminated: <_>::default(), is_sidecars_stream_terminated: <_>::default(), request_type, + peer_id, } } @@ -109,12 +113,14 @@ mod tests { use super::BlocksAndBlobsRequestInfo; use crate::sync::range_sync::ByRangeRequestType; use beacon_chain::test_utils::{generate_rand_block_and_blobs, NumBlobs}; + use lighthouse_network::PeerId; use rand::SeedableRng; use types::{test_utils::XorShiftRng, ForkName, MinimalEthSpec as E}; #[test] fn no_blobs_into_responses() { - let mut info = BlocksAndBlobsRequestInfo::::new(ByRangeRequestType::Blocks); + let peer_id = PeerId::random(); + let mut info = BlocksAndBlobsRequestInfo::::new(ByRangeRequestType::Blocks, peer_id); let mut rng = XorShiftRng::from_seed([42; 16]); let blocks = (0..4) .map(|_| generate_rand_block_and_blobs::(ForkName::Base, NumBlobs::None, &mut rng).0) @@ -133,7 +139,9 @@ mod tests { #[test] fn empty_blobs_into_responses() { - let mut info = BlocksAndBlobsRequestInfo::::new(ByRangeRequestType::BlocksAndBlobs); + let peer_id = PeerId::random(); + let mut info = + BlocksAndBlobsRequestInfo::::new(ByRangeRequestType::BlocksAndBlobs, peer_id); let mut rng = XorShiftRng::from_seed([42; 16]); let blocks = (0..4) .map(|_| { diff --git a/beacon_node/network/src/sync/manager.rs b/beacon_node/network/src/sync/manager.rs index 4c1a1e6b67..0f8cab18c9 100644 --- a/beacon_node/network/src/sync/manager.rs +++ b/beacon_node/network/src/sync/manager.rs @@ -372,16 +372,39 @@ impl SyncManager { Err(_) => self.update_sync_state(), }, } + } else { + debug!( + self.log, + "RPC error for range request has no associated entry in network context, ungraceful disconnect"; + "peer_id" => %peer_id, + "request_id" => %id, + "error" => ?error, + ); } } } } + /// Handles a peer disconnect. + /// + /// It is important that a peer disconnect retries all the batches/lookups as + /// there is no way to guarantee that libp2p always emits a error along with + /// the disconnect. fn peer_disconnect(&mut self, peer_id: &PeerId) { + // Inject a Disconnected error on all requests associated with the disconnected peer + // to retry all batches/lookups + for request_id in self.network.peer_disconnected(peer_id) { + self.inject_error(*peer_id, request_id, RPCError::Disconnected); + } + + // Remove peer from all data structures self.range_sync.peer_disconnect(&mut self.network, peer_id); + let _ = self + .backfill_sync + .peer_disconnected(peer_id, &mut self.network); self.block_lookups.peer_disconnected(peer_id); + // Regardless of the outcome, we update the sync status. - let _ = self.backfill_sync.peer_disconnected(peer_id); self.update_sync_state(); } @@ -951,7 +974,7 @@ impl SyncManager { self.network.insert_range_blocks_and_blobs_request( id, resp.sender_id, - BlocksAndBlobsRequestInfo::new(resp.request_type), + BlocksAndBlobsRequestInfo::new(resp.request_type, peer_id), ); // inform range that the request needs to be treated as failed // With time we will want to downgrade this log diff --git a/beacon_node/network/src/sync/network_context.rs b/beacon_node/network/src/sync/network_context.rs index f3f82ee011..6f89b954b3 100644 --- a/beacon_node/network/src/sync/network_context.rs +++ b/beacon_node/network/src/sync/network_context.rs @@ -177,6 +177,46 @@ impl SyncNetworkContext { } } + /// Returns the ids of all the requests made to the given peer_id. + pub fn peer_disconnected(&mut self, peer_id: &PeerId) -> Vec { + let failed_range_ids = + self.range_blocks_and_blobs_requests + .iter() + .filter_map(|(id, request)| { + if request.1.peer_id == *peer_id { + Some(SyncRequestId::RangeBlockAndBlobs { id: *id }) + } else { + None + } + }); + + let failed_block_ids = self + .blocks_by_root_requests + .iter() + .filter_map(|(id, request)| { + if request.peer_id == *peer_id { + Some(SyncRequestId::SingleBlock { id: *id }) + } else { + None + } + }); + let failed_blob_ids = self + .blobs_by_root_requests + .iter() + .filter_map(|(id, request)| { + if request.peer_id == *peer_id { + Some(SyncRequestId::SingleBlob { id: *id }) + } else { + None + } + }); + + failed_range_ids + .chain(failed_block_ids) + .chain(failed_blob_ids) + .collect() + } + pub fn network_globals(&self) -> &NetworkGlobals { &self.network_beacon_processor.network_globals } @@ -272,8 +312,13 @@ impl SyncNetworkContext { sender_id: RangeRequestId, ) -> Result { let id = self.blocks_by_range_request(peer_id, batch_type, request)?; - self.range_blocks_and_blobs_requests - .insert(id, (sender_id, BlocksAndBlobsRequestInfo::new(batch_type))); + self.range_blocks_and_blobs_requests.insert( + id, + ( + sender_id, + BlocksAndBlobsRequestInfo::new(batch_type, peer_id), + ), + ); Ok(id) } @@ -343,7 +388,10 @@ impl SyncNetworkContext { // Block is known are currently processing, expect a future event with the result of // processing. BlockProcessStatus::NotValidated { .. } => { - return Ok(LookupRequestResult::Pending("block in processing cache")) + // Lookup sync event safety: If the block is currently in the processing cache, we + // are guaranteed to receive a `SyncMessage::GossipBlockProcessResult` that will + // make progress on this lookup + return Ok(LookupRequestResult::Pending("block in processing cache")); } // Block is fully validated. If it's not yet imported it's waiting for missing block // components. Consider this request completed and do nothing. @@ -366,6 +414,12 @@ impl SyncNetworkContext { let request = BlocksByRootSingleRequest(block_root); + // Lookup sync event safety: If network_send.send() returns Ok(_) we are guaranteed that + // eventually at least one this 3 events will be received: + // - StreamTermination(request_id): handled by `Self::on_single_block_response` + // - RPCError(request_id): handled by `Self::on_single_block_response` + // - Disconnect(peer_id) handled by `Self::peer_disconnected``which converts it to a + // ` RPCError(request_id)`event handled by the above method self.network_send .send(NetworkMessage::SendRequest { peer_id, @@ -375,7 +429,7 @@ impl SyncNetworkContext { .map_err(|_| RpcRequestSendError::NetworkSendError)?; self.blocks_by_root_requests - .insert(id, ActiveBlocksByRootRequest::new(request)); + .insert(id, ActiveBlocksByRootRequest::new(request, peer_id)); Ok(LookupRequestResult::RequestSent(req_id)) } @@ -408,6 +462,13 @@ impl SyncNetworkContext { // latter handle the case where if the peer sent no blobs, penalize. // - if `downloaded_block_expected_blobs` is Some = block is downloading or processing. // - if `num_expected_blobs` returns Some = block is processed. + // + // Lookup sync event safety: Reaching this code means that a block is not in any pre-import + // cache nor in the request state of this lookup. Therefore, the block must either: (1) not + // be downloaded yet or (2) the block is already imported into the fork-choice. + // In case (1) the lookup must either successfully download the block or get dropped. + // In case (2) the block will be downloaded, processed, reach `BlockIsAlreadyKnown` and + // get dropped as completed. return Ok(LookupRequestResult::Pending("waiting for block download")); }; @@ -444,6 +505,7 @@ impl SyncNetworkContext { indices, }; + // Lookup sync event safety: Refer to `Self::block_lookup_request` `network_send.send` call self.network_send .send(NetworkMessage::SendRequest { peer_id, @@ -453,7 +515,7 @@ impl SyncNetworkContext { .map_err(|_| RpcRequestSendError::NetworkSendError)?; self.blobs_by_root_requests - .insert(id, ActiveBlobsByRootRequest::new(request)); + .insert(id, ActiveBlobsByRootRequest::new(request, peer_id)); Ok(LookupRequestResult::RequestSent(req_id)) } @@ -660,6 +722,8 @@ impl SyncNetworkContext { .ok_or(SendErrorProcessor::ProcessorNotAvailable)?; debug!(self.log, "Sending block for processing"; "block" => ?block_root, "id" => id); + // Lookup sync event safety: If `beacon_processor.send_rpc_beacon_block` returns Ok() sync + // must receive a single `SyncMessage::BlockComponentProcessed` with this process type beacon_processor .send_rpc_beacon_block( block_root, @@ -689,6 +753,8 @@ impl SyncNetworkContext { .ok_or(SendErrorProcessor::ProcessorNotAvailable)?; debug!(self.log, "Sending blobs for processing"; "block" => ?block_root, "id" => id); + // Lookup sync event safety: If `beacon_processor.send_rpc_blobs` returns Ok() sync + // must receive a single `SyncMessage::BlockComponentProcessed` event with this process type beacon_processor .send_rpc_blobs( block_root, diff --git a/beacon_node/network/src/sync/network_context/requests.rs b/beacon_node/network/src/sync/network_context/requests.rs index 6e4683701b..8387e9b0e1 100644 --- a/beacon_node/network/src/sync/network_context/requests.rs +++ b/beacon_node/network/src/sync/network_context/requests.rs @@ -1,5 +1,8 @@ use beacon_chain::get_block_root; -use lighthouse_network::rpc::{methods::BlobsByRootRequest, BlocksByRootRequest}; +use lighthouse_network::{ + rpc::{methods::BlobsByRootRequest, BlocksByRootRequest}, + PeerId, +}; use std::sync::Arc; use strum::IntoStaticStr; use types::{ @@ -20,13 +23,15 @@ pub enum LookupVerifyError { pub struct ActiveBlocksByRootRequest { request: BlocksByRootSingleRequest, resolved: bool, + pub(crate) peer_id: PeerId, } impl ActiveBlocksByRootRequest { - pub fn new(request: BlocksByRootSingleRequest) -> Self { + pub fn new(request: BlocksByRootSingleRequest, peer_id: PeerId) -> Self { Self { request, resolved: false, + peer_id, } } @@ -94,14 +99,16 @@ pub struct ActiveBlobsByRootRequest { request: BlobsByRootSingleBlockRequest, blobs: Vec>>, resolved: bool, + pub(crate) peer_id: PeerId, } impl ActiveBlobsByRootRequest { - pub fn new(request: BlobsByRootSingleBlockRequest) -> Self { + pub fn new(request: BlobsByRootSingleBlockRequest, peer_id: PeerId) -> Self { Self { request, blobs: vec![], resolved: false, + peer_id, } } diff --git a/beacon_node/network/src/sync/range_sync/chain.rs b/beacon_node/network/src/sync/range_sync/chain.rs index 63cafa9aca..122e8287e6 100644 --- a/beacon_node/network/src/sync/range_sync/chain.rs +++ b/beacon_node/network/src/sync/range_sync/chain.rs @@ -174,8 +174,30 @@ impl SyncingChain { /// Removes a peer from the chain. /// If the peer has active batches, those are considered failed and re-requested. - pub fn remove_peer(&mut self, peer_id: &PeerId) -> ProcessingResult { - self.peers.remove(peer_id); + pub fn remove_peer( + &mut self, + peer_id: &PeerId, + network: &mut SyncNetworkContext, + ) -> ProcessingResult { + if let Some(batch_ids) = self.peers.remove(peer_id) { + // fail the batches. + for id in batch_ids { + if let Some(batch) = self.batches.get_mut(&id) { + if let BatchOperationOutcome::Failed { blacklist } = + batch.download_failed(true)? + { + return Err(RemoveChain::ChainFailed { + blacklist, + failing_batch: id, + }); + } + self.retry_batch_download(network, id)?; + } else { + debug!(self.log, "Batch not found while removing peer"; + "peer" => %peer_id, "batch" => id) + } + } + } if self.peers.is_empty() { Err(RemoveChain::EmptyPeerPool) diff --git a/beacon_node/network/src/sync/range_sync/range.rs b/beacon_node/network/src/sync/range_sync/range.rs index fe48db35b4..c8e8266684 100644 --- a/beacon_node/network/src/sync/range_sync/range.rs +++ b/beacon_node/network/src/sync/range_sync/range.rs @@ -278,8 +278,9 @@ where /// for this peer. If so we mark the batch as failed. The batch may then hit it's maximum /// retries. In this case, we need to remove the chain. fn remove_peer(&mut self, network: &mut SyncNetworkContext, peer_id: &PeerId) { - for (removed_chain, sync_type, remove_reason) in - self.chains.call_all(|chain| chain.remove_peer(peer_id)) + for (removed_chain, sync_type, remove_reason) in self + .chains + .call_all(|chain| chain.remove_peer(peer_id, network)) { self.on_chain_removed( removed_chain, From b38019cb1071e5b5a3a3c1b4ec95836842b7ff64 Mon Sep 17 00:00:00 2001 From: Lion - dapplion <35266934+dapplion@users.noreply.github.com> Date: Thu, 27 Jun 2024 01:53:55 +0200 Subject: [PATCH 02/23] Attempt to continue lookups after adding peers (#5993) * Attempt to continue lookups after adding peers --- .../network/src/sync/block_lookups/mod.rs | 22 ++++++++++++------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/beacon_node/network/src/sync/block_lookups/mod.rs b/beacon_node/network/src/sync/block_lookups/mod.rs index 0148a6548d..0ae9bfec52 100644 --- a/beacon_node/network/src/sync/block_lookups/mod.rs +++ b/beacon_node/network/src/sync/block_lookups/mod.rs @@ -291,7 +291,7 @@ impl BlockLookups { } } - if let Err(e) = self.add_peers_to_lookup_and_ancestors(lookup_id, peers) { + if let Err(e) = self.add_peers_to_lookup_and_ancestors(lookup_id, peers, cx) { warn!(self.log, "Error adding peers to ancestor lookup"; "error" => ?e); } @@ -844,14 +844,17 @@ impl BlockLookups { &mut self, lookup_id: SingleLookupId, peers: &[PeerId], + cx: &mut SyncNetworkContext, ) -> Result<(), String> { let lookup = self .single_block_lookups .get_mut(&lookup_id) .ok_or(format!("Unknown lookup for id {lookup_id}"))?; + let mut added_some_peer = false; for peer in peers { if lookup.add_peer(*peer) { + added_some_peer = true; debug!(self.log, "Adding peer to existing single block lookup"; "block_root" => ?lookup.block_root(), "peer" => ?peer @@ -859,22 +862,25 @@ impl BlockLookups { } } - // We may choose to attempt to continue a lookup here. It is possible that a lookup had zero - // peers and after adding this set of peers it can make progress again. Note that this - // recursive function iterates from child to parent, so continuing the child first is weird. - // However, we choose to not attempt to continue the lookup for simplicity. It's not - // strictly required and just and optimization for a rare corner case. - if let Some(parent_root) = lookup.awaiting_parent() { if let Some((&child_id, _)) = self .single_block_lookups .iter() .find(|(_, l)| l.block_root() == parent_root) { - self.add_peers_to_lookup_and_ancestors(child_id, peers) + self.add_peers_to_lookup_and_ancestors(child_id, peers, cx) } else { Err(format!("Lookup references unknown parent {parent_root:?}")) } + } else if added_some_peer { + // If this lookup is not awaiting a parent and we added at least one peer, attempt to + // make progress. It is possible that a lookup is created with zero peers, attempted to + // make progress, and then receives peers. After that time the lookup will never be + // pruned with `drop_lookups_without_peers` because it has peers. This is rare corner + // case, but it can result in stuck lookups. + let result = lookup.continue_requests(cx); + self.on_lookup_result(lookup_id, result, "add_peers", cx); + Ok(()) } else { Ok(()) } From a910a498a81b76fe4ed171728c52a6425a0ae48a Mon Sep 17 00:00:00 2001 From: Jimmy Chen Date: Thu, 27 Jun 2024 17:11:22 +1000 Subject: [PATCH 03/23] Enable jemalloc by default on non windows targets (#5995) * Enable jemalloc by default on non windows target. * Update `allocator_name` function to check for `target_os` instead as we've deprecated `jemalloc` feature. --- Makefile | 8 -------- lighthouse/Cargo.toml | 10 ++++++++-- lighthouse/src/main.rs | 6 +++--- 3 files changed, 11 insertions(+), 13 deletions(-) diff --git a/Makefile b/Makefile index c3c8d13238..3f8e688df1 100644 --- a/Makefile +++ b/Makefile @@ -14,14 +14,6 @@ BUILD_PATH_AARCH64 = "target/$(AARCH64_TAG)/release" PINNED_NIGHTLY ?= nightly CLIPPY_PINNED_NIGHTLY=nightly-2022-05-19 -# List of features to use when building natively. Can be overridden via the environment. -# No jemalloc on Windows -ifeq ($(OS),Windows_NT) - FEATURES?= -else - FEATURES?=jemalloc -endif - # List of features to use when cross-compiling. Can be overridden via the environment. CROSS_FEATURES ?= gnosis,slasher-lmdb,slasher-mdbx,jemalloc diff --git a/lighthouse/Cargo.toml b/lighthouse/Cargo.toml index 20466b5de7..64b08b113e 100644 --- a/lighthouse/Cargo.toml +++ b/lighthouse/Cargo.toml @@ -22,8 +22,14 @@ gnosis = [] slasher-mdbx = ["slasher/mdbx"] # Support slasher LMDB backend. slasher-lmdb = ["slasher/lmdb"] -# Use jemalloc. -jemalloc = ["malloc_utils/jemalloc"] +# Deprecated. This is now enabled by default on non windows targets. +jemalloc = [] + +[target.'cfg(not(target_os = "windows"))'.dependencies] +malloc_utils = { workspace = true, features = ["jemalloc"] } + +[target.'cfg(target_os = "windows")'.dependencies] +malloc_utils = { workspace = true } [dependencies] beacon_node = { workspace = true } diff --git a/lighthouse/src/main.rs b/lighthouse/src/main.rs index abee30737c..47b44d3828 100644 --- a/lighthouse/src/main.rs +++ b/lighthouse/src/main.rs @@ -51,10 +51,10 @@ fn bls_library_name() -> &'static str { } fn allocator_name() -> &'static str { - if cfg!(feature = "jemalloc") { - "jemalloc" - } else { + if cfg!(target_os = "windows") { "system" + } else { + "jemalloc" } } From f106533ebc28707f71a22fef1c98724f911bfd71 Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Thu, 27 Jun 2024 00:35:11 -0700 Subject: [PATCH 04/23] Avoid rayon in lighthouse block verification (#5992) * Avoid rayon in lighthouse --- .../per_block_processing/block_signature_verifier.rs | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/consensus/state_processing/src/per_block_processing/block_signature_verifier.rs b/consensus/state_processing/src/per_block_processing/block_signature_verifier.rs index a0c044219d..28ca8935e4 100644 --- a/consensus/state_processing/src/per_block_processing/block_signature_verifier.rs +++ b/consensus/state_processing/src/per_block_processing/block_signature_verifier.rs @@ -4,7 +4,6 @@ use super::signature_sets::{Error as SignatureSetError, *}; use crate::per_block_processing::errors::{AttestationInvalid, BlockOperationError}; use crate::{ConsensusContext, ContextError}; use bls::{verify_signature_sets, PublicKey, PublicKeyBytes, SignatureSet}; -use rayon::prelude::*; use std::borrow::Cow; use types::{ AbstractExecPayload, BeaconState, BeaconStateError, ChainSpec, EthSpec, Hash256, @@ -411,15 +410,10 @@ impl<'a> ParallelSignatureSets<'a> { /// It is not possible to know exactly _which_ signature is invalid here, just that /// _at least one_ was invalid. /// - /// Uses `rayon` to do a map-reduce of Vitalik's method across multiple cores. + /// Blst library spreads the signature verification work across multiple available cores, so + /// this function is already parallelized. #[must_use] pub fn verify(self) -> bool { - let num_sets = self.sets.len(); - let num_chunks = std::cmp::max(1, num_sets / rayon::current_num_threads()); - self.sets - .into_par_iter() - .chunks(num_chunks) - .map(|chunk| verify_signature_sets(chunk.iter())) - .reduce(|| true, |current, this| current && this) + verify_signature_sets(self.sets.iter()) } } From 9b093c8459a1d8e8cc12acdb5a048503d8220bd5 Mon Sep 17 00:00:00 2001 From: Age Manning Date: Thu, 27 Jun 2024 17:35:14 +1000 Subject: [PATCH 05/23] Prevent connections from peers with a banned ip history (#6008) * Block peers based on past ips * Remove unused type --- .../lighthouse_network/src/peer_manager/network_behaviour.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/beacon_node/lighthouse_network/src/peer_manager/network_behaviour.rs b/beacon_node/lighthouse_network/src/peer_manager/network_behaviour.rs index b776347ad0..3858a2a539 100644 --- a/beacon_node/lighthouse_network/src/peer_manager/network_behaviour.rs +++ b/beacon_node/lighthouse_network/src/peer_manager/network_behaviour.rs @@ -14,7 +14,6 @@ use slog::{debug, error, trace}; use types::EthSpec; use crate::discovery::enr_ext::EnrExt; -use crate::peer_manager::peerdb::BanResult; use crate::rpc::GoodbyeReason; use crate::types::SyncState; use crate::{metrics, ClearDialError}; @@ -201,7 +200,7 @@ impl NetworkBehaviour for PeerManager { ) -> Result, ConnectionDenied> { trace!(self.log, "Inbound connection"; "peer_id" => %peer_id, "multiaddr" => %remote_addr); // We already checked if the peer was banned on `handle_pending_inbound_connection`. - if let Some(BanResult::BadScore) = self.ban_status(&peer_id) { + if self.ban_status(&peer_id).is_some() { return Err(ConnectionDenied::new( "Connection to peer rejected: peer has a bad score", )); From f14f21f37bcdf878c6cb955f1e8bb985c2411d1c Mon Sep 17 00:00:00 2001 From: Lion - dapplion <35266934+dapplion@users.noreply.github.com> Date: Thu, 27 Jun 2024 20:21:37 +0200 Subject: [PATCH 06/23] Bound lookup parent chain length with tip extension (#5705) * Bound lookup parent chain length with tip extension * Add test --- .../network/src/sync/block_lookups/mod.rs | 18 +++++++- .../network/src/sync/block_lookups/tests.rs | 41 +++++++++++++++---- 2 files changed, 51 insertions(+), 8 deletions(-) diff --git a/beacon_node/network/src/sync/block_lookups/mod.rs b/beacon_node/network/src/sync/block_lookups/mod.rs index f685b7e59d..37f5365944 100644 --- a/beacon_node/network/src/sync/block_lookups/mod.rs +++ b/beacon_node/network/src/sync/block_lookups/mod.rs @@ -194,7 +194,15 @@ impl BlockLookups { let parent_chains = self.active_parent_lookups(); for (chain_idx, parent_chain) in parent_chains.iter().enumerate() { - if parent_chain.ancestor() == child_block_root_trigger + // `block_root_to_search` will trigger a new lookup, and it will extend a parent_chain + // beyond its max length + let block_would_extend_chain = parent_chain.ancestor() == child_block_root_trigger; + // `block_root_to_search` already has a lookup, and with the block trigger it extends + // the parent_chain beyond its length. This can happen because when creating a lookup + // for a new root we don't do any parent chain length checks + let trigger_is_chain_tip = parent_chain.tip == child_block_root_trigger; + + if (block_would_extend_chain || trigger_is_chain_tip) && parent_chain.len() >= PARENT_DEPTH_TOLERANCE { debug!(self.log, "Parent lookup chain too long"; "block_root" => ?block_root_to_search); @@ -375,6 +383,14 @@ impl BlockLookups { "response_type" => ?response_type, ); + // Here we could check if response extends a parent chain beyond its max length. + // However we defer that check to the handling of a processing error ParentUnknown. + // + // Here we could check if there's already a lookup for parent_root of `response`. In + // that case we know that sending the response for processing will likely result in + // a `ParentUnknown` error. However, for simplicity we choose to not implement this + // optimization. + // Register the download peer here. Once we have received some data over the wire we // attribute it to this peer for scoring latter regardless of how the request was // done. diff --git a/beacon_node/network/src/sync/block_lookups/tests.rs b/beacon_node/network/src/sync/block_lookups/tests.rs index a607151bde..3f681f8ec2 100644 --- a/beacon_node/network/src/sync/block_lookups/tests.rs +++ b/beacon_node/network/src/sync/block_lookups/tests.rs @@ -278,8 +278,11 @@ impl TestRig { } } - fn failed_chains_contains(&mut self, chain_hash: &Hash256) -> bool { - self.sync_manager.get_failed_chains().contains(chain_hash) + fn assert_failed_chain(&mut self, chain_hash: Hash256) { + let failed_chains = self.sync_manager.get_failed_chains(); + if !failed_chains.contains(&chain_hash) { + panic!("expected failed chains to contain {chain_hash:?}: {failed_chains:?}"); + } } fn find_single_lookup_for(&self, block_root: Hash256) -> Id { @@ -1201,7 +1204,7 @@ fn test_parent_lookup_too_many_download_attempts_no_blacklist() { // Trigger the request rig.trigger_unknown_parent_block(peer_id, block.into()); for i in 1..=PARENT_FAIL_TOLERANCE { - assert!(!rig.failed_chains_contains(&block_root)); + rig.assert_not_failed_chain(block_root); let id = rig.expect_block_parent_request(parent_root); if i % 2 != 0 { // The request fails. It should be tried again. @@ -1214,8 +1217,8 @@ fn test_parent_lookup_too_many_download_attempts_no_blacklist() { } } - assert!(!rig.failed_chains_contains(&block_root)); - assert!(!rig.failed_chains_contains(&parent.canonical_root())); + rig.assert_not_failed_chain(block_root); + rig.assert_not_failed_chain(parent.canonical_root()); rig.expect_no_active_lookups_empty_network(); } @@ -1253,7 +1256,7 @@ fn test_parent_lookup_too_many_processing_attempts_must_blacklist() { } #[test] -fn test_parent_lookup_too_deep() { +fn test_parent_lookup_too_deep_grow_ancestor() { let mut rig = TestRig::test_setup(); let mut blocks = rig.rand_blockchain(PARENT_DEPTH_TOLERANCE); @@ -1278,7 +1281,31 @@ fn test_parent_lookup_too_deep() { } rig.expect_penalty(peer_id, "chain_too_long"); - assert!(rig.failed_chains_contains(&chain_hash)); + rig.assert_failed_chain(chain_hash); +} + +#[test] +fn test_parent_lookup_too_deep_grow_tip() { + let mut rig = TestRig::test_setup(); + let blocks = rig.rand_blockchain(PARENT_DEPTH_TOLERANCE - 1); + let peer_id = rig.new_connected_peer(); + let tip = blocks.last().unwrap().clone(); + + for block in blocks.into_iter() { + let block_root = block.canonical_root(); + rig.trigger_unknown_block_from_attestation(block_root, peer_id); + let id = rig.expect_block_parent_request(block_root); + rig.single_lookup_block_response(id, peer_id, Some(block.clone())); + rig.single_lookup_block_response(id, peer_id, None); + rig.expect_block_process(ResponseType::Block); + rig.single_block_component_processed( + id.lookup_id, + BlockError::ParentUnknown(RpcBlock::new_without_blobs(None, block)).into(), + ); + } + + rig.expect_penalty(peer_id, "chain_too_long"); + rig.assert_failed_chain(tip.canonical_root()); } #[test] From 784ef5fb43e2630572640c519e2cbf7f6bb50e73 Mon Sep 17 00:00:00 2001 From: Lion - dapplion <35266934+dapplion@users.noreply.github.com> Date: Thu, 27 Jun 2024 20:21:40 +0200 Subject: [PATCH 07/23] Pass vec to range sync batch (#5710) * Pass vec to range sync batch --- .../network/src/sync/backfill_sync/mod.rs | 12 +--- beacon_node/network/src/sync/manager.rs | 59 ++++++++----------- .../network/src/sync/range_sync/batch.rs | 42 ++++--------- .../network/src/sync/range_sync/chain.rs | 10 +--- .../network/src/sync/range_sync/range.rs | 8 +-- 5 files changed, 46 insertions(+), 85 deletions(-) diff --git a/beacon_node/network/src/sync/backfill_sync/mod.rs b/beacon_node/network/src/sync/backfill_sync/mod.rs index ce7d04ac0a..728642cc78 100644 --- a/beacon_node/network/src/sync/backfill_sync/mod.rs +++ b/beacon_node/network/src/sync/backfill_sync/mod.rs @@ -369,7 +369,7 @@ impl BackFillSync { batch_id: BatchId, peer_id: &PeerId, request_id: Id, - beacon_block: Option>, + blocks: Vec>, ) -> Result { // check if we have this batch let batch = match self.batches.get_mut(&batch_id) { @@ -392,20 +392,14 @@ impl BackFillSync { } }; - if let Some(block) = beacon_block { - // This is not a stream termination, simply add the block to the request - if let Err(e) = batch.add_block(block) { - self.fail_sync(BackFillError::BatchInvalidState(batch_id, e.0))?; - } - Ok(ProcessResult::Successful) - } else { + { // A stream termination has been sent. This batch has ended. Process a completed batch. // Remove the request from the peer's active batches self.active_requests .get_mut(peer_id) .map(|active_requests| active_requests.remove(&batch_id)); - match batch.download_completed() { + match batch.download_completed(blocks) { Ok(received) => { let awaiting_batches = self.processing_target.saturating_sub(batch_id) / BACKFILL_EPOCHS_PER_BATCH; diff --git a/beacon_node/network/src/sync/manager.rs b/beacon_node/network/src/sync/manager.rs index 4c1a1e6b67..fc1a218d82 100644 --- a/beacon_node/network/src/sync/manager.rs +++ b/beacon_node/network/src/sync/manager.rs @@ -908,39 +908,32 @@ impl SyncManager { { match resp.responses { Ok(blocks) => { - for block in blocks - .into_iter() - .map(Some) - // chain the stream terminator - .chain(vec![None]) - { - match resp.sender_id { - RangeRequestId::RangeSync { chain_id, batch_id } => { - self.range_sync.blocks_by_range_response( - &mut self.network, - peer_id, - chain_id, - batch_id, - id, - block, - ); - self.update_sync_state(); - } - RangeRequestId::BackfillSync { batch_id } => { - match self.backfill_sync.on_block_response( - &mut self.network, - batch_id, - &peer_id, - id, - block, - ) { - Ok(ProcessResult::SyncCompleted) => self.update_sync_state(), - Ok(ProcessResult::Successful) => {} - Err(_error) => { - // The backfill sync has failed, errors are reported - // within. - self.update_sync_state(); - } + match resp.sender_id { + RangeRequestId::RangeSync { chain_id, batch_id } => { + self.range_sync.blocks_by_range_response( + &mut self.network, + peer_id, + chain_id, + batch_id, + id, + blocks, + ); + self.update_sync_state(); + } + RangeRequestId::BackfillSync { batch_id } => { + match self.backfill_sync.on_block_response( + &mut self.network, + batch_id, + &peer_id, + id, + blocks, + ) { + Ok(ProcessResult::SyncCompleted) => self.update_sync_state(), + Ok(ProcessResult::Successful) => {} + Err(_error) => { + // The backfill sync has failed, errors are reported + // within. + self.update_sync_state(); } } } diff --git a/beacon_node/network/src/sync/range_sync/batch.rs b/beacon_node/network/src/sync/range_sync/batch.rs index 75cb49d176..baba8c9a62 100644 --- a/beacon_node/network/src/sync/range_sync/batch.rs +++ b/beacon_node/network/src/sync/range_sync/batch.rs @@ -116,7 +116,7 @@ pub enum BatchState { /// The batch has failed either downloading or processing, but can be requested again. AwaitingDownload, /// The batch is being downloaded. - Downloading(PeerId, Vec>, Id), + Downloading(PeerId, Id), /// The batch has been completely downloaded and is ready for processing. AwaitingProcessing(PeerId, Vec>), /// The batch is being processed. @@ -199,7 +199,7 @@ impl BatchInfo { /// Verifies if an incoming block belongs to this batch. pub fn is_expecting_block(&self, peer_id: &PeerId, request_id: &Id) -> bool { - if let BatchState::Downloading(expected_peer, _, expected_id) = &self.state { + if let BatchState::Downloading(expected_peer, expected_id) = &self.state { return peer_id == expected_peer && expected_id == request_id; } false @@ -209,7 +209,7 @@ impl BatchInfo { pub fn current_peer(&self) -> Option<&PeerId> { match &self.state { BatchState::AwaitingDownload | BatchState::Failed => None, - BatchState::Downloading(peer_id, _, _) + BatchState::Downloading(peer_id, _) | BatchState::AwaitingProcessing(peer_id, _) | BatchState::Processing(Attempt { peer_id, .. }) | BatchState::AwaitingValidation(Attempt { peer_id, .. }) => Some(peer_id), @@ -250,36 +250,18 @@ impl BatchInfo { &self.failed_processing_attempts } - /// Adds a block to a downloading batch. - pub fn add_block(&mut self, block: RpcBlock) -> Result<(), WrongState> { - match self.state.poison() { - BatchState::Downloading(peer, mut blocks, req_id) => { - blocks.push(block); - self.state = BatchState::Downloading(peer, blocks, req_id); - Ok(()) - } - BatchState::Poisoned => unreachable!("Poisoned batch"), - other => { - self.state = other; - Err(WrongState(format!( - "Add block for batch in wrong state {:?}", - self.state - ))) - } - } - } - /// Marks the batch as ready to be processed if the blocks are in the range. The number of /// received blocks is returned, or the wrong batch end on failure #[must_use = "Batch may have failed"] pub fn download_completed( &mut self, + blocks: Vec>, ) -> Result< usize, /* Received blocks */ Result<(Slot, Slot, BatchOperationOutcome), WrongState>, > { match self.state.poison() { - BatchState::Downloading(peer, blocks, _request_id) => { + BatchState::Downloading(peer, _request_id) => { // verify that blocks are in range if let Some(last_slot) = blocks.last().map(|b| b.slot()) { // the batch is non-empty @@ -336,7 +318,7 @@ impl BatchInfo { mark_failed: bool, ) -> Result { match self.state.poison() { - BatchState::Downloading(peer, _, _request_id) => { + BatchState::Downloading(peer, _request_id) => { // register the attempt and check if the batch can be tried again if mark_failed { self.failed_download_attempts.push(peer); @@ -369,7 +351,7 @@ impl BatchInfo { ) -> Result<(), WrongState> { match self.state.poison() { BatchState::AwaitingDownload => { - self.state = BatchState::Downloading(peer, Vec::new(), request_id); + self.state = BatchState::Downloading(peer, request_id); Ok(()) } BatchState::Poisoned => unreachable!("Poisoned batch"), @@ -536,13 +518,9 @@ impl std::fmt::Debug for BatchState { BatchState::AwaitingProcessing(ref peer, ref blocks) => { write!(f, "AwaitingProcessing({}, {} blocks)", peer, blocks.len()) } - BatchState::Downloading(peer, blocks, request_id) => write!( - f, - "Downloading({}, {} blocks, {})", - peer, - blocks.len(), - request_id - ), + BatchState::Downloading(peer, request_id) => { + write!(f, "Downloading({}, {})", peer, request_id) + } BatchState::Poisoned => f.write_str("Poisoned"), } } diff --git a/beacon_node/network/src/sync/range_sync/chain.rs b/beacon_node/network/src/sync/range_sync/chain.rs index 63cafa9aca..13c9f4be3b 100644 --- a/beacon_node/network/src/sync/range_sync/chain.rs +++ b/beacon_node/network/src/sync/range_sync/chain.rs @@ -200,7 +200,7 @@ impl SyncingChain { batch_id: BatchId, peer_id: &PeerId, request_id: Id, - beacon_block: Option>, + blocks: Vec>, ) -> ProcessingResult { // check if we have this batch let batch = match self.batches.get_mut(&batch_id) { @@ -221,18 +221,14 @@ impl SyncingChain { } }; - if let Some(block) = beacon_block { - // This is not a stream termination, simply add the block to the request - batch.add_block(block)?; - Ok(KeepChain) - } else { + { // A stream termination has been sent. This batch has ended. Process a completed batch. // Remove the request from the peer's active batches self.peers .get_mut(peer_id) .map(|active_requests| active_requests.remove(&batch_id)); - match batch.download_completed() { + match batch.download_completed(blocks) { Ok(received) => { let awaiting_batches = batch_id .saturating_sub(self.optimistic_start.unwrap_or(self.processing_target)) diff --git a/beacon_node/network/src/sync/range_sync/range.rs b/beacon_node/network/src/sync/range_sync/range.rs index fe48db35b4..5393b8792c 100644 --- a/beacon_node/network/src/sync/range_sync/range.rs +++ b/beacon_node/network/src/sync/range_sync/range.rs @@ -210,11 +210,11 @@ where chain_id: ChainId, batch_id: BatchId, request_id: Id, - beacon_block: Option>, + blocks: Vec>, ) { // check if this chunk removes the chain match self.chains.call_by_id(chain_id, |chain| { - chain.on_block_response(network, batch_id, &peer_id, request_id, beacon_block) + chain.on_block_response(network, batch_id, &peer_id, request_id, blocks) }) { Ok((removed_chain, sync_type)) => { if let Some((removed_chain, remove_reason)) = removed_chain { @@ -795,7 +795,7 @@ mod tests { rig.cx.update_execution_engine_state(EngineState::Offline); // send the response to the request - range.blocks_by_range_response(&mut rig.cx, peer1, chain1, batch1, id1, None); + range.blocks_by_range_response(&mut rig.cx, peer1, chain1, batch1, id1, vec![]); // the beacon processor shouldn't have received any work rig.expect_empty_processor(); @@ -809,7 +809,7 @@ mod tests { rig.complete_range_block_and_blobs_response(block_req, blob_req_opt); // send the response to the request - range.blocks_by_range_response(&mut rig.cx, peer2, chain2, batch2, id2, None); + range.blocks_by_range_response(&mut rig.cx, peer2, chain2, batch2, id2, vec![]); // the beacon processor shouldn't have received any work rig.expect_empty_processor(); From 9e12c21f268c80a3f002ae0ca27477f9f512eb6f Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Fri, 28 Jun 2024 12:10:32 +1000 Subject: [PATCH 08/23] Release v5.2.1 (testing branch) (#5989) * Release v5.2.1 --- Cargo.lock | 8 ++++---- beacon_node/Cargo.toml | 2 +- boot_node/Cargo.toml | 2 +- common/lighthouse_version/src/lib.rs | 4 ++-- lcli/Cargo.toml | 2 +- lighthouse/Cargo.toml | 2 +- 6 files changed, 10 insertions(+), 10 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index a1865289b0..814a9b45ed 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -855,7 +855,7 @@ dependencies = [ [[package]] name = "beacon_node" -version = "5.2.0" +version = "5.2.1" dependencies = [ "beacon_chain", "clap", @@ -1061,7 +1061,7 @@ dependencies = [ [[package]] name = "boot_node" -version = "5.2.0" +version = "5.2.1" dependencies = [ "beacon_node", "clap", @@ -4322,7 +4322,7 @@ checksum = "830d08ce1d1d941e6b30645f1a0eb5643013d835ce3779a5fc208261dbe10f55" [[package]] name = "lcli" -version = "5.2.0" +version = "5.2.1" dependencies = [ "account_utils", "beacon_chain", @@ -4893,7 +4893,7 @@ dependencies = [ [[package]] name = "lighthouse" -version = "5.2.0" +version = "5.2.1" dependencies = [ "account_manager", "account_utils", diff --git a/beacon_node/Cargo.toml b/beacon_node/Cargo.toml index b95720e807..a5fd29c971 100644 --- a/beacon_node/Cargo.toml +++ b/beacon_node/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "beacon_node" -version = "5.2.0" +version = "5.2.1" authors = [ "Paul Hauner ", "Age Manning "] edition = { workspace = true } diff --git a/common/lighthouse_version/src/lib.rs b/common/lighthouse_version/src/lib.rs index 6fb06cc543..d32d799468 100644 --- a/common/lighthouse_version/src/lib.rs +++ b/common/lighthouse_version/src/lib.rs @@ -17,8 +17,8 @@ pub const VERSION: &str = git_version!( // NOTE: using --match instead of --exclude for compatibility with old Git "--match=thiswillnevermatchlol" ], - prefix = "Lighthouse/v5.2.0-", - fallback = "Lighthouse/v5.2.0" + prefix = "Lighthouse/v5.2.1-", + fallback = "Lighthouse/v5.2.1" ); /// Returns the first eight characters of the latest commit hash for this build. diff --git a/lcli/Cargo.toml b/lcli/Cargo.toml index 73dd93dc3e..3cddd8ee60 100644 --- a/lcli/Cargo.toml +++ b/lcli/Cargo.toml @@ -1,7 +1,7 @@ [package] name = "lcli" description = "Lighthouse CLI (modeled after zcli)" -version = "5.2.0" +version = "5.2.1" authors = ["Paul Hauner "] edition = { workspace = true } diff --git a/lighthouse/Cargo.toml b/lighthouse/Cargo.toml index 20466b5de7..a1674d8d2c 100644 --- a/lighthouse/Cargo.toml +++ b/lighthouse/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "lighthouse" -version = "5.2.0" +version = "5.2.1" authors = ["Sigma Prime "] edition = { workspace = true } autotests = false From 16b81132ca0fa23672cb69c2f81bddaff847e786 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Sat, 29 Jun 2024 09:43:21 +1000 Subject: [PATCH 09/23] Electra epoch processing (#5761) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Attestation superstruct changes for EIP 7549 (#5644) * update * experiment * superstruct changes * revert * superstruct changes * fix tests * indexed attestation * indexed attestation superstruct * updated TODOs * `superstruct` the `AttesterSlashing` (#5636) * `superstruct` Attester Fork Variants * Push a little further * Deal with Encode / Decode of AttesterSlashing * not so sure about this.. * Stop Encode/Decode Bounds from Propagating Out * Tons of Changes.. * More Conversions to AttestationRef * Add AsReference trait (#15) * Add AsReference trait * Fix some snafus * Got it Compiling! :D * Got Tests Building * Get beacon chain tests compiling --------- Co-authored-by: Michael Sproul * Merge remote-tracking branch 'upstream/unstable' into electra_attestation_changes * Make EF Tests Fork-Agnostic (#5713) * Finish EF Test Fork Agnostic (#5714) * Superstruct `AggregateAndProof` (#5715) * Upgrade `superstruct` to `0.8.0` * superstruct `AggregateAndProof` * Merge remote-tracking branch 'sigp/unstable' into electra_attestation_changes * cargo fmt * Merge pull request #5726 from realbigsean/electra_attestation_changes Merge unstable into Electra attestation changes * process withdrawals updates * cleanup withdrawals processing * update `process_operations` deposit length check * add apply_deposit changes * add execution layer withdrawal request processing * process deposit receipts * add consolidation processing * update process operations function * exit updates * clean up * update slash_validator * EIP7549 `get_attestation_indices` (#5657) * get attesting indices electra impl * fmt * get tests to pass * fmt * fix some beacon chain tests * fmt * fix slasher test * fmt got me again * fix more tests * fix tests * Some small changes (#5739) * Merge branch 'electra_attestation_changes' of https://github.com/sigp/lighthouse into block-processing-electra * cargo fmt (#5740) * Merge branch 'electra_attestation_changes' of https://github.com/sigp/lighthouse into block-processing-electra * fix attestation verification * Sketch op pool changes * fix get attesting indices (#5742) * fix get attesting indices * better errors * fix compile * only get committee index once * Merge branch 'electra_attestation_changes' of https://github.com/sigp/lighthouse into block-processing-electra * Ef test fixes (#5753) * attestation related ef test fixes * delete commented out stuff * Merge branch 'electra_attestation_changes' of https://github.com/sigp/lighthouse into block-processing-electra * Fix Aggregation Pool for Electra (#5754) * Fix Aggregation Pool for Electra * Remove Outdated Interface * fix ssz (#5755) * Get `electra_op_pool` up to date (#5756) * fix get attesting indices (#5742) * fix get attesting indices * better errors * fix compile * only get committee index once * Ef test fixes (#5753) * attestation related ef test fixes * delete commented out stuff * Fix Aggregation Pool for Electra (#5754) * Fix Aggregation Pool for Electra * Remove Outdated Interface * fix ssz (#5755) --------- Co-authored-by: realbigsean * Revert "Get `electra_op_pool` up to date (#5756)" (#5757) This reverts commit ab9e58aa3d0e6fe2175a4996a5de710e81152896. * Merge branch 'electra_attestation_changes' of https://github.com/sigp/lighthouse into electra_op_pool * Merge branch 'electra_attestation_changes' of https://github.com/sigp/lighthouse into block-processing-electra * Compute on chain aggregate impl (#5752) * add compute_on_chain_agg impl to op pool changes * fmt * get op pool tests to pass * update the naive agg pool interface (#5760) * Merge branch 'electra_attestation_changes' of https://github.com/sigp/lighthouse into block-processing-electra * Fix bugs in cross-committee aggregation * Add comment to max cover optimisation * Fix assert * Electra epoch processing * Merge pull request #5749 from sigp/electra_op_pool Optimise Electra op pool aggregation * don't fail on empty consolidations * Merge branch 'electra_attestation_changes' of https://github.com/sigp/lighthouse into block-processing-electra * Merge branch 'block-processing-electra' of https://github.com/sigp/lighthouse into electra-epoch-proc * update committee offset * Merge branch 'electra_attestation_changes' of https://github.com/sigp/lighthouse into block-processing-electra * update committee offset * only increment the state deposit index on old deposit flow * Merge branch 'block-processing-electra' of https://github.com/sigp/lighthouse into electra-epoch-proc * use correct max eb in epoch cache initialization * drop initiate validator ordering optimization * fix initiate exit for single pass * Fix Electra Fork Choice Tests (#5764) * Fix Electra Fork Choice Tests (#5764) * Fix Electra Fork Choice Tests (#5764) * Merge branch 'electra_attestation_changes' of https://github.com/sigp/lighthouse into block-processing-electra * Merge branch 'block-processing-electra' of https://github.com/sigp/lighthouse into electra-epoch-proc * Fix Consolidation Sigs & Withdrawals * Merge pull request #5766 from ethDreamer/two_fixes Fix Consolidation Sigs & Withdrawals * Merge branches 'block-processing-electra' and 'electra-epoch-proc' of https://github.com/sigp/lighthouse into electra-epoch-proc * Subscribe to the correct subnets for electra attestations (#5782) * subscribe to the correct att subnets for electra * subscribe to the correct att subnets for electra * cargo fmt * Subscribe to the correct subnets for electra attestations (#5782) * subscribe to the correct att subnets for electra * subscribe to the correct att subnets for electra * cargo fmt * Subscribe to the correct subnets for electra attestations (#5782) * subscribe to the correct att subnets for electra * subscribe to the correct att subnets for electra * cargo fmt * fix slashing handling * Fix Bug In Block Processing with 0x02 Credentials * Merge remote-tracking branch 'upstream/unstable' * Send unagg attestation based on fork * Publish all aggregates * just one more check bro plz.. * Merge pull request #5832 from ethDreamer/electra_attestation_changes_merge_unstable Merge `unstable` into `electra_attestation_changes` * Merge pull request #5835 from realbigsean/fix-validator-logic Fix validator logic * Merge pull request #5816 from realbigsean/electra-attestation-slashing-handling Electra slashing handling * Merge branch 'electra_attestation_changes' of https://github.com/sigp/lighthouse into block-processing-electra * Merge branch 'block-processing-electra' of https://github.com/sigp/lighthouse into electra-epoch-proc * Electra attestation changes rm decode impl (#5856) * Remove Crappy Decode impl for Attestation * Remove Inefficient Attestation Decode impl * Implement Schema Upgrade / Downgrade * Update beacon_node/beacon_chain/src/schema_change/migration_schema_v20.rs Co-authored-by: Michael Sproul --------- Co-authored-by: Michael Sproul * Fix failing attestation tests and misc electra attestation cleanup (#5810) * - get attestation related beacon chain tests to pass - observed attestations are now keyed off of data + committee index - rename op pool attestationref to compactattestationref - remove unwraps in agg pool and use options instead - cherry pick some changes from ef-tests-electra * cargo fmt * fix failing test * Revert dockerfile changes * make committee_index return option * function args shouldnt be a ref to attestation ref * fmt * fix dup imports --------- Co-authored-by: realbigsean * fix some todos (#5817) * Merge branch 'unstable' of https://github.com/sigp/lighthouse into electra_attestation_changes * add consolidations to merkle calc for inclusion proof * Merge branch 'electra_attestation_changes' of https://github.com/sigp/lighthouse into block-processing-electra * Merge branch 'block-processing-electra' of https://github.com/sigp/lighthouse into electra-epoch-proc * Remove Duplicate KZG Commitment Merkle Proof Code (#5874) * Remove Duplicate KZG Commitment Merkle Proof Code * s/tree_lists/fields/ * Merge branch 'unstable' of https://github.com/sigp/lighthouse into electra_attestation_changes * Merge branch 'electra_attestation_changes' of https://github.com/sigp/lighthouse into block-processing-electra * Merge branch 'block-processing-electra' of https://github.com/sigp/lighthouse into electra-epoch-proc * fix compile * Merge branch 'electra_attestation_changes' of https://github.com/sigp/lighthouse into block-processing-electra * Merge branch 'block-processing-electra' of https://github.com/sigp/lighthouse into electra-epoch-proc * Fix slasher tests (#5906) * Fix electra tests * Add electra attestations to double vote tests * Update superstruct to 0.8 * Merge remote-tracking branch 'origin/unstable' into electra_attestation_changes * Small cleanup in slasher tests * Clean up Electra observed aggregates (#5929) * Use consistent key in observed_attestations * Remove unwraps from observed aggregates * Merge branch 'unstable' of https://github.com/sigp/lighthouse into electra_attestation_changes * De-dup attestation constructor logic * Remove unwraps in Attestation construction * Dedup match_attestation_data * Remove outdated TODO * Use ForkName Ord in fork-choice tests * Use ForkName Ord in BeaconBlockBody * Make to_electra not fallible * Remove TestRandom impl for IndexedAttestation * Remove IndexedAttestation faulty Decode impl * Drop TestRandom impl * Add PendingAttestationInElectra * Indexed att on disk (#35) * indexed att on disk * fix lints * Update slasher/src/migrate.rs Co-authored-by: ethDreamer <37123614+ethDreamer@users.noreply.github.com> --------- Co-authored-by: Lion - dapplion <35266934+dapplion@users.noreply.github.com> Co-authored-by: ethDreamer <37123614+ethDreamer@users.noreply.github.com> * add electra fork enabled fn to ForkName impl (#36) * add electra fork enabled fn to ForkName impl * remove inadvertent file * Update common/eth2/src/types.rs Co-authored-by: ethDreamer <37123614+ethDreamer@users.noreply.github.com> * Dedup attestation constructor logic in attester cache * Use if let Ok for committee_bits * Dedup Attestation constructor code * Diff reduction in tests * Fix beacon_chain tests * Diff reduction * Use Ord for ForkName in pubsub * Resolve into_attestation_and_indices todo * Remove stale TODO * Fix beacon_chain tests * Test spec invariant * Use electra_enabled in pubsub * Remove get_indexed_attestation_from_signed_aggregate * Use ok_or instead of if let else * committees are sorted * remove dup method `get_indexed_attestation_from_committees` * Merge pull request #5940 from dapplion/electra_attestation_changes_lionreview Electra attestations #5712 review * update default persisted op pool deserialization * ensure aggregate and proof uses serde untagged on ref * Fork aware ssz static attestation tests * Electra attestation changes from Lions review (#5971) * dedup/cleanup and remove unneeded hashset use * remove irrelevant TODOs * Merge branch 'unstable' of https://github.com/sigp/lighthouse into electra_attestation_changes * Merge branch 'electra_attestation_changes' of https://github.com/sigp/lighthouse into block-processing-electra * Merge branch 'block-processing-electra' of https://github.com/sigp/lighthouse into electra-epoch-proc * Electra attestation changes sean review (#5972) * instantiate empty bitlist in unreachable code * clean up error conversion * fork enabled bool cleanup * remove a couple todos * return bools instead of options in `aggregate` and use the result * delete commented out code * use map macros in simple transformations * remove signers_disjoint_from * get ef tests compiling * get ef tests compiling * update intentionally excluded files * Avoid changing slasher schema for Electra * Delete slasher schema v4 * Fix clippy * Fix compilation of beacon_chain tests * Update database.rs * Update per_block_processing.rs * Add electra lightclient types * Update slasher/src/database.rs * fix imports * Merge pull request #5980 from dapplion/electra-lightclient Add electra lightclient types * Merge pull request #5975 from michaelsproul/electra-slasher-no-migration Avoid changing slasher schema for Electra * Update beacon_node/beacon_chain/src/attestation_verification.rs * Update beacon_node/beacon_chain/src/attestation_verification.rs * Merge branch 'unstable' of https://github.com/sigp/lighthouse into electra_attestation_changes * Merge branch 'electra_attestation_changes' of https://github.com/realbigsean/lighthouse into block-processing-electra * Merge branch 'block-processing-electra' of https://github.com/sigp/lighthouse into electra-epoch-proc * Merge branch 'unstable' of https://github.com/sigp/lighthouse into electra-epoch-proc * Update consensus/state_processing/src/per_epoch_processing/single_pass.rs * Update consensus/state_processing/src/per_epoch_processing/single_pass.rs * Update consensus/state_processing/src/per_epoch_processing/single_pass.rs * Update consensus/state_processing/src/per_epoch_processing/single_pass.rs * Update consensus/state_processing/src/per_epoch_processing/single_pass.rs --- .../src/common/initiate_validator_exit.rs | 21 +- consensus/state_processing/src/epoch_cache.rs | 49 +- .../src/per_epoch_processing/errors.rs | 3 + .../src/per_epoch_processing/single_pass.rs | 463 ++++++++++++++++-- consensus/types/src/chain_spec.rs | 10 +- consensus/types/src/validator.rs | 30 +- 6 files changed, 506 insertions(+), 70 deletions(-) diff --git a/consensus/state_processing/src/common/initiate_validator_exit.rs b/consensus/state_processing/src/common/initiate_validator_exit.rs index 8874e9ed4b..49e3a7390d 100644 --- a/consensus/state_processing/src/common/initiate_validator_exit.rs +++ b/consensus/state_processing/src/common/initiate_validator_exit.rs @@ -8,12 +8,12 @@ pub fn initiate_validator_exit( index: usize, spec: &ChainSpec, ) -> Result<(), Error> { - // We do things in a slightly different order to the spec here. Instead of immediately checking - // whether the validator has already exited, we instead prepare the exit cache and compute the - // cheap-to-calculate values from that. *Then* we look up the validator a single time in the - // validator tree (expensive), make the check and mutate as appropriate. Compared to the spec - // ordering, this saves us from looking up the validator in the validator registry multiple - // times. + let validator = state.get_validator_cow(index)?; + + // Return if the validator already initiated exit + if validator.exit_epoch != spec.far_future_epoch { + return Ok(()); + } // Ensure the exit cache is built. state.build_exit_cache(spec)?; @@ -36,14 +36,7 @@ pub fn initiate_validator_exit( exit_queue_epoch }; - let validator = state.get_validator_cow(index)?; - - // Return if the validator already initiated exit - if validator.exit_epoch != spec.far_future_epoch { - return Ok(()); - } - - let validator = validator.into_mut()?; + let validator = state.get_validator_mut(index)?; validator.exit_epoch = exit_queue_epoch; validator.withdrawable_epoch = exit_queue_epoch.safe_add(spec.min_validator_withdrawability_delay)?; diff --git a/consensus/state_processing/src/epoch_cache.rs b/consensus/state_processing/src/epoch_cache.rs index b2f2d85407..0e940fabe4 100644 --- a/consensus/state_processing/src/epoch_cache.rs +++ b/consensus/state_processing/src/epoch_cache.rs @@ -9,6 +9,7 @@ use types::{ActivationQueue, BeaconState, ChainSpec, EthSpec, ForkName, Hash256} pub struct PreEpochCache { epoch_key: EpochCacheKey, effective_balances: Vec, + total_active_balance: u64, } impl PreEpochCache { @@ -36,27 +37,59 @@ impl PreEpochCache { Ok(Self { epoch_key, effective_balances: Vec::with_capacity(state.validators().len()), + total_active_balance: 0, }) } - pub fn push_effective_balance(&mut self, effective_balance: u64) { - self.effective_balances.push(effective_balance); + pub fn update_effective_balance( + &mut self, + validator_index: usize, + effective_balance: u64, + is_active_next_epoch: bool, + ) -> Result<(), EpochCacheError> { + if validator_index == self.effective_balances.len() { + self.effective_balances.push(effective_balance); + if is_active_next_epoch { + self.total_active_balance + .safe_add_assign(effective_balance)?; + } + + Ok(()) + } else if let Some(existing_balance) = self.effective_balances.get_mut(validator_index) { + // Update total active balance for a late change in effective balance. This happens when + // processing consolidations. + if is_active_next_epoch { + self.total_active_balance + .safe_add_assign(effective_balance)?; + self.total_active_balance + .safe_sub_assign(*existing_balance)?; + } + *existing_balance = effective_balance; + Ok(()) + } else { + Err(EpochCacheError::ValidatorIndexOutOfBounds { validator_index }) + } + } + + pub fn get_total_active_balance(&self) -> u64 { + self.total_active_balance } pub fn into_epoch_cache( self, - total_active_balance: u64, activation_queue: ActivationQueue, spec: &ChainSpec, ) -> Result { let epoch = self.epoch_key.epoch; + let total_active_balance = self.total_active_balance; let sqrt_total_active_balance = SqrtTotalActiveBalance::new(total_active_balance); let base_reward_per_increment = BaseRewardPerIncrement::new(total_active_balance, spec)?; let effective_balance_increment = spec.effective_balance_increment; - let max_effective_balance_eth = spec - .max_effective_balance - .safe_div(effective_balance_increment)?; + let max_effective_balance = + spec.max_effective_balance_for_fork(spec.fork_name_at_epoch(epoch)); + let max_effective_balance_eth = + max_effective_balance.safe_div(effective_balance_increment)?; let mut base_rewards = Vec::with_capacity(max_effective_balance_eth.safe_add(1)? as usize); @@ -131,9 +164,9 @@ pub fn initialize_epoch_cache( decision_block_root, }, effective_balances, + total_active_balance, }; - *state.epoch_cache_mut() = - pre_epoch_cache.into_epoch_cache(total_active_balance, activation_queue, spec)?; + *state.epoch_cache_mut() = pre_epoch_cache.into_epoch_cache(activation_queue, spec)?; Ok(()) } diff --git a/consensus/state_processing/src/per_epoch_processing/errors.rs b/consensus/state_processing/src/per_epoch_processing/errors.rs index de481ec676..b6c9dbea52 100644 --- a/consensus/state_processing/src/per_epoch_processing/errors.rs +++ b/consensus/state_processing/src/per_epoch_processing/errors.rs @@ -25,6 +25,9 @@ pub enum EpochProcessingError { InvalidFlagIndex(usize), MilhouseError(milhouse::Error), EpochCache(EpochCacheError), + SinglePassMissingActivationQueue, + MissingEarliestExitEpoch, + MissingExitBalanceToConsume, } impl From for EpochProcessingError { 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 a9629e73e4..e5905b8fa2 100644 --- a/consensus/state_processing/src/per_epoch_processing/single_pass.rs +++ b/consensus/state_processing/src/per_epoch_processing/single_pass.rs @@ -1,20 +1,24 @@ use crate::{ - common::update_progressive_balances_cache::initialize_progressive_balances_cache, + common::{ + decrease_balance, increase_balance, + update_progressive_balances_cache::initialize_progressive_balances_cache, + }, epoch_cache::{initialize_epoch_cache, PreEpochCache}, per_epoch_processing::{Delta, Error, ParticipationEpochSummary}, }; use itertools::izip; use safe_arith::{SafeArith, SafeArithIter}; use std::cmp::{max, min}; -use std::collections::BTreeSet; +use std::collections::{BTreeSet, HashMap}; use types::{ consts::altair::{ NUM_FLAG_INDICES, PARTICIPATION_FLAG_WEIGHTS, TIMELY_HEAD_FLAG_INDEX, TIMELY_TARGET_FLAG_INDEX, WEIGHT_DENOMINATOR, }, milhouse::Cow, - ActivationQueue, BeaconState, BeaconStateError, ChainSpec, Epoch, EthSpec, ExitCache, ForkName, - ParticipationFlags, ProgressiveBalancesCache, RelativeEpoch, Unsigned, Validator, + ActivationQueue, BeaconState, BeaconStateError, ChainSpec, Checkpoint, Epoch, EthSpec, + ExitCache, ForkName, List, ParticipationFlags, ProgressiveBalancesCache, RelativeEpoch, + Unsigned, Validator, }; pub struct SinglePassConfig { @@ -22,6 +26,8 @@ pub struct SinglePassConfig { pub rewards_and_penalties: bool, pub registry_updates: bool, pub slashings: bool, + pub pending_balance_deposits: bool, + pub pending_consolidations: bool, pub effective_balance_updates: bool, } @@ -38,6 +44,8 @@ impl SinglePassConfig { rewards_and_penalties: true, registry_updates: true, slashings: true, + pending_balance_deposits: true, + pending_consolidations: true, effective_balance_updates: true, } } @@ -48,6 +56,8 @@ impl SinglePassConfig { rewards_and_penalties: false, registry_updates: false, slashings: false, + pending_balance_deposits: false, + pending_consolidations: false, effective_balance_updates: false, } } @@ -57,6 +67,7 @@ impl SinglePassConfig { struct StateContext { current_epoch: Epoch, next_epoch: Epoch, + finalized_checkpoint: Checkpoint, is_in_inactivity_leak: bool, total_active_balance: u64, churn_limit: u64, @@ -73,6 +84,15 @@ struct SlashingsContext { target_withdrawable_epoch: Epoch, } +struct PendingBalanceDepositsContext { + /// The value to set `next_deposit_index` to *after* processing completes. + next_deposit_index: usize, + /// The value to set `deposit_balance_to_consume` to *after* processing completes. + deposit_balance_to_consume: u64, + /// Total balance increases for each validator due to pending balance deposits. + validator_deposits_to_process: HashMap, +} + struct EffectiveBalancesContext { downward_threshold: u64, upward_threshold: u64, @@ -129,6 +149,7 @@ pub fn process_epoch_single_pass( let state_ctxt = &StateContext { current_epoch, next_epoch, + finalized_checkpoint, is_in_inactivity_leak, total_active_balance, churn_limit, @@ -139,6 +160,16 @@ pub fn process_epoch_single_pass( let slashings_ctxt = &SlashingsContext::new(state, state_ctxt, spec)?; let mut next_epoch_cache = PreEpochCache::new_for_next_epoch(state)?; + let pending_balance_deposits_ctxt = + if fork_name.electra_enabled() && conf.pending_balance_deposits { + Some(PendingBalanceDepositsContext::new(state, spec)?) + } else { + None + }; + + let mut earliest_exit_epoch = state.earliest_exit_epoch().ok(); + let mut exit_balance_to_consume = state.exit_balance_to_consume().ok(); + // Split the state into several disjoint mutable borrows. let ( validators, @@ -165,12 +196,19 @@ pub fn process_epoch_single_pass( // Compute shared values required for different parts of epoch processing. let rewards_ctxt = &RewardsAndPenaltiesContext::new(progressive_balances, state_ctxt, spec)?; - let activation_queue = &epoch_cache - .activation_queue()? - .get_validators_eligible_for_activation( - finalized_checkpoint.epoch, - activation_churn_limit as usize, - ); + + let mut activation_queues = if !fork_name.electra_enabled() { + let activation_queue = epoch_cache + .activation_queue()? + .get_validators_eligible_for_activation( + finalized_checkpoint.epoch, + activation_churn_limit as usize, + ); + let next_epoch_activation_queue = ActivationQueue::default(); + Some((activation_queue, next_epoch_activation_queue)) + } else { + None + }; let effective_balances_ctxt = &EffectiveBalancesContext::new(spec)?; // Iterate over the validators and related fields in one pass. @@ -178,10 +216,6 @@ pub fn process_epoch_single_pass( let mut balances_iter = balances.iter_cow(); let mut inactivity_scores_iter = inactivity_scores.iter_cow(); - // Values computed for the next epoch transition. - let mut next_epoch_total_active_balance = 0; - let mut next_epoch_activation_queue = ActivationQueue::default(); - for (index, &previous_epoch_participation, ¤t_epoch_participation) in izip!( 0..num_validators, previous_epoch_participation.iter(), @@ -246,13 +280,17 @@ pub fn process_epoch_single_pass( // `process_registry_updates` if conf.registry_updates { + let activation_queue_refs = activation_queues + .as_mut() + .map(|(current_queue, next_queue)| (&*current_queue, next_queue)); process_single_registry_update( &mut validator, validator_info, exit_cache, - activation_queue, - &mut next_epoch_activation_queue, + activation_queue_refs, state_ctxt, + earliest_exit_epoch.as_mut(), + exit_balance_to_consume.as_mut(), spec, )?; } @@ -262,13 +300,22 @@ pub fn process_epoch_single_pass( process_single_slashing(&mut balance, &validator, slashings_ctxt, state_ctxt, spec)?; } + // `process_pending_balance_deposits` + if let Some(pending_balance_deposits_ctxt) = &pending_balance_deposits_ctxt { + process_pending_balance_deposits_for_validator( + &mut balance, + validator_info, + pending_balance_deposits_ctxt, + )?; + } + // `process_effective_balance_updates` if conf.effective_balance_updates { process_single_effective_balance_update( + validator_info.index, *balance, &mut validator, - validator_info, - &mut next_epoch_total_active_balance, + validator_info.current_epoch_participation, &mut next_epoch_cache, progressive_balances, effective_balances_ctxt, @@ -278,15 +325,56 @@ pub fn process_epoch_single_pass( } } - if conf.effective_balance_updates { - state.set_total_active_balance(next_epoch, next_epoch_total_active_balance, spec); - *state.epoch_cache_mut() = next_epoch_cache.into_epoch_cache( - next_epoch_total_active_balance, - next_epoch_activation_queue, + if conf.registry_updates && fork_name >= ForkName::Electra { + if let Ok(earliest_exit_epoch_state) = state.earliest_exit_epoch_mut() { + *earliest_exit_epoch_state = + earliest_exit_epoch.ok_or(Error::MissingEarliestExitEpoch)?; + } + if let Ok(exit_balance_to_consume_state) = state.exit_balance_to_consume_mut() { + *exit_balance_to_consume_state = + exit_balance_to_consume.ok_or(Error::MissingExitBalanceToConsume)?; + } + } + + // Finish processing pending balance deposits if relevant. + // + // This *could* be reordered after `process_pending_consolidations` which pushes only to the end + // of the `pending_balance_deposits` list. But we may as well preserve the write ordering used + // by the spec and do this first. + if let Some(ctxt) = pending_balance_deposits_ctxt { + let new_pending_balance_deposits = List::try_from_iter( + state + .pending_balance_deposits()? + .iter_from(ctxt.next_deposit_index)? + .cloned(), + )?; + *state.pending_balance_deposits_mut()? = new_pending_balance_deposits; + *state.deposit_balance_to_consume_mut()? = ctxt.deposit_balance_to_consume; + } + + // Process consolidations outside the single-pass loop, as they depend on balances for multiple + // validators and cannot be computed accurately inside the loop. + if fork_name.electra_enabled() && conf.pending_consolidations { + process_pending_consolidations( + state, + &mut next_epoch_cache, + effective_balances_ctxt, + state_ctxt, spec, )?; } + // Finally, finish updating effective balance caches. We need this to happen *after* processing + // of pending consolidations, which recomputes some effective balances. + if conf.effective_balance_updates { + let next_epoch_total_active_balance = next_epoch_cache.get_total_active_balance(); + state.set_total_active_balance(next_epoch, next_epoch_total_active_balance, spec); + let next_epoch_activation_queue = + activation_queues.map_or_else(ActivationQueue::default, |(_, queue)| queue); + *state.epoch_cache_mut() = + next_epoch_cache.into_epoch_cache(next_epoch_activation_queue, spec)?; + } + Ok(summary) } @@ -455,7 +543,42 @@ impl RewardsAndPenaltiesContext { } } +#[allow(clippy::too_many_arguments)] fn process_single_registry_update( + validator: &mut Cow, + validator_info: &ValidatorInfo, + exit_cache: &mut ExitCache, + activation_queues: Option<(&BTreeSet, &mut ActivationQueue)>, + state_ctxt: &StateContext, + earliest_exit_epoch: Option<&mut Epoch>, + exit_balance_to_consume: Option<&mut u64>, + spec: &ChainSpec, +) -> Result<(), Error> { + if !state_ctxt.fork_name.electra_enabled() { + let (activation_queue, next_epoch_activation_queue) = + activation_queues.ok_or(Error::SinglePassMissingActivationQueue)?; + process_single_registry_update_pre_electra( + validator, + validator_info, + exit_cache, + activation_queue, + next_epoch_activation_queue, + state_ctxt, + spec, + ) + } else { + process_single_registry_update_post_electra( + validator, + exit_cache, + state_ctxt, + earliest_exit_epoch.ok_or(Error::MissingEarliestExitEpoch)?, + exit_balance_to_consume.ok_or(Error::MissingExitBalanceToConsume)?, + spec, + ) + } +} + +fn process_single_registry_update_pre_electra( validator: &mut Cow, validator_info: &ValidatorInfo, exit_cache: &mut ExitCache, @@ -472,7 +595,7 @@ fn process_single_registry_update( if validator.is_active_at(current_epoch) && validator.effective_balance <= spec.ejection_balance { - initiate_validator_exit(validator, exit_cache, state_ctxt, spec)?; + initiate_validator_exit(validator, exit_cache, state_ctxt, None, None, spec)?; } if activation_queue.contains(&validator_info.index) { @@ -491,10 +614,49 @@ fn process_single_registry_update( Ok(()) } +fn process_single_registry_update_post_electra( + validator: &mut Cow, + exit_cache: &mut ExitCache, + state_ctxt: &StateContext, + earliest_exit_epoch: &mut Epoch, + exit_balance_to_consume: &mut u64, + spec: &ChainSpec, +) -> Result<(), Error> { + let current_epoch = state_ctxt.current_epoch; + + if validator.is_eligible_for_activation_queue(spec, state_ctxt.fork_name) { + validator.make_mut()?.activation_eligibility_epoch = current_epoch.safe_add(1)?; + } + + if validator.is_active_at(current_epoch) && validator.effective_balance <= spec.ejection_balance + { + initiate_validator_exit( + validator, + exit_cache, + state_ctxt, + Some(earliest_exit_epoch), + Some(exit_balance_to_consume), + spec, + )?; + } + + if validator.is_eligible_for_activation_with_finalized_checkpoint( + &state_ctxt.finalized_checkpoint, + spec, + ) { + validator.make_mut()?.activation_epoch = + spec.compute_activation_exit_epoch(current_epoch)?; + } + + Ok(()) +} + fn initiate_validator_exit( validator: &mut Cow, exit_cache: &mut ExitCache, state_ctxt: &StateContext, + earliest_exit_epoch: Option<&mut Epoch>, + exit_balance_to_consume: Option<&mut u64>, spec: &ChainSpec, ) -> Result<(), Error> { // Return if the validator already initiated exit @@ -502,16 +664,27 @@ fn initiate_validator_exit( return Ok(()); } - // Compute exit queue epoch - let delayed_epoch = spec.compute_activation_exit_epoch(state_ctxt.current_epoch)?; - let mut exit_queue_epoch = exit_cache - .max_epoch()? - .map_or(delayed_epoch, |epoch| max(epoch, delayed_epoch)); - let exit_queue_churn = exit_cache.get_churn_at(exit_queue_epoch)?; + let exit_queue_epoch = if state_ctxt.fork_name.electra_enabled() { + compute_exit_epoch_and_update_churn( + validator, + state_ctxt, + earliest_exit_epoch.ok_or(Error::MissingEarliestExitEpoch)?, + exit_balance_to_consume.ok_or(Error::MissingExitBalanceToConsume)?, + spec, + )? + } else { + // Compute exit queue epoch + let delayed_epoch = spec.compute_activation_exit_epoch(state_ctxt.current_epoch)?; + let mut exit_queue_epoch = exit_cache + .max_epoch()? + .map_or(delayed_epoch, |epoch| max(epoch, delayed_epoch)); + let exit_queue_churn = exit_cache.get_churn_at(exit_queue_epoch)?; - if exit_queue_churn >= state_ctxt.churn_limit { - exit_queue_epoch.safe_add_assign(1)?; - } + if exit_queue_churn >= state_ctxt.churn_limit { + exit_queue_epoch.safe_add_assign(1)?; + } + exit_queue_epoch + }; let validator = validator.make_mut()?; validator.exit_epoch = exit_queue_epoch; @@ -522,6 +695,64 @@ fn initiate_validator_exit( Ok(()) } +fn compute_exit_epoch_and_update_churn( + validator: &mut Cow, + state_ctxt: &StateContext, + earliest_exit_epoch_state: &mut Epoch, + exit_balance_to_consume_state: &mut u64, + spec: &ChainSpec, +) -> Result { + let exit_balance = validator.effective_balance; + let mut earliest_exit_epoch = std::cmp::max( + *earliest_exit_epoch_state, + spec.compute_activation_exit_epoch(state_ctxt.current_epoch)?, + ); + + let per_epoch_churn = get_activation_exit_churn_limit(state_ctxt, spec)?; + // New epoch for exits + let mut exit_balance_to_consume = if *earliest_exit_epoch_state < earliest_exit_epoch { + per_epoch_churn + } else { + *exit_balance_to_consume_state + }; + + // Exit doesn't fit in the current earliest epoch + if exit_balance > exit_balance_to_consume { + let balance_to_process = exit_balance.safe_sub(exit_balance_to_consume)?; + let additional_epochs = balance_to_process + .safe_sub(1)? + .safe_div(per_epoch_churn)? + .safe_add(1)?; + earliest_exit_epoch.safe_add_assign(additional_epochs)?; + exit_balance_to_consume.safe_add_assign(additional_epochs.safe_mul(per_epoch_churn)?)?; + } + // Consume the balance and update state variables + *exit_balance_to_consume_state = exit_balance_to_consume.safe_sub(exit_balance)?; + *earliest_exit_epoch_state = earliest_exit_epoch; + + Ok(earliest_exit_epoch) +} + +fn get_activation_exit_churn_limit( + state_ctxt: &StateContext, + spec: &ChainSpec, +) -> Result { + Ok(std::cmp::min( + spec.max_per_epoch_activation_exit_churn_limit, + get_balance_churn_limit(state_ctxt, spec)?, + )) +} + +fn get_balance_churn_limit(state_ctxt: &StateContext, spec: &ChainSpec) -> Result { + let total_active_balance = state_ctxt.total_active_balance; + let churn = std::cmp::max( + spec.min_per_epoch_churn_limit_electra, + total_active_balance.safe_div(spec.churn_limit_quotient)?, + ); + + Ok(churn.safe_sub(churn.safe_rem(spec.effective_balance_increment)?)?) +} + impl SlashingsContext { fn new( state: &BeaconState, @@ -568,6 +799,146 @@ fn process_single_slashing( Ok(()) } +impl PendingBalanceDepositsContext { + fn new(state: &BeaconState, spec: &ChainSpec) -> Result { + let available_for_processing = state + .deposit_balance_to_consume()? + .safe_add(state.get_activation_exit_churn_limit(spec)?)?; + let mut processed_amount = 0; + let mut next_deposit_index = 0; + let mut validator_deposits_to_process = HashMap::new(); + + let pending_balance_deposits = state.pending_balance_deposits()?; + + for deposit in pending_balance_deposits.iter() { + if processed_amount.safe_add(deposit.amount)? > available_for_processing { + break; + } + validator_deposits_to_process + .entry(deposit.index as usize) + .or_insert(0) + .safe_add_assign(deposit.amount)?; + + processed_amount.safe_add_assign(deposit.amount)?; + next_deposit_index.safe_add_assign(1)?; + } + + let deposit_balance_to_consume = if next_deposit_index == pending_balance_deposits.len() { + 0 + } else { + available_for_processing.safe_sub(processed_amount)? + }; + + Ok(Self { + next_deposit_index, + deposit_balance_to_consume, + validator_deposits_to_process, + }) + } +} + +fn process_pending_balance_deposits_for_validator( + balance: &mut Cow, + validator_info: &ValidatorInfo, + pending_balance_deposits_ctxt: &PendingBalanceDepositsContext, +) -> Result<(), Error> { + if let Some(deposit_amount) = pending_balance_deposits_ctxt + .validator_deposits_to_process + .get(&validator_info.index) + { + balance.make_mut()?.safe_add_assign(*deposit_amount)?; + } + Ok(()) +} + +/// We process pending consolidations after all of single-pass epoch processing, and then patch up +/// the effective balances for affected validators. +/// +/// This is safe because processing consolidations does not depend on the `effective_balance`. +fn process_pending_consolidations( + state: &mut BeaconState, + next_epoch_cache: &mut PreEpochCache, + effective_balances_ctxt: &EffectiveBalancesContext, + state_ctxt: &StateContext, + spec: &ChainSpec, +) -> Result<(), Error> { + let mut next_pending_consolidation: usize = 0; + let current_epoch = state.current_epoch(); + let pending_consolidations = state.pending_consolidations()?.clone(); + + let mut affected_validators = BTreeSet::new(); + + for pending_consolidation in &pending_consolidations { + let source_index = pending_consolidation.source_index as usize; + let target_index = pending_consolidation.target_index as usize; + let source_validator = state.get_validator(source_index)?; + if source_validator.slashed { + next_pending_consolidation.safe_add_assign(1)?; + continue; + } + if source_validator.withdrawable_epoch > current_epoch { + break; + } + + // Calculate the active balance while we have the source validator loaded. This is a safe + // reordering. + let source_balance = *state + .balances() + .get(source_index) + .ok_or(BeaconStateError::UnknownValidator(source_index))?; + let active_balance = + source_validator.get_active_balance(source_balance, spec, state_ctxt.fork_name); + + // Churn any target excess active balance of target and raise its max. + state.switch_to_compounding_validator(target_index, spec)?; + + // Move active balance to target. Excess balance is withdrawable. + decrease_balance(state, source_index, active_balance)?; + increase_balance(state, target_index, active_balance)?; + + affected_validators.insert(source_index); + affected_validators.insert(target_index); + + next_pending_consolidation.safe_add_assign(1)?; + } + + let new_pending_consolidations = List::try_from_iter( + state + .pending_consolidations()? + .iter_from(next_pending_consolidation)? + .cloned(), + )?; + *state.pending_consolidations_mut()? = new_pending_consolidations; + + // Re-process effective balance updates for validators affected by consolidations. + let (validators, balances, _, current_epoch_participation, _, progressive_balances, _, _) = + state.mutable_validator_fields()?; + for validator_index in affected_validators { + let balance = *balances + .get(validator_index) + .ok_or(BeaconStateError::UnknownValidator(validator_index))?; + let mut validator = validators + .get_cow(validator_index) + .ok_or(BeaconStateError::UnknownValidator(validator_index))?; + let validator_current_epoch_participation = *current_epoch_participation + .get(validator_index) + .ok_or(BeaconStateError::UnknownValidator(validator_index))?; + + process_single_effective_balance_update( + validator_index, + balance, + &mut validator, + validator_current_epoch_participation, + next_epoch_cache, + progressive_balances, + effective_balances_ctxt, + state_ctxt, + spec, + )?; + } + Ok(()) +} + impl EffectiveBalancesContext { fn new(spec: &ChainSpec) -> Result { let hysteresis_increment = spec @@ -584,18 +955,24 @@ impl EffectiveBalancesContext { } } +/// This function abstracts over phase0 and Electra effective balance processing. #[allow(clippy::too_many_arguments)] fn process_single_effective_balance_update( + validator_index: usize, balance: u64, validator: &mut Cow, - validator_info: &ValidatorInfo, - next_epoch_total_active_balance: &mut u64, + validator_current_epoch_participation: ParticipationFlags, next_epoch_cache: &mut PreEpochCache, progressive_balances: &mut ProgressiveBalancesCache, eb_ctxt: &EffectiveBalancesContext, state_ctxt: &StateContext, spec: &ChainSpec, ) -> Result<(), Error> { + // Use the higher effective balance limit if post-Electra and compounding withdrawal credentials + // are set. + let effective_balance_limit = + validator.get_validator_max_effective_balance(spec, state_ctxt.fork_name); + let old_effective_balance = validator.effective_balance; let new_effective_balance = if balance.safe_add(eb_ctxt.downward_threshold)? < validator.effective_balance @@ -606,15 +983,13 @@ fn process_single_effective_balance_update( { min( balance.safe_sub(balance.safe_rem(spec.effective_balance_increment)?)?, - spec.max_effective_balance, + effective_balance_limit, ) } else { validator.effective_balance }; - if validator.is_active_at(state_ctxt.next_epoch) { - next_epoch_total_active_balance.safe_add_assign(new_effective_balance)?; - } + let is_active_next_epoch = validator.is_active_at(state_ctxt.next_epoch); if new_effective_balance != old_effective_balance { validator.make_mut()?.effective_balance = new_effective_balance; @@ -623,14 +998,18 @@ fn process_single_effective_balance_update( // previous epoch once the epoch transition completes. progressive_balances.on_effective_balance_change( validator.slashed, - validator_info.current_epoch_participation, + validator_current_epoch_participation, old_effective_balance, new_effective_balance, )?; } - // Caching: update next epoch effective balances. - next_epoch_cache.push_effective_balance(new_effective_balance); + // Caching: update next epoch effective balances and total active balance. + next_epoch_cache.update_effective_balance( + validator_index, + new_effective_balance, + is_active_next_epoch, + )?; Ok(()) } diff --git a/consensus/types/src/chain_spec.rs b/consensus/types/src/chain_spec.rs index d2f5909396..5c131d77c9 100644 --- a/consensus/types/src/chain_spec.rs +++ b/consensus/types/src/chain_spec.rs @@ -393,13 +393,21 @@ impl ChainSpec { state: &BeaconState, ) -> u64 { let fork_name = state.fork_name_unchecked(); - if fork_name >= ForkName::Electra { + if fork_name.electra_enabled() { self.whistleblower_reward_quotient_electra } else { self.whistleblower_reward_quotient } } + pub fn max_effective_balance_for_fork(&self, fork_name: ForkName) -> u64 { + if fork_name.electra_enabled() { + self.max_effective_balance_electra + } else { + self.max_effective_balance + } + } + /// Returns a full `Fork` struct for a given epoch. pub fn fork_at_epoch(&self, epoch: Epoch) -> Fork { let current_fork_name = self.fork_name_at_epoch(epoch); diff --git a/consensus/types/src/validator.rs b/consensus/types/src/validator.rs index 0054e95f9d..b5e92d1f5d 100644 --- a/consensus/types/src/validator.rs +++ b/consensus/types/src/validator.rs @@ -1,6 +1,6 @@ use crate::{ - test_utils::TestRandom, Address, BeaconState, ChainSpec, Epoch, EthSpec, ForkName, Hash256, - PublicKeyBytes, + test_utils::TestRandom, Address, BeaconState, ChainSpec, Checkpoint, Epoch, EthSpec, ForkName, + Hash256, PublicKeyBytes, }; use serde::{Deserialize, Serialize}; use ssz_derive::{Decode, Encode}; @@ -87,15 +87,25 @@ impl Validator { } /// Returns `true` if the validator is eligible to be activated. - /// - /// Spec v0.12.1 pub fn is_eligible_for_activation( &self, state: &BeaconState, spec: &ChainSpec, + ) -> bool { + self.is_eligible_for_activation_with_finalized_checkpoint( + &state.finalized_checkpoint(), + spec, + ) + } + + /// Returns `true` if the validator is eligible to be activated. + pub fn is_eligible_for_activation_with_finalized_checkpoint( + &self, + finalized_checkpoint: &Checkpoint, + spec: &ChainSpec, ) -> bool { // Placement in queue is finalized - self.activation_eligibility_epoch <= state.finalized_checkpoint().epoch + self.activation_eligibility_epoch <= finalized_checkpoint.epoch // Has not yet been activated && self.activation_epoch == spec.far_future_epoch } @@ -255,6 +265,16 @@ impl Validator { spec.max_effective_balance } } + + pub fn get_active_balance( + &self, + validator_balance: u64, + spec: &ChainSpec, + current_fork: ForkName, + ) -> u64 { + let max_effective_balance = self.get_validator_max_effective_balance(spec, current_fork); + std::cmp::min(validator_balance, max_effective_balance) + } } impl Default for Validator { From 70bcba1e6b7a7123b2f48e1c77b9afb9bbb11c26 Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Mon, 1 Jul 2024 03:36:40 +0200 Subject: [PATCH 10/23] Redb slasher backend impl (#4529) * initial redb impl * redb impl * remove phantom data * fixed table definition * fighting the borrow checker * a rough draft that doesnt cause lifetime issues * refactoring * refactor * refactor * passing unit tests * refactor * refactor * refactor * commit * move everything to one database * remove panics, ready for a review * merge * a working redb impl * passing a ref of txn to cursor * this tries to create a second write transaction when initializing cursor. breaks everything * Use 2 lifetimes and subtyping Also fixes a bug in last_key caused by rev and next_back cancelling out * Move table into cursor * Merge remote-tracking branch 'origin/unstable' into redb-slasher-backend-impl * changes based on feedback * update lmdb * fix lifetime issues * moving everything from Cursor to Transaction * update * upgrade to redb 2.0 * Merge branch 'unstable' of https://github.com/sigp/lighthouse into redb-slasher-backend-impl * bring back cursor * Merge branch 'unstable' of https://github.com/sigp/lighthouse into redb-slasher-backend-impl * fix delete while * linting * linting * switch to lmdb * update redb to v2.1 * build fixes, remove unwrap or default * another build error * hopefully this is the last build error * fmt * cargo.toml * fix mdbx * Merge branch 'unstable' of https://github.com/sigp/lighthouse into redb-slasher-backend-impl * Remove a collect * Merge remote-tracking branch 'origin/unstable' into redb-slasher-backend-impl * Merge branch 'redb-slasher-backend-impl' of https://github.com/eserilev/lighthouse into redb-slasher-backend-impl * re-enable test * fix failing slasher test * Merge remote-tracking branch 'origin/unstable' into redb-slasher-backend-impl * Rename DB file to `slasher.redb` --- Cargo.lock | 11 ++ Makefile | 2 +- lighthouse/Cargo.toml | 2 + lighthouse/tests/beacon_node.rs | 2 + slasher/Cargo.toml | 4 + slasher/src/config.rs | 9 +- slasher/src/database.rs | 61 +++--- slasher/src/database/interface.rs | 86 ++++++--- slasher/src/database/lmdb_impl.rs | 25 ++- slasher/src/database/mdbx_impl.rs | 25 ++- slasher/src/database/redb_impl.rs | 276 ++++++++++++++++++++++++++++ slasher/src/error.rs | 38 ++++ slasher/src/lib.rs | 2 +- slasher/tests/attester_slashings.rs | 2 +- slasher/tests/backend.rs | 20 +- slasher/tests/proposer_slashings.rs | 2 +- slasher/tests/random.rs | 2 +- slasher/tests/wrap_around.rs | 2 +- 18 files changed, 499 insertions(+), 72 deletions(-) create mode 100644 slasher/src/database/redb_impl.rs diff --git a/Cargo.lock b/Cargo.lock index d0ada21c80..f23777bc4c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6678,6 +6678,15 @@ dependencies = [ "yasna", ] +[[package]] +name = "redb" +version = "2.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ed7508e692a49b6b2290b56540384ccae9b1fb4d77065640b165835b56ffe3bb" +dependencies = [ + "libc", +] + [[package]] name = "redox_syscall" version = "0.2.16" @@ -7635,6 +7644,7 @@ version = "0.1.0" dependencies = [ "bincode", "byteorder", + "derivative", "ethereum_ssz", "ethereum_ssz_derive", "filesystem", @@ -7650,6 +7660,7 @@ dependencies = [ "parking_lot 0.12.3", "rand", "rayon", + "redb", "safe_arith", "serde", "slog", diff --git a/Makefile b/Makefile index 3f8e688df1..7d144e55fb 100644 --- a/Makefile +++ b/Makefile @@ -15,7 +15,7 @@ PINNED_NIGHTLY ?= nightly CLIPPY_PINNED_NIGHTLY=nightly-2022-05-19 # List of features to use when cross-compiling. Can be overridden via the environment. -CROSS_FEATURES ?= gnosis,slasher-lmdb,slasher-mdbx,jemalloc +CROSS_FEATURES ?= gnosis,slasher-lmdb,slasher-mdbx,slasher-redb,jemalloc # Cargo profile for Cross builds. Default is for local builds, CI uses an override. CROSS_PROFILE ?= release diff --git a/lighthouse/Cargo.toml b/lighthouse/Cargo.toml index 67c3dc260e..912602776a 100644 --- a/lighthouse/Cargo.toml +++ b/lighthouse/Cargo.toml @@ -22,6 +22,8 @@ gnosis = [] slasher-mdbx = ["slasher/mdbx"] # Support slasher LMDB backend. slasher-lmdb = ["slasher/lmdb"] +# Support slasher redb backend. +slasher-redb = ["slasher/redb"] # Deprecated. This is now enabled by default on non windows targets. jemalloc = [] diff --git a/lighthouse/tests/beacon_node.rs b/lighthouse/tests/beacon_node.rs index caadf208e3..cd499f2ada 100644 --- a/lighthouse/tests/beacon_node.rs +++ b/lighthouse/tests/beacon_node.rs @@ -2242,6 +2242,8 @@ fn slasher_broadcast_flag_false() { assert!(!slasher_config.broadcast); }); } + +#[cfg(all(feature = "lmdb"))] #[test] fn slasher_backend_override_to_default() { // Hard to test this flag because all but one backend is disabled by default and the backend diff --git a/slasher/Cargo.toml b/slasher/Cargo.toml index 563c4599d8..01a8b9fb00 100644 --- a/slasher/Cargo.toml +++ b/slasher/Cargo.toml @@ -8,11 +8,13 @@ edition = { workspace = true } default = ["lmdb"] mdbx = ["dep:mdbx"] lmdb = ["lmdb-rkv", "lmdb-rkv-sys"] +redb = ["dep:redb"] portable = ["types/portable"] [dependencies] bincode = { workspace = true } byteorder = { workspace = true } +derivative = { workspace = true } ethereum_ssz = { workspace = true } ethereum_ssz_derive = { workspace = true } flate2 = { version = "1.0.14", features = ["zlib"], default-features = false } @@ -36,6 +38,8 @@ mdbx = { package = "libmdbx", git = "https://github.com/sigp/libmdbx-rs", tag = lmdb-rkv = { git = "https://github.com/sigp/lmdb-rs", rev = "f33845c6469b94265319aac0ed5085597862c27e", optional = true } lmdb-rkv-sys = { git = "https://github.com/sigp/lmdb-rs", rev = "f33845c6469b94265319aac0ed5085597862c27e", optional = true } +redb = { version = "2.1", optional = true } + [dev-dependencies] maplit = { workspace = true } rayon = { workspace = true } diff --git a/slasher/src/config.rs b/slasher/src/config.rs index 1851e2e441..33d68fa0e5 100644 --- a/slasher/src/config.rs +++ b/slasher/src/config.rs @@ -15,16 +15,19 @@ pub const DEFAULT_MAX_DB_SIZE: usize = 512 * 1024; // 512 GiB pub const DEFAULT_ATTESTATION_ROOT_CACHE_SIZE: NonZeroUsize = new_non_zero_usize(100_000); pub const DEFAULT_BROADCAST: bool = false; -#[cfg(all(feature = "mdbx", not(feature = "lmdb")))] +#[cfg(all(feature = "mdbx", not(any(feature = "lmdb", feature = "redb"))))] pub const DEFAULT_BACKEND: DatabaseBackend = DatabaseBackend::Mdbx; #[cfg(feature = "lmdb")] pub const DEFAULT_BACKEND: DatabaseBackend = DatabaseBackend::Lmdb; -#[cfg(not(any(feature = "mdbx", feature = "lmdb")))] +#[cfg(all(feature = "redb", not(any(feature = "mdbx", feature = "lmdb"))))] +pub const DEFAULT_BACKEND: DatabaseBackend = DatabaseBackend::Redb; +#[cfg(not(any(feature = "mdbx", feature = "lmdb", feature = "redb")))] pub const DEFAULT_BACKEND: DatabaseBackend = DatabaseBackend::Disabled; pub const MAX_HISTORY_LENGTH: usize = 1 << 16; pub const MEGABYTE: usize = 1 << 20; pub const MDBX_DATA_FILENAME: &str = "mdbx.dat"; +pub const REDB_DATA_FILENAME: &str = "slasher.redb"; #[derive(Debug, Clone, Serialize, Deserialize)] pub struct Config { @@ -64,6 +67,8 @@ pub enum DatabaseBackend { Mdbx, #[cfg(feature = "lmdb")] Lmdb, + #[cfg(feature = "redb")] + Redb, Disabled, } diff --git a/slasher/src/database.rs b/slasher/src/database.rs index 801abe9283..4f4729a123 100644 --- a/slasher/src/database.rs +++ b/slasher/src/database.rs @@ -1,6 +1,7 @@ pub mod interface; mod lmdb_impl; mod mdbx_impl; +mod redb_impl; use crate::{ metrics, AttesterRecord, AttesterSlashingStatus, CompactAttesterRecord, Config, Error, @@ -489,8 +490,7 @@ impl SlasherDB { } // Store the new indexed attestation at the end of the current table. - let db = &self.databases.indexed_attestation_db; - let mut cursor = txn.cursor(db)?; + let mut cursor = txn.cursor(&self.databases.indexed_attestation_db)?; let indexed_att_id = match cursor.last_key()? { // First ID is 1 so that 0 can be used to represent `null` in `CompactAttesterRecord`. @@ -504,7 +504,6 @@ impl SlasherDB { cursor.put(attestation_key.as_ref(), &data)?; drop(cursor); - // Update the (epoch, hash) to ID mapping. self.put_indexed_attestation_id(txn, &id_key, attestation_key)?; @@ -743,21 +742,17 @@ impl SlasherDB { return Ok(()); } - loop { - let (key_bytes, _) = cursor.get_current()?.ok_or(Error::MissingProposerKey)?; - - let (slot, _) = ProposerKey::parse(key_bytes)?; + let should_delete = |key: &[u8]| -> Result { + let mut should_delete = false; + let (slot, _) = ProposerKey::parse(Cow::from(key))?; if slot < min_slot { - cursor.delete_current()?; - - // End the loop if there is no next entry. - if cursor.next_key()?.is_none() { - break; - } - } else { - break; + should_delete = true; } - } + + Ok(should_delete) + }; + + cursor.delete_while(should_delete)?; Ok(()) } @@ -771,9 +766,6 @@ impl SlasherDB { .saturating_add(1u64) .saturating_sub(self.config.history_length as u64); - // Collect indexed attestation IDs to delete. - let mut indexed_attestation_ids = vec![]; - let mut cursor = txn.cursor(&self.databases.indexed_attestation_id_db)?; // Position cursor at first key, bailing out if the database is empty. @@ -781,27 +773,20 @@ impl SlasherDB { return Ok(()); } - loop { - let (key_bytes, value) = cursor - .get_current()? - .ok_or(Error::MissingIndexedAttestationIdKey)?; - - let (target_epoch, _) = IndexedAttestationIdKey::parse(key_bytes)?; - + let should_delete = |key: &[u8]| -> Result { + let (target_epoch, _) = IndexedAttestationIdKey::parse(Cow::from(key))?; if target_epoch < min_epoch { - indexed_attestation_ids.push(IndexedAttestationId::new( - IndexedAttestationId::parse(value)?, - )); - - cursor.delete_current()?; - - if cursor.next_key()?.is_none() { - break; - } - } else { - break; + return Ok(true); } - } + + Ok(false) + }; + + let indexed_attestation_ids = cursor + .delete_while(should_delete)? + .into_iter() + .map(|id| IndexedAttestationId::parse(id).map(IndexedAttestationId::new)) + .collect::, Error>>()?; drop(cursor); // Delete the indexed attestations. diff --git a/slasher/src/database/interface.rs b/slasher/src/database/interface.rs index 5bb920383c..46cf9a4a0c 100644 --- a/slasher/src/database/interface.rs +++ b/slasher/src/database/interface.rs @@ -7,6 +7,8 @@ use std::path::PathBuf; use crate::database::lmdb_impl; #[cfg(feature = "mdbx")] use crate::database::mdbx_impl; +#[cfg(feature = "redb")] +use crate::database::redb_impl; #[derive(Debug)] pub enum Environment { @@ -14,6 +16,8 @@ pub enum Environment { Mdbx(mdbx_impl::Environment), #[cfg(feature = "lmdb")] Lmdb(lmdb_impl::Environment), + #[cfg(feature = "redb")] + Redb(redb_impl::Environment), Disabled, } @@ -23,6 +27,8 @@ pub enum RwTransaction<'env> { Mdbx(mdbx_impl::RwTransaction<'env>), #[cfg(feature = "lmdb")] Lmdb(lmdb_impl::RwTransaction<'env>), + #[cfg(feature = "redb")] + Redb(redb_impl::RwTransaction<'env>), Disabled(PhantomData<&'env ()>), } @@ -32,6 +38,8 @@ pub enum Database<'env> { Mdbx(mdbx_impl::Database<'env>), #[cfg(feature = "lmdb")] Lmdb(lmdb_impl::Database<'env>), + #[cfg(feature = "redb")] + Redb(redb_impl::Database<'env>), Disabled(PhantomData<&'env ()>), } @@ -54,6 +62,8 @@ pub enum Cursor<'env> { Mdbx(mdbx_impl::Cursor<'env>), #[cfg(feature = "lmdb")] Lmdb(lmdb_impl::Cursor<'env>), + #[cfg(feature = "redb")] + Redb(redb_impl::Cursor<'env>), Disabled(PhantomData<&'env ()>), } @@ -67,6 +77,8 @@ impl Environment { DatabaseBackend::Mdbx => mdbx_impl::Environment::new(config).map(Environment::Mdbx), #[cfg(feature = "lmdb")] DatabaseBackend::Lmdb => lmdb_impl::Environment::new(config).map(Environment::Lmdb), + #[cfg(feature = "redb")] + DatabaseBackend::Redb => redb_impl::Environment::new(config).map(Environment::Redb), DatabaseBackend::Disabled => Err(Error::SlasherDatabaseBackendDisabled), } } @@ -77,6 +89,8 @@ impl Environment { Self::Mdbx(env) => env.create_databases(), #[cfg(feature = "lmdb")] Self::Lmdb(env) => env.create_databases(), + #[cfg(feature = "redb")] + Self::Redb(env) => env.create_databases(), _ => Err(Error::MismatchedDatabaseVariant), } } @@ -87,6 +101,8 @@ impl Environment { Self::Mdbx(env) => env.begin_rw_txn().map(RwTransaction::Mdbx), #[cfg(feature = "lmdb")] Self::Lmdb(env) => env.begin_rw_txn().map(RwTransaction::Lmdb), + #[cfg(feature = "redb")] + Self::Redb(env) => env.begin_rw_txn().map(RwTransaction::Redb), _ => Err(Error::MismatchedDatabaseVariant), } } @@ -98,6 +114,8 @@ impl Environment { Self::Mdbx(env) => env.filenames(config), #[cfg(feature = "lmdb")] Self::Lmdb(env) => env.filenames(config), + #[cfg(feature = "redb")] + Self::Redb(env) => env.filenames(config), _ => vec![], } } @@ -106,7 +124,7 @@ impl Environment { impl<'env> RwTransaction<'env> { pub fn get + ?Sized>( &'env self, - db: &Database<'env>, + db: &'env Database, key: &K, ) -> Result>, Error> { match (self, db) { @@ -114,6 +132,8 @@ impl<'env> RwTransaction<'env> { (Self::Mdbx(txn), Database::Mdbx(db)) => txn.get(db, key), #[cfg(feature = "lmdb")] (Self::Lmdb(txn), Database::Lmdb(db)) => txn.get(db, key), + #[cfg(feature = "redb")] + (Self::Redb(txn), Database::Redb(db)) => txn.get(db, key), _ => Err(Error::MismatchedDatabaseVariant), } } @@ -129,6 +149,8 @@ impl<'env> RwTransaction<'env> { (Self::Mdbx(txn), Database::Mdbx(db)) => txn.put(db, key, value), #[cfg(feature = "lmdb")] (Self::Lmdb(txn), Database::Lmdb(db)) => txn.put(db, key, value), + #[cfg(feature = "redb")] + (Self::Redb(txn), Database::Redb(db)) => txn.put(db, key, value), _ => Err(Error::MismatchedDatabaseVariant), } } @@ -139,16 +161,8 @@ impl<'env> RwTransaction<'env> { (Self::Mdbx(txn), Database::Mdbx(db)) => txn.del(db, key), #[cfg(feature = "lmdb")] (Self::Lmdb(txn), Database::Lmdb(db)) => txn.del(db, key), - _ => Err(Error::MismatchedDatabaseVariant), - } - } - - pub fn cursor<'a>(&'a mut self, db: &Database) -> Result, Error> { - match (self, db) { - #[cfg(feature = "mdbx")] - (Self::Mdbx(txn), Database::Mdbx(db)) => txn.cursor(db).map(Cursor::Mdbx), - #[cfg(feature = "lmdb")] - (Self::Lmdb(txn), Database::Lmdb(db)) => txn.cursor(db).map(Cursor::Lmdb), + #[cfg(feature = "redb")] + (Self::Redb(txn), Database::Redb(db)) => txn.del(db, key), _ => Err(Error::MismatchedDatabaseVariant), } } @@ -159,6 +173,20 @@ impl<'env> RwTransaction<'env> { Self::Mdbx(txn) => txn.commit(), #[cfg(feature = "lmdb")] Self::Lmdb(txn) => txn.commit(), + #[cfg(feature = "redb")] + Self::Redb(txn) => txn.commit(), + _ => Err(Error::MismatchedDatabaseVariant), + } + } + + pub fn cursor<'a>(&'a mut self, db: &'a Database) -> Result, Error> { + match (self, db) { + #[cfg(feature = "mdbx")] + (Self::Mdbx(txn), Database::Mdbx(db)) => txn.cursor(db).map(Cursor::Mdbx), + #[cfg(feature = "lmdb")] + (Self::Lmdb(txn), Database::Lmdb(db)) => txn.cursor(db).map(Cursor::Lmdb), + #[cfg(feature = "redb")] + (Self::Redb(txn), Database::Redb(db)) => txn.cursor(db).map(Cursor::Redb), _ => Err(Error::MismatchedDatabaseVariant), } } @@ -172,6 +200,8 @@ impl<'env> Cursor<'env> { Cursor::Mdbx(cursor) => cursor.first_key(), #[cfg(feature = "lmdb")] Cursor::Lmdb(cursor) => cursor.first_key(), + #[cfg(feature = "redb")] + Cursor::Redb(cursor) => cursor.first_key(), _ => Err(Error::MismatchedDatabaseVariant), } } @@ -183,6 +213,8 @@ impl<'env> Cursor<'env> { Cursor::Mdbx(cursor) => cursor.last_key(), #[cfg(feature = "lmdb")] Cursor::Lmdb(cursor) => cursor.last_key(), + #[cfg(feature = "redb")] + Cursor::Redb(cursor) => cursor.last_key(), _ => Err(Error::MismatchedDatabaseVariant), } } @@ -193,17 +225,8 @@ impl<'env> Cursor<'env> { Cursor::Mdbx(cursor) => cursor.next_key(), #[cfg(feature = "lmdb")] Cursor::Lmdb(cursor) => cursor.next_key(), - _ => Err(Error::MismatchedDatabaseVariant), - } - } - - /// Get the key value pair at the current position. - pub fn get_current(&mut self) -> Result, Error> { - match self { - #[cfg(feature = "mdbx")] - Cursor::Mdbx(cursor) => cursor.get_current(), - #[cfg(feature = "lmdb")] - Cursor::Lmdb(cursor) => cursor.get_current(), + #[cfg(feature = "redb")] + Cursor::Redb(cursor) => cursor.next_key(), _ => Err(Error::MismatchedDatabaseVariant), } } @@ -214,6 +237,8 @@ impl<'env> Cursor<'env> { Cursor::Mdbx(cursor) => cursor.delete_current(), #[cfg(feature = "lmdb")] Cursor::Lmdb(cursor) => cursor.delete_current(), + #[cfg(feature = "redb")] + Cursor::Redb(cursor) => cursor.delete_current(), _ => Err(Error::MismatchedDatabaseVariant), } } @@ -224,6 +249,23 @@ impl<'env> Cursor<'env> { Self::Mdbx(cursor) => cursor.put(key, value), #[cfg(feature = "lmdb")] Self::Lmdb(cursor) => cursor.put(key, value), + #[cfg(feature = "redb")] + Self::Redb(cursor) => cursor.put(key, value), + _ => Err(Error::MismatchedDatabaseVariant), + } + } + + pub fn delete_while( + &mut self, + f: impl Fn(&[u8]) -> Result, + ) -> Result>, Error> { + match self { + #[cfg(feature = "mdbx")] + Self::Mdbx(txn) => txn.delete_while(f), + #[cfg(feature = "lmdb")] + Self::Lmdb(txn) => txn.delete_while(f), + #[cfg(feature = "redb")] + Self::Redb(txn) => txn.delete_while(f), _ => Err(Error::MismatchedDatabaseVariant), } } diff --git a/slasher/src/database/lmdb_impl.rs b/slasher/src/database/lmdb_impl.rs index 78deaf1767..20d89a36fb 100644 --- a/slasher/src/database/lmdb_impl.rs +++ b/slasher/src/database/lmdb_impl.rs @@ -100,7 +100,7 @@ impl Environment { impl<'env> RwTransaction<'env> { pub fn get + ?Sized>( &'env self, - db: &Database<'env>, + db: &'env Database, key: &K, ) -> Result>, Error> { Ok(self.txn.get(db.db, key).optional()?.map(Cow::Borrowed)) @@ -182,6 +182,29 @@ impl<'env> Cursor<'env> { .put(&key, &value, RwTransaction::write_flags())?; Ok(()) } + + pub fn delete_while( + &mut self, + f: impl Fn(&[u8]) -> Result, + ) -> Result>, Error> { + let mut result = vec![]; + + loop { + let (key_bytes, value) = self.get_current()?.ok_or(Error::MissingKey)?; + + if f(&key_bytes)? { + result.push(value); + self.delete_current()?; + if self.next_key()?.is_none() { + break; + } + } else { + break; + } + } + + Ok(result) + } } /// Mix-in trait for loading values from LMDB that may or may not exist. diff --git a/slasher/src/database/mdbx_impl.rs b/slasher/src/database/mdbx_impl.rs index d25f17e7ac..e249de963f 100644 --- a/slasher/src/database/mdbx_impl.rs +++ b/slasher/src/database/mdbx_impl.rs @@ -113,7 +113,7 @@ impl<'env> RwTransaction<'env> { pub fn get + ?Sized>( &'env self, - db: &Database<'env>, + db: &'env Database, key: &K, ) -> Result>, Error> { Ok(self.txn.get(&db.db, key.as_ref())?) @@ -183,4 +183,27 @@ impl<'env> Cursor<'env> { .put(key.as_ref(), value.as_ref(), RwTransaction::write_flags())?; Ok(()) } + + pub fn delete_while( + &mut self, + f: impl Fn(&[u8]) -> Result, + ) -> Result>, Error> { + let mut result = vec![]; + + loop { + let (key_bytes, value) = self.get_current()?.ok_or(Error::MissingKey)?; + + if f(&key_bytes)? { + result.push(value); + self.delete_current()?; + if self.next_key()?.is_none() { + break; + } + } else { + break; + } + } + + Ok(result) + } } diff --git a/slasher/src/database/redb_impl.rs b/slasher/src/database/redb_impl.rs new file mode 100644 index 0000000000..da7b4e38ed --- /dev/null +++ b/slasher/src/database/redb_impl.rs @@ -0,0 +1,276 @@ +#![cfg(feature = "redb")] +use crate::{ + config::REDB_DATA_FILENAME, + database::{ + interface::{Key, OpenDatabases, Value}, + *, + }, + Config, Error, +}; +use derivative::Derivative; +use redb::{ReadableTable, TableDefinition}; +use std::{borrow::Cow, path::PathBuf}; + +#[derive(Debug)] +pub struct Environment { + _db_count: usize, + db: redb::Database, +} + +#[derive(Debug)] +pub struct Database<'env> { + table_name: String, + _phantom: PhantomData<&'env ()>, +} + +#[derive(Derivative)] +#[derivative(Debug)] +pub struct RwTransaction<'env> { + #[derivative(Debug = "ignore")] + txn: redb::WriteTransaction, + _phantom: PhantomData<&'env ()>, +} + +#[derive(Derivative)] +#[derivative(Debug)] +pub struct Cursor<'env> { + #[derivative(Debug = "ignore")] + txn: &'env redb::WriteTransaction, + db: &'env Database<'env>, + current_key: Option>, +} + +impl Environment { + pub fn new(config: &Config) -> Result { + let db_path = config.database_path.join(REDB_DATA_FILENAME); + let database = redb::Database::create(db_path)?; + + Ok(Environment { + _db_count: MAX_NUM_DBS, + db: database, + }) + } + + pub fn create_databases(&self) -> Result { + let indexed_attestation_db = self.create_table(INDEXED_ATTESTATION_DB)?; + let indexed_attestation_id_db = self.create_table(INDEXED_ATTESTATION_ID_DB)?; + let attesters_db = self.create_table(ATTESTERS_DB)?; + let attesters_max_targets_db = self.create_table(ATTESTERS_MAX_TARGETS_DB)?; + let min_targets_db = self.create_table(MIN_TARGETS_DB)?; + let max_targets_db = self.create_table(MAX_TARGETS_DB)?; + let current_epochs_db = self.create_table(CURRENT_EPOCHS_DB)?; + let proposers_db = self.create_table(PROPOSERS_DB)?; + let metadata_db = self.create_table(METADATA_DB)?; + + Ok(OpenDatabases { + indexed_attestation_db, + indexed_attestation_id_db, + attesters_db, + attesters_max_targets_db, + min_targets_db, + max_targets_db, + current_epochs_db, + proposers_db, + metadata_db, + }) + } + + pub fn create_table<'env>( + &'env self, + table_name: &'env str, + ) -> Result, Error> { + let table_definition: TableDefinition<'_, &[u8], &[u8]> = TableDefinition::new(table_name); + let tx = self.db.begin_write()?; + tx.open_table(table_definition)?; + tx.commit()?; + + Ok(crate::Database::Redb(Database { + table_name: table_name.to_string(), + _phantom: PhantomData, + })) + } + + pub fn filenames(&self, config: &Config) -> Vec { + vec![config.database_path.join(BASE_DB)] + } + + pub fn begin_rw_txn(&self) -> Result { + let mut txn = self.db.begin_write()?; + txn.set_durability(redb::Durability::Eventual); + Ok(RwTransaction { + txn, + _phantom: PhantomData, + }) + } +} + +impl<'env> RwTransaction<'env> { + pub fn get + ?Sized>( + &'env self, + db: &'env Database, + key: &K, + ) -> Result>, Error> { + let table_definition: TableDefinition<'_, &[u8], &[u8]> = + TableDefinition::new(&db.table_name); + let table = self.txn.open_table(table_definition)?; + let result = table.get(key.as_ref())?; + if let Some(access_guard) = result { + let value = access_guard.value().to_vec(); + Ok(Some(Cow::from(value))) + } else { + Ok(None) + } + } + + pub fn put, V: AsRef<[u8]>>( + &mut self, + db: &Database, + key: K, + value: V, + ) -> Result<(), Error> { + let table_definition: TableDefinition<'_, &[u8], &[u8]> = + TableDefinition::new(&db.table_name); + let mut table = self.txn.open_table(table_definition)?; + table.insert(key.as_ref(), value.as_ref())?; + + Ok(()) + } + + pub fn del>(&mut self, db: &Database, key: K) -> Result<(), Error> { + let table_definition: TableDefinition<'_, &[u8], &[u8]> = + TableDefinition::new(&db.table_name); + let mut table = self.txn.open_table(table_definition)?; + table.remove(key.as_ref())?; + + Ok(()) + } + + pub fn commit(self) -> Result<(), Error> { + self.txn.commit()?; + Ok(()) + } + + pub fn cursor<'a>(&'a mut self, db: &'a Database) -> Result, Error> { + Ok(Cursor { + txn: &self.txn, + db, + current_key: None, + }) + } +} + +impl<'env> Cursor<'env> { + pub fn first_key(&mut self) -> Result, Error> { + let table_definition: TableDefinition<'_, &[u8], &[u8]> = + TableDefinition::new(&self.db.table_name); + let table = self.txn.open_table(table_definition)?; + let first = table + .iter()? + .next() + .map(|x| x.map(|(key, _)| key.value().to_vec())); + + if let Some(owned_key) = first { + let owned_key = owned_key?; + self.current_key = Some(Cow::from(owned_key)); + Ok(self.current_key.clone()) + } else { + Ok(None) + } + } + + pub fn last_key(&mut self) -> Result>, Error> { + let table_definition: TableDefinition<'_, &[u8], &[u8]> = + TableDefinition::new(&self.db.table_name); + let table = self.txn.open_table(table_definition)?; + let last = table + .iter()? + .next_back() + .map(|x| x.map(|(key, _)| key.value().to_vec())); + + if let Some(owned_key) = last { + let owned_key = owned_key?; + self.current_key = Some(Cow::from(owned_key)); + return Ok(self.current_key.clone()); + } + Ok(None) + } + + pub fn get_current(&self) -> Result, Value<'env>)>, Error> { + let table_definition: TableDefinition<'_, &[u8], &[u8]> = + TableDefinition::new(&self.db.table_name); + let table = self.txn.open_table(table_definition)?; + if let Some(key) = &self.current_key { + let result = table.get(key.as_ref())?; + + if let Some(access_guard) = result { + let value = access_guard.value().to_vec(); + return Ok(Some((key.clone(), Cow::from(value)))); + } + } + Ok(None) + } + + pub fn next_key(&mut self) -> Result>, Error> { + let table_definition: TableDefinition<'_, &[u8], &[u8]> = + TableDefinition::new(&self.db.table_name); + let table = self.txn.open_table(table_definition)?; + if let Some(current_key) = &self.current_key { + let range: std::ops::RangeFrom<&[u8]> = current_key..; + + let next = table + .range(range)? + .next() + .map(|x| x.map(|(key, _)| key.value().to_vec())); + + if let Some(owned_key) = next { + let owned_key = owned_key?; + self.current_key = Some(Cow::from(owned_key)); + return Ok(self.current_key.clone()); + } + } + Ok(None) + } + + pub fn delete_current(&self) -> Result<(), Error> { + let table_definition: TableDefinition<'_, &[u8], &[u8]> = + TableDefinition::new(&self.db.table_name); + let mut table = self.txn.open_table(table_definition)?; + if let Some(key) = &self.current_key { + table.remove(key.as_ref())?; + } + Ok(()) + } + + pub fn delete_while( + &self, + f: impl Fn(&[u8]) -> Result, + ) -> Result>, Error> { + let mut deleted_values = vec![]; + if let Some(current_key) = &self.current_key { + let table_definition: TableDefinition<'_, &[u8], &[u8]> = + TableDefinition::new(&self.db.table_name); + + let mut table = self.txn.open_table(table_definition)?; + + let deleted = + table.extract_from_if(current_key.as_ref().., |key, _| f(key).unwrap_or(false))?; + + deleted.for_each(|result| { + if let Ok(item) = result { + let value = item.1.value().to_vec(); + deleted_values.push(Cow::from(value)); + } + }) + }; + Ok(deleted_values) + } + + pub fn put, V: AsRef<[u8]>>(&mut self, key: K, value: V) -> Result<(), Error> { + let table_definition: TableDefinition<'_, &[u8], &[u8]> = + TableDefinition::new(&self.db.table_name); + let mut table = self.txn.open_table(table_definition)?; + table.insert(key.as_ref(), value.as_ref())?; + + Ok(()) + } +} diff --git a/slasher/src/error.rs b/slasher/src/error.rs index 8d3295b22a..b2e32f3dcd 100644 --- a/slasher/src/error.rs +++ b/slasher/src/error.rs @@ -8,6 +8,8 @@ pub enum Error { DatabaseMdbxError(mdbx::Error), #[cfg(feature = "lmdb")] DatabaseLmdbError(lmdb::Error), + #[cfg(feature = "redb")] + DatabaseRedbError(redb::Error), SlasherDatabaseBackendDisabled, MismatchedDatabaseVariant, DatabaseIOError(io::Error), @@ -67,6 +69,7 @@ pub enum Error { MissingIndexedAttestationId, MissingIndexedAttestationIdKey, InconsistentAttestationDataRoot, + MissingKey, } #[cfg(feature = "mdbx")] @@ -89,6 +92,41 @@ impl From for Error { } } +#[cfg(feature = "redb")] +impl From for Error { + fn from(e: redb::TableError) -> Self { + Error::DatabaseRedbError(e.into()) + } +} + +#[cfg(feature = "redb")] +impl From for Error { + fn from(e: redb::TransactionError) -> Self { + Error::DatabaseRedbError(e.into()) + } +} + +#[cfg(feature = "redb")] +impl From for Error { + fn from(e: redb::DatabaseError) -> Self { + Error::DatabaseRedbError(e.into()) + } +} + +#[cfg(feature = "redb")] +impl From for Error { + fn from(e: redb::StorageError) -> Self { + Error::DatabaseRedbError(e.into()) + } +} + +#[cfg(feature = "redb")] +impl From for Error { + fn from(e: redb::CommitError) -> Self { + Error::DatabaseRedbError(e.into()) + } +} + impl From for Error { fn from(e: io::Error) -> Self { Error::DatabaseIOError(e) diff --git a/slasher/src/lib.rs b/slasher/src/lib.rs index 4d58fa7702..d3a26337d6 100644 --- a/slasher/src/lib.rs +++ b/slasher/src/lib.rs @@ -1,6 +1,6 @@ #![deny(missing_debug_implementations)] #![cfg_attr( - not(any(feature = "mdbx", feature = "lmdb")), + not(any(feature = "mdbx", feature = "lmdb", feature = "redb")), allow(unused, clippy::drop_non_drop) )] diff --git a/slasher/tests/attester_slashings.rs b/slasher/tests/attester_slashings.rs index 902141d971..cc6e57d95d 100644 --- a/slasher/tests/attester_slashings.rs +++ b/slasher/tests/attester_slashings.rs @@ -1,4 +1,4 @@ -#![cfg(any(feature = "mdbx", feature = "lmdb"))] +#![cfg(any(feature = "mdbx", feature = "lmdb", feature = "redb"))] use logging::test_logger; use maplit::hashset; diff --git a/slasher/tests/backend.rs b/slasher/tests/backend.rs index fd1a6ae14f..c24b861b18 100644 --- a/slasher/tests/backend.rs +++ b/slasher/tests/backend.rs @@ -1,4 +1,4 @@ -#![cfg(feature = "lmdb")] +#![cfg(any(feature = "lmdb", feature = "redb"))] use slasher::{config::MDBX_DATA_FILENAME, Config, DatabaseBackend, DatabaseBackendOverride}; use std::fs::File; @@ -41,7 +41,7 @@ fn no_override_with_existing_mdbx_db() { } #[test] -#[cfg(all(not(feature = "mdbx"), feature = "lmdb"))] +#[cfg(all(not(feature = "mdbx"), feature = "lmdb", not(feature = "redb")))] fn failed_override_with_existing_mdbx_db() { let tempdir = tempdir().unwrap(); let mut config = Config::new(tempdir.path().into()); @@ -55,3 +55,19 @@ fn failed_override_with_existing_mdbx_db() { ); assert_eq!(config.backend, DatabaseBackend::Lmdb); } + +#[test] +#[cfg(feature = "redb")] +fn failed_override_with_existing_mdbx_db() { + let tempdir = tempdir().unwrap(); + let mut config = Config::new(tempdir.path().into()); + + let filename = config.database_path.join(MDBX_DATA_FILENAME); + File::create(&filename).unwrap(); + + assert_eq!( + config.override_backend(), + DatabaseBackendOverride::Failure(filename) + ); + assert_eq!(config.backend, DatabaseBackend::Redb); +} diff --git a/slasher/tests/proposer_slashings.rs b/slasher/tests/proposer_slashings.rs index 2d2738087d..6d2a1f5176 100644 --- a/slasher/tests/proposer_slashings.rs +++ b/slasher/tests/proposer_slashings.rs @@ -1,4 +1,4 @@ -#![cfg(any(feature = "mdbx", feature = "lmdb"))] +#![cfg(any(feature = "mdbx", feature = "lmdb", feature = "redb"))] use logging::test_logger; use slasher::{ diff --git a/slasher/tests/random.rs b/slasher/tests/random.rs index ebfe0ef4e9..0aaaa63f65 100644 --- a/slasher/tests/random.rs +++ b/slasher/tests/random.rs @@ -1,4 +1,4 @@ -#![cfg(any(feature = "mdbx", feature = "lmdb"))] +#![cfg(any(feature = "mdbx", feature = "lmdb", feature = "redb"))] use logging::test_logger; use rand::prelude::*; diff --git a/slasher/tests/wrap_around.rs b/slasher/tests/wrap_around.rs index 9a42aeb60b..2ec56bc7d5 100644 --- a/slasher/tests/wrap_around.rs +++ b/slasher/tests/wrap_around.rs @@ -1,4 +1,4 @@ -#![cfg(any(feature = "mdbx", feature = "lmdb"))] +#![cfg(any(feature = "mdbx", feature = "lmdb", feature = "redb"))] use logging::test_logger; use slasher::{ From 3f2af5cf808210881125e8c3c5d8f4c3f3ea15ef Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Tue, 2 Jul 2024 02:06:59 +1000 Subject: [PATCH 11/23] Delete duplicated serde code (#6027) * Delete duplicated serde code --- consensus/types/src/indexed_attestation.rs | 41 ++-------------------- 1 file changed, 2 insertions(+), 39 deletions(-) diff --git a/consensus/types/src/indexed_attestation.rs b/consensus/types/src/indexed_attestation.rs index 19d1501075..9274600ed2 100644 --- a/consensus/types/src/indexed_attestation.rs +++ b/consensus/types/src/indexed_attestation.rs @@ -53,10 +53,10 @@ use tree_hash_derive::TreeHash; pub struct IndexedAttestation { /// Lists validator registry indices, not committee indices. #[superstruct(only(Base), partial_getter(rename = "attesting_indices_base"))] - #[serde(with = "quoted_variable_list_u64")] + #[serde(with = "ssz_types::serde_utils::quoted_u64_var_list")] pub attesting_indices: VariableList, #[superstruct(only(Electra), partial_getter(rename = "attesting_indices_electra"))] - #[serde(with = "quoted_variable_list_u64")] + #[serde(with = "ssz_types::serde_utils::quoted_u64_var_list")] pub attesting_indices: VariableList, pub data: AttestationData, pub signature: AggregateSignature, @@ -203,43 +203,6 @@ impl Hash for IndexedAttestation { } } -/// Serialize a variable list of `u64` such that each int is quoted. Deserialize a variable -/// list supporting both quoted and un-quoted ints. -/// -/// E.g.,`["0", "1", "2"]` -mod quoted_variable_list_u64 { - use super::*; - use crate::Unsigned; - use serde::ser::SerializeSeq; - use serde::{Deserializer, Serializer}; - use serde_utils::quoted_u64_vec::{QuotedIntVecVisitor, QuotedIntWrapper}; - - pub fn serialize(value: &VariableList, serializer: S) -> Result - where - S: Serializer, - T: Unsigned, - { - let mut seq = serializer.serialize_seq(Some(value.len()))?; - for &int in value.iter() { - seq.serialize_element(&QuotedIntWrapper { int })?; - } - seq.end() - } - - pub fn deserialize<'de, D, T>(deserializer: D) -> Result, D::Error> - where - D: Deserializer<'de>, - T: Unsigned, - { - deserializer - .deserialize_any(QuotedIntVecVisitor) - .and_then(|vec| { - VariableList::new(vec) - .map_err(|e| serde::de::Error::custom(format!("invalid length: {:?}", e))) - }) - } -} - #[cfg(test)] mod tests { use super::*; From 2a13b4f3fa678ce672f0ee30ab55c5d9f3fbac97 Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Mon, 1 Jul 2024 09:33:55 -0700 Subject: [PATCH 12/23] Electra engine api (#5743) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Attestation superstruct changes for EIP 7549 (#5644) * update * experiment * superstruct changes * revert * superstruct changes * fix tests * indexed attestation * indexed attestation superstruct * updated TODOs * `superstruct` the `AttesterSlashing` (#5636) * `superstruct` Attester Fork Variants * Push a little further * Deal with Encode / Decode of AttesterSlashing * not so sure about this.. * Stop Encode/Decode Bounds from Propagating Out * Tons of Changes.. * More Conversions to AttestationRef * Add AsReference trait (#15) * Add AsReference trait * Fix some snafus * Got it Compiling! :D * Got Tests Building * Get beacon chain tests compiling --------- Co-authored-by: Michael Sproul * Merge remote-tracking branch 'upstream/unstable' into electra_attestation_changes * Make EF Tests Fork-Agnostic (#5713) * Finish EF Test Fork Agnostic (#5714) * Superstruct `AggregateAndProof` (#5715) * Upgrade `superstruct` to `0.8.0` * superstruct `AggregateAndProof` * Merge remote-tracking branch 'sigp/unstable' into electra_attestation_changes * cargo fmt * Merge pull request #5726 from realbigsean/electra_attestation_changes Merge unstable into Electra attestation changes * process withdrawals updates * cleanup withdrawals processing * update `process_operations` deposit length check * add apply_deposit changes * add execution layer withdrawal request processing * process deposit receipts * add consolidation processing * update process operations function * exit updates * clean up * update slash_validator * EIP7549 `get_attestation_indices` (#5657) * get attesting indices electra impl * fmt * get tests to pass * fmt * fix some beacon chain tests * fmt * fix slasher test * fmt got me again * fix more tests * fix tests * Some small changes (#5739) * Merge branch 'electra_attestation_changes' of https://github.com/sigp/lighthouse into block-processing-electra * cargo fmt (#5740) * Merge branch 'electra_attestation_changes' of https://github.com/sigp/lighthouse into block-processing-electra * fix attestation verification * Add new engine api methods * Fix the versioning of v4 requests * Handle new engine api methods in mock EL * Note todo * Fix todos * Add support for electra fields in getPayloadBodies * Add comments for potential versioning confusion * Sketch op pool changes * fix get attesting indices (#5742) * fix get attesting indices * better errors * fix compile * only get committee index once * Merge branch 'electra_attestation_changes' of https://github.com/sigp/lighthouse into block-processing-electra * Ef test fixes (#5753) * attestation related ef test fixes * delete commented out stuff * Merge branch 'electra_attestation_changes' of https://github.com/sigp/lighthouse into block-processing-electra * Merge branch 'block-processing-electra' of https://github.com/sigp/lighthouse into electra-engine-api * Fix Aggregation Pool for Electra (#5754) * Fix Aggregation Pool for Electra * Remove Outdated Interface * fix ssz (#5755) * Get `electra_op_pool` up to date (#5756) * fix get attesting indices (#5742) * fix get attesting indices * better errors * fix compile * only get committee index once * Ef test fixes (#5753) * attestation related ef test fixes * delete commented out stuff * Fix Aggregation Pool for Electra (#5754) * Fix Aggregation Pool for Electra * Remove Outdated Interface * fix ssz (#5755) --------- Co-authored-by: realbigsean * Revert "Get `electra_op_pool` up to date (#5756)" (#5757) This reverts commit ab9e58aa3d0e6fe2175a4996a5de710e81152896. * Merge branch 'electra_attestation_changes' of https://github.com/sigp/lighthouse into electra_op_pool * Merge branch 'electra_attestation_changes' of https://github.com/sigp/lighthouse into block-processing-electra * Merge branch 'block-processing-electra' of https://github.com/sigp/lighthouse into electra-engine-api * Compute on chain aggregate impl (#5752) * add compute_on_chain_agg impl to op pool changes * fmt * get op pool tests to pass * update the naive agg pool interface (#5760) * Merge branch 'electra_attestation_changes' of https://github.com/sigp/lighthouse into block-processing-electra * Merge branch 'block-processing-electra' of https://github.com/sigp/lighthouse into electra-engine-api * Fix bugs in cross-committee aggregation * Add comment to max cover optimisation * Fix assert * Electra epoch processing * Merge branch 'electra-epoch-proc' of https://github.com/sigp/lighthouse into electra-engine-api * Merge pull request #5749 from sigp/electra_op_pool Optimise Electra op pool aggregation * don't fail on empty consolidations * Merge branch 'electra_attestation_changes' of https://github.com/sigp/lighthouse into block-processing-electra * Merge branch 'block-processing-electra' of https://github.com/sigp/lighthouse into electra-epoch-proc * Merge branch 'electra-epoch-proc' of https://github.com/sigp/lighthouse into electra-engine-api * update committee offset * Merge branch 'electra_attestation_changes' of https://github.com/sigp/lighthouse into block-processing-electra * update committee offset * update committee offset * only increment the state deposit index on old deposit flow * Merge branch 'block-processing-electra' of https://github.com/sigp/lighthouse into electra-epoch-proc * Merge branch 'electra-epoch-proc' of https://github.com/sigp/lighthouse into electra-engine-api * use correct max eb in epoch cache initialization * drop initiate validator ordering optimization * fix initiate exit for single pass * Merge branch 'electra-epoch-proc' of https://github.com/sigp/lighthouse into electra-engine-api * accept new payload v4 in mock el * Fix Electra Fork Choice Tests (#5764) * Fix Electra Fork Choice Tests (#5764) * Fix Electra Fork Choice Tests (#5764) * Merge branch 'electra_attestation_changes' of https://github.com/sigp/lighthouse into block-processing-electra * Merge branch 'block-processing-electra' of https://github.com/sigp/lighthouse into electra-epoch-proc * Merge branch 'electra-epoch-proc' of https://github.com/sigp/lighthouse into electra-engine-api * Fix Consolidation Sigs & Withdrawals * Merge pull request #5766 from ethDreamer/two_fixes Fix Consolidation Sigs & Withdrawals * Merge branches 'block-processing-electra' and 'electra-epoch-proc' of https://github.com/sigp/lighthouse into electra-epoch-proc * Merge branch 'electra-epoch-proc' of https://github.com/sigp/lighthouse into electra-engine-api * Fix ser/de * Subscribe to the correct subnets for electra attestations (#5782) * subscribe to the correct att subnets for electra * subscribe to the correct att subnets for electra * cargo fmt * Subscribe to the correct subnets for electra attestations (#5782) * subscribe to the correct att subnets for electra * subscribe to the correct att subnets for electra * cargo fmt * Subscribe to the correct subnets for electra attestations (#5782) * subscribe to the correct att subnets for electra * subscribe to the correct att subnets for electra * cargo fmt * Subscribe to the correct subnets for electra attestations (#5782) * subscribe to the correct att subnets for electra * subscribe to the correct att subnets for electra * cargo fmt * update electra readiness with new endpoints * fix slashing handling * Fix Bug In Block Processing with 0x02 Credentials * Merge remote-tracking branch 'upstream/unstable' * Send unagg attestation based on fork * Publish all aggregates * just one more check bro plz.. * Merge pull request #5832 from ethDreamer/electra_attestation_changes_merge_unstable Merge `unstable` into `electra_attestation_changes` * Merge pull request #5835 from realbigsean/fix-validator-logic Fix validator logic * Merge pull request #5816 from realbigsean/electra-attestation-slashing-handling Electra slashing handling * Merge branch 'electra_attestation_changes' of https://github.com/sigp/lighthouse into block-processing-electra * Merge branch 'block-processing-electra' of https://github.com/sigp/lighthouse into electra-epoch-proc * Merge branch 'electra-epoch-proc' of https://github.com/sigp/lighthouse into electra-engine-api * fix: serde rename camle case for execution payload body (#5846) * Electra attestation changes rm decode impl (#5856) * Remove Crappy Decode impl for Attestation * Remove Inefficient Attestation Decode impl * Implement Schema Upgrade / Downgrade * Update beacon_node/beacon_chain/src/schema_change/migration_schema_v20.rs Co-authored-by: Michael Sproul --------- Co-authored-by: Michael Sproul * Fix failing attestation tests and misc electra attestation cleanup (#5810) * - get attestation related beacon chain tests to pass - observed attestations are now keyed off of data + committee index - rename op pool attestationref to compactattestationref - remove unwraps in agg pool and use options instead - cherry pick some changes from ef-tests-electra * cargo fmt * fix failing test * Revert dockerfile changes * make committee_index return option * function args shouldnt be a ref to attestation ref * fmt * fix dup imports --------- Co-authored-by: realbigsean * fix some todos (#5817) * Merge branch 'unstable' of https://github.com/sigp/lighthouse into electra_attestation_changes * add consolidations to merkle calc for inclusion proof * Merge branch 'electra_attestation_changes' of https://github.com/sigp/lighthouse into block-processing-electra * Merge branch 'block-processing-electra' of https://github.com/sigp/lighthouse into electra-epoch-proc * Merge branch 'electra-epoch-proc' of https://github.com/sigp/lighthouse into electra-engine-api * Remove Duplicate KZG Commitment Merkle Proof Code (#5874) * Remove Duplicate KZG Commitment Merkle Proof Code * s/tree_lists/fields/ * Merge branch 'unstable' of https://github.com/sigp/lighthouse into electra_attestation_changes * Merge branch 'electra_attestation_changes' of https://github.com/sigp/lighthouse into block-processing-electra * Merge branch 'block-processing-electra' of https://github.com/sigp/lighthouse into electra-epoch-proc * Merge branch 'electra-epoch-proc' of https://github.com/sigp/lighthouse into electra-engine-api * fix compile * Merge branch 'electra_attestation_changes' of https://github.com/sigp/lighthouse into block-processing-electra * Merge branch 'block-processing-electra' of https://github.com/sigp/lighthouse into electra-epoch-proc * Merge branch 'electra-epoch-proc' of https://github.com/sigp/lighthouse into electra-engine-api * Fix slasher tests (#5906) * Fix electra tests * Add electra attestations to double vote tests * Update superstruct to 0.8 * Merge remote-tracking branch 'origin/unstable' into electra_attestation_changes * Small cleanup in slasher tests * Clean up Electra observed aggregates (#5929) * Use consistent key in observed_attestations * Remove unwraps from observed aggregates * Merge branch 'unstable' of https://github.com/sigp/lighthouse into electra_attestation_changes * De-dup attestation constructor logic * Remove unwraps in Attestation construction * Dedup match_attestation_data * Remove outdated TODO * Use ForkName Ord in fork-choice tests * Use ForkName Ord in BeaconBlockBody * Make to_electra not fallible * Remove TestRandom impl for IndexedAttestation * Remove IndexedAttestation faulty Decode impl * Drop TestRandom impl * Add PendingAttestationInElectra * Indexed att on disk (#35) * indexed att on disk * fix lints * Update slasher/src/migrate.rs Co-authored-by: ethDreamer <37123614+ethDreamer@users.noreply.github.com> --------- Co-authored-by: Lion - dapplion <35266934+dapplion@users.noreply.github.com> Co-authored-by: ethDreamer <37123614+ethDreamer@users.noreply.github.com> * add electra fork enabled fn to ForkName impl (#36) * add electra fork enabled fn to ForkName impl * remove inadvertent file * Update common/eth2/src/types.rs Co-authored-by: ethDreamer <37123614+ethDreamer@users.noreply.github.com> * Dedup attestation constructor logic in attester cache * Use if let Ok for committee_bits * Dedup Attestation constructor code * Diff reduction in tests * Fix beacon_chain tests * Diff reduction * Use Ord for ForkName in pubsub * Resolve into_attestation_and_indices todo * Remove stale TODO * Fix beacon_chain tests * Test spec invariant * Use electra_enabled in pubsub * Remove get_indexed_attestation_from_signed_aggregate * Use ok_or instead of if let else * committees are sorted * remove dup method `get_indexed_attestation_from_committees` * Merge pull request #5940 from dapplion/electra_attestation_changes_lionreview Electra attestations #5712 review * update default persisted op pool deserialization * ensure aggregate and proof uses serde untagged on ref * Fork aware ssz static attestation tests * Electra attestation changes from Lions review (#5971) * dedup/cleanup and remove unneeded hashset use * remove irrelevant TODOs * Merge branch 'unstable' of https://github.com/sigp/lighthouse into electra_attestation_changes * Merge branch 'electra_attestation_changes' of https://github.com/sigp/lighthouse into block-processing-electra * Merge branch 'block-processing-electra' of https://github.com/sigp/lighthouse into electra-epoch-proc * Merge branch 'electra-epoch-proc' of https://github.com/sigp/lighthouse into electra-engine-api * Electra attestation changes sean review (#5972) * instantiate empty bitlist in unreachable code * clean up error conversion * fork enabled bool cleanup * remove a couple todos * return bools instead of options in `aggregate` and use the result * delete commented out code * use map macros in simple transformations * remove signers_disjoint_from * get ef tests compiling * get ef tests compiling * update intentionally excluded files * Avoid changing slasher schema for Electra * Delete slasher schema v4 * Fix clippy * Fix compilation of beacon_chain tests * Update database.rs * Update per_block_processing.rs * Add electra lightclient types * Update slasher/src/database.rs * fix imports * Merge pull request #5980 from dapplion/electra-lightclient Add electra lightclient types * Merge pull request #5975 from michaelsproul/electra-slasher-no-migration Avoid changing slasher schema for Electra * Update beacon_node/beacon_chain/src/attestation_verification.rs * Update beacon_node/beacon_chain/src/attestation_verification.rs * Merge branch 'unstable' of https://github.com/sigp/lighthouse into electra_attestation_changes * Merge branch 'electra_attestation_changes' of https://github.com/realbigsean/lighthouse into block-processing-electra * Merge branch 'block-processing-electra' of https://github.com/sigp/lighthouse into electra-epoch-proc * Merge branch 'electra-epoch-proc' of https://github.com/sigp/lighthouse into electra-engine-api * The great renaming receipt -> request * Address some more review comments * Merge branch 'unstable' of https://github.com/sigp/lighthouse into electra-engine-api * Update beacon_node/beacon_chain/src/electra_readiness.rs * Update consensus/types/src/chain_spec.rs --- .../beacon_chain/src/electra_readiness.rs | 26 ++-- beacon_node/execution_layer/src/engine_api.rs | 52 ++++++-- .../execution_layer/src/engine_api/http.rs | 61 +++++++-- .../src/engine_api/json_structures.rs | 123 +++++++++++++++++- beacon_node/execution_layer/src/lib.rs | 50 ++++++- .../test_utils/execution_block_generator.rs | 2 +- .../src/test_utils/handle_rpc.rs | 53 +++++--- .../execution_layer/src/test_utils/mod.rs | 2 + beacon_node/store/src/partial_beacon_state.rs | 6 +- .../process_operations.rs | 16 +-- .../src/per_epoch_processing/single_pass.rs | 2 +- .../state_processing/src/upgrade/electra.rs | 2 +- consensus/types/presets/gnosis/electra.yaml | 2 +- consensus/types/presets/mainnet/electra.yaml | 2 +- consensus/types/presets/minimal/electra.yaml | 2 +- consensus/types/src/beacon_state.rs | 2 +- consensus/types/src/chain_spec.rs | 6 +- consensus/types/src/config_and_preset.rs | 2 +- consensus/types/src/deposit_receipt.rs | 4 +- consensus/types/src/eth_spec.rs | 14 +- consensus/types/src/execution_payload.rs | 6 +- .../types/src/execution_payload_header.rs | 2 +- consensus/types/src/lib.rs | 2 +- consensus/types/src/payload.rs | 32 ++--- consensus/types/src/preset.rs | 4 +- 25 files changed, 361 insertions(+), 114 deletions(-) diff --git a/beacon_node/beacon_chain/src/electra_readiness.rs b/beacon_node/beacon_chain/src/electra_readiness.rs index 42ee743fe6..551d43f9fd 100644 --- a/beacon_node/beacon_chain/src/electra_readiness.rs +++ b/beacon_node/beacon_chain/src/electra_readiness.rs @@ -2,9 +2,7 @@ //! transition. use crate::{BeaconChain, BeaconChainTypes}; -use execution_layer::http::{ - ENGINE_FORKCHOICE_UPDATED_V3, ENGINE_GET_PAYLOAD_V3, ENGINE_NEW_PAYLOAD_V3, -}; +use execution_layer::http::{ENGINE_GET_PAYLOAD_V4, ENGINE_NEW_PAYLOAD_V4}; use serde::{Deserialize, Serialize}; use std::fmt; use std::time::Duration; @@ -21,8 +19,8 @@ pub const ENGINE_CAPABILITIES_REFRESH_INTERVAL: u64 = 300; pub enum ElectraReadiness { /// The execution engine is electra-enabled (as far as we can tell) Ready, - /// We are connected to an execution engine which doesn't support the V3 engine api methods - V3MethodsNotSupported { error: String }, + /// We are connected to an execution engine which doesn't support the V4 engine api methods + V4MethodsNotSupported { error: String }, /// The transition configuration with the EL failed, there might be a problem with /// connectivity, authentication or a difference in configuration. ExchangeCapabilitiesFailed { error: String }, @@ -47,7 +45,7 @@ impl fmt::Display for ElectraReadiness { "The --execution-endpoint flag is not specified, this is a \ requirement post-merge" ), - ElectraReadiness::V3MethodsNotSupported { error } => write!( + ElectraReadiness::V4MethodsNotSupported { error } => write!( f, "Execution endpoint does not support Electra methods: {}", error @@ -88,29 +86,23 @@ impl BeaconChain { } } Ok(capabilities) => { - // TODO(electra): Update in the event we get V4s. let mut missing_methods = String::from("Required Methods Unsupported:"); let mut all_good = true; - if !capabilities.get_payload_v3 { + if !capabilities.get_payload_v4 { missing_methods.push(' '); - missing_methods.push_str(ENGINE_GET_PAYLOAD_V3); + missing_methods.push_str(ENGINE_GET_PAYLOAD_V4); all_good = false; } - if !capabilities.forkchoice_updated_v3 { + if !capabilities.new_payload_v4 { missing_methods.push(' '); - missing_methods.push_str(ENGINE_FORKCHOICE_UPDATED_V3); - all_good = false; - } - if !capabilities.new_payload_v3 { - missing_methods.push(' '); - missing_methods.push_str(ENGINE_NEW_PAYLOAD_V3); + missing_methods.push_str(ENGINE_NEW_PAYLOAD_V4); all_good = false; } if all_good { ElectraReadiness::Ready } else { - ElectraReadiness::V3MethodsNotSupported { + ElectraReadiness::V4MethodsNotSupported { error: missing_methods, } } diff --git a/beacon_node/execution_layer/src/engine_api.rs b/beacon_node/execution_layer/src/engine_api.rs index ce1e0fec5d..6a56a5d076 100644 --- a/beacon_node/execution_layer/src/engine_api.rs +++ b/beacon_node/execution_layer/src/engine_api.rs @@ -3,7 +3,8 @@ use crate::http::{ ENGINE_FORKCHOICE_UPDATED_V1, ENGINE_FORKCHOICE_UPDATED_V2, ENGINE_FORKCHOICE_UPDATED_V3, ENGINE_GET_CLIENT_VERSION_V1, ENGINE_GET_PAYLOAD_BODIES_BY_HASH_V1, ENGINE_GET_PAYLOAD_BODIES_BY_RANGE_V1, ENGINE_GET_PAYLOAD_V1, ENGINE_GET_PAYLOAD_V2, - ENGINE_GET_PAYLOAD_V3, ENGINE_NEW_PAYLOAD_V1, ENGINE_NEW_PAYLOAD_V2, ENGINE_NEW_PAYLOAD_V3, + ENGINE_GET_PAYLOAD_V3, ENGINE_GET_PAYLOAD_V4, ENGINE_NEW_PAYLOAD_V1, ENGINE_NEW_PAYLOAD_V2, + ENGINE_NEW_PAYLOAD_V3, ENGINE_NEW_PAYLOAD_V4, }; use eth2::types::{ BlobsBundle, SsePayloadAttributes, SsePayloadAttributesV1, SsePayloadAttributesV2, @@ -19,6 +20,7 @@ use reqwest::StatusCode; use serde::{Deserialize, Serialize}; use strum::IntoStaticStr; use superstruct::superstruct; +use types::execution_payload::{DepositRequests, WithdrawalRequests}; pub use types::{ Address, BeaconBlockRef, EthSpec, ExecutionBlockHash, ExecutionPayload, ExecutionPayloadHeader, ExecutionPayloadRef, FixedVector, ForkName, Hash256, Transactions, Uint256, VariableList, @@ -40,6 +42,8 @@ pub use new_payload_request::{ NewPayloadRequestDeneb, NewPayloadRequestElectra, }; +use self::json_structures::{JsonDepositRequest, JsonWithdrawalRequest}; + pub const LATEST_TAG: &str = "latest"; pub type PayloadId = [u8; 8]; @@ -60,9 +64,10 @@ pub enum Error { ExecutionHeadBlockNotFound, ParentHashEqualsBlockHash(ExecutionBlockHash), PayloadIdUnavailable, - TransitionConfigurationMismatch, SszError(ssz_types::Error), DeserializeWithdrawals(ssz_types::Error), + DeserializeDepositRequests(ssz_types::Error), + DeserializeWithdrawalRequests(ssz_types::Error), BuilderApi(builder_client::Error), IncorrectStateVariant, RequiredMethodUnsupported(&'static str), @@ -197,6 +202,10 @@ pub struct ExecutionBlockWithTransactions { #[superstruct(only(Deneb, Electra))] #[serde(with = "serde_utils::u64_hex_be")] pub excess_blob_gas: u64, + #[superstruct(only(Electra))] + pub deposit_requests: Vec, + #[superstruct(only(Electra))] + pub withdrawal_requests: Vec, } impl TryFrom> for ExecutionBlockWithTransactions { @@ -304,6 +313,16 @@ impl TryFrom> for ExecutionBlockWithTransactions .collect(), blob_gas_used: block.blob_gas_used, excess_blob_gas: block.excess_blob_gas, + deposit_requests: block + .deposit_requests + .into_iter() + .map(|deposit| deposit.into()) + .collect(), + withdrawal_requests: block + .withdrawal_requests + .into_iter() + .map(|withdrawal| withdrawal.into()) + .collect(), }) } }; @@ -526,6 +545,8 @@ impl GetPayloadResponse { pub struct ExecutionPayloadBodyV1 { pub transactions: Transactions, pub withdrawals: Option>, + pub deposit_requests: Option>, + pub withdrawal_requests: Option>, } impl ExecutionPayloadBodyV1 { @@ -613,7 +634,14 @@ impl ExecutionPayloadBodyV1 { } } ExecutionPayloadHeader::Electra(header) => { - if let Some(withdrawals) = self.withdrawals { + let withdrawals_exist = self.withdrawals.is_some(); + let deposit_requests_exist = self.deposit_requests.is_some(); + let withdrawal_requests_exist = self.withdrawal_requests.is_some(); + if let (Some(withdrawals), Some(deposit_requests), Some(withdrawal_requests)) = ( + self.withdrawals, + self.deposit_requests, + self.withdrawal_requests, + ) { Ok(ExecutionPayload::Electra(ExecutionPayloadElectra { parent_hash: header.parent_hash, fee_recipient: header.fee_recipient, @@ -632,14 +660,14 @@ impl ExecutionPayloadBodyV1 { withdrawals, blob_gas_used: header.blob_gas_used, excess_blob_gas: header.excess_blob_gas, - // TODO(electra) - deposit_receipts: <_>::default(), - withdrawal_requests: <_>::default(), + deposit_requests, + withdrawal_requests, })) } else { Err(format!( - "block {} is post-capella but payload body doesn't have withdrawals", - header.block_hash + "block {} is post-electra but payload body doesn't have withdrawals/deposit_requests/withdrawal_requests \ + withdrawals: {}, deposit_requests: {}, withdrawal_requests: {}", + header.block_hash, withdrawals_exist, deposit_requests_exist, withdrawal_requests_exist )) } } @@ -652,6 +680,7 @@ pub struct EngineCapabilities { pub new_payload_v1: bool, pub new_payload_v2: bool, pub new_payload_v3: bool, + pub new_payload_v4: bool, pub forkchoice_updated_v1: bool, pub forkchoice_updated_v2: bool, pub forkchoice_updated_v3: bool, @@ -660,6 +689,7 @@ pub struct EngineCapabilities { pub get_payload_v1: bool, pub get_payload_v2: bool, pub get_payload_v3: bool, + pub get_payload_v4: bool, pub get_client_version_v1: bool, } @@ -675,6 +705,9 @@ impl EngineCapabilities { if self.new_payload_v3 { response.push(ENGINE_NEW_PAYLOAD_V3); } + if self.new_payload_v4 { + response.push(ENGINE_NEW_PAYLOAD_V4); + } if self.forkchoice_updated_v1 { response.push(ENGINE_FORKCHOICE_UPDATED_V1); } @@ -699,6 +732,9 @@ impl EngineCapabilities { if self.get_payload_v3 { response.push(ENGINE_GET_PAYLOAD_V3); } + if self.get_payload_v4 { + response.push(ENGINE_GET_PAYLOAD_V4); + } if self.get_client_version_v1 { response.push(ENGINE_GET_CLIENT_VERSION_V1); } diff --git a/beacon_node/execution_layer/src/engine_api/http.rs b/beacon_node/execution_layer/src/engine_api/http.rs index 48095870ff..1c03cc81fc 100644 --- a/beacon_node/execution_layer/src/engine_api/http.rs +++ b/beacon_node/execution_layer/src/engine_api/http.rs @@ -34,11 +34,13 @@ pub const ETH_SYNCING_TIMEOUT: Duration = Duration::from_secs(1); pub const ENGINE_NEW_PAYLOAD_V1: &str = "engine_newPayloadV1"; pub const ENGINE_NEW_PAYLOAD_V2: &str = "engine_newPayloadV2"; pub const ENGINE_NEW_PAYLOAD_V3: &str = "engine_newPayloadV3"; +pub const ENGINE_NEW_PAYLOAD_V4: &str = "engine_newPayloadV4"; pub const ENGINE_NEW_PAYLOAD_TIMEOUT: Duration = Duration::from_secs(8); pub const ENGINE_GET_PAYLOAD_V1: &str = "engine_getPayloadV1"; pub const ENGINE_GET_PAYLOAD_V2: &str = "engine_getPayloadV2"; pub const ENGINE_GET_PAYLOAD_V3: &str = "engine_getPayloadV3"; +pub const ENGINE_GET_PAYLOAD_V4: &str = "engine_getPayloadV4"; pub const ENGINE_GET_PAYLOAD_TIMEOUT: Duration = Duration::from_secs(2); pub const ENGINE_FORKCHOICE_UPDATED_V1: &str = "engine_forkchoiceUpdatedV1"; @@ -66,9 +68,11 @@ pub static LIGHTHOUSE_CAPABILITIES: &[&str] = &[ ENGINE_NEW_PAYLOAD_V1, ENGINE_NEW_PAYLOAD_V2, ENGINE_NEW_PAYLOAD_V3, + ENGINE_NEW_PAYLOAD_V4, ENGINE_GET_PAYLOAD_V1, ENGINE_GET_PAYLOAD_V2, ENGINE_GET_PAYLOAD_V3, + ENGINE_GET_PAYLOAD_V4, ENGINE_FORKCHOICE_UPDATED_V1, ENGINE_FORKCHOICE_UPDATED_V2, ENGINE_FORKCHOICE_UPDATED_V3, @@ -830,7 +834,7 @@ impl HttpJsonRpc { Ok(response.into()) } - pub async fn new_payload_v3_electra( + pub async fn new_payload_v4_electra( &self, new_payload_request_electra: NewPayloadRequestElectra<'_, E>, ) -> Result { @@ -842,7 +846,7 @@ impl HttpJsonRpc { let response: JsonPayloadStatusV1 = self .rpc_request( - ENGINE_NEW_PAYLOAD_V3, + ENGINE_NEW_PAYLOAD_V4, params, ENGINE_NEW_PAYLOAD_TIMEOUT * self.execution_timeout_multiplier, ) @@ -926,19 +930,43 @@ impl HttpJsonRpc { .await?; Ok(JsonGetPayloadResponse::V3(response).into()) } + ForkName::Base + | ForkName::Altair + | ForkName::Bellatrix + | ForkName::Capella + | ForkName::Electra => Err(Error::UnsupportedForkVariant(format!( + "called get_payload_v3 with {}", + fork_name + ))), + } + } + + pub async fn get_payload_v4( + &self, + fork_name: ForkName, + payload_id: PayloadId, + ) -> Result, Error> { + let params = json!([JsonPayloadIdRequest::from(payload_id)]); + + match fork_name { ForkName::Electra => { let response: JsonGetPayloadResponseV4 = self .rpc_request( - ENGINE_GET_PAYLOAD_V3, + ENGINE_GET_PAYLOAD_V4, params, ENGINE_GET_PAYLOAD_TIMEOUT * self.execution_timeout_multiplier, ) .await?; Ok(JsonGetPayloadResponse::V4(response).into()) } - ForkName::Base | ForkName::Altair | ForkName::Bellatrix | ForkName::Capella => Err( - Error::UnsupportedForkVariant(format!("called get_payload_v3 with {}", fork_name)), - ), + ForkName::Base + | ForkName::Altair + | ForkName::Bellatrix + | ForkName::Capella + | ForkName::Deneb => Err(Error::UnsupportedForkVariant(format!( + "called get_payload_v4 with {}", + fork_name + ))), } } @@ -1064,6 +1092,7 @@ impl HttpJsonRpc { new_payload_v1: capabilities.contains(ENGINE_NEW_PAYLOAD_V1), new_payload_v2: capabilities.contains(ENGINE_NEW_PAYLOAD_V2), new_payload_v3: capabilities.contains(ENGINE_NEW_PAYLOAD_V3), + new_payload_v4: capabilities.contains(ENGINE_NEW_PAYLOAD_V4), forkchoice_updated_v1: capabilities.contains(ENGINE_FORKCHOICE_UPDATED_V1), forkchoice_updated_v2: capabilities.contains(ENGINE_FORKCHOICE_UPDATED_V2), forkchoice_updated_v3: capabilities.contains(ENGINE_FORKCHOICE_UPDATED_V3), @@ -1074,6 +1103,7 @@ impl HttpJsonRpc { get_payload_v1: capabilities.contains(ENGINE_GET_PAYLOAD_V1), get_payload_v2: capabilities.contains(ENGINE_GET_PAYLOAD_V2), get_payload_v3: capabilities.contains(ENGINE_GET_PAYLOAD_V3), + get_payload_v4: capabilities.contains(ENGINE_GET_PAYLOAD_V4), get_client_version_v1: capabilities.contains(ENGINE_GET_CLIENT_VERSION_V1), }) } @@ -1196,11 +1226,11 @@ impl HttpJsonRpc { } } NewPayloadRequest::Electra(new_payload_request_electra) => { - if engine_capabilities.new_payload_v3 { - self.new_payload_v3_electra(new_payload_request_electra) + if engine_capabilities.new_payload_v4 { + self.new_payload_v4_electra(new_payload_request_electra) .await } else { - Err(Error::RequiredMethodUnsupported("engine_newPayloadV3")) + Err(Error::RequiredMethodUnsupported("engine_newPayloadV4")) } } } @@ -1218,17 +1248,24 @@ impl HttpJsonRpc { ForkName::Bellatrix | ForkName::Capella => { if engine_capabilities.get_payload_v2 { self.get_payload_v2(fork_name, payload_id).await - } else if engine_capabilities.new_payload_v1 { + } else if engine_capabilities.get_payload_v1 { self.get_payload_v1(payload_id).await } else { Err(Error::RequiredMethodUnsupported("engine_getPayload")) } } - ForkName::Deneb | ForkName::Electra => { + ForkName::Deneb => { if engine_capabilities.get_payload_v3 { self.get_payload_v3(fork_name, payload_id).await } else { - Err(Error::RequiredMethodUnsupported("engine_getPayloadV3")) + Err(Error::RequiredMethodUnsupported("engine_getPayloadv3")) + } + } + ForkName::Electra => { + if engine_capabilities.get_payload_v4 { + self.get_payload_v4(fork_name, payload_id).await + } else { + Err(Error::RequiredMethodUnsupported("engine_getPayloadv4")) } } ForkName::Base | ForkName::Altair => Err(Error::UnsupportedForkVariant(format!( diff --git a/beacon_node/execution_layer/src/engine_api/json_structures.rs b/beacon_node/execution_layer/src/engine_api/json_structures.rs index 306972ada2..fbffc47e29 100644 --- a/beacon_node/execution_layer/src/engine_api/json_structures.rs +++ b/beacon_node/execution_layer/src/engine_api/json_structures.rs @@ -4,7 +4,10 @@ use strum::EnumString; use superstruct::superstruct; use types::beacon_block_body::KzgCommitments; use types::blob_sidecar::BlobsList; -use types::{FixedVector, Unsigned}; +use types::{ + DepositRequest, ExecutionLayerWithdrawalRequest, FixedVector, PublicKeyBytes, Signature, + Unsigned, +}; #[derive(Debug, PartialEq, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] @@ -101,6 +104,12 @@ pub struct JsonExecutionPayload { #[superstruct(only(V3, V4))] #[serde(with = "serde_utils::u64_hex_be")] pub excess_blob_gas: u64, + #[superstruct(only(V4))] + // TODO(electra): Field name should be changed post devnet-0. See https://github.com/ethereum/execution-apis/pull/544 + pub deposit_requests: VariableList, + #[superstruct(only(V4))] + pub withdrawal_requests: + VariableList, } impl From> for JsonExecutionPayloadV1 { @@ -203,6 +212,18 @@ impl From> for JsonExecutionPayloadV4 .into(), blob_gas_used: payload.blob_gas_used, excess_blob_gas: payload.excess_blob_gas, + deposit_requests: payload + .deposit_requests + .into_iter() + .map(Into::into) + .collect::>() + .into(), + withdrawal_requests: payload + .withdrawal_requests + .into_iter() + .map(Into::into) + .collect::>() + .into(), } } } @@ -319,9 +340,18 @@ impl From> for ExecutionPayloadElectra .into(), blob_gas_used: payload.blob_gas_used, excess_blob_gas: payload.excess_blob_gas, - // TODO(electra) - deposit_receipts: Default::default(), - withdrawal_requests: Default::default(), + deposit_requests: payload + .deposit_requests + .into_iter() + .map(Into::into) + .collect::>() + .into(), + withdrawal_requests: payload + .withdrawal_requests + .into_iter() + .map(Into::into) + .collect::>() + .into(), } } } @@ -690,10 +720,14 @@ impl From for JsonForkchoiceUpdatedV1Response { #[derive(Clone, Debug, Serialize, Deserialize)] #[serde(bound = "E: EthSpec")] +#[serde(rename_all = "camelCase")] pub struct JsonExecutionPayloadBodyV1 { #[serde(with = "ssz_types::serde_utils::list_of_hex_var_list")] pub transactions: Transactions, pub withdrawals: Option>, + pub deposit_requests: Option>, + pub withdrawal_requests: + Option>, } impl From> for ExecutionPayloadBodyV1 { @@ -708,6 +742,22 @@ impl From> for ExecutionPayloadBodyV1< .collect::>(), ) }), + deposit_requests: value.deposit_requests.map(|json_receipts| { + DepositRequests::::from( + json_receipts + .into_iter() + .map(Into::into) + .collect::>(), + ) + }), + withdrawal_requests: value.withdrawal_requests.map(|json_withdrawal_requests| { + WithdrawalRequests::::from( + json_withdrawal_requests + .into_iter() + .map(Into::into) + .collect::>(), + ) + }), } } } @@ -786,3 +836,68 @@ impl TryFrom for ClientVersionV1 { }) } } + +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] +#[serde(rename_all = "camelCase")] +pub struct JsonDepositRequest { + pub pubkey: PublicKeyBytes, + pub withdrawal_credentials: Hash256, + #[serde(with = "serde_utils::u64_hex_be")] + pub amount: u64, + pub signature: Signature, + #[serde(with = "serde_utils::u64_hex_be")] + pub index: u64, +} + +impl From for JsonDepositRequest { + fn from(deposit: DepositRequest) -> Self { + Self { + pubkey: deposit.pubkey, + withdrawal_credentials: deposit.withdrawal_credentials, + amount: deposit.amount, + signature: deposit.signature, + index: deposit.index, + } + } +} + +impl From for DepositRequest { + fn from(json_deposit: JsonDepositRequest) -> Self { + Self { + pubkey: json_deposit.pubkey, + withdrawal_credentials: json_deposit.withdrawal_credentials, + amount: json_deposit.amount, + signature: json_deposit.signature, + index: json_deposit.index, + } + } +} + +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] +#[serde(rename_all = "camelCase")] +pub struct JsonWithdrawalRequest { + pub source_address: Address, + pub validator_public_key: PublicKeyBytes, + #[serde(with = "serde_utils::u64_hex_be")] + pub amount: u64, +} + +impl From for JsonWithdrawalRequest { + fn from(withdrawal_request: ExecutionLayerWithdrawalRequest) -> Self { + Self { + source_address: withdrawal_request.source_address, + validator_public_key: withdrawal_request.validator_pubkey, + amount: withdrawal_request.amount, + } + } +} + +impl From for ExecutionLayerWithdrawalRequest { + fn from(json_withdrawal_request: JsonWithdrawalRequest) -> Self { + Self { + source_address: json_withdrawal_request.source_address, + validator_pubkey: json_withdrawal_request.validator_public_key, + amount: json_withdrawal_request.amount, + } + } +} diff --git a/beacon_node/execution_layer/src/lib.rs b/beacon_node/execution_layer/src/lib.rs index f78803a909..eaa739d7a5 100644 --- a/beacon_node/execution_layer/src/lib.rs +++ b/beacon_node/execution_layer/src/lib.rs @@ -1985,10 +1985,52 @@ impl ExecutionLayer { excess_blob_gas: deneb_block.excess_blob_gas, }) } - ExecutionBlockWithTransactions::Electra(_) => { - return Err(ApiError::UnsupportedForkVariant(format!( - "legacy payload construction for {fork} is not implemented" - ))); + ExecutionBlockWithTransactions::Electra(electra_block) => { + let withdrawals = VariableList::new( + electra_block + .withdrawals + .into_iter() + .map(Into::into) + .collect(), + ) + .map_err(ApiError::DeserializeWithdrawals)?; + let deposit_requests = VariableList::new( + electra_block + .deposit_requests + .into_iter() + .map(Into::into) + .collect(), + ) + .map_err(ApiError::DeserializeDepositRequests)?; + let withdrawal_requests = VariableList::new( + electra_block + .withdrawal_requests + .into_iter() + .map(Into::into) + .collect(), + ) + .map_err(ApiError::DeserializeWithdrawalRequests)?; + ExecutionPayload::Electra(ExecutionPayloadElectra { + parent_hash: electra_block.parent_hash, + fee_recipient: electra_block.fee_recipient, + state_root: electra_block.state_root, + receipts_root: electra_block.receipts_root, + logs_bloom: electra_block.logs_bloom, + prev_randao: electra_block.prev_randao, + block_number: electra_block.block_number, + gas_limit: electra_block.gas_limit, + gas_used: electra_block.gas_used, + timestamp: electra_block.timestamp, + extra_data: electra_block.extra_data, + base_fee_per_gas: electra_block.base_fee_per_gas, + block_hash: electra_block.block_hash, + transactions: convert_transactions(electra_block.transactions)?, + withdrawals, + blob_gas_used: electra_block.blob_gas_used, + excess_blob_gas: electra_block.excess_blob_gas, + deposit_requests, + withdrawal_requests, + }) } }; diff --git a/beacon_node/execution_layer/src/test_utils/execution_block_generator.rs b/beacon_node/execution_layer/src/test_utils/execution_block_generator.rs index e80c6b2370..8619e24a23 100644 --- a/beacon_node/execution_layer/src/test_utils/execution_block_generator.rs +++ b/beacon_node/execution_layer/src/test_utils/execution_block_generator.rs @@ -659,7 +659,7 @@ impl ExecutionBlockGenerator { withdrawals: pa.withdrawals.clone().into(), blob_gas_used: 0, excess_blob_gas: 0, - deposit_receipts: vec![].into(), + deposit_requests: vec![].into(), withdrawal_requests: vec![].into(), }), _ => unreachable!(), diff --git a/beacon_node/execution_layer/src/test_utils/handle_rpc.rs b/beacon_node/execution_layer/src/test_utils/handle_rpc.rs index 1dc8f0ab83..0dc7a7759c 100644 --- a/beacon_node/execution_layer/src/test_utils/handle_rpc.rs +++ b/beacon_node/execution_layer/src/test_utils/handle_rpc.rs @@ -98,7 +98,10 @@ pub async fn handle_rpc( .unwrap()) } } - ENGINE_NEW_PAYLOAD_V1 | ENGINE_NEW_PAYLOAD_V2 | ENGINE_NEW_PAYLOAD_V3 => { + ENGINE_NEW_PAYLOAD_V1 + | ENGINE_NEW_PAYLOAD_V2 + | ENGINE_NEW_PAYLOAD_V3 + | ENGINE_NEW_PAYLOAD_V4 => { let request = match method { ENGINE_NEW_PAYLOAD_V1 => JsonExecutionPayload::V1( get_param::>(params, 0) @@ -111,20 +114,14 @@ pub async fn handle_rpc( .map(|jep| JsonExecutionPayload::V1(jep)) }) .map_err(|s| (s, BAD_PARAMS_ERROR_CODE))?, - ENGINE_NEW_PAYLOAD_V3 => get_param::>(params, 0) + // From v3 onwards, we use the newPayload version only for the corresponding + // ExecutionPayload version. So we return an error instead of falling back to + // older versions of newPayload + ENGINE_NEW_PAYLOAD_V3 => get_param::>(params, 0) + .map(|jep| JsonExecutionPayload::V3(jep)) + .map_err(|s| (s, BAD_PARAMS_ERROR_CODE))?, + ENGINE_NEW_PAYLOAD_V4 => get_param::>(params, 0) .map(|jep| JsonExecutionPayload::V4(jep)) - .or_else(|_| { - get_param::>(params, 0) - .map(|jep| JsonExecutionPayload::V3(jep)) - .or_else(|_| { - get_param::>(params, 0) - .map(|jep| JsonExecutionPayload::V2(jep)) - .or_else(|_| { - get_param::>(params, 0) - .map(|jep| JsonExecutionPayload::V1(jep)) - }) - }) - }) .map_err(|s| (s, BAD_PARAMS_ERROR_CODE))?, _ => unreachable!(), }; @@ -190,7 +187,10 @@ pub async fn handle_rpc( } } ForkName::Electra => { - if method == ENGINE_NEW_PAYLOAD_V1 || method == ENGINE_NEW_PAYLOAD_V2 { + if method == ENGINE_NEW_PAYLOAD_V1 + || method == ENGINE_NEW_PAYLOAD_V2 + || method == ENGINE_NEW_PAYLOAD_V3 + { return Err(( format!("{} called after Electra fork!", method), GENERIC_ERROR_CODE, @@ -259,7 +259,10 @@ pub async fn handle_rpc( Ok(serde_json::to_value(JsonPayloadStatusV1::from(response)).unwrap()) } - ENGINE_GET_PAYLOAD_V1 | ENGINE_GET_PAYLOAD_V2 | ENGINE_GET_PAYLOAD_V3 => { + ENGINE_GET_PAYLOAD_V1 + | ENGINE_GET_PAYLOAD_V2 + | ENGINE_GET_PAYLOAD_V3 + | ENGINE_GET_PAYLOAD_V4 => { let request: JsonPayloadIdRequest = get_param(params, 0).map_err(|s| (s, BAD_PARAMS_ERROR_CODE))?; let id = request.into(); @@ -309,7 +312,9 @@ pub async fn handle_rpc( .read() .get_fork_at_timestamp(response.timestamp()) == ForkName::Electra - && method == ENGINE_GET_PAYLOAD_V1 + && (method == ENGINE_GET_PAYLOAD_V1 + || method == ENGINE_GET_PAYLOAD_V2 + || method == ENGINE_GET_PAYLOAD_V3) { return Err(( format!("{} called after Electra fork!", method), @@ -338,6 +343,9 @@ pub async fn handle_rpc( } _ => unreachable!(), }), + // From v3 onwards, we use the getPayload version only for the corresponding + // ExecutionPayload version. So we return an error if the ExecutionPayload version + // we get does not correspond to the getPayload version. ENGINE_GET_PAYLOAD_V3 => Ok(match JsonExecutionPayload::from(response) { JsonExecutionPayload::V3(execution_payload) => { serde_json::to_value(JsonGetPayloadResponseV3 { @@ -353,6 +361,9 @@ pub async fn handle_rpc( }) .unwrap() } + _ => unreachable!(), + }), + ENGINE_GET_PAYLOAD_V4 => Ok(match JsonExecutionPayload::from(response) { JsonExecutionPayload::V4(execution_payload) => { serde_json::to_value(JsonGetPayloadResponseV4 { execution_payload, @@ -578,6 +589,14 @@ pub async fn handle_rpc( .withdrawals() .ok() .map(|withdrawals| VariableList::from(withdrawals.clone())), + deposit_requests: block.deposit_requests().ok().map( + |deposit_requests| VariableList::from(deposit_requests.clone()), + ), + withdrawal_requests: block.withdrawal_requests().ok().map( + |withdrawal_requests| { + VariableList::from(withdrawal_requests.clone()) + }, + ), })); } None => response.push(None), diff --git a/beacon_node/execution_layer/src/test_utils/mod.rs b/beacon_node/execution_layer/src/test_utils/mod.rs index a6d47995af..7b00ca9fbc 100644 --- a/beacon_node/execution_layer/src/test_utils/mod.rs +++ b/beacon_node/execution_layer/src/test_utils/mod.rs @@ -43,6 +43,7 @@ pub const DEFAULT_ENGINE_CAPABILITIES: EngineCapabilities = EngineCapabilities { new_payload_v1: true, new_payload_v2: true, new_payload_v3: true, + new_payload_v4: true, forkchoice_updated_v1: true, forkchoice_updated_v2: true, forkchoice_updated_v3: true, @@ -51,6 +52,7 @@ pub const DEFAULT_ENGINE_CAPABILITIES: EngineCapabilities = EngineCapabilities { get_payload_v1: true, get_payload_v2: true, get_payload_v3: true, + get_payload_v4: true, get_client_version_v1: true, }; diff --git a/beacon_node/store/src/partial_beacon_state.rs b/beacon_node/store/src/partial_beacon_state.rs index 5e6054bc06..8f40b4b924 100644 --- a/beacon_node/store/src/partial_beacon_state.rs +++ b/beacon_node/store/src/partial_beacon_state.rs @@ -121,7 +121,7 @@ where // Electra #[superstruct(only(Electra))] - pub deposit_receipts_start_index: u64, + pub deposit_requests_start_index: u64, #[superstruct(only(Electra))] pub deposit_balance_to_consume: u64, #[superstruct(only(Electra))] @@ -284,7 +284,7 @@ impl PartialBeaconState { latest_execution_payload_header, next_withdrawal_index, next_withdrawal_validator_index, - deposit_receipts_start_index, + deposit_requests_start_index, deposit_balance_to_consume, exit_balance_to_consume, earliest_exit_epoch, @@ -557,7 +557,7 @@ impl TryInto> for PartialBeaconState { latest_execution_payload_header, next_withdrawal_index, next_withdrawal_validator_index, - deposit_receipts_start_index, + deposit_requests_start_index, deposit_balance_to_consume, exit_balance_to_consume, earliest_exit_epoch, 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 ff126beabe..17607f7f33 100644 --- a/consensus/state_processing/src/per_block_processing/process_operations.rs +++ b/consensus/state_processing/src/per_block_processing/process_operations.rs @@ -44,9 +44,9 @@ pub fn process_operations>( if let Some(requests) = requests { process_execution_layer_withdrawal_requests(state, &requests, spec)?; } - let receipts = block_body.execution_payload()?.deposit_receipts()?; + let receipts = block_body.execution_payload()?.deposit_requests()?; if let Some(receipts) = receipts { - process_deposit_receipts(state, &receipts, spec)?; + process_deposit_requests(state, &receipts, spec)?; } process_consolidations(state, block_body.consolidations()?, verify_signatures, spec)?; } @@ -372,9 +372,9 @@ pub fn process_deposits( // [Modified in Electra:EIP6110] // Disable former deposit mechanism once all prior deposits are processed // - // If `deposit_receipts_start_index` does not exist as a field on `state`, electra is disabled + // If `deposit_requests_start_index` does not exist as a field on `state`, electra is disabled // which means we always want to use the old check, so this field defaults to `u64::MAX`. - let eth1_deposit_index_limit = state.deposit_receipts_start_index().unwrap_or(u64::MAX); + let eth1_deposit_index_limit = state.deposit_requests_start_index().unwrap_or(u64::MAX); if state.eth1_deposit_index() < eth1_deposit_index_limit { let expected_deposit_len = std::cmp::min( @@ -625,15 +625,15 @@ pub fn process_execution_layer_withdrawal_requests( Ok(()) } -pub fn process_deposit_receipts( +pub fn process_deposit_requests( state: &mut BeaconState, - receipts: &[DepositReceipt], + receipts: &[DepositRequest], spec: &ChainSpec, ) -> Result<(), BlockProcessingError> { for receipt in receipts { // Set deposit receipt start index - if state.deposit_receipts_start_index()? == spec.unset_deposit_receipts_start_index { - *state.deposit_receipts_start_index_mut()? = receipt.index + if state.deposit_requests_start_index()? == spec.unset_deposit_requests_start_index { + *state.deposit_requests_start_index_mut()? = receipt.index } let deposit_data = DepositData { pubkey: receipt.pubkey, 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 e5905b8fa2..a1fe9efbef 100644 --- a/consensus/state_processing/src/per_epoch_processing/single_pass.rs +++ b/consensus/state_processing/src/per_epoch_processing/single_pass.rs @@ -325,7 +325,7 @@ pub fn process_epoch_single_pass( } } - if conf.registry_updates && fork_name >= ForkName::Electra { + if conf.registry_updates && fork_name.electra_enabled() { if let Ok(earliest_exit_epoch_state) = state.earliest_exit_epoch_mut() { *earliest_exit_epoch_state = earliest_exit_epoch.ok_or(Error::MissingEarliestExitEpoch)?; diff --git a/consensus/state_processing/src/upgrade/electra.rs b/consensus/state_processing/src/upgrade/electra.rs index 1e60bf488d..03cd96aebc 100644 --- a/consensus/state_processing/src/upgrade/electra.rs +++ b/consensus/state_processing/src/upgrade/electra.rs @@ -78,7 +78,7 @@ pub fn upgrade_to_electra( next_withdrawal_validator_index: pre.next_withdrawal_validator_index, historical_summaries: pre.historical_summaries.clone(), // Electra - deposit_receipts_start_index: spec.unset_deposit_receipts_start_index, + deposit_requests_start_index: spec.unset_deposit_requests_start_index, deposit_balance_to_consume: 0, exit_balance_to_consume: 0, earliest_exit_epoch, diff --git a/consensus/types/presets/gnosis/electra.yaml b/consensus/types/presets/gnosis/electra.yaml index 72c626ded2..38f6960bac 100644 --- a/consensus/types/presets/gnosis/electra.yaml +++ b/consensus/types/presets/gnosis/electra.yaml @@ -35,7 +35,7 @@ MAX_CONSOLIDATIONS: 1 # Execution # --------------------------------------------------------------- # 2**13 (= 8192) receipts -MAX_DEPOSIT_RECEIPTS_PER_PAYLOAD: 8192 +MAX_DEPOSIT_REQUESTS_PER_PAYLOAD: 8192 # 2**4 (= 16) withdrawal requests MAX_WITHDRAWAL_REQUESTS_PER_PAYLOAD: 16 diff --git a/consensus/types/presets/mainnet/electra.yaml b/consensus/types/presets/mainnet/electra.yaml index 72c626ded2..38f6960bac 100644 --- a/consensus/types/presets/mainnet/electra.yaml +++ b/consensus/types/presets/mainnet/electra.yaml @@ -35,7 +35,7 @@ MAX_CONSOLIDATIONS: 1 # Execution # --------------------------------------------------------------- # 2**13 (= 8192) receipts -MAX_DEPOSIT_RECEIPTS_PER_PAYLOAD: 8192 +MAX_DEPOSIT_REQUESTS_PER_PAYLOAD: 8192 # 2**4 (= 16) withdrawal requests MAX_WITHDRAWAL_REQUESTS_PER_PAYLOAD: 16 diff --git a/consensus/types/presets/minimal/electra.yaml b/consensus/types/presets/minimal/electra.yaml index 11aa5e1f50..cf726e004b 100644 --- a/consensus/types/presets/minimal/electra.yaml +++ b/consensus/types/presets/minimal/electra.yaml @@ -35,7 +35,7 @@ MAX_CONSOLIDATIONS: 1 # Execution # --------------------------------------------------------------- # [customized] -MAX_DEPOSIT_RECEIPTS_PER_PAYLOAD: 4 +MAX_DEPOSIT_REQUESTS_PER_PAYLOAD: 4 # [customized] 2**1 (= 2) withdrawal requests MAX_WITHDRAWAL_REQUESTS_PER_PAYLOAD: 2 diff --git a/consensus/types/src/beacon_state.rs b/consensus/types/src/beacon_state.rs index 0426d43cac..45f9178ff3 100644 --- a/consensus/types/src/beacon_state.rs +++ b/consensus/types/src/beacon_state.rs @@ -481,7 +481,7 @@ where #[superstruct(only(Electra), partial_getter(copy))] #[metastruct(exclude_from(tree_lists))] #[serde(with = "serde_utils::quoted_u64")] - pub deposit_receipts_start_index: u64, + pub deposit_requests_start_index: u64, #[superstruct(only(Electra), partial_getter(copy))] #[metastruct(exclude_from(tree_lists))] #[serde(with = "serde_utils::quoted_u64")] diff --git a/consensus/types/src/chain_spec.rs b/consensus/types/src/chain_spec.rs index 5c131d77c9..16d4f046ba 100644 --- a/consensus/types/src/chain_spec.rs +++ b/consensus/types/src/chain_spec.rs @@ -180,7 +180,7 @@ pub struct ChainSpec { pub electra_fork_version: [u8; 4], /// The Electra fork epoch is optional, with `None` representing "Electra never happens". pub electra_fork_epoch: Option, - pub unset_deposit_receipts_start_index: u64, + pub unset_deposit_requests_start_index: u64, pub full_exit_request_amount: u64, pub min_activation_balance: u64, pub max_effective_balance_electra: u64, @@ -747,7 +747,7 @@ impl ChainSpec { */ electra_fork_version: [0x05, 00, 00, 00], electra_fork_epoch: None, - unset_deposit_receipts_start_index: u64::MAX, + unset_deposit_requests_start_index: u64::MAX, full_exit_request_amount: 0, min_activation_balance: option_wrapper(|| { u64::checked_pow(2, 5)?.checked_mul(u64::checked_pow(10, 9)?) @@ -1049,7 +1049,7 @@ impl ChainSpec { */ electra_fork_version: [0x05, 0x00, 0x00, 0x64], electra_fork_epoch: None, - unset_deposit_receipts_start_index: u64::MAX, + unset_deposit_requests_start_index: u64::MAX, full_exit_request_amount: 0, min_activation_balance: option_wrapper(|| { u64::checked_pow(2, 5)?.checked_mul(u64::checked_pow(10, 9)?) diff --git a/consensus/types/src/config_and_preset.rs b/consensus/types/src/config_and_preset.rs index 6fc6e0642e..110392d4b7 100644 --- a/consensus/types/src/config_and_preset.rs +++ b/consensus/types/src/config_and_preset.rs @@ -124,7 +124,7 @@ pub fn get_extra_fields(spec: &ChainSpec) -> HashMap { "versioned_hash_version_kzg".to_uppercase() => deneb::VERSIONED_HASH_VERSION_KZG.to_string().into(), // Electra "compounding_withdrawal_prefix".to_uppercase() => u8_hex(spec.compounding_withdrawal_prefix_byte), - "unset_deposit_receipts_start_index".to_uppercase() => spec.unset_deposit_receipts_start_index.to_string().into(), + "unset_deposit_requests_start_index".to_uppercase() => spec.unset_deposit_requests_start_index.to_string().into(), "full_exit_request_amount".to_uppercase() => spec.full_exit_request_amount.to_string().into(), "domain_consolidation".to_uppercase()=> u32_hex(spec.domain_consolidation), } diff --git a/consensus/types/src/deposit_receipt.rs b/consensus/types/src/deposit_receipt.rs index 6a08f717f3..f6ddf8b63a 100644 --- a/consensus/types/src/deposit_receipt.rs +++ b/consensus/types/src/deposit_receipt.rs @@ -19,7 +19,7 @@ use tree_hash_derive::TreeHash; TreeHash, TestRandom, )] -pub struct DepositReceipt { +pub struct DepositRequest { pub pubkey: PublicKeyBytes, pub withdrawal_credentials: Hash256, #[serde(with = "serde_utils::quoted_u64")] @@ -33,5 +33,5 @@ pub struct DepositReceipt { mod tests { use super::*; - ssz_and_tree_hash_tests!(DepositReceipt); + ssz_and_tree_hash_tests!(DepositRequest); } diff --git a/consensus/types/src/eth_spec.rs b/consensus/types/src/eth_spec.rs index 1c379f5de4..c6e816a487 100644 --- a/consensus/types/src/eth_spec.rs +++ b/consensus/types/src/eth_spec.rs @@ -144,7 +144,7 @@ pub trait EthSpec: type PendingPartialWithdrawalsLimit: Unsigned + Clone + Sync + Send + Debug + PartialEq; type PendingConsolidationsLimit: Unsigned + Clone + Sync + Send + Debug + PartialEq; type MaxConsolidations: Unsigned + Clone + Sync + Send + Debug + PartialEq; - type MaxDepositReceiptsPerPayload: Unsigned + Clone + Sync + Send + Debug + PartialEq; + type MaxDepositRequestsPerPayload: Unsigned + Clone + Sync + Send + Debug + PartialEq; type MaxAttesterSlashingsElectra: Unsigned + Clone + Sync + Send + Debug + PartialEq; type MaxAttestationsElectra: Unsigned + Clone + Sync + Send + Debug + PartialEq; type MaxWithdrawalRequestsPerPayload: Unsigned + Clone + Sync + Send + Debug + PartialEq; @@ -330,9 +330,9 @@ pub trait EthSpec: Self::MaxConsolidations::to_usize() } - /// Returns the `MAX_DEPOSIT_RECEIPTS_PER_PAYLOAD` constant for this specification. - fn max_deposit_receipts_per_payload() -> usize { - Self::MaxDepositReceiptsPerPayload::to_usize() + /// Returns the `MAX_DEPOSIT_REQUESTS_PER_PAYLOAD` constant for this specification. + fn max_deposit_requests_per_payload() -> usize { + Self::MaxDepositRequestsPerPayload::to_usize() } /// Returns the `MAX_ATTESTER_SLASHINGS_ELECTRA` constant for this specification. @@ -405,7 +405,7 @@ impl EthSpec for MainnetEthSpec { type PendingPartialWithdrawalsLimit = U134217728; type PendingConsolidationsLimit = U262144; type MaxConsolidations = U1; - type MaxDepositReceiptsPerPayload = U8192; + type MaxDepositRequestsPerPayload = U8192; type MaxAttesterSlashingsElectra = U1; type MaxAttestationsElectra = U8; type MaxWithdrawalRequestsPerPayload = U16; @@ -442,7 +442,7 @@ impl EthSpec for MinimalEthSpec { type KzgCommitmentInclusionProofDepth = U9; type PendingPartialWithdrawalsLimit = U64; type PendingConsolidationsLimit = U64; - type MaxDepositReceiptsPerPayload = U4; + type MaxDepositRequestsPerPayload = U4; type MaxWithdrawalRequestsPerPayload = U2; params_from_eth_spec!(MainnetEthSpec { @@ -528,7 +528,7 @@ impl EthSpec for GnosisEthSpec { type PendingPartialWithdrawalsLimit = U134217728; type PendingConsolidationsLimit = U262144; type MaxConsolidations = U1; - type MaxDepositReceiptsPerPayload = U8192; + type MaxDepositRequestsPerPayload = U8192; type MaxAttesterSlashingsElectra = U1; type MaxAttestationsElectra = U8; type MaxWithdrawalRequestsPerPayload = U16; diff --git a/consensus/types/src/execution_payload.rs b/consensus/types/src/execution_payload.rs index 0946b9ecff..eca561f735 100644 --- a/consensus/types/src/execution_payload.rs +++ b/consensus/types/src/execution_payload.rs @@ -13,6 +13,10 @@ pub type Transactions = VariableList< >; pub type Withdrawals = VariableList::MaxWithdrawalsPerPayload>; +pub type DepositRequests = + VariableList::MaxDepositRequestsPerPayload>; +pub type WithdrawalRequests = + VariableList::MaxWithdrawalRequestsPerPayload>; #[superstruct( variants(Bellatrix, Capella, Deneb, Electra), @@ -90,7 +94,7 @@ pub struct ExecutionPayload { #[serde(with = "serde_utils::quoted_u64")] pub excess_blob_gas: u64, #[superstruct(only(Electra))] - pub deposit_receipts: VariableList, + pub deposit_requests: VariableList, #[superstruct(only(Electra))] pub withdrawal_requests: VariableList, diff --git a/consensus/types/src/execution_payload_header.rs b/consensus/types/src/execution_payload_header.rs index b8d0c8c2b0..962e7a16fb 100644 --- a/consensus/types/src/execution_payload_header.rs +++ b/consensus/types/src/execution_payload_header.rs @@ -295,7 +295,7 @@ impl<'a, E: EthSpec> From<&'a ExecutionPayloadElectra> for ExecutionPayloadHe withdrawals_root: payload.withdrawals.tree_hash_root(), blob_gas_used: payload.blob_gas_used, excess_blob_gas: payload.excess_blob_gas, - deposit_receipts_root: payload.deposit_receipts.tree_hash_root(), + deposit_receipts_root: payload.deposit_requests.tree_hash_root(), withdrawal_requests_root: payload.withdrawal_requests.tree_hash_root(), } } diff --git a/consensus/types/src/lib.rs b/consensus/types/src/lib.rs index 6a98d7ade9..60c5a021e0 100644 --- a/consensus/types/src/lib.rs +++ b/consensus/types/src/lib.rs @@ -150,7 +150,7 @@ pub use crate::contribution_and_proof::ContributionAndProof; pub use crate::deposit::{Deposit, DEPOSIT_TREE_DEPTH}; pub use crate::deposit_data::DepositData; pub use crate::deposit_message::DepositMessage; -pub use crate::deposit_receipt::DepositReceipt; +pub use crate::deposit_receipt::DepositRequest; pub use crate::deposit_tree_snapshot::{DepositTreeSnapshot, FinalizedExecutionBlock}; pub use crate::enr_fork_id::EnrForkId; pub use crate::epoch_cache::{EpochCache, EpochCacheError, EpochCacheKey}; diff --git a/consensus/types/src/payload.rs b/consensus/types/src/payload.rs index 644d401ec7..362cb6d386 100644 --- a/consensus/types/src/payload.rs +++ b/consensus/types/src/payload.rs @@ -45,9 +45,9 @@ pub trait ExecPayload: Debug + Clone + PartialEq + Hash + TreeHash + Option>, Error, >; - fn deposit_receipts( + fn deposit_requests( &self, - ) -> Result>, Error>; + ) -> Result>, Error>; /// Is this a default payload with 0x0 roots for transactions and withdrawals? fn is_default_with_zero_roots(&self) -> bool; @@ -303,15 +303,15 @@ impl ExecPayload for FullPayload { } } - fn deposit_receipts( + fn deposit_requests( &self, - ) -> Result>, Error> { + ) -> Result>, Error> { match self { FullPayload::Bellatrix(_) | FullPayload::Capella(_) | FullPayload::Deneb(_) => { Err(Error::IncorrectStateVariant) } FullPayload::Electra(inner) => { - Ok(Some(inner.execution_payload.deposit_receipts.clone())) + Ok(Some(inner.execution_payload.deposit_requests.clone())) } } } @@ -464,15 +464,15 @@ impl<'b, E: EthSpec> ExecPayload for FullPayloadRef<'b, E> { } } - fn deposit_receipts( + fn deposit_requests( &self, - ) -> Result>, Error> { + ) -> Result>, Error> { match self { FullPayloadRef::Bellatrix(_) | FullPayloadRef::Capella(_) | FullPayloadRef::Deneb(_) => Err(Error::IncorrectStateVariant), FullPayloadRef::Electra(inner) => { - Ok(Some(inner.execution_payload.deposit_receipts.clone())) + Ok(Some(inner.execution_payload.deposit_requests.clone())) } } } @@ -666,9 +666,9 @@ impl ExecPayload for BlindedPayload { Ok(None) } - fn deposit_receipts( + fn deposit_requests( &self, - ) -> Result>, Error> { + ) -> Result>, Error> { Ok(None) } @@ -782,9 +782,9 @@ impl<'b, E: EthSpec> ExecPayload for BlindedPayloadRef<'b, E> { Ok(None) } - fn deposit_receipts( + fn deposit_requests( &self, - ) -> Result>, Error> { + ) -> Result>, Error> { Ok(None) } @@ -890,9 +890,9 @@ macro_rules! impl_exec_payload_common { i(self) } - fn deposit_receipts( + fn deposit_requests( &self, - ) -> Result>, Error> { + ) -> Result>, Error> { let j = $j; j(self) } @@ -1052,11 +1052,11 @@ macro_rules! impl_exec_payload_for_fork { let c: for<'a> fn( &'a $wrapper_type_full, ) -> Result< - Option>, + Option>, Error, > = |payload: &$wrapper_type_full| { let wrapper_ref_type = FullPayloadRef::$fork_variant(&payload); - wrapper_ref_type.deposit_receipts() + wrapper_ref_type.deposit_requests() }; c } diff --git a/consensus/types/src/preset.rs b/consensus/types/src/preset.rs index f4008d62e1..9e9c5aaf41 100644 --- a/consensus/types/src/preset.rs +++ b/consensus/types/src/preset.rs @@ -248,7 +248,7 @@ pub struct ElectraPreset { #[serde(with = "serde_utils::quoted_u64")] pub max_consolidations: u64, #[serde(with = "serde_utils::quoted_u64")] - pub max_deposit_receipts_per_payload: u64, + pub max_deposit_requests_per_payload: u64, #[serde(with = "serde_utils::quoted_u64")] pub max_attester_slashings_electra: u64, #[serde(with = "serde_utils::quoted_u64")] @@ -270,7 +270,7 @@ impl ElectraPreset { pending_partial_withdrawals_limit: E::pending_partial_withdrawals_limit() as u64, pending_consolidations_limit: E::pending_consolidations_limit() as u64, max_consolidations: E::max_consolidations() as u64, - max_deposit_receipts_per_payload: E::max_deposit_receipts_per_payload() as u64, + max_deposit_requests_per_payload: E::max_deposit_requests_per_payload() as u64, max_attester_slashings_electra: E::max_attester_slashings_electra() as u64, max_attestations_electra: E::max_attestations_electra() as u64, max_withdrawal_requests_per_payload: E::max_withdrawal_requests_per_payload() as u64, From 937f8b2d016aa6b1dff6d8dc6fe17a4fc64db819 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Wed, 3 Jul 2024 08:54:44 +1000 Subject: [PATCH 13/23] Fix SigVerifiedOp SSZ implementation (#6035) * Fix SigVerifiedOp SSZ implementation --- Cargo.lock | 5 + Cargo.toml | 2 +- consensus/state_processing/Cargo.toml | 2 + .../state_processing/src/verify_operation.rs | 170 ++++++++++-------- consensus/types/src/attester_slashing.rs | 11 ++ consensus/types/src/test_utils/test_random.rs | 16 ++ 6 files changed, 135 insertions(+), 71 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index f23777bc4c..adefb20420 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7824,6 +7824,9 @@ name = "smallvec" version = "1.13.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "3c5e1a9a646d36c3599cd173a41282daf47c44583ad367b8e6837255952e5c67" +dependencies = [ + "arbitrary", +] [[package]] name = "snap" @@ -7942,10 +7945,12 @@ dependencies = [ "lazy_static", "lighthouse_metrics", "merkle_proof", + "rand", "rayon", "safe_arith", "smallvec", "ssz_types", + "test_random_derive", "tokio", "tree_hash", "types", diff --git a/Cargo.toml b/Cargo.toml index d67f6edf1d..c09a3af7ce 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -158,7 +158,7 @@ slog = { version = "2", features = ["max_level_trace", "release_max_level_trace" slog-async = "2" slog-term = "2" sloggers = { version = "2", features = ["json"] } -smallvec = "1.11.2" +smallvec = { version = "1.11.2", features = ["arbitrary"] } snap = "1" ssz_types = "0.6" strum = { version = "0.24", features = ["derive"] } diff --git a/consensus/state_processing/Cargo.toml b/consensus/state_processing/Cargo.toml index be5367eb08..e05c0bcfeb 100644 --- a/consensus/state_processing/Cargo.toml +++ b/consensus/state_processing/Cargo.toml @@ -28,6 +28,8 @@ arbitrary = { workspace = true } lighthouse_metrics = { workspace = true } lazy_static = { workspace = true } derivative = { workspace = true } +test_random_derive = { path = "../../common/test_random_derive" } +rand = { workspace = true } [features] default = ["legacy-arith"] diff --git a/consensus/state_processing/src/verify_operation.rs b/consensus/state_processing/src/verify_operation.rs index c4b7c6a026..3b20c67b4d 100644 --- a/consensus/state_processing/src/verify_operation.rs +++ b/consensus/state_processing/src/verify_operation.rs @@ -7,17 +7,17 @@ use crate::per_block_processing::{ verify_proposer_slashing, }; use crate::VerifySignatures; +use arbitrary::Arbitrary; use derivative::Derivative; use smallvec::{smallvec, SmallVec}; use ssz::{Decode, Encode}; use ssz_derive::{Decode, Encode}; use std::marker::PhantomData; +use test_random_derive::TestRandom; use types::{ - AttesterSlashing, AttesterSlashingBase, AttesterSlashingOnDisk, AttesterSlashingRefOnDisk, -}; -use types::{ - BeaconState, ChainSpec, Epoch, EthSpec, Fork, ForkVersion, ProposerSlashing, - SignedBlsToExecutionChange, SignedVoluntaryExit, + test_utils::TestRandom, AttesterSlashing, AttesterSlashingBase, AttesterSlashingOnDisk, + AttesterSlashingRefOnDisk, BeaconState, ChainSpec, Epoch, EthSpec, Fork, ForkVersion, + ProposerSlashing, SignedBlsToExecutionChange, SignedVoluntaryExit, }; const MAX_FORKS_VERIFIED_AGAINST: usize = 2; @@ -39,12 +39,13 @@ pub trait TransformPersist { /// /// The inner `op` field is private, meaning instances of this type can only be constructed /// by calling `validate`. -#[derive(Derivative, Debug, Clone)] +#[derive(Derivative, Debug, Clone, Arbitrary)] #[derivative( PartialEq, Eq, Hash(bound = "T: TransformPersist + std::hash::Hash, E: EthSpec") )] +#[arbitrary(bound = "T: TransformPersist + Arbitrary<'arbitrary>, E: EthSpec")] pub struct SigVerifiedOp { op: T, verified_against: VerifiedAgainst, @@ -53,92 +54,68 @@ pub struct SigVerifiedOp { impl Encode for SigVerifiedOp { fn is_ssz_fixed_len() -> bool { - ::is_ssz_fixed_len() - && ::is_ssz_fixed_len() + as Encode>::is_ssz_fixed_len() } - #[allow(clippy::expect_used)] fn ssz_fixed_len() -> usize { - if ::is_ssz_fixed_len() { - ::ssz_fixed_len() - .checked_add(::ssz_fixed_len()) - .expect("encode ssz_fixed_len length overflow") - } else { - ssz::BYTES_PER_LENGTH_OFFSET - } - } - - #[allow(clippy::expect_used)] - fn ssz_bytes_len(&self) -> usize { - if ::is_ssz_fixed_len() { - ::ssz_fixed_len() - } else { - let persistable = self.op.as_persistable_ref(); - persistable - .ssz_bytes_len() - .checked_add(self.verified_against.ssz_bytes_len()) - .expect("ssz_bytes_len length overflow") - } + as Encode>::ssz_fixed_len() } fn ssz_append(&self, buf: &mut Vec) { - let mut encoder = ssz::SszEncoder::container(buf, ::ssz_fixed_len()); - let persistable = self.op.as_persistable_ref(); - encoder.append(&persistable); - encoder.append(&self.verified_against); - encoder.finalize(); + let persistable_ref = self.op.as_persistable_ref(); + SigVerifiedOpEncode { + op: persistable_ref, + verified_against: &self.verified_against, + } + .ssz_append(buf) + } + + fn ssz_bytes_len(&self) -> usize { + let persistable_ref = self.op.as_persistable_ref(); + SigVerifiedOpEncode { + op: persistable_ref, + verified_against: &self.verified_against, + } + .ssz_bytes_len() } } impl Decode for SigVerifiedOp { fn is_ssz_fixed_len() -> bool { - ::is_ssz_fixed_len() - && ::is_ssz_fixed_len() + as Decode>::is_ssz_fixed_len() } - #[allow(clippy::expect_used)] fn ssz_fixed_len() -> usize { - if ::is_ssz_fixed_len() { - ::ssz_fixed_len() - .checked_add(::ssz_fixed_len()) - .expect("decode ssz_fixed_len length overflow") - } else { - ssz::BYTES_PER_LENGTH_OFFSET - } + as Decode>::ssz_fixed_len() } fn from_ssz_bytes(bytes: &[u8]) -> Result { - let mut builder = ssz::SszDecoderBuilder::new(bytes); - - // Register types based on whether they are fixed or variable length - if ::is_ssz_fixed_len() { - builder.register_type::()?; - } else { - builder.register_anonymous_variable_length_item()?; - } - - if ::is_ssz_fixed_len() { - builder.register_type::()?; - } else { - builder.register_anonymous_variable_length_item()?; - } - - let mut decoder = builder.build()?; - // Decode each component - let persistable: T::Persistable = decoder.decode_next()?; - let verified_against: VerifiedAgainst = decoder.decode_next()?; - - // Use TransformPersist to convert persistable back into the original type - let op = T::from_persistable(persistable); - + let on_disk = SigVerifiedOpDecode::::from_ssz_bytes(bytes)?; Ok(SigVerifiedOp { - op, - verified_against, + op: T::from_persistable(on_disk.op), + verified_against: on_disk.verified_against, _phantom: PhantomData, }) } } +/// On-disk variant of `SigVerifiedOp` that implements `Encode`. +/// +/// We use separate types for Encode and Decode so we can efficiently handle references: the Encode +/// type contains references, while the Decode type does not. +#[derive(Debug, Encode)] +struct SigVerifiedOpEncode<'a, P: Encode> { + op: P, + verified_against: &'a VerifiedAgainst, +} + +/// On-disk variant of `SigVerifiedOp` that implements `Encode`. +#[derive(Debug, Decode)] +struct SigVerifiedOpDecode { + op: P, + verified_against: VerifiedAgainst, +} + /// Information about the fork versions that this message was verified against. /// /// In general it is not safe to assume that a `SigVerifiedOp` constructed at some point in the past @@ -156,7 +133,7 @@ impl Decode for SigVerifiedOp { /// /// We need to store multiple `ForkVersion`s because attester slashings contain two indexed /// attestations which may be signed using different versions. -#[derive(Debug, PartialEq, Eq, Clone, Hash, Encode, Decode)] +#[derive(Debug, PartialEq, Eq, Clone, Hash, Encode, Decode, TestRandom, Arbitrary)] pub struct VerifiedAgainst { fork_versions: SmallVec<[ForkVersion; MAX_FORKS_VERIFIED_AGAINST]>, } @@ -435,3 +412,56 @@ impl TransformPersist for SignedBlsToExecutionChange { persistable } } + +#[cfg(all(test, not(debug_assertions)))] +mod test { + use super::*; + use types::{ + test_utils::{SeedableRng, TestRandom, XorShiftRng}, + MainnetEthSpec, + }; + + type E = MainnetEthSpec; + + fn roundtrip_test() { + let runs = 10; + let mut rng = XorShiftRng::seed_from_u64(0xff0af5a356af1123); + + for _ in 0..runs { + let op = T::random_for_test(&mut rng); + let verified_against = VerifiedAgainst::random_for_test(&mut rng); + + let verified_op = SigVerifiedOp { + op, + verified_against, + _phantom: PhantomData::, + }; + + let serialized = verified_op.as_ssz_bytes(); + let deserialized = SigVerifiedOp::from_ssz_bytes(&serialized).unwrap(); + let reserialized = deserialized.as_ssz_bytes(); + assert_eq!(verified_op, deserialized); + assert_eq!(serialized, reserialized); + } + } + + #[test] + fn sig_verified_op_exit_roundtrip() { + roundtrip_test::(); + } + + #[test] + fn proposer_slashing_roundtrip() { + roundtrip_test::(); + } + + #[test] + fn attester_slashing_roundtrip() { + roundtrip_test::>(); + } + + #[test] + fn bls_to_execution_roundtrip() { + roundtrip_test::(); + } +} diff --git a/consensus/types/src/attester_slashing.rs b/consensus/types/src/attester_slashing.rs index a8d4e6989c..c8e2fb4f82 100644 --- a/consensus/types/src/attester_slashing.rs +++ b/consensus/types/src/attester_slashing.rs @@ -3,6 +3,7 @@ use crate::indexed_attestation::{ }; use crate::{test_utils::TestRandom, EthSpec}; use derivative::Derivative; +use rand::{Rng, RngCore}; use serde::{Deserialize, Serialize}; use ssz_derive::{Decode, Encode}; use superstruct::superstruct; @@ -160,6 +161,16 @@ impl AttesterSlashing { } } +impl TestRandom for AttesterSlashing { + fn random_for_test(rng: &mut impl RngCore) -> Self { + if rng.gen_bool(0.5) { + AttesterSlashing::Base(AttesterSlashingBase::random_for_test(rng)) + } else { + AttesterSlashing::Electra(AttesterSlashingElectra::random_for_test(rng)) + } + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/consensus/types/src/test_utils/test_random.rs b/consensus/types/src/test_utils/test_random.rs index 72a7a036cc..00355779d2 100644 --- a/consensus/types/src/test_utils/test_random.rs +++ b/consensus/types/src/test_utils/test_random.rs @@ -2,6 +2,7 @@ use crate::*; use rand::RngCore; use rand::SeedableRng; use rand_xorshift::XorShiftRng; +use smallvec::{smallvec, SmallVec}; use std::marker::PhantomData; use std::sync::Arc; @@ -118,6 +119,21 @@ where } } +impl TestRandom for SmallVec<[U; N]> +where + U: TestRandom, +{ + fn random_for_test(rng: &mut impl RngCore) -> Self { + let mut output = smallvec![]; + + for _ in 0..(usize::random_for_test(rng) % 4) { + output.push(::random_for_test(rng)); + } + + output + } +} + macro_rules! impl_test_random_for_u8_array { ($len: expr) => { impl TestRandom for [u8; $len] { From 3f82952f66251325b1d043dafe8a4b6ae51e1e8b Mon Sep 17 00:00:00 2001 From: Jimmy Chen Date: Wed, 3 Jul 2024 17:30:51 +1000 Subject: [PATCH 14/23] Broadcast validator registration to all synced beacon nodes (#5976) * Broadcast validator registration to all sycnc'd nodes. * Eagerly send out validator registrations even whe BN is not synced. --- validator_client/src/preparation_service.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/validator_client/src/preparation_service.rs b/validator_client/src/preparation_service.rs index 7aabc7d5ab..474f9f4760 100644 --- a/validator_client/src/preparation_service.rs +++ b/validator_client/src/preparation_service.rs @@ -477,7 +477,7 @@ impl PreparationService { for batch in signed.chunks(self.validator_registration_batch_size) { match self .beacon_nodes - .first_success( + .broadcast( RequireSynced::No, OfflineOnFailure::No, |beacon_node| async move { From 42d12848246647d0618c94ef6504b00cf30f7097 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Wed, 3 Jul 2024 17:30:54 +1000 Subject: [PATCH 15/23] Update MDBX (#6024) * Update MDBX --- Cargo.lock | 19 +++++++------------ slasher/Cargo.toml | 2 +- 2 files changed, 8 insertions(+), 13 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index adefb20420..6bbb79edcb 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -918,21 +918,22 @@ dependencies = [ [[package]] name = "bindgen" -version = "0.59.2" +version = "0.69.4" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "2bd2a9a458e8f4304c52c43ebb0cfbd520289f8379a52e329a38afda99bf8eb8" +checksum = "a00dc851838a2120612785d195287475a3ac45514741da670b735818822129a0" dependencies = [ - "bitflags 1.3.2", + "bitflags 2.5.0", "cexpr", "clang-sys", + "itertools", "lazy_static", "lazycell", - "peeking_take_while", "proc-macro2", "quote", "regex", "rustc-hash", "shlex", + "syn 2.0.66", ] [[package]] @@ -4427,7 +4428,7 @@ checksum = "4ec2a862134d2a7d32d7983ddcdd1c4923530833c9f2ea1a44fc5fa473989058" [[package]] name = "libmdbx" version = "0.1.4" -source = "git+https://github.com/sigp/libmdbx-rs?tag=v0.1.4#096da80a83d14343f8df833006483f48075cd135" +source = "git+https://github.com/sigp/libmdbx-rs?rev=e6ff4b9377c1619bcf0bfdf52bee5a980a432a1a#e6ff4b9377c1619bcf0bfdf52bee5a980a432a1a" dependencies = [ "bitflags 1.3.2", "byteorder", @@ -5181,7 +5182,7 @@ dependencies = [ [[package]] name = "mdbx-sys" version = "0.11.6-4" -source = "git+https://github.com/sigp/libmdbx-rs?tag=v0.1.4#096da80a83d14343f8df833006483f48075cd135" +source = "git+https://github.com/sigp/libmdbx-rs?rev=e6ff4b9377c1619bcf0bfdf52bee5a980a432a1a#e6ff4b9377c1619bcf0bfdf52bee5a980a432a1a" dependencies = [ "bindgen", "cc", @@ -6000,12 +6001,6 @@ dependencies = [ "sha2 0.10.8", ] -[[package]] -name = "peeking_take_while" -version = "0.1.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "19b17cddbe7ec3f8bc800887bab5e717348c95ea2ca0b1bf0837fb964dc67099" - [[package]] name = "pem" version = "1.1.1" diff --git a/slasher/Cargo.toml b/slasher/Cargo.toml index 01a8b9fb00..ad0bb00963 100644 --- a/slasher/Cargo.toml +++ b/slasher/Cargo.toml @@ -34,7 +34,7 @@ strum = { workspace = true } ssz_types = { workspace = true } # MDBX is pinned at the last version with Windows and macOS support. -mdbx = { package = "libmdbx", git = "https://github.com/sigp/libmdbx-rs", tag = "v0.1.4", optional = true } +mdbx = { package = "libmdbx", git = "https://github.com/sigp/libmdbx-rs", rev = "e6ff4b9377c1619bcf0bfdf52bee5a980a432a1a", optional = true } lmdb-rkv = { git = "https://github.com/sigp/lmdb-rs", rev = "f33845c6469b94265319aac0ed5085597862c27e", optional = true } lmdb-rkv-sys = { git = "https://github.com/sigp/lmdb-rs", rev = "f33845c6469b94265319aac0ed5085597862c27e", optional = true } From 4cfdd82aedeabff70ff62256acbdbbe2246542ba Mon Sep 17 00:00:00 2001 From: Age Manning Date: Wed, 3 Jul 2024 17:30:57 +1000 Subject: [PATCH 16/23] Update Sepolia Bootnodes (#6037) * Update to latest working sepolia bootnodes * Empty commit to fix `target-branch-check`. --- .../built_in_network_configs/sepolia/boot_enr.yaml | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/common/eth2_network_config/built_in_network_configs/sepolia/boot_enr.yaml b/common/eth2_network_config/built_in_network_configs/sepolia/boot_enr.yaml index f88fbc765a..22b711861f 100644 --- a/common/eth2_network_config/built_in_network_configs/sepolia/boot_enr.yaml +++ b/common/eth2_network_config/built_in_network_configs/sepolia/boot_enr.yaml @@ -1,5 +1,6 @@ -# EF Team -- enr:-Iq4QMCTfIMXnow27baRUb35Q8iiFHSIDBJh6hQM5Axohhf4b6Kr_cOCu0htQ5WvVqKvFgY28893DHAg8gnBAXsAVqmGAX53x8JggmlkgnY0gmlwhLKAlv6Jc2VjcDI1NmsxoQK6S-Cii_KmfFdUJL2TANL3ksaKUnNXvTCv1tLwXs0QgIN1ZHCCIyk -- enr:-KG4QE5OIg5ThTjkzrlVF32WT_-XT14WeJtIz2zoTqLLjQhYAmJlnk4ItSoH41_2x0RX0wTFIe5GgjRzU2u7Q1fN4vADhGV0aDKQqP7o7pAAAHAyAAAAAAAAAIJpZIJ2NIJpcISlFsStiXNlY3AyNTZrMaEC-Rrd_bBZwhKpXzFCrStKp1q_HmGOewxY3KwM8ofAj_ODdGNwgiMog3VkcIIjKA -# Teku team (Consensys) -- enr:-Ly4QFoZTWR8ulxGVsWydTNGdwEESueIdj-wB6UmmjUcm-AOPxnQi7wprzwcdo7-1jBW_JxELlUKJdJES8TDsbl1EdNlh2F0dG5ldHOI__78_v2bsV-EZXRoMpA2-lATkAAAcf__________gmlkgnY0gmlwhBLYJjGJc2VjcDI1NmsxoQI0gujXac9rMAb48NtMqtSTyHIeNYlpjkbYpWJw46PmYYhzeW5jbmV0cw-DdGNwgiMog3VkcIIjKA +# EF bootnodes +- enr:-Ku4QDZ_rCowZFsozeWr60WwLgOfHzv1Fz2cuMvJqN5iJzLxKtVjoIURY42X_YTokMi3IGstW5v32uSYZyGUXj9Q_IECh2F0dG5ldHOIAAAAAAAAAACEZXRoMpCo_ujukAAAaf__________gmlkgnY0gmlwhIpEe5iJc2VjcDI1NmsxoQNHTpFdaNSCEWiN_QqT396nb0PzcUpLe3OVtLph-AciBYN1ZHCCIy0 +- enr:-Ku4QHRyRwEPT7s0XLYzJ_EeeWvZTXBQb4UCGy1F_3m-YtCNTtDlGsCMr4UTgo4uR89pv11uM-xq4w6GKfKhqU31hTgCh2F0dG5ldHOIAAAAAAAAAACEZXRoMpCo_ujukAAAaf__________gmlkgnY0gmlwhIrFM7WJc2VjcDI1NmsxoQI4diTwChN3zAAkarf7smOHCdFb1q3DSwdiQ_Lc_FdzFIN1ZHCCIy0 +- enr:-Ku4QOkvvf0u5Hg4-HhY-SJmEyft77G5h3rUM8VF_e-Hag5cAma3jtmFoX4WElLAqdILCA-UWFRN1ZCDJJVuEHrFeLkDh2F0dG5ldHOIAAAAAAAAAACEZXRoMpCo_ujukAAAaf__________gmlkgnY0gmlwhJK-AWeJc2VjcDI1NmsxoQLFcT5VE_NMiIC8Ll7GypWDnQ4UEmuzD7hF_Hf4veDJwIN1ZHCCIy0 +- enr:-Ku4QH6tYsHKITYeHUu5kdfXgEZWI18EWk_2RtGOn1jBPlx2UlS_uF3Pm5Dx7tnjOvla_zs-wwlPgjnEOcQDWXey51QCh2F0dG5ldHOIAAAAAAAAAACEZXRoMpCo_ujukAAAaf__________gmlkgnY0gmlwhIs7Mc6Jc2VjcDI1NmsxoQIET4Mlv9YzhrYhX_H9D7aWMemUrvki6W4J2Qo0YmFMp4N1ZHCCIy0 +- enr:-Ku4QDmz-4c1InchGitsgNk4qzorWMiFUoaPJT4G0IiF8r2UaevrekND1o7fdoftNucirj7sFFTTn2-JdC2Ej0p1Mn8Ch2F0dG5ldHOIAAAAAAAAAACEZXRoMpCo_ujukAAAaf__________gmlkgnY0gmlwhKpA-liJc2VjcDI1NmsxoQMpHP5U1DK8O_JQU6FadmWbE42qEdcGlllR8HcSkkfWq4N1ZHCCIy0 From d9ad1f5bfa14c40403906ccd38c2069f274827da Mon Sep 17 00:00:00 2001 From: chonghe <44791194+chong-he@users.noreply.github.com> Date: Thu, 4 Jul 2024 11:57:23 +0800 Subject: [PATCH 17/23] Downgrade duplication log of attestation to Debug (#6007) * Test * Simplify * summary_opt * Simplify * Test * fix * move to warn * Revise * Downgrade to debug * Add block * Put inside individual tracking --- .../beacon_chain/src/validator_monitor.rs | 94 ++++++++++++++----- 1 file changed, 72 insertions(+), 22 deletions(-) diff --git a/beacon_node/beacon_chain/src/validator_monitor.rs b/beacon_node/beacon_chain/src/validator_monitor.rs index 4ce5383819..d452490081 100644 --- a/beacon_node/beacon_chain/src/validator_monitor.rs +++ b/beacon_node/beacon_chain/src/validator_monitor.rs @@ -1383,17 +1383,43 @@ impl ValidatorMonitor { }); if self.individual_tracking() { - info!( - self.log, - "Attestation included in aggregate"; - "head" => ?data.beacon_block_root, - "index" => %data.index, - "delay_ms" => %delay.as_millis(), - "epoch" => %epoch, - "slot" => %data.slot, - "src" => src, - "validator" => %id, - ); + let is_first_inclusion_aggregate = validator + .get_from_epoch_summary(epoch, |summary_opt| { + if let Some(summary) = summary_opt { + Some(summary.attestation_aggregate_inclusions == 0) + } else { + // No data for this validator: no inclusion. + Some(true) + } + }) + .unwrap_or(true); + + if is_first_inclusion_aggregate { + info!( + self.log, + "Attestation included in aggregate"; + "head" => ?data.beacon_block_root, + "index" => %data.index, + "delay_ms" => %delay.as_millis(), + "epoch" => %epoch, + "slot" => %data.slot, + "src" => src, + "validator" => %id, + ); + } else { + // Downgrade to Debug for second and onwards of logging to reduce verbosity + debug!( + self.log, + "Attestation included in aggregate"; + "head" => ?data.beacon_block_root, + "index" => %data.index, + "delay_ms" => %delay.as_millis(), + "epoch" => %epoch, + "slot" => %data.slot, + "src" => src, + "validator" => %id, + ) + }; } validator.with_epoch_summary(epoch, |summary| { @@ -1434,7 +1460,6 @@ impl ValidatorMonitor { &["block", label], ); }); - if self.individual_tracking() { metrics::set_int_gauge( &metrics::VALIDATOR_MONITOR_ATTESTATION_IN_BLOCK_DELAY_SLOTS, @@ -1442,16 +1467,41 @@ impl ValidatorMonitor { delay.as_u64() as i64, ); - info!( - self.log, - "Attestation included in block"; - "head" => ?data.beacon_block_root, - "index" => %data.index, - "inclusion_lag" => format!("{} slot(s)", delay), - "epoch" => %epoch, - "slot" => %data.slot, - "validator" => %id, - ); + let is_first_inclusion_block = validator + .get_from_epoch_summary(epoch, |summary_opt| { + if let Some(summary) = summary_opt { + Some(summary.attestation_block_inclusions == 0) + } else { + // No data for this validator: no inclusion. + Some(true) + } + }) + .unwrap_or(true); + + if is_first_inclusion_block { + info!( + self.log, + "Attestation included in block"; + "head" => ?data.beacon_block_root, + "index" => %data.index, + "inclusion_lag" => format!("{} slot(s)", delay), + "epoch" => %epoch, + "slot" => %data.slot, + "validator" => %id, + ); + } else { + // Downgrade to Debug for second and onwards of logging to reduce verbosity + debug!( + self.log, + "Attestation included in block"; + "head" => ?data.beacon_block_root, + "index" => %data.index, + "inclusion_lag" => format!("{} slot(s)", delay), + "epoch" => %epoch, + "slot" => %data.slot, + "validator" => %id, + ); + } } validator.with_epoch_summary(epoch, |summary| { From d84e3e391e336a5819b8734a3cf285f8821204a7 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Thu, 4 Jul 2024 14:27:41 +1000 Subject: [PATCH 18/23] Store pubkey cache decompressed on disk (#5897) * Support uncompressed keys in crypto/bls * Use uncompressed keys in cache * Implement DB upgrade * Implement downgrade * More logging on v20 upgrade * Revert "More logging on v20 upgrade" This reverts commit cc5789b9d36bdebe30206c656d7bc6b788089146. * Merge remote-tracking branch 'origin/unstable' into uncompressed-pubkeys * Add a little more logging * Merge remote-tracking branch 'origin/unstable' into uncompressed-pubkeys --- beacon_node/beacon_chain/src/schema_change.rs | 9 ++ .../src/schema_change/migration_schema_v20.rs | 4 + .../src/schema_change/migration_schema_v21.rs | 83 +++++++++++++++++++ .../src/validator_pubkey_cache.rs | 58 ++++++++----- beacon_node/store/src/metadata.rs | 2 +- crypto/bls/src/generic_public_key.rs | 24 ++++++ crypto/bls/src/impls/blst.rs | 23 ++++- crypto/bls/src/impls/fake_crypto.rs | 12 ++- crypto/bls/src/lib.rs | 4 +- crypto/bls/tests/tests.rs | 5 ++ 10 files changed, 199 insertions(+), 25 deletions(-) create mode 100644 beacon_node/beacon_chain/src/schema_change/migration_schema_v21.rs diff --git a/beacon_node/beacon_chain/src/schema_change.rs b/beacon_node/beacon_chain/src/schema_change.rs index 06d189a8c0..3fe75e348c 100644 --- a/beacon_node/beacon_chain/src/schema_change.rs +++ b/beacon_node/beacon_chain/src/schema_change.rs @@ -3,6 +3,7 @@ mod migration_schema_v17; mod migration_schema_v18; mod migration_schema_v19; mod migration_schema_v20; +mod migration_schema_v21; use crate::beacon_chain::BeaconChainTypes; use crate::types::ChainSpec; @@ -87,6 +88,14 @@ pub fn migrate_schema( let ops = migration_schema_v20::downgrade_from_v20::(db.clone(), log)?; db.store_schema_version_atomically(to, ops) } + (SchemaVersion(20), SchemaVersion(21)) => { + let ops = migration_schema_v21::upgrade_to_v21::(db.clone(), log)?; + db.store_schema_version_atomically(to, ops) + } + (SchemaVersion(21), SchemaVersion(20)) => { + let ops = migration_schema_v21::downgrade_from_v21::(db.clone(), log)?; + db.store_schema_version_atomically(to, ops) + } // Anything else is an error. (_, _) => Err(HotColdDBError::UnsupportedSchemaVersion { target_version: to, diff --git a/beacon_node/beacon_chain/src/schema_change/migration_schema_v20.rs b/beacon_node/beacon_chain/src/schema_change/migration_schema_v20.rs index 737fcd0a93..d556d5988d 100644 --- a/beacon_node/beacon_chain/src/schema_change/migration_schema_v20.rs +++ b/beacon_node/beacon_chain/src/schema_change/migration_schema_v20.rs @@ -11,6 +11,8 @@ pub fn upgrade_to_v20( db: Arc>, log: Logger, ) -> Result, Error> { + info!(log, "Upgrading from v19 to v20"); + // Load a V15 op pool and transform it to V20. let Some(PersistedOperationPoolV15:: { attestations_v15, @@ -52,6 +54,8 @@ pub fn downgrade_from_v20( db: Arc>, log: Logger, ) -> Result, Error> { + info!(log, "Downgrading from v20 to v19"); + // Load a V20 op pool and transform it to V15. let Some(PersistedOperationPoolV20:: { attestations, diff --git a/beacon_node/beacon_chain/src/schema_change/migration_schema_v21.rs b/beacon_node/beacon_chain/src/schema_change/migration_schema_v21.rs new file mode 100644 index 0000000000..4042d32820 --- /dev/null +++ b/beacon_node/beacon_chain/src/schema_change/migration_schema_v21.rs @@ -0,0 +1,83 @@ +use crate::beacon_chain::BeaconChainTypes; +use crate::validator_pubkey_cache::DatabasePubkey; +use slog::{info, Logger}; +use ssz::{Decode, Encode}; +use std::sync::Arc; +use store::{ + get_key_for_col, DBColumn, Error, HotColdDB, KeyValueStore, KeyValueStoreOp, StoreItem, +}; +use types::{Hash256, PublicKey}; + +const LOG_EVERY: usize = 200_000; + +pub fn upgrade_to_v21( + db: Arc>, + log: Logger, +) -> Result, Error> { + info!(log, "Upgrading from v20 to v21"); + + let mut ops = vec![]; + + // Iterate through all pubkeys and decompress them. + for (i, res) in db + .hot_db + .iter_column::(DBColumn::PubkeyCache) + .enumerate() + { + let (key, value) = res?; + let pubkey = PublicKey::from_ssz_bytes(&value)?; + let decompressed = DatabasePubkey::from_pubkey(&pubkey); + ops.push(decompressed.as_kv_store_op(key)); + + if i > 0 && i % LOG_EVERY == 0 { + info!( + log, + "Public key decompression in progress"; + "keys_decompressed" => i + ); + } + } + info!(log, "Public key decompression complete"); + + Ok(ops) +} + +pub fn downgrade_from_v21( + db: Arc>, + log: Logger, +) -> Result, Error> { + info!(log, "Downgrading from v21 to v20"); + + let mut ops = vec![]; + + // Iterate through all pubkeys and recompress them. + for (i, res) in db + .hot_db + .iter_column::(DBColumn::PubkeyCache) + .enumerate() + { + let (key, value) = res?; + let decompressed = DatabasePubkey::from_ssz_bytes(&value)?; + let (_, pubkey_bytes) = decompressed.as_pubkey().map_err(|e| Error::DBError { + message: format!("{e:?}"), + })?; + + let db_key = get_key_for_col(DBColumn::PubkeyCache.into(), key.as_bytes()); + ops.push(KeyValueStoreOp::PutKeyValue( + db_key, + pubkey_bytes.as_ssz_bytes(), + )); + + if i > 0 && i % LOG_EVERY == 0 { + info!( + log, + "Public key compression in progress"; + "keys_compressed" => i + ); + } + } + + info!(log, "Public key compression complete"); + + Ok(ops) +} diff --git a/beacon_node/beacon_chain/src/validator_pubkey_cache.rs b/beacon_node/beacon_chain/src/validator_pubkey_cache.rs index e1b5070628..576fbf0fd1 100644 --- a/beacon_node/beacon_chain/src/validator_pubkey_cache.rs +++ b/beacon_node/beacon_chain/src/validator_pubkey_cache.rs @@ -1,6 +1,9 @@ use crate::errors::BeaconChainError; use crate::{BeaconChainTypes, BeaconStore}; +use bls::PUBLIC_KEY_UNCOMPRESSED_BYTES_LEN; +use smallvec::SmallVec; use ssz::{Decode, Encode}; +use ssz_derive::{Decode, Encode}; use std::collections::HashMap; use std::marker::PhantomData; use store::{DBColumn, Error as StoreError, StoreItem, StoreOp}; @@ -49,14 +52,13 @@ impl ValidatorPubkeyCache { let mut pubkey_bytes = vec![]; for validator_index in 0.. { - if let Some(DatabasePubkey(pubkey)) = + if let Some(db_pubkey) = store.get_item(&DatabasePubkey::key_for_index(validator_index))? { - pubkeys.push((&pubkey).try_into().map_err(|e| { - BeaconChainError::ValidatorPubkeyCacheError(format!("{:?}", e)) - })?); - pubkey_bytes.push(pubkey); - indices.insert(pubkey, validator_index); + let (pk, pk_bytes) = DatabasePubkey::as_pubkey(&db_pubkey)?; + pubkeys.push(pk); + indices.insert(pk_bytes, validator_index); + pubkey_bytes.push(pk_bytes); } else { break; } @@ -104,29 +106,29 @@ impl ValidatorPubkeyCache { self.indices.reserve(validator_keys.len()); let mut store_ops = Vec::with_capacity(validator_keys.len()); - for pubkey in validator_keys { + for pubkey_bytes in validator_keys { let i = self.pubkeys.len(); - if self.indices.contains_key(&pubkey) { + if self.indices.contains_key(&pubkey_bytes) { return Err(BeaconChainError::DuplicateValidatorPublicKey); } + let pubkey = (&pubkey_bytes) + .try_into() + .map_err(BeaconChainError::InvalidValidatorPubkeyBytes)?; + // Stage the new validator key for writing to disk. // It will be committed atomically when the block that introduced it is written to disk. // Notably it is NOT written while the write lock on the cache is held. // See: https://github.com/sigp/lighthouse/issues/2327 store_ops.push(StoreOp::KeyValueOp( - DatabasePubkey(pubkey).as_kv_store_op(DatabasePubkey::key_for_index(i)), + DatabasePubkey::from_pubkey(&pubkey) + .as_kv_store_op(DatabasePubkey::key_for_index(i)), )); - self.pubkeys.push( - (&pubkey) - .try_into() - .map_err(BeaconChainError::InvalidValidatorPubkeyBytes)?, - ); - self.pubkey_bytes.push(pubkey); - - self.indices.insert(pubkey, i); + self.pubkeys.push(pubkey); + self.pubkey_bytes.push(pubkey_bytes); + self.indices.insert(pubkey_bytes, i); } Ok(store_ops) @@ -166,7 +168,10 @@ impl ValidatorPubkeyCache { /// Wrapper for a public key stored in the database. /// /// Keyed by the validator index as `Hash256::from_low_u64_be(index)`. -struct DatabasePubkey(PublicKeyBytes); +#[derive(Encode, Decode)] +pub struct DatabasePubkey { + pubkey: SmallVec<[u8; PUBLIC_KEY_UNCOMPRESSED_BYTES_LEN]>, +} impl StoreItem for DatabasePubkey { fn db_column() -> DBColumn { @@ -174,11 +179,11 @@ impl StoreItem for DatabasePubkey { } fn as_store_bytes(&self) -> Vec { - self.0.as_ssz_bytes() + self.as_ssz_bytes() } fn from_store_bytes(bytes: &[u8]) -> Result { - Ok(Self(PublicKeyBytes::from_ssz_bytes(bytes)?)) + Ok(Self::from_ssz_bytes(bytes)?) } } @@ -186,6 +191,19 @@ impl DatabasePubkey { fn key_for_index(index: usize) -> Hash256 { Hash256::from_low_u64_be(index as u64) } + + pub fn from_pubkey(pubkey: &PublicKey) -> Self { + Self { + pubkey: pubkey.serialize_uncompressed().into(), + } + } + + pub fn as_pubkey(&self) -> Result<(PublicKey, PublicKeyBytes), BeaconChainError> { + let pubkey = PublicKey::deserialize_uncompressed(&self.pubkey) + .map_err(BeaconChainError::InvalidValidatorPubkeyBytes)?; + let pubkey_bytes = pubkey.compress(); + Ok((pubkey, pubkey_bytes)) + } } #[cfg(test)] diff --git a/beacon_node/store/src/metadata.rs b/beacon_node/store/src/metadata.rs index 116926ad3f..a22dc4aab4 100644 --- a/beacon_node/store/src/metadata.rs +++ b/beacon_node/store/src/metadata.rs @@ -4,7 +4,7 @@ use ssz::{Decode, Encode}; use ssz_derive::{Decode, Encode}; use types::{Checkpoint, Hash256, Slot}; -pub const CURRENT_SCHEMA_VERSION: SchemaVersion = SchemaVersion(20); +pub const CURRENT_SCHEMA_VERSION: SchemaVersion = SchemaVersion(21); // All the keys that get stored under the `BeaconMeta` column. // diff --git a/crypto/bls/src/generic_public_key.rs b/crypto/bls/src/generic_public_key.rs index 462e4cb2cb..80b42dfa71 100644 --- a/crypto/bls/src/generic_public_key.rs +++ b/crypto/bls/src/generic_public_key.rs @@ -11,6 +11,9 @@ use tree_hash::TreeHash; /// The byte-length of a BLS public key when serialized in compressed form. pub const PUBLIC_KEY_BYTES_LEN: usize = 48; +/// The byte-length of a BLS public key when serialized in uncompressed form. +pub const PUBLIC_KEY_UNCOMPRESSED_BYTES_LEN: usize = 96; + /// Represents the public key at infinity. pub const INFINITY_PUBLIC_KEY: [u8; PUBLIC_KEY_BYTES_LEN] = [ 0xc0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, @@ -23,8 +26,17 @@ pub trait TPublicKey: Sized + Clone { /// Serialize `self` as compressed bytes. fn serialize(&self) -> [u8; PUBLIC_KEY_BYTES_LEN]; + /// Serialize `self` as uncompressed bytes. + fn serialize_uncompressed(&self) -> [u8; PUBLIC_KEY_UNCOMPRESSED_BYTES_LEN]; + /// Deserialize `self` from compressed bytes. fn deserialize(bytes: &[u8]) -> Result; + + /// Deserialize `self` from uncompressed bytes. + /// + /// This function *does not* perform thorough checks of the input bytes and should only be + /// used with bytes output from `Self::serialize_uncompressed`. + fn deserialize_uncompressed(bytes: &[u8]) -> Result; } /// A BLS public key that is generic across some BLS point (`Pub`). @@ -65,6 +77,11 @@ where self.point.serialize() } + /// Serialize `self` as uncompressed bytes. + pub fn serialize_uncompressed(&self) -> [u8; PUBLIC_KEY_UNCOMPRESSED_BYTES_LEN] { + self.point.serialize_uncompressed() + } + /// Deserialize `self` from compressed bytes. pub fn deserialize(bytes: &[u8]) -> Result { if bytes == &INFINITY_PUBLIC_KEY[..] { @@ -75,6 +92,13 @@ where }) } } + + /// Deserialize `self` from compressed bytes. + pub fn deserialize_uncompressed(bytes: &[u8]) -> Result { + Ok(Self { + point: Pub::deserialize_uncompressed(bytes)?, + }) + } } impl Eq for GenericPublicKey {} diff --git a/crypto/bls/src/impls/blst.rs b/crypto/bls/src/impls/blst.rs index 0049d79cc5..54c7ad2944 100644 --- a/crypto/bls/src/impls/blst.rs +++ b/crypto/bls/src/impls/blst.rs @@ -1,10 +1,12 @@ use crate::{ generic_aggregate_public_key::TAggregatePublicKey, generic_aggregate_signature::TAggregateSignature, - generic_public_key::{GenericPublicKey, TPublicKey, PUBLIC_KEY_BYTES_LEN}, + generic_public_key::{ + GenericPublicKey, TPublicKey, PUBLIC_KEY_BYTES_LEN, PUBLIC_KEY_UNCOMPRESSED_BYTES_LEN, + }, generic_secret_key::TSecretKey, generic_signature::{TSignature, SIGNATURE_BYTES_LEN}, - Error, Hash256, ZeroizeHash, INFINITY_SIGNATURE, + BlstError, Error, Hash256, ZeroizeHash, INFINITY_SIGNATURE, }; pub use blst::min_pk as blst_core; use blst::{blst_scalar, BLST_ERROR}; @@ -121,6 +123,10 @@ impl TPublicKey for blst_core::PublicKey { self.compress() } + fn serialize_uncompressed(&self) -> [u8; PUBLIC_KEY_UNCOMPRESSED_BYTES_LEN] { + blst_core::PublicKey::serialize(self) + } + fn deserialize(bytes: &[u8]) -> Result { // key_validate accepts uncompressed bytes too so enforce byte length here. // It also does subgroup checks, noting infinity check is done in `generic_public_key.rs`. @@ -132,6 +138,19 @@ impl TPublicKey for blst_core::PublicKey { } Self::key_validate(bytes).map_err(Into::into) } + + fn deserialize_uncompressed(bytes: &[u8]) -> Result { + if bytes.len() != PUBLIC_KEY_UNCOMPRESSED_BYTES_LEN { + return Err(Error::InvalidByteLength { + got: bytes.len(), + expected: PUBLIC_KEY_UNCOMPRESSED_BYTES_LEN, + }); + } + // Ensure we use the `blst` function rather than the one from this trait. + let result: Result = Self::deserialize(bytes); + let key = result?; + Ok(key) + } } /// A wrapper that allows for `PartialEq` and `Clone` impls. diff --git a/crypto/bls/src/impls/fake_crypto.rs b/crypto/bls/src/impls/fake_crypto.rs index f2d8b79b98..a09fb347e6 100644 --- a/crypto/bls/src/impls/fake_crypto.rs +++ b/crypto/bls/src/impls/fake_crypto.rs @@ -1,7 +1,9 @@ use crate::{ generic_aggregate_public_key::TAggregatePublicKey, generic_aggregate_signature::TAggregateSignature, - generic_public_key::{GenericPublicKey, TPublicKey, PUBLIC_KEY_BYTES_LEN}, + generic_public_key::{ + GenericPublicKey, TPublicKey, PUBLIC_KEY_BYTES_LEN, PUBLIC_KEY_UNCOMPRESSED_BYTES_LEN, + }, generic_secret_key::{TSecretKey, SECRET_KEY_BYTES_LEN}, generic_signature::{TSignature, SIGNATURE_BYTES_LEN}, Error, Hash256, ZeroizeHash, INFINITY_PUBLIC_KEY, INFINITY_SIGNATURE, @@ -46,11 +48,19 @@ impl TPublicKey for PublicKey { self.0 } + fn serialize_uncompressed(&self) -> [u8; PUBLIC_KEY_UNCOMPRESSED_BYTES_LEN] { + panic!("fake_crypto does not support uncompressed keys") + } + fn deserialize(bytes: &[u8]) -> Result { let mut pubkey = Self::infinity(); pubkey.0[..].copy_from_slice(&bytes[0..PUBLIC_KEY_BYTES_LEN]); Ok(pubkey) } + + fn deserialize_uncompressed(_: &[u8]) -> Result { + panic!("fake_crypto does not support uncompressed keys") + } } impl Eq for PublicKey {} diff --git a/crypto/bls/src/lib.rs b/crypto/bls/src/lib.rs index fef9804b78..af269b943d 100644 --- a/crypto/bls/src/lib.rs +++ b/crypto/bls/src/lib.rs @@ -33,7 +33,9 @@ mod zeroize_hash; pub mod impls; -pub use generic_public_key::{INFINITY_PUBLIC_KEY, PUBLIC_KEY_BYTES_LEN}; +pub use generic_public_key::{ + INFINITY_PUBLIC_KEY, PUBLIC_KEY_BYTES_LEN, PUBLIC_KEY_UNCOMPRESSED_BYTES_LEN, +}; pub use generic_secret_key::SECRET_KEY_BYTES_LEN; pub use generic_signature::{INFINITY_SIGNATURE, SIGNATURE_BYTES_LEN}; pub use get_withdrawal_credentials::get_withdrawal_credentials; diff --git a/crypto/bls/tests/tests.rs b/crypto/bls/tests/tests.rs index 478c1b7dc2..dac2e97f40 100644 --- a/crypto/bls/tests/tests.rs +++ b/crypto/bls/tests/tests.rs @@ -341,6 +341,11 @@ macro_rules! test_suite { .assert_single_message_verify(true) } + #[test] + fn deserialize_infinity_public_key() { + PublicKey::deserialize(&bls::INFINITY_PUBLIC_KEY).unwrap_err(); + } + /// A helper struct to make it easer to deal with `SignatureSet` lifetimes. struct OwnedSignatureSet { signature: AggregateSignature, From 8e0bc9a40b04042c38abe9f00327a663507ec3a1 Mon Sep 17 00:00:00 2001 From: chonghe <44791194+chong-he@users.noreply.github.com> Date: Fri, 5 Jul 2024 10:37:54 +0800 Subject: [PATCH 19/23] Record `BEACON_BLOCK_DELAY_GOSSIP` metric only after a bock is verified (#6046) * Only record metrics after block verified --- .../src/network_beacon_processor/gossip_methods.rs | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) 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 8baa0f773d..07173f2416 100644 --- a/beacon_node/network/src/network_beacon_processor/gossip_methods.rs +++ b/beacon_node/network/src/network_beacon_processor/gossip_methods.rs @@ -907,17 +907,19 @@ impl NetworkBeaconProcessor { get_block_delay_ms(seen_duration, block.message(), &self.chain.slot_clock); // Log metrics to track delay from other nodes on the network. - metrics::set_gauge( - &metrics::BEACON_BLOCK_DELAY_GOSSIP, - block_delay.as_millis() as i64, - ); - let verification_result = self .chain .clone() .verify_block_for_gossip(block.clone()) .await; + if verification_result.is_ok() { + metrics::set_gauge( + &metrics::BEACON_BLOCK_DELAY_GOSSIP, + block_delay.as_millis() as i64, + ); + } + let block_root = if let Ok(verified_block) = &verification_result { verified_block.block_root } else { From ef956e61e5d7d2b196f36509b714909541a45555 Mon Sep 17 00:00:00 2001 From: Lion - dapplion <35266934+dapplion@users.noreply.github.com> Date: Fri, 5 Jul 2024 06:05:40 +0200 Subject: [PATCH 20/23] Drop skip too large condition for liveness safety (#6014) * Drop skip too large condition for liveness safety --- beacon_node/beacon_chain/src/beacon_chain.rs | 18 +----------------- beacon_node/beacon_chain/src/errors.rs | 6 ------ 2 files changed, 1 insertion(+), 23 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 66e2964669..7f09430227 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -114,7 +114,7 @@ use std::collections::HashSet; use std::io::prelude::*; use std::marker::PhantomData; use std::sync::Arc; -use std::time::{Duration, Instant}; +use std::time::Duration; use store::iter::{BlockRootsIterator, ParentRootBlockIterator, StateRootsIterator}; use store::{ DatabaseBlock, Error as DBError, HotColdDB, KeyValueStore, KeyValueStoreOp, StoreItem, StoreOp, @@ -1410,10 +1410,6 @@ impl BeaconChain { ) } - let start_slot = head_state.slot(); - let task_start = Instant::now(); - let max_task_runtime = Duration::from_secs(self.spec.seconds_per_slot); - let head_state_slot = head_state.slot(); let mut state = head_state; @@ -1423,18 +1419,6 @@ impl BeaconChain { }; while state.slot() < slot { - // Do not allow and forward state skip that takes longer than the maximum task duration. - // - // This is a protection against nodes doing too much work when they're not synced - // to a chain. - if task_start + max_task_runtime < Instant::now() { - return Err(Error::StateSkipTooLarge { - start_slot, - requested_slot: slot, - max_task_runtime, - }); - } - // Note: supplying some `state_root` when it is known would be a cheap and easy // optimization. match per_slot_processing(&mut state, skip_state_root, &self.spec) { diff --git a/beacon_node/beacon_chain/src/errors.rs b/beacon_node/beacon_chain/src/errors.rs index 3d61d4f32d..819de1f5c1 100644 --- a/beacon_node/beacon_chain/src/errors.rs +++ b/beacon_node/beacon_chain/src/errors.rs @@ -28,7 +28,6 @@ use state_processing::{ state_advance::Error as StateAdvanceError, BlockProcessingError, BlockReplayError, EpochProcessingError, SlotProcessingError, }; -use std::time::Duration; use task_executor::ShutdownReason; use tokio::task::JoinError; use types::milhouse::Error as MilhouseError; @@ -77,11 +76,6 @@ pub enum BeaconChainError { ProposerSlashingValidationError(ProposerSlashingValidationError), AttesterSlashingValidationError(AttesterSlashingValidationError), BlsExecutionChangeValidationError(BlsExecutionChangeValidationError), - StateSkipTooLarge { - start_slot: Slot, - requested_slot: Slot, - max_task_runtime: Duration, - }, MissingFinalizedStateRoot(Slot), /// Returned when an internal check fails, indicating corrupt data. InvariantViolated(String), From 94d55be6faa3655b141ead6159da0705bf41c80a Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Fri, 5 Jul 2024 14:30:15 +1000 Subject: [PATCH 21/23] Increase penalty for old block gossip spam (#6050) * Increase penalty for old block gossip spam --- .../gossip_methods.rs | 30 +++++++++++++++---- 1 file changed, 25 insertions(+), 5 deletions(-) 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 07173f2416..ab25053258 100644 --- a/beacon_node/network/src/network_beacon_processor/gossip_methods.rs +++ b/beacon_node/network/src/network_beacon_processor/gossip_methods.rs @@ -1012,11 +1012,12 @@ impl NetworkBeaconProcessor { self.propagate_validation_result(message_id, peer_id, MessageAcceptance::Ignore); return None; } - Err(e @ BlockError::FutureSlot { .. }) - | Err(e @ BlockError::WouldRevertFinalizedSlot { .. }) - | Err(e @ BlockError::NotFinalizedDescendant { .. }) => { - debug!(self.log, "Could not verify block for gossip. Ignoring the block"; - "error" => %e); + Err(e @ BlockError::FutureSlot { .. }) => { + debug!( + self.log, + "Could not verify block for gossip. Ignoring the block"; + "error" => %e + ); // Prevent recurring behaviour by penalizing the peer slightly. self.gossip_penalize_peer( peer_id, @@ -1026,6 +1027,25 @@ impl NetworkBeaconProcessor { self.propagate_validation_result(message_id, peer_id, MessageAcceptance::Ignore); return None; } + Err(e @ BlockError::WouldRevertFinalizedSlot { .. }) + | Err(e @ BlockError::NotFinalizedDescendant { .. }) => { + debug!( + self.log, + "Could not verify block for gossip. Ignoring the block"; + "error" => %e + ); + // The spec says we must IGNORE these blocks but there's no reason for an honest + // and non-buggy client to be gossiping blocks that blatantly conflict with + // finalization. Old versions of Erigon/Caplin are known to gossip pre-finalization + // blocks and we want to isolate them to encourage an update. + self.gossip_penalize_peer( + peer_id, + PeerAction::LowToleranceError, + "gossip_block_low", + ); + self.propagate_validation_result(message_id, peer_id, MessageAcceptance::Ignore); + return None; + } Err(ref e @ BlockError::ExecutionPayloadError(ref epe)) if !epe.penalize_peer() => { debug!(self.log, "Could not verify block for gossip. Ignoring the block"; "error" => %e); From 5b2edfa0bd8489beb8f874e1d70085b0d20a312e Mon Sep 17 00:00:00 2001 From: scafe3 <161312198+scafe3@users.noreply.github.com> Date: Fri, 5 Jul 2024 07:49:07 +0100 Subject: [PATCH 22/23] Show `blst` hardware support in `lighthouse --version` (#6039) * Show blst hardware support in lighthouse --version * fix: detect adx extension in runtime * fix * add arm detect --- lighthouse/src/main.rs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/lighthouse/src/main.rs b/lighthouse/src/main.rs index 47b44d3828..a7521d5f8c 100644 --- a/lighthouse/src/main.rs +++ b/lighthouse/src/main.rs @@ -26,12 +26,14 @@ lazy_static! { pub static ref LONG_VERSION: String = format!( "{}\n\ BLS library: {}\n\ + BLS hardware acceleration: {}\n\ SHA256 hardware acceleration: {}\n\ Allocator: {}\n\ Profile: {}\n\ Specs: mainnet (true), minimal ({}), gnosis ({})", SHORT_VERSION.as_str(), bls_library_name(), + bls_hardware_acceleration(), have_sha_extensions(), allocator_name(), build_profile_name(), @@ -50,6 +52,15 @@ fn bls_library_name() -> &'static str { } } +#[inline(always)] +fn bls_hardware_acceleration() -> bool { + #[cfg(target_arch = "x86_64")] + return std::is_x86_feature_detected!("adx"); + + #[cfg(target_arch = "aarch64")] + return std::arch::is_aarch64_feature_detected!("neon"); +} + fn allocator_name() -> &'static str { if cfg!(target_os = "windows") { "system" From 48c55ae2952bfc92618cba04e38ed9cfd96918cf Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Mon, 8 Jul 2024 20:33:50 +1000 Subject: [PATCH 23/23] Delete `cached_tree_hash` (#6060) * Delete `cached_tree_hash` --- Cargo.lock | 16 - Cargo.toml | 2 - consensus/cached_tree_hash/Cargo.toml | 21 - consensus/cached_tree_hash/src/cache.rs | 237 --------- consensus/cached_tree_hash/src/cache_arena.rs | 498 ------------------ consensus/cached_tree_hash/src/impls.rs | 138 ----- consensus/cached_tree_hash/src/lib.rs | 46 -- consensus/cached_tree_hash/src/test.rs | 153 ------ consensus/types/Cargo.toml | 1 - consensus/types/src/beacon_state.rs | 7 - consensus/types/src/historical_summary.rs | 49 +- consensus/types/src/lib.rs | 2 - consensus/types/src/participation_list.rs | 55 -- 13 files changed, 1 insertion(+), 1224 deletions(-) delete mode 100644 consensus/cached_tree_hash/Cargo.toml delete mode 100644 consensus/cached_tree_hash/src/cache.rs delete mode 100644 consensus/cached_tree_hash/src/cache_arena.rs delete mode 100644 consensus/cached_tree_hash/src/impls.rs delete mode 100644 consensus/cached_tree_hash/src/lib.rs delete mode 100644 consensus/cached_tree_hash/src/test.rs delete mode 100644 consensus/types/src/participation_list.rs diff --git a/Cargo.lock b/Cargo.lock index 6bbb79edcb..de325243c5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1169,21 +1169,6 @@ dependencies = [ "libc", ] -[[package]] -name = "cached_tree_hash" -version = "0.1.0" -dependencies = [ - "ethereum-types 0.14.1", - "ethereum_hashing", - "ethereum_ssz", - "ethereum_ssz_derive", - "quickcheck", - "quickcheck_macros", - "smallvec", - "ssz_types", - "tree_hash", -] - [[package]] name = "camino" version = "1.1.7" @@ -8776,7 +8761,6 @@ dependencies = [ "arbitrary", "beacon_chain", "bls", - "cached_tree_hash", "compare_fields", "compare_fields_derive", "criterion", diff --git a/Cargo.toml b/Cargo.toml index c09a3af7ce..eedc47470e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -51,7 +51,6 @@ members = [ "database_manager", - "consensus/cached_tree_hash", "consensus/int_to_bytes", "consensus/fork_choice", "consensus/proto_array", @@ -188,7 +187,6 @@ beacon_chain = { path = "beacon_node/beacon_chain" } beacon_node = { path = "beacon_node" } beacon_processor = { path = "beacon_node/beacon_processor" } bls = { path = "crypto/bls" } -cached_tree_hash = { path = "consensus/cached_tree_hash" } clap_utils = { path = "common/clap_utils" } compare_fields = { path = "common/compare_fields" } deposit_contract = { path = "common/deposit_contract" } diff --git a/consensus/cached_tree_hash/Cargo.toml b/consensus/cached_tree_hash/Cargo.toml deleted file mode 100644 index 05edc34856..0000000000 --- a/consensus/cached_tree_hash/Cargo.toml +++ /dev/null @@ -1,21 +0,0 @@ -[package] -name = "cached_tree_hash" -version = "0.1.0" -authors = ["Michael Sproul "] -edition = { workspace = true } - -[dependencies] -ethereum-types = { workspace = true } -ssz_types = { workspace = true } -ethereum_hashing = { workspace = true } -ethereum_ssz_derive = { workspace = true } -ethereum_ssz = { workspace = true } -tree_hash = { workspace = true } -smallvec = { workspace = true } - -[dev-dependencies] -quickcheck = { workspace = true } -quickcheck_macros = { workspace = true } - -[features] -arbitrary = ["ethereum-types/arbitrary"] diff --git a/consensus/cached_tree_hash/src/cache.rs b/consensus/cached_tree_hash/src/cache.rs deleted file mode 100644 index 450128f15e..0000000000 --- a/consensus/cached_tree_hash/src/cache.rs +++ /dev/null @@ -1,237 +0,0 @@ -use crate::cache_arena; -use crate::SmallVec8; -use crate::{Error, Hash256}; -use ethereum_hashing::{hash32_concat, ZERO_HASHES}; -use smallvec::smallvec; -use ssz_derive::{Decode, Encode}; -use tree_hash::BYTES_PER_CHUNK; - -type CacheArena = cache_arena::CacheArena; -type CacheArenaAllocation = cache_arena::CacheArenaAllocation; - -/// Sparse Merkle tree suitable for tree hashing vectors and lists. -#[derive(Debug, PartialEq, Clone, Default, Encode, Decode)] -pub struct TreeHashCache { - pub initialized: bool, - /// Depth is such that the tree has a capacity for 2^depth leaves - depth: usize, - /// Sparse layers. - /// - /// The leaves are contained in `self.layers[self.depth]`, and each other layer `i` - /// contains the parents of the nodes in layer `i + 1`. - layers: SmallVec8, -} - -impl TreeHashCache { - /// Create a new cache with the given `depth` with enough nodes allocated to suit `leaves`. All - /// leaves are set to `Hash256::zero()`. - pub fn new(arena: &mut CacheArena, depth: usize, leaves: usize) -> Self { - let mut layers = SmallVec8::with_capacity(depth + 1); - - for i in 0..=depth { - let vec = arena.alloc(); - vec.extend_with_vec( - arena, - smallvec![Hash256::zero(); nodes_per_layer(i, depth, leaves)], - ) - .expect("A newly allocated sub-arena cannot fail unless it has reached max capacity"); - - layers.push(vec) - } - - TreeHashCache { - initialized: false, - depth, - layers, - } - } - - /// Compute the updated Merkle root for the given `leaves`. - pub fn recalculate_merkle_root( - &mut self, - arena: &mut CacheArena, - leaves: impl ExactSizeIterator, - ) -> Result { - let dirty_indices = self.update_leaves(arena, leaves)?; - self.update_merkle_root(arena, dirty_indices) - } - - /// Phase 1 of the algorithm: compute the indices of all dirty leaves. - pub fn update_leaves( - &mut self, - arena: &mut CacheArena, - mut leaves: impl ExactSizeIterator, - ) -> Result, Error> { - let new_leaf_count = leaves.len(); - - if new_leaf_count < self.leaves().len(arena)? { - return Err(Error::CannotShrink); - } else if new_leaf_count > 2usize.pow(self.depth as u32) { - return Err(Error::TooManyLeaves); - } - - let mut dirty = SmallVec8::new(); - - // Update the existing leaves - self.leaves() - .iter_mut(arena)? - .enumerate() - .zip(&mut leaves) - .for_each(|((i, leaf), new_leaf)| { - if !self.initialized || leaf.as_bytes() != new_leaf { - leaf.assign_from_slice(&new_leaf); - dirty.push(i); - } - }); - - // Push the rest of the new leaves (if any) - dirty.extend(self.leaves().len(arena)?..new_leaf_count); - self.leaves() - .extend_with_vec(arena, leaves.map(|l| Hash256::from_slice(&l)).collect())?; - - Ok(dirty) - } - - /// Phase 2: propagate changes upwards from the leaves of the tree, and compute the root. - /// - /// Returns an error if `dirty_indices` is inconsistent with the cache. - pub fn update_merkle_root( - &mut self, - arena: &mut CacheArena, - mut dirty_indices: SmallVec8, - ) -> Result { - if dirty_indices.is_empty() { - return Ok(self.root(arena)); - } - - let mut depth = self.depth; - - while depth > 0 { - let new_dirty_indices = lift_dirty(&dirty_indices); - - for &idx in &new_dirty_indices { - let left_idx = 2 * idx; - let right_idx = left_idx + 1; - - let left = self.layers[depth] - .get(arena, left_idx)? - .ok_or(Error::MissingLeftIdx(left_idx))?; - let right = self.layers[depth] - .get(arena, right_idx)? - .copied() - .unwrap_or_else(|| Hash256::from_slice(&ZERO_HASHES[self.depth - depth])); - - let new_hash = hash32_concat(left.as_bytes(), right.as_bytes()); - - match self.layers[depth - 1].get_mut(arena, idx)? { - Some(hash) => { - hash.assign_from_slice(&new_hash); - } - None => { - // Parent layer should already contain nodes for all non-dirty indices - if idx != self.layers[depth - 1].len(arena)? { - return Err(Error::CacheInconsistent); - } - self.layers[depth - 1].push(arena, Hash256::from_slice(&new_hash))?; - } - } - } - - dirty_indices = new_dirty_indices; - depth -= 1; - } - - self.initialized = true; - - Ok(self.root(arena)) - } - - /// Get the root of this cache, without doing any updates/computation. - pub fn root(&self, arena: &CacheArena) -> Hash256 { - self.layers[0] - .get(arena, 0) - .expect("cached tree should have a root layer") - .copied() - .unwrap_or_else(|| Hash256::from_slice(&ZERO_HASHES[self.depth])) - } - - pub fn leaves(&mut self) -> &mut CacheArenaAllocation { - &mut self.layers[self.depth] - } -} - -/// Compute the dirty indices for one layer up. -fn lift_dirty(dirty_indices: &[usize]) -> SmallVec8 { - let mut new_dirty = SmallVec8::with_capacity(dirty_indices.len()); - - for index in dirty_indices { - new_dirty.push(index / 2) - } - - new_dirty.dedup(); - new_dirty -} - -/// Returns the number of nodes that should be at each layer of a tree with the given `depth` and -/// number of `leaves`. -/// -/// Note: the top-most layer is `0` and a tree that has 8 leaves (4 layers) has a depth of 3 (_not_ -/// a depth of 4). -/// -/// ## Example -/// -/// Consider the following tree that has `depth = 3` and `leaves = 5`. -/// -///```ignore -/// 0 o <-- height 0 has 1 node -/// / \ -/// 1 o o <-- height 1 has 2 nodes -/// / \ / -/// 2 o o o <-- height 2 has 3 nodes -/// /\ /\ / -/// 3 o o o o o <-- height 3 have 5 nodes -/// ``` -fn nodes_per_layer(layer: usize, depth: usize, leaves: usize) -> usize { - if layer == depth { - leaves - } else { - let leaves_per_node = 1 << (depth - layer); - (leaves + leaves_per_node - 1) / leaves_per_node - } -} - -#[cfg(test)] -mod test { - use super::*; - - #[test] - fn zero_leaves() { - let arena = &mut CacheArena::default(); - - let depth = 3; - let num_leaves = 0; - - let mut cache = TreeHashCache::new(arena, depth, num_leaves); - let leaves: Vec<[u8; BYTES_PER_CHUNK]> = vec![]; - - cache - .recalculate_merkle_root(arena, leaves.into_iter()) - .expect("should calculate root"); - } - - #[test] - fn test_node_per_layer_unbalanced_tree() { - assert_eq!(nodes_per_layer(0, 3, 5), 1); - assert_eq!(nodes_per_layer(1, 3, 5), 2); - assert_eq!(nodes_per_layer(2, 3, 5), 3); - assert_eq!(nodes_per_layer(3, 3, 5), 5); - } - - #[test] - fn test_node_per_layer_balanced_tree() { - assert_eq!(nodes_per_layer(0, 3, 8), 1); - assert_eq!(nodes_per_layer(1, 3, 8), 2); - assert_eq!(nodes_per_layer(2, 3, 8), 4); - assert_eq!(nodes_per_layer(3, 3, 8), 8); - } -} diff --git a/consensus/cached_tree_hash/src/cache_arena.rs b/consensus/cached_tree_hash/src/cache_arena.rs deleted file mode 100644 index 42819e8df5..0000000000 --- a/consensus/cached_tree_hash/src/cache_arena.rs +++ /dev/null @@ -1,498 +0,0 @@ -use crate::SmallVec8; -use ssz::{Decode, Encode}; -use ssz_derive::{Decode, Encode}; -use std::cmp::Ordering; -use std::marker::PhantomData; -use std::ops::Range; - -#[derive(Debug, PartialEq, Clone)] -pub enum Error { - UnknownAllocId(usize), - OffsetOverflow, - OffsetUnderflow, - RangeOverFlow, -} - -/// Inspired by the `TypedArena` crate, the `CachedArena` provides a single contiguous memory -/// allocation from which smaller allocations can be produced. In effect this allows for having -/// many `Vec`-like objects all stored contiguously on the heap with the aim of reducing memory -/// fragmentation. -/// -/// Because all of the allocations are stored in one big `Vec`, resizing any of the allocations -/// will mean all items to the right of that allocation will be moved. -#[derive(Debug, PartialEq, Clone, Default, Encode, Decode)] -pub struct CacheArena { - /// The backing array, storing cached values. - backing: Vec, - /// A list of offsets indicating the start of each allocation. - offsets: Vec, -} - -impl CacheArena { - /// Instantiate self with a backing array of the given `capacity`. - pub fn with_capacity(capacity: usize) -> Self { - Self { - backing: Vec::with_capacity(capacity), - offsets: vec![], - } - } - - /// Produce an allocation of zero length at the end of the backing array. - pub fn alloc(&mut self) -> CacheArenaAllocation { - let alloc_id = self.offsets.len(); - self.offsets.push(self.backing.len()); - - CacheArenaAllocation { - alloc_id, - _phantom: PhantomData, - } - } - - /// Update `self.offsets` to reflect an allocation increasing in size. - fn grow(&mut self, alloc_id: usize, grow_by: usize) -> Result<(), Error> { - if alloc_id < self.offsets.len() { - self.offsets - .iter_mut() - .skip(alloc_id + 1) - .try_for_each(|offset| { - *offset = offset.checked_add(grow_by).ok_or(Error::OffsetOverflow)?; - - Ok(()) - }) - } else { - Err(Error::UnknownAllocId(alloc_id)) - } - } - - /// Update `self.offsets` to reflect an allocation decreasing in size. - fn shrink(&mut self, alloc_id: usize, shrink_by: usize) -> Result<(), Error> { - if alloc_id < self.offsets.len() { - self.offsets - .iter_mut() - .skip(alloc_id + 1) - .try_for_each(|offset| { - *offset = offset - .checked_sub(shrink_by) - .ok_or(Error::OffsetUnderflow)?; - - Ok(()) - }) - } else { - Err(Error::UnknownAllocId(alloc_id)) - } - } - - /// Similar to `Vec::splice`, however the range is relative to some allocation (`alloc_id`) and - /// the replaced items are not returned (i.e., it is forgetful). - /// - /// To reiterate, the given `range` should be relative to the given `alloc_id`, not - /// `self.backing`. E.g., if the allocation has an offset of `20` and the range is `0..1`, then - /// the splice will translate to `self.backing[20..21]`. - fn splice_forgetful>( - &mut self, - alloc_id: usize, - range: Range, - replace_with: I, - ) -> Result<(), Error> { - let offset = *self - .offsets - .get(alloc_id) - .ok_or(Error::UnknownAllocId(alloc_id))?; - let start = range - .start - .checked_add(offset) - .ok_or(Error::RangeOverFlow)?; - let end = range.end.checked_add(offset).ok_or(Error::RangeOverFlow)?; - - let prev_len = self.backing.len(); - - self.backing.splice(start..end, replace_with); - - match prev_len.cmp(&self.backing.len()) { - Ordering::Greater => self.shrink(alloc_id, prev_len - self.backing.len())?, - Ordering::Less => self.grow(alloc_id, self.backing.len() - prev_len)?, - Ordering::Equal => {} - } - - Ok(()) - } - - /// Returns the length of the specified allocation. - fn len(&self, alloc_id: usize) -> Result { - let start = self - .offsets - .get(alloc_id) - .ok_or(Error::UnknownAllocId(alloc_id))?; - let end = self - .offsets - .get(alloc_id + 1) - .copied() - .unwrap_or(self.backing.len()); - - Ok(end - start) - } - - /// Get the value at position `i`, relative to the offset at `alloc_id`. - fn get(&self, alloc_id: usize, i: usize) -> Result, Error> { - if i < self.len(alloc_id)? { - let offset = self - .offsets - .get(alloc_id) - .ok_or(Error::UnknownAllocId(alloc_id))?; - Ok(self.backing.get(i + offset)) - } else { - Ok(None) - } - } - - /// Mutably get the value at position `i`, relative to the offset at `alloc_id`. - fn get_mut(&mut self, alloc_id: usize, i: usize) -> Result, Error> { - if i < self.len(alloc_id)? { - let offset = self - .offsets - .get(alloc_id) - .ok_or(Error::UnknownAllocId(alloc_id))?; - Ok(self.backing.get_mut(i + offset)) - } else { - Ok(None) - } - } - - /// Returns the range in `self.backing` that is occupied by some allocation. - fn range(&self, alloc_id: usize) -> Result, Error> { - let start = *self - .offsets - .get(alloc_id) - .ok_or(Error::UnknownAllocId(alloc_id))?; - let end = self - .offsets - .get(alloc_id + 1) - .copied() - .unwrap_or(self.backing.len()); - - Ok(start..end) - } - - /// Iterate through all values in some allocation. - fn iter(&self, alloc_id: usize) -> Result, Error> { - Ok(self.backing[self.range(alloc_id)?].iter()) - } - - /// Mutably iterate through all values in some allocation. - fn iter_mut(&mut self, alloc_id: usize) -> Result, Error> { - let range = self.range(alloc_id)?; - Ok(self.backing[range].iter_mut()) - } - - /// Returns the total number of items stored in the arena, the sum of all values in all - /// allocations. - pub fn backing_len(&self) -> usize { - self.backing.len() - } -} - -/// An allocation from a `CacheArena` that behaves like a `Vec`. -/// -/// All functions will modify the given `arena` instead of `self`. As such, it is safe to have -/// multiple instances of this allocation at once. -/// -/// For all functions that accept a `CacheArena` parameter, that arena should always be the one -/// that created `Self`. I.e., do not mix-and-match allocations and arenas unless you _really_ know -/// what you're doing (or want to have a bad time). -#[derive(Debug, PartialEq, Clone, Default, Encode, Decode)] -pub struct CacheArenaAllocation { - alloc_id: usize, - #[ssz(skip_serializing, skip_deserializing)] - _phantom: PhantomData, -} - -impl CacheArenaAllocation { - /// Grow the allocation in `arena`, appending `vec` to the current values. - pub fn extend_with_vec( - &self, - arena: &mut CacheArena, - vec: SmallVec8, - ) -> Result<(), Error> { - let len = arena.len(self.alloc_id)?; - arena.splice_forgetful(self.alloc_id, len..len, vec)?; - Ok(()) - } - - /// Push `item` to the end of the current allocation in `arena`. - /// - /// An error is returned if this allocation is not known to the given `arena`. - pub fn push(&self, arena: &mut CacheArena, item: T) -> Result<(), Error> { - let len = arena.len(self.alloc_id)?; - arena.splice_forgetful(self.alloc_id, len..len, vec![item])?; - Ok(()) - } - - /// Get the i'th item in the `arena` (relative to this allocation). - /// - /// An error is returned if this allocation is not known to the given `arena`. - pub fn get<'a>(&self, arena: &'a CacheArena, i: usize) -> Result, Error> { - arena.get(self.alloc_id, i) - } - - /// Mutably get the i'th item in the `arena` (relative to this allocation). - /// - /// An error is returned if this allocation is not known to the given `arena`. - pub fn get_mut<'a>( - &self, - arena: &'a mut CacheArena, - i: usize, - ) -> Result, Error> { - arena.get_mut(self.alloc_id, i) - } - - /// Iterate through all items in the `arena` (relative to this allocation). - pub fn iter<'a>(&self, arena: &'a CacheArena) -> Result, Error> { - arena.iter(self.alloc_id) - } - - /// Mutably iterate through all items in the `arena` (relative to this allocation). - pub fn iter_mut<'a>( - &self, - arena: &'a mut CacheArena, - ) -> Result, Error> { - arena.iter_mut(self.alloc_id) - } - - /// Return the number of items stored in this allocation. - pub fn len(&self, arena: &CacheArena) -> Result { - arena.len(self.alloc_id) - } - - /// Returns true if this allocation is empty. - pub fn is_empty(&self, arena: &CacheArena) -> Result { - self.len(arena).map(|len| len == 0) - } -} - -#[cfg(test)] -mod tests { - use crate::Hash256; - use smallvec::smallvec; - - type CacheArena = super::CacheArena; - type CacheArenaAllocation = super::CacheArenaAllocation; - - fn hash(i: usize) -> Hash256 { - Hash256::from_low_u64_be(i as u64) - } - - fn test_routine(arena: &mut CacheArena, sub: &mut CacheArenaAllocation) { - let mut len = sub.len(arena).expect("should exist"); - - sub.push(arena, hash(len)).expect("should push"); - len += 1; - - assert_eq!( - sub.len(arena).expect("should exist"), - len, - "after first push sub should have len {}", - len - ); - assert!( - !sub.is_empty(arena).expect("should exist"), - "new sub should not be empty" - ); - - sub.push(arena, hash(len)).expect("should push again"); - len += 1; - - assert_eq!( - sub.len(arena).expect("should exist"), - len, - "after second push sub should have len {}", - len - ); - - sub.extend_with_vec(arena, smallvec![hash(len), hash(len + 1)]) - .expect("should extend with vec"); - len += 2; - - assert_eq!( - sub.len(arena).expect("should exist"), - len, - "after extend sub should have len {}", - len - ); - - let collected = sub - .iter(arena) - .expect("should get iter") - .cloned() - .collect::>(); - let collected_mut = sub - .iter_mut(arena) - .expect("should get mut iter") - .map(|v| *v) - .collect::>(); - - for i in 0..len { - assert_eq!( - *sub.get(arena, i) - .expect("should exist") - .expect("should get sub index"), - hash(i), - "get({}) should be hash({})", - i, - i - ); - - assert_eq!( - collected[i], - hash(i), - "collected[{}] should be hash({})", - i, - i - ); - - assert_eq!( - collected_mut[i], - hash(i), - "collected_mut[{}] should be hash({})", - i, - i - ); - } - } - - #[test] - fn single() { - let arena = &mut CacheArena::default(); - - assert_eq!(arena.backing.len(), 0, "should start with an empty backing"); - assert_eq!(arena.offsets.len(), 0, "should start without any offsets"); - - let mut sub = arena.alloc(); - - assert_eq!( - sub.len(arena).expect("should exist"), - 0, - "new sub should have len 0" - ); - assert!( - sub.is_empty(arena).expect("should exist"), - "new sub should be empty" - ); - - test_routine(arena, &mut sub); - } - - #[test] - fn double() { - let arena = &mut CacheArena::default(); - - assert_eq!(arena.backing.len(), 0, "should start with an empty backing"); - assert_eq!(arena.offsets.len(), 0, "should start without any offsets"); - - let mut sub_01 = arena.alloc(); - assert_eq!( - sub_01.len(arena).expect("should exist"), - 0, - "new sub should have len 0" - ); - assert!( - sub_01.is_empty(arena).expect("should exist"), - "new sub should be empty" - ); - - let mut sub_02 = arena.alloc(); - assert_eq!( - sub_02.len(arena).expect("should exist"), - 0, - "new sub should have len 0" - ); - assert!( - sub_02.is_empty(arena).expect("should exist"), - "new sub should be empty" - ); - - test_routine(arena, &mut sub_01); - test_routine(arena, &mut sub_02); - } - - #[test] - fn one_then_other() { - let arena = &mut CacheArena::default(); - - assert_eq!(arena.backing.len(), 0, "should start with an empty backing"); - assert_eq!(arena.offsets.len(), 0, "should start without any offsets"); - - let mut sub_01 = arena.alloc(); - assert_eq!( - sub_01.len(arena).expect("should exist"), - 0, - "new sub should have len 0" - ); - assert!( - sub_01.is_empty(arena).expect("should exist"), - "new sub should be empty" - ); - - test_routine(arena, &mut sub_01); - - let mut sub_02 = arena.alloc(); - assert_eq!( - sub_02.len(arena).expect("should exist"), - 0, - "new sub should have len 0" - ); - assert!( - sub_02.is_empty(arena).expect("should exist"), - "new sub should be empty" - ); - - test_routine(arena, &mut sub_02); - test_routine(arena, &mut sub_01); - test_routine(arena, &mut sub_02); - } - - #[test] - fn many() { - let arena = &mut CacheArena::default(); - - assert_eq!(arena.backing.len(), 0, "should start with an empty backing"); - assert_eq!(arena.offsets.len(), 0, "should start without any offsets"); - - let mut subs = vec![]; - - for i in 0..50 { - if i == 0 { - let sub = arena.alloc(); - assert_eq!( - sub.len(arena).expect("should exist"), - 0, - "new sub should have len 0" - ); - assert!( - sub.is_empty(arena).expect("should exist"), - "new sub should be empty" - ); - subs.push(sub); - - continue; - } else if i % 2 == 0 { - test_routine(arena, &mut subs[i - 1]); - } - - let sub = arena.alloc(); - assert_eq!( - sub.len(arena).expect("should exist"), - 0, - "new sub should have len 0" - ); - assert!( - sub.is_empty(arena).expect("should exist"), - "new sub should be empty" - ); - subs.push(sub); - } - - for sub in subs.iter_mut() { - test_routine(arena, sub); - } - } -} diff --git a/consensus/cached_tree_hash/src/impls.rs b/consensus/cached_tree_hash/src/impls.rs deleted file mode 100644 index efdba32b59..0000000000 --- a/consensus/cached_tree_hash/src/impls.rs +++ /dev/null @@ -1,138 +0,0 @@ -use crate::{CacheArena, CachedTreeHash, Error, Hash256, TreeHashCache}; -use ssz_types::{typenum::Unsigned, FixedVector, VariableList}; -use std::mem::size_of; -use tree_hash::{mix_in_length, BYTES_PER_CHUNK}; - -/// Compute ceil(log(n)) -/// -/// Smallest number of bits d so that n <= 2^d -pub fn int_log(n: usize) -> usize { - match n.checked_next_power_of_two() { - Some(x) => x.trailing_zeros() as usize, - None => 8 * std::mem::size_of::(), - } -} - -pub fn hash256_leaf_count(len: usize) -> usize { - len -} - -pub fn u64_leaf_count(len: usize) -> usize { - let type_size = size_of::(); - let vals_per_chunk = BYTES_PER_CHUNK / type_size; - - (len + vals_per_chunk - 1) / vals_per_chunk -} - -pub fn hash256_iter( - values: &[Hash256], -) -> impl ExactSizeIterator + '_ { - values.iter().copied().map(Hash256::to_fixed_bytes) -} - -pub fn u64_iter(values: &[u64]) -> impl ExactSizeIterator + '_ { - let type_size = size_of::(); - let vals_per_chunk = BYTES_PER_CHUNK / type_size; - values.chunks(vals_per_chunk).map(move |xs| { - xs.iter().map(|x| x.to_le_bytes()).enumerate().fold( - [0; BYTES_PER_CHUNK], - |mut chunk, (i, x_bytes)| { - chunk[i * type_size..(i + 1) * type_size].copy_from_slice(&x_bytes); - chunk - }, - ) - }) -} - -impl CachedTreeHash for FixedVector { - fn new_tree_hash_cache(&self, arena: &mut CacheArena) -> TreeHashCache { - TreeHashCache::new( - arena, - int_log(N::to_usize()), - hash256_leaf_count(self.len()), - ) - } - - fn recalculate_tree_hash_root( - &self, - arena: &mut CacheArena, - cache: &mut TreeHashCache, - ) -> Result { - cache.recalculate_merkle_root(arena, hash256_iter(self)) - } -} - -impl CachedTreeHash for FixedVector { - fn new_tree_hash_cache(&self, arena: &mut CacheArena) -> TreeHashCache { - let vals_per_chunk = BYTES_PER_CHUNK / size_of::(); - TreeHashCache::new( - arena, - int_log(N::to_usize() / vals_per_chunk), - u64_leaf_count(self.len()), - ) - } - - fn recalculate_tree_hash_root( - &self, - arena: &mut CacheArena, - cache: &mut TreeHashCache, - ) -> Result { - cache.recalculate_merkle_root(arena, u64_iter(self)) - } -} - -impl CachedTreeHash for VariableList { - fn new_tree_hash_cache(&self, arena: &mut CacheArena) -> TreeHashCache { - TreeHashCache::new( - arena, - int_log(N::to_usize()), - hash256_leaf_count(self.len()), - ) - } - - fn recalculate_tree_hash_root( - &self, - arena: &mut CacheArena, - cache: &mut TreeHashCache, - ) -> Result { - Ok(mix_in_length( - &cache.recalculate_merkle_root(arena, hash256_iter(self))?, - self.len(), - )) - } -} - -impl CachedTreeHash for VariableList { - fn new_tree_hash_cache(&self, arena: &mut CacheArena) -> TreeHashCache { - let vals_per_chunk = BYTES_PER_CHUNK / size_of::(); - TreeHashCache::new( - arena, - int_log(N::to_usize() / vals_per_chunk), - u64_leaf_count(self.len()), - ) - } - - fn recalculate_tree_hash_root( - &self, - arena: &mut CacheArena, - cache: &mut TreeHashCache, - ) -> Result { - Ok(mix_in_length( - &cache.recalculate_merkle_root(arena, u64_iter(self))?, - self.len(), - )) - } -} - -#[cfg(test)] -mod test { - use super::*; - - #[test] - fn test_int_log() { - for i in 0..63 { - assert_eq!(int_log(2usize.pow(i)), i as usize); - } - assert_eq!(int_log(10), 4); - } -} diff --git a/consensus/cached_tree_hash/src/lib.rs b/consensus/cached_tree_hash/src/lib.rs deleted file mode 100644 index af333f2670..0000000000 --- a/consensus/cached_tree_hash/src/lib.rs +++ /dev/null @@ -1,46 +0,0 @@ -mod cache; -mod cache_arena; -mod impls; -#[cfg(test)] -mod test; -use smallvec::SmallVec; - -type SmallVec8 = SmallVec<[T; 8]>; -pub type CacheArena = cache_arena::CacheArena; - -pub use crate::cache::TreeHashCache; -pub use crate::impls::int_log; -use ethereum_types::H256 as Hash256; - -#[derive(Debug, PartialEq, Clone)] -pub enum Error { - /// Attempting to provide more than 2^depth leaves to a Merkle tree is disallowed. - TooManyLeaves, - /// Shrinking a Merkle tree cache by providing it with less leaves than it currently has is - /// disallowed (for simplicity). - CannotShrink, - /// Cache is inconsistent with the list of dirty indices provided. - CacheInconsistent, - CacheArenaError(cache_arena::Error), - /// Unable to find left index in Merkle tree. - MissingLeftIdx(usize), -} - -impl From for Error { - fn from(e: cache_arena::Error) -> Error { - Error::CacheArenaError(e) - } -} - -/// Trait for types which can make use of a cache to accelerate calculation of their tree hash root. -pub trait CachedTreeHash { - /// Create a new cache appropriate for use with values of this type. - fn new_tree_hash_cache(&self, arena: &mut CacheArena) -> Cache; - - /// Update the cache and use it to compute the tree hash root for `self`. - fn recalculate_tree_hash_root( - &self, - arena: &mut CacheArena, - cache: &mut Cache, - ) -> Result; -} diff --git a/consensus/cached_tree_hash/src/test.rs b/consensus/cached_tree_hash/src/test.rs deleted file mode 100644 index 69b49826bf..0000000000 --- a/consensus/cached_tree_hash/src/test.rs +++ /dev/null @@ -1,153 +0,0 @@ -use crate::impls::hash256_iter; -use crate::{CacheArena, CachedTreeHash, Error, Hash256, TreeHashCache}; -use ethereum_hashing::ZERO_HASHES; -use quickcheck_macros::quickcheck; -use ssz_types::{ - typenum::{Unsigned, U16, U255, U256, U257}, - FixedVector, VariableList, -}; -use tree_hash::TreeHash; - -fn int_hashes(start: u64, end: u64) -> Vec { - (start..end).map(Hash256::from_low_u64_le).collect() -} - -type List16 = VariableList; -type Vector16 = FixedVector; -type Vector16u64 = FixedVector; - -#[test] -fn max_leaves() { - let arena = &mut CacheArena::default(); - let depth = 4; - let max_len = 2u64.pow(depth as u32); - let mut cache = TreeHashCache::new(arena, depth, 2); - assert!(cache - .recalculate_merkle_root(arena, hash256_iter(&int_hashes(0, max_len - 1))) - .is_ok()); - assert!(cache - .recalculate_merkle_root(arena, hash256_iter(&int_hashes(0, max_len))) - .is_ok()); - assert_eq!( - cache.recalculate_merkle_root(arena, hash256_iter(&int_hashes(0, max_len + 1))), - Err(Error::TooManyLeaves) - ); - assert_eq!( - cache.recalculate_merkle_root(arena, hash256_iter(&int_hashes(0, max_len * 2))), - Err(Error::TooManyLeaves) - ); -} - -#[test] -fn cannot_shrink() { - let arena = &mut CacheArena::default(); - let init_len = 12; - let list1 = List16::new(int_hashes(0, init_len)).unwrap(); - let list2 = List16::new(int_hashes(0, init_len - 1)).unwrap(); - - let mut cache = list1.new_tree_hash_cache(arena); - assert!(list1.recalculate_tree_hash_root(arena, &mut cache).is_ok()); - assert_eq!( - list2.recalculate_tree_hash_root(arena, &mut cache), - Err(Error::CannotShrink) - ); -} - -#[test] -fn empty_leaves() { - let arena = &mut CacheArena::default(); - let depth = 20; - let mut cache = TreeHashCache::new(arena, depth, 0); - assert_eq!( - cache - .recalculate_merkle_root(arena, vec![].into_iter()) - .unwrap() - .as_bytes(), - &ZERO_HASHES[depth][..] - ); -} - -#[test] -fn fixed_vector_hash256() { - let arena = &mut CacheArena::default(); - let len = 16; - let vec = Vector16::new(int_hashes(0, len)).unwrap(); - - let mut cache = vec.new_tree_hash_cache(arena); - - assert_eq!( - vec.tree_hash_root(), - vec.recalculate_tree_hash_root(arena, &mut cache).unwrap() - ); -} - -#[test] -fn fixed_vector_u64() { - let arena = &mut CacheArena::default(); - let len = 16; - let vec = Vector16u64::new((0..len).collect()).unwrap(); - - let mut cache = vec.new_tree_hash_cache(arena); - - assert_eq!( - vec.tree_hash_root(), - vec.recalculate_tree_hash_root(arena, &mut cache).unwrap() - ); -} - -#[test] -fn variable_list_hash256() { - let arena = &mut CacheArena::default(); - let len = 13; - let list = List16::new(int_hashes(0, len)).unwrap(); - - let mut cache = list.new_tree_hash_cache(arena); - - assert_eq!( - list.tree_hash_root(), - list.recalculate_tree_hash_root(arena, &mut cache).unwrap() - ); -} - -#[quickcheck] -fn quickcheck_variable_list_h256_256(leaves_and_skips: Vec<(u64, bool)>) -> bool { - variable_list_h256_test::(leaves_and_skips) -} - -#[quickcheck] -fn quickcheck_variable_list_h256_255(leaves_and_skips: Vec<(u64, bool)>) -> bool { - variable_list_h256_test::(leaves_and_skips) -} - -#[quickcheck] -fn quickcheck_variable_list_h256_257(leaves_and_skips: Vec<(u64, bool)>) -> bool { - variable_list_h256_test::(leaves_and_skips) -} - -fn variable_list_h256_test(leaves_and_skips: Vec<(u64, bool)>) -> bool { - let arena = &mut CacheArena::default(); - let leaves: Vec<_> = leaves_and_skips - .iter() - .map(|(l, _)| Hash256::from_low_u64_be(*l)) - .take(Len::to_usize()) - .collect(); - - let mut list: VariableList; - let init: VariableList = VariableList::new(vec![]).unwrap(); - let mut cache = init.new_tree_hash_cache(arena); - - for (end, (_, update_cache)) in leaves_and_skips.into_iter().enumerate() { - list = VariableList::new(leaves[..end].to_vec()).unwrap(); - - if update_cache - && list - .recalculate_tree_hash_root(arena, &mut cache) - .unwrap() - .as_bytes() - != &list.tree_hash_root()[..] - { - return false; - } - } - true -} diff --git a/consensus/types/Cargo.toml b/consensus/types/Cargo.toml index fd1f862a92..28207f828a 100644 --- a/consensus/types/Cargo.toml +++ b/consensus/types/Cargo.toml @@ -33,7 +33,6 @@ test_random_derive = { path = "../../common/test_random_derive" } tree_hash = { workspace = true, features = ["arbitrary"] } tree_hash_derive = { workspace = true } rand_xorshift = "0.3.0" -cached_tree_hash = { workspace = true } serde_yaml = { workspace = true } tempfile = { workspace = true } derivative = { workspace = true } diff --git a/consensus/types/src/beacon_state.rs b/consensus/types/src/beacon_state.rs index 45f9178ff3..0622039179 100644 --- a/consensus/types/src/beacon_state.rs +++ b/consensus/types/src/beacon_state.rs @@ -121,7 +121,6 @@ pub enum Error { state: Slot, }, TreeHashError(tree_hash::Error), - CachedTreeHashError(cached_tree_hash::Error), InvalidValidatorPubkey(ssz::DecodeError), ValidatorRegistryShrunk, TreeHashCacheInconsistent, @@ -2560,12 +2559,6 @@ impl From for Error { } } -impl From for Error { - fn from(e: cached_tree_hash::Error) -> Error { - Error::CachedTreeHashError(e) - } -} - impl From for Error { fn from(e: tree_hash::Error) -> Error { Error::TreeHashError(e) diff --git a/consensus/types/src/historical_summary.rs b/consensus/types/src/historical_summary.rs index 95d015a0f7..76bb111ea2 100644 --- a/consensus/types/src/historical_summary.rs +++ b/consensus/types/src/historical_summary.rs @@ -1,14 +1,10 @@ use crate::test_utils::TestRandom; -use crate::Unsigned; use crate::{BeaconState, EthSpec, Hash256}; -use cached_tree_hash::Error; -use cached_tree_hash::{int_log, CacheArena, CachedTreeHash, TreeHashCache}; use compare_fields_derive::CompareFields; use serde::{Deserialize, Serialize}; use ssz_derive::{Decode, Encode}; -use ssz_types::VariableList; use test_random_derive::TestRandom; -use tree_hash::{mix_in_length, TreeHash, BYTES_PER_CHUNK}; +use tree_hash::TreeHash; use tree_hash_derive::TreeHash; /// `HistoricalSummary` matches the components of the phase0 `HistoricalBatch` @@ -44,46 +40,3 @@ impl HistoricalSummary { } } } - -/// Wrapper type allowing the implementation of `CachedTreeHash`. -#[derive(Debug)] -pub struct HistoricalSummaryCache<'a, N: Unsigned> { - pub inner: &'a VariableList, -} - -impl<'a, N: Unsigned> HistoricalSummaryCache<'a, N> { - pub fn new(inner: &'a VariableList) -> Self { - Self { inner } - } - - #[allow(clippy::len_without_is_empty)] - pub fn len(&self) -> usize { - self.inner.len() - } -} - -impl<'a, N: Unsigned> CachedTreeHash for HistoricalSummaryCache<'a, N> { - fn new_tree_hash_cache(&self, arena: &mut CacheArena) -> TreeHashCache { - TreeHashCache::new(arena, int_log(N::to_usize()), self.len()) - } - - fn recalculate_tree_hash_root( - &self, - arena: &mut CacheArena, - cache: &mut TreeHashCache, - ) -> Result { - Ok(mix_in_length( - &cache.recalculate_merkle_root(arena, leaf_iter(self.inner))?, - self.len(), - )) - } -} - -pub fn leaf_iter( - values: &[HistoricalSummary], -) -> impl ExactSizeIterator + '_ { - values - .iter() - .map(|value| value.tree_hash_root()) - .map(Hash256::to_fixed_bytes) -} diff --git a/consensus/types/src/lib.rs b/consensus/types/src/lib.rs index 60c5a021e0..8e72d5647a 100644 --- a/consensus/types/src/lib.rs +++ b/consensus/types/src/lib.rs @@ -84,7 +84,6 @@ pub mod config_and_preset; pub mod execution_block_header; pub mod fork_context; pub mod participation_flags; -pub mod participation_list; pub mod payload; pub mod preset; pub mod slot_epoch; @@ -200,7 +199,6 @@ pub use crate::light_client_update::{ LightClientUpdateCapella, LightClientUpdateDeneb, LightClientUpdateElectra, }; pub use crate::participation_flags::ParticipationFlags; -pub use crate::participation_list::ParticipationList; pub use crate::payload::{ AbstractExecPayload, BlindedPayload, BlindedPayloadBellatrix, BlindedPayloadCapella, BlindedPayloadDeneb, BlindedPayloadElectra, BlindedPayloadRef, BlockType, ExecPayload, diff --git a/consensus/types/src/participation_list.rs b/consensus/types/src/participation_list.rs deleted file mode 100644 index 6e3d916dee..0000000000 --- a/consensus/types/src/participation_list.rs +++ /dev/null @@ -1,55 +0,0 @@ -#![allow(clippy::arithmetic_side_effects)] - -use crate::{Hash256, ParticipationFlags, Unsigned, VariableList}; -use cached_tree_hash::{int_log, CacheArena, CachedTreeHash, Error, TreeHashCache}; -use tree_hash::{mix_in_length, BYTES_PER_CHUNK}; - -/// Wrapper type allowing the implementation of `CachedTreeHash`. -#[derive(Debug)] -pub struct ParticipationList<'a, N: Unsigned> { - pub inner: &'a VariableList, -} - -impl<'a, N: Unsigned> ParticipationList<'a, N> { - pub fn new(inner: &'a VariableList) -> Self { - Self { inner } - } -} - -impl<'a, N: Unsigned> CachedTreeHash for ParticipationList<'a, N> { - fn new_tree_hash_cache(&self, arena: &mut CacheArena) -> TreeHashCache { - TreeHashCache::new( - arena, - int_log(N::to_usize() / BYTES_PER_CHUNK), - leaf_count(self.inner.len()), - ) - } - - fn recalculate_tree_hash_root( - &self, - arena: &mut CacheArena, - cache: &mut TreeHashCache, - ) -> Result { - Ok(mix_in_length( - &cache.recalculate_merkle_root(arena, leaf_iter(self.inner))?, - self.inner.len(), - )) - } -} - -pub fn leaf_count(len: usize) -> usize { - (len + BYTES_PER_CHUNK - 1) / BYTES_PER_CHUNK -} - -pub fn leaf_iter( - values: &[ParticipationFlags], -) -> impl ExactSizeIterator + '_ { - values.chunks(BYTES_PER_CHUNK).map(|xs| { - // Zero-pad chunks on the right. - let mut chunk = [0u8; BYTES_PER_CHUNK]; - for (byte, x) in chunk.iter_mut().zip(xs) { - *byte = x.into_u8(); - } - chunk - }) -}