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

Resharding V3 - state witness, implementation #577

Merged
merged 3 commits into from
Nov 22, 2024
Merged

Conversation

Longarithm
Copy link
Member

Adding State Witness section and filling some empty sections.

@Longarithm Longarithm requested a review from a team as a code owner November 20, 2024 10:07
Copy link

@Trisfald Trisfald left a comment

Choose a reason for hiding this comment

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

🚀

neps/nep-0568.md Outdated
```text
[All NEPs that introduce backwards incompatibilities must include a section describing these incompatibilities and their severity. Author must explain a proposes to deal with these incompatibilities. Submissions without a sufficient backwards compatibility treatise may be rejected outright.]
```
Approach is fully backwards compatible, just adding new protocol upgrade on top of existing implementation. Also, we were able to completely remove previous resharding logic, as it was already approved by validators, and to process chunks from any layout, it is enough to take state from that layout from archival node.

Choose a reason for hiding this comment

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

We retained the capability to replay reshardingV2 in archive nodes, not sure if it's worth mentioning

Copy link
Member Author

Choose a reason for hiding this comment

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

I just deleted mention of this, as I don't have enough understanding.

neps/nep-0568.md Outdated
### Positive

* p1
* The protocol is able to execute resharding even while only a fraction of nodes track the split shard.
* New resharding can happen in the matter of minutes instead of hours.

Choose a reason for hiding this comment

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

Should we say the resharding happens "instantaneously" from the point of view of NEAR users?

Choose a reason for hiding this comment

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

Btw, is this section supposed to be filled by us or the NEP reviewers? I've seen it empty for a few NEPs.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, it happened instantaneously before as well. It's just that we need less "background compute power" now. I clarified it.

For NEP reviewers we have "Benefits" and "Concerns" sections below. It seems very reasonable to provide our initial view at pros and cons before review.

neps/nep-0568.md Outdated
and then it calls Trie(state_transition_proof).retain_split_shard(boundary_account) which should succeed if proof is sufficient and generates new state root
finally, it checks that the new state root matches the state root proposed in ChunkStateWitness. if the whole ChunkStateWitness is valid, then chunk validator sends endorsement which also endorses the resharding.

Choose a reason for hiding this comment

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

We may want to take out these long text lines from the code block, or newline them manually

Copy link
Contributor

@wacban wacban left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -163,6 +163,16 @@ supporting smooth transitions without altering storage structures directly.

### Stateless Validation

### State Witness
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a quick preface with very high level description? It feels like you're jumping right into the nitty gritty details.

neps/nep-0568.md Outdated
@@ -187,18 +197,39 @@ In this NEP, we propose updating the ShardId semantics to allow for arbitrary id

## Reference Implementation

```text
[This technical section is required for Protocol proposals but optional for other categories. A draft implementation should demonstrate a minimal implementation that assists in understanding or implementing this proposal. Explain the design in sufficient detail that:
```
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a short comment to each block? The pseudo code is nice but I think it would be good to also have something in human words ;)

Choose a reason for hiding this comment

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

And please also add a subsection heading if necessary?

Copy link

@shreyan-gupta shreyan-gupta left a comment

Choose a reason for hiding this comment

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

Btw, you might want to use markdown lint.

@Longarithm Longarithm merged commit 910cf6c into resharding Nov 22, 2024
1 check passed
@Longarithm Longarithm deleted the witness-etc branch November 22, 2024 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Shipped 🚀
Status: NEW
Development

Successfully merging this pull request may close these issues.

4 participants