index: Commit MuHash and best block together for coinstatsindex #24138

mzumsande · 2022-01-24T14:10:20Z

Coinstatsindex currently writes the MuHash (DB_MUHASH) to disk in CoinStatsIndex::WriteBlock() and CoinStatsIndex::ReverseBlock(), but the best synced block is written in BaseIndex::Commit(). These are called at different points in time, both during the ThreadSync phase, and also after the initial sync is finished and validation callbacks (BlockConnected() vs ChainStateFlushed()) perform the syncing.

As a result, the index DB is temporarily in an inconsistent state, and if bitcoind is terminated uncleanly (so that there is no time to call Commit() by receiving an interrupt or by flushing the chainstate) this leads to problems:
On the next startup, Init() will read the best block and a MuHash that corresponds to a different (higher) block. Indexing will be picked up at the the best block processing some blocks again, but since MuHash is a rolling hash, it will process some utxos twice and the muhashes for all future blocks will be wrong, as was observed in #24076.

Fix this by always committing DB_MUHASH together with DB_BEST_BLOCK.

Note that the block data for the index is still written at different times, but this does not corrupt the index - at worst, these entries will be processed another time and overwritten after an unclean shutdown and restart.

mzumsande · 2022-01-24T14:10:47Z

FYI @fjahr

DrahtBot · 2022-01-26T00:58:52Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#24230 (indexes: Stop using node internal types and locking cs_main, improve sync logic by ryanofsky)
#24150 (refactor: move index class members from protected to private by jonatack)

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.

luke-jr · 2022-01-29T20:59:12Z

src/index/coinstatsindex.cpp

-    CDBBatch batch(*m_db);
-    batch.Write(DBHeightKey(pindex->nHeight), value);
-    batch.Write(DB_MUHASH, m_muhash);
-    return m_db->WriteBatch(batch);


I don't see why this should change. The CommitInternal override feels like a hacky refactor.

I think this is spot is problematic, and the reason for the behavior reported in #24076:

If we persist DB_MUHASH here in each BlockConnected() callback, but not DB_BEST_BLOCK (which is persisted only in ChainStateFlushed() callbacks and will therefore at this point in time have a lower height), the db is temporarily in an inconsistent state.

So in case of an unclean shutdown (with no time to flush the chainstate to disk to get in sync again), on next startup the index will initialize with the older DB_BEST_BLOCK but a DB_MUHASH which contains data from newer blocks. The index then reprocesses the blocks after DB_BEST_BLOCK's height a second time.
But MuHash is a rolling hash, so it will incorporate data from these blocks twice, and the hash will be incorrect.

I think it was not considered hacky by the author of the current index architecture because it's done the same way in blockfilterindex.cpp and the documentation in index/base.h also states that this method can be overridden.

Obviously, I did it differently here but I can't remember if that was a deliberate choice or rather just lack of understanding (probably the latter).

And that doesn't mean there isn't mayor room for refactoring here. But it looks like #24230 should be considered first when thinking about it first. I have not started reviewing it yet.

In commit "index: Commit DB_MUHASH and DB_BEST_BLOCK to disk together" (010156c)

The CommitInternal override feels like a hacky refactor.

I think I agree with luke if the point is that the override-and-call-base class code in CommitInternal is a little hacky, but fjahr's right this just the way the base index class was designed, so the override seems like appropriate way to handle this now. #24230 does clean up all the overrides and will simplify this a bit.

Just to make this clear: This is not a refactor, it changes behavior (the points in time when DB_MUHASH is written to disk).

My understanding is that the indexes work in general such that that all persisted data describing the current progress of indexing (such as the best block) must be in sync with the persisted chainstate information (which is sometimes not flushed for several blocks received) - that is why ChainStateFlushed() callbacks are used. In case of an unclean shutdown, the chainstate will be constructed from this point on, so the indexes must as well.

In contrast to this is the actual indexed data for each block - I think we could in principle cache it and also write it to disk with each ChainStateFlushed() callback and not use BlockConnected() at all.

But it is also possible to write it to disk in advance with each BlockConnected() callback, as it is done:
This is dependent on the assumption that we will not read (and possibly overwrite) the indexed data in case it is ahead of the flushed chainstate data (and the best block), and also that before flushing the chainstate, we have called BlockConnected() for each block.

With this in mind, I don't see a better way of doing this other than overriding CommitInternal.

luke-jr · 2022-01-29T21:00:08Z

src/index/coinstatsindex.cpp

@@ -481,5 +486,5 @@ bool CoinStatsIndex::ReverseBlock(const CBlock& block, const CBlockIndex* pindex
    Assert(m_total_unspendables_scripts == read_out.second.total_unspendables_scripts);
    Assert(m_total_unspendables_unclaimed_rewards == read_out.second.total_unspendables_unclaimed_rewards);

-    return m_db->Write(DB_MUHASH, m_muhash);


Seems like the only problem is this lone line?

If I replace only that line with true (or false for that matter) the reorg test in feature_coinstatsindex.py still passes. But not sure if that helps with the original issue (and it indicates an obvious test coverage omission, cc @fjahr).

Only replacing this line means the rolling muhash state is not persisted during reorgs. That would be a problem if the node gets shut down during the reorg, then we would risk an inconsistent state or probably the index wouldn't even start up anymore because the guard check in Init() would fail.

if the node gets shut down during the reorg,

I guess this is very non trivial to test, but maybe add a comment?

I added a comment at the end of CoinStatsIndex::WriteBlock (as suggested by ryanofsky below). I think that duplicating this comment in CoinStatsIndex::ReverseBlock is not required, since this function doesn't write anything else to disk there wouldn't really be the expectation that for it to write DB_MUHASH here.

Sjors

Concept ACK

It might be worth rebasing this on #24133 and if at all possible, add a test that triggers the error added there.

I think overriding CommitInternal makes sense given that we require DB_MUHASH and DB_BEST_BLOCK to be in sync (which we explicitly check as of #24133).

The alternative would be to drop DB_MUHASH altogether, but since MuHash can't (?) be initialized from a finalized hash, we would have to index both the denominator and numerator.

fjahr

Code review ACK 010156c

While I can not reproduce the issue I agree that this is likely to fix #24076.

I think there could still be inconsistencies between the running muhash and the coinstats data since those are not batched anymore now but at least that would then lead to a failure in Init() at restart. This could be avoided by squashing all of that into CommitInternal() as well (drafted here) but I don't think this is necessary right now. As I have reviewed this I am now really not sure about the boundaries between Commit() and WriteBlock() anymore and will think about refactoring that while reviewing #24230 (maybe that's already solved there).

fjahr · 2022-02-12T19:44:24Z

src/index/coinstatsindex.cpp

@@ -481,5 +486,5 @@ bool CoinStatsIndex::ReverseBlock(const CBlock& block, const CBlockIndex* pindex
    Assert(m_total_unspendables_scripts == read_out.second.total_unspendables_scripts);
    Assert(m_total_unspendables_unclaimed_rewards == read_out.second.total_unspendables_unclaimed_rewards);

-    return m_db->Write(DB_MUHASH, m_muhash);


Only replacing this line means the rolling muhash state is not persisted during reorgs. That would be a problem if the node gets shut down during the reorg, then we would risk an inconsistent state or probably the index wouldn't even start up anymore because the guard check in Init() would fail.

fjahr · 2022-02-12T20:30:13Z

src/index/coinstatsindex.cpp

-    CDBBatch batch(*m_db);
-    batch.Write(DBHeightKey(pindex->nHeight), value);
-    batch.Write(DB_MUHASH, m_muhash);
-    return m_db->WriteBatch(batch);


I think it was not considered hacky by the author of the current index architecture because it's done the same way in blockfilterindex.cpp and the documentation in index/base.h also states that this method can be overridden.

Obviously, I did it differently here but I can't remember if that was a deliberate choice or rather just lack of understanding (probably the latter).

And that doesn't mean there isn't mayor room for refactoring here. But it looks like #24230 should be considered first when thinking about it first. I have not started reviewing it yet.

ryanofsky

Code review ACK 010156c

It would be good to add a unit test to coinstatsindex_tests that triggers the issue this is supposed to fix. I think the test would just need to make the index object, send a BlockConnected notification, and then destroy the index object without sending ChainStateFlushed notification. This would save a corrupted index that should result in errors next time it is loaded.

re: #24138 (review)

I think there could still be inconsistencies between the running muhash and the coinstats data since those are not batched anymore now but at least that would then lead to a failure in Init() at restart.

I could be wrong but this seems fine to me since coinstats data is accessed by height and written before the muhash & best block. So if the coinstats data is ahead of the muhash. it is just ignored. In your suggested followup a90394e, I think the if (!m_db->Read(DBHeightKey(pindex->nHeight)) check would be good to add, but I think it should it should just trigger a fatal error if the key isn't found. I don't see how the recovery code added in that commit would work.

#24230 (maybe that's already solved there).

My guess is #24230 doesn't affect this, since it's trying to leave index's internal behavior the same, and just change the way it receives block connected and disconnected notifications, without changing the way it handles them.

ryanofsky · 2022-02-17T20:17:30Z

src/index/coinstatsindex.cpp

-    CDBBatch batch(*m_db);
-    batch.Write(DBHeightKey(pindex->nHeight), value);
-    batch.Write(DB_MUHASH, m_muhash);
-    return m_db->WriteBatch(batch);


In commit "index: Commit DB_MUHASH and DB_BEST_BLOCK to disk together" (010156c)

The CommitInternal override feels like a hacky refactor.

I think I agree with luke if the point is that the override-and-call-base class code in CommitInternal is a little hacky, but fjahr's right this just the way the base index class was designed, so the override seems like appropriate way to handle this now. #24230 does clean up all the overrides and will simplify this a bit.

src/index/coinstatsindex.cpp

If these are written to disk at different times, unclean shutdowns can lead to index corruption.

mzumsande · 2022-02-21T16:51:50Z

I pushed an update, rebasing (with #24133 merged) and addressing feedback (adding a comment).

It would be good to add a unit test to coinstatsindex_tests that triggers the issue this is supposed to fix. I think the test would just need to make the index object, send a BlockConnected notification, and then destroy the index object without sending ChainStateFlushed notification. This would save a corrupted index that should result in errors next time it is loaded.

This would be great. I haven't done this with this push, because
a) I couldn't make it work yet, I had some technical difficulties with the test framework when restarting the index. I will look into it more unless
b) @fjahr who said here that he is working on this has more success.

ryanofsky

Code review ACK eb6cc05. Only changes since previous review are rebasing and adding comments.

@mzumsande I wrote a little test which passes with this bugfix, and fails with the fix reverted. Feel free to add it to this PR or use the code in any way: ~~ded9fda~~ d3c2ebb

mzumsande · 2022-03-01T14:02:19Z

@mzumsande I wrote a little test which passes with this bugfix, and fails with the fix reverted. Feel free to add it to this PR or use the code in any way: ~~ded9fda~~ d3c2ebb

Thank you! The test looks great, I added your commit to this PR (and confirmed that it fails on master).

ryanofsky

Code review ACK 691d45f. Only change since last review is adding test

fjahr

ACK 691d45f

Reviewed code and ensured that the newly added test fails when eb6cc05 is reverted.

fjahr · 2022-03-06T13:47:05Z

src/test/coinstatsindex_tests.cpp

+    {
+        CoinStatsIndex index{1 << 20};
+        // Make sure the index can be loaded.
+        BOOST_REQUIRE(index.Start(chainstate));


in 691d45f:

Not sure why we need this whole block with the new index instead of reusing the old index above. It doesn't seem to make a difference in my testing?

Is there a huge cost to spinning up the second index? If not, I think both versions are fine.

… coinst…

mzumsande mentioned this pull request Jan 24, 2022

coinstatsindex way more fragile than any other data w.r.t. power loss #24076

Closed

mzumsande force-pushed the 202201_coinstatsindex_commits branch from 2c630fe to 010156c Compare January 24, 2022 14:19

DrahtBot added the UTXO Db and Indexes label Jan 24, 2022

DrahtBot mentioned this pull request Jan 26, 2022

refactor: move index class members from protected to private #24150

Closed

luke-jr suggested changes Jan 29, 2022

View reviewed changes

DrahtBot mentioned this pull request Feb 2, 2022

indexes: Stop using node internal types and locking cs_main, improve sync logic #24230

Draft

Sjors reviewed Feb 8, 2022

View reviewed changes

fjahr reviewed Feb 13, 2022

View reviewed changes

ryanofsky mentioned this pull request Feb 17, 2022

index: Improve robustness of coinstatsindex at restart #24133

Merged

ryanofsky approved these changes Feb 17, 2022

View reviewed changes

index: Commit DB_MUHASH and DB_BEST_BLOCK to disk together

eb6cc05

If these are written to disk at different times, unclean shutdowns can lead to index corruption.

mzumsande force-pushed the 202201_coinstatsindex_commits branch from 010156c to eb6cc05 Compare February 21, 2022 16:39

ryanofsky approved these changes Feb 22, 2022

View reviewed changes

Add coinstatsindex_unclean_shutdown test

691d45f

ryanofsky approved these changes Mar 2, 2022

View reviewed changes

fjahr reviewed Mar 6, 2022

View reviewed changes

maflcko merged commit 7003b6a into bitcoin:master Mar 9, 2022

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Mar 9, 2022

Merge bitcoin#24138: index: Commit MuHash and best block together for…

fea9761

… coinst…

bitcoin locked and limited conversation to collaborators Mar 9, 2023

index: Commit MuHash and best block together for coinstatsindex #24138

index: Commit MuHash and best block together for coinstatsindex #24138

Uh oh!

Conversation

mzumsande commented Jan 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mzumsande commented Jan 24, 2022

Uh oh!

DrahtBot commented Jan 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fjahr Feb 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sjors Feb 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

fjahr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fjahr Feb 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mzumsande commented Feb 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mzumsande commented Mar 1, 2022

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

fjahr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mzumsande commented Jan 24, 2022 •

edited

Loading

DrahtBot commented Jan 26, 2022 •

edited

Loading

fjahr Feb 12, 2022 •

edited

Loading

Sjors Feb 8, 2022 •

edited

Loading

fjahr Feb 12, 2022 •

edited

Loading

mzumsande commented Feb 21, 2022 •

edited

Loading

ryanofsky left a comment •

edited

Loading