Commit graph

381 commits

Author SHA1 Message Date
MarcoFalke
fa9604c46f
doc: noban precludes maxuploadtarget disconnects 2020-06-04 16:39:23 -04:00
MarcoFalke
fa3999fe35
net: Reformat excessively long if condition into multiple lines
Can be reviewed with the git option
--word-diff-regex=.
2020-06-04 16:39:17 -04:00
Sebastian Falbesoner
8b3136bd30 refactor: replace CNode pointers by references within net_processing.{h,cpp} 2020-06-02 01:42:55 +02:00
MarcoFalke
07d0e0d59f
Merge #19044: net processing: Add support for getcfilters
9e36067d8c [test] Add test for cfilters. (Jim Posen)
11106a4722 [net processing] Message handling for getcfilters. (Jim Posen)
e535670726 [indexes] Fix default [de]serialization of BlockFilter. (Jim Posen)
bb911ae7f5 [refactor] Pass CNode and CConnman by reference (John Newbery)

Pull request description:

  Support `getcfilters` requests when `-peerblockfilters` is set.

  Does not advertise compact filter support in version messages.

ACKs for top commit:
  Empact:
    re-Code Review ACK 9e36067d8c
  MarcoFalke:
    re-ACK 9e36067d8c , only change is adding commit "[refactor] Pass CNode and CConnman by reference" 🥑
  jkczyz:
    ACK 9e36067d8c
  fjahr:
    Code review ACK 9e36067d8c

Tree-SHA512: b45b42a25905ef0bd9e195029185300c86856c87f78cbe17921f4a25e159ae0f6f003e61714fa43779017eb97cd89d3568419be88e47d19dc8095562939e7887
2020-05-31 18:20:17 -04:00
MarcoFalke
826fe9c667
Merge #18807: [doc / test / mempool] unbroadcast follow-ups
9e1cb1adf1 [trivial/doc] Fix comment type (Amiti Uttarwar)
8f30260a67 [doc] Update unbroadcast description in RPC results (Amiti Uttarwar)
750456d6f2 [trivial] Remove misleading 'const' (Amiti Uttarwar)
fa32e676e5 [test] Manage node connections better in mempool persist test (Amiti Uttarwar)
1f94bb0c74 [doc] Provide rationale for randomization in scheduling. (Amiti Uttarwar)
9c8a55d9cb [mempool] Don't throw expected error message when upgrading (Amiti Uttarwar)
ba54983182 [test] Test that wallet transactions aren't rebroadcast before 12 hours (Amiti Uttarwar)
00d44a534b [test] P2P connection behavior should meet expectations (Amiti Uttarwar)
bd093ca15d [test] updates to unbroadcast test (Amiti Uttarwar)
dab298d9ab [docs] add release notes (Amiti Uttarwar)

Pull request description:

  This PR is a follow up to #18038 which introduced the idea of an unbroadcast set & focuses mostly on documentation updates and test fixes. One small functionality update to not throw an expected error in `LoadMempool` when you upgrade software versions.

  #18895 is another follow up to that addresses other functionality updates.

  Background context:
  The unbroadcast set is a mechanism for the mempool to track locally submitted transactions (via wallet or RPC). The node does a best-effort of delivering the transactions to the network via retries every 10-15 minutes until either a `GETDATA` is received or the transaction is removed from the mempool.

ACKs for top commit:
  MarcoFalke:
    ACK 9e1cb1adf1 👁
  gzhao408:
    ACK [`9e1cb1a`](9e1cb1adf1)

Tree-SHA512: 0cd51c4ca368b9dce92d50d73ec6e9df278a259e609eef2858f24cb8595ad07acc3db781d9eb0c351715f18fca5a2b4526838981fdb34a522427e9dc868bdaa6
2020-05-30 12:22:09 -04:00
Jim Posen
11106a4722 [net processing] Message handling for getcfilters.
Handle getcfilters request if -peercfilter is configured.
2020-05-26 17:38:20 -04:00
John Newbery
bb911ae7f5 [refactor] Pass CNode and CConnman by reference
Pass CNode and CConnman by reference instead of by pointer to
ProcessGetCFCheckPt() and ProcessGetCFHeaders().
2020-05-26 17:24:17 -04:00
MarcoFalke
7d32cce3e7
Merge #19010: net processing: Add support for getcfheaders
5308c97cca [test] Add test for cfheaders (Jim Posen)
f6b58c1506 [net processing] Message handling for getcfheaders. (Jim Posen)
3bdc7c2d39 [doc] Add comment for m_headers_cache (John Newbery)

Pull request description:

  Support `getcfheaders` requests when `-peerblockfilters` is set.

  Does not advertise compact filter support in version messages.

ACKs for top commit:
  jkczyz:
    ACK 5308c97cca
  MarcoFalke:
    re-ACK 5308c97cca , only change is doc related 🗂
  theStack:
    ACK 5308c97cca 🚀

Tree-SHA512: 240fc654f6f634c191d9f7628b6c4801f87ed514a1dd55c7de5d454d4012d1c09509a2d5a246bc7da445cd920252b4cd56a493c060cdb207b04af4ffe53b95f7
2020-05-26 07:27:00 -04:00
Amiti Uttarwar
1f94bb0c74 [doc] Provide rationale for randomization in scheduling. 2020-05-25 11:27:07 -07:00
MarcoFalke
793e0ff22c
Merge #18698: Make g_chainman internal to validation
fab6b9d18f validation: Mark g_chainman DEPRECATED (MarcoFalke)
fa1d97b256 validation: Make ProcessNewBlock*() members of ChainstateManager (MarcoFalke)
fa24d49098 validation: Make PruneOneBlockFile() a member of ChainstateManager (MarcoFalke)
fa84b1cd84 validation: Make LoadBlockIndex() a member of ChainstateManager (MarcoFalke)
fa05fdf0f1 net: Pass chainman into PeerLogicValidation (MarcoFalke)
fa7b626d7a node: Add chainman alias for g_chainman (MarcoFalke)

Pull request description:

  The global `g_chainman` has recently been introduced in #17737. The chainstate manager is primarily needed for the assumeutxo feature, but it can also simplify testing in the future.

  The goal of this pull is to make the global chainstate manager internal to validation, so that all external code does not depend on globals and that unit or fuzz tests can pass in their (potentially mocked) chainstate manager.

  I suggest reviewing the pull request commit-by-commit. It should be relatively straightforward refactoring that does not change behavior at all.

ACKs for top commit:
  ryanofsky:
    Code review ACK fab6b9d18f. Had to be rebased but still looks good

Tree-SHA512: dcbf114aeef4f8320d466369769f22ce4dd8f46a846870354df176c3de9ff17c64630fbd777e7121d7470d7a8564ed8d37b77168746e8df7489c6877e55d7b4f
2020-05-23 07:58:13 -04:00
Jim Posen
f6b58c1506 [net processing] Message handling for getcfheaders.
if -peerblockfilters is configured, handle requests for cfheaders.
2020-05-22 11:59:58 -04:00
fanquake
ad3a61c5f5
Merge #18895: p2p: unbroadcast followups: rpcs, nLastResend, mempool sanity check
651f1d816f [test] wait for inital broadcast before comparing mempool entries (gzhao408)
9d3f7eb986 [mempool] sanity check that all unbroadcast txns are in mempool (gzhao408)
a7ebe48b94 [rpc] add unbroadcast info to mempool entries and getmempoolinfo (gzhao408)
d160069604 [wallet] remove nLastResend logic (gzhao408)

Pull request description:

  Followup to #18038 by amitiuttarwar which introduces the unbroadcast set: "a mechanism for the mempool to track locally submitted transactions" and decreases the frequency of rebroadcast from 10-15 minutes to 12-36 hours.

  This PR addresses some of the outstanding TODOs building on top of it:
  - remove `nLastResend` logic, which is used to ensure rebroadcast doesn't happen again if no new block has been mined (makes sense in 10-15 min period, but not necessary for 12-36 hour period). (#18038 [comment](https://github.com/bitcoin/bitcoin/pull/18038#discussion_r416826914))
  - expose unbroadcast info via RPCs, for more informative queries and testing (#18038 [comment](https://github.com/bitcoin/bitcoin/pull/18038#discussion_r416837980))
  - add sanity check to verify unbroadcast transactions still exist in mempool before rebroadcasting (#18038 [comment](https://github.com/bitcoin/bitcoin/pull/18038#discussion_r416861609))

ACKs for top commit:
  naumenkogs:
    Code review ACK 651f1d816f
  amitiuttarwar:
    ACK 651f1d816f 🎉
  MarcoFalke:
    Review ACK 651f1d816f

Tree-SHA512: d5327e95ef39d44152b48df5c610502ae11c168f43dbbfb2885340c93d1ba9426eb3a5794573f5fc843502109cb3ffb63efa3f2db4f8f112efcde8f76d9a8845
2020-05-22 07:51:51 +08:00
Wladimir J. van der Laan
4479eb04d9
Merge #18960: indexes: Add compact block filter headers cache
0187d4c118 [indexes] Add compact block filter headers cache (John Newbery)

Pull request description:

  Cache block filter headers at heights of multiples of 1000 in memory.

  Block filter headers at height 1000x are checkpointed, and will be the most frequently requested. Cache them in memory to avoid costly disk reads.

ACKs for top commit:
  jkczyz:
    ACK 0187d4c118
  theStack:
    ACK 0187d4c118 🎉
  fjahr:
    re-utACK 0187d4c118
  laanwj:
    code review ACK 0187d4c118
  ariard:
    Code Review ACK 0187d4c.

Tree-SHA512: 2075ae36901ebcdc4a217eae5203ebc8582181a0831fb7a53a119f031c46bca960a610a38a3d0636a9a405f713efcf4200c85f10c8559fd80139036d89473c56
2020-05-21 19:34:29 +02:00
MarcoFalke
fa1d97b256
validation: Make ProcessNewBlock*() members of ChainstateManager 2020-05-21 09:56:16 -04:00
MarcoFalke
fa05fdf0f1
net: Pass chainman into PeerLogicValidation 2020-05-21 09:55:58 -04:00
MarcoFalke
cfe22a5f9e
Merge #18530: Add test for -blocksonly and -whitelistforcerelay param interaction
0ea5d70b47 Updated comment for the condition where a transaction relay is denied (glowang)
be01449cc8 Add test for param interaction b/w -blocksonly and -whitelistforcerelay (glowang)

Pull request description:

  Related to: #18428

  When -blocksonly is turned on, a node would still relay transactions from whitelisted peers. This funcitonality has not been tested.

ACKs for top commit:
  MarcoFalke:
    ACK 0ea5d70b47

Tree-SHA512: 4e99c88281cb518cc67f5f3be7171a7b413933047b5d24a04bb3ff2210a82e914d69079f64cd5bac9206ec435e21a622c8e69cedbc2ccb39d2328ac5c01668e5
2020-05-21 09:00:25 -04:00
gzhao408
9d3f7eb986 [mempool] sanity check that all unbroadcast txns are in mempool
- before reattempting broadcast for unbroadcast txns, check they are in mempool and remove if not
- this protects from memory leaks and network spam just in case unbroadcast set (incorrectly) has extra txns
- check that tx is in mempool before adding to unbroadcast set to try to prevent this from happening
2020-05-19 14:23:19 -07:00
fanquake
c73bd004ae
Merge #18861: Do not answer GETDATA for to-be-announced tx
2896c412fa Do not answer GETDATA for to-be-announced tx (Pieter Wuille)
f2f32a3dee Push down use of cs_main into FindTxForGetData (Pieter Wuille)
c6131bf407 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)
      )
    )
  ```

ACKs for top commit:
  naumenkogs:
    utACK 2896c41
  ajtowns:
    ACK 2896c412fa
  amitiuttarwar:
    code review ACK 2896c412fa
  jonatack:
    ACK 2896c412fa per `git diff 2b3f101 2896c41` only change since previous review is moving the recency check up to be verified first in `FindTxForGetData`, as it was originally in 353a391 (good catch), before looking up the transaction in the relay pool.
  jnewbery:
    code review ACK 2896c412fa

Tree-SHA512: e7d5bc006e626f60a2c108a9334f3bbb67205ace04a7450a1e4d4db1d85922a7589e0524500b7b4953762cf70554c4a08eec62c7b38b486cbca3d86321600868
2020-05-19 15:18:06 +08:00
John Newbery
0187d4c118 [indexes] Add compact block filter headers cache
Cache block filter headers at heights of multiples of 1000 in memory.

Block filter headers at height 1000x are checkpointed, and will be the
most frequently requested. Cache them in memory to avoid costly disk
reads.
2020-05-18 12:54:07 -04:00
glowang
0ea5d70b47 Updated comment for the condition where a transaction relay is denied 2020-05-17 08:33:09 -07:00
Pieter Wuille
2896c412fa Do not answer GETDATA for to-be-announced tx 2020-05-12 15:33:18 -07:00
John Newbery
746736639e [net processing] Only send a getheaders for one block in an INV
Headers-first is the primary method of announcement on the network. If a
node fell back sending blocks by inv, it's probably for a re-org. The
final block hash provided should be the highest, so send a getheaders
and then fetch the blocks we need to catch up.
2020-05-12 16:29:49 -04:00
Pieter Wuille
f2f32a3dee Push down use of cs_main into FindTxForGetData 2020-05-12 13:17:42 -07:00
Pieter Wuille
c6131bf407 Abstract logic to determine whether to answer tx GETDATA 2020-05-12 13:16:55 -07:00
MarcoFalke
e45fb7e0d2
Merge #18877: Serve cfcheckpt requests
23083856a5 [test] Add test for cfcheckpt (Jim Posen)
f9e00bb25a [net processing] Message handling for getcfcheckpt. (Jim Posen)
9ccaaba11e [init] Add -peerblockfilters option (Jim Posen)

Pull request description:

  Serve cfcheckpt messages if basic block filter index is enabled and `-peercfilters` is set.

  `NODE_COMPACT_FILTERS` is not signaled to peers, but functionality can be used for testing and serving pre-configured clients.

ACKs for top commit:
  jonatack:
    Code review re-ACK 23083856a5 the only change since my review @ 967e2b1 is an update required for #16224 that was merged yesterday.
  fjahr:
    re-ACK 23083856a5
  jkczyz:
    re-ACK 23083856a5
  ariard:
    re-Code Review ACK 2308385
  clarkmoody:
    Tested ACK 23083856a
  MarcoFalke:
    re-ACK 23083856a5 🌳
  theStack:
    ACK 23083856a5

Tree-SHA512: 8c751bbd7d1c31a413096462ae025c3d2f3163c7016cbec472a5f5ec267f8dd19a2dfc4d749876d7409c1db546e6fdd16461c6863effcfa0d3e993edcfa92a08
2020-05-12 09:03:07 -04:00
fanquake
7a5767423f
Merge #18808: [net processing] Drop unknown types in getdata
9847e205bf [docs] Improve commenting in ProcessGetData() (John Newbery)
2f032556e0 [test] test that an invalid GETDATA doesn't prevent processing of future messages (Amiti Uttarwar)
e257cf71c8 [net processing] ignore unknown INV types in GETDATA messages (Amiti Uttarwar)
047ceac142 [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 9847e205bf
  brakmic:
    ACK 9847e205bf
  luke-jr:
    utACK 9847e205bf
  naumenkogs:
    utACK 9847e20
  ajtowns:
    utACK 9847e205bf

Tree-SHA512: 6007f2fd839ffe737727f6fb8e8f083b2d9e05a510748f1d40b8f9be8fdf7b5419a36d8f1039923eec1ba2983e8f6f0436ec5fc196d9f6dcb0657f2ff8ff8e4c
2020-05-12 09:13:48 +08:00
Jim Posen
f9e00bb25a [net processing] Message handling for getcfcheckpt.
If -peerblockfilters is configured, handle requests for cfcheckpt.
2020-05-08 16:36:19 -04:00
fanquake
551dc7f664
Merge #18806: net: remove is{Empty,Full} flags from CBloomFilter, clarify CVE fix
1ad8ea2b73 net: remove is{Empty,Full} flags from CBloomFilter, clarify CVE fix (Sebastian Falbesoner)

Pull request description:

  The BIP37 bloom filter class `CBloomFilter` contains two flags `isEmpty`/`isFull` together with an update method with the purpose to, according to the comments, "avoid wasting cpu", i.e. the mechanism should serve as an optimization for the trivial cases of empty (all bits zero) or full (all bits one) filters.
  However, the real reason of adding those flags (introduced with commit 37c6389c5a by gmaxwell) was a _covert fix_ of [CVE-2013-5700](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2013-5700), a vulnerability that allowed a divide-by-zero remote node crash.
  According to gmaxwell himself (https://github.com/bitcoin/bitcoin/pull/9060#issuecomment-257749165):
  > the IsEmpty/IsFull optimizations were largely a pretextual optimization intended to make unexploitable a remote crash vulnerability (integer division by zero) that existed in the original bloom filtering code without disclosing it. I'm doubtful that they are all that useful. :)

  For more information on how to trigger this crash, see PR https://github.com/bitcoin/bitcoin/pull/18515 which contains a detailled description and a regression test. It has also been discussed on a [recent PR club meeting on fuzzing](https://bitcoincore.reviews/18521.html).

  The covert fix code already led to issues and PR based on the wrong assumption that the flags are there for optimization reasons (see #16886 and #16922). This PR gets rid of the flags and the update method and just focuses on the CVE fix itself, i.e. it can be seen as a revert of the covert fix commit modulo the actual fix.

ACKs for top commit:
  meshcollider:
    utACK 1ad8ea2b73
  laanwj:
    Concept and code review ACK 1ad8ea2b73
  jkczyz:
    ACK 1ad8ea2b73
  MarcoFalke:
    ACK 1ad8ea2b73
  fjahr:
    Code review ACK 1ad8ea2b73

Tree-SHA512: 29f7ff9faece0285e11e16c024851f5bcb772dec64118ccc3f9067ec256267ec8e1b1e3105c7de2a72fd122c3b085e8fc840ab8f4e49813f1cc7a444df1867f7
2020-05-06 15:40:06 +08:00
John Newbery
9847e205bf [docs] Improve commenting in ProcessGetData() 2020-04-29 19:34:01 -04:00
Amiti Uttarwar
e257cf71c8 [net processing] ignore unknown INV types in GETDATA messages
Co-Authored-By: John Newbery <john@johnnewbery.com>
2020-04-29 10:54:55 -04:00
Amiti Uttarwar
047ceac142 [net processing] ignore tx GETDATA from blocks-only peers
Co-Authored-By: John Newbery <john@johnnewbery.com>
2020-04-29 10:54:48 -04:00
fanquake
0ef0d33f75
Merge #18038: P2P: Mempool tracks locally submitted transactions to improve wallet privacy
50fc4df6c4 [mempool] Persist unbroadcast set to mempool.dat (Amiti Uttarwar)
297a178536 [test] Integration tests for unbroadcast functionality (Amiti Uttarwar)
6851502472 [refactor/test] Extract P2PTxInvStore into test framework (Amiti Uttarwar)
dc1da48dc5 [wallet] Update the rebroadcast frequency to be ~1/day. (Amiti Uttarwar)
e25e42f20a [p2p] Reattempt initial send of unbroadcast transactions (Amiti Uttarwar)
7e93eecce3 [util] Add method that returns random time in milliseconds (Amiti Uttarwar)
89eeb4a333 [mempool] Track "unbroadcast" transactions (Amiti Uttarwar)

Pull request description:

  This PR introduces mempool tracking of unbroadcast transactions and periodic reattempts at initial broadcast. This is a part of the rebroadcast project, and a standalone privacy win.

  The current rebroadcast logic is terrible for privacy because 1. only the source wallet rebroadcasts transactions and 2. it does so quite frequently. In the current system, if a user submits a transaction that does not immediately get broadcast to the network (eg. they are offline), this "rebroadcast" behavior is the safety net that can actually serve as the initial broadcast. So, keeping the attempts frequent is important for initial delivery within a reasonable timespan.

  This PR aims to improve # 2 by reducing the wallet rebroadcast frequency to ~1/day from ~1/15 min. It achieves this by separating the notion of initial broadcast from rebroadcasts. With these changes, the mempool tracks locally submitted transactions & periodically reattempts initial broadcast. Transactions submitted via the wallet or RPC are added to an "unbroadcast" set & are removed when a peer sends a `getdata` request, or the transaction is removed from the mempool. Every 10-15 minutes, the node reattempts an initial broadcast. This enables reducing the wallet rebroadcast frequency while ensuring the transactions will be propagated to the network.

  For privacy improvements around # 1, please see #16698.
  Thank you to gmaxwell for the idea of how to break out this subset of functionality (https://github.com/bitcoin/bitcoin/pull/16698#issuecomment-571399346)

ACKs for top commit:
  fjahr:
    Code review ACK 50fc4df6c4
  MarcoFalke:
    ACK 50fc4df6c4, I think this is ready for merge now 👻
  amitiuttarwar:
    The current tip `50fc4df` currently has 6 ACKs on it, so I've opened #18807 to address the last bits.
  jnewbery:
    utACK 50fc4df6c4.
  ariard:
    Code Review ACK 50fc4df (minor points no need to invalid other ACKs)
  robot-visions:
    ACK 50fc4df6c4
  sipa:
    utACK 50fc4df6c4
  naumenkogs:
    utACK 50fc4df

Tree-SHA512: 2dd935d645d5e209f8abf87bfaa3ef0e4492705ce7e89ea64279cb27ffd37f4727fa94ad62d41be331177332f8edbebf3c7f4972f8cda10dd951b80a28ab3c0f
2020-04-29 16:32:37 +08:00
Sebastian Falbesoner
1ad8ea2b73 net: remove is{Empty,Full} flags from CBloomFilter, clarify CVE fix 2020-04-28 19:27:22 +02:00
Amiti Uttarwar
e25e42f20a [p2p] Reattempt initial send of unbroadcast transactions
Every 10-15 minutes, the scheduler kicks off a job that queues unbroadcast
transactions onto each node.
2020-04-23 14:42:25 -07:00
Amiti Uttarwar
89eeb4a333 [mempool] Track "unbroadcast" transactions
- Mempool tracks locally submitted transactions (wallet or rpc)
- Transactions are removed from set when the node receives a GETDATA request
  from a peer, or if the transaction is removed from the mempool.
2020-04-23 14:42:25 -07:00
John Newbery
e9ea95a30d [net processing] Move all const declarations to top of net_processing.cpp 2020-04-23 12:54:06 -04:00
John Newbery
b8580cacc7 [net processing] Move net processing consts to net_processing.cpp 2020-04-23 12:54:03 -04:00
MarcoFalke
da4cbb7927
Merge #18544: net: limit BIP37 filter lifespan (active between 'filterload'..'filterclear')
a9ecbdfcaa test: add more inactive filter tests to p2p_filter.py (Sebastian Falbesoner)
5eae034996 net: limit BIP37 filter lifespan (active between 'filterload' and 'filterclear') (Sebastian Falbesoner)

Pull request description:

  This PR fixes https://github.com/bitcoin/bitcoin/issues/18483. On the master branch, there is currently _always_ a BIP37 filter set for every peer: if not a specific filter is set through a `filterload` message, a default match-everything filter is instanciated and pointed to via the `CBloomFilter` default constructor; that happens both initially, when the containing structure `TxRelay` is constructed:

  c0b389b335/src/net.h (L812)

  and after a loaded filter is removed again through a `filterclear` message:

  c0b389b335/src/net_processing.cpp (L3201)

  The behaviour was introduced by commit 37c6389c5a (an intentional covert fix for [CVE-2013-5700](https://github.com/bitcoin/bitcoin/pull/18515), according to gmaxwell).

  This default match-everything filter leads to some unintended side-effects:
  1. `getdata` request for filtered blocks (i.e. type `MSG_FILTERED_BLOCK`) are always responded to with `merkleblock`s, even if no filter was set by the peer, see issue #18483 (strictly speaking, this is a violation of BIP37) c0b389b335/src/net_processing.cpp (L1504-L1507)
  2. if a peer sends a `filteradd` message without having loaded a filter via `filterload` before, the intended increasing of the banscore never happens (triggered if `bad` is set to true, a few lines below) c0b389b335/src/net_processing.cpp (L3182-L3186)

  This PR basically activates the `else`-branch code paths for all checks of `pfilter` again (on the master branch, they are dead code) by limiting the pointer's lifespan: instead of always having a filter set, the `pfilter` is only pointing to a `CBloomFilter`-instance after receiving a `filterload` message and the instance is destroyed again (and the pointer nullified) after receiving a `filterclear` message.

  Here is a before/after comparison in behaviour:
  | code part / scenario                          |    master branch                   |   PR branch                                          |
  | --------------------------------------------- | ---------------------------------- | ---------------------------------------------------- |
  | `getdata` processing for `MSG_FILTERED_BLOCK` | always responds with `merkleblock` | only responds if filter was set via `filterload`     |
  | `filteradd` processing, no filter was loaded  | nothing                            | peer's banscore increases by 100 (i.e. disconnect)   |

  On the other code parts where `pfilter` is checked there is no change in the logic behaviour (except that `CBloomFilter::IsRelevantAndUpdate()` is unnecessarily called and immediately returned in the master branch).
  Note that the default constructor of `CBloomFilter` is only used for deserializing the received `filterload` message and nowhere else. The PR also contains a functional test checking that sending `getdata` for filtered blocks is ignored by the node if no bloom filter is set.

ACKs for top commit:
  MarcoFalke:
    re-ACK a9ecbdfcaa, only change is in test code 🕙

Tree-SHA512: 1a656a6d74ccaf628e7fdca063ba63fbab2089e0b6d0a11be9bbd387c2ee6d3230706ff8ffc1a55711481df3d4547137dd7c9d9184d89eaa43ade4927792d0b6
2020-04-20 06:59:53 -04:00
MarcoFalke
29893ec875
Merge #18454: net: Make addr relay mockable, add test
fa1da3d4bf test: Add basic addr relay test (MarcoFalke)
fa1793c1c4 net: Pass connman const when relaying address (MarcoFalke)
fa47a0b003 net: Make addr relay mockable (MarcoFalke)

Pull request description:

  As usual:

  * Switch to std::chrono time to be type-safe and mockable
  * Add basic test that relies on mocktime to add code coverage

ACKs for top commit:
  naumenkogs:
    utACK  fa1da3d
  promag:
    ACK fa1da3d4bf (fabe56e44b6f683e24e37246a7a8851190947cb3 before https://github.com/bitcoin/bitcoin/pull/18454#issuecomment-607866453), fa5bf23d527a450e72c2bf13d013e5393b664ca3 was dropped since last review.

Tree-SHA512: 0552bf8fcbe375baa3cab62acd8c23b2994efa47daff818ad1116d0ffaa0b9e520dc1bca2bbc68369b25584e85e54861fe6fd0968de4f503b95439c099df9bd7
2020-04-10 10:12:46 -04:00
Sebastian Falbesoner
5eae034996 net: limit BIP37 filter lifespan (active between 'filterload' and 'filterclear')
Previously, a default match-everything bloom filter was set for every peer,
i.e. even before receiving a 'filterload' message and after receiving a
'filterclear' message code branches checking for the existence of the filter
by testing the pointer "pfilter" were _always_ executed.
2020-04-09 11:26:24 +02:00
MarcoFalke
7777e3624f
scripted-diff: Replace strCommand with msg_type
-BEGIN VERIFY SCRIPT-
sed -i 's/\<strCommand\>/msg_type/g' ./src/net_processing.cpp ./src/test/fuzz/process_message.cpp
-END VERIFY SCRIPT-
2020-04-06 08:00:34 +08:00
MarcoFalke
fa1793c1c4
net: Pass connman const when relaying address 2020-04-02 21:56:20 +08:00
MarcoFalke
fa47a0b003
net: Make addr relay mockable 2020-03-27 17:58:51 -04:00
Wladimir J. van der Laan
312d27b11c
Merge #17477: Remove the mempool's NotifyEntryAdded and NotifyEntryRemoved signals
e57980b473 [mempool] Remove NotifyEntryAdded and NotifyEntryRemoved callbacks (John Newbery)
2dd561f361 [validation] Remove pool member from ConnectTrace (John Newbery)
969b65f3f5 [validation] Remove NotifyEntryRemoved callback from ConnectTrace (John Newbery)
5613f9842b [validation] Remove conflictedTxs from PerBlockConnectTrace (John Newbery)
cdb893443c [validation interface] Remove vtxConflicted from BlockConnected (John Newbery)
1168394d75 [wallet] Notify conflicted transactions in TransactionRemovedFromMempool (John Newbery)

Pull request description:

  These boost signals were added in #9371, before we had a `TransactionRemovedFromMempool` method in the validation interface. The `NotifyEntryAdded` callback was used by validation to build a vector of conflicted transactions when connecting a block, which the wallet was notified of in the `BlockConnected` CValidationInterface callback.

  Now that we have a `TransactionRemovedFromMempool` callback, we can fire that signal directly from the mempool for conflicted transactions.

  Note that #9371 was implemented to ensure `-walletnotify` events were fired for these conflicted transaction. We inadvertently stopped sending these notifications in #16624 (Sep 2019 commit 7e89994). We should probably fix that, but in a different PR.

ACKs for top commit:
  jonatack:
    Re-ACK e57980b
  ryanofsky:
    Code review ACK e57980b473, no code changes since previous review, but helpful new code comments have been added and the PR description is now more clear about where the old code came from

Tree-SHA512: 3bdbaf1ef2731e788462d4756e69c42a1efdcf168691ce1bbfdaa4b7b55ac3c5b1fd4ab7b90bcdec653703600501b4224d252cfc086aef28f9ce0da3b0563a69
2020-03-19 17:26:51 +01:00
Vasil Dimov
7d8e1dec3b
net: fix use-after-free in tests
In PeerLogicValidation::PeerLogicValidation() we would schedule a lambda
function to execute later, capturing the local variable
`consensusParams` by reference.

Presumably this was considered safe because `consensusParams` is a
reference itself to a global variable which is not supposed to change,
but it can in tests.

Fixes https://github.com/bitcoin/bitcoin/issues/18372
2020-03-18 13:03:26 +01:00
MarcoFalke
ce87d5613a
Merge #18289: refactor: Make scheduler methods type safe
fa36f3a295 refactor: move DUMP_BANS_INTERVAL to banman.h (MarcoFalke)
fadafb83cf scheduler: Make schedule* methods type safe (MarcoFalke)
fa70ccc6c4 scheduler: Use C++11 member initialization, add shutdown assert (MarcoFalke)

Pull request description:

  Main benefit is that stuff like `15 * 60 * 1000` is replaced by `minutes{15}`

ACKs for top commit:
  vasild:
    ACK fa36f3a (code review, not tested)
  ajtowns:
    ACK fa36f3a295
  jonatack:
    ACK fa36f3a

Tree-SHA512: f35f1a1d643dfa676bd47474659f6492ed05cca04cdb556064b126f654a6a44a4b93fcaddcdcd41faf81b8f11439c11e5c7ab88685ba2eef12f7188843d17ad8
2020-03-17 16:34:53 -04:00
MarcoFalke
fa7fea3654
refactor: Remove mempool global from net
This refactor does two things:
* Pass mempool in to PeerLogicValidation
* Pass m_mempool around where needed
2020-03-12 09:23:56 -04:00
John Newbery
cdb893443c [validation interface] Remove vtxConflicted from BlockConnected
The wallet now uses TransactionRemovedFromMempool to be notified about
conflicted wallet, and no other clients use vtxConflicted.
2020-03-11 18:38:33 -04:00
practicalswift
fd1dae10b4 tests: Add fuzzing harness for ProcessMessage(...) 2020-03-11 06:57:55 +00:00
MarcoFalke
fadafb83cf
scheduler: Make schedule* methods type safe 2020-03-10 09:47:32 -04:00