💬 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.
(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.
(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.
(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).
(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.
(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
...
(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
...
(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
(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?
(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?
(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?
(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?
(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?
(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?
(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.
(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.
💬 maflcko commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2063275827)
Also, some more stuff:
```
$ git grep -i 'legacy wallets' src/script/ src/wallet doc/*.md
doc/build-netbsd.md:`db4` is required to enable support for legacy wallets.
doc/managing-wallets.md:## Migrating Legacy Wallets to Descriptor Wallets
doc/managing-wallets.md:Legacy wallets (traditional non-descriptor wallets) can be migrated to become Descriptor wallets
doc/managing-wallets.md:Legacy wallets can know about, be watching for, and be able to sign for, `migratewallet` only
doc/psbt.md:
...
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2063275827)
Also, some more stuff:
```
$ git grep -i 'legacy wallets' src/script/ src/wallet doc/*.md
doc/build-netbsd.md:`db4` is required to enable support for legacy wallets.
doc/managing-wallets.md:## Migrating Legacy Wallets to Descriptor Wallets
doc/managing-wallets.md:Legacy wallets (traditional non-descriptor wallets) can be migrated to become Descriptor wallets
doc/managing-wallets.md:Legacy wallets can know about, be watching for, and be able to sign for, `migratewallet` only
doc/psbt.md:
...
💬 maflcko commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#issuecomment-2834673183)
> 2\. In that same commit, the churn in `wallet/scriptpubkeyman.cpp` is hard 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?
You can use `--patience`. Maybe add this to the commit message?
(https://github.com/bitcoin/bitcoin/pull/28710#issuecomment-2834673183)
> 2\. In that same commit, the churn in `wallet/scriptpubkeyman.cpp` is hard 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?
You can use `--patience`. Maybe add this to the commit message?
💬 Sjors commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2834678435)
Concept ACK
Moderation suggestion: close to non-contributors.
It's a bit draconian, but it's hard to review code in a pull request that's being brigaded, which unfortunately often happens when `OP_RETURN` or full RBF is involved. At the same time this isn't a technically difficult PR that needs thousands of eyes on it. Even if a non-contributor finds a bug and can't report it here, there's plenty of time to fix it in a followup.
The conceptual discusion should be continued on the mailin
...
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2834678435)
Concept ACK
Moderation suggestion: close to non-contributors.
It's a bit draconian, but it's hard to review code in a pull request that's being brigaded, which unfortunately often happens when `OP_RETURN` or full RBF is involved. At the same time this isn't a technically difficult PR that needs thousands of eyes on it. Even if a non-contributor finds a bug and can't report it here, there's plenty of time to fix it in a followup.
The conceptual discusion should be continued on the mailin
...
💬 michael1011 commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2834687180)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2834687180)
Concept ACK
💬 maflcko commented on issue "[rfc] build: Reject unclean configure?":
(https://github.com/bitcoin/bitcoin/issues/31942#issuecomment-2834719208)
I guess an alternative could be to take the message from https://github.com/bitcoin/bitcoin/issues/31942#issuecomment-2737914378 and adjust it to also include linker or compile errors?
(https://github.com/bitcoin/bitcoin/issues/31942#issuecomment-2834719208)
I guess an alternative could be to take the message from https://github.com/bitcoin/bitcoin/issues/31942#issuecomment-2737914378 and adjust it to also include linker or compile errors?