💬 hodlinator commented on pull request "contrib: Make deterministic-coverage error messages more readable":
(https://github.com/bitcoin/bitcoin/pull/32074#discussion_r1999832708)
nit: Unrelated to this change, but would be nice to avoid "shadowing".
```suggestion
let corpora_dir = Path::new(args.get(2).ok_or(exit_help("Must set fuzz corpora dir"))?);
```

💬 hodlinator commented on pull request "contrib: Make deterministic-coverage error messages more readable":
(https://github.com/bitcoin/bitcoin/pull/32074#discussion_r2001916136)
Unrelated, but while looking for faults with this PR and trying to trigger errors, I got distracted into making this run in parallel. Earlier version triggered OOM-killer and this one is still in a super-unbaked state. (Output should be buffered and sequenced, parallization logic should be refined, file naming uniqueness-approach is bad, etc).

<details><summary>diff</summary>

```diff
diff --git a/contrib/devtools/deterministic-fuzz-coverage/src/main.rs b/contrib/devtools/deterministic-fuz
...

💬 hodlinator commented on pull request "contrib: Make deterministic-coverage error messages more readable":
(https://github.com/bitcoin/bitcoin/pull/32074#discussion_r2001921947)
This is slightly boilerplate-y but it's a price I'm willing to pay for the rest of the diff.

Might be less annoying to have at the bottom of the file so it's out of the way?

💬 hodlinator commented on pull request "contrib: Make deterministic-coverage error messages more readable":
(https://github.com/bitcoin/bitcoin/pull/32074#discussion_r1999707442)
info: Would have expected:
```suggestion
_ => return Err(exit_help(&format!("The tool {} is not installed", tool))),
```
`?` feels like returning is an open question, but after `Err()` it's always true... maybe this is still idiomatic though. `?` in other places implies a *possible* `return`, so probably just takes some getting used to.

💬 hodlinator commented on pull request "contrib: Make deterministic-coverage error messages more readable":
(https://github.com/bitcoin/bitcoin/pull/32074#discussion_r2001944320)
Tempting to suggest
```suggestion
struct AppError(String);
```
But it does add a fair amount of noise where it's instantiated, so I understand we might want to go a bit less type-safe for these tools.

🤔 fjahr reviewed a pull request: "qa: fix an off-by-one in utxo snapshot fuzz target and sanity check its snapshot data"
(https://github.com/bitcoin/bitcoin/pull/31910#pullrequestreview-2695499527)
tACK 63b534f97e591d4e107fd5148909852eb2965d27

Reviewed the code and played around with the fuzzer changes a bit.

💬 fjahr commented on pull request "qa: fix an off-by-one in utxo snapshot fuzz target and sanity check its snapshot data":
(https://github.com/bitcoin/bitcoin/pull/31910#discussion_r2001560332)
Hm, this now has the exact same text here as `MineBlock` above and in general it's doesn't explain what happens internally. I think there should be something here or above that makes clear what the difference is. Maybe add something like "Expects the block submitted to have valid PoW." or so.

🤔 ryanofsky reviewed a pull request: "refactor: Improve assumeutxo state representation"
(https://github.com/bitcoin/bitcoin/pull/30214#pullrequestreview-2691157663)
Rebased d09b82156ced5d543d08226e8d7b8fda2e0ec532 -> 467528960689c2913c101ef75bc833e8f04bd0f3 ([`pr/cstate.9`](https://github.com/ryanofsky/bitcoin/commits/pr/cstate.9) -> [`pr/cstate.10`](https://github.com/ryanofsky/bitcoin/commits/pr/cstate.10), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/cstate.9-rebase..pr/cstate.10)) due to conflict 31961 and implementing various suggestions.

re: https://github.com/bitcoin/bitcoin/pull/30214#pullrequestreview-2685423528

Thanks for the re
...

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

> I think this `SnapshotcompletionResult` variant can be removed, no?

Good catch, removed.

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

> Nit: Are we still doing `-present` now? I thought I saw some discussion about that recently, but can't find it anymore.

The change to "present" never made sense to me because I think the only thing "present" could mean is "when this line was written", not what we might want it to mean like "whenever the code here was last modified" or "whenever the code here was published."

Since "present" just seems like a more
...

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

> In commit [9853c83](https://github.com/bitcoin/bitcoin/commit/9853c8384e1df9ad14c2801e780cc189ac20c6d0): Is the validity check required? AFAICT the historic chainstate is never invalid.

That's true. This anticipates a later change but there is no reason to add that complexity here. Dropped this condition.

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

> Nit: `s/if is/if it is/`. I think there are some missing articles around here too.

Thanks, fixed up various comments in this header.

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

> In commit [22bfd94](https://github.com/bitcoin/bitcoin/commit/22bfd948f0d45083748724a304fa2c8482464028): Just a note: The removal of this particular check in this commit seems a bit random with all the following kind of redundant assertions, but the code does get cleaned up in later commits.

We might be noticing different things here so let me know if you have suggestions for improving this, but I don't think this c
...

💬 ryanofsky commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2001594456)
In commit "refactor: Add ChainstateManager::ActivateBestChains() method" (58265b955a3d8622f22958943f79409129d58b36)

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

> Shouldn't this skip over the historical chainstate if `m_target_utxohash` is set?

Yes good catch. Updated this to be equivalent.

💬 ryanofsky commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2001593749)
In commit "refactor: Add ChainstateManager::m_chainstates member" (5e22fdcdb2665c0bb5e47c53719796096afe2dd1)

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

> I don't quite follow this change. What is wrong with using `CurrentChainstate` here?

IMO chainstatemanager should generally just validate and update chainstates with incoming data and not rely on concept of a "current" chainstate. The concept of a current chainstate is a little vague and makes more sense a
...

💬 ryanofsky commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2001594940)
In commit "refactor: Delete ChainstateManager::GetAll() method" (ca48478fbfcc83cc6f8c928e57800707fe0c3b51)

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

> I'm surprised this is not just taking the `m_chainstates` size. Why is that?

Wrote a better comment since existing comment wasn't really explaining what was happening here. It's possible this code might need to be updated to customize pruning behavior if more chainstates are added in the future. But I think
...

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

> Nit: `s/this a/this is a/`

Thanks, fixed

💬 darosior commented on pull request "qa: fix an off-by-one in utxo snapshot fuzz target and sanity check its snapshot data":
(https://github.com/bitcoin/bitcoin/pull/31910#discussion_r2002057033)
I figured its name already implies what it's doing and expecting (an entirely valid block, not only valid PoW), and didn't want to just add a comment like "Process a block" (akin to `PrepareBlock` right below whose docstring states "Prepare a block"). I felt only the return value was not clear from the function declaration, hence why specifying it in the docstring.

💬 mzumsande commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2002112881)
That's a good idea! I think we can even go one step further and use `highpow_outofchain_headers` for everything, also for `cand_invalid_descendants` (at the cost of doing a few `GetAncestor()` calls, which should be cheap enough). I pushed an updated version that uses this approach.

💬 mzumsande commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#issuecomment-2734888712)
[4ba2e48](https://github.com/bitcoin/bitcoin/commit/4ba2e480ffa0b77113953bee4ff5c9349e277e7e) to [fd39a64](https://github.com/bitcoin/bitcoin/commit/fd39a6420dad4cfc8bfff73f32df663da1ec00e2):
I changed the approach to `InvalidateBlock()` after the suggestion by @stickies-v: Instead of having 3 slightly different caches, now only one cache is used for everything (`highpow_outofchain_headers`). This should make the changes more simple and have a negligible overhead of a few additional `GetAncesto
...