Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Refinements to VRF types #14036

Merged
merged 19 commits into from
May 4, 2023
Merged

Conversation

davxy
Copy link
Member

@davxy davxy commented Apr 27, 2023

Some more tweaks were required to the VRF structs to satisfy Sassafras requirements.

In particular we require the keystore to expose a vrf_output method to get the VRF configuration (aka pre-output) from which we can make random bytes (without requiring to sign the input - at all).

This new api can be also used for BABE during authoring. Here we may prefer to first get the random output, test it against the threshold and then optionally generate a signature.

The PR also introduces a better and more flexible VrfSignData for sr25519.

Such a structured vrf input can:

  1. contain pre-computed pre-outputs (not a huge deal with shnorrkel but the design is useful for bandersnatch vrf)
  2. contain extra data to sign that doesn't contribute to the vrf output. This is useful in scenarios such as this to use the keystore instead of directly the keypair.... (as probably should be, and finally prevent disclosure of implementation details such as the internal structure of the proof, etc)
  3. (not included but possible in the future) include a closure to test the output before sign.

Polkadot companion: paritytech/polkadot#7145

@davxy davxy requested a review from andresilva as a code owner April 27, 2023 17:21
@davxy davxy self-assigned this Apr 27, 2023
@davxy davxy added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Apr 27, 2023
@paritytech paritytech deleted a comment from paritytech-cicd-pr Apr 27, 2023
@davxy davxy requested a review from a team April 27, 2023 17:37
@paritytech paritytech deleted a comment from paritytech-cicd-pr Apr 27, 2023
@paritytech paritytech deleted a comment from paritytech-cicd-pr Apr 27, 2023
@davxy davxy requested a review from bkchr April 29, 2023 11:26
@paritytech paritytech deleted a comment from paritytech-cicd-pr Apr 29, 2023
@davxy davxy requested a review from burdges April 29, 2023 11:28
@davxy davxy mentioned this pull request May 1, 2023
frame/babe/src/mock.rs Show resolved Hide resolved
primitives/consensus/babe/src/lib.rs Show resolved Hide resolved
frame/babe/src/lib.rs Show resolved Hide resolved
let result = preout.make_bytes::<32>(b"rand", &input, &pair.public());
assert!(result.is_ok());
}

Copy link
Contributor

@michalkucharczyk michalkucharczyk May 2, 2023

Choose a reason for hiding this comment

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

It would be nice add new (simple) test to check returning None scenario for non-existing mapping for sr25519_vrf_output. (The scnario from fn's doc).

/// Extra transcript data to be signed by the VRF.
pub(super) extra: Option<VrfTranscript>,
/// Optional pre-computed output
pub(super) output: Option<VrfOutput>,
Copy link
Contributor

@michalkucharczyk michalkucharczyk May 2, 2023

Choose a reason for hiding this comment

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

are pub(super)s required here?

@michalkucharczyk michalkucharczyk requested review from a team and removed request for burdges May 2, 2023 09:02
Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

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

lgtm 👍

pub type VrfInput = VrfTranscript;

/// VRF input ready to be used for VRF sign and verify operations.
// Note: here we may potentially add a closure to check-before-sign...
Copy link
Member

Choose a reason for hiding this comment

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

What you mean by this?

Copy link
Member Author

@davxy davxy May 2, 2023

Choose a reason for hiding this comment

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

Preamble

The data to be signed via the vrf_sign is passed within the VrfSignData.
This is an associated type that can store whatever the implementation (in this case sr25519) needs.

In the current form for sr25519 we pass:

  • transcript : data that can be used to produce an arbitrary number of bytes via the vrf.
  • extra: data to be signed but that does not contribute to the vrf output bytes. Used for example in the approval voting protocol here
  • output: some pre-computed vrf output. In this case can save one operation during signing if it has been pre computed via the pair.vrf_output method.

Extension

The comment about the possibility to add a closure refers to :

  • how this extra is passed and
  • if we want to perform a check before sign.

For example (in babe) the closure can call make_bytes to get the "score", check that the value is below the threshold and then sign if this is below it (in contrast, now we sign and only after we check).

With the new keystore interface (i.e. the added vrf_output) this check before sign now can be done by doing something like:

// get vrf output bytes
let bytes = keystore.vrf_output(transcript).make_bytes();
if bytes < threshold {
    // sign if we are below the threshold
    sign = keystore.vrf_sign(transcript.into_sign_data())
}

With the closure I'm talking about in the comment we can perform something like:

let sign_data = transcript.into_sign_data().with_check(|out| {
    if out.make_bytes() < threshold {
        Ok(None)
     } else {
        Err
    }
});
let sign = keystore.vrf_sign(sign_data);

Notice that I'm returning Ok(None). Well here we can also return the optional extra data to be signed. So that it will also cover the polkadot approval voting use case.

What is cool here is that the sign data can store whatever the particular vrf requires. The keystore act just as a trampoline to the implementation.
Is then the implementation of the particular vrf that is able to interpret and use the content of VrfSignData.

The advantage to use a closure to perform the check-before-sign is that it requires only one access to the keystore (one call) instead of 2.

We can easily introduce it in a follow-up

(sorry for the verbose explanation 😃)

Copy link
Member

Choose a reason for hiding this comment

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

The advantage to use a closure to perform the check-before-sign is that it requires only one access to the keystore (one call) instead of 2.

Not sure that brings that much :P

However, if you think it is some nice "sugar" that we need/want, please remove the Note comment and create an issue of your explanation here :D

Copy link

@burdges burdges May 3, 2023

Choose a reason for hiding this comment

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

I've expressed my current tentative opinions on VRF interfaces in https://github.com/w3f/ring-vrf/blob/master/dleq_vrf/src/traits.rs#L101 It won't work for sr25519 but something similar & simpler works for sr25519. In particular, the Secret and Signer are the same for sr25519, but not for ring VRFs.

These sign_after_check methods are useful primarily as pedagogy, meaning a dev who reads them might realize "oh I can make the input-output pair first, run business logic on them which produces this auxiliary message, and then sign the whole result". I'm not sure there needs to be more than one such method, so the ring VRF one is called simply vrf_sign_one since it single only one input-output pair.

@davxy
Copy link
Member Author

davxy commented May 4, 2023

bot merge

@paritytech-processbot
Copy link

Error: Github API says paritytech/polkadot#7145 is not mergeable

@davxy davxy merged commit 7d6084b into paritytech:master May 4, 2023
@davxy davxy deleted the davxy-improve-sr25519-api branch May 4, 2023 13:43
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
* Allow extra signing data

* Fix tests after renaming

* Rename VrfSecret/VrfVerifier to VrfSecret/VrfPublic

* Further encrapsulation of 'transcript' type to the sr25519 implementation

* Keystore sr25519 pre-output

* Leave additional custom input field hidden in the associated VrfInput type

* Fix test

* More ergonomic output_bytes

* Trigger pipeline

* Define a separated type for vrf signature data

* Fix docs

* Fix doc

* Remove annotation

* Directly use dleq_proove and dleq_verify in sr25519

* Trigger CI

* Remove cruft before merge
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants