Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Integrate with the Bolt registry to check if pubkeys are registered #340

Merged
merged 15 commits into from
Oct 30, 2024
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion bolt-sidecar/src/crypto/ecdsa.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
use std::fmt::Debug;

use alloy::signers::{local::PrivateKeySigner, Signature as AlloySignature, Signer};
use alloy::{
primitives::Address,
signers::{local::PrivateKeySigner, Signature as AlloySignature, Signer},
};
use secp256k1::{ecdsa::Signature, Message, PublicKey, SecretKey};

/// Trait for any types that can be signed and verified with ECDSA.
Expand Down Expand Up @@ -57,12 +60,18 @@ impl ECDSASigner {
/// A generic signing trait to generate ECDSA signatures.
#[async_trait::async_trait]
pub trait SignerECDSA: Send + Debug {
/// Returns the public key of the signer.
fn public_key(&self) -> Address;
thedevbirb marked this conversation as resolved.
Show resolved Hide resolved
/// Sign the given hash and return the signature.
async fn sign_hash(&self, hash: &[u8; 32]) -> eyre::Result<AlloySignature>;
}

#[async_trait::async_trait]
impl SignerECDSA for PrivateKeySigner {
fn public_key(&self) -> Address {
self.address()
}

async fn sign_hash(&self, hash: &[u8; 32]) -> eyre::Result<AlloySignature> {
Ok(Signer::sign_hash(self, hash.into()).await?)
}
Expand Down
14 changes: 12 additions & 2 deletions bolt-sidecar/src/driver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,8 @@ use crate::{
start_builder_proxy_server,
state::{fetcher::StateFetcher, ConsensusState, ExecutionState, HeadTracker, StateClient},
telemetry::ApiMetrics,
BuilderProxyConfig, CommitBoostSigner, ConstraintsApi, ConstraintsClient, LocalBuilder, Opts,
SignerBLS,
BoltManager, BuilderProxyConfig, CommitBoostSigner, ConstraintsApi, ConstraintsClient,
LocalBuilder, Opts, SignerBLS,
};

/// The driver for the sidecar, responsible for managing the main event loop.
Expand Down Expand Up @@ -161,6 +161,16 @@ impl<C: StateFetcher, ECDSA: SignerECDSA> SidecarDriver<C, ECDSA> {
commitment_signer: ECDSA,
fetcher: C,
) -> eyre::Result<Self> {
// Verify the operator and validator keys with the bolt manager
if let Some(bolt_manager) =
BoltManager::from_chain(opts.execution_api_url.clone(), opts.chain.chain)
{
bolt_manager.verify_operator(commitment_signer.public_key()).await?;
bolt_manager
.verify_validator_pubkeys(&Vec::from_iter(constraint_signer.available_pubkeys()))
.await?;
thedevbirb marked this conversation as resolved.
Show resolved Hide resolved
}
thedevbirb marked this conversation as resolved.
Show resolved Hide resolved

let beacon_client = BeaconClient::new(opts.beacon_api_url.clone());
let execution = ExecutionState::new(fetcher, opts.limits).await?;

Expand Down
3 changes: 3 additions & 0 deletions bolt-sidecar/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,9 @@ pub mod state;
mod signer;
pub use signer::{commit_boost::CommitBoostSigner, SignerBLS};

mod onchain_registry;
pub use onchain_registry::BoltManager;

/// Utilities for testing
#[cfg(test)]
mod test_util;
163 changes: 163 additions & 0 deletions bolt-sidecar/src/onchain_registry.rs
thedevbirb marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,163 @@
use std::str::FromStr;

use alloy::{
contract::Error as ContractError,
primitives::{Address, Bytes, B256, B512},
providers::{ProviderBuilder, RootProvider},
sol,
sol_types::{Error as SolError, SolInterface},
transports::{http::Http, TransportError},
};
use ethereum_consensus::primitives::BlsPublicKey;
use eyre::bail;
use reqwest::{Client, Url};
use reth_primitives::keccak256;
use serde::Serialize;

use BoltManagerContract::{BoltManagerContractErrors, BoltManagerContractInstance, ProposerStatus};

use crate::config::chain::Chain;

/// A wrapper over a BoltManagerContract that exposes various utility methods.
#[derive(Debug, Clone)]
pub struct BoltManager(BoltManagerContractInstance<Http<Client>, RootProvider<Http<Client>>>);

impl BoltManager {
/// Returns the address of the canonical BoltManager contract for a given chain, if present
pub fn address(chain: Chain) -> Option<Address> {
match chain {
Chain::Holesky => Some(
Address::from_str("0x440202829b493F9FF43E730EB5e8379EEa3678CF")
.expect("valid address"),
),
_ => None,
}
}
thedevbirb marked this conversation as resolved.
Show resolved Hide resolved

/// Creates a new BoltRegistry instance. Returns `None` if a canonical BoltManager contract is
/// not deployed on such chain.
pub fn from_chain<U: Into<Url>>(execution_client_url: U, chain: Chain) -> Option<Self> {
let address = Self::address(chain)?;
Some(Self::from_address(execution_client_url, address))
}

/// Creates a new BoltRegistry instance.
pub fn from_address<U: Into<Url>>(execution_client_url: U, manager_address: Address) -> Self {
let provider = ProviderBuilder::new().on_http(execution_client_url.into());
let registry = BoltManagerContract::new(manager_address, provider);

Self(registry)
}

/// Verify the provided operator address is registered in Bolt, returning an error if it
/// doesn't
pub async fn verify_operator(&self, operator: Address) -> eyre::Result<()> {
let returndata = self.0.isOperator(operator).call().await;

if !returndata.map(|data| data.isOperator)? {
bail!("operator not found in Bolt Manager contract");
}

Ok(())
}

/// Verify the provided validator public keys are registered in Bolt and are active
pub async fn verify_validator_pubkeys(
&self,
keys: &[BlsPublicKey],
) -> eyre::Result<Vec<ProposerStatus>> {
let hashes = BoltValidators::pubkey_hashes(keys);

let returndata = self.0.getProposerStatuses(hashes).call().await;

// TODO: clean this after https://github.com/alloy-rs/alloy/issues/787 is merged
let error = match returndata.map(|data| data.statuses) {
Ok(statuses) => {
for status in &statuses {
if !status.active {
bail!(
"validator with public key hash {:?} is not active in Bolt",
status.pubkeyHash
);
}
}

return Ok(statuses);
}
Err(error) => match error {
ContractError::TransportError(TransportError::ErrorResp(err)) => {
let data = err.data.unwrap_or_default();
let data = data.get().trim_matches('"');
let data = Bytes::from_str(data).unwrap_or_default();
thedevbirb marked this conversation as resolved.
Show resolved Hide resolved

BoltManagerContractErrors::abi_decode(&data, true)?
}
e => return Err(e)?,
thedevbirb marked this conversation as resolved.
Show resolved Hide resolved
},
};

if matches!(error, BoltManagerContractErrors::ValidatorDoesNotExist(_)) {
// TODO: improve this error message once https://github.com/chainbound/bolt/issues/338
// is solved
bail!("not all validators are registered in Bolt");
} else {
Err(SolError::custom(format!(
"unexpected Solidity error selector: {:?}",
error.selector()
))
.into())
}
}
thedevbirb marked this conversation as resolved.
Show resolved Hide resolved
}

/// Utility functions related to the BoltValidators contract
pub struct BoltValidators;

impl BoltValidators {
/// Hash the public keys of the proposers. This follows the same
/// implementation done on-chain in the BoltValidators contract.
pub fn pubkey_hashes(keys: &[BlsPublicKey]) -> Vec<B256> {
keys.iter().map(Self::pubkey_hash).collect()
}

/// Hash the public key of the proposer. This follows the same
/// implementation done on-chain in the BoltValidators contract.
pub fn pubkey_hash(key: &BlsPublicKey) -> B256 {
let digest = Self::pubkey_hash_digest(key);
keccak256(digest)
}

fn pubkey_hash_digest(key: &BlsPublicKey) -> B512 {
let mut onchain_pubkey_repr = B512::ZERO;

// copy the pubkey bytes into the rightmost 48 bytes of the 512-bit buffer.
// the result should look like this:
//
// 0x00000000000000000000000000000000b427fd179b35ef085409e4a98fb3ab84ee29c689df5c64020eab0b20a4f91170f610177db172dc091682df627c9f4021
// |<---------- 16 bytes ---------->||<----------------------------------------- 48 bytes ----------------------------------------->|
onchain_pubkey_repr[16..].copy_from_slice(key);
onchain_pubkey_repr
}
}

sol! {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the future we should probably do something to keep these interfaces up to date, @coderabbitai please draft an issue

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mempirate, I've created an issue to track keeping the interfaces up to date: Issue #341.

🔗 Analysis chain

@mempirate, I've drafted an issue to keep these interfaces up to date.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Create a GitHub issue to track keeping interfaces up to date
gh issue create \
  --title "Maintain Up-to-Date Interfaces in Codebase" \
  --body "As discussed in [this comment](https://github.com/chainbound/bolt/pull/340#discussion_r1821409808) by @mempirate on [PR #340](https://github.com/chainbound/bolt/pull/340), we should implement a process to ensure that interfaces remain up to date." \
  --assignee mempirate

Length of output: 393


✏️ Learnings added
Learnt from: mempirate
PR: chainbound/bolt#340
File: bolt-sidecar/src/onchain_registry.rs:143-143
Timestamp: 2024-10-29T19:21:54.868Z
Learning: Ensure that interfaces in the codebase are kept up to date by implementing a process to maintain them.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beast

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mempirate, happy to help!

(_/)
(^.^)
(")(")

#[sol(rpc)]
interface BoltManagerContract {
#[derive(Debug, Default, Serialize)]
struct ProposerStatus {
bytes32 pubkeyHash;
bool active;
address operator;
string operatorRPC;
address[] collaterals;
uint256[] amounts;
}

function getProposerStatuses(bytes32[] calldata pubkeyHashes) public view returns (ProposerStatus[] memory statuses);

function isOperator(address operator) external view returns (bool isOperator);

error InvalidQuery();
error ValidatorDoesNotExist();
}
}
6 changes: 5 additions & 1 deletion bolt-sidecar/src/signer/commit_boost.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use std::{str::FromStr, sync::Arc};

use alloy::{rpc::types::beacon::BlsSignature, signers::Signature};
use alloy::{primitives::Address, rpc::types::beacon::BlsSignature, signers::Signature};
use cb_common::{
commit::{client::SignerClient, error::SignerClientError, request::SignConsensusRequest},
signer::EcdsaPublicKey,
Expand Down Expand Up @@ -142,6 +142,10 @@ impl CommitBoostSigner {

#[async_trait::async_trait]
impl SignerECDSA for CommitBoostSigner {
fn public_key(&self) -> Address {
Address::try_from(self.get_proxy_ecdsa_pubkey().as_ref()).expect("valid address")
}

async fn sign_hash(&self, hash: &[u8; 32]) -> eyre::Result<Signature> {
let request = SignProxyRequest::builder(
*self.proxy_ecdsa.read().first().expect("proxy ecdsa key loaded"),
Expand Down
7 changes: 4 additions & 3 deletions bolt-sidecar/src/signer/keystore.rs
Original file line number Diff line number Diff line change
Expand Up @@ -203,8 +203,8 @@ mod tests {
const KEYSTORES_DEFAULT_PATH_TEST: &str = "test_data/keys";
const KEYSTORES_SECRETS_DEFAULT_PATH_TEST: &str = "test_data/secrets";

/// If `path` is `Some`, returns a clone of it. Otherwise, returns the path to the `fallback_relative_path`
/// starting from the root of the cargo project.
/// If `path` is `Some`, returns a clone of it. Otherwise, returns the path to the
/// `fallback_relative_path` starting from the root of the cargo project.
fn make_path(relative_path: &str) -> PathBuf {
let project_root = env!("CARGO_MANIFEST_DIR");
Path::new(project_root).join(relative_path)
Expand Down Expand Up @@ -302,7 +302,8 @@ mod tests {
let keystore_path = PathBuf::from(keystore_path);

for test_keystore_json in tests_keystore_json {
// 1. Write the keystore in a `test-voting-keystore.json` file so we test both scrypt and PBDKF2
// 1. Write the keystore in a `test-voting-keystore.json` file so we test both scrypt
// and PBDKF2

let mut tmp_keystore_file =
File::create(keystore_path.join("test-voting-keystore.json"))
Expand Down
17 changes: 9 additions & 8 deletions bolt-sidecar/src/state/consensus.rs
Original file line number Diff line number Diff line change
Expand Up @@ -117,8 +117,8 @@ impl ConsensusState {
}

// If the request is for the next slot, check if it's within the commitment deadline
if req.slot == self.latest_slot + 1
&& self.latest_slot_timestamp + self.commitment_deadline_duration < Instant::now()
if req.slot == self.latest_slot + 1 &&
self.latest_slot_timestamp + self.commitment_deadline_duration < Instant::now()
{
return Err(ConsensusError::DeadlineExceeded);
}
Expand Down Expand Up @@ -161,7 +161,8 @@ impl ConsensusState {
Ok(())
}

/// Fetch proposer duties for the given epoch and the next one if the unsafe lookahead flag is set
/// Fetch proposer duties for the given epoch and the next one if the unsafe lookahead flag is
/// set
async fn fetch_proposer_duties(&mut self, epoch: u64) -> Result<(), ConsensusError> {
let duties = if self.unsafe_lookahead_enabled {
let two_epoch_duties = join!(
Expand Down Expand Up @@ -200,9 +201,9 @@ impl ConsensusState {
/// Returns the furthest slot for which a commitment request is considered valid, whether in
/// the current epoch or next epoch (if unsafe lookahead is enabled)
fn furthest_slot(&self) -> u64 {
self.epoch.start_slot
+ SLOTS_PER_EPOCH
+ if self.unsafe_lookahead_enabled { SLOTS_PER_EPOCH } else { 0 }
self.epoch.start_slot +
SLOTS_PER_EPOCH +
if self.unsafe_lookahead_enabled { SLOTS_PER_EPOCH } else { 0 }
}
}

Expand Down Expand Up @@ -324,8 +325,8 @@ mod tests {
};

let epoch =
state.beacon_api_client.get_beacon_header(BlockId::Head).await?.header.message.slot
/ SLOTS_PER_EPOCH;
state.beacon_api_client.get_beacon_header(BlockId::Head).await?.header.message.slot /
SLOTS_PER_EPOCH;

state.fetch_proposer_duties(epoch).await?;
assert_eq!(state.epoch.proposer_duties.len(), SLOTS_PER_EPOCH as usize * 2);
Expand Down
Loading