Page MenuHomePhabricator

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

Authored by majcosta on Feb 3 2021, 23:35.

Details

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

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable