💬 letetrautre commented on pull request "test: Force named args in RPCOverloadWrapper optional args":
(https://github.com/bitcoin/bitcoin/pull/32360#discussion_r2063286019)
bc1qw4lvl39reqarm65hlq2rfnvr4s4hpr8cmdttaf

💬 letetrautre commented on pull request "test: Force named args in RPCOverloadWrapper optional args":
(https://github.com/bitcoin/bitcoin/pull/32360#discussion_r2063287118)
bc1qw4lvl39reqarm65hlq2rfnvr4s4hpr8cmdttaf

💬 hebasto commented on pull request "test: avoid stack overflow in `FindChallenges` via manual iteration":
(https://github.com/bitcoin/bitcoin/pull/32351#issuecomment-2834604584)
@theuni

Due to your https://github.com/bitcoin/bitcoin/pull/31367#issuecomment-2524210640, you might b e interested in reviewing this PR.

🤔 Sjors requested changes to a pull request: "Remove the legacy wallet and BDB dependency"
(https://github.com/bitcoin/bitcoin/pull/28710#pullrequestreview-2798630677)
Concept ACK.

bc3f07e384c2e145d6d14cfa3ad65b976233b538 looks good, except:

1. The GUI now shows a watch-only balance for descriptor wallets. See inline for the culprit.

2. In that same commit, the churn in `wallet/scriptpubkeyman.cpp` to follow, both here on Github as well as a `git show --color-moved=dimmed-zebra`. I'm guessing you're moving stuff around, deleting and modifying?

And a few details:
- there's one more `-DWITH_BDB=ON` in `test-each-commit-exec.sh`
- `test_framework/bd
...

💬 Sjors commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2063255286)
In dc7bf5fd6a320c4528a28cef2a565366bbab3877 "wallet: Delete LegacySPKM": why doesn't this just return `false` like before? And if so, can't the entire helper be dropped?

💬 Sjors commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2063278522)
In dc7bf5fd6a320c4528a28cef2a565366bbab3877 "wallet: Delete LegacySPKM": this test (file) and the one below still exist.

💬 maflcko commented on issue "ctest -C Debug fails on vs2022 (miniscript_tests (SEGFAULT))":
(https://github.com/bitcoin/bitcoin/issues/32341#issuecomment-2834611665)
> I'd say for the CI we want to reset `/Ob0 /Od` back to `/O2 /Ob1` to avoid slowness.

@hebasto If you don't mind, could you update your CI branch with this. If not, that's fine also and I'll try it myself.

💬 hebasto commented on issue "build: CMake caching failing PIE check":
(https://github.com/bitcoin/bitcoin/issues/32323#issuecomment-2834619343)
> are you saying we don't fix this?

Correct.

> Are you saying we should add a recommendation to the build docs to always delete the build dir before running CMake? Something else?

Not "always" — only when configuration fails. Similar to https://github.com/bitcoin/bitcoin/issues/31942#issuecomment-2737914378.

💬 tomasandroil 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-2834621951)
@hebasto
I have upstreamed the changes as requested: [arun11299/cpp-subprocess#117](https://github.com/arun11299/cpp-subprocess/pull/117).

💬 hebasto commented on issue "ctest -C Debug fails on vs2022 (miniscript_tests (SEGFAULT))":
(https://github.com/bitcoin/bitcoin/issues/32341#issuecomment-2834635374)
> > I'd say for the CI we want to reset `/Ob0 /Od` back to `/O2 /Ob1` to avoid slowness.
>
> [@hebasto](https://github.com/hebasto) If you don't mind, could you update your CI branch with this. If not, that's fine also and I'll try it myself.

Will do.

⚠️ fanquake opened an issue: "windows: usage of deprecated `std:wstring_convert`"
(https://github.com/bitcoin/bitcoin/issues/32361)
`std:wstring_convert` was deprecated in C++17 & is removed in C++26.
Cross-compiling for Windows with GCC 15.1.0 is warning about this:
```bash
../../src/common/system.cpp: In function 'void runCommand(const std::string&)':
../../src/common/system.cpp:52:32: warning: 'template<class _Codecvt, class _Elem, class _Wide_alloc, class _Byte_alloc> class std::__cxx11::wstring_convert' is deprecated [-Wdeprecated-declarations]
52 | int nErr = ::_wsystem(std::wstring_convert<std::codecvt_utf8_utf
...

👍 maflcko approved a pull request: "Remove the legacy wallet and BDB dependency"
(https://github.com/bitcoin/bitcoin/pull/28710#pullrequestreview-2798181194)
lgtm ACK bc3f07e384c2e145d6d14cfa3ad65b976233b538 💇

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK bc3f07e384c2e145
...

💬 maflcko commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2063011962)
As a follow-up idea, someone could investigate if this is still needed or if it can be cleaned up

💬 maflcko commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2063234336)
in dc7bf5fd6a320c4528a28cef2a565366bbab3877: Are `labelWatch*` still needed?

💬 maflcko commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2063241867)
in https://github.com/bitcoin/bitcoin/commit/dc7bf5fd6a320c4528a28cef2a565366bbab3877: Does not look right?

💬 maflcko commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2063255483)
dc7bf5fd6a320c4528a28cef2a565366bbab3877: Needs to update the RPC help, to remove the field?

💬 maflcko commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2063310245)
(same commit): I wonder if this is fine to remove, given that someone could have a non-hd backup where some keys in the keypool have been used. For migration, it could make sense to take those as well, or document that they will not be taken?

💬 maflcko commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2063257060)
Same?

💬 maflcko commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2063301086)
(same commit): Should remove both comments, or none?

💬 maflcko commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2063317183)
cc3c3b9958b31ddfc03905ed969c4da0a3bf4409: could probably fully remove this, given that it would be trivial to re-add, if ever needed.