Skip to content
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

Merged
merged 47 commits into from
Oct 2, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
47 commits
Select commit Hold shift + click to select a range
2dd92b4
Switch to using a `StorageDoubleMap` instead
HCastano Sep 23, 2024
74e1ef4
Use loose coupling between attestation and staking extension pallets
HCastano Sep 23, 2024
e41b8ec
Update test mocks to use loose coupling
HCastano Sep 23, 2024
ccce0c3
Appease Clippy
HCastano Sep 23, 2024
382a6e6
Add associated types to runtime implementation of `pallet_attestation`
HCastano Sep 23, 2024
b7aeea5
Change how an X25519 public key is grabbed from storage
HCastano Sep 24, 2024
9b3f11c
Update attestation pallet to use new interface
HCastano Sep 24, 2024
0b54e11
Write happy path test for full `validate` + `attest` flow
HCastano Sep 24, 2024
dd21370
Fix compliation of `pallet_propagation` tests
HCastano Sep 24, 2024
79c8237
TaploFmt
HCastano Sep 24, 2024
06f664e
Remove `dbg!` statements
HCastano Sep 24, 2024
13f47c0
Use `Weight::zero()` instead of `0.into()`
HCastano Sep 24, 2024
54bf340
Fix staking pallet tests
HCastano Sep 25, 2024
07b5013
Add new event for `validate` extrinsic
HCastano Sep 25, 2024
bbdc743
Inline some imports
HCastano Sep 25, 2024
661121f
Add some docs around the key provider and attestation queue
HCastano Sep 25, 2024
102fd57
Clean up docs around validation queue
HCastano Sep 25, 2024
e253047
Use counted double map for validation queue
HCastano Sep 25, 2024
f76561b
Use new NMap key syntax for staking tests
HCastano Sep 25, 2024
ff90202
Add limit to number of pending attestations in queue
HCastano Sep 25, 2024
8e85de2
Add test for max pending attestations
HCastano Sep 25, 2024
4683974
Hardcode pending attestation limit
HCastano Sep 26, 2024
706c51d
Add benchmark for Staking Extension's `on_initialize` hook
HCastano Sep 26, 2024
9c53231
Fix a few staking pallet benchmarks
HCastano Sep 26, 2024
3449d1b
RustFmt
HCastano Sep 26, 2024
af06541
Add hack for writing to Staking pallet storage from Attestation benches
HCastano Sep 27, 2024
8acb8e5
RustFmt
HCastano Sep 27, 2024
14af641
Run through benchmark for `Staking::on_intialize()`
HCastano Sep 27, 2024
7cc89ff
Run through benchmark for `Attestation::on_intialize()`
HCastano Sep 27, 2024
bf3000f
Merge branch 'master' into hc/add-attestation-check-to-validate
HCastano Sep 27, 2024
ecdad8f
Add new method to key handler for dealing with PCKs
HCastano Sep 27, 2024
9a02898
Bump TDX Quote and fix staking tests
HCastano Sep 27, 2024
b59a008
Clean up `KeyProvider` implementation
HCastano Sep 27, 2024
79ec172
Fix attestation benchmarks
HCastano Sep 27, 2024
f75a8ed
Fix staking benchmarks
HCastano Sep 27, 2024
6f023f2
Rename `X25519KeyProvider` to `KeyProvider`
HCastano Sep 27, 2024
143569c
RustFmt
HCastano Sep 27, 2024
d712e2d
Add check for expected values in test
HCastano Sep 27, 2024
07b1c31
Add note about pallet ordering for `on_initialize()`
HCastano Sep 27, 2024
d942404
RustFmt
HCastano Sep 27, 2024
78c74bf
Clean up some small things
HCastano Sep 30, 2024
34f33c8
Use randomness in quote generation
HCastano Sep 30, 2024
0cd8167
Add test randomness provider to Propagation mock
HCastano Sep 30, 2024
a62cf31
Add `CHANGELOG` entry
HCastano Sep 30, 2024
56be187
Change method comment into doccomment
HCastano Oct 1, 2024
0e5187a
Clarify accounts used in `ValidationQueue`
HCastano Oct 1, 2024
a7c435d
Add error type for missing PCK
HCastano Oct 1, 2024
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
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,11 @@ At the moment this project **does not** adhere to
As such, `UserSignatureRequest` no longer requires the `validators_info` field since the the
relayer adds that in after. The response received from the validator is now a `Vec<Responses>`
from the signers.
- In ([#1063](https://github.com/entropyxyz/entropy-core/pull/1063)) the
`pallet_staking_extension::validate()` extrinsic was changed so that in order to populate certain
data structures related to a candidates state (namely `ThresholdToStash` and `ThresholdServer`) an
attestation from the Attestation pallet must have been received. Success of the `validate()`
extrinsic **does not** mean the caller is a candidate or validator.

### Added
- Jumpstart network ([#918](https://github.com/entropyxyz/entropy-core/pull/918))
Expand All @@ -51,6 +56,7 @@ At the moment this project **does not** adhere to
- Fix TSS `AccountId` keys in chainspec ([#993](https://github.com/entropyxyz/entropy-core/pull/993))
- No unbonding when signer or next signer ([#1031](https://github.com/entropyxyz/entropy-core/pull/1031))
- Add relay tx endpoint ([#1050](https://github.com/entropyxyz/entropy-core/pull/1050))
- Trigger attestation check during validate ([#1063](https://github.com/entropyxyz/entropy-core/pull/1063))

### Removed
- Remove `prune_registration` extrinsic ([#1022](https://github.com/entropyxyz/entropy-core/pull/1022))
Expand Down
4 changes: 4 additions & 0 deletions Cargo.lock

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

31 changes: 31 additions & 0 deletions crates/shared/src/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -131,3 +131,34 @@ impl QuoteInputData {
Self(hasher.finalize().into())
}
}

/// A trait used to get different stored keys for a given account ID.
///
/// Not every account ID will have an given key, in which case the implementer is expected to
/// return `None`.
pub trait KeyProvider<T> {
Copy link
Contributor

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 by entropy-tss, right?

Copy link
Collaborator Author

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

/// Get an X25519 public key, if any, for the given account ID.
fn x25519_public_key(account_id: &T) -> Option<X25519PublicKey>;

/// Get a provisioning certification key, if any, for the given account ID.
fn provisioning_key(account_id: &T) -> Option<EncodedVerifyingKey>;
}

/// A trait used to describe a queue of attestations.
pub trait AttestationQueue<T> {
/// Indicate that a given attestation is ready to be moved from a pending state to a confirmed
/// state.
fn confirm_attestation(account_id: &T);

/// Request that an attestation get added to the queue for later processing.
fn push_pending_attestation(
signer: T,
tss_account: T,
x25519_public_key: X25519PublicKey,
endpoint: Vec<u8>,
provisioning_certification_key: EncodedVerifyingKey,
);

/// The list of pending (not processed) attestations.
fn pending_attestations() -> Vec<T>;
}
2 changes: 2 additions & 0 deletions pallets/attestation/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ sp-staking ={ version="27.0.0", default-features=false }
frame-benchmarking={ version="29.0.0", default-features=false, optional=true }
sp-std ={ version="14.0.0", default-features=false }
pallet-session ={ version="29.0.0", default-features=false, optional=true }
rand_chacha ={ version="0.3", default-features=false }

pallet-parameters={ version="0.2.0", path="../parameters", default-features=false }
entropy-shared={ version="0.2.0", path="../../crates/shared", features=[
Expand Down Expand Up @@ -54,5 +55,6 @@ std=[
'pallet-parameters/std',
'sp-io/std',
"sp-runtime/std",
"rand_chacha/std",
]
try-runtime=['frame-support/try-runtime']
64 changes: 48 additions & 16 deletions pallets/attestation/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,9 @@
// You should have received a copy of the GNU Affero General Public License
// along with this program. If not, see <https://www.gnu.org/licenses/>.

use entropy_shared::QuoteInputData;
use frame_benchmarking::{benchmarks, impl_benchmark_test_suite, whitelisted_caller};
use frame_support::BoundedVec;
use entropy_shared::{AttestationQueue, QuoteInputData};
use frame_benchmarking::{account, benchmarks, impl_benchmark_test_suite, whitelisted_caller};
use frame_system::{EventRecord, RawOrigin};
use pallet_staking_extension::{ServerInfo, ThresholdServers, ThresholdToStash};

use super::*;
#[allow(unused)]
Expand Down Expand Up @@ -64,18 +62,16 @@ benchmarks! {
// Insert a pending attestation so that this quote is expected
<PendingAttestations<T>>::insert(attestee.clone(), nonce);

let stash_account = <T as pallet_session::Config>::ValidatorId::try_from(attestee.clone())
.or(Err(()))
.unwrap();

<ThresholdToStash<T>>::insert(attestee.clone(), stash_account.clone());
<ThresholdServers<T>>::insert(stash_account.clone(), ServerInfo {
tss_account: attestee.clone(),
x25519_public_key: [0; 32],
endpoint: b"http://localhost:3001".to_vec(),
provisioning_certification_key: pck_encoded.to_vec().try_into().unwrap(),
});

// We also need to write to the queue (whose specific implementation writes to the staking
// pallet in this case) to ensure that the `attest` extrinsic has all the information about our
// attestee available.
T::AttestationQueue::push_pending_attestation(
attestee.clone(),
attestee.clone(),
[0; 32],
b"http://localhost:3001".to_vec(),
pck_encoded,
);
}: _(RawOrigin::Signed(attestee.clone()), quote.clone())
verify {
assert_last_event::<T>(
Expand All @@ -84,6 +80,42 @@ benchmarks! {
// Check that there is no longer a pending attestation
assert!(!<PendingAttestations<T>>::contains_key(attestee));
}

on_initialize {
// Note: We should technically be using `MaxPendingAttestations` but we don't have access to
// that here...
let s in 1 .. 250;

let caller: T::AccountId = whitelisted_caller();
let nonce = [0; 32];
let block_number = <frame_system::Pallet<T>>::block_number();

let pck = tdx_quote::SigningKey::from_bytes(&PCK.into()).unwrap();
let pck_encoded = tdx_quote::encode_verifying_key(pck.verifying_key()).unwrap();

for i in 0..s {
let threshold_account_id: T::AccountId = account("threshold", 0, i);
T::AttestationQueue::push_pending_attestation(
threshold_account_id.clone(),
threshold_account_id.clone(),
[0; 32],
b"http://localhost:3001".to_vec(),
pck_encoded,
);
}
}: {
use frame_support::traits::Hooks;
let _ = AttestationPallet::<T>::on_initialize(block_number);
} verify {
// Here we'll just spot check one account instead of `s` accounts since if one was written
// all were
let threshold_account_id: T::AccountId = account("threshold", 0, 0);
assert!(PendingAttestations::<T>::get(threshold_account_id).is_some());

// We want to ensure that all the requests that we added to the `T::AttestationQueue` were
// also added to `AttestationRequests`
assert!(AttestationRequests::<T>::get(block_number).unwrap().len() == s as usize);
}
}

impl_benchmark_test_suite!(AttestationPallet, crate::mock::new_test_ext(), crate::mock::Test);
81 changes: 63 additions & 18 deletions pallets/attestation/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,10 +47,17 @@ 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_support::traits::Randomness;
use frame_system::pallet_prelude::*;
use sp_runtime::traits::TrailingZeroInput;
use sp_std::vec::Vec;

use rand_chacha::{
rand_core::{RngCore, SeedableRng},
ChaCha20Rng, ChaChaRng,
};
use tdx_quote::{decode_verifying_key, Quote};

pub use crate::weights::WeightInfo;
Expand All @@ -63,11 +70,17 @@ 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;
/// Something that provides randomness in the runtime.
type Randomness: Randomness<Self::Hash, BlockNumberFor<Self>>;
/// 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]
Expand All @@ -89,7 +102,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> =
Expand Down Expand Up @@ -117,10 +130,10 @@ 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,
/// The given account doesn't have a registered provisioning certification key.
NoPCKForAccount,
/// Unacceptable VM image running
BadMrtdValue,
/// Cannot decode verifying key (PCK)
Expand Down Expand Up @@ -149,13 +162,9 @@ pub mod pallet {
// Parse the quote (which internally verifies the attestation key signature)
let quote = Quote::from_bytes(&quote).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 = {
Expand All @@ -165,7 +174,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
Expand All @@ -177,21 +186,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>::NoPCKForAccount)?;

// 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);
Copy link
Contributor

Choose a reason for hiding this comment

The 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, confirm_attestation is gonna get called regardless of whether this particular attestation request was related to something in the attestation queue. This is not a problem, since nothing bad happens if we call it several times when they are already in a Confirmed state.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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

Expand All @@ -200,4 +212,37 @@ pub mod pallet {
Ok(())
}
}

#[pallet::hooks]
impl<T: Config> Hooks<BlockNumberFor<T>> for Pallet<T> {
fn on_initialize(now: BlockNumberFor<T>) -> Weight {
Copy link
Contributor

Choose a reason for hiding this comment

The 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 on_initialize was called once at genesis. But having looked it up, its called each block, right?

Copy link
Member

Choose a reason for hiding this comment

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

that is correct

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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 construct_runtime! macro.

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 mut nonce = [0; 32];
Self::get_randomness().fill_bytes(&mut nonce[..]);
PendingAttestations::<T>::insert(&account_id, nonce);
requests.push(account_id.encode());
}

AttestationRequests::<T>::insert(now, requests);
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Opened #1089.


<T as Config>::WeightInfo::on_initialize(num_pending_attestations)
}
}

impl<T: Config> Pallet<T> {
pub fn get_randomness() -> ChaCha20Rng {
Copy link
Contributor

Choose a reason for hiding this comment

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

This function is in the staking pallet and is public, and the staking pallet is a dependency of this pallet. Is there a reason why we need it here as well?

In the registry pallet, its called like this:

 let mut rng = pallet_staking_extension::Pallet::<T>::get_randomness();

Copy link
Collaborator Author

@HCastano HCastano Oct 1, 2024

Choose a reason for hiding this comment

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

We can't do that anymore because the Attestation pallet doesn't (from Substrate's point of view) depend on the Staking Extension pallet anymore.

Said in code, this isn't satisfied:

pub trait pallet_attestation::Config: pallet_staking_extension::Config { }

The Registry pallet still has that trait bound so it's allowed to pull in code from the Staking Extension.

let phrase = b"quote_creation";
// TODO: Is randomness freshness an issue here
// https://github.com/paritytech/substrate/issues/8312
let (seed, _) = T::Randomness::random(phrase);
// seed needs to be guaranteed to be 32 bytes.
let seed = <[u8; 32]>::decode(&mut TrailingZeroInput::new(seed.as_ref()))
.expect("input is padded with zeroes; qed");
ChaChaRng::from_seed(seed)
}
}
}
5 changes: 5 additions & 0 deletions pallets/attestation/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,9 @@ frame_support::construct_runtime!(
impl pallet_attestation::Config for Test {
type RuntimeEvent = RuntimeEvent;
type WeightInfo = ();
type Randomness = TestPastRandomness;
type KeyProvider = Staking;
type AttestationQueue = Staking;
}

parameter_types! {
Expand Down Expand Up @@ -309,12 +312,14 @@ impl Randomness<H256, BlockNumber> for TestPastRandomness {
}
parameter_types! {
pub const MaxEndpointLength: u32 = 3;
pub const MaxPendingAttestations: u32 = 4;
}
impl pallet_staking_extension::Config for Test {
type Currency = Balances;
type MaxEndpointLength = MaxEndpointLength;
type Randomness = TestPastRandomness;
type RuntimeEvent = RuntimeEvent;
type MaxPendingAttestations = MaxPendingAttestations;
type WeightInfo = ();
}

Expand Down
Loading
Loading