-
Notifications
You must be signed in to change notification settings - Fork 870
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
feat: Eip 6110 #8204
feat: Eip 6110 #8204
Changes from 3 commits
ace5ef7
46aa6a2
d9e16ef
938ae94
0da8fef
2162971
70e1094
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,75 @@ | ||
//! EIP-6110 deposit requests parsing | ||
use alloy_consensus::Request; | ||
use alloy_eips::eip6110::{DepositRequest, MAINNET_DEPOSIT_CONTRACT_ADDRESS}; | ||
use alloy_sol_types::{sol, SolEvent}; | ||
use reth_interfaces::executor::BlockValidationError; | ||
use reth_primitives::Receipt; | ||
use revm_primitives::Log; | ||
|
||
pub(crate) fn parse_deposits_from_receipts( | ||
receipts: &[Receipt], | ||
) -> Result<Vec<Request>, BlockValidationError> { | ||
let res = receipts | ||
.iter() | ||
.flat_map(|receipt| receipt.logs.iter()) | ||
// No need to filter for topic because there's only one event and that's the Deposit event | ||
// in the deposit contract. | ||
.filter(|log| log.address == MAINNET_DEPOSIT_CONTRACT_ADDRESS) | ||
.map(|log| DepositEvent::decode_log(log, false)) | ||
.map(|res| { | ||
let deposit = parse_deposit_from_log(&res?); | ||
Ok(Request::DepositRequest(deposit)) | ||
}) | ||
.collect::<Result<Vec<_>, _>>() | ||
// todo: this is ugly, we should clean it up | ||
.map_err(|err: alloy_sol_types::Error| { | ||
BlockValidationError::DepositRequestDecode(err.to_string()) | ||
})?; | ||
Ok(res) | ||
} | ||
|
||
sol! { | ||
#[allow(missing_docs)] | ||
event DepositEvent( | ||
bytes pubkey, | ||
bytes withdrawal_credentials, | ||
bytes amount, | ||
bytes signature, | ||
bytes index | ||
); | ||
} | ||
|
||
fn parse_deposit_from_log(log: &Log<DepositEvent>) -> DepositRequest { | ||
Comment on lines
+33
to
+44
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe we should put the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. agreed, let's do it after |
||
// SAFETY: These `expect` https://github.com/ethereum/consensus-specs/blob/5f48840f4d768bf0e0a8156a3ed06ec333589007/solidity_deposit_contract/deposit_contract.sol#L107-L110 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. great There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: should be NOTE or something else, SAFETY should be reserved for unsafe {} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
// are safe because the `DepositEvent` is the only event in the deposit contract and the length | ||
// checks are done there. | ||
DepositRequest { | ||
pubkey: log | ||
.pubkey | ||
.as_ref() | ||
.try_into() | ||
.expect("pubkey length should be enforced in deposit contract"), | ||
withdrawal_credentials: log | ||
.withdrawal_credentials | ||
.as_ref() | ||
.try_into() | ||
.expect("pubkey length should be enforced in deposit contract"), | ||
amount: u64::from_le_bytes( | ||
log.amount | ||
.as_ref() | ||
.try_into() | ||
.expect("pubkey length should be enforced in deposit contract"), | ||
), | ||
signature: log | ||
.signature | ||
.as_ref() | ||
.try_into() | ||
.expect("pubkey length should be enforced in deposit contract"), | ||
index: u64::from_le_bytes( | ||
log.index | ||
.as_ref() | ||
.try_into() | ||
.expect("pubkey length should be enforced in deposit contract"), | ||
), | ||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -168,7 +168,7 @@ where | |||||
transaction_gas_limit: transaction.gas_limit(), | ||||||
block_available_gas, | ||||||
} | ||||||
.into()) | ||||||
.into()); | ||||||
} | ||||||
|
||||||
EvmConfig::fill_tx_env(evm.tx_mut(), transaction, *sender); | ||||||
|
@@ -209,13 +209,17 @@ where | |||||
gas: GotExpected { got: cumulative_gas_used, expected: block.gas_used }, | ||||||
gas_spent_by_tx: receipts.gas_spent_by_tx()?, | ||||||
} | ||||||
.into()) | ||||||
.into()); | ||||||
} | ||||||
|
||||||
// Collect all EIP-6110 deposits | ||||||
let deposits = crate::eip6110::parse_deposits_from_receipts(&receipts)?; | ||||||
gakonst marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
||||||
// Collect all EIP-7685 requests | ||||||
let withdrawal_requests = | ||||||
post_block_withdrawal_requests(&self.chain_spec, block.timestamp, &mut evm)?; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should accumulate, extending a vector, rather than two vecs and concatenating after. Can be done with &mut Vec as argument and transforming the iterator into a for loop There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The withdrawal function returns a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i see, what about moving the parse_deposits to after post_block and extending that list? |
||||||
let requests = withdrawal_requests; | ||||||
// TODO: Does order matter? | ||||||
let requests = [deposits, withdrawal_requests].concat(); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes and this is the correct order
gakonst marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
||||||
Ok(EthExecuteOutput { receipts, requests, gas_used: cumulative_gas_used }) | ||||||
} | ||||||
|
@@ -311,7 +315,7 @@ where | |||||
receipts.iter(), | ||||||
) { | ||||||
debug!(target: "evm", %error, ?receipts, "receipts verification failed"); | ||||||
return Err(error) | ||||||
return Err(error); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hehe
Suggested change
|
||||||
}; | ||||||
} | ||||||
|
||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -88,12 +88,21 @@ pub enum BlockValidationError { | |
/// The error message. | ||
message: String, | ||
}, | ||
/// EVM error during withdrawal requests contract call | ||
/// EVM error during withdrawal requests contract call [EIP-7002] | ||
/// | ||
/// [EIP-7002]: https://eips.ethereum.org/EIPS/eip-7002 | ||
#[error("failed to apply withdrawal requests contract call: {message}")] | ||
WithdrawalRequestsContractCall { | ||
/// The error message. | ||
message: String, | ||
}, | ||
/// Error when decoding deposit requests from receipts [EIP-6110] | ||
/// | ||
/// [EIP-6110]: https://eips.ethereum.org/EIPS/eip-6110 | ||
// TODO(gakonst): This is an RLP decoding error but we don't import alloy_rlp here, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why not? It should be in reth_primitives already |
||
// do we want to? | ||
#[error("could not decode deposit request: {0}")] | ||
DepositRequestDecode(String), | ||
} | ||
|
||
/// BlockExecutor Errors | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -158,11 +158,11 @@ impl BundleStateWithReceipts { | |
/// Transform block number to the index of block. | ||
fn block_number_to_index(&self, block_number: BlockNumber) -> Option<usize> { | ||
if self.first_block > block_number { | ||
return None | ||
return None; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. grrrr |
||
} | ||
let index = block_number - self.first_block; | ||
if index >= self.receipts.len() as u64 { | ||
return None | ||
return None; | ||
} | ||
Some(index as usize) | ||
} | ||
|
@@ -269,7 +269,7 @@ impl BundleStateWithReceipts { | |
/// If the target block number is not included in the state block range. | ||
pub fn split_at(self, at: BlockNumber) -> (Option<Self>, Self) { | ||
if at == self.first_block { | ||
return (None, self) | ||
return (None, self); | ||
} | ||
|
||
let (mut lower_state, mut higher_state) = (self.clone(), self); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make it pub