Repository navigation
wallet: chain-aware repush and mempool reconciliation - #2127
Conversation
|
🔍 OpenCodeReview found 15 issue(s) in this PR.
📄
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
d5c58de to
6aaf2e0
Compare
763db5d to
9cf32a0
Compare
6aaf2e0 to
de7869c
Compare
9cf32a0 to
e05f896
Compare
de7869c to
779835c
Compare
e05f896 to
ce9f3be
Compare
779835c to
af26962
Compare
615e584 to
642f937
Compare
642f937 to
fca8530
Compare
|
Rebased and force-pushed ( Findings from this review thread, against the new head:
Also addressed from earlier summaries:
|
Replaces the wallet's blind rebroadcast loop (every 2-5 minutes, resubmitting the whole user-transaction table in hash order, unbounded) with a reconcile-and-rebroadcast pass that: - orders pending transactions topologically (parents before children), with account-nonce tie-breaking for nonce-bearing transactions; - probes the node's mempool for each pending transaction via the existing `mempool_get_transaction` RPC (no node upgrade required); a failed probe only excludes that transaction (and its descendants) from the pass; - prunes deterministically-rejected chains: a transaction missing from the mempool while *all* its pending parents are present there, with a node-observed submission, over `PRUNE_ABSENCE_THRESHOLD` (3) consecutive passes, is removed from the spendable cache and the rebroadcast set, together with its pending descendants; transactions that were never delivered (transport errors) or previously succeeded are resubmitted instead of pruned; - repushes the remaining missing transactions with per-transaction exponential backoff (30 s -> 15 min) and a retry budget (10 attempts or 24 h), after which the transaction is marked stuck and skipped until it is abandoned manually (`transaction_abandon`) or the wallet restarts. Delivery failures do not burn the rejection budget; descendants of failed or not-yet-due submissions are deferred to a later pass; - clears tracker state for confirmed/pruned transactions and when nothing is pending (stuck markers persist, as documented). The planner, tracker and reconciliation logic are pure functions (`wallet-controller/src/rebroadcast.rs`) with unit tests; the controller run loop is the imperative shell. Supporting changes: - storage: `get_user_transactions_for_account` (a scoped read over the existing `DBUserTx` keyspace; no schema change); - wallet: `get_unconfirmed_transactions_per_account`, filtered to transactions that may still need a (re)broadcast (confirmed, conflicted and abandoned states are excluded; unknown state is kept); - account/output_cache: `prune_dead_transaction`, a variant of `abandon_transaction` that tolerates a stale `InMempool` state (used when the node confirms the transaction is not in the mempool); `abandon_transaction` behavior is unchanged; - `NodeInterfaceError::is_node_rejection` (default `false` = the safe under-pruning choice): the RPC client implements it as "JSON-RPC server-application error response", the handles client as `MempoolError` / `P2p(MempoolError)`. No consensus surface is touched; no wallet DB migration is needed.
fca8530 to
6073a1c
Compare
Second round of review findings (OCR + security review): - classify mempool rejections by error variant, not by Display text: the wire prefix makes any substring check vacuous. Transient and state-dependent outcomes (tip moved, chainstate/subsystem/reorg errors, store races, IBD, mempool/orphan pressure, mempool conflicts, nonce gaps, output-spent and nonce-incrementality races) no longer set node_observed and never feed the prune evidence; - JSON-RPC path: require the CALL_EXECUTION_FAILED code (-32000, which is what the mintlayer server reports for all application errors) and mirror the typed decision on the actual wire message; protocol errors (-326xx) stay delivery failures; - gate the absence streak on node_observed: evidence built before the node first observed a submission cannot combine with a single later rejection into an instant prune; - on_success keeps first_seen, so MAX_PENDING_AGE applies across accept/evict resubmission cycles; - get_unconfirmed_transactions_per_account propagates real storage errors instead of treating every lookup failure as not-found; - extend regression tests: both classifiers per variant, the RPC code gate, the evidence gate, and the age-clock stickiness.
|
All findings from the two latest review runs are addressed in
Not changed —
|
- typed rejection classifier: internal chainstate/storage/accounting
ConnectTransactionError variants (storage, utxo/undo, verifier storage,
PoS/tokens/orders accounting, nonce-bookkeeping races) no longer count
as deterministic rejections; a transient DB hiccup can never prune;
- wire classifier: positive wrapper-prefix gate ("mempool error:" /
"orphan transaction error:") instead of a loose keyword search;
- RPC code gate narrowed to exactly CALL_EXECUTION_FAILED (-32000), the
code the mintlayer server reports for application errors;
- reconcile takes the pass time and only advances the absence streak on
passes where the transaction was actually due, so a prune cannot happen
before the remaining retry attempts had a chance to run;
- the pass is re-timed to the earliest due attempt when that is sooner
than the 2-5 minute interval, so the documented backoff schedule is
honored instead of being clamped to the pass cadence (with a 5s minimum
gap between passes);
- account-nonce chains are modeled as submission dependencies: a nonce-n
transaction is deferred when its same-account nonce-(n-1) predecessor
failed or was skipped, symmetric to the UTXO parent rule;
- mempool probes run with bounded concurrency (8) instead of one serial
round-trip per pending transaction;
- prune_dead_transaction doc comments state the caller obligation and the
probe/prune race (self-healing on confirmation or rescan);
- new tests: internal-variant indeterminacy, derived typed<->wire
deterministic mirror, backoff-gated streak.
|
All findings from the 16:28 review run are addressed in
|
- next_wake: only honor wake times in the future - an elapsed next_attempt belongs to a deferred/blocked transaction, not a backoff-scheduled one, and must not shrink the pass interval to the minimum gap indefinitely; - pre-seed the unsubmitted set with every not-due pending transaction (backoff and stuck): reconcile excludes stuck parents from to_submit, so without the seed their children saw them as ready and were submitted against a permanently absent parent - a guaranteed rejection on every pass that cascaded the subtree into stuck; - defensive early-continues in the submit loop record the transaction as unsubmitted, keeping the child-deferral invariant airtight; - wire-mirror test extended to every constructible excluded variant (chainstate/subsystem wrappers, nonce-bookkeeping races, orphan conflicts), and a new test ties the classifier's wrapper prefixes to the actual p2p/mempool Display attributes.
|
All findings from the 17:25 review run are addressed in
|
- reconcile: established rejection evidence (threshold reached under observation) survives parent-set changes and keeps the transaction in to_prune, so a failed wallet-side prune is retried on the next pass instead of the transaction - whose dead parent may already have left the wallet - being resubmitted until stuck; - the unsubmitted seed excludes transactions the probe showed to be in the mempool: a present parent never blocks a child, and seeding it would defer a due child while its deterministic-rejection evidence (missing while the present parent is there) accumulates toward a prune without the child ever being attempted; - INDETERMINATE wire terms extended to cover the internal chainstate/storage/accounting Displays that carry no mempool keyword of their own (BlockUndo, undo-info, block-index, fee-sum, TransactionVerifierStorage, ConfigError variants) plus the ReorgError inner texts, pinned by a dedicated wire-string test.
The wire classifier's deny-list cannot be derived from Display text for every future variant, so make adding a variant fail CI instead: strum::EnumCount on mempool Error/TxValidationError/MempoolPolicyError/ OrphanPoolError and chainstate ConnectTransactionError, with a test pinning the counts - a mismatch forces a review of both classifiers and the wire-mirror tests in the same change. Also note the durable fix (structured mempool verdict on the RPC error object) in the classifier docs as a node-side follow-up.
A recorded node rejection used to stay valid forever, so a single historic rejection (e.g. a transient server-busy reply) could support a prune much later - after mempool churn, a node restart, or a load balancer rotation made the deterministic-rejection signature appear around a live transaction. Rejection evidence now expires after one hour (REJECTION_EVIDENCE_MAX_AGE): stale evidence falls back to resubmission, which either refreshes the evidence (another rejection) or clears it (an acceptance). Also document the nonce-chain asymmetry in reconcile: nonce dependencies are not modeled in the planner, so a deterministically rejected nonce transaction is never pruned via evidence - it burns its budget and ends up stuck, which is the safe under-pruning direction.
| // Pruned transactions take their pending descendants with them. | ||
| let children: BTreeMap<Id<Transaction>, Vec<Id<Transaction>>> = { | ||
| let mut children: BTreeMap<Id<Transaction>, Vec<Id<Transaction>>> = BTreeMap::new(); | ||
| for tx in pending { | ||
| for parent in &tx.pending_parents { | ||
| children.entry(*parent).or_default().push(tx.id); | ||
| } | ||
| } | ||
| children | ||
| }; |
There was a problem hiding this comment.
Prune propagation (both in reconcile here and in the wallet's remove_from_unconfirmed_descendants) only follows UTXO parent edges via pending_parents/unconfirmed_descendants. Account-nonce chains have no UTXO edge: when a nonce-n predecessor is pruned/abandoned, its nonce-(n+1) successor is neither pruned with it nor classified as deterministically rejected (its predecessor is now absent from the mempool, so the 'parent present, child missing' signature never holds and the absence streak is reset each pass). The successor will then be resubmitted every pass until it either burns its whole retry budget and goes stuck, or hits MAX_PENDING_AGE — a guaranteed-rejection resubmission loop that this whole change was designed to avoid. Consider treating a missing nonce predecessor that was pruned/abandoned this pass as a prune trigger for its successors, or at least excluding such successors from submission.
Suggestion:
| // Pruned transactions take their pending descendants with them. | |
| let children: BTreeMap<Id<Transaction>, Vec<Id<Transaction>>> = { | |
| let mut children: BTreeMap<Id<Transaction>, Vec<Id<Transaction>>> = BTreeMap::new(); | |
| for tx in pending { | |
| for parent in &tx.pending_parents { | |
| children.entry(*parent).or_default().push(tx.id); | |
| } | |
| } | |
| children | |
| }; | |
| // Pruned transactions take their pending descendants with them, for both | |
| // UTXO-parent edges and account-nonce-chain edges (a nonce successor of a | |
| // dead transaction can never confirm). | |
| let children: BTreeMap<Id<Transaction>, Vec<Id<Transaction>>> = { | |
| let mut children: BTreeMap<Id<Transaction>, Vec<Id<Transaction>>> = BTreeMap::new(); | |
| for tx in pending { | |
| for parent in &tx.pending_parents { | |
| children.entry(*parent).or_default().push(tx.id); | |
| } | |
| } | |
| // nonce edges: (account, nonce) -> successor id | |
| ... | |
| children | |
| }; |
- treat a wallet prune failing with CannotChangeTransactionState(Abandoned) as success: the root's prune_dead_transaction already abandons its descendants, so re-pruning them per-id only produced spurious warnings and stale tracker entries within the pass; - document why TxState::Inactive is included in the rebroadcast filter (distinct from user-requested Abandoned) and where the retry cap lives (the controller's tracker budget).
| let now = get_time(); | ||
| // Transactions known not to be in the mempool by the end of this pass: |
There was a problem hiding this comment.
now is re-read here after the probe round, the prune loop and the serial submit-loop interleaving, but the unsubmitted seed set and the is_due guard inside the loop use this later timestamp while reconcile classified with the earlier pass_now. A pass that takes longer than a backoff window (many pending transactions, slow RPC) can classify a transaction as due and submit it even though reconcile treated it as in-backoff (or vice versa), making the streak/no-resubmit-during-backoff guarantees time-dependent on RPC latency. Consider passing a single consistent timestamp captured once at the start of the pass to all of reconcile, the seed filter and the submit guard.
Suggestion:
| let now = get_time(); | |
| // Transactions known not to be in the mempool by the end of this pass: | |
| let now = pass_now; // use the single pass timestamp consistently |
| if parent_not_ready { | ||
| unsubmitted.insert(*tx_id); | ||
| log::debug!("Skipping transaction {tx_id:x}: a parent is not ready this pass"); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Nonce-chain deferral can feed the prune-evidence streak for a transaction that was never actually submitted. Scenario: the nonce predecessor is missing this pass (in backoff), so this child is inserted into unsubmitted and deferred; but reconcile only knows about UTXO parents — next pass, if the child is missing from the mempool, its UTXO parents are present, it has a recent rejection (from an earlier attempt) and is due, so bump_absence advances even though the child was never retried. Three such passes prune a live child purely because its predecessor was in backoff. Consider resetting the child's absence streak here (e.g. self.repush_tracker.reset_absence(tx_id)) when it is deferred due to a nonce/parent not being ready, so evidence only accumulates on passes where the transaction was actually attempted.
Suggestion:
| if parent_not_ready { | |
| unsubmitted.insert(*tx_id); | |
| log::debug!("Skipping transaction {tx_id:x}: a parent is not ready this pass"); | |
| continue; | |
| } | |
| if parent_not_ready { | |
| unsubmitted.insert(*tx_id); | |
| // The transaction was not attempted this pass, so its continued | |
| // absence must not advance the deterministic-rejection streak. | |
| self.repush_tracker.reset_absence(tx_id); | |
| log::debug!("Skipping transaction {tx_id:x}: a parent is not ready this pass"); | |
| continue; | |
| } |
| "tip moved", | ||
| "chainstate error", | ||
| "subsystem call error", |
There was a problem hiding this comment.
The INDETERMINATE term list assumes chainstate failures surface as "chainstate error" on the wire, but that's not true for transparent wrappers. mempool::Error::Validity(TxValidationError::ChainstateError(_)) uses #[error(transparent)] at both wrapper levels, and chainstate::ChainstateError variants display as "Block storage error: ...", "I/O error: ...", "Initialization error: ...", "Block processing failed: ...", etc. — none of these match "chainstate error" (or any other term in the list). Concretely, Mempool error: I/O error: boom (an unhealthy-node outcome that the typed classifier correctly returns false for) will be classified here as a deterministic rejection, since the only required prefix ("mempool error:") matches and no INDETERMINATE term does. On the JSON-RPC transport this flips an indeterminate/internal-failure outcome into a pruning decision — the exact failure mode the doc comment says must never happen. (The wire_message_mirror_matches_typed_classification test includes ChainstateError(IoError("boom")), so it should already be failing.) Suggestion: add the concrete chainstate-storage Display prefixes ("i/o error", "block storage error", "initialization error", "block processing failed", "property read error", "bootstrap error", "error invoking block invalidator") to the term list, or better, move to a structured verdict rather than text matching.
Suggestion:
| "tip moved", | |
| "chainstate error", | |
| "subsystem call error", | |
| "tip moved", | |
| "chainstate error", | |
| // Transparent wrappers mean chainstate failures don't always say | |
| // "chainstate error" on the wire; match the concrete | |
| // chainstate::ChainstateError Display prefixes too. | |
| "i/o error", | |
| "block storage error", | |
| "initialization error", | |
| "block processing failed", | |
| "property read error", | |
| "bootstrap error", | |
| "error invoking block invalidator", | |
| "subsystem call error", |
|
All findings from the 17:25, 17:38, 17:51 and 20:03 review runs are addressed across Highlights:
|
The buffer_unordered probe stream trips the rustc auto-trait leak
("implementation of Send is not general enough") inside the wallet
event-loop future, which breaks tokio_spawn of the wallet worker under
the workspace all-features build. Probes run sequentially again, with a
comment documenting the limitation; concurrency can be revisited if the
RPC surface ever carries structured futures.
| for tx in db_tx.get_user_transactions_for_account(&account.get_account_id())? { | ||
| match account.get_transaction(tx.transaction().get_id()) { |
There was a problem hiding this comment.
This query materializes every signed transaction of the account into a Vec on each call, then filters most of them out by state. Since this feeds the periodic rebroadcast loop, accounts with long histories pay O(history) memory/CPU per tick. Consider a storage-level variant that iterates lazily (the prefix iterator is already available in prefix_iter_decoded) or filters by state, avoiding the intermediate Vec.
| txs.push(tx); | ||
| } | ||
| } | ||
| Err(WalletError::NoTransactionFound(_)) => txs.push(tx), |
There was a problem hiding this comment.
OutputCache::get_transaction returns NoTransactionFound for WalletTx::Block(_) entries too (see output_cache/mod.rs line 1907: None | Some(WalletTx::Block(_)) => Err(WalletError::NoTransactionFound(..))). Block-reward wallet transactions will therefore fall into this Err(WalletError::NoTransactionFound(_)) arm and be pushed as rebroadcast candidates on every tick, even though they are confirmed and never need rebroadcasting. Consider matching on the wallet-tx entry kind (or checking whether the SignedTransaction's tx id equals a block tx id) before re-including it.
Suggestion:
| Err(WalletError::NoTransactionFound(_)) => txs.push(tx), | |
| // Distinguish 'no entry at all' from block-reward txs, which are | |
| // confirmed by construction and must not be rebroadcast candidates. | |
| match db_tx.get_wallet_tx_for_account(...)? { | |
| None => txs.push(tx), | |
| Some(WalletTx::Block(_)) => {} | |
| ... | |
| } |
| let mut skipped: BTreeSet<Id<Transaction>> = | ||
| pending_ids.iter().filter(|id| !probed.contains(*id)).copied().collect(); | ||
| let mut queue: Vec<Id<Transaction>> = skipped.iter().copied().collect(); |
There was a problem hiding this comment.
When a mempool probe fails, transactions are cascaded into skipped only via UTXO-parent children edges. A nonce-successor (account-command chain) of a skipped transaction is still probed and classified by reconcile: if all of its UTXO pending parents happen to be present, the 'parent present, tx missing' signature holds and, combined with an earlier node rejection, its absence streak accumulates toward a prune — even though the real reason it is missing is that its nonce predecessor was skipped, which is a transient, resubmit-able condition. Consider also cascading skips along nonce-successor edges (same (account, nonce+1) map as nonce_of), or excluding transactions with an unresolved nonce predecessor from prune-evidence accumulation.
| || match (account_of.get(tx_id), pending_tx.nonce) { | ||
| (Some(account_index), Some(nonce)) if nonce > 0 => { | ||
| nonce_of.get(&(*account_index, nonce - 1)).is_some_and(|predecessor| { | ||
| // A predecessor missing from `pending_by_id` was | ||
| // skipped this pass (e.g. its probe failed). | ||
| !pending_by_id.contains_key(predecessor) | ||
| || unsubmitted.contains(predecessor) | ||
| }) | ||
| } | ||
| _ => false, | ||
| }; |
There was a problem hiding this comment.
The nonce-chain ordering invariant is not actually guaranteed here. reconcile/topological_order order transactions only by UTXO parent edges; the account nonce is merely a tie-break among txs that happen to be 'ready' in the same batch. So a child with nonce n can be processed before its nonce n-1 predecessor (e.g. the predecessor has a pending UTXO parent and lands in a later ready batch, while the child has none). At that point unsubmitted does not yet contain the predecessor and the check passes, so the child is submitted while its predecessor is still missing from the mempool — exactly the guaranteed rejection this code tries to avoid, burning the child's retry budget (and possibly feeding it to stuck/prune). Consider adding nonce-predecessor edges as real ordering constraints in PendingTx::pending_parents-like structure (or a separate predecessor map consumed by topological_order) instead of relying on the batch tie-break.
Suggestion:
| || match (account_of.get(tx_id), pending_tx.nonce) { | |
| (Some(account_index), Some(nonce)) if nonce > 0 => { | |
| nonce_of.get(&(*account_index, nonce - 1)).is_some_and(|predecessor| { | |
| // A predecessor missing from `pending_by_id` was | |
| // skipped this pass (e.g. its probe failed). | |
| !pending_by_id.contains_key(predecessor) | |
| || unsubmitted.contains(predecessor) | |
| }) | |
| } | |
| _ => false, | |
| }; | |
| let predecessor_not_ready = match (account_of.get(tx_id), pending_tx.nonce) { | |
| (Some(account_index), Some(nonce)) if nonce > 0 => { | |
| nonce_of.get(&(*account_index, nonce - 1)).is_some_and(|predecessor| { | |
| // The predecessor must already have been handled this pass; | |
| // if it is still queued (neither submitted nor unsubmitted), | |
| // the topological order did not honor the nonce chain. | |
| !pending_by_id.contains_key(predecessor) | |
| || unsubmitted.contains(predecessor) | |
| }) | |
| } | |
| _ => false, | |
| }; |
| TxValidationError::TxValidation(connect_error) => !matches!( | ||
| connect_error, | ||
| ConnectTransactionError::MissingOutputOrSpent(_) |
There was a problem hiding this comment.
The typed classifier defaults to true (deterministic rejection) for every currently-unlisted variant in TxValidationError::TxValidation, Policy, and Orphan. This is the opposite of the 'safe under-pruning' default documented in the is_node_rejection trait: any variant added to these enums in the future will be silently classified as a deterministic rejection and may get a pending transaction pruned on a transient outcome. The variant-count test mitigates this, but only if someone notices. Prefer an explicit allowlist of known-deterministic variants (default false), mirroring the wire classifier's allowlist-by-prefix structure, so new variants fail safe.
Suggestion:
| TxValidationError::TxValidation(connect_error) => !matches!( | |
| connect_error, | |
| ConnectTransactionError::MissingOutputOrSpent(_) | |
| // Deterministic verdicts must be explicitly allowlisted; unknown variants | |
| // default to `false` (under-pruning) so newly added variants are safe. | |
| TxValidationError::TxValidation(connect_error) => matches!( | |
| connect_error, | |
| ConnectTransactionError::AttemptToSpendBurnedAmount | /* ... */ | |
| ), |
2 participants
Summary
Replaces the wallet's blind rebroadcast loop (every 2–5 minutes, resubmitting the whole user-transaction table in hash order, unbounded) with a chain-aware reconcile-and-rebroadcast pass that:
mempool_get_transactionRPC (no node upgrade required); a failed probe only excludes that transaction and its descendants from the pass;PRUNE_ABSENCE_THRESHOLD(3) consecutive passes in which the transaction was actually due, is removed from the spendable cache and the rebroadcast set together with its pending descendants. Rejection evidence expires afterREJECTION_EVIDENCE_MAX_AGE(1 h), so a single historic rejection cannot support a much later prune. Once evidence is established, a transaction is never resubmitted — a failed wallet-side prune is retried on the next pass. Transactions that were never delivered (transport errors) or previously succeeded are resubmitted instead of pruned;transaction_abandon) or the wallet restarts. Delivery failures do not burn the rejection budget; the pass is re-timed to the earliest due attempt so the documented backoff schedule is honored;strum::EnumCountvariant-count pins that fail CI when an error enum gains a variant;The planner, tracker and reconciliation logic are pure functions (
wallet-controller/src/rebroadcast.rs) with unit tests; the controller run loop is the imperative shell.Supporting changes
get_user_transactions_for_account(a scoped read over the existingDBUserTxkeyspace; no schema change);get_unconfirmed_transactions_per_account— filtered to transactions that may still need a (re)broadcast; confirmed, conflicted and abandoned states are excluded, unknown state is kept;prune_dead_transaction, a variant ofabandon_transactionthat tolerates a staleInMempoolstate (used when the mempool evidence says the transaction is dead);abandon_transactionbehavior is unchanged;NodeInterfaceError::is_node_rejection(defaultfalse= the safe under-pruning choice): the handles client classifies by typedmempool::errorvariants; the RPC client requires theCALL_EXECUTION_FAILED(−32000) code plus the wire-message mirror of the same decision;strum::EnumCountderives on the mempool error enums (test-only consumer; no behavior change).No consensus surface is touched; no wallet DB migration is needed.
Tests
abandon_transactionstill rejects stale in-mempool state; pruning rejects confirmed/abandoned targets (exact error variants asserted);wallet,wallet-controller,node-command end-to-endwallet-rpc-libsuites green.Review history
The branch went through four OpenCodeReview rounds after the initial push; every finding is addressed and mapped to its resolution in the comments below (see especially: stuck-parent deferral, typed classification replacing substring matching, rejection-evidence recency, pass re-timing, nonce-chain dependencies, established-evidence prune retries). The final two review runs reported zero findings. Concurrent mempool probing was attempted and reverted:
buffer_unorderedover the async-trait probes trips the rustc auto-trait leak (Sendnot general enough) in the wallet worker's event loop.