💬 fanquake commented on pull request "[POC] build: Use clang-cl to build on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31507#discussion_r1990498934)
In 76659d92f1925f561736ffe0f070ed0b9fef8d23: It'd be good if commits like this could contain actual details, rather than just "Adjust diagnostic flags"; especially given this is touching non-Windows code. Also, any reason to not fix the issues in leveldb, rather than adding more suppressions?

💬 fanquake commented on pull request "qa: Enable feature_init.py on Windows":
(https://github.com/bitcoin/bitcoin/pull/32021#discussion_r1990503238)
No. `CTRL_BREAK_EVENT` only exists on Windows: https://docs.python.org/3/library/signal.html#signal.CTRL_BREAK_EVENT.

💬 vasild commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#issuecomment-2716480960)
`4492abbf0a...8c0ce1ca1c`: address https://github.com/bitcoin/bitcoin/pull/32015#discussion_r1989217778

💬 vasild commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#discussion_r1990577831)
Removed the declaration as well, thanks!

💬 aiswaryasankar commented on pull request "validation, fix: Use wtxid instead of txid in `CheckEphemeralSpends`":
(https://github.com/bitcoin/bitcoin/pull/32025#discussion_r1990699940)
The change from `Txid` to `Wtxid` in `CheckEphemeralSpends` calls could cause issues with transaction identification. The function now stores witness transaction IDs instead of regular transaction IDs, which might affect how transactions are tracked and identified in error cases.

📝 kevincatty opened a pull request: "chore: remove redundant words in comment"
(https://github.com/bitcoin/bitcoin/pull/32037)
remove redundant words in comment

✅ fanquake closed a pull request: "chore: remove redundant words in comment"
(https://github.com/bitcoin/bitcoin/pull/32037)

💬 maflcko commented on pull request "Require sqlite when building the wallet":
(https://github.com/bitcoin/bitcoin/pull/31961#issuecomment-2716994263)
No strong opinion about depends (follow-up or here), otherwise:

review ACK ac6cde731314d981391bc313c98d671c68211d33 🐥

<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+krx
...

💬 maflcko commented on pull request "Require sqlite when building the wallet":
(https://github.com/bitcoin/bitcoin/pull/31961#discussion_r1990912432)
In any case, this is unrelated to the changes here. If this is done, it would have to be done for all python packages like py-zmq, not only py-sqlite, in a separate change.

💬 fanquake commented on pull request "depends: patch around PlacementNew issue in capnp":
(https://github.com/bitcoin/bitcoin/pull/31998#discussion_r1990914356)
Thanks, I've adjusted this to use `-p2` (should have just done this in the first place rather than adjusting the paths).

💬 maflcko commented on pull request "tests: Improve stderr validation in test_runner.py":
(https://github.com/bitcoin/bitcoin/pull/31966#issuecomment-2717005621)
Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

💬 fanquake commented on pull request "cmake: Check for `makensis` and `zip` tools before using them for optional `deploy` targets":
(https://github.com/bitcoin/bitcoin/pull/32019#discussion_r1990943885)
We also don't do this for other similar targets, like `docs`. Where we just print `"Error: Doxygen not found"`.

💬 maflcko commented on pull request "qt: 29.0 translations update":
(https://github.com/bitcoin/bitcoin/pull/32004#issuecomment-2717053653)
> I'd like to reiterate my point that Transifex is poorly suited for Bitcoin Core translations, and I'm keen to use GitHub to track translation changes (as was done many years ago). This seems doable in the context of the recently discussed separation of the GUI repository.

The gui repository is already a separate monotree in https://github.com/bitcoin-core/gui, so it should be possible to drop transifex already today, if translators and translation reviewers prefer to use GitHub. Maybe I am
...

👍 dergoegge approved a pull request: "validation, fix: Use wtxid instead of txid in `CheckEphemeralSpends`"
(https://github.com/bitcoin/bitcoin/pull/32025#pullrequestreview-2677593378)
Code review ACK e637dc2c01c3b566e6c51c911c5881a8d206c924

💬 dergoegge commented on pull request "cmake: Avoid fuzzer "multiple definition of `main'" errors":
(https://github.com/bitcoin/bitcoin/pull/31992#issuecomment-2717130966)
Why do we want IPC enabled for the fuzzing builds? It's not used/tested by our fuzzing harnesses.

💬 laanwj commented on issue "guix: re-enable exported symbol checking for RISC-V":
(https://github.com/bitcoin/bitcoin/issues/28095#issuecomment-2717186516)
Filed an upstream issue for binutils: https://sourceware.org/bugzilla/show_bug.cgi?id=32783

💬 Sjors commented on pull request "Require sqlite when building the wallet":
(https://github.com/bitcoin/bitcoin/pull/31961#issuecomment-2717203847)
I initially considered dropping the `NO_WALLET` option from depends. Most other depends packages refer to the library name rather than the feature, so this seemed better than dropping `NO_SQLITE`.

However it could break automations (and habits) of anyone who tries to build the node without wallet support. So I ended up dropping `NO_SQLITE` and keeping `NO_WALLET`.

Also rebased after #31161 just in case.

💬 Sjors commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#issuecomment-2717384152)
This seems like a nice simplification. It would be good to do before nobody remembers what the old code was trying to do (and prevent). Since @theUni tried something similar before in #10738 it would be great if he can look at this again.

In particular the 2017 PR claimed "special care must be taken" as to which thread destroys the object, but @vasild believes this doesn't matter (here): https://github.com/bitcoin/bitcoin/pull/32015#discussion_r1984875189

The end result of that PR seems a
...

💬 Sjors commented on pull request "Update `secp256k1` subtree to latest master":
(https://github.com/bitcoin/bitcoin/pull/32028#issuecomment-2717395528)
Concept ACK on (temporarily) using master instead of a release tag. However we should probably wait with merging this until #31507 has at least one code review ACK from a build system expert?

ACK 7803eb4a51f3a7a7e76b4f45fb211ffa87a7e9bd

Ran the `git-subtree-check.sh` check. I didn't review the upstream changes.

On (ARM) macOS 15.3.1 I ran a testnet4 IBD with a debug build.

🤔 rkrux reviewed a pull request: "Require sqlite when building the wallet"
(https://github.com/bitcoin/bitcoin/pull/31961#pullrequestreview-2677975455)
Concept ACK 47e5ed3a5ba21049a9a8f3c4c44023e82e6ab88e

I've checked that there are no more instances of the following make options present and `WITH_SQLITE` doesn't appear anymore in the output of `ccmake build`.

```
➜ bitcoin git:(2025/02/sqlite) ✗ git grep "USE_SQLITE"
➜ bitcoin git:(2025/02/sqlite) ✗ git grep "WITH_SQLITE"
➜ bitcoin git:(2025/02/sqlite) ✗ git grep "NO_SQLITE"
```

Since the PR is rebased after the #31161 changes, I rm-rf'ed my build directory before configuring
...