This is based on #18044 (review).
A new type GenTxid is added to protocol.h, which represents a tagged txid-or-wtxid. The tx request logic is updated to use these instead of uint256s, permitting per-announcement distinguishing of txid/wtxid (instead of assuming that everything we want to request from a wtxid peer is wtx). Then the restriction of orphan-parent requesting to non-wtxid peers is lifted.
Also document BIP339 in doc/bips.md.
@ajtowns I made a few changes compared to your branch:
m_tx_process_time - this feels more obviously correct.Let me know what you think.
398+public:
399+ GenTxid(bool is_wtxid, const uint256& hash) : m_is_wtxid(is_wtxid), m_hash(hash) {}
400+ bool IsWtxid() const { return m_is_wtxid; }
401+ const uint256& GetHash() const { return m_hash; }
402+ friend bool operator==(const GenTxid& a, const GenTxid& b) { return a.m_is_wtxid == b.m_is_wtxid && a.m_hash == b.m_hash; }
403+ 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); }
Since you’re using accessor methods anyway, could make it:
0private:
1 const std::tuple<bool, uint256> data;
2public:
3 friend bool operator<(const GenTxid& a, const GenTxid& b) { return a.data < b.data; }
4521@@ -4519,24 +4522,24 @@ bool PeerLogicValidation::SendMessages(CNode* pto)
4522
4523 auto& tx_process_time = state.m_tx_download.m_tx_process_time;
4524 while (!tx_process_time.empty() && tx_process_time.begin()->first <= current_time && state.m_tx_download.m_tx_in_flight.size() < MAX_PEER_TX_IN_FLIGHT) {
4525- const uint256 txid = tx_process_time.begin()->second;
4526+ const GenTxid txid = tx_process_time.begin()->second;
gtxid or gentxid everywhere. txid used to mean something different before, and it still sort of does?
GenTxid variables in net_processing to be gtxid.
3009+ // wtxidrelay peers.
3010+ // Eventually we should replace this with an improved
3011+ // protocol for getting all unconfirmed parents.
3012+ CInv _inv(MSG_TX | nFetchFlags, txin.prevout.hash);
3013+ pfrom.AddKnownTx(txin.prevout.hash);
3014+ if (!AlreadyHave(_inv, mempool)) RequestTx(State(pfrom.GetId()), ToGenTxid(_inv), current_time);
I believe this wouldn’t work at the moment, because there’s a check:
0 if (State(pfrom.GetId())->m_wtxid_relay) {
1 if (inv.type == MSG_TX) continue;
2 } else {
3 if (inv.type == MSG_WTX) continue;
4 }
Does it rely on some other pending PR? Is that what you referring to: “Based on a commit by Anthony Towns.”?
This is in INV handling. When requesting parents of an orphan, there is no INV involved: a GETDATA is sent directly in response to receiving the TX.
Am I missing something why changing this is needed?
4505@@ -4505,7 +4506,7 @@ bool PeerLogicValidation::SendMessages(CNode* pto)
4506 if (state.m_tx_download.m_check_expiry_timer <= current_time) {
4507 for (auto it=state.m_tx_download.m_tx_in_flight.begin(); it != state.m_tx_download.m_tx_in_flight.end();) {
4508 if (it->second <= current_time - TX_EXPIRY_INTERVAL) {
4509- LogPrint(BCLog::NET, "timeout of inflight tx %s from peer=%d\n", it->first.ToString(), pto->GetId());
4510+ LogPrint(BCLog::NET, "timeout of inflight tx %s from peer=%d\n", it->first.GetHash().ToString(), pto->GetId());
GenTxid::ToString which specifies whether the hash is a wtxid or a regular txid
All of the txrequest data structures use it, not just m_tx_process_time - this feels more obviously correct.
:+1:
The restriction that wtxidrelay peers can only announce using MSG_WTX invs remains - this isn’t violated by txid-based orphan parent requests (which are get GETDATAs, not INVs).
Ha. I thought when I was writing that patch that removing that restriction wasn’t supposed to be necessary.
Code review ACK 154778d1e4f053145979e9faf5178d0ece84370a
41@@ -42,3 +42,4 @@ BIPs that are implemented by Bitcoin Core (up-to-date up to **v0.19.0**):
42 * [`BIP 173`](https://github.com/bitcoin/bips/blob/master/bip-0173.mediawiki): Bech32 addresses for native Segregated Witness outputs are supported as of **v0.16.0** ([PR 11167](https://github.com/bitcoin/bitcoin/pull/11167)). Bech32 addresses are generated by default as of **v0.20.0** ([PR 16884](https://github.com/bitcoin/bitcoin/pull/16884)).
43 * [`BIP 174`](https://github.com/bitcoin/bips/blob/master/bip-0174.mediawiki): RPCs to operate on Partially Signed Bitcoin Transactions (PSBT) are present as of **v0.17.0** ([PR 13557](https://github.com/bitcoin/bitcoin/pull/13557)).
44 * [`BIP 176`](https://github.com/bitcoin/bips/blob/master/bip-0176.mediawiki): Bits Denomination [QT only] is supported as of **v0.16.0** ([PR 12035](https://github.com/bitcoin/bitcoin/pull/12035)).
45+* [`BIP 339`](https://github.com/bitcoin/bips/blob/master/bip-0339.mediawiki): Support for announcing and fetching transactions by wtxid is supported as of **v0.21.0** ([PR 18044](https://github.com/bitcoin/bitcoin/pull/18044)).
f8e8a44d suggestion for “support… is supported” here if you retouch
0* [`BIP 339`](https://github.com/bitcoin/bips/blob/master/bip-0339.mediawiki): Announcing and fetching transactions by wtxid is supported as of **v0.21.0** ([PR 18044](https://github.com/bitcoin/bitcoin/pull/18044)).
or just “Relaying…”
ACK 154778d1e code review, debug build/tests/ran node, checked test here fails on master and reverted test fails on the branch.
Reviewers: suggest reviewing 54a0936 with -w.
git range-diff 007e15dc 154778d 338f80e and re-code review. Re-debug build with Clang 12, ran tests and node.
2940- nodestate->m_tx_download.m_tx_in_flight.erase(hash);
2941- EraseTxRequest(hash);
2942+ for (auto gtxid : {GenTxid{false, txid}, GenTxid{true, wtxid}}) {
2943+ nodestate->m_tx_download.m_tx_announced.erase(gtxid);
2944+ nodestate->m_tx_download.m_tx_in_flight.erase(gtxid);
2945+ EraseTxRequest(gtxid);
Is this a slight behavior change ?
By erasing both wtxid/txid from g_already_asked_for, if we receive a wtxid from a wtixd-relay peer and at same time gets an announcement for the same tx from few txid-relay only peers, when we monitor our getdatas in SendMessages, we would send a spurious GETDATA for the first-processed txid-relay peer as last_request_time is null.
3005- pfrom.AddKnownTx(txin.prevout.hash);
3006- if (!AlreadyHave(_inv, mempool)) RequestTx(State(pfrom.GetId()), ToGenTxid(_inv), current_time);
3007- }
3008+ for (const CTxIn& txin : tx.vin) {
3009+ // Here, we only have the txid (and not wtxid) of the
3010+ // inputs, so we only request in txid mode, even for
Strictly interpreting the BIP, the requirement for wtxid-relay on fetching is scoped to “announced transactions from that peer”, and orphan parents have not been announced so we don’t violate wtxid-relay policy here. Concern is on future BIP implementers on how they interpret “is required” and severing the connection for violations. Lightweight clients broadcasting a parent + CPFP tx may know issues due to this.
In the meanwhile, I share Suhas point to not commit a given handling of this behavior. Maybe we should amend the BIP to invite liberal enforcement of violations, just dropping non-complying invs/getdatas ?
458@@ -459,7 +459,7 @@ struct CNodeState {
459 };
460
461 // Keeps track of the time (in microseconds) when transactions were requested last time
462-limitedmap<uint256, std::chrono::microseconds> g_already_asked_for GUARDED_BY(cs_main)(MAX_INV_SZ);
463+limitedmap<GenTxid, std::chrono::microseconds> g_already_asked_for GUARDED_BY(cs_main)(MAX_INV_SZ);
Switching from uint256 here to GenTxid means that when a non-segwit transaction is announced by both a wtxid-relay peer and txid-relay peer, we’ll be more likely to request it twice – for instance, if we first hear of it from a txidrelay peer, and then hear about it >2s later from a wtxidrelay peer (but before the transaction is successfully fetched).
It doesn’t seem to me that we need to change the key in this map; am I missing something? I don’t necessarily expect this to be a large issue, relatively speaking – we already expect some tx download duplication while having both txidrelay and wtxidrelay peers – but not sure if this is an oversight or has some good reasons behind it.
The reason I’m changing it here is because I have a hard time reasoning about the consistency of the various data structures around tx requesting if we don’t. That, plus the fact that I don’t really see a way to maintain the same behaviour sanely in #19184 where I’d hope to not duplicate the information any more.
Could this be addressed by instead testing for both WTX+wtxid and TX+wtxid?
I haven’t yet reviewed #19184, so I’ll try to do that to get a better sense of what you’re referring to.
Could this be addressed by instead testing for both WTX+wtxid and TX+wtxid?
I think that would also work, but if that’s how we would do all lookups into g_already_asked_for, I don’t know why we’d bother with that complexity?
To be clear, my suggested change is fairly minimal code:
0diff --git a/src/net_processing.cpp b/src/net_processing.cpp
1index 6cee368ed72..36c890e395b 100644
2--- a/src/net_processing.cpp
3+++ b/src/net_processing.cpp
4@@ -459,7 +459,7 @@ struct CNodeState {
5 };
6
7 // Keeps track of the time (in microseconds) when transactions were requested last time
8-limitedmap<GenTxid, std::chrono::microseconds> g_already_asked_for GUARDED_BY(cs_main)(MAX_INV_SZ);
9+limitedmap<uint256, std::chrono::microseconds> g_already_asked_for GUARDED_BY(cs_main)(MAX_INV_SZ);
10
11 /** Map maintaining per-node state. */
12 static std::map<NodeId, CNodeState> mapNodeState GUARDED_BY(cs_main);
13@@ -756,12 +756,12 @@ static void FindNextBlocksToDownload(NodeId nodeid, unsigned int count, std::vec
14
15 void EraseTxRequest(const GenTxid& gtxid) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
16 {
17- g_already_asked_for.erase(gtxid);
18+ g_already_asked_for.erase(gtxid.GetHash());
19 }
20
21 std::chrono::microseconds GetTxRequestTime(const GenTxid& gtxid) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
22 {
23- auto it = g_already_asked_for.find(gtxid);
24+ auto it = g_already_asked_for.find(gtxid.GetHash());
25 if (it != g_already_asked_for.end()) {
26 return it->second;
27 }
28@@ -770,9 +770,9 @@ std::chrono::microseconds GetTxRequestTime(const GenTxid& gtxid) EXCLUSIVE_LOCKS
29
30 void UpdateTxRequestTime(const GenTxid& gtxid, std::chrono::microseconds request_time) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
31 {
32- auto it = g_already_asked_for.find(gtxid);
33+ auto it = g_already_asked_for.find(gtxid.GetHash());
34 if (it == g_already_asked_for.end()) {
35- g_already_asked_for.insert(std::make_pair(gtxid, request_time));
36+ g_already_asked_for.insert(std::make_pair(gtxid.GetHash(), request_time));
37 } else {
38 g_already_asked_for.update(it, request_time);
39 }
This would capture my understanding of the g_already_asked_for data structure: when scheduling transaction requests, we don’t care how a given txid/wtxid was requested (via txid or wtxid), just that a getdata for that hash was sent out at a certain time.
I see.
It’s just a question of whether we treat the tx request data as conceptually “keyed” by hash (and seeing is_wtxid as metadata along with the request), or by (is_wtxid, hash). The fact that we may want to treat a wtxid request for the same hash as a txid request is a sign that it should probably be the first.
I picked the (is_wtxid, hash} based keying first as it seemed easier to reason about, but if consistently applied (treating all requests for the same hash as identical), there doesn’t seem to be much difference.
Both are possible to integrate into #19184, with not much complexity difference. But it has to be consistent - we can’t be treating them as identical in some cases, and differentiate in others.
I think this would also mean making m_tx_in_flight and m_tx_announced be hash-keyed (as @ajtowns’ original patch did)?
3011+ // wtxidrelay peers.
3012+ // Eventually we should replace this with an improved
3013+ // protocol for getting all unconfirmed parents.
3014+ CInv _inv(MSG_TX | nFetchFlags, txin.prevout.hash);
3015+ pfrom.AddKnownTx(txin.prevout.hash);
3016+ if (!AlreadyHave(_inv, mempool)) RequestTx(State(pfrom.GetId()), ToGenTxid(_inv), current_time);
uint256 might already be in-flight (from the same peer or another) as a WTX, but because the map keys have all been changed to be by GenTxid, this would go undetected.
I have a hard time reasoning about the consistency of the various data structures around tx requesting if we don’t.
I 100% agree with this. Net processing is complicated enough as it is without having data structures hold a mixture of different types. txids and wtxids are different types that just happen to both be encoded in a uint256, which means the compiler can’t protect us from bugs where we mistake one for the other.
If we’re going to do this, we should at least make it very clear to the programmer by adding comments to the data structures and functions that are doing this. For example, EraseTxRequest(), GetTxRequestTime()and UpdateTxRequestTime() all have a parameter named txid. Anyone reading/reviewing that code would therefore expect those parameters to be txids (and by extension, g_already_asked_for to be keyed by txid).
2937@@ -2936,9 +2938,11 @@ void ProcessMessage(
2938
2939 TxValidationState state;
2940
2941- nodestate->m_tx_download.m_tx_announced.erase(hash);
2942- nodestate->m_tx_download.m_tx_in_flight.erase(hash);
2943- EraseTxRequest(hash);
2944+ for (uint256 hash : {txid, wtxid}) {
ed2c0b97baa
0 for (const uint256& hash : {txid, wtxid}) {
GenTxids.
ACK 707d0f4b0b0f21
I added a few CInv tx msg type boolean helpers on this branch to simplify ToGenTxid() and the net processing code, if you’re inclined to use any of it: https://github.com/jonatack/bitcoin/commits/add-inv-msg-booleans (EDIT: opened #19590 for the parts non-specific to this branch).
400@@ -401,7 +401,7 @@ struct CNodeState {
401 /* Track when to attempt download of announced transactions (process
402 * time in micros -> txid)
403 */
404- std::multimap<std::chrono::microseconds, uint256> m_tx_process_time;
405+ std::multimap<std::chrono::microseconds, GenTxid> m_tx_process_time;
CInv as the value here, rather than creating a new class just for use here?
2935@@ -2936,9 +2936,11 @@ void ProcessMessage(
2936
2937 TxValidationState state;
2938
2939- nodestate->m_tx_download.m_tx_announced.erase(hash);
2940- nodestate->m_tx_download.m_tx_in_flight.erase(hash);
2941- EraseTxRequest(hash);
2942+ for (uint256 hash : {txid, wtxid}) {
2943+ nodestate->m_tx_download.m_tx_announced.erase(hash);
2944+ nodestate->m_tx_download.m_tx_in_flight.erase(hash);
This is a behavior change? We used to call it once on txid OR wtxid, now you calling it twice?
That’s probably fine, but why use this for loop at all? hash is already defined above, depending on the peer’s wtxid support
Before, we only ever asked for hashes in one way from a peer, so it was sufficient to only erase that one.
That’s no longer the case: outgoing GETDATA requests may be txid or wtxid based, and we don’t remember which of the two (it depends on whether it was in response to an INV, or a request for an orphan parent), so either we track that, or just delete both.
It’s of course a change in behavior, but that’s the point: we’re now requesting orphans from wtxidrelay peers (using txid requests), and this is one of the consequences.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
CInv block message helpers; use in net processing by jonatack)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.
GenTxid uses, without changing behavior.
715@@ -716,22 +716,24 @@ class CTxMemPool
716 return totalTxSize;
717 }
718
719- bool exists(const uint256& hash, bool wtxid=false) const
720+ bool exists(const GenTxid& txid) const
1674@@ -1676,9 +1675,9 @@ void static ProcessGetBlockData(CNode& pfrom, const CChainParams& chainparams, c
1675 }
1676
1677 //! Determine whether or not a peer can request a transaction, and return it (or nullptr if not found or not allowed).
1678-CTransactionRef static FindTxForGetData(const CNode& peer, const uint256& txid_or_wtxid, bool use_wtxid, const std::chrono::seconds mempool_req, const std::chrono::seconds now) LOCKS_EXCLUDED(cs_main)
1679+CTransactionRef static FindTxForGetData(const CNode& peer, const GenTxid& txid_or_wtxid, const std::chrono::seconds mempool_req, const std::chrono::seconds now) LOCKS_EXCLUDED(cs_main)
txid_or_wtxid to gtxid to match other functions.
9@@ -10,6 +10,7 @@
10 #ifndef BITCOIN_PROTOCOL_H
11 #define BITCOIN_PROTOCOL_H
12
13+#include <primitives/transaction.h>
2936@@ -2936,9 +2937,11 @@ void ProcessMessage(
2937
2938 TxValidationState state;
2939
2940- nodestate->m_tx_download.m_tx_announced.erase(hash);
2941- nodestate->m_tx_download.m_tx_in_flight.erase(hash);
2942- EraseTxRequest(hash);
2943+ for (const GenTxid& gtxid : {GenTxid(false, txid), GenTxid(true, wtxid)}) {
Code review ACK c048357f24. A few comments inline.
Thanks for updating the functions to take GenTxids. I think that’s clearer. I still think that the original version of this PR was better still, and that storing GenTxids in the various tx download data structures is easier to reason about and worth the marginal loss in efficiency, but I guess that all this is going away with #19184 so it doesn’t matter too much.
2683@@ -2683,7 +2684,7 @@ void ProcessMessage(
2684 pfrom.fDisconnect = true;
2685 return;
2686 } else if (!fAlreadyHave && !chainman.ActiveChainstate().IsInitialBlockDownload()) {
2687- RequestTx(State(pfrom.GetId()), inv.hash, current_time);
2688+ RequestTx(State(pfrom.GetId()), ToGenTxid(inv), current_time);
ToGenTxid() expects that we constructed the CInv ourselves or that we verified that it’s for a transaction type CInv. Here, the only guarantee that inv is transaction type is that AlreadyHave returns true for non-transaction and non-block type invs. That’s not obvious and seems a bit brittle. Perhaps we should change this else branch into an else if (inv.type == MSG_TX || inv.type == MSG_WITNESS_TX || inv.type == MSG_WTX) to make this safer?
AlreadyHave was split into different functions for block and tx, and the tx version took a GenTxid. Could be done in a follow up. I have a branch here that does that: https://github.com/jnewbery/bitcoin/tree/2020-07-split-already-have
else if (inv.type == MSG_TX || inv.type == MSG_WITNESS_TX || inv.type == MSG_WTX)to make this safer?
per change in protocol.h:L447 this would now be simpler: else if (inv.IsGenTxMsg())
Based on a commit by Anthony Towns.