💬 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?
💬 hebasto commented on pull request "[29.x] Backports":
(https://github.com/bitcoin/bitcoin/pull/32292#issuecomment-2834719701)
> * [Replace stray tfm::format to cerr with qWarning bitcoin-core/gui#868](https://github.com/bitcoin-core/gui/pull/868)
It seems `#include <QDebug>` is missed.
(https://github.com/bitcoin/bitcoin/pull/32292#issuecomment-2834719701)
> * [Replace stray tfm::format to cerr with qWarning bitcoin-core/gui#868](https://github.com/bitcoin-core/gui/pull/868)
It seems `#include <QDebug>` is missed.
💬 fanquake commented on issue "[rfc] build: Reject unclean configure?":
(https://github.com/bitcoin/bitcoin/issues/31942#issuecomment-2834730805)
> and adjust it to also include linker or compile errors?
I guess we should also be adjusting it to recommend deleting the whole build dir? Otherwise I'd assume it's just going to lead to even more subtle CMake issues later on.
(https://github.com/bitcoin/bitcoin/issues/31942#issuecomment-2834730805)
> and adjust it to also include linker or compile errors?
I guess we should also be adjusting it to recommend deleting the whole build dir? Otherwise I'd assume it's just going to lead to even more subtle CMake issues later on.
💬 maflcko commented on pull request "doc: Improve `dependencies.md`":
(https://github.com/bitcoin/bitcoin/pull/31895#discussion_r2063356972)
Is there any value in documenting the version used twice with the additional maintenance overhead and risk of it being stale? The depends package definition is already linked, so anyone can get the real version with just one more click/open. If they want to find out the pull request/commit it will be just another step (git blame).
Could consider removing it?
(https://github.com/bitcoin/bitcoin/pull/31895#discussion_r2063356972)
Is there any value in documenting the version used twice with the additional maintenance overhead and risk of it being stale? The depends package definition is already linked, so anyone can get the real version with just one more click/open. If they want to find out the pull request/commit it will be just another step (git blame).
Could consider removing it?
⚠️ maflcko reopened an issue: "[rfc] build: Reject unclean configure?"
(https://github.com/bitcoin/bitcoin/issues/31942)
When re-configuring a build, it is possible to do so in an unclean way on a stale build folder.
The average person initiating such a build does not know when a stale build is safe and when not. Time is wasted for the builder when a broken build silently succeeds and then leads to a nonsensical error later on. Just to give the last two examples of the last few days: (https://github.com/bitcoin/bitcoin/issues/31922#issuecomment-2673888781, https://github.com/bitcoin/bitcoin/pull/31933#discussion_
...
(https://github.com/bitcoin/bitcoin/issues/31942)
When re-configuring a build, it is possible to do so in an unclean way on a stale build folder.
The average person initiating such a build does not know when a stale build is safe and when not. Time is wasted for the builder when a broken build silently succeeds and then leads to a nonsensical error later on. Just to give the last two examples of the last few days: (https://github.com/bitcoin/bitcoin/issues/31922#issuecomment-2673888781, https://github.com/bitcoin/bitcoin/pull/31933#discussion_
...