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

ChainDB.Iterator: fix EBB bug #1475

Merged
merged 3 commits into from
Jan 21, 2020
Merged

ChainDB.Iterator: fix EBB bug #1475

merged 3 commits into from
Jan 21, 2020

Conversation

mrBliss
Copy link
Contributor

@mrBliss mrBliss commented Jan 17, 2020

Closes #1435.

See the comment for more information.

We were using the very simple `Test.Util.TestBlock` test block, which was
easier to generate, but could not represent EBBs.

Now by using the `Test.Storage.TestBlock` test block, we can test with EBBs.
In the past, `ImmDB.getPointAtTip` used to be more expensive (it had to read a
block from disk to get its hash). That's why we had `itGetImmDBTip`,
implemented using the in-memory chain fragment, in `IteratorEnv`.

The ImmDB (and ImmutableDB) keeps the hash of the block at the tip in memory,
so we can use `itImmDB` directly and no longer need `itGetImmDBTip`.

While at it, add `getTipInfo`, which also includes `IsEBB`, and use that to
inspect the tip of the `ImmDB`.
@mrBliss mrBliss added consensus issues related to ouroboros-consensus chain db labels Jan 17, 2020
@mrBliss mrBliss requested a review from nfrisby January 17, 2020 19:17
@mrBliss
Copy link
Contributor Author

mrBliss commented Jan 17, 2020

Many thanks to @nfrisby for providing such a detailed bug report!

Copy link
Contributor

@nfrisby nfrisby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love the thorough tests! I only selected "Request changes" because I'm not sure the fallbackTo is required, and if it is, then the comments as-is didn't address my confusion about that.

Closes #1435.

See the comment for more information.
@mrBliss
Copy link
Contributor Author

mrBliss commented Jan 20, 2020

You're right. I have rewritten the logic and made all 6 cases explicit (in the tests too). There was indeed no fallback to the VolatileDB needed.

@mrBliss mrBliss requested a review from nfrisby January 20, 2020 08:17
Copy link
Contributor

@nfrisby nfrisby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mrBliss
Copy link
Contributor Author

mrBliss commented Jan 21, 2020

bors r+

iohk-bors bot added a commit that referenced this pull request Jan 21, 2020
1475: ChainDB.Iterator: fix EBB bug r=mrBliss a=mrBliss

Closes #1435.

See the comment for more information.

Co-authored-by: Thomas Winant <[email protected]>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jan 21, 2020

@iohk-bors iohk-bors bot merged commit 652dac5 into master Jan 21, 2020
@iohk-bors iohk-bors bot deleted the mrBliss/fix-1435 branch January 21, 2020 06:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chain db consensus issues related to ouroboros-consensus
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ChainDB.Iterator incorrectly returns ForkTooOld when streaming to an EBB
2 participants