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

TLA+: Updating the fastsync spec after model checking #466

Merged
merged 34 commits into from
Aug 7, 2020

Conversation

konnov
Copy link
Contributor

@konnov konnov commented Jul 21, 2020

This is a new version that:

  • introduces a more refined block model that allows us to check the protocol invariants such as CorrectBlocksInv
  • fixes the block execution logic, so the faulty nodes cannot inject corrupted blocks
  • goes through the apalache model checker.

@konnov konnov requested a review from josef-widder July 21, 2020 12:02
@josef-widder
Copy link
Member

Should we add tags?

@liamsi
Copy link
Member

liamsi commented Jul 24, 2020

This is a new version

This indicates there is a older version that this PR is updating? The diff is indicating that this is mostly new code (added files only with a tiny exception). I'll add @brapse as a reviewer as he is likely the most familiar with fast-sync here.

I'm happy to review this as well but I think I need someone to walk me through this. It's a really large change and I'm neither deeply familiar with tla nor with fast-sync.

@liamsi liamsi requested a review from brapse July 24, 2020 08:43
@konnov
Copy link
Contributor Author

konnov commented Jul 28, 2020

Right. There are plenty of files. The changes to the protocol spec can be found in fastsync.tla, whereas the other files are harness for model checking. There a few interesting changes in the protocol that are related to hashes and using a reference chain, which is modeled in Tinychain.tla.

josef-widder
josef-widder previously approved these changes Jul 29, 2020
Copy link
Member

@josef-widder josef-widder left a comment

Choose a reason for hiding this comment

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

Great specification and model checking results! I think it would be good to write a small markdown file that explains the hash abstraction. I looked at the spec some weeks ago and understood it back then, and now struggled to get it back into my brain. I would be happy work contribute to that.

docs/spec/fastsync/Tinychain.tla Show resolved Hide resolved
docs/spec/fastsync/Tinychain.tla Outdated Show resolved Hide resolved
docs/spec/fastsync/Tinychain.tla Show resolved Hide resolved
docs/spec/fastsync/Tinychain.tla Outdated Show resolved Hide resolved
docs/spec/fastsync/Tinychain.tla Outdated Show resolved Hide resolved
docs/spec/fastsync/Tinychain.tla Show resolved Hide resolved
docs/spec/fastsync/fastsync.tla Outdated Show resolved Hide resolved
docs/spec/fastsync/fastsync.tla Outdated Show resolved Hide resolved
docs/spec/fastsync/fastsync.tla Show resolved Hide resolved
docs/spec/fastsync/fastsync.tla Outdated Show resolved Hide resolved
@josef-widder josef-widder changed the title Updating the fastsync spec after model checking TLA+: Updating the fastsync spec after model checking Aug 7, 2020
@josef-widder josef-widder self-requested a review August 7, 2020 13:58
josef-widder
josef-widder previously approved these changes Aug 7, 2020
Copy link
Member

@josef-widder josef-widder left a comment

Choose a reason for hiding this comment

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

looks great!

Copy link
Member

@josef-widder josef-widder left a comment

Choose a reason for hiding this comment

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

Thanks for addressing my comments. Looks great!

@josef-widder josef-widder merged commit a124e21 into master Aug 7, 2020
@josef-widder josef-widder deleted the igor/fastsync-mc branch August 7, 2020 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants