*** Please remove the following help text before submitting: ***#1
Open
Zaidalamari wants to merge 2174 commits intoZaidalamari:masterfrom
Open
*** Please remove the following help text before submitting: ***#1Zaidalamari wants to merge 2174 commits intoZaidalamari:masterfrom
Zaidalamari wants to merge 2174 commits intoZaidalamari:masterfrom
Conversation
Exercise the case where tasks remain pending and verify that the thread calling Stop() participates in draining the queue
Move the operator<< overloads used by BOOST_CHECK_* out of the unit test machinery test/setup_common, into test/util/common.h. And replace the individual per-type ToString() overloads with a single concept-constrained template that covers any type exposing a ToString() method. This is important to not add uint256.h and transaction_identifier.h dependencies to the shared test/util/common.h file. Co-authored-by: furszy <matiasfurszyfer@protonmail.com>
Avoid providing the entire unit test framework dependency to tests that only require access to the HasReason utility class. E.g. reverselock_tests.cpp, sync_tests.cpp, util_check_tests.cpp, util_string_tests.cpp, and script_parse_tests.cpp only require access to HasReason and nothing else.
Submit tasks to a non-started, interrupted, or stopped pool and verify the proper error is always returned.
Useful for debugging issues. Co-authored-by: Matias Furszyfer <matiasfurszyfer@protonmail.com>
The standard and fuzz matrix jobs share the same github.job value (windows-native-dll), so both try to save the vcpkg tools cache with the same key. Since the tools are identical across build types, let them share a single cache entry by restricting the save to the standard job only.
Can be reviewed via --color-moved=dimmed-zebra
This does not change any behavior.
… matrix jobs 17a079c ci: fix vcpkg tools cache key collision between windows matrix jobs (will) Pull request description: The standard and fuzz matrix jobs share the same github.job value (windows-native-dll), so both try to save the vcpkg tools cache with the same key. Since the tools are identical across build types, let them share a single cache entry by restricting the save to the standard job only. ACKs for top commit: maflcko: lgtm ACK 17a079c hebasto: ACK 17a079c, this should fix issues like #34559 (comment). Tree-SHA512: 2e83846bfc88947b4bc321baa23563e0c093cd4f55f11f8105c2ecf867b78028aa71aa4780f928103b7a9f1f2e3cf72dbb072f05e7925bc1d00069234acf23c9
0a8d303 test: fix test_limit_enforcement_package (Greg Sanders) Pull request description: The current test has a couple issues: 1) the parent_tx_good is regenerating the exact same transaction that is already in the cluster, so it's resulting in no replacements on submission 2) once fixed, the additional fee needs to be allocated to the parent transaction in the package, not the child. If the RBF fees are allocated to the child, this triggers the package RBF logic, which requires no in-mempool ancestors to be present. Fix the bug and add a few assertions to protect against regressions. ACKs for top commit: bensig: ACK 0a8d303 achow101: ACK 0a8d303 sipa: ACK 0a8d303 Tree-SHA512: 0ba184d82edc5a502e9119a6876e80c4564c876fa51ee39293d47bd30c18bf3ded50fbd2f6f2a3394784fad05d8f6370a90682068b30358b077280abd2477252
Appveyor is not longer used however the test still requires to check for permissions including 666 as otherwise the tests fail on Windows
… dependency cleanup 408d5b1 test: include response body in non-JSON HTTP error msg (Matthew Zipkin) 9dc653b test: threadpool, add coverage for all Submit() errors (furszy) ce2a984 test: cleanup, use HasReason in threadpool_tests.cpp (l0rinc) d9c6769 test: refactor, decouple HasReason from test framework machinery (furszy) dbbb780 test: move and simplify BOOST_CHECK ostream helpers (Hodlinator) 3b7cbca test: ensure Stop() thread helps drain the queue (seduless) ca101a2 test: coverage for queued tasks completion after interrupt (furszy) bf2c607 threadpool: active-wait during shutdown (furszy) e88d274 test: add threadpool Start-Stop race coverage (furszy) 8cd4a43 threadpool: guard against Start-Stop race (furszy) 9ff1e82 test: cleanup, block threads via semaphore instead of shared_future (l0rinc) Pull request description: A few follow-ups to #33689, includes: 1) `ThreadPool` active-wait during shutdown: Instead of just waiting for workers to finish processing tasks, `Stop()` now helps them actively. This speeds up the JSON-RPC and REST server shutdown, resulting in a faster node shutdown when many requests remain unhandled. This wasn't included in the original PR due to the behavior change this introduces. 2) Decouple `HasReason` from the unit test framework machinery This avoids providing the entire unit test framework dependency to low-level tests that only require access to the `HasReason` utility class. Examples are: `reverselock_tests.cpp`, `sync_tests.cpp`, `util_check_tests.cpp`, `util_string_tests.cpp`, `script_parse_tests.cpp` and `threadpool_tests.cpp`. These tests no longer gain access to unnecessary components like the chainstate, node context, caches, etc. It includes l0rinc's `threadpool_tests.cpp` `HasReason` changes. 3) Include response body in non-JSON HTTP error messages Straight from pinheadmz [comment](#33689 (comment)), it makes debugging CI issues easier. ACKs for top commit: maflcko: review ACK 408d5b1 🕗 achow101: ACK 408d5b1 hodlinator: re-ACK 408d5b1 Tree-SHA512: 57aa0ef96886f32bf95a0bd7f87c878d31c9df9e34cb96de615eee703ce0824b5cfdf8f5c9cd19a3594559994295b5810c38c94f5efd6291cbbd83a95473357a
```
src/common/netif.cpp:137:51: error: comparison of integers of different signs: 'int64_t' (aka 'long') and 'unsigned long' [-Werror,-Wsign-compare]
137 | for (nlmsghdr* hdr = (nlmsghdr*)response; NLMSG_OK(hdr, recv_result); hdr = NLMSG_NEXT(hdr, recv_result)) {
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/netlink/netlink.h:220:31: note: expanded from macro 'NLMSG_OK'
220 | #define NLMSG_OK(_hdr, _len) NL_ITEM_OK(_hdr, _len, NLMSG_HDRLEN, _NLMSG_LEN)
| ^ ~~~~ ~~~~~~~~~~~~
/usr/include/netlink/netlink.h:203:10: note: expanded from macro 'NL_ITEM_OK'
203 | ((_len) >= _hlen && _LEN_M(_ptr) >= _hlen && _LEN_M(_ptr) <= (_len))
| ~~~~ ^ ~~~~~
1 error generated.
```
Happens on FreeBSD 15.0, with the default compiler (Clang 19).
On FreeBSD 14, `/usr/include/netlink/netlink.h` contains:
```
#define NLMSG_HDRLEN ((int)sizeof(struct nlmsghdr))
```
On FreeBSD 15, `/usr/include/netlink/netlink.h` contains:
```
#define NLMSG_HDRLEN (sizeof(struct nlmsghdr))
```
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
8834e4e test: remove appveyor reference in comment (Max Edwards) Pull request description: Appveyor is not longer used however the test still requires to check for permissions including 666 as otherwise the tests fail on Windows Fixes: #32576 ACKs for top commit: maflcko: lgtm ACK 8834e4e hebasto: ACK 8834e4e. Tree-SHA512: 655b44e52da5e0c6c11c79bb4f92c701c6e0e66dce8d7791ccf1d64e4561fe4d1d5f37c1317bead89c88e4d7208a278925168b419482a6be17abf93d0ebc5dfa
45133c5 doc: clarify `git range-diff` add/delete output (Lőrinc) Pull request description: ### Problem Range diffs in git are useful after PR rebases, but it has an easy-to-misread failure mode: if it cannot match a commit between the old and new ranges, it will show the old commit as removed (<) and the new commit as added (>), without showing any patch contents for that commit. It can look like there were no code changes when in reality the commit was just treated as unrelated and needs full re-review. ### Example ```bash git fetch upstream ff338fd dd76338 git range-diff ff338fd...dd76338 ``` This produced output like: ```patch 1: 0ca4295 = 93: 139aa4b bench: add on-disk `HaveInputs` benchmark 2: 4b32181 < -: ---------- test: add `HaveInputs` call-path unit tests -: ---------- > 94: 277c57f test: add `HaveInputs` call-path unit tests 3: 8c57687 ! 95: c0c94ec dbwrapper: have `Read` and `Exists` reuse `ReadRaw` @@ Metadata ## Commit message ## dbwrapper: have `Read` and `Exists` reuse `ReadRaw` - `ExistsImpl` was removed since it duplicates `CDBWrapper::ReadImpl` (except that it copies the resulting string on success, but that will be needed for caching anyway). + `ExistsImpl` was removed since it duplicates `CDBWrapper::ReadImpl`. ``` Even though the subject matches, there is no diff shown because the commits did not match - the reviewer could think that only the commit message was changed. This should be treated as **unmatched** rather than **unchanged**. If you expected a match, you can try increasing the search effort: ```bash git range-diff --creation-factor=95 ff338fd...dd76338 ``` which would show for example: ```patch 1: 0ca4295 = 93: 139aa4b bench: add on-disk `HaveInputs` benchmark 2: 4b32181 ! 94: 277c57f test: add `HaveInputs` call-path unit tests @@ Commit message The tests document that `HaveInputs()` consults the cache first and that a cache miss pulls from the backing view via `GetCoin()`. + Co-authored-by: Novo <eunovo9@gmail.com> + ## src/test/coins_tests.cpp ## @@ src/test/coins_tests.cpp: BOOST_FIXTURE_TEST_CASE(ccoins_flush_behavior, FlushTest) } } -+BOOST_AUTO_TEST_CASE(ccoins_haveinputs_cache_miss_uses_base_getcoin) ++BOOST_AUTO_TEST_CASE(ccoins_cache_behavior) ``` ### Fix This PR updates `doc/productivity.md` to raise awareness and document this pitfall and mentions `--creation-factor` as a knob to try when the output seems unexpectedly empty. ACKs for top commit: maflcko: review ACK 45133c5 🏦 Sjors: ACK 45133c5 rkrux: crACK 45133c5 sedited: ACK 45133c5 Tree-SHA512: 52dcf6db51425a3ac9789627f80233fb1e3437f7a351acf4a761504d9917837aa1ff8c964605a842ee099fae9842946784f7603f9bffa7051429b2f04b7900be
fa6af85 refactor: Use static_cast<decltype(...)> to suppress integer sanitizer warning (MarcoFalke) fa69297 util: Fix UB in SetStdinEcho when ENOTTY (MarcoFalke) Pull request description: The call to `tcgetattr` may fail with `ENOTTY`, leaving the struct possibly uninitialized (UB). Fix this UB by returning early when `isatty` fails, or when `tcgetattr` fails. (Same for Windows) This can be tested by a command that fails valgrind before the change and passes after: ``` echo 'pipe' | valgrind --quiet ./bld-cmake/bin/bitcoin-cli -stdinrpcpass uptime ACKs for top commit: achow101: ACK fa6af85 l0rinc: lightly tested code review ACK fa6af85 sedited: ACK fa6af85 Tree-SHA512: 76e2fbcb6c323b17736ee057dbd5e932b2e8cbb7d9fe4488c1dc7ab6ea928a3cde7e72ca0a63f8c8c78871ccb8b669263b712c0e1b530d88f2d45ea41f071201
Normally, when a symlinked test script is executed directly (e.g., `./bld-cmake/test/functional/wallet_disable.py`), Python's default behavior is to resolve the symlink of the script itself, setting `sys.path[0]` to the directory containing the physical source file. Consequently, the test framework util.py is imported from the source tree, and `Path(__file__).parents[3]` correctly resolves to the source root. However, `feature_framework_testshell.py` is unique because it manually inserts `Path(__file__).parent` into `sys.path`. That refers to the build tree and when importing the test framework util.py, the `Path(__file__).parents[3]` will incorrectly point to the build directory instead of the source root. Use `.resolve()` to ensure the Valgrind suppressions file path is always calculated relative to the physical source file, regardless of how the framework was imported.
The wallet doesn't only rebroadcast transactions it created, but also relevant transactions received via p2p. Since this is not self-evident, add test coverage for it.
…pl() c1361fc netif: fix compilation warning in QueryDefaultGatewayImpl() (MarcoFalke) Pull request description: ``` src/common/netif.cpp:137:51: error: comparison of integers of different signs: 'int64_t' (aka 'long') and 'unsigned long' [-Werror,-Wsign-compare] 137 | for (nlmsghdr* hdr = (nlmsghdr*)response; NLMSG_OK(hdr, recv_result); hdr = NLMSG_NEXT(hdr, recv_result)) { | ^~~~~~~~~~~~~~~~~~~~~~~~~~ /usr/include/netlink/netlink.h:220:31: note: expanded from macro 'NLMSG_OK' 220 | #define NLMSG_OK(_hdr, _len) NL_ITEM_OK(_hdr, _len, NLMSG_HDRLEN, _NLMSG_LEN) | ^ ~~~~ ~~~~~~~~~~~~ /usr/include/netlink/netlink.h:203:10: note: expanded from macro 'NL_ITEM_OK' 203 | ((_len) >= _hlen && _LEN_M(_ptr) >= _hlen && _LEN_M(_ptr) <= (_len)) | ~~~~ ^ ~~~~~ 1 error generated. ``` Happens on FreeBSD 15.0, with the default compiler (Clang 19). On FreeBSD 14, `/usr/include/netlink/netlink.h` contains: ``` #define NLMSG_HDRLEN ((int)sizeof(struct nlmsghdr)) ``` On FreeBSD 15, `/usr/include/netlink/netlink.h` contains: ``` #define NLMSG_HDRLEN (sizeof(struct nlmsghdr)) ``` ACKs for top commit: maflcko: lgtm ACK c1361fc hebasto: ACK c1361fc. Tree-SHA512: f8f00e2106fdf91550ab388a65bb8408fcf8c4557da9614e2e1dd90e70fc010dff72bfabbbec4315335afdedb546f7b554f5c98133c5aa1d3077c5641d94b956
…runner switch fa18be2 test: Fix typo (MarcoFalke) fac9326 ci: Set TEST_RUNNER_PORT_MIN in test-each after cirrus runner switch (MarcoFalke) Pull request description: This is needed after the recent switch to cirrus runners in the task in commit c8c9c1e. Otherwise, the CI will fail: ``` node1 stderr Error: Unable to bind to 127.0.0.1:12321 on this computer. Bitcoin Core is probably already running. ``` (https://github.com/bitcoin/bitcoin/actions/runs/22398358349/job/64837827234?pr=31723#step:9:2605) Also, include a random second commit, so that the CI task is run in this pull. ACKs for top commit: l0rinc: ACK fa18be2 willcl-ark: ACK fa18be2 Tree-SHA512: 6b63f645bf62d3e951ca155cddf3dc562b7ce675ccae4f9179e2202679685b5c147844eb350bd219b173fe2bb976376d0caa073d3e827a48c13aa015f4745b2c
…est shell fab51e4 test: Move valgrind.supp to the other sanitizer_suppressions files (MarcoFalke) fa9cf81 test: Add missing resolve() to valgrind.supp file (MarcoFalke) Pull request description: (see commit message for details) Can be tested via: ``` ./bld-cmake/test/functional/feature_framework_testshell.py --valgrind ``` Before: ``` bitcoind exited with status 1 during initialization. ==1735813== FATAL: can't open suppressions file "/b-c/b-c-2/bld-cmake/contrib/valgrind.supp" ``` After: (passes) Also, move the suppression file to all the other suppression files. ACKs for top commit: frankomosh: Re-ACK fab51e4 Tree-SHA512: d3e3e130c0e2292ca3ab9e80d2ebec6b4edc7914280ed90fb4af8a65cd1c9edd19064d86375a6787b864925fe0e47bab2321f89b9be8d1613a0aebf4ec2443fe
fa36ade ci: [refactor] Drop last use of pwsh (MarcoFalke) fae31b1 ci: [refactor] Move github_import_vs_env to python script (MarcoFalke) Pull request description: The use of pwsh was a bit confusing and inconsistent around the exit code. See also #32672 I think it is fine to drop it and purely use Bash/Python. This also moves a bit of code from the github yaml to the python script. ACKs for top commit: m3dwards: Looks good! re-ACK fa36ade hodlinator: re-ACK fa36ade Tree-SHA512: 78edffc60c58c476b0acca5224150169d154b0b818114844a04295af9ba19b7cdf1fb2afb738f6cafd6172f0f477d546018ebf95061eb5bd8bbb35e065a129d4
…rfaceQueue() Co-Authored-By: stickies-v <stickies-v@protonmail.com>
For RPCResults, the type may be ELISION, which is confusing and brittle:
* The elision should only affect the help output, not the type.
* The type should be the real type, so that type checks can be run on
it.
Fix this issue by introducing a new print_elision option and using it
in decodepsbt.
This change will ensure that RPCResult::MatchesType is properly run.
Also, this clarifies the RPC output minimally:
```diff
--- a/decodepsbt
+++ b/decodepsbt
@@ -35,7 +35,7 @@ Result:
"inputs" : [ (json array)
{ (json object)
"non_witness_utxo" : { (json object, optional) Decoded network transaction for non-witness UTXOs
- ...
+ ... The layout is the same as the output of decoderawtransaction.
},
"witness_utxo" : { (json object, optional) Transaction output for witness UTXOs
"amount" : n, (numeric) The value in BTC
```
Without this, NodeFullyConnected() filters out every fuzz-constructed node, making ForEachNode's callback unreachable (0/1.13M branch hits from my end).
Replace the tabs with spaces.
sync.h is low-level and should not require any other subsystems. Move the lone remaining logging call to the .cpp. Any cost incurred by an additional function call should be trivial compared to the logging itself.
f55c891 lint: more reuse of SHARED_EXCLUDED_SUBTREES (fanquake) 8864917 lint: add missing ipc/test to grep_boost_fixture_test_suite (fanquake) ecefc12 lint: fix lint issue in lint script (fanquake) ee8c22e contrib: fix whitespace issues in scripts (fanquake) 04e2118 lint: remove excluded .cpp/.h files from whitespace check (fanquake) Pull request description: The `.cpp`/`.h` have been fixed since #32482 and 5d25a82. Fix other offending files. ACKs for top commit: stickies-v: re-ACK f55c891 maflcko: re-ACK f55c891 🗿 kevkevinpal: ACK [f55c891](f55c891) sedited: Re-ACK f55c891 Tree-SHA512: 975c217d1cc82433b7960dad47976254d36526f3bd156133dcd4eb51b34616beb89e1449c245deb8b41ab6e1f746a96a0b629013e1996f355e521d075f4692c8
… IsTerrible 6202acd test: addrman: successive failures in the last week for IsTerrible (brunoerg) f611d3b refactor: addrman: move consts to .h (brunoerg) Pull request description: This PR adds test coverage for the case that an address is considered terrible if we had N successive failures in the last week. It kills the following mutant (https://corecheck.dev/mutation/src/addrman.cpp#L88): ```diff diff --git a/src/addrman.cpp b/src/addrman.cpp index e3981e6..f8045491c1 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -65,7 +65,7 @@ bool AddrInfo::IsTerrible(NodeSeconds now) const } if (now - m_last_success > ADDRMAN_MIN_FAIL && nAttempts >= ADDRMAN_MAX_FAILURES) { // N successive failures in the last week - return true; + return false; } return false; ``` ACKs for top commit: frankomosh: re-ACK 6202acd naiyoma: tACK 6202acd danielabrozzoni: tACK 6202acd sedited: ACK 6202acd Tree-SHA512: b4736ef91b75ba4c060dc18a2b796aee94d0d8be5ca58b9b873156248cd0dc6910595595e0f56d75bffff4f94c035319ac8428f7d79f07fe685bdba27a188829
bde35d6 depends: capnp 1.4.0 (fanquake) Pull request description: Update capnp in depends to [`1.4.0`](https://github.com/capnproto/capnproto/releases/tag/v1.4.0). It contains a number of bugfixes, and fixes for 2 CVEs, of which I think only `Fix benign(?) buffer overrun in async readMessage()` is relevant to us, and it seems to be considered benign: > This is technically undefined behavior (a buffer overrun), but we suspect that it is benign with all known memory allocators. In C++, a zero-sized allocation (made with `operator new(0)`, as is the case here) is required to return a unique pointer, different from any other such allocation. Because of this, all common memory allocators round up a zero-byte allocation to a word-sized allocation (32-bit or 64-bit, depending on the architecture). The overrun written to this allocation was exactly one pointer in size, so always fits into the actual allocation space. > Nevertheless, the code is in fact relying on undefined behavior, and it is theoretically possible that some memory allocator implements zero-sized allocations in a way that would make this overrun dangerous. See capnproto/capnproto@release-1.3.0...release-1.4.0 for all changes since 1.3.0. ACKs for top commit: sedited: ACK bde35d6 janb84: ACK bde35d6 hebasto: ACK bde35d6. Tree-SHA512: 33a6c12684b9a6046a38c3b9dd1a5730db352eae07b5dbfe7244228fde3d1627d039c0e0ba7d35fe0968f91a0f476c239fa8f2e356a37b8ac975ac268d271bc2
Replace the hardcoded x86_64 binary name with $(uname --machine) so the correct binary is downloaded when building the lint container, where at all possible.
5518753 ci: Use arch-appropriate binaries in lint install (will) Pull request description: In testing #34547 it has been observed that the lint container does not run on aarch64-linux without `qemu binfmt` (or similar). This is because some tools are hardcoded to download x64 linux binaries. This has meant the linter works fine on: - x64 linux - aarch64 MacOS (via Rosetta) - platforms using qemu But does not work on e.g. aarch64-linux _without qemu_. `shellcheck`` offer many platforms: https://github.com/koalaman/shellcheck/releases/tag/v0.11.0 and `mlc` offers are least x64 and aarch64 linux https://github.com/becheran/mlc/releases/tag/v1.2.0. Try to download the correct binary for the platform using `uname` detection. This should see the linter work on native aarch64 + amd64, whilst maintaining current (emulated) compatibility. ACKs for top commit: maflcko: lgtm ACK 5518753 Tree-SHA512: 636cccbed3ffff995549c666b0cad1aa9790291a73a0f2212f0374c8878bd916c04e4ecb17fac1611fc2d72d363cececeeaa997af918ad4225355231376ff7b0
685a44c fuzz: set fSuccessfullyConnected in connman harness (frankomosh) Pull request description: The connman fuzz harness never sets `fSuccessfullyConnected=true` on nodes added through `AddTestNode()`. `NodeFullyConnected()` gates `ForEachNode()` on that flag, making its callback unreachable in the current harness. Set `fSuccessfullyConnected=true` before `AddTestNode()` to simulate a node that has completed the version handshake. ACKs for top commit: maflcko: lgtm ACK 685a44c sedited: ACK 685a44c marcofleon: ACK 685a44c Tree-SHA512: 2c696b8674cb465f468642b5fd37a467bc34dcbf61dc901d784fd2fe0dd13ced5cd6bd9873bcce0f8e60e25d450052e9a562ece08abeb2ab6472e584dba65c40
CMake version 4.3 deprecated the imported target `Sqlite::Sqlite3`. Use the preferred name `Sqlite3::Sqlite3` instead and provide an alias for older versions of CMake. Also define the same alias when using vcpkg.
Currently arguments passed through the validation interface are copied three times. Once on capture in the event, again when the event itself is copied into the task runner lambda, and when the various arguments used by the logging statement are copied into the lambda. This change avoids the variables captured by the event being copied again. Next to avoiding needless copies, this is done in preparation of the following two commits, which seek to clarify the ownership semantics of the blocks passed through the validation interface. Co-authored-by: stickies-v <stickies-v@protonmail.com>
This makes existing behaviour of the block's destructor triggering on the scheduler thread more explicit by moving it to the thread. The scheduler thread doing so is useful, since it does not block the thread doing validation while releasing a block's memory. Previously, both the caller and the queued event lambda held copies of the shared_ptr. The block would typically be freed on the scheduler thread - but only because it went out of scope before the queued event on the scheduler thread ran. If the scheduler ran first, the block would instead be freed on the validation thread. Now, ownership is transferred at each step when invoking the BlockConnected signal: connected_blocks yields via std::move, BlockConnected takes by value, and the event lambda move-captures the shared_ptr. Though it is possible that this only decrements the block's reference count, blocks are also read from disk in `ConnectTip`, which now explicitly results in their memory being released on the scheduler thread.
This makes existing behaviour of the block's destructor triggering on the scheduler thread more explicit by moving it to the thread. The scheduler thread doing so is useful, since it does not block the thread doing validation while releasing a block's memory. DisconnectTip already creates and destroys the block itself, so moving it into the validation signals is well scoped.
32debfa fuzz: set whitelist permissions on connman target (Bruno Garcia) Pull request description: I noticed that `AddWhitelistPermissionFlags` was always being called with an empty `ranges` (whitelist permissions). This function is called when connecting to a node (`ConnectNode`), and if the connection is a manual one, it uses the `vWhitelistedRangeOutgoing` to populate the permissions. However, since we were not setting any whitelist permission on the target, this vector is always empty. This PR improves this target by populating both `vWhitelistedRangeIncoming` and `vWhitelistedRangeOutgoing`. ACKs for top commit: Crypt-iQ: utACK 32debfa maflcko: lgtm ACK 32debfa frankomosh: crACK 32debfa Tree-SHA512: 400ce780b9ae41d849ccad04258da4ad92d9bf780bfdeb9bb9c1684722bcc02ae45f13081b809f9e16b28078bd4efb928c6c7e1c3da638307b6c11b193d6dc04
faea12e test: Fixup docs for NodeClockContext and SteadyClockContext (MarcoFalke) eeeeb2a fuzz: Use NodeClockContext (MarcoFalke) fa4fae6 test: Add NodeClockContext (MarcoFalke) Pull request description: Iterating over fuzz inputs will usually be done in the same process. As the mocktime is global, it can theoretically leak from one fuzz input run into the next run, making it less deterministic. Fix this issue, by adding and using a context manager to handle the mocktime and reset it before the end. This refactor should not change any behavior. ACKs for top commit: seduless: re-ACK faea12e dergoegge: utACK faea12e brunoerg: code review ACK faea12e Tree-SHA512: e222c4e4217a504d058b30f1e975dfdfff019363c82385bd62f368b16fb029c46a5d1b43cd773dbdd9efcd7f968d46dbe2c75812971696b1b879b8f081fc6b1b
This is less confusing than the verbose=0 alias.
…syncio.WindowsSelectorEventLoopPolicy(), move loop creation fa050da test: Move event loop creation to network thread (MarcoFalke) fa9168f test: Use asyncio.SelectorEventLoop() over deprecated asyncio.WindowsSelectorEventLoopPolicy() (MarcoFalke) Pull request description: It is deprecated according to https://docs.python.org/3.14/library/asyncio-policy.html#asyncio.WindowsSelectorEventLoopPolicy The replacement exists since python 3.7 Also, move the event loop creation to happen in the thread that runs the loop. ACKs for top commit: l0rinc: ACK fa050da sedited: ACK fa050da Tree-SHA512: dce25596a04e8f133630d84c03a770185a81b1bcd0aae975f0dbdd579d22b7b79a9b1172abf46c61d0845d3f5ab4a6414fa0f17c59f0ea0f6fa9bdcac085a2a7
fadf901 rpc: Run type check on decodepsbt result (MarcoFalke) fa4d589 refactor: Introduce TxDocOptions (MarcoFalke) fa8250e refactor: Add and use RPCResultOptions (MarcoFalke) Pull request description: For RPCResults, the type may be ELISION, which is confusing and brittle: * The elision should only affect the help output, not the type. * The type should be the real type, so that type checks can be run on it. Fix this issue by introducing a new print_elision option and using it in `decodepsbt`. This change will ensure that `RPCResult::MatchesType` is properly run. Can be tested by introducing a bug: ```diff diff --git a/src/core_io.cpp b/src/core_io.cpp index 7492e9c..4927b70c8e 100644 --- a/src/core_io.cpp +++ b/src/core_io.cpp @@ -436,2 +436,3 @@ void TxToUniv(const CTransaction& tx, const uint256& block_hash, UniValue& entry entry.pushKV("version", tx.version); + entry.pushKV("bug", "error!"); entry.pushKV("size", tx.ComputeTotalSize()); ``` And then running (in a debug build) `decodepsbt cHNidP8BAAoCAAAAAAAAAAAAAA==` Before, on master: passes Now, on this pull: Properly detects the bug ACKs for top commit: nervana21: tACK fadf901 achow101: ACK fadf901 willcl-ark: ACK fadf901 satsfy: re-ACK fadf901 seduless: re-ACK fadf901 Tree-SHA512: 4fb000dba9fe39bcd2bac72e2d88553f54134a250c985b4ca7150b483d7185009047d8fe4ba75c522bfc26706de20c913b8905e7552ab0c41802ae744cb92038
faf71d6 test: [refactor] Use verbosity=0 named arg (MarcoFalke) 99996f6 test: Fix intermittent issue in feature_assumeutxo.py (MarcoFalke) Pull request description: The test has many issues: * It fails intermittently, due to the use of `-stopatheight` (#33635) * Using `-stopatheight` is expensive, because it requires two restarts, making the test slow Fix all issues by using `dumb_sync_blocks` and avoid the two restarts. Fixes #33635 Diff to test: ```diff diff --git a/src/util/tokenpipe.cpp b/src/util/tokenpipe.cpp index c982fa6..1dc12ea1d5 100644 --- a/src/util/tokenpipe.cpp +++ b/src/util/tokenpipe.cpp @@ -4,2 +4,3 @@ #include <util/tokenpipe.h> +#include <util/time.h> @@ -60,2 +61,3 @@ int TokenPipeEnd::TokenRead() ssize_t result = read(m_fd, &token, 1); + UninterruptibleSleep(999ms); if (result < 0) { ``` On master: Test fails On this pull: Test passes ACKs for top commit: fjahr: Code review ACK faf71d6 sedited: ACK faf71d6 Tree-SHA512: 8f7705d2b3f17881134d6e696207fe6710d7c4766f1b74edf5a40b4a6eb5e0f4b12be1adfabe56934d27abc46e789437526bcdd26e4f8172f41a11bc6bed8605
498b6eb cmake: Migrate away from deprecated SQLite3 target (Daniel Pfeifer) Pull request description: CMake version 4.3 deprecated the imported target `Sqlite::Sqlite3`. Use the preferred name `Sqlite3::Sqlite3` instead and provide an alias for older versions of CMake. Also define the same alias when using vcpkg. ACKs for top commit: hebasto: ACK 498b6eb, tested on Fedora 43 using CMake 3.22.6 and 4.3.0. Tree-SHA512: 8ec0e9673ea39c4383d2995d804498675f9bcd1196ec586f338cb3fa63aa632f160dd179bda3c2d70d932d8b1ac821c15d8247e7302465420937c9227f1bf20a
f3bf63e kernel: acquire coinstats cursor and block info atomically (w0xlt) 5e77072 rpc: fix race condition in gettxoutsetinfo (w0xlt) Pull request description: Fixes #34263 A `CHECK_NONFATAL` assertion failure could occur in `gettxoutsetinfo` when a new block was connected between capturing `pindex` and validating it in `GetUTXOStats()`. The fix removes the early `pindex` capture since `ComputeUTXOStats()` independently fetches the current best block under lock. The response now uses `stats.hashBlock` and `stats.nHeight` (the actual computed values) instead of the potentially stale `pindex`. ACKs for top commit: sedited: ACK f3bf63e fjahr: utACK f3bf63e rkrux: Concept ACK f3bf63e for removal of the race condition. Tree-SHA512: c2d5cd5a1b4b4f1c22023c03970fea400a0b78005fa3d09d6567255615ab461c01b584d8a158651ee08769ec86fc4a1200f640ad58fdaa4879c335d90c891f6a
79467e3 threading: never require logging from sync.h (Cory Fields) Pull request description: This is an updated version of #34793 after stickies-v [pointed out that the approach there was broken](#34793 (comment)). sync.h is low-level and should not require any other subsystems. Move the lone remaining logging call to the .cpp. Any cost incurred by an additional function call should be trivial compared to the logging itself. ACKs for top commit: stickies-v: re-ACK 79467e3 , thanks for doing the modernization right away 👍 sedited: ACK 79467e3 Tree-SHA512: 53e4b19cbf6ec81ce6aee06092a07e56877e2247a2614a148ba25c276dc5cf00d68445f9cd43dbd71e4481da74b788e3e20d3634484760d037f94d44bf13ea9c
779e782 fuzz: wallet: add target for `MigrateToDescriptor` (brunoerg) Pull request description: This PR adds fuzz coverage for the scriptpubkeyman migration (`MigrateToDescriptor`). Note that it's a test for the migration of the scriptpubkey manager, not for the whole migration process as tried in #29694, because: 1) The wallet migration deals with DBs which is expensive for fuzzing (was getting around 3 exec/s); 2) Mocking would require lots of refactors. This target loads keys, HDChain (even inactive ones), watch only and might add tons of different scripts, then calls `MigrateToDescriptor`. It does not play with encrypted stuff because it would make the target super slow. Also, after the migration there are some assertions that would work as a regression test for #31452, for example. ACKs for top commit: frankomosh: Code Review ACK 779e782 marcofleon: reACK 779e782 Tree-SHA512: 08ef5166602c21658765bc063c5421e81055d094d346c4e2a28215209c6b7768b99a424f3ba47cf718dc8d827a588da22394ba23402a40a71a976d80d65e6c2e
d6f680b validation: Move block into BlockDisconnected signal (sedited) 4d02d2b validation: Move block into BlockConnected signal (sedited) 8b0fb64 validation: Move validation signal events to task runner (sedited) Pull request description: This enforces behaviour that is currently already implicit: The destructor for blocks runs mostly in the [scheduler thread](https://bitcoin-dev-tools.github.io/benchcoin/results/pr-176/20472174834/mainnet-default-instrumented-head-flamegraph.svg?x=2762391536960&y=684). The change should make it a bit clearer what the ownership semantics for these validation signals are. `BlockConnected` already takes a reference to a block that is emplaced in `connected_blocks`. Once `connected_blocks` is iterated through, it is not reused. Similarly `BlockDisconnected` currently takes a reference to a block that is discarded after the call to it. Note that this does not give the guarantee that blocks' lifetimes are extended by other means once they are connected. For example after IBD, the block's lifetime is extended in net_processing's `m_most_recent_block` and `ActivateBestChain` itself takes a copy of the block's shared pointer, meaning its caller may delay de-allocation. ACKs for top commit: maflcko: re-review ACK d6f680b 🔌 stickies-v: re-ACK d6f680b frankomosh: Re-ACK d6f680b Tree-SHA512: 9209a7d23e7af0d76fa70dff958b1329f38ef29ccc49b5a32bcf9f349d59cc2bf70464ebdb130d26077c0ff9362ce9211472231d375ff1c9c89c0ec3020eac80
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
20 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.