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

No strong opinion for/against disallowing 0-size ranges.

💬 hodlinator commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2614038938)
(meganit: Would add the comment in the first commit instead of the last if you re-touch).

🤔 ryanofsky reviewed a pull request: "refactor: Improve assumeutxo state representation"
(https://github.com/bitcoin/bitcoin/pull/30214#pullrequestreview-3571658129)
Thanks for the reviews!

<!-- begin push-24 -->
Rebased 5dd7cbf675e7ce058ccb0666dabcb39a175ae83c -> 82be652e40ec7e1bea4b260ee804a92a3e05f496 ([`pr/cstate.23`](https://github.com/ryanofsky/bitcoin/commits/pr/cstate.23) -> [`pr/cstate.24`](https://github.com/ryanofsky/bitcoin/commits/pr/cstate.24), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/cstate.23-rebase..pr/cstate.24))<!-- end --> fixing conflict #32414 and also adding suggested arg name comments

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

> llm nit in [6c98ddc](https://github.com/bitcoin/bitcoin/commit/6c98ddc87a895d9bbc392db8fedc0e8268ff1cdd): could use named args while touching?

Makes sense, added.

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

> i'd say assumes are fine to detect logic bugs or storage corruption, but no strong opinion, if the goal is to somehow make this more flexible for future changes

Yeah I think it needs to be up to callers to check their own expected states or to check the return status of this function. Having this function make unnecessary assumptions about particular states it thinks it will be called in seems like it could be easil
...

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