txdb: handle malformed first coin cursor key #35191

pull ArtSabintsev wants to merge 1 commits into bitcoin:master from ArtSabintsev:codex/fix-txdb-cursor-malformed-key changing 2 files +23 −5

ArtSabintsev commented at 1:47 AM on May 1, 2026: none
Fixes #35172.

I am still getting familiar with the codebase, so doing easy tickets for now. Thanks for bearing with me.

What?

CCoinsViewDB::Cursor() caches the first coin key after seeking into the chainstate DB. If the first DB_COIN entry is malformed, CDBIterator::GetKey() returns false, but the cursor warmup path ignored that return value.

Because CoinEntry defaults its key marker to DB_COIN, a malformed key like just C could leave the cursor reporting Valid() == true and GetKey() == true even though the key was not decoded.

This makes the warmup path match CCoinsViewDBCursor::Next(): if the iterator is invalid or the key cannot be decoded, invalidate the cached key.

Scenario

This would require an already-corrupted or manually modified local chainstate LevelDB where the first coin key is readable by LevelDB but not decodable as a CoinEntry.

Normal node operation should not create this key shape, so this is a correctness/hardening fix rather than a common user-facing issue.

Testing

The regression test writes a single-byte C key, which enters the DB_COIN keyspace but is too short to decode as a CoinEntry. It then reopens the DB through CCoinsViewDB and checks that the returned cursor is invalid.
```
cmake --build build --target test_bitcoin -j8
build/bin/test_bitcoin --run_test=coins_tests_dbbase/malformed_first_coin_key_cursor_invalid
build/bin/test_bitcoin --run_test=coins_tests_dbbase
```
DrahtBot added the label UTXO Db and Indexes on May 1, 2026
DrahtBot commented at 1:47 AM on May 1, 2026: contributor



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



Code Coverage & Benchmarks

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



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK junbyjun1238

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.
ArtSabintsev force-pushed on May 1, 2026
txdb: handle malformed first coin cursor key c02c4aea2a
ArtSabintsev force-pushed on May 1, 2026
ArtSabintsev marked this as ready for review on May 1, 2026
junbyjun1238 commented at 6:19 AM on May 1, 2026: none

utACK c02c4aea2a0c126f9defb255b3da695e2ee619bc

Reviewed the diff. This makes the initial CCoinsViewDB::Cursor() warmup consistent with CCoinsViewDBCursor::Next() by invalidating the cached key when the first DB_COIN key cannot be decoded, instead of exposing the default DB_COIN marker as a valid cursor entry. The regression test covers the malformed first-key case. I did not run the tests.

Type	Reviewers
ACK	junbyjun1238

in src/txdb.cpp:210 in c02c4aea2a

 206 | @@ -207,12 +207,11 @@ std::unique_ptr<CCoinsViewCursor> CCoinsViewDB::Cursor() const
 207 |         that restriction.  */
 208 |      i->pcursor->Seek(DB_COIN);
 209 |      // Cache key of first record
 210 | -    if (i->pcursor->Valid()) {
 211 | -        CoinEntry entry(&i->keyTmp.second);
 212 | -        i->pcursor->GetKey(entry);
 213 | -        i->keyTmp.first = entry.key;
 214 | -    } else {
 215 | +    CoinEntry entry(&i->keyTmp.second);

optout21 commented at 2:14 PM on May 5, 2026:

A slight change in behavior is that the entry is created even if the cursor is invalid. Other versions where this is not the case:

    if (i->pcursor->Valid() && (CoinEntry entry(&i->keyTmp.second); i->pcursor->GetKey(entry))) {
        i->keyTmp.first = entry.key;
    } else {
        i->keyTmp.first = 0; // Make sure Valid() and GetKey() return false
    }

    if (i->pcursor->Valid()) {
        CoinEntry entry(&i->keyTmp.second);
        if (i->pcursor->GetKey(entry)) {
            i->keyTmp.first = entry.key;
        } else {
            i->keyTmp.first = 0; // Make sure Valid() and GetKey() return false
        }
    } else {
        i->keyTmp.first = 0; // Make sure Valid() and GetKey() return false
    }

The latter is more verbose, but more readable and less error prone for future changes, does not relay on early exit conditional evaluation, and is closest to the original. Please comment, consider. (Note: untested code.)

achow101 commented at 10:24 AM on May 7, 2026: member

Thanks, but LLM output is not accepted if the author can not properly explain, test or could have written the change themselves.

The bottleneck in this project has always been review and testing, not writing code. Development here is intentionally conservative and slow, and reviewer attention is the scarcest resource we have. LLMs have made this worse, anyone can now prompt them and post their output as PRs. There is an infinite amount plausible looking "improvements" for LLMs to suggest and work on.

Unless we fully trust LLMs to both write and review code, humans still have to spend time understanding the proposed changes, which incurs a non-zero cost for every opened PR.

I understand that contributing to this project can be intimidating, and using LLMs may seem tempting, but it really creates more issues for this project than it solves. The best way to help this project, is to review and test changes. You can use LLMs for this, but you shouldn't solely rely on them, or just post their output.

Please reconsider whether it's something you genuinely think the project should pursue, independent of what your LLM suggested.

I'll close this for now.
achow101 closed this on May 7, 2026

Contributors

Labels

Linked (view graph)

#35172 txdb: Cursor warmup accepts malformed first coin key

txdb: handle malformed first coin cursor key #35191

What?

Scenario

Testing

Code Coverage & Benchmarks

Reviews