💬 davidgumberg commented on pull request "wallet: Fix logging of wallet version":
(https://github.com/bitcoin/bitcoin/pull/32553#issuecomment-2903003935)
post-merge ACK https://github.com/bitcoin/bitcoin/pull/32553/commits/4b2cd0b41ff4800c8801f2c44883eaec60a035fa
(https://github.com/bitcoin/bitcoin/pull/32553#issuecomment-2903003935)
post-merge ACK https://github.com/bitcoin/bitcoin/pull/32553/commits/4b2cd0b41ff4800c8801f2c44883eaec60a035fa
💬 hebasto commented on issue "`system_tests/run_command` test fails on illumos OS":
(https://github.com/bitcoin/bitcoin/issues/32574#issuecomment-2903353573)
The underlying issue is not specific to illumos.
With the following patch:
```diff
--- a/src/test/system_tests.cpp
+++ b/src/test/system_tests.cpp
@@ -67,7 +67,7 @@ BOOST_AUTO_TEST_CASE(run_command)
const std::string expected{"err"};
BOOST_CHECK_EXCEPTION(RunCommandParseJSON(command), std::runtime_error, [&](const std::runtime_error& e) {
const std::string what(e.what());
- BOOST_CHECK(what.find(strprintf("RunCommandParseJSON error: process(%s) returned
...
(https://github.com/bitcoin/bitcoin/issues/32574#issuecomment-2903353573)
The underlying issue is not specific to illumos.
With the following patch:
```diff
--- a/src/test/system_tests.cpp
+++ b/src/test/system_tests.cpp
@@ -67,7 +67,7 @@ BOOST_AUTO_TEST_CASE(run_command)
const std::string expected{"err"};
BOOST_CHECK_EXCEPTION(RunCommandParseJSON(command), std::runtime_error, [&](const std::runtime_error& e) {
const std::string what(e.what());
- BOOST_CHECK(what.find(strprintf("RunCommandParseJSON error: process(%s) returned
...
💬 maflcko commented on pull request "rev_32343_wip_nomerge_ci":
(https://github.com/bitcoin/bitcoin/pull/32535#issuecomment-2903371304)
The failure remains, when triggered, so it would be good to investigate this. Let's continue discussion in https://github.com/bitcoin/bitcoin/issues/32524
(https://github.com/bitcoin/bitcoin/pull/32535#issuecomment-2903371304)
The failure remains, when triggered, so it would be good to investigate this. Let's continue discussion in https://github.com/bitcoin/bitcoin/issues/32524
✅ maflcko closed a pull request: "rev_32343_wip_nomerge_ci"
(https://github.com/bitcoin/bitcoin/pull/32535)
(https://github.com/bitcoin/bitcoin/pull/32535)
💬 maflcko commented on issue "intermittent issue in rpc_signer.py (enumeratesigners timeout) under GLIBCXX debug mode":
(https://github.com/bitcoin/bitcoin/issues/32524#issuecomment-2903373050)
This is closed, but the failure remains, when triggered, so it would be good to investigate this long-term at some point. Discussion can continue here, or in a new issue.
(https://github.com/bitcoin/bitcoin/issues/32524#issuecomment-2903373050)
This is closed, but the failure remains, when triggered, so it would be good to investigate this long-term at some point. Discussion can continue here, or in a new issue.
💬 hebasto commented on pull request "subprocess: Let shell parse command on non-Windows systems":
(https://github.com/bitcoin/bitcoin/pull/32577#issuecomment-2903379032)
> * It's not clear to me what the problem in [#32574](https://github.com/bitcoin/bitcoin/issues/32574) on illumos or actually is or how this change fixes it.
I've updated https://github.com/bitcoin/bitcoin/issues/32574 with more details.
> On the c++11 issue, I think maybe just compiling the subprocess unit test in c++11 would be a good way to ensure compatibility. It seems cmake has a [CXX_STANDARD](https://cmake.org/cmake/help/latest/prop_tgt/CXX_STANDARD.html) per-target property. I
...
(https://github.com/bitcoin/bitcoin/pull/32577#issuecomment-2903379032)
> * It's not clear to me what the problem in [#32574](https://github.com/bitcoin/bitcoin/issues/32574) on illumos or actually is or how this change fixes it.
I've updated https://github.com/bitcoin/bitcoin/issues/32574 with more details.
> On the c++11 issue, I think maybe just compiling the subprocess unit test in c++11 would be a good way to ensure compatibility. It seems cmake has a [CXX_STANDARD](https://cmake.org/cmake/help/latest/prop_tgt/CXX_STANDARD.html) per-target property. I
...
💬 hebasto commented on pull request "Use subprocess library for notifications":
(https://github.com/bitcoin/bitcoin/pull/32566#discussion_r2103886263)
A related discussion takes place at https://github.com/bitcoin/bitcoin/pull/32577.
(https://github.com/bitcoin/bitcoin/pull/32566#discussion_r2103886263)
A related discussion takes place at https://github.com/bitcoin/bitcoin/pull/32577.
⚠️ maflcko opened an issue: "intermittent issue in feature_init.py (bitcoind should have exited with expected error LevelDB error: Corruption)"
(https://github.com/bitcoin/bitcoin/issues/32600)
https://cirrus-ci.com/task/5185890844147712?logs=ci#L3553:
```
267/267 - [1mfeature_init.py [0m failed, Duration: 2420 s
[17:16:50.450]
[17:16:50.450] [1mstdout:
[17:16:50.450] [0m2025-05-21T20:36:30.563000Z TestFramework (INFO): PRNG seed is: 630547506121562679
...
[17:16:50.450] 2025-05-21T20:36:49.282000Z TestFramework (INFO): Perturbing file to ensure failure /ci_container_base/ci/scratch/test_runner/test_runner_₿_🏃_20250521_201845/feature_init_53/node0/regtest/indexes/coinstats/db/0
...
(https://github.com/bitcoin/bitcoin/issues/32600)
https://cirrus-ci.com/task/5185890844147712?logs=ci#L3553:
```
267/267 - [1mfeature_init.py [0m failed, Duration: 2420 s
[17:16:50.450]
[17:16:50.450] [1mstdout:
[17:16:50.450] [0m2025-05-21T20:36:30.563000Z TestFramework (INFO): PRNG seed is: 630547506121562679
...
[17:16:50.450] 2025-05-21T20:36:49.282000Z TestFramework (INFO): Perturbing file to ensure failure /ci_container_base/ci/scratch/test_runner/test_runner_₿_🏃_20250521_201845/feature_init_53/node0/regtest/indexes/coinstats/db/0
...
💬 vasild commented on pull request "p2p: protect addnode peers during IBD":
(https://github.com/bitcoin/bitcoin/pull/32051#issuecomment-2903394731)
Concept ACK to improve this, but:
> what should happen in the case of a stalling situation where the addnode peer is actually slow and causing congestion? ... one addnode peer the power to let our entire IBD come to a halt completely
Would it be too difficult to request a given block from more than one peer concurrently? That is, if a block is requested from a peer and not received within some time, then request it from other(s) as well, but don't cancel the original request. Then we take
...
(https://github.com/bitcoin/bitcoin/pull/32051#issuecomment-2903394731)
Concept ACK to improve this, but:
> what should happen in the case of a stalling situation where the addnode peer is actually slow and causing congestion? ... one addnode peer the power to let our entire IBD come to a halt completely
Would it be too difficult to request a given block from more than one peer concurrently? That is, if a block is requested from a peer and not received within some time, then request it from other(s) as well, but don't cancel the original request. Then we take
...
💬 hebasto commented on pull request "Use subprocess library for notifications":
(https://github.com/bitcoin/bitcoin/pull/32566#discussion_r2103895670)
This is a first use of the `Popen(std::initializer_list<const char*> cmd_args, Args&& ...args)` ctor. Before this, it was a candidate for cleanup.
(https://github.com/bitcoin/bitcoin/pull/32566#discussion_r2103895670)
This is a first use of the `Popen(std::initializer_list<const char*> cmd_args, Args&& ...args)` ctor. Before this, it was a candidate for cleanup.
💬 maflcko commented on pull request "util: Abort on failing CHECK_NONFATAL in debug builds":
(https://github.com/bitcoin/bitcoin/pull/32588#issuecomment-2903414680)
> `CHECK_THROW`
I don't think this solves issue (1). Instead of NONFATAL being inaccurately named in debug builds, it will now be THROW, because the check is neither nonfatal nor throwing in debug builds.
> Then, current `assert` uses could become `CHECK`, current `Assume` uses in hotspots could become `DCHECK`, majority of other `Assume` uses could become `CHECK_LOG`, and current `CHECK_NONFATAL` uses could become `CHECK_THROW`.
No objection, just mentioning that this would be a larger
...
(https://github.com/bitcoin/bitcoin/pull/32588#issuecomment-2903414680)
> `CHECK_THROW`
I don't think this solves issue (1). Instead of NONFATAL being inaccurately named in debug builds, it will now be THROW, because the check is neither nonfatal nor throwing in debug builds.
> Then, current `assert` uses could become `CHECK`, current `Assume` uses in hotspots could become `DCHECK`, majority of other `Assume` uses could become `CHECK_LOG`, and current `CHECK_NONFATAL` uses could become `CHECK_THROW`.
No objection, just mentioning that this would be a larger
...
💬 hebasto commented on pull request "Use subprocess library for notifications":
(https://github.com/bitcoin/bitcoin/pull/32566#issuecomment-2903499043)
Tested 82aeba60c2b3a166302e10fbf6e3eaf0593fab4d.
The `-startupnotify="echo {\"success\": true}"` option prints `{success: true}`, which is the same as on the master branch.
But would it be appropriate to expect `{"success": true}` instead? If not, what is the correct way to output a quoted string, which may be required to represent a valid JSON object?
(https://github.com/bitcoin/bitcoin/pull/32566#issuecomment-2903499043)
Tested 82aeba60c2b3a166302e10fbf6e3eaf0593fab4d.
The `-startupnotify="echo {\"success\": true}"` option prints `{success: true}`, which is the same as on the master branch.
But would it be appropriate to expect `{"success": true}` instead? If not, what is the correct way to output a quoted string, which may be required to represent a valid JSON object?
💬 Sjors commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#issuecomment-2903559188)
utACK a5ac43d98d1ad3ebed934f2c50208a85aae17e5e
Since my last review this just adds `throw std::runtime_error("execvp returned unexpectedly")` and the copyright change.
(https://github.com/bitcoin/bitcoin/pull/31375#issuecomment-2903559188)
utACK a5ac43d98d1ad3ebed934f2c50208a85aae17e5e
Since my last review this just adds `throw std::runtime_error("execvp returned unexpectedly")` and the copyright change.
💬 Sjors commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2104017923)
Just to clarify my initial comment: I can remove this loop _in the final version of this PR_ without breaking any test.
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2104017923)
Just to clarify my initial comment: I can remove this loop _in the final version of this PR_ without breaking any test.
💬 Sjors commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2104022638)
Being able to generate a (derived) descriptor without parsing is still on my wish list: https://github.com/bitcoin/bitcoin/issues/24003
But even in that case it could generate, convert to string and then re-parse as a sanity check.
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2104022638)
Being able to generate a (derived) descriptor without parsing is still on my wish list: https://github.com/bitcoin/bitcoin/issues/24003
But even in that case it could generate, convert to string and then re-parse as a sanity check.
💬 Sjors commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#issuecomment-2903590736)
> Split the LockCoin commit into https://github.com/bitcoin/bitcoin/pull/3259,
#32593
(https://github.com/bitcoin/bitcoin/pull/32489#issuecomment-2903590736)
> Split the LockCoin commit into https://github.com/bitcoin/bitcoin/pull/3259,
#32593
⚠️ fanquake reopened an issue: "intermittent issue in rpc_signer.py (enumeratesigners timeout) under GLIBCXX debug mode"
(https://github.com/bitcoin/bitcoin/issues/32524)
https://cirrus-ci.com/task/5094568045051904?logs=ci#L4419
```
[18:26:32.573] test_framework.authproxy.JSONRPCException: 'enumeratesigners' RPC took longer than 1200.000000 seconds. Consider using larger timeout for calls that take longer to return. (-344)
```
Looking at the full log, the test is just idle and then times out. However, there is no indication why the RPC does not proceed.
(https://github.com/bitcoin/bitcoin/issues/32524)
https://cirrus-ci.com/task/5094568045051904?logs=ci#L4419
```
[18:26:32.573] test_framework.authproxy.JSONRPCException: 'enumeratesigners' RPC took longer than 1200.000000 seconds. Consider using larger timeout for calls that take longer to return. (-344)
```
Looking at the full log, the test is just idle and then times out. However, there is no indication why the RPC does not proceed.
💬 vasild commented on pull request "tests: Expand HTTP coverage to assert libevent behavior":
(https://github.com/bitcoin/bitcoin/pull/32408#discussion_r2104068784)
Ok, it could be that libevent behaves in an unexpected way. In https://github.com/bitcoin/bitcoin/pull/32061 we could tighten up the test to:
* connect
* start timer in the test (A)
* send, server timer starts after last packet received from the client (B)
* try to recv, connection will be closed by the server due to server timeout (C)
* stop the timer in the test (D)
(https://github.com/bitcoin/bitcoin/pull/32408#discussion_r2104068784)
Ok, it could be that libevent behaves in an unexpected way. In https://github.com/bitcoin/bitcoin/pull/32061 we could tighten up the test to:
* connect
* start timer in the test (A)
* send, server timer starts after last packet received from the client (B)
* try to recv, connection will be closed by the server due to server timeout (C)
* stop the timer in the test (D)
💬 maflcko commented on issue "intermittent issue in rpc_signer.py (enumeratesigners timeout) under GLIBCXX debug mode":
(https://github.com/bitcoin/bitcoin/issues/32524#issuecomment-2903656040)
Another data point: I haven't see a failure so far in my G++ snapshot DEBUG=1 nightly run: https://github.com/maflcko/b-c-nightly/actions/runs/15153825944/job/42604725571
(https://github.com/bitcoin/bitcoin/issues/32524#issuecomment-2903656040)
Another data point: I haven't see a failure so far in my G++ snapshot DEBUG=1 nightly run: https://github.com/maflcko/b-c-nightly/actions/runs/15153825944/job/42604725571
💬 mrberlinorg commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2104140401)
Removing the check for nDataOut > 1 would allow multiple OP_RETURN outputs in a single transaction, which goes against the standard behavior of the Bitcoin protocol. This could introduce several issues:
Non-standard transactions: Multiple OP_RETURN outputs in a transaction are considered non-standard. Allowing them could lead to network inconsistencies, as some nodes might not be able to properly process or relay these transactions.
Network congestion: More OP_RETURN outputs would increase
...
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2104140401)
Removing the check for nDataOut > 1 would allow multiple OP_RETURN outputs in a single transaction, which goes against the standard behavior of the Bitcoin protocol. This could introduce several issues:
Non-standard transactions: Multiple OP_RETURN outputs in a transaction are considered non-standard. Allowing them could lead to network inconsistencies, as some nodes might not be able to properly process or relay these transactions.
Network congestion: More OP_RETURN outputs would increase
...