HomePhabricator

[backport#18861] Do not answer GETDATA for to-be-announced tx

Description

[backport#18861] Do not answer GETDATA for to-be-announced tx

Summary:
2896c412fadbc03916a33028f4f50fd87ac48edb Do not answer GETDATA for to-be-announced tx (Pieter Wuille)
f2f32a3dee9a965c8198f9ddd3aaebc627c273e4 Push down use of cs_main into FindTxForGetData (Pieter Wuille)
c6131bf407c1ada78a0e5509a702bc7da0bfd57d Abstract logic to determine whether to answer tx GETDATA (Pieter Wuille)

Pull request description:

This PR intends to improve transaction-origin privacy.

In general, we should try to not leak information about what transactions we have (recently) learned about before deciding to announce them to our peers. There is a controlled transaction dissemination process that reveals our transactions to peers that has various safeguards for privacy (it's rate-limited, delayed & batched, deterministically sorted, ...), and ideally there is no way to test which transactions we have before that controlled process reveals them. The handling of the `mempool` BIP35 message has protections in this regard as well, as it would be an obvious way to bypass these protections (handled asynchronously after a delay, also deterministically sorted).

However, currently, if we receive a GETDATA for a transaction that we have not yet announced to the requester, we will still respond to it if it was announced to *some* other peer already (because it needs to be in `mapRelay`, which only happens on the first announcement). This is a slight privacy leak.

Thankfully, this seems easy to solve: `setInventontoryTxToSend` keeps track of the txids we have yet to announce to a peer - which almost(*) exactly corresponds to the transactions we know of that we haven't revealed to that peer. By checking whether a txid is in that set before responding to a GETDATA, we can filter these out.

(*) Locally resubmitted or rebroadcasted transactions may end up in setInventoryTxToSend while the peer already knows we have them, which could result in us incorrectly claiming we don't have such transactions if coincidentally requested right after we schedule reannouncing them, but before they're actually INVed. This is made even harder by the fact that filterInventoryKnown will generally keep known reannouncements out of setInventoryTxToSend unless it overflows (which needs 50000 INVs in either direction before it happens).

The condition for responding now becomes:

```
  (not in setInventoryTxToSend) AND
  (
    (in relay map) OR
    (
      (in mempool) AND
      (old enough that it could have expired from relay map) AND
      (older than our last getmempool response)
    )
  )
```

Backport of Core PR18861

notes:

  • uses references to CNode rather than pointers as in the original PR, see D8480
  • changed the CNode* parameter in FindTxForGet Data to const CNode& because that's what it is

Test Plan:

CC=clang CXX=clang++ cmake -GNinja -DENABLE_SANITIZERS=thread
ninja check-all

cmake -GNinja -DCMAKE_BUILD_TYPE=Debug
ninja check-all

Reviewers: #bitcoin_abc, PiRK

Reviewed By: #bitcoin_abc, PiRK

Differential Revision: https://reviews.bitcoinabc.org/D9162

Details

Provenance
fanquake <fanquake@gmail.com>Authored on May 19 2020, 06:50
majcostaCommitted on Feb 4 2021, 12:45
majcostaPushed on Feb 4 2021, 12:45
Reviewer
Restricted Project
Differential Revision
D9162: [backport#18861] Do not answer GETDATA for to-be-announced tx
Parents
rABCa7bb5a014c3b: [backport#16946] wallet: include a checksum of encrypted private keys
Branches
Unknown
Tags
Unknown