🤔 w0xlt reviewed a pull request: "Split `CWallet::Create()` into `CreateNew` and `LoadExisting`"
(https://github.com/bitcoin/bitcoin/pull/32636#pullrequestreview-2879497779)
Approach ACK

💬 m3dwards commented on pull request "ci: remove 3rd party js from windows dll gha job":
(https://github.com/bitcoin/bitcoin/pull/32513#discussion_r2114687612)
Don't worry, I'm reworking it to do more manifest checks anyway so changes at this point are fine.

Will look at this.

💬 achow101 commented on pull request "test: test MAX_SCRIPT_SIZE for block validity":
(https://github.com/bitcoin/bitcoin/pull/32304#issuecomment-2920598430)
ACK b1ea542ae651cec433910d8c727abc41f17a7678

🚀 achow101 merged a pull request: "test: test MAX_SCRIPT_SIZE for block validity"
(https://github.com/bitcoin/bitcoin/pull/32304)

💬 ryanofsky commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#issuecomment-2920658650)
Rebased bda7626e987ce3d7287f72e6989dedde2c4ab666 -> d253fa9ebe4305118417a02aa72cc06810662ac6 ([`pr/libexec.5`](https://github.com/ryanofsky/bitcoin/commits/pr/libexec.5) -> [`pr/libexec.6`](https://github.com/ryanofsky/bitcoin/commits/pr/libexec.6), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/libexec.5-rebase..pr/libexec.6)) due to conflict with #28710

💬 achow101 commented on pull request "Split `CWallet::Create()` into `CreateNew` and `LoadExisting`":
(https://github.com/bitcoin/bitcoin/pull/32636#issuecomment-2920676506)
`PopulateWalletFromDB` inside of `CreateNew` should no longer be necessary.

It'd be nice if the load or create intention could be propagated down to the DB level as well. In `SQLiteDatabase::Open`, we pass the flags `SQLITE_OPEN_READWRITE | SQLITE_OPEN_CREATE` which does both load and create. I think ideally it would be just `SQLITE_OPEN_READWRITE` for loading, and `SQLITE_OPEN_CREATE` for creating to further enforce that we expect a file to already exist for loading, and for no files to exis
...

💬 achow101 commented on pull request "wallet: re-activate "AmountWithFeeExceedsBalance" error":
(https://github.com/bitcoin/bitcoin/pull/25269#discussion_r2114901397)
Since fees are not included when SFFO, I don't think this is reachable as should `available_balance` is always greater than `recipients_sum` in that case, so I don't think this needs any special SFFO treatment. Furthermore, if it were possible to reach this with SFFO, I think the error message would be confusing and it would actually be better to return "Insufficent Funds", so this whole check could probably be guarded by an `if (m_subtract_fee_outputs)` to skip it when SFFO is on.

💬 achow101 commented on pull request "wallet: migration, avoid creating spendable wallet from a watch-only legacy wallet":
(https://github.com/bitcoin/bitcoin/pull/31423#discussion_r2114913663)
In b78990734621b8fe46c68a6e7edaf1fbd2f7d351 "wallet: migration, avoid creating spendable wallet from a watch-only legacy wallet"

I think `empty_local_wallet` is unnecessary and the condition can check `local_wallet->GetAllScriptPubKeyMans().empty()` directly.

📝 theStack opened a pull request: "test: update BIP340 test vectors and implementation (variable-length messages)"
(https://github.com/bitcoin/bitcoin/pull/32642)
This PR updates the Schnorr signatures implementation in the functional test framework to the latest BIP changes (see https://github.com/bitcoin/bips/pull/1446,commit 200f9b26fe0a2f235a2af8b30c4be9f12f6bc9cb) and syncs the [test vectors](https://github.com/bitcoin/bips/blob/master/bip-0340/test-vectors.csv) accordingly. Practically, we probably don't need non-32-bytes message signing/verifying any time soon, but it seems good practice anyways to update anyways.

💬 davidgumberg commented on pull request "walletdb: Log additional exception error messages for corrupted wallets":
(https://github.com/bitcoin/bitcoin/pull/32598#issuecomment-2920938821)
ACK https://github.com/bitcoin/bitcoin/commit/ad9a13fc424e9deb262e2b1d54bcdc7370263ea0

💬 davidgumberg commented on pull request "deps: Bump lief to 0.16.6":
(https://github.com/bitcoin/bitcoin/pull/32431#issuecomment-2921038966)
- Rebased against master to include https://github.com/bitcoin/bitcoin/pull/32634/commits/dbb2d4c3d54759f8c346882b6f98d26d5bb8e816
- Bumped LIEF to 0.16.6 which resolves https://github.com/lief-project/LIEF/issues/1217
- Added a workaround for https://github.com/lief-project/LIEF/pull/1218, this is acceptable and won't need to be resolved once the upstream patch is merged, since this change tightens our MachO `has_nx` checks to both the stack and heap.
- Added a commit to enable a previously
...

👋 davidgumberg's pull request is ready for review: "deps: Bump lief to 0.16.6"
(https://github.com/bitcoin/bitcoin/pull/32431)

💬 Sjors commented on pull request "Update libmultiprocess subtree to fix clang-tidy errors":
(https://github.com/bitcoin/bitcoin/pull/32641#issuecomment-2921262265)
ACK 9f6565488fc169898abc5f7ee620ce7a9a4e5e50

Tested with `git-subtree-check.sh -r`

💬 Sjors commented on pull request "Add checkBlock() to Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31981#discussion_r2115179447)
At least we have a test that checks the UTXO set isn't updated: https://github.com/bitcoin/bitcoin/blob/1def3bc3773b3013a92f54daa75acb82c30beaac/test/functional/mining_template_verification.py#L145-L156

But that doesn't cover all conceivable future ways that we might accidentally flush.

If we do want to add a belt-and-suspenders way to prevent such a flush, it seems better for a followup.

> there could be better ways of preventing it like adding a read-only option to `CCoinsViewCache` t
...

💬 Sjors commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#issuecomment-2921299759)
Just #28710, no conflict with #31375? Will re-review shortly.

🚀 fanquake merged a pull request: "Update libmultiprocess subtree to fix clang-tidy errors"
(https://github.com/bitcoin/bitcoin/pull/32641)

⚠️ fanquake opened an issue: "fs: use `ftruncate` on OpenBSD in `AllocateFileRange`"
(https://github.com/bitcoin/bitcoin/issues/32643)
OpenBSD doesn't have `fallocate` or `posix_fallocate`, which means it's hitting our fallback. We should instead have it use `ftruncate`. See related discussion in #32460.

cc @theStack.

💬 fanquake commented on pull request "fs: remove `_POSIX_C_SOURCE` defining":
(https://github.com/bitcoin/bitcoin/pull/32460#issuecomment-2921749068)
Opened #32643 to followup.

🚀 fanquake merged a pull request: "fs: remove `_POSIX_C_SOURCE` defining"
(https://github.com/bitcoin/bitcoin/pull/32460)

📝 fanquake converted_to_draft a pull request: "threading: remove ancient CRITICAL_SECTION macros"
(https://github.com/bitcoin/bitcoin/pull/32592)
Now that #32467 is merged, the only remaining usage of our old `CRITICAL_SECTION` macros (other than tests) is in `getblocktemplate()` and it can safely be replaced with a `REVERSE_LOCK`.

This PR makes that replacement, replaces the old `CRITICAL_SECTION` macro usage in tests, then deletes the macros themselves.

While testing this a few weeks ago, I noticed that `REVERSE_LOCK` does not currently work properly with our thread-safety annotations as after the `REVERSE_LOCK` is acquired, clang
...