Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
6a6b33f
Introduce trait
Jun 2, 2020
363bb02
Implement VRFSigner in keystore
Jun 2, 2020
45c7582
Use vrf_sign from keystore
Jun 2, 2020
4fcf607
Convert output to VRFInOut
Jun 2, 2020
3c148b7
Simplify conversion
Jun 2, 2020
3ca49f6
vrf_sign secondary slot using keystore
Jun 2, 2020
f292623
Fix RPC call to claim_slot
Jun 2, 2020
df6063d
Use Public instead of Pair
Jun 2, 2020
76bcb2c
Check primary threshold in signer
Jun 4, 2020
81e31c5
Fix interface to return error
Jun 4, 2020
1d64e56
Move vrf_sign to BareCryptoStore
Jun 4, 2020
95a7b93
Merge remote-tracking branch 'upstream/master' into vrf-signing
Jun 4, 2020
f62b1d7
Fix authorship_works test
Jun 4, 2020
04ec073
Fix BABE logic leaks
Jun 7, 2020
6c6f380
Acquire a read lock once
Jun 7, 2020
c572d13
Also fix RPC acquiring the read lock once
Jun 7, 2020
b5415f6
Merge remote-tracking branch 'upstream/master' into vrf-signing
Jun 7, 2020
e4db9bf
Implement a generic way to construct VRF Transcript
Jun 8, 2020
b581c6c
Use make_transcript_data to call sr25519_vrf_sign
Jun 8, 2020
022b706
Make sure VRFTranscriptData is serializable
Jun 8, 2020
0c23a48
Cleanup
Jun 8, 2020
c3f17de
Move VRF to it's own module
Jun 8, 2020
a691f24
Implement & test VRF signing in testing module
Jun 8, 2020
a5821f0
Merge remote-tracking branch 'upstream/master' into vrf-signing
Jun 8, 2020
b5322c3
Remove leftover
Jun 8, 2020
53bed39
Fix feature requirements
Jun 8, 2020
43596d7
Revert removing vec macro
Jun 8, 2020
61e3b8d
Drop keystore pointer to prevent deadlock
Jun 8, 2020
383bf44
Nitpicks
Jun 8, 2020
2508594
Add test to make sure make_transcript works
Jun 8, 2020
1fd6936
Fix mismatch in VRF transcript
Jun 10, 2020
b45a1ca
Add a test to verify transcripts match in babe
Jun 10, 2020
df7dd65
Merge remote-tracking branch 'upstream/master' into vrf-signing
Jun 10, 2020
67b1c03
Return VRFOutput and VRFProof from keystore
Jun 10, 2020
e46467b
Merge remote-tracking branch 'upstream/master' into vrf-signing
Jun 10, 2020
ecd7cf1
Merge remote-tracking branch 'upstream/master' into vrf-signing
Jun 18, 2020
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion client/consensus/babe/rpc/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,12 @@ derive_more = "0.99.2"
sp-api = { version = "2.0.0-rc2", path = "../../../../primitives/api" }
sp-consensus = { version = "0.8.0-rc2", path = "../../../../primitives/consensus/common" }
sp-core = { version = "2.0.0-rc2", path = "../../../../primitives/core" }
sp-application-crypto = { version = "2.0.0-rc2", path = "../../../../primitives/application-crypto" }
sc-keystore = { version = "2.0.0-rc2", path = "../../../keystore" }

[dev-dependencies]
sc-consensus = { version = "0.8.0-rc2", path = "../../../consensus/common" }
serde_json = "1.0.50"
sp-application-crypto = { version = "2.0.0-rc2", path = "../../../../primitives/application-crypto" }
sp-keyring = { version = "2.0.0-rc2", path = "../../../../primitives/keyring" }
substrate-test-runtime-client = { version = "2.0.0-rc2", path = "../../../../test-utils/runtime/client" }
tempfile = "3.1.0"
29 changes: 16 additions & 13 deletions client/consensus/babe/rpc/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,11 @@ use sp_consensus_babe::{
digests::PreDigest,
};
use serde::{Deserialize, Serialize};
use sp_core::{
crypto::Public,
traits::BareCryptoStore,
};
use sp_application_crypto::AppKey;
use sc_keystore::KeyStorePtr;
use sc_rpc_api::DenyUnsafe;
use sp_api::{ProvideRuntimeApi, BlockId};
Expand Down Expand Up @@ -126,22 +131,20 @@ impl<B, C, SC> BabeApi for BabeRpcHandler<B, C, SC>

let mut claims: HashMap<AuthorityId, EpochAuthorship> = HashMap::new();

let key_pairs = {
let keystore = keystore.read();
epoch.authorities.iter()
.enumerate()
.flat_map(|(i, a)| {
keystore
.key_pair::<sp_consensus_babe::AuthorityPair>(&a.0)
.ok()
.map(|kp| (kp, i))
})
.collect::<Vec<_>>()
};
let keys = epoch.authorities.iter()
.enumerate()
.filter_map(|(i, a)| {
if keystore.read().has_keys(&[(a.0.to_raw_vec(), AuthorityId::ID)]) {
Some((a.0.clone(), i))
} else {
None
}
})
.collect::<Vec<_>>();

for slot_number in epoch_start..epoch_end {
if let Some((claim, key)) =
authorship::claim_slot_using_key_pairs(slot_number, &epoch, &key_pairs)
authorship::claim_slot_using_keys(slot_number, &epoch, &keystore, &keys)
{
match claim {
PreDigest::Primary { .. } => {
Expand Down
122 changes: 67 additions & 55 deletions client/consensus/babe/src/authorship.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,18 +16,19 @@

//! BABE authority selection and slot claiming.

use sp_application_crypto::AppKey;
use sp_consensus_babe::{
make_transcript, AuthorityId, BabeAuthorityWeight, BABE_VRF_PREFIX,
SlotNumber, AuthorityPair,
BABE_ENGINE_ID, BABE_VRF_PREFIX,
AuthorityId, BabeAuthorityWeight,
SlotNumber,
};
use sp_consensus_babe::digests::{
PreDigest, PrimaryPreDigest, SecondaryPlainPreDigest, SecondaryVRFPreDigest,
};
use sp_consensus_vrf::schnorrkel::{VRFOutput, VRFProof};
use sp_core::{U256, blake2_256};
use sp_core::{U256, blake2_256, traits::BareCryptoStore};
use codec::Encode;
use schnorrkel::vrf::VRFInOut;
use sp_core::Pair;
use sc_keystore::KeyStorePtr;
use super::Epoch;

Expand Down Expand Up @@ -124,7 +125,8 @@ pub(super) fn secondary_slot_author(
fn claim_secondary_slot(
slot_number: SlotNumber,
epoch: &Epoch,
key_pairs: &[(AuthorityPair, usize)],
keys: &[(AuthorityId, usize)],
keystore: &KeyStorePtr,
author_secondary_vrf: bool,
) -> Option<(PreDigest, AuthorityId)> {
let Epoch { authorities, randomness, epoch_index, .. } = epoch;
Expand All @@ -139,31 +141,42 @@ fn claim_secondary_slot(
*randomness,
)?;

for (pair, authority_index) in key_pairs {
if pair.public() == *expected_author {
for (authority_id, authority_index) in keys {
if authority_id == expected_author {
let pre_digest = if author_secondary_vrf {
let transcript = super::authorship::make_transcript(
let result = keystore.read().vrf_sign(
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this API has way too many arguments and I'd prefer it to be extracted to a VrfSigningRequest struct which is the single parameter to the function.

Copy link
Contributor

@rphmeier rphmeier Jun 6, 2020

Choose a reason for hiding this comment

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

The result of make_transcript has the type merlin::Transcript which is very broad so I would expect only that the transcript needs to be passed to a signing request.

I guess that this is because the async signing request is meant to go to a remote signer that has more information on what it is signing. In that case, the API design presented in this PR does not accurately represent the goals or contract of the features you are intending to eventually deliver.

And without knowing the context of Substrate's roadmap, it's impossible to make any case in favor of this change. Knowing all the features we intend to build should not be a requirement to understand the code and why decisions are made.

I recommend starting again with better APIs that avoid leaking BABE consensus logic out to the signer. And if BABE data (not logic!) needs to be sent to a signing service, the API should be clear that the API is for BABE, not any VRF signature as vrf_sign would imply.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I totally agree that it wasn't a good design. I have now changed things so that the function gets transcript which is passed from BABE as suggested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, after actually implementing it... i now realize that the there was actually a reason behind passing all those parameters to vrf_sign, mainly that the transcript needs to somehow be serialized so that it can be sent over the wire to the remote signer. However, doesn't seem that this is possible, which is why sending the parameters to the remote signer to build the transcript there was my go-to solution.

What do you think about this?

Copy link
Contributor

@rphmeier rphmeier Jun 7, 2020

Choose a reason for hiding this comment

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

I'm OK with that under these two conditions:

  • The interface is "honest". It only works for BABE, so it should be specific that it's about BABE. Best if this interface is on an extension trait to Keystore not the main Keystore traits as it ruins clean abstraction otherwise.
  • The BABE code is also responsible for computing the transcript and checking whether the resulting VRF is within the primary threshold. It is not the responsibility of the signing service to do consensus-critical checks.

However I still find that approach far from ideal because it is probably hard to do the necessary trait-juggling to impose a clean boundary between Keystore & BABE. And we would be entering into this approach with the understanding that every consensus engine will now need its own extension trait for the Keystore. And it is a little harder to answer the question "how would a user implement their own consensus engine with raw signing?". IMO that is how we should view consensus crates and core code anyway - core interfaces should be general enough that altering consensus engine behavior doesn't require changing anything in the core. So I'm "OK" with that approach but I think we can do better...

It would better to do a deeper refactoring so we can send the raw components of the transcript over the wire. Then you could have an actual vrf_sign function which accepted transcript components in a general way.

maybe set up vrf_sign as a generic function like this:

fn vrf_sign<T: Encode>( // maybe `Serialize`. Don't worry about `Deserialize` though.
	&self,
	key_type: KeyTypeId,
	public: &Sr25519Public,
	transcript_data: T,
	make_transcript: impl Fn(&T) -> merlin::Transcript,
) -> ...

transcript_data, key_type, public can all go over the wire, even though the transcript doesn't. But the details of what the transcript data is and how it is transformed into a transcript are 100% up to the caller, as it should be.

Now in practice in BABE, transcript_data would be a struct BabeAuthorshipVrfTranscriptData { randomness, slot_number, epoch_index, }, all parameters to sp_babe::make_transcript, which the make_transcript passed to vrf_sign would be a thin wrapper around.

And I can see that the remote signer may want access to threshold, so we can also put threshold in the BabeAuthorshipVrfTranscriptData so it is sent over the wire.

Copy link
Contributor

@rphmeier rphmeier Jun 7, 2020

Choose a reason for hiding this comment

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

Also I vastly prefer sr25519_vrf_sign as a name because we might add another VRF implementation to Substrate at some point and our API should accommodate that without requiring a major version bump.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for such a detailed explanations. I tried to go with your suggested approach, but apparently this would cause the following issue:

error[E0038]: the trait `traits::BareCryptoStore` cannot be made into an object
   --> primitives/core/src/traits.rs:211:1
    |
64  |   pub trait BareCryptoStore: Send + Sync {
    |             --------------- this trait cannot be made into an object...
...
199 |       fn sr25519_vrf_sign<VRFData: Encode>(
    |          ---------------- ...because method `sr25519_vrf_sign` has generic type parameters
...
211 | / sp_externalities::decl_extension! {
212 | |     /// The keystore extension to register/retrieve from the externalities.
213 | |     pub struct KeystoreExt(BareCryptoStorePtr);
214 | | }
    | |_^ the trait `traits::BareCryptoStore` cannot be made into an object
    |
    = help: consider moving `sr25519_vrf_sign` to another trait
    = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)

To build up on your idea, i will introduce BabeAuthorshipVrfTranscriptData but i will change the vrf_sign method interface to accept the data pre-encoded so that they are sent over the wire and on the call-site, the result would be decoded accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The generic parameter issue is caused by both <VRFData: Encode> and the fact that one of method parameters is a generic (make_transcript)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have implemented an alternative that i think makes sr25519_vrf_sign generic enough. Could you please let me know your thoughts again?

Copy link
Contributor

Choose a reason for hiding this comment

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

The issue is that it causes the trait to become non object-safe. I didn't realize that it was used as a trait object somewhere, which complicates things...

AuthorityId::ID,
authority_id.as_ref(),
&BABE_ENGINE_ID,
BABE_VRF_PREFIX,
randomness,
slot_number,
*epoch_index,
u128::MAX,
);

let s = get_keypair(&pair).vrf_sign(transcript);

PreDigest::SecondaryVRF(SecondaryVRFPreDigest {
slot_number,
vrf_output: VRFOutput(s.0.to_output()),
vrf_proof: VRFProof(s.1),
authority_index: *authority_index as u32,
})
if let Ok(Some((output, proof))) = result {
let proof = schnorrkel::vrf::VRFProof::from_bytes(&proof).ok()?;
let output = schnorrkel::vrf::VRFOutput::from_bytes(&output).ok()?;

Some(PreDigest::SecondaryVRF(SecondaryVRFPreDigest {
slot_number,
vrf_output: VRFOutput(output),
vrf_proof: VRFProof(proof),
authority_index: *authority_index as u32,
}))
} else {
None
}
} else {
PreDigest::SecondaryPlain(SecondaryPlainPreDigest {
Some(PreDigest::SecondaryPlain(SecondaryPlainPreDigest {
slot_number,
authority_index: *authority_index as u32,
})
}))
};

return Some((pre_digest, pair.public()));
if let Some(pre_digest) = pre_digest {
return Some((pre_digest, authority_id.clone()));
}
}
}

Expand All @@ -179,34 +192,31 @@ pub fn claim_slot(
epoch: &Epoch,
keystore: &KeyStorePtr,
) -> Option<(PreDigest, AuthorityId)> {
let key_pairs = {
let keystore = keystore.read();
epoch.authorities.iter()
.enumerate()
.flat_map(|(i, a)| {
keystore.key_pair::<AuthorityPair>(&a.0).ok().map(|kp| (kp, i))
})
.collect::<Vec<_>>()
};
claim_slot_using_key_pairs(slot_number, epoch, &key_pairs)
let authorities = epoch.authorities.iter()
.enumerate()
.map(|(index, a)| (a.0.clone(), index))
.collect::<Vec<_>>();
claim_slot_using_keys(slot_number, epoch, keystore, &authorities)
}

/// Like `claim_slot`, but allows passing an explicit set of key pairs. Useful if we intend
/// to make repeated calls for different slots using the same key pairs.
pub fn claim_slot_using_key_pairs(
pub fn claim_slot_using_keys(
slot_number: SlotNumber,
epoch: &Epoch,
key_pairs: &[(AuthorityPair, usize)],
keystore: &KeyStorePtr,
keys: &[(AuthorityId, usize)],
) -> Option<(PreDigest, AuthorityId)> {
claim_primary_slot(slot_number, epoch, epoch.config.c, &key_pairs)
claim_primary_slot(slot_number, epoch, epoch.config.c, keystore, &keys)
.or_else(|| {
if epoch.config.allowed_slots.is_secondary_plain_slots_allowed() ||
epoch.config.allowed_slots.is_secondary_vrf_slots_allowed()
{
claim_secondary_slot(
slot_number,
&epoch,
&key_pairs,
keys,
keystore,
epoch.config.allowed_slots.is_secondary_vrf_slots_allowed(),
)
} else {
Expand All @@ -215,11 +225,6 @@ pub fn claim_slot_using_key_pairs(
})
}

fn get_keypair(q: &AuthorityPair) -> &schnorrkel::Keypair {
use sp_core::crypto::IsWrappedBy;
sp_core::sr25519::Pair::from_ref(q).as_ref()
}

/// Claim a primary slot if it is our turn. Returns `None` if it is not our turn.
/// This hashes the slot number, epoch, genesis hash, and chain randomness into
/// the VRF. If the VRF produces a value less than `threshold`, it is our turn,
Expand All @@ -228,33 +233,40 @@ fn claim_primary_slot(
slot_number: SlotNumber,
epoch: &Epoch,
c: (u64, u64),
key_pairs: &[(AuthorityPair, usize)],
keystore: &KeyStorePtr,
keys: &[(AuthorityId, usize)],
) -> Option<(PreDigest, AuthorityId)> {
let Epoch { authorities, randomness, epoch_index, .. } = epoch;

for (pair, authority_index) in key_pairs {
let transcript = super::authorship::make_transcript(randomness, slot_number, *epoch_index);

for (authority_id, authority_index) in keys {
// Compute the threshold we will use.
//
// We already checked that authorities contains `key.public()`, so it can't
// be empty. Therefore, this division in `calculate_threshold` is safe.
let threshold = super::authorship::calculate_primary_threshold(c, authorities, *authority_index);

let pre_digest = get_keypair(pair)
.vrf_sign_after_check(transcript, |inout| super::authorship::check_primary_threshold(inout, threshold))
.map(|s| {
PreDigest::Primary(PrimaryPreDigest {
slot_number,
vrf_output: VRFOutput(s.0.to_output()),
vrf_proof: VRFProof(s.1),
authority_index: *authority_index as u32,
})
let result = keystore.read().vrf_sign(
AuthorityId::ID,
authority_id.as_ref(),
&BABE_ENGINE_ID,
BABE_VRF_PREFIX,
randomness,
slot_number,
*epoch_index,
threshold,
);
if let Ok(Some((output, proof))) = result {
let proof = schnorrkel::vrf::VRFProof::from_bytes(&proof).ok()?;
let output = schnorrkel::vrf::VRFOutput::from_bytes(&output).ok()?;

let pre_digest = PreDigest::Primary(PrimaryPreDigest {
slot_number,
vrf_output: VRFOutput(output),
vrf_proof: VRFProof(proof),
authority_index: *authority_index as u32,
});

// early exit on first successful claim
if let Some(pre_digest) = pre_digest {
return Some((pre_digest, pair.public()));
return Some((pre_digest, authority_id.clone()));
}
}

Expand Down
4 changes: 3 additions & 1 deletion client/keystore/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,12 @@ derive_more = "0.99.2"
sp-core = { version = "2.0.0-rc2", path = "../../primitives/core" }
sp-application-crypto = { version = "2.0.0-rc2", path = "../../primitives/application-crypto" }
hex = "0.4.0"
merlin = { version = "2.0", default-features = false }
parking_lot = "0.10.0"
rand = "0.7.2"
serde_json = "1.0.41"
subtle = "2.1.1"
parking_lot = "0.10.0"
schnorrkel = { version = "0.9.1", features = ["preaudit_deprecated", "u64_backend"], default-features = false}

[dev-dependencies]
tempfile = "3.1.0"
46 changes: 45 additions & 1 deletion client/keystore/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,14 @@
use std::{collections::{HashMap, HashSet}, path::PathBuf, fs::{self, File}, io::{self, Write}, sync::Arc};
use sp_core::{
crypto::{IsWrappedBy, CryptoTypePublicPair, KeyTypeId, Pair as PairT, Protected, Public},
traits::{BareCryptoStore, BareCryptoStoreError as TraitError},
traits::{BareCryptoStore, Error as TraitError},
sr25519::{Public as Sr25519Public, Pair as Sr25519Pair},
Encode,
};
use sp_application_crypto::{AppKey, AppPublic, AppPair, ed25519, sr25519, ecdsa};
use parking_lot::RwLock;
use merlin::Transcript;
use schnorrkel::vrf::VRFInOut;

/// Keystore pointer
pub type KeyStorePtr = Arc<RwLock<Store>>;
Expand Down Expand Up @@ -296,6 +299,24 @@ impl Store {

Ok(public_keys)
}

fn make_vrf_transcript(
&self,
label: &'static [u8],
randomness: &[u8],
slot_number: u64,
epoch: u64
) -> Transcript {
let mut transcript = Transcript::new(label.clone());
transcript.append_u64(b"slot number", slot_number);
transcript.append_u64(b"current epoch", epoch);
transcript.append_message(b"chain randomness", &randomness[..]);
transcript
}

fn check_primary_threshold(&self, prefix: &'static [u8], inout: &VRFInOut, threshold: u128) -> bool {
u128::from_le_bytes(inout.make_bytes::<[u8; 16]>(prefix)) < threshold
}
}

impl BareCryptoStore for Store {
Expand Down Expand Up @@ -438,6 +459,29 @@ impl BareCryptoStore for Store {
fn has_keys(&self, public_keys: &[(Vec<u8>, KeyTypeId)]) -> bool {
public_keys.iter().all(|(p, t)| self.key_phrase_by_type(&p, *t).is_ok())
}

fn vrf_sign(
&self,
key_type: KeyTypeId,
public: &Sr25519Public,
label: &'static [u8],
prefix: &'static [u8],
randomness: &[u8],
slot_number: u64,
epoch: u64,
threshold: u128,
) -> std::result::Result<Option<(Vec<u8>, Vec<u8>)>, TraitError> {
let transcript = self.make_vrf_transcript(label, randomness, slot_number, epoch);
let pair = self.key_pair_by_type::<Sr25519Pair>(public, key_type)
.map_err(|e| TraitError::PairNotFound(e.to_string()))?;

let (inout, proof, _) = pair.as_ref().vrf_sign(transcript);
if self.check_primary_threshold(prefix, &inout, threshold) {
Ok(Some((inout.to_output().to_bytes().to_vec(), proof.to_bytes().to_vec())))
} else {
Ok(None)
}
}
}

#[cfg(test)]
Expand Down
Loading