IntersectMBO / ouroboros-consensus

Implementation of a Consensus Layer for the Ouroboros family of protocols
https://ouroboros-consensus.cardano.intersectmbo.org
Apache License 2.0
35 stars 23 forks source link

Enable use of VRF batch verification crypto from Conway era onward #547

Open nfrisby opened 1 year ago

nfrisby commented 1 year ago

The crypto team has (re-)prepared newer versions of the VRF and KES algorithms/formats for use in eras after Babbage (ie starting with Conway after the Chang HF). This is materialised as three PI objectives that will involve Consensus:

  1. VRF batch-compatibility (requires HF)
  2. KES secure forgetting
  3. KES signature size reduction (requires HF)

The goal of this issue is to implement batch VRF verification in consensus.

This implies:

nfrisby commented 1 year ago

For reference, this is a previous, backed out PR that might be relevant, or point to relevant upstream commits: PR input-output-hk/ouroboros-network#3801

nfrisby commented 1 year ago

To clarify, the Crypto team has three PI objectives that will involve Consensus:

  • KES secure forgetting
  • VRF batch-compatibility (requires HF)
  • KES signature size reduction (requires HF)

If I understand correctly, this Issue pertains only to the second to; it does not involve secure forgetting.

ghost commented 1 year ago

From @nfrisby

This Issue tracks the Consensus team's work to integrate those changes into the Conway branch. I think the main challenge will be choosing how to increase the expressiveness of the CardanoBlock type to have varying crypto c arguments to the ShelleyBlocks within the list of eras argument to HardForkBlock. Byron through Babbage will use c1 and Conway will use c2. It should be just a bit more bookkeeping and maybe a few careful placed ~ constraints, but that's all.

ghost commented 1 year ago

I tried latest solution proposed by @yogeshsajanikar in input-output-hk/ouroboros-network#4151 and it does not work nicely. While I was able to modify O.C.C.Block easily to introduce the type families providing an additional level of indirection to resolve the actual crypto used, I got bitten by this error in the O.C.C.CanHardFork module:

src/Ouroboros/Consensus/Cardano/CanHardFork.hs:308:42: error:
    • Illegal type synonym family application ‘StdCrypto
                                                 c’ in instance:
        CanHardFork (CardanoEras c)
    • In the instance declaration for ‘CanHardFork (CardanoEras c)’
    |
308 | instance CardanoHardForkConstraints c => CanHardFork (CardanoEras c) where
    |                                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^
ghost commented 1 year ago

Introducing an additional type variable to CardanoEras promises to be extremely annoying and impactful across the codebase as this implies modifying this type and every other type derived from it (eg. CardanoBlock) everywhere. What I am currently investigating is whether or not this dispatching could not be done at a lower level, eg. at the point where the crypto is used. Ideally, because we have ProtoCrypto proto and EraCrypto era type families around, it would be nice if I could somehow override those for the ConwayEra (and future) cases where a different crypto is needed.

ghost commented 1 year ago

I have tried several approaches which all failed, following discussions with @dnadales and @nfrisby, starting top-down from the definition of CardanoEras:

So there is a unit test in place that should fail and then succeed at some point when implementing correctly the dispatching but I wasn't able to have it implemented, and I think the crux of the issue is that the consensus code does not control how the crypto (for VRF and KES at least) is related to each era as this is defined in the ledger. So this points back to cardano-ledger#3262 PR which aims at splitting the Crypto class in 2, one for hashes and signatures, the other one for VRF and KES which has a very limited use in ledger and probably should be moved to consensus anyway.

@lehins insisted on the potential impact of what we are doing here in the cardano-node codebase as StandardCrypto is used all over the place (>350 occurrences) so this is another thing to keep in mind.

I still very much would like to find a way to introduce stronger boundaries between consensus types and ledger types so that those kind of changes could be less impactful and more easily implemented in a modular way.

ghost commented 1 year ago

Rebasing PR 3262 on newest ledger is not trivial and I am still failing to understand why this change in ledger is needed on the consensus side. Or rather, I feel that if this change is really needed, then something is wrong in our architecture and we should fix that, which might be the point of 3262 but then we should clarify the scope of this work.

ghost commented 1 year ago

It's already been a couple of weeks since I have started working on this, and it's time to take step back and distill what I learn about the problem and the potential solutions. The introductory comment to this issue defines the goal and frame it in the broader context of support for Conway era and new cryptography implementations improving security.

This is probably old news to most but here is what I learnt. The whole consensus code is currently parameterised over a single Crypto c variable which is passed to all important data structures, most notably the block, headers, transactions...

What I have done/attempted so far, along with some comments about it:

So the current situation is that I am blocked in CanHardFork on various translations issues:

The latter two might entail changes in the ledger code but the crypto should be compatible anyway so perhaps the changes would be minimal?

ghost commented 1 year ago

With @amesgen and @dnadales we were able to make good progress and get to the point where CanHardFork compiles with some hopefully mundane and boilerplate undefineds left to translate ledger state and transactions across eras with different crypto. Then the next big step is fixing the crypto type variables handling in the Node module which ties everything together and handles configuration.

Key takeaways:

lehins commented 1 year ago

we store information about VRF (keys?) in ledger state might still require conversion

Keys are stored in the ledger state, only the hashes of the keys. So, if new batch compat VRF crypto keys are compatible in binary form, then no translation is necessary. It looks like they are compatible, but I might be missing some context: https://github.com/input-output-hk/cardano-base/blob/2c1486e0bcece554b46a05d4fbefb13cbe673630/cardano-crypto-praos/src/Cardano/Crypto/VRF/Praos.hs#L427-L431

ghost commented 1 year ago

@lehins They are indeed identical but the typechecker is still unhappy, I have had to walk the various data structures to be able to convert between 2 crypto types even though the representations are the same. I may be missing something obvious but I could not appease the type gods otherwise.

ghost commented 1 year ago

Latest status: Most packages of the branch compile and o-c-c-test run, however they still fail because I am not able to use 2 different cryptos with the generators, so I may have done something wrong somewhere, perhaps a rogue equality constraint or something.

ghost commented 1 year ago

If anyone wants to pick this up before next week, here is the current situation:

abailly commented 1 year ago

Thanks to @nfrisby's sharp eye and deep expertise, I was able to solve the c1 ~ c2 spurious constraint problem I was blocked on. Turns out I threw in some VerKeyDSIGN c1 ~ VerKeyDSIGN c2 constraint which implied c1 ~ c2 because of injectivity where it was not strictly needed (can't remember why I did this, perhaps followed GHC's suggestions...).

So I now have a branch that nearly could work as is and enables different VRF crypto for Conway than we now have in Babbage, albeit with some abuse of translateXXX functions to transform ledger state and blocks from one era to the other. I think I will try to finish that version so that we at least have a "Dirt road solution" before we move to using HeaderCrypto which should be introduced by Yogesh in cardano-ledger thanks to this PR

ghost commented 1 year ago

Blocked on https://github.com/input-output-hk/cardano-ledger/pull/3388 and repository migration

dnadales commented 1 year ago

At the moment we do not have this among our priorities, so we're putting this on hold.