Should have silent merge failure with master#5
Open
Conversation
…vations While reviewing the MuSig2 descriptors PR 31244, I realized that the enum `DeriveType` here logically refers to the derive type for ranged descriptors. This became evident to me while going through the implementations of `IsRange` & `IsHardened` functions of `BIP32PubkeyProvider`, and the `ParsePubkeyInner` function. Initially I got confused by reading `IsRange` translating to `!= DeriveType::NO`, but later realised it specifically referred to the presence of ranged derivations. I propose explicitly mentioning "RANGED" in the values of the `DeriveType` enum would make it easier to parse the descriptors code. This enum is used in one file only - `script/descriptors.cpp`. That's why I explicitly passed it as the argument in the `sed` commands in the script below. -BEGIN VERIFY SCRIPT- sed -i 's/HARDENED\b/HARDENED_RANGED/g' src/script/descriptor.cpp sed -i 's/\bNO\b/NON_RANGED/g' src/script/descriptor.cpp -END VERIFY SCRIPT-
m3dwards
changed the title
m3dwards
force-pushed
the
master
branch
2 times, most recently
from
f61c52b to
bc70695
Compare
m3dwards
force-pushed
the
master
branch
5 times, most recently
from
49e6a7e to
3ad6acc
Compare
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.
When compiling with clang-cl on Windows, `src/util/subprocess.h` emits `-Wunused-private-field` warnings about unused private fields in the `Popen` class.
When compiling with clang-cl on Windows, `src/util/subprocess.h` emits `-Wunused-private-field` warnings about unused private fields in the `Child` class.
Replace the TODO comment in wallet_assumeutxo.py with actual test assertions that verify node and wallet behavior after a restart during assumeutxo background sync. The new tests verify: - Two chainstates exist (background validation not complete) - Background chainstate is still at START_HEIGHT - Snapshot chainstate has synced to at least PAUSE_HEIGHT - Wallets cannot be loaded after restart (expected behavior during background sync because blocks before snapshot are unavailable for rescanning) - Wallet backup from before snapshot height cannot be restored This documents the expected behavior that wallets cannot be loaded after a node restart during assumeutxo background sync, which is an important edge case for users to be aware of. refs bitcoin#28648
m3dwards
pushed a commit
that referenced
this pull request
Jan 27, 2026
… corruption check in fees.dat fa1d17d refactor: Use uint64_t over size_t for serialize corruption check in fees.dat (MarcoFalke) Pull request description: Serialization should not behave differently on different architectures. See also the related commit 3789215. However, on fees.dat file corruption, 32-bit builds may run into an unsigned integer overflow and report the wrong corruption reason, or may even silently continue after the corruption. This is a bit hard to reproduce, because 32-bit platforms are rare and most of them don't support running the unsigned integer overflow sanitizer. So the possible options to reproduce are: * Run on armhf and manually annotate the code to detect the overflow * Run on i386 with the integer sanitizer (possibly via `podman run -it --rm --platform linux/i386 'debian:trixie'`) * Run the integer sanitizer on any 64-bit platform and manually replace type in the affected line by `uint32_t` Afterwards, the steps to reproduce are: ``` export DEBIAN_FRONTEND=noninteractive && apt update && apt install curl wget htop git vim ccache -y && git clone https://github.com/bitcoin/bitcoin.git --depth=1 ./b-c && cd b-c && apt install build-essential cmake pkg-config python3-zmq libzmq3-dev libevent-dev libboost-dev libsqlite3-dev systemtap-sdt-dev libcapnp-dev capnproto libqrencode-dev qt6-tools-dev qt6-l10n-tools qt6-base-dev clang llvm libc++-dev libc++abi-dev -y cmake -B ./bld-cmake -DCMAKE_BUILD_TYPE=Debug -DCMAKE_C_COMPILER='clang' -DCMAKE_CXX_COMPILER='clang++' -DSANITIZERS=undefined,integer,float-divide-by-zero --preset=dev-mode cmake --build ./bld-cmake --parallel $(nproc) curl -fLO 'https://github.com/bitcoin-core/qa-assets/raw/b5ad78e070e4cf36beb415d7b490d948d70ba73f/fuzz_corpora/policy_estimator_io/607473137013139e3676e30ec4b29639e673fa9b' UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" FUZZ=policy_estimator_io ./bld-cmake/bin/fuzz ./607473137013139e3676e30ec4b29639e673fa9b ``` The output will be something like: ``` /b-c/src/policy/fees/block_policy_estimator.cpp:448:25: runtime error: unsigned integer overflow: 346685954 * 219 cannot be represented in type 'unsigned int' #0 0x5b0b1bbe in TxConfirmStats::Read(AutoFile&, unsigned int) /b-c/bld-cmake/src/./policy/fees/block_policy_estimator.cpp:448:25 #1 0x5b0b7d3f in CBlockPolicyEstimator::Read(AutoFile&) /b-c/bld-cmake/src/./policy/fees/block_policy_estimator.cpp:1037:29 #2 0x592a9783 in policy_estimator_io_fuzz_target(std::span<unsigned char const, 4294967295u>) /b-c/bld-cmake/src/test/fuzz/./test/fuzz/policy_estimator_io.cpp:32:32 #3 0x5896ba8e in void std::__invoke_impl<void, void (*&)(std::span<unsigned char const, 4294967295u>), std::span<unsigned char const, 4294967295u>>(std::__invoke_other, void (*&)(std::span<unsigned char const, 4294967295u>), std::span<unsigned char const, 4294967295u>&&) /usr/lib/gcc/i686-linux-gnu/14/../../../../include/c++/14/bits/invoke.h:61:14 #4 0x5896b8eb in std::enable_if<is_invocable_r_v<void, void (*&)(std::span<unsigned char const, 4294967295u>), std::span<unsigned char const, 4294967295u>>, void>::type std::__invoke_r<void, void (*&)(std::span<unsigned char const, 4294967295u>), std::span<unsigned char const, 4294967295u>>(void (*&)(std::span<unsigned char const, 4294967295u>), std::span<unsigned char const, 4294967295u>&&) /usr/lib/gcc/i686-linux-gnu/14/../../../../include/c++/14/bits/invoke.h:111:2 #5 0x5896b44b in std::_Function_handler<void (std::span<unsigned char const, 4294967295u>), void (*)(std::span<unsigned char const, 4294967295u>)>::_M_invoke(std::_Any_data const&, std::span<unsigned char const, 4294967295u>&&) /usr/lib/gcc/i686-linux-gnu/14/../../../../include/c++/14/bits/std_function.h:290:9 #6 0x59845c95 in std::function<void (std::span<unsigned char const, 4294967295u>)>::operator()(std::span<unsigned char const, 4294967295u>) const /usr/lib/gcc/i686-linux-gnu/14/../../../../include/c++/14/bits/std_function.h:591:9 #7 0x5983a0da in test_one_input(std::span<unsigned char const, 4294967295u>) /b-c/bld-cmake/src/test/fuzz/util/./test/fuzz/fuzz.cpp:88:5 #8 0x5983cb80 in main /b-c/bld-cmake/src/test/fuzz/util/./test/fuzz/fuzz.cpp:271:13 bitcoin#9 0xf75aecc2 (/lib/i386-linux-gnu/libc.so.6+0x24cc2) (BuildId: 2dc5f2945fad35c1b07d1a5a32520b3c41afaa75) bitcoin#10 0xf75aed87 in __libc_start_main (/lib/i386-linux-gnu/libc.so.6+0x24d87) (BuildId: 2dc5f2945fad35c1b07d1a5a32520b3c41afaa75) bitcoin#11 0x58932db6 in _start (/b-c/bld-cmake/bin/fuzz+0x235ddb6) (BuildId: 7d8d83a77923f14e99c0de64acbc5f5bfc2cce9b) SUMMARY: UndefinedBehaviorSanitizer: unsigned-integer-overflow /b-c/src/policy/fees/block_policy_estimator.cpp:448:25 ``` Note: This is marked a "refactor", because the code change does not affect 64-bit builds, and on the still remaining rare 32-bit builds today it is extremely unlikely to happen in production. ACKs for top commit: bensig: ACK fa1d17d ismaelsadeeq: utACK fa1d17d luke-jr: Also, utACK fa1d17d as an improvement. Tree-SHA512: 696bf8e0dbe4777c84cb90e313c7f8f9ee90d4b3e64de1222f8472b2d9d0f3a0f6f027fda743dd6ca8c6aab94f404db7a65bb562a76000d9c33a8a39de28d8d4
For key_exp_index to count correctly for miniscript expressions, KeyParser should hold a reference to it.
Simplifies the callsites for incrementing key_exp_index
Since pk(), pk_k(), pkh(), pk_h(), sha256(), ripemd160(), hash256(), hash160(), after(), and older() all are single argument expressions that are parsed immediately, we can use the Expr and Func parsing functions to determine what the arguments of these expressions are, rather than searching for the next closing parentheses. This fixes an issue when pk(), pk_k(), pkh(), and pk_h() include a musig() expression as Expr properly handles nested expressions.
The data embedded is from the latest ASMap file from the asmap-data repository: https://github.com/asmap/asmap-data/blob/main/1755187200_asmap.dat
This can be disabled with -DWITH_EMBEDDED_ASMAP=OFF.
Extend `test-each-commit` to run on every non-head pull request commit.
The PR tip is excluded because it is already covered by other CI jobs.
Runner was changed to a performant cirrus runner.
Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
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.
…windows 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 bitcoin#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
m3dwards
force-pushed
the
250910-silent-merge-test-pr
branch
from
94a6874 to
22f9c50
Compare
…sReason 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 bitcoin#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](bitcoin#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: bitcoin#32576 ACKs for top commit: maflcko: lgtm ACK 8834e4e hebasto: ACK 8834e4e. Tree-SHA512: 655b44e52da5e0c6c11c79bb4f92c701c6e0e66dce8d7791ccf1d64e4561fe4d1d5f37c1317bead89c88e4d7208a278925168b419482a6be17abf93d0ebc5dfa
…te output 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.
…tewayImpl() 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
…cirrus 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
…e for test 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 bitcoin#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 bitcoin@fa36ade hodlinator: re-ACK fa36ade Tree-SHA512: 78edffc60c58c476b0acca5224150169d154b0b818114844a04295af9ba19b7cdf1fb2afb738f6cafd6172f0f477d546018ebf95061eb5bd8bbb35e065a129d4
…' failed`` errors on early startup bbc8f1e ipc mining: Prevent ``Assertion `m_node.chainman' failed`` errors on early startup (Ryan Ofsky) a7cabf9 init refactor: Only initialize node.notifications one time (Ryan Ofsky) c8e332c init refactor: Remove node.init accesss in AppInitInterfaces (Ryan Ofsky) Pull request description: This fixes ``Assertion `m_node.chainman' failed`` errors first reported bitcoin#33994 (comment) when IPC mining methods are called before ChainstateManager is loaded. The fix works by making the `Init.makeMining` method wait until chainstate data is loaded. It's probably the simplest possible fix but other alternatives like moving the wait to `Mining.createNewBlock` were discussed in the thread bitcoin#34661 (comment) and could be implemented later without changes to clients. ACKs for top commit: Sjors: utACK bbc8f1e ismaelsadeeq: ACK bbc8f1e achow101: ACK bbc8f1e Tree-SHA512: 3e2e4e28ccff364b2303efd06ce337a229c28609076638500acb29559f716a15ad99409c6970ce9ad91776d53e3f9d959f1bbbd144ea9a4a2fb578ddbf2da267
fa48f8c test: Add missing timeout_factor to zmq socket (MarcoFalke) Pull request description: Fixes bitcoin#34189 Otherwise, the test may intermittently fail on slow CI systems that have `--timeout-factor=` properly set. It can be tested by running `./bld-cmake/test/functional/interface_zmq.py --timeout-factor=10` with this diff: ```diff diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index c7be6ab..b14cf2aee6 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -166,2 +166,3 @@ void ValidationSignals::SyncWithValidationInterfaceQueue() LOG_EVENT(fmt, local_name, __VA_ARGS__); \ + UninterruptibleSleep(45ms); \ event(); \ diff --git a/test/functional/interface_zmq.py b/test/functional/interface_zmq.py index 6717007..eee377daea 100755 --- a/test/functional/interface_zmq.py +++ b/test/functional/interface_zmq.py @@ -176,3 +176,3 @@ class ZMQTest (BitcoinTestFramework): for sub in subscribers: - sub.socket.set(zmq.RCVTIMEO, recv_timeout*1000) + sub.socket.set(zmq.RCVTIMEO, int(recv_timeout * 1000)) @@ -271,3 +271,3 @@ class ZMQTest (BitcoinTestFramework): [(topic, address) for topic in ["hashblock", "hashtx"]], - recv_timeout=2) # 2 second timeout to check end of notifications + recv_timeout=0.2) # 2 second timeout to check end of notifications self.disconnect_nodes(0, 1) ``` Before this pull: Test fails with `zmq.error.Again: Resource temporarily unavailable` After this pull: Test passes ACKs for top commit: l0rinc: code review ACK fa48f8c achow101: ACK fa48f8c Tree-SHA512: 5ff8bdd807ff4b644daa634bb7b469da3da3915f58afba63a90e662df99cbebc86636e34e2b1b313c8629773aef2a239fb3025226a84d2ec22f6ecd4cea666c4
…_private_broadcast.py da7f70a test: use port 0 for I2P addresses in p2p_private_broadcast.py (Vasil Dimov) a8ebcfd test: let connections happen in any order in p2p_private_broadcast.py (Vasil Dimov) 67696b2 net: extend log message to include attempted connection type (Vasil Dimov) Pull request description: If the following two events happen: * (likely) the automatic 10 initial connections are not made to all networks * (unlikely) the network-specific logic kicks in almost immediately. It is using exponential distribution with a mean of 5 minutes (`rng.rand_exp_duration(EXTRA_NETWORK_PEER_INTERVAL)`). So if both happen, then the 11th connection may not be the expected private broadcast, but a network-specific connection. Fix this by retrieving the connection type from `destinations_factory()`. This is more flexible because it allows connections to happen in any order and does not break if e.g. the 11th connection is not the expected first private broadcast. This also makes the test run faster: before: 19-44 sec now: 10-25 sec because for example there is no need to wait for the initial 10 automatic outbound connections to be made in order to proceed. Fixes: bitcoin#34387 ACKs for top commit: achow101: ACK da7f70a andrewtoth: ACK da7f70a mzumsande: Code Review ACK da7f70a Tree-SHA512: 7c293e59c15c148a438e0119343b05eb278205640658c99336d4caf4848c5bae92b48e15f325fa616cbc9d5f394649abfa02406a76e802cffbd3d312a22a6885
m3dwards
force-pushed
the
250910-silent-merge-test-pr
branch
from
22f9c50 to
e2950dc
Compare
m3dwards
force-pushed
the
master
branch
2 times, most recently
from
4528d85 to
1ca64cf
Compare
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.
This change should not be merged and will cause silent merge failure when PR #7 gets merged