💬 AMoynahan89 commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2932168428)
Why is this pr still open?

"It's abundantly clear that this PR is controversial and, in its current state, has no hope of reaching a conclusion that is acceptable to everyone." -Achow101

https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1878853896

🤔 mabu44 reviewed a pull request: "policy: make pathological transactions packed with legacy sigops non-standard"
(https://github.com/bitcoin/bitcoin/pull/32521#pullrequestreview-2889699475)
ACK abd0749fae7259a292cb652fc9974c8cdcdd4d9d
Reviewed the code, compiled it, and ran the new tests. Additionally, made modifications to both the code and the tests to intentionally cause the tests to fail in expected ways.

💬 achow101 commented on pull request "wallet: `addhdkey` RPC to add just keys to wallets via new `unused(KEY)` descriptor":
(https://github.com/bitcoin/bitcoin/pull/29136#discussion_r2122002429)
Fixed

💬 achow101 commented on pull request "wallet: Track no-longer-spendable TXOs separately":
(https://github.com/bitcoin/bitcoin/pull/27865#discussion_r2122004809)
Fixed

👍 ryanofsky approved a pull request: "Add bitcoin-{node,gui} to release binaries for IPC"
(https://github.com/bitcoin/bitcoin/pull/31802#pullrequestreview-2889705787)
Code review ACK ed172d3002da68a3ddbd5d13e7d3f0618c1378d4, just rebased due to various conflicts since last review.

---

re: https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-2930369097

> We should treat the `libmultiprocess` subtree the same as all other subtrees and disable the `EXPORT_COMPILE_COMMANDS` property for _all_ its targets.

This could be reasonable, though I'm not very sure what the benefits of disabling linting here would be. Also, I think even with EXPORT_COMPIL
...

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