faf1af58f8 fuzz: Add Temporary debug assert for oss-fuzz issue (MarcoFalke)
Pull request description:
oss-fuzz is acting weird, so add an earlier assert to help troubleshooting
ACKs for top commit:
practicalswift:
cr ACK faf1af58f8
Tree-SHA512: 85830d7d47cf6b4edfe91a07bd5aa8f7110db0bade8df93868cf276ed04d5dd17e671f769e6a0fb5092012b86aa82bb411fb171411f15746981104ce634c88c1
The unit test is single threaded, so there's no need to hold the mutex
between Good() and Attempt().
This change avoids recursive locking in the CAddrMan::Attempt function.
Co-authored-by: John Newbery <john@johnnewbery.com>
1b1088d52f test: add combined I2P/onion/localhost eviction protection tests (Jon Atack)
7c2284eda2 test: add tests for inbound eviction protection of I2P peers (Jon Atack)
ce02dd1ef1 p2p: extend inbound eviction protection by network to I2P peers (Jon Atack)
70bbc62711 test: add combined onion/localhost eviction protection coverage (Jon Atack)
045cb40192 p2p: remove unused m_is_onion member from NodeEvictionCandidate struct (Jon Atack)
310fab4928 p2p: remove unused CompareLocalHostTimeConnected() (Jon Atack)
9e889e8a5c p2p: remove unused CompareOnionTimeConnected() (Jon Atack)
787d46bb2a p2p: update ProtectEvictionCandidatesByRatio() doxygen docs (Jon Atack)
1e15acf478 p2p: make ProtectEvictionCandidatesByRatio() fully ratio-based (Jon Atack)
3f8105c4d2 test: remove combined onion/localhost eviction protection tests (Jon Atack)
38a81a8e20 p2p: add CompareNodeNetworkTime() comparator struct (Jon Atack)
4ee7aec47e p2p: add m_network to NodeEvictionCandidate struct (Jon Atack)
7321e6f2fe p2p, refactor: rename vEvictionCandidates to eviction_candidates (Jon Atack)
ec590f1d91 p2p, refactor: improve constness in ProtectEvictionCandidatesByRatio() (Jon Atack)
4a19f501ab test: add ALL_NETWORKS to test utilities (Jon Atack)
519e76bb64 test: speed up and simplify peer_eviction_test (Jon Atack)
1cde800523 p2p, refactor: rm redundant erase_size calculation in SelectNodeToEvict() (Jon Atack)
Pull request description:
Continuing the work in #20197 and #20685, this pull updates and abstracts our inbound eviction protection to make it fully ratio-based and easily extensible to peers connected via high-latency privacy networks that we newly support, like I2P and perhaps others soon, as these peers are disadvantaged by the latency criteria of our eviction logic.
It then adds eviction protection for peers connected over I2P. As described in https://github.com/bitcoin/bitcoin/pull/20685#issuecomment-767486499, we've observed over the past few months that I2P peers have a min ping latency similar to or greater than that of onion peers.
The algorithm is a basically a multi-pass knapsack:
- Count the number of eviction candidates in each of the disadvantaged
privacy networks.
- Sort the networks from lower to higher candidate counts, so that
a network with fewer candidates will have the first opportunity
for any unused slots remaining from the previous iteration. In
the case of a tie in candidate counts, priority is given by array
member order from first to last, guesstimated to favor more unusual
networks.
- Iterate through the networks in this order. On each iteration,
allocate each network an equal number of protected slots targeting
a total number of candidates to protect, provided any slots remain
in the knapsack.
- Protect the candidates in that network having the longest uptime,
if any in that network are present.
- Continue iterating as long as we have non-allocated slots
remaining and candidates available to protect.
The goal of this logic is to favorise the diversity of our peer connections.
The individual commit messages describe each change in more detail.
Special thank you to Vasil Dimov for the excellent review feedback and the algorithm improvement that made this change much better than it would have been otherwise. Thanks also to Antoine Riard, whose review feedback nudged this change to protect disadvantaged networks having fewer, rather than more, eviction candidates.
ACKs for top commit:
laanwj:
Code review re-ACK 1b1088d52f
vasild:
ACK 1b1088d52f
Tree-SHA512: 722f790ff11f2969c79e45a5e0e938d94df78df8687e77002f32e3ef5c72a9ac10ebf8c7a9eb7f71882c97ab0e67b2778191effdb747d9ca54d7c23c2ed19a90
This commit extends our inbound eviction protection to I2P peers to
favorise the diversity of peer connections, as peers connected
through the I2P network are otherwise disadvantaged by our eviction
criteria for their higher latency (higher min ping times) relative
to IPv4 and IPv6 peers, as well as relative to Tor onion peers.
The `networks` array is order-dependent in the case of a tie in
candidate counts between networks (earlier array members receive
priority in the case of a tie).
Therefore, we place I2P candidates before localhost and onion ones
in terms of opportunity to recover unused remaining protected slots
from the previous iteration, guesstimating that most nodes allowing
both onion and I2P inbounds will have more onion peers, followed by
localhost, then I2P, as I2P support is only being added in the
upcoming v22.0 release.
with a more abstract framework to allow easily extending inbound
eviction protection to peers connected through new higher-latency
networks that are disadvantaged by our inbound eviction criteria,
such as I2P and perhaps other BIP155 networks in the future like
CJDNS. This is a change in behavior.
The algorithm is a basically a multi-pass knapsack:
- Count the number of eviction candidates in each of the disadvantaged
privacy networks.
- Sort the networks from lower to higher candidate counts, so that
a network with fewer candidates will have the first opportunity
for any unused slots remaining from the previous iteration. In
the case of a tie in candidate counts, priority is given by array
member order from first to last, guesstimated to favor more unusual
networks.
- Iterate through the networks in this order. On each iteration,
allocate each network an equal number of protected slots targeting
a total number of candidates to protect, provided any slots remain
in the knapsack.
- Protect the candidates in that network having the longest uptime,
if any in that network are present.
- Continue iterating as long as we have non-allocated slots
remaining and candidates available to protect.
Localhost peers are treated as a network like Tor or I2P by aliasing
them to an unused Network enumerator: Network::NET_MAX.
The goal is to favorise diversity of our inbound connections.
Credit to Vasil Dimov for improving the algorithm from single-pass
to multi-pass to better allocate unused protection slots.
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
as we are about the change the behavior sufficiently that when we
have multiple disadvantaged networks and a small number of peers
under test, the number of protected peers per network can be different.
This speeds up the test significantly, which helps when
running it repeatedly.
Suggest reviewing the diff with:
colorMoved = dimmed-zebra
colorMovedWs = allow-indentation-change
6f994882de validation: Farewell, global Chainstate! (Carl Dong)
972c5166ee qt/test: Reset chainman in ~ChainstateManager instead (Carl Dong)
6c3b5dc0c1 scripted-diff: tree-wide: Remove all review-only assertions (Carl Dong)
3e82abb8dd tree-wide: Remove stray review-only assertion (Carl Dong)
f323248aba qt/test: Use existing chainman in ::TestGUI (can be scripted-diff) (Carl Dong)
6c15de129c scripted-diff: wallet/test: Use existing chainman (Carl Dong)
ee0ab1e959 fuzz: Initialize a TestingSetup for test_one_input (Carl Dong)
0d61634c06 scripted-diff: test: Use existing chainman in unit tests (Carl Dong)
e197076219 test: Pass in CoinsTip to ValidateCheckInputsForAllFlags (Carl Dong)
4d99b61014 test/miner_tests: Pass in chain tip to CreateBlockIndex (Carl Dong)
f0dd5e6bb4 test/util: Use existing chainman in ::PrepareBlock (Carl Dong)
464c313e30 init: Use existing chainman (Carl Dong)
Pull request description:
Based on: #21767
à la Mr. Sandman
```
Mr. Chainman, bring me a tip (bung, bung, bung, bung)
Make it the most work that I've ever seen (bung, bung, bung, bung)
Rewind old tip till we're at the fork point (bung, bung, bung, bung)
Then tell it that it's time to call Con-nectTip
Chainman, I'm so alone (bung, bung, bung, bung)
No local objects to call my own (bung, bung, bung, bung)
Please make sure I have a ref
Mr. Chainman, bring me a tip!
```
This is the last bundle in the #20158 series. Thanks everyone for their diligent review.
I would like to call attention to https://github.com/bitcoin/bitcoin/issues/21766, where a few leftover improvements were collated.
- Remove globals:
- `ChainstateManager g_chainman`
- `CChainState& ChainstateActive()`
- `CChain& ChainActive()`
- Remove all review-only assertions.
ACKs for top commit:
jamesob:
reACK 6f994882de based on the contents of
ariard:
Code Review ACK 6f99488.
jnewbery:
utACK 6f994882de
achow101:
Code Review ACK 6f994882de
ryanofsky:
Code review ACK 6f994882de.
Tree-SHA512: 4052ea79360cf0efd81ad0ee3f982e1d93aab1837dcec75f875a56ceda085de078bb3099a2137935d7cc2222004ad88da94b605ef5efef35cb6bc733725debe6
493fb47c57 Make SetupServerArgs callable without NodeContext (Russell Yanofsky)
Pull request description:
`bitcoin-gui` code needs to call `SetupServerArgs` but will not have a `NodeContext` object if it is communicating with an external `bitcoin-node` process, so this just passes `ArgsManager` directly.
---
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10). The commit was first part of larger PR #10102.
ACKs for top commit:
MarcoFalke:
review ACK 493fb47c57
Tree-SHA512: 94cda4350113237976e32f1935e3602d1e6ea90c29c4434db2094be70dddf4b63702c3094385258bdf1c3e5b52c7d23bbc1f0282bdd4965557eedd5aef9a0fd4
fa72fce7c9 test: Use ConnmanTestMsg from test lib in denialofservice_tests (MarcoFalke)
Pull request description:
This allows to remove code.
Also, required for https://github.com/bitcoin/bitcoin/pull/18470
ACKs for top commit:
mjdietzx:
crACK fa72fce7c9👍👍
fanquake:
ACK fa72fce7c9
Tree-SHA512: 12aa68cde697c0f7c25d60bb0c02783e5462eb3ba39947b0d94a7798bc278e7d5f092f3ab2a3d0547947c3502cde7c4a599419055a57f78ef1f70f9f637e14c7
e.g.:
In file included from /usr/local/include/boost/test/test_tools.hpp:46:
/usr/local/include/boost/test/tools/old/impl.hpp:107:17: error: comparison of integers of different signs: 'const unsigned int' and 'const int' [-Werror,-Wsign-compare]
return left == right;
~~~~ ^ ~~~~~
/usr/local/include/boost/test/tools/old/impl.hpp:130:16: note: in instantiation of function template specialization 'boost::test_tools::tt_detail::equal_impl<unsigned int, int>' requested here
return equal_impl( left, right );
^
/usr/local/include/boost/test/tools/old/impl.hpp:145:16: note: in instantiation of function template specialization 'boost::test_tools::tt_detail::equal_impl_frwd::call_impl<unsigned int, int>' requested here
return call_impl( left, right, left_is_array() );
^
/usr/local/include/boost/test/tools/old/impl.hpp:92:50: note: in instantiation of function template specialization 'boost::test_tools::tt_detail::equal_impl_frwd::operator()<unsigned int, int>' requested here
BOOST_PP_REPEAT( BOOST_TEST_MAX_PREDICATE_ARITY, IMPL_FRWD, _ )
^
/usr/local/include/boost/preprocessor/repetition/repeat.hpp:30:26: note: expanded from macro 'BOOST_PP_REPEAT'
^
/usr/local/include/boost/preprocessor/cat.hpp:22:32: note: expanded from macro 'BOOST_PP_CAT'
^
/usr/local/include/boost/preprocessor/cat.hpp:29:34: note: expanded from macro 'BOOST_PP_CAT_I'
^
<scratch space>:153:1: note: expanded from here
BOOST_PP_REPEAT_1
^
test/streams_tests.cpp:122:5: note: in instantiation of function template specialization 'boost::test_tools::tt_detail::check_frwd<boost::test_tools::tt_detail::equal_impl_frwd, unsigned int, int>' requested here
BOOST_CHECK_EQUAL(varint, 54321);
^
/usr/local/include/boost/test/tools/old/impl.hpp:107:17: error: comparison of integers of different signs: 'const unsigned long long' and 'const long' [-Werror,-Wsign-compare]
return left == right;
~~~~ ^ ~~~~~
/usr/local/include/boost/test/tools/old/impl.hpp:130:16: note: in instantiation of function template specialization 'boost::test_tools::tt_detail::equal_impl<unsigned long long, long>' requested here
return equal_impl( left, right );
^
/usr/local/include/boost/test/tools/old/impl.hpp:145:16: note: in instantiation of function template specialization 'boost::test_tools::tt_detail::equal_impl_frwd::call_impl<unsigned long long, long>' requested here
return call_impl( left, right, left_is_array() );
^
/usr/local/include/boost/test/tools/old/impl.hpp:92:50: note: in instantiation of function template specialization 'boost::test_tools::tt_detail::equal_impl_frwd::operator()<unsigned long long, long>' requested here
BOOST_PP_REPEAT( BOOST_TEST_MAX_PREDICATE_ARITY, IMPL_FRWD, _ )
^
/usr/local/include/boost/preprocessor/repetition/repeat.hpp:30:26: note: expanded from macro 'BOOST_PP_REPEAT'
^
/usr/local/include/boost/preprocessor/cat.hpp:22:32: note: expanded from macro 'BOOST_PP_CAT'
^
/usr/local/include/boost/preprocessor/cat.hpp:29:34: note: expanded from macro 'BOOST_PP_CAT_I'
^
<scratch space>:161:1: note: expanded from here
BOOST_PP_REPEAT_1
^
test/serfloat_tests.cpp:41:5: note: in instantiation of function template specialization 'boost::test_tools::tt_detail::check_frwd<boost::test_tools::tt_detail::equal_impl_frwd, unsigned long long, long>' requested here
BOOST_CHECK_EQUAL(TestDouble(std::numeric_limits<double>::infinity()), 0x7ff0000000000000);
^
The Tor v2 addresses, left over from when Tor v2 was supported will be
unserialized as a dummy, invalid `::` (all zeros) IPv6 address. Remove
them so that they do not take up space in addrman.
The return type is already enforced to be void by the
ternary operator:
./test/fuzz/util.h:47:25: error: right operand to ? is void, but left operand is of type *OTHER_TYPE*
((i++ == call_index ? callables() : void()), ...);
^ ~~~~~~~~~~~ ~~~~~~
3737d35fee fuzz: Terminate immediately if a fuzzing harness ever tries to perform a DNS lookup (belts and suspenders) (practicalswift)
Pull request description:
Terminate immediately if a fuzzing harness tries to perform a DNS lookup (belt and suspenders).
Obviously this _should_ never happen, but if it _does_ happen we want immediate termination instead of a DNS lookup :)
ACKs for top commit:
MarcoFalke:
review ACK 3737d35fee
Tree-SHA512: 51cd2d32def7f9f052e02f99c354656af1f807cc9fdf592ab765e620bfe660f1ed26e0484763f94aba650424b44959eafaf352bfd0f81aa273e350510e97356e
2667366aaa tests: check derivation of P2TR (Pieter Wuille)
7cedafc541 Add tr() descriptor (derivation only, no signing) (Pieter Wuille)
90fcac365e Add TaprootBuilder class (Pieter Wuille)
5f6cc8daa8 Add XOnlyPubKey::CreateTapTweak (Pieter Wuille)
2fbfb1becb Make consensus checking of tweaks in pubkey.* Taproot-specific (Pieter Wuille)
a4bf84039c Separate WitnessV1Taproot variant in CTxDestination (Pieter Wuille)
41839bdb89 Avoid dependence on CTxDestination index order (Pieter Wuille)
31df02a070 Change Solver() output for WITNESS_V1_TAPROOT (Pieter Wuille)
4b1cc08f9f Make XOnlyPubKey act like byte container (Pieter Wuille)
Pull request description:
This is a subset of #21365, to aide review.
This adds support `tr(KEY)` or `tr(KEY,SCRIPT)` or `tr(KEY,{{S1,{{S2,S3},...}},...})` descriptors, describing Taproot outputs with specified internal key, and optionally any number of scripts, in nested groups of 2 inside `{`/`}` if there are more than one. While it permits importing `tr(KEY)`, anything beyond that is just laying foundations for more features later.
Missing:
* Signing support (see #21365)
* Support for more interesting scripts inside the tree (only `pk(KEY)` is supported for now). In particular, a multisig policy based on the new `OP_CHECKSIGADD` opcode would be very useful.
* Inferring `tr()` descriptors from outputs (given sufficient information).
* `getaddressinfo` support.
* MuSig support. Standardizing that is still an ongoing effort, and is generally kind of useless without corresponding PSBT support.
* Convenient ways of constructing descriptors without spendable internal key (especially ones that arent't trivially recognizable as such).
ACKs for top commit:
Sjors:
utACK 2667366 (based on https://github.com/bitcoin/bitcoin/pull/21365#issuecomment-846945215 review, plus the new functional test)
achow101:
Code Review ACK 2667366aaa
lsilva01:
Tested ACK 2667366aaa
meshcollider:
utACK 2667366aaa
Tree-SHA512: 61046fef22c561228338cb178422f0b782ef6587ec8208d3ce2bd07afcff29a664b54b35c6b01226eb70b6540b43f6dd245043d09aa6cb6db1381b6042667e75
5d82a57db4 contrib: remove torv2 seed nodes (Jon Atack)
5f7e086dac contrib: update generate-seeds.py to ignore torv2 addresses (Jon Atack)
8be56f0f8e p2p, refactor: extract OnionToString() from CNetAddr::ToStringIp() (Jon Atack)
5f9d3c09b4 p2p: remove torv2 from CNetAddr::ToStringIP() (Jon Atack)
3d39042144 p2p: remove torv2 in SetIP() and ADDR_TORV2_SIZE constant (Jon Atack)
cff5ec477a p2p: remove pre-addrv2 onions from SerializeV1Array() (Jon Atack)
4192a74413 p2p: ignore torv2-in-ipv6 addresses in SetLegacyIPv6() (Jon Atack)
1d631e956f p2p: remove BIP155Network::TORV2 from GetBIP155Network() (Jon Atack)
7d1769bc45 p2p: remove torv2 from SetNetFromBIP155Network() (Jon Atack)
eba9a94b9f fuzz: rename CNetAddr/CService deserialize targets (Jon Atack)
c56a1c9b18 p2p: drop onions from IsAddrV1Compatible(), no longer relay torv2 (Jon Atack)
f8e94002fc p2p: remove torv2/ADDR_TORV2_SIZE from SetTor() (Jon Atack)
0f1c58ae87 test: update feature_proxy to torv3 (Jon Atack)
Pull request description:
This patch removes support in Bitcoin Core for Tor v2 onions, which are already removed from the release of Tor 0.4.6.
- no longer serialize/deserialize and relay Tor v2 addresses
- ignore incoming Tor v2 addresses
- remove Tor v2 addresses from the addrman and peers.dat on node launch
- update generate-seeds.py to ignore Tor v2 addresses
- remove Tor v2 hard-coded seeds
Tested with tor-0.4.6.1-alpha (no v2 support) and 0.4.5.7 (v2 support). With the latest Tor (no v2 support), this removes all the warnings like those reported with current master in https://github.com/bitcoin/bitcoin/issues/21351
```
<bitcoind debug log>
Socks5() connect to […].onion:8333 failed: general failure
<tor log>
Invalid hostname [scrubbed]; rejecting
```
and the addrman no longer has Tor v2 addresses on launching bitcoind.
```rake
$ ./src/bitcoin-cli -addrinfo
{
"addresses_known": {
"ipv4": 44483,
"ipv6": 8467,
"torv2": 0,
"torv3": 2296,
"i2p": 6,
"total": 55252
}
}
```
After recompiling back to current master and restarting with either of the two Tor versions (0.4.5.7 or 0.4.6.1), -addrinfo initially returns 0 Tor v2 addresses and then begins finding them again.
Ran nodes on this patch over the past week on mainnet/testnet/signet/regtest after building with DEBUG_ADDRMAN.
Verified that this patch bootstraps an onlynet=onion node from the Tor v3 hardcoded fixed seeds on mainnet and testnet and connects to blocks and v3 onion peers: `rm ~/.bitcoin/testnet3/peers.dat ; ./src/bitcoind -testnet -dnsseed=0 -onlynet=onion`
Tested using `addnode`, `getaddednodeinfo`,`addpeeraddress`, `disconnectnode` and `-addrinfo` that a currently valid, connectable Tor v2 peer can no longer be added:
Thanks to Vasil Dimov, Carl Dong, and Wladimir J. van der Laan for their work on BIP155 and Tor v3 that got us here.
ACKs for top commit:
laanwj:
Code review ACK 5d82a57db4
Tree-SHA512: 590ff3d2f6ef682608596facb4b01f44fef69716d2ab3552ae1655aa225f4bf104f9ee08d6769abb9982a8031de93340df553279ce1f5023771f9f2b651178bb
37371268d1 Mark `CheckTxInputs` `[[nodiscard]]` (out-param `txfee` only set if call is successful). Avoid UUM in fuzzing harness `coins_view`. (practicalswift)
Pull request description:
Mark `CheckTxInputs` `[[nodiscard]]` (out-param `txfee` only set if call is successful).
Avoid use of uninitialised memory (UUM) in fuzzing harness `coins_view`.
ACKs for top commit:
MarcoFalke:
review ACK 37371268d1
Tree-SHA512: edada5b2e80ce9ad3bd57b4c445bedefffa0a2d1cc880957d6848e4b7d9fc1ce036cd17f8b18bc03a36fbf84fc29c166cd6ac3dfbfe03e69d6fdbda13697754d
6c3fcd5591 test: remove BasicTestingSetup from util_threadnames unit tests (fanquake)
b53d3c1b1f test: remove BasicTestingSetup from uint256 unit tests (fanquake)
c0497a4928 test: remove BasicTestingSetup from torcontrol unit tests (fanquake)
ef8bb0473b test: remove BasicTestingSetup from sync unit tests (fanquake)
1aee83421f test: remove BasicTestingSetup from reverse_lock unit tests (fanquake)
57ba949ef5 test: remove BasicTestingSetup from policy_fee unit tests (fanquake)
3974c962b6 test: remove BasicTestingSetup from merkleblock tests (fanquake)
cd5bc4b470 test: remove BasicTestingSetup from hash unit tests (fanquake)
39cec22935 test: remove BasicTestingSetup from compilerbug unit tests (fanquake)
6d3b78c0e2 test: remove BasicTestingSetup from bswap unit tests (fanquake)
a13dc24831 test: remove BasicTestingSetup from bech32 unit tests (fanquake)
f4dcbe4498 test: remove BasicTestingSetup from base64 unit tests (fanquake)
fd144f6426 test: remove BasicTestingSetup from base32 unit tests (fanquake)
4c389ba04b test: remove BasicTestingSetup from arith_uint256 unit tests (fanquake)
05590651a0 test: remove BasicTestingSetup from amount unit tests (fanquake)
883a5c7d02 test: remove BasicTestingSetup from allocator unit tests (fanquake)
Pull request description:
* Less setup/overhead for tests that don't need it. Some naive bench-marking would suggest that a full `test_bitcoin` run is a few % faster after this change.
* Tests which don't need the BasicTestingSetup can't accidentally end up depending on it somehow.
* Already the case in at least the scheduler and block_filter tests.
This adds missing includes, but more significant is the removal of `setup_common.h` from tests where it isn't needed. This saves recompiling those tests when changes are made in the header.
ACKs for top commit:
practicalswift:
cr ACK 6c3fcd5591: patch looks correct
laanwj:
ACK 6c3fcd5591
Tree-SHA512: 69b891e2b4740402d62b86a4fc98c329a432d125971342a6f97334e166b3537ed3d4cdbb2531fa05c1feae32339c9fcb2dceda9afeeaed4edc70e8caa0962161
7a799c9c2b index: refactor-only: Reuse CChain ref (Carl Dong)
db33cde80f index: Add chainstate member to BaseIndex (Carl Dong)
f4a47a1feb bench: Use existing chainman in AssembleBlock (Carl Dong)
91226eb917 bench: Use existing NodeContext in DuplicateInputs (Carl Dong)
e6b4aa6eb5 miner: Pass in chainman to RegenerateCommitments (Carl Dong)
9ecade1425 rest: Add GetChainman function and use it (Carl Dong)
fc1c282845 rpc/blockchain: Use existing blockman in gettxoutsetinfo (Carl Dong)
Pull request description:
Overall PR: #20158 (tree-wide: De-globalize ChainstateManager)
The first 2 commits are fixups addressing review for the last bundle: #21391
NEW note:
1. I have opened #21766 which keeps track of potential improvements where the flaws already existed before the de-globalization work, please post on that issue about these improvements, thanks!
Note to reviewers:
1. This bundle may _apparently_ introduce usage of `g_chainman` or `::Chain(state|)Active()` globals, but these are resolved later on in the overall PR. [Commits of overall PR](https://github.com/bitcoin/bitcoin/pull/20158/commits)
2. There may be seemingly obvious local references to `ChainstateManager` or other validation objects which are not being used in callers of the current function in question, this is done intentionally to **_keep each commit centered around one function/method_** to ease review and to make the overall change systematic. We don't assume anything about our callers. Rest assured that once we are considering that particular caller in later commits, we will use the obvious local references. [Commits of overall PR](https://github.com/bitcoin/bitcoin/pull/20158/commits)
3. When changing a function/method that has many callers (e.g. `LookupBlockIndex` with 55 callers), it is sometimes easier (and less error-prone) to use a scripted-diff. When doing so, there will be 3 commits in sequence so that every commit compiles like so:
1. Add `new_function`, make `old_function` a wrapper of `new_function`, divert all calls to `old_function` to `new_function` **in the local module only**
2. Scripted-diff to divert all calls to `old_function` to `new_function` **in the rest of the codebase**
3. Remove `old_function`
ACKs for top commit:
jarolrod:
ACK 7a799c9
ariard:
Code Review ACK 7a799c9
fjahr:
re-ACK 7a799c9c2b
MarcoFalke:
review ACK 7a799c9c2b🌠
ryanofsky:
Code review ACK 7a799c9c2b. Basically no change since last review except fixed rebase conflicts and a new comment about REST Ensure()
jamesob:
conditional ACK 7a799c9c2b ([`jamesob/ackr/21767.1.dongcarl.bundle_6_n_prune_g_chai`](https://github.com/jamesob/bitcoin/tree/ackr/21767.1.dongcarl.bundle_6_n_prune_g_chai))
Tree-SHA512: 531c00ddcb318817457db2812d9a9d930bc664e58e6f7f1c746350732b031dd624270bfa6b9f49d8056aeb6321d973f0e38e4ff914acd6768edd8602c017d10e
ffff0d0442 refactor: Switch serialize to uint8_t (1/n) (MarcoFalke)
Pull request description:
Replace `char` -> `uint8_t` in serialization where a sign doesn't make sense (char might be signed/unsigned).
ACKs for top commit:
practicalswift:
cr ACK ffff0d0442: patch looks correct and commit hash is ffffresh (was bbbbadass)
kristapsk:
ACK ffff0d0442
Tree-SHA512: cda682280c21d37cc3a6abd62569732079b31d18df3f157aa28bed80bd6f9f29a7db5c133b1f57b3a8f8d5ba181a76e473763c6e26a2df6d9244813f56f893ee
