💬 ryanofsky commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2087237457)
In commit "wallet: Write best block record on unload" (8ecc7653556a1977eae5a902d51a2ececde94913)
Just to be sure, current state is that if WriteBestBlock is called here, rather than being called in the destructor or being called in FlushAndDeleteWallet, the best block locator could be a little out of date and this is maybe not ideal but fine?
re: https://github.com/bitcoin/bitcoin/pull/30221#issuecomment-2864638615
> It can't be static because the `FUZZ_TARGET` below needs to access it.
...
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2087237457)
In commit "wallet: Write best block record on unload" (8ecc7653556a1977eae5a902d51a2ececde94913)
Just to be sure, current state is that if WriteBestBlock is called here, rather than being called in the destructor or being called in FlushAndDeleteWallet, the best block locator could be a little out of date and this is maybe not ideal but fine?
re: https://github.com/bitcoin/bitcoin/pull/30221#issuecomment-2864638615
> It can't be static because the `FUZZ_TARGET` below needs to access it.
...
💬 theuni commented on pull request "[28.x] build: suppress `-Wunterminated-string-initialization` in secp256k1":
(https://github.com/bitcoin/bitcoin/pull/32484#issuecomment-2877276631)
Hmm, my first thought was just to backport the "fix" instead, but I guess we've never really backported secp changes for the sake of keeping in sync.
Either way, in this case I agree it's simple/safe enough to just use the warning for 28.x.
(https://github.com/bitcoin/bitcoin/pull/32484#issuecomment-2877276631)
Hmm, my first thought was just to backport the "fix" instead, but I guess we've never really backported secp changes for the sake of keeping in sync.
Either way, in this case I agree it's simple/safe enough to just use the warning for 28.x.
💬 stringintech commented on pull request "kernel: Separate UTXO set access from validation functions":
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2087260406)
As @TheCharlatan noted [here](https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2083133917), delaying `txdata.Init` could be on purpose for the potential cache hit [here](https://github.com/TheCharlatan/bitcoin/blob/16a695fbff4a92eba3eb72ec6aa766e71c52f773/src/validation.cpp#L2195:L2201). I wonder if there is a cleaner version for achieving the same goal though.
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2087260406)
As @TheCharlatan noted [here](https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2083133917), delaying `txdata.Init` could be on purpose for the potential cache hit [here](https://github.com/TheCharlatan/bitcoin/blob/16a695fbff4a92eba3eb72ec6aa766e71c52f773/src/validation.cpp#L2195:L2201). I wonder if there is a cleaner version for achieving the same goal though.
👍 hebasto approved a pull request: "[28.x] build: suppress `-Wunterminated-string-initialization` in secp256k1"
(https://github.com/bitcoin/bitcoin/pull/32484#pullrequestreview-2837501535)
ACK 5c8a55f917d089c8bdf5f59195116d9201c597e7, tested on Ubuntu 25.04 with GCC-15.
(https://github.com/bitcoin/bitcoin/pull/32484#pullrequestreview-2837501535)
ACK 5c8a55f917d089c8bdf5f59195116d9201c597e7, tested on Ubuntu 25.04 with GCC-15.
💬 fanquake commented on pull request "[27.x] Backports":
(https://github.com/bitcoin/bitcoin/pull/32479#issuecomment-2877292087)
> This is failing in the macOS CI. An issue with Boost Process:
Looks this is because Boost Process V1 compat was broken in Boost 1.88.0, https://github.com/boostorg/process/issues/480, which is the version being used in the macOS CI here, and `--enable-external-signer` is passed globally in the CI.
(https://github.com/bitcoin/bitcoin/pull/32479#issuecomment-2877292087)
> This is failing in the macOS CI. An issue with Boost Process:
Looks this is because Boost Process V1 compat was broken in Boost 1.88.0, https://github.com/boostorg/process/issues/480, which is the version being used in the macOS CI here, and `--enable-external-signer` is passed globally in the CI.
💬 stringintech commented on pull request "kernel: Separate UTXO set access from validation functions":
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2087263016)
It compiles for me as well.
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2087263016)
It compiles for me as well.
👍 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.