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

[Merged by Bors] - Remove builder redundancy #3294

Closed
Closed
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
35 changes: 35 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ members = [

"beacon_node",
"beacon_node/beacon_chain",
"beacon_node/builder_client",
"beacon_node/client",
"beacon_node/eth1",
"beacon_node/lighthouse_network",
Expand Down
12 changes: 10 additions & 2 deletions beacon_node/beacon_chain/src/beacon_chain.rs
Original file line number Diff line number Diff line change
Expand Up @@ -334,7 +334,7 @@ pub struct BeaconChain<T: BeaconChainTypes> {
/// Provides information from the Ethereum 1 (PoW) chain.
pub eth1_chain: Option<Eth1Chain<T::Eth1Chain, T::EthSpec>>,
/// Interfaces with the execution client.
pub execution_layer: Option<ExecutionLayer>,
pub execution_layer: Option<ExecutionLayer<T::EthSpec>>,
/// Stores a "snapshot" of the chain at the time the head-of-the-chain block was received.
pub(crate) canonical_head: TimeoutRwLock<BeaconSnapshot<T::EthSpec>>,
/// The root of the genesis block.
Expand Down Expand Up @@ -3218,6 +3218,14 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
let slot = state.slot();
let proposer_index = state.get_beacon_proposer_index(state.slot(), &self.spec)? as u64;

let pubkey_opt = match self.validator_pubkey_bytes(proposer_index as usize) {
Copy link
Member

Choose a reason for hiding this comment

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

Since you have a BeaconState in scope here, I'd suggest doing state.validators().get(proposer_index as usize).map(|v| v.pubkey) rather than hitting the pubkey cache.

There can be some lock contention on the validator pubkey cache at times. There's also something very definite about saying "this validator doesn't exist in the state of the block they're proposing". You could even return an error in that case, I don't see how any block could be valid if the proposer index isn't in the validator registry. I would only do that if there's a benefit in dropping the Option around the pubkey, otherwise we might as well just leave it a warning and have one less way that block proposal can fail.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok used this state to get the pubkey here: ac15207

I've left the Option because dropping it doesn't gain us anything, but that's a good point, if the validator isn't in the state I too have no idea how it could propose. Seemed more like a possibility when I was using the cache 😂

Ok(p) => p,
Err(e) => {
warn!(self.log, "Can't access proposer's pubkey, cannot use external builder"; "error" => ?e);
None
}
};

// Closure to fetch a sync aggregate in cases where it is required.
let get_sync_aggregate = || -> Result<SyncAggregate<_>, BlockProductionError> {
Ok(self
Expand Down Expand Up @@ -3276,7 +3284,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
BeaconState::Merge(_) => {
let sync_aggregate = get_sync_aggregate()?;
let execution_payload =
get_execution_payload::<T, Payload>(self, &state, proposer_index)?;
get_execution_payload::<T, Payload>(self, &state, proposer_index, pubkey_opt)?;
BeaconBlock::Merge(BeaconBlockMerge {
slot,
proposer_index,
Expand Down
4 changes: 2 additions & 2 deletions beacon_node/beacon_chain/src/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ pub struct BeaconChainBuilder<T: BeaconChainTypes> {
>,
op_pool: Option<OperationPool<T::EthSpec>>,
eth1_chain: Option<Eth1Chain<T::Eth1Chain, T::EthSpec>>,
execution_layer: Option<ExecutionLayer>,
execution_layer: Option<ExecutionLayer<T::EthSpec>>,
event_handler: Option<ServerSentEventHandler<T::EthSpec>>,
slot_clock: Option<T::SlotClock>,
shutdown_sender: Option<Sender<ShutdownReason>>,
Expand Down Expand Up @@ -481,7 +481,7 @@ where
}

/// Sets the `BeaconChain` execution layer.
pub fn execution_layer(mut self, execution_layer: Option<ExecutionLayer>) -> Self {
pub fn execution_layer(mut self, execution_layer: Option<ExecutionLayer<TEthSpec>>) -> Self {
self.execution_layer = execution_layer;
self
}
Expand Down
11 changes: 8 additions & 3 deletions beacon_node/beacon_chain/src/execution_payload.rs
Original file line number Diff line number Diff line change
Expand Up @@ -247,9 +247,10 @@ pub fn get_execution_payload<T: BeaconChainTypes, Payload: ExecPayload<T::EthSpe
chain: &BeaconChain<T>,
state: &BeaconState<T::EthSpec>,
proposer_index: u64,
pubkey: Option<PublicKeyBytes>,
) -> Result<Payload, BlockProductionError> {
Ok(
prepare_execution_payload_blocking::<T, Payload>(chain, state, proposer_index)?
prepare_execution_payload_blocking::<T, Payload>(chain, state, proposer_index, pubkey)?
.unwrap_or_default(),
)
}
Expand All @@ -259,6 +260,7 @@ pub fn prepare_execution_payload_blocking<T: BeaconChainTypes, Payload: ExecPayl
chain: &BeaconChain<T>,
state: &BeaconState<T::EthSpec>,
proposer_index: u64,
pubkey: Option<PublicKeyBytes>,
) -> Result<Option<Payload>, BlockProductionError> {
let execution_layer = chain
.execution_layer
Expand All @@ -267,7 +269,7 @@ pub fn prepare_execution_payload_blocking<T: BeaconChainTypes, Payload: ExecPayl

execution_layer
.block_on_generic(|_| async {
prepare_execution_payload::<T, Payload>(chain, state, proposer_index).await
prepare_execution_payload::<T, Payload>(chain, state, proposer_index, pubkey).await
})
.map_err(BlockProductionError::BlockingFailed)?
}
Expand All @@ -290,6 +292,7 @@ pub async fn prepare_execution_payload<T: BeaconChainTypes, Payload: ExecPayload
chain: &BeaconChain<T>,
state: &BeaconState<T::EthSpec>,
proposer_index: u64,
pubkey: Option<PublicKeyBytes>,
) -> Result<Option<Payload>, BlockProductionError> {
let spec = &chain.spec;
let execution_layer = chain
Expand Down Expand Up @@ -345,12 +348,14 @@ pub async fn prepare_execution_payload<T: BeaconChainTypes, Payload: ExecPayload

// Note: the suggested_fee_recipient is stored in the `execution_layer`, it will add this parameter.
let execution_payload = execution_layer
.get_payload::<T::EthSpec, Payload>(
.get_payload::<Payload>(
parent_hash,
timestamp,
random,
finalized_block_hash.unwrap_or_else(ExecutionBlockHash::zero),
proposer_index,
pubkey,
state.slot(),
)
.await
.map_err(BlockProductionError::GetPayloadFailed)?;
Expand Down
3 changes: 2 additions & 1 deletion beacon_node/beacon_chain/src/test_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ pub struct Builder<T: BeaconChainTypes> {
store: Option<Arc<HotColdDB<T::EthSpec, T::HotStore, T::ColdStore>>>,
initial_mutator: Option<BoxedMutator<T::EthSpec, T::HotStore, T::ColdStore>>,
store_mutator: Option<BoxedMutator<T::EthSpec, T::HotStore, T::ColdStore>>,
execution_layer: Option<ExecutionLayer>,
execution_layer: Option<ExecutionLayer<T::EthSpec>>,
mock_execution_layer: Option<MockExecutionLayer<T::EthSpec>>,
runtime: TestRuntime,
log: Logger,
Expand Down Expand Up @@ -361,6 +361,7 @@ where
DEFAULT_TERMINAL_BLOCK,
spec.terminal_block_hash,
spec.terminal_block_hash_activation_epoch,
None,
);
self.execution_layer = Some(mock.el.clone());
self.mock_execution_layer = Some(mock);
Expand Down
2 changes: 1 addition & 1 deletion beacon_node/beacon_chain/tests/payload_invalidation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ impl InvalidPayloadRig {
self
}

fn execution_layer(&self) -> ExecutionLayer {
fn execution_layer(&self) -> ExecutionLayer<E> {
self.harness.chain.execution_layer.clone().unwrap()
}

Expand Down
12 changes: 12 additions & 0 deletions beacon_node/builder_client/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
[package]
name = "builder_client"
version = "0.1.0"
edition = "2021"
authors = ["Sean Anderson <sean@sigmaprime.io>"]

[dependencies]
reqwest = { version = "0.11.0", features = ["json","stream"] }
sensitive_url = { path = "../../common/sensitive_url" }
eth2 = { path = "../../common/eth2" }
serde = { version = "1.0.116", features = ["derive"] }
serde_json = "1.0.58"
Loading