💬 ryanofsky commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2122005059)
In commit "build: depends makes libmultiprocess by default" (adc1bcf784ae9763246f1f143ebc225214bf1730)

Not important, but might be nice to replace `multiprocess_packages` with `ipc_packages` to be more consistent.

💬 bigspider commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-2932221955)
> @bigspider Ledger Live says my LedgerX device is up to date. The mainnet Bitcoin app version is 2.4.0, but the testnet is at 2.3.0.
>
> Or do I need the Bitcoin Test Legacy app? That one has a version 2.4.7, but it throws "ClaNotSupported" if I try it with [async-hwi](https://github.com/wizardsardine/async-hwi/tree/master/cli) `device list` command.

No, "Bitcoin Test" version 2.4.0 is the correct app; maybe something went wrong and its deployment is missing - I'll check tomorrow. What is
...

💬 achow101 commented on pull request "wallet, rpc: Return normalized descriptor in parent_descs":
(https://github.com/bitcoin/bitcoin/pull/32594#discussion_r2122024645)
Good point, dropped the apostrophe check.

💬 achow101 commented on pull request "wallet, rpc: Return normalized descriptor in parent_descs":
(https://github.com/bitcoin/bitcoin/pull/32594#discussion_r2122025089)
Taken suggestion, with some modifications.

💬 ryanofsky commented on pull request "wallet: Translate [default wallet] string in progress messages":
(https://github.com/bitcoin/bitcoin/pull/31296#issuecomment-2932279122)
Rebased a46ec1dece8d8a1b16ff1fc3d2932bca130bf67f -> db225cea56b0531cc42d4b89dc61b02890f432ff ([`pr/dtran.3`](https://github.com/ryanofsky/bitcoin/commits/pr/dtran.3) -> [`pr/dtran.4`](https://github.com/ryanofsky/bitcoin/commits/pr/dtran.4), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/dtran.3-rebase..pr/dtran.4)) due to conflict with #28710

💬 achow101 commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#issuecomment-2932328786)
> According to BIP 390 [_"Repeated participant public keys are not allowed."_](https://github.com/bitcoin/bips/blob/72af87fc72999e3f0a26a06e6e0a7f3134236337/bip-0390.mediawiki?plain=1#L36), but it seems there is currently no check preventing that? Example test case that passes but shouldn't (IIUC): [theStack@f260c7e](https://github.com/theStack/bitcoin/commit/f260c7ec06e8bbb7148877a9a9b7e96707c41fa1)

Good catch. Fixed and added a test.

👍 hebasto approved a pull request: "depends: Bump boost to 1.88.0 and use new CMake buildsystem"
(https://github.com/bitcoin/bitcoin/pull/32665#pullrequestreview-2889828316)
My Guix build:
```
aarch64
32f567efe7c9d428083d9eab15e9fb215e2860132b3a31a60f288d21d3ed3644 guix-build-3a350c8a1b51/output/aarch64-linux-gnu/SHA256SUMS.part
13bfeec126cef80d97a7b90f9cec32962cce8b3bbf445006f1233e1b45a74d43 guix-build-3a350c8a1b51/output/aarch64-linux-gnu/bitcoin-3a350c8a1b51-aarch64-linux-gnu-debug.tar.gz
22b967a5fdcc39fb999e9e30ac74b18509048f8918ddf08ab8a1de47bd587565 guix-build-3a350c8a1b51/output/aarch64-linux-gnu/bitcoin-3a350c8a1b51-aarch64-linux-gnu.tar.gz
3ce04680
...

💬 achow101 commented on pull request "util: Abort on failing CHECK_NONFATAL in debug builds":
(https://github.com/bitcoin/bitcoin/pull/32588#issuecomment-2932373695)
Concept ACK

🤔 hodlinator reviewed a pull request: "util: explicitly close all AutoFiles that have been written"
(https://github.com/bitcoin/bitcoin/pull/29307#pullrequestreview-2861079249)
Code review c7b68dceb29ef67796f7edf79a161bbda766169c

This is a clear incremental improvement over what is on master.

Thanks for taking my suggestions!

Don't foresee any further issues preventing an A-C-K beyond the inline comments below.

💬 hodlinator commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2122083385)
Not sure how to handle this in the callers, probably something along these lines:
```diff
diff --git a/src/index/blockfilterindex.cpp b/src/index/blockfilterindex.cpp
index 6b47a0fa0a..332e700995 100644
--- a/src/index/blockfilterindex.cpp
+++ b/src/index/blockfilterindex.cpp
@@ -227,10 +227,13 @@ size_t BlockFilterIndex::WriteFilterToDisk(FlatFilePos& pos, const BlockFilter&

// Pre-allocate sufficient space for filter data.
bool out_of_space;
- m_filter_fileseq->Allocat
...

💬 hodlinator commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2110214040)
In this specific case of mempool_persist.cpp/`DumpMempool` my variant simply had:
```C++
FileWriter file{mockable_fopen_function(file_fspath, "wb"), [] (int err) {
Assume(std::uncaught_exceptions() > 0); // Only expected when exception is thrown before fclose() below.
}};
```
Which would serve to enforce that we either already explicitly `fclose()`d the file and checked `errno` further down in the function (leading to the lambda not being called upon destruction), or that a
...

💬 hodlinator commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2102466670)
Probably safer to check against `0` in this file as you've done elsewhere in this PR:
```suggestion
if (fclose(file) != 0) {
```
(Seems like we can be more certain of the value of `0`/success than `EOF`/failure:
https://www.man7.org/linux/man-pages/man3/fclose.3.html
https://en.cppreference.com/w/c/io/fclose)

🚀 achow101 merged a pull request: "wallet: init, don't error out when loading legacy wallets"
(https://github.com/bitcoin/bitcoin/pull/32449)

💬 achow101 commented on pull request "rpc: Use type-safe HelpException":
(https://github.com/bitcoin/bitcoin/pull/32660#issuecomment-2932409311)
It doesn't seem like this is catching the issue in https://github.com/bitcoin/bitcoin/pull/29136#discussion_r2120666028? I tried making a similar change to a RPC in this PR and the test still passed.

💬 hebasto commented on pull request "depends: Bump boost to 1.88.0 and use new CMake buildsystem":
(https://github.com/bitcoin/bitcoin/pull/32665#discussion_r2122112935)
The latter.

👍 ryanofsky approved a pull request: "util: Abort on failing CHECK_NONFATAL in debug builds"
(https://github.com/bitcoin/bitcoin/pull/32588#pullrequestreview-2889836067)
Code review ACK faae9a294798c6808ec82f827385d920f8d697cf. I think this is a good change. It makes sense conceptually to have check macros that always abort in debug builds, but do different things depending on cost of the check & severity of the error in release builds.

re: https://github.com/bitcoin/bitcoin/pull/32588#issuecomment-2903414680

> I don't think this solves issue (1). Instead of NONFATAL being inaccurately named in debug builds, it will now be THROW, because the check is neith
...

💬 ryanofsky commented on pull request "util: Abort on failing CHECK_NONFATAL in debug builds":
(https://github.com/bitcoin/bitcoin/pull/32588#discussion_r2122090155)
In commit "util: Abort on failing CHECK_NONFATAL in debug builds" (fae8c02268c11f2ad6165b6437b3e4846babfaf5)

IMO, it would be better not to drop this test coverage so that we can verify that the RPC code catching the
`NonFatalCheckError` exception and turning into a JSON response works.

The commit message mentions Assume and Assert don't have test coverage either, but I don't see why they shouldn't have it.

It seems like it would be pretty easy to keep the test coverage here either by
...

💬 achow101 commented on pull request "wallet: add codex32 argument to addhdkey":
(https://github.com/bitcoin/bitcoin/pull/32652#issuecomment-2932435166)
Concept ACK-ish

This is certainly better than the previous approach.

💬 enirox001 commented on pull request "contrib: Add external RPC code generator with unit tests":
(https://github.com/bitcoin/bitcoin/pull/32140#issuecomment-2932446848)
> Are you still working on this, or can it be closed?

No, not working on this at the moment, can be closed. Will probably look into it in the future, but not at the moment.

💬 willcl-ark commented on pull request "depends: Bump boost to 1.88.0 and use new CMake buildsystem":
(https://github.com/bitcoin/bitcoin/pull/32665#issuecomment-2932452212)
Would we not want to bump the boost version in [AddBoostIfNeeded.cmake](https://github.com/bitcoin/bitcoin/blob/f999c3775c12ef732a1920dd52257a8cd34cdcc8/cmake/module/AddBoostIfNeeded.cmake#L29) too?