net_processing: ignore transactions while in IBD #21327

glozow · 2021-03-01T16:27:23Z

This is basically a mini, IBD-only version of #21224

Incoming transactions aren't really relevant until we're caught up. That's why we send a giant feefilter and don't send tx getdatas, but we also shouldn't process them if peers send them anyway. Simply ignore them.

DrahtBot · 2021-03-01T22:24:01Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#20277 (test: Extend p2p_ibd_tx_relay.py to verify no-transaction are requested during IBD by ariard)

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

While this probably has no effect in practice, it shouldn't hurt to do either.

src/net_processing.cpp

jnewbery

Concept ACK. Thanks for adding tests!

src/net_processing.cpp

test/functional/p2p_ibd_txrelay.py

ariard

Thanks for tackling this, I exercised new test coverage, works as expected!

W.r.t to Marco point, I don't think logging this minor rejection case is that important.

test/functional/p2p_ibd_txrelay.py

jnewbery · 2021-03-03T09:14:16Z

utACK d631448

src/net_processing.cpp

luke-jr

utACK

sdaftuar · 2021-03-09T23:49:51Z

Issues like those being explored in #21106 — where IBD may kick in at unintuitive times, complicating our understanding of the network due to special-case interactions throughout our codebase — are why I think we should hesitate before adding more checks gated on IBD and ensure we have a good reason for doing so.

In this case, I think our code is simpler to understand if we either always or never processed unrequested transactions (and the benefit to skipping this while in IBD seems small in comparison).

brunoerg

tACK d631448

Ubuntu 20.04

Ran the test individually and via test_runner (with all tests)
Code looks fine (comments/docs are very good - they explains each test step perfectly)

DrahtBot · 2021-03-15T16:49:26Z

🕵️ @sipa has been requested to review this pull request as specified in the REVIEWERS file.

glozow · 2021-03-24T22:05:05Z

Just pushed some changes addressing @ariard’s review comment and clarifying the comment about “not a protocol violation.”

On why add a gate for unsolicited transactions in IBD but not the general case:

There can perhaps be an observable difference if we stop processing unsolicited transactions in the general case - someone mentioned in #21224 that there are clients that wouldn’t be able to broadcast their transactions because they don’t follow the inv/getdata dialogue. I'm not sure what the status is on that discussion so I won't speak to it too much.

On the other hand, I don’t think there’s any downside to ignoring unsolicited transactions in IBD. We just have less wasted effort: Right now, if we can’t validate the transaction yet, we waste time putting it into orphan pool. If it conflicts with a block tx we see later on, we need to go through removing it from our mempool. If it’s valid and we can validate it, doing so while in IBD doesn’t benefit us - even if we come out of IBD really quickly (e.g. on a restart), we can download it again in a rebroadcast (or, later on, a reconciliation with Erlay). My personal opinion is that getting to exit early is worth the complexity of an extra IsIBD check, even if we later change it to ignore all unsolicited transactions.

jnewbery · 2021-06-09T12:32:09Z

Code review ACK 89ebd01

test/functional/p2p_ibd_txrelay.py

jnewbery · 2021-06-11T12:31:02Z

ACK 648c5c7

lsilva01

Tested ACK 648c5c7 on Ubuntu 20.04.
This simplifies the IBD logic a bit.

Co-authored-by: Amiti Uttarwar <amiti@uttarwar.org> Github-Pull: bitcoin#21327 Rebased-From: 8d5697c

Co-authored-by: Amiti Uttarwar <amiti@uttarwar.org> Github-Pull: bitcoin#21327 Rebased-From: 648c5c7

naumenkogs · 2021-09-29T10:28:27Z

The code 648c5c7 looks correct, but I share the hesitation suhas expressed above.

test/functional/p2p_ibd_txrelay.py

Co-authored-by: Amiti Uttarwar <amiti@uttarwar.org>

jnewbery · 2021-11-02T13:05:47Z

reACK 6aed8b7

laanwj · 2021-11-30T18:05:39Z

Code review ACK 6aed8b7
I've always shared Suhas' hesitation with regard to gating things on some (pretty arbitrary) "initial block download" criterion. But in this specific case, without an up-to-date view of blocks it's not that useful to process transactions, so I think it makes sense.

…e in IBD 3f4019a [test] tx processing before and after ibd (glozow) ef3eea0 [net_processing] ignore all transactions during ibd (glozow) Pull request description: This is basically a mini, IBD-only version of #21224 Incoming transactions aren't really relevant until we're caught up. That's why we send a giant feefilter and don't send tx getdatas, but we also shouldn't process them if peers send them anyway. Simply ignore them. ACKs for top commit: jnewbery: reACK 3f4019a laanwj: Code review ACK 3f4019a Tree-SHA512: 8e1616bf355f9d0b180bdbc5461f24c757dc5d7bc7bf651470f3b0bffcca5d5e68287106255b5cede2d96b42bce448a0f8c0649de35a530c5e079f7c89c70a35

DrahtBot added the P2P label Mar 1, 2021

DrahtBot mentioned this pull request Mar 2, 2021

test: Extend p2p_ibd_tx_relay.py to verify no-transaction are requested during IBD #20277

Closed

maflcko reviewed Mar 2, 2021

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

jnewbery reviewed Mar 2, 2021

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

test/functional/p2p_ibd_txrelay.py Outdated Show resolved Hide resolved

test/functional/p2p_ibd_txrelay.py Show resolved Hide resolved

glozow force-pushed the 2021-02-ibd-txrelay branch from 5b9036e to d631448 Compare March 2, 2021 19:17

ariard reviewed Mar 2, 2021

View reviewed changes

test/functional/p2p_ibd_txrelay.py Outdated Show resolved Hide resolved

amitiuttarwar mentioned this pull request Mar 4, 2021

net processing: Extract addr send functionality into MaybeSendAddr() #21236

Merged

sdaftuar reviewed Mar 4, 2021

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

luke-jr approved these changes Mar 9, 2021

View reviewed changes

brunoerg approved these changes Mar 13, 2021

View reviewed changes

glozow force-pushed the 2021-02-ibd-txrelay branch from d631448 to 89ebd01 Compare March 24, 2021 21:56

maflcko approved these changes Jun 11, 2021

View reviewed changes

test/functional/p2p_ibd_txrelay.py Outdated Show resolved Hide resolved

test/functional/p2p_ibd_txrelay.py Outdated Show resolved Hide resolved

glozow force-pushed the 2021-02-ibd-txrelay branch from 89ebd01 to 1665bbd Compare June 11, 2021 09:47

maflcko approved these changes Jun 11, 2021

View reviewed changes

test/functional/p2p_ibd_txrelay.py Outdated Show resolved Hide resolved

glozow force-pushed the 2021-02-ibd-txrelay branch from 1665bbd to 648c5c7 Compare June 11, 2021 10:23

lsilva01 approved these changes Jun 22, 2021

View reviewed changes

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 27, 2021

[net_processing] ignore all transactions during ibd

0822955

Co-authored-by: Amiti Uttarwar <amiti@uttarwar.org> Github-Pull: bitcoin#21327 Rebased-From: 8d5697c

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 27, 2021

[test] tx processing before and after ibd

15088af

Co-authored-by: Amiti Uttarwar <amiti@uttarwar.org> Github-Pull: bitcoin#21327 Rebased-From: 648c5c7

DrahtBot mentioned this pull request Sep 15, 2021

scripted-diff: update license URLs to https #22975

Closed

luke-jr suggested changes Oct 10, 2021

View reviewed changes

test/functional/p2p_ibd_txrelay.py Outdated Show resolved Hide resolved

glozow and others added 2 commits October 28, 2021 16:31

[net_processing] ignore all transactions during ibd

b9e105b

Co-authored-by: Amiti Uttarwar <amiti@uttarwar.org>

[test] tx processing before and after ibd

6aed8b7

Co-authored-by: Amiti Uttarwar <amiti@uttarwar.org>

glozow force-pushed the 2021-02-ibd-txrelay branch from 648c5c7 to 6aed8b7 Compare October 28, 2021 15:36

laanwj merged commit 63c0d0e into bitcoin:master Nov 30, 2021

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 1, 2021

Merge bitcoin#21327: net_processing: ignore transactions while in IBD

6b96aff

glozow deleted the 2021-02-ibd-txrelay branch January 27, 2022 15:11

bitcoin locked and limited conversation to collaborators Jan 27, 2023

net_processing: ignore transactions while in IBD #21327

net_processing: ignore transactions while in IBD #21327

Uh oh!

Conversation

glozow commented Mar 1, 2021

Uh oh!

DrahtBot commented Mar 1, 2021 • 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 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jnewbery commented Mar 3, 2021

Uh oh!

Uh oh!

luke-jr left a comment

Choose a reason for hiding this comment

Uh oh!

sdaftuar commented Mar 9, 2021

Uh oh!

brunoerg left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Mar 15, 2021

Uh oh!

glozow commented Mar 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery commented Jun 9, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jnewbery commented Jun 11, 2021

Uh oh!

lsilva01 left a comment

Choose a reason for hiding this comment

Uh oh!

naumenkogs commented Sep 29, 2021

Uh oh!

Uh oh!

jnewbery commented Nov 2, 2021

Uh oh!

laanwj commented Nov 30, 2021

Uh oh!

Uh oh!

DrahtBot commented Mar 1, 2021 •

edited

Loading

brunoerg left a comment •

edited

Loading

glozow commented Mar 24, 2021 •

edited

Loading