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

Cannot register and deregister (or the other way) the same stake credential in the same transaction #512

Open
2 tasks
WhatisRT opened this issue Jul 15, 2024 · 5 comments · May be fixed by #547
Open
2 tasks
Assignees
Labels
bug Something isn't working conformance era: conway
Milestone

Comments

@WhatisRT
Copy link
Collaborator

WhatisRT commented Jul 15, 2024

This is because the deposits are updated in UTXO, so during CERT the deposits map is not being updated and only at most one of those certs will be valid.

Here are some ways of fixing this:

  • Add a temporaryDeposits field to CertState, that is updated and properly but then discarded
  • Check the correctness of refunds in UTXO, maybe during updateCertDeposits
@WhatisRT WhatisRT added bug Something isn't working era: conway conformance labels Jul 15, 2024
@WhatisRT WhatisRT added this to the May - Jul milestone Jul 15, 2024
@TimSheard
Copy link

TimSheard commented Jul 18, 2024

Consider this work around.

newtype DeltaDeposit = DeltaDeposit (Map DepositPurpose DeltaCoin)
emptyDD = DeltaDepost Map.empty
delta :: DeltaDeposit ->Cert -> DeltaDeposit
addDD, subDD :: DeltaDeposit -> CertState -> CertState
To add and subtract the changes to the CertState

In a transaction with N certs, we let deltaDD = foldl' delta emptyDD certs
In the conformance test, in the CERTS rule we subtract the delta deposits (since the Agda rule didn't add them)
In the UTXO rule we add the delta deposits, since the Agda rule added them but the STS rule did not.

@TimSheard
Copy link

TimSheard commented Jul 19, 2024

  1. Define DepositPurpose (Teodora)
  2. Define DeltaDeposit (Teodora)
  3. CertState -> DeltaDeposit (all the deposits) (Teodora)
  4. write addDD and subDD (maybe CertState is not the right State since Proposal deposits may not be in there) (Tim)
  5. Generate a ExecSpecRule Environment and ExecSpecRule State that align on deposits (Tim)
  6. write code for conformance testing that uses this stuff to equate the STS State and the ExexRuleSpec state (??)

@TimSheard
Copy link

TimSheard commented Jul 22, 2024

Something strange going on here. There are 4 constructors in the DELEG signal.

data ConwayDelegCert c
  = -- | Register staking credential. Deposit, when present, must match the expected deposit
    -- amount specified by `ppKeyDepositL` in the protocol parameters.
    ConwayRegCert !(StakeCredential c) !(StrictMaybe Coin)
  | -- | De-Register the staking credential. Deposit, if present, must match the amount
    -- that was left as a deposit upon stake credential registration.
    ConwayUnRegCert !(StakeCredential c) !(StrictMaybe Coin)
  | -- | Redelegate to another delegatee. Staking credential must already be registered.
    ConwayDelegCert !(StakeCredential c) !(Delegatee c)
  | -- | This is a new type of certificate, which allows to register staking credential
    -- and delegate within a single certificate. Deposit is required and must match the
    -- expected deposit amount specified by `ppKeyDepositL` in the protocol parameters.
    ConwayRegDelegCert !(StakeCredential c) !(Delegatee c) !Coin

But the spec only has 2 rules for DELEG Figure 22 page 25. why is that? Are we not showing some of the rules that already appear in earlier Eras?

williamdemeo added a commit that referenced this issue Aug 16, 2024
@williamdemeo williamdemeo linked a pull request Aug 16, 2024 that will close this issue
3 tasks
@williamdemeo williamdemeo linked a pull request Aug 16, 2024 that will close this issue
3 tasks
@jmchapman
Copy link

@WhatisRT - William's initial version uses the first suggestion above
"Add a temporaryDeposits field to CertState, that is updated and properly but then discarded".

Does this solve the problem? If so, in the interests of time would it make sense to go with this and add a possible later refactoring as a separate issue in the backlog?

@williamdemeo williamdemeo self-assigned this Aug 29, 2024
@WhatisRT
Copy link
Collaborator Author

We agreed to push this back a bit. When this issue is continued it makes sense to interleave this work with #467 to some degree.

It is also probably a good idea to make an STS version of the deposit update functions. This should make the POV proof a bit easier. There are two options here:

  • Either make it only a tool for the proof, basically showing that compute for this STS is extensionally equal to updateDeposits. Then the new STS wouldn't show up in any literate files and would be completely in the background.
  • Actually expose the new STS. This has the benefit of not having to prove an extra equality but it means that we now have an STS that's not in the implementation. I'd only go for this option if it is sufficiently nicer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working conformance era: conway
Projects
Status: In Progress
Development

Successfully merging a pull request may close this issue.

5 participants