refactor: move hash memoization out of signed #1485

prestwich · 2024-10-15T17:28:13Z

This is a breaking change

Motivation

See discussion in #1460 and #1483

Memoizing the hash in Signed is inappropriate, as the transaction does not know its full 2718 serialization without being embedded in an envelope. This breaks the hash out into a Sealed type that is produced when the Envelope is constructed. The sealed type is transparent to the user

Solution

Introduce Enveloped type alias
ensure that From<> impls properly Seal
ensure that impl Encodable2718 for TxEnvelope produces the correct trie_hash by returning the memoized hash. This previously produced incorrect hashes for 4844 txns with sidecars

PR Checklist

Added Tests
Added Documentation
Breaking changes

klkvr · 2024-10-15T20:29:27Z

crates/consensus/src/transaction/envelope.rs

+        let mut buf = Vec::with_capacity(v.tx().encoded_len_with_signature(v.signature()));
+        v.tx().encode_with_signature_fields(v.signature(), &mut buf);
+        let h = keccak256(buf);


should all of those be Sealable impls?

no, only the envelope knows the correct serialization for its components (tx types don't know their flag as it may be network dependent c.f. issue #1483)

prestwich · 2024-10-16T14:57:07Z

this is ready for re-review

emhane · 2024-10-16T15:10:33Z

crates/consensus/src/transaction/envelope.rs

 pub enum TxEnvelope {
    /// An untagged [`TxLegacy`].
-    Legacy(Signed<TxLegacy>),
+    Legacy(Enveloped<TxLegacy>),
    /// A [`TxEip2930`] tagged with type 1.
-    Eip2930(Signed<TxEip2930>),
+    Eip2930(Enveloped<TxEip2930>),
    /// A [`TxEip1559`] tagged with type 2.
-    Eip1559(Signed<TxEip1559>),
+    Eip1559(Enveloped<TxEip1559>),
    /// A TxEip4844 tagged with type 3.
    /// An EIP-4844 transaction has two network representations:
    /// 1 - The transaction itself, which is a regular RLP-encoded transaction and used to retrieve
    /// historical transactions..
    ///
    /// 2 - The transaction with a sidecar, which is the form used to
    /// send transactions to the network.
-    Eip4844(Signed<TxEip4844Variant>),
+    Eip4844(Enveloped<TxEip4844Variant>),
    /// A [`TxEip7702`] tagged with type 4.
-    Eip7702(Signed<TxEip7702>),
+    Eip7702(Enveloped<TxEip7702>),
 }


excellent, but, what's the plan for alloy_consensus::TypedTransaction now? it's essentially the same type

TypedTransaction is the unsigned version. As such neither signature or hash is known, and it just includes a signable version of the inner transaction and is used in the NetworkWallet::<N>::sign_transaction_from API

actually now that you mention it, we need to apply #1489 to TypedTransaction as well

mattsse · 2024-10-16T15:23:25Z

crates/consensus/src/transaction/envelope.rs

+        let mut buf = Vec::with_capacity(v.tx().encoded_len_with_signature(v.signature()));
+        v.tx().encode_with_signature_fields(v.signature(), &mut buf);


is there no other way to get the tx hash?

if the only solution is to go through the txenvelope enum this will be very inconvenient

c.f. #1483. Technically it's legal for networks to make envelopes that do the following:

use the same inner type, but a different serialization (e.g. a TxLegacy serialized borsht instead of RLP)

use a different tx flag for the same inner type (e.g. a TxEnvelope containing TxEip1559 at 23 instead of 2

use the same inner type at multiple tx flags (e.g. a TxEnvelope containing TxEip1559 at both 2 and 23)

So there is no way to correctly get the hash of a transaction without wrapping it in the specific network's envelope first

imo if a network does this then that's on them, because the type id is even part of the eip itself, if they'd choose 23 then this would no longer be 1559

all of those usecases can be solved by introducing a separate inner transaction type (e.g. network can provide its own TxLegacy)

right now it makes total sense for separate transaction types to know their full encoding because we don't yet have a case of network rolling out encoding of transaction conflicting with existing tx types. also given that most of the networks target for compatibility with ethereum I'd consider such case very unlikely

I also consider it unlikely. But it's still the only correct abstraction of an EIP-2718 envelope if we want our code to be reusable.

because the type id is even part of the eip itself

This is a fair reading of the spec. And @klkvr is right that people downstream can just duplicate 500 lines of code per tx type to change a single constant. However, it costs us very little to have the envelope drive serialization and hashing and prevents future headaches when someone inevitably does something silly here

For another interesting example, consider the extension sentinel value in eip-2718 0xff 🙃 This is also unlikely ofc, but tx-association falls down in extended envelopes

klkvr · 2024-10-16T15:26:57Z

At current state of alloy transactions do know their encoding and their type byte. I've seen a lot of examples of people using encode_with_signature and I don't think there's a reason to break this API (which would be required to make tx type envelope-associated)

IMO this edge case (Ethereum transactions with different type byte) is very unlikely and already can be solved by introducing separate types

prestwich · 2024-10-16T15:34:19Z

At current state of alloy transactions do know their encoding and their type byte.

yes, this is the logic bug in #1483. They do, but they should not, as the type byte is an envelope feature, not a transaction feature. See #1483 and #1485 (comment)

I do not know of any network that do this, but i do think that the cost of implementing it correctly now is small, while the cost of fixing it later is large

I've seen a lot of examples of people using encode_with_signature and I don't think there's a reason to break this API (which would be required to make tx type envelope-associated)

This API is broken right now, and their usage is almost certainly incorrect. It's why the encoding/decoding functions were private before #529, and why #529 hides them from users. We deliberately set out to prevent people from doing this and made it officially not part of the public API. as TxEnvelope is the only safe and correct API for transaction enc/decoding in Ethereum.

klkvr · 2024-10-16T15:48:19Z

This API is broken right now, and their usage is almost certainly incorrect.

could you please elaborate a bit on that? is it broken for 4844 case?

prestwich · 2024-10-16T16:17:49Z

could you please elaborate a bit on that? is it broken for 4844 case?

There's that, yes. 4844 transaction with sidecars produce incorrect trie_hash output right now. So that needs to be solved regardless.

The docs for encode_with_signature are wrong
encode_with_signature produces 2718 output when with_header is false, and RLP in network format when with_header is true. All other encode_ methods produce RLP output in non-network format
decode_with_signature is missing entirely
EncodableSignature seems to now be redundant as reth migrated to alloy-primitives::Signature
sometimes header presence is determined by method name encode_fields, while sometimes it's by arg with_header: bool (related to second bullet above)

I can continue, but basically this is a broken API that confuses multiple serialization schemes and is almost impossible to correctly use

The long term fix is to change the RLP derive to allow flattening and field-only serialization

prestwich · 2024-10-16T16:27:28Z

actually this is a good time to just make a cleanup PR. I'll go do that

prestwich · 2024-10-19T01:03:48Z

holding off on rebasing this as it's better with #1496

prestwich · 2024-10-20T14:27:29Z

post-#1496 would y'all prefer a PR that does not break out memoization, and instead just fixes #1537 using the new tx_hash api?

prestwich self-assigned this Oct 15, 2024

prestwich requested review from DaniPopes, gakonst, mattsse, onbjerg and emhane as code owners October 15, 2024 17:28

klkvr reviewed Oct 15, 2024

View reviewed changes

klkvr mentioned this pull request Oct 15, 2024

Embed TxEnvelope into rpc-types-eth::Transaction #1460

Open

3 tasks

prestwich force-pushed the prestwich/memo-hash branch from e1b260a to 23a5505 Compare October 16, 2024 02:48

prestwich added 5 commits October 16, 2024 08:25

refactor: move hash memoization out of signed

2c03b2e

fix: trie_hash

c6b64df

fix: import Vec

829aa10

fix: flat serde for Sealed

078c65e

fix: doc links

2f04789

prestwich force-pushed the prestwich/memo-hash branch from c5d37ef to 2f04789 Compare October 16, 2024 12:25

emhane approved these changes Oct 16, 2024

View reviewed changes

prestwich mentioned this pull request Oct 16, 2024

feat: expand Seal api alloy-rs/core#773

Merged

3 tasks

mattsse reviewed Oct 16, 2024

View reviewed changes

prestwich mentioned this pull request Oct 16, 2024

fix: make a sensible encoding api #1496

Open

3 tasks

prestwich linked an issue Oct 20, 2024 that may be closed by this pull request

[Bug] trie_hash impl incorrect for tx envelope #1537

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: move hash memoization out of signed #1485

refactor: move hash memoization out of signed #1485

prestwich commented Oct 15, 2024 •

edited

Loading

klkvr Oct 15, 2024

prestwich Oct 15, 2024

prestwich commented Oct 16, 2024

emhane Oct 16, 2024

prestwich Oct 16, 2024

prestwich Oct 16, 2024

mattsse Oct 16, 2024 •

edited

Loading

prestwich Oct 16, 2024

mattsse Oct 16, 2024

klkvr Oct 16, 2024

prestwich Oct 16, 2024 •

edited

Loading

klkvr commented Oct 16, 2024

prestwich commented Oct 16, 2024 •

edited

Loading

klkvr commented Oct 16, 2024

prestwich commented Oct 16, 2024 •

edited

Loading

prestwich commented Oct 16, 2024

prestwich commented Oct 19, 2024 •

edited

Loading

prestwich commented Oct 20, 2024

		let mut buf = Vec::with_capacity(v.tx().encoded_len_with_signature(v.signature()));
		v.tx().encode_with_signature_fields(v.signature(), &mut buf);

refactor: move hash memoization out of signed #1485

Are you sure you want to change the base?

refactor: move hash memoization out of signed #1485

Conversation

prestwich commented Oct 15, 2024 • edited Loading

Motivation

Solution

PR Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prestwich commented Oct 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattsse Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prestwich Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

klkvr commented Oct 16, 2024

prestwich commented Oct 16, 2024 • edited Loading

klkvr commented Oct 16, 2024

prestwich commented Oct 16, 2024 • edited Loading

prestwich commented Oct 16, 2024

prestwich commented Oct 19, 2024 • edited Loading

prestwich commented Oct 20, 2024

prestwich commented Oct 15, 2024 •

edited

Loading

mattsse Oct 16, 2024 •

edited

Loading

prestwich Oct 16, 2024 •

edited

Loading

prestwich commented Oct 16, 2024 •

edited

Loading

prestwich commented Oct 16, 2024 •

edited

Loading

prestwich commented Oct 19, 2024 •

edited

Loading