index: Improve robustness of coinstatsindex at restart #24133
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
|
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. |
|
ACK 2fc159b - I verified that init aborts now for a corrupted index, and doesn't abort for an uncorrupted one. |
2fc159b to
820c03a
Compare
|
Rebased |
|
re-ACK 820c03a |
1 similar comment
|
re-ACK 820c03a |
| uint256 out; | ||
| m_muhash.Finalize(out); | ||
| if (entry.muhash != out) { | ||
| return error("%s: Cannot read current %s state; index may be corrupted", |
There was a problem hiding this comment.
In commit "index: check muhash is in sync on coinstatsindex launch" (820c03a)
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.
There was a problem hiding this comment.
re: #24133 (comment)
I wonder if it would be good to include hashes or other information in this message
Never mind if #24138 fixes this though
There was a problem hiding this comment.
I will address this together with your idea for a unit test here in a small follow-up.
…estart 820c03a index: check muhash is in sync on coinstatsindex launch (Fabian Jahr) 38ed58b index: remove txindex references from base index (Fabian Jahr) Pull request description: 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 bitcoin#24076 seems to indicate that the might still be an issue. Since bitcoin#24076 so far can not be reproduced reliably, the issue should not be closed yet. Further investigation and testing needs to be done. ACKs for top commit: Sjors: re-ACK 820c03a mzumsande: re-ACK 820c03a ryanofsky: Code review ACK 820c03a. Good to catch the error earlier Tree-SHA512: 3c985d7152698d25bad95d4ad512ff87dff13fabef790589c5a6cf93ca4251ad599e12feb7251a084503e2a213b022eaacfbaaa601464114ad372b029f64f204
This change lets the
coinstatsindexfail loudly in case the internalmuhashstate differs from the last finalized output saved on disk, which would indicate that themuhashstate 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.