💬 maflcko commented on pull request "doc: Improve comments in FindCoins function for clarity":
(https://github.com/bitcoin/bitcoin/pull/33053#issuecomment-3113745311)
Thanks, but there is no need to add LLM generated docs to self-explanatory code.

💬 Sjors commented on pull request "Don't fix Python patch version":
(https://github.com/bitcoin/bitcoin/pull/33051#issuecomment-3113814862)
Ah I see. For some reason the linter is calling `python-build` directly instead of `pyenv install`, which just picks a patch version. Added a workaround.

👍 ryanofsky approved a pull request: "Add bitcoin-{node,gui} to release binaries for IPC"
(https://github.com/bitcoin/bitcoin/pull/31802#pullrequestreview-3051962961)
Code review ACK 082b416a1bcbfbbdcafb3a3eb1ae800c93385203. Since last review: squashed commit, updated release notes and reverted ci.yml reformatting.

Maybe it's just me, but I feel like this PR is more comprehensible now that the squashed depends commit is first. And the new release notes are also much better. (Though I would still like to unify them later with the bitcoin wrapper & libexec notes as suggested https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2228654424).

> I could
...

💬 ryanofsky commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2228720121)
In commit "build: add bitcoin-{node,gui} to Maintenance.cmake" (362e233da72b89723cdb9f4f56342716b60976eb)

Note for reviewers: Since this PR was opened, full windows support (for both connecting to unix sockets and spawning subprocesses) was implemented in #32387. #32387 is a draft just because I don't want it to be a review burden while there are other changes like #32345 that I think are more important.

💬 fanquake commented on pull request "Don't fix Python patch version":
(https://github.com/bitcoin/bitcoin/pull/33051#issuecomment-3113844771)
> E.g. if the user installed Python 3.10.14 and 3.10.16 through PyEnv, if HWI was locked to 3.10 it would default to 3.10.16. HWI then can't find any of its dependencies.

Then the user should reinstall the HWI dependencies, given they've changed Python version. They would have the same issue any time they update their Python, to a newer `3.10.x` version.

Regardless, can you solve your issue using something out of https://github.com/pyenv/pyenv?tab=readme-ov-file#understanding-python-versio
...

📝 raul-anton-2005 opened a pull request: "test: ensure change address is set only if it exists"
(https://github.com/bitcoin/bitcoin/pull/33054)
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improv
...

🤔 glozow reviewed a pull request: "wallet, rpc: add v3 transaction creation and wallet support"
(https://github.com/bitcoin/bitcoin/pull/32896#pullrequestreview-3051854806)
Looks pretty good! Generally, I prefer more detailed comments when the code is not 100% self-explanatory, particularly for the confusing areas like mempool conflicts

💬 glozow commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2228715272)
nit: Naming these `parent_it` and `parent_wtx` would make it easier to understand

💬 glozow commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2228645591)
This might be clearer:


```suggestion

// an unconfirmed TRUC must spend only TRUC, and non-TRUC must only spend non-TRUC
if ((wtx.tx->version == TRUC_VERSION) != (coinControl->m_version == TRUC_VERSION)) continue;
```

💬 glozow commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2228722754)
nit: I think "marks as unspendable" seems a little strong? Perhaps also explain why you are doing this

```suggestion
// Unconfirmed TRUC transactions are only allowed a 1-parent-1-child topology.
// For any unconfirmed v3 parents (there should be a maximum of 1 except in reorgs),
// record this child so the wallet doesn't try to spend any other outputs
```

💬 glozow commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2228739872)
I think you could `Assume(!wtx.v3_spend.has_value())` here

💬 glozow commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2228744739)
Naming everywhere should replace "v3" with "truc." We have so many versions and "legacy" types everywhere, so it's best to try to reduce ambiguity when possible. Also, "child" is more clear than "spend," and "in_mempool" would also help with clarity.

```suggestion
std::optional<Txid> truc_child_in_mempool;
```

💬 glozow commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2228762116)
Naming this `sibling_txid` would be clearer

💬 glozow commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2228759954)
Again would be helpful to add one or two sentences to explain why we need to do this and why marking as mempool conflict is the appropriate treatment for a sibling (I don't think it's immediately obvious to everyone).

💬 glozow commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2228751179)
Question: why is eecff95b6fe611c8ed40895108aacd6b44543234 necessary? It would be helpful to put the motivation in the commit message.

💬 glozow commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2228783155)
I think perhaps the wallet's default tx version should be specified in a separate constexpr that lives in the wallet code?

💬 Sjors commented on pull request "Don't fix Python patch version":
(https://github.com/bitcoin/bitcoin/pull/33051#issuecomment-3113870419)
It's not really a problem for me, but I noticed HWI stopped using patch versions in https://github.com/bitcoin-core/HWI/pull/695 and I don't think there's a good reason for us to insist on one.

Everywhere else in the project we only specify the Python minor version. And if there's ever a security issue with the patch version we picked, we probably should go and update it (which we've never done).

✅ raul-anton-2005 closed a pull request: "test: ensure change address is set only if it exists"
(https://github.com/bitcoin/bitcoin/pull/33054)

💬 fanquake commented on pull request "Don't fix Python patch version":
(https://github.com/bitcoin/bitcoin/pull/33051#issuecomment-3113880043)
> and I don't think there's a good reason for us to insist on one.

Well at least one is that we don't need to add more code/"workarounds", for a problem we don't have. (https://github.com/bitcoin/bitcoin/pull/33051#issuecomment-3113814862)

💬 Sjors commented on pull request "Don't fix Python patch version":
(https://github.com/bitcoin/bitcoin/pull/33051#issuecomment-3113885802)
But why are we cloning the PyEnv repo, but then not just using `pyenv` install? Then the workaround wouldn't be needed.