I have been thinking that the public structs TxBroadcastInfo and PeerSendInfo are the same as the private ones TxSendStatus and SendStatus. Before it wasn't so striking, but now it becomes more obvious as more fields are added and they have to be added to both and copied here in GetBroadcastInfo()
We can have just one set of structs and return m_transactions from GetBroadcastInfo(). The callers of GetBroadcastInfo() don't care whether it is a vector or unordered_map as long as they can iterate over it.
Here is a change that does that, on top of this PR. If you do not want to bloat this PR with it, I will submit as a followup:
<details>
<summary>[patch] Simplify GetBroadcastInfo(); 71 insertions(+), 109 deletions(-)</summary>
diff --git i/src/net_processing.cpp w/src/net_processing.cpp
index ed3e5ebf98..db8a007a04 100644
--- i/src/net_processing.cpp
+++ w/src/net_processing.cpp
@@ -539,13 +539,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
{
@@ -1854,25 +1854,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 auto& tx{tx_info.tx};
+ for (const auto& [tx, status] : 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 36ae021f67..9472aa7735 100644
--- i/src/net_processing.h
+++ w/src/net_processing.h
@@ -118,13 +118,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 0c70fff7d8..e0349032be 100644
--- i/src/private_broadcast.cpp
+++ w/src/private_broadcast.cpp
@@ -124,37 +124,20 @@ std::vector<CTransactionRef> PrivateBroadcast::GetStale() const
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& [tx, state] : m_transactions) {
- std::vector<PeerSendInfo> peers;
- peers.reserve(state.send_statuses.size());
- for (const auto& status : state.send_statuses) {
- peers.emplace_back(PeerSendInfo{.address = status.address, .sent = status.picked, .received = status.confirmed});
- }
- entries.emplace_back(TxBroadcastInfo{
- .tx = tx,
- .peers = std::move(peers),
- .received_from = state.received_from,
- .received_time = state.received_time,
- });
- }
-
- return entries;
+ return m_transactions;
}
-PrivateBroadcast::Priority PrivateBroadcast::DerivePriority(const TxSendStatus& tx_status)
+PrivateBroadcast::Priority PrivateBroadcast::DerivePriority(const Status& tx_status)
{
Priority p;
p.num_picked = tx_status.send_statuses.size();
for (const auto& send_status : tx_status.send_statuses) {
p.last_picked = std::max(p.last_picked, send_status.picked);
if (send_status.confirmed.has_value()) {
@@ -176,17 +159,17 @@ std::optional<PrivateBroadcast::TxAndSendStatusForNode> PrivateBroadcast::GetSen
}
}
}
return std::nullopt;
}
-std::vector<PrivateBroadcast::TransactionMap::iterator> PrivateBroadcast::GetPendingTransactions()
+std::vector<PrivateBroadcast::Transactions::iterator> PrivateBroadcast::GetPendingTransactions()
EXCLUSIVE_LOCKS_REQUIRED(m_mutex)
{
AssertLockHeld(m_mutex);
- std::vector<TransactionMap::iterator> result;
+ std::vector<Transactions::iterator> result;
for (auto it{m_transactions.begin()}; it != m_transactions.end(); ++it) {
if (!it->second.received_from.has_value()) {
result.push_back(it);
}
}
return result;
diff --git i/src/private_broadcast.h w/src/private_broadcast.h
index 5c70754df2..3c39ad298a 100644
--- i/src/private_broadcast.h
+++ w/src/private_broadcast.h
@@ -28,25 +28,47 @@
* - Query whether a given recipient node has confirmed reception
* - Query whether any transactions that need sending are currently on the list
*/
class PrivateBroadcast
{
public:
- struct PeerSendInfo {
- CService address;
- NodeClock::time_point sent;
- std::optional<NodeClock::time_point> received;
+ /// Status of a transaction sent to a given node.
+ struct SendStatus {
+ const NodeId nodeid; /// Node to which the transaction will be sent (or was sent).
+ const CService address; /// Address of the node.
+ const NodeClock::time_point picked; ///< When was the transaction picked for sending to the node.
+ std::optional<NodeClock::time_point> confirmed; ///< When was the transaction reception confirmed by the node (by PONG).
+
+ SendStatus(const NodeId& nodeid, const CService& address, const NodeClock::time_point& picked) : nodeid{nodeid}, address{address}, picked{picked} {}
};
- struct TxBroadcastInfo {
- CTransactionRef tx;
- std::vector<PeerSendInfo> peers;
- std::optional<CService> received_from;
- std::optional<NodeClock::time_point> received_time;
+ /// Status of a transaction, including all send attempts and a possible reception info.
+ struct Status {
+ std::vector<SendStatus> send_statuses; /// All send attempts.
+ std::optional<CService> received_from; /// When we receive back the transaction from the network, this is the peer we got it from.
+ std::optional<NodeClock::time_point> received_time; /// Time of receiving back the transaction.
};
+ // 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, or reset a transaction's state if the
* transaction has been marked received.
* [@param](/bitcoin-bitcoin/contributor/param/)[in] tx The transaction to add.
* [@retval](/bitcoin-bitcoin/contributor/retval/) true The transaction was added or reset.
* [@retval](/bitcoin-bitcoin/contributor/retval/) false The transaction was already present and not marked received.
@@ -124,32 +146,16 @@ 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:
- /// Status of a transaction sent to a given node.
- struct SendStatus {
- const NodeId nodeid; /// Node to which the transaction will be sent (or was sent).
- const CService address; /// Address of the node.
- const NodeClock::time_point picked; ///< When was the transaction picked for sending to the node.
- std::optional<NodeClock::time_point> confirmed; ///< When was the transaction reception confirmed by the node (by PONG).
-
- SendStatus(const NodeId& nodeid, const CService& address, const NodeClock::time_point& picked) : nodeid{nodeid}, address{address}, picked{picked} {}
- };
-
- struct TxSendStatus {
- std::vector<SendStatus> send_statuses;
- std::optional<CService> received_from;
- std::optional<NodeClock::time_point> received_time;
- };
-
/// 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.
NodeClock::time_point last_picked{}; ///< The most recent time when the transaction was picked for sending.
size_t num_confirmed{0}; ///< Number of nodes that have confirmed reception of a transaction (by PONG).
NodeClock::time_point last_confirmed{}; ///< The most recent time when the transaction was confirmed.
@@ -167,46 +173,28 @@ 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.
- }
- };
-
- using TransactionMap = std::unordered_map<CTransactionRef, TxSendStatus, CTransactionRefHash, CTransactionRefComp>;
-
/**
* Derive the sending priority of a transaction.
* [@param](/bitcoin-bitcoin/contributor/param/)[in] status The send status of the transaction.
*/
- static Priority DerivePriority(const TxSendStatus& status);
+ static Priority DerivePriority(const Status& status);
/**
* Find which transaction we sent to a given node (marked by PickTxForSend()).
* [@return](/bitcoin-bitcoin/contributor/return/) That transaction together with the send status or nullopt if we did not
* send any transaction to the given node.
*/
std::optional<TxAndSendStatusForNode> GetSendStatusByNode(const NodeId& nodeid)
EXCLUSIVE_LOCKS_REQUIRED(m_mutex);
- std::vector<TransactionMap::iterator> GetPendingTransactions()
+ std::vector<Transactions::iterator> GetPendingTransactions()
EXCLUSIVE_LOCKS_REQUIRED(m_mutex);
mutable Mutex m_mutex;
- TransactionMap 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 caa0c9e636..bdc61a3e8c 100644
--- i/src/rpc/mempool.cpp
+++ w/src/rpc/mempool.cpp
@@ -174,34 +174,36 @@ static RPCHelpMan getprivatebroadcastinfo()
+ HelpExampleRpc("getprivatebroadcastinfo", "")
},
[&](const RPCHelpMan& 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("txid", tx->GetHash().ToString());
+ o.pushKV("wtxid", tx->GetWitnessHash().ToString());
+ o.pushKV("hex", EncodeHexTx(*tx));
UniValue peers(UniValue::VARR);
- for (const auto& peer : tx_info.peers) {
+ for (const auto& peer : status.send_statuses) {
UniValue p(UniValue::VOBJ);
p.pushKV("address", peer.address.ToStringAddrPort());
- p.pushKV("sent", TicksSinceEpoch<std::chrono::seconds>(peer.sent));
- if (peer.received.has_value()) {
- p.pushKV("received", TicksSinceEpoch<std::chrono::seconds>(*peer.received));
+ p.pushKV("sent", TicksSinceEpoch<std::chrono::seconds>(peer.picked));
+ if (peer.confirmed.has_value()) {
+ p.pushKV("received", TicksSinceEpoch<std::chrono::seconds>(*peer.confirmed));
}
peers.push_back(std::move(p));
}
o.pushKV("peers", std::move(peers));
- if (tx_info.received_from.has_value()) {
- o.pushKV("received_from", tx_info.received_from->ToStringAddrPort());
- o.pushKV("received_time", TicksSinceEpoch<std::chrono::seconds>(*tx_info.received_time));
+ if (status.received_from.has_value()) {
+ o.pushKV("received_from", status.received_from->ToStringAddrPort());
+ }
+ if (status.received_time.has_value()) {
+ o.pushKV("received_time", TicksSinceEpoch<std::chrono::seconds>(status.received_time.value()));
}
transactions.push_back(std::move(o));
}
UniValue ret(UniValue::VOBJ);
ret.pushKV("transactions", std::move(transactions));
diff --git i/src/test/private_broadcast_tests.cpp w/src/test/private_broadcast_tests.cpp
index 715cafb190..0cf58e5d0e 100644
--- i/src/test/private_broadcast_tests.cpp
+++ w/src/test/private_broadcast_tests.cpp
@@ -21,19 +21,12 @@ static CTransactionRef MakeDummyTx(uint32_t id, size_t num_witness)
mtx.vin[0].scriptWitness = CScriptWitness{};
mtx.vin[0].scriptWitness.stack.resize(num_witness);
}
return MakeTransactionRef(mtx);
}
-static auto FindTxInfo(const std::vector<PrivateBroadcast::TxBroadcastInfo>& infos, const CTransactionRef& tx)
-{
- const auto it{std::ranges::find(infos, tx->GetWitnessHash(), [](const auto& info) { return info.tx->GetWitnessHash(); })};
- BOOST_REQUIRE(it != infos.end());
- return it;
-}
-
} // namespace
BOOST_FIXTURE_TEST_SUITE(private_broadcast_tests, BasicTestingSetup)
BOOST_AUTO_TEST_CASE(basic)
{
@@ -63,14 +56,14 @@ BOOST_AUTO_TEST_CASE(basic)
BOOST_REQUIRE(tx1->GetWitnessHash() != tx2->GetWitnessHash());
BOOST_CHECK(pb.Add(tx2));
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);
- BOOST_CHECK_EQUAL(FindTxInfo(infos, tx1)->peers.size(), tx1_peer_count);
- BOOST_CHECK_EQUAL(FindTxInfo(infos, tx2)->peers.size(), tx2_peer_count);
+ BOOST_CHECK_EQUAL(infos.at(tx1).send_statuses.size(), tx1_peer_count);
+ BOOST_CHECK_EQUAL(infos.at(tx2).send_statuses.size(), tx2_peer_count);
}};
check_peer_counts(/*tx1_peer_count=*/0, /*tx2_peer_count=*/0);
const auto tx_for_recipient1{pb.PickTxForSend(/*will_send_to_nodeid=*/recipient1, /*will_send_to_address=*/addr1).value()};
BOOST_CHECK(tx_for_recipient1 == tx1 || tx_for_recipient1 == tx2);
@@ -105,24 +98,22 @@ 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 info_it{FindTxInfo(infos, tx_for_recipient1)};
- const auto& peers{info_it->peers};
+ const auto& peers{infos.at(tx_for_recipient1).send_statuses};
BOOST_CHECK_EQUAL(peers.size(), 1);
BOOST_CHECK_EQUAL(peers[0].address.ToStringAddrPort(), addr1.ToStringAddrPort());
- BOOST_CHECK(peers[0].received.has_value());
+ BOOST_CHECK(peers[0].confirmed.has_value());
}
{
- const auto info_it{FindTxInfo(infos, tx_for_recipient2)};
- const auto& peers{info_it->peers};
+ const auto& peers{infos.at(tx_for_recipient2).send_statuses};
BOOST_CHECK_EQUAL(peers.size(), 1);
BOOST_CHECK_EQUAL(peers[0].address.ToStringAddrPort(), addr2.ToStringAddrPort());
- BOOST_CHECK(!peers[0].received.has_value());
+ BOOST_CHECK(!peers[0].confirmed.has_value());
}
BOOST_CHECK_EQUAL(pb.GetStale().size(), 1);
BOOST_CHECK_EQUAL(pb.GetStale()[0], tx_for_recipient2);
SetMockTime(Now<NodeSeconds>() + 10h);
@@ -164,36 +155,35 @@ BOOST_AUTO_TEST_CASE(mark_received)
// MarkReceived succeeds and returns the number of confirmed sends.
BOOST_CHECK_EQUAL(pb.MarkReceived(tx, received_from).value(), 1);
{
const auto infos{pb.GetBroadcastInfo()};
- const auto info{FindTxInfo(infos, tx)};
- BOOST_CHECK_EQUAL(info->received_from->ToStringAddrPort(), received_from.ToStringAddrPort());
- BOOST_CHECK(info->received_time.has_value());
+ const auto& info{infos.at(tx)};
+ BOOST_CHECK_EQUAL(info.received_from->ToStringAddrPort(), received_from.ToStringAddrPort());
+ BOOST_CHECK(info.received_time.has_value());
}
BOOST_CHECK(!pb.HavePendingTransactions());
BOOST_CHECK(!pb.PickTxForSend(/*will_send_to_nodeid=*/recipient2, /*will_send_to_address=*/addr2).has_value());
// Subsequent MarkReceived returns nullopt and does not overwrite.
in_addr ipv4Addr2;
ipv4Addr2.s_addr = 0xa0b0c099;
const CService received_from2{ipv4Addr2, 4444};
BOOST_CHECK(!pb.MarkReceived(tx, received_from2).has_value());
{
const auto infos{pb.GetBroadcastInfo()};
- const auto info{FindTxInfo(infos, tx)};
- BOOST_CHECK_EQUAL(info->received_from->ToStringAddrPort(), received_from.ToStringAddrPort());
+ BOOST_CHECK_EQUAL(infos.at(tx).received_from->ToStringAddrPort(), received_from.ToStringAddrPort());
BOOST_CHECK(!pb.HavePendingTransactions());
}
// Re-adding after received clears the received state and makes it pending again.
BOOST_CHECK(pb.Add(tx));
BOOST_CHECK(pb.HavePendingTransactions());
const auto infos{pb.GetBroadcastInfo()};
- const auto info{FindTxInfo(infos, tx)};
- BOOST_CHECK(!info->received_from.has_value());
- BOOST_CHECK(!info->received_time.has_value());
+ const auto& info{infos.at(tx)};
+ BOOST_CHECK(!info.received_from.has_value());
+ BOOST_CHECK(!info.received_time.has_value());
}
BOOST_AUTO_TEST_SUITE_END()
</details>
