💬 maflcko commented on pull request "ci: use Mold linker for asan-lsan-ubsan-integer-no-depends-usdt workflow":
(https://github.com/bitcoin/bitcoin/pull/33370#issuecomment-3337423480)
lgtm ACK f031536f2d267655a0fb40ab84d03e7ffa903d4c
Makes the config a bit more verbose, but otherwise I don't see any harm, so should be fine.
(https://github.com/bitcoin/bitcoin/pull/33370#issuecomment-3337423480)
lgtm ACK f031536f2d267655a0fb40ab84d03e7ffa903d4c
Makes the config a bit more verbose, but otherwise I don't see any harm, so should be fine.
💬 willcl-ark commented on pull request "test: Run bench sanity checks in parallel with functional tests":
(https://github.com/bitcoin/bitcoin/pull/33142#issuecomment-3337628247)
I have had a play with porting this to cmake-native (ctest) code, and IMO the main difference is that (in my implementation) it just that it feels more obscure to collect the benchmarks, cost them (not implemented in this PR), and run them in parallel using cmake files vs a few lines of python.
Perhaps I'm doing it wrong, but I found I needed a whole new target `generate_bench_tests` to dynamically parse benchmark names _after_ bitcoin_bench was built, which can then create the necessary `cte
...
(https://github.com/bitcoin/bitcoin/pull/33142#issuecomment-3337628247)
I have had a play with porting this to cmake-native (ctest) code, and IMO the main difference is that (in my implementation) it just that it feels more obscure to collect the benchmarks, cost them (not implemented in this PR), and run them in parallel using cmake files vs a few lines of python.
Perhaps I'm doing it wrong, but I found I needed a whole new target `generate_bench_tests` to dynamically parse benchmark names _after_ bitcoin_bench was built, which can then create the necessary `cte
...
💬 maflcko commented on pull request "test: Run bench sanity checks in parallel with functional tests":
(https://github.com/bitcoin/bitcoin/pull/33142#issuecomment-3337747034)
> Perhaps I'm doing it wrong, but I found I needed a whole new target `generate_bench_tests` to dynamically parse benchmark names _after_ bitcoin_bench was built, which can then create the necessary `ctest` file, which can then be run in "true parallel" by `ctest`.
Yes, it is possible (and thanks for trying), see also https://habla.news/u/purplekarrot.net/cmake-and-test-suites. However,
* it requires more code, and the review for it.
* the win-cross issue is pre-existing and even if the c
...
(https://github.com/bitcoin/bitcoin/pull/33142#issuecomment-3337747034)
> Perhaps I'm doing it wrong, but I found I needed a whole new target `generate_bench_tests` to dynamically parse benchmark names _after_ bitcoin_bench was built, which can then create the necessary `ctest` file, which can then be run in "true parallel" by `ctest`.
Yes, it is possible (and thanks for trying), see also https://habla.news/u/purplekarrot.net/cmake-and-test-suites. However,
* it requires more code, and the review for it.
* the win-cross issue is pre-existing and even if the c
...
💬 hebasto commented on pull request "ci: Remove bash -c from cmake invocation using eval":
(https://github.com/bitcoin/bitcoin/pull/33401#discussion_r2381735638)
```suggestion
eval "CMAKE_ARGS=($BITCOIN_CONFIG_ALL $BITCOIN_CONFIG)"
```
(https://github.com/bitcoin/bitcoin/pull/33401#discussion_r2381735638)
```suggestion
eval "CMAKE_ARGS=($BITCOIN_CONFIG_ALL $BITCOIN_CONFIG)"
```
👍 hebasto approved a pull request: "ci: Remove bash -c from cmake invocation using eval"
(https://github.com/bitcoin/bitcoin/pull/33401#pullrequestreview-3271334044)
ACK 8406c1e6321fbc3ce739aa7ccb18c67fb706a4b9.
(https://github.com/bitcoin/bitcoin/pull/33401#pullrequestreview-3271334044)
ACK 8406c1e6321fbc3ce739aa7ccb18c67fb706a4b9.
💬 Sjors commented on pull request "descriptor: fix comments in descriptor.cpp::DescriptorImpl":
(https://github.com/bitcoin/bitcoin/pull/33384#issuecomment-3337806987)
cc @achow101
(https://github.com/bitcoin/bitcoin/pull/33384#issuecomment-3337806987)
cc @achow101
💬 hebasto commented on pull request "ci: Enable more shellcheck":
(https://github.com/bitcoin/bitcoin/pull/32970#discussion_r2381744613)
> (can be done in a follow-up)
See: https://github.com/bitcoin/bitcoin/pull/33401.
(https://github.com/bitcoin/bitcoin/pull/32970#discussion_r2381744613)
> (can be done in a follow-up)
See: https://github.com/bitcoin/bitcoin/pull/33401.
💬 maflcko commented on pull request "rpc: refactor: use string_view in Arg/MaybeArg":
(https://github.com/bitcoin/bitcoin/pull/32983#discussion_r2381751418)
I actually like the `value_or` version better, because it has less branches (can be read left-to-right without jumps). Also, it is shorter. So my recommendation would be to merge this as-is.
(https://github.com/bitcoin/bitcoin/pull/32983#discussion_r2381751418)
I actually like the `value_or` version better, because it has less branches (can be read left-to-right without jumps). Also, it is shorter. So my recommendation would be to merge this as-is.
💬 hebasto commented on pull request "Modernize use of UTF-8 in Windows code":
(https://github.com/bitcoin/bitcoin/pull/32380#issuecomment-3337865481)
Rebased to resolve a conflict with the merged https://github.com/bitcoin/bitcoin/pull/33229.
(https://github.com/bitcoin/bitcoin/pull/32380#issuecomment-3337865481)
Rebased to resolve a conflict with the merged https://github.com/bitcoin/bitcoin/pull/33229.
💬 Sjors commented on pull request "wallet: derivehdkey RPC to get xpub at arbitrary path":
(https://github.com/bitcoin/bitcoin/pull/32784#issuecomment-3337924467)
(will rebase this later)
(https://github.com/bitcoin/bitcoin/pull/32784#issuecomment-3337924467)
(will rebase this later)
💬 vasild commented on pull request "test: fix p2p_leak_tx.py":
(https://github.com/bitcoin/bitcoin/pull/33121#discussion_r2381936863)
Here is a change against `master` that could replace the first commit in this PR. It solves the same problem - when `NextInvToInbounds()` decides to wait more than the test timeout. In addition, by using mocktime, the diff below eliminates the waits in a few places of the test due to the `NextInvToInbounds()` delays. As a result the test passes in about 1 second instead of the current random few seconds to few tens of seconds.
```diff
--- i/test/functional/p2p_leak_tx.py
+++ w/test/function
...
(https://github.com/bitcoin/bitcoin/pull/33121#discussion_r2381936863)
Here is a change against `master` that could replace the first commit in this PR. It solves the same problem - when `NextInvToInbounds()` decides to wait more than the test timeout. In addition, by using mocktime, the diff below eliminates the waits in a few places of the test due to the `NextInvToInbounds()` delays. As a result the test passes in about 1 second instead of the current random few seconds to few tens of seconds.
```diff
--- i/test/functional/p2p_leak_tx.py
+++ w/test/function
...
👍 vasild approved a pull request: "test: fix p2p_leak_tx.py"
(https://github.com/bitcoin/bitcoin/pull/33121#pullrequestreview-3271613419)
ACK a52c148ece2296f22e2fe1b7e9584d49e23f03f2
The changes are an improvement. It seems some of that maybe can be done better, see https://github.com/bitcoin/bitcoin/pull/33121#discussion_r2381936863. I do not see that as a blocker.
(https://github.com/bitcoin/bitcoin/pull/33121#pullrequestreview-3271613419)
ACK a52c148ece2296f22e2fe1b7e9584d49e23f03f2
The changes are an improvement. It seems some of that maybe can be done better, see https://github.com/bitcoin/bitcoin/pull/33121#discussion_r2381936863. I do not see that as a blocker.
✅ maflcko closed an issue: "Corecheck isn't run for latest PRs"
(https://github.com/bitcoin/bitcoin/issues/33359)
(https://github.com/bitcoin/bitcoin/issues/33359)
💬 maflcko commented on issue "Corecheck isn't run for latest PRs":
(https://github.com/bitcoin/bitcoin/issues/33359#issuecomment-3338152690)
thx for the fix
(https://github.com/bitcoin/bitcoin/issues/33359#issuecomment-3338152690)
thx for the fix
💬 maflcko commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r2382098129)
Due to a silent merge conflict, this won't actually work. To pass the setting into the container, the hidden setting has to be "documented" now. For context, see https://github.com/bitcoin/bitcoin/blob/65e909dfdd934f033727e5404b5616a29dc18209/ci/test/02_run_container.py#L29-L34
However, the silent conflict should have also fixed the bug that made this setting necessary in the first place.
It was added for https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-2586911979. However, I exp
...
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r2382098129)
Due to a silent merge conflict, this won't actually work. To pass the setting into the container, the hidden setting has to be "documented" now. For context, see https://github.com/bitcoin/bitcoin/blob/65e909dfdd934f033727e5404b5616a29dc18209/ci/test/02_run_container.py#L29-L34
However, the silent conflict should have also fixed the bug that made this setting necessary in the first place.
It was added for https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-2586911979. However, I exp
...
💬 waketraindev commented on pull request "Add reset button and console commands for clearing output/history":
(https://github.com/bitcoin-core/gui/pull/882#issuecomment-3338256434)
Thanks a lot for the review!
The shortcut is already documented as `"Use %9 to clear both the screen and the command history.\n", but I can update it to match the exact wording you suggested.`
About splitting the clear functionality: since the output history already includes the input commands, I’m not sure there’s much benefit in clearing just the commands while keeping the output. But if you think it’s more useful that way, I’m happy to adjust and split them.
On the QML side, I don’t see a
...
(https://github.com/bitcoin-core/gui/pull/882#issuecomment-3338256434)
Thanks a lot for the review!
The shortcut is already documented as `"Use %9 to clear both the screen and the command history.\n", but I can update it to match the exact wording you suggested.`
About splitting the clear functionality: since the output history already includes the input commands, I’m not sure there’s much benefit in clearing just the commands while keeping the output. But if you think it’s more useful that way, I’m happy to adjust and split them.
On the QML side, I don’t see a
...
💬 maflcko commented on pull request "Fix compatibility with `-debuglogfile` command-line option":
(https://github.com/bitcoin/bitcoin/pull/33215#discussion_r2382175112)
Yeah, this is the RPC doc, not the RPC result. No opinion about the result, but it must not be in the doc. Otherwise, the doc is confusing, brittle and highly mutable.
(https://github.com/bitcoin/bitcoin/pull/33215#discussion_r2382175112)
Yeah, this is the RPC doc, not the RPC result. No opinion about the result, but it must not be in the doc. Otherwise, the doc is confusing, brittle and highly mutable.
✅ maflcko closed a pull request: "Release `LockData::dd_mutex` before calling `*_detected` functions"
(https://github.com/bitcoin/bitcoin/pull/26781)
(https://github.com/bitcoin/bitcoin/pull/26781)
💬 maflcko commented on pull request "Release `LockData::dd_mutex` before calling `*_detected` functions":
(https://github.com/bitcoin/bitcoin/pull/26781#issuecomment-3338319471)
+gha ci
(https://github.com/bitcoin/bitcoin/pull/26781#issuecomment-3338319471)
+gha ci
📝 maflcko reopened a pull request: "Release `LockData::dd_mutex` before calling `*_detected` functions"
(https://github.com/bitcoin/bitcoin/pull/26781)
Both `double_lock_detected()` and `potential_deadlock_detected()` functions call `LogPrintf()` which in turn implies locking of the `Logger::m_cs` mutex. To avoid a deadlock, the latter must not have the `Mutex` type (see https://github.com/bitcoin/bitcoin/pull/16112).
With this PR the mentioned restriction has been lifted, and it is possible now to use our regular `Mutex` type for the `Logger::m_cs` mutex instead of a dedicated `StdMutex` type (not introducing that change here, as its diff i
...
(https://github.com/bitcoin/bitcoin/pull/26781)
Both `double_lock_detected()` and `potential_deadlock_detected()` functions call `LogPrintf()` which in turn implies locking of the `Logger::m_cs` mutex. To avoid a deadlock, the latter must not have the `Mutex` type (see https://github.com/bitcoin/bitcoin/pull/16112).
With this PR the mentioned restriction has been lifted, and it is possible now to use our regular `Mutex` type for the `Logger::m_cs` mutex instead of a dedicated `StdMutex` type (not introducing that change here, as its diff i
...