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

💬 maflcko commented on pull request "test: handle potential None value for change address in setlabel":
(https://github.com/bitcoin/bitcoin/pull/33055#issuecomment-3113970375)
Thanks, but there is no need to open an LLM generated pull request without any description and motivation. Please do not submit LLM generated stuff that you do not understand yourself.

🤔 marcofleon reviewed a pull request: "[29.x] backport #32069"
(https://github.com/bitcoin/bitcoin/pull/33052#pullrequestreview-3052237027)
ACK c6fe6971bfa52d8fe2901a051f18da9f3bcb26a9

💬 raul-anton-2005 commented on pull request "test: handle potential None value for change address in setlabel":
(https://github.com/bitcoin/bitcoin/pull/33055#issuecomment-3113984652)
Mmmm, it was not LLM generated. I understood what I did since it was not a significant but good practice change. I can open it with an explenation if you want.

💬 maflcko commented on pull request "util: Abort on failing CHECK_NONFATAL in debug builds":
(https://github.com/bitcoin/bitcoin/pull/32588#discussion_r2228928982)
> 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.

I think it is 100% accurate, because `test_only_CheckFailuresAreExceptionsNotAborts` does not affect `CHECK_NONFATAL` in commit https://github.com/bitcoin/bitcoin/commit/fa8251bb883300bb84cad5486468d07b5b1b7322, so the fuzz test and unit test are 100% identical and redundant in this commit.

💬 mzumsande commented on pull request "net: Fix Discover() not running when using -bind=0.0.0.0:port":
(https://github.com/bitcoin/bitcoin/pull/32757#issuecomment-3114010941)
> ifconfig en0 alias 1.1.1.1

ok, you don't add it to the loopback interface.
But the instructions for Linux
`ifconfig lo:0 1.1.1.1/32 up && ifconfig lo:1 2.2.2.2/32 up # to set up`
do that, while #29984 resulted in `IFF_LOOPBACK` addresses being filtered out. So I don't think the problem from https://github.com/bitcoin/bitcoin/issues/31336 is fixed by this as the OP says.

🚀 fanquake merged a pull request: "[29.x] backport #32069"
(https://github.com/bitcoin/bitcoin/pull/33052)

💬 sstone commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r2228937333)
I had forgotten this one , fixed in https://github.com/bitcoin/bitcoin/pull/24539/commits/f2a496c827442c7650e5742fdfd49147f0da0143

💬 maflcko commented on pull request "test: handle potential None value for change address in setlabel":
(https://github.com/bitcoin/bitcoin/pull/33055#issuecomment-3114023892)
If it wasn't LLM generated, it is still wrong, because the value is never `None`. And if it was, it would have already been handled properly.

💬 fanquake commented on pull request "guix: Always canonicalize HOST using `./depends/config.sub`":
(https://github.com/bitcoin/bitcoin/pull/21671#issuecomment-3114038666)
I'm going to remove "Up for grabs" here. Not entirely convinvced that we should end up doing this.