Add a belt-and-suspenders feature, limit the amount of memory and cpu possible when unlucky or simply misconfigured. The worst case limit is roughly 400kB * 10,000 = 4GB, regardless of usage pattern.
Before this change, sheer volume of broadcasts, mismatches in standardness rules, or simply fee mismatches may result in unbounded growth of memory usage. As the feature may be expanded in the future, explicit bounds helps reasoning going forward.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35406.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | vasild, andrewtoth, frankomosh, stickies-v |
| Stale ACK | optout21 |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
Is this for backport?
@fanquake worthy of backport for safety and user expectations going forward imo
21 | + m_transactions.try_emplace(tx, std::prev(m_insertion_order.end())); 22 | + 23 | + if (m_transactions.size() > MAX_TRANSACTIONS) { 24 | + // Evict the oldest entry (FIFO). The front of m_insertion_order is the 25 | + // earliest-added tx; the just-inserted tx sits at the back. 26 | + const CTransactionRef oldest{m_insertion_order.front()};
I feel like this would be a lot simpler by just using the time_added field, instead of adding a parallel data structure? It would be an O(n) scan, but it's capped at MAX_TRANSACTIONs.
const CTransactionRef oldest{std::ranges::min_element(
m_transactions, {},
[](const auto& el) { return el.second.time_added; })};
Fair, if we're landing on 1000 cap or less I'll simplify.
Even 10k should be fine. It's not performance sensitive. This is guarding an edge case. I don't think anyone would hit this limit in practice.
done (right, obviously overkill for something that only happens once per rpc submission! not a recurring task)
156 | @@ -157,4 +157,59 @@ BOOST_AUTO_TEST_CASE(stale_unpicked_tx) 157 | BOOST_CHECK_EQUAL(stale_state[0], tx); 158 | } 159 | 160 | +BOOST_AUTO_TEST_CASE(fifo_eviction_at_cap) 161 | +{ 162 | + PrivateBroadcast pb; 163 | + constexpr size_t kCap{PrivateBroadcast::MAX_TRANSACTIONS}; 164 | + constexpr size_t kOver{5};
style nit: don't use hungarian notation.
done
59 | @@ -60,6 +60,9 @@ static RPCMethod sendrawtransaction()
60 | "dedicated, short-lived connections to Tor or I2P peers or IPv4/IPv6 peers\n"
In commit: de1059f51bfbebc5f59423ee8e23608093381c92
The commit message:
As the feature may be expande in the future, explicity bounds helps reasoning going forward.
Should be:
As the feature may be expanded in the future, explicit bounds helps reasoning going forward.
The same applies to the PR description as well.
done
17 | + 18 | +class PrivateBroadcastCapTest(BitcoinTestFramework): 19 | + def set_test_params(self): 20 | + self.num_nodes = 1 21 | + # -privatebroadcast is incompatible with the framework's default 22 | + # -connect=0; allow autoconnect (no actual peers will succeed though).
We can use the -maxconnections=0 workaround for this if needed (not sure it's needed).
leaving as is
13 | @@ -12,8 +14,25 @@ bool PrivateBroadcast::Add(const CTransactionRef& tx) 14 | EXCLUSIVE_LOCKS_REQUIRED(!m_mutex) 15 | { 16 | LOCK(m_mutex); 17 | - const bool inserted{m_transactions.try_emplace(tx).second}; 18 | - return inserted; 19 | + if (!m_transactions.try_emplace(tx).second) return false; 20 | + 21 | + if (m_transactions.size() > MAX_TRANSACTIONS) {
de1059f private broadcast: limit outstanding txs to count of 1000:
The implicit assumption here is that size exceeds max by exactly one. This is true, for as long as adding only happens only through Add, and never simultaneously. Changing the if to while would be more robust -- it's not really needed, but it's worth considering.
We have the trailing Assume() that would catch this in debug builds, I think it's easiest to think about this way? Leaving as is.
ACK 401459243159e34fed93dc3ae1ec19c1c19075fc
Reviewed code, executed tests locally, LGTM.
This PR adds a safety-net max limit for the private broadcast queue. While it can be argued that over-use is not very likely and not very catastrophic (usage through protected RPC interface, modest expected regular usage, reduced memory usage), it is good practice to have safety limits in place. It makes the risk explicit (however small), and introduces a "just in case" limit. The limit of 1000 transaction is mostly arbitrary number, and not configurable. This is perfectly acceptable, having in mind the low-probability, low-harm nature of the queue being full.
23 | + // expanded in scope. 24 | + const auto oldest{std::ranges::min_element( 25 | + m_transactions, {}, 26 | + [](const auto& el) { return el.second.time_added; })}; 27 | + const CTransactionRef& tx_to_evict{oldest->first}; 28 | + LogDebug(BCLog::PRIVBROADCAST,
I think this should be LogInfo at least?
Now that the caller is getting an error, going to leave it as debug
Concept ACK for bounding resource usage.
Approach-wise, did you consider letting sendrawtransaction error instead of dropping the oldest tx? When it fails, user can use abortprivatebroadcast to drop what they want, rather than us doing it silently.
Approach-wise, did you consider letting sendrawtransaction error instead of dropping the oldest tx?
Might be easiest way, just do a length check in Add and return false if max or higher.
Approach-wise, did you consider letting sendrawtransaction error instead of dropping the oldest tx? When it fails, user can use abortprivatebroadcast to drop what they want, rather than us doing it silently.
In the misconfigured scenario, I think either strategy works just fine, we just don't want to crash and nothing is propagating for some reason.
In the high volume scenario, where we were simply wrong about how users intend to use it (now or going forward), they will have to know about and able to handle this explicitly (I have low confidence that people are doing sensible things with our weird error reporting fwiw).
One pro of explicit handling is that the caller will never be surprised that something ended up being dropped.
I'm slightly predisposed to handling it implicitly and allowing naive rebroadcasting to "fix" things, but I can see it both ways.
I think hitting this means something is misconfigured, so explicitly erroring will alert the user to the issue. I would be in favor of @stickies-v's approach.
they will have to know about and able to handle this explicitly
Isn't that true regardless? When we silently drop, they'll need to (well, should) track / manage submitted txs and implement logic on how/when to retry the tx. Regardless, from an interface design pov, I think explicit is better than implicit.
Not familiar with wallet, so just my 2c, won't push back on it further.
so explicitly erroring will alert the user to the issue.
Just noting again to be clear: Outside of manual use I expect the error to simply be ignore/logged by many integrations, along with "already in mempool" kinda messages.
I suppose with good docs, the error-ignoring user can simply check the broadcast set, evict whatever the like, and it will self-heal next attempt at broadcasting.
I think hitting this means something is misconfigured
We shouldn't ignore usage patterns that plausibly exist. Question is what is best when our assumptions are violated.
Isn't that true regardless? When we silently drop, they'll need to (well, should) track / manage submitted txs and implement logic on how/when to retry the tx.
The caller knows a tx isn't being confirmed by looking at blocks and calls sendrawtransaction again. No need to even look at error messages in normal operation with or without private broadcast.
Regardless, from an interface design pov, I think explicit is better than implicit.
Agreed. I think with good docs it can be made idiot proof. Let me get that implemented.
A few thoughts:
Just 1000? The OP says 400kB * 1000 = 400MB, but where did that 400kB come from? A typical transaction is a few hundred bytes. Plus some storage overhead, I think we can reasonably assume one transaction takes 500-1000 bytes of RAM in the private broadcast queue. So, in 400MB one can store about 400'000 transactions, conservatively.
I think it would be better to return an immediate error from the sendrawtransaction RPC because then the user (or the user program driving this):
Just 1000? The OP says 400kB * 1000 = 400MB, but where did that 400kB come from?
it's the currently maximum sized transaction we will accept into our mempool (or private broadcast set). I think the simplest alternative is to have the serialized sized accounting instead of pure tx accounting, so the average case would be much higher and unlikely to be hit. Just increases the required testing a bit, and users rectifying it would have to abandon a variable amount of stuck broadcasts to make room for new ones.
I think I'd rather bump this number a few X and keep the accounting / abandon flow as simple as possible.
I guess Bitcoin Core runs on a variety of hardware. Maybe some machines have 100x more RAM than others. It is not reasonable to assume that one number will fit all. So consider making this manually configurable via config option (eh :/) or set it internally based on the amount of RAM available.
eh same to me, would rather we keep things as simple as possible unless there is real demand for something other than a anti-footgun
Pushed an update where the caller is informed that the queue is full and FIFO is removed, as discussed.
20 | + // Re-adding an already-tracked transaction is a no-op regardless of the cap. 21 | + if (m_transactions.contains(tx)) return AddResult::AlreadyPresent; 22 | + 23 | + if (m_transactions.size() >= MAX_TRANSACTIONS) return AddResult::QueueFull; 24 | + 25 | + m_transactions.try_emplace(tx);
It's building a pair with the key being default value. I don't think an assert gives us much given the check just above. Will leave as is unless others agree.
ACK e96c343137f638b8eb096d262edc743695bc414d
Rebase only.
Prev: ACK 46b858ce7a43d39a28b85492c327602e8099a22d Re-review, new changes: The private queue full case is handled now as an explicit error, without implicitly deleting old entries. The error is propagated all the way through transaction broadcast RPC. This makes the case more explicit for callers, avoids the Core node to make policy decisions, but makes it more difficult for clients to handle it properly (i.e., delete old ones explicitly.). Overall I ACK this change.
(Prev: #35406#pullrequestreview-4387609655)
0 | @@ -0,0 +1,8 @@ 1 | +P2P and network changes 2 | +----------------------- 3 | + 4 | +- The private-broadcast queue (transactions submitted via `sendrawtransaction` 5 | +when `-privatebroadcast` is enabled and not yet echoed back from the network) 6 | +is now capped at 1000 entries. When full, new submissions are rejected. It is 7 | +up to the caller to inspect the queue via `getprivatebroadcastinfo` and free 8 | +up space when stuck via `abandonprivatebroadcast`. (#35406)
up space when stuck via `abortprivatebroadcast`. (#35406)
done
59 | @@ -60,6 +60,9 @@ static RPCMethod sendrawtransaction() 60 | "dedicated, short-lived connections to Tor or I2P peers or IPv4/IPv6 peers\n" 61 | "via the Tor network. This conceals the transaction's origin. The transaction\n" 62 | "will only enter the local mempool when it is received back from the network.\n" 63 | + "The private broadcast queue is bounded: when it is full, this RPC fails and\n" 64 | + "the transaction is not scheduled, until an existing one completes or is\n" 65 | + "aborted. Use getprivatebroadcastinfo to inspect the queue.\n"
"aborted. Use getprivatebroadcastinfo to inspect the queue and abortprivatebroadcast to abort.\n"
done
132 | @@ -133,7 +133,9 @@ TransactionError BroadcastTransaction(NodeContext& node, 133 | node.peerman->InitiateTxBroadcastToAll(txid, wtxid); 134 | break; 135 | case TxBroadcast::NO_MEMPOOL_PRIVATE_BROADCAST: 136 | - node.peerman->InitiateTxBroadcastPrivate(tx); 137 | + if (!node.peerman->InitiateTxBroadcastPrivate(tx)) { 138 | + return TransactionError::PRIVATE_BROADCAST_FULL;
It seems a bit awkward to compress the PrivateBroadcast::AddResult result into a bool to then later decompress it into a TransactionError again. Have you considered returning a TransactionError from InitiateTxBroadcastPrivate instead? Seems more upgradable too.
I also think [[nodiscard]] would be good here.
<details> <summary>git diff on e96c343137</summary>
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 8b6bdba75c..bdc4292166 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -540,7 +540,7 @@ public:
std::vector<CTransactionRef> AbortPrivateBroadcast(const uint256& id) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
void SendPings() override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
void InitiateTxBroadcastToAll(const Txid& txid, const Wtxid& wtxid) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
- bool InitiateTxBroadcastPrivate(const CTransactionRef& tx) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
+ node::TransactionError InitiateTxBroadcastPrivate(const CTransactionRef& tx) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
void SetBestBlock(int height, std::chrono::seconds time) override
{
m_best_height = height;
@@ -2264,20 +2264,20 @@ void PeerManagerImpl::InitiateTxBroadcastToAll(const Txid& txid, const Wtxid& wt
}
}
-bool PeerManagerImpl::InitiateTxBroadcastPrivate(const CTransactionRef& tx)
+node::TransactionError PeerManagerImpl::InitiateTxBroadcastPrivate(const CTransactionRef& tx)
{
const auto txstr{strprintf("txid=%s, wtxid=%s", tx->GetHash().ToString(), tx->GetWitnessHash().ToString())};
switch (m_tx_for_private_broadcast.Add(tx)) {
case PrivateBroadcast::AddResult::Added:
LogDebug(BCLog::PRIVBROADCAST, "Requesting %d new connections due to %s", NUM_PRIVATE_BROADCAST_PER_TX, txstr);
m_connman.m_private_broadcast.NumToOpenAdd(NUM_PRIVATE_BROADCAST_PER_TX);
- return true;
+ return node::TransactionError::OK;
case PrivateBroadcast::AddResult::AlreadyPresent:
LogDebug(BCLog::PRIVBROADCAST, "Ignoring unnecessary request to schedule an already scheduled transaction: %s", txstr);
- return true;
+ return node::TransactionError::OK;
case PrivateBroadcast::AddResult::QueueFull:
LogDebug(BCLog::PRIVBROADCAST, "Rejecting private broadcast, queue full (cap=%u): %s", PrivateBroadcast::MAX_TRANSACTIONS, txstr);
- return false;
+ return node::TransactionError::PRIVATE_BROADCAST_FULL;
} // no default case, so the compiler can warn about missing cases
assert(false);
}
diff --git a/src/net_processing.h b/src/net_processing.h
index 651e669ca2..b251e868c1 100644
--- a/src/net_processing.h
+++ b/src/net_processing.h
@@ -9,6 +9,7 @@
#include <consensus/amount.h>
#include <net.h>
#include <node/txorphanage.h>
+#include <node/types.h>
#include <private_broadcast.h>
#include <protocol.h>
#include <uint256.h>
@@ -146,10 +147,10 @@ public:
/**
* Initiate a private transaction broadcast. This is done
* asynchronously via short-lived connections to peers on privacy networks.
- * [@retval](/github-metadata-backup-bitcoin-bitcoin/contributor/retval/) true The transaction is scheduled for private broadcast (or was already scheduled).
- * [@retval](/github-metadata-backup-bitcoin-bitcoin/contributor/retval/) false Rejected because the private broadcast queue is full.
+ * [@retval](/github-metadata-backup-bitcoin-bitcoin/contributor/retval/) node::TransactionError::OK The transaction is scheduled for private broadcast (or was already scheduled).
+ * [@retval](/github-metadata-backup-bitcoin-bitcoin/contributor/retval/) node::TransactionError::PRIVATE_BROADCAST_FULL Rejected because the private broadcast queue is full.
*/
- virtual bool InitiateTxBroadcastPrivate(const CTransactionRef& tx) = 0;
+ [[nodiscard]] virtual node::TransactionError InitiateTxBroadcastPrivate(const CTransactionRef& tx) = 0;
/** Send ping message to all peers */
virtual void SendPings() = 0;
diff --git a/src/node/transaction.cpp b/src/node/transaction.cpp
index 0d462103d4..e7877c6985 100644
--- a/src/node/transaction.cpp
+++ b/src/node/transaction.cpp
@@ -133,10 +133,7 @@ TransactionError BroadcastTransaction(NodeContext& node,
node.peerman->InitiateTxBroadcastToAll(txid, wtxid);
break;
case TxBroadcast::NO_MEMPOOL_PRIVATE_BROADCAST:
- if (!node.peerman->InitiateTxBroadcastPrivate(tx)) {
- return TransactionError::PRIVATE_BROADCAST_FULL;
- }
- break;
+ return node.peerman->InitiateTxBroadcastPrivate(tx);
}
return TransactionError::OK;
</details>
taken thanks
53 | @@ -50,13 +54,20 @@ class PrivateBroadcast 54 | std::vector<PeerSendInfo> peers; 55 | }; 56 | 57 | + /// Outcome of Add(). 58 | + enum class AddResult { 59 | + Added, //!< The transaction was newly added. 60 | + AlreadyPresent, //!< The transaction was already present; no change. 61 | + QueueFull, //!< Rejected: the queue is already at MAX_TRANSACTIONS.
nit: this documentation style is now discouraged by developer-notes, instead it is recommended to:
<details> <summary>git diff on e96c343137</summary>
diff --git a/src/private_broadcast.h b/src/private_broadcast.h
index 1968d496fe..27af749cf0 100644
--- a/src/private_broadcast.h
+++ b/src/private_broadcast.h
@@ -56,9 +56,12 @@ public:
/// Outcome of Add().
enum class AddResult {
- Added, //!< The transaction was newly added.
- AlreadyPresent, //!< The transaction was already present; no change.
- QueueFull, //!< Rejected: the queue is already at MAX_TRANSACTIONS.
+ //! The transaction was newly added.
+ Added,
+ //! The transaction was already present; no change.
+ AlreadyPresent,
+ //! Rejected: the queue is already at MAX_TRANSACTIONS.
+ QueueFull,
};
/**
</details>
done
2 | @@ -3,17 +3,24 @@ 3 | // file COPYING or https://opensource.org/license/mit/. 4 | 5 | #include <private_broadcast.h> 6 | + 7 | +#include <logging.h>
nit: I don't think we need this?
<details> <summary> iwyu output</summary>
2026-06-04T16:25:41.6566108Z /home/runner/work/_temp/src/private_broadcast.cpp should add these lines:
2026-06-04T16:25:41.6566591Z #include <compare> // for operator<, strong_ordering
2026-06-04T16:25:41.6568246Z #include <memory> // for shared_ptr
2026-06-04T16:25:41.6568572Z #include <utility> // for get, move
2026-06-04T16:25:41.6568793Z
2026-06-04T16:25:41.6569029Z /home/runner/work/_temp/src/private_broadcast.cpp should remove these lines:
2026-06-04T16:25:41.6569486Z - #include <logging.h> // lines 7-7
2026-06-04T16:25:41.6569789Z - #include <util/check.h> // lines 8-8
2026-06-04T16:25:41.6569994Z
2026-06-04T16:25:41.6570223Z The full include-list for /home/runner/work/_temp/src/private_broadcast.cpp:
2026-06-04T16:25:41.6570741Z #include <private_broadcast.h>
2026-06-04T16:25:41.6571101Z #include <algorithm> // for __max_element_fn, max, max_element
2026-06-04T16:25:41.6571514Z #include <compare> // for operator<, strong_ordering
2026-06-04T16:25:41.6571872Z #include <memory> // for shared_ptr
2026-06-04T16:25:41.6572174Z #include <utility> // for get, move
</details>
removed
68 | - * @retval false The transaction was already present. 69 | + * @return Whether the transaction was newly added, was already present, or 70 | + * was rejected because the queue is full (see AddResult). 71 | */ 72 | - bool Add(const CTransactionRef& tx) 73 | + AddResult Add(const CTransactionRef& tx)
I think this would benefit from [[nodiscard]] since we're returning error codes.
done
81 | + assert child["wtxid"] in present_wtxids, \ 82 | + f"tx index {i} (wtxid={child['wtxid']}) should still be present" 83 | + for child in children[MAX_TRANSACTIONS:]: 84 | + assert child["wtxid"] not in present_wtxids, \ 85 | + f"rejected tx (wtxid={child['wtxid']}) must not be in the queue" 86 | +
I think this can be simplified by just checking the response is unchanged?
<details> <summary>git diff on e96c343137</summary>
diff --git a/test/functional/p2p_private_broadcast_cap.py b/test/functional/p2p_private_broadcast_cap.py
index 57b9cf5390..909116aba1 100755
--- a/test/functional/p2p_private_broadcast_cap.py
+++ b/test/functional/p2p_private_broadcast_cap.py
@@ -74,15 +74,7 @@ class PrivateBroadcastCapTest(BitcoinTestFramework):
assert_raises_rpc_error(-25, "Private broadcast queue is full",
node.sendrawtransaction, child["hex"])
- pbinfo = node.getprivatebroadcastinfo()
- assert_equal(len(pbinfo["transactions"]), MAX_TRANSACTIONS)
- present_wtxids = {t["wtxid"] for t in pbinfo["transactions"]}
- for i, child in enumerate(children[:MAX_TRANSACTIONS]):
- assert child["wtxid"] in present_wtxids, \
- f"tx index {i} (wtxid={child['wtxid']}) should still be present"
- for child in children[MAX_TRANSACTIONS:]:
- assert child["wtxid"] not in present_wtxids, \
- f"rejected tx (wtxid={child['wtxid']}) must not be in the queue"
+ assert_equal(pbinfo["transactions"], node.getprivatebroadcastinfo()["transactions"])
# Re-submitting an already-queued transaction is a no-op, not an error,
# even when the queue is full.
</details>
done
9 | 10 | #include <algorithm> 11 | 12 | 13 | -bool PrivateBroadcast::Add(const CTransactionRef& tx) 14 | +PrivateBroadcast::AddResult PrivateBroadcast::Add(const CTransactionRef& tx)
nit: would benefit from [[nodiscard]]
done
I don't think documenting this in init has a lot of value, there is nothing the user can do here, it shouldn't generally occur, and the only way this can manifest is through the sendrawtransaction RPC where you've added proper documentation?
As the feature expands, various places will be effected yet this limit will still be enforced. Removed since I weakly hold this position.
Approach ACK, did pretty thorough code review on 143125343fcfe9ebe701d8a714e3e43209738a75 so I think I'm mostly out of comments after this
One thing I noticed is that we're returning a generic rpc error (-25), probably worth adding a new code for this to allow the caller to programmatically react to it? Or is this generic error sufficient.
17 | - const bool inserted{m_transactions.try_emplace(tx).second}; 18 | - return inserted; 19 | + // Re-adding an already-tracked transaction is a no-op regardless of the cap. 20 | + if (m_transactions.contains(tx)) return AddResult::AlreadyPresent; 21 | + 22 | + if (m_transactions.size() >= MAX_TRANSACTIONS) return AddResult::QueueFull;
Not too difficult to switch the limit from "number of transactions" to "number of bytes" and also base the limit on the actual amount of RAM on the system.
const size_t max_mem{GetTotalRam().value_or(16_GiB) / 100}; // Use max 1% of the RAM.
size_t used_mem{0};
for (const auto& [t, _] : m_transactions) {
used_mem += t->ComputeTotalSize();
if (used_mem > max_mem) {
return AddResult::QueueFull;
}
}
I do not want to do dynamic cap. Switching to aggregate serialized size cap alone would bring the effective capacity up ~100x to ~500x based on most usage patterns. The downside is the aborting UX becomes a bit more complicated if it's ever hit (and I need to write more comprehensive tests to test boundary stuff, which is fine)
edit: I'd want to do some more analysis and testing to see how things work when you have >>1k oustanding broadcasts probably first. I'll think along these lines for a bit.
made a longer comment in the main thread #35406 (comment)
I'm declining to take on such a change at this time.
Just 1000? The OP says 400kB * 1000 = 400MB, but where did that 400kB come from?
it's the currently maximum sized transaction we will accept into our mempool (or private broadcast set).
I see. That is imposing a generic limit on everybody based on an extreme edge case. That would unnecessary cripple normal usage. I mean - trying to protect a user on Raspberry Pi who sends 1000 400KB transactions within seconds and has a problem because the memory usage goes up with 400MB. I think that is less likely compared to a bitcoind running on a normal machine and sending many normal-sized transactions and hitting the 1000 limit unnecessary (that would take about 1MB of RAM).
I think the simplest alternative is to have the serialized sized accounting ...
Yes, that looks better to me. See below for an example on how to do that.
One thing I noticed is that we're returning a generic rpc error (-25), probably worth adding a new code for this to allow the caller to programmatically react to it? Or is this generic error sufficient.
I think a separate code could be useful for the callers of the RPC. Maybe use RPC_OUT_OF_MEMORY instead of introducing a new one.
Along the lines of seeing if we should raise the effective cap by doing byte accounting rather than tx count, I worked on a set of benchmarks to see how this all scales. Some of the bigger benchmarks take a super long time to setup:
https://github.com/instagibbs/bitcoin/commit/33c32490f3e02061b64a930680c267e1c731d054
A few salient points:
All this to say, I think it's reasonable to do the 1k count limit for now for backport, work on bounding the overall memory/cpu usage is a more principled fashion, then relaxing the limit later.
Another mitigation (not being proposed for this PR) could be to not hold cs_main the entire loop and test in batches.
42 | @@ -43,15 +43,15 @@ BOOST_AUTO_TEST_CASE(basic) 43 | // Make a transaction and add it. 44 | const auto tx1{MakeDummyTx(/*id=*/1, /*num_witness=*/0)}; 45 | 46 | - BOOST_CHECK(pb.Add(tx1)); 47 | - BOOST_CHECK(!pb.Add(tx1)); 48 | + BOOST_CHECK(pb.Add(tx1) == PrivateBroadcast::AddResult::Added);
These can all be BOOST_CHECK_EQUAL.
Was going to mention this in my review, but is mentioned here already, so +1 for that, also BOOST_REQUIRE() can be made BOOST_REQUIRE_EQUAL() below.
78 | + 79 | + # Re-submitting an already-queued transaction is a no-op, not an error, 80 | + # even when the queue is full. 81 | + self.log.info("Re-submitting an already-queued tx should not error") 82 | + node.sendrawtransaction(children[0]["hex"]) 83 | + assert_equal(len(node.getprivatebroadcastinfo()["transactions"]), MAX_TRANSACTIONS)
Can do the same here as above for consistency
assert_equal(pbinfo["transactions"], node.getprivatebroadcastinfo()["transactions"])
We did design the private broadcast queue to assume there will not be a very large data set. See #29415 (comment). So, it makes sense to bound it. I think 1000 entries is on the small side though. Using worst case 400kb transactions seems less than ideal.
@vasild the OOM code is unused, I honestly don't know what the guidance is for usage of it...
@instagibbs, thanks for the benchmarks! I played with them. So, with the PickTxForSend() and GetStale() cases the pattern is more or less the same - comparing 1k queue vs 100k queue, those methods take 200 times more in the latter case. It is expected that querying a 100k queue would be 100x slower compared to querying 1k queue. The benchmark shows that it is 200x instead of 100x.
Looking at the absolute numbers of those queries - one completes in about 0.001 seconds on a 100k queue. How often is PickTxForSend() called? Lets say one broadcast takes about 10 seconds to complete - pick an address, connect to them (or retry to another address a few times if cannot connect, this is the slowest), PickTxForSend(), exchange a bunch of P2P messages, wait for PONG and close the connection. So, from those 10 seconds, 0.001 seconds would be spent in PickTxForSend() if the queue has 100k elements in it. There are maximum 64 (MAX_PRIVATE_BROADCAST_CONNECTIONS) connections that could be doing this "concurrently". So, max 64 calls to PickTxForSend() per 10 minutes. That sounds fine to me in the extreme extreme case of 100k transactions to be broadcast.
GetStale() is called from ReattemptPrivateBroadcast() every ~3 minutes and similarly to PickTxForSend() it takes about 0.001 seconds with a 100k queue.
Holding cs_main for too long - yes, indeed, that seems problematic. The slowness does not come from the private broadcast queue but from checking many transactions for mempool acceptance. And that is a work that has to be done. It is to be expected that if somebody sends 10x more transactions, their node will do 10x more mempool acceptance checks. To avoid holding cs_main while checking all transactions it can be released now-and-then using REVERSE_LOCK() at the end of the loop. It does not have to be held for the entire loop in one go.
Given the above, what about setting a limit of 100k transactions in the private broadcast queue and adding REVERSE_LOCK() to ReattemptPrivateBroadcast()?
@vasild the OOM code is unused, I honestly don't know what the guidance is for usage of it...
What is "the OOM code"?
I agree that the re-test loop is by far the biggest cost in terms of CPU, and the most common sense mitigation would be to take the lock once for every tx separately, similarly to how we handle inbound TX messages. I'm less familiar with seemingly aggressive lock->unlock patterns so I'll defer to others chiming in.
With that, I think raising the cap seems much safer, though I'd also want the memory usage for bookkeeping (the ever accumulating broadcast attempt records) to be bounded sooner rather than later, which is probably not a backporting target and no one has even implemented. 10x here, 10x there, we might introduce problems down the line by not paying attention.
It is to be expected that if somebody sends 10x more transactions, their node will do 10x more mempool acceptance checks
Again, cautious meta-pushback here is that we may do things differently in the future, say decoy transmissions, so we should be conservative where possible.
What is "the OOM code"?
RPC_OUT_OF_MEMORY
seemingly aggressive lock->unlock patterns
Lets see, the benchmarks you made come to the help - I moved the LOCK(cs_main) from outside of the loop to inside of the loop in ReattemptTestAccept(). It didn't make a difference in the timing. I guess because lock+unlock takes much less time compared to the other work done in the loop body - ProcessTransaction(). To confirm that I tested the timings of each one:
One call to ProcessTransaction() takes about 11000-12000 μs in both PrivateBroadcastReattempt1k and PrivateBroadcastReattempt10k cases.
One lock+unlock takes about 8-9 μs.
So I think it would be overengineering to lock+unlock every N calls to ProcessTransaction() and is simpler to do so for every call to ProcessTransaction() (i.e. go with N=1). That is, the below is justified:
@@ -1645,14 +1645,14 @@ void PeerManagerImpl::ReattemptPrivateBroadcast(CScheduler& scheduler)
{
// Remove stale transactions that are no longer relevant (e.g. already in
// the mempool or mined) and count the remaining ones.
size_t num_for_rebroadcast{0};
const auto stale_txs = m_tx_for_private_broadcast.GetStale();
if (!stale_txs.empty()) {
- LOCK(cs_main);
for (const auto& stale_tx : stale_txs) {
+ LOCK(cs_main);
auto mempool_acceptable = m_chainman.ProcessTransaction(stale_tx, /*test_accept=*/true);
It is to be expected that if somebody sends 10x more transactions, their node will do 10x more mempool acceptance checks
Again, cautious meta-pushback here is that we may do things differently in the future, say decoy transmissions, so we should be conservative where possible.
Good point, I agree.
What is "the OOM code"?
RPC_OUT_OF_MEMORY
That seems to be the perfect return code from sendrawtransaction if the limit is hit.
So I think it would be overengineering to lock+unlock every N calls to ProcessTransaction() and is simpler to do so for every call to ProcessTransaction() (i.e. go with N=1). That is, the below is justified:
Done
That seems to be the perfect return code from sendrawtransaction if the limit is hit.
Done
changed to 10k items
186 | + 187 | + // Nothing was evicted: all originally-added transactions are still present. 188 | + const auto infos{pb.GetBroadcastInfo()}; 189 | + std::set<uint256> present_wtxids; 190 | + for (const auto& info : infos) { 191 | + present_wtxids.insert(info.tx->GetWitnessHash().ToUint256());
Might "require" that the insert adds a new element:
const auto& [_, inserted] = present_wtxids.insert(info.tx->GetWitnessHash().ToUint256());
BOOST_REQUIRE(inserted);
added a check for total container size at end
202 | + // Removing one frees exactly one slot for a new transaction. 203 | + BOOST_REQUIRE(pb.Remove(txs[0]).has_value()); 204 | + BOOST_CHECK_EQUAL(pb.GetBroadcastInfo().size(), num_cap - 1); 205 | + const auto fresh{MakeDummyTx(/*id=*/0xffffffff, /*num_witness=*/0)}; 206 | + BOOST_CHECK(pb.Add(fresh) == PrivateBroadcast::AddResult::Added); 207 | + BOOST_CHECK_EQUAL(pb.GetBroadcastInfo().size(), num_cap);
What about one more test to check what happens when a transaction is removed and then re-added? That is - to ensure no traces are left of it after the removal. It would be a bug if the (re)add says "already present". Or maybe not worth it?
one liner, worth it, added
40 | + # Mature one coinbase to spend. 41 | + self.generate(wallet, 101) 42 | + 43 | + # Build a parent that fans out to MAX_TRANSACTIONS + OVER_CAP outputs. 44 | + # Inject it directly via generateblock since -privatebroadcast bypasses 45 | + # the mempool
Why is it necessary to spend different outputs? Would it not suffice to create a pile of transactions that spend the same output and skip this step and make the test simpler? Those transactions would be conflicting with each other, but that should be irrelevant as they never go into the mempool. Conflicting transactions can co-exit in the private broadcast queue.
This might be overkill but it forced distinct (w)txids, I don't recall exactly why I did this and not something else like setting locktime. Ok if I just leave it?
oh just noting I'm not sure we have functional coverage of conflicting txs in queue, when the txid doesn't match?
I'm not sure we have functional coverage of conflicting txs in queue, when the txid doesn't match?
Without checking the tests, I think we do not have such tests. That seems like not an interesting thing since we do not bother to check for conflictness. That is, if conflicting transactions would not be allowed in the private broadcast queue, then we would need to have a test to ensure that this check works. Conflicting as in - they spend the same coin, aka double spend.
So I am -0.1 on adding such test as it would be a bloat to some extent. But no objection if you feel strongly that it would be useful and want to add it. Anyway, definitely out of the scope of this PR.
ACK 4c5f2a17e521e5e0e7976d1cf89b6ca82bce199e
I would go with a higher limit than 10k, but 10k seems fine as well, so lets go! :)
<details> <summary>Show Signature</summary>
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
ACK 4c5f2a17e521e5e0e7976d1cf89b6ca82bce199e
I would go with a higher limit than 10k, but 10k seems fine as well, so lets go! :)
-----BEGIN PGP SIGNATURE-----
iQRPBAEBCAA5FiEE5k2NRWFNsHVF2czBVN8G9ktVy78FAmpCo68bFIAAAAAABAAO
bWFudTIsMi41KzEuMTIsMiwzAAoJEFTfBvZLVcu/vtAf/izVMIFHTa+/Eom6MJYW
Vex3oSHUa06xQ7mfqlw/ucGJIl4iuBrLz0j4q53+/ML2qqT5VmaKVcQ2kimWnYzH
Zjhi+Nhi/G3oV0Hqn+Yot2xXeM89ZnttqFcolslPmpj84GEKg/rAwEa3FfRK6bop
574nKh+uyXcj33ZO9NPzkcSAU7M0lcHt5Vk3vx5jvwA9mQOb6JR9lZ/DmIViO/kK
VRr4WQvlnf+rR9HQHrdHkjQGEtE8esxwXgXMBUqEayPIyRxK8kghg3Mhxd4Ssf0r
leEBjGH4sz88MZ9UiJi2Q76qjQvv7qYZUOHUJPGfIRdt3Xwp+Oi4BUTbydi6oyRT
MWSRzN6XkGMkoQNBaWddDGN7F8OzsR2QQVTTRTGnwDayutMn5qOkZaA9HqQYIlUs
s7asqkZ/JNKdnn+KkWNq7P4eVT9z4sX+Uu0bPqVbuqw2BTjpsue36NxIZ75a8YaK
foLVUXBpvDWcPECfJgsEzCb8mvY4YhnA7WAB0rLsSu9ZMx5ZCFnf2Tw8tGKyn7gu
grsTHZJuwOQ7vEixtGADro02Yt0nqWQLTjVPfbGdGsDaY4CaXfIQf7pggqRLZpD3
H4arMxgWTCLGa+rh16KtcEk6gkhLTT+sLyYPAYumgy3ctAC0l48m3XvWQ3YqXM72
3Ge97I8/BbEEfUOtCU3kuXYHJoewQkYnf1SlM3NWRmvoiwejZ0/U/3Nh+BYvGdfx
pREfrc/8hfhxcPNYaTg8FUk9KhDwrmoX6mj2cAhb2apeFdDTGolmmJOAPO+zyfo3
HDV3g9S9htwm3/UGPizk0ybvbe6w+zTQw8VVDkJWXXXcZ+vt+WAkhgZ1t8Ns7D1z
25Uxhr+NemJvi8ZBfBc8jDRgs0tQccoS6236a6OAvPIA+No2yDISnUFxooodbwVT
88qbc7gMTPuVtdEvsjSr34/c1S6iyOTanPE/BI7KW6dT4qYy0vBBvBI+CePSIMvT
WhUKU1KypYokoApgkwomnRQJj8aKaMYR2Ud1Xutd9AVX779UtTZ5/bf/OJ4WY69W
pV5ad9a88HozkXTPx7BwfQcp9gBP8/szlgwIVCVlkqgqBFRvIA/XWVHqRl9pWuGA
HyKoWEPXoSRncL+aG+M80CeU3MHzI6vEI7hF4gdjsVwB8CTjFq2bkngM8hWeDYs6
T5at8e6XqbQy4c6SZogrfogC12hitkFolAKgF6mQcjivVEXE0lAuf978mYem1Y+M
hHsVflhphYTMcyTPVOxz7cJeCs2vs7Bor2xryKR4VOdsSTIMsS9GodCf7/7iaEnD
8BiOUnGQrrv/yuyAzANYaZtl1l24ioMbEBbhufMbsfIUXQHQSVEuY6ko2evcEq/5
xSM=
=uw0G
-----END PGP SIGNATURE-----
vasild's public key is on openpgp.org
</details>
72 | + self.log.info(f"Submitting {OVER_CAP} more; each should be rejected (queue full)") 73 | + for child in children[MAX_TRANSACTIONS:]: 74 | + assert_raises_rpc_error(-7, "Private broadcast queue is full", 75 | + node.sendrawtransaction, child["hex"]) 76 | + 77 | + assert_equal(pbinfo["transactions"], node.getprivatebroadcastinfo()["transactions"])
nit: pin the abortprivatebroadcast-frees-a-slot contract (per the help text and release note) with a small functional test. This is just an optional addition and non-blocking, though, since cap rejection and abort mechanics are already tested separately.
<details> <summary>diff</summary>
@@ -75,6 +75,12 @@ class PrivateBroadcastCapTest(BitcoinTestFramework):
node.sendrawtransaction, child["hex"])
assert_equal(pbinfo["transactions"], node.getprivatebroadcastinfo()["transactions"])
+ self.log.info("Checking abortprivatebroadcast frees a slot for a new submission")
+ abort_res = node.abortprivatebroadcast(children[1]["txid"])
+ assert_equal(len(abort_res["removed_transactions"]), 1)
+ assert_equal(len(node.getprivatebroadcastinfo()["transactions"]), MAX_TRANSACTIONS - 1)
+ node.sendrawtransaction(children[MAX_TRANSACTIONS]["hex"]) # first previously-rejected tx
+ assert_equal(len(node.getprivatebroadcastinfo()["transactions"]), MAX_TRANSACTIONS)
# Re-submitting an already-queued transaction is a no-op, not an error,
# even when the queue is full.
Used children[1] since the noop-resubmit check just below already operates on children[0].
</details>
took as inspiration, made tighter assertions
concept ACK
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed.
<sub>Task test ancestor commits: https://github.com/bitcoin/bitcoin/actions/runs/28450887009/job/84312520620</sub>
<sub>LLM reason (✨ experimental): CI failed because the fuzz target wouldn’t compile: AddResult was not contextually convertible to bool in private_broadcast.cpp.</sub>
<details><summary>Hints</summary>
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
</details>
Add a belt-and-suspenders feature, limit the amount of
memory and cpu possible when unlucky or simply misconfigured.
The worst case limit is roughly 400kB * 10,000 = 4GB, regardless
of usage pattern.
Before this change, sheer volume of broadcasts, mismatches in
standardness rules, or simply fee mismatches may result in unbounded
growth of memory usage. As the feature may be expanded in
the future, explicit bounds helps reasoning going forward.
This queue may be hundreds to thousands of items, each of
which are being revalidated at short interval. Allow the
node to be more responsive in this scenario.
rebased on top of new private tx relay fuzz target, giving additional coverage (and fixing silent merge conflict)
204 | + BOOST_CHECK(pb.Add(fresh) == PrivateBroadcast::AddResult::Added); 205 | + BOOST_CHECK_EQUAL(pb.GetBroadcastInfo().size(), num_cap); 206 | + 207 | + // A previously-removed tx can be added again as a brand-new entry 208 | + BOOST_REQUIRE(pb.Remove(fresh).has_value()); 209 | + BOOST_CHECK(pb.Add(txs[0]) == PrivateBroadcast::AddResult::Added);
This is correct because txs[0] was removed earlier. But looking at this snippet in isolation I find it a bit confusing because it removes one transaction and adds another one. I would expect the snippet that checks "A previously-removed tx can be added again as a brand-new entry" to do "remove X, add X".
BOOST_CHECK_EQUAL(pb.Add(txs[0]), PrivateBroadcast::AddResult::Added);
will do if I touch commit again
seems to be an old comment? resolving, please speak up if not
77 | + assert_equal(pbinfo["transactions"], node.getprivatebroadcastinfo()["transactions"]) 78 | + 79 | + self.log.info("Checking abortprivatebroadcast frees a slot for a new submission") 80 | + abort_res = node.abortprivatebroadcast(children[1]["txid"]) 81 | + assert_equal([t["wtxid"] for t in abort_res["removed_transactions"]], 82 | + [children[1]["wtxid"]])
Since this is just one transaction, no need to compare two arrays that contain just one element and can be simplified to:
assert_equal(abort_res["removed_transactions"]["wtxid"], children[1]["wtxid"])
this isn't well formatted; the way I did it here checks that the lists of aggregated wtxids match, marking as resolved for now
43 | @@ -44,15 +44,15 @@ BOOST_AUTO_TEST_CASE(basic) 44 | // Make a transaction and add it. 45 | const auto tx1{MakeDummyTx(/*id=*/1, /*num_witness=*/0)}; 46 | 47 | - BOOST_CHECK(pb.Add(tx1)); 48 | - BOOST_CHECK(!pb.Add(tx1)); 49 | + BOOST_CHECK(pb.Add(tx1) == PrivateBroadcast::AddResult::Added);
BOOST_CHECK_EQUAL(pb.Add(tx1), PrivateBroadcast::AddResult::Added);
will do if I touch commit again
43 | @@ -44,15 +44,15 @@ BOOST_AUTO_TEST_CASE(basic) 44 | // Make a transaction and add it. 45 | const auto tx1{MakeDummyTx(/*id=*/1, /*num_witness=*/0)}; 46 | 47 | - BOOST_CHECK(pb.Add(tx1)); 48 | - BOOST_CHECK(!pb.Add(tx1)); 49 | + BOOST_CHECK(pb.Add(tx1) == PrivateBroadcast::AddResult::Added); 50 | + BOOST_CHECK(pb.Add(tx1) == PrivateBroadcast::AddResult::AlreadyPresent);
BOOST_CHECK_EQUAL(pb.Add(tx1), PrivateBroadcast::AddResult::AlreadyPresent);
will do if I touch commit again
49 | + BOOST_CHECK(pb.Add(tx1) == PrivateBroadcast::AddResult::Added); 50 | + BOOST_CHECK(pb.Add(tx1) == PrivateBroadcast::AddResult::AlreadyPresent); 51 | 52 | // Make another transaction with same txid, different wtxid and add it. 53 | const auto tx2{MakeDummyTx(/*id=*/1, /*num_witness=*/1)}; 54 | BOOST_REQUIRE(tx1->GetHash() == tx2->GetHash());
BOOST_REQUIRE_EQUAL(tx1->GetHash(), tx2->GetHash());
don't think this is me
50 | + BOOST_CHECK(pb.Add(tx1) == PrivateBroadcast::AddResult::AlreadyPresent);
51 |
52 | // Make another transaction with same txid, different wtxid and add it.
53 | const auto tx2{MakeDummyTx(/*id=*/1, /*num_witness=*/1)};
54 | BOOST_REQUIRE(tx1->GetHash() == tx2->GetHash());
55 | BOOST_REQUIRE(tx1->GetWitnessHash() != tx2->GetWitnessHash());
BOOST_REQUIRE_NE(tx1->GetWitnessHash(), tx2->GetWitnessHash());
Not sure how this happened, but feel free to ignore suggestions for lines that are not modified or added by this PR.
don't think this is me
53 | const auto tx2{MakeDummyTx(/*id=*/1, /*num_witness=*/1)};
54 | BOOST_REQUIRE(tx1->GetHash() == tx2->GetHash());
55 | BOOST_REQUIRE(tx1->GetWitnessHash() != tx2->GetWitnessHash());
56 |
57 | - BOOST_CHECK(pb.Add(tx2));
58 | + BOOST_CHECK(pb.Add(tx2) == PrivateBroadcast::AddResult::Added);
BOOST_CHECK_EQUAL(pb.Add(tx2), PrivateBroadcast::AddResult::Added);
will do if I touch commit again
56 | 57 | - BOOST_CHECK(pb.Add(tx2)); 58 | + BOOST_CHECK(pb.Add(tx2) == PrivateBroadcast::AddResult::Added); 59 | const auto find_tx_info{[](auto& infos, const CTransactionRef& tx) -> const PrivateBroadcast::TxBroadcastInfo& { 60 | const auto it{std::ranges::find(infos, tx->GetWitnessHash(), [](const auto& info) { return info.tx->GetWitnessHash(); })}; 61 | BOOST_REQUIRE(it != infos.end());
BOOST_REQUIRE_NE(it, infos.end());
will do if I touch commit again
145 | @@ -146,7 +146,7 @@ BOOST_AUTO_TEST_CASE(stale_unpicked_tx) 146 | 147 | PrivateBroadcast pb; 148 | const auto tx{MakeDummyTx(/*id=*/42, /*num_witness=*/0)}; 149 | - BOOST_REQUIRE(pb.Add(tx)); 150 | + BOOST_REQUIRE(pb.Add(tx) == PrivateBroadcast::AddResult::Added);
BOOST_REQUIRE_EQUAL(pb.Add(tx), PrivateBroadcast::AddResult::Added);
will do if I touch commit again
167 | + // Fill the queue exactly to the cap; every distinct Add() succeeds. 168 | + std::vector<CTransactionRef> txs; 169 | + txs.reserve(num_cap); 170 | + for (size_t i{0}; i < num_cap; ++i) { 171 | + auto tx{MakeDummyTx(/*id=*/static_cast<uint32_t>(i), /*num_witness=*/0)}; 172 | + BOOST_REQUIRE(pb.Add(tx) == PrivateBroadcast::AddResult::Added);
BOOST_REQUIRE_EQUAL(pb.Add(tx), PrivateBroadcast::AddResult::Added);
will do if I touch commit again
175 | + BOOST_CHECK_EQUAL(pb.GetBroadcastInfo().size(), num_cap); 176 | + 177 | + // Further distinct transactions are rejected, and the queue is unchanged. 178 | + for (size_t i{0}; i < num_over; ++i) { 179 | + const auto tx{MakeDummyTx(/*id=*/static_cast<uint32_t>(num_cap + i), /*num_witness=*/0)}; 180 | + BOOST_CHECK(pb.Add(tx) == PrivateBroadcast::AddResult::QueueFull);
BOOST_CHECK_EQUAL(pb.Add(tx), PrivateBroadcast::AddResult::QueueFull);
will do if I touch commit again
192 | + BOOST_CHECK_MESSAGE(present_wtxids.contains(txs[i]->GetWitnessHash().ToUint256()), 193 | + "tx index " << i << " should still be present"); 194 | + } 195 | + 196 | + // Re-adding an already-present tx is AlreadyPresent even at the cap (not QueueFull). 197 | + BOOST_CHECK(pb.Add(txs[0]) == PrivateBroadcast::AddResult::AlreadyPresent);
BOOST_CHECK_EQUAL(pb.Add(txs[0]), PrivateBroadcast::AddResult::AlreadyPresent);
will do if I touch commit again
199 | + 200 | + // Removing one frees exactly one slot for a new transaction. 201 | + BOOST_REQUIRE(pb.Remove(txs[0]).has_value()); 202 | + BOOST_CHECK_EQUAL(pb.GetBroadcastInfo().size(), num_cap - 1); 203 | + const auto fresh{MakeDummyTx(/*id=*/0xffffffff, /*num_witness=*/0)}; 204 | + BOOST_CHECK(pb.Add(fresh) == PrivateBroadcast::AddResult::Added);
BOOST_CHECK_EQUAL(pb.Add(fresh), PrivateBroadcast::AddResult::Added);
will do if I touch commit again
ACK 4e29de719e6dc2a080f8a0c00cd137d3cd4dac96
<details> <summary>Show Signature</summary>
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
ACK 4e29de719e6dc2a080f8a0c00cd137d3cd4dac96
-----BEGIN PGP SIGNATURE-----
iQRPBAEBCAA5FiEE5k2NRWFNsHVF2czBVN8G9ktVy78FAmpH62QbFIAAAAAABAAO
bWFudTIsMi41KzEuMTIsMiwzAAoJEFTfBvZLVcu/D9Af/jYQkC9sv7v1eb9AzExV
/9GmKlG+8wKAedBY3BgyoUzeykxq0FLSN3zRGHk9R5E0IJ5qiPveQhOApXqrNEQR
ajdUVCHAef+wX8pbcR2r+6IrqcrUF7AAsT3BC1XHMvsbG8ErMJ+WpiBuE1S9Ipw1
hM2i8nmX9g2Yp6Pd2sTaucEu4mgEl4fENm97SwJfo7SXKoMPM77JXtROrnjeiJO6
6gnm0Wtng0JkqQVCkjYSGgbcsVXAkgLB0+3P4IVYtrLE/zKbiZEJzK0f1wj9t1Og
gaMTGboNpznh6FqIv/CQMdeWAC9pLeSyiB5ukmoWAEsUgvdwtiLnO4k4n3TCoiQY
7awRa0+I5qUopdURewV+U9X9xn/CijZWjLuk2DKz38G8etSqMpzj/AY0ndop9n52
RXzQvRM6GkvrlBjzrDv24stpQR9Mk4abvymSynrTzs+kNSN0yJWbYvXiP4nhmXn0
P9dR0k109UY32y+QFCMMy7H1Tp3S+xg9yEPC98y/W2JV1KjdPJ7SFVLyatjLZAo8
XdGg00crmd3f5hyuHghFc62RvAG86jn+Y4BFNp3IhUYUhxeExoN6xFIM785QPINO
k9e226pa/mZ5sMnvjcfSWaZorwI+0S+jU3GjMxCu0z1iM2hNJHuQp57rks8OaJJh
sdekUst4HyMPwmOvLfgsHRq2+Cr6iX7mvYzjt0/W+jIg71r3xwNSRG7A80KCUWZ5
Ksp6w22242HppXeCfll3QnUh1a8J2gV2oF0WXBB6/e4/lE9adI/5VI5lECJoAGnm
pTRAigTyHbeFjfPRyNupRGr5oSkVMCvAoucWwgerEzsLC/R8TYRTUEKY+mEdPCB+
+mF/jFNmJPpfdrQ3PN0ucM8PyYD7ht5WVHY5VbPO2KMacfIvtxqOuUuVZQ2JdZpV
19NVwAvuXT7tLYozlQFVzfBLEmhWV61KGLD5NpZVw9deCwOJmUF9CHNvxsSNX3KE
YHOCw92DMEnYhwxIQ7tPAcJgXbAZhvp2IrETQHEcsQzVD6Bu/J8zm1zCXzp9Z2s5
jE1qpdpwWA6jisDcIi6DzCOfhoSAVHiN2MSD1KmqG303VewLxvUf0ck3Rc9snySo
L6OKh+/aptApkGOh4waOdCijV73KLkjpluPeDUK7TL+BgDF8mxVJRbxpnWrk0M14
Drc9fsUJAJb+GE4XfAdcIyP7pEJcxELh9Rz985jpLCrDvj7OV63rG3Tb27J2yNOU
/kXbg7Iz0JTLlVcwTUoP3E3lbD2VN6K8zMhIBm2DC0W9fUXvg8gdaB1DGK7b0XCH
rZCxcabN78VA3LicgkcwGAdZV+c/xZ/GcDjaOgOrBRxF543t/2dyaE2qJQcrTBs0
LRM=
=qeFE
-----END PGP SIGNATURE-----
vasild's public key is on openpgp.org
</details>
157 | @@ -158,4 +158,56 @@ BOOST_AUTO_TEST_CASE(stale_unpicked_tx) 158 | BOOST_CHECK_EQUAL(stale_state[0], tx); 159 | } 160 | 161 | +BOOST_AUTO_TEST_CASE(rejection_at_cap) 162 | +{ 163 | + PrivateBroadcast pb;
We could use a custom max count here to test non-default?
I added the custom cap to make fuzz target more efficient; leaving as-is unless I'm missing a wrinkle
ACK 4e29de719e6dc2a080f8a0c00cd137d3cd4dac96
tACK 4e29de719e6dc2a080f8a0c00cd137d3cd4dac96. Ran private_broadcast_tests and p2p_private_broadcast_cap.py. Great to have an explicit bound as the belt-and-suspenders against unbounded queue growth.
Not touching commits for now, thanks for all the review everyone so far!
394 | @@ -395,6 +395,8 @@ RPCErrorCode RPCErrorFromTransactionError(TransactionError terr) 395 | return RPC_TRANSACTION_REJECTED; 396 | case TransactionError::ALREADY_IN_UTXO_SET: 397 | return RPC_VERIFY_ALREADY_IN_UTXO_SET; 398 | + case TransactionError::PRIVATE_BROADCAST_FULL: 399 | + return RPC_OUT_OF_MEMORY;
I don't think RPC_OUT_OF_MEMORY is a sane code to use here. OOM is a fairly general and well established term, and I don't think we should give the wrong impression here that the server is running out of memory when it isn't.
RPC_OUT_OF_MEMORY usage was removed in commits cf0c47b269 and dd87b6dff3. In both cases, it seems like the implementation wrongly assumed there were OOM issues from a failing new resulting in a nullptr, when actually std::bad_alloc would've been throw, so the RPC_OUT_OF_MEMORY code was seemingly intended for memory allocation issues but never actually used that way. (note: until cf0c47b269, the RPC_OUT_OF_MEMORY code could also be returned for failing wallet keypool issues, which doesn't seem OOM related at all).
The impact is probably limited, so I don't think this should be a blocker, so I'm happy to leave as-is if you prefer, but I wanted to raise that I think this sets bad precedent. Adding a dedicated RPC code seems straightforward enough.
8 | 9 | #include <algorithm> 10 | 11 | 12 | -bool PrivateBroadcast::Add(const CTransactionRef& tx) 13 | +[[nodiscard]] PrivateBroadcast::AddResult PrivateBroadcast::Add(const CTransactionRef& tx)
nit: [[nodiscard]] is a declaration attribute, and it's correctly set in the header (sorry, I'm aware I left earlier comment in the wrong place)
PrivateBroadcast::AddResult PrivateBroadcast::Add(const CTransactionRef& tx)
ACK 4e29de719e6dc2a080f8a0c00cd137d3cd4dac96
I think the RPC_OUT_OF_MEMORY code sets bad precedent and I would prefer it being fixed in this PR, but am also happy open to open a follow-up if the author disagrees, so not considered blocking.
Opened #35678 to address the RPC error code, and other outstanding suggestions/nits I found here. Ping me if there are any I've missed that you'd like to add.