wip: feat(txpool): initial sketch by mattsse · Pull Request #7 · paradigmxyz/reth

mattsse · 2022-09-29T17:57:02Z

WIP...

~~interesting, looks like you can't create draft PRs in private repos.~~

Update:

rkrasiuk · 2022-10-03T14:15:00Z

@mattsse the org has to be on paid plan to be able to make draft PRs in private repos

mattsse · 2022-10-06T13:58:16Z

@rakita opening this up for review, however there are still some things missing and things I want to change, especially when it comes to how transactions are stored.

this started out as a modification of the pool we wrote for anvil which works like a graph, where a transaction can depend on a previous transaction, and unlock the following transaction.

For this, there are a few more things to consider like dynamic fee, balance, which are not handled yet.

erigon's txpool is essentially a BTreeMap<(sender, nonce), Tx> and the Tx references the sub-pool where the transaction is currently stored. go's BTreeMap allows traversing the map in both directions starting at a given key, which is used to find all dependent transactions of a (sender, nonce) pair efficiently when the pool needs to update after a new block was mined.

This currently references the next transaction by storing its (sender, nonce) id.

So basically erigon can cheaply update account changes because there's a sender -> transactions mapping, whereas this is currently like tx -> next tx, which would make maintaining things like cumulative gas used more expensive.

trying to come up with a solution for this

EDIT: just found out you can do something like this with rust's BTreeMap via: map.range(..key).next_back().unwrap() so I'll try to refactor internals and store all transactions as (sender, nonce) => TX with a reference to the subpools

Also, this still needs a bunch of tests.

rakita

It was hard for me to wrap my head around this, there seems like there are things in need of cleanup or simplification. There are a lot of pools and new transaction types. SenderId is weird one, why not just use the account address? Parity really knows how to overcomplicate things :).

Maybe the best path is to merge this as it is, and continue working on it there, wdyt @mattsse?

rakita · 2022-10-07T18:00:26Z

crates/transaction-pool/src/client.rs

+
+/// The interface used to interact with the blockchain and access storage.
+#[async_trait::async_trait]
+pub trait PoolClient: Send + Sync + TransactionValidator {


Why is TransactionValidator needed here?

rakita · 2022-10-07T18:01:52Z

crates/transaction-pool/src/client.rs

+    fn ensure_block_number(&self, block_id: &BlockID) -> PoolResult<U64> {
+        self.convert_block_id(block_id)
+            .and_then(|number| number.ok_or(PoolError::BlockNumberNotFound(*block_id)))
+    }


note: we probably need account_info(address) to get the account nonce and balance.

rakita · 2022-10-07T18:09:10Z

crates/transaction-pool/src/client.rs

+    type Error: Into<PoolError>;
+
+    /// Returns the block number for the given block identifier.
+    fn convert_block_id(&self, block_id: &BlockID) -> PoolResult<Option<U64>>;


I am not sure if i understand why would we need get block number and mapping of hash->height. TxPool should only care about base_fee and transactions that are removed/added, and that is broadcasted to txpool on block inclusion.

rakita · 2022-10-07T18:09:41Z

crates/transaction-pool/src/identifier.rs

+///
+/// This assigns a _unique_ `SenderId` for a new `Address`.
+#[derive(Debug)]
+pub struct SenderIdentifiers {


I searched code and this seems not used

rakita · 2022-10-07T18:13:44Z

crates/transaction-pool/src/identifier.rs

+/// This is the identifier of an internal `address` mapping that is valid in the context of this
+/// program.
+#[derive(Debug, Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash)]
+pub struct SenderId(u64);


It seems like there is some internal mapping that uses SenderId, and it is used inside TransactionId as a field.
GraphPool has it as sender_info but it seems like it is not used, maybe it is just a leftover, that still needs impl to be used.

rakita · 2022-10-07T18:28:21Z

crates/transaction-pool/src/pool/mod.rs

+//! In essence the transaction pool is made of two separate sub-pools:
+//!
+//!      _Pending Pool_: Contains all transactions that are valid on the current state and satisfy
+//! (3. a)(1): _No_ nonce gaps      _Queued Pool_: Contains all transactions that are currently


Suggested change

//! (3. a)(1): _No_ nonce gaps _Queued Pool_: Contains all transactions that are currently

//! (3. a)(1): _No_ nonce gaps

//! _Queued Pool_: Contains all transactions that are currently

rakita · 2022-10-07T18:36:26Z

crates/transaction-pool/src/pool/mod.rs

+///
+/// This pool maintains a dependency graph of transactions and provides the currently ready
+/// transactions.
+pub struct GraphPool<T: TransactionOrdering> {


This is the main pool that is inside PoolInner that contains all data/Transaction

rakita · 2022-10-07T18:50:50Z

crates/transaction-pool/src/pool/pending.rs

+
+/// A reference to a transaction in the _pending_ pool
+#[derive(Debug)]
+pub(crate) struct PoolTransactionRef<T: TransactionOrdering> {


There are a lot of transaction types, maybe we can consolidate them into one that is easier to follow. It is just an idea, unsure if it is good.

rakita · 2022-10-07T18:55:50Z

crates/transaction-pool/src/pool/queued.rs

+    ///
+    /// This will be an empty list if there are no nonce gaps across multiple transactions of the
+    /// same sender in the pool. If there are gaps, this will include the missing transactions.
+    pub(crate) missing_dependencies: HashSet<TransactionId>,


TransactionId contains of SenderId (that is internal u64) and nonce, we probably need account address to verify dependencies

rakita · 2022-10-07T19:02:09Z

crates/transaction-pool/src/traits.rs

+    /// Returns the EIP-1559 Priority fee the caller is paying to the block author.
+    ///
+    /// This will return `None` for non-EIP1559 transactions
+    fn max_priority_fee_per_gas(&self) -> Option<&U256>;


There is something called effective gas price that for new transactions is base_fee+max_priority_fee_per_gas and for legact transaction this is gas_price, this was used to sort transactions inside pool.

mattsse · 2022-10-07T19:59:38Z

It was hard for me to wrap my head around this, there seems like there are things in need of cleanup or simplification.

Definitely. Need to refactor some internals to handle things like base fee and balance. I'm currently in the process of integrating some designs that erigon took. which will eventually replace the GraphPool.

There are a lot of pools and new transaction types. SenderId is weird one, why not just use the account address?

idea was that this would reduce an Address to a simple u64, but since of the maps used are BTreeMap, perhaps this doesn't even matter much.

Parity really knows how to overcomplicate things :).

Maybe the best path is to merge this as it is, and continue working on it there, wdyt @mattsse?

sgtm! I'll merge this as is, and follow up with additional PRs.

Resolution checkpoint Resolution checkpoint paradigmxyz#2 Resolution checkpoint paradigmxyz#3 x Resolution checkpoint paradigmxyz#4 Resolution checkpoint paradigmxyz#5 Resolution checkpoint paradigmxyz#6 Resolution checkpoint paradigmxyz#7 Resolution checkpoint paradigmxyz#8 Resolve checkpoint paradigmxyz#9 (transaction primitive) Resolve checkpoint paradigmxyz#10 (rpc api transactions) Resolve checkpoint paradigmxyz#11 (building w/o feature flag) Start review Compiling with and without `optimism` feature flag Remove `DepositTx` from txpool mock tests, they never go into the txpool fmt code lint fix signature tests Co-authored-by: nicolas <48695862+merklefruit@users.noreply.github.com> Use free CI runners (revert before upstream) Co-authored-by: refcell <abigger87@gmail.com> Signature test fixes Co-authored-by refcell <abigger87@gmail.com> Fix Receipt proptest Co-authored-by BB <brian.t.bland@gmail.com> lint Fix variable-length compact for txtype/transaction Co-authored-by: Brian Bland <brian.t.bland@gmail.com> Fix basefee tests Remove unnecessary rpc deps Co-authored-by: Brian Bland <brian.t.bland@gmail.com> Co-authored-by: refcell <abigger87@gmail.com> Co-authored-by: nicolas <48695862+merklefruit@users.noreply.github.com> Co-authored-by: Roberto <bayardo@alum.mit.edu>

* add feature 'enable_tps_gas_record' * format and rebase code --------- Co-authored-by: anonymousGiga <cryptonymGong@gmail.com>

Merge v0.2.0 beta.7

…mxyz#7) Concurrent verification of transaction finality state

* upstreamed to alloy * upstreamed to alloy

…length - Issue #5: Add flush_current_changeset_offset() before reading sidecar in truncate_changesets() to ensure in-memory offset is written - Issue #7: Use ChangesetOffsetReader::with_len() instead of new() to respect committed length from header, ignoring uncommitted records - Add regression tests for both issues - Update changeset-healing-review.md to reflect 17 tests - Remove unused variables in tests

…aradigmxyz#7 slow re-download - Issue paradigmxyz#6 downgraded P0→P3: bad FCU feeder was the cause (all 3 hashes same) - Fixed reth-fcu-feeder.py to use genesis as finalizedBlockHash - Pipeline completes ALL 13 stages with single FCU (1M+ blocks!) - Added Issue paradigmxyz#7: reverse headers downloader extremely slow after restart - Updated status and strategy

feat(txpool): initial sketch

5f869fa

mattsse changed the title ~~feat(txpool): initial sketch~~ wip: feat(txpool): initial sketch Sep 29, 2022

mattsse added 6 commits September 30, 2022 15:43

more abstractions

91efdc2

work on listeners

4f8b9d9

Merge branch 'master' into matt/add-tx-pool

ddcf584

work on tx pool

f679140

Merge branch 'master' into matt/add-tx-pool

8a04dd6

work on tx pool

6238619

mattsse added 5 commits October 3, 2022 18:22

feat: more transaction types

22f1d88

Merge branch 'master' into matt/add-tx-pool

00ad854

chore: cleanup

6028f47

work on queued pool

b2ff8b7

work on pending pool

9e1e377

mattsse force-pushed the matt/add-tx-pool branch from e2ec70c to 9e1e377 Compare October 4, 2022 12:37

mattsse added 6 commits October 4, 2022 16:25

docs: more docs

3e20868

feat: add transactions iterator

6b28560

impl transaction pruning

07aab5c

refactor: separate pool functions

56dfb76

refactor: different pool types

df66604

feat: work on PoolApi

8ba1501

mattsse force-pushed the matt/add-tx-pool branch from c8afd48 to 8ba1501 Compare October 4, 2022 18:19

mattsse added 8 commits October 5, 2022 12:13

docs: more pool docs

153e5bf

refactor: unify transaction associated types

a319290

feat: add basic event handling

fc0e773

feat: more helper functions

957e330

style: cleanup, clippy fixes

9fdea85

rename pool

1d710a3

feat: implement constructors

5152cd1

feat: extend transaction pool api

acea23a

mattsse added 13 commits October 5, 2022 17:19

feat: more pool api impls

9ea4337

feat: introduce chainevents

53ec974

feat: more pool api

b6936fa

feat: start integrating dynamic fee

eb0fa84

simplify ids

62a9953

use address type

9368ec0

add sender info

24c10c3

more refactoring

3002938

refactor: remove associated Id type

0eb1539

ease warnings

9f23ade

add dependency function

82c5c4b

remove Hash associated type

af1aaf3

Merge branch 'master' into matt/add-tx-pool

28324d1

mattsse requested a review from rakita October 6, 2022 13:58

Merge branch 'master' into matt/add-tx-pool

fe4ca48

rakita approved these changes Oct 7, 2022

View reviewed changes

mattsse merged commit 791ee28 into main Oct 7, 2022

mattsse deleted the matt/add-tx-pool branch October 7, 2022 20:00

anonymousGiga added a commit to anonymousGiga/reth that referenced this pull request Feb 20, 2024

add feature 'enable_tps_gas_record' (paradigmxyz#7)

cefbf98

* add feature 'enable_tps_gas_record' * format and rebase code --------- Co-authored-by: anonymousGiga <cryptonymGong@gmail.com>

yutianwu pushed a commit to yutianwu/reth that referenced this pull request Jun 5, 2024

Merge pull request paradigmxyz#7 from bnb-chain/merge_v0.2.0-beta.7

aadf535

Merge v0.2.0 beta.7

AshinGau added a commit to AshinGau/reth that referenced this pull request Sep 12, 2024

grevm: concurrent verification of transaction finality state (paradig…

cca338f

…mxyz#7) Concurrent verification of transaction finality state

AshinGau added a commit to AshinGau/reth that referenced this pull request Oct 13, 2024

grevm: concurrent verification of transaction finality state (paradig…

35808a6

…mxyz#7) Concurrent verification of transaction finality state

jenpaff pushed a commit that referenced this pull request Oct 6, 2025

chore: upstreamed to alloy (#7)

3cbb9a9

* upstreamed to alloy * upstreamed to alloy

kartik-ritual mentioned this pull request Feb 20, 2026

docs(rpc): improve eth rpc docs #275

Merged

	//! (3. a)(1): _No_ nonce gaps _Queued Pool_: Contains all transactions that are currently
	//! (3. a)(1): _No_ nonce gaps
	//! _Queued Pool_: Contains all transactions that are currently

Conversation

mattsse commented Sep 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rkrasiuk commented Oct 3, 2022

Uh oh!

mattsse commented Oct 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rakita 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattsse commented Oct 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mattsse commented Sep 29, 2022 •

edited

Loading

mattsse commented Oct 6, 2022 •

edited

Loading