p2p: don't add AlreadyHave transactions to recentRejects

troygiorshev commented at 7:01 pm on August 17, 2020: contributor

If we already have a transaction, don’t add it to recentRejects

Now, we only add a transaction to our recentRejects filter if we didn’t already have it, meaning that it is added at most once, as intended.

DrahtBot added the label P2P on Aug 17, 2020

benthecarman commented at 0:07 am on August 18, 2020: contributor

Concept ACK

Looks like the p2p_permissions.py test is failing

https://travis-ci.org/github/bitcoin/bitcoin/jobs/718732253#L3341

amitiuttarwar commented at 0:35 am on August 18, 2020: contributor

strong concept ACK! nice find :)

jnewbery commented at 2:07 pm on August 18, 2020: member

Concept ACK

troygiorshev force-pushed on Aug 18, 2020

troygiorshev commented at 3:21 pm on August 18, 2020: contributor

Fixed the broken test

in test/functional/p2p_permissions.py:162 in 9325a06bed outdated

145@@ -146,7 +146,7 @@ def check_tx_relay(self):
146             [tx],
147             self.nodes[1],
148             success=False,
149-            reject_reason='Not relaying non-mempool transaction {} from forcerelay peer=0'.format(txid),
150+            reject_reason='{} from peer=0 was not accepted: txn-mempool-conflict'.format(txid)

jnewbery commented at 9:00 am on August 19, 2020:

I think to fully test that sending an invalid transaction from a forcerelay peer doesn’t result in the tx being relayed, you’d need to add:

0+        p2p_rebroadcast_wallet.send_txs_and_test(
1+            [tx],
2+            self.nodes[1],
3+            success=False,
4+            reject_reason='Not relaying non-mempool transaction {} from forcerelay peer=0'.format(txid)
5+        )
6+

The first time the transaction is sent, it’s rejected and not added to the mempool, but is added to recentRejects. The second time the transaction is sent, we AlreadyHave it and choose not to relay it because it’s not in the mempool.

troygiorshev commented at 4:18 pm on August 24, 2020:

thanks, fixed

jnewbery commented at 8:00 am on September 2, 2020:

You should also add a comment here to explain why you’re sending the transaction twice.

jnewbery renamed this:
~~p2p: fix recentRejects filling bug~~
p2p: don't add AlreadyHave transactions to recentRejects
on Aug 19, 2020

jnewbery force-pushed on Aug 19, 2020

jnewbery commented at 9:28 am on August 19, 2020: member

utACK. One suggested change to the test.

DrahtBot commented at 8:06 pm on August 20, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#20158 (tree-wide: De-globalize ChainstateManager by dongcarl)

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.

in src/net_processing.cpp:2975 in 95134c836b outdated

2988+                    LogPrintf("Not relaying non-mempool transaction %s from forcerelay peer=%d\n", tx.GetHash().ToString(), pfrom.GetId());
2989+                } else {
2990+                    LogPrintf("Force relaying tx %s from peer=%d\n", tx.GetHash().ToString(), pfrom.GetId());
2991+                    RelayTransaction(tx.GetHash(), tx.GetWitnessHash(), connman);
2992+                }
2993+            }

MarcoFalke commented at 7:35 am on August 23, 2020:

An early return would simplify the code flow

0            }
1            return;

Neither state, nor lRemovedTxn will be set, so there is no need for the fallthrough over 200 LOC for nothing.

troygiorshev commented at 3:59 pm on August 24, 2020:

Good point, done

MarcoFalke commented at 4:40 pm on August 24, 2020:

nit: state and lRemovedTxn can now be moved down to after the return. They are not needed in this scope at all

MarcoFalke commented at 7:41 am on August 23, 2020: member

Not sure if this is the right approach. One could also argue that all txs accepted to the mempool should be added to recentRejects, so that prior versions of rbfed txs will be rejected right away without going through another costly ATMP round. No strong opinion though.

DrahtBot added the label Needs rebase on Aug 24, 2020

jnewbery commented at 3:40 pm on August 24, 2020: member

One could also argue that all txs accepted to the mempool should be added to recentRejects

No. That’s completely changing the meaning of recentRejects.

guardian939 commented at 3:46 pm on August 24, 2020: none

Concept ACK

Looks like the p2p_permissions.py test is failing

https://travis-ci.org/github/bitcoin/bitcoin/jobs/718732253#L3341

troygiorshev force-pushed on Aug 24, 2020

troygiorshev commented at 4:21 pm on August 24, 2020: contributor

git range-diff master 95134c8 47132bb

rebase
now uses early exit
made suggested test change

I’ll leave any idea changes to recentRejects for a follow-up.

in test/functional/p2p_permissions.py:159 in 47132bbdaf outdated

145@@ -146,7 +146,14 @@ def check_tx_relay(self):
146             [tx],

MarcoFalke commented at 4:57 pm on August 24, 2020:

nit: Could add a comment here that this first relay is only needed to make the node think it already has the transaction

(sorry for leaving nits in succession instead of in one reply, feel free to ignore)

troygiorshev commented at 5:03 pm on August 24, 2020:

keep them coming! I’m going to ignore this one, I’ll reconsider it if there are more changes.

troygiorshev force-pushed on Aug 24, 2020

troygiorshev commented at 4:58 pm on August 24, 2020: contributor

git range-diff master 47132bb 68bb294

early return now returns earlier

MarcoFalke commented at 5:00 pm on August 24, 2020: member

I wanted to point out the slight behaviour change with regard to rbf’ed txs, but that doesn’t seem to open any issues.

Approach ACK

DrahtBot removed the label Needs rebase on Aug 24, 2020

instagibbs commented at 6:03 pm on August 24, 2020: member

anyone track this regression’s origin? concept ack

jnewbery commented at 7:17 pm on August 24, 2020: member

anyone track this regression’s origin? concept ack

https://github.com/bitcoin/bitcoin/commit/a9f3d3db5c0c8d1697998ed9b3e192ddbf9a31f4#diff-7ec3c68a81efff79b6ca22ac1f1eabbaL4424-L4428

jnewbery commented at 8:03 am on September 2, 2020: member

utACK 68bb2940d9589d6a7146916c8096c22aafaf047e

One request for better commenting. Not essential.

DrahtBot added the label Needs rebase on Sep 2, 2020

jnewbery force-pushed on Sep 4, 2020

jnewbery commented at 8:15 am on September 4, 2020: member

I’ve rebased and added the suggested comment here: #19753 (review)

Also removed the second commit, which wasn’t necessary as part of this fix.

DrahtBot removed the label Needs rebase on Sep 4, 2020

DrahtBot added the label Needs rebase on Sep 7, 2020

troygiorshev force-pushed on Sep 16, 2020

DrahtBot removed the label Needs rebase on Sep 16, 2020

troygiorshev force-pushed on Sep 16, 2020

troygiorshev commented at 12:58 pm on September 16, 2020: contributor

git range-diff master 9bcdd34 f996019

Trivial rebase

DrahtBot added the label Needs rebase on Oct 7, 2020

[net processing] Don't add AlreadyHave txs to recentRejects

Now, we only add a transaction to our recentRejects filter if we didn't
already have it, meaning that it is added at most once, as intended.

d419fdedbe

troygiorshev force-pushed on Oct 28, 2020

troygiorshev commented at 2:19 am on October 28, 2020: contributor

git range-diff master f996019 d419fde

Rebased

PR Status: Ready For Review

Should be quick for anyone interested 😄

DrahtBot removed the label Needs rebase on Oct 28, 2020

jnewbery commented at 8:51 am on October 28, 2020: member

Code review ACK d419fdedbe34c7ea19c0473660cc1b486b4e70d8

jnewbery added the label Bug on Oct 28, 2020

laanwj added this to the milestone 0.21.1 on Oct 28, 2020

laanwj removed this from the milestone 0.21.1 on Oct 28, 2020

laanwj added this to the milestone 0.21.0 on Oct 28, 2020

jonatack commented at 10:20 pm on October 28, 2020: member

Concept ACK

laanwj commented at 10:38 am on October 29, 2020: member

Code review ACK d419fdedbe34c7ea19c0473660cc1b486b4e70d8

laanwj merged this on Oct 29, 2020

laanwj closed this on Oct 29, 2020

sidhujag referenced this in commit a5d00a7604 on Oct 29, 2020

jonatack commented at 2:51 pm on October 31, 2020: member

Posthumous ACK d419fdedbe34c7ea19c0473660cc1b486b4e70d8

Fabcien referenced this in commit 1371b60fd0 on Dec 15, 2021

DrahtBot locked this on Feb 15, 2022

p2p: don’t add AlreadyHave transactions to recentRejects #19753

Conflicts