Backport bitcoin#14897 and bitcoin#15834 and modify it to work with Dash messages by codablock · Pull Request #3397 · dashpay/dash

codablock · 2020-04-07T05:22:26Z

The original intent of bitcoin#14897 was to fix a few attack scenarios. It also fixes a few performance issues we had in the past for which we implemented our own fixes. Our own fixes are not needed anymore, so this simply removes the old AskFor and RemoveAskFor code.

This PR also modifies the freshly backported code to work with Dash specific messages again.

The reason for this out-of-order backport is that our version of RemoveAskFor turned out to be extremely slow when hundreds of nodes were connected, as it required to loop through all nodes and call RemoveAskFor on them.

@gwillen

…e bias toward outbound 1cff3d6 Change in transaction pull scheduling to prevent InvBlock-related attacks (Gleb Naumenko) Pull request description: This code makes executing two particular (and potentially other) attacks harder. ### InvBlock This behavior was described well [here](https://www.cs.umd.edu/projects/coinscope/coinscope.pdf) (page 11). Per current implementation, if node A receives _INV_ (tx) from node B, node A sends _GETDATA_ to B and waits for _TX_ message back. Node A is likely to receive more _INVs_ (regarding the same tx) from other peers. But node A would not send another _GETDATA_ unless it does not hear _TX_ back from node B for next 2 minutes (to save bandwidth) Thus, if B is a malicious node, it can prevent node A from getting the transaction (even if all A’s peers have it) for 2 minutes. This behavior seems to be an inherent limitation of the current P2P relay protocol, and I don’t see how it can be fundamentally changed (I can see workarounds which involve rewriting a lot of P2P code though). ### What does this PR fix? The attacks I’m looking at involve preventing A from learning the transaction for 2*N minutes. To do that, an attacker has to spin up N nodes and send N _INVs_ simultaneously to node A (then InvBlocks will be queued with an interval of 2 minutes according to current implementation) More precisely, 2 scenarios I’m looking at are: 1. An attacker censors a particular transaction. By performing InvBlock from different nodes, an attacker can execute a network-wide censorship of a particular transaction (or all transactions). The earlier an attacker founds the transaction he wants to censor, the easier it is to perform an attack. As it was pointed out by @gwillen, this is even more dangerous in the case of lightning, where transactions are known in advance. 2. Topology inference described in papers [1](https://www.cs.umd.edu/projects/coinscope/coinscope.pdf), [2](https://arxiv.org/pdf/1812.00942.pdf) involve network-wide InvBlock. This fix would not mitigate this type of inference, but I believe it will make it more expensive to perform (an attacker would have to create more transactions and perform more rounds to learn the topology, the second paper itself notes that InvBlock isolation is important for the attack). ### How does it work This PR introduces bias toward outbound connections (they have higher priority when a node chooses from whom it should request a transaction) and randomizes the order. As per @gmaxwell suggestion, GETDATA requests queue is created after processing all incoming messages from all nodes. After this fix, if the incoming messages were [I1, I2, I3, O1, O2, O3, O4], the queue for _GETDATA_ may look like [O2, O1, O3, O4, I1, I3, I2, ….]. If {I1, I2, I3} were significantly earlier (but the difference is less than TX_TIMEOUT=60 s) than others, the queue for _GETDATA_ may look like [I2, O2, O1, O3, O4, I1, I3, ….]. ### Other comments: 1. This mitigation works better if the connectivity is higher (especially outbound, because it would be less likely that 2 _GETDATAs_ for inbound malicious nodes queued together) Tree-SHA512: 2ad1e80c3c7e16ff0f2d1160aa7d9a5eaae88baa88467f156b987fe2a387f767a41e11507d7f99ea02ab75e89ab93b6a278d138cb1054f1aaa2df336e9b2ca6a

…#14897 and expire transactions from peer in-flight map 308b767 Fix bug around transaction requests (Suhas Daftuar) f635a3b Expire old entries from the in-flight tx map (Suhas Daftuar) e32e084 Remove NOTFOUND transactions from in-flight data structures (Suhas Daftuar) 23163b7 Add an explicit memory bound to m_tx_process_time (Suhas Daftuar) 218697b Improve NOTFOUND comment (Suhas Daftuar) Pull request description: bitcoin#14897 introduced several bugs that could lead to a node no longer requesting transactions from one or more of its peers. Credit to ajtowns for originally reporting many of these bugs along with an originally proposed fix in bitcoin#15776. This PR does a few things: - Fix a bug in NOTFOUND processing, where the in-flight map for a peer was keeping transactions it shouldn't - Eliminate the possibility of a memory attack on the CNodeState `m_tx_process_time` data structure by explicitly bounding its size - Remove entries from a peer's in-flight map after 10 minutes, so that we should always eventually resume transaction requests even if there are other bugs like the NOTFOUND one - Fix a bug relating to the coordination of request times when multiple peers announce the same transaction The expiry mechanism added here is something we'll likely want to remove in the future, but is belt-and-suspenders for now to try to ensure we don't have other bugs that could lead to transaction relay failing due to some unforeseen conditions. ACKs for commit 308b76: ajtowns: utACK 308b767 morcos: light ACK 308b767 laanwj: Code review ACK 308b767 jonatack: Light ACK 308b767. jamesob: ACK 308b767 MarcoFalke: ACK 308b767 (Tested two of the three bugs this pull fixes, see comment above) jamesob: Concept ACK bitcoin@308b767 MarcoFalke: ACK 308b767 Tree-SHA512: 8865dca5294447859d95655e8699085643db60c22f0719e76e961651a1398251bc932494b68932e33f68d4f6084579ab3bed7d0e7dd4ac6c362590eaf9414eda

…tRequest is called Otherwise they'll run into false-positive timeouts.

fa01366 util: Add type safe GetTime (MarcoFalke) Pull request description: There are basically two ways to get the time in Bitcoin Core: * get the system time (via `GetSystemTimeInSeconds` or `GetTime{Millis,Micros}`) * get the mockable time (via `GetTime`) Both return the same type (a plain int). This can lead to (test-only) bugs such as 99464bc. Fix that by deprecating `GetTime` and adding a `GetTime<>` that returns the mockable time in a non-int type. The new util function is currently unused, but new code should it where possible. ACKs for commit fa0136: promag: utACK fa01366. Tree-SHA512: efab9c463f079fd8fd3030c479637c7b1e8be567a881234bd0f555c8f87e518e3b43ef2466128103db8fc40295aaf24e87ad76d91f338c631246fc703477e95c

# Conflicts: # src/net_processing.cpp # src/random.cpp # src/random.h

+ corresponding changes in comments

PastaPastaPasta · 2020-04-08T19:56:17Z

 static constexpr int32_t MAX_PEER_TX_ANNOUNCEMENTS = 2 * MAX_INV_SZ;
 /** How many microseconds to delay requesting transactions from inbound peers */
-static constexpr int64_t INBOUND_PEER_TX_DELAY = 2 * 1000000; // 2 seconds
+static constexpr std::chrono::microseconds INBOUND_PEER_TX_DELAY{std::chrono::seconds{2}};


is 8e5fbed part of a backport?

Yes, as discussed in #3397 (comment)

Otherwise the inv/getdata logic won't work with inbound connections due to the added delay of 2 seconds.

UdjinM6

utACK

Backport bitcoin#14897 and bitcoin#15834 and modify it to work with Dash messages

29d3b75 (dashpay#3397)

Cherrypick of 414943b dashpay#3397

Cherry-pick of ef14b19 dashpay#3397

…tRequest is called Cherry-pick of 26fcd3f dashpay#3397

sipa and others added 8 commits April 7, 2020 07:14

Remove MSG_WITNESS_TX

8c11a8e

Generalize TX request code

29d3b75

Fix governance.cpp

56da433

Make interval and timeout dependend on INV type

414943b

Replace uses of CConnman::RemoveAskFor with EraseObjectRequest

004d922

Replace uses of CNode::AskFor with RequestObject

2443c8f

codablock added this to the 16 milestone Apr 7, 2020

codablock force-pushed the pr_backport_txrequests branch from 154588e to e3de830 Compare April 7, 2020 08:32

Don't re-request erased object requests

ef14b19

codablock force-pushed the pr_backport_txrequests branch from e3de830 to ef14b19 Compare April 7, 2020 08:33

codablock added 4 commits April 7, 2020 13:23

Force re-requesting of IS locked TXs

4bfc20c

Also remove m_tx_announced and m_tx_in_flight entries when EraseObjec…

26fcd3f

…tRequest is called Otherwise they'll run into false-positive timeouts.

No inbound delay for non-TX objects and masternodes

8150359

More logging for object request handling

2d4cc8a

thephez reviewed Apr 7, 2020

View reviewed changes

Comment thread src/net_processing.cpp Outdated

codablock force-pushed the pr_backport_txrequests branch from fa8afc8 to 463e5ba Compare April 8, 2020 12:37

laanwj and others added 5 commits April 8, 2020 14:50

net: Use mockable time for tx download

8e5fbed

# Conflicts: # src/net_processing.cpp # src/random.cpp # src/random.h

Fix GetObjectInterval and EraseObjectRequest

a7b38ef

transactions -> objects

775e4ba

+ corresponding changes in comments

Fix compilation

6b32192

codablock force-pushed the pr_backport_txrequests branch from 463e5ba to 6b32192 Compare April 8, 2020 12:55

codablock added the P2P Some notable changes on p2p level label Apr 8, 2020

Fix compilation of util_time.cpp

6d93b33

codablock force-pushed the pr_backport_txrequests branch from ddcac60 to 6d93b33 Compare April 8, 2020 16:01

PastaPastaPasta reviewed Apr 8, 2020

View reviewed changes

Add wait_func to sync_mempool

24b25e1

codablock force-pushed the pr_backport_txrequests branch from 9199df3 to 548fe82 Compare April 8, 2020 20:27

Bump mocktime while syncing mempools

5cf417b

Otherwise the inv/getdata logic won't work with inbound connections due to the added delay of 2 seconds.

codablock force-pushed the pr_backport_txrequests branch from 548fe82 to 5cf417b Compare April 8, 2020 20:28

UdjinM6 approved these changes Apr 8, 2020

View reviewed changes

codablock merged commit 26dec64 into dashpay:develop Apr 8, 2020

codablock deleted the pr_backport_txrequests branch April 8, 2020 22:12

Liquid369 added a commit to Liquid369/PIVX that referenced this pull request Jan 15, 2025

Merge pull request dashpay#3397 from codablock/pr_backport_txrequests

217f977

Backport bitcoin#14897 and bitcoin#15834 and modify it to work with Dash messages

Duddino pushed a commit to Duddino/PIVX that referenced this pull request Feb 4, 2025

Cherrypick: Generalize TX request code

63b2f62

29d3b75 (dashpay#3397)

Duddino pushed a commit to Duddino/PIVX that referenced this pull request Feb 4, 2025

Make interval and timeout dependend on INV type

acf2c47

Cherrypick of 414943b dashpay#3397

Duddino mentioned this pull request Feb 4, 2025

Backport bitcoin#14897 and bitcoin#15834 and modify it to work with PIVX messages PIVX-Project/PIVX#2963

Open

Duddino pushed a commit to Duddino/PIVX that referenced this pull request Feb 13, 2025

Don't re-request erased object requests

e317219

Cherry-pick of ef14b19 dashpay#3397

Duddino pushed a commit to Duddino/PIVX that referenced this pull request Feb 13, 2025

Also remove m_tx_announced and m_tx_in_flight entries when EraseObjec…

9ad3b6b

…tRequest is called Cherry-pick of 26fcd3f dashpay#3397

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Backport bitcoin#14897 and bitcoin#15834 and modify it to work with Dash messages#3397

Backport bitcoin#14897 and bitcoin#15834 and modify it to work with Dash messages#3397
codablock merged 21 commits intodashpay:developfrom
codablock:pr_backport_txrequests

codablock commented Apr 7, 2020 •

edited

Loading

Uh oh!

Uh oh!

PastaPastaPasta Apr 8, 2020

Uh oh!

codablock Apr 8, 2020

Uh oh!

UdjinM6 left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

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

Uh oh!

Uh oh!

Uh oh!

PastaPastaPasta Apr 8, 2020

Choose a reason for hiding this comment

Uh oh!

codablock Apr 8, 2020

Choose a reason for hiding this comment

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codablock commented Apr 7, 2020 •

edited

Loading