aboutsummaryrefslogtreecommitdiff
path: root/src/net_processing.cpp
AgeCommit message (Collapse)Author
2022-06-29Merge bitcoin/bitcoin#25290: [kernel 3a/n] Decouple `CTxMemPool` from ↵MacroFake
`ArgsManager` d1684beabe5b738c2cc83de83e1aaef11a761b69 fees: Pass in a filepath instead of referencing gArgs (Carl Dong) 9a3d825c30e8e6118d74a4e568744cb9d03f7f5d init: Remove redundant -*mempool*, -limit* queries (Carl Dong) 6c5c60c4124293d948735756f84efc85262ea66f mempool: Use m_limit for UpdateTransactionsFromBlock (Carl Dong) 9e93b1030182eff92ef91181e17c7dd498c7e164 node/ifaces: Use existing MemPoolLimits (Carl Dong) 38af2bcf358a72b9457d370282e57f4be1c5c849 mempoolaccept: Use limits from mempool in constructor (Carl Dong) 9333427014695ac235c96d48791098168dfdc9db mempool: Introduce (still-unused) MemPoolLimits (Carl Dong) 716bb5fbd31077bbe99d11a54d6c2c250afc8085 scripted-diff: Rename anc/desc size limit vars to indicate SI unit (Carl Dong) 1ecc77321deb61b9f6888e4e10752b9d972fd26e scripted-diff: Rename DEFAULT_MEMPOOL_EXPIRY to indicate time unit (Carl Dong) aa9141cd8185cb7ad532bc16feb9d302b05d9697 mempool: Pass in -mempoolexpiry instead of referencing gArgs (Carl Dong) 51c7a41a5eb6fcb60333812c770d80227cf7b64d init: Only determine maxmempool once (Carl Dong) 386c9472c8764738282e6d163b42e15a8feda7ea mempool: Make GetMinFee() with custom size protected (Carl Dong) 82f00de7a6a60cbc9ad0c6e1d0ffb1bc70c49af5 mempool: Pass in -maxmempool instead of referencing gArgs (Carl Dong) f1941e8bfd2eecc478c7660434b1ebf6a64095a0 pool: Add and use MemPoolOptions, ApplyArgsManOptions (Carl Dong) 0199bd35bb44e32ee0db9b51c9d1bd7518c26f19 fuzz/rbf: Add missing TestingSetup (Carl Dong) ccbaf546a68d6cda8ed3efd0598c0e4121b366bb scripted-diff: Rename DEFAULT_MAX_MEMPOOL_SIZE to indicate SI unit (Carl Dong) fc02f77ca604f0221171bfde3059b34f5d0fb1cd ArgsMan: Add Get*Arg functions returning optional (Carl Dong) Pull request description: This is part of the `libbitcoinkernel` project: #24303, https://github.com/bitcoin/bitcoin/projects/18 ----- As mentioned in the Stage 1 Step 2 description of [the `libbitcoinkernel` project](https://github.com/bitcoin/bitcoin/issues/24303), `ArgsManager` will not be part of `libbitcoinkernel`. Therefore, it is important that we remove any dependence on `ArgsManager` by code that will be part of `libbitcoinkernel`. This is the first in a series of PRs aiming to achieve this. This PR removes `CTxMemPool+MempoolAccept`'s dependency on `ArgsManager` by introducing a `CTxMemPool::Options` struct, which is used to specify `CTxMemPool`'s various options at construction time. These options are: - `-maxmempool` -> `CTxMemPool::Options::max_size` - `-mempoolexpiry` -> `CTxMemPool::Options::expiry` - `-limitancestorcount` -> `CTxMemPool::Options::limits::ancestor_count` - `-limitancestorsize` -> `CTxMemPool::Options::limits::ancestor_size` - `-limitdescendantcount` -> `CTxMemPool::Options::limits::descendant_count` - `-limitdescendantsize` -> `CTxMemPool::Options::limits::descendant_size` More context can be gleaned from the commit messages. The important commits are: - 56eb479ded8bfb2ef635bb6f3b484f9d5952c70d "pool: Add and use MemPoolOptions, ApplyArgsManOptions" - a1e08b70f3068f4e8def1c630d8f50cd54da7832 "mempool: Pass in -maxmempool instead of referencing gArgs" - 6f4bf3ede5812b374828f08fc728ceded2f10024 "mempool: Pass in -mempoolexpiry instead of referencing gArgs" - 5958a7fe4806599fc620ee8c1a881ca10fa2dd16 "mempool: Introduce (still-unused) MemPoolLimits" Reviewers: Help needed in the following commits (see commit messages): - a1e08b70f3068f4e8def1c630d8f50cd54da7832 "mempool: Pass in -maxmempool instead of referencing gArgs" - 0695081a797e9a5d7787b78b0f8289dafcc6bff7 "node/ifaces: Use existing MemPoolLimits" Note to Reviewers: There are perhaps an infinite number of ways to architect `CTxMemPool::Options`, the current one tries to keep it simple, usable, and flexible. I hope we don't spend too much time arguing over the design here since that's not the point. In the case that you're 100% certain that a different design is strictly better than this one in every regard, please show us a fully-implemented branch. ----- TODO: - [x] Use the more ergonomic `CTxMemPool::Options` where appropriate - [x] Doxygen comments for `ApplyArgsManOptions`, `MemPoolOptions` ----- Questions for Reviewers: 1. Should we use `std::chrono::seconds` for `CTxMemPool::Options::expiry` and `CTxMemPool::m_expiry` instead of an `int64_t`? Something else? (`std::chrono::hours`?) 2. Should I merge `CTxMemPool::Limits` inside `CTxMemPool::Options`? ACKs for top commit: MarcoFalke: ACK d1684beabe5b738c2cc83de83e1aaef11a761b69 🍜 ryanofsky: Code review ACK d1684beabe5b738c2cc83de83e1aaef11a761b69. Just minor cleanups since last review, mostly switching to brace initialization Tree-SHA512: 2c138e52d69f61c263f1c3648f01c801338a8f576762c815f478ef5148b8b2f51e91ded5c1be915e678c0b14f6cfba894b82afec58d999d39a7bb7c914736e0b
2022-06-28mempool: Pass in -maxmempool instead of referencing gArgsCarl Dong
- Store the mempool size limit (-maxmempool) in CTxMemPool as a member. - Remove the requirement to explicitly specify a mempool size limit for CTxMemPool::GetMinFee(...) and LimitMempoolSize(...), just use the stored mempool size limit where possible. - Remove all now-unnecessary instances of: gArgs.GetIntArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE_MB) * 1000000 The code change in CChainState::GetCoinsCacheSizeState() is correct since the coinscache should not repurpose "extra" mempool memory headroom for itself if the mempool doesn't even exist.
2022-06-28Merge bitcoin/bitcoin#25480: Replace CountSecondsDouble with ↵fanquake
Ticks<SecondsDouble> fa956e7508986991008e2f6126ab307924b3f353 Replace CountSecondsDouble with Ticks<SecondsDouble> (MacroFake) Pull request description: Seems odd to have two ways to say exactly the same thing when one is sufficient. ACKs for top commit: fanquake: ACK fa956e7508986991008e2f6126ab307924b3f353 shaavan: ACK fa956e7508986991008e2f6126ab307924b3f353 w0xlt: ACK https://github.com/bitcoin/bitcoin/pull/25480/commits/fa956e7508986991008e2f6126ab307924b3f353 Tree-SHA512: b599470e19b693da1ed1102d1e86b08cb03adaddf2048752b6d050fdf86055be117ff0ae10b6953d03e00eaaf7b0cfa350137968b67d6c5b3ca68c5aa50ca6aa
2022-06-27Merge bitcoin/bitcoin#25404: p2p, doc: Use MAX_BLOCKS_TO_ANNOUNCE consistentlyfanquake
e357c8953880715a943b892deed04e7777187999 p2p, doc: Use MAX_BLOCKS_TO_ANNOUNCE consistently (Martin Zumsande) Pull request description: Block announcements via headers may have up to `MAX_BLOCKS_TO_ANNOUNCE = 8` entries according to the definition of this constant. However, there are a few spots saying they should have a size _less than_ `MAX_BLOCKS_TO_ANNOUNCE`. Fix these. I don't think that this is critical (this only changes behavior when we get a headers announcement with exactly `MAX_BLOCKS_TO_ANNOUNCE` blocks which we can't connect), but it would be nice to handle this limit consistently. ACKs for top commit: dergoegge: utACK e357c8953880715a943b892deed04e7777187999 - This PR makes the usage and docs of `MAX_BLOCKS_TO_ANNOUNCE` consistent with its description. Tree-SHA512: f3772026ab0f402e3a551127ef6e4a98fa9e7af250715fe317c05988b5b33f2f3e098a00e03960d4d28c8bd2b7a97231f7f99f22f1c152c000b2e27b658cf8f2
2022-06-27Merge bitcoin/bitcoin#25144: refactor: Pass Peer& to Misbehaving()MacroFake
fa8aa0aa8180c3a0369c7589b8747666778a0deb Pass Peer& to Misbehaving() (MacroFake) Pull request description: `Misbehaving` has several coding related issues (ignoring the conceptual issues here for now): * It is public, but it is not supposed to be called from outside of net_processing. Fix that by making it private and creating a public `UnitTestMisbehaving` method for unit testing only. * It doesn't do anything if a `nullptr` is passed. It would be less confusing to just skip the call instead. Fix that by passing `Peer&` to `Misbehaving()`. * It calls `GetPeerRef`, causing `!m_peer_mutex` lock annotations to be propagated. This is harmless, but verbose. Fix it by removing the no longer needed call to `GetPeerRef` and the no longer needed lock annotations. ACKs for top commit: vasild: ACK fa8aa0aa8180c3a0369c7589b8747666778a0deb w0xlt: Code Review ACK https://github.com/bitcoin/bitcoin/pull/25144/commits/fa8aa0aa8180c3a0369c7589b8747666778a0deb Tree-SHA512: e60a6b317f2b826f9e0724285d00b632d3e2a91ded9fa5ba01c80766c5d39270b719be234c01302d46eaba600910032693836aa116ff05ee1b590c7530881cd3
2022-06-27Replace CountSecondsDouble with Ticks<SecondsDouble>MacroFake
2022-06-22scripted-diff: Rename DEFAULT_MAX_MEMPOOL_SIZE to indicate SI unitCarl Dong
Better to be explicit when it comes to sizes to avoid unintentional bugs. We use MB and KB all over the place. -BEGIN VERIFY SCRIPT- find_regex="DEFAULT_MAX_MEMPOOL_SIZE" \ && git grep -l -E "$find_regex" \ | xargs sed -i -E "s@$find_regex@\0_MB@g" -END VERIFY SCRIPT-
2022-06-22[net processing] Set CNode::m_relays_txs=true when receiving BIP37 filtersdergoegge
This line was accidentally removed in #22778.
2022-06-17p2p, doc: Use MAX_BLOCKS_TO_ANNOUNCE consistentlyMartin Zumsande
It is an inclusive upper bound according to its definition.
2022-06-16Merge bitcoin/bitcoin#25215: [kernel 2d/n] Reduce CTxMemPool constructor ↵laanwj
call sites d273e53b6e2cabd91a83f0ff0f9b6cfe1815b637 bench/rpc_mempool: Create ChainTestingSetup, use its CTxMemPool (Carl Dong) 020caba3df727ae8ede50eace86ae76971c0fea1 bench: Use existing CTxMemPool in TestingSetup (Carl Dong) 86e732def3983f99ec84b59375615bedcdc0e664 scripted-diff: test: Use CTxMemPool in TestingSetup (Carl Dong) 213457e170ce41a0b26c644aa010111df36414a6 test/policyestimator: Use ChainTestingSetup's CTxMemPool (Carl Dong) 319f0ceeeb25f28e027fc41be2755092dc5365b4 rest/getutxos: Don't construct empty mempool (Carl Dong) 03574b956a274207ba90591781e0914609225136 tree-wide: clang-format CTxMemPool references (Carl Dong) Pull request description: This is part of the `libbitcoinkernel` project: #24303, https://github.com/bitcoin/bitcoin/projects/18 This PR reduces the number of call sites where we explicitly construct CTxMemPool. This is done in preparation for later PRs which decouple the mempool module from `ArgsManager`, eventually all of libbitcoinkernel will be decoupled from `ArgsManager`. The changes in this PR: - Allows us to have less code churn as we modify `CTxMemPool`'s constructor in later PRs - In many cases, we can make use of existing `CTxMemPool` instances, getting rid of extraneous constructions - In other cases, we construct a `ChainTestingSetup` and use the `CTxMemPool` there, so that we can rely on the logic in `setup_common` to set things up correctly ## Notes for Reviewers ### A note on using existing mempools When evaluating whether or not it's appropriate to use an existing mempool in a `*TestingSetup` struct, the key is to make sure that the mempool has the same lifetime as the `*TestingSetup` struct. Example 1: In [`src/fuzz/tx_pool.cpp`](https://github.com/bitcoin/bitcoin/blob/b4f686952a60bbadc7ed2250651d0d6af0959f4d/src/test/fuzz/tx_pool.cpp), the `TestingSetup` is initialized in `initialize_tx_pool` and lives as a static global, while the `CTxMemPool` is in the `tx_pool_standard` fuzz target, meaning that each time the `tx_pool_standard` fuzz target gets run, a new `CTxMemPool` is created. If we were to use the static global `TestingSetup`'s CTxMemPool we might run into problems since its `CTxMemPool` will carry state between subsequent runs. This is why we don't modify `src/fuzz/tx_pool.cpp` in this PR. Example 2: In [`src/bench/mempool_eviction.cpp`](https://github.com/bitcoin/bitcoin/blob/b4f686952a60bbadc7ed2250651d0d6af0959f4d/src/bench/mempool_eviction.cpp), we see that the `TestingSetup` is in the same scope as the constructed `CTxMemPool`, so it is safe to use its `CTxMemPool`. ### A note on checking `CTxMemPool` ctor call sites After the "tree-wide: clang-format CTxMemPool references" commit, you can find all `CTxMemPool` ctor call sites with the following command: ```sh git grep -E -e 'make_unique<CTxMemPool>' \ -e '\bCTxMemPool\s+[^({;]+[({]' \ -e '\bCTxMemPool\s+[^;]+;' \ -e '\bnew\s+CTxMemPool\b' ``` At the end of the PR, you will find that there are still quite a few call sites that we can seemingly get rid of: ```sh $ git grep -E -e 'make_unique<CTxMemPool>' -e '\bCTxMemPool\s+[^({;]+[({]' -e '\bCTxMemPool\s+[^;]+;' -e '\bnew\s+CTxMemPool\b' # rearranged for easier explication src/init.cpp: node.mempool = std::make_unique<CTxMemPool>(node.fee_estimator.get(), mempool_check_ratio); src/test/util/setup_common.cpp: m_node.mempool = std::make_unique<CTxMemPool>(m_node.fee_estimator.get(), 1); src/rpc/mining.cpp: CTxMemPool empty_mempool; src/test/util/setup_common.cpp: CTxMemPool empty_pool; src/bench/mempool_stress.cpp: CTxMemPool pool; src/bench/mempool_stress.cpp: CTxMemPool pool; src/test/fuzz/rbf.cpp: CTxMemPool pool; src/test/fuzz/tx_pool.cpp: CTxMemPool tx_pool_{/*estimator=*/nullptr, /*check_ratio=*/1}; src/test/fuzz/tx_pool.cpp: CTxMemPool tx_pool_{/*estimator=*/nullptr, /*check_ratio=*/1}; src/test/fuzz/validation_load_mempool.cpp: CTxMemPool pool{}; src/txmempool.h: /** Create a new CTxMemPool. ``` Let's break them down one by one: ``` src/init.cpp: node.mempool = std::make_unique<CTxMemPool>(node.fee_estimator.get(), mempool_check_ratio); src/test/util/setup_common.cpp: m_node.mempool = std::make_unique<CTxMemPool>(m_node.fee_estimator.get(), 1); ``` Necessary ----- ``` src/rpc/mining.cpp: CTxMemPool empty_mempool; src/test/util/setup_common.cpp: CTxMemPool empty_pool; ``` These are fixed in #25223 where we stop requiring the `BlockAssembler` to have a `CTxMemPool` if it's not going to consult it anyway (as is the case in these two call sites) ----- ``` src/bench/mempool_stress.cpp: CTxMemPool pool; src/bench/mempool_stress.cpp: CTxMemPool pool; ``` Fixed in #24927. ----- ``` src/test/fuzz/rbf.cpp: CTxMemPool pool; src/test/fuzz/tx_pool.cpp: CTxMemPool tx_pool_{/*estimator=*/nullptr, /*check_ratio=*/1}; src/test/fuzz/tx_pool.cpp: CTxMemPool tx_pool_{/*estimator=*/nullptr, /*check_ratio=*/1}; src/test/fuzz/validation_load_mempool.cpp: CTxMemPool pool{}; ``` These are all cases where we don't want the `CTxMemPool` state to persist between runs, see the previous section "A note on using existing mempools" ----- ``` src/txmempool.h: /** Create a new CTxMemPool. ``` It's a comment (someone link me to a grep that understands syntax plz thx) ACKs for top commit: laanwj: Code review ACK d273e53b6e2cabd91a83f0ff0f9b6cfe1815b637 Tree-SHA512: c4ff3d23217a7cc4a7145defc7b901725073ef73bcac3a252ed75f672c87e98ca0368d1d8c3f606b5b49f641e7d8387d26ef802141b650b215876f191fb6d5f9
2022-06-14refactor: Introduce PeerManagerImpl::RejectIncomingTxsMacroFake
Currently there are some confusions in net_processing: * There is confusion between `-blocksonly mode` and `block-relay-only`, so adjust all comments to use the same nomenclature. * Whether to disconnect peers for providing invs/txs is implemented differently. For example, it seems a bit confusing to disconnect `block-relay-only` peers with `relay` permission when they send a tx message, but not when they send an inv message. Also, keeping track of their inv announcements seems both wasteful and confusing, as it does nothing. This isn't possible in practice, as outbound connections do not have permissions assigned, but sees fragile to rely on. Especially in light of proposed changes to make that possible: https://github.com/bitcoin/bitcoin/pull/17167
2022-06-10Merge bitcoin/bitcoin#24931: Strengthen thread safety assertionsMacroFake
ce893c0497fc9b8ab9752153dfcc77c9f427545e doc: Update developer notes (Anthony Towns) d2852917eecad6ab422a7b2c9892d351a7f0cc96 sync.h: Imply negative assertions when calling LOCK (Anthony Towns) bba87c0553780eacf0317fbfec7330ea27aa02f8 scripted-diff: Convert global Mutexes to GlobalMutexes (Anthony Towns) a559509a0b8cade27199740212d7b589f71a0e3b sync.h: Add GlobalMutex type (Anthony Towns) be6aa72f9f8d50b6b5b19b319a74abe7ab4099ff qt/clientmodel: thread safety annotation for m_cached_tip_mutex (Anthony Towns) f24bd45b37e1b2d19e5a053dbfefa30306c1d41a net_processing: thread safety annotation for m_tx_relay_mutex (Anthony Towns) Pull request description: This changes `LOCK(mutex)` for non-global, non-recursive mutexes to be annotated with the negative capability for the mutex it refers to, to prevent . clang applies negative capabilities recursively, so this helps avoid forgetting to annotate functions. This can't reasonably be used for globals, because clang would require every function to be annotated with `EXCLUSIVE_LOCKS_REQUIRED(!g_mutex)` for each global mutex; so this introduces a trivial `GlobalMutex` subclass of `Mutex`, and reduces the annotations for both `GlobalMutex` to `LOCKS_EXCLUDED` which only catches trivial errors (eg (`LOCK(x); LOCK(x);`). ACKs for top commit: MarcoFalke: review ACK ce893c0497fc9b8ab9752153dfcc77c9f427545e 🐦 hebasto: ACK ce893c0497fc9b8ab9752153dfcc77c9f427545e Tree-SHA512: 5c35e8c7677ce3d994a7e3774f4344adad496223a51b3a1d1d3b5f20684b2e1d5cff688eb3fbc8d33e1b9940dfa76e515f9434e21de6f3ce3c935e29a319f529
2022-06-07Merge bitcoin/bitcoin#25254: Move minRelayTxFee to policy/settingsMacroFake
fa4068b4e2192f168bb120624eca5735f0dadf6f Move minRelayTxFee to policy/settings (MacroFake) Pull request description: Seems a bit confusing to put policy stuff into validation, so fix that. Also fix includes via `iwyu`. ACKs for top commit: ariard: ACK fa4068b, the includes move compiles well locally. ryanofsky: Code review ACK fa4068b4e2192f168bb120624eca5735f0dadf6f. Make sense to move the global variable to policy/settings and the default constant to policy/policy. Ariard points out other constants that could be moved, which seems fine, but it seems like moving the global variable to be with other related global variables is more significant. Tree-SHA512: adf9619002610d1877f3aef0a9e6115fc4c2ad64135a3e5100824c650b560c47f47ac28894c6214a50a7888355252a9f6f7cec98c23a771a1964160ef1ca77de
2022-06-02Merge bitcoin/bitcoin#24171: p2p: Sync chain more readily from inbound peers ↵laanwj
during IBD 48262a00f58489d705314ee3c31136133040bb0e Add functional test for block sync from inbound peers (Suhas Daftuar) 0569b5c4bbf8f725e3969d76f7cb081cdf1e4195 Sync chain more readily from inbound peers during IBD (Suhas Daftuar) Pull request description: When in IBD, if the honest chain is only known by inbound peers, then we must eventually sync from them in order to learn it. This change allows us to perform initial headers sync and fetch blocks from inbound peers, if we have no blocks in flight. The restriction on having no blocks in flight means that we will naturally throttle our block downloads to any such inbound peers that we may be downloading from, until we leave IBD. This is a tradeoff between preferring outbound peers for most of our block download, versus making sure we always eventually will get blocks we need that are only known by inbound peers even during IBD, as otherwise we may be stuck in IBD indefinitely (which could have cascading failure on the network, if a large fraction of the network managed to get stuck in IBD). Note that the test in the second commit fails on master, without the first commit. ACKs for top commit: ajtowns: ACK 48262a00f58489d705314ee3c31136133040bb0e sipa: ACK 48262a00f58489d705314ee3c31136133040bb0e Tree-SHA512: ffad3a05fa9a32a92226843c9128f52c275e8d51930fde7368badc340227f2ed680561c4c9f2937b4e3bd722474464849ec9b624f912f5e380ce98d71b55764d
2022-05-31Move minRelayTxFee to policy/settingsMacroFake
Also fix includes using iwyu
2022-05-31Merge bitcoin/bitcoin#24178: p2p: Respond to getheaders if we have ↵MacroFake
sufficient chainwork a35f963edf1a14ee572abea106fb0147575e4694 Add test for getheaders behavior (Suhas Daftuar) ef6dbe6863d92710fd2da7781e5b2aac87578751 Respond to getheaders if we have sufficient chainwork (Suhas Daftuar) Pull request description: Previously, we would check to see if we were in IBD and ignore getheaders requests accordingly. However, the IBD criteria -- an optimization mostly targeted at behavior when we have peers serving us many blocks we need to download -- is difficult to reason about in edge-case scenarios, such as if the network were to go a long time without any blocks found and nodes are getting restarted during that time. To make things simpler to reason about, just use `nMinimumChainWork` as our anti-DoS threshold for responding to a getheaders request; as long as our chain has that much work, it should be fine to respond to a peer asking for our headers (and this should allow such a peer to request blocks from us if needed). ACKs for top commit: klementtan: crACK a35f963edf1a14ee572abea106fb0147575e4694 naumenkogs: ACK a35f963edf1a14ee572abea106fb0147575e4694 MarcoFalke: review ACK a35f963edf1a14ee572abea106fb0147575e4694 🗯 Tree-SHA512: 131e3872e7fe80382ea9c1ec202d6c2dc59c006355c69000aa3f4ce6bccd02a6c689c8cb8f3542b5d9bc48bfa61edcbd1a78535c0b79018971d02bed2655d284
2022-05-27Pass Peer& to Misbehaving()MacroFake
`Misbehaving` has several coding related issues (ignoring the conceptual issues here for now): * It is public, but it is not supposed to be called from outside of net_processing. Fix that by making it private and creating a public `UnitTestMisbehaving` method for unit testing only. * It doesn't do anything if a `nullptr` is passed. It would be less confusing to just skip the call instead. Fix that by passing `Peer&` to `Misbehaving()`. * It calls `GetPeerRef`, causing `!m_peer_mutex` lock annotations to be propagated. This is harmless, but verbose. Fix it by removing the no longer needed call to `GetPeerRef` and the no longer needed lock annotations.
2022-05-25tree-wide: clang-format CTxMemPool referencesCarl Dong
[META] Do this so that we can more easily grep for all actual instances of CTxMemPool construction.
2022-05-20Merge bitcoin/bitcoin#25175: refactor: Improve thread safety analysis by ↵MacroFake
propagating some negative capabilities 2b3373c1520aa0b41277cd89956224e08cbd79dd refactor: Propagate negative `!m_tx_relay_mutex` capability (Hennadii Stepanov) 5a6e3c1db3e9d8ee2ecb0f0fe2fba073a442ad76 refactor: Propagate negative `!m_most_recent_block_mutex` capability (Hennadii Stepanov) Pull request description: This PR is a follow-up for bitcoin/bitcoin#22778 and bitcoin/bitcoin#24062, and it seems [required](https://github.com/bitcoin/bitcoin/pull/24931#issuecomment-1132800173) for bitcoin/bitcoin#24931. See details in the commit messages. ACKs for top commit: ajtowns: ACK 2b3373c1520aa0b41277cd89956224e08cbd79dd w0xlt: ACK https://github.com/bitcoin/bitcoin/pull/25175/commits/2b3373c1520aa0b41277cd89956224e08cbd79dd Tree-SHA512: 8a4bb9641af8d79824ec12e2d6bfce0e09524094b298a2edcdb2ab115fbaa21215b9c97a6b059f8aa023551071fd5798eca66ab8d262a3f97246a91d960850d0
2022-05-21net_processing: thread safety annotation for m_tx_relay_mutexAnthony Towns
2022-05-20Merge bitcoin/bitcoin#25168: refactor: Avoid passing params where not neededMacroFake
fa1b76aeb064b315a3767a8f59836ca18aeb117e Do not call global Params() when chainman is in scope (MacroFake) fa30234be81b6f49ae8150478a9255daa1611083 Do not pass CChainParams& to PeerManager::make (MacroFake) fafe5c0ca2927642cbcec63ac73994737e1653d6 Do not pass CChainParams& to BlockAssembler constructor (MacroFake) faf012b438b451dced785e7f031e07c0c55665e1 Do not pass Consensus::Params& to Chainstate helpers (MacroFake) fa4ee53dca5ccf1b87f019f372ffc10528add943 Do not pass time getter to Chainstate helpers (MacroFake) Pull request description: It seems confusing to pass chain params, consensus params, or a time function around when it is not needed. Fix this by: * Inlining the passed time getter function. I don't see a use case why this should be mockable. * Using `chainman.GetConsensus()` or `chainman.GetParams()`, where possible. ACKs for top commit: promag: Code review ACK fa1b76aeb064b315a3767a8f59836ca18aeb117e. vincenzopalazzo: ACK https://github.com/bitcoin/bitcoin/pull/25168/commits/fa1b76aeb064b315a3767a8f59836ca18aeb117e Tree-SHA512: 1abff5cba4b4871d97f17dbcdf67bc9255ff21fa4150a79a74e39b28f0610eab3e7dee24d56872dd6e111f003b55e288958cdd467e6218368d896f191e4ec9cd
2022-05-20refactor: Propagate negative `!m_tx_relay_mutex` capabilityHennadii Stepanov
Could be verified with $ ./configure CC=clang CXX=clang++ CXXFLAGS='-Wthread-safety -Wthread-safety-negative' $ make clean $ make 2>&1 | grep m_tx_relay_mutex
2022-05-20refactor: Propagate negative `!m_most_recent_block_mutex` capabilityHennadii Stepanov
Could be verified with $ ./configure CC=clang CXX=clang++ CXXFLAGS='-Wthread-safety -Wthread-safety-negative' $ make clean $ make 2>&1 | grep m_most_recent_block_mutex
2022-05-19Merge bitcoin/bitcoin#25147: Net processing: follow ups to #20799 (removing ↵fanquake
support for v1 compact blocks) bf6526f4a0ab30f77952ecdee90cd77dd9ba06f6 [test] Remove segwit argument from build_block_on_tip() (John Newbery) c65bf50b44a38bc55224d8967e0df7af60ea4f1b Remove fUseWTXID parameter from CBlockHeaderAndShortTxIDs constructor (John Newbery) Pull request description: This implements two of the suggestions from code reviews of PR 20799: - Remove fUseWTXID parameter from CBlockHeaderAndShortTxIDs constructor - Remove segwit argument from build_block_on_tip() ACKs for top commit: dergoegge: Code review ACK bf6526f4a0ab30f77952ecdee90cd77dd9ba06f6 naumenkogs: ACK bf6526f4a0ab30f77952ecdee90cd77dd9ba06f6 Tree-SHA512: d553791d1364b9e655183755e829b195c9b47f59c62371dbae49d9c0f8d84fec58cf18f4dde89591672ef5658e18c9cf0206c2efd70606980f87e506bc3bd4e5
2022-05-19Merge bitcoin/bitcoin#22778: net processing: Reduce resource usage for ↵fanquake
inbound block-relay-only connections 9db82f1bca0bb51c2180ca4a4ad63ba490e79da4 [net processing] Don't initialize TxRelay for non-tx-relay peers. (John Newbery) b0a4ac9c26f60fd4993d89f45cafffaa389db2d4 [net processing] Add m_tx_relay_mutex to protect m_tx_relay ptr (John Newbery) 290a8dab0288344fa5731ec2ffd09478e9420a2f [net processing] Comment all TxRelay members (John Newbery) 42e3250497b03478d61cd6bfe6cd904de73d57b1 [net processing] [refactor] Move m_next_send_feefilter and m_fee_filter_sent (John Newbery) Pull request description: block-relay-only connections are additional outbound connections that bitcoind makes since v0.19. They participate in block relay, but do not propagate transactions or addresses. They were introduced in #15759. When creating an outbound block-relay-only connection, since we know that we're never going to announce transactions over that connection, we can save on memory usage by not a `TxRelay` data structure for that connection. When receiving an inbound connection, we don't know whether the connection was opened by the peer as block-relay-only or not, and therefore we always construct a `TxRelay` data structure for inbound connections. However, it is possible to tell whether an inbound connection will ever request that we start announcing transactions to it. The `fRelay` field in the `version` message may be set to `0` to indicate that the peer does not wish to receive transaction announcements. The peer may later request that we start announcing transactions to it by sending a `filterload` or `filterclear` message, **but only if we have offered `NODE_BLOOM` services to that peer**. `NODE_BLOOM` services are disabled by default, and it has been recommended for some time that users not enable `NODE_BLOOM` services on public connections, for privacy and anti-DoS reasons. Therefore, if we have not offered `NODE_BLOOM` to the peer _and_ it has set `fRelay` to `0`, then we know that it will never request transaction announcements, and that we can save resources by not initializing the `TxRelay` data structure. ACKs for top commit: MarcoFalke: review ACK 9db82f1bca0bb51c2180ca4a4ad63ba490e79da4 🖖 dergoegge: Code review ACK 9db82f1bca0bb51c2180ca4a4ad63ba490e79da4 naumenkogs: ACK 9db82f1bca0bb51c2180ca4a4ad63ba490e79da4 Tree-SHA512: 83a449a56cd6bf6ad05369f5ab91516e51b8c471c07ae38c886d51461e942d492ca34ae63d329c46e56d96d0baf59a3e34233e4289868f911db3b567072bdc41
2022-05-18Do not pass CChainParams& to PeerManager::makeMacroFake
2022-05-18[net processing] Don't initialize TxRelay for non-tx-relay peers.John Newbery
Delay initializing the TxRelay data structure for a peer until we receive a version message from that peer. At that point we'll know whether it will ever relay transactions. We only initialize the m_tx_relay data structure if: - this isn't an outbound block-relay-only connection; AND - fRelay=true OR we're offering NODE_BLOOM to this peer (NODE_BLOOM means that the peer may turn on tx relay later)
2022-05-18[net processing] Add m_tx_relay_mutex to protect m_tx_relay ptrJohn Newbery
2022-05-18[net processing] Comment all TxRelay membersJohn Newbery
This fully comments all the TxRelay members. The only significant change is to the comment for m_relay_txs. Previously the comment stated that one of the purposes of the field was that "We don't relay tx invs before receiving the peer's version message". However, even without the m_relay_txs flag, we would not send transactions to the peer before receiving the `version` message, since SendMessages() returns immediately if fSuccessfullyConnected is not set to true, which only happens once a `version` and `verack` message have been received.
2022-05-18[net processing] [refactor] Move m_next_send_feefilter and m_fee_filter_sentJohn Newbery
Move m_next_send_feefilter and m_fee_filter_sent out of the `TxRelay` data structure. All of the other members of `TxRelay` are related to sending transactions _to_ the peer, whereas m_fee_filter_sent and m_next_send_feefilter are both related to receiving transactions _from_ the peer. A node's tx relay behaviour is not always symmetrical (eg a blocksonly node will ignore incoming transactions, but may still send out its own transactions), so it doesn't make sense to group the feefilter sending data with the TxRelay data in a single structure. This does not change behaviour, since IsBlockOnlyConn() is always equal to !peer.m_tx_relay. We still don't send feefilter messages to outbound block-relay-only peers (tested in p2p_feefilter.py).
2022-05-17Sync chain more readily from inbound peers during IBDSuhas Daftuar
When in IBD, if the honest chain is only known by inbound peers, then we must eventually sync from them in order to learn it. This change allows us to perform initial headers sync and fetch blocks from inbound peers, if we have no blocks in flight. The restriction on having no blocks in flight means that we will naturally throttle our block downloads to any such inbound peers that we may be downloading from, until we leave IBD. This is a tradeoff between preferring outbound peers for most of our block download, versus making sure we always eventually will get blocks we need that are only known by inbound peers even during IBD, as otherwise we may be stuck in IBD indefinitely (which could have cascading failure on the network, if a large fraction of the network managed to get stuck in IBD).
2022-05-17Remove fUseWTXID parameter from CBlockHeaderAndShortTxIDs constructorJohn Newbery
All uses of CBlockHeaderAndShortTxIDs in the product code are constructed with fUseWTXID=true, so remove the parameter. There is one use of the CBlockHeaderAndShortTxIDs constructor with fUseWTXID=false in the unit tests. This is used to construct a CBlockHeaderAndShortTxIDs for a block with only the coinbase transaction, so setting fUseWTXID to true or false makes no difference. Suggested in https://github.com/bitcoin/bitcoin/pull/20799#pullrequestreview-963480278
2022-05-16refactor: replace RecursiveMutex `m_most_recent_block_mutex` with MutexSebastian Falbesoner
In each of the critical sections, only the the guarded variables are accessed, without any chance that within one section another one is called. Hence, we can use an ordinary Mutex instead of RecursiveMutex.
2022-05-16refactor: reduce scope of lock `m_most_recent_block_mutex`Sebastian Falbesoner
This avoids calling the non-trivial method `CConnman::PushMessage` within the critical section.
2022-05-16Merge bitcoin/bitcoin#25109: Strengthen AssertLockNotHeld assertionsMacroFake
436ce0233c276e263dcb441255dc0b881cb39cfb sync.h: strengthen AssertLockNotHeld assertion (Anthony Towns) 7d73f58e9cea8f4b0bc16512983898fddde3d764 Increase threadsafety annotation coverage (Anthony Towns) Pull request description: This changes `AssertLockNotHeld` so that it is annotated with the negative capability for the mutex it refers to. clang applies negative capabilities recursively, so this helps avoid forgetting to annotate functions. Note that this can't reasonably be used for globals, because clang would require every function to be annotated with `EXCLUSIVE_LOCKS_REQUIRED(!g_mutex)` for each global mutex. At present, the only global mutexes that use `AssertLockNotHeld` are `RecursiveMutex` so we treat that as an exception in order to avoid having to add an excessive number of negative annotations. ACKs for top commit: vasild: ACK 436ce0233c276e263dcb441255dc0b881cb39cfb MarcoFalke: review ACK 436ce0233c276e263dcb441255dc0b881cb39cfb 🌺 Tree-SHA512: 5f16d098790a36b5277324d5ee89cdc87033c19b11c7943c2f630a41c2e3998eb39d356a763e857f4d8fefb6c0c02291f720bb6769bcbdf5e2cd765bf266ab8c
2022-05-15[net processing] Remove redundant nodestate->m_sendcmpct check in ↵John Newbery
MaybeSetPeerAsAnnouncingHeaderAndIDs()
2022-05-15[net processing] Only relay blocks by cmpctblock and cache for fast relay if ↵John Newbery
segwit is enabled This introduces an early exit in PeerManagerImpl::NewPoWValidBlock() if segwit has not been activated for the block. This means that we won't cache the block/compact block for fast relay and won't relay the cmpctblock immediately to peers that have requested hb compact blocks. This is fine because any block where segwit is not yet activated is buried deep in the chain, and so compact block relay will not be effective. It's ok not to cache the block/compact block for fast relay for the same reason - the block must be very deeply buried in the block chain. ProcessBlockAvailability() also won't get called for all nodes. This is also fine, since that function only updates hashLastUnknownBlock and pindexBestKnownBlock, and is called early in every SendMessages() call.
2022-05-15[net processing] Rename CNodeState compact block membersJohn Newbery
fPreferHeaderAndIDs -> m_requested_hb_cmpctblocks fProvidesHeaderAndIDs -> m_provides_cmpctblocks
2022-05-15[net processing] Tidy up `sendcmpct` processingJohn Newbery
- use better local variable names - drop unnecessary if statements
2022-05-15[net processing] fPreferHeaderAndIDs implies fProvidesHeaderAndIDsJohn Newbery
Remove all if(fProvidesHeaderAndIDs) conditionals inside if(fPreferHeaderAndIDs) conditionals.
2022-05-15[net processing] Remove fWantsCmpctWitnessJohn Newbery
It is now completely redundant with fProvidesHeadersAndIDs.
2022-05-15[net processing] Remove fSupportsDesiredCmpctVersionJohn Newbery
It is now completely redundant with fProvidesHeadersAndIDs.
2022-05-15[net processing] Simplify `sendcmpct` processingJohn Newbery
nCMPCTBLOCKVersion must always be 2 when processing.
2022-05-15[net processing] Only accept `sendcmpct` with version=2John Newbery
Subsequent commits will remove support for other versions of compact blocks. Add a test that a received `sendcmpct` message with version = 1 is ignored.
2022-05-15[net processing] Only advertise support for version 2 compact blocksJohn Newbery
Subsequent commits will remove support.
2022-05-13Merge bitcoin/bitcoin#24595: deploymentstatus: move g_versionbitscache ↵MacroFake
global to ChainstateManager bb5c24b120a3ac7df367a1c5d9b075ca564efb5f validation: move g_versionbitscache into ChainstateManager (Anthony Towns) eca22c726ac48b4216bb68cc0f0bbd655c43ac12 test/versionbits: make versionbitscache a parameter (Anthony Towns) d603f1d8a7cdc0a158ed80ade8a843b61b6ad08e deploymentstatus: make versionbitscache a parameter (Anthony Towns) 78adef17536edef833a0bfca06b61ce28120e486 refactor: use chainman instead of chainParams for DeploymentActive* (Anthony Towns) deffe0df6c36225bada18603b5a840139f030f2c deploymentstatus: allow chainman in place of consensusParams (Anthony Towns) eaa2e3f25cefbd1b9a1214102f88dbfa8109d244 validation: move UpdateUncommittedBlockStructures and GenerateCoinbaseCommitment into ChainstateManager (Anthony Towns) 5c67e84d37d452e9186a6357e5405fabeff241c7 validation: replace ::Params() calls with chainstate/chainman member (Anthony Towns) 38860f93b680f152fc6fc3d9ae574a4c0659e775 validation: remove redundant CChainParams params from ChainstateManager methods (Anthony Towns) 69675ea4e73dcf5e9dd0f94802bd3463e4262081 validation: add CChainParams to ChainstateManager (Anthony Towns) Pull request description: Gives `ChainstateManager` a reference to the `CChainParams` its working on, and simplifies some of the functions that would otherwise take that as a parameter. Removes the `g_versionbitscache` global by moving it into `ChainstateManager`. ACKs for top commit: dongcarl: reACK bb5c24b120a3ac7df367a1c5d9b075ca564efb5f MarcoFalke: review ACK bb5c24b120a3ac7df367a1c5d9b075ca564efb5f 📙 Tree-SHA512: 3fa74905e5df561e3e74bb0b8fce6085c5311e6633e7d74c0fb0c82a907f5bbb1fd4ebc5d11d4f0b1c019bb51eabb9f6e4bcc4652a696d36a5878c807b85f121
2022-05-12Merge bitcoin/bitcoin#24925: refactor: make GetRand a template, remove ↵MacroFake
GetRandInt ab1ea29ba1b8379a21fabd3dc859552c470a6421 refactor: make GetRand a template, remove GetRandInt (pasta) Pull request description: makes GetRand a template for which any integral type can be used, where the default behavior is to return a random integral up to the max of the integral unless a max is provided. This simplifies a lot of code from GetRand(std::numeric_limits<uint64_t>::max() -> GetRand<uint64_t>() ACKs for top commit: laanwj: Code review ACK ab1ea29ba1b8379a21fabd3dc859552c470a6421 Tree-SHA512: db5082a0e21783389f1be898ae73e097b31ab48cab1a2c0e29348a4adeb545d4098193aa72a547c6baa6e8205699aafec38d6a27b3d65522fb3246f91b4daae9
2022-05-12Increase threadsafety annotation coverageAnthony Towns
2022-05-10refactor: use chainman instead of chainParams for DeploymentActive*Anthony Towns
2022-05-10validation: remove redundant CChainParams params from ChainstateManager methodsAnthony Towns
generated by cgit v1.2.3 (git 2.26.2) at 2024-10-12 10:30:10 +0000