💬 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:
...

💬 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?

💬 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
...

💬 michael1011 commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(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?

💬 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.

💬 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.

💬 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?

⚠️ 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_
...

💬 Sjors commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2063363548)
It causes a bug in the GUI, see (1) here: https://github.com/bitcoin/bitcoin/pull/28710#pullrequestreview-2798630677

💬 Sjors commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#issuecomment-2834750612)
@maflcko `--patience` did the trick. It can be made default too if your computer is fast enough: `git config --global diff.algorithm patience`

💬 1ma commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2834776509)
@Sjors are you not interested in the feedback of the end users of the software?

💬 hebasto commented on issue "ctest -C Debug fails on vs2022 (miniscript_tests (SEGFAULT))":
(https://github.com/bitcoin/bitcoin/issues/32341#issuecomment-2834824481)
> > 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.

https://github.com/hebasto/bitcoin-core-nightly/actions/runs/14705754388/job/41265724818

💬 Sjors commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2834825682)
> @Sjors are you not interested in the feedback of the end users of the software?

Yes, but brigading is generally done by a very small group of users that don't represent the whole ecosystem. And we already know the reasons some people insist on keeping limits on `OP_RETURN`. That's why I suggested releasing a forked version of the software for those people (and I would recommend against running it, but it's your decision).

🤔 shahsb reviewed a pull request: "depends: Fix cross-compiling `qt` package from macOS to Windows"
(https://github.com/bitcoin/bitcoin/pull/32357#pullrequestreview-2798879688)
ACK https://github.com/bitcoin/bitcoin/pull/32357/commits/35e57fbe336cdcb348ff088fc04216f1f5cf2742

💬 jesterhodl commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2834844514)
> This is apparently a controversial issue and so this thread will be watched closely for moderation. Please make sure all comments are technical in nature, and on topic: the topic is the title, description, and code changes of this pull request. ~References to people~ will result in 24 hour ban, to start.
>
> edit: criticism of people, not of ideas, will result in a ban. See https://github.com/bitcoin-core/meta/blob/main/MODERATION-GUIDELINES.md

When an issue is controversial it means it
...

👍 rkrux approved a pull request: "test: Slim down previous releases bdb check"
(https://github.com/bitcoin/bitcoin/pull/32350#pullrequestreview-2798880061)
ACK fa58f40b898ba6c57655bf38a241fb10107d4a3a

It appears that the 18, 19 releases are not used elsewhere in the test. It makes sense to me to test the "descriptor-wallets-file-corrupted" error only for the last release that doesn't support descriptor wallets, which is 20. Also, helps in reducing the number of nodes needed in this test.

I have noticed that the 17 release was removed from this test recently in some PR and now 18, 19 are removed. I'd assume that the 20 release is kept in this
...

💬 rkrux commented on pull request "test: Slim down previous releases bdb check":
(https://github.com/bitcoin/bitcoin/pull/32350#discussion_r2063409325)
Nit if retouched: These 3 versions can be removed in a separate commit as they were unused before this pull as well; it would be helpful for future reference.

💬 rkrux commented on pull request "test: Slim down previous releases bdb check":
(https://github.com/bitcoin/bitcoin/pull/32350#discussion_r2063406691)
A small point: I don't find this assertion helpful and makes me wonder if it's asserting the obvious. The earlier assertion was still helpful because it asserted that the major version is less than 21 in which descriptor wallets were introduced.

I feel the earlier assertion should be preserved here.

```python
assert self.major_version_less_than(node, 21)
```

💬 laanwj commented on issue "windows: usage of deprecated `std:wstring_convert`":
(https://github.com/bitcoin/bitcoin/issues/32361#issuecomment-2834909223)
Apparently there is no direct replacement in the standard library.

i hope we can replace runCommand with use of the subprocess library soon, to unify the "launch subprocess" path.