1afc0e4aa1 doc: remove potentially confusing ChainstateManager comment (James O'Beirne)
769a1ef9fd test: Add tests with maleated snapshot data (Fabian Jahr)
4d8de04f32 tests: add snapshot activation test (James O'Beirne)
31d225274f tests: add deterministic chain generation unittest fixture (James O'Beirne)
6606a4f8c6 move-onlyish: break out CreateUTXOSnapshot from dumptxoutset (James O'Beirne)
ad949ba449 txdb: don't reset during in-memory cache resize (James O'Beirne)
f6e2da5fb7 simplify ChainstateManager::SnapshotBlockhash() return semantics (James O'Beirne)
7a6c46b37e chainparams: add allowed assumeutxo values (James O'Beirne)
Pull request description:
This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11):
Parent PR: #15606
Issue: #15605
Specification: https://github.com/jamesob/assumeutxo-docs/tree/master/proposal
---
This change proposes logic for activating UTXO snapshots, which is unused at the moment aside from an included unittest. There are a few moveonyish/refactoring commits to allow for halfway decent unittests.
Basic structure is included for specifying and checking the assumeutxo hash values used to validate activated snapshots. Initially I had specified a few height/hash pairs for mainnet in this change, but because of the security-critical nature of those parameters, I figured it was better to leave their inclusion to a future PR that includes only that change - my intent being that reviewers will be more likely to verify those parameters firsthand in a dedicated PR.
Aside from that and the snapshot activation logic, there are a few related changes:
- ~~allow caching the `nChainTx` value in the CCoinsViewDB; this is set during snapshot activation. Because we don't necessarily have access to the full chain at the time of snapshot load, this value is communicated through the snapshot metadata and must be cached within the chainstate to survive restarts.~~
- break out `CreateUTXOSnapshot()` from dumptxoutset. This is essentially a move-only change to allow the reuse of snapshot creation logic from within unittests.
- ...and a few other misc. changes that are solely related to unittests.
The move-onlyish commit is most easily reviewed with `--color-moved=zebra`.
ACKs for top commit:
fjahr:
Code review ACK 1afc0e4aa1
laanwj:
Code review ACK 1afc0e4aa1
Tree-SHA512: a4e4f0698f00a53ec298b5e8b7ef1c9fdf0185f95139d1b1f63cfdf6cbbd6d17b8c6e51bbf1de2e5f1a946bf49f8466232698ef55acce5a012c80b067da366ea
a5e15ae45c scripted-diff: rename ping members (John Newbery)
45dcf22661 [net processing] Move ping data fields to net processing (John Newbery)
dd2646d12c [net processing] Move ping timeout logic to net processing (John Newbery)
0b43b81f69 [net processing] Move send ping message logic into function (John Newbery)
1a07600b4b [net] Add RunInactivityChecks() (John Newbery)
f8b3058992 [net processing] Add Peer& arg to MaybeDiscourageAndDisconnect() (John Newbery)
Pull request description:
This continues the work of moving application layer data into net_processing, by moving all ping data into the new Peer object added in #19607.
For motivation, see #19398.
ACKs for top commit:
glozow:
reACK a5e15ae45c
MarcoFalke:
review ACK a5e15ae45c🥉
amitiuttarwar:
ACK a5e15ae45c
Tree-SHA512: fb84241613d6a6e1f2832fa5378030b5877a02e8308188f57ab545a6eaf2ab731a93abb7dcd3a7f7285bb66700f938096378a8e90cd6a3e6f3309f81d85a344e
ffff84a9cb fuzz: Remove expensive and redundant muhash from crypto fuzz target (MarcoFalke)
Pull request description:
Remove because it is redundant with `src/test/fuzz/muhash.cpp` and incredibly expensive
ACKs for top commit:
practicalswift:
Tested ACK ffff84a9cb
Tree-SHA512: c91ea2406db857127c789b9cdeb714a719d88b54132e9cef74fffd229532d874b6c043353793ec687504b5784afc74995f8982243d41f976b63d57454a5ed339
Values for mainnet and testnet will be specified in a follow-up PR that can be
scrutinized accordingly. This structure is required for use in snapshot activation
logic.
060a2a64d4 ci: remove boost thread installation (fanquake)
06e1d7d81d build: don't build or use Boost Thread (fanquake)
7097add83c refactor: replace Boost shared_mutex with std shared_mutex in sigcache (fanquake)
8e55981ef8 refactor: replace Boost shared_mutex with std shared_mutex in cuckoocache tests (fanquake)
Pull request description:
This replaces `boost::shared_mutex` and `boost::unique_lock` with [`std::shared_mutex`](https://en.cppreference.com/w/cpp/thread/shared_mutex) & [`std::unique_lock`](https://en.cppreference.com/w/cpp/thread/unique_lock).
Even though [some concerns were raised](https://github.com/bitcoin/bitcoin/issues/16684#issuecomment-726214696) in #16684 with regard to `std::shared_mutex` being unsafe to use across some glibc versions, I still think this change is an improvement. As I mentioned in #21022, I also think trying to restrict standard library feature usage based on bugs in glibc is not only hard to do, but it's not currently clear exactly how we do that in practice (does it also extend to patching out use in our dependencies, should we be implementing more runtime checks for features we are using, when do we consider an affected glibc "old enough" not to worry about? etc). If you take a look through the [glibc bug tracker](https://sourceware.org/bugzilla/describecomponents.cgi?product=glibc) you'll no doubt find plenty of (active) bug reports for standard library code we already using. Obviously not to say we shouldn't try and avoid buggy code where possible.
Two other points:
[Cory mentioned in #21022](https://github.com/bitcoin/bitcoin/pull/21022#issuecomment-769274179):
> It also seems reasonable to me to worry that boost hits the same underlying glibc bug, and we've just not happened to trigger the right conditions yet.
Moving away from Boost to the standard library also removes the potential for differences related to Boosts configuration. Boost has multiple versions of `shared_mutex`, and what you end up using, and what it's backed by depends on:
* The version of Boost.
* The platform you're building for.
* Which version of `BOOST_THREAD_VERSION` is defined: (2,3,4 or 5) default=2. (see [here](https://www.boost.org/doc/libs/1_70_0/doc/html/thread/build.html#thread.build.configuration) for some of the differences).
* Is `BOOST_THREAD_V2_SHARED_MUTEX` defined? (not by default). If so, you might get the ["less performant, but more robust"](https://github.com/boostorg/thread/issues/230#issuecomment-475937761) version of `shared_mutex`.
A lot of these factors are eliminated by our use of depends, but users will have varying configurations. It's also not inconceivable to think that a distro, or some package manager might start defining something like `BOOST_THREAD_VERSION=3`. Boost tried to change the default from 2 to 3 at one point.
With this change, we no longer use Boost Thread, so this PR also removes it from depends, the build system, CI etc.
Previous similar PRs were #19183 & #20922. The authors are included in the commits here.
Also related to #21022 - pthread sanity checking.
ACKs for top commit:
laanwj:
Code review ACK 060a2a64d4
vasild:
ACK 060a2a64d4
Tree-SHA512: 572d14d8c9de20bc434511f20d3f431836393ff915b2fe9de5a47a02dca76805ad5c3fc4cceecb4cd43f3ba939a0508178c4e60e62abdbaaa6b3e8db20b75b03
This fixes:
```bash
In file included from test/sock_tests.cpp:10:
In file included from /usr/local/include/boost/test/unit_test.hpp:18:
In file included from /usr/local/include/boost/test/test_tools.hpp:46:
/usr/local/include/boost/test/tools/old/impl.hpp:107:17: warning: comparison of integers of different signs: 'const long' and 'const unsigned long' [-Wsign-compare]
return left == right;
~~~~ ^ ~~~~~
```
which was introduced in #20788.
fabce459bb fuzz: version handshake (MarcoFalke)
Pull request description:
Not fuzzing the version handshake will limit fuzz coverage
ACKs for top commit:
practicalswift:
cr ACK fabce459bb: patch looks very much correct
Tree-SHA512: 4091d27d39edee781d033e471b352084bb54df250d0890e4821a325926a44dff9b26a2614d67dd0529f73bd366b075d7a0a1a570c2837de286a1b93a59a8fb91
53e716ea11 [refactor] improve style for touched code (gzhao408)
174cb5330a [refactor] const ATMPArgs and non-const Workspace (gzhao408)
f82baf0762 [refactor] return MempoolAcceptResult (gzhao408)
9db10a5506 [refactor] clean up logic in testmempoolaccept (gzhao408)
Pull request description:
This is the first 4 commits of #20833, and does refactoring only. It should be relatively simple to review, and offers a few nice things:
- It makes accessing values that don't make sense (e.g. fee) when the tx is invalid an error.
- Returning `MempoolAcceptResult` from ATMP makes the interface cleaner. The caller can get a const instead of passing in a mutable "out" param.
- We don't have to be iterating through a bunch of lists for package validation, we can just return a `std::vector<MempoolAcceptResult>`.
- We don't have to refactor all ATMP call sites again if/when we want to return more stuff from it.
ACKs for top commit:
MarcoFalke:
ACK 53e716ea11💿
jnewbery:
Code review ACK 53e716ea11
ariard:
Code Review ACK 53e716e, I did tweak a bit the touched paths to see if we had good test coverage. Didn't find holes.
Tree-SHA512: fa6ec324a08ad9e6e55948615cda324cba176255708bf0a0a0f37cedb7a75311aa334ac6f223be7d8df3c7379502b1081102b9589f9a9afa1713ad3d9ab3c24f
615ba0eb96 test: add Sock unit tests (Vasil Dimov)
7bd21ce1ef style: rename hSocket to sock (Vasil Dimov)
04ae846904 net: use Sock in InterruptibleRecv() and Socks5() (Vasil Dimov)
ba9d73268f net: add RAII socket and use it instead of bare SOCKET (Vasil Dimov)
dec9b5e850 net: move CloseSocket() from netbase to util/sock (Vasil Dimov)
aa17a44551 net: move MillisToTimeval() from netbase to util/time (Vasil Dimov)
Pull request description:
Introduce a class to manage the lifetime of a socket - when the object
that contains the socket goes out of scope, the underlying socket will
be closed.
In addition, the new `Sock` class has a `Send()`, `Recv()` and `Wait()`
methods that can be overridden by unit tests to mock the socket
operations.
The `Wait()` method also hides the
`#ifdef USE_POLL poll() #else select() #endif` technique from higher
level code.
ACKs for top commit:
laanwj:
Re-ACK 615ba0eb96
jonatack:
re-ACK 615ba0eb96
Tree-SHA512: 3003e6bc0259295ca0265ccdeb1522ee25b4abe66d32e6ceaa51b55e0a999df7ddee765f86ce558a788c1953ee2009bfa149b09d494593f7d799c0d7d930bee8
fa650ca7f1 Use -Wswitch for TxoutType where possible (MarcoFalke)
fa59e0b5bd test: Add missing script_standard_Solver_success cases (MarcoFalke)
Pull request description:
This removes unused `default:` cases for all `switch` statements on `TxoutType` and adds the cases (`MULTISIG`, `NULL_DATA`, `NONSTANDARD`) to `ExtractDestination` for clarity.
Also, the compiler is now able to use `-Wswitch`.
ACKs for top commit:
practicalswift:
cr ACK fa650ca7f1: patch looks correct and `assert(false);` is better than UB :)
hebasto:
ACK fa650ca7f1, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 282458b6523bd8923a0c0f5c423d1db2dce2a2d1b1d1dae455415c6fc995bb41ce82c1f9b0a1c0dcc6d874d171e04c30eca585f147582f52c7048c140358630a
fac726b1b8 doc: Fixup docs in fuzz/script_assets_test_minimizer.cpp (MarcoFalke)
fafca47adc fuzz: Hide script_assets_test_minimizer (MarcoFalke)
Pull request description:
This is not an actual fuzz target. It is a hack to exploit the built-in capability of fuzz engines to measure coverage.
ACKs for top commit:
practicalswift:
cr ACK fac726b1b8: patch looks correct and touches only `src/test/fuzz/`
Tree-SHA512: 0652dd8d9e95746b0906be4044467435d8204a34a30366ae9bdb75b9cb0788d429db7cedf2760fd543565d9d4f7ee206873ed10a29dd715a792a26337f65b53c
1bca2aa694 Introduce GetUniquePath(base) helper method to replace boost::filesystem::unique_path() which is not available in std::filesystem. (Kiminuo)
Pull request description:
This PR makes it easier in #20744 to remove our dependency on the `boost::filesystem::unique_path()` function which does not have a direct equivalent in C++17.
This PR attempts to re-implement `boost::filesystem::unique_path()` as `GetUniquePath(path)` but the implementations are not meant to be the same.
Note:
* Boost 1.75.0 implementation of `unique_path`: 9cab675b71/src/unique_path.cpp (L235)
* In the previous implementation, I attempted to add:
```cpp
fs::path GetUniquePath(const fs::path& base)
{
FastRandomContext rnd;
fs::path tmpFile = base / HexStr(rnd.randbytes(8));
return tmpFile;
}
```
to `fs.cpp` but this leads to a circular dependency: "fs -> random -> logging -> fs". That is why the modified implementation adds a new file.
ACKs for top commit:
laanwj:
Code review ACK 1bca2aa694
ryanofsky:
Code review ACK 1bca2aa694. It's a simple change and extra test coverage is nice
Tree-SHA512: f324bdf0e254160c616b5033c3ece33d87db23eb0135acee99346ade7b5cf0d30f3ceefe359a25a8e9b53ba8e4419f459c2bdd369e10fc0152ce95031d1f221c
This creates a cleaner interface with ATMP, allows us to make results const,
and makes accessing values that don't make sense (e.g. fee when tx is
invalid) an error.
faf7d7418c fuzz: Avoid extraneous copy of input data, using Span<> (MarcoFalke)
Pull request description:
Seeing speedup here in the fuzz framework part (non-fuzz-target part). Speedup is only visible for input data larger than 100kB.
ACKs for top commit:
practicalswift:
cr ACK faf7d7418c: patch looks correct :)
laanwj:
Code review ACK faf7d7418c
Tree-SHA512: 41af7118846e0dfee237a6d5269a6c7cfbc775d7bd1cc2a85814cb60f6c2b37fe7fd35f1a788d4f08e6e0202c48b71054b67d2931160c445c79fc59e5347dadf
fa61b9d1a6 util: Add ArgsManager::GetCommand() and use it in bitcoin-wallet (MarcoFalke)
7777105a24 refactor: Move all command dependend checks to ExecuteWalletToolFunc (MarcoFalke)
fa06bce4ac test: Add tests (MarcoFalke)
fac05ccdad wallet: [refactor] Pass ArgsManager to WalletAppInit (MarcoFalke)
Pull request description:
This not only moves the parsing responsibility out from the wallet tool, but it also makes it easier to implement bitcoin-util #19937Fixes: #20902
ACKs for top commit:
ajtowns:
ACK fa61b9d1a6
fjahr:
Code review ACK fa61b9d1a6
Tree-SHA512: 79622b806e8bf9dcd0dc24a8a6687345710df57720992e83a41cd8d6762a6dc112044ebc58fcf6e8fbf45de29a79b04873c5b8c2494a1eaaf902a2884703e47b
fa29272459 Remove redundant MakeUCharSpan wrappers (MarcoFalke)
faf4aa2f47 Remove CDataStream::Init in favor of C++11 member initialization (MarcoFalke)
fada14b948 Treat CDataStream bytes as uint8_t (MarcoFalke)
fa8bdb048e refactor: Drop CDataStream constructors in favor of one taking a Span of bytes (MarcoFalke)
faa96f841f Remove unused CDataStream methods (MarcoFalke)
Pull request description:
Using `uint8_t` for raw bytes has a style benefit:
* The signedness is clear from reading the code, as it does not depend on the architecture
Other clean-ups in this pull include:
* Remove unused methods
* Constructor is simplified with `Span`
* Remove `Init()` member in favor of C++11 member initialization
ACKs for top commit:
laanwj:
code review ACK fa29272459
theStack:
ACK fa29272459🍾
Tree-SHA512: 931ee28bd99843d7e894b48e90e1187ffb0278677c267044b3c0c255069d9bbd9298ab2e539b1002a30b543d240450eaec718ef4ee95a7fd4be0a295e926343f
dc8be12510 refactor: remove boost::thread_group usage (fanquake)
Pull request description:
Post #18710, there isn't much left using `boost::thread_group`, so should just be able to replace it with the standard library. This also removes the last use of `boost::thread_interrupted`.
After this change, last piece of Boost Thread we'd be using is `boost::shared_mutex`. See the commentary [here](https://github.com/bitcoin/bitcoin/issues/16684#issuecomment-726214696) as to why it may be non-trivial to swap that for `std::shared_mutex` in the near future.
Closes#17307
ACKs for top commit:
laanwj:
Code review re-ACK dc8be12510
MarcoFalke:
review ACK dc8be12510🔁
jonatack:
Non-expert code review ACK dc8be12510, also checked range-diff since last review and that local debug build is clean with gcc 10.2.1-6 on Debian
Tree-SHA512: 5510e2d760cce824234207dc86b1551ca8f21cbf3a2ce753c0254a0d03ffd83c94e449aec202fb7bd76e6fc64df783a6b70a736b0add9ece3734bb9c8ce8fc2f
67c9a83df1 style-only: Remove redundant sentence in ActivateBestChain comment (Carl Dong)
b8e95658d5 style-only: Make TestBlockValidity signature readable (Carl Dong)
0cdad75390 validation: Use accessible chainstate in ChainstateManager::ProcessNewBlock (Carl Dong)
ea4fed9021 validation: Use existing chainstate in ChainstateManager::ProcessNewBlockHeaders (Carl Dong)
e0dc305727 validation: Move LoadExternalBlockFile to CChainState (Carl Dong)
5f8cd7b3a5 validation: Remove global ::ActivateBestChain (Carl Dong)
2a696472a1 validation: Pass in chainstate to ::NotifyHeaderTip (Carl Dong)
9c300cc8b3 validation: Pass in chainstate to TestBlockValidity (Carl Dong)
0e17c833cd validation: Make CChainState.m_blockman public (Carl Dong)
d363d06bf7 validation: Pass in blockman to ContextualCheckBlockHeader (Carl Dong)
f11d11600d validation: Move GetLastCheckpoint to BlockManager (Carl Dong)
e4b95eefbc validation: Move GetSpendHeight to BlockManager (Carl Dong)
b026e318c3 validation: Move FindForkInGlobalIndex to BlockManager (Carl Dong)
3664a150ac validation: Remove global LookupBlockIndex (Carl Dong)
eae54e6e60 scripted-diff: Use BlockManager::LookupBlockIndex (Carl Dong)
15d20f40e1 validation: Move LookupBlockIndex to BlockManager (Carl Dong)
f92dc6557a validation: Guard the active_chainstate with cs_main (Carl Dong)
Pull request description:
Overall PR: #20158 (tree-wide: De-globalize ChainstateManager)
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:
jnewbery:
utACK 67c9a83df1
laanwj:
re-ACK 67c9a83df1
ryanofsky:
Code review ACK 67c9a83df1. Changes since last review:
Tree-SHA512: 8744aba2dd57a40cd2fedca809b0fe24d771bc60da1bffde89601999384aa0df428057a86644a3f72fbeedbc8b04db6c4fd264ea0db2e73c279e5acc6d056cbf
