Use wtxid for transaction relay #18044

I wonder if this should be sent immediately in response to a VERSION message, rather than in response to a VERACK. It looks like the way I’ve done it here creates a race condition where a peer could send a txid-based inv to us before it gets this message, which I think would cause relay of that transaction to fail.

https://travis-ci.org/bitcoin/bitcoin/jobs/644609638?utm_medium=notification&utm_source=github_status

Seeing a build warning here:

0net_processing.cpp: In member function ‘virtual bool PeerLogicValidation::SendMessages(CNode*)’:
1net_processing.cpp:4135:42: warning: enumeral and non-enumeral type in conditional expression [-Wextra]
2             CInv inv(state.m_wtxid_relay ? MSG_WTX : (MSG_TX | GetFetchFlags(pto)), txid);
3                      ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Fixed, thanks.

Not sure exactly what you’re asking but it seemed easier to add an additional map, rather than change the existing data structure to support wtxid-lookup.

From a single node’s perspective, as long as you have at least 1 honest wtxid-relay peer, such an attack would not work, because the wtxid for the honest version of the transaction would not be added to your filter (only the txid), and so you would eventually request the valid version of the transaction from your honest wtxid-relay peer.

Currently we preference outbound peers for transaction download, so once we believe that some sufficient majority of listening nodes support wtxid-relay, such that the chances of having 0 or just a few wtxid-relay-outbound-peers is pretty small, then I think we can just get rid of this.

I don’t think our threat model has ever really been too concerned with every node getting every transaction, anyway – we just don’t want the network as a whole to not relay a transaction altogether because of something like this. So exactly how long we should wait before dropping this logic is debatable IMO; anyway we should defer the debate to the future when someone thinks this is ready.

Disconnecting is too much, but ignoring the wtxid_relay message may be reasonable.

Alternatively (and probably better), we can just make the comment more clear. Something like “currently this is not enforced, because at most we get slight inefficiency if not following this rule”.

And then the responsibility would be on the future developer to introduce the disconnect/ignore behavior if they choose to fundamentally rely on this assumption.

54d58e7

3 new pointers for boost::multi_index::tag<index_by_wtxid>, mempoolentry_wtxid, SaltedTxidHasher, not one for new boost::multi_index::hashed_unique ?

@ariard I don’t understand this comment (https://github.com/bitcoin/bitcoin/pull/18044/commits/1c382b500b2b6025f9a4cf4cd37af678172c6ae1#r385362270).

I think 3 pointers for an extra index is probably close to correct, but #18086 may make these kinds of heuristics unnecessary, so that’s probably a better approach long term.

d0f03e9

Is this rule a best-effort right now? At tx reception, we may have to request parent based only on the input. If so is the BIP clear enough on the 5) ? Precise also there is no current sanction of the rule by receiver as of today

Yes, you made a good observation – our fetching of parents of orphan transactions must be by txid, rather than wtxid. I did try to carefully word the BIP to avoid referencing this situation:

After a node has sent and received a “wtxidrelay” message to/from a given peer, the node is required to use the MSG_WTX inv-type when announcing transactions to that peer, or requesting announced transactions from that peer.

So the BIP says that newly announced transactions must be by wtxid. And if your peer announces a transaction by wtxid, you must fetch it by wtxid (and not txid). The BIP is silent on requests for transactions that are not announced – we could make this explicit but this has never been documented behavior, and I’m not sure we should commit to a certain handling of this kind of thing anyway… But open to suggestions.

cf8c70f

For context-tracking, would be fine to even further comment, “right now if we reject witness transactions based on their txids, relying on wtxid-based relay peers might lead to a stripped-witness tx starvation due to the low number of wtxid-relay peers at protocol deployment”

Also, once we reach a deployment meaningful enough, should we merge together TX_WITNESS_MUTATED and TX_WITNESS_STRIPPED ?

s/the filter/the recentRejects filter/ in 6e7290f to aid git grepping (in which case can drop “reject” in the next line).

Idem for the section in ProcessMessage()::L2684 containing the same paragraph.

The comment is technically correct because it’s referring to the TXID_RELAY_DELAY constant, not how it is computed. Compare it with how before it might have been static const int TXID_RELAY_DELAY = 2 * 1000000, where 2 was still a value in seconds.

It’s a silly semantics discussion though. With the std::chrono types everywhere the comment could just be “Duration of delay …”.

the comment could just be “Duration of delay …”.

Yes.

Slightly more efficient and C++11ish:

0                            auto ret2 = mapRelay.emplace(ret.first->second->GetWitnessHash(), ret.first->second);

I think this is just a style choice, trying to balance different tradeoffs:

• if you change a function’s arguments, then you have to change all call sites, unless you use an optional argument
• changing lots of call sites is not great as it adds noise to PR review
• optional arguments are not great as they can make it harder to maintain code and do review in the future

But sometimes optional arguments are the easier way to go, to avoid having to embed hairy logic at a bunch of call sites to decide which function to call, which can also harm readability. So I think it’s just a balance, and I tried to strike the balance I thought made sense when I was writing all this which wasn’t always the same for every example, but maybe I could have done better.

Anyway at this point I’d prefer not to make stylistic changes to this PR.

FWIW, I think mempool.exists(hash, by_wtxid) is more convenient/readable in AlreadyHave (the only place wtxid_exists() is used); it would let you say:

0    const bool by_wtxid = (inv.type == MSG_WTX); 
1    return mempool.exists(inv.hash, by_wtxid));

instead of

0    return (inv.type != MSG_WTX && mempool.exists(inv.hash)) ||
1           (inv.type == MSG_WTX && mempool.wtxid_exists(inv.hash));

In commit “Add a wtxid-index to the mempool”

I think this would be a nice improvement.

This logic of requesting unconfirmed parent transactions when an orphan is received should eventually go away, in favor of some sort of package relay scheme, where orphan transactions trigger discovery of all unconfirmed ancestors – so I think the question is what to do in the meantime. I think wtxid-based relay is more important than optimizing this behavior when we encounter an orphan.

Note that we only just recently (#16851, merged last October) fixed things so that these requests worked for transactions that are not in mapRelay. Also note that for transactions that are still in mapRelay, or for non-segwit transactions where txid and wtxid are the same, these requests will still work. However this is admittedly a regression in behavior, undoing the benefit of #16851. However given how long that behavior was broken (and that the recent fix was largely at my urging, I believe), it’s hard for me to think that anyone is really too concerned with this.

Anyway it is possible to improve this by keeping extra bookkeeping so that requests generated this way go out as MSG_TX, but I don’t think it’s worth it – it’s a lot of additional complexity for very little benefit, and I think effort spent on improving orphan transaction relay should be devoted to a p2p protocol extension that supports it directly.

I disagree with the argument “this was broken for a long time and we only recently fixed it, so it’s ok to break it again”

it is possible to improve this by keeping extra bookkeeping … it’s a lot of additional complexity.

This doesn’t seem like too much additional complexity. We store in m_tx_process_time whether the request should be for a txid or a wtxid, and set the GETDATA INV based on that. That’s one additional bit of information, which could be a bool or a enum GetDataMsg.

for transactions that are still in mapRelay […] these requests will still work

Oh, I hadn’t realised that we’d respond to a MSG_WTX GETDATA request for a wtxid with a transaction that has that txid. The draft BIP doesn’t specify that we should do that, and it seems extremely unintuitive. Could we have two mapRelays - one based on txid and one based on wtxid so we don’t do this?

(I also think that we should remove mapRelay entirely (#17303), which depends on responding to NOTFOUNDs (#18238), which currently is broken and seems to be waiting for this PR (https://github.com/bitcoin/bitcoin/pull/18238#discussion_r398318859))

I disagree with the argument “this was broken for a long time and we only recently fixed it, so it’s ok to break it again”

Please, that is not the entirety of the argument; the substance of what is being “broken” – and how significant it is – is relevant too. It’s not like transaction relay was horribly broken until the last release.

For context: fixing the behavior as we did was something that I suggested we do in order to make another improvement I wanted to make – package relay – something that we could introduce in two parts, first as a pure client-side behavior integrated with orphan handling, and then as a p2p protocol extension. Since then, I concluded that splitting those two things up doesn’t make sense, as getting the orphan handling part right is complicated enough that it isn’t worth the review burden for something that would just be ripped out later (and so I think we just go straight to package relay with a p2p protocol extension).

I think it’d be perfectly reasonable to just rip out this behavior of trying to request parents when we receive an orphan – it is fundamentally unreliable and is already just an opportunistic way to improve relay in one edge case situation. Adding complexity solely for the purpose of supporting something that flaky is something we should all be skeptical about; this part of the codebase is already very difficult for most people to understand.

Oh, I hadn’t realised that we’d respond to a MSG_WTX GETDATA request for a wtxid with a transaction that has that txid. The draft BIP doesn’t specify that we should do that, and it seems extremely unintuitive. Could we have two mapRelays - one based on txid and one based on wtxid so we don’t do this?

I don’t see the downside – the behavior I’ve implemented seems strictly better, though non-essential and not something I think we need to specify in a BIP as behavior we promise to maintain.

I don’t see the downside

Just my opinion, but: the p2p code is already really complicated and not really well understood, adding new behaviours and having the implementation and the spec differ in ways that matter in practice and aren’t obvious seems like it makes that a fair bit worse. This makes the review burden for both this PR and future p2p changes much higher, since you can’t just read the BIPs to understand how things should work, or the local bits of code to see how they do work to see if a change makes sense, but you have to deeply understand how many things interact to even get started.

I was going to suggest adding code to explicitly ignore INVs that come in with the wrong TX/WTX marker for the peer sending them, and to not request orphan txs’ parents when we’re in WTX mode – I do agree that behaviour’s not too crucial to keep even without package relay; I just don’t think mixing wtxid/txid values and having functionality relying on weird undocumented special cases like that working sometimes (only while the tx is in mapRelay but not after expiry?) is okay.

(Not a nack, just my opinion; I’m open to being convinced otherwise. But I think I’ll go back to poking at the notfound pr rather than holding off for this one getting merged for now)

@ajtowns My view on this is that what goes into a BIP are the essential behaviors that all implementations should abide by in order to make an idea work, but non-essential implementation details are left for projects to individually figure out.

This makes it easier for new protocol extensions to get adopted by the whole community (the fewer requirements, the better), and for our own software to stay compatible with the BIPs we claim to implement, as otherwise we’d have to constantly deprecate/redraft/repropose, as we make minor changes to our own implementation.

I’d say that how we handle a getdata request for a value that we never announced is one of those non-essential behaviors – ignoring it is certainly fair, and returning something useful is obviously better still. No one should assume, or would assume, that this is something that ought to work, so I don’t think any software would be relying on this.

Also, if we had a set of design docs for how our own project implements particular BIPs, I’d agree that those should reflect more exactly what our code does – but that would be a new initiative for us.

I find this behavior very confusing. Can you help me understand if I’m getting any of this wrong:

1. We have a peer that has negotiated wtxid relay
2. That peer relays us a transaction whose parents we don’t have
3. We want to request the parent transaction, so we put the txid in the m_tx_process_time and m_tx_announced for this peer (https://github.com/bitcoin/bitcoin/pull/18044/files#diff-eff7adeaec73a769788bb78858815c91R2654). That txid will later be added to the global g_already_asked_for (https://github.com/bitcoin/bitcoin/pull/18044/files#diff-eff7adeaec73a769788bb78858815c91R4151)
4. In SendMessages() we request that transaction with a MSG_WTX GETDATA, but with the hash set to the txid (https://github.com/bitcoin/bitcoin/pull/18044/files#diff-eff7adeaec73a769788bb78858815c91R3990).
5. The peer responds to that GETDATA with a TX because it looks in its mapRelay for transactions with a txid OR wtxid that match (https://github.com/bitcoin/bitcoin/pull/18044/files#diff-eff7adeaec73a769788bb78858815c91R1628)
6. When we receive the TX, we try to remove the wtxid from the peer’s m_tx_announced and m_tx_in_flight, and the global g_already_asked_for (https://github.com/bitcoin/bitcoin/pull/18044/files#diff-eff7adeaec73a769788bb78858815c91R2587-R2594)

The thing I don’t understand is that we seem to be adding the txid to these per-peer and global structures, and then trying to remove the wtxid from them. Will the txid ever be cleared? Do we end up in a request loop with the peer?

I also can’t quite follow the logic here: https://github.com/bitcoin/bitcoin/pull/18044/files#diff-eff7adeaec73a769788bb78858815c91R4139-R4140

It looks to me like the txid has been added to m_tx_process_time, but when we call into AlreadyHave(inv, m_mempool), the inv is a MSG_WTX, so we’ll check in our mempool for a transaction with that wtxid (which we won’t find). We’ll then rerequest the transaction from our peer.

@sdaftuar “No one should assume, or would assume, that this is something that ought to work” – but… aren’t we assuming that it will work when we send a WTX request with a txid instead of a wtxid?

Aside from any bugs, I think this adds three lots of technical debt:

• we’re replying to GETDATA WTX <txid> and GETDATA TX <wtxid> if the tx is still in mapRelay – this will go away if/when mapRelay goes away
• we’re asking for WTX <txid> when looking for parents of orphan transactions – if we figure out how to replace orphan handling with package relay, this will go away too; but in the meantime it means mapRelay going away breaks orphan handling, and I think it breaks orphan handling when the parent’s >15m old immediately
• in the functional tests, as of #18446 at least, we’re still sending TX INVs/GETDATAs etc despite claiming to be of a version that supports wtxid relay

One alternative might be to embrace the ambiguity: make the BIP be “if you signal wtxidrelay, then we’ll send INV TX wtxid instead of INV TX txid, and also allow you to request txs by either GETDATA TX txid or GETDATA TX wtxid” ?

Seems okay. I think it could make sense to explicitly skip INVs offering a TX/WTX that we’d incorrectly GETDATA as a WTX/TX (respectively) as well:

 0--- a/src/net_processing.cpp
 1+++ b/src/net_processing.cpp
 2@@ -2323,6 +2323,13 @@ bool ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStream& vR
 3             if (interruptMsgProc)
 4                 return true;
 5 
 6+            // ignore INVs that don't match wtxidrelay setting
 7+            if (State(pfrom->GetId())->m_wtxid_relay) {
 8+                if (inv.type == MSG_TX) continue;
 9+            } else {
10+                if (inv.type == MSG_WTX) continue;
11+            }
12+
13             bool fAlreadyHave = AlreadyHave(inv, mempool);
14             LogPrint(BCLog::NET, "got inv: %s  %s peer=%d\n", inv.ToString(), fAlreadyHave ? "have" : "new", pfrom->GetId());

I had a go at updating fjahr’s tests, and it seems to make reasonable sense (either with or without the change I suggest here)

(This still leaves us with the tech debt of our functional tests mostly testing tx relay when we’ll be moving towards doing wtxid relay most of the time in the real world, but that’s not really avoidable)

(This still leaves us with the tech debt of our functional tests mostly testing tx relay when we’ll be moving towards doing wtxid relay most of the time in the real world, but that’s not really avoidable)

I think that should be addressed with this update: https://github.com/fjahr/bitcoin/commit/becdb68ab37fdcbc1dcf743b19ea8629f52677bb

I think it could make sense to explicitly skip INVs offering a TX/WTX that we’d incorrectly GETDATA as a WTX/TX (respectively) as well:

And I agree with @ajtowns , this behavior makes intuitive sense to me and it matches my interpretation of the current description in the BIP: “After a node has sent and received a “wtxidrelay” message to/from a given peer, the node is required to use the MSG_WTX inv-type when announcing transactions to that peer, or requesting announced transactions from that peer.”

I think it would even be reasonable to argue about banning peers for this behavior if there is an agreement that this behavior is wrong. The node is unnecessarily spamming the network and just silently dropping the message might let it take longer until an issue with this is discovered by the node user.

from a quick look, seems like using const and reference declarator together on this std::pair<uint256,uint256> type is causing the build warnings. I don’t yet understand why.

leaving & but removing the const throws an error:

0net_processing.cpp:835:39: error: non-const lvalue reference to type 'pair<uint256, [...]>' cannot bind to a value of unrelated type 'pair<const uint256, [...]>'
1    for (std::pair<uint256, uint256>& elem : unbroadcast_txids) {

but removing the reference declarator allows it to build with no errors / warnings. with or without the const.

The reason is that the element type of unbroadcast_txids is std::pair<const uint256, uint256> which can’t be converted to std::pair<uint256, uint256>. If you want a const reference to such a thing, it will be copied, and you’ll get a reference to the copy.

The correct solution is either (const) std::pair<const uint256, uint256>&, or just const auto&.

As a follow-up, I think we may scope this under a connection type, like #19316.

Side-note, @amitiuttarwar, I had only a super-quick overview on connection type, it’s a great move but IMO I would adopt a more fine-grained typology. Right now it sounds to confuse type of traffic (block, addr, tx) and type of connection selection (outbound/inbound, normal/anchor, …)

AFAICT, if we have sighash_noinput+Eltoo channel design in the future, commitment transactions are going to be symmetric but witness likely asymmetric (one per-party). So you would observe wtxidA != wtxidB but txidA == txidB which means in case of concurrent broadcast by different parties you will spuriously re-download transactions and only gets one version in your mempool.

Assuming such class of transactions traffic (multiple-parties, asymmetric witnesses, high-rate of concurrent broadcasts) get few digits of overall traffic it may be a serious bandwidth increase and may not be compatible with a wtxid-based tx-relay. Of course, as of today it’s really hypothetical but you may find such pattern with vault protocols (multiple-parties, concurrent broadcast, asymmetric witnesses).

“spuriously re-download transactions and only gets one version in your mempool” already happens with RBF; I don’t think this is meaningfully different?

(If you’re just varying the witness you don’t need SIGHASH_NOINPUT, though, so I think there’s a good chance the txid would be different too for eltoo transactions if that comes into play)

I don’t understand the comment here – are you saying wtxid-relay is somehow incompatible with a future change to the protocol? It seems to me that (a) wtxid-relay is more likely to support the ability to relay transactions with same underlying txid but different wtxid than our existing relay protocol, and (b) whether we adopt changes to the mempool acceptance rules to allow for this is otherwise orthogonal to this PR.

If you’re asking for a change here, can you indicate what that is?

“spuriously re-download transactions and only gets one version in your mempool” already happens with RBF; I don’t think this is meaningfully different?

I think you’re right we do already have this kind of case due to single-party RBF. After thoughts, qualifying them of spurious was a fallacy as a smaller valid witness may increase transaction feerate and such increase its probabilities of propagating and confirming. So we have to download them anyway.

(I agree SIGHASH_NOINPUT doesn’t force you to vary witness, it’s more likely we would do it, fyi another generalized channel proposal leveraging per-party adaptor sigs to get symmetric states)

I’m not advocating on any change here, I was more brooding on the fact that fancier on-top-of-Bitcoin protocols and automatic dynamic-fee bumping as currently gauged by LN implementations will have for side-effect to increase bandwidth demand in case of mempool congestion.

and (b) whether we adopt changes to the mempool acceptance rules to allow for this is otherwise orthogonal to this PR.

Yes, I was wrong that’s orthogonal to tx-relay, it’s more being sure that these potential bandwidth demand surges are priced “correctly” with regards to BIP 125 rule 4. Not a discussion to have in this PR.

I think we should be really careful about including TX_WITNESS_STRIPPED to the rejection filter.

In LN, your channel counterparty knows your own version of the commitment transaction as it has to build it to sign it. It means a malicious counterparty would be able to broadcast a witness stripped version of it at any moment and that way get it added to the rejection filters of any p2p peers. If the full-node used by your LN-node is non-upgraded it will announce broadcast of a witness transaction (the local commitment) and it won’t be fetched by the lured peers thus effectively censoring your time-sensitive transactions.

Is this understanding of wtxid tx-relay correct ? I don’t think it changes anything that your topology is well-established with wtxid-relay peers, the issue is you not being upgraded and subject to intentional reject filters collisions.

Can you explain what you think needs to change in the code here?

I think the documentation is trying to explain a necessary (but not sufficient) condition for changing the code at some unspecified point in the future. If you want to argue that we should never change this code because other ecosystem software might break, then that’s something worthy of discussion, but I don’t know that we need to have that discussion in this PR, given that we’re not proposing actually doing that now (nor even proposing a timeline for it).

Can you reference this comment/issue directly around the code ?

I think that’s a bit different than Matt’s previous comment here where we only considered censorship of a wtixd-relay requester, which should be fine if you have at least 1 honest wtxid-relay peer as you pointed.

The scenario I’m describing is I think different, it’s censorship of an non-upgraded wtxid-relay initial sender being only connected to reject-witness-stripped wtxid-relay peers. This sender announcing through txid only won’t have its transaction fetched and propagated due to an already existent collision in the rejection filter. Such topology configuration sounds realistic for private nodes.

I don’t want to argue that we should never change this but when we do it we give a clear warning and time buffer to the rest of the ecosystem to be sure they upgrade. Because by switching p2p policy rules you would effectively introduce a critical vulnerability for any higher-layer software pending on a non-upgraded full-node. LN security model fundamentally reverse the double-spend problem to a private matter, it’s the duty of your individual LN-node and full-node to ensure that your transactions propagate well and confirm quickly.

And I don’t think we have guidelines for this today, it sounds more like an illustration of the lack of a set of design docs for a) how we implement BIPs b) what assumptions ecosystem software can do on stability of such implemented rules that you’re describing here.

Do you mean just add a link to your github comment, or something else?

Here’s the current comment, which I think captures the issue you’re describing (as far as I understand, what you’re talking about is fundamentally the same as the issue we flagged years ago when we first deployed segwit, see #8279):

0                // Do not add txids of witness transactions or witness-stripped
1                // transactions to the filter, as they can have been malleated;
2                // adding such txids to the reject filter would potentially
3                // interfere with relay of valid transactions from peers that
4                // do not support wtxid-based relay. See
5                // [#8279](/bitcoin-bitcoin/8279/) for details.
6                // We can remove this restriction (and always add wtxids to
7                // the filter even for witness stripped transactions) once
8                // wtxid-based relay is broadly deployed.

But if there’s specific different language or a link you’d like to see in here, please just provide alternate text.

Right the scenario I’m describing is leveraging the same original issue from #8279, just concerning censorship of a non-upgraded initial sender.

0               // Note, any on-top-of-base-layer software (eg LN-node) relying on a non-upgraded to wtxid-relay full-node 
1              //  would be subject to censor of its time-sensitive transactions. See [#18044 (review)](/bitcoin-bitcoin/18044/#discussion_r443419034)
2             //  Removing this restriction should be done considering ecosystem coordination.

I think “broadly” is to dim, ideally you want 99% of upgrade among LN-node’s full-nodes.

I will update the comment to incorporate your suggestion somewhat, including providing a link to the comments as you request, but I have a philosophical difference of opinion about the implications here of ecosystem software that is dependent on particular p2p behavior (specifically with regard to your “ideally you want 99% of upgrade among LN-node’s full nodes” comment).

We have anti-CPU DoS reasons for wanting to get rid of the code that tries to determine whether a transaction is witness stripped (as well as generally simplifying the code that handles transactions that fail mempool acceptance). Requiring that ecosystem software eventually update – which can be as simple as putting txid-relay nodes behind wtxid-relay border nodes – in order to be protected against an esoteric attack, with many years of notice, seems completely reasonable to me.

In particular, if we see that the vast majority of full nodes upgrade to wtxid-relay support over the next few years, but a handful of stubborn lightning implementations have not, I think at some point we could say, too bad, find your own workaround for transaction relay if you’re worried about this kind of attack. For these kinds of non-consensus security issues, I think it’s reasonable for this project to provide software and solutions (including documenting our behavior and communicating that to our users) and expect that people worried about those kinds of problems will take care of it themselves. At some point it should be okay for us to have shipped solutions and given people plenty of time to upgrade, and it’s not our responsibility if there are people who have chosen not to follow our advice.

Believe the same use of const auto& is needed here as used in net_processing.

0        for (const auto& elem : unbroadcast_txids) {

I think you may have a slight InvBlock until wtxid-relay is double-digit deployed, a wtxid-relay-signaling inbound peer can announce a MSG_TX and thus being processed with same time priority than txid-relay outbound peers. By holding the TX and assume your victim only have txid-relay peers you block transaction propagation

Maybe CalculateTxGetDataTime should use the inv type instead of peer signaling to apply the delay ?

Agree with @sdaftuar. It’s annoying that it adds variables in many places, but the various CInv states really don’t correspond to what we need here either.

It may be useful to have an encapsulated (use_wtxid, txid_or_wtxid) pair type, but let’s keep that for later (I think it would make sense for combining with #19184 as well).

I don’t know what SIGHASH flags has to do with this, can you elaborate on the scenario you have in mind?

My understanding of the logic here is that at this point in the code, we know that a parent of this orphan transaction will definitely not be accepted by our node (as it is already in our reject filter). Since a txid is a commitment to the inputs of a transaction, this means that we can definitively know that we’ll never accept any transaction with the same txid as this orphan, regardless of what witness it comes with.

In commit “Add wtxids of confirmed transactions to bloom filter”

It may be worth doing this only in case the txid and wtxid differ (inserting the same thing twice still counts towards expiration of old entries). In the worst case it doesn’t matter (it maintains the last 24000 inserted txid/wtxid pairs always), but in the average case, as long as there are non-witness transactions doing so would increase that to somewhere between 24000 and 48000.

This will only request parents of orphan transactions when received from non-wtxid peers? What if all peers support wtxid?

Can’t we still request them, but with MSG_TX instead of MSG_WTX? That’d require keeping track of wtxidness per request, unfortunately.

In commit “Add support for tx-relay via wtxid”

I think RemoveUnbroadcastTx will always take txids, not wtxids, so this should be changed to tx->GetHash().

We set fRejectedParents based on parent’s txid in recentRejects. If parent has been previously announced and rejected based on wtxid only we won’t found it. So we won’t add this orphan and may fetch it again. I don’t see this case covered in new test ?

Yes, this is an unfortunate problem, but I don’t see a way to fix the redownloading of transactions with failed parent without implementing a new p2p protocol to support efficiently communicating the unconfirmed ancestor wtxid’s of a given transaction (in order to know whether to reprocess a given transaction).

Note that before this PR, we have this same problem not just for child transactions of a reject parent, but for all transactions that have witnesses which fail our policy checks.

In theory we could distinguish whether failure is due to a definitely-not-witness-related reason, and in that case, add both the txid and wtxid to the recentRejects filter. I don’t think this accomplishes much, because it won’t occur in non-adverserial scenarios, and in adverserial ones, the attacker can just use invalid witnesses if he wants to cause harm with invalid transactions.

Edit: seems the code is already doing that for transactions spending rejected ones.

I think we could try to special case the common scenario that we expect to happen at some point: some transaction spending a segwit version 1 output is rejected by (older) software as nonstandard, and then child transactions are relayed and redownloaded from each upgraded peer that announces it because the parent transaction’s txid wouldn’t be found in the reject filter. We could improve this behavior by treating failures in AreInputsStandard() as indicative of the txid being definitely bad, and therefore add the txid to the reject filter (even though the transaction has a witness).

I think my preference would be to defer this kind of somewhat-tricky-to-reason-about change to a future PR, since this PR seems ready to me and is already an improvement, but if others feel differently we should discuss.

I don’t think this change is necessary (or desirable). I guess it was prompted by this comment: #18038 (review) , but storing this as a map from txid->wtxid is unnecessary and confusing, since it obfuscates the purpose of this data, which is a set of transactions that may need to be rebroadcast.

The only place that the wtxid is read from this map is in ReattemptInitialBroadcast(), at which point we can fetch the transaction from the mempool (we very nearly do that with m_mempool.exists()), and get the wtxid from there.

cc @amitiuttarwar @ariard

My mistake here, effectively you need to announce transactions from the unbroadcast set by wtxid to upgraded peers but as you point out it doesn’t mean we need to cache them as a solution.

Maybe we should precise unbroadcast set semantic wrt same txid, different wtxids, we only guarantee reattempt to the best mempool candidate known for a given txid at the time we call ReattemptInitialBroadcast, not insertion (AddUnbroadcastTx). #18044 (review) isn’t an issue.

😮 wow that’s a great point @jnewbery. thank you!!

I thought during implementation I ran into something that prevented me from exclusively having txids on the set, but that evaluation is simply wrong. I’ve taken a first pass at reverting unbroadcast to a set here: https://github.com/amitiuttarwar/bitcoin/commit/f51cccdd6c97d1b1e45163d56ebd6b7d29a2f587. I’ll need to revisit with fresh eyes and then will PR.

I agree set makes more sense than a map- its simpler, communicates the intent better, and there’s no noticeable efficiency gain with the map. Since we check the transaction is the mempool, the difference between map vs set is the difference between calling CTxMemPool::get vs CTxMemPool::exists. This boils down to mapTx.find(hash) vs mapTx.count(hash), which both have log(n) complexity (according to the boost docs I found).