💬 ryanofsky commented on issue "Rename bitcoin-wallet?":
(https://github.com/bitcoin/bitcoin/issues/31827#issuecomment-2648684591)
> Maybe to phrase it differently, it seems that the idea is to expand the current `bitcoin-wallet` to support IPC, as implemented in [#19460](https://github.com/bitcoin/bitcoin/pull/19460). So there's no name clash.

I just think that's an implementation detail and there's no need to break the command line or rename binaries in any case.

There are two cases (1) wallet ipc features (like connecting to a node to perform some online operation, or starting an dedicated RPC server) are implemented i
...

💬 danielabrozzoni commented on pull request "logging: Ensure -debug=0/none behaves consistently with -nodebug":
(https://github.com/bitcoin/bitcoin/pull/31767#discussion_r1949530919)
This is way nicer! Updated, thanks

🤔 danielabrozzoni reviewed a pull request: "logging: Ensure -debug=0/none behaves consistently with -nodebug"
(https://github.com/bitcoin/bitcoin/pull/31767#pullrequestreview-2606566000)
Thank you for the reviews! In my last push:
- I removed every occurences of "rpc" (sorry I didn't catch them the first time!)
- I've renamed a couple of variables, added a comment, and addressed @hodlinator's nits, to improve clarity

💬 danielabrozzoni commented on pull request "logging: Ensure -debug=0/none behaves consistently with -nodebug":
(https://github.com/bitcoin/bitcoin/pull/31767#discussion_r1949532227)
I find this a bit harder to read, so I'll keep it as it is.

💬 Sjors commented on pull request "ci: build multiprocess on most jobs":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2648772960)
@ryanofsky ab831be2a93d9c30408192ca4eaa1552ac6bc3dc didn't help, same `undefined reference to `LLVMFuzzerTestOneInput'`: https://cirrus-ci.com/task/5233064575500288?logs=ci#L3205

💬 achow101 commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#discussion_r1949615521)
Huh. It's supposed to be either `signature-osx-x86_64.tar.gz` or `signature-osx-arm64.tar.gz`. Is `file` different on Mac vs Linux?

💬 achow101 commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1949629548)
> These invalid scripts could be added as candidates because the underlying script could be a P2SH?

Yes

💬 achow101 commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1949631388)
`deriveaddresses` is not a wallet RPC.

💬 achow101 commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1949631776)
`validateaddress` is not a wallet RPC.

💬 jimhashhq commented on pull request "cmake: Copy `cov_tool_wrapper.sh.in` to the build tree":
(https://github.com/bitcoin/bitcoin/pull/31722#issuecomment-2648828450)
One yes/vote, one question, one clarification..

_Yes/vote_ to move test execution to CMakeLists config via "ctest" targets.
This resource is anyway legacy to Autotools, and imagine it be supplanted by ctest targets.

_Question_ @hebasto: Since supplied fix has been refactored, will a cmake-only feature branch be available to acceptance test this? Understanding that support for additional build types and fuzzing resolution may be pending.

_Clarification_ @theuni: Not an expert myself,
...

💬 achow101 commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1949643820)
The legacy wallet does not know that it has the internal key for this output, nor does it know any of the other taproot spend data. Therefore, it is unable to sign the input as it does not know which key to use.

💬 achow101 commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1949644736)
Walking through the signing logic with every possible script type I could think of, and then contriving a test.

💬 achow101 commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1949647905)
Going to leave this as is since `migrate_and_get_rpc` does not check the descriptor-ness of the watchonly and solvables wallets.

💬 achow101 commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1949653191)
> You mean a transaction with both a taproot and non-taproot input?

Yes

💬 achow101 commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1949655955)
> Is it because the `finalizepsbt` doesn't actually sign the inputs again because there are signatures already in the PSBT albeit using a different sighash type?

`finalizepsbt` does not actually sign even though it calls the sign functions. It does not have access to private keys.

> Shouldn't `PSBTInputSignedAndVerified()` fail here because the sighash type doesn't match with the signature?

The BIP states that the sighash field is merely a suggestion and signers are allowed to ignore it
...

💬 achow101 commented on pull request "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`":
(https://github.com/bitcoin/bitcoin/pull/31243#discussion_r1949663437)
The benefit is in the MuSig2 PRs where `GetPrivKey` can insert multiple private keys to the `FlatSigningProvider`, none of which are the private key for the MuSig2 aggregate pubkey itself.

For the `ConstPubkeyProvider`, it's useful to have another `GetPrivKey` that returns the single private key since this is behavior is used in a few places within this class's member functions.

💬 achow101 commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1949666663)
Done

💬 achow101 commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1949666789)
Done

💬 achow101 commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1949666916)
Done

💬 achow101 commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1949667018)
Done