aboutsummaryrefslogtreecommitdiff
path: root/src/net_processing.cpp
AgeCommit message (Collapse)Author
2020-06-24Merge #19272: net, test: invalid p2p messages and test framework improvementsMarcoFalke
56010f92564a94b0ca6c008c0e6f74a19fad4a2a test: hoist p2p values to test framework constants (Jon Atack) 75447f0893f9ad9bf83d182b301d139430d8de1c test: improve msg sends and p2p disconnections in p2p_invalid_messages (Jon Atack) 57960192a5362ff1a7b996995332535f4c2a25c3 test: refactor test_large_inv() into 3 tests with common method (Jon Atack) e2b21d8a597c536a8617408d43958bfe9f98a442 test: add p2p_invalid_messages logging (Jon Atack) 9fa494dc0969c61d5ef33708a08923cca19ce091 net: update misbehavior logging for oversized messages (Jon Atack) Pull request description: ...seen while reviewing #19264, #19252, #19304 and #19107: in `net_processing.cpp` - make the debug logging for oversized message size misbehavior the same for `addr`, `getdata`, `headers` and `inv` messages in `p2p_invalid_messages` - add missing logging - improve assertions/message sends, move cleanup disconnections outside the assertion scopes - split a slowish 3-part test into 3 order-independent tests - add a few p2p constants to the test framework ACKs for top commit: troygiorshev: reACK 56010f92564a94b0ca6c008c0e6f74a19fad4a2a MarcoFalke: ACK 56010f9256 🎛 Tree-SHA512: db67b70278f8d4c318907e105af54b54eb3afd15500f9aa0c98034f6fd4bd1cf9ad1663037bd9b237ff4890f3059b37291a6498d8d6ae2cc38efb9f045f73310
2020-06-21doc: Remove -whitelistforcerelay from commentMarcoFalke
Instead, permission flags should be used. For example -whitelist=forcerelay@127.0.0.1
2020-06-19net: update misbehavior logging for oversized messagesJon Atack
so that oversized ADDR, GETDATA, HEADERS and INV messages print the same consistent debug logs.
2020-06-19Merge #19293: net: Avoid redundant and confusing FAILED logfanquake
fa1904e5f0d164fbcf41398f9ebbaafe82c28419 net: Remove dead logging code (MarcoFalke) fac12ebf4f3b77b05112d2b00f8d3f4669621a4c net: Avoid redundant and confusing FAILED log (MarcoFalke) Pull request description: Remove a redundant and confusing "FAILED" log message and gets rid of the unused return type in `ProcessMessage` ACKs for top commit: jnewbery: utACK fa1904e5f0d164fbcf41398f9ebbaafe82c28419 gzhao408: utACK https://github.com/bitcoin/bitcoin/commit/fa1904e5f0d164fbcf41398f9ebbaafe82c28419 troygiorshev: ACK fa1904e5f0d164fbcf41398f9ebbaafe82c28419 naumenkogs: utACK fa1904e Tree-SHA512: bfa553d5efa022727ed17877fb7c08c14849d804fe6d6a7ce172d513857beba35de41ea40b27ff1aedf68b81e2cda7b2a948ac985fcaaf1b6cfb96cce4837c90
2020-06-18[net] split PushInventory()John Newbery
PushInventory() is currently called with a CInv object, which can be a MSG_TX or MSG_BLOCK. PushInventory() only uses the type to determine whether to add the hash to setInventoryTxToSend or vInventoryBlockToSend. Since the caller always knows what type of inventory they're pushing, the CInv is wastefully constructed and thrown away, and tx/block relay is being split out, we split the function into PushTxInventory() and PushBlockInventory().
2020-06-16net: Remove dead logging codeMarcoFalke
fRet is never false, so the dead code can be removed and the return type can be made void
2020-06-16net: Avoid redundant and confusing FAILED logMarcoFalke
Every `return false` is preceeded by a detailed debug log message to explain that a disconnect or misbehavior happened. Logging another generic "FAILED" message seems redundant. Also, the size of the message and the message type has already been logged and is thus redundant as well. Finally, claiming that message processing FAILED seems odd, because the message was fully processed to the point where it was concluded that the peer should be either disconnected or marked as misbehaving.
2020-06-14[p2p/refactor] move disconnect logic and remove misbehavinggzhao408
-Increasing the banscore and/or banning is too harsh, just disconnecting is enough. -Return true from ProcessMessage because we already log receipt of filterclear and disconnect.
2020-06-14[netprocessing] disconnect node that sends filtercleargzhao408
-nodes not serving bloomfilters should disconnect peers that send filterclear, just like filteradd and filterload -nodes that want to enable/disable txrelay should use feefilter
2020-06-04doc: noban precludes maxuploadtarget disconnectsMarcoFalke
2020-06-04net: Reformat excessively long if condition into multiple linesMarcoFalke
Can be reviewed with the git option --word-diff-regex=.
2020-06-02refactor: replace CNode pointers by references within net_processing.{h,cpp}Sebastian Falbesoner
2020-05-31Merge #19044: net processing: Add support for getcfiltersMarcoFalke
9e36067d8cd02830c7e5a88a391dff6ac3adbe0c [test] Add test for cfilters. (Jim Posen) 11106a4722558765a44ae45c7892724a73ce514c [net processing] Message handling for getcfilters. (Jim Posen) e535670726952e43483763dfca6fc6ec2f4b0691 [indexes] Fix default [de]serialization of BlockFilter. (Jim Posen) bb911ae7f5cbe4974ec61266d2334b95067fa49d [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 https://github.com/bitcoin/bitcoin/pull/19044/commits/9e36067d8cd02830c7e5a88a391dff6ac3adbe0c MarcoFalke: re-ACK 9e36067d8c , only change is adding commit "[refactor] Pass CNode and CConnman by reference" 🥑 jkczyz: ACK 9e36067d8cd02830c7e5a88a391dff6ac3adbe0c fjahr: Code review ACK 9e36067d8cd02830c7e5a88a391dff6ac3adbe0c Tree-SHA512: b45b42a25905ef0bd9e195029185300c86856c87f78cbe17921f4a25e159ae0f6f003e61714fa43779017eb97cd89d3568419be88e47d19dc8095562939e7887
2020-05-30Merge #18807: [doc / test / mempool] unbroadcast follow-upsMarcoFalke
9e1cb1adf1800efe429e348650931f2669b0d2c0 [trivial/doc] Fix comment type (Amiti Uttarwar) 8f30260a67166a6ab7c0f33f7ec1990d3c31761e [doc] Update unbroadcast description in RPC results (Amiti Uttarwar) 750456d6f29c63d57af05bfbdd6035bb9c965de2 [trivial] Remove misleading 'const' (Amiti Uttarwar) fa32e676e5833a5c5fc735ef00c0a80f5fab7a2c [test] Manage node connections better in mempool persist test (Amiti Uttarwar) 1f94bb0c744a103b633c1051e8fbc01e612097dc [doc] Provide rationale for randomization in scheduling. (Amiti Uttarwar) 9c8a55d9cb0ec73f10b196e79b637aa601c0a6b7 [mempool] Don't throw expected error message when upgrading (Amiti Uttarwar) ba5498318233ab81decbc585e9619d8ffe2df1b0 [test] Test that wallet transactions aren't rebroadcast before 12 hours (Amiti Uttarwar) 00d44a534b4e5ae249b8011360c6b0f7dc731581 [test] P2P connection behavior should meet expectations (Amiti Uttarwar) bd093ca15de762fdaf0937a0877d17b0c2bce16e [test] updates to unbroadcast test (Amiti Uttarwar) dab298d9ab5a5a41685f437db9081fa7b395fa73 [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`](https://github.com/bitcoin/bitcoin/pull/18807/commits/9e1cb1adf1800efe429e348650931f2669b0d2c0) Tree-SHA512: 0cd51c4ca368b9dce92d50d73ec6e9df278a259e609eef2858f24cb8595ad07acc3db781d9eb0c351715f18fca5a2b4526838981fdb34a522427e9dc868bdaa6
2020-05-26[net processing] Message handling for getcfilters.Jim Posen
Handle getcfilters request if -peercfilter is configured.
2020-05-26[refactor] Pass CNode and CConnman by referenceJohn Newbery
Pass CNode and CConnman by reference instead of by pointer to ProcessGetCFCheckPt() and ProcessGetCFHeaders().
2020-05-26Merge #19010: net processing: Add support for getcfheadersMarcoFalke
5308c97ccaf0955e5840956bc1636108a43e6f46 [test] Add test for cfheaders (Jim Posen) f6b58c150686e90bc4952976e488b1605f3ae02a [net processing] Message handling for getcfheaders. (Jim Posen) 3bdc7c2d3977a7864aacea80bffc4df7f37cac51 [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 5308c97ccaf0955e5840956bc1636108a43e6f46 MarcoFalke: re-ACK 5308c97cca , only change is doc related 🗂 theStack: ACK 5308c97ccaf0955e5840956bc1636108a43e6f46 :rocket: Tree-SHA512: 240fc654f6f634c191d9f7628b6c4801f87ed514a1dd55c7de5d454d4012d1c09509a2d5a246bc7da445cd920252b4cd56a493c060cdb207b04af4ffe53b95f7
2020-05-25[doc] Provide rationale for randomization in scheduling.Amiti Uttarwar
2020-05-23Merge #18698: Make g_chainman internal to validationMarcoFalke
fab6b9d18fd48bbbd1939b1173723bc04c5824b5 validation: Mark g_chainman DEPRECATED (MarcoFalke) fa1d97b25686a5caca623599f6d608fd08616fe8 validation: Make ProcessNewBlock*() members of ChainstateManager (MarcoFalke) fa24d4909864096934577abc26cfa9be47f634ba validation: Make PruneOneBlockFile() a member of ChainstateManager (MarcoFalke) fa84b1cd846f6499b741710fd478ec9ad49b5120 validation: Make LoadBlockIndex() a member of ChainstateManager (MarcoFalke) fa05fdf0f19fa4b557cc5e9ba436e3215b83c4e6 net: Pass chainman into PeerLogicValidation (MarcoFalke) fa7b626d7a150e5cbd4d163d2dab6f8a55fc2cc4 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 fab6b9d18fd48bbbd1939b1173723bc04c5824b5. Had to be rebased but still looks good Tree-SHA512: dcbf114aeef4f8320d466369769f22ce4dd8f46a846870354df176c3de9ff17c64630fbd777e7121d7470d7a8564ed8d37b77168746e8df7489c6877e55d7b4f
2020-05-22[net processing] Message handling for getcfheaders.Jim Posen
if -peerblockfilters is configured, handle requests for cfheaders.
2020-05-22Merge #18895: p2p: unbroadcast followups: rpcs, nLastResend, mempool sanity ↵fanquake
check 651f1d816f054cb9c637f8a99c9360bba381ef58 [test] wait for inital broadcast before comparing mempool entries (gzhao408) 9d3f7eb9860254eb787ebe2734fd6a26bcf365c1 [mempool] sanity check that all unbroadcast txns are in mempool (gzhao408) a7ebe48b94c5a9195c8eabd193204c499cb4bfdb [rpc] add unbroadcast info to mempool entries and getmempoolinfo (gzhao408) d16006960443c2efe37c896e46edae9dca86c57d [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 651f1d816f054cb9c637f8a99c9360bba381ef58 amitiuttarwar: ACK 651f1d816f054cb9c637f8a99c9360bba381ef58 🎉 MarcoFalke: Review ACK 651f1d816f054cb9c637f8a99c9360bba381ef58 Tree-SHA512: d5327e95ef39d44152b48df5c610502ae11c168f43dbbfb2885340c93d1ba9426eb3a5794573f5fc843502109cb3ffb63efa3f2db4f8f112efcde8f76d9a8845
2020-05-21Merge #18960: indexes: Add compact block filter headers cacheWladimir J. van der Laan
0187d4c118ab4c0f5c2d4fb180c2a8dea8ac53cf [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 0187d4c118ab4c0f5c2d4fb180c2a8dea8ac53cf theStack: ACK 0187d4c118ab4c0f5c2d4fb180c2a8dea8ac53cf :tada: fjahr: re-utACK 0187d4c118ab4c0f5c2d4fb180c2a8dea8ac53cf laanwj: code review ACK 0187d4c118ab4c0f5c2d4fb180c2a8dea8ac53cf ariard: Code Review ACK 0187d4c. Tree-SHA512: 2075ae36901ebcdc4a217eae5203ebc8582181a0831fb7a53a119f031c46bca960a610a38a3d0636a9a405f713efcf4200c85f10c8559fd80139036d89473c56
2020-05-21validation: Make ProcessNewBlock*() members of ChainstateManagerMarcoFalke
2020-05-21net: Pass chainman into PeerLogicValidationMarcoFalke
2020-05-21Merge #18530: Add test for -blocksonly and -whitelistforcerelay param ↵MarcoFalke
interaction 0ea5d70b4756f376342417e0019490233cb4a918 Updated comment for the condition where a transaction relay is denied (glowang) be01449cc8eb7bb97531a967f5d1dcc7b8865d1e 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 0ea5d70b4756f376342417e0019490233cb4a918 Tree-SHA512: 4e99c88281cb518cc67f5f3be7171a7b413933047b5d24a04bb3ff2210a82e914d69079f64cd5bac9206ec435e21a622c8e69cedbc2ccb39d2328ac5c01668e5
2020-05-19[mempool] sanity check that all unbroadcast txns are in mempoolgzhao408
- 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-19Merge #18861: Do not answer GETDATA for to-be-announced txfanquake
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) ) ) ``` ACKs for top commit: naumenkogs: utACK 2896c41 ajtowns: ACK 2896c412fadbc03916a33028f4f50fd87ac48edb amitiuttarwar: code review ACK 2896c412fa jonatack: ACK 2896c412fadbc03916 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 2896c412fadbc03916a33028f4f50fd87ac48edb Tree-SHA512: e7d5bc006e626f60a2c108a9334f3bbb67205ace04a7450a1e4d4db1d85922a7589e0524500b7b4953762cf70554c4a08eec62c7b38b486cbca3d86321600868
2020-05-18[indexes] Add compact block filter headers cacheJohn Newbery
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-17Updated comment for the condition where a transaction relay is deniedglowang
2020-05-12Do not answer GETDATA for to-be-announced txPieter Wuille
2020-05-12[net processing] Only send a getheaders for one block in an INVJohn Newbery
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-12Push down use of cs_main into FindTxForGetDataPieter Wuille
2020-05-12Abstract logic to determine whether to answer tx GETDATAPieter Wuille
2020-05-12Merge #18877: Serve cfcheckpt requestsMarcoFalke
23083856a551ca13e8b142791c296ecb25cc4e7f [test] Add test for cfcheckpt (Jim Posen) f9e00bb25ac4039056808affeb5ffa86a2c317fe [net processing] Message handling for getcfcheckpt. (Jim Posen) 9ccaaba11e94571fe984857494042ac292c17156 [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 23083856a551ca13e8b142791c296ecb25cc4e7f the only change since my review @ 967e2b1 is an update required for #16224 that was merged yesterday. fjahr: re-ACK 23083856a551ca13e8b142791c296ecb25cc4e7f jkczyz: re-ACK 23083856a551ca13e8b142791c296ecb25cc4e7f ariard: re-Code Review ACK 2308385 clarkmoody: Tested ACK 23083856a MarcoFalke: re-ACK 23083856a5 🌳 theStack: ACK https://github.com/bitcoin/bitcoin/commit/23083856a551ca13e8b142791c296ecb25cc4e7f Tree-SHA512: 8c751bbd7d1c31a413096462ae025c3d2f3163c7016cbec472a5f5ec267f8dd19a2dfc4d749876d7409c1db546e6fdd16461c6863effcfa0d3e993edcfa92a08
2020-05-12Merge #18808: [net processing] Drop unknown types in getdatafanquake
9847e205bf7edcac4c30ce4b6d62f482aa7bc1b7 [docs] Improve commenting in ProcessGetData() (John Newbery) 2f032556e08a04807c71eb02104ca9589eaadf1b [test] test that an invalid GETDATA doesn't prevent processing of future messages (Amiti Uttarwar) e257cf71c851e25e1a533bf1d4296f6b55c81332 [net processing] ignore unknown INV types in GETDATA messages (Amiti Uttarwar) 047ceac142246b5d51056a51dbf4645b31802be4 [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 9847e205bf7edcac4c30ce4b6d62f482aa7bc1b7 brakmic: ACK https://github.com/bitcoin/bitcoin/commit/9847e205bf7edcac4c30ce4b6d62f482aa7bc1b7 luke-jr: utACK 9847e205bf7edcac4c30ce4b6d62f482aa7bc1b7 naumenkogs: utACK 9847e20 ajtowns: utACK 9847e205bf7edcac4c30ce4b6d62f482aa7bc1b7 Tree-SHA512: 6007f2fd839ffe737727f6fb8e8f083b2d9e05a510748f1d40b8f9be8fdf7b5419a36d8f1039923eec1ba2983e8f6f0436ec5fc196d9f6dcb0657f2ff8ff8e4c
2020-05-08[net processing] Message handling for getcfcheckpt.Jim Posen
If -peerblockfilters is configured, handle requests for cfcheckpt.
2020-05-06Merge #18806: net: remove is{Empty,Full} flags from CBloomFilter, clarify ↵fanquake
CVE fix 1ad8ea2b73134bdd8d6b50704a019d47ad2191d8 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 https://github.com/bitcoin/bitcoin/commit/37c6389c5a0ca63ae3573440ecdfe95d28ad8f07 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 1ad8ea2b73134bdd8d6b50704a019d47ad2191d8 laanwj: Concept and code review ACK 1ad8ea2b73134bdd8d6b50704a019d47ad2191d8 jkczyz: ACK 1ad8ea2b73134bdd8d6b50704a019d47ad2191d8 MarcoFalke: ACK 1ad8ea2b73134bdd8d6b50704a019d47ad2191d8 fjahr: Code review ACK 1ad8ea2b73134bdd8d6b50704a019d47ad2191d8 Tree-SHA512: 29f7ff9faece0285e11e16c024851f5bcb772dec64118ccc3f9067ec256267ec8e1b1e3105c7de2a72fd122c3b085e8fc840ab8f4e49813f1cc7a444df1867f7
2020-04-29[docs] Improve commenting in ProcessGetData()John Newbery
2020-04-29[net processing] ignore unknown INV types in GETDATA messagesAmiti Uttarwar
Co-Authored-By: John Newbery <john@johnnewbery.com>
2020-04-29[net processing] ignore tx GETDATA from blocks-only peersAmiti Uttarwar
Co-Authored-By: John Newbery <john@johnnewbery.com>
2020-04-29Merge #18038: P2P: Mempool tracks locally submitted transactions to improve ↵fanquake
wallet privacy 50fc4df6c4e8a84bdda13ade7bed7a2131796f00 [mempool] Persist unbroadcast set to mempool.dat (Amiti Uttarwar) 297a1785360c4db662a7f3d3ade7b6b503258d39 [test] Integration tests for unbroadcast functionality (Amiti Uttarwar) 6851502472d3625416f0e7796e9f2a0379d14d49 [refactor/test] Extract P2PTxInvStore into test framework (Amiti Uttarwar) dc1da48dc5e5526215561311c184a8cbc345ecdc [wallet] Update the rebroadcast frequency to be ~1/day. (Amiti Uttarwar) e25e42f20a3aa39651fbc1f9fa3df1a49f1f5868 [p2p] Reattempt initial send of unbroadcast transactions (Amiti Uttarwar) 7e93eecce3bc5a1b7bb0284e06f9e2e69454f5ba [util] Add method that returns random time in milliseconds (Amiti Uttarwar) 89eeb4a3335f8e871cc3f5286af4546dff66172a [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 50fc4df6c4e8a84bdda13ade7bed7a2131796f00 MarcoFalke: ACK 50fc4df6c4e8a84bdda13ade7bed7a2131796f00, 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 50fc4df6c4e8a84bdda13ade7bed7a2131796f00. ariard: Code Review ACK 50fc4df (minor points no need to invalid other ACKs) robot-visions: ACK 50fc4df6c4e8a84bdda13ade7bed7a2131796f00 sipa: utACK 50fc4df6c4e8a84bdda13ade7bed7a2131796f00 naumenkogs: utACK 50fc4df Tree-SHA512: 2dd935d645d5e209f8abf87bfaa3ef0e4492705ce7e89ea64279cb27ffd37f4727fa94ad62d41be331177332f8edbebf3c7f4972f8cda10dd951b80a28ab3c0f
2020-04-28net: remove is{Empty,Full} flags from CBloomFilter, clarify CVE fixSebastian Falbesoner
2020-04-23[p2p] Reattempt initial send of unbroadcast transactionsAmiti Uttarwar
Every 10-15 minutes, the scheduler kicks off a job that queues unbroadcast transactions onto each node.
2020-04-23[mempool] Track "unbroadcast" transactionsAmiti Uttarwar
- 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[net processing] Move all const declarations to top of net_processing.cppJohn Newbery
2020-04-23[net processing] Move net processing consts to net_processing.cppJohn Newbery
2020-04-20Merge #18544: net: limit BIP37 filter lifespan (active between ↵MarcoFalke
'filterload'..'filterclear') a9ecbdfcaa15499644d16e9c8ad2c63dfc45b37b test: add more inactive filter tests to p2p_filter.py (Sebastian Falbesoner) 5eae034996b340c19cebab9efb6c89d20fe051ef 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: https://github.com/bitcoin/bitcoin/blob/c0b389b33516fb3eaaad7c30bd11dba768882a7e/src/net.h#L812 and after a loaded filter is removed again through a `filterclear` message: https://github.com/bitcoin/bitcoin/blob/c0b389b33516fb3eaaad7c30bd11dba768882a7e/src/net_processing.cpp#L3201 The behaviour was introduced by commit https://github.com/bitcoin/bitcoin/commit/37c6389c5a0ca63ae3573440ecdfe95d28ad8f07 (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) https://github.com/bitcoin/bitcoin/blob/c0b389b33516fb3eaaad7c30bd11dba768882a7e/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) https://github.com/bitcoin/bitcoin/blob/c0b389b33516fb3eaaad7c30bd11dba768882a7e/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-10Merge #18454: net: Make addr relay mockable, add testMarcoFalke
fa1da3d4bfc0511a89f5b19d5a4d89e55ff7ccde test: Add basic addr relay test (MarcoFalke) fa1793c1c44a3f75a09f9c636467b8274c541bdd net: Pass connman const when relaying address (MarcoFalke) fa47a0b003f53708b6d5df1ed4e7f8a7c68aa3ac 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 fa1da3d4bfc0511a89f5b19d5a4d89e55ff7ccde (fabe56e44b6f683e24e37246a7a8851190947cb3 before https://github.com/bitcoin/bitcoin/pull/18454#issuecomment-607866453), fa5bf23d527a450e72c2bf13d013e5393b664ca3 was dropped since last review. Tree-SHA512: 0552bf8fcbe375baa3cab62acd8c23b2994efa47daff818ad1116d0ffaa0b9e520dc1bca2bbc68369b25584e85e54861fe6fd0968de4f503b95439c099df9bd7
2020-04-09net: limit BIP37 filter lifespan (active between 'filterload' and 'filterclear')Sebastian Falbesoner
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-06scripted-diff: Replace strCommand with msg_typeMarcoFalke
-BEGIN VERIFY SCRIPT- sed -i 's/\<strCommand\>/msg_type/g' ./src/net_processing.cpp ./src/test/fuzz/process_message.cpp -END VERIFY SCRIPT-
generated by cgit v1.2.3 (git 2.26.2) at 2024-09-18 16:14:12 +0000