index: Improve robustness of coinstatsindex at restart #24133

1. 
   fjahr commented at 4:33 pm on January 23, 2022: member

   This change lets the coinstatsindex fail loudly in case the internal muhash state differs from the last finalized output saved on disk, which would indicate that the muhash state somehow got out of sync. This should generally not happen since both are written to disk in a batch but #24076 seems to indicate that the might still be an issue.

   Since #24076 so far can not be reproduced reliably, the issue should not be closed yet. Further investigation and testing needs to be done.

2. DrahtBot added the label UTXO Db and Indexes on Jan 23, 2022
3. 
   DrahtBot commented at 9:30 pm on January 23, 2022: member

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

   Conflicts

   No conflicts as of last run.

4. 
   mzumsande commented at 1:27 am on January 24, 2022: member

   Concept ACK.

   I was just able to locally reproduce the fails reported in #24076 and find the root cause - will report in more detail / possibly PR a fix tomorrow. This additional check makes sense in any case.

5. 
   mzumsande commented at 6:10 pm on January 24, 2022: member

   ACK 2fc159b02187a57980c67268cbf70b9998caa488 - I verified that init aborts now for a corrupted index, and doesn’t abort for an uncorrupted one.
6. Successahead approved
7. in src/index/coinstatsindex.cpp:371 in 2fc159b021 outdated

   362@@ -363,6 +363,13 @@ bool CoinStatsIndex::Init()
   363             return false;
   364         }
   365 
   366+        uint256 out;
   367+        m_muhash.Finalize(out);
   368+        if (entry.muhash != out) {
   369+            return error("%s: Cannot read current %s state; index may be corrupted",
   370+                         __func__, GetName());
   

   ---

   ---

   

   Sjors commented at 11:58 am on February 8, 2022:

   Should we also return an error if the LookUpOne above fails?

   ---

   

   fjahr commented at 6:48 pm on February 12, 2022:

   That’s done in #24117 , I expect both to get in soon so did not repeat the code here.
8. Sjors approved
9. 
   Sjors commented at 12:13 pm on February 8, 2022: member

   ACK 2fc159b02187a57980c67268cbf70b9998caa488

   Maybe also annotate MuHash3072 m_muhash; in the header to point out a MuHash is initialized to 1 by default.

10. DrahtBot added the label Needs rebase on Feb 15, 2022
11. index: remove txindex references from base index 38ed58b850
12. index: check muhash is in sync on coinstatsindex launch 820c03aff5
13. fjahr force-pushed on Feb 16, 2022
14. 
    fjahr commented at 11:43 pm on February 16, 2022: member

    Rebased
15. DrahtBot removed the label Needs rebase on Feb 17, 2022
16. 
    Sjors commented at 8:39 am on February 17, 2022: member

    re-ACK 820c03aff5295fff68a4577aa51667198036e372
17. 
    mzumsande commented at 6:46 pm on February 17, 2022: member

    re-ACK 820c03aff5295fff68a4577aa51667198036e372
18. in src/index/coinstatsindex.cpp:370 in 820c03aff5

    362@@ -363,6 +363,14 @@ bool CoinStatsIndex::Init()
    363             return error("%s: Cannot read current %s state; index may be corrupted",
    364                          __func__, GetName());
    365         }
    366+
    367+        uint256 out;
    368+        m_muhash.Finalize(out);
    369+        if (entry.muhash != out) {
    370+            return error("%s: Cannot read current %s state; index may be corrupted",
    

    ---

    ---

    

    ryanofsky commented at 7:57 pm on February 17, 2022:

    In commit “index: check muhash is in sync on coinstatsindex launch” (820c03aff5295fff68a4577aa51667198036e372)

    I wonder if it would be good to include hashes or other information in this message (transaction total numbers, pindex height) in the error message, especially if this is supposed to help debug an unresolved issue.

    ---

    

    ryanofsky commented at 8:06 pm on February 17, 2022:

    re: #24133 (review)

    I wonder if it would be good to include hashes or other information in this message

    Never mind if #24138 fixes this though

    ---

    

    fjahr commented at 10:43 am on February 20, 2022:

    I will address this together with your idea for a unit test here in a small follow-up.
19. ryanofsky approved
20. 
    ryanofsky commented at 8:03 pm on February 17, 2022: member

    Code review ACK 820c03aff5295fff68a4577aa51667198036e372. Good to catch the error earlier
21. fanquake merged this on Feb 20, 2022
22. fanquake closed this on Feb 20, 2022

    ---

23. sidhujag referenced this in commit d746aa5b1c on Feb 20, 2022
24. DrahtBot locked this on Feb 20, 2023

Contributors
fjahr DrahtBot mzumsande Successahead Sjors ryanofsky

Labels
UTXO Db and Indexes