From 6fd8cd299e18177986bf06f2094ec0334ba33f7b Mon Sep 17 00:00:00 2001 From: owen Date: Thu, 25 Jan 2024 14:06:42 +0000 Subject: [PATCH 01/24] switch housekeeper from config to redis lock --- crates/api/src/service.rs | 22 +++++------ crates/common/src/config.rs | 2 - .../src/auctioneer/mock_auctioneer.rs | 4 ++ crates/datastore/src/auctioneer/traits.rs | 2 + crates/datastore/src/redis/redis_cache.rs | 38 ++++++++++++++++++- crates/housekeeper/src/housekeeper.rs | 5 +++ 6 files changed, 58 insertions(+), 15 deletions(-) diff --git a/crates/api/src/service.rs b/crates/api/src/service.rs index d1571979..19f9c3d7 100644 --- a/crates/api/src/service.rs +++ b/crates/api/src/service.rs @@ -48,19 +48,17 @@ impl ApiService { ForkInfoConfig::Holesky => ForkInfo::for_holesky(), }); - // Housekeeper should only be run on one instance. - if config.run_housekeeper { - let housekeeper = - Housekeeper::new(db.clone(), multi_beacon_client.clone(), auctioneer.clone(), fork_info.clone()); - tokio::task::spawn(async move { - loop { - if let Err(err) = housekeeper.start().await { - tracing::error!("Housekeeper error: {}", err); - sleep(Duration::from_secs(5)).await; - } + + let housekeeper = + Housekeeper::new(db.clone(), multi_beacon_client.clone(), auctioneer.clone(), fork_info.clone()); + tokio::task::spawn(async move { + loop { + if let Err(err) = housekeeper.start().await { + tracing::error!("Housekeeper error: {}", err); + sleep(Duration::from_secs(5)).await; } - }); - } + } + }); // Initialise relay signing context let signing_key_str = env::var("RELAY_KEY").expect("could not find RELAY_KEY in env"); diff --git a/crates/common/src/config.rs b/crates/common/src/config.rs index bc55bee6..e87c8ed5 100644 --- a/crates/common/src/config.rs +++ b/crates/common/src/config.rs @@ -19,8 +19,6 @@ pub struct RelayConfig { pub fork_info: ForkInfoConfig, #[serde(default)] pub logging: LoggingConfig, - #[serde(default)] - pub run_housekeeper: bool, pub validator_preferences: ValidatorPreferences, pub router_config: RouterConfig, #[serde(default = "default_duration")] diff --git a/crates/datastore/src/auctioneer/mock_auctioneer.rs b/crates/datastore/src/auctioneer/mock_auctioneer.rs index 1f212295..5b2f020d 100644 --- a/crates/datastore/src/auctioneer/mock_auctioneer.rs +++ b/crates/datastore/src/auctioneer/mock_auctioneer.rs @@ -213,4 +213,8 @@ impl Auctioneer for MockAuctioneer { ) -> Result, AuctioneerError> { Ok(None) } + + async fn try_become_housekeeper(&self) -> bool { + true + } } diff --git a/crates/datastore/src/auctioneer/traits.rs b/crates/datastore/src/auctioneer/traits.rs index 130fe177..011128bd 100644 --- a/crates/datastore/src/auctioneer/traits.rs +++ b/crates/datastore/src/auctioneer/traits.rs @@ -143,4 +143,6 @@ pub trait Auctioneer: Send + Sync + Clone { state: &mut SaveBidAndUpdateTopBidResponse, signing_context: &RelaySigningContext, ) -> Result, AuctioneerError>; + + async fn try_become_housekeeper(&self) -> bool; } diff --git a/crates/datastore/src/redis/redis_cache.rs b/crates/datastore/src/redis/redis_cache.rs index 466ad15b..28778855 100644 --- a/crates/datastore/src/redis/redis_cache.rs +++ b/crates/datastore/src/redis/redis_cache.rs @@ -8,7 +8,7 @@ use ethereum_consensus::{ }; use helix_common::{bid_submission::BidSubmission, versioned_payload::PayloadAndBlobs}; use helix_common::bid_submission::v2::header_submission::SignedHeaderSubmission; -use redis::AsyncCommands; +use redis::{AsyncCommands, RedisResult, Value}; use serde::{de::DeserializeOwned, Serialize}; use tracing::error; @@ -38,6 +38,7 @@ use crate::{ }; const BID_CACHE_EXPIRY_S: usize = 45; +const HOUSEKEEPER_LOCK_EXPIRY_S: usize = 2; #[derive(Clone)] pub struct RedisCache { @@ -144,6 +145,31 @@ impl RedisCache { } } + async fn set_lock( + &self, + key: &str, + expiry: usize, + ) -> bool { + let mut conn = match self.pool.get().await { + Ok(conn) => conn, + Err(_) => return false, + }; + + let result: RedisResult = redis::cmd("SET") + .arg(key) + .arg(1) + .arg("NX") + .arg("PX") + .arg(expiry) + .query_async(&mut conn) + .await; + + match result { + Ok(Value::Okay) => true, + Ok(_) | Err(_) => false, + } + } + async fn hset( &self, key: &str, @@ -748,6 +774,16 @@ impl Auctioneer for RedisCache { Ok(Some(builder_bid)) } + + // Sets the housekeeper lock if it's not already set. + // Returns true if the lock was set, false otherwise. + // This function is used to ensure that only one housekeeper is running at a time. + // The lock is set to expire after HOUSEKEEPER_LOCK_EXPIRY_S seconds. + // This will ensure that if a housekeeper crashes, the lock will eventually expire. + // Expiry is set to 2 seconds to ensure it will expire before the next slot. + async fn try_become_housekeeper(&self) -> bool { + self.set_lock("housekeeper_lock", HOUSEKEEPER_LOCK_EXPIRY_S).await + } } fn get_top_bid(bid_values: &HashMap) -> Option<(String, U256)> { diff --git a/crates/housekeeper/src/housekeeper.rs b/crates/housekeeper/src/housekeeper.rs index 3130be2c..7f987c8c 100644 --- a/crates/housekeeper/src/housekeeper.rs +++ b/crates/housekeeper/src/housekeeper.rs @@ -116,6 +116,11 @@ impl return; } + // Only allow one housekeeper task to run at a time. + if !self.auctioneer.try_become_housekeeper().await { + return; + } + // Demote builders with expired pending blocks let cloned_self = self.clone(); tokio::spawn(async move { From fa9686daec4dab7ce1d9d149f106534b9ebd0728 Mon Sep 17 00:00:00 2001 From: gd-0 <90608901+gd-0@users.noreply.github.com> Date: Thu, 25 Jan 2024 18:18:15 +0000 Subject: [PATCH 02/24] refactor validator preferences + add trusted_builders --- crates/api/src/builder/simulator/mod.rs | 2 +- .../simulator/optimistic_simulator_tests.rs | 2 +- .../api/src/builder/simulator/simulator_tests.rs | 2 +- crates/api/src/builder/tests.rs | 2 +- crates/api/src/proposer/api.rs | 2 +- crates/api/src/proposer/tests.rs | 2 +- crates/api/src/test_utils.rs | 2 +- crates/common/src/api/builder_api.rs | 2 +- crates/common/src/api/proposer_api.rs | 6 +----- crates/common/src/config.rs | 6 +++--- crates/common/src/lib.rs | 2 ++ crates/common/src/validator.rs | 2 +- crates/common/src/validator_preferences/mod.rs | 14 ++++++++++++++ .../src/validator_preferences/trusted_builders.rs | 6 ++++++ .../src/postgres/postgres_db_row_parsing.rs | 13 ++++++++++++- 15 files changed, 47 insertions(+), 18 deletions(-) create mode 100644 crates/common/src/validator_preferences/mod.rs create mode 100644 crates/common/src/validator_preferences/trusted_builders.rs diff --git a/crates/api/src/builder/simulator/mod.rs b/crates/api/src/builder/simulator/mod.rs index dd5f4a1c..a75fa88c 100644 --- a/crates/api/src/builder/simulator/mod.rs +++ b/crates/api/src/builder/simulator/mod.rs @@ -9,7 +9,7 @@ use std::sync::Arc; use ethereum_consensus::types::mainnet::ExecutionPayload; use helix_common::bid_submission::{BidTrace, SignedBidSubmission, BidSubmission}; -use helix_common::api::proposer_api::ValidatorPreferences; +use helix_common::ValidatorPreferences; use ethereum_consensus::serde::as_str; use ethereum_consensus::primitives::BlsSignature; diff --git a/crates/api/src/builder/simulator/optimistic_simulator_tests.rs b/crates/api/src/builder/simulator/optimistic_simulator_tests.rs index 7ec14319..45f50c21 100644 --- a/crates/api/src/builder/simulator/optimistic_simulator_tests.rs +++ b/crates/api/src/builder/simulator/optimistic_simulator_tests.rs @@ -25,7 +25,7 @@ mod simulator_tests { simulator::BlockSimError, BuilderInfo, }; - use helix_common::api::proposer_api::ValidatorPreferences; + use helix_common::ValidatorPreferences; // ++++ HELPERS ++++ fn get_optimistic_simulator( diff --git a/crates/api/src/builder/simulator/simulator_tests.rs b/crates/api/src/builder/simulator/simulator_tests.rs index affafeba..f9e27730 100644 --- a/crates/api/src/builder/simulator/simulator_tests.rs +++ b/crates/api/src/builder/simulator/simulator_tests.rs @@ -20,7 +20,7 @@ mod simulator_tests { bid_submission::{BidTrace, SignedBidSubmission, SignedBidSubmissionCapella}, simulator::BlockSimError, }; - use helix_common::api::proposer_api::ValidatorPreferences; + use helix_common::ValidatorPreferences; // ++++ HELPERS ++++ fn get_simulator(endpoint: &str) -> RpcSimulator { diff --git a/crates/api/src/builder/tests.rs b/crates/api/src/builder/tests.rs index fb8cc86c..350f04fe 100644 --- a/crates/api/src/builder/tests.rs +++ b/crates/api/src/builder/tests.rs @@ -33,7 +33,7 @@ mod tests { api::builder_api::{BuilderGetValidatorsResponseEntry, BuilderGetValidatorsResponse}, bid_submission::{SignedBidSubmission, v2::header_submission::{SignedHeaderSubmission, SignedHeaderSubmissionCapella, SignedHeaderSubmissionDeneb}, BidSubmission}, SubmissionTrace, HeaderSubmissionTrace, }; use helix_common::api::proposer_api::ValidatorRegistrationInfo; - use helix_common::api::proposer_api::ValidatorPreferences; + use helix_common::ValidatorPreferences; use helix_housekeeper::{ChainUpdate, PayloadAttributesUpdate, SlotUpdate}; use helix_utils::request_encoding::Encoding; use tokio::sync::{ diff --git a/crates/api/src/proposer/api.rs b/crates/api/src/proposer/api.rs index 50aa77a9..5b7a0651 100644 --- a/crates/api/src/proposer/api.rs +++ b/crates/api/src/proposer/api.rs @@ -44,9 +44,9 @@ use helix_common::{ proposer_api::{ ValidatorRegistrationInfo, GetPayloadResponse, - ValidatorPreferences, }, }, + ValidatorPreferences, chain_info::{ChainInfo, Network}, try_execution_header_from_payload, BidRequest, GetHeaderTrace, GetPayloadTrace, RegisterValidatorsTrace, signed_proposal::VersionedSignedProposal, versioned_payload::PayloadAndBlobs, diff --git a/crates/api/src/proposer/tests.rs b/crates/api/src/proposer/tests.rs index 7566e74f..6368913b 100644 --- a/crates/api/src/proposer/tests.rs +++ b/crates/api/src/proposer/tests.rs @@ -44,7 +44,7 @@ mod proposer_api_tests { use tokio::time::{sleep}; use crate::proposer::PATH_REGISTER_VALIDATORS; use crate::proposer::tests::gen_signed_vr; - use helix_common::api::proposer_api::ValidatorPreferences; + use helix_common::ValidatorPreferences; use helix_common::api::proposer_api::ValidatorRegistrationInfo; // +++ HELPER VARIABLES +++ diff --git a/crates/api/src/test_utils.rs b/crates/api/src/test_utils.rs index 898fd2e0..621f541e 100644 --- a/crates/api/src/test_utils.rs +++ b/crates/api/src/test_utils.rs @@ -12,7 +12,7 @@ use helix_database::MockDatabaseService; use helix_datastore::MockAuctioneer; use helix_common::signing::RelaySigningContext; use helix_housekeeper::ChainUpdate; -use helix_common::api::proposer_api::ValidatorPreferences; +use helix_common::ValidatorPreferences; use crate::{ builder::{ diff --git a/crates/common/src/api/builder_api.rs b/crates/common/src/api/builder_api.rs index 1501f7a4..c13b06c7 100644 --- a/crates/common/src/api/builder_api.rs +++ b/crates/common/src/api/builder_api.rs @@ -3,7 +3,7 @@ use ethereum_consensus::{ }; use serde::{Deserialize, Serialize}; -use super::proposer_api::{ValidatorPreferences, ValidatorRegistrationInfo}; +use crate::{ValidatorPreferences, api::proposer_api::ValidatorRegistrationInfo}; #[derive(Serialize, Deserialize, Debug, Clone, Default)] pub struct BuilderGetValidatorsResponseEntry { diff --git a/crates/common/src/api/proposer_api.rs b/crates/common/src/api/proposer_api.rs index 2fe5738f..c75ff863 100644 --- a/crates/common/src/api/proposer_api.rs +++ b/crates/common/src/api/proposer_api.rs @@ -3,7 +3,7 @@ use ethereum_consensus::{ Fork, builder::SignedValidatorRegistration, }; -use crate::versioned_payload::PayloadAndBlobs; +use crate::{validator_preferences::ValidatorPreferences, versioned_payload::PayloadAndBlobs}; #[derive(Debug, serde::Serialize)] @@ -54,10 +54,6 @@ impl<'de> serde::Deserialize<'de> for GetPayloadResponse { } } -#[derive(Debug, Clone, Default, serde::Serialize, serde::Deserialize)] -pub struct ValidatorPreferences { - pub censoring: bool, -} #[derive(Debug, Clone, Default, serde::Serialize, serde::Deserialize)] pub struct ValidatorRegistrationInfo { diff --git a/crates/common/src/config.rs b/crates/common/src/config.rs index cbd989cb..5e5e4a9e 100644 --- a/crates/common/src/config.rs +++ b/crates/common/src/config.rs @@ -2,7 +2,7 @@ use clap::Parser; use serde::{Deserialize, Serialize}; use std::{fs::File, collections::HashSet}; use helix_utils::request_encoding::Encoding; -use crate::api::proposer_api::ValidatorPreferences; +use crate::ValidatorPreferences; #[derive(Serialize, Deserialize, Clone, Default)] pub struct RelayConfig { @@ -179,7 +179,7 @@ fn default_duration() -> u64 { #[cfg(test)] #[test] fn test_config() { - use crate::api::proposer_api::ValidatorPreferences; + use crate::ValidatorPreferences; let mut config = RelayConfig::default(); config.redis.url = "redis://localhost:6379".to_string(); @@ -189,7 +189,7 @@ fn test_config() { config.network_config = NetworkConfig::Mainnet; config.logging = LoggingConfig::File { dir_path: "hello".to_string(), file_name: "test".to_string() }; - config.validator_preferences = ValidatorPreferences { censoring: true }; + config.validator_preferences = ValidatorPreferences { censoring: true, trusted_builders: None }; config.router_config = RouterConfig { enabled_routes: [ Route::GetValidators, diff --git a/crates/common/src/lib.rs b/crates/common/src/lib.rs index c6cb3d8c..74636cb4 100644 --- a/crates/common/src/lib.rs +++ b/crates/common/src/lib.rs @@ -10,6 +10,7 @@ pub mod simulator; pub mod traces; pub mod validator; pub mod pending_block; +pub mod validator_preferences; pub use builder_info::*; pub use config::*; @@ -17,3 +18,4 @@ pub use eth::*; pub use proposer::*; pub use traces::*; pub use validator::*; +pub use validator_preferences::*; diff --git a/crates/common/src/validator.rs b/crates/common/src/validator.rs index fc6f6134..99eef9df 100644 --- a/crates/common/src/validator.rs +++ b/crates/common/src/validator.rs @@ -16,7 +16,7 @@ use reth_primitives::hex; use serde::{Deserialize, Serialize}; use tokio_postgres::Row; -use crate::api::proposer_api::{ValidatorPreferences, ValidatorRegistrationInfo}; +use crate::{ValidatorPreferences, api::proposer_api::ValidatorRegistrationInfo}; #[derive(Debug, Serialize, Deserialize, Clone)] diff --git a/crates/common/src/validator_preferences/mod.rs b/crates/common/src/validator_preferences/mod.rs new file mode 100644 index 00000000..a6b40fa1 --- /dev/null +++ b/crates/common/src/validator_preferences/mod.rs @@ -0,0 +1,14 @@ +use self::trusted_builders::BuilderID; + +pub mod trusted_builders; + +#[derive(Debug, Clone, Default, serde::Serialize, serde::Deserialize)] +pub struct ValidatorPreferences { + /// A boolean flag indicating whether the validator requests the relay + /// to enforce censoring of sanctioned transactions. + pub censoring: bool, + /// An optional list of `BuilderID`s. If this is set, the relay will only accept + /// submissions from builders whose public keys are linked to the IDs in this list. + /// This allows for limiting submissions to a trusted set of builders. + pub trusted_builders: Option> +} \ No newline at end of file diff --git a/crates/common/src/validator_preferences/trusted_builders.rs b/crates/common/src/validator_preferences/trusted_builders.rs new file mode 100644 index 00000000..14a9b5a8 --- /dev/null +++ b/crates/common/src/validator_preferences/trusted_builders.rs @@ -0,0 +1,6 @@ + +#[derive(Debug, Clone, serde::Serialize, serde::Deserialize)] +pub enum BuilderID { + Titan, + TitanS, +} \ No newline at end of file diff --git a/crates/database/src/postgres/postgres_db_row_parsing.rs b/crates/database/src/postgres/postgres_db_row_parsing.rs index 60b4077d..bdd258d4 100644 --- a/crates/database/src/postgres/postgres_db_row_parsing.rs +++ b/crates/database/src/postgres/postgres_db_row_parsing.rs @@ -6,7 +6,16 @@ use ethereum_consensus::{ }; use thiserror::Error; use helix_common::{ - api::builder_api::BuilderGetValidatorsResponseEntry, api::proposer_api::{ValidatorPreferences, ValidatorRegistrationInfo}, bellatrix::{ByteList, ByteVector, List}, bid_submission::BidTrace, pending_block::PendingBlock, BuilderInfo, GetPayloadTrace, ProposerInfo, SignedValidatorRegistrationEntry + api::builder_api::BuilderGetValidatorsResponseEntry, + api::proposer_api::ValidatorRegistrationInfo, + bellatrix::{ByteList, ByteVector, List}, + bid_submission::BidTrace, + pending_block::PendingBlock, + BuilderInfo, + GetPayloadTrace, + ProposerInfo, + SignedValidatorRegistrationEntry, + ValidatorPreferences, }; use crate::{ @@ -173,6 +182,7 @@ impl FromRow for BuilderGetValidatorsResponseEntry { }, preferences: ValidatorPreferences { censoring: parse_bool_to_bool(row.get::<&str, bool>("censoring"))?, + trusted_builders: todo!(), }, }, }) @@ -245,6 +255,7 @@ impl FromRow for SignedValidatorRegistrationEntry { registration: SignedValidatorRegistration::from_row(row)?, preferences: ValidatorPreferences { censoring: parse_bool_to_bool(row.get::<&str, bool>("censoring"))?, + trusted_builders: todo!(), }, }, inserted_at: parse_timestamptz_to_u64( From ada56c0cf03da23da84285991cefe4ededac73dd Mon Sep 17 00:00:00 2001 From: gd-0 <90608901+gd-0@users.noreply.github.com> Date: Thu, 25 Jan 2024 21:31:01 +0000 Subject: [PATCH 03/24] validate trusted builders through builder_info --- crates/api/src/builder/api.rs | 239 +++++++++++++----- crates/api/src/builder/error.rs | 8 +- .../src/builder/simulator/mock_simulator.rs | 3 +- .../builder/simulator/optimistic_simulator.rs | 23 +- .../simulator/optimistic_simulator_tests.rs | 28 +- .../src/builder/simulator/rpc_simulator.rs | 2 + .../src/builder/simulator/simulator_tests.rs | 12 +- crates/api/src/builder/simulator/traits.rs | 3 +- crates/common/src/builder_info.rs | 14 + .../common/src/validator_preferences/mod.rs | 3 +- .../validator_preferences/trusted_builders.rs | 6 - .../src/postgres/postgres_db_row_parsing.rs | 1 + .../src/postgres/postgres_db_service_tests.rs | 7 +- .../src/auctioneer/mock_auctioneer.rs | 1 + crates/datastore/src/auctioneer/traits.rs | 2 +- crates/datastore/src/redis/redis_cache.rs | 8 +- 16 files changed, 240 insertions(+), 120 deletions(-) delete mode 100644 crates/common/src/validator_preferences/trusted_builders.rs diff --git a/crates/api/src/builder/api.rs b/crates/api/src/builder/api.rs index 7b341ce4..7bdebf26 100644 --- a/crates/api/src/builder/api.rs +++ b/crates/api/src/builder/api.rs @@ -39,12 +39,9 @@ use helix_common::{ api::{ builder_api::{BuilderGetValidatorsResponse, BuilderGetValidatorsResponseEntry}, proposer_api::ValidatorRegistrationInfo, - }, - bid_submission::{ + }, bid_submission::{ v2::header_submission::{SignedHeaderSubmission, SignedHeaderSubmissionCapella, SignedHeaderSubmissionDeneb}, BidSubmission, BidTrace, SignedBidSubmission, - }, - HeaderSubmissionTrace, chain_info::ChainInfo, signing::RelaySigningContext, - simulator::BlockSimError, SubmissionTrace, SignedBuilderBid, GossipedHeaderTrace, GossipedPayloadTrace, versioned_payload::PayloadAndBlobs, + }, chain_info::ChainInfo, signing::RelaySigningContext, simulator::BlockSimError, versioned_payload::PayloadAndBlobs, BuilderID, BuilderInfo, GossipedHeaderTrace, GossipedPayloadTrace, HeaderSubmissionTrace, SignedBuilderBid, SubmissionTrace }; use helix_utils::{calculate_withdrawals_root, has_reached_fork, try_decode_into}; @@ -181,8 +178,6 @@ where timestamp_request_start = trace.receive, ); - - // TODO TEMPORARY, remove again! // Decode the incoming request body into a payload let (payload, is_cancellations_enabled) = decode_payload(req, &mut trace, &request_id).await?; @@ -222,6 +217,21 @@ where let (next_duty, payload_attributes) = api.fetch_proposer_and_attributes(payload.slot(), payload.parent_hash(), &request_id).await?; + // Fetch builder info + let builder_info = api.fetch_builder_info(payload.builder_public_key()).await; + + // Handle trusted builders check + if !api.check_if_trusted_builder(&next_duty, &builder_info).await? { + let proposer_trusted_builders = next_duty.entry.preferences.trusted_builders.unwrap(); + warn!( + request_id = %request_id, + builder_pub_key = ?payload.builder_public_key(), + proposer_trusted_builders = ?proposer_trusted_builders, + "builder not in proposer trusted builders list", + ); + return Err(BuilderApiError::BuilderNotInProposersTrustedList { proposer_trusted_builders }); + } + // Verify payload has not already been delivered match api.auctioneer.get_last_slot_delivered().await { Ok(Some(slot)) => { @@ -240,6 +250,7 @@ where payload, next_duty, &payload_attributes, + &builder_info, head_slot, &mut trace, &request_id, @@ -337,9 +348,12 @@ where "header submission decoded", ); + // Fetch builder info + let builder_info = api.fetch_builder_info(payload.builder_public_key()).await; + // Submit header can only be processed optimistically. // Make sure that the builder has enough collateral to cover the submission. - if let Err(err) = api.check_builder_collateral(&payload, &request_id).await { + if let Err(err) = api.check_builder_collateral(&payload, &builder_info, &request_id).await { warn!(request_id = %request_id, error = %err, "builder has insufficient collateral"); return Err(err); } @@ -367,6 +381,19 @@ where warn!(request_id = %request_id, error = %err, "failed sanity check"); return Err(err); } + + // Handle trusted builders check + if !api.check_if_trusted_builder(&next_duty, &builder_info).await? { + let proposer_trusted_builders = next_duty.entry.preferences.trusted_builders.unwrap(); + warn!( + request_id = %request_id, + builder_pub_key = ?payload.builder_public_key(), + proposer_trusted_builders = ?proposer_trusted_builders, + "builder not in proposer trusted builders list", + ); + return Err(BuilderApiError::BuilderNotInProposersTrustedList { proposer_trusted_builders }); + } + trace.pre_checks = get_nanos_timestamp()?; // Verify the payload signature @@ -490,9 +517,12 @@ where .await .map_err(|_| BuilderApiError::InternalError)?; + // Fetch builder info + let builder_info = api.fetch_builder_info(payload.builder_public_key()).await; + // submit_block_v2 can only be processed optimistically. // Make sure that the builder has enough collateral to cover the submission. - if let Err(err) = api.check_builder_collateral(&payload, &request_id).await { + if let Err(err) = api.check_builder_collateral(&payload, &builder_info, &request_id).await { warn!(request_id = %request_id, error = %err, "builder has insufficient collateral"); return Err(err); } @@ -501,6 +531,18 @@ where let (next_duty, payload_attributes) = api.fetch_proposer_and_attributes(payload.slot(), payload.parent_hash(), &request_id).await?; + // Handle trusted builders check + if !api.check_if_trusted_builder(&next_duty, &builder_info).await? { + let proposer_trusted_builders = next_duty.entry.preferences.trusted_builders.unwrap(); + warn!( + request_id = %request_id, + builder_pub_key = ?payload.builder_public_key(), + proposer_trusted_builders = ?proposer_trusted_builders, + "builder not in proposer trusted builders list", + ); + return Err(BuilderApiError::BuilderNotInProposersTrustedList { proposer_trusted_builders }); + } + // Verify payload has not already been delivered match api.auctioneer.get_last_slot_delivered().await { Ok(Some(slot)) => { @@ -519,6 +561,7 @@ where payload, next_duty, &payload_attributes, + &builder_info, head_slot, &mut trace, &request_id, @@ -801,6 +844,7 @@ where mut payload: SignedBidSubmission, next_duty: BuilderGetValidatorsResponseEntry, payload_attributes: &PayloadAttributesUpdate, + builder_info: &BuilderInfo, head_slot: u64, trace: &mut SubmissionTrace, request_id: &Uuid, @@ -828,7 +872,13 @@ where // Simulate the submission let payload = Arc::new(payload); - let was_simulated_optimistically = self.simulate_submission(payload.clone(), trace, next_duty.entry, request_id).await?; + let was_simulated_optimistically = self.simulate_submission( + payload.clone(), + builder_info, + trace, + next_duty.entry, + request_id, + ).await?; Ok((payload, was_simulated_optimistically)) } @@ -926,6 +976,30 @@ where Ok(floor_bid_value) } + /// If the proposer has specified a list of trusted builders ensure + /// that the submitting builder pubkey is in that list. + /// Verifies that if the proposer has specified a list of trusted builders, + /// the builder submitting a request is in that list. + /// + /// The auctioneer maintains a mapping of builder public keys to corresponding IDs. + /// This function retrieves the ID associated with the builder's public key from the auctioneer. + /// It then checks if this ID is included in the list of trusted builders specified by the proposer. + async fn check_if_trusted_builder( + &self, + next_duty: &BuilderGetValidatorsResponseEntry, + builder_info: &BuilderInfo, + ) -> Result { + if let Some(trusted_builders) = &next_duty.entry.preferences.trusted_builders { + // Cannot trust Unknown ID + if let BuilderID::Unknown = builder_info.builder_id { + return Ok(false); + } + return Ok(trusted_builders.contains(&builder_info.builder_id)); + } else { + Ok(true) + } + } + /// Simulates a new block payload. /// /// 1. Checks the current top bid value from the auctioneer. @@ -933,6 +1007,7 @@ where async fn simulate_submission( &self, payload: Arc, + builder_info: &BuilderInfo, trace: &mut SubmissionTrace, registration_info: ValidatorRegistrationInfo, request_id: &Uuid, @@ -962,24 +1037,24 @@ where ); let result = self .simulator - .process_request(sim_request, is_top_bid, self.db_sender.clone(), *request_id) - .await; + .process_request( + sim_request, + builder_info, + is_top_bid, + self.db_sender.clone(), + *request_id, + ).await; match result { - Ok(sim_optimistic) => { - info!(request_id = %request_id, "block simulation successful"); trace.simulation = get_nanos_timestamp()?; debug!(request_id = %request_id, sim_latency = trace.simulation - trace.signature); Ok(sim_optimistic) - }, - Err(err) => { - match &err { BlockSimError::BlockValidationFailed(reason) => { warn!(request_id = %request_id, error = %reason, "block validation failed"); @@ -1149,57 +1224,99 @@ where async fn check_builder_collateral( &self, payload: &impl BidSubmission, + builder_info: &BuilderInfo, request_id: &Uuid, ) -> Result<(), BuilderApiError> { - match self.auctioneer.get_builder_info(payload.builder_public_key()).await { - Ok(info) => { - if !info.is_optimistic { - warn!( - request_id = %request_id, - builder=%payload.builder_public_key(), - "builder is not optimistic" - ); - return Err(BuilderApiError::BuilderDemoted { - builder_pub_key: payload.builder_public_key().clone(), - }); - } else if info.collateral < payload.value() { - warn!( - request_id = %request_id, - builder=%payload.builder_public_key(), - collateral=%info.collateral, - collateral_required=%payload.value(), - "builder does not have enough collateral" - ); - return Err(BuilderApiError::NotEnoughOptimisticCollateral { - builder_pub_key: payload.builder_public_key().clone(), - collateral: info.collateral, - collateral_required: payload.value(), - is_optimistic: info.is_optimistic, - }); - } - }, - Err(err) => { - // No builder info stored for this pubkey - debug!( - request_id = %request_id, - error=%err, - builder=%payload.builder_public_key(), - block_hash=%payload.block_hash(), - "failed to retrieve builder info" - ); - return Err(BuilderApiError::NotEnoughOptimisticCollateral { - builder_pub_key: payload.builder_public_key().clone(), - collateral: U256::ZERO, - collateral_required: payload.value(), - is_optimistic: false, - }); - } - }; + if !builder_info.is_optimistic { + warn!( + request_id = %request_id, + builder=%payload.builder_public_key(), + "builder is not optimistic" + ); + return Err(BuilderApiError::BuilderDemoted { + builder_pub_key: payload.builder_public_key().clone(), + }); + } else if builder_info.collateral < payload.value() { + warn!( + request_id = %request_id, + builder=%payload.builder_public_key(), + collateral=%builder_info.collateral, + collateral_required=%payload.value(), + "builder does not have enough collateral" + ); + return Err(BuilderApiError::NotEnoughOptimisticCollateral { + builder_pub_key: payload.builder_public_key().clone(), + collateral: builder_info.collateral, + collateral_required: payload.value(), + is_optimistic: builder_info.is_optimistic, + }); + } + + + // match self.auctioneer.get_builder_info(payload.builder_public_key()).await { + // Ok(info) => { + // if !info.is_optimistic { + // warn!( + // request_id = %request_id, + // builder=%payload.builder_public_key(), + // "builder is not optimistic" + // ); + // return Err(BuilderApiError::BuilderDemoted { + // builder_pub_key: payload.builder_public_key().clone(), + // }); + // } else if info.collateral < payload.value() { + // warn!( + // request_id = %request_id, + // builder=%payload.builder_public_key(), + // collateral=%info.collateral, + // collateral_required=%payload.value(), + // "builder does not have enough collateral" + // ); + // return Err(BuilderApiError::NotEnoughOptimisticCollateral { + // builder_pub_key: payload.builder_public_key().clone(), + // collateral: info.collateral, + // collateral_required: payload.value(), + // is_optimistic: info.is_optimistic, + // }); + // } + // }, + // Err(err) => { + // // No builder info stored for this pubkey + // debug!( + // request_id = %request_id, + // error=%err, + // builder=%payload.builder_public_key(), + // block_hash=%payload.block_hash(), + // "failed to retrieve builder info" + // ); + // return Err(BuilderApiError::NotEnoughOptimisticCollateral { + // builder_pub_key: payload.builder_public_key().clone(), + // collateral: U256::ZERO, + // collateral_required: payload.value(), + // is_optimistic: false, + // }); + // } + // }; // Builder has enough collateral Ok(()) } + /// Fetch the builder's information. Default info is returned if fetching fails. + async fn fetch_builder_info(&self, builder_pub_key: &BlsPublicKey) -> BuilderInfo { + match self.auctioneer.get_builder_info(builder_pub_key).await { + Ok(info) => info, + Err(err) => { + warn!( + builder=%builder_pub_key, + err=%err, + "Failed to retrieve builder info" + ); + BuilderInfo { collateral: U256::ZERO, is_optimistic: false, builder_id: BuilderID::Unknown } + } + } + } + async fn demote_builder( &self, builder_pub_key: &BlsPublicKey, diff --git a/crates/api/src/builder/error.rs b/crates/api/src/builder/error.rs index 8fd55a90..9c08bf45 100644 --- a/crates/api/src/builder/error.rs +++ b/crates/api/src/builder/error.rs @@ -5,7 +5,7 @@ use ethereum_consensus::{ }; use hyper::StatusCode; use helix_datastore::error::AuctioneerError; -use helix_common::simulator::BlockSimError; +use helix_common::{simulator::BlockSimError, BuilderID}; #[derive(Debug, thiserror::Error)] pub enum BuilderApiError { @@ -106,6 +106,9 @@ pub enum BuilderApiError { #[error("builder has been demoted. builder_pub_key: {builder_pub_key:?}")] BuilderDemoted{builder_pub_key: BlsPublicKey}, + + #[error("builder not in proposer's trusted list: {proposer_trusted_builders:?}")] + BuilderNotInProposersTrustedList{proposer_trusted_builders: Vec}, } impl IntoResponse for BuilderApiError { @@ -219,6 +222,9 @@ impl IntoResponse for BuilderApiError { BuilderApiError::BuilderDemoted { builder_pub_key } => { (StatusCode::BAD_REQUEST, format!("builder has been demoted. builder_pub_key: {builder_pub_key:?}")).into_response() }, + BuilderApiError::BuilderNotInProposersTrustedList { proposer_trusted_builders } => { + (StatusCode::BAD_REQUEST, format!("builder not in proposer's trusted list: {proposer_trusted_builders:?}")).into_response() + } } } } diff --git a/crates/api/src/builder/simulator/mock_simulator.rs b/crates/api/src/builder/simulator/mock_simulator.rs index 6fa52ba3..2e4db6a9 100644 --- a/crates/api/src/builder/simulator/mock_simulator.rs +++ b/crates/api/src/builder/simulator/mock_simulator.rs @@ -1,7 +1,7 @@ use async_trait::async_trait; use tokio::sync::mpsc::Sender; -use helix_common::simulator::BlockSimError; +use helix_common::{simulator::BlockSimError, BuilderInfo}; use uuid::Uuid; use crate::builder::{DbInfo, traits::BlockSimulator, BlockSimRequest}; @@ -20,6 +20,7 @@ impl BlockSimulator for MockSimulator { async fn process_request( &self, _request: BlockSimRequest, + _builder_info: &BuilderInfo, _is_top_bid: bool, _sim_result_saver_sender: Sender, _request_id: Uuid, diff --git a/crates/api/src/builder/simulator/optimistic_simulator.rs b/crates/api/src/builder/simulator/optimistic_simulator.rs index 5433d395..2e19d09e 100644 --- a/crates/api/src/builder/simulator/optimistic_simulator.rs +++ b/crates/api/src/builder/simulator/optimistic_simulator.rs @@ -59,7 +59,7 @@ impl OptimisticSimulator ) -> Result<(), BlockSimError> { if let Err(err) = self .simulator - .process_request(request.clone(), is_top_bid, sim_result_saver_sender, request_id) + .process_request(request.clone(), &builder_info, is_top_bid, sim_result_saver_sender, request_id) .await { if let BlockSimError::BlockValidationFailed(_) = err { @@ -133,22 +133,6 @@ impl OptimisticSimulator } false } - - /// Fetch the builder's information. Default info is returned if fetching fails. - async fn fetch_builder_info(&self, request: &BlockSimRequest) -> BuilderInfo { - match self.auctioneer.get_builder_info(&request.message.builder_public_key).await { - Ok(info) => info, - Err(err) => { - warn!( - builder=%request.message.builder_public_key, - block_hash=%request.execution_payload.block_hash(), - err=%err, - "Failed to retrieve builder info" - ); - BuilderInfo { collateral: U256::ZERO, is_optimistic: false } - } - } - } } #[async_trait] @@ -156,11 +140,11 @@ impl BlockSimulator for OptimisticSimulator< async fn process_request( &self, request: BlockSimRequest, + builder_info: &BuilderInfo, is_top_bid: bool, sim_result_saver_sender: Sender, request_id: Uuid, ) -> Result { - let builder_info = self.fetch_builder_info(&request).await; // if self.should_process_optimistically(&request, &builder_info).await { if true { @@ -171,6 +155,7 @@ impl BlockSimulator for OptimisticSimulator< ); let cloned_self = self.clone_for_async(); + let builder_info = builder_info.clone(); tokio::spawn(async move { cloned_self .handle_simulation(request, is_top_bid, sim_result_saver_sender, builder_info, request_id) @@ -187,7 +172,7 @@ impl BlockSimulator for OptimisticSimulator< request=?request.message, "processing simulation request" ); - self.handle_simulation(request, is_top_bid, sim_result_saver_sender, builder_info, request_id) + self.handle_simulation(request, is_top_bid, sim_result_saver_sender, builder_info.clone(), request_id) .await .map(|_| false) } diff --git a/crates/api/src/builder/simulator/optimistic_simulator_tests.rs b/crates/api/src/builder/simulator/optimistic_simulator_tests.rs index 45f50c21..4774c724 100644 --- a/crates/api/src/builder/simulator/optimistic_simulator_tests.rs +++ b/crates/api/src/builder/simulator/optimistic_simulator_tests.rs @@ -21,9 +21,7 @@ mod simulator_tests { use helix_database::MockDatabaseService; use helix_datastore::MockAuctioneer; use helix_common::{ - bid_submission::{BidTrace, SignedBidSubmission, SignedBidSubmissionCapella}, - simulator::BlockSimError, - BuilderInfo, + bid_submission::{BidTrace, SignedBidSubmission, SignedBidSubmissionCapella}, simulator::BlockSimError, BuilderID, BuilderInfo }; use helix_common::ValidatorPreferences; @@ -97,11 +95,11 @@ mod simulator_tests { let builder_demoted = Arc::new(AtomicBool::new(false)); let (sim_res_sender, _sim_res_receiver) = tokio::sync::mpsc::channel(100); - let builder_info = BuilderInfo { collateral: U256::from(100), is_optimistic: true }; + let builder_info = BuilderInfo { collateral: U256::from(100), is_optimistic: true, builder_id: BuilderID::Titan }; let simulator = - get_optimistic_simulator(&server.url(), Some(builder_info), builder_demoted.clone()); + get_optimistic_simulator(&server.url(), Some(builder_info.clone()), builder_demoted.clone()); - let result = simulator.process_request(get_sim_req(), true, sim_res_sender, Uuid::new_v4()).await; + let result = simulator.process_request(get_sim_req(), &builder_info, true, sim_res_sender, Uuid::new_v4()).await; // give the simulator time to process the request tokio::time::sleep(tokio::time::Duration::from_millis(100)).await; @@ -122,11 +120,11 @@ mod simulator_tests { let builder_demoted = Arc::new(AtomicBool::new(false)); let (sim_res_sender, _sim_res_receiver) = tokio::sync::mpsc::channel(100); - let builder_info = BuilderInfo { collateral: U256::from(100), is_optimistic: true }; + let builder_info = BuilderInfo { collateral: U256::from(100), is_optimistic: true, builder_id: BuilderID::Titan }; let simulator = - get_optimistic_simulator(&server.url(), Some(builder_info), builder_demoted.clone()); + get_optimistic_simulator(&server.url(), Some(builder_info.clone()), builder_demoted.clone()); - let result = simulator.process_request(get_sim_req(), true, sim_res_sender, Uuid::new_v4()).await; + let result = simulator.process_request(get_sim_req(), &builder_info, true, sim_res_sender, Uuid::new_v4()).await; // give the simulator time to process the request tokio::time::sleep(tokio::time::Duration::from_millis(100)).await; @@ -147,11 +145,11 @@ mod simulator_tests { let builder_demoted = Arc::new(AtomicBool::new(false)); let (sim_res_sender, _sim_res_receiver) = tokio::sync::mpsc::channel(100); - let builder_info = BuilderInfo { collateral: U256::from(100), is_optimistic: false }; + let builder_info = BuilderInfo { collateral: U256::from(100), is_optimistic: false, builder_id: BuilderID::Titan }; let simulator = - get_optimistic_simulator(&server.url(), Some(builder_info), builder_demoted.clone()); + get_optimistic_simulator(&server.url(), Some(builder_info.clone()), builder_demoted.clone()); - let result = simulator.process_request(get_sim_req(), true, sim_res_sender, Uuid::new_v4()).await; + let result = simulator.process_request(get_sim_req(), &builder_info, true, sim_res_sender, Uuid::new_v4()).await; // give the simulator time to process the request tokio::time::sleep(tokio::time::Duration::from_millis(100)).await; @@ -172,11 +170,11 @@ mod simulator_tests { let builder_demoted = Arc::new(AtomicBool::new(false)); let (sim_res_sender, _sim_res_receiver) = tokio::sync::mpsc::channel(100); - let builder_info = BuilderInfo { collateral: U256::from(100), is_optimistic: false }; + let builder_info = BuilderInfo { collateral: U256::from(100), is_optimistic: false, builder_id: BuilderID::Titan }; let simulator = - get_optimistic_simulator(&server.url(), Some(builder_info), builder_demoted.clone()); + get_optimistic_simulator(&server.url(), Some(builder_info.clone()), builder_demoted.clone()); - let result = simulator.process_request(get_sim_req(), true, sim_res_sender, Uuid::new_v4()).await; + let result = simulator.process_request(get_sim_req(), &builder_info, true, sim_res_sender, Uuid::new_v4()).await; // give the simulator time to process the request tokio::time::sleep(tokio::time::Duration::from_millis(100)).await; diff --git a/crates/api/src/builder/simulator/rpc_simulator.rs b/crates/api/src/builder/simulator/rpc_simulator.rs index d7acb7a7..e9d274ef 100644 --- a/crates/api/src/builder/simulator/rpc_simulator.rs +++ b/crates/api/src/builder/simulator/rpc_simulator.rs @@ -1,4 +1,5 @@ use async_trait::async_trait; +use helix_common::BuilderInfo; use hyper::StatusCode; use reqwest::header::{HeaderMap, HeaderValue, CONTENT_TYPE}; use reqwest::{Client, Response}; @@ -80,6 +81,7 @@ impl BlockSimulator for RpcSimulator { async fn process_request( &self, request: BlockSimRequest, + _builder_info: &BuilderInfo, is_top_bid: bool, sim_result_saver_sender: Sender, request_id: Uuid, diff --git a/crates/api/src/builder/simulator/simulator_tests.rs b/crates/api/src/builder/simulator/simulator_tests.rs index f9e27730..b5bdec43 100644 --- a/crates/api/src/builder/simulator/simulator_tests.rs +++ b/crates/api/src/builder/simulator/simulator_tests.rs @@ -17,8 +17,7 @@ mod simulator_tests { use std::sync::Arc; use ethereum_consensus::types::mainnet::ExecutionPayload; use helix_common::{ - bid_submission::{BidTrace, SignedBidSubmission, SignedBidSubmissionCapella}, - simulator::BlockSimError, + bid_submission::{BidTrace, SignedBidSubmission, SignedBidSubmissionCapella}, simulator::BlockSimError, BuilderInfo }; use helix_common::ValidatorPreferences; @@ -67,7 +66,8 @@ mod simulator_tests { let (sim_res_sender, mut sim_res_receiver) = tokio::sync::mpsc::channel(100); let simulator = get_simulator(&server.url()); - let result = simulator.process_request(get_sim_req(), true, sim_res_sender, Uuid::new_v4()).await; + let builder_info = BuilderInfo::default(); + let result = simulator.process_request(get_sim_req(), &builder_info, true, sim_res_sender, Uuid::new_v4()).await; mock.assert(); assert!(result.is_ok()); @@ -97,7 +97,8 @@ mod simulator_tests { let (sim_res_sender, _sim_res_receiver) = tokio::sync::mpsc::channel(100); let simulator = get_simulator(&server.url()); - let result = simulator.process_request(get_sim_req(), true, sim_res_sender, Uuid::new_v4()).await; + let builder_info = BuilderInfo::default(); + let result = simulator.process_request(get_sim_req(), &builder_info, true, sim_res_sender, Uuid::new_v4()).await; mock.assert(); assert!(result.is_err()); @@ -126,7 +127,8 @@ mod simulator_tests { let (sim_res_sender, _sim_res_receiver) = tokio::sync::mpsc::channel(100); let simulator = get_simulator(&server.url()); - let result = simulator.process_request(get_sim_req(), true, sim_res_sender, Uuid::new_v4()).await; + let builder_info = BuilderInfo::default(); + let result = simulator.process_request(get_sim_req(), &builder_info, true, sim_res_sender, Uuid::new_v4()).await; mock.assert(); assert!(result.is_err()); diff --git a/crates/api/src/builder/simulator/traits.rs b/crates/api/src/builder/simulator/traits.rs index 9364f4bb..7132ba0f 100644 --- a/crates/api/src/builder/simulator/traits.rs +++ b/crates/api/src/builder/simulator/traits.rs @@ -1,5 +1,5 @@ use async_trait::async_trait; -use helix_common::simulator::BlockSimError; +use helix_common::{simulator::BlockSimError, BuilderInfo}; use tokio::sync::mpsc::Sender; use uuid::Uuid; @@ -11,6 +11,7 @@ pub trait BlockSimulator: Send + Sync + Clone { async fn process_request( &self, request: BlockSimRequest, + builder_info: &BuilderInfo, is_top_bid: bool, sim_result_saver_sender: Sender, request_id: Uuid, diff --git a/crates/common/src/builder_info.rs b/crates/common/src/builder_info.rs index 69847d6a..55444e2a 100644 --- a/crates/common/src/builder_info.rs +++ b/crates/common/src/builder_info.rs @@ -9,4 +9,18 @@ pub struct BuilderInfo { #[serde(with = "as_str")] pub collateral: U256, pub is_optimistic: bool, + pub builder_id: BuilderID, } + +#[derive(Debug, Clone, serde::Serialize, serde::Deserialize, PartialEq, Eq)] +pub enum BuilderID { + Titan, + TitanS, + Unknown, +} + +impl Default for BuilderID { + fn default() -> Self { + BuilderID::Unknown + } +} \ No newline at end of file diff --git a/crates/common/src/validator_preferences/mod.rs b/crates/common/src/validator_preferences/mod.rs index a6b40fa1..e6453b0f 100644 --- a/crates/common/src/validator_preferences/mod.rs +++ b/crates/common/src/validator_preferences/mod.rs @@ -1,6 +1,5 @@ -use self::trusted_builders::BuilderID; +use crate::BuilderID; -pub mod trusted_builders; #[derive(Debug, Clone, Default, serde::Serialize, serde::Deserialize)] pub struct ValidatorPreferences { diff --git a/crates/common/src/validator_preferences/trusted_builders.rs b/crates/common/src/validator_preferences/trusted_builders.rs deleted file mode 100644 index 14a9b5a8..00000000 --- a/crates/common/src/validator_preferences/trusted_builders.rs +++ /dev/null @@ -1,6 +0,0 @@ - -#[derive(Debug, Clone, serde::Serialize, serde::Deserialize)] -pub enum BuilderID { - Titan, - TitanS, -} \ No newline at end of file diff --git a/crates/database/src/postgres/postgres_db_row_parsing.rs b/crates/database/src/postgres/postgres_db_row_parsing.rs index bdd258d4..823a1cb0 100644 --- a/crates/database/src/postgres/postgres_db_row_parsing.rs +++ b/crates/database/src/postgres/postgres_db_row_parsing.rs @@ -209,6 +209,7 @@ impl FromRow for BuilderInfo { Ok(BuilderInfo { collateral: parse_numeric_to_u256(row.get::<&str, PostgresNumeric>("collateral"))?, is_optimistic: parse_bool_to_bool(row.get::<&str, bool>("is_optimistic"))?, + builder_id: todo!(), }) } } diff --git a/crates/database/src/postgres/postgres_db_service_tests.rs b/crates/database/src/postgres/postgres_db_service_tests.rs index 87cfcf87..87fa6954 100644 --- a/crates/database/src/postgres/postgres_db_service_tests.rs +++ b/crates/database/src/postgres/postgres_db_service_tests.rs @@ -13,8 +13,7 @@ use crate::{postgres::postgres_db_service::PostgresDatabaseService, DatabaseServ time::{SystemTime, UNIX_EPOCH}, }; use helix_common::{ - bid_submission::{BidTrace, SignedBidSubmission, v2::header_submission::SignedHeaderSubmission, BidSubmission}, - GetPayloadTrace, bellatrix::{ByteVector, ByteList, List}, HeaderSubmissionTrace, versioned_payload::PayloadAndBlobs, + bellatrix::{ByteVector, ByteList, List}, bid_submission::{BidTrace, SignedBidSubmission, v2::header_submission::SignedHeaderSubmission, BidSubmission}, versioned_payload::PayloadAndBlobs, BuilderID, GetPayloadTrace, HeaderSubmissionTrace }; use deadpool_postgres::{Config, ManagerConfig, Pool, RecyclingMethod}; @@ -293,7 +292,7 @@ use crate::{postgres::postgres_db_service::PostgresDatabaseService, DatabaseServ let key = SecretKey::random(&mut rng).unwrap(); let public_key = key.public_key(); let builder_info = - helix_common::BuilderInfo { collateral: Default::default(), is_optimistic: false }; + helix_common::BuilderInfo { collateral: Default::default(), is_optimistic: false, builder_id: BuilderID::Titan }; let result = db_service.store_builder_info(&public_key, builder_info).await; assert!(result.is_ok()); @@ -314,7 +313,7 @@ use crate::{postgres::postgres_db_service::PostgresDatabaseService, DatabaseServ let public_key = key.public_key(); let builder_info = - helix_common::BuilderInfo { collateral: Default::default(), is_optimistic: false }; + helix_common::BuilderInfo { collateral: Default::default(), is_optimistic: false, builder_id: BuilderID::Titan }; let result = db_service.store_builder_info(&public_key, builder_info).await; assert!(result.is_ok()); diff --git a/crates/datastore/src/auctioneer/mock_auctioneer.rs b/crates/datastore/src/auctioneer/mock_auctioneer.rs index 36d391c0..68fcc74b 100644 --- a/crates/datastore/src/auctioneer/mock_auctioneer.rs +++ b/crates/datastore/src/auctioneer/mock_auctioneer.rs @@ -3,6 +3,7 @@ use std::sync::{atomic::AtomicBool, Arc, Mutex}; use async_trait::async_trait; use ethereum_consensus::primitives::{BlsPublicKey, Hash32, U256}; +use helix_common::BuilderID; use helix_common::versioned_payload::PayloadAndBlobs; use helix_common::{ProposerInfo, ProposerInfoSet}; use helix_database::types::BuilderInfoDocument; diff --git a/crates/datastore/src/auctioneer/traits.rs b/crates/datastore/src/auctioneer/traits.rs index 4880994f..6050ba6a 100644 --- a/crates/datastore/src/auctioneer/traits.rs +++ b/crates/datastore/src/auctioneer/traits.rs @@ -2,7 +2,7 @@ use async_trait::async_trait; use ethereum_consensus::primitives::{BlsPublicKey, Hash32, U256}; use helix_database::BuilderInfoDocument; use helix_common::{ - bid_submission::{BidTrace, SignedBidSubmission, v2::header_submission::SignedHeaderSubmission}, builder_info::BuilderInfo, eth::SignedBuilderBid, signing::RelaySigningContext, versioned_payload::PayloadAndBlobs, ProposerInfo, ProposerInfoSet + bid_submission::{BidTrace, SignedBidSubmission, v2::header_submission::SignedHeaderSubmission}, builder_info::BuilderInfo, eth::SignedBuilderBid, signing::RelaySigningContext, BuilderID, versioned_payload::PayloadAndBlobs, ProposerInfo, ProposerInfoSet }; use crate::{error::AuctioneerError, types::SaveBidAndUpdateTopBidResponse}; diff --git a/crates/datastore/src/redis/redis_cache.rs b/crates/datastore/src/redis/redis_cache.rs index a7d9ff8b..ee3cca63 100644 --- a/crates/datastore/src/redis/redis_cache.rs +++ b/crates/datastore/src/redis/redis_cache.rs @@ -6,7 +6,7 @@ use ethereum_consensus::{ primitives::{BlsPublicKey, Hash32}, ssz::prelude::*, }; -use helix_common::{bid_submission::BidSubmission, versioned_payload::PayloadAndBlobs, ProposerInfo, ProposerInfoSet}; +use helix_common::{bid_submission::BidSubmission, BuilderID, versioned_payload::PayloadAndBlobs, ProposerInfo, ProposerInfoSet}; use helix_common::bid_submission::v2::header_submission::SignedHeaderSubmission; use redis::AsyncCommands; use serde::{de::DeserializeOwned, Serialize}; @@ -1323,7 +1323,7 @@ mod tests { let builder_pub_key = BlsPublicKey::default(); let unknown_builder_pub_key = BlsPublicKey::try_from([23u8; 48].as_ref()).unwrap(); - let builder_info = BuilderInfo { collateral: U256::from(12), is_optimistic: true }; + let builder_info = BuilderInfo { collateral: U256::from(12), is_optimistic: true, builder_id: BuilderID::Titan }; // Test case 1: Builder exists let set_result = @@ -1353,7 +1353,7 @@ mod tests { cache.clear_cache().await.unwrap(); let builder_pub_key = BlsPublicKey::try_from([23u8; 48].as_ref()).unwrap(); - let builder_info = BuilderInfo { collateral: U256::from(12), is_optimistic: false }; + let builder_info = BuilderInfo { collateral: U256::from(12), is_optimistic: false, builder_id: BuilderID::Titan }; // Set builder info in the cache let set_result = @@ -1371,7 +1371,7 @@ mod tests { cache.clear_cache().await.unwrap(); let builder_pub_key_optimistic = BlsPublicKey::try_from([11u8; 48].as_ref()).unwrap(); - let builder_info = BuilderInfo { collateral: U256::from(12), is_optimistic: true }; + let builder_info = BuilderInfo { collateral: U256::from(12), is_optimistic: true, builder_id: BuilderID::Titan }; // Set builder info in the cache let set_result = cache From ef5e9d67252464b3f91441d5f825fe9624e92a7e Mon Sep 17 00:00:00 2001 From: Rubenduburck Date: Fri, 26 Jan 2024 12:37:52 +0000 Subject: [PATCH 04/24] fix: bug where ```256``` was not encoded and decoded properly to and from postgres ```Numeric``` --- .../src/postgres/postgres_db_u256_parsing.rs | 182 ++++++++++++------ 1 file changed, 123 insertions(+), 59 deletions(-) diff --git a/crates/database/src/postgres/postgres_db_u256_parsing.rs b/crates/database/src/postgres/postgres_db_u256_parsing.rs index 33f9fe98..e48ca411 100644 --- a/crates/database/src/postgres/postgres_db_u256_parsing.rs +++ b/crates/database/src/postgres/postgres_db_u256_parsing.rs @@ -1,9 +1,7 @@ - use bytes::BufMut; use ethereum_consensus::primitives::U256; -use std::io::Read; -use tokio_postgres::types::{ToSql, FromSql}; +use tokio_postgres::types::{FromSql, ToSql}; #[derive(Debug, Clone)] pub struct PostgresNumeric(U256); @@ -20,29 +18,44 @@ impl From for U256 { } } -fn read_two_bytes(cursor: &mut std::io::Cursor<&[u8]>) -> std::io::Result<[u8; 2]> { - let mut result = [0; 2]; - cursor.read_exact(&mut result)?; - Ok(result) -} - -impl <'a>FromSql<'a> for PostgresNumeric { - fn from_sql(_: &tokio_postgres::types::Type, raw: &[u8]) -> Result> { - let mut raw = std::io::Cursor::new(raw); - let num_groups = u16::from_be_bytes(read_two_bytes(&mut raw)?); - - // We don't use all these, but we might at some point? - // In any case don't remove these because they advance the cursor - let _weight = i16::from_be_bytes(read_two_bytes(&mut raw)?); - let _sign = u16::from_be_bytes(read_two_bytes(&mut raw)?); - let _scale = u16::from_be_bytes(read_two_bytes(&mut raw)?); +/// Implements the `FromSql` trait for `PostgresNumeric`. +/// Some things to note about this implementation: +/// - Assumes positive numbers +/// - Assumes scale of 0 +/// - Assumes weight of 0 +/// As such not generalized, but good enough for our purposes +impl<'a> FromSql<'a> for PostgresNumeric { + fn from_sql( + _: &tokio_postgres::types::Type, + raw: &[u8], + ) -> Result> { + let mut offset = 0; + + // Function to read two bytes and advance the offset + let read_two_bytes = + |raw: &[u8], offset: &mut usize| -> std::io::Result { + if raw.len() < *offset + 2 { + return Err(std::io::Error::new( + std::io::ErrorKind::UnexpectedEof, + "Not enough bytes to read", + )); + } + let value = u16::from_be_bytes([raw[*offset], raw[*offset + 1]]); + *offset += 2; + Ok(value) + }; + + let num_groups = read_two_bytes(raw, &mut offset)?; + + // Skip the next 6 bytes (_weight, _sign, _scale) + offset += 6; let mut value = U256::from(0); - + let mut scalar = U256::from(1); for _ in 0..num_groups { - let group = u16::from_be_bytes(read_two_bytes(&mut raw)?); - value *= U256::from(10000_u64); - value += U256::from(group); + let group = read_two_bytes(raw, &mut offset)?; + value += scalar * U256::from(group); + scalar *= U256::from(10000_u64); } Ok(PostgresNumeric(value)) @@ -51,9 +64,18 @@ impl <'a>FromSql<'a> for PostgresNumeric { fn accepts(ty: &tokio_postgres::types::Type) -> bool { matches!(*ty, tokio_postgres::types::Type::NUMERIC) } - } +/// Implements the `ToSql` trait for `PostgresNumeric`. +/// Some things to note about this implementation: +/// - Assumes positive numbers +/// - Assumes scale of 0 +/// - Assumes weight of 0 +/// As such not generalized, but good enough for our purposes +/// Allows for MAX_GROUP_COUNT digit groups, each group is a value between 0 and 9999 +/// so the maximum value is 10000^MAX_GROUP_COUNT - 1 +/// with MAX_GROUP_COUNT = 32 this should be plenty to store any U256 +/// Obviously not sufficient for arbitrary precision. impl ToSql for PostgresNumeric { fn to_sql( &self, @@ -61,66 +83,108 @@ impl ToSql for PostgresNumeric { out: &mut bytes::BytesMut, ) -> std::result::Result> { - const MAX_GROUP_COUNT: usize = 16; - let mut digits = Vec::with_capacity(MAX_GROUP_COUNT); - let mut mut_self = self.0; - while mut_self != U256::from(0) { - let digit: i16 = (mut_self.as_limbs()[0] % 10000_u64).try_into().unwrap(); - digits.push(digit); - mut_self /= U256::from(10000_u64); + const MAX_GROUP_COUNT: usize = 32; + let divisor = U256::from(10000_u64); + let mut temp = self.0; + let mut digits = [0i16; MAX_GROUP_COUNT]; + let mut num_digits = 0; + + while temp != U256::from(0) { + let (quotient, remainder) = temp.div_rem(divisor); + digits[num_digits] = remainder.as_limbs()[0] as i16; + num_digits += 1; + temp = quotient; } - if digits.is_empty() { - digits.push(0); + + if num_digits == 0 { + num_digits = 1; // Ensure at least one digit } - let num_digits = digits.len(); - let weight = (num_digits - 1).try_into().unwrap(); - let neg = false; - let scale = 0_u16; + let weight = num_digits as i16 - 1; // Reserve bytes out.reserve(8 + num_digits * 2); // Number of groups - out.put_u16(num_digits.try_into().unwrap()); + out.put_u16(num_digits as u16); // Weight of first group out.put_i16(weight); - // Sign - out.put_u16(if neg { 0x4000 } else { 0x0000 }); - // DScale - out.put_u16(scale); - // Now process the number - for digit in digits[0..num_digits].iter() { - out.put_i16(*digit); + // Sign (assuming positive numbers) + out.put_u16(0x0000); + // DScale (assuming scale of 0) + out.put_u16(0); + + // Process the number + for i in 0..num_digits { + out.put_i16(digits[i]); } Ok(tokio_postgres::types::IsNull::No) } - fn accepts( - _: &tokio_postgres::types::Type, - ) -> bool - { + fn accepts(_: &tokio_postgres::types::Type) -> bool { true } tokio_postgres::types::to_sql_checked!(); - } - #[cfg(test)] mod tests { + use super::*; + use crate::postgres::postgres_db_u256_parsing::PostgresNumeric; use ethereum_consensus::primitives::U256; - use tokio_postgres::types::ToSql; - use crate::postgres::postgres_db_u256_parsing::PostgresNumeric; + fn get_values() -> Vec { + vec![ + U256::from(0), + U256::from(1), + U256::from(1234), + U256::from(12345678), + U256::from(u64::MAX), + U256::from_str_radix("1000_000_000_000_000_000", 10).unwrap(), + U256::from_str_radix("1000_000_000_000_000_000_000", 10).unwrap(), + U256::from_str_radix( + "1000_000_000_000_000_000_000_000_000_000_000_000_000_000_000", + 10, + ) + .unwrap(), + U256::MAX, + ] + } + #[test] + fn test_to_sql_manual_reconstruction() { + for value in get_values().into_iter() { + let mut bytes = bytes::BytesMut::new(); + let result = PostgresNumeric::from(value) + .to_sql(&tokio_postgres::types::Type::NUMERIC, &mut bytes); + assert!(result.is_ok()); + let digits = &bytes[8..] + .chunks_exact(2) + .map(|chunk| u16::from_be_bytes([chunk[0], chunk[1]])) + .collect::>(); + + let reconstructed_value = digits + .iter() + .rev() + .fold(U256::from(0), |acc, digit| acc * U256::from(10000_u64) + U256::from(*digit)); + + assert_eq!(value, reconstructed_value); + } + } #[test] - fn test_to_sql() { - let value = U256::from(1234); - let mut bytes = bytes::BytesMut::new(); - let result = PostgresNumeric::from(value).to_sql(&tokio_postgres::types::Type::NUMERIC, &mut bytes); - println!("bytes {:?}", bytes); - assert!(result.is_ok()); + fn test_to_sql_from_sql() { + for value in get_values().into_iter() { + let mut bytes = bytes::BytesMut::new(); + let result = PostgresNumeric::from(value) + .to_sql(&tokio_postgres::types::Type::NUMERIC, &mut bytes); + assert!(result.is_ok()); + let reconstructed_value = + PostgresNumeric::from_sql(&tokio_postgres::types::Type::NUMERIC, &bytes[..]) + .unwrap(); + + assert_eq!(value, reconstructed_value.0); + } } } + From 00703d2de1310a95e78805162557598120a7e6c5 Mon Sep 17 00:00:00 2001 From: Rubenduburck Date: Fri, 26 Jan 2024 14:24:29 +0000 Subject: [PATCH 05/24] fix: bug where digits for postgres ```Numeric``` type were ordered from least significant to most significant. --- .../src/postgres/postgres_db_service_tests.rs | 7 ++++--- .../src/postgres/postgres_db_u256_parsing.rs | 16 +++++++--------- 2 files changed, 11 insertions(+), 12 deletions(-) diff --git a/crates/database/src/postgres/postgres_db_service_tests.rs b/crates/database/src/postgres/postgres_db_service_tests.rs index 87cfcf87..8758c0a2 100644 --- a/crates/database/src/postgres/postgres_db_service_tests.rs +++ b/crates/database/src/postgres/postgres_db_service_tests.rs @@ -1,6 +1,6 @@ #[cfg(test)] mod tests { - use std::default::Default; + use std::{default::Default, str::FromStr}; use crate::{postgres::postgres_db_service::PostgresDatabaseService, DatabaseService}; use ethereum_consensus::{ builder::{SignedValidatorRegistration, ValidatorRegistration}, @@ -29,7 +29,8 @@ use crate::{postgres::postgres_db_service::PostgresDatabaseService, DatabaseServ /// These tests depend on a local instance of postgres running on port 5433 /// e.g. to start a local postgres instance in docker: - /// docker run -d --name postgres -e POSTGRES_PASSWORD=password -p 5433:5432 timescale/timescaledb + /// docker run -d --name postgres -e POSTGRES_PASSWORD=password -p 5433:5432 timescale/timescaledb-ha:pg16 + /// https://docs.timescale.com/self-hosted/latest/install/installation-docker/ fn test_config() -> Config { let mut cfg = Config::new(); @@ -293,7 +294,7 @@ use crate::{postgres::postgres_db_service::PostgresDatabaseService, DatabaseServ let key = SecretKey::random(&mut rng).unwrap(); let public_key = key.public_key(); let builder_info = - helix_common::BuilderInfo { collateral: Default::default(), is_optimistic: false }; + helix_common::BuilderInfo { collateral: U256::from_str("1000000000000000000000000000").unwrap(), is_optimistic: false }; let result = db_service.store_builder_info(&public_key, builder_info).await; assert!(result.is_ok()); diff --git a/crates/database/src/postgres/postgres_db_u256_parsing.rs b/crates/database/src/postgres/postgres_db_u256_parsing.rs index e48ca411..85abddd9 100644 --- a/crates/database/src/postgres/postgres_db_u256_parsing.rs +++ b/crates/database/src/postgres/postgres_db_u256_parsing.rs @@ -51,11 +51,8 @@ impl<'a> FromSql<'a> for PostgresNumeric { offset += 6; let mut value = U256::from(0); - let mut scalar = U256::from(1); for _ in 0..num_groups { - let group = read_two_bytes(raw, &mut offset)?; - value += scalar * U256::from(group); - scalar *= U256::from(10000_u64); + value = value * U256::from(10000_u64) + U256::from(read_two_bytes(raw, &mut offset)?); } Ok(PostgresNumeric(value)) @@ -113,9 +110,8 @@ impl ToSql for PostgresNumeric { // DScale (assuming scale of 0) out.put_u16(0); - // Process the number - for i in 0..num_digits { - out.put_i16(digits[i]); + for digit in digits.iter().take(num_digits).rev() { + out.put_i16(*digit); } Ok(tokio_postgres::types::IsNull::No) @@ -165,8 +161,9 @@ mod tests { let reconstructed_value = digits .iter() - .rev() - .fold(U256::from(0), |acc, digit| acc * U256::from(10000_u64) + U256::from(*digit)); + .fold(U256::from(0), |acc, digit| { + acc * U256::from(10000_u64) + U256::from(*digit) + }); assert_eq!(value, reconstructed_value); } @@ -186,5 +183,6 @@ mod tests { assert_eq!(value, reconstructed_value.0); } } + } From 1d376b7a7ab3f15abee5cecca99def556e0013d4 Mon Sep 17 00:00:00 2001 From: Rubenduburck Date: Fri, 26 Jan 2024 15:45:52 +0000 Subject: [PATCH 06/24] fix: bug where unexpected postgres behaviour caused from_sql to fail. This is because postgres does NOT store byte values as provided. It checks the type it receives and optimizes storage in some way, so we cannot expect the same bytes back as we put in. --- .../src/postgres/postgres_db_u256_parsing.rs | 34 ++++++++++++------- 1 file changed, 22 insertions(+), 12 deletions(-) diff --git a/crates/database/src/postgres/postgres_db_u256_parsing.rs b/crates/database/src/postgres/postgres_db_u256_parsing.rs index 85abddd9..32b1c95f 100644 --- a/crates/database/src/postgres/postgres_db_u256_parsing.rs +++ b/crates/database/src/postgres/postgres_db_u256_parsing.rs @@ -18,17 +18,23 @@ impl From for U256 { } } +const NBASE: u64 = 10000; + /// Implements the `FromSql` trait for `PostgresNumeric`. -/// Some things to note about this implementation: -/// - Assumes positive numbers -/// - Assumes scale of 0 -/// - Assumes weight of 0 -/// As such not generalized, but good enough for our purposes +/// We need a slightly generalized implementation since postgres +/// optimizes some stuff when storing so that the bytes we provide are not stored +/// in the exact way we provide them. +/// E.g. some tests have shown that 1000_000_000_000_000_000_000_000_000 is stored as +/// [0, 1, 0, 6, 0, 0, 0, 0, 3, 232] +/// sign and dscale are still not used + impl<'a> FromSql<'a> for PostgresNumeric { fn from_sql( _: &tokio_postgres::types::Type, raw: &[u8], ) -> Result> { + let n_base = U256::from(NBASE); + println!("from sql raw: {:?}", raw); let mut offset = 0; // Function to read two bytes and advance the offset @@ -46,15 +52,17 @@ impl<'a> FromSql<'a> for PostgresNumeric { }; let num_groups = read_two_bytes(raw, &mut offset)?; - - // Skip the next 6 bytes (_weight, _sign, _scale) - offset += 6; + let weight = read_two_bytes(raw, &mut offset)?; + let _sign = read_two_bytes(raw, &mut offset)?; + let _dscale = read_two_bytes(raw, &mut offset)?; let mut value = U256::from(0); for _ in 0..num_groups { - value = value * U256::from(10000_u64) + U256::from(read_two_bytes(raw, &mut offset)?); + value = value * n_base + U256::from(read_two_bytes(raw, &mut offset)?); } + value = value * n_base.pow(U256::from(weight)); + Ok(PostgresNumeric(value)) } @@ -70,7 +78,7 @@ impl<'a> FromSql<'a> for PostgresNumeric { /// - Assumes weight of 0 /// As such not generalized, but good enough for our purposes /// Allows for MAX_GROUP_COUNT digit groups, each group is a value between 0 and 9999 -/// so the maximum value is 10000^MAX_GROUP_COUNT - 1 +/// so the maximum value is NBASE^MAX_GROUP_COUNT - 1 /// with MAX_GROUP_COUNT = 32 this should be plenty to store any U256 /// Obviously not sufficient for arbitrary precision. impl ToSql for PostgresNumeric { @@ -81,7 +89,7 @@ impl ToSql for PostgresNumeric { ) -> std::result::Result> { const MAX_GROUP_COUNT: usize = 32; - let divisor = U256::from(10000_u64); + let divisor = U256::from(NBASE); let mut temp = self.0; let mut digits = [0i16; MAX_GROUP_COUNT]; let mut num_digits = 0; @@ -114,6 +122,8 @@ impl ToSql for PostgresNumeric { out.put_i16(*digit); } + println!("to sql out: {:?}", out.to_vec()); + Ok(tokio_postgres::types::IsNull::No) } @@ -162,7 +172,7 @@ mod tests { let reconstructed_value = digits .iter() .fold(U256::from(0), |acc, digit| { - acc * U256::from(10000_u64) + U256::from(*digit) + acc * U256::from(NBASE) + U256::from(*digit) }); assert_eq!(value, reconstructed_value); From 2842325a18a42f394bbef6b4ea1dc8dee380a150 Mon Sep 17 00:00:00 2001 From: gd-0 <90608901+gd-0@users.noreply.github.com> Date: Fri, 26 Jan 2024 15:54:33 +0000 Subject: [PATCH 07/24] trusted builders is now a String --- crates/api/src/builder/api.rs | 17 +++++++---------- crates/api/src/builder/error.rs | 4 ++-- .../simulator/optimistic_simulator_tests.rs | 10 +++++----- crates/common/src/builder_info.rs | 15 +-------------- crates/common/src/validator_preferences/mod.rs | 7 ++----- .../src/postgres/postgres_db_service_tests.rs | 6 +++--- .../datastore/src/auctioneer/mock_auctioneer.rs | 1 - crates/datastore/src/auctioneer/traits.rs | 2 +- crates/datastore/src/redis/redis_cache.rs | 8 ++++---- 9 files changed, 25 insertions(+), 45 deletions(-) diff --git a/crates/api/src/builder/api.rs b/crates/api/src/builder/api.rs index 7bdebf26..769a609e 100644 --- a/crates/api/src/builder/api.rs +++ b/crates/api/src/builder/api.rs @@ -1,11 +1,8 @@ use std::{ - collections::HashMap, - io::Read, - sync::{ + collections::HashMap, f64::consts::E, io::Read, sync::{ atomic::{AtomicU64, Ordering}, Arc, - }, - time::{SystemTime, UNIX_EPOCH}, + }, time::{SystemTime, UNIX_EPOCH} }; use axum::{ @@ -41,7 +38,7 @@ use helix_common::{ proposer_api::ValidatorRegistrationInfo, }, bid_submission::{ v2::header_submission::{SignedHeaderSubmission, SignedHeaderSubmissionCapella, SignedHeaderSubmissionDeneb}, BidSubmission, BidTrace, SignedBidSubmission, - }, chain_info::ChainInfo, signing::RelaySigningContext, simulator::BlockSimError, versioned_payload::PayloadAndBlobs, BuilderID, BuilderInfo, GossipedHeaderTrace, GossipedPayloadTrace, HeaderSubmissionTrace, SignedBuilderBid, SubmissionTrace + }, chain_info::ChainInfo, signing::RelaySigningContext, simulator::BlockSimError, versioned_payload::PayloadAndBlobs, BuilderInfo, GossipedHeaderTrace, GossipedPayloadTrace, HeaderSubmissionTrace, SignedBuilderBid, SubmissionTrace }; use helix_utils::{calculate_withdrawals_root, has_reached_fork, try_decode_into}; @@ -990,11 +987,11 @@ where builder_info: &BuilderInfo, ) -> Result { if let Some(trusted_builders) = &next_duty.entry.preferences.trusted_builders { - // Cannot trust Unknown ID - if let BuilderID::Unknown = builder_info.builder_id { + if let Some(builder_id) = &builder_info.builder_id { + return Ok(trusted_builders.contains(builder_id)); + } else { return Ok(false); } - return Ok(trusted_builders.contains(&builder_info.builder_id)); } else { Ok(true) } @@ -1312,7 +1309,7 @@ where err=%err, "Failed to retrieve builder info" ); - BuilderInfo { collateral: U256::ZERO, is_optimistic: false, builder_id: BuilderID::Unknown } + BuilderInfo { collateral: U256::ZERO, is_optimistic: false, builder_id: None } } } } diff --git a/crates/api/src/builder/error.rs b/crates/api/src/builder/error.rs index 9c08bf45..ba6db17a 100644 --- a/crates/api/src/builder/error.rs +++ b/crates/api/src/builder/error.rs @@ -5,7 +5,7 @@ use ethereum_consensus::{ }; use hyper::StatusCode; use helix_datastore::error::AuctioneerError; -use helix_common::{simulator::BlockSimError, BuilderID}; +use helix_common::simulator::BlockSimError; #[derive(Debug, thiserror::Error)] pub enum BuilderApiError { @@ -108,7 +108,7 @@ pub enum BuilderApiError { BuilderDemoted{builder_pub_key: BlsPublicKey}, #[error("builder not in proposer's trusted list: {proposer_trusted_builders:?}")] - BuilderNotInProposersTrustedList{proposer_trusted_builders: Vec}, + BuilderNotInProposersTrustedList{proposer_trusted_builders: Vec}, } impl IntoResponse for BuilderApiError { diff --git a/crates/api/src/builder/simulator/optimistic_simulator_tests.rs b/crates/api/src/builder/simulator/optimistic_simulator_tests.rs index 4774c724..15ab714b 100644 --- a/crates/api/src/builder/simulator/optimistic_simulator_tests.rs +++ b/crates/api/src/builder/simulator/optimistic_simulator_tests.rs @@ -21,7 +21,7 @@ mod simulator_tests { use helix_database::MockDatabaseService; use helix_datastore::MockAuctioneer; use helix_common::{ - bid_submission::{BidTrace, SignedBidSubmission, SignedBidSubmissionCapella}, simulator::BlockSimError, BuilderID, BuilderInfo + bid_submission::{BidTrace, SignedBidSubmission, SignedBidSubmissionCapella}, simulator::BlockSimError, BuilderInfo }; use helix_common::ValidatorPreferences; @@ -95,7 +95,7 @@ mod simulator_tests { let builder_demoted = Arc::new(AtomicBool::new(false)); let (sim_res_sender, _sim_res_receiver) = tokio::sync::mpsc::channel(100); - let builder_info = BuilderInfo { collateral: U256::from(100), is_optimistic: true, builder_id: BuilderID::Titan }; + let builder_info = BuilderInfo { collateral: U256::from(100), is_optimistic: true, builder_id: None }; let simulator = get_optimistic_simulator(&server.url(), Some(builder_info.clone()), builder_demoted.clone()); @@ -120,7 +120,7 @@ mod simulator_tests { let builder_demoted = Arc::new(AtomicBool::new(false)); let (sim_res_sender, _sim_res_receiver) = tokio::sync::mpsc::channel(100); - let builder_info = BuilderInfo { collateral: U256::from(100), is_optimistic: true, builder_id: BuilderID::Titan }; + let builder_info = BuilderInfo { collateral: U256::from(100), is_optimistic: true, builder_id: None }; let simulator = get_optimistic_simulator(&server.url(), Some(builder_info.clone()), builder_demoted.clone()); @@ -145,7 +145,7 @@ mod simulator_tests { let builder_demoted = Arc::new(AtomicBool::new(false)); let (sim_res_sender, _sim_res_receiver) = tokio::sync::mpsc::channel(100); - let builder_info = BuilderInfo { collateral: U256::from(100), is_optimistic: false, builder_id: BuilderID::Titan }; + let builder_info = BuilderInfo { collateral: U256::from(100), is_optimistic: false, builder_id: None }; let simulator = get_optimistic_simulator(&server.url(), Some(builder_info.clone()), builder_demoted.clone()); @@ -170,7 +170,7 @@ mod simulator_tests { let builder_demoted = Arc::new(AtomicBool::new(false)); let (sim_res_sender, _sim_res_receiver) = tokio::sync::mpsc::channel(100); - let builder_info = BuilderInfo { collateral: U256::from(100), is_optimistic: false, builder_id: BuilderID::Titan }; + let builder_info = BuilderInfo { collateral: U256::from(100), is_optimistic: false, builder_id: None }; let simulator = get_optimistic_simulator(&server.url(), Some(builder_info.clone()), builder_demoted.clone()); diff --git a/crates/common/src/builder_info.rs b/crates/common/src/builder_info.rs index 55444e2a..28f3a784 100644 --- a/crates/common/src/builder_info.rs +++ b/crates/common/src/builder_info.rs @@ -9,18 +9,5 @@ pub struct BuilderInfo { #[serde(with = "as_str")] pub collateral: U256, pub is_optimistic: bool, - pub builder_id: BuilderID, + pub builder_id: Option, } - -#[derive(Debug, Clone, serde::Serialize, serde::Deserialize, PartialEq, Eq)] -pub enum BuilderID { - Titan, - TitanS, - Unknown, -} - -impl Default for BuilderID { - fn default() -> Self { - BuilderID::Unknown - } -} \ No newline at end of file diff --git a/crates/common/src/validator_preferences/mod.rs b/crates/common/src/validator_preferences/mod.rs index e6453b0f..88ecd4f2 100644 --- a/crates/common/src/validator_preferences/mod.rs +++ b/crates/common/src/validator_preferences/mod.rs @@ -1,13 +1,10 @@ -use crate::BuilderID; - - #[derive(Debug, Clone, Default, serde::Serialize, serde::Deserialize)] pub struct ValidatorPreferences { /// A boolean flag indicating whether the validator requests the relay /// to enforce censoring of sanctioned transactions. pub censoring: bool, - /// An optional list of `BuilderID`s. If this is set, the relay will only accept + /// An optional list of BuilderIDs. If this is set, the relay will only accept /// submissions from builders whose public keys are linked to the IDs in this list. /// This allows for limiting submissions to a trusted set of builders. - pub trusted_builders: Option> + pub trusted_builders: Option> } \ No newline at end of file diff --git a/crates/database/src/postgres/postgres_db_service_tests.rs b/crates/database/src/postgres/postgres_db_service_tests.rs index 87fa6954..325fb2af 100644 --- a/crates/database/src/postgres/postgres_db_service_tests.rs +++ b/crates/database/src/postgres/postgres_db_service_tests.rs @@ -13,7 +13,7 @@ use crate::{postgres::postgres_db_service::PostgresDatabaseService, DatabaseServ time::{SystemTime, UNIX_EPOCH}, }; use helix_common::{ - bellatrix::{ByteVector, ByteList, List}, bid_submission::{BidTrace, SignedBidSubmission, v2::header_submission::SignedHeaderSubmission, BidSubmission}, versioned_payload::PayloadAndBlobs, BuilderID, GetPayloadTrace, HeaderSubmissionTrace + bellatrix::{ByteVector, ByteList, List}, bid_submission::{BidTrace, SignedBidSubmission, v2::header_submission::SignedHeaderSubmission}, versioned_payload::PayloadAndBlobs, GetPayloadTrace, HeaderSubmissionTrace }; use deadpool_postgres::{Config, ManagerConfig, Pool, RecyclingMethod}; @@ -292,7 +292,7 @@ use crate::{postgres::postgres_db_service::PostgresDatabaseService, DatabaseServ let key = SecretKey::random(&mut rng).unwrap(); let public_key = key.public_key(); let builder_info = - helix_common::BuilderInfo { collateral: Default::default(), is_optimistic: false, builder_id: BuilderID::Titan }; + helix_common::BuilderInfo { collateral: Default::default(), is_optimistic: false, builder_id: None }; let result = db_service.store_builder_info(&public_key, builder_info).await; assert!(result.is_ok()); @@ -313,7 +313,7 @@ use crate::{postgres::postgres_db_service::PostgresDatabaseService, DatabaseServ let public_key = key.public_key(); let builder_info = - helix_common::BuilderInfo { collateral: Default::default(), is_optimistic: false, builder_id: BuilderID::Titan }; + helix_common::BuilderInfo { collateral: Default::default(), is_optimistic: false, builder_id: None }; let result = db_service.store_builder_info(&public_key, builder_info).await; assert!(result.is_ok()); diff --git a/crates/datastore/src/auctioneer/mock_auctioneer.rs b/crates/datastore/src/auctioneer/mock_auctioneer.rs index 68fcc74b..36d391c0 100644 --- a/crates/datastore/src/auctioneer/mock_auctioneer.rs +++ b/crates/datastore/src/auctioneer/mock_auctioneer.rs @@ -3,7 +3,6 @@ use std::sync::{atomic::AtomicBool, Arc, Mutex}; use async_trait::async_trait; use ethereum_consensus::primitives::{BlsPublicKey, Hash32, U256}; -use helix_common::BuilderID; use helix_common::versioned_payload::PayloadAndBlobs; use helix_common::{ProposerInfo, ProposerInfoSet}; use helix_database::types::BuilderInfoDocument; diff --git a/crates/datastore/src/auctioneer/traits.rs b/crates/datastore/src/auctioneer/traits.rs index 6050ba6a..4880994f 100644 --- a/crates/datastore/src/auctioneer/traits.rs +++ b/crates/datastore/src/auctioneer/traits.rs @@ -2,7 +2,7 @@ use async_trait::async_trait; use ethereum_consensus::primitives::{BlsPublicKey, Hash32, U256}; use helix_database::BuilderInfoDocument; use helix_common::{ - bid_submission::{BidTrace, SignedBidSubmission, v2::header_submission::SignedHeaderSubmission}, builder_info::BuilderInfo, eth::SignedBuilderBid, signing::RelaySigningContext, BuilderID, versioned_payload::PayloadAndBlobs, ProposerInfo, ProposerInfoSet + bid_submission::{BidTrace, SignedBidSubmission, v2::header_submission::SignedHeaderSubmission}, builder_info::BuilderInfo, eth::SignedBuilderBid, signing::RelaySigningContext, versioned_payload::PayloadAndBlobs, ProposerInfo, ProposerInfoSet }; use crate::{error::AuctioneerError, types::SaveBidAndUpdateTopBidResponse}; diff --git a/crates/datastore/src/redis/redis_cache.rs b/crates/datastore/src/redis/redis_cache.rs index ee3cca63..01ce4d0c 100644 --- a/crates/datastore/src/redis/redis_cache.rs +++ b/crates/datastore/src/redis/redis_cache.rs @@ -6,7 +6,7 @@ use ethereum_consensus::{ primitives::{BlsPublicKey, Hash32}, ssz::prelude::*, }; -use helix_common::{bid_submission::BidSubmission, BuilderID, versioned_payload::PayloadAndBlobs, ProposerInfo, ProposerInfoSet}; +use helix_common::{bid_submission::BidSubmission, versioned_payload::PayloadAndBlobs, ProposerInfo, ProposerInfoSet}; use helix_common::bid_submission::v2::header_submission::SignedHeaderSubmission; use redis::AsyncCommands; use serde::{de::DeserializeOwned, Serialize}; @@ -1323,7 +1323,7 @@ mod tests { let builder_pub_key = BlsPublicKey::default(); let unknown_builder_pub_key = BlsPublicKey::try_from([23u8; 48].as_ref()).unwrap(); - let builder_info = BuilderInfo { collateral: U256::from(12), is_optimistic: true, builder_id: BuilderID::Titan }; + let builder_info = BuilderInfo { collateral: U256::from(12), is_optimistic: true, builder_id: None }; // Test case 1: Builder exists let set_result = @@ -1353,7 +1353,7 @@ mod tests { cache.clear_cache().await.unwrap(); let builder_pub_key = BlsPublicKey::try_from([23u8; 48].as_ref()).unwrap(); - let builder_info = BuilderInfo { collateral: U256::from(12), is_optimistic: false, builder_id: BuilderID::Titan }; + let builder_info = BuilderInfo { collateral: U256::from(12), is_optimistic: false, builder_id: None }; // Set builder info in the cache let set_result = @@ -1371,7 +1371,7 @@ mod tests { cache.clear_cache().await.unwrap(); let builder_pub_key_optimistic = BlsPublicKey::try_from([11u8; 48].as_ref()).unwrap(); - let builder_info = BuilderInfo { collateral: U256::from(12), is_optimistic: true, builder_id: BuilderID::Titan }; + let builder_info = BuilderInfo { collateral: U256::from(12), is_optimistic: true, builder_id: None }; // Set builder info in the cache let set_result = cache From 7670de866b0e6c174369504ac9e9ca271ed09376 Mon Sep 17 00:00:00 2001 From: owen Date: Fri, 26 Jan 2024 15:54:50 +0000 Subject: [PATCH 08/24] wip --- crates/housekeeper/src/housekeeper.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/crates/housekeeper/src/housekeeper.rs b/crates/housekeeper/src/housekeeper.rs index 7f987c8c..8a8724dd 100644 --- a/crates/housekeeper/src/housekeeper.rs +++ b/crates/housekeeper/src/housekeeper.rs @@ -1,4 +1,4 @@ -use std::{collections::HashMap, sync::Arc, time::{Duration, SystemTime}}; +use std::{collections::HashMap, sync::{atomic::AtomicBool, Arc}, time::{Duration, SystemTime}}; use ethereum_consensus::primitives::BlsPublicKey; use reth_primitives::{constants::EPOCH_SLOTS, revm_primitives::HashSet}; @@ -67,6 +67,8 @@ pub struct Housekeeper< re_sync_builder_info_slot: Mutex, re_sync_builder_info_lock: Mutex<()>, + + leader: Arc } impl From 3dcc5b68878746c9a32e6e2226ab156b44b946de Mon Sep 17 00:00:00 2001 From: gd-0 <90608901+gd-0@users.noreply.github.com> Date: Fri, 26 Jan 2024 16:03:33 +0000 Subject: [PATCH 09/24] code tidy --- crates/api/src/builder/api.rs | 65 +++++++---------------------------- 1 file changed, 12 insertions(+), 53 deletions(-) diff --git a/crates/api/src/builder/api.rs b/crates/api/src/builder/api.rs index 769a609e..d2041ba3 100644 --- a/crates/api/src/builder/api.rs +++ b/crates/api/src/builder/api.rs @@ -218,7 +218,7 @@ where let builder_info = api.fetch_builder_info(payload.builder_public_key()).await; // Handle trusted builders check - if !api.check_if_trusted_builder(&next_duty, &builder_info).await? { + if !api.check_if_trusted_builder(&next_duty, &builder_info).await { let proposer_trusted_builders = next_duty.entry.preferences.trusted_builders.unwrap(); warn!( request_id = %request_id, @@ -380,7 +380,7 @@ where } // Handle trusted builders check - if !api.check_if_trusted_builder(&next_duty, &builder_info).await? { + if !api.check_if_trusted_builder(&next_duty, &builder_info).await { let proposer_trusted_builders = next_duty.entry.preferences.trusted_builders.unwrap(); warn!( request_id = %request_id, @@ -529,7 +529,7 @@ where api.fetch_proposer_and_attributes(payload.slot(), payload.parent_hash(), &request_id).await?; // Handle trusted builders check - if !api.check_if_trusted_builder(&next_duty, &builder_info).await? { + if !api.check_if_trusted_builder(&next_duty, &builder_info).await { let proposer_trusted_builders = next_duty.entry.preferences.trusted_builders.unwrap(); warn!( request_id = %request_id, @@ -985,15 +985,20 @@ where &self, next_duty: &BuilderGetValidatorsResponseEntry, builder_info: &BuilderInfo, - ) -> Result { + ) -> bool { if let Some(trusted_builders) = &next_duty.entry.preferences.trusted_builders { + // Handle case where proposer specifies an empty list. + if trusted_builders.is_empty() { + return true; + } + if let Some(builder_id) = &builder_info.builder_id { - return Ok(trusted_builders.contains(builder_id)); + return trusted_builders.contains(builder_id); } else { - return Ok(false); + return false; } } else { - Ok(true) + true } } @@ -1249,52 +1254,6 @@ where }); } - - // match self.auctioneer.get_builder_info(payload.builder_public_key()).await { - // Ok(info) => { - // if !info.is_optimistic { - // warn!( - // request_id = %request_id, - // builder=%payload.builder_public_key(), - // "builder is not optimistic" - // ); - // return Err(BuilderApiError::BuilderDemoted { - // builder_pub_key: payload.builder_public_key().clone(), - // }); - // } else if info.collateral < payload.value() { - // warn!( - // request_id = %request_id, - // builder=%payload.builder_public_key(), - // collateral=%info.collateral, - // collateral_required=%payload.value(), - // "builder does not have enough collateral" - // ); - // return Err(BuilderApiError::NotEnoughOptimisticCollateral { - // builder_pub_key: payload.builder_public_key().clone(), - // collateral: info.collateral, - // collateral_required: payload.value(), - // is_optimistic: info.is_optimistic, - // }); - // } - // }, - // Err(err) => { - // // No builder info stored for this pubkey - // debug!( - // request_id = %request_id, - // error=%err, - // builder=%payload.builder_public_key(), - // block_hash=%payload.block_hash(), - // "failed to retrieve builder info" - // ); - // return Err(BuilderApiError::NotEnoughOptimisticCollateral { - // builder_pub_key: payload.builder_public_key().clone(), - // collateral: U256::ZERO, - // collateral_required: payload.value(), - // is_optimistic: false, - // }); - // } - // }; - // Builder has enough collateral Ok(()) } From 56434aa3617b5238582408944368996b5c3d39c5 Mon Sep 17 00:00:00 2001 From: owen Date: Fri, 26 Jan 2024 17:15:35 +0000 Subject: [PATCH 10/24] leader should be able to renew leadership --- .../src/auctioneer/mock_auctioneer.rs | 2 +- crates/datastore/src/auctioneer/traits.rs | 4 +- crates/datastore/src/redis/redis_cache.rs | 71 ++++++++++++++++--- crates/housekeeper/src/housekeeper.rs | 3 +- 4 files changed, 67 insertions(+), 13 deletions(-) diff --git a/crates/datastore/src/auctioneer/mock_auctioneer.rs b/crates/datastore/src/auctioneer/mock_auctioneer.rs index 5b2f020d..9278e5e6 100644 --- a/crates/datastore/src/auctioneer/mock_auctioneer.rs +++ b/crates/datastore/src/auctioneer/mock_auctioneer.rs @@ -214,7 +214,7 @@ impl Auctioneer for MockAuctioneer { Ok(None) } - async fn try_become_housekeeper(&self) -> bool { + async fn try_acquire_or_renew_leadership(&self, leader: Arc) -> bool { true } } diff --git a/crates/datastore/src/auctioneer/traits.rs b/crates/datastore/src/auctioneer/traits.rs index 011128bd..9919ffde 100644 --- a/crates/datastore/src/auctioneer/traits.rs +++ b/crates/datastore/src/auctioneer/traits.rs @@ -1,3 +1,5 @@ +use std::sync::{atomic::AtomicBool, Arc}; + use async_trait::async_trait; use ethereum_consensus::primitives::{BlsPublicKey, Hash32, U256}; use helix_database::BuilderInfoDocument; @@ -144,5 +146,5 @@ pub trait Auctioneer: Send + Sync + Clone { signing_context: &RelaySigningContext, ) -> Result, AuctioneerError>; - async fn try_become_housekeeper(&self) -> bool; + async fn try_acquire_or_renew_leadership(&self, leader: Arc) -> bool; } diff --git a/crates/datastore/src/redis/redis_cache.rs b/crates/datastore/src/redis/redis_cache.rs index 28778855..5e2d4dee 100644 --- a/crates/datastore/src/redis/redis_cache.rs +++ b/crates/datastore/src/redis/redis_cache.rs @@ -1,4 +1,4 @@ -use std::collections::HashMap; +use std::{collections::HashMap, sync::{atomic::{AtomicBool, Ordering}, Arc}}; use async_trait::async_trait; use deadpool_redis::{Config, CreatePoolError, Pool, Runtime}; @@ -38,7 +38,7 @@ use crate::{ }; const BID_CACHE_EXPIRY_S: usize = 45; -const HOUSEKEEPER_LOCK_EXPIRY_S: usize = 2; +const HOUSEKEEPER_LOCK_EXPIRY_S: usize = 17; #[derive(Clone)] pub struct RedisCache { @@ -170,6 +170,31 @@ impl RedisCache { } } + async fn renew_lock( + &self, + key: &str, + expiry: usize, + ) -> bool { + let mut conn = match self.pool.get().await { + Ok(conn) => conn, + Err(_) => return false, + }; + + let result: RedisResult = redis::cmd("SET") + .arg(key) + .arg(1) + .arg("XX") + .arg("PX") + .arg(expiry) + .query_async(&mut conn) + .await; + + match result { + Ok(Value::Okay) => true, + Ok(_) | Err(_) => false, + } + } + async fn hset( &self, key: &str, @@ -775,14 +800,40 @@ impl Auctioneer for RedisCache { Ok(Some(builder_bid)) } - // Sets the housekeeper lock if it's not already set. - // Returns true if the lock was set, false otherwise. - // This function is used to ensure that only one housekeeper is running at a time. - // The lock is set to expire after HOUSEKEEPER_LOCK_EXPIRY_S seconds. - // This will ensure that if a housekeeper crashes, the lock will eventually expire. - // Expiry is set to 2 seconds to ensure it will expire before the next slot. - async fn try_become_housekeeper(&self) -> bool { - self.set_lock("housekeeper_lock", HOUSEKEEPER_LOCK_EXPIRY_S).await + /// Attempts to acquire or renew leadership for a distributed task. + /// + /// This function checks if the current instance is already the leader based on the shared `leader` flag. + /// If not the leader, it attempts to acquire the leadership by setting a lock in Redis. + /// If already the leader, it attempts to renew the lock to maintain leadership. + /// The `leader` flag is updated based on the success of these operations. + /// + /// Expiry is set to `HOUSEKEEPER_LOCK_EXPIRY_S` seconds to ensure that the lock is released if the instance crashes. + /// HOUSEKEEPER_LOCK_EXPIRY_S should be long enought to allow the leader to renew the lock before it expires. + /// + /// Arguments: + /// - `leader`: An `Arc` shared among threads, indicating the current leadership status. + /// + /// Returns: + /// - `true` if the instance successfully acquires or renews the leadership. + /// - `false` if it fails to acquire or renew the leadership, or if the leadership is lost. + /// + /// Note: This function uses stronger atomic ordering (`Ordering::SeqCst`) for consistency across threads. + async fn try_acquire_or_renew_leadership(&self, leader: Arc) -> bool { + if !leader.load(Ordering::SeqCst) { + let now_leader = self.set_lock("housekeeper_lock", HOUSEKEEPER_LOCK_EXPIRY_S).await; + if now_leader { + leader.store(true, Ordering::SeqCst); + return true; + } + return false; + } else { + let still_leader = self.renew_lock("housekeeper_lock", HOUSEKEEPER_LOCK_EXPIRY_S).await; + if !still_leader { + leader.store(false, Ordering::SeqCst); + return false; + } + return true; + } } } diff --git a/crates/housekeeper/src/housekeeper.rs b/crates/housekeeper/src/housekeeper.rs index 8a8724dd..0777827a 100644 --- a/crates/housekeeper/src/housekeeper.rs +++ b/crates/housekeeper/src/housekeeper.rs @@ -87,6 +87,7 @@ impl refresh_validators_lock: Mutex::new(()), re_sync_builder_info_slot: Mutex::new(0), re_sync_builder_info_lock: Mutex::new(()), + leader: Arc::new(AtomicBool::new(false)), }) } @@ -119,7 +120,7 @@ impl } // Only allow one housekeeper task to run at a time. - if !self.auctioneer.try_become_housekeeper().await { + if !self.auctioneer.try_acquire_or_renew_leadership(self.leader.clone()).await { return; } From 54bfff4d9a11b69678d3e7ea1628d1fcd89ddd1a Mon Sep 17 00:00:00 2001 From: Rubenduburck Date: Fri, 26 Jan 2024 17:42:35 +0000 Subject: [PATCH 11/24] fix: removed debug prints --- crates/database/src/postgres/postgres_db_u256_parsing.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/crates/database/src/postgres/postgres_db_u256_parsing.rs b/crates/database/src/postgres/postgres_db_u256_parsing.rs index 32b1c95f..f48f8de6 100644 --- a/crates/database/src/postgres/postgres_db_u256_parsing.rs +++ b/crates/database/src/postgres/postgres_db_u256_parsing.rs @@ -34,7 +34,6 @@ impl<'a> FromSql<'a> for PostgresNumeric { raw: &[u8], ) -> Result> { let n_base = U256::from(NBASE); - println!("from sql raw: {:?}", raw); let mut offset = 0; // Function to read two bytes and advance the offset @@ -122,7 +121,6 @@ impl ToSql for PostgresNumeric { out.put_i16(*digit); } - println!("to sql out: {:?}", out.to_vec()); Ok(tokio_postgres::types::IsNull::No) } From cdb0d780b481ca03b4e129c990ab2c2912ed4d30 Mon Sep 17 00:00:00 2001 From: owen Date: Fri, 26 Jan 2024 17:35:10 +0000 Subject: [PATCH 12/24] move atomic bool update into housekeeper --- .../src/auctioneer/mock_auctioneer.rs | 2 +- crates/datastore/src/auctioneer/traits.rs | 2 +- crates/datastore/src/redis/redis_cache.rs | 40 ++++++++----------- crates/housekeeper/src/housekeeper.rs | 13 ++++-- 4 files changed, 29 insertions(+), 28 deletions(-) diff --git a/crates/datastore/src/auctioneer/mock_auctioneer.rs b/crates/datastore/src/auctioneer/mock_auctioneer.rs index 9278e5e6..74d9d3fa 100644 --- a/crates/datastore/src/auctioneer/mock_auctioneer.rs +++ b/crates/datastore/src/auctioneer/mock_auctioneer.rs @@ -214,7 +214,7 @@ impl Auctioneer for MockAuctioneer { Ok(None) } - async fn try_acquire_or_renew_leadership(&self, leader: Arc) -> bool { + async fn try_acquire_or_renew_leadership(&self, leader: bool) -> bool { true } } diff --git a/crates/datastore/src/auctioneer/traits.rs b/crates/datastore/src/auctioneer/traits.rs index 9919ffde..58eeb199 100644 --- a/crates/datastore/src/auctioneer/traits.rs +++ b/crates/datastore/src/auctioneer/traits.rs @@ -146,5 +146,5 @@ pub trait Auctioneer: Send + Sync + Clone { signing_context: &RelaySigningContext, ) -> Result, AuctioneerError>; - async fn try_acquire_or_renew_leadership(&self, leader: Arc) -> bool; + async fn try_acquire_or_renew_leadership(&self, leader: bool) -> bool; } diff --git a/crates/datastore/src/redis/redis_cache.rs b/crates/datastore/src/redis/redis_cache.rs index 5e2d4dee..de200273 100644 --- a/crates/datastore/src/redis/redis_cache.rs +++ b/crates/datastore/src/redis/redis_cache.rs @@ -800,40 +800,34 @@ impl Auctioneer for RedisCache { Ok(Some(builder_bid)) } - /// Attempts to acquire or renew leadership for a distributed task. - /// - /// This function checks if the current instance is already the leader based on the shared `leader` flag. - /// If not the leader, it attempts to acquire the leadership by setting a lock in Redis. - /// If already the leader, it attempts to renew the lock to maintain leadership. - /// The `leader` flag is updated based on the success of these operations. + + /// Attempts to acquire or renew leadership for a distributed task based on the current leadership status. + /// + /// If the instance is already a leader (indicated by the `leader` argument), it attempts to renew the lock. + /// If the lock renewal is successful, it returns `true`. + /// + /// If the instance is not currently a leader or fails to renew the lock, it attempts to acquire the lock. + /// The function returns `true` if the lock acquisition is successful, indicating leadership has been obtained. /// /// Expiry is set to `HOUSEKEEPER_LOCK_EXPIRY_S` seconds to ensure that the lock is released if the instance crashes. /// HOUSEKEEPER_LOCK_EXPIRY_S should be long enought to allow the leader to renew the lock before it expires. /// /// Arguments: - /// - `leader`: An `Arc` shared among threads, indicating the current leadership status. + /// - `leader`: A `bool` indicating whether the current instance believes it is the leader. /// /// Returns: - /// - `true` if the instance successfully acquires or renews the leadership. - /// - `false` if it fails to acquire or renew the leadership, or if the leadership is lost. + /// - `true` if the instance is the leader and successfully renews the lock, or if it successfully acquires the lock. + /// - `false` if it fails to renew or acquire the lock. /// - /// Note: This function uses stronger atomic ordering (`Ordering::SeqCst`) for consistency across threads. - async fn try_acquire_or_renew_leadership(&self, leader: Arc) -> bool { - if !leader.load(Ordering::SeqCst) { - let now_leader = self.set_lock("housekeeper_lock", HOUSEKEEPER_LOCK_EXPIRY_S).await; - if now_leader { - leader.store(true, Ordering::SeqCst); + /// Note: This function assumes that the caller manages and passes the current leadership status. + async fn try_acquire_or_renew_leadership(&self, leader: bool) -> bool { + if leader { + if self.renew_lock("housekeeper_lock", HOUSEKEEPER_LOCK_EXPIRY_S).await { return true; } - return false; - } else { - let still_leader = self.renew_lock("housekeeper_lock", HOUSEKEEPER_LOCK_EXPIRY_S).await; - if !still_leader { - leader.store(false, Ordering::SeqCst); - return false; - } - return true; } + + return self.set_lock("housekeeper_lock", HOUSEKEEPER_LOCK_EXPIRY_S).await; } } diff --git a/crates/housekeeper/src/housekeeper.rs b/crates/housekeeper/src/housekeeper.rs index 0777827a..0bde026b 100644 --- a/crates/housekeeper/src/housekeeper.rs +++ b/crates/housekeeper/src/housekeeper.rs @@ -68,7 +68,7 @@ pub struct Housekeeper< re_sync_builder_info_slot: Mutex, re_sync_builder_info_lock: Mutex<()>, - leader: Arc + leader: AtomicBool } impl @@ -87,7 +87,7 @@ impl refresh_validators_lock: Mutex::new(()), re_sync_builder_info_slot: Mutex::new(0), re_sync_builder_info_lock: Mutex::new(()), - leader: Arc::new(AtomicBool::new(false)), + leader: AtomicBool::new(false), }) } @@ -119,8 +119,15 @@ impl return; } + let current_leadership_status = self.leader.load(std::sync::atomic::Ordering::SeqCst); + let updated_leadership_status = self.auctioneer.try_acquire_or_renew_leadership(current_leadership_status).await; + + if updated_leadership_status != current_leadership_status { + self.leader.store(updated_leadership_status, std::sync::atomic::Ordering::SeqCst); + } + // Only allow one housekeeper task to run at a time. - if !self.auctioneer.try_acquire_or_renew_leadership(self.leader.clone()).await { + if !updated_leadership_status { return; } From 7f380fe8e9b6ed726563a7075e72b691f79a014e Mon Sep 17 00:00:00 2001 From: Dominik Thalmann Date: Fri, 26 Jan 2024 20:34:44 +0100 Subject: [PATCH 13/24] using hget / hgetall / hset to update and retrieve trusted proposer data from the auctioneer --- crates/api/src/proposer/api.rs | 7 +- crates/common/src/proposer.rs | 16 ---- .../src/auctioneer/mock_auctioneer.rs | 9 +- crates/datastore/src/auctioneer/traits.rs | 7 +- crates/datastore/src/redis/redis_cache.rs | 85 +++++++++++++++++-- 5 files changed, 90 insertions(+), 34 deletions(-) diff --git a/crates/api/src/proposer/api.rs b/crates/api/src/proposer/api.rs index 1b1d7012..6e59a649 100644 --- a/crates/api/src/proposer/api.rs +++ b/crates/api/src/proposer/api.rs @@ -988,11 +988,8 @@ where &self, public_key: &BlsPublicKey, ) -> Result { - Ok(self - .auctioneer - .get_trusted_proposers() - .await? - .map_or(false, |whitelist| whitelist.contains(public_key))) + let is_trusted_proposer = self.auctioneer.is_trusted_proposer(public_key).await?; + Ok(is_trusted_proposer) } } diff --git a/crates/common/src/proposer.rs b/crates/common/src/proposer.rs index 9a440987..cc15e08e 100644 --- a/crates/common/src/proposer.rs +++ b/crates/common/src/proposer.rs @@ -3,7 +3,6 @@ use ethereum_consensus::{ primitives::{BlsPublicKey, Slot, ValidatorIndex}, serde::as_str, }; -use reth_primitives::revm_primitives::HashSet; use serde::{Deserialize, Serialize}; #[derive(Debug, Serialize, Deserialize, Clone)] @@ -31,18 +30,3 @@ pub struct ProposerInfo { #[serde(rename = "pubkey")] pub pub_key: BlsPublicKey, } - -impl From> for ProposerInfoSet { - fn from(proposer_infos: Vec) -> Self { - ProposerInfoSet(proposer_infos.into_iter().map(|info| info.pub_key).collect()) - } -} - -#[derive(Debug, Serialize, Deserialize, Clone)] -pub struct ProposerInfoSet(HashSet); - -impl ProposerInfoSet { - pub fn contains(&self, public_key: &BlsPublicKey) -> bool { - self.0.contains(public_key) - } -} \ No newline at end of file diff --git a/crates/datastore/src/auctioneer/mock_auctioneer.rs b/crates/datastore/src/auctioneer/mock_auctioneer.rs index 36d391c0..c225830d 100644 --- a/crates/datastore/src/auctioneer/mock_auctioneer.rs +++ b/crates/datastore/src/auctioneer/mock_auctioneer.rs @@ -4,7 +4,7 @@ use async_trait::async_trait; use ethereum_consensus::primitives::{BlsPublicKey, Hash32, U256}; use helix_common::versioned_payload::PayloadAndBlobs; -use helix_common::{ProposerInfo, ProposerInfoSet}; +use helix_common::ProposerInfo; use helix_database::types::BuilderInfoDocument; use helix_common::{signing::RelaySigningContext, bid_submission::v2::header_submission::SignedHeaderSubmission}; use helix_common::{ @@ -222,9 +222,10 @@ impl Auctioneer for MockAuctioneer { Ok(()) } - async fn get_trusted_proposers( + async fn is_trusted_proposer( &self, - ) -> Result, AuctioneerError> { - Ok(Some(ProposerInfoSet::from(vec![]))) + _proposer_pub_key: &BlsPublicKey, + ) -> Result { + Ok(true) } } diff --git a/crates/datastore/src/auctioneer/traits.rs b/crates/datastore/src/auctioneer/traits.rs index 4880994f..9b29bb0c 100644 --- a/crates/datastore/src/auctioneer/traits.rs +++ b/crates/datastore/src/auctioneer/traits.rs @@ -2,7 +2,7 @@ use async_trait::async_trait; use ethereum_consensus::primitives::{BlsPublicKey, Hash32, U256}; use helix_database::BuilderInfoDocument; use helix_common::{ - bid_submission::{BidTrace, SignedBidSubmission, v2::header_submission::SignedHeaderSubmission}, builder_info::BuilderInfo, eth::SignedBuilderBid, signing::RelaySigningContext, versioned_payload::PayloadAndBlobs, ProposerInfo, ProposerInfoSet + bid_submission::{BidTrace, SignedBidSubmission, v2::header_submission::SignedHeaderSubmission}, builder_info::BuilderInfo, eth::SignedBuilderBid, signing::RelaySigningContext, versioned_payload::PayloadAndBlobs, ProposerInfo, }; use crate::{error::AuctioneerError, types::SaveBidAndUpdateTopBidResponse}; @@ -146,5 +146,8 @@ pub trait Auctioneer: Send + Sync + Clone { proposer_whitelist: Vec, ) -> Result<(), AuctioneerError>; - async fn get_trusted_proposers(&self) -> Result, AuctioneerError>; + async fn is_trusted_proposer( + &self, + proposer_pub_key: &BlsPublicKey, + ) -> Result; } diff --git a/crates/datastore/src/redis/redis_cache.rs b/crates/datastore/src/redis/redis_cache.rs index a7d9ff8b..06b80017 100644 --- a/crates/datastore/src/redis/redis_cache.rs +++ b/crates/datastore/src/redis/redis_cache.rs @@ -6,7 +6,7 @@ use ethereum_consensus::{ primitives::{BlsPublicKey, Hash32}, ssz::prelude::*, }; -use helix_common::{bid_submission::BidSubmission, versioned_payload::PayloadAndBlobs, ProposerInfo, ProposerInfoSet}; +use helix_common::{bid_submission::BidSubmission, versioned_payload::PayloadAndBlobs, ProposerInfo}; use helix_common::bid_submission::v2::header_submission::SignedHeaderSubmission; use redis::AsyncCommands; use serde::{de::DeserializeOwned, Serialize}; @@ -753,16 +753,40 @@ impl Auctioneer for RedisCache { &self, proposer_whitelist: Vec, ) -> Result<(), AuctioneerError> { - let proposer_whitelist_map: ProposerInfoSet = proposer_whitelist.into(); - self.set(PROPOSER_WHITELIST_KEY, &proposer_whitelist_map, None) - .await?; + // get keys + let proposer_keys: Vec = proposer_whitelist + .iter() + .map(|proposer| format!("{:?}", proposer.pub_key)) + .collect(); + + // add or update proposers + for proposer in proposer_whitelist { + let key_str = format!("{:?}", proposer.pub_key); + self.hset(PROPOSER_WHITELIST_KEY, &key_str, &proposer).await?; + } + + // remove any proposers that are no longer in the list + let proposer_info: Option> = + self.hgetall(PROPOSER_WHITELIST_KEY).await?; + if let Some(proposer_info) = proposer_info { + for key in proposer_info.keys() { + if !proposer_keys.contains(key) { + self.hdel(PROPOSER_WHITELIST_KEY, key).await?; + } + } + } + Ok(()) } - async fn get_trusted_proposers(&self) -> Result, AuctioneerError> { - let proposer_whitelist_map = self.get(PROPOSER_WHITELIST_KEY).await?; - Ok(proposer_whitelist_map) + async fn is_trusted_proposer( + &self, + proposer_pub_key: &BlsPublicKey, + ) -> Result { + let key_str = format!("{proposer_pub_key:?}"); + let proposer_info: Option = self.hget(PROPOSER_WHITELIST_KEY, &key_str).await?; + Ok(proposer_info.is_some()) } } @@ -1347,6 +1371,53 @@ mod tests { ); } + #[tokio::test] + async fn test_get_trusted_proposers_and_update_trusted_proposers() { + + let cache = RedisCache::new("redis://127.0.0.1/", Vec::new()).await.unwrap(); + cache.clear_cache().await.unwrap(); + + let is_trusted = cache.is_trusted_proposer(&BlsPublicKey::default()).await.unwrap(); + assert!(!is_trusted, "Failed to check trusted proposer"); + + cache.update_trusted_proposers( + vec![ + ProposerInfo { + name: "test".to_string(), + pub_key: BlsPublicKey::default(), + }, + ProposerInfo { + name: "test2".to_string(), + pub_key: BlsPublicKey::try_from([23u8; 48].as_ref()).unwrap(), + }, + ] + ).await.unwrap(); + + let is_trusted = cache.is_trusted_proposer(&BlsPublicKey::default()).await.unwrap(); + assert!(is_trusted, "Failed to check trusted proposer"); + + let is_trusted = cache.is_trusted_proposer(&BlsPublicKey::try_from([23u8; 48].as_ref()).unwrap()).await.unwrap(); + assert!(is_trusted, "Failed to check trusted proposer"); + + let is_trusted = cache.is_trusted_proposer(&BlsPublicKey::try_from([24u8; 48].as_ref()).unwrap()).await.unwrap(); + assert!(!is_trusted, "Failed to check trusted proposer"); + + cache.update_trusted_proposers( + vec![ + ProposerInfo { + name: "test2".to_string(), + pub_key: BlsPublicKey::try_from([25u8; 48].as_ref()).unwrap(), + }, + ] + ).await.unwrap(); + + let is_trusted = cache.is_trusted_proposer(&BlsPublicKey::default()).await.unwrap(); + assert!(!is_trusted, "Failed to check trusted proposer"); + + let is_trusted = cache.is_trusted_proposer(&BlsPublicKey::try_from([25u8; 48].as_ref()).unwrap()).await.unwrap(); + assert!(is_trusted, "Failed to check trusted proposer"); + } + #[tokio::test] async fn test_demote_non_optimistic_builder() { let cache = RedisCache::new("redis://127.0.0.1/", Vec::new()).await.unwrap(); From b8353067911ce97d6b1575482ad9ed2e8b929cd0 Mon Sep 17 00:00:00 2001 From: gd-0 <90608901+gd-0@users.noreply.github.com> Date: Mon, 29 Jan 2024 13:13:38 +0000 Subject: [PATCH 14/24] fix some submit_block_v2 timestamps --- crates/api/src/builder/api.rs | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/crates/api/src/builder/api.rs b/crates/api/src/builder/api.rs index 53363c75..8a3f89fa 100644 --- a/crates/api/src/builder/api.rs +++ b/crates/api/src/builder/api.rs @@ -281,7 +281,7 @@ where request_id = %request_id, trace = ?trace, request_duration_ns = trace.receive - trace.request_finish, - "request finished" + "submit_block request finished" ); let optimistic_version = if was_simulated_optimistically { @@ -433,7 +433,7 @@ where request_id = %request_id, trace = ?trace, request_duration_ns = trace.receive - trace.request_finish, - "request finished" + "submit_header request finished" ); // Save submission to db. @@ -541,10 +541,20 @@ where error!(request_id = %request_id, error = %err, "failed to save execution payload"); return Err(BuilderApiError::AuctioneerError(err)); } + trace.auctioneer_update = get_nanos_timestamp()?; // Gossip to other relays api.gossip_payload(&payload, payload.payload_and_blobs(), &request_id).await; + // Log some final info + trace.request_finish = get_nanos_timestamp()?; + info!( + request_id = %request_id, + trace = ?trace, + request_duration_ns = trace.receive - trace.request_finish, + "sumbit_block_v2 request finished" + ); + // Gossip payload api.db_sender .send(DbInfo::NewSubmission(payload.clone(), Arc::new(trace), OptimisticVersion::V2)) From f5d4457ad4024f1facea64a936f6a9cbc1f5c6c7 Mon Sep 17 00:00:00 2001 From: gd-0 <90608901+gd-0@users.noreply.github.com> Date: Mon, 29 Jan 2024 13:17:27 +0000 Subject: [PATCH 15/24] refactor --- crates/common/src/traces/builder_api_trace.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/common/src/traces/builder_api_trace.rs b/crates/common/src/traces/builder_api_trace.rs index f9e90a4c..7127d606 100644 --- a/crates/common/src/traces/builder_api_trace.rs +++ b/crates/common/src/traces/builder_api_trace.rs @@ -2,9 +2,9 @@ pub struct SubmissionTrace { pub receive: u64, pub decode: u64, + pub floor_bid_checks: u64, pub pre_checks: u64, pub signature: u64, - pub floor_bid_checks: u64, pub simulation: u64, pub auctioneer_update: u64, pub request_finish: u64, From 089e2a61e9fa1ede236fb4b549811714a29fa4cd Mon Sep 17 00:00:00 2001 From: owen Date: Mon, 29 Jan 2024 14:49:50 +0000 Subject: [PATCH 16/24] impl db parts for trusted builders --- .../migrations/V12__trusted_builders.sql | 5 +++ .../src/postgres/postgres_db_row_parsing.rs | 18 +++++++++-- .../src/postgres/postgres_db_service.rs | 32 ++++++++++--------- .../src/postgres/postgres_db_service_tests.rs | 6 +++- 4 files changed, 42 insertions(+), 19 deletions(-) create mode 100644 crates/database/src/postgres/migrations/V12__trusted_builders.sql diff --git a/crates/database/src/postgres/migrations/V12__trusted_builders.sql b/crates/database/src/postgres/migrations/V12__trusted_builders.sql new file mode 100644 index 00000000..b34f92e0 --- /dev/null +++ b/crates/database/src/postgres/migrations/V12__trusted_builders.sql @@ -0,0 +1,5 @@ +ALTER TABLE builder_info +ADD COLUMN "builder_id" varchar; + +ALTER TABLE validator_preferences +ADD COLUMN "trusted_builders" varchar[]; \ No newline at end of file diff --git a/crates/database/src/postgres/postgres_db_row_parsing.rs b/crates/database/src/postgres/postgres_db_row_parsing.rs index 823a1cb0..a3ea3b4e 100644 --- a/crates/database/src/postgres/postgres_db_row_parsing.rs +++ b/crates/database/src/postgres/postgres_db_row_parsing.rs @@ -182,7 +182,13 @@ impl FromRow for BuilderGetValidatorsResponseEntry { }, preferences: ValidatorPreferences { censoring: parse_bool_to_bool(row.get::<&str, bool>("censoring"))?, - trusted_builders: todo!(), + trusted_builders: row.get::<&str, Option>>("trusted_builders") + .map(|trusted_builders| { + trusted_builders + .into_iter() + .map(|builder| builder.to_string()) + .collect() + }), }, }, }) @@ -209,7 +215,7 @@ impl FromRow for BuilderInfo { Ok(BuilderInfo { collateral: parse_numeric_to_u256(row.get::<&str, PostgresNumeric>("collateral"))?, is_optimistic: parse_bool_to_bool(row.get::<&str, bool>("is_optimistic"))?, - builder_id: todo!(), + builder_id: row.get::<&str, Option<&str>>("builder_id").map(|s| s.to_string()), }) } } @@ -256,7 +262,13 @@ impl FromRow for SignedValidatorRegistrationEntry { registration: SignedValidatorRegistration::from_row(row)?, preferences: ValidatorPreferences { censoring: parse_bool_to_bool(row.get::<&str, bool>("censoring"))?, - trusted_builders: todo!(), + trusted_builders: row.get::<&str, Option>>("trusted_builders") + .map(|trusted_builders| { + trusted_builders + .into_iter() + .map(|builder| builder.to_string()) + .collect() + }), }, }, inserted_at: parse_timestamptz_to_u64( diff --git a/crates/database/src/postgres/postgres_db_service.rs b/crates/database/src/postgres/postgres_db_service.rs index 0b51d3e5..57718aea 100644 --- a/crates/database/src/postgres/postgres_db_service.rs +++ b/crates/database/src/postgres/postgres_db_service.rs @@ -155,7 +155,7 @@ impl PostgresDatabaseService { let mut structured_params_for_reg: Vec<(&[u8], i32, i64, &[u8], &[u8], SystemTime)> = Vec::with_capacity(chunk.len()); - let mut structured_params_for_pref: Vec<(&[u8], bool)> = + let mut structured_params_for_pref: Vec<(&[u8], bool, Option>)> = Vec::with_capacity(chunk.len()); for entry in chunk.iter() { @@ -176,7 +176,7 @@ impl PostgresDatabaseService { inserted_at, )); - structured_params_for_pref.push((public_key.as_ref(), entry.preferences.censoring)); + structured_params_for_pref.push((public_key.as_ref(), entry.preferences.censoring, entry.preferences.trusted_builders.clone())); } // Prepare the params vector from the structured parameters @@ -226,28 +226,28 @@ impl PostgresDatabaseService { let params: Vec<&(dyn ToSql + Sync)> = structured_params_for_pref .iter() - .flat_map(|tuple| vec![&tuple.0 as &(dyn ToSql + Sync), &tuple.1]) + .flat_map(|tuple| vec![&tuple.0 as &(dyn ToSql + Sync), &tuple.1, &tuple.2]) .collect(); // Construct the SQL statement with multiple VALUES clauses let mut sql = - String::from("INSERT INTO validator_preferences (public_key, censoring) VALUES "); + String::from("INSERT INTO validator_preferences (public_key, censoring, trusted_builders) VALUES "); let values_clauses: Vec = params - .chunks(2) + .chunks(3) .enumerate() .map(|(i, _)| { if i == 0 { - String::from("($1, $2)") + String::from("($1, $2, $3)") } else { - let offset = i * 2; - format!("(${}, ${})", offset + 1, offset + 2,) + let offset = i * 3; + format!("(${}, ${}, ${})", offset + 1, offset + 2, offset + 3,) } }) .collect(); // Join the values clauses and append them to the SQL statement sql.push_str(&values_clauses.join(", ")); - sql.push_str(" ON CONFLICT (public_key) DO UPDATE SET censoring = excluded.censoring"); + sql.push_str(" ON CONFLICT (public_key) DO UPDATE SET censoring = excluded.censoring, trusted_builders = excluded.trusted_builders"); // Execute the query transaction.execute(&sql, ¶ms[..]).await?; @@ -306,13 +306,13 @@ impl DatabaseService for PostgresDatabaseService { transaction .execute( - "INSERT INTO validator_preferences (public_key, censoring) - VALUES ($1, $2) + "INSERT INTO validator_preferences (public_key, censoring, trusted_builders) + VALUES ($1, $2, $3) ON CONFLICT (public_key) DO UPDATE SET - censoring = excluded.censoring + censoring = excluded.censoring, trusted_builders = excluded.trusted_builders ", - &[&public_key.as_ref(), ®istration_info.preferences.censoring], + &[&public_key.as_ref(), ®istration_info.preferences.censoring, ®istration_info.preferences.trusted_builders], ) .await?; @@ -398,6 +398,7 @@ impl DatabaseService for PostgresDatabaseService { validator_registrations.public_key, validator_registrations.signature, validator_preferences.censoring, + validator_preferences.trusted_builders, validator_registrations.inserted_at FROM validator_registrations INNER JOIN validator_preferences ON validator_registrations.public_key = validator_preferences.public_key @@ -907,8 +908,8 @@ impl DatabaseService for PostgresDatabaseService { .await? .execute( " - INSERT INTO builder_info (public_key, collateral, is_optimistic) - VALUES ($1, $2, $3) + INSERT INTO builder_info (public_key, collateral, is_optimistic, builder_id) + VALUES ($1, $2, $3, $4) ON CONFLICT (public_key) DO UPDATE SET collateral = excluded.collateral, @@ -918,6 +919,7 @@ impl DatabaseService for PostgresDatabaseService { &(builder_pub_key.as_ref()), &(PostgresNumeric::from(builder_info.collateral)), &(builder_info.is_optimistic), + &(builder_info.builder_id), ], ) .await?; diff --git a/crates/database/src/postgres/postgres_db_service_tests.rs b/crates/database/src/postgres/postgres_db_service_tests.rs index 4ebd0559..88449d41 100644 --- a/crates/database/src/postgres/postgres_db_service_tests.rs +++ b/crates/database/src/postgres/postgres_db_service_tests.rs @@ -23,6 +23,7 @@ use crate::{postgres::postgres_db_service::PostgresDatabaseService, DatabaseServ }; use tokio_postgres::NoTls; use helix_common::api::proposer_api::ValidatorRegistrationInfo; + use helix_common::validator_preferences::ValidatorPreferences; use crate::postgres::postgres_db_init::run_migrations_async; @@ -92,7 +93,10 @@ use crate::{postgres::postgres_db_service::PostgresDatabaseService, DatabaseServ }, signature, }, - preferences: Default::default(), + preferences: ValidatorPreferences { + censoring: false, + trusted_builders: Some(vec!["test".to_string(), "test2".to_string()]), + }, } } From f1c80766d2ef873de65dbc9b99578b80e80ff855 Mon Sep 17 00:00:00 2001 From: gd-0 <90608901+gd-0@users.noreply.github.com> Date: Mon, 29 Jan 2024 15:08:47 +0000 Subject: [PATCH 17/24] bump dockerfile rust version to 1.75 --- Dockerfile | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Dockerfile b/Dockerfile index 6dda5b3d..67817189 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,4 +1,4 @@ -FROM rust:1.72.0 as helix +FROM rust:1.75.0 as helix RUN apt update -y RUN apt install -y clang @@ -45,7 +45,7 @@ RUN --mount=type=cache,target=/root/.cargo \ RUN mv /app/$REPO_NAME/target/release/helix-cmd /app/helix-cmd # our final base -FROM debian:stable-slim +FROM debian:bullseye-slim RUN mkdir /root/logs From f65037a4ddffbc92515fd8ea07a2ab7670938dc3 Mon Sep 17 00:00:00 2001 From: gd-0 <90608901+gd-0@users.noreply.github.com> Date: Mon, 29 Jan 2024 15:42:00 +0000 Subject: [PATCH 18/24] set rust v back to 1.72 --- Dockerfile | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Dockerfile b/Dockerfile index 67817189..6dda5b3d 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,4 +1,4 @@ -FROM rust:1.75.0 as helix +FROM rust:1.72.0 as helix RUN apt update -y RUN apt install -y clang @@ -45,7 +45,7 @@ RUN --mount=type=cache,target=/root/.cargo \ RUN mv /app/$REPO_NAME/target/release/helix-cmd /app/helix-cmd # our final base -FROM debian:bullseye-slim +FROM debian:stable-slim RUN mkdir /root/logs From 6c393faa9bf3aa75c7cb1755df09208fe6e6ab5c Mon Sep 17 00:00:00 2001 From: gd-0 <90608901+gd-0@users.noreply.github.com> Date: Mon, 29 Jan 2024 16:14:41 +0000 Subject: [PATCH 19/24] trusted proposers log improvement --- crates/housekeeper/src/housekeeper.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/crates/housekeeper/src/housekeeper.rs b/crates/housekeeper/src/housekeeper.rs index c3a6fbae..31cb3df1 100644 --- a/crates/housekeeper/src/housekeeper.rs +++ b/crates/housekeeper/src/housekeeper.rs @@ -513,15 +513,14 @@ impl ); let proposer_whitelist = self.db.get_trusted_proposers().await?; - if proposer_whitelist.is_empty() { - warn!("The trusted proposers list is empty."); - } - + let num_trusted_proposers = proposer_whitelist.len(); + self.auctioneer.update_trusted_proposers(proposer_whitelist).await?; *self.refreshed_trusted_proposers_slot.lock().await = head_slot; debug!( head_slot = head_slot, + num_trusted_proposers = num_trusted_proposers, "updated trusted proposers" ); From 76fa6a7e745ce6dadba024182d33e09c145d3b9d Mon Sep 17 00:00:00 2001 From: gd-0 <90608901+gd-0@users.noreply.github.com> Date: Mon, 29 Jan 2024 16:41:57 +0000 Subject: [PATCH 20/24] better logs for housekeeper leadership + increase HOUSEKEEPER_LOCK_EXPIRY_S --- crates/datastore/src/auctioneer/traits.rs | 2 ++ crates/datastore/src/redis/redis_cache.rs | 33 ++++++++++++++++++----- crates/datastore/src/types/keys.rs | 1 + crates/housekeeper/src/housekeeper.rs | 19 ++++++------- 4 files changed, 39 insertions(+), 16 deletions(-) diff --git a/crates/datastore/src/auctioneer/traits.rs b/crates/datastore/src/auctioneer/traits.rs index d31dace8..9c563050 100644 --- a/crates/datastore/src/auctioneer/traits.rs +++ b/crates/datastore/src/auctioneer/traits.rs @@ -153,5 +153,7 @@ pub trait Auctioneer: Send + Sync + Clone { proposer_pub_key: &BlsPublicKey, ) -> Result; + /// Try to acquire or renew leadership for the housekeeper. + /// Returns: true if the housekeeper is the leader, false if it isn't. async fn try_acquire_or_renew_leadership(&self, leader: bool) -> bool; } diff --git a/crates/datastore/src/redis/redis_cache.rs b/crates/datastore/src/redis/redis_cache.rs index 04e5587f..d31a3a78 100644 --- a/crates/datastore/src/redis/redis_cache.rs +++ b/crates/datastore/src/redis/redis_cache.rs @@ -30,7 +30,7 @@ use crate::{ }, types::{ keys::{ - BUILDER_INFO_KEY, LAST_HASH_DELIVERED_KEY, LAST_SLOT_DELIVERED_KEY, PROPOSER_WHITELIST_KEY + BUILDER_INFO_KEY, HOUSEKEEPER_LOCK_KEY, LAST_HASH_DELIVERED_KEY, LAST_SLOT_DELIVERED_KEY, PROPOSER_WHITELIST_KEY }, SaveBidAndUpdateTopBidResponse, }, @@ -38,7 +38,6 @@ use crate::{ }; const BID_CACHE_EXPIRY_S: usize = 45; -const HOUSEKEEPER_LOCK_EXPIRY_S: usize = 17; #[derive(Clone)] pub struct RedisCache { @@ -145,11 +144,23 @@ impl RedisCache { } } + /// Attempts to set a lock in Redis with a specified key and expiry. + /// + /// This method uses an asynchronous Redis connection from a pool to execute the SET command. + /// It employs a locking pattern where the lock is set only if the key doesn't already exist + /// (`NX` option) and sets the lock to expire after a given duration (`expiry` in milliseconds). + /// + /// # Arguments + /// * `key` - A reference to a string slice that holds the key for the lock. + /// * `expiry` - The duration in milliseconds for which the lock should be valid. + /// + /// # Returns + /// Returns `true` if the lock was successfully acquired, or `false` if the lock was not set async fn set_lock( &self, key: &str, expiry: usize, - ) -> bool { + ) -> bool { let mut conn = match self.pool.get().await { Ok(conn) => conn, Err(_) => return false, @@ -170,6 +181,14 @@ impl RedisCache { } } + /// Attempts to renew an existing lock in Redis with a specified key and new expiry time. + /// + /// This method uses an asynchronous Redis connection from a pool to execute the SET command with + /// the 'XX' option, ensuring that the lock is only renewed if it already exists. + /// + /// # Arguments + /// * `key` - A reference to a string slice that holds the key of the lock to renew. + /// * `expiry` - The new duration in milliseconds for which the lock should be valid. async fn renew_lock( &self, key: &str, @@ -849,8 +868,8 @@ impl Auctioneer for RedisCache { /// If the instance is not currently a leader or fails to renew the lock, it attempts to acquire the lock. /// The function returns `true` if the lock acquisition is successful, indicating leadership has been obtained. /// - /// Expiry is set to `HOUSEKEEPER_LOCK_EXPIRY_S` seconds to ensure that the lock is released if the instance crashes. - /// HOUSEKEEPER_LOCK_EXPIRY_S should be long enought to allow the leader to renew the lock before it expires. + /// Expiry is set to `BID_CACHE_EXPIRY_S` seconds to ensure that the lock is released if the instance crashes. + /// BID_CACHE_EXPIRY_S should be long enought to allow the leader to renew the lock before it expires. /// /// Arguments: /// - `leader`: A `bool` indicating whether the current instance believes it is the leader. @@ -862,12 +881,12 @@ impl Auctioneer for RedisCache { /// Note: This function assumes that the caller manages and passes the current leadership status. async fn try_acquire_or_renew_leadership(&self, leader: bool) -> bool { if leader { - if self.renew_lock("housekeeper_lock", HOUSEKEEPER_LOCK_EXPIRY_S).await { + if self.renew_lock(HOUSEKEEPER_LOCK_KEY, BID_CACHE_EXPIRY_S).await { return true; } } - return self.set_lock("housekeeper_lock", HOUSEKEEPER_LOCK_EXPIRY_S).await; + return self.set_lock(HOUSEKEEPER_LOCK_KEY, BID_CACHE_EXPIRY_S).await; } } diff --git a/crates/datastore/src/types/keys.rs b/crates/datastore/src/types/keys.rs index 14519599..6f40683b 100644 --- a/crates/datastore/src/types/keys.rs +++ b/crates/datastore/src/types/keys.rs @@ -13,3 +13,4 @@ pub(crate) const EXEC_PAYLOAD_KEY: &str = "cache-exec-payload"; pub(crate) const BUILDER_INFO_KEY: &str = "builder-info"; pub(crate) const SEEN_BLOCK_HASHES_KEY: &str = "seen-block-hashes"; pub(crate) const PROPOSER_WHITELIST_KEY: &str = "proposer-whitelist"; +pub(crate) const HOUSEKEEPER_LOCK_KEY: &str = "housekeeper-lock"; diff --git a/crates/housekeeper/src/housekeeper.rs b/crates/housekeeper/src/housekeeper.rs index 31cb3df1..d68fddc6 100644 --- a/crates/housekeeper/src/housekeeper.rs +++ b/crates/housekeeper/src/housekeeper.rs @@ -1,4 +1,4 @@ -use std::{collections::HashMap, sync::{atomic::AtomicBool, Arc}, time::{Duration, SystemTime}}; +use std::{collections::HashMap, sync::{atomic::{AtomicBool, Ordering}, Arc}, time::{Duration, SystemTime}}; use ethereum_consensus::primitives::BlsPublicKey; use reth_primitives::{constants::EPOCH_SLOTS, revm_primitives::HashSet}; @@ -70,7 +70,7 @@ pub struct Housekeeper< refreshed_trusted_proposers_slot: Mutex, refresh_trusted_proposers_lock: Mutex<()>, - leader: AtomicBool + is_leader: AtomicBool } impl @@ -90,7 +90,7 @@ impl re_sync_builder_info_lock: Mutex::new(()), refreshed_trusted_proposers_slot: Mutex::new(0), refresh_trusted_proposers_lock: Mutex::new(()), - leader: AtomicBool::new(false), + is_leader: AtomicBool::new(false), }) } @@ -122,15 +122,16 @@ impl return; } - let current_leadership_status = self.leader.load(std::sync::atomic::Ordering::SeqCst); - let updated_leadership_status = self.auctioneer.try_acquire_or_renew_leadership(current_leadership_status).await; + let original_leadership_status = self.is_leader.load(Ordering::SeqCst); + let is_leader = self.auctioneer.try_acquire_or_renew_leadership(original_leadership_status).await; - if updated_leadership_status != current_leadership_status { - self.leader.store(updated_leadership_status, std::sync::atomic::Ordering::SeqCst); + // If the leadership status has changed, update the is_leader flag. + if is_leader != original_leadership_status { + self.is_leader.store(is_leader, Ordering::SeqCst); } // Only allow one housekeeper task to run at a time. - if !updated_leadership_status { + if !is_leader { return; } @@ -514,7 +515,7 @@ impl let proposer_whitelist = self.db.get_trusted_proposers().await?; let num_trusted_proposers = proposer_whitelist.len(); - + self.auctioneer.update_trusted_proposers(proposer_whitelist).await?; *self.refreshed_trusted_proposers_slot.lock().await = head_slot; From 9c43d481a44fdfb2dbea6a1c3b5616048b472304 Mon Sep 17 00:00:00 2001 From: gd-0 <90608901+gd-0@users.noreply.github.com> Date: Mon, 29 Jan 2024 16:45:00 +0000 Subject: [PATCH 21/24] bugfix: housekeeper expiry needs to be ms if we use redis: "PX" command --- crates/datastore/src/redis/redis_cache.rs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/crates/datastore/src/redis/redis_cache.rs b/crates/datastore/src/redis/redis_cache.rs index d31a3a78..c196c7de 100644 --- a/crates/datastore/src/redis/redis_cache.rs +++ b/crates/datastore/src/redis/redis_cache.rs @@ -1,4 +1,4 @@ -use std::{collections::HashMap, sync::{atomic::{AtomicBool, Ordering}, Arc}}; +use std::collections::HashMap; use async_trait::async_trait; use deadpool_redis::{Config, CreatePoolError, Pool, Runtime}; @@ -38,6 +38,7 @@ use crate::{ }; const BID_CACHE_EXPIRY_S: usize = 45; +const HOUSEKEEPER_LOCK_EXPIRY_MS: usize = 17_000; #[derive(Clone)] pub struct RedisCache { @@ -868,8 +869,8 @@ impl Auctioneer for RedisCache { /// If the instance is not currently a leader or fails to renew the lock, it attempts to acquire the lock. /// The function returns `true` if the lock acquisition is successful, indicating leadership has been obtained. /// - /// Expiry is set to `BID_CACHE_EXPIRY_S` seconds to ensure that the lock is released if the instance crashes. - /// BID_CACHE_EXPIRY_S should be long enought to allow the leader to renew the lock before it expires. + /// Expiry is set to `HOUSEKEEPER_LOCK_EXPIRY_MS` milliseconds to ensure that the lock is released if the instance crashes. + /// `HOUSEKEEPER_LOCK_EXPIRY_MS`` should be long enought to allow the leader to renew the lock before it expires. /// /// Arguments: /// - `leader`: A `bool` indicating whether the current instance believes it is the leader. @@ -881,12 +882,12 @@ impl Auctioneer for RedisCache { /// Note: This function assumes that the caller manages and passes the current leadership status. async fn try_acquire_or_renew_leadership(&self, leader: bool) -> bool { if leader { - if self.renew_lock(HOUSEKEEPER_LOCK_KEY, BID_CACHE_EXPIRY_S).await { + if self.renew_lock(HOUSEKEEPER_LOCK_KEY, HOUSEKEEPER_LOCK_EXPIRY_MS).await { return true; } } - return self.set_lock(HOUSEKEEPER_LOCK_KEY, BID_CACHE_EXPIRY_S).await; + return self.set_lock(HOUSEKEEPER_LOCK_KEY, HOUSEKEEPER_LOCK_EXPIRY_MS).await; } } From c43f37e3ad9a3d402b43e448a10a83bc93ff9e51 Mon Sep 17 00:00:00 2001 From: gd-0 <90608901+gd-0@users.noreply.github.com> Date: Mon, 29 Jan 2024 18:01:46 +0000 Subject: [PATCH 22/24] remove debug logs --- crates/common/src/eth/mod.rs | 6 ------ 1 file changed, 6 deletions(-) diff --git a/crates/common/src/eth/mod.rs b/crates/common/src/eth/mod.rs index 2096959d..60b06a9a 100644 --- a/crates/common/src/eth/mod.rs +++ b/crates/common/src/eth/mod.rs @@ -78,9 +78,6 @@ impl SignedBuilderBid { public_key, }; let signature = sign_builder_message(&mut message, signing_key, context)?; - - tracing::info!(builder_bid = ?message, "V2-DEBUG .THIS IS A V1 SUBMISSION"); - Ok(Self::Capella(capella::SignedBuilderBid {message, signature})) } ExecutionPayload::Deneb(payload) => { @@ -124,9 +121,6 @@ impl SignedBuilderBid { public_key, }; let signature = sign_builder_message(&mut message, signing_key, context)?; - - tracing::info!(builder_bid = ?message, "V2-DEBUG .THIS IS A V2 SUBMISSION"); - Ok(Self::Capella(capella::SignedBuilderBid {message, signature})) } ExecutionPayloadHeader::Deneb(header) => { From 9183ff4696f3eca8f3f6766be944be50bbe21bf1 Mon Sep 17 00:00:00 2001 From: gd-0 <90608901+gd-0@users.noreply.github.com> Date: Mon, 29 Jan 2024 19:26:43 +0000 Subject: [PATCH 23/24] if the proposer has censoring enabled, do not accept v2 submissions --- crates/api/src/builder/api.rs | 18 ++++++++++++++++-- crates/api/src/builder/error.rs | 6 ++++++ 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/crates/api/src/builder/api.rs b/crates/api/src/builder/api.rs index 80e8583b..fff7ed1f 100644 --- a/crates/api/src/builder/api.rs +++ b/crates/api/src/builder/api.rs @@ -1,5 +1,5 @@ use std::{ - collections::HashMap, f64::consts::E, io::Read, sync::{ + collections::HashMap, io::Read, sync::{ atomic::{AtomicU64, Ordering}, Arc, }, time::{SystemTime, UNIX_EPOCH} @@ -364,9 +364,17 @@ where &request_id, ).await?; - // Fetch the next proposer duty/ payload attributes and validate basic information about the payload + // Fetch the next proposer duty/ payload attributes let (next_duty, payload_attributes) = api.fetch_proposer_and_attributes(payload.slot(), payload.parent_hash(), &request_id).await?; + + // Discard any OptimisticV2 submissions if the proposer has censoring enabled + if next_duty.entry.preferences.censoring { + warn!(request_id = %request_id, "proposer has censoring enabled, discarding optimistic v2 submission"); + return Err(BuilderApiError::V2SubmissionsInvalidIfProposerCensors); + } + + // Validate basic information about the payload if let Err(err) = sanity_check_block_submission( &payload, payload.bid_trace(), @@ -528,6 +536,12 @@ where let (next_duty, payload_attributes) = api.fetch_proposer_and_attributes(payload.slot(), payload.parent_hash(), &request_id).await?; + // Discard any OptimisticV2 submissions if the proposer has censoring enabled + if next_duty.entry.preferences.censoring { + warn!(request_id = %request_id, "proposer has censoring enabled, discarding optimistic v2 submission"); + return Err(BuilderApiError::V2SubmissionsInvalidIfProposerCensors); + } + // Handle trusted builders check if !api.check_if_trusted_builder(&next_duty, &builder_info).await { let proposer_trusted_builders = next_duty.entry.preferences.trusted_builders.unwrap(); diff --git a/crates/api/src/builder/error.rs b/crates/api/src/builder/error.rs index ba6db17a..c19e271c 100644 --- a/crates/api/src/builder/error.rs +++ b/crates/api/src/builder/error.rs @@ -109,6 +109,9 @@ pub enum BuilderApiError { #[error("builder not in proposer's trusted list: {proposer_trusted_builders:?}")] BuilderNotInProposersTrustedList{proposer_trusted_builders: Vec}, + + #[error("V2 submissions invalid if proposer censors")] + V2SubmissionsInvalidIfProposerCensors, } impl IntoResponse for BuilderApiError { @@ -224,6 +227,9 @@ impl IntoResponse for BuilderApiError { }, BuilderApiError::BuilderNotInProposersTrustedList { proposer_trusted_builders } => { (StatusCode::BAD_REQUEST, format!("builder not in proposer's trusted list: {proposer_trusted_builders:?}")).into_response() + }, + BuilderApiError::V2SubmissionsInvalidIfProposerCensors => { + (StatusCode::BAD_REQUEST, "V2 submissions invalid if proposer censors").into_response() } } } From 014b64ac34e68966daebceac5f388c6dd0d210b0 Mon Sep 17 00:00:00 2001 From: gd-0 <90608901+gd-0@users.noreply.github.com> Date: Tue, 30 Jan 2024 09:20:26 +0000 Subject: [PATCH 24/24] bugfix: retain payload attributes that are greater or equal to head slot instead of lower --- crates/api/src/builder/api.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/api/src/builder/api.rs b/crates/api/src/builder/api.rs index fff7ed1f..5aa92a69 100644 --- a/crates/api/src/builder/api.rs +++ b/crates/api/src/builder/api.rs @@ -1400,7 +1400,7 @@ where // Clean up old payload attributes let mut all_payload_attributes = self.payload_attributes.write().await; - all_payload_attributes.retain(|_, value| value.slot < head_slot); + all_payload_attributes.retain(|_, value| value.slot >= head_slot); // Save new one all_payload_attributes.insert(payload_attributes.parent_hash.clone(), payload_attributes);