Skip to content

Commit b28eb3b

Browse files
mraksoll4mraksoll4
committed
Backport 5 unmerged bitcoin/bitcoin fixes: BIP152 sendcmpct validation, private-broadcast hardening, mempool package cleanup
bitcoin/bitcoin/pull/35550 bitcoin/bitcoin/pull/35267 bitcoin/bitcoin/pull/34873 bitcoin/bitcoin/pull/35129 bitcoin/bitcoin/pull/35017 - net_processing: enforce BIP152's requirement that sendcmpct's announce field be 0 or 1; misbehave and disconnect otherwise. - rpc: getprivatebroadcastinfo/abortprivatebroadcast now throw if the node wasn't started with -privatebroadcast=1, instead of operating on an inactive state. - private_broadcast: wrap the per-tx send-status vector in a TxSendStatus struct carrying time_added, and use it to give a freshly-added, not-yet-picked tx a 5min grace period before GetStale() flags it -- previously it was flagged stale (and rebroadcast) within seconds of being queued. Also enforce that PickTxForSend() is never called twice with the same node id (privacy leak otherwise), and add a general-purpose fuzz harness covering the whole PrivateBroadcast API and this invariant. - validation: on the belt-and-suspenders "PolicyScriptChecks succeeded but ConsensusScriptChecks failed" path (not known to be reachable, guarded by Assume(false)), remove the failing tx AND all subsequent txs in the package from the mempool, not just the one that failed -- avoids leaving a later tx whose validity depended on the failed one. None of these are in official bitcoin/bitcoin 31.x. Ported from bitcoin/bitcoin: bitcoin/bitcoin@2d0dce0af5 -- net_processing: fix BIP152 first integer interpretation (brunoerg) bitcoin/bitcoin@abc33ff043 -- test: announce field must be 0 or 1 in sendcmpct (brunoerg) bitcoin/bitcoin@7b821ef9b7 -- rpc: getprivatebroadcastinfo/abortprivatebroadcast throw if -privatebroadcast is disabled (Pol Espinasa) bitcoin/bitcoin@fdc9fc1df2 -- test: check getprivatebroadcast/abortprivatebroadcast throw without -privatebroadcast (Pol Espinasa) bitcoin/bitcoin@999d18ab1ca6 -- net: introduce TxSendStatus internal state container (Mccalabrese) bitcoin/bitcoin@325afe664d10 -- net: delay stale evaluation and expose time_added in private broadcast (Mccalabrese) bitcoin/bitcoin@08b7c61fc703 -- private broadcast: enforce sending to unique node ids (Vasil Dimov) bitcoin/bitcoin@2ee4fafa3f48 -- test: add fuzz test for private broadcast (kevkevinpal, co-authored by Greg Sanders, Vasil Dimov) bitcoin/bitcoin@ac9aa71b7f -- mempool: remove all subsequent tx in pkg on failure (Greg Sanders) also addet helper for 31.x code base to src/test/util/time.h to support "FakeNodeTime" Tree-SHA512: 8dd7987da716d9920c4920b32fbb84adb44727a3f259b10ae7aae1eab1c9e78c69615e822cfab2abd8ae339ef3b2ac6632092f35cbe7dab7d34860249aeec931
1 parent d5db1bc commit b28eb3b

13 files changed

Lines changed: 394 additions & 37 deletions

‎src/net_processing.cpp‎

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1854,6 +1854,7 @@ PeerManagerInfo PeerManagerImpl::GetInfo() const
18541854
return PeerManagerInfo{
18551855
.median_outbound_time_offset = m_outbound_time_offsets.Median(),
18561856
.ignores_incoming_txs = m_opts.ignore_incoming_txs,
1857+
.private_broadcast = m_opts.private_broadcast,
18571858
};
18581859
}
18591860

@@ -1868,7 +1869,8 @@ std::vector<CTransactionRef> PeerManagerImpl::AbortPrivateBroadcast(const uint25
18681869
std::vector<CTransactionRef> removed_txs;
18691870

18701871
size_t connections_cancelled{0};
1871-
for (const auto& [tx, _] : snapshot) {
1872+
for (const auto& tx_info : snapshot) {
1873+
const CTransactionRef& tx{tx_info.tx};
18721874
if (tx->GetHash().ToUint256() != id && tx->GetWitnessHash().ToUint256() != id) continue;
18731875
if (const auto peer_acks{m_tx_for_private_broadcast.Remove(tx)}) {
18741876
removed_txs.push_back(tx);
@@ -3914,10 +3916,19 @@ void PeerManagerImpl::ProcessMessage(Peer& peer, CNode& pfrom, const std::string
39143916
}
39153917

39163918
if (msg_type == NetMsgType::SENDCMPCT) {
3917-
bool sendcmpct_hb{false};
3919+
// BACKPORT (upstream bitcoin/bitcoin PR #35550, commit 2d0dce0af5; not yet in 31.x
3920+
// as of 2026-07-04): DO NOT DROP ON NEXT UPSTREAM MERGE/REBASE.
3921+
uint8_t sendcmpct_hb{0};
39183922
uint64_t sendcmpct_version{0};
39193923
vRecv >> sendcmpct_hb >> sendcmpct_version;
39203924

3925+
// BIP152: the first integer is interpreted as a boolean and MUST have a
3926+
// value of either 1 or 0.
3927+
if (sendcmpct_hb > 1) {
3928+
Misbehaving(peer, "invalid sendcmpct announce field");
3929+
return;
3930+
}
3931+
39213932
// Only support compact block relay with witnesses
39223933
if (sendcmpct_version != CMPCTBLOCKS_VERSION) return;
39233934

‎src/net_processing.h‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,11 @@ struct CNodeStateStats {
7171
struct PeerManagerInfo {
7272
std::chrono::seconds median_outbound_time_offset{0s};
7373
bool ignores_incoming_txs{false};
74+
// BACKPORT (upstream bitcoin/bitcoin commit 7b821ef9b7, PR #35267; not yet in 31.x
75+
// as of 2026-07-04): DO NOT DROP ON NEXT UPSTREAM MERGE/REBASE. Lets the RPC layer
76+
// check whether -privatebroadcast is enabled before serving getprivatebroadcastinfo /
77+
// abortprivatebroadcast.
78+
bool private_broadcast{DEFAULT_PRIVATE_BROADCAST};
7479
};
7580

7681
class PeerManager : public CValidationInterface, public NetEventsInterface

‎src/private_broadcast.cpp‎

Lines changed: 25 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -7,10 +7,6 @@
77

88
#include <algorithm>
99

10-
/// If a transaction is not received back from the network for this duration
11-
/// after it is broadcast, then we consider it stale / for rebroadcasting.
12-
static constexpr auto STALE_DURATION{1min};
13-
1410
bool PrivateBroadcast::Add(const CTransactionRef& tx)
1511
EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
1612
{
@@ -25,7 +21,7 @@ std::optional<size_t> PrivateBroadcast::Remove(const CTransactionRef& tx)
2521
LOCK(m_mutex);
2622
const auto handle{m_transactions.extract(tx)};
2723
if (handle) {
28-
const auto p{DerivePriority(handle.mapped())};
24+
const auto p{DerivePriority(handle.mapped().send_statuses)};
2925
return p.num_confirmed;
3026
}
3127
return std::nullopt;
@@ -36,14 +32,22 @@ std::optional<CTransactionRef> PrivateBroadcast::PickTxForSend(const NodeId& wil
3632
{
3733
LOCK(m_mutex);
3834

35+
// BACKPORT (upstream 08b7c61fc703; not yet in 31.x as of 2026-07-04): DO NOT DROP ON
36+
// NEXT UPSTREAM MERGE/REBASE. GetSendStatusByNode() assumes unique node ids; sending
37+
// more than one transaction to a given node would be a privacy leak.
38+
if (GetSendStatusByNode(will_send_to_nodeid).has_value()) { // nodeid reuse, shouldn't send >1 tx to a given node
39+
Assume(false);
40+
return std::nullopt;
41+
}
42+
3943
const auto it{std::ranges::max_element(
4044
m_transactions,
4145
[](const auto& a, const auto& b) { return a < b; },
42-
[](const auto& el) { return DerivePriority(el.second); })};
46+
[](const auto& el) { return DerivePriority(el.second.send_statuses); })};
4347

4448
if (it != m_transactions.end()) {
45-
auto& [tx, sent_to]{*it};
46-
sent_to.emplace_back(will_send_to_nodeid, will_send_to_address, NodeClock::now());
49+
auto& [tx, state]{*it};
50+
state.send_statuses.emplace_back(will_send_to_nodeid, will_send_to_address, NodeClock::now());
4751
return tx;
4852
}
4953

@@ -93,12 +97,14 @@ std::vector<CTransactionRef> PrivateBroadcast::GetStale() const
9397
EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
9498
{
9599
LOCK(m_mutex);
96-
const auto stale_time{NodeClock::now() - STALE_DURATION};
100+
const auto now{NodeClock::now()};
97101
std::vector<CTransactionRef> stale;
98-
for (const auto& [tx, send_status] : m_transactions) {
99-
const Priority p{DerivePriority(send_status)};
100-
if (p.last_confirmed < stale_time) {
101-
stale.push_back(tx);
102+
for (const auto& [tx, state] : m_transactions) {
103+
const Priority p{DerivePriority(state.send_statuses)};
104+
if (p.num_confirmed == 0) {
105+
if (state.time_added < now - INITIAL_STALE_DURATION) stale.push_back(tx);
106+
} else {
107+
if (p.last_confirmed < now - STALE_DURATION) stale.push_back(tx);
102108
}
103109
}
104110
return stale;
@@ -111,13 +117,13 @@ std::vector<PrivateBroadcast::TxBroadcastInfo> PrivateBroadcast::GetBroadcastInf
111117
std::vector<TxBroadcastInfo> entries;
112118
entries.reserve(m_transactions.size());
113119

114-
for (const auto& [tx, sent_to] : m_transactions) {
120+
for (const auto& [tx, state] : m_transactions) {
115121
std::vector<PeerSendInfo> peers;
116-
peers.reserve(sent_to.size());
117-
for (const auto& status : sent_to) {
122+
peers.reserve(state.send_statuses.size());
123+
for (const auto& status : state.send_statuses) {
118124
peers.emplace_back(PeerSendInfo{.address = status.address, .sent = status.picked, .received = status.confirmed});
119125
}
120-
entries.emplace_back(TxBroadcastInfo{.tx = tx, .peers = std::move(peers)});
126+
entries.emplace_back(TxBroadcastInfo{.tx = tx, .time_added = state.time_added, .peers = std::move(peers)});
121127
}
122128

123129
return entries;
@@ -141,8 +147,8 @@ std::optional<PrivateBroadcast::TxAndSendStatusForNode> PrivateBroadcast::GetSen
141147
EXCLUSIVE_LOCKS_REQUIRED(m_mutex)
142148
{
143149
AssertLockHeld(m_mutex);
144-
for (auto& [tx, sent_to] : m_transactions) {
145-
for (auto& send_status : sent_to) {
150+
for (auto& [tx, state] : m_transactions) {
151+
for (auto& send_status : state.send_statuses) {
146152
if (send_status.nodeid == nodeid) {
147153
return TxAndSendStatusForNode{.tx = tx, .send_status = send_status};
148154
}

‎src/private_broadcast.h‎

Lines changed: 27 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,20 @@
3030
class PrivateBroadcast
3131
{
3232
public:
33+
// BACKPORT (upstream bitcoin/bitcoin commits 999d18ab1ca6 + 325afe664d10; not yet in
34+
// 31.x as of 2026-07-04): DO NOT DROP ON NEXT UPSTREAM MERGE/REBASE.
35+
// Fixes a freshly-added, not-yet-picked private-broadcast transaction being immediately
36+
// considered stale (Priority::last_confirmed defaulted to the Unix epoch), causing an
37+
// unnecessary extra rebroadcast attempt seconds after the original send (upstream #34862).
38+
39+
/// If a transaction is not sent to any peer for this duration,
40+
/// then we consider it stale / for rebroadcasting.
41+
static constexpr auto INITIAL_STALE_DURATION{5min};
42+
43+
/// If a transaction is not received back from the network for this duration
44+
/// after it is broadcast, then we consider it stale / for rebroadcasting.
45+
static constexpr auto STALE_DURATION{1min};
46+
3347
struct PeerSendInfo {
3448
CService address;
3549
NodeClock::time_point sent;
@@ -38,6 +52,7 @@ class PrivateBroadcast
3852

3953
struct TxBroadcastInfo {
4054
CTransactionRef tx;
55+
NodeClock::time_point time_added;
4156
std::vector<PeerSendInfo> peers;
4257
};
4358

@@ -64,7 +79,9 @@ class PrivateBroadcast
6479
* Pick the transaction with the fewest send attempts, and confirmations,
6580
* and oldest send/confirm times.
6681
* @param[in] will_send_to_nodeid Will remember that the returned transaction
67-
* was picked for sending to this node.
82+
* was picked for sending to this node. Calling this method more than once with
83+
* the same `will_send_to_nodeid` is not allowed because sending more than one
84+
* transaction to one node would be a privacy leak.
6885
* @param[in] will_send_to_address Address of the peer to which this transaction
6986
* will be sent.
7087
* @return Most urgent transaction or nullopt if there are no transactions.
@@ -178,8 +195,16 @@ class PrivateBroadcast
178195
std::optional<TxAndSendStatusForNode> GetSendStatusByNode(const NodeId& nodeid)
179196
EXCLUSIVE_LOCKS_REQUIRED(m_mutex);
180197

198+
// BACKPORT (upstream 999d18ab1ca6, part of the stale-eval fix above): wraps the per-tx
199+
// send-status vector together with the time the transaction was added, needed to apply
200+
// INITIAL_STALE_DURATION before any send attempt has happened.
201+
struct TxSendStatus {
202+
const NodeClock::time_point time_added{NodeClock::now()};
203+
std::vector<SendStatus> send_statuses;
204+
};
205+
181206
mutable Mutex m_mutex;
182-
std::unordered_map<CTransactionRef, std::vector<SendStatus>, CTransactionRefHash, CTransactionRefComp>
207+
std::unordered_map<CTransactionRef, TxSendStatus, CTransactionRefHash, CTransactionRefComp>
183208
m_transactions GUARDED_BY(m_mutex);
184209
};
185210

‎src/rpc/mempool.cpp‎

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -143,7 +143,8 @@ static RPCHelpMan getprivatebroadcastinfo()
143143
{
144144
return RPCHelpMan{
145145
"getprivatebroadcastinfo",
146-
"Returns information about transactions that are currently being privately broadcast.\n",
146+
"Returns information about transactions that are currently being privately broadcast.\n"
147+
"This method is only available when running with -privatebroadcast enabled.\n",
147148
{},
148149
RPCResult{
149150
RPCResult::Type::OBJ, "", "",
@@ -155,6 +156,7 @@ static RPCHelpMan getprivatebroadcastinfo()
155156
{RPCResult::Type::STR_HEX, "txid", "The transaction hash in hex"},
156157
{RPCResult::Type::STR_HEX, "wtxid", "The transaction witness hash in hex"},
157158
{RPCResult::Type::STR_HEX, "hex", "The serialized, hex-encoded transaction data"},
159+
{RPCResult::Type::NUM_TIME, "time_added", "The time this transaction was added to the private broadcast queue (seconds since epoch)"},
158160
{RPCResult::Type::ARR, "peers", "Per-peer send and acknowledgment information for this transaction",
159161
{
160162
{RPCResult::Type::OBJ, "", "",
@@ -175,6 +177,12 @@ static RPCHelpMan getprivatebroadcastinfo()
175177
{
176178
const NodeContext& node{EnsureAnyNodeContext(request.context)};
177179
const PeerManager& peerman{EnsurePeerman(node)};
180+
// BACKPORT (upstream bitcoin/bitcoin commit 7b821ef9b7, PR #35267; not yet in
181+
// 31.x as of 2026-07-04): DO NOT DROP ON NEXT UPSTREAM MERGE/REBASE.
182+
if (!peerman.GetInfo().private_broadcast) {
183+
throw JSONRPCError(RPC_METHOD_NOT_FOUND, "Private broadcast is not enabled. Ensure you're running Bitcoin Core with -privatebroadcast=1.");
184+
}
185+
178186
const auto txs{peerman.GetPrivateBroadcastInfo()};
179187

180188
UniValue transactions(UniValue::VARR);
@@ -183,6 +191,7 @@ static RPCHelpMan getprivatebroadcastinfo()
183191
o.pushKV("txid", tx_info.tx->GetHash().ToString());
184192
o.pushKV("wtxid", tx_info.tx->GetWitnessHash().ToString());
185193
o.pushKV("hex", EncodeHexTx(*tx_info.tx));
194+
o.pushKV("time_added", TicksSinceEpoch<std::chrono::seconds>(tx_info.time_added));
186195
UniValue peers(UniValue::VARR);
187196
for (const auto& peer : tx_info.peers) {
188197
UniValue p(UniValue::VOBJ);
@@ -209,7 +218,8 @@ static RPCHelpMan abortprivatebroadcast()
209218
return RPCHelpMan{
210219
"abortprivatebroadcast",
211220
"Abort private broadcast attempts for a transaction currently being privately broadcast.\n"
212-
"The transaction will be removed from the private broadcast queue.\n",
221+
"The transaction will be removed from the private broadcast queue.\n"
222+
"This method is only available when running with -privatebroadcast enabled.\n",
213223
{
214224
{"id", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "A transaction identifier to abort. It will be matched against both txid and wtxid for all transactions in the private broadcast queue.\n"
215225
"If the provided id matches a txid that corresponds to multiple transactions with different wtxids, multiple transactions will be removed and returned."},
@@ -234,10 +244,15 @@ static RPCHelpMan abortprivatebroadcast()
234244
},
235245
[&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
236246
{
237-
const uint256 id{ParseHashV(self.Arg<UniValue>("id"), "id")};
238-
239247
const NodeContext& node{EnsureAnyNodeContext(request.context)};
240248
PeerManager& peerman{EnsurePeerman(node)};
249+
// BACKPORT (upstream bitcoin/bitcoin commit 7b821ef9b7, PR #35267; not yet in
250+
// 31.x as of 2026-07-04): DO NOT DROP ON NEXT UPSTREAM MERGE/REBASE.
251+
if (!peerman.GetInfo().private_broadcast) {
252+
throw JSONRPCError(RPC_METHOD_NOT_FOUND, "Private broadcast is not enabled. Ensure you're running Bitcoin Core with -privatebroadcast=1.");
253+
}
254+
255+
const uint256 id{ParseHashV(self.Arg<UniValue>("id"), "id")};
241256

242257
const auto removed_txs{peerman.AbortPrivateBroadcast(id)};
243258
if (removed_txs.empty()) {

‎src/test/fuzz/CMakeLists.txt‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,7 @@ add_executable(fuzz
9797
pow_cache_check.cpp # Bitweb Params: fuzz CheckProofOfWorkCached
9898
prevector.cpp
9999
primitives_transaction.cpp
100+
private_broadcast.cpp
100101
process_message.cpp
101102
process_messages.cpp
102103
protocol.cpp

0 commit comments

Comments
 (0)