💬 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
...
💬 cdecker commented on pull request "chainparams: Re-add seed.bitcoinstats.com":
(https://github.com/bitcoin/bitcoin/pull/31086#issuecomment-2903836922)
Sure, I just don't want something that isn't progressing wasting too much time. I will eventually pick this up, stabilize and reopen, but for now my head is on other things, hence the retraction for now.
(https://github.com/bitcoin/bitcoin/pull/31086#issuecomment-2903836922)
Sure, I just don't want something that isn't progressing wasting too much time. I will eventually pick this up, stabilize and reopen, but for now my head is on other things, hence the retraction for now.
💬 willcl-ark commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2104201681)
A gentle reminder not to post AI-generated comments like this again here to avoid being banned.
> multiple OP_RETURN outputs in a single transaction, which goes against the standard behavior of the Bitcoin protocol.
There are no protocol rules defining how many OP_RETURN outputs can be included in a transaction.
> 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 no
...
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2104201681)
A gentle reminder not to post AI-generated comments like this again here to avoid being banned.
> multiple OP_RETURN outputs in a single transaction, which goes against the standard behavior of the Bitcoin protocol.
There are no protocol rules defining how many OP_RETURN outputs can be included in a transaction.
> 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 no
...
💬 willcl-ark commented on pull request "build: add a depends dependency provider":
(https://github.com/bitcoin/bitcoin/pull/32595#issuecomment-2903870430)
The windows job fails as currently we activate the dependency provider automatically based on whether a(ny) toolchain is in use.
This works fine for depends, but doesn't make sense on windows where the vcpkg toolchain is used, without depends.
The idea for doing it this way was to try and avoid users having to pass _two_ flags in order to build from depends -- a toolchain and a dependency-provider -- but perhaps this is necessary after all...
I do recall reading somewhere in cmake docs
...
(https://github.com/bitcoin/bitcoin/pull/32595#issuecomment-2903870430)
The windows job fails as currently we activate the dependency provider automatically based on whether a(ny) toolchain is in use.
This works fine for depends, but doesn't make sense on windows where the vcpkg toolchain is used, without depends.
The idea for doing it this way was to try and avoid users having to pass _two_ flags in order to build from depends -- a toolchain and a dependency-provider -- but perhaps this is necessary after all...
I do recall reading somewhere in cmake docs
...
👍 hodlinator approved a pull request: "multiprocess: Add bitcoin wrapper executable"
(https://github.com/bitcoin/bitcoin/pull/31375#pullrequestreview-2863857334)
ACK a5ac43d98d1ad3ebed934f2c50208a85aae17e5e
Concept: https://github.com/bitcoin/bitcoin/pull/31375#pullrequestreview-2821205668
`git range-diff master 4e1aae1 a5ac43d` shows only minor fixups since previous review (https://github.com/bitcoin/bitcoin/pull/31375#pullrequestreview-2838013470).
(https://github.com/bitcoin/bitcoin/pull/31375#pullrequestreview-2863857334)
ACK a5ac43d98d1ad3ebed934f2c50208a85aae17e5e
Concept: https://github.com/bitcoin/bitcoin/pull/31375#pullrequestreview-2821205668
`git range-diff master 4e1aae1 a5ac43d` shows only minor fixups since previous review (https://github.com/bitcoin/bitcoin/pull/31375#pullrequestreview-2838013470).
💬 TheCharlatan commented on pull request "kernel: Separate UTXO set access from validation functions":
(https://github.com/bitcoin/bitcoin/pull/32317#issuecomment-2903947412)
Rebased e8ac6d29273c48328f1f77886c3c3f653e6cc58e -> e604a57dccac1c4e6802a3ecc7f4cb4c3229c8ed ([spendblock_2](https://github.com/TheCharlatan/bitcoin/tree/spendblock_2) -> [spendblock_3](https://github.com/TheCharlatan/bitcoin/tree/spendblock_3), [compare](https://github.com/TheCharlatan/bitcoin/compare/spendblock_2..spendblock_3))
* Fixed conflict with #32575
(https://github.com/bitcoin/bitcoin/pull/32317#issuecomment-2903947412)
Rebased e8ac6d29273c48328f1f77886c3c3f653e6cc58e -> e604a57dccac1c4e6802a3ecc7f4cb4c3229c8ed ([spendblock_2](https://github.com/TheCharlatan/bitcoin/tree/spendblock_2) -> [spendblock_3](https://github.com/TheCharlatan/bitcoin/tree/spendblock_3), [compare](https://github.com/TheCharlatan/bitcoin/compare/spendblock_2..spendblock_3))
* Fixed conflict with #32575