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

💬 maflcko commented on pull request "common: Close non-std fds before exec in RunCommandJSON":
(https://github.com/bitcoin/bitcoin/pull/32343#issuecomment-2834929279)
Scheduled 10 runs in https://cirrus-ci.com/task/5397894338969600, but it seems they are all passing. Huh

💬 laanwj commented on pull request "common: Close non-std fds before exec in RunCommandJSON":
(https://github.com/bitcoin/bitcoin/pull/32343#issuecomment-2834937440)
> Scheduled 10 runs in https://cirrus-ci.com/task/5397894338969600, but it seems they are all passing. Huh

It could be 4a6b99b3172fe007328bd260968acdb0dd78c569, though i have no idea why that would make a difference. In any case, nice that it passes now.

💬 maflcko commented on pull request "test: Slim down previous releases bdb check":
(https://github.com/bitcoin/bitcoin/pull/32350#issuecomment-2834978311)
> I have noticed that the 17 release was removed from this test recently in some PR and now 18, 19 are removed.

Correct. For reference, the reasons for removal were different. This removal in this pull request only became possible after the bdb removal removing the test for the v0.19 18075 behavior (https://github.com/bitcoin/bitcoin/issues/18075)

> I'd assume that the 20 release is kept in this test for a considerable time as it marks the last release that didn't contain descriptors (& SQ
...

💬 maflcko commented on pull request "test: Slim down previous releases bdb check":
(https://github.com/bitcoin/bitcoin/pull/32350#discussion_r2063491579)
Correct. Thanks for adding the reference.

💬 maflcko commented on pull request "test: Slim down previous releases bdb check":
(https://github.com/bitcoin/bitcoin/pull/32350#discussion_r2063492156)
thx, may do if I have to re-touch.

💬 fanquake commented on issue "windows: usage of deprecated `std:wstring_convert`":
(https://github.com/bitcoin/bitcoin/issues/32361#issuecomment-2835002973)
> with use of the subprocess library soon

Note that `subprocess.h` is also using `wstring_convert`: https://github.com/bitcoin/bitcoin/blob/d62c2d82e14d27307d8790fd9d921b740b784668/src/util/subprocess.h#L1071.

💬 fanquake commented on pull request "build: Drop option to disable hardening.":
(https://github.com/bitcoin/bitcoin/pull/32071#issuecomment-2835045019)
Guix Build:
```bash
db95c26122204cf686ae4d0a7d6f2e2f2cd30d7ea0dce425f28bf4fa1af9fa12 guix-build-77e553ab6a0a/output/aarch64-linux-gnu/SHA256SUMS.part
3b5b3fd4b6ca8539e2ea5e50afdea37593f1f2e0c0b26851c20fc0505dd79a42 guix-build-77e553ab6a0a/output/aarch64-linux-gnu/bitcoin-77e553ab6a0a-aarch64-linux-gnu-debug.tar.gz
4c6b3a3949cc70e3f1a82fa38ed5c204c54074a79b267d67c02439279099c2ff guix-build-77e553ab6a0a/output/aarch64-linux-gnu/bitcoin-77e553ab6a0a-aarch64-linux-gnu.tar.gz
b9d2d30e3b5df7ad
...

💬 vasild commented on pull request "net: improve the interface around FindNode() and avoid a recursive mutex lock":
(https://github.com/bitcoin/bitcoin/pull/32326#issuecomment-2835048806)
> Is making `m_nodes_mutex` non-recursive a goal?

No, I did not think about that before you mentioned it. The goal here is to make the interface around `FindNode()` safer and simpler (don't return a possibly dangling pointer and don't return a pointer at all since callers can do with a boolean).

I did mention dropping the recursive lock because I think less recursive locking is good, even if the mutex remains recursive.

> I'm curious how far we are from a non-recursive `m_nodes_mutex`?
...

💬 maflcko commented on pull request "common: Close non-std fds before exec in RunCommandJSON":
(https://github.com/bitcoin/bitcoin/pull/32343#discussion_r2063545701)
style nit: fd's probably can't be negative nor include `+` as prefix? So `ToIntegral<uint64_t>` may be a better fit.