txdb: reject undecodable first coin cursor key #35654

Problem: CCoinsViewDB::Cursor() already handles the case where Seek(DB_COIN) finds no coin record, but could still accept an undecodable first coin key as a valid cursor position. CCoinsViewDBCursor::Next() already invalidates the cursor for the same decode failure later in iteration, so initial cursor setup has a small correctness mismatch.

Fix: Make initial cursor setup match Next(): keep the cached key invalid unless the first key decodes successfully.

Fixes #35172, with credit to the issue author and previous attempts #35191 and #35248.

<!--e57a25ab6845829454e8d69fc972939a-->

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

<!--006a51241073e994b41acfe9ec718e94-->

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35654.

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline and AI policy for information on the review process.

If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #34320 (coins: delegate CCoinsViewDB::HaveCoin to GetCoin by l0rinc)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

<!--5faf32d7da4f0f540f40219e4f7537a3-->

Quoting myself from the issue: If the chainstate is corrupted to the point that the condition described here is reached, I'm not sure if the check introduced here actually improves the situation for the running node. Commit https://github.com/bitcoin/bitcoin/commit/822755a424d0abfa408dc34313f4aca4b816f54f also explicitly works around the "no valid first entry" scenario.

The reasons stated here (crc collisions, and fork tools) also don't seem very compelling to me. If this is really something worthwhile, maybe just frame it as a correctness improvement and getting rid of a weird albeit harmless edge case?

maybe just frame it as a correctness improvement and getting rid of a weird albeit harmless edge case

Adjusted the code comment, commit message, PR description and title - let me know if this is what you meant

let me know if this is what you meant

Yes, that is what I meant. I'm still not sure if the change is worthwhile. This was repeatedly flagged and picked up by AI agents. I get the impression that this is more busywork than anything, but at this point it will just be picked up again and again, which seems like even more of a time sink, so I'd rather this change get in.

ACK 3837d9192a719b2232a2e8f330dac347bf09742d

Following up on the "is this worthwhile" question above traced through the typical consumer pattern (while (cursor->Valid()) { ...; cursor->Next(); }).

Before this fix: a corrupted first record means one garbage entry gets processed, but Next() still runs afterward, so the rest of a valid UTXO set is still visited correctly.

After this fix: Valid() is false immediately, so Next() is never called and the loop exits before visiting any of the (potentially many) valid coins after the bad one trading "one garbage entry mixed into an otherwise- complete result" for "reports entirely empty despite valid data existing."

Given the project's general preference for failing loudly on corrupted state (e.g. the -reindex guidance elsewhere in this file) rather than silently continuing, an obviously-empty result may still be preferable to a subtly- wrong one. But it might be worth an explicit LogError here so operators aren't left guessing why a tool suddenly reports nothing happy to be corrected if I am missing something about how these call sites actually handle an empty cursor.