💬 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.

📝 raul-anton-2005 opened a pull request: "test: handle potential None value for change address in setlabel"
(https://github.com/bitcoin/bitcoin/pull/33055)
<!--
*** 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
...

💬 Sjors commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-3113898522)
> so maybe if we had a separate PR enabling IPC in CI jobs first, and then this PR could just flip the depends

That's what I did before and people didn't like that: https://github.com/bitcoin/bitcoin/pull/30975#pullrequestreview-2761133797

💬 martinatime commented on issue "bitcoind 29.0 much slower than 28.0 on my system: cause found":
(https://github.com/bitcoin/bitcoin/issues/32455#issuecomment-3113904145)
> I'm running a Raspberry Pi 5 with 8GB of RAM and a 4TB SSD on RPi's Debian Lite Bookworm. This is a fresh install of v29 and after more than two weeks I'm only at blockheight of 829564 of 901531.

Just to update....this setup is still running and is at blockheight of 877015 of 906866. This means that I have been doing the initial state verification for close to two months and my estimation is that I have about 20 more days.

💬 fanquake commented on pull request "Don't fix Python patch version":
(https://github.com/bitcoin/bitcoin/pull/33051#issuecomment-3113924602)
> but then not just using pyenv install

`pyenv install` is a wrapper around `python-build`. Looks like it was used to use slightly less resources.

👍 ryanofsky approved a pull request: "util: Abort on failing CHECK_NONFATAL in debug builds"
(https://github.com/bitcoin/bitcoin/pull/32588#pullrequestreview-3052141897)
Code review ACK fac50ee1f9e577527650397891d356bca6316321, just rebased, added back functional test, and tweaked fuzz test since last review.

Overall this looks good and conceptually I like this change because it makes all the checking macros do the exact same thing in debug builds and abort, only having varying behavior in release builds.

💬 ryanofsky commented on pull request "util: Abort on failing CHECK_NONFATAL in debug builds":
(https://github.com/bitcoin/bitcoin/pull/32588#discussion_r2228833280)
In commit "test: Allow testing of check failures" (fa8251bb883300bb84cad5486468d07b5b1b7322)

Not important, but I think it makes more sense to drop this test in the next commit instead dropping it here, because the next commit is what actually breaks this test and the claim that this fuzz test is "redundant to the new unit test" is not 100% accurate, since the fuzz test and unit test execute with different `CheckFailuresAreExceptions` values.

✅ maflcko closed a pull request: "test: handle potential None value for change address in setlabel"
(https://github.com/bitcoin/bitcoin/pull/33055)