docs: rewrite pool.md for current txpool/stempool design#3887
Conversation
Replace outdated DAG mempool description with the implemented model: flat PoolEntry lists, aggregate "one big tx" validation, separate txpool and stempool layers, reorg cache, mining buckets, and reconciliation. Addresses mimblewimble#2447.
wiesche89
left a comment
wiesche89
left a comment
There was a problem hiding this comment.
Overall, this is a substantial improvement over the old DAG based document and matches the current pool design well. Most of my comments are small precision or wording points. My only broader concern is keeping the implementation level walkthroughs maintainable as the code changes. With the validation boundaries and bucket behavior clarified, this looks good to me.
|
|
||
| The first step is the validation of the transaction itself. This involves the enforcement of all consensus rules surrounding the construction of the transaction itself, and the verification of all relevant signatures and proofs. | ||
| - Mimblewimble already supports transaction aggregation and cut-through at the protocol level. | ||
| - A single aggregate validation reuses the same rules as block validation (kernel sums, UTXO membership, coinbase maturity, NRD rules, etc.). |
There was a problem hiding this comment.
coinbase maturity isn't checked inside the aggregate validation, it runs separately beforehand in the add path using the result of locate_spends, as step 7 below correctly describes. I'd drop coinbase maturity from this list so it doesn't read as part of validate_raw_tx.
| 1. Sort and group pool txs with **bucket** logic (`bucket_transactions`): | ||
| - Prefer keeping dependency order and maximizing cut-through within a bucket. | ||
| - Prefer higher aggregate fee rate; avoid merging if it would lower fee rate. | ||
| - Txs with multiple in-pool parents are skipped for this block (picked up later). |
There was a problem hiding this comment.
The bucket logic rejects a tx on the second input spending a pool output, even when both outputs come from the same parent. Could we describe this as more than one input spending unconfirmed pool outputs instead?
| 2. Re-validate the ordered list incrementally as aggregates against chain state under the miner’s `mineable_max_weight`. | ||
| 3. Return the list of individually mineable transactions for block building. | ||
|
|
||
| Eviction under capacity pressure uses the same bucket ordering and removes a last (low fee-rate, non-dependent) transaction. |
There was a problem hiding this comment.
Non dependent is a little ambiguous because the evicted tx may itself depend on a parent. Could we say a low-fee-rate transaction that no other pool tx depends on instead?
| The pool is required for mining and for normal transaction relay. It is implemented as two related layers plus a short re-org cache: | ||
|
|
||
| The primary structure of the transaction pool is a pair of Directed Acyclic Graphs. Since each transaction is rooted directly by its inputs in a non-cyclic way, this structure naturally encompasses the directionality of the chains of unconfirmed transactions. Defining this structure has a few other nice properties: descendent invalidation (when a conflicting transaction is accepted for a given input) is nearly free, and the mineability of a given transaction is clearly depicted in its location in the hierarchy. | ||
| | Layer | Name in code | Visibility | Role | |
There was a problem hiding this comment.
All three fields are pub in Rust, so Visibility may be easy to misread as code visibility. Could we call this column Network exposure or something similar?
| - Private holding area for Dandelion **stem** transactions. | ||
| - Stem txs are validated with the current **txpool aggregate** as `extra_tx`, so they cannot double-spend against fluffed txs. | ||
| - Contents are not used for compact-block reconstruction or mining (`retrieve_transactions` and `prepare_mineable_transactions` use **txpool only**). | ||
| - The Dandelion monitor (`servers/src/grin/dandelion_monitor.rs`) periodically fluffs stem entries when embargo or epoch timers expire by moving them into the txpool path. |
There was a problem hiding this comment.
Could we mention the aggregation timer here as well? During a fluff epoch, the monitor also fluffs when an entry exceeds aggregation_secs, while embargo expiry is handled separately per entry.
| The first step is the validation of the transaction itself. This involves the enforcement of all consensus rules surrounding the construction of the transaction itself, and the verification of all relevant signatures and proofs. | ||
| - Mimblewimble already supports transaction aggregation and cut-through at the protocol level. | ||
| - A single aggregate validation reuses the same rules as block validation (kernel sums, UTXO membership, coinbase maturity, NRD rules, etc.). | ||
| - Dependency ordering is not required for storage; it is reconstructed only when selecting txs for a block (see [Mining selection](#mining-selection)). |
There was a problem hiding this comment.
Only seems slightly too strong here because the same bucket dependency ordering is also reconstructed for eviction. Could we mention mining and eviction, or simply drop only?
| Given the focus of grin (and mimblewimble) on reduced resource consumption, the memory pool should be an optional but recommended component for non-mining nodes. | ||
| 1. Can be selected by the mining service when building a new block. | ||
| 2. Can be relayed to peers (after any stem embargo ends). | ||
| 3. Moderate broadcast behavior so only transactions valid against current chain state (plus the pool itself) are accepted. |
There was a problem hiding this comment.
this breaks the transactions that can... structure of the list. Maybe Act as a broadcast moderator, so only transactions valid against the current chain and pool state are accepted?
There was a problem hiding this comment.
On second thought, a better fit for the existing list structure might be: Have been validated against the current chain and pool state before acceptance and relay.
| | Block building from pool | `servers/src/mining/mine_block.rs` | | ||
| | Reconcile on new block / reorg | `servers/src/common/adapters.rs` | | ||
|
|
||
| ## Historical note |
There was a problem hiding this comment.
This mostly repeats the DAG explanation near the start of the document. Would it be simpler to keep the historical context there and drop this final section?
|
|
||
| ## Adding a transaction | ||
|
|
||
| High-level flow in `TransactionPool::add_to_pool`: |
There was a problem hiding this comment.
This is accurate today, but the exact ten step ordering is fairly implementation specific and may become stale after a refactor. Could we keep this section focused on the important validation boundaries rather than mirroring the function step by step?
2 participants
Summary
Rewrite
doc/internal/pool.mdso it matches the currentpoolcrate instead of the old DAG + orphan-graph design.Document the one big transaction validation model (
aggregate(pool ∪ new_tx)vs chain state), nested as:[ new_tx + [ stempool + [ txpool + [ chain_state ] ] ] ]Use consistent txpool / stempool terminology, plus reorg cache, mining bucket selection, reconciliation, config, and a short code map.
Addresses #2447 (maintainer notes on hashmap / one-big-tx representation and stempool terminology).
Test plan
pool/src/transaction_pool.rs,pool/src/pool.rs, andpool/src/types.rs