💬 achow101 commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2059298259)
Done.

💬 achow101 commented on pull request "Fix failing util_time_GetTime test on Windows":
(https://github.com/bitcoin/bitcoin/pull/32318#issuecomment-2828964023)
ACK 3dbd50a576be55941cb4b5034dc2171c03afb07c

🚀 achow101 merged a pull request: "Fix failing util_time_GetTime test on Windows"
(https://github.com/bitcoin/bitcoin/pull/32318)

💬 pablomartin4btc commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#issuecomment-2828981936)
> I tried to restore a legacy wallet with this PR and it doesn't do it, nor are there any issues...

Ok, I thought the restore would allow to do it and then the load_on_startup would fail but the restore fail on the wallet file verification, so all good.

Tested `restorewallet` creating a legacy on `master`, backed it up there and trying to restore it on this PR's branch.
```
./build_31250/bin/bitcoin-cli -regtest -datadir=/tmp/btc-wallet restorewallet "restored_legacy_master" /tmp/btc-wal
...

💬 pablomartin4btc commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#issuecomment-2828988031)
> Do you mean there is a possibility here if the interface is used incorrectly internally?

I've tested it with this branch's QT and same failure on wallet file verifitcation while trying to restore a legacy backup using the GUI. All good.

💬 w0xlt commented on pull request "refactor: Update `XOnlyPubKey::GetKeyIDs()` to return a pair of pubkeys":
(https://github.com/bitcoin/bitcoin/pull/32332#issuecomment-2828996310)
@hodlinato yes, this decreases heap usage, but I don't think there are any relevant performance gains.

🤔 w0xlt reviewed a pull request: "Wallet: Fix Non-Ranged Descriptors with Range [0,0] Trigger Unexpected Wallet Errors in AddWalletDescriptor"
(https://github.com/bitcoin/bitcoin/pull/32344#pullrequestreview-2792671179)
Concept ACK.

Functional tests might be necessary.

💬 davidgumberg commented on pull request "net: remove unnecessary check from AlreadyConnectedToAddress()":
(https://github.com/bitcoin/bitcoin/pull/32338#issuecomment-2829070906)
It would also be good (maybe sufficient) to add a unit test which enforces/documents the behavior that a peer with the same address as an existing peer and a different peer will be seen as already connected to. e.g.:

```diff
diff --git a/src/test/net_peer_connection_tests.cpp b/src/test/net_peer_connection_tests.cpp
index e60ce8b99d..4441a8dde4 100644
--- a/src/test/net_peer_connection_tests.cpp
+++ b/src/test/net_peer_connection_tests.cpp
@@ -155,6 +155,13 @@ BOOST_FIXTURE_TEST_CASE(tes
...

💬 furszy commented on pull request "bench: close wallets after migration":
(https://github.com/bitcoin/bitcoin/pull/32309#issuecomment-2829077127)
> I believe we've kept it merely as a sanity check because it's the only test that will survive the annihilation of the legacy wallet code.

Best bench-test ever. Just caught a real bug in #31423.

💬 achow101 commented on pull request "util: detect and warn when using exFAT on MacOS":
(https://github.com/bitcoin/bitcoin/pull/31453#issuecomment-2829171034)
It looks like this is happening early enough in init that the warning never actually makes it to the GUI, so GUI users (probably a majority of MacOS users) won't actually see the warning.

🤔 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