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

Change the way SPO votes are counted #594

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

williamdemeo
Copy link
Contributor

@williamdemeo williamdemeo commented Oct 11, 2024

Description

This closes issue #578.

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • Any semantic changes to the specifications are documented in CHANGELOG.md
  • Code is formatted according to CONTRIBUTING.md
  • Self-reviewed the diff

@williamdemeo williamdemeo linked an issue Oct 11, 2024 that may be closed by this pull request
(It's unclear to me whether this is the intent of the WIP section of the
CHANGELOG, or whether I should have left this issue in the 0.9 section.)
@williamdemeo williamdemeo marked this pull request as ready for review October 11, 2024 22:03
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@WhatisRT WhatisRT left a comment

Choose a reason for hiding this comment

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

Sorry, what actually needs to be done here is a bit more complicated. DState.voteDelegs needs to be available to actualSPOVotes. Then, if you have a credVoter SPO c, you need to lookup c in voteDelegs and the result of that lookup is the default vote.

@williamdemeo
Copy link
Contributor Author

@WhatisRT @Lucsanszky I've revised the SPO default vote function. I think it's correct now, but let me know if you notice anything wrong with it. Thanks!

@Lucsanszky
Copy link
Contributor

Lucsanszky commented Oct 17, 2024

@WhatisRT @williamdemeo conformance is still failing for me. I had a better look at the ledger implementation (here's my PR which updates the SPO vote counting: IntersectMBO/cardano-ledger#4659) and the spec. To me it seems like that they indeed don't really match. In the implementation, we check if the SPO's reward address is delegated to an AlwaysAbstain or an AlwaysNoConfidence DRep. See: https://github.com/IntersectMBO/cardano-ledger/pull/4659/files#diff-d479e8ce99ea392464360b8cea0bb7c42e04863929fd3f4ae4c92dd5742e535cR222-R223
That is, on top of DState.voteDelegs, PState.pools needs to be available to actualSPOVotes as well, so we can first lookup the pool parameters of the given pool in pools, grab the reward address and look that up in voteDelegs. Unless:

  1. My implementation is wrong.
  2. I misunderstand something here as I'm not too familiar with Agda and the spec and somehow the c in credVoter SPO c is the reward address of the given pool.

@williamdemeo
Copy link
Contributor Author

williamdemeo commented Oct 18, 2024

@Lucsanszky Your description seems reasonable to me. I'll go ahead and try to change the Agda code to match your understanding of how the policy should be implemented. I'll let you know when the changes are done.

@williamdemeo
Copy link
Contributor Author

@Lucsanszky I believe Ratify.lagda now agrees with the implementation as you describe it above. Thanks for the feedback. Let me know how the conformance tests go.

@Lucsanszky
Copy link
Contributor

At a first glance, this looks good! Unfortunately, conformance is still failing but that could be because of something else (and I have an idea why). Anyways, I'll wait for @WhatisRT's approval. Thanks!

@Lucsanszky
Copy link
Contributor

At a first glance, this looks good! Unfortunately, conformance is still failing but that could be because of something else (and I have an idea why). Anyways, I'll wait for @WhatisRT's approval. Thanks!

Okay, conformance passes. :) I opened a PR which also adds your changes to the conformance files, feel free to cherry pick the commits. Also, I took the liberty to slightly change your specification and opted not to pass in the complete CertState to RatifyEnv. I think it's better to be more specific with what we need and also helps with the conformance tests on the cardano-ledger side.

@williamdemeo williamdemeo self-assigned this Oct 21, 2024
@williamdemeo
Copy link
Contributor Author

Looks good. Thanks @Lucsanszky! I've merged your mods into this PR, so you can close your PR.

@Lucsanszky Lucsanszky mentioned this pull request Oct 21, 2024
4 tasks
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.

Change the way SPO votes are counted
3 participants