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

Explorer add gc and confirmed block notion #2962

Merged
merged 9 commits into from
Feb 4, 2021

Conversation

ecioppettini
Copy link
Contributor

This adds the notion of epoch_stability_depth to the explorer. It's used both for multiverse garbage collection and to know if a block is confirmed.

Also, when working on this I found a problem with the way tip changes were handled in #2903

The thing is, tip changes are being sent to the explorer before having the block. This means, that we can't get the State at that particular hash, so we don't have the block and we can't know it's chain_length to find out which one is confirmed.

Also means that a query may come in the middle and try to use a branch not fully indexed and break things like @dkijania also found in #2950

This PR fixes it in a way that may be a bit hacky. It works by saving the tip update for the time the block comes, if it's not already there, so it doesn't care which one comes first.

A proper solution could be to send blocks to the explorer after knowing the branch solution result, but the problem is that by that point the node only has a Ref and not a Block, so that would require either carrying the block there, or sending the branch comparison result back (up in the stack trace).

An alternative could be to only send hashes, and get blocks from the storage, but I think this is blocking? and maybe not fast.

@ecioppettini ecioppettini added bug Something isn't working enhancement New feature or request A-explorer Area: Explorer API and backend labels Jan 27, 2021
@ecioppettini ecioppettini self-assigned this Jan 27, 2021
@@ -43,12 +43,9 @@ impl<T: Clone> Multiverse<T> {
}
}

impl Multiverse<Ledger> {
impl<T> Multiverse<T> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Huh? I was pretty sure I already did that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm, probably you did it the same with the actual multiverse in chain-libs

.chain_length()
.nth_ancestor(self.blockchain_config.epoch_stability_depth)
{
debug_assert!(
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just assert!? Might be difficult to track problems in production.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I just thought debug_assert was paranoid enough, and I wanted to write the invariant in some way at least. I don't really mind using assert anyway, but if this actually happens it is most likely a programmer error.

@ecioppettini ecioppettini merged commit 2d9ad4c into master Feb 4, 2021
@ecioppettini ecioppettini deleted the explorer-add-gc-and-confirmed-block-notion branch February 4, 2021 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-explorer Area: Explorer API and backend bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants