-
Notifications
You must be signed in to change notification settings - Fork 1
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
Trigger attestation check during validate
#1063
Changes from 39 commits
2dd92b4
74e1ef4
e41b8ec
ccce0c3
382a6e6
b7aeea5
9b3f11c
0b54e11
dd21370
79c8237
06f664e
13f47c0
54bf340
07b5013
bbdc743
661121f
102fd57
e253047
f76561b
ff90202
8e85de2
4683974
706c51d
9c53231
3449d1b
af06541
8acb8e5
14af641
7cc89ff
bf3000f
ecdad8f
9a02898
b59a008
79ec172
f75a8ed
6f023f2
143569c
d712e2d
07b1c31
d942404
78c74bf
34f33c8
0cd8167
a62cf31
56be187
0e5187a
a7c435d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -47,7 +47,7 @@ mod tests; | |
|
||
#[frame_support::pallet] | ||
pub mod pallet { | ||
use entropy_shared::QuoteInputData; | ||
use entropy_shared::{AttestationQueue, KeyProvider, QuoteInputData}; | ||
use frame_support::pallet_prelude::*; | ||
use frame_system::pallet_prelude::*; | ||
use sp_std::vec::Vec; | ||
|
@@ -63,11 +63,15 @@ pub mod pallet { | |
pub struct Pallet<T>(_); | ||
|
||
#[pallet::config] | ||
pub trait Config: frame_system::Config + pallet_staking_extension::Config { | ||
pub trait Config: frame_system::Config + pallet_parameters::Config { | ||
/// The overarching event type. | ||
type RuntimeEvent: From<Event<Self>> + IsType<<Self as frame_system::Config>::RuntimeEvent>; | ||
/// Describes the weights of the dispatchables exposed by this pallet. | ||
type WeightInfo: WeightInfo; | ||
/// A type used to get different keys for a given account ID. | ||
type KeyProvider: entropy_shared::KeyProvider<Self::AccountId>; | ||
/// A type used to describe a queue of attestations. | ||
type AttestationQueue: entropy_shared::AttestationQueue<Self::AccountId>; | ||
} | ||
|
||
#[pallet::genesis_config] | ||
|
@@ -89,7 +93,7 @@ pub mod pallet { | |
} | ||
} | ||
|
||
/// A map of TSS account id to quote nonce for pending attestations | ||
/// A map of TSS Account ID to quote nonce for pending attestations | ||
#[pallet::storage] | ||
#[pallet::getter(fn pending_attestations)] | ||
pub type PendingAttestations<T: Config> = | ||
|
@@ -117,10 +121,8 @@ pub mod pallet { | |
UnexpectedAttestation, | ||
/// Hashed input data does not match what was expected | ||
IncorrectInputData, | ||
/// Cannot lookup associated stash account | ||
NoStashAccount, | ||
/// Cannot lookup associated TS server info | ||
NoServerInfo, | ||
/// The given account doesn't have a registered X25519 public key. | ||
NoX25519KeyForAccount, | ||
/// Unacceptable VM image running | ||
BadMrtdValue, | ||
/// Cannot decode verifying key (PCK) | ||
|
@@ -149,13 +151,9 @@ pub mod pallet { | |
// Parse the quote (which internally verifies the attestation key signature) | ||
let quote = Quote::from_bytes("e).map_err(|_| Error::<T>::BadQuote)?; | ||
|
||
// Get associated server info from staking pallet | ||
let server_info = { | ||
let stash_account = pallet_staking_extension::Pallet::<T>::threshold_to_stash(&who) | ||
.ok_or(Error::<T>::NoStashAccount)?; | ||
pallet_staking_extension::Pallet::<T>::threshold_server(&stash_account) | ||
.ok_or(Error::<T>::NoServerInfo)? | ||
}; | ||
// Get associated x25519 public key from staking pallet | ||
let x25519_public_key = | ||
T::KeyProvider::x25519_public_key(&who).ok_or(Error::<T>::NoX25519KeyForAccount)?; | ||
|
||
// Get current block number | ||
let block_number: u32 = { | ||
|
@@ -165,7 +163,7 @@ pub mod pallet { | |
|
||
// Check report input data matches the nonce, TSS details and block number | ||
let expected_input_data = | ||
QuoteInputData::new(&who, server_info.x25519_public_key, nonce, block_number); | ||
QuoteInputData::new(&who, x25519_public_key, nonce, block_number); | ||
ensure!( | ||
quote.report_input_data() == expected_input_data.0, | ||
Error::<T>::IncorrectInputData | ||
|
@@ -177,21 +175,24 @@ pub mod pallet { | |
let accepted_mrtd_values = pallet_parameters::Pallet::<T>::accepted_mrtd_values(); | ||
ensure!(accepted_mrtd_values.contains(&mrtd_value), Error::<T>::BadMrtdValue); | ||
|
||
let provisioning_certification_key = | ||
T::KeyProvider::provisioning_key(&who).ok_or(Error::<T>::NoX25519KeyForAccount)?; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Strictly speaking we should rename this error variant as here we are getting the PCK, not the x25519 public key. |
||
|
||
// Check that the attestation public key is signed with the PCK | ||
let provisioning_certification_key = decode_verifying_key( | ||
&server_info | ||
.provisioning_certification_key | ||
&provisioning_certification_key | ||
.to_vec() | ||
.try_into() | ||
.map_err(|_| Error::<T>::CannotDecodeVerifyingKey)?, | ||
) | ||
.map_err(|_| Error::<T>::CannotDecodeVerifyingKey)?; | ||
|
||
quote | ||
.verify_with_pck(provisioning_certification_key) | ||
.map_err(|_| Error::<T>::PckVerification)?; | ||
|
||
// Remove the entry from PendingAttestations | ||
PendingAttestations::<T>::remove(&who); | ||
T::AttestationQueue::confirm_attestation(&who); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are probably already aware of this - but if and when we add other ways of triggering attestation requests, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ya agreed. As I mentioned elsewhere, it would be nice to consolidate these if possible |
||
|
||
// TODO #982 If anything fails, don't just return an error - do something mean | ||
|
||
|
@@ -200,4 +201,23 @@ pub mod pallet { | |
Ok(()) | ||
} | ||
} | ||
|
||
#[pallet::hooks] | ||
impl<T: Config> Hooks<BlockNumberFor<T>> for Pallet<T> { | ||
fn on_initialize(now: BlockNumberFor<T>) -> Weight { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was getting confused here, and with this hook in the staking pallet, because i was assuming There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that is correct There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it's called at the start of each block and in a specific order depending on where the pallet was declared in the |
||
let pending_validators = T::AttestationQueue::pending_attestations(); | ||
let num_pending_attestations = pending_validators.len() as u32; | ||
let mut requests = AttestationRequests::<T>::get(now).unwrap_or_default(); | ||
|
||
for account_id in pending_validators { | ||
let nonce = [0; 32]; // TODO (Nando): Fill this out properly | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ameba23 what's the expected flow for nonce generation here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It should be random - or as unpredictable as possible (eg: previous block hash). So i guess we use the on-chain randomness. |
||
PendingAttestations::<T>::insert(&account_id, nonce); | ||
requests.push(account_id.encode()); | ||
} | ||
|
||
AttestationRequests::<T>::insert(now, requests); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It feels a little bit like this data structure is now redundant and the propagation pallet could read directly from the attestation queue that the staking pallet add requests to. But i guess we need to keep the nonce generation logic inside this pallet, and it might also be useful for when we add other methods of triggering attestation requests. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. well maybe Id like to explore this a little more because this does trigger an extra on init, the nonce creation doesn't look like it needs to be here, could potentially be done when the attest request is created, could be put into a generalize function like create request and be called instead of having to go through each pending request to add the nonce There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree, there probably is a way to consolidate these data structures but it's not something I wanted to prioritize for this PR. And yeah, this will probably need to change a bit once we start adding more attestation checks. Can open a follow up issue for this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Opened #1089. |
||
|
||
<T as Config>::WeightInfo::on_initialize(num_pending_attestations) | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to be clear - these types are in
entropy-shared
to avoid circular dependencies between pallets. They are not intendend to be used byentropy-tss
, right?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct. These traits had to be defined somewhere that multiple unrelated pallets could access them