index: Fix coinstats overflow and introduce index versioning #30469

1. 
   fjahr commented at 10:46 am on July 17, 2024: contributor

   Closes #26362

   This continues the work that was started with #26426. It fixes the overflow issue by switching most of the values tracked in the index from historical totals to values per block. The only effect of this is that it requires iterating over all the entries to get to these historical values, however nothing changes for the capabilities of the RPCs we have today because the switched values were always report per block and the totals were used in concepts like #19792.

   The change requires a rebuild of the index. Because of this, the PR introduces the concept of versioning to indexes and I am looking for feedback on the approach here. If an outdated version is detected the node won’t start until the user has removed the index manually or start without the option. This should prevent unwanted data loss for the user.

   Ideally the approach chosen here should be compatible with any future cases where we require an index to upgrade. The most recently such a scenario was discussed in #28241. I am currently convinced that a different approach would be a better fit there, however the discussion is still in the early stages.

   This PR also does quite a bit of renaming, reorgs some lines and adds some white space. All of this made it easier for me to work with the code.

2. 
   DrahtBot commented at 10:46 am on July 17, 2024: 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/30469.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   Concept ACK	mzumsande
   Approach ACK	TheCharlatan

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

   Conflicts

   No conflicts as of last run.

3. DrahtBot added the label UTXO Db and Indexes on Jul 17, 2024
4. DrahtBot added the label CI failed on Jul 17, 2024
5. fjahr force-pushed on Jul 23, 2024
6. fjahr force-pushed on Jul 23, 2024
7. 
   mzumsande commented at 6:07 pm on July 23, 2024: contributor

   Concept ACK

   Haven’t tested anything yet, but did you check what happens in case of a downgrade? (an index generated with this PR is loaded with master)

8. DrahtBot removed the label CI failed on Jul 23, 2024
9. 
   mzumsande commented at 5:09 pm on July 24, 2024: contributor

   Since rebuilding the coinstatsindex takes a long time and it’s not corrupted (yet) on mainnet I thought that auto-migrating to the new format instead of making the user reindex might be more user-friendly: I tried that out in https://github.com/mzumsande/bitcoin/commits/202407_coinstats_v1_automigrate/ (just an untested POC - also doesn’t process any blocks saved by hash instead of height yet, but that should be possible to add.) What do you think of that option?
10. DrahtBot added the label CI failed on Sep 8, 2024
11. DrahtBot removed the label CI failed on Sep 14, 2024
12. DrahtBot added the label CI failed on Oct 13, 2024
13. achow101 requested review from TheCharlatan on Oct 15, 2024
14. DrahtBot removed the label CI failed on Oct 19, 2024
15. DrahtBot added the label CI failed on Mar 17, 2025
16. fjahr force-pushed on Mar 30, 2025
17. 
    fjahr commented at 8:59 pm on March 30, 2025: contributor

    Picking this up again after this has gone a bit quiet for a while. After having a brief conversation about it with @mzumsande at CoreDev I took another look at his migration idea. Initially I was a bit undecided honestly but I now think that it’s reasonable to try this and I would like to open this up for additional approach feedback from other reviewers. Aside from a rebase this has now cherry-picked @mzumsande ’s draft commit with minor changes by me and I adapted the tests to check the new behavior is a separate commit. Test-each-commit CI will probably fail right now but I plan to make a clean history once I have some additional feedback that including the migration is the way to go.

    I have also tested the migration behavior on Signet again and it looks good to me.

     02025-03-30T18:08:00Z Opened LevelDB successfully
     12025-03-30T18:08:00Z Using obfuscation key for /Users/FJ/Library/Application Support/Bitcoin/signet/indexes/coinstats/db: 0000000000000000
     22025-03-30T18:08:00Z Migrating coinstatsindex to new format (v1). This might take a few minutes.
     32025-03-30T18:08:00Z Migrating block at height 240000
     42025-03-30T18:08:00Z Migrating block at height 230000
     52025-03-30T18:08:00Z Migrating block at height 220000
     62025-03-30T18:08:00Z Migrating block at height 210000
     72025-03-30T18:08:00Z Migrating block at height 200000
     82025-03-30T18:08:00Z Migrating block at height 190000
     92025-03-30T18:08:00Z Migrating block at height 180000
    102025-03-30T18:08:00Z Migrating block at height 170000
    112025-03-30T18:08:00Z Migrating block at height 160000
    122025-03-30T18:08:00Z Migrating block at height 150000
    132025-03-30T18:08:00Z Migrating block at height 140000
    142025-03-30T18:08:00Z Migrating block at height 130000
    152025-03-30T18:08:00Z Migrating block at height 120000
    162025-03-30T18:08:00Z [error] Coinstatsindex is corrupted at height 112516
    172025-03-30T18:08:00Z [error] coinstatsindex migration: Error while migrating to v1. In order to rebuild the index, remove the indexes/coinstats directory in your datadir
    

18. DrahtBot removed the label CI failed on Mar 30, 2025
19. fjahr force-pushed on Apr 24, 2025
20. fjahr force-pushed on Apr 25, 2025
21. 
    fjahr commented at 9:20 pm on April 25, 2025: contributor

    Needed to reorg the commits a bit to get the test-every-commit CI to pass
22. 
    fjahr commented at 9:26 pm on April 25, 2025: contributor

    I guess this could use a bug label too if someone with permission can get around to that :)
23. achow101 added this to the milestone 30.0 on Apr 25, 2025
24. DrahtBot added the label CI failed on May 10, 2025
25. 
    maflcko commented at 8:25 pm on May 20, 2025: member

    Looks like CI failed
26. index: Fix coinstatsindex overflow issue 954b2d01f4
27. index: Introduce versioning concept to indexes fe8ddebffb
28. add migration function for coinstatsindex

    If the index is not corrupted due to an integer overflow (which is
    currently only the case on signet), we can use the existing entries
    to migrate to the new format. That way, users don't have to manually
    remove the index and build it again.

    8031519207
29. test: Add test for new coinstats index version 73571b9c19
30. fjahr force-pushed on May 21, 2025
31. DrahtBot removed the label CI failed on May 22, 2025
32. 
    fjahr commented at 12:47 pm on May 22, 2025: contributor

    Looks like CI failed

    Thanks, I had missed that. Seems to have been a silent merge conflict. Kind of weird that only one environment failed but I have come up with an easier way to produce the corrupted db failure case and added some documentation on that as well.

33. 
    maflcko commented at 1:07 pm on May 22, 2025: member

    Kind of weird that only one environment failed

    There are not enough CI resources to re-run every task on every pull request every few days. The CI resources would have to be ~doubled for that. For now, only the lint and previous-releases task are re-run.

34. 
    fjahr commented at 1:17 pm on May 22, 2025: contributor

    Kind of weird that only one environment failed

    There are not enough CI resources to re-run every task on every pull request every few days. The CI resources would have to be ~doubled for that. For now, only the lint and previous-releases task are re-run.

    Good to know, thanks!

35. in src/index/coinstatsindex.cpp:556 in 8031519207 outdated

    551+        return false;
    552+    }
    553+
    554+    // Loop backwards until we hit the genesis block which doesn't need to be updated
    555+    while (pindex->pprev) {
    556+        if (pindex->nHeight % 10000 == 0) LogInfo("Migrating block at height %i\n", pindex->nHeight);
    

    ---

    ---

    

    TheCharlatan commented at 8:28 pm on May 23, 2025:

    Nit: I think this message should be a little clearer. How about: “Migrating coinstatsindex, current block height %s”.
36. 
    TheCharlatan commented at 8:40 pm on May 23, 2025: contributor

    Approach ACK

    This all looks good to me, but I am not sure if embedding the database in our functional test suite is a good idea. I don’t think it is too much data, but I am not sure how stable it is in terms of file formats. Is this really compatible with all posix systems? Then again it could just be removed once/if it starts becoming unreliable.

Contributors
fjahr DrahtBot mzumsande maflcko TheCharlatan

Labels
UTXO Db and Indexes

Milestone
30.0