💬 rkrux commented on pull request "refactor: use options struct for signing and PSBT operations":
(https://github.com/bitcoin/bitcoin/pull/32876#discussion_r2200122553)
In d94c02e109617a7a4d31a5f03d6f5221f2faf878 "refactor: use SignOptions for MutableTransactionSignatureCreator"

Possible to use `SIGHASH_ALL` here instead of `1` now?

💬 rkrux commented on pull request "refactor: use options struct for signing and PSBT operations":
(https://github.com/bitcoin/bitcoin/pull/32876#discussion_r2200043048)
In 576da4c7dd83f7ab871bac9d76685d112e60e4ab "refactor: use PSBTFillOptions for filling and signing"

ISTM that the default value of `bip32derivs` is switched from `false` to `true` with the use of `PSBTFillOptions` here. Is this intended?

💬 rkrux commented on pull request "refactor: use options struct for signing and PSBT operations":
(https://github.com/bitcoin/bitcoin/pull/32876#discussion_r2200086138)
In 576da4c7dd83f7ab871bac9d76685d112e60e4ab "refactor: use PSBTFillOptions for filling and signing"

This also does the same change for `bip32derivs` default value in SPKMs. Intended?

🤔 rkrux reviewed a pull request: "refactor: use options struct for signing and PSBT operations"
(https://github.com/bitcoin/bitcoin/pull/32876#pullrequestreview-3009152210)
Code review d94c02e109617a7a4d31a5f03d6f5221f2faf878

Mostly lgtm, asked couple questions along with couple nits.

Using `SIGHASH_DEFAULT` as the default value in this PR instead of relying on `int sighash` being `0` as done previously is much better from readability POV.

💬 rkrux commented on pull request "refactor: use options struct for signing and PSBT operations":
(https://github.com/bitcoin/bitcoin/pull/32876#discussion_r2200027956)
In 576da4c7dd83f7ab871bac9d76685d112e60e4ab "refactor: use PSBTFillOptions for filling and signing"

Opinionated supernit if retouched: For variables that are supposed to take action, names starting with a verb is easier to relate to imho. Eg: `sign` & `finalize` in this struct.
So, maybe `derive_bip32.`

💬 rkrux commented on pull request "refactor: use options struct for signing and PSBT operations":
(https://github.com/bitcoin/bitcoin/pull/32876#discussion_r2199999001)
In 576da4c7dd83f7ab871bac9d76685d112e60e4ab "refactor: use PSBTFillOptions for filling and signing"

Nit if retouched:
```diff
- if (SignPSBTInput(DUMMY_SIGNING_PROVIDER, psbtx, i, nullptr, std::nullopt) != PSBTError::OK || !psbtx.GetInputUTXO(newcoin.out, i)) {
+ if (SignPSBTInput(DUMMY_SIGNING_PROVIDER, psbtx, i, nullptr, /*options=*/{}) != PSBTError::OK || !psbtx.GetInputUTXO(newcoin.out, i)) {
```

💬 fanquake commented on pull request "Resolve guix non-determinism with emplace_back instead of push_back":
(https://github.com/bitcoin/bitcoin/pull/32930#issuecomment-3061439683)
Guix Build (aarch64):
```bash
b75d75ed9b64aadadcccce97bf1fcafa2802c916f4a12befe7fef3260f2d5bb7 guix-build-f43571010e38/output/aarch64-linux-gnu/SHA256SUMS.part
a9cdcb840b05cad7c69dafb13003ff2e051ab162e30a8959af011f76c5abe576 guix-build-f43571010e38/output/aarch64-linux-gnu/bitcoin-f43571010e38-aarch64-linux-gnu-debug.tar.gz
7269ff5420cd864c351fc075c0cd8b57c9f73264dd5083eabeddf5f673a88263 guix-build-f43571010e38/output/aarch64-linux-gnu/bitcoin-f43571010e38-aarch64-linux-gnu.tar.gz
8c169b
...

✅ fanquake closed an issue: "guix: Windows build is non-deterministic across build architectures"
(https://github.com/bitcoin/bitcoin/issues/32923)

🚀 fanquake merged a pull request: "Resolve guix non-determinism with emplace_back instead of push_back"
(https://github.com/bitcoin/bitcoin/pull/32930)

💬 fanquake commented on pull request "depends: Force `CMAKE_EXPORT_NO_PACKAGE_REGISTRY=TRUE`":
(https://github.com/bitcoin/bitcoin/pull/32943#issuecomment-3061514214)
This is going to need backporting, to fix the 29.x branch.

💬 Sjors commented on pull request "Enable `-Werror=dev` in CI & Guix":
(https://github.com/bitcoin/bitcoin/pull/32937#issuecomment-3061521322)
ACK 35e0331493e97f8c9a79e40449fe2787dc798f3c

Test by deliberately triggering one with `message(AUTHOR_WARNING`. By default for `cmake -B build` it's just a warning, but with `--preset dev-mode` it's an error.

Guix build on x86_64:

```
c7870f6c402ac407d0de4edc21d42f5040d19374deaba50b496e4f99a421c8e8 guix-build-35e0331493e9/output/aarch64-linux-gnu/SHA256SUMS.part
d64532e1bd83568f2386a6b67c49e0843f08c0d486828b1cd0ce955a179b07bb guix-build-35e0331493e9/output/aarch64-linux-gnu/bitcoin-
...

💬 Sjors commented on pull request "refactor: use options struct for signing and PSBT operations":
(https://github.com/bitcoin/bitcoin/pull/32876#discussion_r2200231328)
Although I agree that's a better name, renaming would introduce (even) more churn.

👍 dergoegge approved a pull request: "fuzz: Add missing calls to `SetMockTime` for determinism"
(https://github.com/bitcoin/bitcoin/pull/32927#pullrequestreview-3009530556)
utACK fa8862723c14cdd471bbffc7a4897ece2e6d8a7f

👍 dergoegge approved a pull request: "fuzz: Make process_message(s) more deterministic"
(https://github.com/bitcoin/bitcoin/pull/32822#pullrequestreview-3009550765)
utACK fa501ea5ed874ffcba2b606806460e61a1204bd2

As a follow up (if someone is interested), we could add the block hashes for the blocks with mature coinbases (as well as the mature coinbase prevouts) to the [dictionary](https://github.com/bitcoin-core/qa-assets/blob/main/fuzz_dicts/net_processing.dict) in qa-assets.

💬 Sjors commented on pull request "refactor: use options struct for signing and PSBT operations":
(https://github.com/bitcoin/bitcoin/pull/32876#discussion_r2200273610)
Done

💬 Sjors commented on pull request "refactor: use options struct for signing and PSBT operations":
(https://github.com/bitcoin/bitcoin/pull/32876#discussion_r2200273799)
Every call to `FillPSBT(` sets `bip32derivs` explicitly so this change shouldn't matter.

Except in `src/wallet/feebumper.cpp` where I originally dropped `/*bip32derivs=*/true`. I brought that back for clarity.

💬 Sjors commented on pull request "refactor: use options struct for signing and PSBT operations":
(https://github.com/bitcoin/bitcoin/pull/32876#issuecomment-3061603729)
Rebased (just in case after ##32930) and addressed comments, mainly:
- using `SIGHASH_ALL` instead of `1`
- making sure `bip32derivs` is explicitly set everywhere (added `SignTransaction` in `fee_bumper.cpp`) because the default changed

🤔 rkrux reviewed a pull request: "wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively"
(https://github.com/bitcoin/bitcoin/pull/28333#pullrequestreview-3009605243)
I seem to be getting onboard with this idea as I can notice readability improvements on a cursory glance. Do you anticipate any performance improvements as well?

The last sentence related to legacy SPKMs of the last paragraph in the PR description can be removed now.

Will do full review soon.

👍 dergoegge approved a pull request: "refactor: Convert GenTxid to `std::variant`"
(https://github.com/bitcoin/bitcoin/pull/32631#pullrequestreview-3009607187)
Code review ACK a60f863d3e276534444571282f432b913d3967db

💬 dergoegge commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2200291621)
nit: Imo, function parameters should reflect what a function needs to do its job, as opposed to passing it what you have available at the call sites.

`CInv` can be more than a tx announcement, and it's only used here to be passed to `ToGenTxid` (which crashes if its not a tx inv). Distinguishing tx from other announcements is not the job of `FindTxForGetData`. It only needs a wtxid/txid to do its job and should take a `GenTxid` (or be templated).