💬 l0rinc commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#issuecomment-3646374389)
[Since my last ACK](https://github.com/bitcoin/bitcoin/compare/389eafc631733c1ac2890e1b012a94f66a31ccad..07135290c1720a14c9d2f18a5700bb6565ae7a10): deduplicated and sorted the unit test cases, moved non-const return value in rpc `GetRawBlockChecked`, added default switch comment, renamed functional test lambda, and changed functional test category to be delimited by log instead of comment.

ACK 07135290c1720a14c9d2f18a5700bb6565ae7a10

📝 HowHsu opened a pull request: "test: add some txgraph tests"
(https://github.com/bitcoin/bitcoin/pull/34057)
Add tests for GetWorstMainChunk(), which picks the worst chunk from txgraph.

📝 billymcbip opened a pull request: "test: Improve code coverage for pubkey checks"
(https://github.com/bitcoin/bitcoin/pull/34058)
Cover these branches in `IsCompressedOrUncompressedPubKey` and `IsCompressedPubKey`:
- `Non-canonical public key: invalid length for uncompressed key`
- `Non-canonical public key: invalid length for compressed key`
- `Non-canonical public key: invalid prefix for compressed key`

See the missed branches here: https://maflcko.github.io/b-c-cov/total.coverage/src/script/interpreter.cpp.gcov.html

`script_tests` succeed on my end.

👍 hodlinator approved a pull request: "rpc: Distinguish between vsize and sigop adjusted mempool vsize"
(https://github.com/bitcoin/bitcoin/pull/32800#pullrequestreview-3571746826)
re-ACK ad07093b96c32f0e4e8427302f7d1352b12f1c76

Just resolved conflict with b0417ba94437d8bb23a7b66a3641ee8f3682a2dc in *doc/policy/README.md* (also removing double-newline at EOF) since my previous ACK (https://github.com/bitcoin/bitcoin/pull/32800#pullrequestreview-3390537459).

💬 fanquake commented on pull request "test: add some txgraph tests":
(https://github.com/bitcoin/bitcoin/pull/34057#issuecomment-3646410678)
cc @sipa

💬 brunoerg commented on pull request "test: add some txgraph tests":
(https://github.com/bitcoin/bitcoin/pull/34057#issuecomment-3646433224)
FWIW `txgraph` has a lot of unkilled mutants (some in GetWorstMainChunk) that could be addressed: https://corecheck.dev/mutation/src/txgraph.cpp

💬 fjahr commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#issuecomment-3646442588)
re-ACK 82be652e40ec7e1bea4b260ee804a92a3e05f496

Confirmed that changes since last review were addressing nits and rebasing.

🚀 fanquake merged a pull request: "rest: allow reading partial block data from storage"
(https://github.com/bitcoin/bitcoin/pull/33657)

💬 fanquake commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#issuecomment-3646475941)
https://github.com/bitcoin/bitcoin/actions/runs/20165856545/job/57889037572#step:8:1220:
```bash
/home/runner/work/bitcoin/bitcoin/src/test/chain_tests.cpp:87:30: error: use of undeclared identifier 'GetSkipHeight'
87 | int heightSkip = GetSkipHeight(heightWalk);
| ^
/home/runner/work/bitcoin/bitcoin/src/test/chain_tests.cpp:88:34: error: use of undeclared identifier 'GetSkipHeight'
88 | int heightSkipPrev = GetSkipHeight(he
...

🤔 rkrux reviewed a pull request: "wallet, doc: clarify the coin selection filters that enforce cluster count"
(https://github.com/bitcoin/bitcoin/pull/34037#pullrequestreview-3571803699)
Concept ACK dc5e20cf97f87fa9a37aeea76a480e412c49834c

Nice, this change comes at a right time. Recently, I got confused by `getTransaction
Ancestry()` in a previous PR: https://github.com/bitcoin/bitcoin/pull/33528#discussion_r2559474078 - not anymore.

I have limited idea of Cluster Mempool currently, thus I have based this review off of the `GetTransactionAncestry` implementation in `src/txmempool.h|cpp`.

💬 rkrux commented on pull request "wallet, doc: clarify the coin selection filters that enforce cluster count":
(https://github.com/bitcoin/bitcoin/pull/34037#discussion_r2614199422)
In dc5e20cf97f87fa9a37aeea76a480e412c49834c "[doc] coin selection filters by max cluster count, not descendant"

This portion seems suitable for the first commit - 9c299f6c4364bb7110daec16dd8e1feb36737e97 "[refactor] rename variable to clarify it is unused and cluster count".

💬 rkrux commented on pull request "wallet, doc: clarify the coin selection filters that enforce cluster count":
(https://github.com/bitcoin/bitcoin/pull/34037#discussion_r2614223412)
In dc5e20cf97f87fa9a37aeea76a480e412c49834c "[doc] coin selection filters by max cluster count, not descendant"

The following remaining places within `GroupOutputs` seems appropriate to be updated in this commit because `getTransactionAncestry` and `OutputGroup.Insert()` accept `cluster_count` now.

Based on the tone in the PR description, I'm not certain if it was intentional to skip these changes.

```diff
diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp
index 59c6ab116c..e2e2403a99
...

💬 HowHsu commented on pull request "test: add some txgraph tests":
(https://github.com/bitcoin/bitcoin/pull/34057#issuecomment-3646533593)
> FWIW `txgraph` has a lot of unkilled mutants (some in GetWorstMainChunk) that could be addressed: https://corecheck.dev/mutation/src/txgraph.cpp

Thanks, I'll take a look.

💬 hodlinator commented on issue "Stuck in Endless Pre-Syncing Headers Loop":
(https://github.com/bitcoin/bitcoin/issues/26391#issuecomment-3646548064)
Agree with https://github.com/bitcoin/bitcoin/issues/26391#issuecomment-3634191830 that only adding warnings feels unsatisfying. My initial suggestion was also to halt the node. But I now also agree with https://github.com/bitcoin/bitcoin/issues/26391#issuecomment-3634297696, we cannot make a definite classification between corruption and consensus forks.

The original issue at the top here had a log where the node was still syncing ancestor blocks to assumevalid. Within the same run: outputs we
...

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-3646563048)
As mentioned [on IRC](https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2025-12-11#1178515;) yesterday:
> We also observed that on the 16 GB system, runs with -dbcache values of 4 GB and higher were a lot slower than with -dbcache of 3 GB, and that an rpi5 with 16 GB of memory ran out of memory with -dbcache of 10 GB`.

My assumption was that it's caused by the UTXO size getting closer to the total memory, so I ran it on the i9 and i7 servers - both of which have 64 GB memory:

<img width=
...

🤔 ryanofsky reviewed a pull request: "multiprocess: Add capnp wrapper for Chain interface"
(https://github.com/bitcoin/bitcoin/pull/29409#pullrequestreview-3571732845)
Thanks for the close review, and sorry for the delay responding and updating this. You found a lot of annoying discrepencies and a real bug in the `estimateSmartFee` `best_height` return value, which should all be fixed now.

<!-- begin push-22 -->
Rebased d9efd1e49d1df154970b6a60229eedde3ba7cffe -> 89cf624fe811d90d43f22d4519d859716d3ea958 ([`pr/ipc-chain.21`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-chain.21) -> [`pr/ipc-chain.22`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-
...

💬 ryanofsky commented on pull request "multiprocess: Add capnp wrapper for Chain interface":
(https://github.com/bitcoin/bitcoin/pull/29409#discussion_r2614211276)
re: https://github.com/bitcoin/bitcoin/pull/29409#discussion_r2392340952

> I wonder how schema evolution is handled by Cap'n Proto - what happens if a field is deprecated and removed or new values are inserted, do I have to check all existing values to make sure I can find a new valid id?

Yes you do. You need to pick the next unused ID. I think it actually works pretty well in practice, especially if the id's are mostly in order, but it's also possible to include `# Next unused id: 42` com
...

💬 ryanofsky commented on pull request "multiprocess: Add capnp wrapper for Chain interface":
(https://github.com/bitcoin/bitcoin/pull/29409#discussion_r2614237694)
re: https://github.com/bitcoin/bitcoin/pull/29409#discussion_r2559741869

> should we make the param `UInt32` like we did one line below for `ChainstateRole`

Nice find noticing the inconsistency. Both types should be safe because casts between signed and unsigned types are well-defined. I tend to prefer signed types for enums so if the enums use -1, -2, -3 values they will be displayed nicely. But unsigned types could be better for some enums, particularly bitmasks. I updated chainstate rol
...

💬 ryanofsky commented on pull request "multiprocess: Add capnp wrapper for Chain interface":
(https://github.com/bitcoin/bitcoin/pull/29409#discussion_r2614160815)
re: https://github.com/bitcoin/bitcoin/pull/29409#discussion_r2392292721

> Could we add a simple explanation comment for these salts?

Not sure about this. Is there a comment you would suggest? I feel like ids are built into the syntax of Cap'n Proto and ubiquitous. If you aren't familiar with Cap'n Proto, I think a comment here would probably just distract you from more semantically relevant parts of the file and leave you confused. Commenting on IDs in capnproto seems like commenting on #
...

💬 ryanofsky commented on pull request "multiprocess: Add capnp wrapper for Chain interface":
(https://github.com/bitcoin/bitcoin/pull/29409#discussion_r2614228457)
re: https://github.com/bitcoin/bitcoin/pull/29409#discussion_r2559735548

> May be deliberate, but the parameters in [...] are 32 bit

Good catch. Updated these. Using a bigger type was harmless but not intentional. (It is a compiler error if you use a smaller type.)

💬 ryanofsky commented on pull request "multiprocess: Add capnp wrapper for Chain interface":
(https://github.com/bitcoin/bitcoin/pull/29409#discussion_r2614251707)
re: https://github.com/bitcoin/bitcoin/pull/29409#discussion_r2559791820

> same, should likely be

Thanks, updated. Agree signed type is a little nicer here (though both types should be safe)