💬 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
💬 Sjors commented on pull request "wallet, rpc: Return normalized descriptor in parent_descs":
(https://github.com/bitcoin/bitcoin/pull/32594#issuecomment-2903999864)
Concept ACK
In general normalized descriptors are more useful, because you can't derive addresses from e.g. `xpub/1h/*`. So it makes sense to use them for the parent.
ACK 107517ea7d29d959e7a0b94d8217dcb894680547
I checked that the tests fail if I revert the first commit.
nit: "prividing" -> "providing"
(https://github.com/bitcoin/bitcoin/pull/32594#issuecomment-2903999864)
Concept ACK
In general normalized descriptors are more useful, because you can't derive addresses from e.g. `xpub/1h/*`. So it makes sense to use them for the parent.
ACK 107517ea7d29d959e7a0b94d8217dcb894680547
I checked that the tests fail if I revert the first commit.
nit: "prividing" -> "providing"
🤔 Sjors reviewed a pull request: "wallet, rpc: Move (Un)LockCoin WalletBatch creation out of RPC"
(https://github.com/bitcoin/bitcoin/pull/32593#pullrequestreview-2863994337)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/32593#pullrequestreview-2863994337)
Concept ACK
💬 Sjors commented on pull request "wallet, rpc: Move (Un)LockCoin WalletBatch creation out of RPC":
(https://github.com/bitcoin/bitcoin/pull/32593#discussion_r2104321500)
`/*peristed=*/bool` ?
(https://github.com/bitcoin/bitcoin/pull/32593#discussion_r2104321500)
`/*peristed=*/bool` ?
💬 Sjors commented on pull request "wallet, rpc: Move (Un)LockCoin WalletBatch creation out of RPC":
(https://github.com/bitcoin/bitcoin/pull/32593#discussion_r2104331800)
This may be a good time to document this function. In particular why it unlocks coins. I'm guessing because we don't want to pollute the wallet db with locks for things that can't be spend anyway? And if there's a reorg we want them to be available.
(https://github.com/bitcoin/bitcoin/pull/32593#discussion_r2104331800)
This may be a good time to document this function. In particular why it unlocks coins. I'm guessing because we don't want to pollute the wallet db with locks for things that can't be spend anyway? And if there's a reorg we want them to be available.