Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: ImmutableDB Error handling #426

Merged
merged 7 commits into from
Jun 29, 2024

Conversation

Mr-Leshiy
Copy link
Contributor

@Mr-Leshiy Mr-Leshiy commented Mar 28, 2024

Fixed an issue which occur when reading a snapshot data with the inconsistent index data.
If this appears, stops the reader on the last successful read operation and log it.

This happened with the preprod-e133-i2582.4d4c207fbbdd10a3edb487f57670f4091e96fb8c9fae16ac98bdce20e78dd6b4 snapshot downloaded from aggregator

https://storage.googleapis.com/cdn.aggregator.release-preprod.api.mithril.network/preprod-e133-i2582.4d4c207fbbdd10a3edb487f57670f4091e96fb8c9fae16ac98bdce20e78dd6b4.tar.zst

@scarmuega
Copy link
Member

Thanks @Mr-Leshiy for the PR. Please help me understand the rationale.

If I understand correctly, this is not an expected state of the data. I imagine this is an edge-case that occurs if the snapshot is generated from a running node. Is that the case?

If so, I believe that the appropriate behavior should be to track the error and let the lib consumer decide what to do. Either to skip the issue with a warning (as seems to be your requirement) or to handle the exception in some other way.

So my concrete questions are:

  • can you achieve your particular requirement (skip + warn) without modifying the current Pallas behavior?
  • Is there a reason to assume that this logic should be the default behavior?

@scarmuega scarmuega self-assigned this Apr 1, 2024
@scarmuega scarmuega added the needs more info We need more information to triage the issue label Apr 1, 2024
@Mr-Leshiy
Copy link
Contributor Author

Mr-Leshiy commented Apr 4, 2024

@scarmuega

If I understand correctly, this is not an expected state of the data. I imagine this is an edge-case that occurs if the
snapshot is generated from a running node. Is that the case ?

Yes, seems so. So basically I have dowloaded a mithril snapshot from that link which I have provided in description.
So it is a signed and verified mithril snapshot, which if I am not mistaken, generated by the node.
From what I have seen from the node source code of this immutable storage, it is possible that this data could be inconsistent
https://github.com/IntersectMBO/ouroboros-consensus/blob/2a4f737782a432c997d46d8e84d66ad8da20324a/ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/ImmutableDB/Impl/Validation.hs#L290.

If so, I believe that the appropriate behavior should be to track the error and let the lib consumer decide what to do.

Either to skip the issue with a warning (as seems to be your requirement) or to handle the exception in some other way.
Yea it make sense, let me try how it will work in our case. So instead of print a warn! I will return a specified error notifying what is happening (rather than just std::io::Error`).

@Mr-Leshiy Mr-Leshiy changed the title fix: Secondary reader could read with the inconsistent primary index chore: ImmutableDB Error handling May 8, 2024
@Mr-Leshiy
Copy link
Contributor Author

@scarmuega Sorry for the delay.
I've updated this PR. Updated the way of error handling, specified a specific error types and descriptions for every case.
Most probable I haven't cover all the cases and not provided an accurate description but this approach will allow to hide an internal implementation of this module from the user and provides an extensive description what is really happening.

@Mr-Leshiy
Copy link
Contributor Author

@scarmuega do you require any other changes or clarification to this PR?

@scarmuega scarmuega merged commit 07033eb into txpipe:main Jun 29, 2024
@Mr-Leshiy Mr-Leshiy deleted the fix/immutable-secondary branch July 1, 2024 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs more info We need more information to triage the issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants