index: Fix coinstats overflow

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

This continues the work that was started with #26426. It fixes the overflow issue by switching the tracked values that are in danger of overflowing from CAmount to arith_uint256.

The current approach opts for a simple solution to ensure compatibility with datadirs including the previous version of the index: The new version of the index goes into a separate location in the datadir (index/coinstatsindex/ rather than index/coinstats/ before, the new naming is more consistent with the naming of the other indexes). There is no explicit concept of versioning of the index which earlier versions of this PR had. Having the two different versions of the index in separate places allows for downgrading of the node without having to rebuild the index. However, there will be a warning printed in the logs if the new code (v30) detects the old index still being present. A future version could delete a left-over legacy index automatically.

The PR also includes several minor improvements but most notably it lets new entries be calculated and stored without needing to read any DB records.

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
ACK	TheCharlatan, achow101, mzumsande
Approach ACK	ajtowns

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#33306 (index: Force database compaction in coinstatsindex by fjahr)
#32997 (index: Deduplicate HashKey / HeightKey handling by mzumsande)
#32541 (index: store per-block transaction locations for efficient lookups by romanz)
#31308 (ci, iwyu: Treat warnings as errors for specific directories by hebasto)

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.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

// These per-block values should fit uint64 under normal circumstances -> uint64_t / fit in a uint64_t [missing word “in” and use of the standard type name; makes the comment slightly unclear about the intended type]

drahtbot_id_5_m

DrahtBot added the label UTXO Db and Indexes on Jul 17, 2024

DrahtBot added the label CI failed on Jul 17, 2024

fjahr force-pushed on Jul 23, 2024

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)

DrahtBot removed the label CI failed on Jul 23, 2024

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?

DrahtBot added the label CI failed on Sep 8, 2024

DrahtBot removed the label CI failed on Sep 14, 2024

DrahtBot added the label CI failed on Oct 13, 2024

achow101 requested review from TheCharlatan on Oct 15, 2024

DrahtBot removed the label CI failed on Oct 19, 2024

DrahtBot added the label CI failed on Mar 17, 2025

fjahr force-pushed on Mar 30, 2025

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

DrahtBot removed the label CI failed on Mar 30, 2025

fjahr force-pushed on Apr 24, 2025

fjahr force-pushed on Apr 25, 2025

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

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 :)

achow101 added this to the milestone 30.0 on Apr 25, 2025

DrahtBot added the label CI failed on May 10, 2025

maflcko commented at 8:25 pm on May 20, 2025: member

Looks like CI failed

fjahr force-pushed on May 21, 2025

DrahtBot removed the label CI failed on May 22, 2025

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.

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.

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!

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

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.

in test/functional/data/coinstatsindex_v0_corrupt/000006.log:1 in 73571b9c19

maflcko commented at 7:41 am on May 26, 2025:

It is “just” 20kB, but I think we’d want to avoid adding binary blobs to the repo at this point. At a minimum, it would be good to have a reason to do this.

The alternative of just using a previous release to create it on-demand has many benefits:

It works on Windows as well
It doesn’t bloat the git history by 20+ kB forever
It is easier to review and easier to adapt

fjahr commented at 10:29 am on May 26, 2025:

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.

It is “just” 20kB, but I think we’d want to avoid adding binary blobs to the repo at this point. At a minimum, it would be good to have a reason to do this.

Happy to try other approaches that are more stable and easier to grasp, I did think about alternative approaches too. My personal “best” alternative I could come up with was to add a leveldb python library to our test framework and edit the db on the fly with it, e.g. plyvel. On the one hand, I am not sure if it’s worth adding such a dependency for this test case, on the other hand this could be used to simulate leveldb corruption for all kinds of tests all over the codebase so that may make it worth it. What are your thoughts on that?

The alternative of just using a previous release to create it on-demand

Maybe I am not getting it but how would you create the corrupt index with the previous release in the test? The only way I can think of is to mine a deterministic chain long enough to overflow the values just like it happened in signet. But that would require a lot of blocks to be mined and this would make the test extremely slow, probably prohibiting it from being run on the CI. And for being run locally the previous releases would be needed so I fear this test would hardly be run ever. So if this is the way to do it the downsides outweigh the benefits. But you probably have something else in mind that I am not able to think of right now…

Or maybe you would discard the corruption test and only do the happy path test?

maflcko commented at 10:56 am on May 26, 2025:

Or maybe you would discard the corruption test and only do the happy path test?

Yeah, I wonder if we can reliably test anything other than the happy path anyway. Generally, once UB happened, there is no reliable way to recover from it. While in practise here, most likely you’ll end up with negative values, I don’t think this is guaranteed by the C++ standard and probably not something to be able to rely on.

Taking a step back, the size of the index is probably small and it is not too hard to re-create it. So an alternative to attempting to migrate it, could be to just have the index sit under a new name/folder for new releases from now. This should allow downgrades to previous releases to continue using the prior index on a best-effort-basis without user involvement. On upgrades, the new location is used without user involvement. A warning could be printed on every start to offer the user to delete the old index, if they don’t need it for use with previous releases?

fjahr commented at 8:54 pm on June 2, 2025:

Yeah, I wonder if we can reliably test anything other than the happy path anyway. Generally, once UB happened, there is no reliable way to recover from it. While in practise here, most likely you’ll end up with negative values, I don’t think this is guaranteed by the C++ standard and probably not something to be able to rely on.

Sure, there is no guarantee that the check that the corruption test is testing will actually be hit for every corrupt index. I still think it would be good to have test coverage here but I am also happy to remove it (and implement the rest with a previous release) if other reviewers agree we only need the happy path.

Taking a step back, the size of the index is probably small and it is not too hard to re-create it. So an alternative to attempting to migrate it, could be to just have the index sit under a new name/folder for new releases from now. This should allow downgrades to previous releases to continue using the prior index on a best-effort-basis without user involvement. On upgrades, the new location is used without user involvement. A warning could be printed on every start to offer the user to delete the old index, if they don’t need it for use with previous releases?

Hmm, the size of the mainnet coinstats index on my machine is 160MB, but I’m not sure I like any amount of data that will be left over in the users datadir potentially forever. Worst case the user has also used the node for Testnet3 and Signet and it may be over 500MB total. I am not sure how many people notice such warnings and act on them, so I am kind of on the fence between these two solutions. Is there any precedent for something like this that could give some guidance @maflcko ? Nevertheless, it would indeed be a way simpler fix, I have sketched this out here: https://github.com/fjahr/bitcoin/tree/2024-07-csi-overflow-2-keep The fix commit is almost the same except for the folder name change. And then there is a simple compatibility test. The versioning code and migration are scrapped.

I am currently undecided between (potentially) leaving data on users disk that may not be used and making downgrading possible. I will have to think about this a little bit more I would be happy to hear more feedback what other reviewers think. Maybe it’s even worth bringing it up on IRC to get some more input.

maflcko commented at 5:31 am on June 3, 2025:

Deleting the folder right away should be fine, too. (This is what this pull is doing?) An alternative would be to delay the deletion until the next major release and keep the old data for one release.

fjahr commented at 10:46 am on June 4, 2025:

No, it wasn’t deleting it because it sounded like it should be kept around but yeah, deleting makes sense startin from a future version. I will just go with >=31 for now, if this gets backported the version check can be adapted accordingly.

fjahr force-pushed on Jun 4, 2025

fjahr commented at 10:49 am on June 4, 2025: contributor

I have pushed a new version with a simplified approach as suggested by @maflcko. This puts the fixed version into a new path and removes the old version after a grace period. This makes downgrading possible and uses less code. I think this approach isn’t too far off from the migration since syncing the index and running the migration are both processes that take some time.

fjahr force-pushed on Jun 4, 2025

DrahtBot added the label CI failed on Jun 4, 2025

DrahtBot commented at 10:50 am on June 4, 2025: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/runs/43456257400 LLM reason (✨ experimental): Lint check failure due to unused import errors detected by ruff.

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

fjahr force-pushed on Jun 4, 2025

DrahtBot removed the label CI failed on Jun 4, 2025

DrahtBot added the label Needs rebase on Jun 12, 2025

fjahr force-pushed on Jun 13, 2025

fjahr renamed this:
~~index: Fix coinstats overflow and introduce index versioning~~
index: Fix coinstats overflow
on Jun 13, 2025

DrahtBot removed the label Needs rebase on Jun 13, 2025

TheCharlatan commented at 12:13 pm on June 15, 2025: contributor

The new approach seems fine to me, but the pull request description needs updating. In general I’m not too big a fan of leaving unused data lying around, but waiting for a deprecation cycle seems like a good compromise.

fjahr commented at 7:35 pm on June 15, 2025: contributor

Updated the description

in src/index/coinstatsindex.cpp:122 in 1a1b7cf3c5 outdated

118+        if (CLIENT_VERSION >= 310000) {
119+            fs::remove_all(old_path);
120+            LogInfo("Old version of coinstatsindex was found at %s and deleted.", fs::PathToString(old_path));
121+        } else {
122+            LogWarning("Old version of coinstatsindex found at %s. This folder can be safely deleted unless you " \
123+                "plan to downgrade your node to version 29 or lower.", fs::PathToString(old_path));

maflcko commented at 6:56 am on June 16, 2025:

not sure about changing the software behavior based on the version number. This will cause test failures when the version is bumped after the branch off (or it will require to change the tests in the same commit that bumps the version). This seems odd. Also, at least one of the branches will be dead code. So I think it would be better to just inline the non-dead code and have the dead code in a code comment (or other comment) with a note to adjust it after the branch-off.

fjahr commented at 11:01 pm on June 17, 2025:

Removed the version specific removal and replaced it with a TODO to delete the index in the future.

in src/index/coinstatsindex.cpp:111 in 1a1b7cf3c5 outdated

106@@ -106,16 +107,41 @@ std::unique_ptr<CoinStatsIndex> g_coin_stats_index;
107 CoinStatsIndex::CoinStatsIndex(std::unique_ptr<interfaces::Chain> chain, size_t n_cache_size, bool f_memory, bool f_wipe)
108     : BaseIndex(std::move(chain), "coinstatsindex")
109 {
110-    fs::path path{gArgs.GetDataDirNet() / "indexes" / "coinstats"};
111+    // An earlier version of the index used "indexes/coinstats" but it contained
112+    // a bug and is superceded by a fixed version at "indexes/coinstatsindex".

maflcko commented at 7:50 am on June 16, 2025:

superceded -> superseded [correct spelling of “superseded”]

fjahr commented at 11:02 pm on June 17, 2025:

fixed

fjahr force-pushed on Jun 17, 2025

TheCharlatan approved

TheCharlatan commented at 9:01 am on June 18, 2025: contributor

ACK 33668270eeecab4ec2a47d92a71f97abf9d0aceb

DrahtBot requested review from mzumsande on Jun 18, 2025

DrahtBot added the label Needs rebase on Jul 8, 2025

fjahr force-pushed on Jul 8, 2025

fjahr commented at 2:01 pm on July 8, 2025: contributor

Rebased

DrahtBot removed the label Needs rebase on Jul 8, 2025

in test/functional/feature_init.py:157 in 5bbbcc120f outdated

153@@ -154,7 +154,7 @@ def check_clean_start(extra_args):
154                 'startup_args': ['-blockfilterindex=1'],
155             },
156             {
157-                'filepath_glob': 'indexes/coinstats/db/*.*',
158+                'filepath_glob': 'indexes/coinstatsindex/db/*.*',

mzumsande commented at 8:20 pm on July 8, 2025:

it’s commented out, but could also update line 131 above. Also, doc/files.md should be updated.

fjahr commented at 9:46 pm on July 13, 2025:

Done

hodlinator commented at 7:01 pm on August 16, 2025:

Still see indexes/coinstats/db/ above? https://github.com/bitcoin/bitcoin/blob/ca79480a9989379d20279540314fe9bc6b88f0f0/test/functional/feature_init.py#L131

fjahr commented at 12:05 pm on August 17, 2025:

Thanks, fixed the comment too.

in test/functional/feature_coinstatsindex_compatibility.py:54 in e5eb7d3467 outdated

49+
50+        self.log.info("Test that gettxoutsetinfo() output is consistent for the new index running on a datadir with the old version")
51+        self.stop_nodes()
52+        shutil.rmtree(node.chain_path / "indexes" / "coinstatsindex")
53+        shutil.copytree(legacy_node.chain_path / "indexes" / "coinstats", node.chain_path / "indexes" / "coinstats")
54+        msg = f"[CoinStatsIndex] [warning] Old version of coinstatsindex found at {node.chain_path}/indexes/coinstats. This folder can be safely deleted unless you plan to downgrade your node to version 29 or lower."

mzumsande commented at 10:02 pm on July 8, 2025:

FYI test fails because of some windows path-printing isues.

fjahr commented at 10:05 pm on July 13, 2025:

Fixed, and thanks for the ping, I missed the failed CI initially.

mzumsande commented at 10:09 pm on July 8, 2025: contributor

nit: cummulative -> cumulative in 5bbbcc120f1139655787e7454277bb2d73cb49e4 description

Looks good to me besides the nits.

This will also need a release note, warning users that existing indexes will be resynced, which will probably take a few days for mainnet.

fjahr force-pushed on Jul 13, 2025

DrahtBot added the label CI failed on Jul 13, 2025

DrahtBot commented at 9:43 pm on July 13, 2025: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/runs/45886862186 LLM reason (✨ experimental): The CI failure is caused by a syntax error in a Python lint check due to an unterminated f-string.

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

fjahr force-pushed on Jul 13, 2025

fjahr commented at 10:06 pm on July 13, 2025: contributor

Addressed @mzumsande ’s comments and added a release note.

fjahr force-pushed on Jul 14, 2025

DrahtBot removed the label CI failed on Jul 14, 2025

in src/index/coinstatsindex.cpp:131 in 4fc1b2214b outdated

129-    const CAmount block_subsidy{GetBlockSubsidy(block.height, Params().GetConsensus())};
130-    m_total_subsidy += block_subsidy;
131+    CAmount block_unspendable{0};
132+    CBlockUndo block_undo{};
133+
134+    m_block_prevout_spent_amount = 0;

mzumsande commented at 6:46 pm on July 21, 2025:

I wonder if it wouldn’t be simpler if all of the m_block_xyz class members were just local variables within CoinStatsIndex::CustomAppend instead. The previous values are being overwritten here with each new block, and as far as I can see the saved values (while being set to correct values in CustomInit and ReverseBlock) are not used anywhere - except for this line in ReverseBlock - but we could also use the value from read_out there (if we don’t want to calculate the diff).

achow101 commented at 9:38 pm on July 30, 2025:

I also think it would be better to drop the m_block_* class members since they’re basically only used by CustomAppend.

fjahr commented at 0:21 am on August 4, 2025:

Ok, I am working on a PR for this and I should have it ready by tomorrow but I think it’s better to keep it separate from this one.

fjahr commented at 4:22 pm on August 4, 2025:

Addressed in #33134

mzumsande commented at 7:58 pm on July 30, 2025: contributor

ACK 4cb2c0520211634ee3caf6d4c3a1e65b2bdb2dc6

My comment above is not blocking (could also be done later) - considering that this is tagged for 30.0 and feature freeze is soon, I wonder if we should aim for that, or combine it with #26966 (which I think could be enabled for coinstatsindex relatively easily, FYI @furszy) so that all of the users with existing indexes would have the option to resync much faster.

DrahtBot requested review from TheCharlatan on Jul 30, 2025

fjahr commented at 0:30 am on August 4, 2025: contributor

which I think could be enabled for coinstatsindex relatively easily, FYI @furszy

Not sure if I maybe misunderstand that part but do you mean parallelization could be enabled easily for coinstatsindex? I haven’t reviewed #26966 in depth yet but I don’t think that’s the case because the blocks have to be processed in order.

furszy commented at 4:27 am on August 4, 2025: member

which I think could be enabled for coinstatsindex relatively easily, FYI @furszy

Not sure if I maybe misunderstand that part but do you mean parallelization could be enabled easily for coinstatsindex? I haven’t reviewed #26966 in depth yet but I don’t think that’s the case because the blocks have to be processed in order.

It shouldn’t take me much to parallelize it. Most fields here are essentially accumulators. Blocks can be read from disk and processed in parallel. Only the dump to disk has to be done sequentially; merging all accumulators that requires it (the total_* ones) + computing the accumulated muhash. Also, disk writes can be batched this way too.

Can find a similar workflow in #26966 for the block filter index: fa4348463dc44099ef1fdccb919056a0be44b97d. Which had comparable constraints due to the filter headers’ chain structure (each header references its predecessor).

fjahr commented at 4:23 pm on August 4, 2025: contributor

Can find a similar workflow in #26966 for the block filter index: https://github.com/bitcoin/bitcoin/commit/fa4348463dc44099ef1fdccb919056a0be44b97d. Which had comparable constraints due to the filter headers’ chain structure (each header references its predecessor).

Ah, I see, cool, I didn’t know this PR went that far.

in src/kernel/coinstats.h:51 in 4fc1b2214b outdated

47@@ -48,24 +48,26 @@ struct CCoinsStats {
48 
49     // Following values are only available from coinstats index
50 
51-    //! Total cumulative amount of block subsidies up to and including this block
52-    CAmount total_subsidy{0};
53-    //! Total cumulative amount of unspendable coins up to and including this block
54+    //! Amount of unspendable coins in this block

achow101 commented at 7:52 pm on August 4, 2025:

In 4fc1b2214baf10d0481c11ab0fc1de2b5d55d687 “index: Fix coinstatsindex overflow issue”

total_unspendable_amount is still a cumulative value, not per block.

fjahr commented at 11:31 pm on August 15, 2025:

Fixed the comment

achow101 commented at 8:00 pm on August 4, 2025: member

I strongly prefer to have the changes from #33134 be combined with this PR. IMO it would make the change in this PR easier to review.

fjahr force-pushed on Aug 15, 2025

fjahr commented at 11:37 pm on August 15, 2025: contributor

I strongly prefer to have the changes from #33134 be combined with this PR. IMO it would make the change in this PR easier to review.

I have pulled the changes in here and closed #33134. Together with the feedback I received over there I am not sure it’s still easier to review but I did my best to split the commits cleanly as @l0rinc requested. However this required quite a bit of retouching the same lines.

Aside from the feedback from @l0rinc I have added a commit that let’s us skip db reads when appending a new block as @furszy suggested here. However this requires keeping part of the member variables around so looking for renewed acks on that approach.

achow101 commented at 1:51 am on August 16, 2025: member

ACK ca79480a9989379d20279540314fe9bc6b88f0f0

DrahtBot requested review from mzumsande on Aug 16, 2025

fjahr commented at 3:29 pm on August 16, 2025: contributor

The failed CI job looks unrelated fwiw, downloading a previous release failed.

fjahr force-pushed on Aug 17, 2025

fjahr commented at 12:06 pm on August 17, 2025: contributor

Fixed a comment that still had an old path as noted by @hodlinator

in test/functional/feature_coinstatsindex_compatibility.py:31 in 761c25b9d4 outdated

26+        self.add_nodes(
27+            self.num_nodes,
28+            extra_args=self.extra_args,
29+            versions=[
30+                None,
31+                280000,

maflcko commented at 7:00 am on August 18, 2025:

you can’t use this version, because it has not been downloaded (see the ci failure).

You’ll have to download this version or use a downloaded version.

fjahr commented at 3:51 pm on August 18, 2025:

Fixed, I missed that this had changed.

fjahr force-pushed on Aug 18, 2025

in src/index/coinstatsindex.cpp:425 in b8c5bf71a4 outdated

413@@ -413,11 +414,16 @@ bool CoinStatsIndex::RevertBlock(const interfaces::BlockInfo& block)
414     assert(block.undo_data);
415     for (size_t i = 0; i < block.data->vtx.size(); ++i) {
416         const auto& tx{block.data->vtx.at(i)};
417+        const bool is_coinbase{tx->IsCoinBase()};
418+
419+        if (is_coinbase && IsBIP30Unspendable(block.hash, block.height)) {
420+            continue;
421+        }

achow101 commented at 8:10 pm on August 18, 2025:

In b8c5bf71a4be4756740fb7b8a6a580153b007475 “index, refactor: DRY coinbase check”

This seems like a bug fix that should be in it’s own commit.

Should this be decrementing m_total_unspendable_amount and m_total_unspendables_bip30 to reverse what the similar code in CustomAppend does? This doesn’t really matter once those members are removed.

fjahr commented at 11:19 pm on August 25, 2025:

I find it almost a philosophical question if we should cover reorganizing the BIP30 blocks so I honestly didn’t really think much about this and I added it almost by accident when reorganizing (is that a pun?) all the commits between the changes here and #33134 but I guess the removal of the checkpoints and surrounding discussions recently pushed me more into the camp of doing this fully correctly and this is why I added it when I previously didn’t. I will move this into it’s own commit.

This doesn’t really matter once those members are removed.

Right, after all changes here are applied the m_total_unspendables_bip30 doesn’t need fixing because it is removed it’s replacement (block_unspendables_bip30) is handled differently. The value is now calculated only per block so there shouldn’t be an issue because the behavior is not “rolling this value back and forth” like it used to with the member variable tracking. If a BIP30 block would be reorged, it would be moved to the hash key index (CopyHeightIndexToHashIndex in CustomRemove) and the new block at the same height would never save a value for it when it goes through CustomAppend because the IsBIP30Unspendable wouldn’t apply because the hash should be different.

And for m_total_unspendable_amount we don’t need a fix because we pull its value out of the prev block record and update the member variable with this instead of rolling it back:

0    m_total_unspendable_amount = read_out.second.total_unspendable_amount;

Then m_total_unspendable_amount is used in CustomAppend to calculate the value for the next block that comes in. We would need to do this if we were implementing RevertBlock without a disk read like I am doing here with CustomAppend but I think having the consistency check of the muhash is good to have so we get this for free when we read the prev block anyway.

I am taking the easy out here by moving the “fix” of adding the conditional to the end of the commits. Then I think the commits should be consistent again.

mzumsande commented at 9:48 pm on September 2, 2025:

it would be good to add a little explanation to this commit 4a72b62a0d9345c1720de538cd3b42659f7d046b - maybe just saying that it’s not practically relevant because of the huge reorgs it would require, but that RevertBlock should be consistent with CustomAppend.

achow101 commented at 8:34 pm on August 18, 2025: member

053ac55eb569be6b49c5249a7d8c0eeaa149a18b seems like it could cause incorrect total statistics if somehow we ever have the situation where we are appending a block that doesn’t build on the index’s current block hash.

This behavior does match what we do today with the running total members, but it feels incorrect. Although that sounds like it should really be a fatal error.

mzumsande commented at 4:02 pm on August 20, 2025: contributor

I like the approach, though it’s a little bit weird that m_total_amount and m_total_unspendable_amount are first removed in e54317bf7c1b93d790e169759f64a4564a96ea5f, and after that reintroduced in 053ac55eb569be6b49c5249a7d8c0eeaa149a18b. Happy to ACK once @achow101’s point about m_total_unspendables_bip30 is addressed (which I also tripped over).

somehow we ever have the situation where we are appending a block that doesn’t build on the index’s current block hash.

I kinda view it as a basic principle of the index design that this shouldn’t be possible, relying on the order of Block(Dis)Connected signals. The most critical point for this seems to be the transition between Sync() and signal-based processing, otherwise I can’t see how we could get into that situaion.

fjahr force-pushed on Aug 25, 2025

fjahr commented at 11:19 pm on August 25, 2025: contributor

First of all, sorry for the late reply, I will do my best to respond faster despite travelling at the moment.

053ac55 seems like it could cause incorrect total statistics if somehow we ever have the situation where we are appending a block that doesn’t build on the index’s current block hash.

This behavior does match what we do today with the running total members, but it feels incorrect. Although that sounds like it should really be a fatal error.

Hm, I am not sure how this is possible. When a block is connected (BaseIndex::BlockConnected) we check that the new block’s prev block is our best block, if not we try to rewind first:

0        if (best_block_index != pindex->pprev && !Rewind(best_block_index, pindex->pprev)) {

And in 053ac55 we also check the same thing:

0        uint256 expected_block_hash{*Assert(block.prev_hash)};
1        if (m_current_block_hash != expected_block_hash) {

I agree with @mzumsande that this is assumed to be guaranteed by the base index behavior but I think this could probably be documented better. We do have a _test_reorg_index in feature_coinstatsindex.py which should cover this but the particular edge case @mzumsande mentions is really hard to hit.

I like the approach, though it’s a little bit weird that m_total_amount and m_total_unspendable_amount are first removed in https://github.com/bitcoin/bitcoin/commit/e54317bf7c1b93d790e169759f64a4564a96ea5f, and after that reintroduced in https://github.com/bitcoin/bitcoin/commit/053ac55eb569be6b49c5249a7d8c0eeaa149a18b.

Yeah, sorry about that but I didn’t find a better way to make the commits self-contained as requested in #33134. Looking at it again now, it’s probably possible to do this better and I can give it another shot if you like.

fjahr commented at 11:20 pm on August 25, 2025: contributor

Addressed the feedback and also rebased for good measure since there were recent changes to the indexing.

achow101 commented at 8:41 pm on August 26, 2025: member

ACK dd7e696762b4863a5f06d150c4fe3fb882ac69dc

DrahtBot requested review from mzumsande on Aug 26, 2025

in src/index/coinstatsindex.cpp:118 in e7b111a630 outdated

114@@ -115,11 +115,10 @@ CoinStatsIndex::CoinStatsIndex(std::unique_ptr<interfaces::Chain> chain, size_t
115 bool CoinStatsIndex::CustomAppend(const interfaces::BlockInfo& block)
116 {
117     const CAmount block_subsidy{GetBlockSubsidy(block.height, Params().GetConsensus())};
118-    m_total_subsidy += block_subsidy;

davidgumberg commented at 11:29 pm on August 28, 2025:

In commit https://github.com/bitcoin/bitcoin/pull/30469/commits/e7b111a63067cf6c66140228c28ab1fa96f7ff18 (refactor, index: Remove member variables in coinstatsindex):

for other reviewers: read_out.second.total_subidy is updated below:

https://github.com/bitcoin/bitcoin/blob/e7b111a63067cf6c66140228c28ab1fa96f7ff18/src/index/coinstatsindex.cpp#L200

in src/index/coinstatsindex.cpp:176 in c2bc3b97f7 outdated

171@@ -155,22 +172,22 @@ bool CoinStatsIndex::CustomAppend(const interfaces::BlockInfo& block)
172 
173                 // Skip unspendable coins
174                 if (coin.out.scriptPubKey.IsUnspendable()) {
175-                    read_out.second.total_unspendable_amount += coin.out.nValue;
176-                    read_out.second.total_unspendables_scripts += coin.out.nValue;
177+                    block_unspendable += coin.out.nValue;
178+                    new_value.second.block_unspendables_scripts += coin.out.nValue;

davidgumberg commented at 11:50 pm on August 28, 2025:

Isn’t incrementing new_value.second.block_unspendables_scripts undefined behavior since it never gets initialized? (and the other block_* members of DBVal)

ajtowns commented at 2:41 am on August 29, 2025:

I also have this question. Adding {0} default initializers in the struct DBVal declaration seems sensible?

fjahr commented at 11:55 pm on September 1, 2025:

Adding {0} default initializers in the struct DBVal declaration seems sensible?

Done

in src/kernel/coinstats.h:70 in dd7e696762 outdated

82+    //! The unspendable coinbase output amounts caused by BIP30
83+    CAmount block_unspendables_bip30{0};
84+    //! Amount of outputs sent to unspendable scripts (OP_RETURN for example) in this block
85+    CAmount block_unspendables_scripts{0};
86+    //! Amount of coins lost due to unclaimed miner rewards in this block
87+    CAmount block_unspendables_unclaimed_rewards{0};

ajtowns commented at 2:16 am on August 29, 2025:

Keeping the block_unspendables_* as cumulative totals would be more flexible, and would be nice to be able to directly query. (dropping total_unspendable_amount would be possible in that case, considering it’s just the sum of these totals)

Likewise keeping total_subsidy.

With those values, total_subsidy = total_amount + total_unspendable_amount should be an accounting identity, with total_amount also matching a manual sum of values in the utxo set, and total_subsidy being precisely satoshi’s issuance schedule. Having that be conveniently indexed would be good for quick auditing IMO.

fjahr commented at 11:58 pm on September 1, 2025:

Ok, this basically means we are only turning the values into per-block that are most in danger of overflowing. I have done this and since we are not removing most of the members anymore, I squashed that commit which I hope should make review a bit easier now as well. At least overall the change is simpler with this, I think.

in src/index/coinstatsindex.cpp:143 in dd7e696762 outdated

139         uint256 expected_block_hash{*Assert(block.prev_hash)};
140-        if (read_out.first != expected_block_hash) {
141+        if (m_current_block_hash != expected_block_hash) {
142             LogWarning("previous block header belongs to unexpected block %s; expected %s",
143-                      read_out.first.ToString(), expected_block_hash.ToString());
144+                      m_current_block_hash.ToString(), expected_block_hash.ToString());

ajtowns commented at 2:32 am on August 29, 2025:

Shouldn’t this be an error even if you can find the previous block header? You’ll calculate incorrect values for total_amount otherwise?

fjahr commented at 11:54 pm on September 1, 2025:

Right, this code is “belts-and-suspenders” since we should never end up here since the base index should rewind us in this case before entering CustomAppend. So I will turn this into an error and drop the hashkey query below since we aren’t recovering from that anyway.

This isn’t something that is added here so it could be a separate PR but I will add it here since it simplifies the code a bit.

in src/index/coinstatsindex.cpp:188 in dd7e696762 outdated

187                 const auto& tx_undo{Assert(block.undo_data)->vtxundo.at(i - 1)};
188 
189                 for (size_t j = 0; j < tx_undo.vprevout.size(); ++j) {
190-                    Coin coin{tx_undo.vprevout[j]};
191-                    COutPoint outpoint{tx->vin[j].prevout.hash, tx->vin[j].prevout.n};
192+                    const Coin coin{tx_undo.vprevout[j]};

ajtowns commented at 2:45 am on August 29, 2025:

This could be a reference rather than a copy: const Coin& coin{tx_undox.vprevout[j]};

fjahr commented at 11:54 pm on September 1, 2025:

Done

TheCharlatan commented at 11:05 am on September 5, 2025:

Was this reverted by mistake?

fjahr commented at 3:50 pm on September 7, 2025:

Ah, I seem to have only applied this to RevertBlock but not CustomAppend, both have this identical LOC. Fixed.

in src/index/coinstatsindex.cpp:37 in dd7e696762 outdated

40-    CAmount total_unspendables_genesis_block;
41-    CAmount total_unspendables_bip30;
42-    CAmount total_unspendables_scripts;
43-    CAmount total_unspendables_unclaimed_rewards;
44+    CAmount block_prevout_spent_amount;
45+    CAmount block_new_outputs_ex_coinbase_amount;

ajtowns commented at 3:05 am on August 29, 2025:

Both prevout_spent_amount and new_outputs_ex_coinbase_amount can overflow an int64_t in extreme cases – filling a block with 15000 65-byte transactions (975kvB) repeatedly spending the same 7M BTC will hit 105 billion BTC, which is 1.05e19 sats which is about 13% more than 2**63-1 which is the max value a CAmount can store, and this is undefined behaviour in C++. (You’d need to cycle spends of ~12M BTC or more to overflow a uint64_t, though that is not undefined behaviour)

Even if we don’t want to actually fix this now because it’s implausible for mainnet and would require a deliberate attack even on test nets (and is not even possible on regtest, as the halving schedule means total supply is only 15k rtBTC), having a dummy overflow field for these two values that’s always set to zero would make it possible to fix this in future without having to reindex the entire blockchain.

fjahr commented at 11:54 pm on September 1, 2025:

Hm, I am not sure on this one. It sounds like this is so unlikely that the overflow value would probably go without an implementation of how to use it until something that triggers this actually happens. E.g. even if something goes on that looks suspicious the next release may be a few months out so the attack could already be carried out. In that case users would need to resync again anyway if I’m not mistaken. So I would prefer to only add this if there is a clear path to implement the overflow functionality by v31 which means there should be multiple people agreeing that this is important enough to get review. I am happy to do this if there is appetite to review it.

Potentially moving the values to uint seems like the a low impact change but it’s also quite unsatisfying because it worsens the “semantic correctness” by using a less descriptive type and it also doesn’t fix the problem completely. What do you think about introducing a PositiveAmount type, an unsigned CAmount? It might be worth it since that could be used in other places as well?

For the overflow field, do I understand correctly that you would want to use one overflow field for multiple values?

ajtowns commented at 3:20 am on September 2, 2025:

You only need 65 bits to handle a 1MB block full of 61B txs cycling 21M BTC, so could:

add a single uint8_t that we’d use two bits from for each
add two uint16_t’s to count the number of times MAX_MONEY is exceeded
use __int128 or arith_uint256 for those fields

Could also just cap the value:

 0static inline void cap_add(CAmount& v, CAmount x)
 1{
 2    if (std::numeric_limits<CAmount>::max() - x < v) {
 3        v = std::numeric_limits<CAmount>::max();
 4    } else {
 5        v += x;
 6    }
 7}
 8...
 9cap_add(value.second.block_new_outputs_ex_coinbase_amount, coin.out.nValue);
10...
11cap_add(value.second.block_prevout_spent_amount, coin.out.nValue);

Or could combine the two components:

0value.second.block_net_spent += coin.out.nValue; // for each vin
1value.second.block_net_spent -= coin.out.nValue; // for each vout

which should give you the fees that the coinbase should be collecting, which seems like the only really meaningful thing you can do with those numbers anyway. That would be a change to the gettxoutsetinfo RPC though.

ajtowns commented at 3:18 am on August 29, 2025: contributor

Sorry for not looking at this earlier. Approach ACK.

ajtowns commented at 3:27 am on August 29, 2025: contributor

When running this index over signet, I end up with ~700 55kB ldb files, and a single 8.8MB ldb file. That seems like something is probably suboptimal?

fjahr force-pushed on Sep 1, 2025

fjahr commented at 0:00 am on September 2, 2025: contributor

When running this index over signet, I end up with ~700 55kB ldb files, and a single 8.8MB ldb file. That seems like something is probably suboptimal?

This doesn’t seem to be caused by this change afaict, my old coinstatsindex on signet has the same file structure. I have tried to find the cause for this but so far I can’t really say for sure where the difference to the other indexes lies. There does not seem to be any difference in configuration and I looked into additional batching db writes but this became a big, messy change and did not seem to change much. My best guess is that during sync the blocks are written so fast that there is never any compaction of the database as it happens for the other indexes. Forcing compaction of the database merges these 55k files and it doesn’t slow down down the index sync in any meaningful way. I have added a commit for this but this could also be done in a separate PR since it doesn’t seem related to the changes here. But if we want it in v30 it’s probably easier to leave it here since it’s also just a small change. Of course it would be preferred to understand the actual cause for this behavior in LeveDB but I wanted to provide an update here regardless.

DrahtBot added the label CI failed on Sep 2, 2025

DrahtBot commented at 1:10 am on September 2, 2025: contributor

🚧 At least one of the CI tasks failed. Task ARM, unit tests, no functional tests: https://github.com/bitcoin/bitcoin/runs/49360561051 LLM reason (✨ experimental): Segmentation fault in mptest IPC path (callFnAsyncParams) causing the test suite to fail.

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

DrahtBot removed the label CI failed on Sep 2, 2025

mzumsande commented at 9:58 pm on September 2, 2025: contributor

Did another round of review, and everything looked good to me. I would prefer to have ef01a192d0ad821bb3d332ae69cd8b58c010b45d as a separate PR as suggested: As far as I understand it doesn’t break compatibility and therefore doesn’t need to be bundled with this.

Happy to A CK after this.

index, refactor: Rename ReverseBlock to RevertBlock

Semantically this is the correct name for what the function is doing.

fab842b324

index, refactor: DRY coinbase check

Also marks a few additional variables const.

84e813a02b

fjahr force-pushed on Sep 4, 2025

fjahr commented at 4:10 pm on September 4, 2025: contributor

Dear reviewers, sorry for another slight change in approach here but I think this should be the last one and I think it makes reasoning about the review easier or at least not harder. Given the discussion about remaining (edge case) potential for overflows I have decided to switch the values we store to arith_uint256. This feels cleaner to me than dealing with overflows with additional code and a shared field etc. The nice side-effect of this is that this makes it feasible to keep the total values rather than per-block, which means all the changes that were needed to make the change to per-block can go away which simplifies the actual fix commit a bit. Making the values per-block was partly motivated by keeping the CAmount type which is the semantically correct type but it has become clear to me now that this was not the right choice and I think switching to arith_uint256 is the right move.

I would prefer to have https://github.com/bitcoin/bitcoin/commit/ef01a192d0ad821bb3d332ae69cd8b58c010b45d as a separate PR as suggested: As far as I understand it doesn’t break compatibility and therefore doesn’t need to be bundled with this.

Done in #33306.

it would be good to add a little explanation to this commit https://github.com/bitcoin/bitcoin/commit/4a72b62a0d9345c1720de538cd3b42659f7d046b - maybe just saying that it’s not practically relevant because of the huge reorgs it would require, but that RevertBlock should be consistent with CustomAppend.

Done.

fjahr commented at 4:40 pm on September 4, 2025: contributor

Hm, not clear to me why clang-tidy CI complains here about lines (or even files) I haven’t touched in this PR. But that’s what the last fixup commit I just pushed is for.

in src/test/fuzz/utxo_total_supply.cpp:156 in 2ceb1b36fb outdated

152@@ -153,7 +153,7 @@ FUZZ_TARGET(utxo_total_supply)
153                 node::RegenerateCommitments(*current_block, chainman);
154                 const bool was_valid = !MineBlock(node, current_block).IsNull();
155 
156-                const auto prev_utxo_stats = utxo_stats;
157+                const auto& prev_utxo_stats = utxo_stats;

maflcko commented at 6:09 pm on September 4, 2025:

this looks like a false positive in clang-tidy and this looks like the wrong fix. utxo_stats is mutable, and taking a const reference does not make it immutable.

(In languages with a borrow checker, this wouldn’t even compile, see https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=b8c78ddbe26b30473498b64a47ba2a6b)

The correct fix is to disable the clang-tidy rule in this line, or globally.

ajtowns commented at 6:22 pm on September 5, 2025:

More to the point, taking an alias auto& foo = bar means that the later comparison foo.hashSerialized == bar.hashSerialized is just bar.hashSerialized == bar.hashSerialized which isn’t very useful. const uint256 prev_hash_serialized{utxo_stats.hashSerialized}; seems like it would be better.

fjahr commented at 4:03 pm on September 7, 2025:

const uint256 prev_hash_serialized{utxo_stats.hashSerialized}; seems like it would be better.

Taken

maflcko commented at 6:11 pm on September 4, 2025: member

Hm, not clear to me why clang-tidy CI complains here about lines (or even files) I haven’t touched in this PR. But that’s what the last fixup commit I just pushed is for.

I haven’t checked this, but the new struct is no longer trivially copyable, so it may have effects outside of the lines you have touched.

we should probably make it trivially copyable, but this seems unrelated.

 0diff --git a/src/arith_uint256.h b/src/arith_uint256.h
 1index 0cf7aa44b0..14e09c3a6a 100644
 2--- a/src/arith_uint256.h
 3+++ b/src/arith_uint256.h
 4@@ -37,21 +37,6 @@ public:
 5             pn[i] = 0;
 6     }
 7 
 8-    base_uint(const base_uint& b)
 9-    {
10-        for (int i = 0; i < WIDTH; i++)
11-            pn[i] = b.pn[i];
12-    }
13-
14-    base_uint& operator=(const base_uint& b)
15-    {
16-        if (this != &b) {
17-            for (int i = 0; i < WIDTH; i++)
18-                pn[i] = b.pn[i];
19-        }
20-        return *this;
21-    }
22-
23     base_uint(uint64_t b)
24     {
25         pn[0] = (unsigned int)b;

index: Fix coinstatsindex overflow issue

The index originally stored cumulative values in a CAmount type but this allowed for
potential overflow issues which were observed on Signet. Fix this by
storing the values that are in danger of overflowing in a arith_uint256.

Also turns an unnecessary copy into a reference in RevertBlock and
CustomAppend and gets
rid of the explicit total unspendable tracking which can be calculated
by adding the four categories of unspendables together.

431a076ae6

index, refactor: Append blocks to coinstatsindex without db read b2e8b64ddc

test: Add coinstatsindex compatibility test bb8d673183

doc: Add release note for 30469 51df9de8e5

index: Check BIP30 blocks when rewinding Coinstatsindex

This is practically irrelevant due to the unlikeliness of a re-org
reaching so deep that it would drop the BIP30 blocks from the chain
(91842 and 91880). However this serves as documentation and ensures that
the functions RevertBlock and CustomAppend are consistent.

37c4fba1f4

index: Remove unused coinstatsindex recovery code

The coinstatsindex currently looks for block data at a hash key if the prev block in CustomAppend is different than expected. This is not needed since base index should always prevent us ending up in this scenario since it should rewind the index before calling CustomAppend in this case. But even if we run into this and our belt-and-suspenders code is getting hit, the index could not recover properly from the hash key index data so it can be removed without any real impact.

54dc34ec22

clang-tidy: Fix critical warnings

The std::move in coinstatsindex was not necessary since it was passed as a const reference argument.

The other change in the utxo supply fuzz test changes a line that seems to have triggered a false alarm.

c767974811

fjahr force-pushed on Sep 7, 2025

fjahr commented at 4:34 pm on September 7, 2025: contributor

I haven’t checked this, but the new struct is no longer trivially copyable, so it may have effects outside of the lines you have touched.

Correct, I confirmed this with static_assert(std::is_trivially_copyable_v<DBVal>). I am not sure how to test that there are additional effects from this.

we should probably make it trivially copyable, but this seems unrelated.

This seems like a good change to me but I have opened a separate PR for it since I am not sure if this is really necessary to do here. If reviewers would like to see it merged together with this the other PR could still be fast tracked I guess or I can merge it here. But it rather seems like it’s comparable to the LevelDB compaction issue.

in src/index/coinstatsindex.cpp:183 in 84e813a02b outdated

181 
182                 for (size_t j = 0; j < tx_undo.vprevout.size(); ++j) {
183-                    Coin coin{tx_undo.vprevout[j]};
184-                    COutPoint outpoint{tx->vin[j].prevout.hash, tx->vin[j].prevout.n};
185+                    const Coin coin{tx_undo.vprevout[j]};
186+                    const COutPoint outpoint{tx->vin[j].prevout.hash, tx->vin[j].prevout.n};

TheCharlatan commented at 12:56 pm on September 8, 2025:

Can’t we just const COutPoint& outpoint{tx->vin[j].prevout} and similarly on line 443?

fjahr commented at 1:14 pm on September 8, 2025:

Looks right to me, will use it if I have to retouch.

TheCharlatan approved

TheCharlatan commented at 1:02 pm on September 8, 2025: contributor

ACK c76797481155754329ec6a6f58e8402569043944

DrahtBot requested review from achow101 on Sep 8, 2025

DrahtBot requested review from ajtowns on Sep 8, 2025

DrahtBot requested review from mzumsande on Sep 8, 2025

achow101 commented at 7:33 pm on September 8, 2025: member

ACK c76797481155754329ec6a6f58e8402569043944

mzumsande commented at 7:38 pm on September 8, 2025: contributor

Tested / Code Review ACK c76797481155754329ec6a6f58e8402569043944

Please adjust the PR description to the current approach (“switching most of the values tracked in the index from historical totals to values per block” is outdated).

I synced both current branch and master on signet with undefined sanitizer, and verified that muhash / blockstats at selected blocks are identical, and that the overflow doesn’t happen anymore at height 112516.

fjahr commented at 8:52 pm on September 8, 2025: contributor

Please adjust the PR description to the current approach (“switching most of the values tracked in the index from historical totals to values per block” is outdated).

Done

achow101 merged this on Sep 9, 2025

achow101 closed this on Sep 9, 2025

Sjors commented at 11:51 am on September 9, 2025: member

I built the index with v29.1 as well as c76797481155754329ec6a6f58e8402569043944.

getblockstats gives the same result for both at height 913,859. The new coinstatsindex directory is 213 MB, so a bit bigger than the original coinstats, but small in any case.

fjahr commented at 2:47 pm on September 9, 2025: contributor

getblockstats gives the same result for both at height 913,859

Did you mean gettxoutsetinfo here? :)

Sjors commented at 4:21 pm on September 9, 2025: member

Oh oops…

0bitcoin-cli gettxoutsetinfo muhash 913859

Both v29.1 and https://github.com/bitcoin/bitcoin/commit/c76797481155754329ec6a6f58e8402569043944:

 0{
 1  "height": 913859,
 2  "bestblock": "00000000000000000000f7ae1989c423172433aab8aa0164f9fc85fd74ed7f44",
 3  "txouts": 168005479,
 4  "bogosize": 13163671088,
 5  "muhash": "16bf45a4ce9c852aab746826955e3aac8e7e1b964a36af3bce944c17c5e13d55",
 6  "total_amount": 19918082.42878334,
 7  "total_unspendable_amount": 230.07121666,
 8  "block_info": {
 9    "prevout_spent": 2922.18149109,
10    "coinbase": 3.16112162,
11    "new_outputs_ex_coinbase": 2922.14536945,
12    "unspendable": 0.00000002,
13    "unspendables": {
14      "genesis_block": 0.00000000,
15      "bip30": 0.00000000,
16      "scripts": 0.00000002,
17      "unclaimed_rewards": 0.00000000
18    }
19  }
20}

index: Fix coinstats overflow #30469

Code Coverage & Benchmarks

Reviews

Conflicts

LLM Linter (✨ experimental)