Committee query - implement next epoch change

[teodanciu]

Description

Closes: #3810

I'm not convinced about a few things in this implementation:

1. describing members from both current and next epoch committee as Active (with the only difference between them being that the former has NextEpochChange either ToBeRemoved or NoChangeExpected and the latter ToBeEnacted).
   In the current acceptance of the Query, Active means not expired, so it is technically compatible with the above, but I think it can be confusing.
   What can we do ?

• Adding another value to this status, say ActiveNextEpoch ? Maybe, but it would kinda mix the categories. Is it possible for a member from the nextCommittee to be already expired? Then which value would this field take, ActiveNextEpoch or Expired?
• Another option would be to return only the current committee members, and use the nextEpoch committee members to determine if a member will be removed or no change expected. So ToBeEnacted would never appear as a value in this implementation.

2. members that are Unrecognized (so: they have a hot key in the CommitteeState but no corresponding cold key in either the current committee or the committee in the next epoch), will have the field set to NoChangeExpected. Because what else can it be? ToBeRemoved doesn't really match, since they are not in the committee. ToBeEnacted also no, because they are not in the next committee.
3. At first I thought that expiration of the committee member should be part of the computation of this field, but I have decided against it, because
   • the expiration dimension is dealt with in another field , namely MemberStatus
   • expired members are not technically removed from the committee (though their vote doesn't count, in the same way as members without a hot key don't count).
     So i thought it would be better to keep the two separate.

Checklist

• Commit sequence broadly makes sense and commits have useful messages
• New tests are added if needed and existing tests are updated
• When applicable, versions are updated in .cabal and CHANGELOG.md files according to the
  versioning process.
• The version bounds in .cabal files for all affected packages are updated. If you change the bounds in a cabal file, that package itself must have a version increase. (See RELEASING.md)
• All visible changes are prepended to the latest section of a CHANGELOG.md for the affected packages. New section is never added with the code changes. (See RELEASING.md)
• Code is formatted with fourmolu (use scripts/fourmolize.sh)
• Cabal files are formatted (use scripts/cabal-format.sh)
• hie.yaml has been updated (use scripts/gen-hie.sh)
• Self-reviewed the diff

[lehins]

1. .... What can we do ?

If you look at the haddock for Unrecognized, it is exactly that, so no new status is needed. In current state it is still an unknown committee member, but it will be enacted in the future. It might be a confusing name, I am definitely opened to renaming it to something better.
https://github.com/input-output-hk/cardano-ledger/blob/1d167ee0c72c2fdb51774d7998b91f237b9bf0cc/libs/cardano-ledger-api/src/Cardano/Ledger/Api/State/Query/CommitteeMembersState.hs#L44-L46

[lehins]

members that are Unrecognized (so: they have a hot key in the CommitteeState but no corresponding cold key in either the current committee or the committee in the next epoch)

They will be removed at the next epoch, otherwise that Map could grow with garbage CC keys forever

[lehins]

Not quite right, but it is really good first attempt. Could you also add a couple of test cases for the new functionality?

[lehins]

At first I thought that expiration of the committee member should be part of the computation of this field,

That brings me an idea, we could add an extra status: ToBeExpired

[teodanciu]

Could you also add a couple of test cases for the new functionality

There is a test for it:
c3cdf55#diff-fb722545a2358df077b89f77d538341ddfbb9d6a3e68354459abad096b1bfbc7R267

It's still not clear to me what we want to achieve. I have assumed this, if we look at an example ( not saying it's the right thing ,because I can see quite a few things that are confusing, as i pointed out in the description) :

current:   
c1 -> epoch 
c2 ->  epochExpired
c3 -> epoch
c4 -> epochExpired

committeeState: 
c1 -> h1
c2 -> h2
c80 -> h80
c90 -> h90

next:Committe:
c1 -> epoch
c4 -> epoch
c500 -> epoch
c80 -> epoch

Then a query without any filters, would have the domain all the cold keys: so a union of: current committee, next committee and the keyset of committeeState:

c1 - (Authorized h1, Active, NoChangeExpected)
c2 - (Authorized h2, Expired, ToBeRemoved)
c3 - (NotAuthorized, Active, ToBeRemoved)
c4 - (NotAuthorized, Expired, NoChangeExpected)
c80 - (Authorized h80, Active, ToBeEnacted)
c90 - (Authorized h90, Unrecognized, NoChangeExpected)
c500 - (NotAuthorized, Active, ToBeEnacted)

But if I understand from your comment, you would prefer to not have the nextCommittee in the domain?
so basically the query would return results for:

c1
c2
c3 
c4 
c80
c90

as it does now? In this case, we won't be able to set ToBeEnacted status, unless the hot key is in the committeeState (which sound sensible).

[teodanciu]

So something like this?

c1 - (Authorized h1, Active, NoChangeExpected)  (it's in current & nextCommittee)
c2 - (Authorized h2, Expired, ToBeRemoved) (it's in current && not in nextCommittee)
c3 -  (NotAuthorized, Active, ToBeRemoved) (it's in current && no the nextCommittee)
c4 - (NotAuthorized, Expired, NoChangeExpected)  (it's in current && the nextCommittee)
c80 - (Authorized h80, ???, ToBeEnacted) (it's in the next committee but not in the current. MemberStatus Active/Expired (depending on the epoch) is a little weird, but what else can it be?  )
c90 - (Authorized h90, Unrecognized,  ToBeRemoved)  (because it's not in nextCommittee)

So the difference between this and the implementation in this PR would be:

• if cold keys appear in the nextCommitee, but not in the current committe nor in the committeeState, then they won't be returned in the query
• all results with status Unrecognized will have nextChange to be ToBeRemoved

[teodanciu]

As discussed, the result for this would be:

c1 - (Authorized h1, Active, NoChangeExpected)  (it's in current & nextCommittee)
c2 - (Authorized h2, Expired, ToBeRemoved) (it's in current && not in nextCommittee)
c3 -  (NotAuthorized, Active, ToBeRemoved) (it's in current && no the nextCommittee)
c4 - (NotAuthorized, Expired, NoChangeExpected)  (it's in current && the nextCommittee)
c80 - (Authorized h80, Unrecognized, ToBeEnacted)   (Unrecognized, regardless of the epochNo, because it's not in the current committee)
c90 - (Authorized h90, Unrecognized,  ToBeRemoved)  (Unrecognized, because it's not in currentCommitee,  ToBeRemoved, because it's not in the nextCommittee)
c500 -  (NotAuthorized, Unrecognized, ToBeEnacted)  (Unrecognized, because it's not in currentCommittee, ToBeEnacted, because it's in the nextCommittee)

[lehins]

I've suggested some minor changes.
We can discuss them tomorrow if you have questions about it.

[teodanciu]

Updated with the suggested changes, and also reflected them in the ImpTests in the other PR: #3883

[lehins]

There is a test failure on CI due to a faulty tes, otherwise it looks great.

[lehins]

Awesome work!