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

Add version of Voting.tla that can be analyzed by Apalache. #112

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

nano-o
Copy link
Contributor

@nano-o nano-o commented Jan 21, 2024

No description provided.

This includes an inductive invariant that establishes consistency.

Signed-off-by: Giuliano Losa <[email protected]>
@muenchnerkindl
Copy link
Collaborator

Thanks for adding this! See above for two minor remarks on comments in the TLA module. Also, the module should be added to the manifest for the CI to succeed.

@nano-o
Copy link
Contributor Author

nano-o commented Jan 21, 2024

@muenchnerkindl Thanks for having a look. I cannot find your remarks? Where are they?

/\ NoVoteAfterMaxBal
/\ Consistency

\* To install `^Apalache,^' see the `^Apalache^' website at `^https://apalache.informal.systems/^'
Copy link
Member

@lemmy lemmy Jan 21, 2024

Choose a reason for hiding this comment

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

This could additionally mention that Apalache is installed into this repo's devcontainer, which can also be used with, e.g., Gitpod or Codespaces.

@ahelwer
Copy link
Collaborator

ahelwer commented Jan 22, 2024

This looks good to me, interesting to learn that Apalache doesn't use a separate model file for constants but defines everything in one file.

@nano-o
Copy link
Contributor Author

nano-o commented Jan 22, 2024

This looks good to me, interesting to learn that Apalache doesn't use a separate model file for constants but defines everything in one file.

Maybe a better way would be to be to make a few changes to Voting.tla:

  • Rewrite ShowsSafeAt and SafeAt in Voting.tla to not use integer ranges (which Apalache does not support when the bounds are not constant).
  • Make Ballot a constant

Then I think VotingApalache could just instantiate Voting and make the necessary substitutions.

@lemmy
Copy link
Member

lemmy commented Jan 22, 2024

This looks good to me, interesting to learn that Apalache doesn't use a separate model file for constants but defines everything in one file.

Maybe a better way would be to be to make a few changes to Voting.tla:

  • Rewrite ShowsSafeAt and SafeAt in Voting.tla to not use integer ranges (which Apalache does not support when the bounds are not constant).
  • Make Ballot a constant

Then I think VotingApalache could just instantiate Voting and make the necessary substitutions.

Instead of changing Voting.tla, doesn't Apalache come with functionality that lets users redefine operators during model checking (similar to what TLC has)?

@muenchnerkindl
Copy link
Collaborator

@muenchnerkindl Thanks for having a look. I cannot find your remarks? Where are they?

@nano-o One comment was about a spurious "for" in the introductory comment, which I believe you removed already. The second one was about your remark about Ballot being used as the domain of a function: I don't see where you are pointing, and I suspect that the problem is rather the existential quantifier over ballots in the definition of ShowsSafeAt.

@nano-o
Copy link
Contributor Author

nano-o commented Jan 22, 2024

I tried to make it a little cleaner by moving the definitions of constants to a different file.

Instead of changing Voting.tla, doesn't Apalache come with functionality that lets users redefine operators during model checking (similar to what TLC has)?

Module instantiation is supported, but I don't think you can redefine things like you can with a .cfg file. @konnov is that correct?

@muenchnerkindl
Copy link
Collaborator

About making Ballot a constant, what properties would you have to require? You clearly need a linear order, anything else?

@nano-o
Copy link
Contributor Author

nano-o commented Jan 22, 2024

@nano-o One comment was about a spurious "for" in the introductory comment, which I believe you removed already. The second one was about your remark about Ballot being used as the domain of a function: I don't see where you are pointing, and I suspect that the problem is rather the existential quantifier over ballots in the definition of ShowsSafeAt.

You're right, this comment does not make sense.

Signed-off-by: Giuliano Losa <[email protected]>
@@ -0,0 +1,32 @@
--------------------------- MODULE MCVotingApalache -------------------------------
Copy link
Member

Choose a reason for hiding this comment

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

Elsewhere, we recently discussed TLA+ naming conventions. MC*Apalache is really verbose. Perhaps, we want to adopt and encourage a less verbose convention? IIRC, Apalache already prefixes some operators with Apa. So how about ApaMCVoting or APAMCVoting? Would then even be the same number of chars if users want to do APAMCVoting and TLCMCVoting. On the other hand, we might just as well drop MC.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The MC prefix seems superfluous in that case. In this particular example, do you have a suggestion for the name of the specification module? Voting2.tla and ApaVoting2.tla?

Copy link
Member

Choose a reason for hiding this comment

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

ApaVoting2 and TLCVoting2 looks like a sensible convention to me. If one want to distinguish between different modes, she can add a prefix like TLCSimVoting2 to tag this module for simulation.

By the way, the VSCode extension simulates a model/config pair whenever the prefix Smoke* is used. This happens when the editor is saved. Once we establish a convention, we can integrate similar functionalities like type checking.

Signed-off-by: Giuliano Losa <[email protected]>
@nano-o
Copy link
Contributor Author

nano-o commented Jan 22, 2024

About making Ballot a constant, what properties would you have to require? You clearly need a linear order, anything else?

I don't think Apalache supports uninterpreted types of unspecified cardinality. So you would have to explicitly define the set of ballots anyway.

@muenchnerkindl
Copy link
Collaborator

About making Ballot a constant, what properties would you have to require? You clearly need a linear order, anything else?

I don't think Apalache supports uninterpreted types of unspecified cardinality. So you would have to explicitly define the set of ballots anyway.

I understand that the question is not very relevant for model checking, but Voting.tla is also used for theorem proving, so it should state the necessary assumptions for the correctness proof to go through.

@nano-o
Copy link
Contributor Author

nano-o commented Jan 22, 2024

About making Ballot a constant, what properties would you have to require? You clearly need a linear order, anything else?

I don't think Apalache supports uninterpreted types of unspecified cardinality. So you would have to explicitly define the set of ballots anyway.

I understand that the question is not very relevant for model checking, but Voting.tla is also used for theorem proving, so it should state the necessary assumptions for the correctness proof to go through.

Sorry, I lost track of the context. I think that just assuming that Ballot is linearly ordered would be enough if, like in Voting2.tla, we do not use + and ranges (otherwise we would need assumptions about those too).

@konnov
Copy link
Contributor

konnov commented Jan 22, 2024

I tried to make it a little cleaner by moving the definitions of constants to a different file.

Instead of changing Voting.tla, doesn't Apalache come with functionality that lets users redefine operators during model checking (similar to what TLC has)?

Module instantiation is supported, but I don't think you can redefine things like you can with a .cfg file. @konnov is that correct?

I think you can replace constants with constant expressions in the .cfg, but it was long time since I tried that in Apalache. There is also this way to replace operators, but it is very Apalache-specific.

@lemmy
Copy link
Member

lemmy commented Jan 23, 2024

There is also this way to replace operators, but it is very Apalache-specific.

Did you create the prefix convention before introducing type annotations? It appears to me that using some annotation to defining overrides is more preferable than a prefix convention.

@konnov
Copy link
Contributor

konnov commented Jan 23, 2024

There is also this way to replace operators, but it is very Apalache-specific.

Did you create the prefix convention before introducing type annotations? It appears to me that using some annotation to defining overrides is more preferable than a prefix convention.

That's true. This convention was introduced way before annotations in comments. It's actually quite a good idea to do overrides in annotations.

Update: see apalache-mc/apalache#2818.

@shonfeder
Copy link

Instead of changing Voting.tla, doesn't Apalache come with functionality that lets users redefine operators during model checking (similar to what TLC has)?

Unfortunately Apalache cannot currently cope with Voting.tla as is, as explained in apalache-mc/apalache#2816 (comment)

However, it seems like like a small refactor may be enough to make it Apalache compatible?

@lemmy
Copy link
Member

lemmy commented Jan 23, 2024

However, it seems like like a small refactor may be enough to make it Apalache compatible?

Refactoring of Apalache or Voting.tla? In my opinion, TLA+ examples should not be customized for a specific tool unless it is limited to a model checking (MC) file. Specifications are primarily meant to be readable by humans.

@shonfeder
Copy link

shonfeder commented Jan 23, 2024

For Apalache we need to add a new feature. The refactor would be of the Voting spec.

The 5 line refactor I propose in the link would not impact readability afaict. But I have no opinion here nor an agenda. Just sharing the current limitations and the known workaround.

Cheers.

@muenchnerkindl
Copy link
Collaborator

Refactoring of Apalache or Voting.tla? In my opinion, TLA+ examples should not be customized for a specific tool unless it is limited to a model checking (MC) file. Specifications are primarily meant to be readable by humans.

I completely agree. However, separating the proof in Consensus.tla from the specification module (and importing WellFoundedInduction.tla only in the proof) would IMHO be acceptable if this is enough to fix the problem. In fact, it would make sense to add the Paxos proofs to this repo: currently, they are found in the TLAPS distribution where they are much less visible. This would require some light refactoring so that specs and proofs are aligned.

@nano-o
Copy link
Contributor Author

nano-o commented Jan 23, 2024

@muenchnerkindl if you want to do that, make a different PR and then I'll rebase on yours.

@ahelwer
Copy link
Collaborator

ahelwer commented Feb 20, 2024

@muenchnerkindl @nano-o is this still blocked? I'm happy to do a bit of work to get this unblocked if so. Stephan I just need to take the proofs from the TLAPS repo for Paxos and get them set up here?

@nano-o
Copy link
Contributor Author

nano-o commented Feb 22, 2024

@ahelwer this is blocked until we factor out the proofs that cause Apalache to fail.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants