Change getheaders messages so that we wait up to 2 minutes for a response to a
prior getheaders message before issuing a new one.
Also change the handling of the getheaders message sent in response to a block
INV, so that we no longer use the hashstop variable (including the hash stop
will just mean that if our peer's headers chain is longer, then we won't learn
it, so there's no benefit to using hashstop).
Also, now respond to a getheaders during IBD with an empty headers message
(rather than nothing) -- this better conforms to the intent of the new logic
that it's better to not ignore a peer's getheaders message, even if you have
nothing to give. This also avoids a lot of functional tests breaking.
p2p_segwit.py is modified to use this same strategy, as the test logic (of
expecting a getheaders after a block inv) would otherwise be broken.
Also moves the call to happen directly after validation of a headers message
(rather than mixed in with other state updates for the peer), and removes an
incorrect comment in favor of one that explains why headers sync must continue
from the last header a peer has sent.
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
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 fa8aa0aa81
Tree-SHA512: e60a6b317f2b826f9e0724285d00b632d3e2a91ded9fa5ba01c80766c5d39270b719be234c01302d46eaba600910032693836aa116ff05ee1b590c7530881cd3
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`](b4f686952a/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`](b4f686952a/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
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
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
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
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
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
`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.
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 2b3373c152
Tree-SHA512: 8a4bb9641af8d79824ec12e2d6bfce0e09524094b298a2edcdb2ab115fbaa21215b9c97a6b059f8aa023551071fd5798eca66ab8d262a3f97246a91d960850d0
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 fa1b76aeb0
Tree-SHA512: 1abff5cba4b4871d97f17dbcdf67bc9255ff21fa4150a79a74e39b28f0610eab3e7dee24d56872dd6e111f003b55e288958cdd467e6218368d896f191e4ec9cd
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
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
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
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
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)
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.
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).
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).
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
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.
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
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.
Subsequent commits will remove support for other versions of compact blocks.
Add a test that a received `sendcmpct` message with version = 1 is
ignored.
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
