Trigger attestation check during `validate` #1063

HCastano · 2024-09-23T20:14:58Z

When somebody states their intention to become a validator on the network they need to go
through the staking extension pallet's validate() extrinsic. As part of this process we
want to ensure that anybody submitting their candidacy is running on suitable hardware,
and as such we need issue an attestation challenge.

This PR adds a call into the attestation pallet from the validate() extrinsic and only
accepts a caller as a candidate if they successfully pass the attestation check.

There were some issues around using the Attestation pallet directly from the Staking
Extension pallet due to circular dependencies, so I worked around this in two ways:

I changed the Attestation pallet to be soft-coupled to the Staking Extension pallet
I introduced an intermediate attestation queue to the Staking Extension pallet through
which both pallets communicate

For (2) to work I had to add on_initialize hooks to both pallets in order to ensure
that any new attestations or confirmations got picked up and dealt with "automatically".

With the tight coupling we're not able to build out a mock runtime for the staking extension pallet since the compiler isn't able to verify that we have an implementation of the staking extension pallet for our mock attestation pallet (yeah, bit of a roundabout thing...). The loose coupling gives us a bit of flexibility here, and also allows us to scope the iteraction between the pallets in a more fine grained way.

We can't rely on the `ThresholdToStash` or `ThresholdServer` storage entries being populated anymore.

We're going to try and use this to limit the number of entries for benchmarking purposes.

The compiler is complaining abou a missing `Div` implementation which I'm not really sure how to work around.

This isn't the best solution, but for now it needs to be done since the `attest()` extrinsic expects certain Staking pallet specific data structures to be populated before it has try and verify an attestation.

This reflects the more generic nature of the trait now.

HCastano · 2024-09-27T22:51:21Z

pallets/attestation/src/lib.rs

+            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


@ameba23 what's the expected flow for nonce generation here?

It should be random - or as unpredictable as possible (eg: previous block hash). So i guess we use the on-chain randomness.

ameba23

🥇 Great

ameba23 · 2024-10-01T07:40:36Z

crates/shared/src/types.rs

+///
+/// Not every account ID will have an given key, in which case the implementer is expected to
+/// return `None`.
+pub trait KeyProvider<T> {


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?

Correct. These traits had to be defined somewhere that multiple unrelated pallets could access them

ameba23 · 2024-10-01T07:54:32Z

pallets/attestation/src/lib.rs

@@ -177,21 +184,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)?;


Strictly speaking we should rename this error variant as here we are getting the PCK, not the x25519 public key.

ameba23 · 2024-10-01T08:01:17Z

pallets/attestation/src/lib.rs

+    }
+
+    impl<T: Config> Pallet<T> {
+        pub fn get_randomness() -> ChaCha20Rng {


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();

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.

ameba23 · 2024-10-01T08:12:44Z

pallets/attestation/src/lib.rs

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


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?

that is correct

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.

ameba23 · 2024-10-01T08:45:37Z

pallets/attestation/src/lib.rs

+                requests.push(account_id.encode());
+            }
+
+            AttestationRequests::<T>::insert(now, requests);


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.

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

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.

Opened #1089.

ameba23 · 2024-10-01T08:46:55Z

pallets/staking/src/benchmarking.rs

+
+    /// This is a randomly generated secret p256 ECDSA key - for mocking the provisioning certification
+    /// key
+    const PCK: [u8; 32] = [


Nit: I think you also use this constant in the tests. I would put it in mock.rs and make it public, and you can use it here and it the tests.

I tried doing this but there are some issues around differing feature flags for tests and benchmarks, so we can't actually pull in constants from mock.rs in easily.

Another solution would be to move the PCK constant either to the top crate level and feature flag it there but for two uses it doesn't seem worth it to me. But ya can do it if you think it would be better

ameba23 · 2024-10-01T09:01:34Z

pallets/attestation/src/lib.rs

            PendingAttestations::<T>::remove(&who);
+            T::AttestationQueue::confirm_attestation(&who);


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.

Ya agreed. As I mentioned elsewhere, it would be nice to consolidate these if possible

ameba23 · 2024-10-01T09:03:59Z

pallets/staking/src/lib.rs

+        // # Panics
+        //
+        // Panics if an invalid `validator_stash` or `provisioning_certification_key` are passed
+        // in. The caller should check (e.g, using `Self::get_stash()` that these inputs are valid.


Not sure if it makes any difference, but maybe you intended this to be a doccomment ///

It does make a difference. If we use doc comments here it would override the doc comments from the trait definition, which is what I initially wanted.

Thinking about it bit it might be safer to override in this case since it would allow us to warn users of this API through docs (e.g docs.rs) rather than through the source code itself.

JesseAbram

looks good a few comments, the only thing that I don't love here is the two on_inits just a little more elaboration on those comments then id be good to approve

JesseAbram · 2024-10-01T13:12:51Z

pallets/staking/src/lib.rs

+    /// A queue of validator attestation requests.
+    ///
+    /// This tracks the status of each attestation (pending or confirmed), as well as information
+    /// about the validator who is in the process of submitting an attestation.


the key is a TSS account? can you add that to the comments

JesseAbram · 2024-10-01T13:19:28Z

pallets/staking/src/lib.rs

@@ -742,4 +812,77 @@ pub mod pallet {
            I::start_session(start_index);
        }
    }
+
+    impl<T: Config> entropy_shared::KeyProvider<T::AccountId> for Pallet<T> {


this looks inefficient to me, in both cases you call get_server_info making a read, but you call both the functions twice in the same function in attest (and maybe elsewhere) probably a good idea to take server_info as in input so you can reduce reads, or a function that gets both

Yeah not the most efficient thing, but basically it boils down to me not knowing how much of the server logic to expose to the attestation pallet.

I don't really want to couple it that tightly if we don't have to, and having a key provider with just the right amount of fields we need seemed like a nice compromise. For example, we could have these be computed on the fly for example, as opposed to being stuck with the storage read for ServerInfo.

JesseAbram · 2024-10-01T13:46:25Z

pallets/attestation/src/lib.rs

+                requests.push(account_id.encode());
+            }
+
+            AttestationRequests::<T>::insert(now, requests);


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

JesseAbram

happy with the issue looks good

HCastano added 10 commits September 23, 2024 16:10

Switch to using a StorageDoubleMap instead

2dd92b4

Update test mocks to use loose coupling

e41b8ec

Appease Clippy

ccce0c3

Add associated types to runtime implementation of pallet_attestation

382a6e6

Change how an X25519 public key is grabbed from storage

b7aeea5

We can't rely on the `ThresholdToStash` or `ThresholdServer` storage entries being populated anymore.

Update attestation pallet to use new interface

9b3f11c

Write happy path test for full validate + attest flow

0b54e11

Fix compliation of pallet_propagation tests

dd21370

TaploFmt

79c8237

HCastano mentioned this pull request Sep 24, 2024

Handle Provisioning Certification Keys (PCKs) #1051

Merged

HCastano added 19 commits September 24, 2024 17:49

Remove dbg! statements

06f664e

Use Weight::zero() instead of 0.into()

13f47c0

Fix staking pallet tests

54bf340

Add new event for validate extrinsic

07b5013

Inline some imports

bbdc743

Add some docs around the key provider and attestation queue

661121f

Clean up docs around validation queue

102fd57

Use counted double map for validation queue

e253047

We're going to try and use this to limit the number of entries for benchmarking purposes.

Use new NMap key syntax for staking tests

f76561b

Add limit to number of pending attestations in queue

ff90202

Add test for max pending attestations

8e85de2

Hardcode pending attestation limit

4683974

The compiler is complaining abou a missing `Div` implementation which I'm not really sure how to work around.

Add benchmark for Staking Extension's on_initialize hook

706c51d

Fix a few staking pallet benchmarks

9c53231

RustFmt

3449d1b

Add hack for writing to Staking pallet storage from Attestation benches

af06541

This isn't the best solution, but for now it needs to be done since the `attest()` extrinsic expects certain Staking pallet specific data structures to be populated before it has try and verify an attestation.

RustFmt

8acb8e5

Run through benchmark for Staking::on_intialize()

14af641

Run through benchmark for Attestation::on_intialize()

7cc89ff

HCastano added 9 commits September 27, 2024 18:12

Add new method to key handler for dealing with PCKs

ecdad8f

Bump TDX Quote and fix staking tests

9a02898

Clean up KeyProvider implementation

b59a008

Fix attestation benchmarks

79ec172

Fix staking benchmarks

f75a8ed

Rename X25519KeyProvider to KeyProvider

6f023f2

This reflects the more generic nature of the trait now.

RustFmt

143569c

Add check for expected values in test

d712e2d

Add note about pallet ordering for on_initialize()

07b1c31

HCastano commented Sep 27, 2024

View reviewed changes

HCastano added 4 commits September 27, 2024 18:51

RustFmt

d942404

Clean up some small things

78c74bf

Use randomness in quote generation

34f33c8

Add test randomness provider to Propagation mock

0cd8167

HCastano marked this pull request as ready for review September 30, 2024 18:28

HCastano requested review from ameba23 and JesseAbram September 30, 2024 18:28

Add CHANGELOG entry

a62cf31

HCastano added the Bump `spec_version` A change which affects the current runtime behaviour label Sep 30, 2024

ameba23 approved these changes Oct 1, 2024

View reviewed changes

ameba23 mentioned this pull request Oct 1, 2024

Handle PCK certificates #1068

Open

JesseAbram reviewed Oct 1, 2024

View reviewed changes

HCastano added 3 commits October 1, 2024 12:46

Change method comment into doccomment

56be187

Clarify accounts used in ValidationQueue

0e5187a

Add error type for missing PCK

a7c435d

HCastano mentioned this pull request Oct 1, 2024

Consolidate ValidationQueue and PendingAttestations #1089

Open

HCastano requested a review from JesseAbram October 1, 2024 17:09

JesseAbram approved these changes Oct 1, 2024

View reviewed changes

HCastano merged commit d1f2c96 into master Oct 2, 2024
8 checks passed

HCastano deleted the hc/add-attestation-check-to-validate branch October 2, 2024 15:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Trigger attestation check during `validate` #1063

Trigger attestation check during `validate` #1063

HCastano commented Sep 23, 2024 •

edited

Loading

HCastano Sep 27, 2024

ameba23 Sep 30, 2024

ameba23 left a comment

ameba23 Oct 1, 2024

HCastano Oct 1, 2024

ameba23 Oct 1, 2024

ameba23 Oct 1, 2024

HCastano Oct 1, 2024 •

edited

Loading

ameba23 Oct 1, 2024

JesseAbram Oct 1, 2024

HCastano Oct 1, 2024

ameba23 Oct 1, 2024

JesseAbram Oct 1, 2024

HCastano Oct 1, 2024

HCastano Oct 1, 2024

ameba23 Oct 1, 2024

HCastano Oct 1, 2024

ameba23 Oct 1, 2024

HCastano Oct 1, 2024

ameba23 Oct 1, 2024

HCastano Oct 1, 2024

JesseAbram left a comment

JesseAbram Oct 1, 2024

JesseAbram Oct 1, 2024

HCastano Oct 1, 2024

JesseAbram Oct 1, 2024

JesseAbram left a comment

		PendingAttestations::<T>::remove(&who);
		T::AttestationQueue::confirm_attestation(&who);

Trigger attestation check during validate #1063

Trigger attestation check during validate #1063

Conversation

HCastano commented Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ameba23 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HCastano Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JesseAbram left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JesseAbram left a comment

Choose a reason for hiding this comment

Trigger attestation check during `validate` #1063

Trigger attestation check during `validate` #1063

HCastano commented Sep 23, 2024 •

edited

Loading

HCastano Oct 1, 2024 •

edited

Loading