feat: Implemented the is_valid_block_hash, is_valid_versioned_hashes functions #63

Kayden-ML · 2025-02-02T15:30:34Z

Implemented the is_valid_block_hash, is_valid_versioned_hashes functions

https://ethereum.github.io/consensus-specs/specs/deneb/beacon-chain/#is_valid_block_hash

https://ethereum.github.io/consensus-specs/specs/deneb/beacon-chain/#is_valid_versioned_hashes

syjn99 · 2025-02-18T09:44:36Z

crates/common/consensus/Cargo.toml


 [dependencies]
 alloy-primitives.workspace = true
+alloy-rlp = { version = "0.3.8", default-features = false, features = ["derive"] }


Suggested change

alloy-rlp = { version = "0.3.8", default-features = false, features = ["derive"] }

alloy-rlp = { workspace = true }

Same for eth_trie

I would do alloy-rlp.workspace = true as it is more minimalist

syjn99

Mostly seems good!

syjn99 · 2025-02-18T10:01:07Z

crates/common/consensus/src/deneb/execution_payload.rs

+        self.parent_hash.length()
+            + EMPTY_UNCLE_ROOT_HASH.length()
+            + self.fee_recipient.length()
+            + self.state_root.length()
+            + transactions_root.length()
+            + self.receipts_root.length()
+            + self.logs_bloom.length()
+            + U256::ZERO.length()
+            + self.block_number.length()
+            + self.gas_limit.length()
+            + self.gas_used.length()
+            + self.timestamp.length()
+            + self.extra_data.to_vec().as_slice().length()
+            + self.prev_randao.length()
+            + B64::ZERO.length()
+            + self.base_fee_per_gas.length()
+            + withdrawals_root.length()
+            + self.blob_gas_used.length()
+            + self.excess_blob_gas.length()
+            + parent_beacon_block_root.length()


Suggested change

self.parent_hash.length()

+ EMPTY_UNCLE_ROOT_HASH.length()

+ self.fee_recipient.length()

+ self.state_root.length()

+ transactions_root.length()

+ self.receipts_root.length()

+ self.logs_bloom.length()

+ U256::ZERO.length()

+ self.block_number.length()

+ self.gas_limit.length()

+ self.gas_used.length()

+ self.timestamp.length()

+ self.extra_data.to_vec().as_slice().length()

+ self.prev_randao.length()

+ B64::ZERO.length()

+ self.base_fee_per_gas.length()

+ withdrawals_root.length()

+ self.blob_gas_used.length()

+ self.excess_blob_gas.length()

+ parent_beacon_block_root.length()

self.parent_hash.length()

+ EMPTY_UNCLE_ROOT_HASH.length() // ommers_hash

+ self.fee_recipient.length()

+ self.state_root.length()

+ transactions_root.length()

+ self.receipts_root.length()

+ self.logs_bloom.length()

+ U256::ZERO.length() // difficulty

+ self.block_number.length()

+ self.gas_limit.length()

+ self.gas_used.length()

+ self.timestamp.length()

+ self.extra_data.to_vec().as_slice().length()

+ self.prev_randao.length()

+ B64::ZERO.length() // nonce

+ self.base_fee_per_gas.length()

+ withdrawals_root.length()

+ self.blob_gas_used.length()

+ self.excess_blob_gas.length()

+ parent_beacon_block_root.length()

syjn99 · 2025-02-18T10:10:34Z

crates/common/consensus/src/execution_engine/mod.rs

+    pub fn is_valid_versioned_hashes(
+        self: ExecutionEngine,
+        new_payload_request: NewPayloadRequest,
+    ) -> anyhow::Result<bool> {


Would be nice if blob tx decoding fails, then return false.

Shouldn't the higher level code handle the error, doing this suggestion would hide errors no?

syjn99 · 2025-02-18T10:15:48Z

crates/common/consensus/src/execution_engine/mod.rs

+    /// Return ``True`` if and only if the version hashes computed by the blob transactions of
+    /// ``new_payload_request.execution_payload`` matches ``new_payload_request.versioned_hashes``.
+    pub fn is_valid_versioned_hashes(
+        self: ExecutionEngine,


Suggested change

self: ExecutionEngine,

&self,

syjn99 · 2025-02-18T10:18:46Z

crates/common/consensus/src/execution_engine/mod.rs

+}
+
+impl TransactionType {
+    pub fn find_transaction_type(transaction: &[u8]) -> anyhow::Result<TransactionType> {


How about using TryFrom trait? It would be like this... (Code is incomplete)

#[derive(Debug)] pub enum TransactionTypeError { InvalidType(u8), EmptyTransaction, } impl TryFrom<&[u8]> for TransactionType { type Error = TransactionTypeError; fn try_from(transaction: &[u8]) -> anyhow::Result<Self> { let first_byte = transaction.first() .ok_or(TransactionTypeError::EmptyTransaction)?; match first_byte { 3 => Ok(TransactionType::BlobTransaction), 2 => Ok(TransactionType::FeeMarketTransaction), 1 => Ok(TransactionType::AccessListTransaction), 0 => Ok(TransactionType::LegacyTransaction), &x => Err(TransactionTypeError::InvalidType(x)), } } }

0 => Ok(TransactionType::LegacyTransaction), &x => Err(TransactionTypeError::InvalidType(x)),

This is invalid, the behavior already existing in the PR is correct. Legacy transactions are handled differently

I do think using try_from is an improvement though 👍

Ah my bad. yes I just wanted to suggest a naive example of using try_from

KolbyML

Here is some additional feedback

KolbyML · 2025-02-18T16:22:16Z

crates/common/consensus/Cargo.toml


 [dependencies]
 alloy-primitives.workspace = true
+alloy-rlp = { version = "0.3.8", default-features = false, features = ["derive"] }


I would do alloy-rlp.workspace = true as it is more minimalist

KolbyML · 2025-02-18T16:25:42Z

crates/common/consensus/src/deneb/execution_payload.rs

+pub fn calculate_merkle_patricia_root<'a, T: Encodable + 'a>(
+    items: impl IntoIterator<Item = &'a T>,
+) -> anyhow::Result<B256> {
+    let memdb = Arc::new(MemoryDB::new(true));
+    let mut trie = EthTrie::new(memdb);
+
+    // Insert items into merkle patricia trie
+    for (index, tx) in items.into_iter().enumerate() {
+        let path = alloy_rlp::encode(index);
+        let encoded_tx = alloy_rlp::encode(tx);
+        trie.insert(&path, &encoded_tx)
+            .map_err(|err| anyhow!("Error inserting into merkle patricia trie: {err:?}"))?;
+    }
+
+    trie.root_hash()
+        .map_err(|err| anyhow!("Error calculating merkle patricia trie root: {err:?}"))
+}


Suggested change

pub fn calculate_merkle_patricia_root<'a, T: Encodable + 'a>(

items: impl IntoIterator<Item = &'a T>,

) -> anyhow::Result<B256> {

let memdb = Arc::new(MemoryDB::new(true));

let mut trie = EthTrie::new(memdb);

// Insert items into merkle patricia trie

for (index, tx) in items.into_iter().enumerate() {

let path = alloy_rlp::encode(index);

let encoded_tx = alloy_rlp::encode(tx);

trie.insert(&path, &encoded_tx)

.map_err(|err| anyhow!("Error inserting into merkle patricia trie: {err:?}"))?;

}

trie.root_hash()

.map_err(|err| anyhow!("Error calculating merkle patricia trie root: {err:?}"))

}

/// Calculate the Merkle Patricia Trie root hash from a list of items

/// `(rlp(index), encoded(item))` pairs.

pub fn calculate_transactions_root<T>(transactions: &[T]) -> B256

where

T: Encodable,

{

ordered_trie_root_with_encoder(transactions, |tx: &T, buf| tx.encode(buf))

}

/// Calculates the root hash of the withdrawals.

pub fn calculate_withdrawals_root(withdrawals: &[Withdrawal]) -> B256 {

ordered_trie_root(withdrawals)

}

lets use alloy_trie instead to avoid passing an error

KolbyML · 2025-02-18T16:27:36Z

crates/common/consensus/src/deneb/execution_payload.rs

+        let transactions_root = calculate_merkle_patricia_root(transactions.iter()).expect("");
+        let withdrawals_root = calculate_merkle_patricia_root(self.withdrawals.iter()).expect("");


Suggested change

let transactions_root = calculate_merkle_patricia_root(transactions.iter()).expect("");

let withdrawals_root = calculate_merkle_patricia_root(self.withdrawals.iter()).expect("");

let transactions_root = calculate_transactions_root(&transactions);

let withdrawals_root = calculate_withdrawals_root(&self.withdrawal);

KolbyML · 2025-02-18T16:29:41Z

Cargo.toml

 clap = "4"
 discv5 = { version = "0.9.0", features = ["libp2p"] }
 enr = "0.13.0"
+eth_trie = "0.5.0"


Suggested change

eth_trie = "0.5.0"

alloy-consensus = { version = "0.11.1", default-features = false }

KolbyML · 2025-02-18T16:30:00Z

crates/common/consensus/Cargo.toml

 blst.workspace = true
 kzg.workspace = true
 ethereum_hashing.workspace = true
+eth_trie = "0.5.0"


Suggested change

eth_trie = "0.5.0"

alloy-consensus.workspace = true

KolbyML · 2025-02-18T20:12:52Z

Cargo.toml

 clap = "4"
 discv5 = { version = "0.9.0", features = ["libp2p"] }
 enr = "0.13.0"
+alloy-consensus = { version = "0.11.1", default-features = false }


this package should be sorted in the list

KolbyML · 2025-02-18T20:13:26Z

crates/common/consensus/src/deneb/execution_payload.rs

+}
+/// Calculates the root hash of the withdrawals.


Suggested change

}

/// Calculates the root hash of the withdrawals.

}

/// Calculates the root hash of the withdrawals.

can you add a space here

KolbyML

going forward after you address a review, could you let us know by sending a message, and can you indicate which comments you dealt with, one way you could do this is 👍 the comment, or writing a comment if you disagree with the feedback

KolbyML · 2025-02-18T20:14:20Z

crates/common/consensus/Cargo.toml

 blst.workspace = true
 kzg.workspace = true
 ethereum_hashing.workspace = true
+alloy-consensus.workspace = true


please sort

KolbyML

PR looks good

Kayden-ML force-pushed the process_execution_payload branch from 3af4327 to 491f361 Compare February 16, 2025 07:04

feat: implementing execution engine

e02d9e3

Kayden-ML force-pushed the process_execution_payload branch from 491f361 to e02d9e3 Compare February 16, 2025 07:07

Kayden-ML marked this pull request as ready for review February 16, 2025 07:07

Kayden-ML changed the title ~~feat: implementing execution engine~~ feat: Implemented the is_valid_block_hash, is_valid_versioned_hashes functions Feb 16, 2025

Kayden-ML requested review from KolbyML, jihoonsong, syjn99, unnawut and varun-doshi February 16, 2025 07:11

syjn99 reviewed Feb 18, 2025

View reviewed changes

KolbyML reviewed Feb 18, 2025

View reviewed changes

KolbyML assigned Kayden-ML Feb 18, 2025

KolbyML added the Consensus label Feb 18, 2025

fix: fixed requested changes

bacc1ef

KolbyML reviewed Feb 18, 2025

View reviewed changes

fix: requested changes

dfc1ee5

KolbyML reviewed Feb 18, 2025

View reviewed changes

KolbyML approved these changes Feb 18, 2025

View reviewed changes

Kayden-ML added this pull request to the merge queue Feb 18, 2025

Merged via the queue into ReamLabs:master with commit e40343a Feb 18, 2025
5 checks passed

KolbyML added this to the v0.1.0 milestone Feb 23, 2025

	alloy-rlp = { version = "0.3.8", default-features = false, features = ["derive"] }
	alloy-rlp = { workspace = true }

		let transactions_root = calculate_merkle_patricia_root(transactions.iter()).expect("");
		let withdrawals_root = calculate_merkle_patricia_root(self.withdrawals.iter()).expect("");

	eth_trie = "0.5.0"
	alloy-consensus = { version = "0.11.1", default-features = false }

feat: Implemented the is_valid_block_hash, is_valid_versioned_hashes functions #63

feat: Implemented the is_valid_block_hash, is_valid_versioned_hashes functions #63

Uh oh!

Conversation

Kayden-ML commented Feb 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

syjn99 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KolbyML left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KolbyML Feb 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KolbyML left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KolbyML left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Kayden-ML commented Feb 2, 2025 •

edited

Loading

KolbyML Feb 18, 2025 •

edited

Loading