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:
private:
const std::tuple<bool, uint256> data;
public:
friend bool operator<(const GenTxid& a, const GenTxid& b) { return a.data < b.data; }
Maybe personal preference, but I don't think slightly easier comparators weighs up against losing meaningful names for the member variables.
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;
Not a super-strong opinion, but I'm wondering if we should call it gtxid or gentxid everywhere. txid used to mean something different before, and it still sort of does?
Seems reasonable; done. I've changed all 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:
if (State(pfrom.GetId())->m_wtxid_relay) {
if (inv.type == MSG_TX) continue;
} else {
if (inv.type == MSG_WTX) continue;
}
Does it rely on some other pending PR? Is that what you referring to: "Based on a commit by Anthony Towns."?
Or maybe it's just missing a commit to remove this check, I don't know :)
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?
This works as-is for me both live against a mainnet wtxid relay peer, and via the modified test.
You are right, I was confused by INV/GETDATA. Feel free to mark as resolved :)
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());
Could have a GenTxid::ToString which specifies whether the hash is a wtxid or a regular txid
Done.
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
utACK 154778d
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
* [`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..."
Done.
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.
ACK 338f80e per 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.
I could imagine that another way this might manifest itself -- even when only connected to wtxid-relay peers -- is if a non-segwit parent tx and some child tx are announced at roughly the same time, and the child propagates to us before the parent (eg if we request the transactions from different peers), then the orphan fetching on the parent when the child is received might result in a spurious getdata of the parent as a TX when the parent has already been fetched and is in flight as a WTX (possibly even from the same peer!).
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:
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 6cee368ed72..36c890e395b 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -459,7 +459,7 @@ struct CNodeState {
};
// Keeps track of the time (in microseconds) when transactions were requested last time
-limitedmap<GenTxid, std::chrono::microseconds> g_already_asked_for GUARDED_BY(cs_main)(MAX_INV_SZ);
+limitedmap<uint256, std::chrono::microseconds> g_already_asked_for GUARDED_BY(cs_main)(MAX_INV_SZ);
/** Map maintaining per-node state. */
static std::map<NodeId, CNodeState> mapNodeState GUARDED_BY(cs_main);
@@ -756,12 +756,12 @@ static void FindNextBlocksToDownload(NodeId nodeid, unsigned int count, std::vec
void EraseTxRequest(const GenTxid& gtxid) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
{
- g_already_asked_for.erase(gtxid);
+ g_already_asked_for.erase(gtxid.GetHash());
}
std::chrono::microseconds GetTxRequestTime(const GenTxid& gtxid) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
{
- auto it = g_already_asked_for.find(gtxid);
+ auto it = g_already_asked_for.find(gtxid.GetHash());
if (it != g_already_asked_for.end()) {
return it->second;
}
@@ -770,9 +770,9 @@ std::chrono::microseconds GetTxRequestTime(const GenTxid& gtxid) EXCLUSIVE_LOCKS
void UpdateTxRequestTime(const GenTxid& gtxid, std::chrono::microseconds request_time) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
{
- auto it = g_already_asked_for.find(gtxid);
+ auto it = g_already_asked_for.find(gtxid.GetHash());
if (it == g_already_asked_for.end()) {
- g_already_asked_for.insert(std::make_pair(gtxid, request_time));
+ g_already_asked_for.insert(std::make_pair(gtxid.GetHash(), request_time));
} else {
g_already_asked_for.update(it, request_time);
}
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)?
Done.
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);
As I mentioned elsewhere, this same 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.
Worst case this causes us to request/track the same transaction twice?
I already changed the code to address this.
Just did a code review and spotted one potential issue, but otherwise looks good. Will test.
Changed the code to treat tx announcements as keyed by hash (rather than hash+wtxidness), so that txid requests for parents of unconfirmed transactions won't result in a duplicate query if a wtxid with the same hash is already in flight. This reduces the diff a bit as well, and removed the need for ToString().
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
for (const uint256& hash : {txid, wtxid}) {
Done, together with an additional commit to changes it to 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;
Is there a good reason to not use a CInv as the value here, rather than creating a new class just for use here?
I have a follow-up PR that changes several functions to take a GenTxid instead of is_wtxid/txid_or_wtxid arguments.
Added those commits to this PR.
Code review ACK, apart from the test change which I haven't reviewed. Will test.
ACK 707d0f4b0b0f21bbc9f962119fb130e4cceec6dc
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.
I see how it works now. Part of my confusion was that it's part of the "refactor" commit, perhaps better belongs to the next commit.
You're right; moved it to the other commit.
utACK 707d0f4b0b0f21bbc9f962119fb130e4cceec6dc
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
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.
I added 3 extra commits that introduce more GenTxid uses, without changing behavior.
utACK c048357f2421eb7a5b81375be267e71fd2d80334
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
nit: var name should be gtxid
other places in this commit too: c048357f2421eb7a5b81375be267e71fd2d80334
Done.
utACK c048357
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)
Consider renaming txid_or_wtxid to gtxid to match other functions.
Done.
9 | @@ -10,6 +10,7 @@ 10 | #ifndef BITCOIN_PROTOCOL_H 11 | #define BITCOIN_PROTOCOL_H 12 | 13 | +#include <primitives/transaction.h>
sort plz
don.
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)}) {
I agree with @naumenkogs (https://github.com/bitcoin/bitcoin/pull/19569#discussion_r460714058) that this change should be introduced in p2p: enable fetching of orphans from wtxid peers instead of refactor: add GenTxid (=txid or wtxid) type and use it for tx request logic
Done.
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.
Light ACK c048357f2421eb7a5b81375be267e71fd2d80334 modulo Gleb's and John's feedback above.
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?
I think it'd be better if 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())
Let's fix that by splitting AlreadyHave up separately.
Based on a commit by Anthony Towns.
Rebased and addressed comments.
ACK 10b7a6d532148f880568c529e61a6d7edc7c91a9 -- code review. Using gtxid to replace the is_txid_or_wtxid flag for the mempool functions is nice.
utACK 10b7a6d
ACK 10b7a6d532148f880568c529e61a6d7edc7c91a9
Code review ACK 10b7a6d532148f880568c529e61a6d7edc7c91a9
I have a backport of (the first 4 commits of) this to v0.20 here: https://github.com/jnewbery/bitcoin/tree/2020-07-v20-wtxid-orphan. It can be added to #19606 or PR'ed separately. Whatever is easiest for reviewers.