aboutsummaryrefslogtreecommitdiff
path: root/src/net.h
AgeCommit message (Collapse)Author
2020-08-12Merge #19658: [rpc] Allow RPC to fetch all addrman records and add records ↵Wladimir J. van der Laan
to addrman 37a480e0cd94895b6051abef12d984ff74bdc4a3 [net] Add addpeeraddress RPC method (John Newbery) ae8051bbd8377f2458ff1f167dc30c2d5f83e317 [test] Test that getnodeaddresses() can return all known addresses (John Newbery) f26502e9fc8a669b30717525597e3f468eaecf79 [addrman] Specify max addresses and pct when calling GetAddresses() (John Newbery) Pull request description: Currently addrman only allows a maximum of 1000 records or 23% of all records to be returned in a call to `GetAddr()`. Relax this limit and have the client specify the max records they want. For p2p, behaviour is unchanged (but the rate limiting is set inside net_processing, where it belongs). For RPC, `getnodeaddresses` can now return the complete addrman, which is helpful for testing and monitoring. Also add a test-only RPC `addpeeraddress`, which adds an IP address:port to addrman. This is helpful for testing (eg #18991). ACKs for top commit: naumenkogs: utACK 37a480e0cd94895b6051abef12d984ff74bdc4a3 laanwj: Code review and lightly manually tested ACK 37a480e0cd94895b6051abef12d984ff74bdc4a3 Tree-SHA512: f86dcd410aaebaf6e9ca18ce6f23556e5e4649c1325577213d873aa09967298e65ab2dc19a72670641ae92211a923afda1fe124a82e9d2c1cad73d478ef27fdc
2020-08-12[net] Add addpeeraddress RPC methodJohn Newbery
Allows addresses to be added to Address Manager for testing.
2020-08-12[addrman] Specify max addresses and pct when calling GetAddresses()John Newbery
CAddrMan.GetAddr() would previously limit the number and percentage of addresses returned (to ADDRMAN_GETADDR_MAX (1000) and ADDRMAN_GETADDR_MAX_PCT (23) respectively). Instead, make it the callers responsibility to specify the maximum addresses and percentage they want returned. For net_processing, the maximums are MAX_ADDR_TO_SEND (1000) and MAX_PCT_ADDR_TO_SEND (23). For rpc/net, the maximum is specified by the client.
2020-08-12Merge #19316: [net] Cleanup logic around connection typesfanquake
01e283068b9e6214f2d77a2f772a4244ebfe2274 [net] Remove unnecessary default args on CNode constructor (Amiti Uttarwar) bc5d65b3ca41eebb1738fdda4451d1466e77772e [refactor] Remove IsOutboundDisconnectionCandidate (Amiti Uttarwar) 2f2e13b6c2c8741ca9d825eaaef736ede484bc85 [net/refactor] Simplify multiple-connection checks (Amiti Uttarwar) 7f7b83deb2427599c129f4ff581d4d045461e459 [net/refactor] Rework ThreadOpenConnections logic (Amiti Uttarwar) 35839e963bf61d2da0d12f5b8cea74ac0e0fbd7b [net] Fix bug where AddrFetch connections would be counted as outbound full relay (Amiti Uttarwar) 4972c21b671ff73f13a1b5053338b6abbdb471b5 [net/refactor] Clarify logic for selecting connections in ThreadOpenConnections (Amiti Uttarwar) 60156f5fc40d56bb532278f16ce632c5a8b8035e [net/refactor] Remove fInbound flag from CNode (Amiti Uttarwar) 7b322df6296609570e368e5f326979279041c11f [net/refactor] Remove m_addr_fetch member var from CNode (Amiti Uttarwar) 14923422b08ac4b21b35c426bf0e1b9e7c97983b [net/refactor] Remove fFeeler flag from CNode (Amiti Uttarwar) 49efac5cae7333c6700d9b737d09fae0f3f4d7fa [net/refactor] Remove m_manual_connection flag from CNode (Amiti Uttarwar) d3698b5ee309cf0f0cdfb286d6b30a256d7deae5 [net/refactor] Add connection type as a member var to CNode (Amiti Uttarwar) 46578c03e92a55925308363ccdad04dcfc820d96 [doc] Describe different connection types (Amiti Uttarwar) 442abae2bac7bff85886143df01e14215532b974 [net/refactor] Add AddrFetch connections to ConnectionType enum (Amiti Uttarwar) af59feb05235ecb85ec9d75b09c66e71268c9889 [net/refactor] Extract m_addr_known logic from initializer list (Amiti Uttarwar) e1bc29812ddf1d946bc5acca406a7ed2dca064a6 [net/refactor] Add block relay only connections to ConnectionType enum (Amiti Uttarwar) 0e52a659a2de915fc3dce37fc8fac39be1c8b6fa [net/refactor] Add feeler connections to ConnectionType enum (Amiti Uttarwar) 1521c47438537e192230486dffcec0228a53878d [net/refactor] Add manual connections to ConnectionType enum (Amiti Uttarwar) 26304b4100201754fb32440bec3e3b78cd3f0e6d [net/refactor] Introduce an enum to distinguish type of connection (Amiti Uttarwar) 3f1b7140e95d0f8f958cb35f31c3d964c57e484d scripted-diff: Rename OneShot to AddrFetch (Amiti Uttarwar) Pull request description: **This is part 1 of #19315, which enables the ability to test `outbound` and `block-relay-only` connections from the functional tests.** Please see that PR for more information of overall functionality. **This PR simplifies how we manage different connection types.** It introduces an enum with the various types of connections so we can explicitly define the connection type. The existing system relies on a series of independent flags, then has asserts scattered around to ensure that conflicting flags are not enabled at the same time. I find this approach to be both brittle and confusing. While making these changes, I found a small bug due to the silent assumptions. This PR also proposes a rename from `OneShot` to `AddrFetch`. I find the name `OneShot` to be very confusing, especially when we also have `onetry` manual connections. Everyone I've talked to offline has agreed that the name is confusing, so I propose a potential alternative. I think this is a good opportunity for a rename since I'm creating an enum to explicitly define the connection types. (some context for the unfamiliar: `oneshot` or `addrfetch` connections are short-lived connections created on startup. They connect to the seed peers, send a `getaddr` to solicit addresses, then close the connection.) Overview of this PR: * rename `oneshot` to `addrfetch` * introduce `ConnectionType` enum * one by one, add different connection types to the enum * expose the `conn_type` on CNode, and use this to reduce reliance on flags (& asserts) * fix the bug in counting different type of connections * some additional cleanup to simplify logic and make expectations explicit/inclusive rather than implicit/exclusive. ACKs for top commit: jnewbery: utACK 01e283068b9e6214f2d77a2f772a4244ebfe2274 laanwj: Code review ACK 01e283068b9e6214f2d77a2f772a4244ebfe2274, the commits are pretty straightforward to follow, and I think this is a move in the right direction overall vasild: ACK 01e283068 sdaftuar: ACK 01e283068b9e6214f2d77a2f772a4244ebfe2274. fanquake: ACK 01e283068b9e6214f2d77a2f772a4244ebfe2274 - I don't have as much experience with the networking code but these changes look fairly straight forward, the new code seems more robust/understandable and the additional documentation is great. I'm glad that a followup branch is already underway. There might be some more review comments here later today, so keep an eye on the discussion, however I'm going to merge this now. jb55: wow this code was messy before... ACK 01e283068b9e6214f2d77a2f772a4244ebfe2274 Tree-SHA512: 7bb644a6ed5849913d777ebc2ff89133ca0fbef680355a9a344e07496a979e6f9ff21a958e8eea93dcd7d5c343682b0c7174b1a3de380a4247eaae73da436e15
2020-08-07[net] Remove unnecessary default args on CNode constructorAmiti Uttarwar
2020-08-07[net/refactor] Simplify multiple-connection checksAmiti Uttarwar
Extract logic that check multiple connection types into interface functions & structure as switch statements. This makes it very clear what touch points are for accessing `m_conn_type` & using the switch statements enables the compiler to warn if a new connection type is introduced but not handled for these cases.
2020-08-07[net/refactor] Rework ThreadOpenConnections logicAmiti Uttarwar
Make the connection counts explicit and extract into interface functions around m_conn_type. Using explicit counting and switch statements where possible should help prevent counting bugs in the future.
2020-08-07[net/refactor] Remove fInbound flag from CNodeAmiti Uttarwar
2020-08-07[net/refactor] Remove m_addr_fetch member var from CNodeAmiti Uttarwar
2020-08-07[net/refactor] Remove fFeeler flag from CNodeAmiti Uttarwar
2020-08-07[net/refactor] Remove m_manual_connection flag from CNodeAmiti Uttarwar
2020-08-07[net/refactor] Add connection type as a member var to CNodeAmiti Uttarwar
- Directly maintaining the connection type prevents having to deduce it from several flags.
2020-08-07[doc] Describe different connection typesAmiti Uttarwar
2020-08-07[net/refactor] Add AddrFetch connections to ConnectionType enumAmiti Uttarwar
- AddrFetch connections are short lived connections used to getaddr from a peer - previously called "one shot" connections
2020-08-07[net/refactor] Extract m_addr_known logic from initializer listAmiti Uttarwar
2020-08-07[net/refactor] Add block relay only connections to ConnectionType enumAmiti Uttarwar
2020-08-07[net/refactor] Add feeler connections to ConnectionType enumAmiti Uttarwar
2020-08-07[net/refactor] Add manual connections to ConnectionType enumAmiti Uttarwar
2020-08-07[net/refactor] Introduce an enum to distinguish type of connectionAmiti Uttarwar
- extract inbound & outbound types
2020-08-07scripted-diff: Rename OneShot to AddrFetchAmiti Uttarwar
-BEGIN VERIFY SCRIPT- sed -i 's/a oneshot/an addrfetch/g' src/chainparams.cpp #comment sed -i 's/oneshot/addrfetch/g' src/net.cpp #comment sed -i 's/AddOneShot/AddAddrFetch/g' src/net.h src/net.cpp sed -i 's/cs_vOneShots/m_addr_fetches_mutex/g' src/net.h src/net.cpp sed -i 's/vOneShots/m_addr_fetches/g' src/net.h src/net.cpp sed -i 's/fOneShot/m_addr_fetch/g' src/net.h src/net.cpp src/net_processing.cpp sed -i 's/ProcessOneShot/ProcessAddrFetch/g' src/net.h src/net.cpp -END VERIFY SCRIPT-
2020-07-30Add addr permission flag enabling non-cached addr sharingGleb Naumenko
2020-07-30Cache responses to addr requestsGleb Naumenko
Prevents a spy from scraping victim's AddrMan by reconnecting and re-requesting addrs.
2020-07-24Remove useless 2500 limit on AddrMan queriesGleb Naumenko
2020-07-23Merge #19473: net: Add -networkactive optionMarcoFalke
2aac093a3d60e446b85eebdf170ea6bed77bec92 test: Add test coverage for -networkactive option (Hennadii Stepanov) 3c58129b1293742a49aa196cb210ff345a7339e6 net: Log network activity status change unconditionally (Hennadii Stepanov) 62fe6aa87e4cdd8b06207abc1387c68d7bfc04c1 net: Add -networkactive option (Hennadii Stepanov) Pull request description: Some Bitcoin Core activity is completely local (offline), e.g., reindexing. The `setnetworkactive` RPC command is already present. This PR adds the corresponding command-line argument / config option, and allows to start the client with disabled p2p network by providing `-networkactive=0` or `-nonetworkactive`. This was done while reviewing #16981. ACKs for top commit: MarcoFalke: re-ACK 2aac093a3d60e446b85eebdf170ea6bed77bec92 🏠 LarryRuane: ACK 2aac093a3d60e446b85eebdf170ea6bed77bec92 Tree-SHA512: 446d791b46d7b556d7694df7b1f88cd4fbc09301fe4eaf036b45cb8166ed806156353cc03788a07b633d5887d5eee30a7c02a2d4307141c8ccc75e0a88145636
2020-07-22net: Add -networkactive optionHennadii Stepanov
The `setnetworkactive' RPC command is already present. This new option allows to start the client with disabled p2p network activity for testing or reindexing.
2020-07-22Merge #18044: Use wtxid for transaction relayWladimir J. van der Laan
0a4f1422cd1c20e12a05d7ff1a2ef1d5e7c654bb Further improve comments around recentRejects (Suhas Daftuar) 0e20cfedb704c1f76bb727e2009867d3d503a03d Disconnect peers sending wtxidrelay message after VERACK (Suhas Daftuar) cacd85209e20fc0fd08f86eed23b6ef93484ffcf test: Use wtxid relay generally in functional tests (Fabian Jahr) 8d8099e97ab8af2126f6fbd223fbd82c52f2e85e test: Add tests for wtxid tx relay in segwit test (Fabian Jahr) 9a5392fdf67f1c5c90f52d3cdb3dea4f35d1609f test: Update test framework p2p protocol version to 70016 (Fabian Jahr) dd78d1d641178b473ab1156b71a837b9e686792b Rename AddInventoryKnown() to AddKnownTx() (Suhas Daftuar) 4eb515574e1012bc8ea5dafc3042dcdf4c766f26 Make TX_WITNESS_STRIPPED its own rejection reason (Suhas Daftuar) 97141ca442daea8fc9c307cf81a02b38dcc28fd8 Delay getdata requests from peers using txid-based relay (Suhas Daftuar) 46d78d47dea345329ba094310eec56ab00a02ddc Add p2p message "wtxidrelay" (Suhas Daftuar) 2d282e0cba9761574b6b43d134ca95f3052d7fd2 ignore non-wtxidrelay compliant invs (Anthony Towns) ac88e2eb619821ad7ae1d45d4b40be69051d3999 Add support for tx-relay via wtxid (Suhas Daftuar) 8e68fc246d09f1e6c6dfa8c676969d97c2eb4334 Add wtxids to recentRejects instead of txids (Suhas Daftuar) 144c38582042c3b9ec53bb97ba0644fc0114f8fb Add wtxids of confirmed transactions to bloom filter (Suhas Daftuar) 85c78d54af462996a0bca6cf97f91e1aa8778ae8 Add wtxid-index to orphan map (Suhas Daftuar) 08b39955ec7f84e835ab0b1366f0dd28dfd6ce03 Add a wtxid-index to mapRelay (Suhas Daftuar) 60f0acda713e7b9dc188aef54ef93981a93f4e44 Just pass a hash to AddInventoryKnown (Suhas Daftuar) c7eb6b4f1fe5bd76388a023529977674534334a7 Add wtxid to mempool unbroadcast tracking (Amiti Uttarwar) 2b4b90aa8f0440deacefb5997d7bd1f9f5c591b3 Add a wtxid-index to the mempool (Suhas Daftuar) Pull request description: Using txids (a transaction's hash, without witness) for transaction relay is problematic, post-segwit -- if a peer gives us a segwit transaction that fails policy checks, it could be because the txid associated with the transaction is definitely unacceptable to our node (regardless of the witness), or it could be that the transaction was malleated and with a different witness, the txid could be accepted to our mempool. We have a bloom filter of recently rejected transactions, whose purpose is to help us avoid redownloading and revalidating transactions that fail to be accepted, but because of this potential for witness malleability to interfere with relay of valid transactions, we do not use the filter for segwit transactions. This issue is discussed at some length in #8279. The effect of this is that whenever a segwit transaction that fails policy checks is relayed, a node would download that transaction from every peer announcing it, because it has no way presently to cache failure. Historically this hasn't been a big problem, but if/when policy for accepting segwit transactions were to change (eg taproot, or any other change), we could expect older nodes talking to newer nodes to be wasting bandwidth because of this. As discussed in that issue, switching to wtxid-based relay solves this problem -- by using an identifier for a transaction that commits to all the data in our relay protocol, we can be certain if a transaction that a peer is announcing is one that we've already tried to process, or if it's something new. This PR introduces support for wtxid-based relay with peers that support it (and remains backwards compatible with peers that use txids for relay, of course). Apart from code correctness, one issue to be aware of is that by downloading from old and new peers alike, we should expect there to be some bandwidth wasted, because sometimes we might download the same transaction via txid-relay as well as wtxid-relay. The last commit in this PR implements a heuristic I want to analyze, which is to just delay relay from txid-relay peers by 2 seconds, if we have at least 1 wtxid-based peer. I've just started running a couple nodes with this heuristic so I can measure how well it works, but I'm open to other ideas for minimizing that issue. In the long run, I think this will be essentially a non-issue, so I don't think it's too big a concern, we just need to bite the bullet and deal with it during upgrade. Finally, this proposal would need a simple BIP describing the changes, which I haven't yet drafted. However, review and testing of this code in the interim would be welcome. To do items: - [x] Write BIP explaining the spec here (1 new p2p message for negotiating wtxid-based relay, along with a new INV type) - [ ] Measure and evaluate a heuristic for minimizing how often a node downloads the same transaction twice, when connected to old and new nodes. ACKs for top commit: naumenkogs: utACK 0a4f1422cd1c20e12a05d7ff1a2ef1d5e7c654bb laanwj: utACK 0a4f1422cd1c20e12a05d7ff1a2ef1d5e7c654bb Tree-SHA512: d8eb8f0688cf0cbe9507bf738e143edab1f595551fdfeddc2b6734686ea26e7f156b6bfde38bad8bbbe8bec1857c7223e1687f8f018de7463dde8ecaa8f450df
2020-07-21Merge #19217: p2p: disambiguate block-relay-only variable names from ↵MarcoFalke
blocksonly variables ec4c6a17e82a6726d95075b43ebd2525c50b37cd scripted-diff: replace MAX_BLOCKS_ONLY_CONNECTIONS with MAX_BLOCK_RELAY_ONLY_CONNECTIONS (glowang) Pull request description: We have two different concepts that have similar names: `-blocksonly` and `block-relay-only`, and the similarity of names could lead to confusion. `-blocksonly` disables all local receiving & relaying of transactions (with a few exceptions), while `block-relay-only`means that bitcoind will make 2 additional outbound connections that are only used for block relay. In net.h and init.cpp, `MAX_BLOCKS_ONLY_CONNECTIONS` is used to represent the maximum number of `block-relay-only` outbound peers, which is 2. But this name sounds ambiguous, and I proposed a better name, `MAX_BLOCK_RELAY_ONLY_CONNECTION`. ACKs for top commit: jnewbery: ACK ec4c6a17e82a6726d95075b43ebd2525c50b37cd Tree-SHA512: cfa592a7ff936f14d10cfc1e926a51b82bc0feaf104885a41ca8111b906cb3d1ec5536bab143a3cfca70aa49e9575c6995941eb6d3d7f4018d4535712342f155
2020-07-19Rename AddInventoryKnown() to AddKnownTx()Suhas Daftuar
2020-07-18Just pass a hash to AddInventoryKnownSuhas Daftuar
Since it's only used for transactions, there's no need to pass in an inv type.
2020-07-14[net/net processing] check banman pointer before dereferencingJohn Newbery
Although we currently don't do this, it should be possible to create a CConnman or PeerLogicValidation without a Banman instance. Therefore always check that banman exists before dereferencing the pointer. Also add comments to the m_banman members of CConnman and PeerLogicValidation to document that these may be nullptr.
2020-07-10Merge #14033: p2p: Drop CADDR_TIME_VERSION checks now that ↵MarcoFalke
MIN_PEER_PROTO_VERSION is greater 57b0c0a93a243769beb306c89560d1eda61f54bd Drop CADDR_TIME_VERSION checks now that MIN_PEER_PROTO_VERSION is greater (Ben Woosley) Pull request description: We do not connect to peers older than 31800 ACKs for top commit: sipa: Code reivew ACK 57b0c0a93a243769beb306c89560d1eda61f54bd jnewbery: Code review ACK 57b0c0a93a243769beb306c89560d1eda61f54bd vasild: ACK 57b0c0a9 Tree-SHA512: e1ca7c9203cbad83ab7c7a2312777ad07ed6a16119169b256648b8a8738c260a5168acdd4fb33f6e4b17f51ec7e033e110b76bde55b4e3b2d444dc02c01bc2b1
2020-07-10Merge #18638: net: Use mockable time for ping/pong, add testsMarcoFalke
fa3365430c5fb57d7c0b5f2bce9fbbe290be93c3 net: Use mockable time for ping/pong, add tests (MarcoFalke) faab4aaf2fa1153c6d76efc8113fa01b06943ece util: Add count_microseconds helper (MarcoFalke) Pull request description: Switch `CNode::m_ping_start` and `CNetMessage::m_time` to mockable time, so that tests can be added. Mockable time is also type-safe, since it uses `std::chrono` ACKs for top commit: jonatack: Code review re-ACK fa33654 re-read code, verified rebase per `git range-diff 4b5c919 fa94d6f fa33654`, previous tested ACKs still valid troygiorshev: ACK fa3365430c5fb57d7c0b5f2bce9fbbe290be93c3 Tree-SHA512: 7d632bd6019ce7c882029e71b667a61517e783af82755a85dd979ef09380934e172dec8b8f91d57b200a30a6e096aeaf01f19fee7f3aed0e0e871c72eb44d70e
2020-07-09Merge #19314: refactor: Use uint16_t instead of unsigned shortWladimir J. van der Laan
1cabbddbca615b26aa4510c75f459c28d6fe0afd refactor: Use uint16_t instead of unsigned short (Aaron Hook) Pull request description: I wanted to see if the `up for grabs` label works and looked at PR #17822 originally opend by ahook I saw it had many acks for example by jonatack and practicalswift but needed rebasing. So I checked out the remote branch rebased it resolved three conflicts and continued the rebase. Hope everything is as expected (: ACKs for top commit: sipsorcery: ACK 1cabbddbca615b26aa4510c75f459c28d6fe0afd. practicalswift: ACK 1cabbddbca615b26aa4510c75f459c28d6fe0afd -- patch looks correct :) laanwj: ACK 1cabbddbca615b26aa4510c75f459c28d6fe0afd hebasto: ACK 1cabbddbca615b26aa4510c75f459c28d6fe0afd, I have reviewed the code and it looks OK, I agree it can be merged. Tree-SHA512: 0e6bf64f274aae5dacb188358b4d5f65ccb207d4f70922f039bc4ed7934709418ddad19f8bfb7462517427837c3d2bb3f86ef284bb40e87119aad2a1e148d9d6
2020-06-23[net] Make cs_inventory a non-recursive mutexJohn Newbery
cs_inventory is never taken recursively. Make it a non-recursive mutex.
2020-06-23[net processing] Remove PushBlockInventory and PushBlockHashJohn Newbery
PushBlockInventory() and PushBlockHash() are functions that can be replaced with single-line statements. This also eliminates the single place that cs_inventory is taken recursively.
2020-06-23Drop CADDR_TIME_VERSION checks now that MIN_PEER_PROTO_VERSION is greaterBen Woosley
2020-06-22refactor: Use uint16_t instead of unsigned shortAaron Hook
removed trailing whitespace to make linter happy
2020-06-19net: Use mockable time for ping/pong, add testsMarcoFalke
2020-06-19Merge #18937: refactor: s/command/msg_type/ in CNetMsgMaker and ↵MarcoFalke
CSerializedNetMsg 51e9393c1f6c9eaac554f821f5327f63bd09c8cf refactor: s/command/msg_type/ in CNetMsgMaker and CSerializedNetMsg (Sebastian Falbesoner) Pull request description: Follow-up PR for #18533 -- another small step towards getting rid of the confusing "command" terminology. Also see PR #18610 which tackled the functional tests. ACKs for top commit: MarcoFalke: ACK 51e9393c1f6c9eaac554f821f5327f63bd09c8cf Tree-SHA512: bb6f05a7be6823d5c4eab1d05b31fee944e700946827ad9425d59a3957fd879776c88c606319cbe9832d9451b275baedf913b71429ea3e01e4e82bf2d419e819
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-09scripted-diff: replace MAX_BLOCKS_ONLY_CONNECTIONS with ↵glowang
MAX_BLOCK_RELAY_ONLY_CONNECTIONS -BEGIN VERIFY SCRIPT- sed -i 's/\<MAX_BLOCKS_ONLY_CONNECTIONS\>/MAX_BLOCK_RELAY_ONLY_CONNECTIONS/g' src/init.cpp sed -i 's/\<MAX_BLOCKS_ONLY_CONNECTIONS\>/MAX_BLOCK_RELAY_ONLY_CONNECTIONS/g' src/net.h -END VERIFY SCRIPT-
2020-05-27Merge #16127: More thread safety annotation coverageMarcoFalke
5478d6c099e76fe070703cc5383cba7b91468b0f logging: thread safety annotations (Anthony Towns) e685ca19928eec4e687c66f5edfcfff085a42c27 util/system.cpp: add thread safety annotations for dir_locks (Anthony Towns) a7887899480db72328784009181d93904e6d479d test/checkqueue_tests: thread safety annotations (Anthony Towns) 479c5846f7477625ec275fbb8a076c7ef157172b rpc/blockchain.cpp: thread safety annotations for latestblock (Anthony Towns) 8b5af3d4c1270267ad85e78f661bf8fab06f3aad net: fMsgProcWake use LOCK instead of lock_guard (Anthony Towns) de7c5f41aba860751ef7824245e6d9d5088a1200 wallet/wallet.h: Remove mutexScanning which was only protecting a single atomic bool (Anthony Towns) c3cf2f55013c4ea1c1ef4a878fc7ff8e92f2c42d rpc/blockchain.cpp: Remove g_utxosetscan mutex that is only protecting a single atomic variable (Anthony Towns) Pull request description: In a few cases we need to use `std::mutex` rather than the sync.h primitives. But `std::lock_guard<std::mutex>` doesn't include the clang thread safety annotations unless you also use clang's C library, which means you can't indicate when variables should be guarded by `std::mutex` mutexes. This adds an annotated version of `std::lock_guard<std::mutex>` to threadsafety.h to fix that, and modifies places where `std::mutex` is used to take advantage of the annotations. It's based on top of #16112, and turns the thread safety comments included there into annotations. It also changes the RAII classes in wallet/wallet.h and rpc/blockchain.cpp to just use the atomic<bool> flag for synchronisation rather than having a mutex that doesn't actually guard anything as well. ACKs for top commit: MarcoFalke: ACK 5478d6c099e76fe070703cc5383cba7b91468b0f 🗾 hebasto: re-ACK 5478d6c099e76fe070703cc5383cba7b91468b0f, only renamed s/`MutexGuard`/`LockGuard`/, and dropped the commit "test/util_threadnames_tests: add thread safety annotations" since the [previous](https://github.com/bitcoin/bitcoin/pull/16127#pullrequestreview-414184113) review. ryanofsky: Code review ACK 5478d6c099e76fe070703cc5383cba7b91468b0f. Thanks for taking suggestions! Only changes since last review are dropping thread rename test commit d53072ec730d8eec5a5b72f7e65a54b141e62b19 and renaming mutex guard to lock guard Tree-SHA512: 7b00d31f6f2b5a222ec69431eb810a74abf0542db3a65d1bbad54e354c40df2857ec89c00b4a5e466c81ba223267ca95f3f98d5fbc1a1d052a2c3a7d2209790a
2020-05-19net: fMsgProcWake use LOCK instead of lock_guardAnthony Towns
2020-05-12p2p: add MAX_FEELER_CONNECTIONS constanttryphe
2020-05-11refactor: s/command/msg_type/ in CNetMsgMaker and CSerializedNetMsgSebastian Falbesoner
2020-05-05Make ThreadSafe{MessageBox|Question} bilingualHennadii Stepanov
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.
generated by cgit v1.2.3 (git 2.26.2) at 2024-09-06 15:37:54 +0000