🤔 BrandonOdiwuor reviewed a pull request: "test: Add missing check for empty stderr in util tester"
(https://github.com/bitcoin/bitcoin/pull/32327#pullrequestreview-2792840128)
Code Review ACK fadf12a56c294696052c4cb6ee5284030ada7498

💬 theweb3era commented on something "":
(https://github.com/bitcoin/bitcoin/commit/faeb5b59a098578b3e8c552d35b5ba02b12af14d#commitcomment-155858535)
.qodo

<?xml version="1.0" encoding="utf-8"?>
<Project DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
<Import Project="..\common.init.vcxproj" />
<Import Project="..\common.qt.init.vcxproj" />
<PropertyGroup Label="Globals">
<ProjectGuid>{2B4ABFF8-D1FD-4845-88C9-1F3C0A6512BF}</ProjectGuid>
<ConfigurationType>StaticLibrary</ConfigurationType>
</PropertyGroup>
<ItemGroup>
<ClCompile Include="..\..\src\qt\addressbookpage.cpp" />
...

💬 Eunovo commented on pull request "Wallet: Fix Non-Ranged Descriptors with Range [0,0] Trigger Unexpected Wallet Errors in AddWalletDescriptor":
(https://github.com/bitcoin/bitcoin/pull/32344#issuecomment-2829372533)
> Functional tests might be necessary.

The `importdescriptors` RPC sets the range for all non-ranged descriptors to `[0,1]` in every request so this error cannot be triggered using the RPC.

In https://github.com/bitcoin/bitcoin/issues/31728 @JeremyRubin explains that this error may be unexpectedly triggered when " experimenting with non-ranged descriptors in custom wallet workflows."

🤔 shahsb reviewed a pull request: "Fix missing error check in `set_clo_on_exec` for FD_CLOEXEC handling"
(https://github.com/bitcoin/bitcoin/pull/32342#pullrequestreview-2793059925)
Concept ACK

💬 shahsb commented on pull request "Fix missing error check in `set_clo_on_exec` for FD_CLOEXEC handling":
(https://github.com/bitcoin/bitcoin/pull/32342#issuecomment-2829437457)
ACK https://github.com/bitcoin/bitcoin/pull/32342/commits/b0415ab50e6d58375063e4616a0aad7788a6128e

💬 maflcko commented on issue "ctest -C Debug fails on vs2022 (Exit code 0xc0000135)":
(https://github.com/bitcoin/bitcoin/issues/32341#issuecomment-2829441596)
Thx. Though, now it says:

```
The following tests FAILED:
60 - miniscript_tests (SEGFAULT)
Errors while running CTest
```

This raises two questions:

* I'd say for the CI we want to reset `/Ob0 /Od` back to `/O2 /Ob1` to avoid slowness.
* I guess this is not really a segfault/bug in the code, but rather a stack overflow, due to the debug config eating more memory?

💬 hebasto commented on issue "ctest -C Debug fails on vs2022 (Exit code 0xc0000135)":
(https://github.com/bitcoin/bitcoin/issues/32341#issuecomment-2829507443)
> Thx. Though, now it says:
>
> ```
> The following tests FAILED:
> 60 - miniscript_tests (SEGFAULT)
> Errors while running CTest
> ```
>
> * I guess this is not really a segfault/bug in the code, but rather a stack overflow, due to the debug config eating more memory?

I'm not sure. I can reproduce the issue locally and it refers to an exception:
```
Start 58: miniscript_tests
1/1 Test #58: miniscript_tests .................***Exception: SegFault 51.83 sec
```

Will try to narrow it
...

💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2059668657)
I felt we were going in circles here, that's why I illustrated it by linking back up the thread. The code is not dead, it is there to avoid the behavior not being broken when those args are activated by a user or CI.

This change is about testing the behavior of the test framework. We could also add tests to test the testing of the test framework, but at some point, this recursion has to stop.

Maybe we're talking past each other in some way I don't perceive?

💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#issuecomment-2829540751)
Drahtbot's LLM is complaining about an assert string: https://github.com/bitcoin/bitcoin/pull/30660#issuecomment-2291135616
The Python interpreter is fine with mixing regular `""`-strings and `f""`-strings, so it's a false positive.

👍 pablomartin4btc approved a pull request: "Crash fix, disconnect numBlocksChanged() signal during shutdown"
(https://github.com/bitcoin-core/gui/pull/864#pullrequestreview-2793195492)
re-ACK 71656bdfaa6bfe08ce9651246a3ef606f923351b

💬 maflcko commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#issuecomment-2829565889)
> Drahtbot's LLM is complaining ... [#30660 (comment)](https://github.com/bitcoin/bitcoin/pull/30660#issuecomment-2291135616)
> ... it's a false positive.

Yes, there are some rare false-positives. However, there are many more true positives, including one logic bug that was already found in a test in another pull, so I think overall it is worth it. Happy to discuss further in https://github.com/maflcko/DrahtBot/issues, or anywhere else.

💬 maflcko commented on issue "test: bip324_tests & net_tests failure with `-O3 -flto`":
(https://github.com/bitcoin/bitcoin/issues/32337#issuecomment-2829568058)
> gcc (Ubuntu 13.3.0-6ubuntu2~24.04) 13.3.0

You can use g++-14 from Ubuntu, which seems to work. Not sure if it was or will be fixed in gcc 13.4 as well (and if Ubuntu/Debian take that point release at all), but for now g++14 is probably your best bet.

I've also checked g++-15 nightly, and it seems to work as well: https://github.com/maflcko/b-c-nightly/actions/runs/14658126783/job/41136699706

I suspect that the issue initially reported here in this thread is musl related?

💬 maflcko commented on pull request "Crash fix, disconnect numBlocksChanged() signal during shutdown":
(https://github.com/bitcoin-core/gui/pull/864#issuecomment-2829592615)
lgtm ACK 71656bdfaa6bfe08ce9651246a3ef606f923351b

I have not reviewed the code, but I tried to reproduce the segfault with my steps to reproduce and it seemed to pass with about 15 restarts.

💬 maflcko commented on pull request "[29.x] Backports":
(https://github.com/bitcoin/bitcoin/pull/32292#issuecomment-2829596228)
Could also include https://github.com/bitcoin-core/gui/pull/864, possibly?

💬 maflcko commented on pull request "wallet: migration, don't create spendable wallet from a watch-only legacy wallet":
(https://github.com/bitcoin/bitcoin/pull/31423#issuecomment-2829614623)
There are some typos reported, which look real: https://github.com/bitcoin/bitcoin/pull/31423#issuecomment-2520009159.

💬 maflcko commented on issue "ctest -C Debug fails on vs2022 (Exit code 0xc0000135)":
(https://github.com/bitcoin/bitcoin/issues/32341#issuecomment-2829642643)
A third question would be, why the fuzz output is https://github.com/hebasto/bitcoin/actions/runs/14650416249/job/41114604643#step:14:60: `subprocess timed out: Currently only libFuzzer is supported`. A timeout should not happen for this process call, and the expected outcome should be an immediate stderr of `test/fuzz/fuzz.cpp:269 main: Assertion 'read_file(input_path, buffer)' failed. Error processing input "-help=1"` and an ignored exit code.

✅ maflcko closed a pull request: "wallet: Disable creating and loading legacy wallets"
(https://github.com/bitcoin/bitcoin/pull/31250)

💬 maflcko commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#issuecomment-2829667411)
(fresh CI)

📝 maflcko reopened a pull request: "wallet: Disable creating and loading legacy wallets"
(https://github.com/bitcoin/bitcoin/pull/31250)
To prepare for the deletion of legacy wallet code, disable creating or loading new legacy wallets.

Tests for the legacy wallet specifically are deleted.

Split from https://github.com/bitcoin/bitcoin/pull/28710

🤔 pablomartin4btc reviewed a pull request: "fuzz: wallet: add target for spkm migration"
(https://github.com/bitcoin/bitcoin/pull/29694#pullrequestreview-2793362842)
You are skipping the backup restore in commit: "_wallet: skip backup restoration when using in-memory dbs_"; shouldn't the backup itself be skipped too when `in_memory`?