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

jnewbery · 2021-06-16T15:58:52Z

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.

dunxen · 2021-06-16T20:12:18Z

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 · 2021-06-16T23:43:08Z

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

Conflicts

No conflicts as of last run.

jnewbery · 2021-06-17T10:40:35Z

@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 · 2021-06-17T12:33:50Z

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

Subawit

glozow

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.

src/node/transaction.cpp

naumenkogs · 2021-06-18T09:41:16Z

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

dunxen · 2021-06-18T09:41:39Z

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 · 2021-06-18T11:31:33Z

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 · 2021-06-18T12:52:13Z

@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 · 2021-06-18T13:22:50Z

@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:

self.log.info("Rebroadcast transaction & ensure it is not added to unbroadcast set when already in mempool")
rpc_tx_hsh = node.sendrawtransaction(txFS["hex"])
mempool = node.getrawmempool(True)
assert rpc_tx_hsh in mempool
for tx in mempool:
    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 · 2021-06-18T17:33:41Z

ACK ba99f37

lsilva01

Code review ACK ba99f37

michaelfolkson · 2021-06-24T08:31:06Z

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

Concept and approach ACK ba99f37

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 · 2021-07-07T09:37:56Z

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

ACK b8a976f

jnewbery · 2021-07-07T11:24:07Z

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 · 2021-07-07T11:28:33Z

ACK b8a976f

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 · 2021-07-07T11:31:43Z

@naumenkogs see #22253?

…ed 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.

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.

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

jnewbery · 2021-07-09T17:21:52Z

Rebased. This is now ready for review again!

theStack

re-ACK 5a77abd

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

re-ACK 5a77abd

Tested on Ubuntu 20.04.

rajarshimaitra

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.

src/node/transaction.cpp

dunxen · 2021-07-10T20:34:44Z

reACK 5a77abd

src/node/transaction.cpp

naumenkogs · 2021-07-20T11:41:19Z

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

glozow · 2021-07-20T13:06:08Z

post merge ACK 5a77abd

…logic 5a77abd [style] Clean up BroadcastTransaction() (John Newbery) 7282d4c [test] Allow rebroadcast for same-txid-different-wtxid transactions (glozow) cd48372 [mempool] Allow rebroadcast for same-txid-different-wtxid transactions (John Newbery) 847b6ed [test] Test transactions are not re-added to unbroadcast set (Duncan Dean) 2837a9f [mempool] Only add a transaction to the unbroadcast set when it's added to the mempool (John Newbery) Pull request description: 1. 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. 2. 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. ACKs for top commit: duncandean: reACK 5a77abd naumenkogs: ACK 5a77abd theStack: re-ACK 5a77abd lsilva01: re-ACK bitcoin@5a77abd Tree-SHA512: d1a46d32a9f975220e5b432ff6633fac9be01ea41925b4958395b8d641680500dc44476b12d18852e5b674d2d87e4d0160b4483e45d3d149176bdff9f4dc8516

jnewbery added Bug Mempool P2P labels Jun 16, 2021

This was referenced Jun 16, 2021

validation: distinguish between same tx and same-nonwitness-data tx in mempool #22253

Merged

policy: Trim Packages when transaction with same txid exists in mempool #22252

Closed

Subawit approved these changes Jun 17, 2021

View reviewed changes

glozow reviewed Jun 17, 2021

View reviewed changes

src/node/transaction.cpp Show resolved Hide resolved

glozow added the Review club label Jun 21, 2021

lsilva01 approved these changes Jun 21, 2021

View reviewed changes

theStack approved these changes Jul 4, 2021

View reviewed changes

jnewbery force-pushed the 2021-06-broadcast-fixes branch 2 times, most recently from 68d1646 to b8a976f Compare July 7, 2021 09:37

dunxen approved these changes Jul 7, 2021

View reviewed changes

dunxen and others added 2 commits July 9, 2021 17:24

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

847b6ed

DrahtBot added the Needs rebase label Jul 9, 2021

glozow and others added 2 commits July 9, 2021 18:21

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

7282d4c

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

[style] Clean up BroadcastTransaction()

5a77abd

jnewbery force-pushed the 2021-06-broadcast-fixes branch from b8a976f to 5a77abd Compare July 9, 2021 17:22

DrahtBot removed the Needs rebase label Jul 9, 2021

theStack approved these changes Jul 9, 2021

View reviewed changes

lsilva01 approved these changes Jul 9, 2021

View reviewed changes

rajarshimaitra reviewed Jul 10, 2021

View reviewed changes

src/node/transaction.cpp Show resolved Hide resolved

naumenkogs reviewed Jul 12, 2021

View reviewed changes

src/node/transaction.cpp Outdated Show resolved Hide resolved

fanquake merged commit 8ed8164 into bitcoin:master Jul 20, 2021

jnewbery deleted the 2021-06-broadcast-fixes branch July 20, 2021 13:01

JaredTate mentioned this pull request Oct 30, 2021

WIP: Feature/8.22.0 Random Crash Bug & Missing Dandelion Relay TX Code DigiByte-Core/digibyte#77

Closed

gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022

Merge 8ed8164 into merged_master (Bitcoin PR bitcoin/bitcoin#22261)

3fff335

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jnewbery commented Jun 16, 2021

dunxen commented Jun 16, 2021

DrahtBot commented Jun 16, 2021 •

edited

jnewbery commented Jun 17, 2021

dunxen commented Jun 17, 2021

Subawit left a comment

glozow left a comment

naumenkogs commented Jun 18, 2021

dunxen commented Jun 18, 2021

jnewbery commented Jun 18, 2021

glozow commented Jun 18, 2021

dunxen commented Jun 18, 2021

glozow commented Jun 18, 2021

lsilva01 left a comment

michaelfolkson commented Jun 24, 2021

theStack left a comment

jnewbery commented Jul 7, 2021

dunxen left a comment

jnewbery commented Jul 7, 2021

naumenkogs commented Jul 7, 2021

glozow commented Jul 7, 2021

jnewbery commented Jul 9, 2021

theStack left a comment

lsilva01 left a comment

rajarshimaitra left a comment •

edited

dunxen commented Jul 10, 2021

naumenkogs commented Jul 20, 2021

glozow commented Jul 20, 2021

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

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

Conversation

jnewbery commented Jun 16, 2021

dunxen commented Jun 16, 2021

DrahtBot commented Jun 16, 2021 • edited

Conflicts

jnewbery commented Jun 17, 2021

dunxen commented Jun 17, 2021

Subawit left a comment

Choose a reason for hiding this comment

glozow left a comment

Choose a reason for hiding this comment

naumenkogs commented Jun 18, 2021

dunxen commented Jun 18, 2021

jnewbery commented Jun 18, 2021

glozow commented Jun 18, 2021

dunxen commented Jun 18, 2021

glozow commented Jun 18, 2021

lsilva01 left a comment

Choose a reason for hiding this comment

michaelfolkson commented Jun 24, 2021

theStack left a comment

Choose a reason for hiding this comment

jnewbery commented Jul 7, 2021

dunxen left a comment

Choose a reason for hiding this comment

jnewbery commented Jul 7, 2021

naumenkogs commented Jul 7, 2021

glozow commented Jul 7, 2021

jnewbery commented Jul 9, 2021

theStack left a comment

Choose a reason for hiding this comment

lsilva01 left a comment

Choose a reason for hiding this comment

rajarshimaitra left a comment • edited

Choose a reason for hiding this comment

dunxen commented Jul 10, 2021

naumenkogs commented Jul 20, 2021

glozow commented Jul 20, 2021

DrahtBot commented Jun 16, 2021 •

edited

rajarshimaitra left a comment •

edited