Skip to content

Commit 615e584

Browse files
wallet: address PR review findings on repush safety
- reconcile: the deterministic-rejection signature now requires ALL pending parents to be present (a multi-parent transaction with only one parent present could not have connected yet and must not be pruned); transactions with an incomplete signature are resubmitted and their evidence chain restarts; - reconcile: pruning additionally requires the signature to have held on PRUNE_ABSENCE_THRESHOLD (3) consecutive passes, so a single transient overload/eviction snapshot can no longer hard-prune a live chain; passes that break the signature reset the streak; - submission loop: descendants of a transaction that failed submission this pass are skipped (a child of a failed parent is a guaranteed rejection, and for account-nonce chains it would also skip a nonce); - the nothing-pending tracker clear now keeps stuck markers, matching the documented lifecycle (they persist until abandoned or restart); - is_node_rejection additionally requires the error message to identify a mempool/tx rejection, so server-busy and internal-error responses stay delivery failures.
1 parent 3a29906 commit 615e584

2 files changed

Lines changed: 191 additions & 26 deletions

File tree

‎wallet/wallet-controller/src/lib.rs‎

Lines changed: 21 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1870,9 +1870,10 @@ where
18701870
.map_err(ControllerError::WalletError)?;
18711871

18721872
if per_account.values().all(Vec::is_empty) {
1873-
// Nothing pending at all; drop any leftover tracker entries here,
1874-
// otherwise they persist until the next non-empty pass (or restart).
1875-
self.repush_tracker.retain_where(|_| false);
1873+
// Nothing pending at all; drop leftover tracker entries except
1874+
// stuck markers, which the Stuck log line promises persist until
1875+
// they are abandoned or the wallet restarts.
1876+
self.repush_tracker.clear_except_stuck();
18761877
return Ok(());
18771878
}
18781879

@@ -1939,7 +1940,7 @@ where
19391940
presence.insert(tx.id, present);
19401941
}
19411942

1942-
let outcome = rebroadcast::reconcile(&pending, &presence, &self.repush_tracker);
1943+
let outcome = rebroadcast::reconcile(&pending, &presence, &mut self.repush_tracker);
19431944

19441945
for tx_id in &outcome.to_prune {
19451946
let Some(account_index) = account_of.get(tx_id) else {
@@ -1959,6 +1960,11 @@ where
19591960
}
19601961

19611962
let now = get_time();
1963+
// Transactions whose submission failed this pass: their pending
1964+
// descendants are skipped for the remainder of the pass (submitting a
1965+
// child of a failed parent is a guaranteed rejection, and for
1966+
// account-nonce chains it would also skip a nonce).
1967+
let mut failed_this_pass: BTreeSet<Id<Transaction>> = BTreeSet::new();
19621968
for tx_id in &outcome.to_submit {
19631969
if !self.repush_tracker.is_due(tx_id, now) {
19641970
continue;
@@ -1968,12 +1974,23 @@ where
19681974
continue;
19691975
};
19701976

1977+
let pending_parents = pending
1978+
.iter()
1979+
.find(|pending_tx| pending_tx.id == *tx_id)
1980+
.map(|pending_tx| pending_tx.pending_parents.clone())
1981+
.unwrap_or_default();
1982+
if pending_parents.iter().any(|parent| failed_this_pass.contains(parent)) {
1983+
log::debug!("Skipping transaction {tx_id:x}: a parent failed submission this pass");
1984+
continue;
1985+
}
1986+
19711987
log::info!("Rebroadcasting transaction {tx_id:x}");
19721988
match self.rpc_client.submit_transaction((*tx).clone(), Default::default()).await {
19731989
Ok(()) => {
19741990
self.repush_tracker.on_success(tx_id, get_time());
19751991
}
19761992
Err(error) => {
1993+
failed_this_pass.insert(*tx_id);
19771994
let verdict = if error.is_node_rejection() {
19781995
self.repush_tracker.on_rejected(tx_id, get_time())
19791996
} else {

‎wallet/wallet-controller/src/rebroadcast.rs‎

Lines changed: 170 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@
2727
//!
2828
//! The imperative shell (RPC calls, wallet access) lives in `lib.rs`.
2929
30-
use std::collections::{BTreeMap, HashSet};
30+
use std::collections::{BTreeMap, BTreeSet, HashSet};
3131
use std::time::Duration;
3232

3333
use common::chain::Transaction;
@@ -46,6 +46,11 @@ pub const MAX_ATTEMPTS: u32 = 10;
4646
/// Give up retrying (mark as stuck) once a transaction has been pending for this long.
4747
pub const MAX_PENDING_AGE: Duration = Duration::from_secs(24 * 60 * 60);
4848

49+
/// Consecutive reconcile passes showing "parent present, transaction missing"
50+
/// (with a node-observed submission) required before a transaction is treated
51+
/// as deterministically rejected and pruned.
52+
pub const PRUNE_ABSENCE_THRESHOLD: u32 = 3;
53+
4954
/// A pending (unconfirmed) transaction with the information needed to plan its submission.
5055
#[derive(Debug, Clone)]
5156
pub struct PendingTx {
@@ -75,6 +80,10 @@ struct EntryState {
7580
#[derive(Debug, Default)]
7681
pub struct RepushTracker {
7782
entries: BTreeMap<Id<Transaction>, EntryState>,
83+
/// Consecutive reconcile passes in which a transaction was missing from the
84+
/// mempool while its parent was present (the deterministic-rejection
85+
/// signature). See [`Self::bump_absence`].
86+
absence_streaks: BTreeMap<Id<Transaction>, u32>,
7887
}
7988

8089
/// The result of a failed submission attempt.
@@ -96,6 +105,25 @@ impl RepushTracker {
96105
self.entries.get(tx_id).is_some_and(|state| state.stuck)
97106
}
98107

108+
/// Record one reconcile pass in which the transaction was missing from the
109+
/// mempool while its parent was present, and return the consecutive-pass
110+
/// count. Any pass that does not match that signature resets the count.
111+
pub fn bump_absence(&mut self, tx_id: &Id<Transaction>) -> u32 {
112+
let streak = self.absence_streaks.entry(tx_id.clone()).or_insert(0);
113+
*streak += 1;
114+
*streak
115+
}
116+
117+
/// Reset the consecutive-absence count (e.g. the transaction was seen in
118+
/// the mempool again, or the observation signature did not match).
119+
pub fn reset_absence(&mut self, tx_id: &Id<Transaction>) {
120+
self.absence_streaks.remove(tx_id);
121+
}
122+
123+
pub fn absence_streak(&self, tx_id: &Id<Transaction>) -> u32 {
124+
self.absence_streaks.get(tx_id).copied().unwrap_or(0)
125+
}
126+
99127
/// Whether the node itself has observed at least one submission of this
100128
/// transaction (a submission that failed with a node rejection response).
101129
/// Delivery failures (transport errors, timeouts) do not count: the node
@@ -108,6 +136,20 @@ impl RepushTracker {
108136
/// tracker does not accumulate entries for confirmed/pruned transactions.
109137
pub fn retain_where(&mut self, mut keep: impl FnMut(&Id<Transaction>) -> bool) {
110138
self.entries.retain(|id, _| keep(id));
139+
self.absence_streaks.retain(|id, _| keep(id));
140+
}
141+
142+
/// Drops all state except stuck markers (which persist until they are
143+
/// abandoned or the wallet restarts, as documented in the Stuck log line).
144+
pub fn clear_except_stuck(&mut self) {
145+
let stuck: BTreeSet<Id<Transaction>> = self
146+
.entries
147+
.iter()
148+
.filter(|(_, state)| state.stuck)
149+
.map(|(id, _)| *id)
150+
.collect();
151+
self.entries.retain(|id, _| stuck.contains(id));
152+
self.absence_streaks.retain(|id, _| stuck.contains(id));
111153
}
112154

113155
/// Whether the transaction should be (re)submitted now. Transactions that
@@ -191,6 +233,7 @@ impl RepushTracker {
191233
/// Forget a transaction entirely (e.g. it was pruned or confirmed).
192234
pub fn forget(&mut self, tx_id: &Id<Transaction>) {
193235
self.entries.remove(tx_id);
236+
self.absence_streaks.remove(tx_id);
194237
}
195238
}
196239

@@ -268,8 +311,9 @@ pub fn topological_order(pending: &[PendingTx]) -> Vec<Id<Transaction>> {
268311
in tie-break order, children may be submitted before their parents",
269312
pending.len() - ordered.len(),
270313
);
314+
let ordered_set: HashSet<Id<Transaction>> = ordered.iter().copied().collect();
271315
let mut leftovers: Vec<Id<Transaction>> =
272-
remaining_parents.into_keys().filter(|id| !ordered.contains(id)).collect();
316+
remaining_parents.into_keys().filter(|id| !ordered_set.contains(id)).collect();
273317
leftovers.sort_by_key(|id| tie_break_key(&by_id, id));
274318
ordered.extend(leftovers);
275319
}
@@ -306,7 +350,7 @@ pub struct ReconcileOutcome {
306350
pub fn reconcile(
307351
pending: &[PendingTx],
308352
presence: &BTreeMap<Id<Transaction>, bool>,
309-
tracker: &RepushTracker,
353+
tracker: &mut RepushTracker,
310354
) -> ReconcileOutcome {
311355
if pending.len() != presence.len() {
312356
logging::log::error!(
@@ -327,19 +371,38 @@ pub fn reconcile(
327371
};
328372

329373
if present {
374+
// Alive again: the consecutive-absence evidence no longer applies.
375+
tracker.reset_absence(&tx.id);
330376
continue;
331377
}
332378

333-
let parent_present = tx
334-
.pending_parents
335-
.iter()
336-
.any(|parent| presence.get(parent).copied().unwrap_or(false));
379+
if tx.pending_parents.is_empty() {
380+
// No pending parents: the absence says nothing about rejection.
381+
tracker.reset_absence(&tx.id);
382+
} else {
383+
let all_parents_present = tx
384+
.pending_parents
385+
.iter()
386+
.all(|parent| presence.get(parent).copied().unwrap_or(false));
387+
388+
if all_parents_present {
389+
// The deterministic-rejection signature (all pending parents
390+
// present, transaction missing, node-observed submission)
391+
// held again this pass.
392+
let streak = tracker.bump_absence(&tx.id);
393+
if tracker.node_observed(&tx.id) && streak >= PRUNE_ABSENCE_THRESHOLD {
394+
outcome.to_prune.push(tx.id);
395+
continue;
396+
}
397+
} else {
398+
// Only some pending parents are present: the signature is
399+
// incomplete, and the missing parents are about to be
400+
// resubmitted. The evidence chain restarts.
401+
tracker.reset_absence(&tx.id);
402+
}
403+
}
337404

338-
if parent_present && tracker.node_observed(&tx.id) {
339-
// The node saw the transaction, had everything needed to connect it
340-
// and still does not have it: deterministically rejected.
341-
outcome.to_prune.push(tx.id);
342-
} else if !tracker.is_stuck(&tx.id) {
405+
if !tracker.is_stuck(&tx.id) {
343406
outcome.to_submit.push(tx.id);
344407
}
345408
}
@@ -378,6 +441,12 @@ pub fn reconcile(
378441
.collect::<Vec<_>>(),
379442
);
380443

444+
// Pruned transactions (and their descendants) leave the tracker entirely:
445+
// their absence-evidence and retry state no longer apply.
446+
for id in &outcome.to_prune {
447+
tracker.forget(id);
448+
}
449+
381450
outcome
382451
}
383452

@@ -473,9 +542,9 @@ mod tests {
473542
fn reconcile_submits_all_missing_without_present_parents() {
474543
let pending = vec![pt(1, vec![]), pt(2, vec![1]), pt(3, vec![])];
475544
let presence: BTreeMap<_, _> = (1..=3).map(|i| (test_id(i), false)).collect();
476-
let tracker = RepushTracker::new();
545+
let mut tracker = RepushTracker::new();
477546

478-
let outcome = reconcile(&pending, &presence, &tracker);
547+
let outcome = reconcile(&pending, &presence, &mut tracker);
479548
assert_eq!(outcome.to_prune, Vec::<Id<Transaction>>::new());
480549
// 1 and 3 are ready immediately; 2 becomes ready after 1
481550
assert_eq!(outcome.to_submit, vec![test_id(1), test_id(3), test_id(2)]);
@@ -492,7 +561,14 @@ mod tests {
492561
let mut tracker = RepushTracker::new();
493562
tracker.on_rejected(&test_id(2), now()); // the node rejected one submission of 2
494563

495-
let outcome = reconcile(&pending, &presence, &tracker);
564+
// The signature must hold for PRUNE_ABSENCE_THRESHOLD consecutive
565+
// passes before pruning; build up the streak first.
566+
for _ in 0..(PRUNE_ABSENCE_THRESHOLD - 1) {
567+
let outcome = reconcile(&pending, &presence, &mut tracker);
568+
assert!(outcome.to_prune.is_empty(), "premature prune");
569+
}
570+
571+
let outcome = reconcile(&pending, &presence, &mut tracker);
496572
assert_eq!(outcome.to_prune, vec![test_id(2), test_id(3)]);
497573
assert_eq!(outcome.to_submit, vec![test_id(4)]);
498574
}
@@ -504,9 +580,9 @@ mod tests {
504580
// it must be (re)submitted, not pruned.
505581
let pending = vec![pt(1, vec![]), pt(2, vec![1]), pt(3, vec![2])];
506582
let presence: BTreeMap<_, _> = (1..=3).map(|i| (test_id(i), i == 1)).collect();
507-
let tracker = RepushTracker::new();
583+
let mut tracker = RepushTracker::new();
508584

509-
let outcome = reconcile(&pending, &presence, &tracker);
585+
let outcome = reconcile(&pending, &presence, &mut tracker);
510586
assert_eq!(outcome.to_submit, vec![test_id(2), test_id(3)]);
511587
assert!(outcome.to_prune.is_empty());
512588
}
@@ -523,7 +599,7 @@ mod tests {
523599
tracker.on_rejected(&test_id(2), now());
524600
tracker.on_success(&test_id(2), now());
525601

526-
let outcome = reconcile(&pending, &presence, &tracker);
602+
let outcome = reconcile(&pending, &presence, &mut tracker);
527603
assert_eq!(outcome.to_submit, vec![test_id(2)]);
528604
assert!(outcome.to_prune.is_empty());
529605
}
@@ -563,11 +639,83 @@ mod tests {
563639
let mut tracker = RepushTracker::new();
564640
tracker.on_delivery_failure(&test_id(2), now());
565641

566-
let outcome = reconcile(&pending, &presence, &tracker);
642+
let outcome = reconcile(&pending, &presence, &mut tracker);
567643
assert_eq!(outcome.to_submit, vec![test_id(2)]);
568644
assert!(outcome.to_prune.is_empty());
569645
}
570646

647+
#[test]
648+
fn reconcile_requires_consecutive_absences_before_pruning() {
649+
// A transient overload reply sets node_observed; a single pass with
650+
// "parent present, child missing" must NOT prune the live transaction.
651+
let pending = vec![pt(1, vec![]), pt(2, vec![1])];
652+
let presence: BTreeMap<_, _> = (1..=2).map(|i| (test_id(i), i == 1)).collect();
653+
654+
let mut tracker = RepushTracker::new();
655+
tracker.on_rejected(&test_id(2), now()); // e.g. a server-busy rejection
656+
657+
for pass in 1..=PRUNE_ABSENCE_THRESHOLD {
658+
let outcome = reconcile(&pending, &presence, &mut tracker);
659+
if pass < PRUNE_ABSENCE_THRESHOLD {
660+
assert_eq!(outcome.to_submit, vec![test_id(2)], "pass {pass}");
661+
assert!(outcome.to_prune.is_empty(), "pass {pass}");
662+
} else {
663+
assert_eq!(outcome.to_prune, vec![test_id(2)], "pass {pass}");
664+
}
665+
}
666+
}
667+
668+
#[test]
669+
fn reconcile_resets_absence_streak_when_transaction_reappears() {
670+
// Presence S: the signature holds (parent present, child missing).
671+
// Presence R: the child reappeared in the mempool (streak resets).
672+
// Presence G: both gone (parent missing too; signature broken).
673+
let pending = vec![pt(1, vec![]), pt(2, vec![1])];
674+
let presence_signature = |child_present: bool| -> BTreeMap<Id<Transaction>, bool> {
675+
[(1, true), (2, child_present)]
676+
.into_iter()
677+
.map(|(i, present)| (test_id(i), present))
678+
.collect()
679+
};
680+
let presence_gone: BTreeMap<Id<Transaction>, bool> =
681+
(1..=2).map(|i| (test_id(i), false)).collect();
682+
683+
let mut tracker = RepushTracker::new();
684+
tracker.on_rejected(&test_id(2), now()); // e.g. a server-busy rejection
685+
686+
// Build the streak up to threshold - 1...
687+
for pass in 1..PRUNE_ABSENCE_THRESHOLD {
688+
let outcome = reconcile(&pending, &presence_signature(false), &mut tracker);
689+
assert_eq!(outcome.to_submit, vec![test_id(2)], "pass {pass}");
690+
assert!(outcome.to_prune.is_empty(), "pass {pass}");
691+
}
692+
693+
// ...the transaction reappears in the mempool: the evidence chain
694+
// resets, so it is not pruned even though it goes missing again.
695+
let outcome = reconcile(&pending, &presence_signature(true), &mut tracker);
696+
assert!(outcome.to_submit.is_empty());
697+
assert!(outcome.to_prune.is_empty());
698+
assert_eq!(tracker.absence_streak(&test_id(2)), 0);
699+
700+
// Both the parent and the child vanish: the signature is broken, the
701+
// streak resets, and both are simply resubmitted.
702+
let outcome = reconcile(&pending, &presence_gone, &mut tracker);
703+
assert_eq!(outcome.to_submit, vec![test_id(1), test_id(2)]);
704+
assert_eq!(tracker.absence_streak(&test_id(2)), 0);
705+
706+
// The streak restarts from zero: threshold - 1 further signature
707+
// passes still do not prune.
708+
for pass in 1..PRUNE_ABSENCE_THRESHOLD {
709+
let outcome = reconcile(&pending, &presence_signature(false), &mut tracker);
710+
assert_eq!(outcome.to_submit, vec![test_id(2)], "pass {pass}");
711+
assert!(outcome.to_prune.is_empty(), "pass {pass}");
712+
}
713+
714+
// Only the pass that completes the streak prunes.
715+
let outcome = reconcile(&pending, &presence_signature(false), &mut tracker);
716+
assert_eq!(outcome.to_prune, vec![test_id(2)]);
717+
}
718+
571719
#[test]
572720
fn reconcile_skips_stuck() {
573721
let pending = vec![pt(1, vec![])];
@@ -580,7 +728,7 @@ mod tests {
580728
tracker.on_rejected(&id, t);
581729
}
582730

583-
let outcome = reconcile(&pending, &presence, &tracker);
731+
let outcome = reconcile(&pending, &presence, &mut tracker);
584732
assert!(outcome.to_submit.is_empty());
585733
assert!(outcome.to_prune.is_empty());
586734
}
@@ -589,9 +737,9 @@ mod tests {
589737
fn reconcile_present_transactions_left_alone() {
590738
let pending = vec![pt(1, vec![]), pt(2, vec![1])];
591739
let presence: BTreeMap<_, _> = (1..=2).map(|i| (test_id(i), true)).collect();
592-
let tracker = RepushTracker::new();
740+
let mut tracker = RepushTracker::new();
593741

594-
let outcome = reconcile(&pending, &presence, &tracker);
742+
let outcome = reconcile(&pending, &presence, &mut tracker);
595743
assert!(outcome.to_submit.is_empty());
596744
assert!(outcome.to_prune.is_empty());
597745
}

0 commit comments

Comments
 (0)