📝 vasild reopened a pull request: "i2p: make a time gap between creating transient sessions and using them"
(https://github.com/bitcoin/bitcoin/pull/32065)
Connecting to an I2P peer consists of creating a session (the
`SESSION CREATE` command) and then connecting to the peer using that
session (`STREAM CONNECT ID=session_id ...`).
This change is only relevant for transient sessions because when a
persistent session is used it is created once and used for all
connections.
Before this change Bitcoin Core would create the session and use it in
quick succession. That is, the `SESSION CREATE` command would be
immediately followed by `STREAM
...
(https://github.com/bitcoin/bitcoin/pull/32065)
Connecting to an I2P peer consists of creating a session (the
`SESSION CREATE` command) and then connecting to the peer using that
session (`STREAM CONNECT ID=session_id ...`).
This change is only relevant for transient sessions because when a
persistent session is used it is created once and used for all
connections.
Before this change Bitcoin Core would create the session and use it in
quick succession. That is, the `SESSION CREATE` command would be
immediately followed by `STREAM
...
💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2005488831)
The issue can be observed in the following scenario on an `aarch64` machine:
```
$ env HOSTS=x86_64-linux-gnu ./contrib/guix/guix-build
...
Building qt...
ninja: error: 'qtbase/src/platformsupport/input/CMakeFiles/InputSupportPrivate.dir/cmake_pch.hxx.gch', needed by 'qtbase/src/platformsupport/input/CMakeFiles/InputSupportPrivate.dir/InputSupportPrivate_autogen/mocs_compilation.cpp.o', missing and no known rule to make it
make: *** [funcs.mk:303: /bitcoin/depends/work/build/x86_64-linux-
...
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2005488831)
The issue can be observed in the following scenario on an `aarch64` machine:
```
$ env HOSTS=x86_64-linux-gnu ./contrib/guix/guix-build
...
Building qt...
ninja: error: 'qtbase/src/platformsupport/input/CMakeFiles/InputSupportPrivate.dir/cmake_pch.hxx.gch', needed by 'qtbase/src/platformsupport/input/CMakeFiles/InputSupportPrivate.dir/InputSupportPrivate_autogen/mocs_compilation.cpp.o', missing and no known rule to make it
make: *** [funcs.mk:303: /bitcoin/depends/work/build/x86_64-linux-
...
💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2740271781)
Some of @fanquake's recent feedback has been addressed.
Additionally, the behaviour when cross-compiling for `HOST=x86_64-w64-mingw32` on a system with a native Vulkan package installed has been fixed. Qt 6 erroneously (a bug?) labels Vulkan as available for cross-compiling. Vulkan has been disabled for `mingw32`, which effectively makes it disabled for all platforms.
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2740271781)
Some of @fanquake's recent feedback has been addressed.
Additionally, the behaviour when cross-compiling for `HOST=x86_64-w64-mingw32` on a system with a native Vulkan package installed has been fixed. Qt 6 erroneously (a bug?) labels Vulkan as available for cross-compiling. Vulkan has been disabled for `mingw32`, which effectively makes it disabled for all platforms.
💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2005514004)
Thanks! [Fixed](https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2740271781).
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2005514004)
Thanks! [Fixed](https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2740271781).
💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2005514457)
Thanks! [Fixed](https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2740271781).
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2005514457)
Thanks! [Fixed](https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2740271781).
💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2005514872)
Thanks! [Reworked](https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2740271781).
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2005514872)
Thanks! [Reworked](https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2740271781).
💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2005525735)
Otherwise, it fails to find XCB:
```
$ make -C depends -j 16 qt
...
CMake Error at qtbase/cmake/QtBuildInformation.cmake:522 (message):
Feature "xcb": Forcing to "ON" breaks its condition:
QT_FEATURE_thread AND TARGET XCB::XCB AND TEST_xcb_syslibs AND QT_FEATURE_xkbcommon_x11
Condition values dump:
QT_FEATURE_thread = "ON"
TARGET XCB::XCB found
TEST_xcb_syslibs = "1"
QT_FEATURE_xkbcommon_x11 = "OFF"
Call Stack (most recent call first):
qtb
...
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2005525735)
Otherwise, it fails to find XCB:
```
$ make -C depends -j 16 qt
...
CMake Error at qtbase/cmake/QtBuildInformation.cmake:522 (message):
Feature "xcb": Forcing to "ON" breaks its condition:
QT_FEATURE_thread AND TARGET XCB::XCB AND TEST_xcb_syslibs AND QT_FEATURE_xkbcommon_x11
Condition values dump:
QT_FEATURE_thread = "ON"
TARGET XCB::XCB found
TEST_xcb_syslibs = "1"
QT_FEATURE_xkbcommon_x11 = "OFF"
Call Stack (most recent call first):
qtb
...
💬 Eunovo commented on pull request "[EXPERIMENTAL] Schnorr batch verification for blocks":
(https://github.com/bitcoin/bitcoin/pull/29491#discussion_r2005563093)
https://github.com/bitcoin/bitcoin/pull/29491/commits/6055f3a6f530818e2805d11debb1fb2ce81fc5c0: This leaves the master thread to do all the "Pubkey recovery"(I think that's what its called?) work in each `batch.Add` call, which is substantial. We can call "secp256k1_xonly_pubkey_parse" when collecting the signatures so the master thread is not stuck doing all the work to parse the XOnlyPubkey.
(https://github.com/bitcoin/bitcoin/pull/29491#discussion_r2005563093)
https://github.com/bitcoin/bitcoin/pull/29491/commits/6055f3a6f530818e2805d11debb1fb2ce81fc5c0: This leaves the master thread to do all the "Pubkey recovery"(I think that's what its called?) work in each `batch.Add` call, which is substantial. We can call "secp256k1_xonly_pubkey_parse" when collecting the signatures so the master thread is not stuck doing all the work to parse the XOnlyPubkey.
📝 willcl-ark opened a pull request: "Accept unordered tracepoints in interface_usdt_utxocache.py"
(https://github.com/bitcoin/bitcoin/pull/32101)
We have encountered an instance where the tracepoints were not collected in the same order they were fired (#31951).
Tracepoint ordering is not guaranteed in userspace for a number of reasons.
As this test does not require a strict collection/processing order collect `expected` and `actual` events into dicts and compare them.
This will gracefully handle both the number of events, and out-of-order events should they reoccur in the future.
Fixes: #31951
(https://github.com/bitcoin/bitcoin/pull/32101)
We have encountered an instance where the tracepoints were not collected in the same order they were fired (#31951).
Tracepoint ordering is not guaranteed in userspace for a number of reasons.
As this test does not require a strict collection/processing order collect `expected` and `actual` events into dicts and compare them.
This will gracefully handle both the number of events, and out-of-order events should they reoccur in the future.
Fixes: #31951
⚠️ mabu44 opened an issue: "Write access to Testing Guide for 29.0 RC"
(https://github.com/bitcoin/bitcoin/issues/32102)
I am currently working on improving the Testing Guide: Bitcoin Core 29.0 Release Candidate.
I would like to request permission to edit the Wiki so I can contribute.
Specifically, I would like to add a section about the new ephemeral dust feature.
Thank you!
(https://github.com/bitcoin/bitcoin/issues/32102)
I am currently working on improving the Testing Guide: Bitcoin Core 29.0 Release Candidate.
I would like to request permission to edit the Wiki so I can contribute.
Specifically, I would like to add a section about the new ephemeral dust feature.
Thank you!
💬 Chand-ra commented on issue "test: No unit test covers BIP342 tapscript signatures":
(https://github.com/bitcoin/bitcoin/issues/32012#issuecomment-2740381794)
> [@Chand-ra](https://github.com/Chand-ra) The taproot script unit tests are largely, and don't live in this repository. You need to clone the QA assets repository (https://github.com/bitcoin-core/qa-assets) and then specify the location of its `unit_test_data` directory in the `DIR_UNIT_TEST_DATA` environment variable when running the unit test binary.
You're right, cloning the qa-asserts repo and executing the following command:
`$ DIR_UNIT_TEST_DATA=./qa-assets/unit_test_data ./build/bin/te
...
(https://github.com/bitcoin/bitcoin/issues/32012#issuecomment-2740381794)
> [@Chand-ra](https://github.com/Chand-ra) The taproot script unit tests are largely, and don't live in this repository. You need to clone the QA assets repository (https://github.com/bitcoin-core/qa-assets) and then specify the location of its `unit_test_data` directory in the `DIR_UNIT_TEST_DATA` environment variable when running the unit test binary.
You're right, cloning the qa-asserts repo and executing the following command:
`$ DIR_UNIT_TEST_DATA=./qa-assets/unit_test_data ./build/bin/te
...
🚀 ryanofsky merged a pull request: "test, refactor: Add TestNode.binaries to hold binary paths"
(https://github.com/bitcoin/bitcoin/pull/31866)
(https://github.com/bitcoin/bitcoin/pull/31866)
👍 dergoegge approved a pull request: "fuzz: Use immediate task runner to increase fuzz stability"
(https://github.com/bitcoin/bitcoin/pull/31841#pullrequestreview-2702662698)
Code review ACK fa4fb6a8f15c295a2a3d3ffd737e115b8be46c1f
The only downside to this is that we diverge from the production setup and e.g. bugs that only occur when fuzzing and using the `SerialTaskRunner` would be masked by this change. We could consider having a compile or runtime option to enable the serial runner and run both versions in CI.
(https://github.com/bitcoin/bitcoin/pull/31841#pullrequestreview-2702662698)
Code review ACK fa4fb6a8f15c295a2a3d3ffd737e115b8be46c1f
The only downside to this is that we diverge from the production setup and e.g. bugs that only occur when fuzzing and using the `SerialTaskRunner` would be masked by this change. We could consider having a compile or runtime option to enable the serial runner and run both versions in CI.
📝 willcl-ark opened a pull request: "ci: run test-each-commit on merge to master"
(https://github.com/bitcoin/bitcoin/pull/32103)
Previously this CI job was checking out the head ref, which meant it is **not** being run on the result of merging into master.
Use the default checkout action `ref` (merge into default branch) to avoid this. Increment the checkout depth to HEAD~~ to compensate for the new merge commit.
This would have likely helped avoid both failures reported in #31946, and seems to me to be a more robust way of running this test job in any case.
(Probably) closes: #31946
cc @maflcko: Is there a hi
...
(https://github.com/bitcoin/bitcoin/pull/32103)
Previously this CI job was checking out the head ref, which meant it is **not** being run on the result of merging into master.
Use the default checkout action `ref` (merge into default branch) to avoid this. Increment the checkout depth to HEAD~~ to compensate for the new merge commit.
This would have likely helped avoid both failures reported in #31946, and seems to me to be a more robust way of running this test job in any case.
(Probably) closes: #31946
cc @maflcko: Is there a hi
...
👍 hodlinator approved a pull request: "contrib: Make deterministic-coverage error messages more readable"
(https://github.com/bitcoin/bitcoin/pull/32074#pullrequestreview-2700285606)
ACK fab6fe3c1676202bd9469c3e9f9454911c92602b
Removes the use of `assert!()`s, `expect()`, `unwrap_or_else()`, error backtraces and `exit(1)` in favor of more Rust-idiomatic *user/environment error handling* through `Result<>`.
Heavy use of `map_err()` and `?` to enable early returns on error results in quite a small diff and pleasant code.
#### Testing
Tested running fuzz-util on non-existent build-dir with expected error. Tried running fuzz executable without coverage support (see i
...
(https://github.com/bitcoin/bitcoin/pull/32074#pullrequestreview-2700285606)
ACK fab6fe3c1676202bd9469c3e9f9454911c92602b
Removes the use of `assert!()`s, `expect()`, `unwrap_or_else()`, error backtraces and `exit(1)` in favor of more Rust-idiomatic *user/environment error handling* through `Result<>`.
Heavy use of `map_err()` and `?` to enable early returns on error results in quite a small diff and pleasant code.
#### Testing
Tested running fuzz-util on non-existent build-dir with expected error. Tried running fuzz executable without coverage support (see i
...
💬 hodlinator commented on pull request "contrib: Make deterministic-coverage error messages more readable":
(https://github.com/bitcoin/bitcoin/pull/32074#discussion_r2004389857)
nit:
Tried running without coverage support compiled in:
```
₿ cargo run --manifest-path ./contrib/devtools/deterministic-fuzz-coverage/Cargo.toml -- $PWD/build_fuzz ../qa-assets/fuzz_corpora/ process_message
Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.03s
Running `contrib/devtools/deterministic-fuzz-coverage/target/debug/deterministic-fuzz-coverage /home/hodlinator/bitcoin/build_fuzz ../qa-assets/fuzz_corpora/ process_message`
Check if using libFuzzer ...
Che
...
(https://github.com/bitcoin/bitcoin/pull/32074#discussion_r2004389857)
nit:
Tried running without coverage support compiled in:
```
₿ cargo run --manifest-path ./contrib/devtools/deterministic-fuzz-coverage/Cargo.toml -- $PWD/build_fuzz ../qa-assets/fuzz_corpora/ process_message
Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.03s
Running `contrib/devtools/deterministic-fuzz-coverage/target/debug/deterministic-fuzz-coverage /home/hodlinator/bitcoin/build_fuzz ../qa-assets/fuzz_corpora/ process_message`
Check if using libFuzzer ...
Che
...
💬 hodlinator commented on pull request "contrib: Make deterministic-coverage error messages more readable":
(https://github.com/bitcoin/bitcoin/pull/32074#discussion_r2004339782)
Thread https://github.com/bitcoin/bitcoin/pull/32074#discussion_r2004194037:
I like shadowing for `if let Some(foo) = foo`, in other cases it ups the code complexity in my book.
But this is not an egregious case. Fine with leaving as-is unless others agree with me.
(https://github.com/bitcoin/bitcoin/pull/32074#discussion_r2004339782)
Thread https://github.com/bitcoin/bitcoin/pull/32074#discussion_r2004194037:
I like shadowing for `if let Some(foo) = foo`, in other cases it ups the code complexity in my book.
But this is not an egregious case. Fine with leaving as-is unless others agree with me.
💬 hodlinator commented on pull request "contrib: Make deterministic-coverage error messages more readable":
(https://github.com/bitcoin/bitcoin/pull/32074#discussion_r2004343979)
Thread https://github.com/bitcoin/bitcoin/pull/32074#discussion_r2004196829:
If you want to implement some version of it I'm happy to review, or we can defer it, not sure when I'll be able to get it into good shape.
(https://github.com/bitcoin/bitcoin/pull/32074#discussion_r2004343979)
Thread https://github.com/bitcoin/bitcoin/pull/32074#discussion_r2004196829:
If you want to implement some version of it I'm happy to review, or we can defer it, not sure when I'll be able to get it into good shape.
💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#issuecomment-2740492781)
Rebased after the merge of #31519.
(https://github.com/bitcoin/bitcoin/pull/31363#issuecomment-2740492781)
Rebased after the merge of #31519.
💬 instagibbs commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2005673107)
> so the only reasonable strategy I can see is to do:
The other reason to not follow this strategy, aside from reorgs, is "kindred eviction", where the caller is in charge of gathering sufficient information try to get back to an un-oversized state in the staging area, using non-oversized main stage information as a guiding tool.
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2005673107)
> so the only reasonable strategy I can see is to do:
The other reason to not follow this strategy, aside from reorgs, is "kindred eviction", where the caller is in charge of gathering sufficient information try to get back to an un-oversized state in the staging area, using non-oversized main stage information as a guiding tool.