👍 theStack approved a pull request: "policy: uncap datacarrier by default"
(https://github.com/bitcoin/bitcoin/pull/32406#pullrequestreview-2837512117)
re-ACK 35bcd8eed38130445aef5ebe217ab42248fa6f18
(https://github.com/bitcoin/bitcoin/pull/32406#pullrequestreview-2837512117)
re-ACK 35bcd8eed38130445aef5ebe217ab42248fa6f18
💬 mzumsande commented on issue "qa: Failure in wallet_basic.py spendzeroconfchange test":
(https://github.com/bitcoin/bitcoin/issues/32456#issuecomment-2877311830)
See [#32483](https://github.com/bitcoin/bitcoin/pull/32483) for a fix / way to reproduce.
(https://github.com/bitcoin/bitcoin/issues/32456#issuecomment-2877311830)
See [#32483](https://github.com/bitcoin/bitcoin/pull/32483) for a fix / way to reproduce.
💬 yancyribbens commented on pull request "contrib: add xor-blocks tool to obfuscate blocks directory":
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2087278600)
This should also return a non-zero exit code and error.
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2087278600)
This should also return a non-zero exit code and error.
💬 brunoerg commented on pull request "test: added fuzz coverage for consensus/merkle.cpp":
(https://github.com/bitcoin/bitcoin/pull/32243#discussion_r2087281999)
f99fd0b87dcfaf84784ce423f78a950ad377b36b: Now that you're using `ConsumeDeserializable` to create a CBlock, maybe you don't to create the transactions using `ConsumeTransaction` anymore?
(https://github.com/bitcoin/bitcoin/pull/32243#discussion_r2087281999)
f99fd0b87dcfaf84784ce423f78a950ad377b36b: Now that you're using `ConsumeDeserializable` to create a CBlock, maybe you don't to create the transactions using `ConsumeTransaction` anymore?
💬 ismaelsadeeq commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2087286446)
> It seems like it could be more confusing to not mention one of the alternatives than just describe both.
You are right I think it's okay to include both alternative comment in the `doc/README.md`.
However In my opinion, that's sufficient, and we don't need to repeat in every instance that, for example, `bitcoin node` or `bitcoin GUI` can be substituted with `bitcoind`, etc.
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2087286446)
> It seems like it could be more confusing to not mention one of the alternatives than just describe both.
You are right I think it's okay to include both alternative comment in the `doc/README.md`.
However In my opinion, that's sufficient, and we don't need to repeat in every instance that, for example, `bitcoin node` or `bitcoin GUI` can be substituted with `bitcoind`, etc.
📝 fanquake opened a pull request: "Update minisketch subtree"
(https://github.com/bitcoin/bitcoin/pull/32485)
Includes:
* https://github.com/bitcoin-core/minisketch/pull/89
* https://github.com/bitcoin-core/minisketch/pull/94 (in #32482)
(https://github.com/bitcoin/bitcoin/pull/32485)
Includes:
* https://github.com/bitcoin-core/minisketch/pull/89
* https://github.com/bitcoin-core/minisketch/pull/94 (in #32482)
⚠️ fanquake opened an issue: "build: deprecated arg usage in macOS deploy script"
(https://github.com/bitcoin/bitcoin/issues/32486)
`--deep` has been deprecated for at least the last 2 major versions of macos:
> --deep (DEPRECATED for signing as of macOS 13.0) When signing a bundle. ...
Given it's deprecated, I'd think that means we shouldn't actually need to use it. Assuming this also means it could be removed at some point, we should probably stop using it regardless. See our usage here:
https://github.com/bitcoin/bitcoin/blob/8309a9747a8df96517970841b3648937d05939a3/contrib/macdeploy/macdeployqtplus#L497
(https://github.com/bitcoin/bitcoin/issues/32486)
`--deep` has been deprecated for at least the last 2 major versions of macos:
> --deep (DEPRECATED for signing as of macOS 13.0) When signing a bundle. ...
Given it's deprecated, I'd think that means we shouldn't actually need to use it. Assuming this also means it could be removed at some point, we should probably stop using it regardless. See our usage here:
https://github.com/bitcoin/bitcoin/blob/8309a9747a8df96517970841b3648937d05939a3/contrib/macdeploy/macdeployqtplus#L497
✅ ismaelsadeeq closed an issue: "RFC: Should node Wallet Startup Options Apply to Individual Wallets?"
(https://github.com/bitcoin/bitcoin/issues/32462)
(https://github.com/bitcoin/bitcoin/issues/32462)
💬 ismaelsadeeq commented on issue "RFC: Should node Wallet Startup Options Apply to Individual Wallets?":
(https://github.com/bitcoin/bitcoin/issues/32462#issuecomment-2877356452)
I share the same opinion. I also noticed @murchandamus gave a thumbs up, which I interpret as agreement. Therefore, I’ll resolve the comment in https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1468533274 and close this issue.
As mentioned, if there’s a use case for wanting different wallets to have different values, support for that can be added in another PR, and the rationale can be documented in this issue for better tracking.
(https://github.com/bitcoin/bitcoin/issues/32462#issuecomment-2877356452)
I share the same opinion. I also noticed @murchandamus gave a thumbs up, which I interpret as agreement. Therefore, I’ll resolve the comment in https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1468533274 and close this issue.
As mentioned, if there’s a use case for wanting different wallets to have different values, support for that can be added in another PR, and the rationale can be documented in this issue for better tracking.
💬 andrewtoth commented on pull request "rpc: generatetomany":
(https://github.com/bitcoin/bitcoin/pull/32468#issuecomment-2877371202)
> `generateblock` is thought to be used always with a given set of txs and that may not always be the use case. Maybe would be worth to implement it on both RPC?
> you may want to mine txs from the mempool and not care about the exact amount of sats to distribute. Maybe implement this on both RPC is the solution to it.
> What do you think?
I think you are making the case to add a feature to `generateblock` to mine the mempool and collect rewards the same as `generateto` commands. Perhap
...
(https://github.com/bitcoin/bitcoin/pull/32468#issuecomment-2877371202)
> `generateblock` is thought to be used always with a given set of txs and that may not always be the use case. Maybe would be worth to implement it on both RPC?
> you may want to mine txs from the mempool and not care about the exact amount of sats to distribute. Maybe implement this on both RPC is the solution to it.
> What do you think?
I think you are making the case to add a feature to `generateblock` to mine the mempool and collect rewards the same as `generateto` commands. Perhap
...
💬 vicjuma commented on pull request "Fix listdescriptors true fails with 'Can't get descriptor string' in non-watch-only descriptor wallet":
(https://github.com/bitcoin/bitcoin/pull/32471#issuecomment-2877401817)
This PR improves the usability of the `listdescriptors true` RPC, especially for descriptor-based (modern) wallets where some private keys may be missing.
However, it's unclear why someone would create a watch-only wallet and still choose to set `private=true`. Why not just use the default command, `bitcoin-cli listdescriptors` instead of `bitcoin-cli -named listdescriptors private=true`. The former already shows public keys? Unless the user is not certain about the nature of the descriptors
...
(https://github.com/bitcoin/bitcoin/pull/32471#issuecomment-2877401817)
This PR improves the usability of the `listdescriptors true` RPC, especially for descriptor-based (modern) wallets where some private keys may be missing.
However, it's unclear why someone would create a watch-only wallet and still choose to set `private=true`. Why not just use the default command, `bitcoin-cli listdescriptors` instead of `bitcoin-cli -named listdescriptors private=true`. The former already shows public keys? Unless the user is not certain about the nature of the descriptors
...
💬 achow101 commented on pull request "Fix listdescriptors true fails with 'Can't get descriptor string' in non-watch-only descriptor wallet":
(https://github.com/bitcoin/bitcoin/pull/32471#issuecomment-2877413827)
Concept NACK
The purpose of the `private` argument is to show the private keys. If there are no private keys to show, I think it is a reasonable error to say that, rather than just showing the public descriptors. I think changing the behavior could result in a footgun where people use it thinking that they are being shown private keys when they actually are not.
(https://github.com/bitcoin/bitcoin/pull/32471#issuecomment-2877413827)
Concept NACK
The purpose of the `private` argument is to show the private keys. If there are no private keys to show, I think it is a reasonable error to say that, rather than just showing the public descriptors. I think changing the behavior could result in a footgun where people use it thinking that they are being shown private keys when they actually are not.
💬 achow101 commented on pull request "doc: update CWallet::SignTransaction doc to mention SIGHASH_DEFAULT":
(https://github.com/bitcoin/bitcoin/pull/32411#issuecomment-2877418091)
DEFAULT and ALL are effectively the same for Taproot inputs, and DEFAULT is an alias for ALL for non-taproot inputs. I don't think that this change is meaningfully useful as ALL and DEFAULT are essentially aliases for each other, and mean basically the same thing.
(https://github.com/bitcoin/bitcoin/pull/32411#issuecomment-2877418091)
DEFAULT and ALL are effectively the same for Taproot inputs, and DEFAULT is an alias for ALL for non-taproot inputs. I don't think that this change is meaningfully useful as ALL and DEFAULT are essentially aliases for each other, and mean basically the same thing.
💬 vicjuma commented on pull request "Fix listdescriptors true fails with 'Can't get descriptor string' in non-watch-only descriptor wallet":
(https://github.com/bitcoin/bitcoin/pull/32471#issuecomment-2877434573)
Concept NACK
It's unclear why someone should receive public keys with `private=true`. Why not just use the default command, `bitcoin-cli listdescriptors`, which already shows public keys? I assume the user is certain about the nature of the descriptors they have in their wallet. Maybe if I do not fully understand this PR.
(https://github.com/bitcoin/bitcoin/pull/32471#issuecomment-2877434573)
Concept NACK
It's unclear why someone should receive public keys with `private=true`. Why not just use the default command, `bitcoin-cli listdescriptors`, which already shows public keys? I assume the user is certain about the nature of the descriptors they have in their wallet. Maybe if I do not fully understand this PR.
👍 pinheadmz approved a pull request: "test: allow all functional tests to be run or skipped with --usecli"
(https://github.com/bitcoin/bitcoin/pull/32290#pullrequestreview-2837564813)
ACK 7c13240d7b9a03b77ad84460e80007063367e05a
Reviewed all commits and tested on macos/arm64. Compared `test_runner --usecli` on master to branch. Master failed 17 tests and skipped 58. This branch skipped 36 and failed none. Some of the skipped tests are due to my platform.
This PR touches test code only but also by adding CI coverage requires all future changes to the API to be bitcoin-cli compatible (or otherwise pass new tests with cli)
<details><summary>Show Signature</summary>
...
(https://github.com/bitcoin/bitcoin/pull/32290#pullrequestreview-2837564813)
ACK 7c13240d7b9a03b77ad84460e80007063367e05a
Reviewed all commits and tested on macos/arm64. Compared `test_runner --usecli` on master to branch. Master failed 17 tests and skipped 58. This branch skipped 36 and failed none. Some of the skipped tests are due to my platform.
This PR touches test code only but also by adding CI coverage requires all future changes to the API to be bitcoin-cli compatible (or otherwise pass new tests with cli)
<details><summary>Show Signature</summary>
...
💬 pinheadmz commented on pull request "test: allow all functional tests to be run or skipped with --usecli":
(https://github.com/bitcoin/bitcoin/pull/32290#discussion_r2087318320)
Is this only ever needed for passing a hash as a hash-or-height argument?
(https://github.com/bitcoin/bitcoin/pull/32290#discussion_r2087318320)
Is this only ever needed for passing a hash as a hash-or-height argument?
💬 pinheadmz commented on pull request "test: allow all functional tests to be run or skipped with --usecli":
(https://github.com/bitcoin/bitcoin/pull/32290#discussion_r2087299133)
959dc33283be3c465476f604263f7a2de4007da0
Why not add this method to `BitcoinTestFramework` then you won't need to pass `self.options.usecli` every call?
(https://github.com/bitcoin/bitcoin/pull/32290#discussion_r2087299133)
959dc33283be3c465476f604263f7a2de4007da0
Why not add this method to `BitcoinTestFramework` then you won't need to pass `self.options.usecli` every call?
💬 rkrux commented on pull request "doc: update CWallet::SignTransaction doc to mention SIGHASH_DEFAULT":
(https://github.com/bitcoin/bitcoin/pull/32411#issuecomment-2877457566)
True. The main reason for this change was that I didn't see any benefit in leaving ALL around in the function documentation when the implementation uses DEFAULT.
I am open to closing this PR later if this doesn't seem like a strong enough reason.
(https://github.com/bitcoin/bitcoin/pull/32411#issuecomment-2877457566)
True. The main reason for this change was that I didn't see any benefit in leaving ALL around in the function documentation when the implementation uses DEFAULT.
I am open to closing this PR later if this doesn't seem like a strong enough reason.
💬 achow101 commented on pull request "test: fix two intermittent failures in wallet_basic.py":
(https://github.com/bitcoin/bitcoin/pull/32483#issuecomment-2877470966)
ACK 4290a86c6022c8f63610b9c1b185ac115cefbd2c
(https://github.com/bitcoin/bitcoin/pull/32483#issuecomment-2877470966)
ACK 4290a86c6022c8f63610b9c1b185ac115cefbd2c
💬 theuni commented on pull request "Introduce per-txin sighash midstate cache for legacy/p2sh/segwitv0 scripts":
(https://github.com/bitcoin/bitcoin/pull/32473#discussion_r2087368590)
`HashWriter ss{}` in the if() branch below now shadows this one. Rename to avoid confusion (and warnings) ?
(https://github.com/bitcoin/bitcoin/pull/32473#discussion_r2087368590)
`HashWriter ss{}` in the if() branch below now shadows this one. Rename to avoid confusion (and warnings) ?