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
[META] This commit should be followed up by removing the comments and
assertions meant only to show that the change is correct.
LoadExternalBlockFile mainly acts on CChainState.
[META] In a previous commit, we moved ::LookupBlockIndex to become a
member function of BlockManager. This commit is split out from
that one since it can be expressed nicely as a scripted-diff.
-BEGIN VERIFY SCRIPT-
find_regex='LookupBlockIndex' \
&& git grep -l -E "$find_regex" -- src \
| grep -v '^src/validation\.\(cpp\|h\)$' \
| xargs sed -i -E "s@${find_regex}@g_chainman.m_blockman.LookupBlockIndex@g"
-END VERIFY SCRIPT-
fad3d7625a fuzz: Avoid initializing version to less than MIN_PEER_PROTO_VERSION (MarcoFalke)
fa99e33aeb fuzz: move-only FillNode implementation to cpp file (MarcoFalke)
Pull request description:
This fixes a fuzz bug introduced in #20881. Previously the nodes in the fuzz tests had their version initialized to a constant (`PROTOCOL_VERSION`). After #20881, the nodes have their version initialized to an arbitrary signed integer. This is problematic for several reasons:
* Both `nVersion` and `m_greatest_common_version` may be initialized to `0`. If a `version` message is processed, this leads to a crash, because `m_greatest_common_version` must be `INIT_PROTO_VERSION` while the `version` message is processed. See #20138
* The "valid" range for `nVersion` is `[MIN_PEER_PROTO_VERSION, std::numeric_limits<int32_t>::max()]` (see check in net_processing)
* The "valid" range for `m_greatest_common_version` is `std::min(nVersion, PROTOCOL_VERSION)` (see net_processing)
Fix all issues by initializing `nVersion` and `m_greatest_common_version` to their valid ranges.
-----
The crashers, if someone wants to try this at home:
```
( echo 'dmVyc2lvbgAWFhYWFhYWFhYWFhYWFhYWFhYWFhZp/29uAPX//xYWFhYWFhYWFhYWFhYWFhYWFhYW
FhYWFhYWaW9uAOr1//8WFhYWFha0ZXJzaW9uAPX//wAAAAAAABAAAAAAAAAAAAC0ZXJzaW9uAPX/
/wBPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT08AAAAAABAAAAAAAAAAAAAAAAAA
AAAAAAAAAAAAAAAAAAAAAAAAAAAACgAAAAAAAAAAgAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA
AAAAAAAAAAAAAAB2ZXJzaW9uAACDJIO9vXYKAAAAAAAAAAAAAAAAAAAAAAB2ZfS1qmu1qhUVFWs=' | base64 --decode > /tmp/a ) && FUZZ=process_message_version ./src/test/fuzz/fuzz /tmp/a
```
```
( echo 'dmVyc2lvbgD//wAhTmiqN///NDcAAACENDL/iv//8DYAAHL///////79/RtcAJqamhqa/QEAAAD/
///+/f1oZWFkZXJzAAAAAM8BAAAAIAYibkYRGgtZyq8SaGVhZGVycwAAAAD/NDcAAACENDL/iv//
8DYAAHL///////79/RtcAJqamhqa/QEAAAD////+/f1oZWFkZXJzAAAAAM8BAAAAIAYibkYRGgtZ
yq8SaGVhZGVycwAAAADPAQAAACAGIm5GERoLWS1wb3J061u/KMNPOkwFXqZ///b5IgIAAD+5ubkb
XD5hZGRyAJqamhqasP0BAAAAAAAAAP0BAAAAIf39/R0dHQAAAAAAMgAA///7//+gXqZ///b5IgIA
AD+5ubm5ubm5AAAAAAAAAAAAAAAAAAAAAAAAAACAAAAAAAAAFgAAAAAAAAAAAAlBmv39/f1/f39B
f39hZGRyAG5vAACaLgAdGzY2zwEAAAAgBiJuRhEaC1ktcG9ydOtbvyjDTzpMBV6mf//2+SICAAA/
ubm5G1w+YWRkcgCampoamrD9AQAAAAAAAAD9AQAAACH9/f0dHR0AAAAAADIAAP//+///oF6mf//2
+SICAAA/ubm5ubm5uQAAAAAAAAAAAAAAAAAAAAAAAAAAgAAAAAAAABYAAAAAAAAAAAAJQZr9/f39
f39/QX9/YWRkcgBubwAAmi4AHRs2NjY2NjY2NjYCAgI2NgIA/f39/f39Nv39/TUmABxc' | base64 --decode > /tmp/b ) && FUZZ=process_message_version ./src/test/fuzz/fuzz /tmp/b
```
ACKs for top commit:
practicalswift:
cr ACK fad3d7625a
Tree-SHA512: ea64ee99b94d8e619e3949d2d21252c1236412c0e40f44f2b73595ca70cd2da0bdab005fb1a54f65fb291e7b07fdd33577ce4a3a078ca933246b511ebcb0e52a
2a39ccf133 Add include for std::bind. (sinetek)
Pull request description:
Hi, this patch adds in <functional> because the GUI code makes use of std::bind.
That's all.
ACKs for top commit:
jonasschnelli:
utACK 2a39ccf133
Tree-SHA512: fb5ac07d9cd5d006182b52857b289a9926362a2f1bfa4f7f1c78a088670e2ccf39ca28214781df82e8de3909fa3e69685fe1124a7e3ead758575839f5f2277a9
f0f8b1a076 fuzz: Add UBSan suppressions needed for fuzz tests to not warn under -fsanitize=integer (practicalswift)
58232e3ffb fuzz: Avoid -fsanitize=integer warnings in fuzzing harnesses (practicalswift)
Pull request description:
Add UBSan suppressions needed for fuzz tests to not warn under `-fsanitize=integer`.
Avoid `-fsanitize=integer` warnings in fuzzing harnesses.
Suppressed warnings (excluding warnings from `src/crypto/` and `src/test/`):
```
addrman.cpp:306:24: runtime error: implicit conversion from type 'long' of value 5190149478 (64-bit, signed) to type 'uint32_t' (aka 'unsigned int') changed the value to 895182182 (32-bit, unsigned)
addrman.h:446:43: runtime error: implicit conversion from type 'int' of value -22 (32-bit, signed) to type 'const uint8_t' (aka 'const unsigned char') changed the value to 234 (8-bit, unsigned)
arith_uint256.cpp:32:35: runtime error: left shift of 1712128 by 24 places cannot be represented in type 'uint32_t' (aka 'unsigned int')
arith_uint256.cpp:47:39: runtime error: left shift of 4294966784 by 31 places cannot be represented in type 'uint32_t' (aka 'unsigned int')
chain.cpp:151:12: runtime error: implicit conversion from type 'int' of value -1 (32-bit, signed) to type 'unsigned long' changed the value to 18446744073709551615 (64-bit, unsigned)
coins.cpp:114:22: runtime error: unsigned integer overflow: 0 - 96 cannot be represented in type 'unsigned long'
compressor.cpp:162:33: runtime error: unsigned integer overflow: 15617702637291228364 * 10 cannot be represented in type 'unsigned long'
compressor.cpp:188:11: runtime error: unsigned integer overflow: 2265760372865400000 * 10 cannot be represented in type 'unsigned long'
hash.cpp:13:15: runtime error: left shift of 1692305888 by 15 places cannot be represented in type 'uint32_t' (aka 'unsigned int')
pubkey.h:152:23: runtime error: unsigned integer overflow: 0 - 1 cannot be represented in type 'unsigned int'
streams.h:570:31: runtime error: left shift of 350879 by 52 places cannot be represented in type 'uint64_t' (aka 'unsigned long')
util/bip32.cpp:57:36: runtime error: left shift of 3241096244 by 1 places cannot be represented in type 'unsigned int'
util/strencodings.cpp:562:38: runtime error: implicit conversion from type 'unsigned char' of value 255 (8-bit, unsigned) to type 'char' changed the value to -1 (8-bit, signed)
util/strencodings.h:164:24: runtime error: implicit conversion from type 'int' of value -74 (32-bit, signed) to type 'unsigned long' changed the value to 18446744073709551542 (64-bit, unsigned)
```
The warnings above happen here:
32b191fb66/src/addrman.cpp (L306)32b191fb66/src/addrman.h (L446)32b191fb66/src/arith_uint256.cpp (L32)32b191fb66/src/arith_uint256.cpp (L47)32b191fb66/src/chain.cpp (L151)32b191fb66/src/coins.cpp (L114)32b191fb66/src/compressor.cpp (L162)32b191fb66/src/compressor.cpp (L188)32b191fb66/src/hash.cpp (L13)32b191fb66/src/pubkey.h (L152)32b191fb66/src/streams.h (L570)32b191fb66/src/util/bip32.cpp (L57)32b191fb66/src/util/strencodings.cpp (L562)32b191fb66/src/util/strencodings.h (L164)
ACKs for top commit:
MarcoFalke:
review ACK f0f8b1a076🤚
Tree-SHA512: a8f04f7cc055d03653161de1d9d14d106a6280cea1e86a1243abcd57cf8e61dcf5f731d0ab0da5b390790e816022ff7a70759a641463bc7e3303076b8667009f
After the merge of #18710, the linter is warning:
```bash
A new Boost dependency in the form of "boost/thread/mutex.hpp" appears to have been introduced:
src/sync.cpp:#include <boost/thread/mutex.hpp>
src/test/sync_tests.cpp:#include <boost/thread/mutex.hpp>
^---- failure generated from test/lint/lint-includes.sh
```
the interim #19337 was merged, which introduced more `boost::mutex` usage.
Given we no longer use `boost::mutex`, just remove the double lock test
and remaining includes.
bb6fcc75d1 refactor: Drop boost::thread stuff in CCheckQueue (Hennadii Stepanov)
6784ac471b bench: Use CCheckQueue local thread pool (Hennadii Stepanov)
dba30695fc test: Use CCheckQueue local thread pool (Hennadii Stepanov)
01511776ac Add local thread pool to CCheckQueue (Hennadii Stepanov)
0ef938685b refactor: Use member initializers in CCheckQueue (Hennadii Stepanov)
Pull request description:
This PR:
- gets rid of `boost::thread_group` in the `CCheckQueue` class
- allows thread safety annotation usage in the `CCheckQueue` class
- is alternative to #14464 (https://github.com/bitcoin/bitcoin/pull/18710#issuecomment-616618525, https://github.com/bitcoin/bitcoin/pull/18710#issuecomment-617291612)
Also, with this PR (I hope) it could be easier to resurrect a bunch of brilliant ideas from #9938.
Related: #17307
ACKs for top commit:
laanwj:
Code review ACK bb6fcc75d1
LarryRuane:
ACK bb6fcc75d1
jonatack:
Code review ACK bb6fcc75d1 and verified rebase to master builds cleanly with unit/functional tests green
Tree-SHA512: fddeb720d5a391b48bb4c6fa58ed34ccc3f57862fdb8e641745c021841c8340e35c5126338271446cbd98f40bd5484f27926aa6c3e76fa478ba1efafe72e73c1
da9caa1ced Replace fs::absolute calls with AbsPathJoin calls (Kiminuo)
66576c4fd5 test: Clear forced -walletdir setting after wallet init_tests (Kiminuo)
Pull request description:
This adds better test coverage and will make it easier in #20744 to remove our dependency on the two-argument boost::filesystem::absolute() function which does not have a direct equivalent in C++17.
This PR doesn't change behavior aside from adding an assert and fixing a test bug.
ACKs for top commit:
jonatack:
Code review ACK da9caa1ced only doxygen improvements since my last review per `git diff d867d7a da9caa1`
MarcoFalke:
review ACK da9caa1ced📯
ryanofsky:
Code review ACK da9caa1ced. Just comment and test tweaks since previous review.
Tree-SHA512: c940ee60f3ba374d4927cf34cf12d27c4c735c94af591fbc0ca408c641b30f8f8fbcfe521d66bfbddf9877a1fc8cd99bd8a47ebcd2fa59789de6bd87a7b9cf4d
abb6fa7285 fuzz: Initialize a full TestingSetup where appropriate (Carl Dong)
713314abfa fuzz: Consolidate fuzzing TestingSetup initialization (Carl Dong)
Pull request description:
```
Previously, the {Basic,}TestingSetup for fuzzers were set up in many ways:
1. Calling InitializeFuzzingContext, which implicitly constructs a static
const BasicTestingSetup
2. Directly constructing a static const BasicTestingSetup in the initialize_*
function
3. Directly constructing a static TestingSetup and reproducing the
initialization arguments (I'm assuming because
InitializeFuzzingContext only initializes a BasicTestingSetup)
The new, relatively-simple MakeFuzzingContext function allows us to
consolidate these methods of initialization by being flexible enough to
be used in all situations. It:
1. Is templated so that we can choose to initialize any of
the *TestingSetup classes
2. Has sane defaults which are often used in fuzzers but are also
easily overridable
3. Returns a unique_ptr, explicitly transferring ownership to the caller
to deal with according to its situation
```
~~Question for fuzzing people: was it intentional that `src/test/fuzz/net.cpp` would directly instantiate the `BasicTestingSetup` and thus omit the `"-nodebuglogfile"` flag?~~ [Answered](https://github.com/bitcoin/bitcoin/pull/20946#issuecomment-761537108)
ACKs for top commit:
MarcoFalke:
ACK abb6fa7285
Tree-SHA512: 96a5ca6f4cd5ea0e9483b60165b31ae3e9003918c700a7f6ade48010f419f2a6312e10b816b3187f1d263798827571866e4c4ac0bbfb2e0c79dfad254cda68e7
Previously, the {Basic,}TestingSetup for fuzzers were set up in many ways:
1. Calling InitializeFuzzingContext, which implicitly constructs a static
const BasicTestingSetup
2. Directly constructing a static const BasicTestingSetup in the initialize_*
function
3. Directly constructing a static TestingSetup and reproducing the
initialization arguments (I'm assuming because
InitializeFuzzingContext only initializes a BasicTestingSetup)
The new, relatively-simple MakeFuzzingContext function allows us to
consolidate these methods of initialization by being flexible enough to
be used in all situations. It:
1. Is templated so that we can choose to initialize any of
the *TestingSetup classes
2. Has sane defaults which are often used in fuzzers but are also
easily overridable
3. Returns a unique_ptr, explicitly transferring ownership to the caller
to deal with according to its situation
This adds better test coverage and will make it easier in #20744 to remove our dependency on the two-argument boost::filesystem::absolute() function which does not have a direct equivalent in C++17.
fa0a864b38 fuzz: Use mocktime in process_message* fuzz targets (MarcoFalke)
Pull request description:
Use mocktime to allow time to advance deterministically during execution of a fuzz input. This also allows to drop the call to `JumpOutOfIbd`.
ACKs for top commit:
practicalswift:
cr ACK fa0a864b38
Tree-SHA512: e92fc70ec6bd49760173cb202549f364304e22b3f7127b9a4da8447cf9341008e477ad42c2599c2fde167bbcbc0e2d139709b4ef6371788bc2c1c3b7f589e11d
fa75d40ef8 fuzz: Introduce CallOneOf helper to replace switch-case (MarcoFalke)
Pull request description:
The current `switch (fuzzed_data_provider.ConsumeIntegralInRange<int>(0, nn)) { case 0: ... case 1: ... case nn: ...` has several problems:
* It makes it hard to review newly added targets, because it requires manual counting of cases
* It makes it hard to update a target, because updating all case labels is trivial, but tedious to review and causes merge conflicts
* ~~Updating the target raises the question whether the case labels should be preserved to not invalidate the existing fuzz inputs format. Fuzz input format might already change implicitly on every commit, so this isn't something worthwhile to pursue.~~ Edit: This pull doesn't fix this problem.
Fix all issues by adding a new `CallOneOf` helper
ACKs for top commit:
ajtowns:
ACK fa75d40ef8 - code review only
jnewbery:
utACK fa75d40ef8
Tree-SHA512: 2daa602b240b86c8e85a024e008f03a57ba60349377eed771f4d21a97a9dba9b66e93fff16ff1992018d4330be7a1a276944c3dfdf698748ce135626c380e563
