fix: make a sensible encoding api #1496

prestwich · 2024-10-16T17:15:12Z

Spawned from the depths of #1485

Motivation

Fix transaction coding APIs to be usable and clear

[Bug] encode_with_signature_fields encodes an invalid transaction when the signature Parity is unexpected #1510 output for encode_with_signature
Codebase generally confuses the 2 uses of RLP encoding (tx payload and network format)
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

Solution

create a simple API in which encoding and decoding function names specify which format they produce/accept

PR Checklist

Added Tests
Added Documentation
Breaking changes

crates/consensus/src/transaction/envelope.rs

prestwich · 2024-10-16T17:35:14Z

cc @mattsse @klkvr. I'd like feedback on the new API before applying it to the other tx types

mattsse

only looked briefly because rlp gives me headaches

wdyt @Rjected

crates/consensus/src/transaction/rlp.rs

prestwich · 2024-10-17T15:54:42Z

this is rebased and ready for review

The main thing it does is make an INTERNAL trait RlpEcdsaTx that captures common encoding behavior across existing tx types. This trait is not intended to cover all future behavior, just to unify codepaths across the 5 current tx types, and make sure that encoding schemes are correct, simple to use, and impossible to confuse

prestwich · 2024-10-19T01:01:12Z

now closes #1510 as well

prestwich · 2024-10-21T13:09:55Z

this has been rebased onto main after #1528 and is ready for review. Note that 1528 included a version of this API in the blob sidecar rlp impl. I'm also prepping a followup PR to move other RLP impls to this style

prestwich · 2024-10-22T16:39:53Z

Could we get a patch release of alloy-rlp? alloy-rs/rlp#28 would let us reduce code complexity

onbjerg · 2024-10-23T15:32:56Z

@prestwich alloy-rlp 0.3.9 is now released with alloy-rs/rlp#28

prestwich · 2024-10-23T17:19:51Z

@prestwich alloy-rlp 0.3.9 is now released with alloy-rs/rlp#28

Thank you, this PR is updated, rebased, and ready for review 👍

mattsse

cool,
I think this a lot cleaner now.

would like one more sanity rlp review from @klkvr

Rjected

I really like the API, one concern about the header type when this is implemented on legacy txs - I'm thinking legacy txs should probably override all of the default impls for network_ fns

I think we should also specify the type of RLP header when possible (list v string)

crates/consensus/src/transaction/rlp.rs

crates/consensus/src/transaction/legacy.rs

crates/consensus/src/transaction/eip2930.rs

crates/consensus/src/transaction/eip4844.rs

prestwich · 2024-10-24T14:18:07Z

fixes up and ready for re-review

klkvr

lgtm, just a nit

crates/consensus/src/transaction/eip4844.rs

## Motivation Bumps alloy, alloy-core and alloy-eip7702. Code changes: - alloy-rs/alloy#1496 changed transaction encoding API, encoding functions are now prefixed with `rlp`,`eip2718` or `network`. I've updated `TxDeposit` to have methods following this pattern - alloy-rs/core#776 updated signature type to avoid using `Parity` generic and instead just keep a boolean for parity byte. The motivation is to have more correctness in encoding and data structure. Before that it was possible to construct `Signed<TxLegacy>` where transaction chain_id is `Some`, but the parity would not follow EIP-155. This required a few changes to `SpanBatchTransactions` type: - `encode_y_parity_bits` and `encode_tx_sigs_rs` are merged into single `encode_tx_sigs` which firstly encodes parity bits and then `rs` values. Same was done for decoding methods - `recover_v` method is removed. instead, we now pass the `is_protected` flag down to `SpanBatchLegacyTransactionData` and only set `chain_id` to `Some` when `is_protected` is true Blocked by minor releases for alloy, core and revm ## Solution  ## PR Checklist - [ ] Added Tests - [ ] Added Documentation - [ ] Breaking changes

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

prestwich commented Oct 16, 2024

View reviewed changes

crates/consensus/src/transaction/envelope.rs Show resolved Hide resolved

mattsse reviewed Oct 16, 2024

View reviewed changes

crates/consensus/src/transaction/rlp.rs Show resolved Hide resolved

prestwich marked this pull request as draft October 17, 2024 01:12

prestwich marked this pull request as ready for review October 17, 2024 15:53

prestwich force-pushed the prestwich/improve-enc-api branch from 539cf91 to 95844bf Compare October 17, 2024 15:53

prestwich self-assigned this Oct 17, 2024

prestwich requested a review from mattsse October 17, 2024 19:45

prestwich force-pushed the prestwich/improve-enc-api branch from 95844bf to bb3c510 Compare October 19, 2024 00:29

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

[Bug] encode_with_signature_fields encodes an invalid transaction when the signature Parity is unexpected #1510

Closed

This was referenced Oct 19, 2024

refactor: move hash memoization out of signed #1485

Closed

fix: correct implementations of Encodable and Decodable for sidecars #1528

Merged

[Bug] trie_hash impl incorrect for tx envelope #1537

Closed

yash-atreya mentioned this pull request Oct 21, 2024

chore(deps): bumps alloy, revm, fork-db foundry-rs/foundry#9150

Merged

prestwich force-pushed the prestwich/improve-enc-api branch from 86e2c5e to 21f7aef Compare October 21, 2024 12:55

prestwich added 5 commits October 23, 2024 13:19

fix: make a sensible encoding api

e72ce42

fix: k256 test

c0f30a6

fix: single doc hidden

f99d477

fix: missing doc

b7dd75a

nit: undo changed list

f1bd89a

prestwich added 9 commits October 23, 2024 13:19

lint: clippy

f59f2d4

fix: import vec

36ea0bb

fix: import Vec

3f3a4a2

fix: rebase artifacts

0fa47c0

fix: correct handling of legacy signatures when encoding (#1510)

4dec193

lint: clippy

cc7f064

fix: use corrected impls

1216123

nit: remove bug comments

cf6fb4f

refactor: use length_with_payload

1257307

prestwich force-pushed the prestwich/improve-enc-api branch from 8de9c9f to 1257307 Compare October 23, 2024 17:19

mattsse approved these changes Oct 23, 2024

View reviewed changes

Rjected reviewed Oct 23, 2024

View reviewed changes

crates/consensus/src/transaction/rlp.rs Outdated Show resolved Hide resolved

crates/consensus/src/transaction/rlp.rs Outdated Show resolved Hide resolved

crates/consensus/src/transaction/rlp.rs Outdated Show resolved Hide resolved

klkvr requested changes Oct 23, 2024

View reviewed changes

klkvr reviewed Oct 23, 2024

View reviewed changes

crates/consensus/src/transaction/eip4844.rs Outdated Show resolved Hide resolved

prestwich added 4 commits October 24, 2024 08:05

fix: legacy network headers are transparent

a977339

fix: restore signature v check

7b03e1a

fix: check for leftoverbytes

a7aba88

fix: corrected RLP decoding of eip4844 variant

72169eb

prestwich force-pushed the prestwich/improve-enc-api branch from d9c7bd5 to 72169eb Compare October 24, 2024 12:26

fix: list check in 4844 decoding

2a024ee

klkvr approved these changes Oct 24, 2024

View reviewed changes

crates/consensus/src/transaction/eip4844.rs Outdated Show resolved Hide resolved

fix: add map or

fe75cf0

mattsse approved these changes Oct 28, 2024

View reviewed changes

mattsse merged commit f34a2dd into main Oct 28, 2024
26 checks passed

mattsse deleted the prestwich/improve-enc-api branch October 28, 2024 13:23

prestwich mentioned this pull request Oct 28, 2024

Fix: move type flag encoding back into encode_2718 #1483

Closed

klkvr mentioned this pull request Oct 30, 2024

[wip] feat: bump alloy alloy-rs/op-alloy#205

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: make a sensible encoding api #1496

fix: make a sensible encoding api #1496

prestwich commented Oct 16, 2024 •

edited

Loading

prestwich commented Oct 16, 2024 •

edited

Loading

mattsse left a comment •

edited

Loading

prestwich commented Oct 17, 2024 •

edited

Loading

prestwich commented Oct 19, 2024

prestwich commented Oct 21, 2024

prestwich commented Oct 22, 2024

onbjerg commented Oct 23, 2024

prestwich commented Oct 23, 2024

mattsse left a comment

Rjected left a comment •

edited

Loading

prestwich commented Oct 24, 2024

klkvr left a comment

fix: make a sensible encoding api #1496

fix: make a sensible encoding api #1496

Conversation

prestwich commented Oct 16, 2024 • edited Loading

Motivation

Solution

PR Checklist

prestwich commented Oct 16, 2024 • edited Loading

mattsse left a comment • edited Loading

Choose a reason for hiding this comment

prestwich commented Oct 17, 2024 • edited Loading

prestwich commented Oct 19, 2024

prestwich commented Oct 21, 2024

prestwich commented Oct 22, 2024

onbjerg commented Oct 23, 2024

prestwich commented Oct 23, 2024

mattsse left a comment

Choose a reason for hiding this comment

Rjected left a comment • edited Loading

Choose a reason for hiding this comment

prestwich commented Oct 24, 2024

klkvr left a comment

Choose a reason for hiding this comment

prestwich commented Oct 16, 2024 •

edited

Loading

prestwich commented Oct 16, 2024 •

edited

Loading

mattsse left a comment •

edited

Loading

prestwich commented Oct 17, 2024 •

edited

Loading

Rjected left a comment •

edited

Loading