feat: impl `Encodable2718` and `Decodable2718` for `PooledTransactionsElement` #11482

klkvr · 2024-10-04T11:34:57Z

Closes #11153
Similar to #11218

klkvr · 2024-10-04T11:38:49Z

crates/primitives/src/transaction/pooled.rs

@@ -581,23 +360,110 @@ impl Decodable for PooledTransactionsElement {
        // Check if the tx is a list
        if header.list {


Ideally we would just delegate to Decodable2718::network_decode here as it's done for TransactionSigned. However, this decode is more strict as it can't decode enveloped transactions (without header) vs default impl which can:

reth/crates/primitives/src/transaction/mod.rs

Lines 1335 to 1337 in 227e293

/// CAUTION: Due to a quirk in [`Header::decode`], this method will succeed even if a typed

/// transaction is encoded in this format, and does not start with a RLP header:

/// `tx-type || rlp(tx-data)`.

I am wondering if this should be supported by plain rlp decoding at all? feels a bit unsafe and I believe there is always a correct encoding format defined for each context? I guess this is only useful for user-facing methods eg sendRawTransaction where we'd want to give more flexibility and in those cases fallback to enveloped decoding can be done explicitly in-place or through a helper fn

hmm, tbh I really don't know.
wdyt @Rjected

I believe there is always a correct encoding format defined for each context?

Yes, although if we want to make each decoding method strict, we need to add checks to make network_decode only work on network encodings, same for enveloped encoding.

yep, I think decode_2718 is already strict, just need to remove this workaround from network_decode https://github.com/alloy-rs/alloy/blob/47e57b3b47d0dc300a2dbbf75ebe7dbd8b644939/crates/eips/src/eip2718.rs#L154-L159

mattsse

great, all of this looks correct, but I always feel a bit icky about touching rlp stuff so please also take a look @Rjected

unsure what to do about the Decodable

mattsse · 2024-10-04T13:00:05Z

crates/primitives/src/transaction/pooled.rs

@@ -581,23 +360,110 @@ impl Decodable for PooledTransactionsElement {
        // Check if the tx is a list
        if header.list {


hmm, tbh I really don't know.
wdyt @Rjected

mattsse · 2024-10-04T13:03:08Z

crates/primitives/src/transaction/pooled.rs

+        let mut payload_length = self.encode_2718_len();
+        if !self.is_legacy() {
+            payload_length += Header { list: false, payload_length }.length();


is this something we should add to the trait, because we also have encode_2718_len but no network len?

yeah, network_len would make sense imo

Rjected

this looks good to me, nice!

* chore(provider): use `get_in_memory_or_storage` on `transactions_by_block_range` (paradigmxyz#11453) * chore(exex): adjust WAL gauge metric names (paradigmxyz#11454) * chore(provider): use `get_in_memory_or_storage_by_block` on `fn block_body_indices` (paradigmxyz#11452) * chore(provider): find `last_database_block_number` with `BlockState` anchor instead (paradigmxyz#11455) * feat: add metrics for failed deliveries (paradigmxyz#11456) * chore: release 1.0.8 (paradigmxyz#11457) * chore(provider): use `block_ref` instead on `BlockState` (paradigmxyz#11458) * feat(rpc): Add codes in execution witness return (paradigmxyz#11443) * fix: ensure the request's gas limit does not exceed the target gas limit (paradigmxyz#11462) * feat(grafana): ExEx WAL (paradigmxyz#11461) * fix: use correct rpc errors (paradigmxyz#11463) * chore(provider): clone after filtering on `sealed_headers_while` (paradigmxyz#11459) Co-authored-by: Matthias Seitz <[email protected]> * Map `TransferKind::EofCreate` => `OperationType::OpEofCreate` (paradigmxyz#11090) Co-authored-by: Matthias Seitz <[email protected]> * fix: windows build (paradigmxyz#11465) * chore: use `block_ref` on `CanonicalInMemoryState` (paradigmxyz#11467) * chore(rpc): remove include_preimage param on debug_execution_witness (paradigmxyz#11466) * feat: make addons stateful (paradigmxyz#11204) Co-authored-by: Arsenii Kulikov <[email protected]> * Relax Trait Bounds on TransactionPool::Transaction and EthPoolTransaction (paradigmxyz#11079) Co-authored-by: Matthias Seitz <[email protected]> * test: add unit tests for `CanonicalChain` (paradigmxyz#11472) * feat(perf): integrate OnStateHook in executor (paradigmxyz#11345) * feat: cleaned up prepare_call_env() (paradigmxyz#11469) Co-authored-by: Matthias Seitz <[email protected]> * chore: use block.body directly (paradigmxyz#11474) * feat: Add metrics to track transactions by type in txpool (paradigmxyz#11403) Co-authored-by: garwah <garwah@garwah> Co-authored-by: Matthias Seitz <[email protected]> * chore: op chainspec (paradigmxyz#11415) * chore(exex): more backfill debug logs (paradigmxyz#11476) * fix(exex): use thresholds in stream backfill (paradigmxyz#11478) * chore(db): capture tx opening backtrace in debug mode (paradigmxyz#11477) * chore(sdk): `SealedHeader` generic over header (paradigmxyz#11429) * chore(lint): fix lint primitives (paradigmxyz#11487) * chore(provider): add more test coverage on `BlockchainProvider::*by_tx_range` queries (paradigmxyz#11480) * test: ensure default hash matches (paradigmxyz#11486) * chore: rm deposit contract config for op (paradigmxyz#11479) * chore(lint): fix lint storage (paradigmxyz#11485) * chore(provider): add more test coverage on `BlockchainProvider::*by_block_range` queries (paradigmxyz#11488) * fix(rpc-eth-types): incorrect error msg(; -> :) (paradigmxyz#11503) Signed-off-by: jsvisa <[email protected]> * Reexport optimism specific crates from `op-reth` (paradigmxyz#11499) * feat: rpc replace function created (paradigmxyz#11501) Co-authored-by: Matthias Seitz <[email protected]> * Add metrics for failed deliveries to Grafana dashboard (paradigmxyz#11481) * chore: Remove duplicate EthereumChainSpecParser in favor of existing EthChainSpecParser (paradigmxyz#11412) Co-authored-by: garwah <garwah@garwah> Co-authored-by: Matthias Seitz <[email protected]> * chore(lint): fix `clippy::needles_lifetimes` (paradigmxyz#11496) * chore: rm from genesis impl (paradigmxyz#11509) * fix: cap gas limit properly (paradigmxyz#11505) * feat: add PoolBuilderConfigOverrides (paradigmxyz#11507) * feat: expose Op node network_config helper (paradigmxyz#11506) * chore(deps): weekly `cargo update` (paradigmxyz#11518) Co-authored-by: github-merge-queue <[email protected]> * test: add unit tests for `PruneLimiter` (paradigmxyz#11517) * feat(provider): add `test_race` to `BlockchainProvider2` tests (paradigmxyz#11523) * fix(tree): make state methods work for historical blocks (paradigmxyz#11265) Co-authored-by: Roman Krasiuk <[email protected]> Co-authored-by: Federico Gimenez <[email protected]> * rpc: use `eth_api()` method (paradigmxyz#11516) * chore: delete rpc-types (paradigmxyz#11528) * feat: add get_highest_tx_by_sender to pools (paradigmxyz#11514) Co-authored-by: Matthias Seitz <[email protected]> * ci: add `windows` cargo check (paradigmxyz#11468) * fix: acquire permit first (paradigmxyz#11537) * chore: dont fail on ttd (paradigmxyz#11539) * grafana: add metrics of all transactions in pool by type (paradigmxyz#11515) Co-authored-by: Emilia Hane <[email protected]> Co-authored-by: Emilia Hane <[email protected]> * feat(exex): subscribe to notifications with head using `ExExContext` (paradigmxyz#11500) * chore: enforce window (paradigmxyz#11540) * Refactor get_payload_bodies_by_hash_with to be non-blocking (paradigmxyz#11511) Co-authored-by: Matthias Seitz <[email protected]> * Introduce Eth PayloadTypes Impl (paradigmxyz#11519) Co-authored-by: Matthias Seitz <[email protected]> * fix(op-reth): add jemalloc feature to optimism-cli for version (paradigmxyz#11543) * fix(grafana): remove rate function from panel "Transactions by Type in Pool" (paradigmxyz#11542) * chore: move ethfiltererror (paradigmxyz#11552) * chore: rm redundant type hint (paradigmxyz#11557) * Introduce Op PayloadTypes Impl (paradigmxyz#11558) Co-authored-by: Matthias Seitz <[email protected]> * chore: chain manual serialisation implementation (paradigmxyz#11538) * chore: rm unused optimism feature (paradigmxyz#11559) * feat(trie): expose storage proofs (paradigmxyz#11550) * chore: rm unused optimism feature from compat (paradigmxyz#11560) * Added InternalBlockExecutionError to execute.rs exports (paradigmxyz#11525) Co-authored-by: Matthias Seitz <[email protected]> * docs(exex): include code for ExEx book from real files (paradigmxyz#11545) * fix: actually configure the custom gas limit (paradigmxyz#11565) * chore: relax trait bound for `EthTransactions` (paradigmxyz#11571) * fix(provider): fix sub overflow on `tx_range` queries for empty blocks (paradigmxyz#11568) * chore(provider): add more test coverage on `BlockchainProvider` non-range queries (paradigmxyz#11564) * feat: adding a new method to network config builder (paradigmxyz#11569) * chore: rm unused optimism feature from engine api (paradigmxyz#11577) * chore: replace some revm deps (paradigmxyz#11579) * chore: rm bad cap function (paradigmxyz#11562) * feat: impl `Encodable2718` and `Decodable2718` for `PooledTransactionsElement` (paradigmxyz#11482) * fix(exex): exhaust backfill job when using a stream (paradigmxyz#11578) * fix: in-memory trie updates pruning (paradigmxyz#11580) Co-authored-by: Matthias Seitz <[email protected]> * chore(providers): test race condition on all `BlockchainProvider2` macro tests (paradigmxyz#11574) * chore: also derive arb for test (paradigmxyz#11588) * chore: bump alloy primitives 0 8 7 (paradigmxyz#11586) * chore(ci): remove expected failures related to checksummed addresses (paradigmxyz#11589) * chore(rpc): use `block_hash` instead on fetching `debug_trace_block` block (paradigmxyz#11587) * feat: add mul support for SubPoolLimit (paradigmxyz#11591) Co-authored-by: Dan Cline <[email protected]> * docs: delete missing part path (paradigmxyz#11590) Co-authored-by: Dan Cline <[email protected]> * fix: simplify reorg handling (paradigmxyz#11592) * fix: use original bytes for codes (paradigmxyz#11593) * perf(rpc): use `Arc<BlockWithSenders` on `full_block_cache` (paradigmxyz#11585) * fix: active inflight count (paradigmxyz#11598) * fix(net): max inflight tx reqs default (paradigmxyz#11602) * fix: set deposit gasprice correctly (paradigmxyz#11603) * fix: set system tx correctly (paradigmxyz#11601) * fix(trie): prefix set extension (paradigmxyz#11605) * feat: add tx propagation mode (paradigmxyz#11594) * feat: add helper function to provde the tx manager config (paradigmxyz#11608) * fix(grafana): set instance variable from `reth_info` metric (paradigmxyz#11607) * fix(net): add concurrency param from config to `TransactionFetcherInfo` (paradigmxyz#11600) Co-authored-by: Matthias Seitz <[email protected]> * perf(rpc): optimistically retrieve block if near the tip on `eth_getLogs` (paradigmxyz#11582) * update fork base commit * remove new book tests --------- Signed-off-by: jsvisa <[email protected]> Co-authored-by: joshieDo <[email protected]> Co-authored-by: Alexey Shekhirin <[email protected]> Co-authored-by: Matthias Seitz <[email protected]> Co-authored-by: Francis Li <[email protected]> Co-authored-by: Emilia Hane <[email protected]> Co-authored-by: Arsenii Kulikov <[email protected]> Co-authored-by: Eric Woolsey <[email protected]> Co-authored-by: Thomas Coratger <[email protected]> Co-authored-by: Federico Gimenez <[email protected]> Co-authored-by: Varun Doshi <[email protected]> Co-authored-by: garwah <[email protected]> Co-authored-by: garwah <garwah@garwah> Co-authored-by: greged93 <[email protected]> Co-authored-by: Delweng <[email protected]> Co-authored-by: Parikalp Bhardwaj <[email protected]> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: github-merge-queue <[email protected]> Co-authored-by: Dan Cline <[email protected]> Co-authored-by: Roman Krasiuk <[email protected]> Co-authored-by: crazykissshout <[email protected]> Co-authored-by: tedison <[email protected]> Co-authored-by: David <[email protected]> Co-authored-by: Emilia Hane <[email protected]> Co-authored-by: Steven <[email protected]> Co-authored-by: Debjit Bhowal <[email protected]> Co-authored-by: Oliver <[email protected]> Co-authored-by: Luca Provini <[email protected]> Co-authored-by: John <[email protected]>

…sElement` (paradigmxyz#11482)

klkvr added 2 commits October 4, 2024 15:16

encodable

5f020f9

decodable

7dbee80

klkvr requested review from mattsse, Rjected, emhane, onbjerg and gakonst as code owners October 4, 2024 11:34

github-actions bot added A-sdk Related to reth's use as a library C-enhancement New feature or request labels Oct 4, 2024

klkvr commented Oct 4, 2024

View reviewed changes

klkvr added 2 commits October 4, 2024 15:41

fix doc

3a5b770

fix doc

ebbb4a6

mattsse approved these changes Oct 4, 2024

View reviewed changes

mattsse mentioned this pull request Oct 4, 2024

[Feature] Add network_len to Encodable2718 alloy-rs/alloy#1430

Closed

Rjected approved these changes Oct 8, 2024

View reviewed changes

klkvr added this pull request to the merge queue Oct 8, 2024

Merged via the queue into main with commit e18b0ba Oct 8, 2024
36 checks passed

klkvr deleted the klkvr/pool-2718 branch October 8, 2024 14:49

vandenbogart pushed a commit to testmachine-ai/reth that referenced this pull request Oct 14, 2024

feat: impl Encodable2718 and Decodable2718 for `PooledTransaction…

ba8ccf5

…sElement` (paradigmxyz#11482)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: impl `Encodable2718` and `Decodable2718` for `PooledTransactionsElement` #11482

feat: impl `Encodable2718` and `Decodable2718` for `PooledTransactionsElement` #11482

klkvr commented Oct 4, 2024

klkvr Oct 4, 2024 •

edited

Loading

mattsse Oct 4, 2024

Rjected Oct 7, 2024

klkvr Oct 7, 2024

mattsse left a comment

mattsse Oct 4, 2024

mattsse Oct 4, 2024

klkvr Oct 4, 2024

Rjected left a comment

		@@ -581,23 +360,110 @@ impl Decodable for PooledTransactionsElement {
		// Check if the tx is a list
		if header.list {

	/// CAUTION: Due to a quirk in [`Header::decode`], this method will succeed even if a typed
	/// transaction is encoded in this format, and does not start with a RLP header:
	/// `tx-type \|\| rlp(tx-data)`.

feat: impl Encodable2718 and Decodable2718 for PooledTransactionsElement #11482

feat: impl Encodable2718 and Decodable2718 for PooledTransactionsElement #11482

Conversation

klkvr commented Oct 4, 2024

klkvr Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

mattsse Oct 4, 2024

Choose a reason for hiding this comment

Rjected Oct 7, 2024

Choose a reason for hiding this comment

klkvr Oct 7, 2024

Choose a reason for hiding this comment

mattsse left a comment

Choose a reason for hiding this comment

mattsse Oct 4, 2024

Choose a reason for hiding this comment

mattsse Oct 4, 2024

Choose a reason for hiding this comment

klkvr Oct 4, 2024

Choose a reason for hiding this comment

Rjected left a comment

Choose a reason for hiding this comment

feat: impl `Encodable2718` and `Decodable2718` for `PooledTransactionsElement` #11482

feat: impl `Encodable2718` and `Decodable2718` for `PooledTransactionsElement` #11482

klkvr Oct 4, 2024 •

edited

Loading