[net processing] Drop unknown types in getdata #18808

jnewbery · 2020-04-29T00:40:38Z

Currently we'll stall peers that send us an unknown INV type in a GETDATA message. Be a bit more friendly and just drop the invalid request.

Ditto for blocks-relay-only peers that send us a GETDATA for a transaction.

There's a test for the first part. The second is difficult to test in the functional test framework since we aren't able to make blocks-relay-only connections.

DrahtBot · 2020-04-29T02:40:50Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

p2p: Stop relaying non-mempool txs, improve tx privacy slightly #17303 (p2p: Stop relaying non-mempool txs, improve tx privacy slightly by MarcoFalke)

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.

src/net_processing.cpp

maflcko · 2020-04-29T13:20:34Z

Concept ACK

Co-Authored-By: John Newbery <[email protected]>

naumenkogs · 2020-04-29T16:33:40Z

Concept ACK, light code review ack

naumenkogs · 2020-04-29T16:38:13Z

src/net_processing.cpp

What's the worst local processing delay we may expect if 2 blocks are generated within 5 seconds (and got queued together), but we process them separately?
The probability of this is definitely non-zero.

Usually milliseconds. However long it takes to do one round of the messagehandler thread. We won't pause processing for 100ms because there's more work to do for this peer.

mzumsande

Concept ACK.

I think there was discussion in earlier PRs on whether to ignore the unknown data or disconnect the peer. Why did you choose the former / are there legitimate constellations in which peers would send us unknown GETDATA but be worth staying connected to?

mzumsande · 2020-04-29T22:12:20Z

test/functional/p2p_getdata.py

What's the purpose of this override? (No invs seem to be sent in the test)

no reason. Bad copy-paste. Fixed. Thanks!

jnewbery · 2020-04-29T23:31:45Z

Why did you choose the former / are there legitimate constellations in which peers would send us unknown GETDATA but be worth staying connected to?

Just conservatism. Better to not let a logic bug turn into a peer disconnect if possible. In the adversarial case, there are messages that a peer can send us that would be much costlier for us to process, so no need to punish for something that we can just drop.

…ure messages Co-Authored-By: John Newbery <[email protected]>

laanwj · 2020-04-30T08:34:08Z

Concept ACK, though I think disconnecting the peer because the application doesn't know how to handle a request would be slightly preferable in this case.
It indicates a bug at the side of the peer and explicit error handling is better to track down errors.
I could understand the ignore behavior as a kind of extension mechanism, however, I think in case of a protocol extension it needs to be negotiated in some way first, not simply assumed to be present.

brakmic · 2020-04-30T16:16:00Z

ACK 9847e20

Built, run and tested on macOS Catalina 10.15.4.
Functional test p2p_getdata successful.

jnewbery · 2020-04-30T16:58:18Z

I think disconnecting the peer because the application doesn't know how to handle a request would be slightly preferable in this case.
It indicates a bug at the side of the peer and explicit error handling is better to track down errors.

There are lots of p2p messages that indicate bugs in the peer which we just silently drop, eg if a peer sends us multiple VERACKs, multiple SENDHEADERs, a GETDATA with no items, etc. I think if the cost to process those messages is low, then dropping and not disconnecting is the safer thing to do.

luke-jr · 2020-05-02T22:06:21Z

Why did you choose the former / are there legitimate constellations in which peers would send us unknown GETDATA but be worth staying connected to?

It's generally important to avoid splitting the p2p network. Even if peers are implementing different rules allowing invalid stuff (eg, an old node after a softfork), they still need to get the blocks from updated peers...

luke-jr

utACK 9847e20

laanwj · 2020-05-06T12:23:07Z

if a peer sends us multiple VERACKs, multiple SENDHEADERs, a GETDATA with no items, etc. I think if the cost to process those messages is low, then dropping and not disconnecting is the safer thing to do.

I think those are different in that they are quantitatively different not qualitatively. There may be spurious messages (or lack of messages) but the messages that are sent have a known meaning. In this case the type is unknown and it is completely unknown how to handle it.

jnewbery · 2020-05-06T13:50:08Z

In this case the type is unknown and it is completely unknown how to handle it.

Yes, and so the most cautious action is to drop it. We already drop unknown message types:

bitcoin/src/net_processing.cpp

Lines 3299 to 3301 in 60091d2

    
           // Ignore unknown commands for extensibility 
        
           LogPrint(BCLog::NET, "Unknown command \"%s\" from peer=%d\n", SanitizeString(msg_type), pfrom->GetId()); 
        
           return true;

I could use the same extensibility argument here. It's possible that in future another P2P client or a future version of Bitcoin Core might use the inv/getdata/notfound messages for different payload types. We shouldn't disconnect them unnecessarily.

sipa · 2020-05-08T02:22:20Z

utACK 9847e20

naumenkogs · 2020-05-08T14:50:48Z

utACK 9847e20

ajtowns

utACK 9847e20

ajtowns · 2020-05-11T15:47:20Z

src/net_processing.cpp

+
+            if (pfrom->m_tx_relay == nullptr) {
+                // Ignore GETDATA requests for transactions from blocks-only peers.
+                continue;


Seems mildly wasteful to grab the cs_main lock just to skip over MSG_TX and MSG_WITNESS_TX. I think this patch ("ignore tx GETDATA from blocks only peers") could be dropped entirely, allowing the "ignore unknown INV types in GETDATA messages" patch's handling to cover it instead.

Yeah, it's slightly wasteful, but I think it's clearer to have tx and non-tx handling fully separated in different parts of the functions, and explicitly have a code path/comment documenting behaviour, rather than having to think "so what happens if we receive a request for a tx from a blocks-relay-only peer? Oh, it falls through here and then we handle it below as if it were an unknown type".

9847e20 [docs] Improve commenting in ProcessGetData() (John Newbery) 2f03255 [test] test that an invalid GETDATA doesn't prevent processing of future messages (Amiti Uttarwar) e257cf7 [net processing] ignore unknown INV types in GETDATA messages (Amiti Uttarwar) 047ceac [net processing] ignore tx GETDATA from blocks-only peers (Amiti Uttarwar) Pull request description: Currently we'll stall peers that send us an unknown INV type in a GETDATA message. Be a bit more friendly and just drop the invalid request. Ditto for blocks-relay-only peers that send us a GETDATA for a transaction. There's a test for the first part. The second is difficult to test in the functional test framework since we aren't able to make blocks-relay-only connections. ACKs for top commit: sipa: utACK 9847e20 brakmic: ACK bitcoin@9847e20 luke-jr: utACK 9847e20 naumenkogs: utACK 9847e20 ajtowns: utACK 9847e20 Tree-SHA512: 6007f2fd839ffe737727f6fb8e8f083b2d9e05a510748f1d40b8f9be8fdf7b5419a36d8f1039923eec1ba2983e8f6f0436ec5fc196d9f6dcb0657f2ff8ff8e4c

Co-Authored-By: John Newbery <[email protected]> Github-Pull: bitcoin#18808 Rebased-From: 047ceac

Co-Authored-By: John Newbery <[email protected]> Github-Pull: bitcoin#18808 Rebased-From: e257cf7

…ure messages Co-Authored-By: John Newbery <[email protected]> Github-Pull: bitcoin#18808 Rebased-From: 2f03255

Github-Pull: bitcoin#18808 Rebased-From: 9847e20

245c862 test: disable script fuzz tests (fanquake) 9a8fb4c fuzz: Remove enumeration of expected deserialization exceptions in ProcessMessage(...) fuzzer (practicalswift) 6161c94 [net processing] Only send a getheaders for one block in an INV (John Newbery) cf2a6e2 test: Remove const to work around compiler error on xenial (Wladimir J. van der Laan) cc7d344 miner: Avoid stack-use-after-return in validationinterface (MarcoFalke) 37a6207 test: Add unregister_validation_interface_race test (MarcoFalke) ff4dc20 gui: Fix manual coin control with multiple wallets loaded (João Barbosa) ed0afe8 test: Add test for conflicted wallet tx notifications (Russell Yanofsky) 251e321 rpc: Relock wallet only if most recent callback (João Barbosa) ca4dac4 rpc: Add mutex to guard deadlineTimers (João Barbosa) a3fe458 [docs] Improve commenting in ProcessGetData() (John Newbery) 011532e [test] test that an invalid GETDATA doesn't prevent processing of future messages (Amiti Uttarwar) 1e73d72 [net processing] ignore unknown INV types in GETDATA messages (Amiti Uttarwar) fb82173 [net processing] ignore tx GETDATA from blocks-only peers (Amiti Uttarwar) 315ae14 gui: Fix itemWalletAddress leak when not tree mode (João Barbosa) Pull request description: Backports the following PRs to the 0.20 branch: * #18578: gui: Fix leak in CoinControlDialog::updateView * #18808: [net processing] Drop unknown types in getdata * #18814: rpc: Relock wallet only if most recent callback * #18878: test: Add test for conflicted wallet tx notifications * #18894: gui: Fix manual coin control with multiple wallets loaded * #18742: miner: Avoid stack-use-after-return in validationinterface * #18962: net processing: Only send a getheaders for one block in an INV * #18975: test: Remove const to work around compiler error on xenial ACKs for top commit: promag: Tested ACK 245c862 coin control with multiple wallets. laanwj: ACK 245c862 MarcoFalke: ACK 245c862 solved the conflicts myself as a sanity check. Did not re-review 🍷 Tree-SHA512: 285e5a5fad5bbeba6032742c65dc68836e8eccfcceda9e69fec4ddd162a3f61679a96f9bbe3d434267835af67c21ac4c05accf6f63e827c2eb47203c6daafe31

maflcko · 2020-05-23T14:11:46Z

Post merge ACK 9847e20

Summary: Co-Authored-By: John Newbery <[email protected]> > Currently we'll stall peers that send us an unknown INV type in a GETDATA message. Be a bit more friendly and just drop the invalid request. > > Ditto for blocks-relay-only peers that send us a GETDATA for a transaction. > > There's a test for the first part. The second is difficult to test in the functional test framework since we aren't able to make blocks-relay-only connections. This is a backport of Core [[bitcoin/bitcoin#18808 | PR18808]] [1/4] bitcoin/bitcoin@047ceac Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9083

Summary: Co-Authored-By: John Newbery <[email protected]> This is a backport of Core [[bitcoin/bitcoin#18808 | PR18808]] [2/4] bitcoin/bitcoin@e257cf7 Depends on D9084 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9084

…ure messages Summary: Co-Authored-By: John Newbery <[email protected]> This is a backport of Core [[bitcoin/bitcoin#18808 | PR18808]] [3/4] bitcoin/bitcoin@2f03255 Depends on D9084 Test Plan: `ninja && test/functional/test_runner.py p2p_getdata` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9085

Summary: This concludes backport of Core [[bitcoin/bitcoin#18808 | PR18808]] [4/4] bitcoin/bitcoin@9847e20 Depends on D9085 Test Plan: `ninja` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9086

Co-Authored-By: John Newbery <[email protected]> Github-Pull: #18808 Rebased-From: 047ceac

Co-Authored-By: John Newbery <[email protected]> Github-Pull: #18808 Rebased-From: e257cf7

…ure messages Co-Authored-By: John Newbery <[email protected]> Github-Pull: #18808 Rebased-From: 2f03255

Github-Pull: #18808 Rebased-From: 9847e20

The bug was introduced in v19 via 5118

fanquake added the P2P label Apr 29, 2020

This was referenced Apr 29, 2020

test: Add test for wtxid transaction relay #18446

Closed

Erlay: bandwidth-efficient transaction relay protocol #18261

Closed

Use wtxid for transaction relay #18044

Merged

p2p: Stop relaying non-mempool txs, improve tx privacy slightly #17303

Closed

maflcko reviewed Apr 29, 2020

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

amitiuttarwar and others added 2 commits April 29, 2020 10:54

[net processing] ignore tx GETDATA from blocks-only peers

047ceac

Co-Authored-By: John Newbery <[email protected]>

[net processing] ignore unknown INV types in GETDATA messages

e257cf7

Co-Authored-By: John Newbery <[email protected]>

jnewbery force-pushed the 2020-04-getdata branch from c71754b to 4ecd558 Compare April 29, 2020 14:57

naumenkogs reviewed Apr 29, 2020

View reviewed changes

mzumsande reviewed Apr 29, 2020

View reviewed changes

amitiuttarwar and others added 2 commits April 29, 2020 19:34

[test] test that an invalid GETDATA doesn't prevent processing of fut…

2f03255

…ure messages Co-Authored-By: John Newbery <[email protected]>

[docs] Improve commenting in ProcessGetData()

9847e20

jnewbery force-pushed the 2020-04-getdata branch from 4ecd558 to 9847e20 Compare April 29, 2020 23:34

luke-jr approved these changes May 6, 2020

View reviewed changes

ajtowns reviewed May 11, 2020

View reviewed changes

fanquake merged commit 7a57674 into bitcoin:master May 12, 2020

laanwj added the Needs backport (0.20) label May 13, 2020

fanquake removed the Needs backport (0.20) label May 14, 2020

fanquake mentioned this pull request May 14, 2020

[0.20] Final backports for rc2 #18973

Merged

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request May 15, 2020

[net processing] ignore tx GETDATA from blocks-only peers

fb82173

Co-Authored-By: John Newbery <[email protected]> Github-Pull: bitcoin#18808 Rebased-From: 047ceac

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request May 15, 2020

[net processing] ignore unknown INV types in GETDATA messages

1e73d72

Co-Authored-By: John Newbery <[email protected]> Github-Pull: bitcoin#18808 Rebased-From: e257cf7

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request May 15, 2020

[test] test that an invalid GETDATA doesn't prevent processing of fut…

011532e

…ure messages Co-Authored-By: John Newbery <[email protected]> Github-Pull: bitcoin#18808 Rebased-From: 2f03255

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request May 15, 2020

[docs] Improve commenting in ProcessGetData()

a3fe458

Github-Pull: bitcoin#18808 Rebased-From: 9847e20

maflcko mentioned this pull request May 18, 2020

tests: Add fuzzing harnesses for functions in script/ #18994

Merged

jnewbery deleted the 2020-04-getdata branch May 23, 2020 15:13

backpacker69 referenced this pull request in peercoin/peercoin Mar 28, 2021

[net processing] ignore tx GETDATA from blocks-only peers

8a45c78

Co-Authored-By: John Newbery <[email protected]> Github-Pull: #18808 Rebased-From: 047ceac

backpacker69 referenced this pull request in peercoin/peercoin Mar 28, 2021

[net processing] ignore unknown INV types in GETDATA messages

16fee04

Co-Authored-By: John Newbery <[email protected]> Github-Pull: #18808 Rebased-From: e257cf7

backpacker69 referenced this pull request in peercoin/peercoin Mar 28, 2021

[test] test that an invalid GETDATA doesn't prevent processing of fut…

37bd744

…ure messages Co-Authored-By: John Newbery <[email protected]> Github-Pull: #18808 Rebased-From: 2f03255

backpacker69 referenced this pull request in peercoin/peercoin Mar 28, 2021

[docs] Improve commenting in ProcessGetData()

b3bdfea

Github-Pull: #18808 Rebased-From: 9847e20

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

PastaPastaPasta pushed a commit to knst/dash that referenced this pull request Jan 16, 2024

fix: bitcoin#18808 follow-up

8465730

The bug was introduced in v19 via 5118

[net processing] Drop unknown types in getdata #18808

[net processing] Drop unknown types in getdata #18808

Uh oh!

Conversation

jnewbery commented Apr 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

Uh oh!

maflcko commented Apr 29, 2020

Uh oh!

naumenkogs commented Apr 29, 2020

Uh oh!

naumenkogs Apr 29, 2020 • 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!

jnewbery Apr 29, 2020

Choose a reason for hiding this comment

Uh oh!

mzumsande left a comment

Choose a reason for hiding this comment

Uh oh!

mzumsande Apr 29, 2020

Choose a reason for hiding this comment

Uh oh!

jnewbery Apr 29, 2020

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Apr 29, 2020

Uh oh!

laanwj commented Apr 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brakmic commented Apr 30, 2020

Uh oh!

jnewbery commented Apr 30, 2020

Uh oh!

luke-jr commented May 2, 2020

Uh oh!

luke-jr left a comment

Choose a reason for hiding this comment

Uh oh!

laanwj commented May 6, 2020

Uh oh!

jnewbery commented May 6, 2020

Uh oh!

sipa commented May 8, 2020

Uh oh!

naumenkogs commented May 8, 2020

Uh oh!

ajtowns left a comment

Choose a reason for hiding this comment

Uh oh!

ajtowns May 11, 2020

Choose a reason for hiding this comment

Uh oh!

jnewbery May 11, 2020

Choose a reason for hiding this comment

Uh oh!

maflcko commented May 23, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

jnewbery commented Apr 29, 2020 •

edited

Loading

DrahtBot commented Apr 29, 2020 •

edited

Loading

naumenkogs Apr 29, 2020 •

edited

Loading

laanwj commented Apr 30, 2020 •

edited

Loading