highperfocused/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2026-05-31 08:13:52 +02:00
Code Issues Packages Projects Releases Wiki Activity

Merge bitcoin/bitcoin#34873: net: fix premature stale flagging of unpicked private broadcast txs

Browse Source 
325afe664d net: delay stale evaluation and expose time_added in private broadcast (Mccalabrese)
999d18ab1c net: introduce TxSendStatus internal state container (Mccalabrese)

Pull request description:

  **Motivation**
  Currently, freshly added transactions in `private_broadcast` are almost immediately flagged and logged as stale by the `resend-stale` job.

  **The Bug**
  `m_transactions` maps a transaction to a `std::vector<SendStatus>`. When `try_emplace` adds a new transaction, this vector is empty. When `GetStale()` runs, `DerivePriority()` evaluates the empty vector and returns a default `Priority` struct where `last_confirmed` evaluates to the Unix Epoch (Jan 1, 1970). The stale checker sees a 50-year-old timestamp and flags it on the next resend-stale cycle.

  **The Fix**
  Rather than modifying the transient `Priority` struct or creating a "Zombie Transaction" edge case by ignoring transactions with 0 picks, this PR modifies the state container:
  * Wraps the `SendStatus` vector in a new `TxSendStatus` struct inside `private_broadcast.h`.
  * `TxSendStatus` automatically captures `time_added` upon emplace.
  * `GetStale()` now checks `p.num_confirmed == 0` to measure age against `time_added` using a new 5-minute `INITIAL_STALE_DURATION` grace period, falling back to `last_confirmed` and the standard 1-minute `STALE_DURATION` once network interaction begins.

  **Additional Polish**
  * Exposed `time_added` via the `getprivatebroadcastinfo`  RPC endpoint so users can see when a transaction entered the queue.
  * Added a dedicated `stale_unpicked_tx` test case and updated `private_broadcast_tests.cpp` to properly mock the passage of time for the new grace period.
  Closes #34862

ACKs for top commit:
  achow101:
    ACK 325afe664d
  andrewtoth:
    ACK 325afe664d
  vasild:
    ACK 325afe664d

Tree-SHA512: b7790aa5468f7c161ed93e99e9a6d8b4db39ff7d6d6a920764afd18825e08d83bc30b3fb0debeb6175730b5d2496c6be67f3be8674be93f4d07b1e77d17b4a14
This commit is contained in:
Ava Chow
2026-04-03 17:46:32 -07:00
parent fa1f4feac4 325afe664d
commit a7c30da1f6
5 changed files with 66 additions and 25 deletions
Download Patch File Download Diff File Expand all files Collapse all files

3
src/net_processing.cpp
View File

@@ -1866,7 +1866,8 @@ std::vector<CTransactionRef> PeerManagerImpl::AbortPrivateBroadcast(const uint25
std::vector<CTransactionRef> removed_txs;
size_t connections_cancelled{0};
for (const auto& [tx, _] : snapshot) {
for (const auto& tx_info : snapshot) {
const CTransactionRef& tx{tx_info.tx};
if (tx->GetHash().ToUint256() != id && tx->GetWitnessHash().ToUint256() != id) continue;
if (const auto peer_acks{m_tx_for_private_broadcast.Remove(tx)}) {
removed_txs.push_back(tx);

35
src/private_broadcast.cpp
View File

@@ -7,9 +7,6 @@
#include <algorithm>
/// If a transaction is not received back from the network for this duration
/// after it is broadcast, then we consider it stale / for rebroadcasting.
static constexpr auto STALE_DURATION{1min};
bool PrivateBroadcast::Add(const CTransactionRef& tx)
EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
@@ -25,7 +22,7 @@ std::optional<size_t> PrivateBroadcast::Remove(const CTransactionRef& tx)
LOCK(m_mutex);
const auto handle{m_transactions.extract(tx)};
if (handle) {
const auto p{DerivePriority(handle.mapped())};
const auto p{DerivePriority(handle.mapped().send_statuses)};
return p.num_confirmed;
}
return std::nullopt;
@@ -39,11 +36,11 @@ std::optional<CTransactionRef> PrivateBroadcast::PickTxForSend(const NodeId& wil
const auto it{std::ranges::max_element(
m_transactions,
[](const auto& a, const auto& b) { return a < b; },
[](const auto& el) { return DerivePriority(el.second); })};
[](const auto& el) { return DerivePriority(el.second.send_statuses); })};
if (it != m_transactions.end()) {
auto& [tx, sent_to]{*it};
sent_to.emplace_back(will_send_to_nodeid, will_send_to_address, NodeClock::now());
auto& [tx, state]{*it};
state.send_statuses.emplace_back(will_send_to_nodeid, will_send_to_address, NodeClock::now());
return tx;
}
@@ -93,12 +90,14 @@ std::vector<CTransactionRef> PrivateBroadcast::GetStale() const
EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
{
LOCK(m_mutex);
const auto stale_time{NodeClock::now() - STALE_DURATION};
const auto now{NodeClock::now()};
std::vector<CTransactionRef> stale;
for (const auto& [tx, send_status] : m_transactions) {
const Priority p{DerivePriority(send_status)};
if (p.last_confirmed < stale_time) {
stale.push_back(tx);
for (const auto& [tx, state] : m_transactions) {
const Priority p{DerivePriority(state.send_statuses)};
if (p.num_confirmed == 0) {
if (state.time_added < now - INITIAL_STALE_DURATION) stale.push_back(tx);
} else {
if (p.last_confirmed < now - STALE_DURATION) stale.push_back(tx);
}
}
return stale;
@@ -111,13 +110,13 @@ std::vector<PrivateBroadcast::TxBroadcastInfo> PrivateBroadcast::GetBroadcastInf
std::vector<TxBroadcastInfo> entries;
entries.reserve(m_transactions.size());
for (const auto& [tx, sent_to] : m_transactions) {
for (const auto& [tx, state] : m_transactions) {
std::vector<PeerSendInfo> peers;
peers.reserve(sent_to.size());
for (const auto& status : sent_to) {
peers.reserve(state.send_statuses.size());
for (const auto& status : state.send_statuses) {
peers.emplace_back(PeerSendInfo{.address = status.address, .sent = status.picked, .received = status.confirmed});
}
entries.emplace_back(TxBroadcastInfo{.tx = tx, .peers = std::move(peers)});
entries.emplace_back(TxBroadcastInfo{.tx = tx, .time_added = state.time_added, .peers = std::move(peers)});
}
return entries;
@@ -141,8 +140,8 @@ std::optional<PrivateBroadcast::TxAndSendStatusForNode> PrivateBroadcast::GetSen
EXCLUSIVE_LOCKS_REQUIRED(m_mutex)
{
AssertLockHeld(m_mutex);
for (auto& [tx, sent_to] : m_transactions) {
for (auto& send_status : sent_to) {
for (auto& [tx, state] : m_transactions) {
for (auto& send_status : state.send_statuses) {
if (send_status.nodeid == nodeid) {
return TxAndSendStatusForNode{.tx = tx, .send_status = send_status};
}

17
src/private_broadcast.h
View File

@@ -29,6 +29,15 @@
class PrivateBroadcast
{
public:
/// If a transaction is not sent to any peer for this duration,
/// then we consider it stale / for rebroadcasting.
static constexpr auto INITIAL_STALE_DURATION{5min};
/// If a transaction is not received back from the network for this duration
/// after it is broadcast, then we consider it stale / for rebroadcasting.
static constexpr auto STALE_DURATION{1min};
struct PeerSendInfo {
CService address;
NodeClock::time_point sent;
@@ -37,6 +46,7 @@ public:
struct TxBroadcastInfo {
CTransactionRef tx;
NodeClock::time_point time_added;
std::vector<PeerSendInfo> peers;
};
@@ -176,9 +186,12 @@ private:
*/
std::optional<TxAndSendStatusForNode> GetSendStatusByNode(const NodeId& nodeid)
EXCLUSIVE_LOCKS_REQUIRED(m_mutex);
struct TxSendStatus {
const NodeClock::time_point time_added{NodeClock::now()};
std::vector<SendStatus> send_statuses;
};
mutable Mutex m_mutex;
std::unordered_map<CTransactionRef, std::vector<SendStatus>, CTransactionRefHash, CTransactionRefComp>
std::unordered_map<CTransactionRef, TxSendStatus, CTransactionRefHash, CTransactionRefComp>
m_transactions GUARDED_BY(m_mutex);
};

2
src/rpc/mempool.cpp
View File

@@ -155,6 +155,7 @@ static RPCMethod getprivatebroadcastinfo()
{RPCResult::Type::STR_HEX, "txid", "The transaction hash in hex"},
{RPCResult::Type::STR_HEX, "wtxid", "The transaction witness hash in hex"},
{RPCResult::Type::STR_HEX, "hex", "The serialized, hex-encoded transaction data"},
{RPCResult::Type::NUM_TIME, "time_added", "The time this transaction was added to the private broadcast queue (seconds since epoch)"},
{RPCResult::Type::ARR, "peers", "Per-peer send and acknowledgment information for this transaction",
{
{RPCResult::Type::OBJ, "", "",
@@ -183,6 +184,7 @@ static RPCMethod getprivatebroadcastinfo()
o.pushKV("txid", tx_info.tx->GetHash().ToString());
o.pushKV("wtxid", tx_info.tx->GetWitnessHash().ToString());
o.pushKV("hex", EncodeHexTx(*tx_info.tx));
o.pushKV("time_added", TicksSinceEpoch<std::chrono::seconds>(tx_info.time_added));
UniValue peers(UniValue::VARR);
for (const auto& peer : tx_info.peers) {
UniValue p(UniValue::VOBJ);

34
src/test/private_broadcast_tests.cpp
View File

@@ -90,6 +90,13 @@ BOOST_AUTO_TEST_CASE(basic)
BOOST_CHECK(!pb.DidNodeConfirmReception(recipient2));
BOOST_CHECK(!pb.DidNodeConfirmReception(nonexistent_recipient));
// 1. Freshly added transactions should NOT be stale yet.
BOOST_CHECK_EQUAL(pb.GetStale().size(), 0);
// 2. Fast-forward the mock clock past the INITIAL_STALE_DURATION.
SetMockTime(Now<NodeSeconds>() + PrivateBroadcast::INITIAL_STALE_DURATION + 1min);
// 3. Now that the initial duration has passed, both unconfirmed transactions should be stale.
BOOST_CHECK_EQUAL(pb.GetStale().size(), 2);
// Confirm reception by recipient1.
@@ -102,20 +109,21 @@ BOOST_AUTO_TEST_CASE(basic)
const auto infos{pb.GetBroadcastInfo()};
BOOST_CHECK_EQUAL(infos.size(), 2);
{
const auto& [tx, peers]{find_tx_info(infos, tx_for_recipient1)};
const auto& peers{find_tx_info(infos, tx_for_recipient1).peers};
BOOST_CHECK_EQUAL(peers.size(), 1);
BOOST_CHECK_EQUAL(peers[0].address.ToStringAddrPort(), addr1.ToStringAddrPort());
BOOST_CHECK(peers[0].received.has_value());
}
{
const auto& [tx, peers]{find_tx_info(infos, tx_for_recipient2)};
const auto& peers{find_tx_info(infos, tx_for_recipient2).peers};
BOOST_CHECK_EQUAL(peers.size(), 1);
BOOST_CHECK_EQUAL(peers[0].address.ToStringAddrPort(), addr2.ToStringAddrPort());
BOOST_CHECK(!peers[0].received.has_value());
}
BOOST_CHECK_EQUAL(pb.GetStale().size(), 1);
BOOST_CHECK_EQUAL(pb.GetStale()[0], tx_for_recipient2);
const auto stale_state{pb.GetStale()};
BOOST_CHECK_EQUAL(stale_state.size(), 1);
BOOST_CHECK_EQUAL(stale_state[0], tx_for_recipient2);
SetMockTime(Now<NodeSeconds>() + 10h);
@@ -131,4 +139,22 @@ BOOST_AUTO_TEST_CASE(basic)
BOOST_CHECK(!pb.PickTxForSend(/*will_send_to_nodeid=*/nonexistent_recipient, /*will_send_to_address=*/addr_nonexistent).has_value());
}
BOOST_AUTO_TEST_CASE(stale_unpicked_tx)
{
SetMockTime(Now<NodeSeconds>());
PrivateBroadcast pb;
const auto tx{MakeDummyTx(/*id=*/42, /*num_witness=*/0)};
BOOST_REQUIRE(pb.Add(tx));
// Unpicked transactions use the longer INITIAL_STALE_DURATION.
BOOST_CHECK_EQUAL(pb.GetStale().size(), 0);
SetMockTime(Now<NodeSeconds>() + PrivateBroadcast::INITIAL_STALE_DURATION - 1min);
BOOST_CHECK_EQUAL(pb.GetStale().size(), 0);
SetMockTime(Now<NodeSeconds>() + 2min);
const auto stale_state{pb.GetStale()};
BOOST_REQUIRE_EQUAL(stale_state.size(), 1);
BOOST_CHECK_EQUAL(stale_state[0], tx);
}
BOOST_AUTO_TEST_SUITE_END()