`CPubKey::VerifyPubKey` uses rng internally which leads to instability
in the fuzz test.
We fix this by avoiding `VerifyPubKey` in the test and verifying the
decoded public key with a fuzzer chosen message instead.
e5b9ee0221 fuzz: set `nMaxOutboundLimit` in connman target (brunoerg)
Pull request description:
Setting `nMaxOutboundLimit` (`-maxuploadtarget`) will make fuzz to reach more coverage in connman target. This value is used in `GetMaxOutboundTimeLeftInCycle`, `OutboundTargetReached` and `GetOutboundTargetBytesLeft`.
ACKs for top commit:
dergoegge:
utACK e5b9ee0221
jonatack:
ACK e5b9ee0221
Tree-SHA512: d19c83602b0a487e6da0e3be539aa2abc95b8bbf36cf9a3e391a4af53b959f68ca38548a96d27d56742e3b772f648da04e2bf8973dfc0ab1cdabf4f2e8d44de6
fa87f8feb7 doc: Clarify C++20 comments (MarcoFalke)
Pull request description:
Turns out "class template argument deduction for aggregates" is one of the few things implemented only in recent compilers, see https://en.cppreference.com/w/cpp/compiler_support/20
So clarify the comments.
ACKs for top commit:
hebasto:
ACK fa87f8feb7, I verified the code with clang-{16,17}.
Tree-SHA512: f6d20f946cb6f8e34db224e074ed8f9dfa598377c066d1b58a8feb9e64d007444f1e2c0399e91a3e282fd5d59f90e0d7df90aa3956824d96bc78070ee12f603c
a44808fb43 fuzz: rule-out too deep derivation paths in descriptor parsing targets (Antoine Poinsot)
Pull request description:
This fixes the `mocked_descriptor_parse` timeout reported in #28812 and direct the targets more toward what they are intended to fuzz: the descriptor syntax.
ACKs for top commit:
sipa:
utACK a44808fb43
achow101:
ACK a44808fb43
dergoegge:
ACK a44808fb43 - Not running into timeouts anymore
TheCharlatan:
ACK a44808fb43
Tree-SHA512: a5dd1dbe9adf8f088bdc435addab88b56f435e6d7d2065bd6d5c6d80a32e3f1f97d3d2323131ab233618cd6dcc477c458abe3c4c865ab569449b8bc176231e93
b1318dcc56 test: change `m_submitted_in_package` input to fuzz data provider boolean (ismaelsadeeq)
5615e16b70 tx fees: update `m_from_disconnected_block` to `m_mempool_limit_bypassed` (ismaelsadeeq)
fcd4296648 doc: fix typo and update incorrect comment (ismaelsadeeq)
562664d263 test: wait for fee estimator to catch up before estimating fees (ismaelsadeeq)
Pull request description:
This is a simple PR that does two things
1. Fixes #29000 by waiting for the fee estimator to catch up after `removeForBlock` calls before calling `estimateFee` in the `BlockPolicyEstimates` unit test.
2. Addressed some outstanding review comments from #28368
- Updated `NewMempoolTransactionInfo::m_from_disconnected_block` to `NewMempoolTransactionInfo::m_mempool_limit_bypassed` which now correctly indicates what the boolean does.
- Changed input of `processTransaction`'s tx_info `m_submitted_in_package` input from false to fuzz data provider boolean.
- Fixed some typos, and update incorrect comment
ACKs for top commit:
martinus:
re-ACK b1318dcc56
glozow:
utACK b1318dcc56
Tree-SHA512: 45268729bc044da4748fe004524e0df696d2ec92c5bd053db9aad6e15675f3838429b2a7b9061a6b694be4dc319d1782a876b44df506ddd439d62ad07252d0e1
In reality some mempool transaction might be submitted in a package,
so change m_submitted_in_package to fuzz data provider boolean just like
m_has_no_mempool_parents.
The boolean indicates whether the transaction was added without enforcing mempool
fee limits. m_mempool_limit_bypassed is the correct variable name.
Also changes NewMempoolTransactionInfo booleans descriptions to the format that
is consistent with the codebase.
Making the `GenerateRandomKey` helper available to other modules via
key.{h.cpp} allows us to create random private keys directly at
instantiation of CKey, in contrast to the two-step process of creating
the instance and then having to call `MakeNewKey(...)`.
e1281f1bbd wallet: fix key parsing check for miniscript expressions in `ParseScript` (brunoerg)
Pull request description:
In `ParseScript`, when processing miniscript expressions, the way we check for key parsing error is wrong, the actual code is unreachable because we're checking it into `if (node)` (successful parsing) statement.
ACKs for top commit:
sipa:
utACK e1281f1bbd
RandyMcMillan:
utACK e1281f1bbd
achow101:
ACK e1281f1bbd
Tree-SHA512: c4b3765d32673928a1f6d84ecbaa311870da9a9625753ed15ea57c802a9f16ddafa48c1dc66c0e4be284c5862e7821ed94135498ed9b9f3d7342a080035da289
These provide simple and clear ways to write the most common logging
operations:
LogInfo("msg");
LogDebug(BCLog::LogFlags::NET, "msg");
LogError("msg");
LogWarning("msg");
LogTrace(BCLog::LogFlags::NET, "msg");
For cases where the level cannot be hardcoded, LogPrintLevel(category,
level, ...) remains available.
b2fc7a2eda [fuzz] Improve fuzzing stability for minisketch harness (dergoegge)
Pull request description:
The `minisketch` harness has low stability due to:
* Rng internal to minisketch
* Benchmarkning for the best minisketch impl
Fix this by seeding the rng and letting the fuzzer choose the impl.
Also see #29018.
ACKs for top commit:
maflcko:
review ACK b2fc7a2eda
Tree-SHA512: 3d81414299c6803c34e928a53bcf843722fa8c38e1d3676cde7fa80923f9058b1ad4b9a2941f718303a6641b17eeb28b4a22eda09678102e9fb7c4e31d06f8f2
7b45744df3 tests: ensure functional tests set permitbaremultisig=1 when needed (Anthony Towns)
7dfabdcf86 tests: test both settings for permitbaremultisig in p2sh tests (Anthony Towns)
Pull request description:
Update unit and functional tests so that they continue to work if the default for `-permitbaremultisig` is changed.
ACKs for top commit:
maflcko:
lgtm ACK 7b45744df3
instagibbs:
crACK 7b45744df3
ajtowns:
> crACK [7b45744](7b45744df3)
achow101:
ACK 7b45744df3
glozow:
ACK 7b45744df3, changed default locally and all tests passed
Tree-SHA512: f89f9e2bb11f07662cfd57390196df9e531064e1bd662e1db7dcfc97694394ae5e8014e9d209b9405aa09195bf46fc331b7fba10378065cdb270cbd0669ae904
6666713041 refactor: Rename fs::path::u8string() to fs::path::utf8string() (MarcoFalke)
856c88776f ArgsManager: return path by value from GetBlocksDirPath() (Vasil Dimov)
fa3d9304e8 refactor: Remove pre-C++20 fs code (MarcoFalke)
fa00098e1a Add tests for C++20 std::u8string (MarcoFalke)
fa2bac08c2 refactor: Avoid copy/move in fs.h (MarcoFalke)
faea30227b refactor: Use C++20 std::chrono::days (MarcoFalke)
Pull request description:
This:
* Removes dead code.
* Avoids unused copies in some places.
* Adds copies in other places for safety.
ACKs for top commit:
achow101:
ACK 6666713041
ryanofsky:
Code review ACK 6666713041. Just documentation change since last review.
stickies-v:
re-ACK 6666713041
Tree-SHA512: 6176e44f30b310d51632ec2d3827c3819905d0ddc6a4b57acfcb6cfa1f9735176da75ee8ed4a4abd1296cb0b83bee9374cc6f91ffac87c19b63c435eeadf3f46
6db04be102 Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly (Ryan Ofsky)
213542b625 refactor: Add InitContext function to initialize NodeContext with global pointers (Ryan Ofsky)
feeb7b816a refactor: Remove calls to StartShutdown from KernelNotifications (Ryan Ofsky)
6824eecaf1 refactor: Remove call to StartShutdown from stop RPC (Ryan Ofsky)
1d92d89edb util: Get rid of uncaught exceptions thrown by SignalInterrupt class (Ryan Ofsky)
ba93966368 refactor: Remove call to ShutdownRequested from IndexWaitSynced (Ryan Ofsky)
42e5829d97 refactor: Remove call to ShutdownRequested from HTTPRequest (Ryan Ofsky)
73133c36aa refactor: Add NodeContext::shutdown member (Ryan Ofsky)
f4a8bd6e2f refactor: Remove call to StartShutdown from qt (Ryan Ofsky)
f0c73c1336 refactor: Remove call to ShutdownRequested from rpc/mining (Ryan Ofsky)
263b23f008 refactor: Remove call to ShutdownRequested from chainstate init (Ryan Ofsky)
Pull request description:
This change drops `shutdown.h` and `shutdown.cpp` files, replacing them with a `NodeContext::shutdown` member which is used to trigger shutdowns directly. This gets rid of an unnecessary layer of indirection, and allows getting rid of the `kernel::g_context` global.
Additionally, this PR tries to improve error handling of `SignalInterrupt` code by marking relevant methods `[[nodiscard]]` to avoid the possibility of uncaught exceptions mentioned https://github.com/bitcoin/bitcoin/pull/27861#discussion_r1255496707.
Behavior is changing In a few cases which are noted in individual commit messages. Particularly: GUI code more consistently interrupts RPCs when it is shutting down, shutdown state no longer persists between unit tests, the stop RPC now returns an RPC error if requesting shutdown fails instead of aborting, and other failed shutdown calls now log errors instead of aborting.
This PR is a net reduction in lines of code, but in some cases the explicit error handling and lack of global shutdown functions do make it more verbose. The verbosity can be seen as good thing if it discourages more code from directly triggering shutdowns, and instead encourages code to return errors or send notifications that could be translated into shutdowns. Probably a number of existing shutdown calls could just be replaced by better error handling.
ACKs for top commit:
achow101:
ACK 6db04be102
TheCharlatan:
Re-ACK 6db04be102
maflcko:
ACK 6db04be102👗
stickies-v:
re-ACK 6db04be102
Tree-SHA512: 7a34cb69085f37e813c43bdaded1a0cbf6c53bd95fdde96f0cb45346127fc934604c43bccd3328231ca2f1faf712a7418d047ceabd22ef2dca3c32ebb659e634
fa3da629a1 Remove DirIsWritable, GetUniquePath (MarcoFalke)
fad3a9793b Return LockResult::ErrorWrite in LockDirectory (MarcoFalke)
fa0afe7408 refactor: Return enum in LockDirectory (MarcoFalke)
Pull request description:
`GetUniquePath` is only used in tests and in `DirIsWritable`. The check by `DirIsWritable` is redundant with the check done in `LockDirectory`.
Fix the redundancy by removing everything, except `LockDirectory`.
ACKs for top commit:
TheCharlatan:
Re-ACK fa3da629a1
hebasto:
ACK fa3da629a1, I have reviewed the code and it looks OK.
Tree-SHA512: e95f18cd586de7582e9c08ac7ddb860bfcfcbc8963804f45c5784c5e4c0598dc59ae7e45dd4daf30a5020dbf8433f5db2ad06e46a8676371982003790043c6c9
9f265d8825 fuzz: Detect deadlocks in process_message (dergoegge)
fae1e7e012 fuzz: p2p: Detect peer deadlocks (MarcoFalke)
Pull request description:
It may be possible that a peer connection will deadlock, due to software bugs such as https://github.com/bitcoin/bitcoin/pull/18808.
Fix this by detecting them in the fuzz target.
Can be tested by introducing a bug such as:
```diff
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 1067341495..97495a13df 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -2436,3 +2436,3 @@ void PeerManagerImpl::ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic
if (it != peer.m_getdata_requests.end() && !pfrom.fPauseSend) {
- const CInv &inv = *it++;
+ const CInv& inv = *it;
if (inv.IsGenBlkMsg()) {
```
Using a fuzz input such as:
```
$ base64 ./timeout-ada0fecaba2b8c46c6e970cf637d9625b01bf7e5
kNptdNbW1tbWYghvXIpwb25vPQAA////////cwAjLv8AXAB2ZXJhY2sAQW5v/62tra3Pz///////
//////////////////////9c8GZpbHRlcmxvYWQAAAEAAwAAAABVYwC2XABmaWx0ZXJhZGQAAAAX
Fxdn/////2V0F861tcqvEmAAACEAAABjYXB0dXJldmUAAH4AgAA1PNfX11x0Z2V0ZGF0YQBDACOw
AQMAAAAGIm5GERoLWcqvEmBD61u/KMNPOl4zKh/HKLK3PPGIkQ9eE/////////8AAAAAAAAAAFtb
WyjDTzpeMSofx7K3PNfX11x0Z2V0ZGF0YQBDACMwAQMAAAAGIm5GERoLWcqvEmBD61u/KMNPOl4z
Kh/Hsrc88YiRD2/Nzc3Nzc3Nzc3NTc3Nzc3Nzc3Nzc3Nzc3Nzc3Nzc3Nzc3Nzc3Nzc3Nzc3Nzc3N
zWWj1NTUudTU1NTU1P///0j+P/9cdHR4AAAAAAAAy/4AAHR4AAAAAAAAP8v+AAD/+P//////////
AX55bJl8HWnz/////wAgXGF0YVPxY2RkAAAA
```
And running the fuzz target:
```
$ FUZZ=process_messages ./src/test/fuzz/fuzz -runs=1 -timeout=18 ./timeout-ada0fecaba2b8c46c6e970cf637d9625b01bf7e5
INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 3436516708
INFO: Loaded 1 modules (390807 inline 8-bit counters): 390807 [0x55d0d6221e80, 0x55d0d6281517),
INFO: Loaded 1 PC tables (390807 PCs): 390807 [0x55d0d6281518,0x55d0d6877e88),
./src/test/fuzz/fuzz: Running 1 inputs 1 time(s) each.
Running: ./timeout-ada0fecaba2b8c46c6e970cf637d9625b01bf7e5
ALARM: working on the last Unit for 19 seconds
and the timeout value is 18 (use -timeout=N to change)
==375014== ERROR: libFuzzer: timeout after 19 seconds
```
ACKs for top commit:
naumenkogs:
ACK 9f265d8825
dergoegge:
ACK 9f265d8825
brunoerg:
ACK 9f265d8825
Tree-SHA512: da83ff90962bb679aae00e8e9dba639c180b7aaba544e0c4d0978d36e28a9ff1cd7a2e13009d8ab407ef57767656aca1ebc767a7d2f1bc880284f8f57c197a50
15f5a0d0c8 fuzz: Improve fuzzing stability for txorphan harness (dergoegge)
Pull request description:
The `txorphan` harness has low stability as eviction of orphan txs is entirely random at the moment.
Fix this by passing the rng to `LimitOrphans`, which can be deterministic in tests.
Also see #29018.
ACKs for top commit:
maflcko:
lgtm ACK 15f5a0d0c8
brunoerg:
utACK 15f5a0d0c8
Tree-SHA512: 854ec34b3a0f16f26db6dc419096c6e7a380e8400119534aa278d6b1d54c253b572aa2fad13c383c796c431d8ff4263956e6f60326e99f8bf6abd16d9a280e97
fa6e50d6c7 fuzz: Use C++20 starts_with in rpc.cpp (MarcoFalke)
faa48388bc Revert "tracepoints: Disables `-Wgnu-zero-variadic-macro-arguments` to compile without warnings" (MarcoFalke)
fae3b77a87 refactor: Drop unused _Pragma to ignore -Wgnu-zero-variadic-macro-arguments (MarcoFalke)
fa02fc0a86 refactor: modernize-use-default-member-init for bit-fields (C++20) (MarcoFalke)
fa67f096bd build: Require C++20 compiler (MarcoFalke)
Pull request description:
C++20 allows to write safer code, because it allows to enforce more stuff at compile time (`constinit`, `conteval`, `constexpr`, `std::span`, ...).
Also, it allows to write less verbose and easier to understand code (C++ 20 Concepts).
See https://github.com/bitcoin/bitcoin/issues/23363 and https://en.cppreference.com/w/cpp/compiler_support#cpp20
With g++-10 (https://github.com/bitcoin/bitcoin/pull/28348) and clang-13 (https://github.com/bitcoin/bitcoin/pull/28210), there is broad support for almost all features of C++20.
It should be fine to require a C++20 compiler for Bitcoin Core 27.0 in 2024 (next year), not the soon upcoming 26.0 next month.
This pull request includes three small cleanups to make use of C++20 features. If any issues are detected before or after merge, this should be easy to revert. If no issues arise, it should be fine to make use of more involved C++20 features later on.
ACKs for top commit:
fanquake:
ACK fa6e50d6c7
Tree-SHA512: 244d79bfb0b750a4bdd713f40573b9ca33816fb84b6c84a58f027b9d7d4bb0cc4f18642959e4cf3d094808a69e5b8a327ca8521d7c0c08af27dacb5da3e78e71
fa63f16018 test: Add uint256 string parse tests (MarcoFalke)
facf629ce8 refactor: Remove unused and fragile string interface from arith_uint256 (MarcoFalke)
Pull request description:
The string interface (`base_uint(const std::string&)`, as well as `base_uint::SetHex`) is problematic for many reasons:
* It is unused (except in test-only code).
* It is redundant with the `uint256` string interface: `std::string -> uint256 -> UintToArith256`.
* It is brittle, because it inherits the brittle `uint256` string interface, which is brittle due to the use of `c_str()` (embedded null will be treated as end-of string), etc ...
Instead of fixing the interface, remove it since it is unused and redundant with `UintToArith256`.
ACKs for top commit:
ajtowns:
ACK fa63f16018
TheCharlatan:
ACK fa63f16018
Tree-SHA512: a95d5b938ffd0473361336bbf6be093d01265a626c50be1345ce2c5e582c0f3f73eb11af5fd1884019f59d7ba27e670ecffdb41d2c624ffb9aa63bd52b780e62
All functions assume that the pointer is never null, so pass by
reference, to avoid accidental segfaults at runtime, or at least make
them more obvious.
Also, remove unused c-style casts in touched lines.
Also, add CHECK_NONFATAL checks, to turn segfault crashes into an
recoverable runtime error with debug information.
fad1903b8a fuzz: Avoid timeout in bitdeque (MarcoFalke)
Pull request description:
Avoid timeouts such as https://github.com/bitcoin/bitcoin/issues/28812#issuecomment-1842914664
This is done by:
* Limiting the maximum number of iterations if the maximum size of the container is "large" (see the magic numbers in the code).
* Check the equality only once. This should be fine, because if a crash were to happen in the equality check, but the crash doesn't happen if further iterations were run, the fuzz engine should eventually find the crash by truncating the fuzz input.
ACKs for top commit:
sipa:
utACK fad1903b8a
dergoegge:
utACK fad1903b8a
brunoerg:
crACK fad1903b8a
Tree-SHA512: d3d83acb3e736b8fcaf5d17ce225ac82a9f9a2efea048512d2fed594ba6c76c25bae72eb0fab3276d4db37baec0752e5367cecfb18161301b921fed09693045e
3ea54e5db7 net: Add continuous ASMap health check logging (Fabian Jahr)
28d7e55dff test: Add tests for unfiltered GetAddr usage (Fabian Jahr)
b8843d37ae fuzz: Let fuzzers use filter options in GetAddr/GetAddresses (Fabian Jahr)
e16f420547 net: Optionally include terrible addresses in GetAddr results (Fabian Jahr)
Pull request description:
There are certain statistics we can collect by running all our known clearnet addresses against the ASMap file. This could show issues with a maliciously manipulated file or with an old file that has decayed with time.
This is just a proof of concept for now. My idea currently is to run the analysis once per day and print the results to logs if an ASMap file is used.
ACKs for top commit:
achow101:
ACK 3ea54e5db7
mzumsande:
ACK 3ea54e5db7
brunoerg:
crACK 3ea54e5db7
Tree-SHA512: 777acbfac43cc43ce4a0a3612434e4ddbc65f59ae8ffc9e24f21de09011bccb297f0599cbaa82bcf40ef68e5af582c4e98556379db7ceff7d9f97574a1cf8e09
55e3dc3e03 test: Fix test by checking the actual exception instance (Hennadii Stepanov)
Pull request description:
The `system_tests/run_command` test is broken because it passes even with the diff as follows:
```diff
--- a/src/test/system_tests.cpp
+++ b/src/test/system_tests.cpp
@@ -90,7 +90,7 @@ BOOST_AUTO_TEST_CASE(run_command)
});
}
{
- BOOST_REQUIRE_THROW(RunCommandParseJSON("echo \"{\""), std::runtime_error); // Unable to parse JSON
+ BOOST_REQUIRE_THROW(RunCommandParseJSON("invalid_command \"{\""), std::runtime_error); // Unable to parse JSON
}
// Test std::in, except for Windows
#ifndef WIN32
```
The reason of such fragility is that the [`BOOST_REQUIRE_THROW`](https://www.boost.org/doc/libs/1_83_0/libs/test/doc/html/boost_test/utf_reference/testing_tool_ref/assertion_boost_level_throw.html) macro passes even if the command raises an exception in the underlying subprocess implementation, which might have a type derived from `std::runtime_error`.
ACKs for top commit:
maflcko:
lgtm ACK 55e3dc3e03
achow101:
ACK 55e3dc3e03
furszy:
Non-Windows code ACK 55e3dc3e
pablomartin4btc:
ACK 55e3dc3e03
Tree-SHA512: 32f49421bdcc94744c81e82dc10cfa02e3f8ed111974edf1c2a47bdaeb56d7baec1bede67301cc89464fba613029ecb131dedc6bc5948777ab52f0f12df8bfe9
