Attestation pallet #1003

ameba23 · 2024-08-13T09:09:38Z

This is part of #982

This adds a pallet for TDX attestation.

The pallet stores the nonces of all pending (requested) attestations, storing them under associated TSS account ID. So there may be at most one pending attestation per TS server. The nonce is just a random 32 bytes, which is included in the input data to the TDX quote, to prove that this is a freshly made quote.

An attestation request is responded to by submitting the quote using the attest extrinsic. If there was a pending attestation for the caller, the quote is verified. Verification currently just means checking that the quote parses correctly and has a valid signature. But this would eventually also check the build-time or run-time measurement details match our current release and that the public key matches the corresponding PCK certificate. PCK certificates will be handled in a separate PR.

If the quote fails to verify, something should happen to the validator - eg: remove them from the signing committee or block them from joining. This is outside of the scope of this PR and can be handled as part of the slashing feature.

The attestation pallet also stores a mapping of block number to TSS account IDs of nodes for who an attestation request should be initiated. This is used by the propagation pallet to make a POST request to the TS server's /attest endpoint whenever there are requests to be made. Currently, the only place these attestation requests are initiated is in the genesis config for testing.

* master: TSS attestation endpoint (#1001)

* master: Bump serde_json from 1.0.124 to 1.0.125 in the patch-dependencies group (#1007) Signing flow with derived accounts (#990) Add `network-jumpstart` command to `entropy-test-cli` (#1004) Refactor reshare (#994) Migrate circle-ci workflow to github actions (#991) Delete old keyshare if not in next_signers (#999)

…pallet to call it

ameba23 · 2024-08-20T11:58:11Z

crates/shared/src/types.rs

@@ -105,14 +114,14 @@ pub type EncodedVerifyingKey = [u8; VERIFICATION_KEY_LENGTH as usize];
 pub struct QuoteInputData(pub [u8; 64]);

 impl QuoteInputData {
-    pub fn new(
-        tss_account_id: [u8; 32],
+    pub fn new<T: Encode>(


This is just so it can be used with mock account ids in the attestation pallet test.

ameba23 · 2024-08-20T12:00:25Z

crates/threshold-signature-server/src/attestation/api.rs


 /// HTTP POST endpoint to initiate a TDX attestation.
-/// Not yet implemented.
-#[cfg(not(any(test, feature = "unsafe")))]


Before we had a production and mock version of the endpoint function - but that is now replaced with a common endpoint function which internally calls either a mock or production version of the create_quote function

ameba23 · 2024-08-20T12:02:21Z

crates/threshold-signature-server/src/attestation/tests.rs

+            entropy::storage().attestation().pending_attestations(&TSS_ACCOUNTS[0]);
+        if query_chain(&api, &rpc, pending_attestation_query, None).await.unwrap().is_none() {
+            return;
+        }


This test isn't ideal - but currently the only outcome of a successful attestation is that there is no longer a pending attestation.

JesseAbram

Looks good a few issues need to be made

JesseAbram · 2024-08-20T20:41:17Z

crates/threshold-signature-server/src/attestation/api.rs

-    use crate::{chain_api::get_rpc, get_signer_and_x25519_secret};
-    use rand_core::OsRng;
-    use sp_core::Pair;
+) -> Result<StatusCode, AttestationErr> {


we will have to validate this message agains blockchain data (can be in a later PR but needs an issue

I'll put it in the issue - i am adding a bunch of things into the once issue for this feature.

But since we check below with the chain that there is a pending attestation, im not sure what we gain by also checking the OcwMessageAttestationRequest matches on chain.

JesseAbram · 2024-08-20T20:42:37Z

crates/threshold-signature-server/src/attestation/api.rs

+            entropy::storage().attestation().pending_attestations(signer.account_id());
+        query_chain(&api, &rpc, pending_attestation_query, None)
+            .await?
+            .ok_or_else(|| AttestationErr::Unexpected)?


wouldn't hate a better error message here, a generalize one for querying chains maybe that takes a string of a better description

JesseAbram · 2024-08-20T20:48:43Z

pallets/attestation/src/lib.rs

+    /// attestation, used to make attestation requests via an offchain worker
+    #[pallet::storage]
+    #[pallet::getter(fn attestation_requests)]
+    pub type AttestationRequests<T: Config> =


idea here, we can make a trailing request of X blocks on an on init hook that boots a validator if they did not complete their attestation

I've added this to the TODOs in the issue

JesseAbram · 2024-08-20T20:49:23Z

pallets/attestation/src/lib.rs

+            );
+
+            // Remove the entry from PendingAttestations
+            PendingAttestations::<T>::remove(&who);


this should be below the other two probably

I've moved it lower down for now but Im not sure about this.

I think once we have some specific outcome for a failed attestation, we will still want to remove the pending attestation even if it is bad. So pending attestations will just be ones that haven't been responded to yet and we will have some other way of representing ones which have been responded to but were bad.

JesseAbram · 2024-08-20T20:49:52Z

pallets/attestation/src/lib.rs

+
+            // Remove the entry from PendingAttestations
+            PendingAttestations::<T>::remove(&who);
+


Idea: we put in the validator info when their last successful attestation was made to chain

HCastano

Left some comments and questions, but nothing major. Good work 👍

Make sure to add a CHANGELOG entry before merging

HCastano · 2024-08-20T21:14:36Z

pallets/attestation/src/lib.rs

+// 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/>.
+
+//! # Attestation Pallet


Some docs about what this is would be nice

HCastano · 2024-08-20T21:17:34Z

pallets/attestation/src/lib.rs

+    #[derive(frame_support::DefaultNoBound)]
+    pub struct GenesisConfig<T: Config> {
+        pub initial_pending_attestations: Vec<(T::AccountId, [u8; 32])>,
+        pub initial_attestation_requests: Vec<(BlockNumberFor<T>, Vec<Vec<u8>>)>,


Suggested change

pub initial_attestation_requests: Vec<(BlockNumberFor<T>, Vec<Vec<u8>>)>,

pub initial_attestation_requests: Vec<(BlockNumberFor<T>, Vec<T::AccountId>)>,

Why not do something like this instead?

Good point. I thought it is not possible to have a <T::AccountId> as a value in a StorageMap. As in all our other pallets, we use Vec<u8>. But actually i just tried it, and it compiled fine.

So now the issue is that in entropy_shared::OcwMessageAttestationRequest we represent this as a [u8; 32]. Since theres no way to turn a generic T::AccountId into a [u8; 32], we would have to make OcwMessageAttestationRequest generic for chain config.

I'd be happy to do this, but i have the feeling it there might be issues with deriving Serialize and Deserialize and probably it would make sense to also do it to all the other types we have in entropy-shared which represent account ids with Vec<u8>. So maybe its for another PR.

HCastano · 2024-08-20T21:17:57Z

pallets/attestation/src/lib.rs

+    #[pallet::storage]
+    #[pallet::getter(fn pending_attestations)]
+    pub type PendingAttestations<T: Config> =
+        StorageMap<_, Blake2_128Concat, T::AccountId, [u8; 32], OptionQuery>;


We could typedef this as type Nonce = [u8; 32]

HCastano · 2024-08-20T21:20:21Z

pallets/attestation/src/lib.rs

+        BadQuote,
+        UnexpectedAttestation,
+        IncorrectInputData,
+        NoStashAccount,
+        NoServerInfo,


I know we don't do it for the other pallets, but let's try and add doc comments before it's too late lol

HCastano · 2024-08-20T21:21:05Z

pallets/attestation/src/lib.rs

+    impl<T: Config> Pallet<T> {
+        #[pallet::call_index(0)]
+        #[pallet::weight({100})]
+        pub fn attest(origin: OriginFor<T>, quote: Vec<u8>) -> DispatchResult {


Missing doc comments. Would be helpful to know what a quote should look like for example

HCastano · 2024-08-20T21:35:06Z

pallets/attestation/src/benchmarking.rs

+    assert_last_event::<T>(
+        Event::<T>::AttestationMade.into()
+    );


Here we can also test for expected behaviour.

For example, we can check that PendingAttestations doesn't have the validator ID anymore, or that expected events were emitted

Good idea. I've added the check that there is no longer a pending attestation. Not sure what other events i could check for.

HCastano · 2024-08-20T21:40:19Z

pallets/propagation/src/lib.rs

+
+                let deadline = sp_io::offchain::timestamp().add(Duration::from_millis(2_000));
+                let kind = sp_core::offchain::StorageKind::PERSISTENT;
+                let from_local = sp_io::offchain::local_storage_get(kind, b"attest")


I think we also need to reflect this in the service

Ah ok yep will do.

Although i find it strange that we store a bunch of these urls for each endpoint, rather than just a top level one.

HCastano · 2024-08-20T21:44:11Z

node/cli/src/chain_spec/dev.rs

@@ -336,5 +336,9 @@ pub fn development_genesis_config(
                10,
            )],
        },
+        "attestation": AttestationConfig {
+            initial_attestation_requests: vec![(3, vec![crate::chain_spec::tss_account_id::ALICE.to_raw_vec()])],


Do you mind using an account instead of 3 here? Either a typedef or the result from one of the other helpful functions is fine

This is a block number, not an account id

crates/threshold-signature-server/src/attestation/api.rs

HCastano · 2024-08-20T21:51:50Z

crates/threshold-signature-server/src/attestation/tests.rs

+    let api = get_api(&cxt.ws_url).await.unwrap();
+    let rpc = get_rpc(&cxt.ws_url).await.unwrap();
+
+    // Check that there is an attestation request at block 3 from the genesis config


Why block 3?

This was one of those problems which kind of got sorted but i don't completely understand why.

The propagation pallet's off chain worker hooks which run every block don't run on the genesis block. It seems to me that they also don't run on block one - although me and Jesse were getting different behavior. In the end i've ended up with block 3, and also am not subtracting 1 from the block number. Probably this could be either changed to 2, or the subtract 1 could be put back in.

Co-authored-by: Hernando Castano <[email protected]>

WIP attestation pallet

55c9af2

ameba23 marked this pull request as draft August 13, 2024 09:09

ameba23 and others added 28 commits August 13, 2024 11:50

Merge branch 'master' into peg/attestation-pallet

c5df03f

* master: TSS attestation endpoint (#1001)

Improve checks for attest extrinsic

09e877e

Give constant weight just to get it to compile (with warning)

6fe7070

Attempt to write mock runtime

8cc3457

Write trivial test to show mock setup works

0a7ee3c

Clean attestation pallet (#1006)

d9d4a80

fmt

d106227

Make an attestation in the test

430b372

Lockfile

5a2206f

Taplo

305120b

Comments

7425039

Make quote input constructor generic, rm authorship pallet

905359d

Taplo

254a89a

Add benchmarks

2c462b8

Taplo

37d08d0

Add pallet to chain runtime

f57303b

Rm unused import

fb5a4b7

Improve benchmark

69dd18e

Generate weights

74a83ab

Include weights in runtime

e27262a

Update metadata

175166b

Submit the attest extrinsic to test the attestation pallet

c7db0ef

Fixes for submitting extrinsic

01213d1

Update attestation endpoint in preparation for using the propagation …

19c7a7a

…pallet to call it

Propagation pallet propagates attestation requests

8d1f3ca

Decode input from propagation pallet

33797ef

Add initial attestation requests to genesis config

b6f69b8

ameba23 added 6 commits August 19, 2024 12:12

Add initial attestation requests to chainspec

314dc9a

Merge master

7843545

Pull chain metadata

76d95eb

Make mock attestation request at block 3

3ef7fe8

Fix test

fe93e35

Tidy

0abb8be

ameba23 commented Aug 20, 2024

View reviewed changes

ameba23 marked this pull request as ready for review August 20, 2024 12:04

ameba23 requested review from JesseAbram and HCastano August 20, 2024 12:10

JesseAbram approved these changes Aug 20, 2024

View reviewed changes

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

HCastano approved these changes Aug 20, 2024

View reviewed changes

ameba23 and others added 7 commits August 21, 2024 08:45

Update crates/threshold-signature-server/src/attestation/api.rs

cdd22b4

Co-authored-by: Hernando Castano <[email protected]>

Extra check in attestation pallet benchmarks

0e75228

Improve error message for unexpected attestation

d76bb0f

Use actual weight

8ad0c8d

Add endpoint to node service

dba2907

Comments and minor changed following review

9379469

Changelog

2aca946

ameba23 mentioned this pull request Aug 21, 2024

DCAP attestation feature design #982

Open

Merge master

8d07bf9

ameba23 merged commit 305c4be into master Aug 21, 2024
8 checks passed

ameba23 deleted the peg/attestation-pallet branch August 21, 2024 10:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Attestation pallet #1003

Attestation pallet #1003

ameba23 commented Aug 13, 2024 •

edited

Loading

ameba23 Aug 20, 2024

ameba23 Aug 20, 2024

ameba23 Aug 20, 2024

JesseAbram left a comment

JesseAbram Aug 20, 2024

ameba23 Aug 21, 2024

JesseAbram Aug 20, 2024

JesseAbram Aug 20, 2024

ameba23 Aug 21, 2024

JesseAbram Aug 20, 2024

ameba23 Aug 21, 2024

JesseAbram Aug 20, 2024

HCastano left a comment

HCastano Aug 20, 2024

HCastano Aug 20, 2024

ameba23 Aug 21, 2024

HCastano Aug 20, 2024

HCastano Aug 20, 2024

HCastano Aug 20, 2024

HCastano Aug 20, 2024

ameba23 Aug 21, 2024

HCastano Aug 20, 2024

ameba23 Aug 21, 2024

HCastano Aug 20, 2024

ameba23 Aug 21, 2024

HCastano Aug 20, 2024

ameba23 Aug 21, 2024


		// Remove the entry from PendingAttestations
		PendingAttestations::<T>::remove(&who);

	pub initial_attestation_requests: Vec<(BlockNumberFor<T>, Vec<Vec<u8>>)>,
	pub initial_attestation_requests: Vec<(BlockNumberFor<T>, Vec<T::AccountId>)>,

Attestation pallet #1003

Attestation pallet #1003

Conversation

ameba23 commented Aug 13, 2024 • edited Loading

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

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

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

ameba23 commented Aug 13, 2024 •

edited

Loading