index: Fix coinstats overflow #30469
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/30469. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
14c68eb to
73dd2c3
Compare
There was a problem hiding this comment.
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?
73dd2c3 to
31dab6b
Compare
|
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. |
848fce6 to
a581361
Compare
|
Needed to reorg the commits a bit to get the test-every-commit CI to pass |
|
I guess this could use a bug label too if someone with permission can get around to that :) |
|
Looks like CI failed |
There was a problem hiding this comment.
I like the approach, though it's a little bit weird that
m_total_amount and m_total_unspendable_amount are first removed in e54317b, and after that reintroduced in 053ac55. 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.
4e1a3d4 to
03a7318
Compare
|
First of all, sorry for the late reply, I will do my best to respond faster despite travelling at the moment.
Hm, I am not sure how this is possible. When a block is connected ( And in 053ac55 we also check the same thing: 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
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. |
|
Addressed the feedback and also rebased for good measure since there were recent changes to the indexing. |
|
ACK dd7e696 |
src/index/coinstatsindex.cpp
Outdated
| @@ -115,11 +115,10 @@ CoinStatsIndex::CoinStatsIndex(std::unique_ptr<interfaces::Chain> chain, size_t | |||
| bool CoinStatsIndex::CustomAppend(const interfaces::BlockInfo& block) | |||
| { | |||
| const CAmount block_subsidy{GetBlockSubsidy(block.height, Params().GetConsensus())}; | |||
| m_total_subsidy += block_subsidy; | |||
There was a problem hiding this comment.
In commit e7b111a (refactor, index: Remove member variables in coinstatsindex):
for other reviewers: read_out.second.total_subidy is updated below:
bitcoin/src/index/coinstatsindex.cpp
Line 200 in e7b111a
src/kernel/coinstats.h
Outdated
| //! Amount of outputs sent to unspendable scripts (OP_RETURN for example) in this block | ||
| CAmount block_unspendables_scripts{0}; | ||
| //! Amount of coins lost due to unclaimed miner rewards in this block | ||
| CAmount block_unspendables_unclaimed_rewards{0}; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| LogWarning("previous block header belongs to unexpected block %s; expected %s", | ||
| read_out.first.ToString(), expected_block_hash.ToString()); | ||
| m_current_block_hash.ToString(), expected_block_hash.ToString()); |
There was a problem hiding this comment.
Shouldn't this be an error even if you can find the previous block header? You'll calculate incorrect values for total_amount otherwise?
There was a problem hiding this comment.
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.
src/index/coinstatsindex.cpp
Outdated
| CAmount total_unspendables_scripts; | ||
| CAmount total_unspendables_unclaimed_rewards; | ||
| CAmount block_prevout_spent_amount; | ||
| CAmount block_new_outputs_ex_coinbase_amount; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
static inline void cap_add(CAmount& v, CAmount x)
{
if (std::numeric_limits<CAmount>::max() - x < v) {
v = std::numeric_limits<CAmount>::max();
} else {
v += x;
}
}
...
cap_add(value.second.block_new_outputs_ex_coinbase_amount, coin.out.nValue);
...
cap_add(value.second.block_prevout_spent_amount, coin.out.nValue);Or could combine the two components:
value.second.block_net_spent += coin.out.nValue; // for each vin
value.second.block_net_spent -= coin.out.nValue; // for each voutwhich 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.
|
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? |
dd7e696 to
5978a26
Compare
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. |
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
There was a problem hiding this comment.
Did another round of review, and everything looked good to me.
I would prefer to have ef01a19 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.
| if (is_coinbase && IsBIP30Unspendable(block.hash, block.height)) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
it would be good to add a little explanation to this commit 4a72b62 - 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.
Semantically this is the correct name for what the function is doing.
Also marks a few additional variables const.
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 gets rid of the explicit total unspendable tracking which can be calculated by adding the four categories of unspendables together.
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.
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.
5978a26 to
f013fbf
Compare
|
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
Done in #33306.
Done. |
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 replaces a unnecessary copy with a reference.
|
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. |
There was a problem hiding this comment.
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.
a diff
we should probably make it trivially copyable, but this seems unrelated.
diff --git a/src/arith_uint256.h b/src/arith_uint256.h
index 0cf7aa44b0..14e09c3a6a 100644
--- a/src/arith_uint256.h
+++ b/src/arith_uint256.h
@@ -37,21 +37,6 @@ public:
pn[i] = 0;
}
- base_uint(const base_uint& b)
- {
- for (int i = 0; i < WIDTH; i++)
- pn[i] = b.pn[i];
- }
-
- base_uint& operator=(const base_uint& b)
- {
- if (this != &b) {
- for (int i = 0; i < WIDTH; i++)
- pn[i] = b.pn[i];
- }
- return *this;
- }
-
base_uint(uint64_t b)
{
pn[0] = (unsigned int)b;| @@ -153,7 +153,7 @@ FUZZ_TARGET(utxo_total_supply) | |||
| node::RegenerateCommitments(*current_block, chainman); | |||
| const bool was_valid = !MineBlock(node, current_block).IsNull(); | |||
|
|
|||
| const auto prev_utxo_stats = utxo_stats; | |||
| const auto& prev_utxo_stats = utxo_stats; | |||
There was a problem hiding this comment.
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.
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 current approach opts for a simple solution: The new version of the index goes into a separate location in the datadir (
index/coinstatsindex/rather thanindex/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. Beyond v31 the old index will be automatically deleted.