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

use alloy transaction types #9484

Closed
1 task
Tracked by #8721 ...
mattsse opened this issue Jul 13, 2024 · 18 comments · Fixed by #10667
Closed
1 task
Tracked by #8721 ...

use alloy transaction types #9484

mattsse opened this issue Jul 13, 2024 · 18 comments · Fixed by #10667
Labels
C-debt Refactor of code section that is hard to understand or maintain D-good-first-issue Nice and easy! A great choice to get started M-prevent-stale Prevents old inactive issues/PRs from being closed due to inactivity

Comments

@mattsse
Copy link
Collaborator

mattsse commented Jul 13, 2024

the core tx types are quivalent to alloy's:

the only difference is the codec impl

we can implement codec by doing the same as for

/// Withdrawal acts as bridge which simplifies Compact implementation for AlloyWithdrawal.
///
/// Notice: Make sure this struct is 1:1 with `alloy_eips::eip4895::Withdrawal`
#[main_codec]
#[derive(Debug, Clone, PartialEq, Eq, Default)]
struct Withdrawal {

TODO

This should be done in separate prs and involves

there are likely missing/renamed functions in alloy, so we need to surface those and add to alloy if appropriate

@mattsse mattsse added D-good-first-issue Nice and easy! A great choice to get started C-debt Refactor of code section that is hard to understand or maintain labels Jul 13, 2024
@leruaa
Copy link
Contributor

leruaa commented Jul 13, 2024

I can take this on.

@mattsse
Copy link
Collaborator Author

mattsse commented Jul 13, 2024

cool, this will likely be a bit painful, see #9485 for ref
we likely need to do some work on rlp for this

@leruaa
Copy link
Contributor

leruaa commented Jul 14, 2024

@mattsse Looking into this, it seems I will have to convert from reth Signature to alloy one at many places. Do you think I should work on migrating reth Signature to alloy as a prerequisite to this issue, or converting is ok for now?

For instance in the code below, I would like to call encoded_len_with_signature() on alloy TxLegacy that takes an alloy signature as a parameter:

Self::Legacy { transaction, signature, .. } => {
// method computes the payload len with a RLP header
transaction.payload_len_with_signature(signature)
}

@emhane
Copy link
Member

emhane commented Aug 4, 2024

is the end goal to replace reth_primitives::PooledTransactionsElement with alloy_consensus::TypedTransaction ? if so we'd be more efficient if we tackle this by making types that use reth_primitives::PooledTransactionElement generic over a type T: alloy_consensus::Transaction. do we have a complete list of types we want to replace with its alloy equivalent?

@emhane
Copy link
Member

emhane commented Aug 4, 2024

T: alloy_consensus::Transaction + <the codec trait>, and impl the codec trait for alloy types where we define the codec trait

Copy link
Contributor

This issue is stale because it has been open for 21 days with no activity.

@github-actions github-actions bot added the S-stale This issue/PR is stale and will close with no further activity label Aug 26, 2024
@mattsse mattsse added M-prevent-stale Prevents old inactive issues/PRs from being closed due to inactivity and removed S-stale This issue/PR is stale and will close with no further activity labels Aug 26, 2024
@hannahredler
Copy link

I am applying to this issue via OnlyDust platform.

My background and how it can be leveraged

Hello! I am a software engineer with approx 8 years experience in Python, Java and Typescript. I have recently been learning rust and studying the core program with the Privacy and Scaling Explorations team to learn more about applied ZK.

Whilst new to rust, I have a lot of experience in writing maintainable, tested and clean code and know I can contribute positively to your code base!

How I plan on tackling this issue

Since it seems like a fairly straightforward migration, I would check the examples that have been given and ensure that I replicate the methodology.

@klkvr
Copy link
Collaborator

klkvr commented Sep 2, 2024

@tcoratger thank you for working on all PRs from here! would you like to work on TxDeposit migration to close this?

@tcoratger
Copy link
Contributor

@tcoratger thank you for working on all PRs from here! would you like to work on TxDeposit migration to close this?

Mhh not sure we have all the required methods in alloy TxDeposit no? like stuffs to do encoded_len_with_signature or payload_len_for_signature as SignableTransaction is not implemented for TxDeposit at the moment or did I miss something? (I'm not familiar with the op-alloy codebase so maybe I'm missing something here)

@klkvr
Copy link
Collaborator

klkvr commented Sep 2, 2024

@tcoratger thank you for working on all PRs from here! would you like to work on TxDeposit migration to close this?

Mhh not sure we have all the required methods in alloy TxDeposit no? like stuffs to do encoded_len_with_signature or payload_len_for_signature as SignableTransaction is not implemented for TxDeposit at the moment or did I miss something? (I'm not familiar with the op-alloy codebase so maybe I'm missing something here)

deposit transactions can't be signed, so we won't need those

@tcoratger
Copy link
Contributor

@tcoratger thank you for working on all PRs from here! would you like to work on TxDeposit migration to close this?

Mhh not sure we have all the required methods in alloy TxDeposit no? like stuffs to do encoded_len_with_signature or payload_len_for_signature as SignableTransaction is not implemented for TxDeposit at the moment or did I miss something? (I'm not familiar with the op-alloy codebase so maybe I'm missing something here)

deposit transactions can't be signed, so we won't need those

And we need to merge this right? alloy-rs/op-alloy#68

@klkvr
Copy link
Collaborator

klkvr commented Sep 2, 2024

@tcoratger thank you for working on all PRs from here! would you like to work on TxDeposit migration to close this?

Mhh not sure we have all the required methods in alloy TxDeposit no? like stuffs to do encoded_len_with_signature or payload_len_for_signature as SignableTransaction is not implemented for TxDeposit at the moment or did I miss something? (I'm not familiar with the op-alloy codebase so maybe I'm missing something here)

deposit transactions can't be signed, so we won't need those

And we need to merge this right? alloy-rs/op-alloy#68

yep

@tcoratger
Copy link
Contributor

@klkvr Also previously we had a method like:

pub(crate) fn payload_len_without_header(&self) -> usize {
    let payload_length = self.fields_len();
    // 'transaction type byte length' + 'header length' + 'payload length'
    1 + length_of_length(payload_length) + payload_length
}

which calculates basically 'transaction type byte length' + 'header length' + 'payload length' but in op alloy we only have:

fn length(&self) -> usize {
    let payload_length = self.fields_len();
    Header { list: true, payload_length }.length() + payload_length
}

but we lack the 'transaction type byte length' so this is also a method to add in op alloy right?

@klkvr
Copy link
Collaborator

klkvr commented Sep 2, 2024

yeah, I think we can add encoded_len(&self, with_header: bool) and encode(&self, &mut dyn BufMut, with_header: bool)

@hai-rise
Copy link
Contributor

hai-rise commented Sep 4, 2024

Thank you for the awesome work!

I wonder if we have the same plan for Header types? For instance, these two are very similar:

  • pub struct Header {
    /// The Keccak 256-bit hash of the parent
    /// block’s header, in its entirety; formally Hp.
    pub parent_hash: B256,
    /// The Keccak 256-bit hash of the ommers list portion of this block; formally Ho.
    pub ommers_hash: B256,
    /// The 160-bit address to which all fees collected from the successful mining of this block
    /// be transferred; formally Hc.
    pub beneficiary: Address,
    /// The Keccak 256-bit hash of the root node of the state trie, after all transactions are
    /// executed and finalisations applied; formally Hr.
    pub state_root: B256,
    /// The Keccak 256-bit hash of the root node of the trie structure populated with each
    /// transaction in the transactions list portion of the block; formally Ht.
    pub transactions_root: B256,
    /// The Keccak 256-bit hash of the root node of the trie structure populated with the receipts
    /// of each transaction in the transactions list portion of the block; formally He.
    pub receipts_root: B256,
    /// The Keccak 256-bit hash of the withdrawals list portion of this block.
    ///
    /// See [EIP-4895](https://eips.ethereum.org/EIPS/eip-4895).
    pub withdrawals_root: Option<B256>,
    /// The Bloom filter composed from indexable information (logger address and log topics)
    /// contained in each log entry from the receipt of each transaction in the transactions list;
    /// formally Hb.
    pub logs_bloom: Bloom,
    /// A scalar value corresponding to the difficulty level of this block. This can be calculated
    /// from the previous block’s difficulty level and the timestamp; formally Hd.
    pub difficulty: U256,
    /// A scalar value equal to the number of ancestor blocks. The genesis block has a number of
    /// zero; formally Hi.
    pub number: BlockNumber,
    /// A scalar value equal to the current limit of gas expenditure per block; formally Hl.
    pub gas_limit: u64,
    /// A scalar value equal to the total gas used in transactions in this block; formally Hg.
    pub gas_used: u64,
    /// A scalar value equal to the reasonable output of Unix’s time() at this block’s inception;
    /// formally Hs.
    pub timestamp: u64,
    /// A 256-bit hash which, combined with the
    /// nonce, proves that a sufficient amount of computation has been carried out on this block;
    /// formally Hm.
    pub mix_hash: B256,
    /// A 64-bit value which, combined with the mixhash, proves that a sufficient amount of
    /// computation has been carried out on this block; formally Hn.
    pub nonce: u64,
    /// A scalar representing EIP1559 base fee which can move up or down each block according
    /// to a formula which is a function of gas used in parent block and gas target
    /// (block gas limit divided by elasticity multiplier) of parent block.
    /// The algorithm results in the base fee per gas increasing when blocks are
    /// above the gas target, and decreasing when blocks are below the gas target. The base fee per
    /// gas is burned.
    pub base_fee_per_gas: Option<u64>,
    /// The total amount of blob gas consumed by the transactions within the block, added in
    /// EIP-4844.
    pub blob_gas_used: Option<u64>,
    /// A running total of blob gas consumed in excess of the target, prior to the block. Blocks
    /// with above-target blob gas consumption increase this value, blocks with below-target blob
    /// gas consumption decrease it (bounded at 0). This was added in EIP-4844.
    pub excess_blob_gas: Option<u64>,
    /// The hash of the parent beacon block's root is included in execution blocks, as proposed by
    /// EIP-4788.
    ///
    /// This enables trust-minimized access to consensus state, supporting staking pools, bridges,
    /// and more.
    ///
    /// The beacon roots contract handles root storage, enhancing Ethereum's functionalities.
    pub parent_beacon_block_root: Option<B256>,
    /// The Keccak 256-bit hash of the root node of the trie structure populated with each
    /// [EIP-7685] request in the block body.
    ///
    /// [EIP-7685]: https://eips.ethereum.org/EIPS/eip-7685
    pub requests_root: Option<B256>,
    /// An arbitrary byte array containing data relevant to this block. This must be 32 bytes or
    /// fewer; formally Hx.
    pub extra_data: Bytes,
    }
  • https://github.com/alloy-rs/alloy/blob/68065370137088b414f4796e2b3b018e36031e01/crates/consensus/src/header.rs#L28-L134

@mattsse
Copy link
Collaborator Author

mattsse commented Sep 4, 2024

yes, will track

@tcoratger
Copy link
Contributor

yes, will track

@mattsse If you open an issue please tag me. If I've time, will have a look

@mattsse
Copy link
Collaborator Author

mattsse commented Sep 4, 2024

#10687

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-debt Refactor of code section that is hard to understand or maintain D-good-first-issue Nice and easy! A great choice to get started M-prevent-stale Prevents old inactive issues/PRs from being closed due to inactivity
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

7 participants