feat: ethereum elections #6122
Open
feat: ethereum elections #6122
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
TODO: sort the content of the vote before voting (like we do for btc) |
MxmUrw
reviewed
Sep 22, 2025
engine/src/witness/eth2.rs
Outdated
| EngineElectionType::ByHash(hash) => { | ||
| let block = client.block_by_hash(hash).await?; | ||
| if let (Some(block_number), Some(block_hash)) = (block.number, block.hash) { | ||
| assert_eq!(block_number.as_u64(), block_height); |
There was a problem hiding this comment.
Are we sure we want to assert here?
engine/src/witness/start.rs
Outdated
Comment on lines
112
to
120
| // let start_eth = super::eth::start( | ||
| // scope, | ||
| // eth_client, | ||
| // witness_call.clone(), | ||
| // state_chain_client.clone(), | ||
| // state_chain_stream.clone(), | ||
| // epoch_source.clone(), | ||
| // db.clone(), | ||
| // ); |
Comment on lines
1061
to
1077
| pub fn inner_redeemed( | ||
| account_id: AccountId<T>, | ||
| redeemed_amount: FlipBalance<T>, | ||
| ) -> DispatchResult { | ||
| let _ = | ||
| PendingRedemptions::<T>::take(&account_id).ok_or(Error::<T>::NoPendingRedemption)?; | ||
|
|
||
| T::Flip::finalize_redemption(&account_id) | ||
| .expect("This should never return an error because we already ensured above that the pending redemption does indeed exist"); | ||
|
|
||
| Self::kill_account_if_zero_balance(&account_id); | ||
|
|
||
| Self::deposit_event(Event::RedemptionSettled(account_id, redeemed_amount)); | ||
| Ok(()) | ||
| } | ||
|
|
||
| pub fn inner_redemption_expired(account_id: AccountId<T>) -> DispatchResult { |
There was a problem hiding this comment.
I think we can remove the inner_ prefix of these two, since there are no "outer" functions that these are the "inner" functions to.
Comment on lines
1177
to
1193
| #[derive( | ||
| Clone, | ||
| PartialEq, | ||
| Eq, | ||
| Encode, | ||
| Decode, | ||
| TypeInfo, | ||
| DebugNoBound, | ||
| Serialize, | ||
| Deserialize, | ||
| PartialOrd, | ||
| Ord, | ||
| )] | ||
| pub struct EthereumDepositAndSCCall { | ||
| pub deposit: EthereumDeposit, | ||
| pub call: Vec<u8>, | ||
| } |
There was a problem hiding this comment.
What do you think about the following:
Suggested change
| #[derive( | |
| Clone, | |
| PartialEq, | |
| Eq, | |
| Encode, | |
| Decode, | |
| TypeInfo, | |
| DebugNoBound, | |
| Serialize, | |
| Deserialize, | |
| PartialOrd, | |
| Ord, | |
| )] | |
| pub struct EthereumDepositAndSCCall { | |
| pub deposit: EthereumDeposit, | |
| pub call: Vec<u8>, | |
| } | |
| derive_common_traits! { | |
| #[derive(PartialOrd, Ord, TypeInfo)] | |
| pub struct EthereumDepositAndSCCall { | |
| pub deposit: EthereumDeposit, | |
| pub call: Vec<u8>, | |
| } | |
| } |
Comment on lines
1195
to
1208
| #[derive( | ||
| Clone, | ||
| PartialEq, | ||
| Eq, | ||
| Encode, | ||
| Decode, | ||
| TypeInfo, | ||
| DebugNoBound, | ||
| Serialize, | ||
| Deserialize, | ||
| PartialOrd, | ||
| Ord, | ||
| )] | ||
| pub enum EthereumDeposit { |
Comment on lines
118
to
155
| #[async_trait::async_trait] | ||
| impl VoterApi<EthereumBlockHeightWitnesserES> for EthereumBlockHeightWitnesserVoter { | ||
| async fn vote( | ||
| &self, | ||
| _settings: <EthereumBlockHeightWitnesserES as ElectoralSystemTypes>::ElectoralSettings, | ||
| properties: <EthereumBlockHeightWitnesserES as ElectoralSystemTypes>::ElectionProperties, | ||
| ) -> std::result::Result<Option<VoteOf<EthereumBlockHeightWitnesserES>>, anyhow::Error> { | ||
| tracing::debug!("ETH BHW: Block height tracking called properties: {:?}", properties); | ||
| let HeightWitnesserProperties { witness_from_index } = properties; | ||
|
|
||
| let header_from_eth_block = |header: Block<H256>| -> anyhow::Result<Header<EthereumChain>> { | ||
| Ok(Header { | ||
| block_height: header | ||
| .number | ||
| .ok_or_else(|| anyhow::anyhow!("No block number"))? | ||
| .low_u64(), | ||
| hash: header.hash.ok_or_else(|| anyhow::anyhow!("No block hash"))?, | ||
| parent_hash: header.parent_hash, | ||
| }) | ||
| }; | ||
|
|
||
| let best_block_number = self.client.get_block_number().await?; | ||
| let best_block = self.client.block(best_block_number).await?; | ||
|
|
||
| let best_block_header = header_from_eth_block(best_block)?; | ||
|
|
||
| if best_block_header.block_height < witness_from_index { | ||
| tracing::debug!("ETH BHW: no new blocks found since best block height is {} for witness_from={witness_from_index}", best_block_header.block_height); | ||
| return Ok(None) | ||
| } else { | ||
| // The `latest_block_height == 0` is a special case for when starting up the | ||
| // electoral system for the first time. | ||
| let witness_from_index = if witness_from_index == 0 { | ||
| tracing::debug!( | ||
| "ETH BHW: election_property=0, best_block_height={}, submitting last 6 blocks.", | ||
| best_block_header.block_height | ||
| ); | ||
| best_block_header | ||
| .block_height | ||
| .saturating_sub(ETHEREUM_MAINNET_SAFETY_BUFFER as u64) | ||
| } else { | ||
| witness_from_index | ||
| }; | ||
|
|
||
| // Compute the highest block height we want to fetch a header for, | ||
| // since for performance reasons we're bounding the number of headers | ||
| // submitted in one vote. We're submitting at most SAFETY_BUFFER headers. | ||
| let highest_submitted_height = std::cmp::min( | ||
| best_block_header.block_height, | ||
| witness_from_index.saturating_forward(ETHEREUM_MAINNET_SAFETY_BUFFER as usize + 1), | ||
| ); | ||
|
|
||
| // request headers for at most SAFETY_BUFFER heights, in parallel | ||
| let requests = (witness_from_index..highest_submitted_height) | ||
| .map(|index| async move { | ||
| header_from_eth_block(self.client.block(index.into()).await?) | ||
| }) | ||
| .collect::<Vec<_>>(); | ||
| let mut headers: VecDeque<_> = | ||
| future::join_all(requests).await.into_iter().collect::<Result<_>>()?; | ||
|
|
||
| // If we submitted all headers up the highest, we also append the highest | ||
| if highest_submitted_height == best_block_header.block_height { | ||
| headers.push_back(best_block_header); | ||
| } | ||
|
|
||
| let headers_len = headers.len(); | ||
| NonemptyContinuousHeaders::try_new(headers) | ||
| .inspect(|_| tracing::debug!("ETH BHW: Submitting vote for (witness_from={witness_from_index})with {headers_len} headers",)) | ||
| .map(Some) | ||
| .map_err(|err| anyhow::format_err!("ETH BHW: {err:?}")) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm pretty sure we can generalize this voter implementation to be generic over chains. In particular since we're also going to move arbitrum over.
engine/src/witness/eth2.rs
Outdated
Comment on lines
246
to
251
| let previous_address_states = eth_rpc | ||
| .address_states(parent_hash, address_checker_address, addresses.clone()) | ||
| .await?; | ||
|
|
||
| let address_states = | ||
| eth_rpc.address_states(hash, address_checker_address, addresses.clone()).await?; |
There was a problem hiding this comment.
This could be done in parallel, I assume?
engine/src/witness/eth2.rs
Outdated
| self.address_checker_address, | ||
| data.3, | ||
| data.2, | ||
| eth_deposti_channels.clone(), |
There was a problem hiding this comment.
typo:
Suggested change
| eth_deposti_channels.clone(), | |
| eth_deposit_channels.clone(), |
engine/src/witness/eth2.rs
Outdated
| deposit_addresses.into_iter().fold( | ||
| (Vec::new(), HashMap::new()), | ||
| |(mut eth, mut erc20), deposit_channel| { | ||
| let address = deposit_channel.address; |
There was a problem hiding this comment.
nit: I think it's not worth creating a local variable here (I mean address).
Comment on lines
41
to
48
| // fn request_max_redemption<T: Config>(account_id: &T::AccountId) { | ||
| // assert_ok!(Call::<T>::redeem { | ||
| // amount: RedemptionAmount::Max, | ||
| // address: Default::default(), | ||
| // executor: Default::default(), | ||
| // } | ||
| // .dispatch_bypass_filter(RawOrigin::Signed(account_id.clone()).into())); | ||
| // } |
There was a problem hiding this comment.
Should also be removed probably.
marcellorigotti
force-pushed
the
feat/ethereum-elections-rebase
branch
from
697bd49 to
7ee1304
Compare
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
marcellorigotti
requested review from
a team,
albert-llimos,
dandanlen,
nmammeri and
ramizhasan111
as code owners
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull Request
Closes: PRO-1668
Checklist
Please conduct a thorough self-review before opening the PR.
Summary
Switching Ethereum to the election-based witnessing