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

troygiorshev · 2020-08-17T19:01:08Z

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.

benthecarman · 2020-08-18T00:07:46Z

Concept ACK

Looks like the p2p_permissions.py test is failing

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

amitiuttarwar · 2020-08-18T00:35:10Z

strong concept ACK! nice find :)

jnewbery · 2020-08-18T14:07:57Z

Concept ACK

troygiorshev · 2020-08-18T15:21:12Z

Fixed the broken test

jnewbery

utACK. One suggested change to the test.

jnewbery · 2020-08-19T09:00:13Z

test/functional/p2p_permissions.py

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


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:

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

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.

thanks, fixed

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

DrahtBot · 2020-08-20T20:06:39Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

tree-wide: De-globalize ChainstateManager #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.

maflcko

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.

src/net_processing.cpp

jnewbery · 2020-08-24T15:40:02Z

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 · 2020-08-24T15:46:30Z

Concept ACK

Looks like the p2p_permissions.py test is failing

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

troygiorshev · 2020-08-24T16:21:18Z

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.

troygiorshev · 2020-08-24T16:58:30Z

git range-diff master 47132bb 68bb294

early return now returns earlier

maflcko

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

test/functional/p2p_permissions.py

instagibbs · 2020-08-24T18:03:51Z

anyone track this regression's origin? concept ack

jnewbery · 2020-08-24T19:17:14Z

anyone track this regression's origin? concept ack

a9f3d3d#diff-7ec3c68a81efff79b6ca22ac1f1eabbaL4424-L4428

jnewbery

utACK 68bb294

One request for better commenting. Not essential.

jnewbery · 2020-09-02T08:00:57Z

test/functional/p2p_permissions.py

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


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

troygiorshev · 2020-09-16T12:58:03Z

git range-diff master 9bcdd34 f996019

Trivial rebase

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.

troygiorshev · 2020-10-28T02:19:16Z

git range-diff master f996019 d419fde

Rebased

PR Status: Ready For Review

Should be quick for anyone interested 😄

jnewbery · 2020-10-28T08:51:23Z

Code review ACK d419fde

jonatack · 2020-10-28T22:20:16Z

Concept ACK

laanwj · 2020-10-29T10:38:52Z

Code review ACK d419fde

…tRejects d419fde [net processing] Don't add AlreadyHave txs to recentRejects (Troy Giorshev) Pull request description: 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. ACKs for top commit: jnewbery: Code review ACK d419fde laanwj: Code review ACK d419fde Tree-SHA512: cff5c1ba36c4700e2d6ab3eec4a3e51e1bef28fb3cc1bc850c84e06d6e5a9f6c32825207c253cc9cdf596b2eaadb6b5be68b3f8ca752b4ef6c31cf85138e3c99

jonatack

Posthumous ACK d419fde

Summary: 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. This is a backport of [[bitcoin/bitcoin#19753 | core#19753]] Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10681

DrahtBot added the P2P label Aug 17, 2020

troygiorshev force-pushed the 2020-08-clean-tx-processing branch from 8d90d0f to 9325a06 Compare August 18, 2020 15:19

jnewbery changed the title ~~p2p: fix recentRejects filling bug~~ p2p: don't add AlreadyHave transactions to recentRejects Aug 19, 2020

jnewbery force-pushed the 2020-08-clean-tx-processing branch from 9325a06 to 95134c8 Compare August 19, 2020 09:06

jnewbery reviewed Aug 19, 2020

View reviewed changes

maflcko reviewed Aug 23, 2020

View reviewed changes

src/net_processing.cpp Show resolved Hide resolved

DrahtBot added the Needs rebase label Aug 24, 2020

troygiorshev force-pushed the 2020-08-clean-tx-processing branch from 95134c8 to 47132bb Compare August 24, 2020 16:19

troygiorshev force-pushed the 2020-08-clean-tx-processing branch from 47132bb to 68bb294 Compare August 24, 2020 16:57

maflcko reviewed Aug 24, 2020

View reviewed changes

test/functional/p2p_permissions.py Show resolved Hide resolved

DrahtBot removed the Needs rebase label Aug 24, 2020

DrahtBot mentioned this pull request Aug 27, 2020

Erlay: bandwidth-efficient transaction relay protocol #18261

Closed

jnewbery reviewed Sep 2, 2020

View reviewed changes

DrahtBot added the Needs rebase label Sep 2, 2020

jnewbery force-pushed the 2020-08-clean-tx-processing branch from 68bb294 to 39b2418 Compare September 4, 2020 08:09

DrahtBot removed the Needs rebase label Sep 4, 2020

This was referenced Sep 4, 2020

Avoid locking CTxMemPool::cs recursively in some cases #19872

Closed

Remove mempool global #19556

Merged

DrahtBot added the Needs rebase label Sep 7, 2020

troygiorshev force-pushed the 2020-08-clean-tx-processing branch 2 times, most recently from d7aa195 to 77ebed1 Compare September 16, 2020 12:46

DrahtBot removed the Needs rebase label Sep 16, 2020

troygiorshev force-pushed the 2020-08-clean-tx-processing branch 2 times, most recently from 9bcdd34 to f996019 Compare September 16, 2020 12:52

This was referenced Sep 22, 2020

Overhaul transaction request logic #19988

Merged

validation/util: add GetTransactionFee #20025

Closed

DrahtBot added the Needs rebase label Oct 7, 2020

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

d419fde

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.

troygiorshev force-pushed the 2020-08-clean-tx-processing branch from f996019 to d419fde Compare October 28, 2020 02:16

DrahtBot removed the Needs rebase label Oct 28, 2020

jnewbery added the Bug label Oct 28, 2020

laanwj modified the milestones: 0.21.1, 0.21.0 Oct 28, 2020

DrahtBot mentioned this pull request Oct 28, 2020

tree-wide: De-globalize ChainstateManager #20158

Closed

12 tasks

laanwj merged commit 6196cf7 into bitcoin:master Oct 29, 2020

jonatack reviewed Oct 31, 2020

View reviewed changes

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

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

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

Uh oh!

Conversation

troygiorshev commented Aug 17, 2020 • edited by jnewbery Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benthecarman commented Aug 18, 2020

Uh oh!

amitiuttarwar commented Aug 18, 2020

Uh oh!

jnewbery commented Aug 18, 2020

Uh oh!

troygiorshev commented Aug 18, 2020

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery Aug 19, 2020

Choose a reason for hiding this comment

Uh oh!

troygiorshev Aug 24, 2020

Choose a reason for hiding this comment

Uh oh!

jnewbery Sep 2, 2020

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Aug 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jnewbery commented Aug 24, 2020

Uh oh!

guardian939 commented Aug 24, 2020

Uh oh!

troygiorshev commented Aug 24, 2020

Uh oh!

troygiorshev commented Aug 24, 2020

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

instagibbs commented Aug 24, 2020

Uh oh!

jnewbery commented Aug 24, 2020

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery Sep 2, 2020

Choose a reason for hiding this comment

Uh oh!

troygiorshev commented Sep 16, 2020

Uh oh!

troygiorshev commented Oct 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery commented Oct 28, 2020

Uh oh!

jonatack commented Oct 28, 2020

Uh oh!

laanwj commented Oct 29, 2020

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

troygiorshev commented Aug 17, 2020 •

edited by jnewbery

Loading

DrahtBot commented Aug 20, 2020 •

edited

Loading

troygiorshev commented Oct 28, 2020 •

edited

Loading