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

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

> super-nit: I assume the copyright headers are similar to other code in the repo:

Nice catch, fixed this up

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

> The original param name is `delta_seconds`

Thanks renamed to deltaSeconds for consistency. In general parameter names don't matter very much but it is good to be consistent.

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

> Is this an accurate mapping of `std::optional<UniValue>`

Yes, it is. The UniValue fields is serialized as JSON text here. And Cap'n Proto allows text fields to be null so `std::nullopt` value is just represented by an null text field.

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

> we might want to ment why this one doesn't have a corresponding proxy:

Good suggestion, added a version of this comment

📝 maflcko opened a pull request: " refactor: Use NodeClock::time_point for m_addr_token_timestamp "
(https://github.com/bitcoin/bitcoin/pull/34059)
It is a bit confusing to have some code use the deprecated `GetTime`, which returns a duration and not a time point, and other code to use `NodeClock` time points.

Fix one place `m_addr_token_timestamp` to use `NodeClock::time_point`.

Also:

* Extract a `ProcessAddrs` helper, similar to the other `Process*()` helpers, to cut down the `ProcessMessage` with a massive scope.
* Rename the confusing `current_a_time` to `now_seconds`. (The `a` in this context refers to the removed "adjusted"
...