Skip to content

Commit

Permalink
Fix 2 bugs in Electra deposit processing
Browse files Browse the repository at this point in the history 
All those fields recently added to CombinedDeposit still weren't enough.

There seems to have been a similar unobserved bug involving PendingDeposit.withdrawal_credentials.
A top-up deposit may contain withdrawal credentials different from the original ones.

apply_deposits in Electra now works around some of the logic in validate_deposits.
validate_deposits does 2 things:
- It verifies signatures in an optimized way.
- It combines deposits with the same pubkey to minimize BeaconState updates.
The former is still useful in Electra, but the latter is undesirable.
It may be a good idea to refactor validate_deposits to only do the former.
  • Loading branch information
@weekday-grandine-io @povi
weekday-grandine-io authored and povi committed Nov 6, 2024
1 parent ca1f0c0 commit bd507cb
Show file tree
Hide file tree
Showing 5 changed files with 102 additions and 54 deletions.
5 changes: 4 additions & 1 deletion transition_functions/src/altair/block_processing.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -352,6 +352,7 @@ pub fn process_deposit_data<P: Preset>(
withdrawal_credentials: vec![withdrawal_credentials],
amounts: smallvec![amount],
signatures: vec![signature],
positions: smallvec![0],
};

apply_deposits(state, 1, core::iter::once(combined_deposit), NullSlotReport)?;
Expand All @@ -371,9 +372,10 @@ pub fn process_deposit_data<P: Preset>(

let combined_deposit = CombinedDeposit::NewValidator {
pubkey,
withdrawal_credentials,
withdrawal_credentials: vec![withdrawal_credentials],
amounts: smallvec![amount],
signatures: vec![signature],
positions: smallvec![0],
};

apply_deposits(state, 1, core::iter::once(combined_deposit), NullSlotReport)?;
Expand Down Expand Up @@ -405,6 +407,7 @@ pub fn apply_deposits<P: Preset>(
..
} => {
let public_key_bytes = pubkey.to_bytes();
let withdrawal_credentials = withdrawal_credentials[0];
let first_amount = amounts[0];
let total_amount = amounts.iter().sum();

Expand Down
63 changes: 41 additions & 22 deletions transition_functions/src/electra/block_processing.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -780,6 +780,7 @@ pub fn process_deposit_data<P: Preset>(
withdrawal_credentials: vec![withdrawal_credentials],
amounts: smallvec![amount],
signatures: vec![signature],
positions: smallvec![0],
};

apply_deposits(state, core::iter::once(combined_deposit), NullSlotReport)?;
Expand All @@ -799,9 +800,10 @@ pub fn process_deposit_data<P: Preset>(

let combined_deposit = CombinedDeposit::NewValidator {
pubkey,
withdrawal_credentials,
withdrawal_credentials: vec![withdrawal_credentials],
amounts: smallvec![amount],
signatures: vec![signature],
positions: smallvec![0],
};

apply_deposits(state, core::iter::once(combined_deposit), NullSlotReport)?;
Expand Down Expand Up @@ -862,6 +864,8 @@ fn apply_deposits<P: Preset>(
combined_deposits: impl IntoIterator<Item = CombinedDeposit>,
mut slot_report: impl SlotReport,
) -> Result<()> {
let mut pending_deposits_with_positions = vec![];

for combined_deposit in combined_deposits {
match combined_deposit {
// > Add validator and balance entries
Expand All @@ -870,20 +874,27 @@ fn apply_deposits<P: Preset>(
withdrawal_credentials,
amounts,
signatures,
positions,
} => {
let public_key_bytes = pubkey.to_bytes();
let first_withdrawal_credentials = withdrawal_credentials[0];
let validator_index = state.validators().len_u64();
let public_key_bytes = pubkey.to_bytes();

add_validator_to_registry(state, pubkey, withdrawal_credentials, 0)?;
add_validator_to_registry(state, pubkey, first_withdrawal_credentials, 0)?;

for (amount, signature) in amounts.into_iter().zip(signatures) {
state.pending_deposits_mut().push(PendingDeposit {
pubkey: public_key_bytes,
withdrawal_credentials,
amount,
signature,
slot: GENESIS_SLOT,
})?;
for (withdrawal_credentials, amount, signature, position) in
izip!(withdrawal_credentials, amounts, signatures, positions)
{
pending_deposits_with_positions.push((
PendingDeposit {
pubkey: public_key_bytes,
withdrawal_credentials,
amount,
signature,
slot: GENESIS_SLOT,
},
position,
));

// TODO(feature/electra):
slot_report.add_deposit(validator_index, amount);
Expand All @@ -895,28 +906,36 @@ fn apply_deposits<P: Preset>(
withdrawal_credentials,
amounts,
signatures,
positions,
} => {
let pubkey = accessors::public_key(state, validator_index)?.to_bytes();

for ((amount, signature), withdrawal_credentials) in amounts
.into_iter()
.zip(signatures)
.zip(withdrawal_credentials)
for (withdrawal_credentials, amount, signature, position) in
izip!(withdrawal_credentials, amounts, signatures, positions)
{
state.pending_deposits_mut().push(PendingDeposit {
pubkey,
withdrawal_credentials,
amount,
signature,
slot: GENESIS_SLOT,
})?;
pending_deposits_with_positions.push((
PendingDeposit {
pubkey,
withdrawal_credentials,
amount,
signature,
slot: GENESIS_SLOT,
},
position,
));

slot_report.add_deposit(validator_index, amount);
}
}
}
}

pending_deposits_with_positions.sort_unstable_by_key(|(_, position)| *position);

for (pending_deposit, _) in pending_deposits_with_positions {
state.pending_deposits_mut().push(pending_deposit)?;
}

Ok(())
}

Expand Down
5 changes: 4 additions & 1 deletion transition_functions/src/phase0/block_processing.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,7 @@ pub fn process_deposit_data<P: Preset>(
withdrawal_credentials: vec![withdrawal_credentials],
amounts: smallvec![amount],
signatures: vec![signature],
positions: smallvec![0],
};

apply_deposits(state, 1, core::iter::once(combined_deposit), NullSlotReport)?;
Expand All @@ -314,9 +315,10 @@ pub fn process_deposit_data<P: Preset>(

let combined_deposit = CombinedDeposit::NewValidator {
pubkey,
withdrawal_credentials,
withdrawal_credentials: vec![withdrawal_credentials],
amounts: smallvec![amount],
signatures: vec![signature],
positions: smallvec![0],
};

apply_deposits(state, 1, core::iter::once(combined_deposit), NullSlotReport)?;
Expand Down Expand Up @@ -348,6 +350,7 @@ fn apply_deposits<P: Preset>(
..
} => {
let public_key_bytes = pubkey.to_bytes();
let withdrawal_credentials = withdrawal_credentials[0];
let first_amount = amounts[0];
let total_amount = amounts.iter().sum();

Expand Down
81 changes: 52 additions & 29 deletions transition_functions/src/unphased/block_processing.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ use ssz::SszHash as _;
use typenum::Unsigned as _;
use types::{
config::Config,
nonstandard::{AttestationEpoch, GweiVec},
nonstandard::{AttestationEpoch, GweiVec, U64Vec},
phase0::{
consts::FAR_FUTURE_EPOCH,
containers::{
Expand All @@ -43,18 +43,32 @@ use crate::unphased::Error;
pub enum CombinedDeposit {
NewValidator {
pubkey: CachedPublicKey,
withdrawal_credentials: H256,
withdrawal_credentials: Vec<H256>,
amounts: GweiVec,
signatures: Vec<SignatureBytes>,
positions: U64Vec,
},
TopUp {
validator_index: ValidatorIndex,
withdrawal_credentials: Vec<H256>,
amounts: GweiVec,
signatures: Vec<SignatureBytes>,
positions: U64Vec,
},
}

impl CombinedDeposit {
fn positions(&self) -> &[u64] {
match self {
Self::NewValidator { positions, .. } | Self::TopUp { positions, .. } => positions,
}
}

fn first_position(&self) -> Option<u64> {
self.positions().first().copied()
}
}

pub fn process_block_header_for_gossip<P: Preset>(
state: &impl BeaconState<P>,
block: &impl BeaconBlock<P>,
Expand Down Expand Up @@ -371,11 +385,12 @@ pub fn validate_attestation_with_verifier<P: Preset>(
validate_constructed_indexed_attestation(config, state, &indexed_attestation, verifier)
}

#[allow(clippy::too_many_lines)]
pub fn validate_deposits<P: Preset>(
config: &Config,
state: &impl BeaconState<P>,
deposits: impl IntoIterator<Item = Deposit>,
) -> Result<impl Iterator<Item = CombinedDeposit>> {
) -> Result<Vec<CombinedDeposit>> {
let deposits_by_pubkey = (0..)
.zip(deposits)
.into_group_map_by(|(_, deposit)| deposit.data.pubkey)
Expand Down Expand Up @@ -430,27 +445,26 @@ pub fn validate_deposits<P: Preset>(
)?;
}

let (first_position, _) = deposits[0];

if let Some(validator_index) = existing_validator_index {
let ((amounts, withdrawal_credentials), signatures) = deposits
let (amounts, withdrawal_credentials, signatures, positions) = deposits
.into_iter()
.map(|(_, deposit)| {
.map(|(position, deposit)| {
(
(deposit.data.amount, deposit.data.withdrawal_credentials),
deposit.data.amount,
deposit.data.withdrawal_credentials,
deposit.data.signature,
position,
)
})
.unzip();
.multiunzip();

let combined_deposit = CombinedDeposit::TopUp {
return Ok(Some(CombinedDeposit::TopUp {
validator_index,
withdrawal_credentials,
amounts,
signatures,
};

return Ok(Some((first_position, combined_deposit)));
positions,
}));
}

let mut deposits = deposits.into_iter();
Expand All @@ -470,38 +484,47 @@ pub fn validate_deposits<P: Preset>(
})
};

Ok(first_valid.map(|(position, deposit)| {
Ok(first_valid.map(|(first_position, deposit)| {
let DepositData {
pubkey,
withdrawal_credentials,
withdrawal_credentials: first_withdrawal_credentials,
amount: first_amount,
signature: first_signature,
} = deposit.data;

let (amounts, signatures) = core::iter::once((first_amount, first_signature))
.chain(
deposits.map(|(_, deposit)| (deposit.data.amount, deposit.data.signature)),
)
.collect();

let combined_deposit = CombinedDeposit::NewValidator {
let (withdrawal_credentials, amounts, signatures, positions) = core::iter::once((
first_withdrawal_credentials,
first_amount,
first_signature,
first_position,
))
.chain(deposits.map(|(position, deposit)| {
let DepositData {
pubkey: _,
withdrawal_credentials,
amount,
signature,
} = deposit.data;

(withdrawal_credentials, amount, signature, position)
}))
.multiunzip();

CombinedDeposit::NewValidator {
pubkey: pubkey.into(),
withdrawal_credentials,
amounts,
signatures,
};

(position, combined_deposit)
positions,
}
}))
})
.filter_map(Result::transpose)
.collect::<Result<Vec<_>>>()?;

combined_deposits.sort_unstable_by_key(|(position, _)| *position);
combined_deposits.sort_unstable_by_key(CombinedDeposit::first_position);

Ok(combined_deposits
.into_iter()
.map(|(_, combined_deposit)| combined_deposit))
Ok(combined_deposits)
}

pub fn verify_deposit_merkle_branch<P: Preset>(
Expand Down
2 changes: 1 addition & 1 deletion types/src/nonstandard.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ pub type UsizeVec = SmallVec<[usize; 2]>;

assert_eq_size!(UsizeVec, Vec<usize>);

type U64Vec = SmallVec<[u64; 2 * size_of::<usize>() / size_of::<u64>()]>;
pub type U64Vec = SmallVec<[u64; 2 * size_of::<usize>() / size_of::<u64>()]>;

assert_eq_size!(U64Vec, Vec<u64>);

Expand Down

0 comments on commit bd507cb

Please sign in to comment.