refactor: Convert GenTxid to std::variant
#32631
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32631. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, 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. |
|
In diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 011e3cd928..1a47a39dfd 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -206,6 +206,12 @@ struct QueuedBlock {
std::unique_ptr<PartiallyDownloadedBlock> partialBlock;
};
+struct CompareGenTxid {
+ bool operator()(const GenTxid& a, const GenTxid& b) const {
+ return std::tuple(a.index(), a.ToUint256()) < std::tuple(b.index(), b.ToUint256());
+ }
+};
+
/**
* Data structure for an individual peer. This struct is not protected by
* cs_main since it does not contain validation-critical data.
@@ -302,7 +308,7 @@ struct Peer {
* non-wtxid-relay peers, wtxid for wtxid-relay peers). We use the
* mempool to sort transactions in dependency order before relay, so
* this does not have to be sorted. */
- std::set<GenTxid> m_tx_inventory_to_send GUARDED_BY(m_tx_inventory_mutex);
+ std::set<GenTxid, CompareGenTxid> m_tx_inventory_to_send GUARDED_BY(m_tx_inventory_mutex);
/** Whether the peer has requested us to send our complete mempool. Only
* permitted if the peer has NetPermissionFlags::Mempool or we advertise
* NODE_BLOOM. See BIP35. */
@@ -856,7 +862,7 @@ private:
std::shared_ptr<const CBlock> m_most_recent_block GUARDED_BY(m_most_recent_block_mutex);
std::shared_ptr<const CBlockHeaderAndShortTxIDs> m_most_recent_compact_block GUARDED_BY(m_most_recent_block_mutex);
uint256 m_most_recent_block_hash GUARDED_BY(m_most_recent_block_mutex);
- std::unique_ptr<const std::map<GenTxid, CTransactionRef>> m_most_recent_block_txs GUARDED_BY(m_most_recent_block_mutex);
+ std::unique_ptr<const std::map<GenTxid, CTransactionRef, CompareGenTxid>> m_most_recent_block_txs GUARDED_BY(m_most_recent_block_mutex);
// Data about the low-work headers synchronization, aggregated from all peers' HeadersSyncStates.
/** Mutex guarding the other m_headers_presync_* variables. */
@@ -2027,7 +2033,7 @@ void PeerManagerImpl::NewPoWValidBlock(const CBlockIndex *pindex, const std::sha
std::async(std::launch::deferred, [&] { return NetMsg::Make(NetMsgType::CMPCTBLOCK, *pcmpctblock); })};
{
- auto most_recent_block_txs = std::make_unique<std::map<GenTxid, CTransactionRef>>();
+ auto most_recent_block_txs = std::make_unique<std::map<GenTxid, CTransactionRef, CompareGenTxid>>();
for (const auto& tx : pblock->vtx) {
most_recent_block_txs->emplace(tx->GetHash(), tx);
most_recent_block_txs->emplace(tx->GetWitnessHash(), tx);
diff --git a/src/util/transaction_identifier.h b/src/util/transaction_identifier.h
index 3cdeb664d4..6ab5d98263 100644
--- a/src/util/transaction_identifier.h
+++ b/src/util/transaction_identifier.h
@@ -88,13 +88,12 @@ p
8000
ublic:
return std::visit([](const auto& id) -> const uint256& { return id.ToUint256(); }, *this);
}
- friend bool operator==(const GenTxid& a, const GenTxid& b) {
- return a.index() == b.index() && a.ToUint256() == b.ToUint256();
- }
-
- friend bool operator<(const GenTxid& a, const GenTxid& b) {
- return std::tuple(a.index(), a.ToUint256()) < std::tuple(b.index(), b.ToUint256());
- }
+ friend bool operator==(const GenTxid& a, const GenTxid& b) = delete;
+ friend bool operator!=(const GenTxid& a, const GenTxid& b) = delete;
+ friend bool operator<(const GenTxid& a, const GenTxid& b) = delete;
+ friend bool operator<=(const GenTxid& a, const GenTxid& b) = delete;
+ friend bool operator>(const GenTxid& a, const GenTxid& b) = delete;
+ friend bool operator>=(const GenTxid& a, const GenTxid& b) = delete;
};
#endif // BITCOIN_UTIL_TRANSACTION_IDENTIFIER_H
Then run |
e66a554 to
98358c4
Compare
There was a problem hiding this comment.
Concept ACK, the new GenTxid allows us to better benefit from the type safety of Txid and Wtxid and this PR takes a clear and structured approach that seems straightforward to review.
Approach-wise, I think I have a slightly different view. I find code generally easier to understand when we use the underlying types {Txid, Wtxid} directly and as such would prefer functions to be overloaded with those types, pushing the handling of variants to the edge as much as possible. This also has the side benefit of gently nudging users of those functions to not using variants if they don't have to. I have left 2 such comments on the first commits, but I suspect it applies to later commits too (did not investigate that in detail yet). This is largely a style preference, and I don't want to block the PRs progress over it if other reviewers disagree.
|
concept ACK |
98358c4 to
f7c8f3f
Compare
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
a3286b9 to
a853dcb
Compare
|
Concept ACK |
17af8a7 to
b740b6f
Compare
|
Thanks for reviewing @stickies-v! I took your suggestion for mempool's I looked at |
| indexed_transaction_set::const_iterator j = std::visit(util::Overloaded{ | ||
| [this](const Wtxid& wtxid) EXCLUSIVE_LOCKS_REQUIRED(cs) { return get_iter_from_wtxid(wtxid); }, | ||
| [this](const Txid& txid) EXCLUSIVE_LOCKS_REQUIRED(cs) { return mapTx.find(txid); } | ||
| }, hashb); |
There was a problem hiding this comment.
nit in ea58f45:
At first it seems more optimal to keep taking bool wtxid as a function parameter, and assume our GenTxidVariants are of the expected type. The current way gives the impression that we may be mixing Txid with Wtxid for the same peer.
| indexed_transaction_set::const_iterator j = std::visit(util::Overloaded{ | |
| [this](const Wtxid& wtxid) EXCLUSIVE_LOCKS_REQUIRED(cs) { return get_iter_from_wtxid(wtxid); }, | |
| [this](const Txid& txid) EXCLUSIVE_LOCKS_REQUIRED(cs) { return mapTx.find(txid); } | |
| }, hashb); | |
| indexed_transaction_set::const_iterator j = wtxid ? get_iter_from_wtxid(std::get<Wtxid>(hashb)) : mapTx.find(std::get<Txid>(hashb)); |
A strong argument against reverting to that is that std::get still checks the index matches internally, and std::variant has no zero-overhead alternative. So we might as well use std::visit and drop the function parameter as you have done.
Might be worth adding something to the commit message such as:
"Now that we are storing CTxMemPool::CompareDepthAndScore parameters using std::variant we have no portable zero-overhead way of accessing them, so use std::visit and drop bool wtxid in-parameter."
There was a problem hiding this comment.
Thanks for reviewing. I don't think I can improve upon your suggested commit message, so hope you don't mind that I took this directly.
67fbc3a to
d5a6e7a
Compare
There was a problem hiding this comment.
Code review d5a6e7a
Thanks for incorporating most of my feedback!
Mostly focused on reviewing non-test code.
| @@ -435,7 +426,7 @@ class TxRequestTracker::Impl { | |||
| auto it_prev = std::prev(it); | |||
| // The next best CANDIDATE_READY, if any, immediately precedes the REQUESTED or CANDIDATE_BEST | |||
| A93C | // announcement in the ByTxHash index. | ||
| if (it_prev->m_txhash == it->m_txhash && it_prev->GetState() == State::CANDIDATE_READY) { | |||
| if (it_prev->m_txhash.ToUint256() == it->m_txhash.ToUint256() && it_prev->GetState() == State::CANDIDATE_READY) { | |||
There was a problem hiding this comment.
So for segwit peers we use Wtxid and for non-segwit peers we use Txid?
Scary that the variant fails comparison although the hash is the same, so we have to manually find places where it shouldn't matter and do this kind of thing. :\
Might be easier to ACK this PR if you peel off txrequest.cpp/h into it's own PR.
There was a problem hiding this comment.
Yeah the inv should match the peer's wtxid relay setting. The announcements are converted to GenTxids in ProcessMessage and that's what's passed into AddTxAnnouncement and used throughout txdownloadman_impl and txrequest.
I would prefer to keep it all in this PR, as I don't want both the old GenTxid class and the new variant in master. But I agree it's a bit iffy to have to manually ensure that we didn't miss a place where a previous uint256 comparison is now suddenly a GenTxid comparison. The patch I provide at the top should catch anywhere that I might've missed.
Yes, with this PR's change, we would have be intentional about when we're comparing GenTxid objects (type first, then hash) and when we're comparing only the underlying hash. Another option would be to make the patch an actual part of the code, deleting the GenTxid comparators and having custom comparators wherever GenTxids are involved. This felt like overkill to me though, and I think one of the goals of this type safety refactor is to, from now on, differentiate between the transaction identifiers and the underlying transaction hash. In that case, it makes sense to me to have a standard way to compare GenTxids and then make the conversion to uint256 explicit if comparison of the hash is needed.
There was a problem hiding this comment.
Had a closer look at TxRequestTracker.
-
I support replacing
uint256 Announcement::m_txhash+bool Announcement::m_is_wtxidwith aGenTxId, but would prefer renaming the field just to force out any issues. -
On master the interface seems to deliberately use either
GenTxIdoruint256depending on what we want to match on:
Lines 135 to 178 in 1be688f
All of the below are changed in the current version of this PR to useGenTxIdinstead ofuint256, but then only end up using theuint256-part internally:ForgetTxHash()RequestedTx()ReceivedResponse()GetCandidatePeers()ComputePriority()
It feels safer to at least drop those kind of changes on principle, even if they currently do not introduce bugs. We don't want to accidentally introduce cases where a
Wtxidof a transaction without any witness data does not match aTxidwith the same exact hash.
Reimplements the GenTxid class as a variant for better type safety. Also adds two temporary functions to the old GenTxid class that convert to and from the new variant.
Now that we are storing `CTxMemPool::CompareDepthAndScore` parameters using `std::variant` we have no portable zero-overhead way of accessing them, so use `std::visit` and drop `bool wtxid` in-parameter.
Convert all of `txdownloadman_impl` to the new variant except for `GetRequestsToSend`, which will be easier to switch at the same time as `txrequest`.
Switch all instances of GenTxid to the new variant in `txrequest` and complete `txdownloadman_impl` by converting `GetRequestsToSend`.
-BEGIN VERIFY SCRIPT- sed -i 's/GenTxidVariant/GenTxid/g' $(git grep -l 'GenTxidVariant') -END VERIFY SCRIPT-
d5a6e7a to
f73fe94
Compare
| @@ -435,7 +426,7 @@ class TxRequestTracker::Impl { | |||
| auto it_prev = std::prev(it); | |||
| // The next best CANDIDATE_READY, if any, immediately precedes the REQUESTED or CANDIDATE_BEST | |||
| // announcement in the ByTxHash index. | |||
| if (it_prev->m_txhash == it->m_txhash && it_prev->GetState() == State::CANDIDATE_READY) { | |||
| if (it_prev->m_txhash.ToUint256() == it->m_txhash.ToUint256() && it_prev->GetState() == State::CANDIDATE_READY) { | |||
There was a problem hiding this comment.
Had a closer look at TxRequestTracker.
-
I support replacing
uint256 Announcement::m_txhash+bool Announcement::m_is_wtxidwith aGenTxId, but would prefer renaming the field just to force out any issues. -
On master the interface seems to deliberately use either
GenTxIdoruint256depending on what we want to match on:
Lines 135 to 178 in 1be688f
All of the below are changed in the current version of this PR to useGenTxIdinstead ofuint256, but then only end up using theuint256-part internally:ForgetTxHash()RequestedTx()ReceivedResponse()GetCandidatePeers()ComputePriority()
It feels safer to at least drop those kind of changes on principle, even if they currently do not introduce bugs. We don't want to accidentally introduce cases where a
Wtxidof a transaction without any witness data does not match aTxidwith the same exact hash.
|
|
||
| bool IsWtxid() const { return std::holds_alternative<Wtxid>(*this); } | ||
|
|
||
| const uint256& ToUint256() const LIFETIMEBOUND { |
There was a problem hiding this comment.
nit: Braces should be on newline for functions as per developer-notes.md, here and for the next one:
| const uint256& ToUint256() const LIFETIMEBOUND { | |
| const uint256& ToUint256() const LIFETIMEBOUND | |
| { |
There was a problem hiding this comment.
I think the changes here can be simplified quite a bit by first replacing get_iter_from_wtxid with a GetIter(const wtxid&) overload, see my suggestions here and here (or full branch master...stickies-v:bitcoin:2025-06/overload-getiter).
There are also quite a few clang-tidy violations that would be helpful to address here, left per-commit comments.
| @@ -76,4 +80,21 @@ using Txid = transaction_identifier<false>; | |||
| /** Wtxid commits to all transaction fields including the witness. */ | |||
| using Wtxid = transaction_identifier<true>; | |||
|
|
|||
| /** A generic txid reference (txid or wtxid). */ | |||
There was a problem hiding this comment.
nit: this docstring imo doesn't add anything useful. Would either remove or beef up (e.g. explaining when (not) to use this).
| public: | ||
| explicit CompareInvMempoolOrder(CTxMemPool *_mempool, bool use_wtxid) | ||
| explicit CompareInvMempoolOrder(CTxMemPool *_mempool) |
There was a problem hiding this comment.
clang-tidy nit:
| explicit CompareInvMempoolOrder(CTxMemPool *_mempool) | |
| explicit CompareInvMempoolOrder(CTxMemPool* _mempool) |
src/primitives/transaction.h
Outdated
| @@ -437,6 +437,19 @@ class GenTxid | |||
| const uint256& GetHash() const LIFETIMEBOUND { return m_hash; } | |||
| friend bool operator==(const GenTxid& a, const GenTxid& b) { return a.m_is_wtxid == b.m_is_wtxid && a.m_hash == b.m_hash; } | |||
| friend bool operator<(const GenTxid& a, const GenTxid& b) { return std::tie(a.m_is_wtxid, a.m_hash) < std::tie(b.m_is_wtxid, b.m_hash); } | |||
|
|
|||
| GenTxidVariant ToVariant() const { | |||
There was a problem hiding this comment.
clang-tidy nit for 5b5abf8:
git diff on a6f1af5f55
diff --git a/src/primitives/transaction.h b/src/primitives/transaction.h
index 54c2fb578f..adadb2a3e7 100644
--- a/src/primitives/transaction.h
+++ b/src/primitives/transaction.h
@@ -438,17 +438,18 @@ public:
friend bool operator==(const GenTxid& a, const GenTxid& b) { return a.m_is_wtxid == b.m_is_wtxid && a.m_hash == b.m_hash; }
friend bool operator<(const GenTxid& a, const GenTxid& b) { return std::tie(a.m_is_wtxid, a.m_hash) < std::tie(b.m_is_wtxid, b.m_hash); }
- GenTxidVariant ToVariant() const {
+ GenTxidVariant ToVariant() const
+ {
return m_is_wtxid ?
- GenTxidVariant{Wtxid::FromUint256(m_hash)} :
- GenTxidVariant{Txid::FromUint256(m_hash)};
+ GenTxidVariant{Wtxid::FromUint256(m_hash)} :
+ GenTxidVariant{Txid::FromUint256(m_hash)};
}
- static GenTxid FromVariant(const GenTxidVariant& variant) {
+ static GenTxid FromVariant(const GenTxidVariant& variant)
+ {
return GenTxid{
std::holds_alternative<::Wtxid>(variant),
- variant.ToUint256()
- };
+ variant.ToUint256()};
}
};
diff --git a/src/util/transaction_identifier.h b/src/util/transaction_identifier.h
index 4137dd7040..39104a1a5c 100644
--- a/src/util/transaction_identifier.h
+++ b/src/util/transaction_identifier.h
@@ -88,11 +88,13 @@ public:
bool IsWtxid() const { return std::holds_alternative<Wtxid>(*this); }
- const uint256& ToUint256() const LIFETIMEBOUND {
+ const uint256& ToUint256() const LIFETIMEBOUND
+ {
return std::visit([](const auto& id) -> const uint256& { return id.ToUint256(); }, *this);
}
- friend auto operator<=>(const GenTxidVariant& a, const GenTxidVariant& b) {
+ friend auto operator<=>(const GenTxidVariant& a, const GenTxidVariant& b)
+ {
return std::tuple(a.IsWtxid(), a.ToUint256()) <=> std::tuple(b.IsWtxid(), b.ToUint256());
}
};
| @@ -2437,7 +2439,9 @@ void PeerManagerImpl::ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic | |||
| continue; | |||
| } | |||
|
|
|||
| CTransactionRef tx = FindTxForGetData(*tx_relay, ToGenTxid(inv)); | |||
| CTransactionRef tx{inv.IsMsgWtx() ? | |||
| FindTxForGetData(*tx_relay, Wtxid::FromUint256(inv.hash)) : | |||
There was a problem hiding this comment.
clang-tidy nit for da99271:
git diff on 07822f5386
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index df66d4c93d..a0ed1e2620 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -2440,8 +2440,8 @@ void PeerManagerImpl::ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic
}
CTransactionRef tx{inv.IsMsgWtx() ?
- FindTxForGetData(*tx_relay, Wtxid::FromUint256(inv.hash)) :
- FindTxForGetData(*tx_relay, Txid::FromUint256(inv.hash))};
+ FindTxForGetData(*tx_relay, Wtxid::FromUint256(inv.hash)) :
+ FindTxForGetData(*tx_relay, Txid::FromUint256(inv.hash))};
if (tx) {
// WTX and WITNESS_TX imply we serialize with witness
const auto maybe_with_witness = (inv.IsMsgTx() ? TX_NO_WITNESS : TX_WITH_WITNESS);
diff --git a/src/validation.cpp b/src/validation.cpp
index 9d788b50f6..c0bc5f28fc 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -1135,8 +1135,7 @@ bool MemPoolAccept::PackageMempoolChecks(const std::vector<CTransactionRef>& txn
AssertLockHeld(m_pool.cs);
// CheckPackageLimits expects the package transactions to not already be in the mempool.
- assert(std::all_of(txns.cbegin(), txns.cend(), [this](const auto& tx)
- { return !m_pool.exists(tx->GetHash());}));
+ assert(std::all_of(txns.cbegin(), txns.cend(), [this](const auto& tx) { return !m_pool.exists(tx->GetHash()); }));
assert(txns.size() == workspaces.size());
@@ -1346,8 +1345,7 @@ bool MemPoolAccept::SubmitPackage(const ATMPArgs& args, std::vector<Workspace>&
AssertLockHeld(m_pool.cs);
// Sanity check: none of the transactions should be in the mempool, and none of the transactions
// should have a same-txid-different-witness equivalent in the mempool.
- assert(std::all_of(workspaces.cbegin(), workspaces.cend(), [this](const auto& ws){
- return !m_pool.exists(ws.m_ptx->GetHash()); }));
+ assert(std::all_of(workspaces.cbegin(), workspaces.cend(), [this](const auto& ws) { return !m_pool.exists(ws.m_ptx->GetHash()); }));
bool all_submitted = true;
FinalizeSubpackage(args);
| const uint256& hash = gtxid.GetHash(); | ||
|
|
||
| if (gtxid.IsWtxid()) { | ||
| bool in_orphanage = std::visit(util::Overloaded{ |
There was a problem hiding this comment.
clang-tidy nit in 151ab0e:
git diff on 44c6c811ba
diff --git a/src/node/txdownloadman_impl.cpp b/src/node/txdownloadman_impl.cpp
index c169c3bf41..054c8fb7dd 100644
--- a/src/node/txdownloadman_impl.cpp
+++ b/src/node/txdownloadman_impl.cpp
@@ -126,21 +126,21 @@ void TxDownloadManagerImpl::BlockDisconnected()
bool TxDownloadManagerImpl::AlreadyHaveTx(const GenTxidVariant& gtxid, bool include_reconsiderable)
{
bool in_orphanage = std::visit(util::Overloaded{
- // Normal query by wtxid.
- [this](const Wtxid& wtxid) { return m_orphanage.HaveTx(wtxid); },
- // Never query by txid: it is possible that the transaction in the orphanage has the same
- // txid but a different witness, which would give us a false positive result. If we decided
- // not to request the transaction based on this result, an attacker could prevent us from
- // downloading a transaction by intentionally creating a malleated version of it. While
- // only one (or none!) of these transactions can ultimately be confirmed, we have no way of
- // discerning which one that is, so the orphanage can store multiple transactions with the
- // same txid.
- //
- // While we won't query by txid, we can try to "guess" what the wtxid is based on the txid.
- // A non-segwit transaction's txid == wtxid. Query this txid "casted" to a wtxid. This will
- // help us find non-segwit transactions, saving bandwidth, and should have no false positives.
- [this](const Txid& txid) { return m_orphanage.HaveTx(Wtxid::FromUint256(txid.ToUint256())); }
- }, gtxid);
+ // Normal query by wtxid.
+ [this](const Wtxid& wtxid) { return m_orphanage.HaveTx(wtxid); },
+ // Never query by txid: it is possible that the transaction in the orphanage has the same
+ // txid but a different witness, which would give us a false positive result. If we decided
+ // not to request the transaction based on this result, an attacker could prevent us from
+ // downloading a transaction by intentionally creating a malleated version of it. While
+ // only one (or none!) of these transactions can ultimately be confirmed, we have no way of
+ // discerning which one that is, so the orphanage can store multiple transactions with the
+ // same txid.
+ //
+ // While we won't query by txid, we can try to "guess" what the wtxid is based on the txid.
+ // A non-segwit transaction's txid == wtxid. Query this txid "casted" to a wtxid. This will
+ // help us find non-segwit transactions, saving bandwidth, and should have no false positives.
+ [this](const Txid& txid) { return m_orphanage.HaveTx(Wtxid::FromUint256(txid.ToUint256())); }},
+ gtxid);
if (in_orphanage) return true;
@@ -183,7 +183,7 @@ bool TxDownloadManagerImpl::AddTxAnnouncement(NodeId peer, const GenTxidVariant&
if (const auto* wtxid = std::get_if<Wtxid>(>xid)) {
if (auto orphan_tx{m_orphanage.GetTx(*wtxid)}) {
auto unique_parents{GetUniqueParents(*orphan_tx)};
- std::erase_if(unique_parents, [&](const auto& txid){
+ std::erase_if(unique_parents, [&](const auto& txid) {
return AlreadyHaveTx(txid, /*include_reconsiderable=*/false);
});
@@ -398,7 +398,7 @@ node::RejectedTxTodo TxDownloadManagerImpl::MempoolRejectedTx(const CTransaction
// Filter parents that we already have.
// Exclude m_lazy_recent_rejects_reconsiderable: the missing parent may have been
// previously rejected for being too low feerate. This orphan might CPFP it.
- std::erase_if(unique_parents, [&](const auto& txid){
+ std::erase_if(unique_parents, [&](const auto& txid) {
return AlreadyHaveTx(txid, /*include_reconsiderable=*/false);
});
const auto now{GetTime<std::chrono::microseconds>()};
diff --git a/src/test/fuzz/txdownloadman.cpp b/src/test/fuzz/txdownloadman.cpp
index bdb38242f6..f2fb186b8e 100644
--- a/src/test/fuzz/txdownloadman.cpp
+++ b/src/test/fuzz/txdownloadman.cpp
@@ -263,8 +263,7 @@ FUZZ_TARGET(txdownloadman, .init = initialize)
// returned true.
Assert(expect_work);
}
- }
- );
+ });
// Jump forwards or backwards
auto time_skip = fuzzed_data_provider.PickValueInArray(TIME_SKIPS);
if (fuzzed_data_provider.ConsumeBool()) time_skip *= -1;
@@ -438,8 +437,7 @@ FUZZ_TARGET(txdownloadman_impl, .init = initialize)
state_missing_inputs.Invalid(TxValidationResult::TX_MISSING_INPUTS, "");
txdownload_impl.MempoolRejectedTx(ptx, state_missing_inputs, rand_peer, fuzzed_data_provider.ConsumeBool());
}
- }
- );
+ });
auto time_skip = fuzzed_data_provider.PickValueInArray(TIME_SKIPS);
if (fuzzed_data_provider.ConsumeBool()) time_skip *= -1;
| auto requestable = m_txrequest.GetRequestable(nodeid, current_time, &expired); | ||
| for (const auto& entry : expired) { | ||
| LogDebug(BCLog::NET, "timeout of inflight %s %s from peer=%d\n", entry.second.IsWtxid() ? "wtx" : "tx", | ||
| entry.second.GetHash().ToString(), entry.first); | ||
| entry.second.ToUint256().ToString(), entry.first); |
There was a problem hiding this comment.
clang-tidy nit in 6c2ba58:
git diff on 50c04bce87
diff --git a/src/node/txdownloadman_impl.cpp b/src/node/txdownloadman_impl.cpp
index 8db30d21d1..a514ac8143 100644
--- a/src/node/txdownloadman_impl.cpp
+++ b/src/node/txdownloadman_impl.cpp
@@ -275,12 +275,12 @@ std::vector<GenTxidVariant> TxDownloadManagerImpl::GetRequestsToSend(NodeId node
auto requestable = m_txrequest.GetRequestable(nodeid, current_time, &expired);
for (const auto& entry : expired) {
LogDebug(BCLog::NET, "timeout of inflight %s %s from peer=%d\n", entry.second.IsWtxid() ? "wtx" : "tx",
- entry.second.ToUint256().ToString(), entry.first);
+ entry.second.ToUint256().ToString(), entry.first);
}
for (const GenTxidVariant& gtxid : requestable) {
if (!AlreadyHaveTx(gtxid, /*include_reconsiderable=*/false)) {
LogDebug(BCLog::NET, "Requesting %s %s peer=%d\n", gtxid.IsWtxid() ? "wtx" : "tx",
- gtxid.ToUint256().ToString(), nodeid);
+ gtxid.ToUint256().ToString(), nodeid);
requests.emplace_back(gtxid);
m_txrequest.RequestedTx(nodeid, gtxid, current_time + GETDATA_TX_INTERVAL);
} else {
diff --git a/src/test/txrequest_tests.cpp b/src/test/txrequest_tests.cpp
index 298586db37..e6b3e8bc15 100644
--- a/src/test/txrequest_tests.cpp
+++ b/src/test/txrequest_tests.cpp
@@ -161,17 +161,18 @@ public:
* backwards (but note that the ordering of this event only follows the scenario's m_now.
*/
void Check(NodeId peer, const std::vector<GenTxidVariant>& expected, size_t candidates, size_t inflight,
- size_t completed, const std::string& checkname,
- std::chrono::microseconds offset = std::chrono::microseconds{0})
+ size_t completed, const std::string& checkname,
+ std::chrono::microseconds offset = std::chrono::microseconds{0})
{
const auto comment = m_testname + " " + checkname;
auto& runner = m_runner;
const auto now = m_now;
assert(offset.count() <= 0);
- runner.actions.emplace_back(m_now, [=,&runner]() {
+ runner.actions.emplace_back(m_now, [=, &runner]() {
std::vector<std::pair<NodeId, GenTxidVariant>> expired_now;
auto ret = runner.txrequest.GetRequestable(peer, now + offset, &expired_now);
- for (const auto& entry : expired_now) runner.expired.insert(entry);
+ for (const auto& entry : expired_now)
+ runner.expired.insert(entry);
runner.txrequest.SanityCheck();
runner.txrequest.PostGetRequestableSanityCheck(now + offset);
size_t total = candidates + inflight + completed;
@@ -193,7 +194,7 @@ public:
{
const auto& testname = m_testname;
auto& runner = m_runner;
- runner.actions.emplace_back(m_now, [=,&runner]() {
+ runner.actions.emplace_back(m_now, [=, &runner]() {
auto it = runner.expired.find(std::pair<NodeId, GenTxidVariant>{peer, gtxid});
BOOST_CHECK_MESSAGE(it != runner.expired.end(), "[" + testname + "] missing expiration");
if (it != runner.expired.end()) runner.expired.erase(it);
diff --git a/src/txrequest.cpp b/src/txrequest.cpp
index ec2ebe978b..0d0e7543d1 100644
--- a/src/txrequest.cpp
+++ b/src/txrequest.cpp
@@ -575,7 +575,7 @@ public:
}
void ReceivedInv(NodeId peer, const GenTxidVariant& gtxid, bool preferred,
- std::chrono::microseconds reqtime)
+ std::chrono::microseconds reqtime)
{
// Bail out if we already have a CANDIDATE_BEST announcement for this (txhash, peer) combination. The case
// where there is a non-CANDIDATE_BEST announcement already will be caught by the uniqueness property of the
@@ -595,7 +595,7 @@ public:
//! Find the GenTxids to request now from peer.
std::vector<GenTxidVariant> GetRequestable(NodeId peer, std::chrono::microseconds now,
- std::vector<std::pair<NodeId, GenTxidVariant>>* expired)
+ std::vector<std::pair<NodeId, GenTxidVariant>>* expired)
{
// Move time.
SetTimePoint(now, expired);
@@ -730,7 +730,7 @@ void TxRequestTracker::PostGetRequestableSanityCheck(std::chrono::microseconds n
}
void TxRequestTracker::ReceivedInv(NodeId peer, const GenTxidVariant& gtxid, bool preferred,
- std::chrono::microseconds reqtime)
+ std::chrono::microseconds reqtime)
{
m_impl->ReceivedInv(peer, gtxid, preferred, reqtime);
}
@@ -746,7 +746,7 @@ void TxRequestTracker::ReceivedResponse(NodeId peer, const GenTxidVariant& txhas
}
std::vector<GenTxidVariant> TxRequestTracker::GetRequestable(NodeId peer, std::chrono::microseconds now,
- std::vector<std::pair<NodeId, GenTxidVariant>>* expired)
+ std::vector<std::pair<NodeId, GenTxidVariant>>* expired)
{
return m_impl->GetRequestable(peer, now, expired);
}
diff --git a/src/txrequest.h b/src/txrequest.h
index f32fb1f2b9..3f2acef3a3 100644
--- a/src/txrequest.h
+++ b/src/txrequest.h
@@ -133,7 +133,7 @@ public:
* from the specified gtxid.
*/
void ReceivedInv(NodeId peer, const GenTxidVariant& gtxid, bool preferred,
- std::chrono::microseconds reqtime);
+ std::chrono::microseconds reqtime);
/** Deletes all announcements for a given peer.
*
@@ -164,7 +164,7 @@ public:
* simultaneously by one peer, and end up being requested from them, the requests will happen in announcement order.
*/
std::vector<GenTxidVariant> GetRequestable(NodeId peer, std::chrono::microseconds now,
- std::vector<std::pair<NodeId, GenTxidVariant>>* expired = nullptr);
+ std::vector<std::pair<NodeId, GenTxidVariant>>* expired = nullptr);
/** Marks a transaction as requested, with a specified expiry.
*
| indexed_transaction_set::const_iterator j = std::visit(util::Overloaded{ | ||
| [this](const Wtxid& wtxid) EXCLUSIVE_LOCKS_REQUIRED(cs) { return get_iter_from_wtxid(wtxid); }, | ||
| [this](const Txid& txid) EXCLUSIVE_LOCKS_REQUIRED(cs) { return mapTx.find(txid); } | ||
| }, hashb); |
There was a problem hiding this comment.
In 3e378fd:
These visitors can be cleaned up quite a bit by first replacing get_iter_from_wtxid with a new GetIter(const Wtxid&) overload (see stickies-v@2585f12 and stickies-v@589357b). This code block then becomes quite a bit more concise. It increases the diff a bit in other places, but part of that is from increased type safety. Overall, I think it's sensible to make this change in this PR?
auto j{std::visit([&](const auto& id) EXCLUSIVE_LOCKS_REQUIRED(cs) { return GetIter(id); }, hashb)};
if (!j.has_value()) return false;
auto i{std::visit([&](const auto& id) EXCLUSIVE_LOCKS_REQUIRED(cs) { return GetIter(id); }, hasha)};
if (!i.has_value()) return true;
uint64_t counta = i.value()->GetCountWithAncestors();
uint64_t countb = j.value()->GetCountWithAncestors();See the diff here (from branch master...stickies-v:bitcoin:2025-06/overload-getiter)
| } | ||
|
|
||
| TxMempoolInfo CTxMemPool::info_for_relay(const GenTxid& gtxid, uint64_t last_sequence) const | ||
| TxMempoolInfo CTxMemPool::info(const Wtxid& wtxid) const |
There was a problem hiding this comment.
In da99271:
With my other suggestion to implement GetIter(const Wtxid&), the info and info_for_relay duplication can be avoided, because they can now be trivially templated (moving them into txmempool.h). It does also require moving GetInfo to the header (see stickies-v@8d62f6e), but I think that's okay (ideally that function would just be replaced with a TxMempoolInfo(txiter) ctor but that's a much bigger diff).
These two functions can then be implemented as
template <typename T>
requires std::is_same_v<T, Txid> || std::is_same_v<T, Wtxid>
TxMempoolInfo info(const T& txid) const
{
LOCK(cs);
auto i{GetIter(txid)};
return i.has_value() ? GetInfo(*i) : TxMempoolInfo{};
}
/** Returns info for a transaction if its entry_sequence < last_sequence */
template <typename T>
requires std::is_same_v<T, Txid> || std::is_same_v<T, Wtxid>
TxMempoolInfo info_for_relay(const T& txid, uint64_t last_sequence) const
{
LOCK(cs);
auto i{GetIter(txid)};
return (i.has_value() && i.value()->GetSequence() < last_sequence) ? GetInfo(*i) : TxMempoolInfo{};
}See the diff here as part of branch master...stickies-v:bitcoin:2025-06/overload-getiter
Part of the type safety refactor.
This PR changes the GenTxid class to a variant, which holds both Txids and Wtxids. This provides compile-time type safety and eliminates the manual type check (bool m_is_wtxid). Variables that can be either a Txid or a Wtxid are now using the new GenTxid variant, instead of 8000 uint256.