💬 ryanofsky commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2614074730)

re: https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2603120305

> q in [cb3e7af](https://github.com/bitcoin/bitcoin/commit/cb3e7af4ed04295e9928c2b60d8ab4ba64c4efd5): I wonder if this should be fixed in a follow-up, because the comment in `AttachChain` says that the loop called when `hasAssumedValidChain` is true may be slow.

Yes it could be nice to change this and make loading old wallets on non-pruned nodes faster, even though it's a little bit of an edge case

💬 ryanofsky commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2614077388)
re: https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2605693370

> llm nit in the same commit: name args?

Thanks also added this

💬 maflcko commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2614119273)
> nit: Agree on reducing the number of direct conditions by using `SaturatingAdd` as sugggested at the beginning of the thread (passes current unit tests).
>


Let's leave the nits for a follow-up? The thread has more than 250 comments, and at least on my end, I am having difficulties loading, reading, and writing new comments, due to GitHub slowness or brittleness.

🤔 ryanofsky reviewed a pull request: "ipc: add bitcoin-mine test program"
(https://github.com/bitcoin/bitcoin/pull/30437#pullrequestreview-3063099001)
<!-- begin push-35 -->
Updated 2cb5e6472c8c0109390258c298915dd4011f0292 -> 057ea0a677293b6ecaa48008228ca3086ab9e0ff ([`pr/mine.34`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.34) -> [`pr/mine.35`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.35), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/mine.34..pr/mine.35))<!-- end --> just tweaking bitcoin-mine status message

💬 ryanofsky commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2236751665)
re: https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2230294157

+1. I think just having simple code to demonstrate the API with comments pointing out how usage could be more robust is a good middle ground.

💬 ryanofsky commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2236767287)
re: https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2229432505

Thanks, fixed

💬 ryanofsky commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2614116248)
re: https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2491098502

> In [1a844b8](https://github.com/bitcoin/bitcoin/commit/1a844b8ffd351d14b59de4cbae99b6e639068bb9) _bitcoin-mine: Extend example to mine a block_: just thought I got lucky on mainnet, but this does not distinguish between actually mining a block and a peer giving ups a new tip :-)

I guess technically the message didn't say who mined the block. But reworded it to avoid unnecessary excitement

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