💬 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
...
💬 maflcko commented on pull request "tests: cover abortrescan() in-flight True path with dynamic-tail retry":
(https://github.com/bitcoin/bitcoin/pull/33131#discussion_r2382182289)
are you still working on this?
(https://github.com/bitcoin/bitcoin/pull/33131#discussion_r2382182289)
are you still working on this?
✅ maflcko closed a pull request: "Prevent file descriptor exhaustion from too many RPC calls"
(https://github.com/bitcoin/bitcoin/pull/27731)
(https://github.com/bitcoin/bitcoin/pull/27731)
💬 maflcko commented on pull request "Prevent file descriptor exhaustion from too many RPC calls":
(https://github.com/bitcoin/bitcoin/pull/27731#issuecomment-3338327596)
+gha CI
(https://github.com/bitcoin/bitcoin/pull/27731#issuecomment-3338327596)
+gha CI
📝 maflcko reopened a pull request: "Prevent file descriptor exhaustion from too many RPC calls"
(https://github.com/bitcoin/bitcoin/pull/27731)
Fixes #11368
This requires libevent 2.2.1 which so far was only [released in alpha](https://github.com/libevent/libevent/releases/tag/release-2.2.1-alpha).
For more context see specifically https://github.com/bitcoin/bitcoin/issues/11368#issuecomment-632717903, this uses the feature mentioned there as libevent#592.
(https://github.com/bitcoin/bitcoin/pull/27731)
Fixes #11368
This requires libevent 2.2.1 which so far was only [released in alpha](https://github.com/libevent/libevent/releases/tag/release-2.2.1-alpha).
For more context see specifically https://github.com/bitcoin/bitcoin/issues/11368#issuecomment-632717903, this uses the feature mentioned there as libevent#592.
💬 janb84 commented on pull request "build: Move CMAKE_SKIP_INSTALL_RPATH from CMake to Guix script":
(https://github.com/bitcoin/bitcoin/pull/33470#discussion_r2382259990)
Now that you have changed this, please update the PR description to match the recent change:
"Add -DCMAKE_SKIP_INSTALL_RPATH=TRUE to Guix build script cmake configuration" =>
Add -DCMAKE_SKIP_RPATH=TRUE to Guix build script cmake configuration
(https://github.com/bitcoin/bitcoin/pull/33470#discussion_r2382259990)
Now that you have changed this, please update the PR description to match the recent change:
"Add -DCMAKE_SKIP_INSTALL_RPATH=TRUE to Guix build script cmake configuration" =>
Add -DCMAKE_SKIP_RPATH=TRUE to Guix build script cmake configuration
💬 theStack commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2382295705)
in commit fd52cd05d2bf11e059472e74fe6a771aa139b136: I noticed that the creation/extension of the `PSBTInput.m_musig2_{pubnonces,partial_sigs}` maps is slightly more complex here (involving a loop), in contrast to the counter-part in the other direction in `FillSignatureData`, where a single-line `.insert` is used. Is that intentional, or can they be unified in either way (i.e. also only use bare `.insert` here, or introduce loops in `FillSignatureData` as well if it's needed)?
Right now, `Fil
...
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2382295705)
in commit fd52cd05d2bf11e059472e74fe6a771aa139b136: I noticed that the creation/extension of the `PSBTInput.m_musig2_{pubnonces,partial_sigs}` maps is slightly more complex here (involving a loop), in contrast to the counter-part in the other direction in `FillSignatureData`, where a single-line `.insert` is used. Is that intentional, or can they be unified in either way (i.e. also only use bare `.insert` here, or introduce loops in `FillSignatureData` as well if it's needed)?
Right now, `Fil
...
❤1
💬 vasild commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-3338565128)
`151edfaf78...f4fdf81d3a`: rebase and [remove `INTERNET_TRAFFIC_EXPECTED`](https://github.com/bitcoin/bitcoin/pull/31349#discussion_r2333792206).
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-3338565128)
`151edfaf78...f4fdf81d3a`: rebase and [remove `INTERNET_TRAFFIC_EXPECTED`](https://github.com/bitcoin/bitcoin/pull/31349#discussion_r2333792206).
💬 vasild commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r2382314894)
Removed `INTERNET_TRAFFIC_EXPECTED`. Maybe it was an overkill in the first place.
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r2382314894)
Removed `INTERNET_TRAFFIC_EXPECTED`. Maybe it was an overkill in the first place.
💬 willcl-ark commented on pull request "test: Run bench sanity checks in parallel with functional tests":
(https://github.com/bitcoin/bitcoin/pull/33142#issuecomment-3338762732)
Ah interesting, that is quite similar in the end to my "workaround" (of building first, then a `generate_bench_tests` target).
I am Concept ACK using the python framework then, in the absence of any other benefits.
(https://github.com/bitcoin/bitcoin/pull/33142#issuecomment-3338762732)
Ah interesting, that is quite similar in the end to my "workaround" (of building first, then a `generate_bench_tests` target).
I am Concept ACK using the python framework then, in the absence of any other benefits.
💬 theStack commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2382545727)
in commits 0829833bf418d3fae35ceac57cae6137c1e9067d, e54d27d0f81c7a9c8991516f1ed06e86d52d6c79 and b6b66426125e46deb331927a5942e157578e712c: nit, in spirit of #33399: could use `secp256k1_context_static` where sufficient. If I didn't miss anything, I think the only two secp256k1 calls introduced in this PR that need the `secp256k1_context_sign` context are:
* `secp256k1_musig_nonce_gen` and
* `secp256k1_keypair_create`
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2382545727)
in commits 0829833bf418d3fae35ceac57cae6137c1e9067d, e54d27d0f81c7a9c8991516f1ed06e86d52d6c79 and b6b66426125e46deb331927a5942e157578e712c: nit, in spirit of #33399: could use `secp256k1_context_static` where sufficient. If I didn't miss anything, I think the only two secp256k1 calls introduced in this PR that need the `secp256k1_context_sign` context are:
* `secp256k1_musig_nonce_gen` and
* `secp256k1_keypair_create`