Follow-up cleanup in the private broadcast subsystem.
This PR was split out from the discussion in #34707.
This change removes duplication between the internal private broadcast state and the snapshot used by PeerManager, RPC, and tests.
PeerSendInfo and TxSendStatus.SendStatus directly for per-peer broadcast status.PrivateBroadcast::Transactions as the snapshot type.getprivatebroadcastinfo JSON output unchanged.No behavioral changes intended.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | vasild |
| Stale ACK | danielabrozzoni |
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-->
Looks like this was pulled out of the conversation here #34707 (review).
cc @vasild
https://github.com/bitcoin/bitcoin/actions/runs/24045821228/job/70255455947?pr=35016#step:9:1647:
[ 21%] Building CXX object src/CMakeFiles/bitcoin_node.dir/rpc/mempool.cpp.o
cd /home/admin/actions-runner/_work/bitcoin/bitcoin/ci_build/src && /usr/bin/ccache /usr/bin/clang++ -DABORT_ON_FAILED_ASSUME -DBOOST_MULTI_INDEX_DISABLE_SERIALIZATION -DBOOST_NO_CXX98_FUNCTION_BASE -DDEBUG -DDEBUG_LOCKCONTENTION -DDEBUG_LOCKORDER -DENABLE_EMBEDDED_ASMAP=1 -DENABLE_ZMQ=1 -DRPC_DOC_CHECK -I/home/admin/actions-runner/_work/bitcoin/bitcoin/ci_build/src -I/home/admin/actions-runner/_work/bitcoin/bitcoin/src -I/home/admin/actions-runner/_work/bitcoin/bitcoin/src/leveldb/include -I/home/admin/actions-runner/_work/bitcoin/bitcoin/src/minisketch/include -I/home/admin/actions-runner/_work/bitcoin/bitcoin/src/univalue/include -Wno-error=unused-member-function -O0 -ftrapv -g3 -std=c++20 -fPIC -fmacro-prefix-map=/home/admin/actions-runner/_work/bitcoin/bitcoin/src=. -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3 -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection -Wall -Wextra -Wgnu -Wcovered-switch-default -Wformat -Wformat-security -Wvla -Wshadow-field -Wthread-safety -Wloop-analysis -Wredundant-decls -Wunused-member-function -Wdate-time -Wconditional-uninitialized -Woverloaded-virtual -Wsuggest-override -Wimplicit-fallthrough -Wunreachable-code -Wdocumentation -Wself-assign -Wundef -Wno-unused-parameter -Werror -MD -MT src/CMakeFiles/bitcoin_node.dir/rpc/mempool.cpp.o -MF CMakeFiles/bitcoin_node.dir/rpc/mempool.cpp.o.d -o CMakeFiles/bitcoin_node.dir/rpc/mempool.cpp.o -c /home/admin/actions-runner/_work/bitcoin/bitcoin/src/rpc/mempool.cpp -O3 -g2
/home/admin/actions-runner/_work/bitcoin/bitcoin/src/rpc/mempool.cpp:187:86: error: no member named 'time_added' in 'PrivateBroadcast::TxBroadcastInfo'
187 | o.pushKV("time_added", TicksSinceEpoch<std::chrono::seconds>(tx_info.time_added));
| ~~~~~~~ ^
/home/admin/actions-runner/_work/bitcoin/bitcoin/src/rpc/mempool.cpp:189:49: error: no member named 'peers' in 'PrivateBroadcast::TxBroadcastInfo'
189 | for (const auto& peer : tx_info.peers) {
| ~~~~~~~ ^
2 errors generated.
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed.
<sub>Task test ancestor commits: https://github.com/bitcoin/bitcoin/actions/runs/24045821228/job/70255455947</sub>
<sub>LLM reason (✨ experimental): CI failed due to a build error while compiling rpc/mempool.cpp.o for bitcoin_node (CMake/gmake returned non-zero).</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>
Fixed. The CI failure was due to a non-atomic commit split: the first commit changed the TxBroadcastInfo layout while rpc/mempool.cpp was only updated in the second commit. Squashed them into one so each commit builds cleanly.
I also noticed this PR overlaps with the dedup suggested by @vasild in #34707 (comment). Happy to rebase on top of #34707 or close this if it becomes redundant after that PR lands.
156 | @@ -157,4 +157,53 @@ BOOST_AUTO_TEST_CASE(stale_unpicked_tx) 157 | BOOST_CHECK_EQUAL(stale_state[0], tx); 158 | } 159 | 160 | +BOOST_AUTO_TEST_CASE(snapshot_structural_invariant)
This newly added test overlaps with the test basic earlier in this file. It looks like that it covers the same paths and checks the same things, except the check in the new test BOOST_CHECK(info.status.time_added <= NodeClock::now());. It would be better to extend the existent check_peer_counts() in basic with that check and drop the newly added test.
The time_added check is not very interesting given that the time is mocked and frozen. I guess it can use == instead of <=, then it would catch a problem if time_added is 0 for example.
Good point, thanks. I'll fold that into the existing basic test, drop the new test, and use == for the mocked time check.
54 | + std::optional<NodeClock::time_point> confirmed; 55 | + 56 | + SendStatus(const NodeId& nodeid, const CService& address, const NodeClock::time_point& picked) : nodeid{nodeid}, address{address}, picked{picked} {} 57 | + }; 58 | + 59 | + struct TxSendStatus {
Can drop TxSendStatus as well. Maybe just take the patch from #34707 (review) verbatim.
Thanks, I'll take a look at the #34707 patch and fold that in as well if it keeps the scope narrow.
Concept ACK
It is convenient if you cross link a PR in case it has a specific motivator. In this case "This PR is inspired by #34707 (review).
Thanks!
156 | @@ -157,4 +157,53 @@ BOOST_AUTO_TEST_CASE(stale_unpicked_tx) 157 | BOOST_CHECK_EQUAL(stale_state[0], tx); 158 | } 159 | 160 | +BOOST_AUTO_TEST_CASE(snapshot_structural_invariant) 161 | +{ 162 | + SetMockTime(Now<NodeSeconds>());
This will modify a global and leave it modified after the unit test. Please use NodeClockContext clock_ctx{}; in new code.
Good catch, thanks. I'll avoid modifying the global mock time in the new code and use NodeClockContext clock_ctx{} instead.
Addressed the remaining review comments in 4a89e31da7.
Tested:
./build_pr35016_linux/bin/test_bitcoin --run_test=private_broadcast_testspython3 ./build_pr35016_linux/test/functional/test_runner.py p2p_private_broadcast.py -j 1
164 | const auto stale_state{pb.GetStale()};
165 | BOOST_REQUIRE_EQUAL(stale_state.size(), 1);
166 | BOOST_CHECK_EQUAL(stale_state[0], tx);
167 | }
168 |
169 | -BOOST_AUTO_TEST_CASE(snapshot_structural_invariant)
26 | @@ -26,7 +27,7 @@ static CTransactionRef MakeDummyTx(uint32_t id, size_t num_witness) 27 | 28 | BOOST_AUTO_TEST_CASE(basic) 29 | { 30 | - SetMockTime(Now<NodeSeconds>()); 31 | + NodeClockContext clock_ctx{};
185 | @@ -190,7 +186,7 @@ class PrivateBroadcast 186 | EXCLUSIVE_LOCKS_REQUIRED(m_mutex); 187 | 188 | mutable Mutex m_mutex; 189 | - std::unordered_map<CTransactionRef, TxSendStatus, CTransactionRefHash, CTransactionRefComp> 190 | + std::unordered_map<CTransactionRef, TxBroadcastInfo, CTransactionRefHash, CTransactionRefComp>
4a89e31 net: drop TxSendStatus from private broadcast snapshot:
With this change from using TxSendStatus to TxBroadcastInfo, an additional 3rd CTransactionRef tx field is included. I feel this side effect is important enough to be explained in the commit message.
Also, please fix the formatting of the commit message (\n characters show up instead of newline).
Update: see #35016 (comment)
A relatively straightforward merging of duplicated structs, done in two stages to minimize diffs. An existing unit test has been also extended with some extra checks. I have verified by reimplementing the moves and updates. Left some remarks, none major.
Thanks for the review. I've cleaned up the history to remove the transient snapshot_structural_invariant add/remove step, split the NodeClockContext changes into a final test-only commit, and clarified the duplicated CTransactionRef storage tradeoff in the main commit message while fixing its formatting.
Thanks for the changes.
I've cleaned up the history to remove the [...] add/remove step
By suggesting to remove the adding of a large 'temporary' test that is removed in the next commit, I didn't mean to squash to two commits, but I had in mind changing the first commit (and the second too, but that's an automatic merge-type operation). In this particular case the squashed commit is fine, but in general in such cases it's better to preserve the original commits (unless there is an explicit motivation for squash). So it's OK, this remark is more for the future.
cr 9bb2e40f9598787e792f8632cfb09ca6856835b0
My previous concerns have been addressed, LGTM. (https://github.com/bitcoin/bitcoin/pull/35016#pullrequestreview-4132276136)
tACK 9bb2e40f9598787e792f8632cfb09ca6856835b0
The cleanup looks good to me. The PR is removing TxSendStatus and PeerSendInfo, in favor of using TxBroadcastInfo and SendStatus everywhere in the code. As a consequence, SendStatus is moved to public scope.
Also nice to see the private_broadcasts tests use NodeClockContext instead of SetMockTime, as far as I understand this is preferred in unit tests as it doesn't need to be manually cleaned up at the end of the test.
55 | + 56 | + SendStatus(const NodeId& nodeid, const CService& address, const NodeClock::time_point& picked) : nodeid{nodeid}, address{address}, picked{picked} {} 57 | }; 58 | 59 | struct TxBroadcastInfo { 60 | CTransactionRef tx;
In the commit message of 232f1c1970219b1f0580deb82384f1d78125f7fd net: deduplicate private broadcast state and snapshot:
As a narrow tradeoff, the m_transactions value now stores TxBroadcastInfo, which includes tx, so CTransactionRef is present in both the map key and value.
This is not good and can be avoided. Here is a change on top of this PR that further simplifies the things (and is net -10 lines):
tx from TxBroadcastInfoTxBroadcastInfo to Statusm_transactions from GetBroadcastInfo()m_transactions in the public: sectionfind_tx_info() is not needed anymore, so drop it<details> <summary>[patch] drop CTransactionRef from TxBroadcastInfo and rename it</summary>
diff --git i/src/net_processing.cpp w/src/net_processing.cpp
index 4e102730fb..d91e7e6d46 100644
--- i/src/net_processing.cpp
+++ w/src/net_processing.cpp
@@ -535,13 +535,13 @@ public:
void CheckForStaleTipAndEvictPeers() override;
util::Expected<void, std::string> FetchBlock(NodeId peer_id, const CBlockIndex& block_index) override
EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
bool GetNodeStateStats(NodeId nodeid, CNodeStateStats& stats) const override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
std::vector<node::TxOrphanage::OrphanInfo> GetOrphanTransactions() override EXCLUSIVE_LOCKS_REQUIRED(!m_tx_download_mutex);
PeerManagerInfo GetInfo() const override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
- std::vector<PrivateBroadcast::TxBroadcastInfo> GetPrivateBroadcastInfo() const override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
+ PrivateBroadcast::Transactions GetPrivateBroadcastInfo() const override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
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);
void InitiateTxBroadcastPrivate(const CTransactionRef& tx) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
void SetBestBlock(int height, std::chrono::seconds time) override
{
@@ -1852,25 +1852,24 @@ PeerManagerInfo PeerManagerImpl::GetInfo() const
return PeerManagerInfo{
.median_outbound_time_offset = m_outbound_time_offsets.Median(),
.ignores_incoming_txs = m_opts.ignore_incoming_txs,
};
}
-std::vector<PrivateBroadcast::TxBroadcastInfo> PeerManagerImpl::GetPrivateBroadcastInfo() const
+PrivateBroadcast::Transactions PeerManagerImpl::GetPrivateBroadcastInfo() const
{
return m_tx_for_private_broadcast.GetBroadcastInfo();
}
std::vector<CTransactionRef> PeerManagerImpl::AbortPrivateBroadcast(const uint256& id)
{
const auto snapshot{m_tx_for_private_broadcast.GetBroadcastInfo()};
std::vector<CTransactionRef> removed_txs;
size_t connections_cancelled{0};
- for (const auto& tx_info : snapshot) {
- const CTransactionRef& tx{tx_info.tx};
+ for (const auto& [tx, _] : snapshot) {
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);
if (NUM_PRIVATE_BROADCAST_PER_TX > *peer_acks) {
connections_cancelled += (NUM_PRIVATE_BROADCAST_PER_TX - *peer_acks);
}
diff --git i/src/net_processing.h w/src/net_processing.h
index d2050d8f3d..60a6ce6d00 100644
--- i/src/net_processing.h
+++ w/src/net_processing.h
@@ -116,13 +116,13 @@ public:
virtual std::vector<node::TxOrphanage::OrphanInfo> GetOrphanTransactions() = 0;
/** Get peer manager info. */
virtual PeerManagerInfo GetInfo() const = 0;
/** Get info about transactions currently being privately broadcast. */
- virtual std::vector<PrivateBroadcast::TxBroadcastInfo> GetPrivateBroadcastInfo() const = 0;
+ virtual PrivateBroadcast::Transactions GetPrivateBroadcastInfo() const = 0;
/**
* Abort private broadcast attempts for transactions currently being privately broadcast.
*
* [@param](/bitcoin-bitcoin/contributor/param/)[in] id A transaction identifier. It will be matched against both txid and wtxid for
* all transactions in the private broadcast queue.
diff --git i/src/private_broadcast.cpp w/src/private_broadcast.cpp
index 930da217c8..9763dfcf9c 100644
--- i/src/private_broadcast.cpp
+++ w/src/private_broadcast.cpp
@@ -9,13 +9,13 @@
bool PrivateBroadcast::Add(const CTransactionRef& tx)
EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
{
LOCK(m_mutex);
- const bool inserted{m_transactions.try_emplace(tx, TxBroadcastInfo{.tx = tx}).second};
+ const bool inserted{m_transactions.try_emplace(tx).second};
return inserted;
}
std::optional<size_t> PrivateBroadcast::Remove(const CTransactionRef& tx)
EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
{
@@ -100,24 +100,17 @@ std::vector<CTransactionRef> PrivateBroadcast::GetStale() const
if (p.last_confirmed < now - STALE_DURATION) stale.push_back(tx);
}
}
return stale;
}
-std::vector<PrivateBroadcast::TxBroadcastInfo> PrivateBroadcast::GetBroadcastInfo() const
+PrivateBroadcast::Transactions PrivateBroadcast::GetBroadcastInfo() const
EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
{
LOCK(m_mutex);
- std::vector<TxBroadcastInfo> entries;
- entries.reserve(m_transactions.size());
-
- for (const auto& entry : m_transactions) {
- entries.emplace_back(entry.second);
- }
-
- return entries;
+ return m_transactions;
}
PrivateBroadcast::Priority PrivateBroadcast::DerivePriority(const std::vector<SendStatus>& sent_to)
{
Priority p;
p.num_picked = sent_to.size();
diff --git i/src/private_broadcast.h w/src/private_broadcast.h
index 33878e0723..893c3b26f9 100644
--- i/src/private_broadcast.h
+++ w/src/private_broadcast.h
@@ -49,18 +49,38 @@ public:
/// When was the transaction reception confirmed by the node (by PONG).
std::optional<NodeClock::time_point> confirmed;
SendStatus(const NodeId& nodeid, const CService& address, const NodeClock::time_point& picked) : nodeid{nodeid}, address{address}, picked{picked} {}
};
- struct TxBroadcastInfo {
- CTransactionRef tx;
+ /// Complete status of a transaction.
+ struct Status {
+ /// When was the transaction added to the queue.
const NodeClock::time_point time_added{NodeClock::now()};
+ /// All send attempts.
std::vector<SendStatus> send_statuses{};
};
+ // No need for salted hasher because we are going to store just a bunch of locally originating transactions.
+
+ struct CTransactionRefHash {
+ size_t operator()(const CTransactionRef& tx) const
+ {
+ return static_cast<size_t>(tx->GetWitnessHash().ToUint256().GetUint64(0));
+ }
+ };
+
+ struct CTransactionRefComp {
+ bool operator()(const CTransactionRef& a, const CTransactionRef& b) const
+ {
+ return a->GetWitnessHash() == b->GetWitnessHash(); // If wtxid equals, then txid also equals.
+ }
+ };
+
+ using Transactions = std::unordered_map<CTransactionRef, Status, CTransactionRefHash, CTransactionRefComp>;
+
/**
* Add a transaction to the storage.
* [@param](/bitcoin-bitcoin/contributor/param/)[in] tx The transaction to add.
* [@retval](/bitcoin-bitcoin/contributor/retval/) true The transaction was added.
* [@retval](/bitcoin-bitcoin/contributor/retval/) false The transaction was already present.
*/
@@ -125,13 +145,13 @@ public:
std::vector<CTransactionRef> GetStale() const
EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
/**
* Get stats about all transactions currently being privately broadcast.
*/
- std::vector<TxBroadcastInfo> GetBroadcastInfo() const
+ Transactions GetBroadcastInfo() const
EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
private:
/// Cumulative stats from all the send attempts for a transaction. Used to prioritize transactions.
struct Priority {
size_t num_picked{0}; ///< Number of times the transaction was picked for sending.
@@ -152,28 +172,12 @@ private:
/// A pair of a transaction and a sent status for a given node. Convenience return type of GetSendStatusByNode().
struct TxAndSendStatusForNode {
const CTransactionRef& tx;
SendStatus& send_status;
};
- // No need for salted hasher because we are going to store just a bunch of locally originating transactions.
-
- struct CTransactionRefHash {
- size_t operator()(const CTransactionRef& tx) const
- {
- return static_cast<size_t>(tx->GetWitnessHash().ToUint256().GetUint64(0));
- }
- };
-
- struct CTransactionRefComp {
- bool operator()(const CTransactionRef& a, const CTransactionRef& b) const
- {
- return a->GetWitnessHash() == b->GetWitnessHash(); // If wtxid equals, then txid also equals.
- }
- };
-
/**
* Derive the sending priority of a transaction.
* [@param](/bitcoin-bitcoin/contributor/param/)[in] sent_to List of nodes that the transaction has been sent to.
*/
static Priority DerivePriority(const std::vector<SendStatus>& sent_to);
@@ -183,11 +187,10 @@ private:
* send any transaction to the given node.
*/
std::optional<TxAndSendStatusForNode> GetSendStatusByNode(const NodeId& nodeid)
EXCLUSIVE_LOCKS_REQUIRED(m_mutex);
mutable Mutex m_mutex;
- std::unordered_map<CTransactionRef, TxBroadcastInfo, CTransactionRefHash, CTransactionRefComp>
- m_transactions GUARDED_BY(m_mutex);
+ Transactions m_transactions GUARDED_BY(m_mutex);
};
#endif // BITCOIN_PRIVATE_BROADCAST_H
diff --git i/src/rpc/mempool.cpp w/src/rpc/mempool.cpp
index 3cfe40aafb..c1036b05b9 100644
--- i/src/rpc/mempool.cpp
+++ w/src/rpc/mempool.cpp
@@ -173,23 +173,23 @@ static RPCMethod getprivatebroadcastinfo()
+ HelpExampleRpc("getprivatebroadcastinfo", "")
},
[](const RPCMethod& self, const JSONRPCRequest& request) -> UniValue
{
const NodeContext& node{EnsureAnyNodeContext(request.context)};
const PeerManager& peerman{EnsurePeerman(node)};
- const auto txs{peerman.GetPrivateBroadcastInfo()};
+ const auto pbinfo{peerman.GetPrivateBroadcastInfo()};
UniValue transactions(UniValue::VARR);
- for (const auto& tx_info : txs) {
+ for (const auto& [tx, status] : pbinfo) {
UniValue o(UniValue::VOBJ);
- 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));
+ o.pushKV("txid", tx->GetHash().ToString());
+ o.pushKV("wtxid", tx->GetWitnessHash().ToString());
+ o.pushKV("hex", EncodeHexTx(*tx));
+ o.pushKV("time_added", TicksSinceEpoch<std::chrono::seconds>(status.time_added));
UniValue peers(UniValue::VARR);
- for (const auto& ss : tx_info.send_statuses) {
+ for (const auto& ss : status.send_statuses) {
UniValue p(UniValue::VOBJ);
p.pushKV("address", ss.address.ToStringAddrPort());
p.pushKV("sent", TicksSinceEpoch<std::chrono::seconds>(ss.picked));
if (ss.confirmed.has_value()) {
p.pushKV("received", TicksSinceEpoch<std::chrono::seconds>(*ss.confirmed));
}
diff --git i/src/test/private_broadcast_tests.cpp w/src/test/private_broadcast_tests.cpp
index f9fd4167db..b59eb65bdb 100644
--- i/src/test/private_broadcast_tests.cpp
+++ w/src/test/private_broadcast_tests.cpp
@@ -51,22 +51,17 @@ BOOST_AUTO_TEST_CASE(basic)
const auto tx2{MakeDummyTx(/*id=*/1, /*num_witness=*/1)};
BOOST_REQUIRE(tx1->GetHash() == tx2->GetHash());
BOOST_REQUIRE(tx1->GetWitnessHash() != tx2->GetWitnessHash());
BOOST_CHECK(pb.Add(tx2));
const auto time_added{NodeClock::now()};
- const auto find_tx_info{[](const auto& infos, const CTransactionRef& tx) -> const PrivateBroadcast::TxBroadcastInfo& {
- const auto it{std::ranges::find(infos, tx->GetWitnessHash(), [](const auto& info) { return info.tx->GetWitnessHash(); })};
- BOOST_REQUIRE(it != infos.end());
- return *it;
- }};
const auto check_peer_counts{[&](size_t tx1_peer_count, size_t tx2_peer_count) {
const auto infos{pb.GetBroadcastInfo()};
BOOST_CHECK_EQUAL(infos.size(), 2);
- const auto& tx1_info{find_tx_info(infos, tx1)};
- const auto& tx2_info{find_tx_info(infos, tx2)};
+ const auto& tx1_info{infos.at(tx1)};
+ const auto& tx2_info{infos.at(tx2)};
BOOST_CHECK(tx1_info.time_added == time_added);
BOOST_CHECK(tx2_info.time_added == time_added);
BOOST_CHECK_EQUAL(tx1_info.send_statuses.size(), tx1_peer_count);
BOOST_CHECK_EQUAL(tx2_info.send_statuses.size(), tx2_peer_count);
}};
@@ -112,19 +107,19 @@ BOOST_AUTO_TEST_CASE(basic)
BOOST_CHECK(pb.DidNodeConfirmReception(recipient1));
BOOST_CHECK(!pb.DidNodeConfirmReception(recipient2));
const auto infos{pb.GetBroadcastInfo()};
BOOST_CHECK_EQUAL(infos.size(), 2);
{
- const auto& send_statuses{find_tx_info(infos, tx_for_recipient1).send_statuses};
+ const auto& send_statuses{infos.at(tx_for_recipient1).send_statuses};
BOOST_CHECK_EQUAL(send_statuses.size(), 1);
BOOST_CHECK_EQUAL(send_statuses[0].address.ToStringAddrPort(), addr1.ToStringAddrPort());
BOOST_CHECK(send_statuses[0].confirmed.has_value());
}
{
- const auto& send_statuses{find_tx_info(infos, tx_for_recipient2).send_statuses};
+ const auto& send_statuses{infos.at(tx_for_recipient2).send_statuses};
BOOST_CHECK_EQUAL(send_statuses.size(), 1);
BOOST_CHECK_EQUAL(send_statuses[0].address.ToStringAddrPort(), addr2.ToStringAddrPort());
BOOST_CHECK(!send_statuses[0].confirmed.has_value());
}
const auto stale_state{pb.GetStale()};
</details>
Apparently you mechanically took over a diff from a reviewer, without 'owning' it: there is no mention of the new commit, no explanation, blank commit message, no proper merging of the change with the pre-existing commits. This last commit is a significant change. These concerns adds to my previous reservations about commit handling in this PR. This makes reviewing difficult. Therefore I retract my previous review for now, sorry.
57 | + }; 58 | + 59 | + struct Status { 60 | + const NodeClock::time_point time_added{NodeClock::now()}; 61 | + std::vector<SendStatus> send_statuses{}; 62 | + };
It is good to have everything documented, even if the comments may seem a bit redundant. When new fields get added they are more likely to be documented if the surrounding fields are documented.
/// Complete status of a transaction.
struct Status {
/// When was the transaction added to the queue.
const NodeClock::time_point time_added{NodeClock::now()};
/// All send attempts.
std::vector<SendStatus> send_statuses{};
};
ACK 346ac95bda47f340c1599e9ede235f2596a615ae
Long lines in the commit message make it hard to read in some environments. Would be nice to follow the 50/72 rule.
<details> <summary>Show Signature</summary>
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
ACK 346ac95bda47f340c1599e9ede235f2596a615ae
Long lines in the commit message make it hard to read in _some_
environments. Would be nice to follow the [50/72 rule](https://duckduckgo.com/?q=git+commit+50+72+rule).
-----BEGIN PGP SIGNATURE-----
iQQzBAEBCAAdFiEE5k2NRWFNsHVF2czBVN8G9ktVy78FAmnveBUACgkQVN8G9ktV
y79pESAAqRiT4FR6YErMJj67XNigbBiHHVwnb4t2nTnx/wGoaQYFmoWHM3D5X2S2
FVbuNYotybyMtjb/5njJwcFsRaHd3roCJiUUdcp5xVOJ3Ubgmn1Rv0tM/ew90KNe
6MlzSj9svzz7kQQqMorzCeI1yxnyVMNE8wUbLYmwGc6/4dSOSaxdZ4thcZiouPBH
xd/cCaDyqvTJCAkF1MM/osiLvap1bCWm8H7Ag6A8IbdGWHOmLkW7EpctEQ4BqyrH
KEaNXZSPG/vv9o4VL8Sd5IIUKBhuUZ6XUnV+e0WtfQQDe0N/mb0spm+u7TB1lXbH
VXU9b7ef6E5eSQa5+Qzkg1l3ygFAlT3yeOTyGgmNOSEWVOkpiilsNwpnIPO/gMYe
B+zACsPpysfUr8zs+GAWeRjgSkL9/y4b2MR5JCuYJ0k3leKyZX8pXW7R4qdHSfJB
5Ik61xdrZM6oMY40+pZJB4XdRvnc5hehTBgXchmbzFvYMjKAKo/HFbdYkeRnN2LG
9xaR/eGEo7UvPfkLk5mD9gTHEAGaIN5kyM/pwrClpwjdOKivehfQhinbDctaFdC5
69iV1VQkwBKU3v5W2AEcSLowx5yvXXxPTdBIk9PFb4zhMbW4ofNS08jOxXrO76lk
cSYeBRdZRI/BKTpX35f9v/LIQkjVKSVouVcud7t2ICYugsEmSLJV1Ot7xtC5uC6D
BlDkU2SbuZpiMYfhz5qWAZ82stkP9MADyilHmFBcTbdtCJaWooBf1uY5L5QeqvN9
UTvqldy3jvzT7PV8tsv8lw2grhAOLUe2eQoH4hnLRwvTL4jguarwErs1ySeR4QIe
XuUCnpqgBDl9tp5Bu+pkE3MWDH673PV9y81qem02UwmR64dIegG0vUlLkt9eQOV1
cZhGfEvwfo1ZIBvMtIQUYINkBFmClQdyFFX+5Bm5vxSZ94WMjvd80lUGT8OfHVZe
7ijlKePHD5UulnIqRAnDue/H5mbbFwNFd3YbZOc4892VmiD2B/aEowv56UKa4zrm
yCQ2hglpp/SK+U446S0NUorrM0q5uYaG0Z1l5fQyZSfR7YCWAm+RRA2Yi9Z6yhuh
CQUPDfwtEDRUR0eunqlEKWuge0wKbd7axhTl+I1Zal7aNy4JV6hBEvgYXjaYDYiA
n3dwOM8qPShFIeUhDNaL5uMSwO1Difv9jgp7243iJMb3JKUvhNYQUseJSKh1/z96
BgXPZAUux6V4l1N3mlZ+RoxQhwl7sfS3r3oMdusoXBbQFKLYn9LLmBBvdfiaeCgT
fDEhPqAtF6esYbVp6sI/0TObCHbMlfzRnaYAAgNKMm+0vRObWCMgP7UgMpboxMfz
HlA+a40yTGGMyyfMr+GyuBJO4mvIEA==
=KJyR
-----END PGP SIGNATURE-----
vasild's public key is on openpgp.org
</details>
Store private broadcast entries in a transaction-keyed map and
return that map as the snapshot from GetBroadcastInfo().
Remove the snapshot-only wrapper types and avoid storing the same
CTransactionRef in both the map key and value. Callers use the
CTransactionRef key for transaction data, while Status keeps the
per-transaction broadcast state.
The getprivatebroadcastinfo JSON output is unchanged.
No behavior change intended.
Move the private_broadcast_tests mock-time handling to
NodeClockContext.
Keep this test-only time cleanup separate from the private broadcast
state/snapshot cleanup.
Thanks @vasild and @optout21, addressed in e6e6c3fa34.
Changes:
PrivateBroadcast::Status and its fields as suggested;getprivatebroadcastinfo JSON remains unchanged.Range-diff versus 346ac95 is limited to documentation and commit message cleanup.
Tested:
git diff --check origin/master...HEADcmake --build build_pr35016_linux -j"$(nproc)" --target bitcoin_node test_bitcoin./build_pr35016_linux/bin/test_bitcoin --run_test=private_broadcast_testspython3 ./build_pr35016_linux/test/functional/test_runner.py p2p_private_broadcast.py -j 1bitcoin_node, test_bitcoin, and private_broadcast_tests
ACK e6e6c3fa34c61572fafc9e26b01d7d259ec399a8
<details> <summary>Show Signature</summary>
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
ACK e6e6c3fa34c61572fafc9e26b01d7d259ec399a8
-----BEGIN PGP SIGNATURE-----
iQQzBAEBCAAdFiEE5k2NRWFNsHVF2czBVN8G9ktVy78FAmnyCPEACgkQVN8G9ktV
y7/ipR/+KSSnzWAuA+AMgbQt4uJB98v18dtnHhAy+Cdu/74FUBTDda4l8/YWWJhO
QHnvT8V3mEA/4AfnbE5DFStmw1l0wwsfBCouGT7vuk5VNz+gg2p1JHVjGaEL3Ap2
GaQXCbL7txh1Cib9W+YmwDB7cO6/CDG2EjBs7XPp4XB2h9QxgWjrlPY2ysZioeTw
T7wBkNtu2v9dzdHOLthJ0rG4/hjRJFnLT4uRoqs0nBBzd1lHjF2faB5zzCSyUysg
vX9dAKxcUo3qW1GAymw+ZsVAHVo0zf4lQNZop1BL3Ok2w/LNAP4Zr1ijqXGcQljy
OzeuOoy6/ihXKI58Sy0YsiHqEdhMAtc6thHlpmah/pFzboucrLrO5Jkca1xv12Do
V//dpworUzYjnLTHIwV3zuagXi68TH8u08CU02wyLCuLc5MYXtx/MAU7JksbE8vE
jjTLq/vtuyApKaTnN3Sx9itzTn6R2C+HeHKjCkUTOv5YMdBgftP4x7WYehzrS43M
M/+LozU8q0Dc0sAbr2Yf7IXtSwJW8bZKv6uzBnCWyRK6I2TdHBDSG/EQdoZssHfo
5lHJTM5y5hVOMWqFFChSPy3lAjTLMMrdzKhhPO7CVpkZCRWyKTu/b4KkxApHL+IO
zWXCZ4NTcQ0tqBcNfi1HQuk5mi/rHZWOY/n9rqTzNs6C4Anlc1RZW73N9OjWRdYU
AenDS8F0PjUUE+0VKER+7GCleCNXqFi2+r6iJIAZLCWHjoIuhXtwW+krUcUqBaUy
4JEtrkwiIrxOVqPCnq4XbuemtrmSWnVLy5pXCLX63WheIIeWYdFvgpeH3EU+AlNY
LMTeCgKk56N8z08LDmEs6Ru5Do6LocU7aBWSiipsYtiEXhvuOUuzM802XBl+k/so
yCYRE2A3/X8GULwRGK9QIoujZJsK7uss45KqDTZfrM1PCzLXZvKrgGXHyg0nwohM
jrep6W/tjTpEdEVbl1FAY8l8YinR8ou5+E1LBSBFn4mSuSIDJ4Ieo3ZvhonqjKYO
u1fABTbJ8viFlJXM9zJFDJ4c4nEcmWEnDkRk/6ho5FRUq2FhY1yWSyGJW71kbYCc
dbKMgD9gPfxrK3zNi8x+sHOwHDcIeM9jfvcQwE+tzGh4ETWY0KcZJSotq7KszevE
sdH+KR/R6uCrrzQVx39N/2P3bBFlZb+btClWsaSV6s04yN+odAFdaSa1Hv+ksdTH
TIp+MVgRDBCbrvt76nSW0Op30Mf0rhIwy1ktv6MftWAjj28ASNImbZ+btmuTTu0z
RcOcYsGZmH/+o0qudzIK5A3zB5CxE1RLuWhPNeCGwmdyEHQ5uHNLViFYazvBLcQ5
0Oyhe1At6s6RRqk4mkFlTNA7SojmAw==
=oUi/
-----END PGP SIGNATURE-----
vasild's public key is on openpgp.org
</details>