From 2a948a603c4906743bbb95cab138b1f13f20edaf Mon Sep 17 00:00:00 2001 From: Bas van Dijk Date: Tue, 14 Jul 2026 09:41:03 +0000 Subject: [PATCH 1/5] fix: deflake //rs/bitcoin/adapter:adapter_integration_test The doge_test_send_tx sub-test flakes because the adapter's transaction relay is a one-shot: a transaction is advertised (inv) to each peer exactly once, the mark is set even if the send fails, and it survives a connection discard + reconnect. If any step of the unacknowledged inv -> getdata -> tx handshake is lost (connection churn / CPU contention on CI), the transaction is silently never relayed and Bob's balance stays 0. Fix: track when each peer was last advertised to and re-advertise every TX_READVERTISE_PERIOD_SECS (10s) for as long as the transaction stays in the cache. Peers that already received the transaction simply ignore the repeated inv. Co-Authored-By: Claude Fable 5 --- rs/bitcoin/adapter/src/transaction_store.rs | 110 +++++++++++++++----- 1 file changed, 86 insertions(+), 24 deletions(-) diff --git a/rs/bitcoin/adapter/src/transaction_store.rs b/rs/bitcoin/adapter/src/transaction_store.rs index 97ff189d512d..6b7c446c72ef 100644 --- a/rs/bitcoin/adapter/src/transaction_store.rs +++ b/rs/bitcoin/adapter/src/transaction_store.rs @@ -1,4 +1,4 @@ -use std::collections::HashSet; +use std::collections::HashMap; use std::net::SocketAddr; use std::{time::Duration, time::SystemTime}; @@ -18,6 +18,14 @@ use crate::{Channel, Command}; /// How long should the transaction manager hold on to a transaction. const TX_CACHE_TIMEOUT_PERIOD_SECS: u64 = 10 * 60; // 10 minutes +/// How long to wait before re-advertising a transaction to a peer. +/// The `inv → getdata → tx` exchange is not acknowledged, so any of its messages can be +/// lost without a trace, e.g. when the connection to the peer is dropped and +/// re-established. Re-advertising until the transaction leaves the cache makes the relay +/// robust against such losses. Peers that already received the transaction simply ignore +/// the repeated `inv`. +const TX_READVERTISE_PERIOD_SECS: u64 = 10; + /// Maximum number of transaction to advertise. // https://developer.bitcoin.org/reference/p2p_networking.html#inv const MAXIMUM_TRANSACTION_PER_INV: usize = 50_000; @@ -36,12 +44,13 @@ const TX_CACHE_SIZE: usize = 250; struct TransactionInfo { /// The actual transaction to be sent to the BTC network. transaction: Transaction, - /// Set of peer to which we advertised this transaction. - /// Having a peer in this set doesn't guarantee that the peer actually saw the transaction. - /// If the connection is healthy during sending most likely the peer will see the transaction. - /// The adapter maintains a pool of connected peers, so it is unlikely that - /// the transaction won't be seen by at least a few peers. - advertised: HashSet, + /// When this transaction was last advertised to each peer. + /// Having a peer in this map doesn't guarantee that the peer actually saw the + /// transaction: the advertisement or the follow-up `getdata`/`tx` exchange may be + /// lost, e.g. when the connection is dropped and re-established. Therefore the + /// transaction is re-advertised every [TX_READVERTISE_PERIOD_SECS] for as long as + /// it stays in the cache. + advertised: HashMap, /// How long the transaction should be held on to. /// This is needed in order to be able to reply to GetData requests. ttl: SystemTime, @@ -52,7 +61,7 @@ impl TransactionInfo { fn new(transaction: &Transaction) -> Self { Self { transaction: transaction.clone(), - advertised: HashSet::new(), + advertised: HashMap::new(), ttl: SystemTime::now() + Duration::from_secs(TX_CACHE_TIMEOUT_PERIOD_SECS), } } @@ -123,16 +132,22 @@ impl TransactionStore { } /// This method is used to broadcast known transaction IDs to connected peers. - /// If the timeout period has passed for a transaction ID, it is broadcasted again. - /// If the transaction has not been broadcasted, the transaction ID is broadcasted. + /// A transaction ID is broadcasted to a peer if it has never been advertised to that + /// peer, or if the last advertisement was more than [TX_READVERTISE_PERIOD_SECS] ago. pub fn advertise_txids(&mut self, channel: &mut impl Channel) { self.remove_old_txns(); + let now = SystemTime::now(); + let readvertise_period = Duration::from_secs(TX_READVERTISE_PERIOD_SECS); for address in channel.available_connections() { let mut inventory = vec![]; for (txid, info) in self.transactions.iter_mut() { - if !info.advertised.contains(&address) { + let advertise = match info.advertised.get(&address) { + Some(last_advertised) => *last_advertised + readvertise_period <= now, + None => true, + }; + if advertise { inventory.push(Inventory::Transaction(*txid)); - info.advertised.insert(address); + info.advertised.insert(address, now); } // If the inventory contains the maximum allowed number of transactions, we will send it // and start building a new one. @@ -141,14 +156,12 @@ impl TransactionStore { self.logger, "Broadcasting Txids ({:?}) to peer {:?}", inventory, address ); - for address in channel.available_connections() { - channel - .send(Command { - address: Some(address), - message: NetworkMessage::Inv(std::mem::take(&mut inventory)), - }) - .ok(); - } + channel + .send(Command { + address: Some(address), + message: NetworkMessage::Inv(std::mem::take(&mut inventory)), + }) + .ok(); } } if !inventory.is_empty() { @@ -325,7 +338,7 @@ mod test { assert!(manager.transactions.get(&first_tx.compute_txid()).is_none()); } - /// This function tests that we don't readvertise transactions that were already advertised. + /// This function tests that we don't readvertise transactions that were recently advertised. /// Test Steps: /// 1. Add transaction to manager. /// 2. Advertise that transaction and create requests from peer. @@ -368,14 +381,63 @@ mod test { .len(), 1 ); - assert_eq!( + assert!( manager .transactions .get(&transaction.compute_txid()) .unwrap() .advertised - .get(&address), - Some(&address) + .contains_key(&address) + ); + } + + /// This function tests that a transaction is re-advertised to a peer once the + /// re-advertisement period has elapsed. + /// Test Steps: + /// 1. Add transaction to manager. + /// 2. Advertise that transaction. + /// 3. Check that this transaction does not get advertised again during manager tick. + /// 4. Backdate the advertisement to more than [TX_READVERTISE_PERIOD_SECS] ago. + /// 5. Check that the transaction is advertised to the peer again. + #[test] + fn test_adapter_readvertise_after_period() { + let address = SocketAddr::from_str("127.0.0.1:8333").expect("invalid address"); + let mut channel = TestChannel::new(vec![address]); + let mut manager = make_transaction_manager(); + + // 1. + let transaction = get_transaction(); + let raw_tx = serialize(&transaction); + let txid = transaction.compute_txid(); + manager.enqueue_transaction(&raw_tx); + + // 2. + manager.advertise_txids(&mut channel); + assert_eq!(channel.command_count(), 1); + channel.pop_front().unwrap(); + + // 3. + manager.advertise_txids(&mut channel); + assert_eq!(channel.command_count(), 0); + + // 4. + let info = manager + .transactions + .get_mut(&txid) + .expect("transaction should be in the map"); + info.advertised.insert( + address, + SystemTime::now() - Duration::from_secs(TX_READVERTISE_PERIOD_SECS), + ); + + // 5. + manager.advertise_txids(&mut channel); + assert_eq!( + channel.pop_front().expect("there should be one command"), + Command { + address: Some(address), + message: NetworkMessage::Inv(vec![Inventory::Transaction(txid)]) + } ); } From eedb5727d9f4b99b5de06e9bb350c2f4e0e87079 Mon Sep 17 00:00:00 2001 From: Bas van Dijk Date: Tue, 14 Jul 2026 09:43:35 +0000 Subject: [PATCH 2/5] trigger rebuild From 5b0071819d4cfe2335572ebad2771d82f32713d2 Mon Sep 17 00:00:00 2001 From: Bas van Dijk Date: Tue, 14 Jul 2026 09:49:18 +0000 Subject: [PATCH 3/5] docs: clarify inclusive re-advertisement boundary in doc comments Addresses Copilot review comments: the implementation checks `last_advertised + period <= now`, so the docs should say "at least" rather than "more than", and the unit test backdates the advertisement to exactly the period. Co-Authored-By: Claude Fable 5 --- rs/bitcoin/adapter/src/transaction_store.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/rs/bitcoin/adapter/src/transaction_store.rs b/rs/bitcoin/adapter/src/transaction_store.rs index 6b7c446c72ef..8fa530dde81d 100644 --- a/rs/bitcoin/adapter/src/transaction_store.rs +++ b/rs/bitcoin/adapter/src/transaction_store.rs @@ -133,7 +133,7 @@ impl TransactionStore { /// This method is used to broadcast known transaction IDs to connected peers. /// A transaction ID is broadcasted to a peer if it has never been advertised to that - /// peer, or if the last advertisement was more than [TX_READVERTISE_PERIOD_SECS] ago. + /// peer, or if the last advertisement was at least [TX_READVERTISE_PERIOD_SECS] ago. pub fn advertise_txids(&mut self, channel: &mut impl Channel) { self.remove_old_txns(); let now = SystemTime::now(); @@ -397,7 +397,8 @@ mod test { /// 1. Add transaction to manager. /// 2. Advertise that transaction. /// 3. Check that this transaction does not get advertised again during manager tick. - /// 4. Backdate the advertisement to more than [TX_READVERTISE_PERIOD_SECS] ago. + /// 4. Backdate the advertisement to exactly [TX_READVERTISE_PERIOD_SECS] ago, + /// the (inclusive) re-advertisement boundary. /// 5. Check that the transaction is advertised to the peer again. #[test] fn test_adapter_readvertise_after_period() { From 1789ac901035f4614b34238998f6ed1e821097bb Mon Sep 17 00:00:00 2001 From: Bas van Dijk Date: Tue, 14 Jul 2026 10:32:19 +0000 Subject: [PATCH 4/5] fix: use a monotonic clock for the re-advertisement schedule Addresses a Copilot review comment: track when a transaction was last advertised to each peer with Instant instead of SystemTime, so that wall-clock jumps (NTP corrections, VM clock adjustments) cannot perturb the re-advertisement schedule. Co-Authored-By: Claude Fable 5 --- rs/bitcoin/adapter/src/transaction_store.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/rs/bitcoin/adapter/src/transaction_store.rs b/rs/bitcoin/adapter/src/transaction_store.rs index 8fa530dde81d..108f113e7a9f 100644 --- a/rs/bitcoin/adapter/src/transaction_store.rs +++ b/rs/bitcoin/adapter/src/transaction_store.rs @@ -1,6 +1,6 @@ use std::collections::HashMap; use std::net::SocketAddr; -use std::{time::Duration, time::SystemTime}; +use std::time::{Duration, Instant, SystemTime}; use bitcoin::consensus::deserialize; use bitcoin::{ @@ -50,7 +50,9 @@ struct TransactionInfo { /// lost, e.g. when the connection is dropped and re-established. Therefore the /// transaction is re-advertised every [TX_READVERTISE_PERIOD_SECS] for as long as /// it stays in the cache. - advertised: HashMap, + /// [Instant] is used instead of [SystemTime] because the re-advertisement schedule + /// must be immune to wall-clock jumps (NTP corrections, VM clock adjustments). + advertised: HashMap, /// How long the transaction should be held on to. /// This is needed in order to be able to reply to GetData requests. ttl: SystemTime, @@ -136,7 +138,7 @@ impl TransactionStore { /// peer, or if the last advertisement was at least [TX_READVERTISE_PERIOD_SECS] ago. pub fn advertise_txids(&mut self, channel: &mut impl Channel) { self.remove_old_txns(); - let now = SystemTime::now(); + let now = Instant::now(); let readvertise_period = Duration::from_secs(TX_READVERTISE_PERIOD_SECS); for address in channel.available_connections() { let mut inventory = vec![]; @@ -428,7 +430,7 @@ mod test { .expect("transaction should be in the map"); info.advertised.insert( address, - SystemTime::now() - Duration::from_secs(TX_READVERTISE_PERIOD_SECS), + Instant::now() - Duration::from_secs(TX_READVERTISE_PERIOD_SECS), ); // 5. From 61825bcca41c420037eb38a7b68d6439777b0b0b Mon Sep 17 00:00:00 2001 From: Bas van Dijk Date: Tue, 14 Jul 2026 10:53:36 +0000 Subject: [PATCH 5/5] test: avoid Instant underflow panic when backdating advertisement Addresses a Copilot review comment: `Instant - Duration` panics if the result would precede the monotonic clock's epoch (e.g. system boot). Use `checked_sub` and skip the test in that (practically unreachable) case. Co-Authored-By: Claude Fable 5 --- rs/bitcoin/adapter/src/transaction_store.rs | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/rs/bitcoin/adapter/src/transaction_store.rs b/rs/bitcoin/adapter/src/transaction_store.rs index 108f113e7a9f..5cfe0171efc2 100644 --- a/rs/bitcoin/adapter/src/transaction_store.rs +++ b/rs/bitcoin/adapter/src/transaction_store.rs @@ -400,7 +400,9 @@ mod test { /// 2. Advertise that transaction. /// 3. Check that this transaction does not get advertised again during manager tick. /// 4. Backdate the advertisement to exactly [TX_READVERTISE_PERIOD_SECS] ago, - /// the (inclusive) re-advertisement boundary. + /// the (inclusive) re-advertisement boundary. The test is skipped if the + /// monotonic clock cannot represent that instant (i.e. the system booted + /// less than [TX_READVERTISE_PERIOD_SECS] ago). /// 5. Check that the transaction is advertised to the peer again. #[test] fn test_adapter_readvertise_after_period() { @@ -424,14 +426,16 @@ mod test { assert_eq!(channel.command_count(), 0); // 4. + let Some(backdated) = + Instant::now().checked_sub(Duration::from_secs(TX_READVERTISE_PERIOD_SECS)) + else { + return; + }; let info = manager .transactions .get_mut(&txid) .expect("transaction should be in the map"); - info.advertised.insert( - address, - Instant::now() - Duration::from_secs(TX_READVERTISE_PERIOD_SECS), - ); + info.advertised.insert(address, backdated); // 5. manager.advertise_txids(&mut channel);