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

Update LayerStatus for 0.2 Consensus Changes #156

Closed
wants to merge 1 commit into from
Closed

Conversation

avive
Copy link
Contributor

@avive avive commented Jul 12, 2021

Add new layer statuses to conform with consensus protocol changes. Refactor this change from api-related changes in Api 2.0 PR.

@avive avive requested a review from lrettig July 12, 2021 07:54
@avive avive mentioned this pull request Jul 28, 2021
@lrettig
Copy link
Member

lrettig commented Jul 28, 2021

I'm a bit confused about this PR. I thought we already agreed (more or less) upon an updated spec here, for API 2.0? #144 (comment)

Why not just go with this?

@avive
Copy link
Contributor Author

avive commented Jul 28, 2021

this is for spacemesh 0.2 and not for 0.3 hence a separate pr and should go into 1.x and not into 2.0. It has been a while since we discussed this but as far as i remember we reviewed this together and agrees that this reflects 0.2 consensus.

@lrettig
Copy link
Member

lrettig commented Jul 29, 2021

this reflects 0.2 consensus

Could you refresh my memory? Is there a thread I can review?

confirmed by tortoise
verified by tortoise

what's the difference between these two?

@avive
Copy link
Contributor Author

avive commented Jul 29, 2021

The answer depends on possible layer states in the node because the API just need to reflect them. We need to get someone who's familiar with latest Tortoise implementation to chime in on this. It is my understanding that the node has a separate notions of verified and confirmed and also about layers it knows about which are not verifier or confirmed - pending. @noamnelke - what's the deal here?

@lrettig
Copy link
Member

lrettig commented Jul 29, 2021

I'm quite familiar with the latest Tortoise implementation :) From the perspective of Tortoise, a layer can only have two states, unverified or verified. (You could maybe posit a third, but I don't see what we gain from it: tried to verify, and failed, and will try again later.)

You may be thinking of the case where Hare finishes for a layer which hasn't been verified by Tortoise yet, but this is already accounted for in the API.

@avive
Copy link
Contributor Author

avive commented Jul 30, 2021

I'm quite familiar with the latest Tortoise implementation :) From the perspective of Tortoise, a layer can only have two states, unverified or verified. (You could maybe posit a third, but I don't see what we gain from it: tried to verify, and failed, and will try again later.)

Cool, so, are you 100% sure that there is no separate verified and confirmed tortoise layer states in the protocol 0.2? If not then we don't need this change, if yes, then we do.

You may be thinking of the case where Hare finishes for a layer which hasn't been verified by Tortoise yet, but this is already accounted for in the API.

If a layer is not yet approved by hare but known to node that it won't be reported by the 0.1 api. There can be a layer in this state known to node which will not be reported via the 0.1 api as it doesn't have a state for it.

@lrettig
Copy link
Member

lrettig commented Jul 31, 2021

This is not necessary for SM 0.2. Going to go ahead and close this. Let me know if I've missed anything!

@lrettig lrettig closed this Jul 31, 2021
@lrettig lrettig deleted the layer-status-0.3 branch July 31, 2021 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants