[p2p/mempool] Two small fixes to node broadcast logic

jnewbery commented at 3:58 pm on June 16, 2021: member

Only add a transaction to the unbroadcast set when it’s added to the mempool

Currently, if BroadcastTransaction() is called to rebroadcast a transaction (e.g. by ResendWalletTransactions()), then we add the transaction to the unbroadcast set. That transaction has already been broadcast in the past, so peers are unlikely to request it again, meaning RemoveUnbroadcastTx() won’t be called and it won’t be removed from m_unbroadcast_txids.

Net processing will therefore continue to attempt rebroadcast for the transaction every 10-15 minutes. This will most likely continue until the node connects to a new peer which hasn’t yet seen the transaction (or perhaps indefinitely).

Fix by only adding the transaction to the broadcast set when it’s added to the mempool.
Allow rebroadcast for same-txid-different-wtxid transactions

There is some slightly unexpected behaviour when:
- there is already transaction in the mempool (the “mempool tx”)
- BroadcastTransaction() is called for a transaction with the same txid as the mempool transaction but a different witness (the “new tx”)
Prior to this commit, if BroadcastTransaction() is called with relay=true, then it’ll call RelayTransaction() using the txid/wtxid of the new tx, not the txid/wtxid of the mempool tx. For wtxid relay peers, in SendMessages(), the wtxid of the new tx will be taken from setInventoryTxToSend, but will then be filtered out from the vector of wtxids to announce, since m_mempool.info() won’t find the transaction (the mempool contains the mempool tx, which has a different wtxid from the new tx).

Fix this by calling RelayTransaction() with the wtxid of the mempool transaction in this case.

The third commit is a comment/whitespace only change to tidy up the BroadcastTransaction() function.

jnewbery added the label Bug on Jun 16, 2021

jnewbery added the label Mempool on Jun 16, 2021

jnewbery added the label P2P on Jun 16, 2021

dunxen commented at 8:12 pm on June 16, 2021: contributor

Concept ACK.

I’m wondering if it would make sense to test this behaviour in functional/mempool_unbroadcast.py? If so, I wouldn’t mind doing a follow-up PR :)

DrahtBot commented at 11:43 pm on June 16, 2021: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

Subawit approved

jnewbery commented at 10:40 am on June 17, 2021: member

@duncandean I agree a test would be very useful here. If you want to have a go at writing it, please go ahead and I can add it to this PR. If not, I’ll try to write one next week.

dunxen commented at 12:33 pm on June 17, 2021: contributor

Cool! Working on it now. I’ve got 1 down, just busy with 2.

Subawit approved

Subawit commented at 1:18 pm on June 17, 2021: none

in src/node/transaction.cpp:40 in 14cf5dec17 outdated

33@@ -34,7 +34,8 @@ TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef t
34     assert(node.peerman);
35     assert(node.mempool);
36     std::promise<void> promise;
37-    uint256 hashTx = tx->GetHash();
38+    uint256 txid = tx->GetHash();
39+    uint256 wtxid = tx->GetWitnessHash();

glozow commented at 4:42 pm on June 17, 2021:

Very smol suggestion for ease of review - This improvement in the naming is in the “[mempol] Allow rebroadcast for same-txid-different-wtxid transactions” commit 14cf5dec17af9cc57a85dfe4910f30d8e17581d8, but txid part would fit better in the subsequent [style] commit.

jnewbery commented at 11:15 am on June 18, 2021:

I changed the name hashTx -> txid in the same commit as adding the wtxid variable. It seemed to make sense to me to have the variables named txid/wtxid from the start instead of hashTx/wtxid.

Not sure the ordering matters that much. I’m inclined to leave it as it is right now, but will happily reorder/split commits if that’s helpful for other reviewers.

glozow commented at 4:50 pm on June 17, 2021: member

Concept ACK. Nice, clean 🐛 fixes. The same-txid-different-wtxid bug doesn’t seem terrible since nothing would happen when the wtxid isn’t found in mempool, but ResendWalletTransactions re-adding a txid to the unbroadcast set (and causing continuous rebroadcast after initial broadcast was already successful) is a privacy leak.

I was able to reproduce the bugs on this branch - I conveniently had a same-txid-different-wtxid test on hand that made it easy to write.

naumenkogs commented at 9:41 am on June 18, 2021: member

Concept ACK. Nice catch! Looking forward to a test :)

dunxen commented at 9:41 am on June 18, 2021: contributor

I was able to reproduce the bugs on this branch - I conveniently had a same-txid-different-wtxid test on hand that made it easy to write.

Hey @glozow, the first bug was straightforward enough to write a test for. I’ve been struggling a bit for the same-txid-different-wtxid bug. I was able to construct a new tx with same txid and different wtxid, but I’m just having trouble with the actual checks for relay. If you have that test handy, then maybe you could add it to this PR? :) I’d be curious to see what I should be doing.

jnewbery commented at 11:31 am on June 18, 2021: member

Hi @duncandean. I think the following test should demonstrate the new functionality of Allow rebroadcast for same-txid-different-wtxid transactions:

submit tx A to node’s mempool
Make new wtxidrelay P2PConnection to node
submit tx A’ to node’s mempool
wait for node to announce the wtxid from A to P2PConnection

Before the fix, the P2PConnection will never receive an announcement for the transaction. After the fix, it’ll receive an announcement for the wtxid from A. @glozow’s branch gives you the code you need to construct A and A'.

glozow commented at 12:52 pm on June 18, 2021: member

@duncandean this commit implements the test jnewbery described - feel free to put that on top of the same-txid-diff-wtxid code you’ve written.

dunxen commented at 1:22 pm on June 18, 2021: contributor

@duncandean this commit implements the test jnewbery described - feel free to put that on top of the same-txid-diff-wtxid code you’ve written. @glozow Ah, I actually got pretty close to that commit based on the previous work on your branch :D So would be good just to use your commit probably.

For 1 I think it was as simple as adding the following to the end of test_broadcast() in mempool_unbroadcast.py:

0self.log.info("Rebroadcast transaction & ensure it is not added to unbroadcast set when already in mempool")
1rpc_tx_hsh = node.sendrawtransaction(txFS["hex"])
2mempool = node.getrawmempool(True)
3assert rpc_tx_hsh in mempool
4for tx in mempool:
5    assert_equal(mempool[tx]['unbroadcast'], False)

It fails before the fix, with the transaction being added to the unbroadcast set, and it passes after the fix.

glozow commented at 5:33 pm on June 18, 2021: member

ACK ba99f37a51a277284c060e38651071d39dd9b42b

glozow added the label Review club on Jun 21, 2021

lsilva01 approved

lsilva01 commented at 9:06 pm on June 21, 2021: contributor

Code review ACK https://github.com/bitcoin/bitcoin/pull/22261/commits/ba99f37a51a277284c060e38651071d39dd9b42b

michaelfolkson commented at 8:31 am on June 24, 2021: contributor

Concept ACK, Approach ACK.

Took me a while to understand the two problems that were being fixed but I got there eventually in the Bitcoin Core PR review club. Essentially 1) is unnecessary relaying and 2) causes an increased number of relay failures.

Skimmed code, test(s) sounds like they are on the way.

theStack approved

theStack commented at 12:28 pm on July 4, 2021: member

Concept and approach ACK ba99f37a51a277284c060e38651071d39dd9b42b

Took me a while to understand the two problems that were being fixed but I got there eventually in the Bitcoin Core PR review club.

Same here! ;) If anyone isn’t familiar with the concept of the unbroadcast set yet (like me) I can warmly recommend studying the review club log for #18038 (https://bitcoincore.reviews/18038) first in preparation before grasping the one for this PR.

jnewbery force-pushed on Jul 7, 2021

jnewbery commented at 9:37 am on July 7, 2021: member

Thanks for the tests @duncandean and @glozow. I’ve added them as separate commits, and verified that they both fail without the fixes and pass with the fixes.

Also rebased on latest master.

dunxen approved

dunxen commented at 9:50 am on July 7, 2021: contributor

ACK b8a976f

jnewbery commented at 11:24 am on July 7, 2021: member

This has a small conflict in the tests with #22253. @theStack @duncandean @lsilva01 do you mind looking at that PR first? It’s easy enough for me to rebase this on that PR once it’s been merged.

naumenkogs commented at 11:28 am on July 7, 2021: member

ACK b8a976f3945283c26d1e206eabf181305d407c8a

I’m wondering if this is a good opportunity to document same-txid-different-witness somewhere in the codebase? I can totally imagine some wallet or L2 protocol attempt to submit these kinds of transactions in a non-malicious way? And at least what we could do is to clearly set up expectations. At the same time, not even sure what’s the best place to have this knowledge.

glozow commented at 11:31 am on July 7, 2021: member

@naumenkogs see #22253?

[mempool] Only add a transaction to the unbroadcast set when it's added to the mempool

Currently, if BroadcastTransaction() is called to rebroadcast a
transaction (e.g. by ResendWalletTransactions()), then we add the
transaction to the unbroadcast set. That transaction has already been
broadcast in the past, so peers are unlikely to request it again,
meaning RemoveUnbroadcastTx() won't be called and it won't be removed
from m_unbroadcast_txids.

Net processing will therefore continue to attempt rebroadcast for the
transaction every 10-15 minutes. This will most likely continue until
the node connects to a new peer which hasn't yet seen the transaction
(or perhaps indefinitely).

Fix by only adding the transaction to the broadcast set when it's added
to the mempool.

2837a9f1ea

[test] Test transactions are not re-added to unbroadcast set 847b6ed48d

[mempool] Allow rebroadcast for same-txid-different-wtxid transactions

This commit fixes some slightly unexpected behaviour when:

- there is already transaction in the mempool (the "mempool tx")
- BroadcastTransaction() is called for a transaction with the same txid
  as the mempool transaction but a different witness (the "new tx")

Prior to this commit, if BroadcastTransaction() is called with
relay=true, then it'll call RelayTransaction() using the txid/wtxid of
the new tx, not the txid/wtxid of the mempool tx. For wtxid relay peers,
in SendMessages(), the wtxid of the new tx will be taken from
setInventoryTxToSend, but will then be filtered out from the vector of
wtxids to announce, since m_mempool.info() won't find the transaction
(the mempool contains the mempool tx, which has a different wtxid from
the new tx).

Fix this by calling RelayTransaction() with the wtxid of the mempool
transaction in this case.

cd48372b67

DrahtBot added the label Needs rebase on Jul 9, 2021

[test] Allow rebroadcast for same-txid-different-wtxid transactions

Co-authored-by: John Newbery <john@johnnewbery.com>

7282d4c036

[style] Clean up BroadcastTransaction() 5a77abd4e6

jnewbery commented at 5:21 pm on July 9, 2021: member

Rebased. This is now ready for review again!

jnewbery force-pushed on Jul 9, 2021

DrahtBot removed the label Needs rebase on Jul 9, 2021

theStack approved

theStack commented at 7:53 pm on July 9, 2021: member

re-ACK 5a77abd4e657458852875a07692898982f4b1db5

Verified via git range-diff ba99f37a...5a77abd4 that the only change since my last ACK was adding two tests (one for each fix), which LGTM. Also tried running them in the current master branch and as expected, they failed.

lsilva01 approved

lsilva01 commented at 10:19 pm on July 9, 2021: contributor

re-ACK https://github.com/bitcoin/bitcoin/pull/22261/commits/5a77abd4e657458852875a07692898982f4b1db5

Tested on Ubuntu 20.04.

in src/node/transaction.cpp:65 in 5a77abd4e6

82+            // transaction if relay=true.
83+            //
84+            // The mempool transaction may have the same or different witness (and
85+            // wtxid) as this transaction. Use the mempool's wtxid for reannouncement.
86+            wtxid = mempool_tx->GetWitnessHash();
87+        } else {

rajarshimaitra commented at 10:41 am on July 10, 2021:

Maybe I am confused and missing other details, but here’s my doubt.

It seems here that if a new tx (with a same mempool-tx txid but diff wtxid) is passed into broadcast, then it will just check against the txid, see that it already exist in mempool, and then attempt to broadcast the existing wtxid instead of the new wtxid. It also wont try to add it to mempool or the unbroadcast list. But it should, as its a brand new transaction.

It seems to me that in case of same-txid-different-wtxid here, nothing happens with the new transaction, and we just relay the existing tx and call it a day? How is that not a problem?

dunxen commented at 8:33 pm on July 10, 2021:

But it should, as its a brand new transaction.

In terms of adding it to the mempool, it cannot since it’ll be rejected as TX_CONFLICT. And if we cannot add it to our mempool, then we should not relay the new wtxid as this is not in our mempool.

AFAIU with same-txid-different-wtxid the effect of the transaction (in terms of inputs and outputs) would be identical as it’s just the witness that would be different. I don’t think it would matter which one ends up being confirmed.

At least this is how I understand it to be :)

rajarshimaitra commented at 2:56 am on July 11, 2021:

Thanks @duncandean, that makes sense. So in the case of same-txid-different-wtxid transaction, we are really talking about the same transaction (input-output set) but for some reason with a different signature value.

In that case it would make sense to only handle one version of it, and yes its better to handle the version already in the mempool.

rajarshimaitra commented at 10:50 am on July 10, 2021: contributor

Concept ACK.

I understood the unbroadcast list addition change, and seems like a good idea to only add it in list if it wasn’t already in the mempool. And we are saying if it already exists in mempool then its high chance that other peer already knows about it, so no need to re-boradcast, just send it once. Is that correct?

hypothetical question: Is there any chance that we might have a transaction in our local mempool but not in the network? Maybe I just added it manually and forgot to broadcast? In that case the first if clause will return true and we will not add it into unbroadcast list and just relay it once. Do you think that can cause any problem.

I am more confused about the second part of the bug though and below is a detailed question.

Looking forward to step through the test, and may be I will be able to understand the behavior better then.

dunxen commented at 8:34 pm on July 10, 2021: contributor

reACK 5a77abd

in src/node/transaction.cpp:76 in 2837a9f1ea outdated

70@@ -71,6 +71,12 @@ TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef t
71 
72         // Transaction was accepted to the mempool.
73 
74+        if (relay) {
75+            // the mempool tracks locally submitted transactions to make a
76+            // best-effort of initial broadcast

naumenkogs commented at 8:44 am on July 12, 2021:

Previously, we would also best-effort rebroadcast (meaning not initial). After this change, best-effort applies only to the initial broadcast indeed (or the absent-from-mempool broadcast).

Is this the expected rebroadcast behavior? Do we clearly communicate that software-triggered-rebroadcast (wallet or whatever) won’t be assisted by the node if the node was offline at the time in the same way the node assists initial broadcast.

Behavior change:

A transaction added to the mempool initially broadcasted
Node goes offline
A transaction is still in the mempool, triggers rebroadcast.
Rebroadcast fails because offline

The difference is that after this code, UnbroadcastTx won’t help in step 4. Indeed, the peers are unlikely to request it again. It’s still possible though, if our mempool is more persistent (e.g., deeper in size).

This applies backward too: if our mempool is less persistent, we will make useless rebroadcasts (peers won’t send GETDATA because they still have it in the mempool). This part is indeed not a degradation over master, but still something to think about. Small-mempool nodes may become are likely to become “10-15 minutes” rebroadcast spammers (after only one extra broadcast trigger, but because Unbroadcast set never clears)

jnewbery commented at 9:48 am on July 12, 2021:

Previously, we would also best-effort rebroadcast (meaning not initial). After this change, best-effort applies only to the initial broadcast indeed (or the absent-from-mempool broadcast).

Is this the expected rebroadcast behavior?

Yes, this is the intention, and was the original intention behind the unbroadcast set. Locally submitted transactions should be tracked to make sure that at least one peer had requested them at least once.

Behavior change:

A transaction added to the mempool initially broadcasted

Node goes offline

A transaction is still in the mempool, triggers rebroadcast.

Rebroadcast fails because offline

The difference is that after this code, UnbroadcastTx won’t help in step 4.

Correct. That’s intentional. The eventual aim of the rebroadcast project is to remove the need for the local wallet to rebroadcast transactions in the majority of cases. I hope that eventually the default behavior is that (3) doesn’t happen at all.

naumenkogs commented at 11:41 am on July 20, 2021: member

Not 100% sure I agree with all the thoughts from this comment, but this fix is indeed needed. ACK 5a77abd4e657458852875a07692898982f4b1db5

fanquake merged this on Jul 20, 2021

fanquake closed this on Jul 20, 2021

jnewbery deleted the branch on Jul 20, 2021

glozow commented at 1:06 pm on July 20, 2021: member

post merge ACK 5a77abd

sidhujag referenced this in commit 8c27083d3f on Jul 23, 2021

gwillen referenced this in commit 3fff335dff on Jun 1, 2022

DrahtBot locked this on Aug 16, 2022

[p2p/mempool] Two small fixes to node broadcast logic #22261

Conflicts