Extract a new extrinsics factory and nonce cache crate #498

murerfel · 2021-11-09T09:19:12Z

This is the next refactoring step for #301:

Extract the composition of extrinsics to a separate extrinsics-factory crate
Introduce a new nonce-cache crate to handle the enclave internal nonce
Extend the light client to send extrinsics to the parentchain

This is done because in the sidechain we need to compose and send extrinsics to the parentchain and so we need those parts of the code out of the enclave-runtime.

Closes #429

Merge Separate send xts and send blocks #487 first, then rebase this onto master

murerfel · 2021-11-09T09:19:58Z

Cargo.toml

+    "app-libs/stf",
    "cli",
-    "core/light-client",
    "core/direct-rpc-server",
-    "core/tls-websocket-server",
+    "core/light-client",
    "core/rest-client",
+    "core/rpc-client",
+    "core/rpc-server",
+    "core/tls-websocket-server",
    "core-primitives/api-client-extensions",
    "core-primitives/enclave-api",
    "core-primitives/enclave-api/ffi",
-    "core-primitives/types",
+    "core-primitives/extrinsics-factory",
+    "core-primitives/nonce-cache",
    "core-primitives/ocall-api",
-    "core-primitives/storage-verified",
-    "core-primitives/teerex-storage",
    "core-primitives/settings",
    "core-primitives/sgx/crypto",
    "core-primitives/sgx/io",
    "core-primitives/stf-executor",
    "core-primitives/stf-state-handler",
    "core-primitives/storage",
+    "core-primitives/storage-verified",
+    "core-primitives/teerex-storage",
    "core-primitives/test",
    "core-primitives/types",
-    "sidechain/sidechain-crate",
-    "sidechain/validateer-fetch",
-    "sidechain/primitives",
+    "service",
    "sidechain/consensus/aura",
    "sidechain/consensus/common",
    "sidechain/consensus/slots",
+    "sidechain/primitives",
    "sidechain/rpc-handler",
+    "sidechain/sidechain-crate",
    "sidechain/state",
    "sidechain/top-pool",
    "sidechain/top-pool-rpc-author",
-    "app-libs/stf",
-    "service",
-    "core/rpc-client",
-    "core/rpc-server",
-    #	 "enclave",
+    "sidechain/validateer-fetch",


Re-ordered the members alphabetically

murerfel · 2021-11-09T09:22:31Z

core-primitives/extrinsics-factory/src/lib.rs

+/// Create extrinsics from opaque calls
+///
+/// Also increases the nonce counter for each extrinsic that is created.
+pub trait CreateExtrinsics {
+	fn create_extrinsics(&self, calls: &[OpaqueCall]) -> Result<Vec<OpaqueExtrinsic>>;
+}


This is the new trait to create extrinsics, transforming OpaqueCall into OpaqueExtrinsic

murerfel · 2021-11-09T09:26:07Z

core-primitives/extrinsics-factory/src/lib.rs

+		let mut nonce_lock = self.nonce_cache.load_for_mutation()?;
+		let mut nonce_value = nonce_lock.0;
+
+		let extrinsics_buffer: Vec<OpaqueExtrinsic> = calls
+			.iter()
+			.map(|call| {
+				let xt = compose_extrinsic_offline!(
+					self.signer.clone(),
+					call,
+					nonce_value,
+					Era::Immortal,
+					self.genesis_hash,
+					self.genesis_hash,
+					RUNTIME_SPEC_VERSION,
+					RUNTIME_TRANSACTION_VERSION
+				)
+				.encode();
+				nonce_value += 1;
+				xt
+			})
+			.map(|xt| {
+				OpaqueExtrinsic::from_bytes(&xt)
+					.expect("A previously encoded extrinsic has valid codec; qed.")
+			})
+			.collect();
+
+		*nonce_lock = Nonce(nonce_value);


Retrieves the nonce write lock, creates all the extrinsics and increments the nonce value for each of them. Then at the end, write the nonce value back into the cache.
The reason we have a strict nonce lock here, is that we want to prevent any other concurrent process to get or write the nonce while we're incrementing it here.

murerfel · 2021-11-09T09:27:38Z

core-primitives/extrinsics-factory/src/lib.rs

+	// #[test]
+	// pub fn xts_have_increasing_nonce() {
+	// 	let nonce_cache = Arc::new(NonceCache::default());
+	// 	nonce_cache.set_nonce(Nonce(34)).unwrap();
+	// 	let extrinsics_factory =
+	// 		ExtrinsicsFactory::new(test_genesis_hash(), test_account(), nonce_cache);
+	//
+	// 	let opaque_calls =
+	// 		[OpaqueCall(vec![3u8; 42]), OpaqueCall(vec![12u8, 78]), OpaqueCall(vec![15u8, 12])];
+	// 	let xts: Vec<UncheckedExtrinsicV4<OpaqueCall>> = extrinsics_factory
+	// 		.create_extrinsics(&opaque_calls)
+	// 		.unwrap()
+	// 		.iter()
+	// 		.map(|mut x| UncheckedExtrinsicV4::<OpaqueCall>::decode(&mut x))
+	// 		.collect();
+	//
+	// 	assert_eq!(xts.len(), opaque_calls.len());
+	// 	assert_eq!(xts[0].signature.unwrap().2 .2, 34u128);
+	// }


I wanted to write a test where I create extrinsics and then check if the nonce in the resulting extrinsics is correct. Unfortunately, the nonce in the extrinsic is in a private member that I cannot access here 😞

If you want to be hardcore, you could check the relevant bits in the encoded extrinsics. :D.

Anyhow, we can make the nonce public in the api-client. I don't see anything against that.

murerfel · 2021-11-09T09:28:49Z

core-primitives/nonce-cache/src/lib.rs

+lazy_static! {
+	/// Global instance of a nonce cache
+	///
+	/// Concurrent access is managed internally, using RW locks
+	pub static ref GLOBAL_NONCE_CACHE: Arc<NonceCache> = Default::default();
+}


The global nonce cache instance that we can use in the enclave. Lazy initialized.

murerfel · 2021-11-09T09:30:27Z

core-primitives/nonce-cache/src/lib.rs

+pub mod error;
+pub mod nonce_cache;
+
+pub type NonceValue = u32;


NonceValue is an u32. I think there is an open issue where we want to connect this to another type definition Index?

Also I first tried to use u64 or even u128 to be more safe from overflows - but unfortunately deeper down in the extrinsics code, we use u32, so I'm forced to use u32 here too.

Yes, I created the issue yesterday because I noticed the hardcoded u32 in PR #487: #495.

I also think we shouldnt define the value here but rather in the settings or wherever we have our node primitves defined. But lets leave this task to the issue itself. This is atleast one step towards that goal, thanks.

murerfel · 2021-11-09T09:31:53Z

core-primitives/nonce-cache/src/nonce_cache.rs

+/// Local nonce cache
+///
+/// stores the nonce internally, protected by a RW lock for concurrent access
+#[derive(Default)]
+pub struct NonceCache {
+	nonce_lock: RwLock<Nonce>,
+}
+
+impl NonceCache {
+	pub fn new(nonce_lock: RwLock<Nonce>) -> Self {
+		NonceCache { nonce_lock }
+	}
+}
+
+impl MutateNonce for NonceCache {
+	fn load_for_mutation(&self) -> Result<RwLockWriteGuard<'_, Nonce>> {
+		self.nonce_lock.write().map_err(|_| Error::LockPoisoning)
+	}
+}
+
+impl GetNonce for NonceCache {
+	fn get_nonce(&self) -> Result<Nonce> {
+		let nonce_lock = self.nonce_lock.read().map_err(|_| Error::LockPoisoning)?;
+		Ok(*nonce_lock)
+	}
+}


The nonce cache is basically a wrapper around a RwLock, with a bit of convenience added to it.

murerfel · 2021-11-09T09:36:29Z

core/light-client/src/lib.rs

+	fn send_extrinsics<OCallApi: EnclaveOnChainOCallApi>(
+		&mut self,
+		ocall_api: &OCallApi,
+		extrinsics: Vec<OpaqueExtrinsic>,
+	) -> Result<(), Error>;


The light client is extended to have the send_extrinsics method. The OCallApi is passed as parameter, rather than having it as a member of the light client. The reason being, that the light client has to implement Encode and Decode, which will be more tricky with an Arc<OCallApi>.

Could you add some description here? Like that it sends the encoded extrinsics to the parentchain?

yes, done 👍

I think this is a good design. 👍

murerfel · 2021-11-09T09:37:54Z

enclave-runtime/src/lib.rs

-lazy_static! {
-	/// the enclave's parentchain nonce
-	///
-	/// This should be abstracted away better in the future. Currently, this only exists
-	/// because we produce sidechain blocks faster that parentchain chain blocks. So for now this
-	/// design suffices. Later, we also need to sync across parallel ecalls that might both result
-	/// in parentchain xt's. Then we should probably think about how we want to abstract the global
-	/// nonce.
-	static ref NONCE: SgxRwLock<u32> = SgxRwLock::new(Default::default());
-}
-


Remove the global nonce value here, in favor of the nonce cache.

murerfel · 2021-11-09T09:39:18Z

enclave-runtime/src/lib.rs


-	*nonce += 1;
+	*nonce_lock = Nonce(nonce_value + 1);
+	std::mem::drop(nonce_lock);


just a small optimization: we drop the nonce write lock early, to minimize the time we're holding the lock.

murerfel · 2021-11-09T09:40:06Z

enclave-runtime/src/lib.rs

+	let genesis_hash = validator.genesis_hash(validator.num_relays())?;
+	let extrinsics_factory =
+		ExtrinsicsFactory::new(genesis_hash, signer, GLOBAL_NONCE_CACHE.clone());


We now get the genesis hash from the light client when we crate an extrinsics factory

murerfel · 2021-11-09T09:41:20Z

enclave-runtime/src/sidechain_impl.rs

 //! move most parts here to the sidechain crate.

-use crate::{execute_top_pool_trusted_calls, prepare_and_send_xts, Result as EnclaveResult};
+use crate::{execute_top_pool_trusted_calls, Result as EnclaveResult};


The main reason we're doing this refactoring: refactor and remove the prepare_and_send_xts function from the enclave-runtime, so we can move this sidechain_impl code back into the sidechain crate.

(We still have the execute_top_pool_trusted_calls function in enclave-runtime - that will be refactored in the next PR)

haerdib · 2021-11-09T09:42:46Z

core-primitives/extrinsics-factory/src/lib.rs

+
+/// Create extrinsics from opaque calls
+///
+/// Also increases the nonce counter for each extrinsic that is created.


Ohh, thanks alot for that one ❤️

haerdib · 2021-11-09T10:00:06Z

core-primitives/nonce-cache/Cargo.toml

+[features]
+default = ["std"]
+std = [
+    "thiserror",


Suggested change

"thiserror",

"thiserror",

"log/std"

haerdib · 2021-11-09T10:03:44Z

core-primitives/nonce-cache/src/lib.rs

+pub mod error;
+pub mod nonce_cache;
+
+pub type NonceValue = u32;


Yes, I created the issue yesterday because I noticed the hardcoded u32 in PR #487: #495.

I also think we shouldnt define the value here but rather in the settings or wherever we have our node primitves defined. But lets leave this task to the issue itself. This is atleast one step towards that goal, thanks.

haerdib · 2021-11-09T10:10:47Z

core/light-client/src/lib.rs

+	fn send_extrinsics<OCallApi: EnclaveOnChainOCallApi>(
+		&mut self,
+		ocall_api: &OCallApi,
+		extrinsics: Vec<OpaqueExtrinsic>,
+	) -> Result<(), Error>;


Could you add some description here? Like that it sends the encoded extrinsics to the parentchain?

Also introduce a nonce cache in the process

clangenb

Looks very good, only some minor questions.

clangenb · 2021-11-10T10:05:56Z

core-primitives/extrinsics-factory/src/lib.rs

+	// #[test]
+	// pub fn xts_have_increasing_nonce() {
+	// 	let nonce_cache = Arc::new(NonceCache::default());
+	// 	nonce_cache.set_nonce(Nonce(34)).unwrap();
+	// 	let extrinsics_factory =
+	// 		ExtrinsicsFactory::new(test_genesis_hash(), test_account(), nonce_cache);
+	//
+	// 	let opaque_calls =
+	// 		[OpaqueCall(vec![3u8; 42]), OpaqueCall(vec![12u8, 78]), OpaqueCall(vec![15u8, 12])];
+	// 	let xts: Vec<UncheckedExtrinsicV4<OpaqueCall>> = extrinsics_factory
+	// 		.create_extrinsics(&opaque_calls)
+	// 		.unwrap()
+	// 		.iter()
+	// 		.map(|mut x| UncheckedExtrinsicV4::<OpaqueCall>::decode(&mut x))
+	// 		.collect();
+	//
+	// 	assert_eq!(xts.len(), opaque_calls.len());
+	// 	assert_eq!(xts[0].signature.unwrap().2 .2, 34u128);
+	// }


If you want to be hardcore, you could check the relevant bits in the encoded extrinsics. :D.

Anyhow, we can make the nonce public in the api-client. I don't see anything against that.

clangenb · 2021-11-10T10:10:20Z

core/light-client/src/lib.rs

+	fn send_extrinsics<OCallApi: EnclaveOnChainOCallApi>(
+		&mut self,
+		ocall_api: &OCallApi,
+		extrinsics: Vec<OpaqueExtrinsic>,
+	) -> Result<(), Error>;


I think this is a good design. 👍

clangenb · 2021-11-10T10:14:03Z

enclave-runtime/src/lib.rs

+	let xts = extrinsics_factory.create_extrinsics(calls.as_slice())?;

-	ocall_api
-		.send_to_parentchain(extrinsics)
-		.map_err(|e| Error::Other(format!("Failed to send extrinsics: {}", e).into()))?;
+	validator.send_extrinsics(on_chain_ocall_api, xts)?;



Looks very nice now. :)

clangenb · 2021-11-10T10:16:50Z

core-primitives/nonce-cache/src/lib.rs

+
+pub type NonceValue = u32;
+
+/// Nonce type (newtype wrapper for u64)


I can't understand the purpose here. Can you please explain it to me? 😄

you mean the purpose of a newtype for the Nonce? My intention was to have a strong type for the nonce. So in cases where we have other u32 values/parameters, it prevents mixing them by accident. Imagine a function that has 2 parameters:

fn do_work(nonce: u32, my_counter: u32);

When I call this function, it can happen that I mix up the two parameters, because their types are interchangeable. If I have a newtype for the nonce, that cannot happen.

But I see now that the comment is not correct anymore, since we don't have a u64 😛 An example where comments can get out of sync with the code very easily.

I see. I am much in favor of using strong typing here :). However, the comment was indeed misleading. 😄

Ah so you're initial question was regarding the comment? Sorry I misunderstood then 😄 The comment is now fixed in the latest commit.

clangenb

Looks good to me! We are making some serious progress here. :)

author echevrier <[email protected]> 1633529369 +0200 committer echevrier <[email protected]> 1636648987 +0100 Exchange Rate Oracle Showcase Add exchange-oracle crate and implement get_exchange_rate for coinGecko Clean according review Call update market data (Exchange rate) from enclave Rename pallet market to teeracle cargo fmt Set MARKET_DATA_UPDATE_INTERVAL to 24h [GA] bump node artifact Changes from review Set substrate-fixed version to 0.5.6 (before update to new substrate) Changes from review Rebase Changes due to review Rebase to get PR #498 to rework code according review

…ko (#442) author echevrier <[email protected]> 1633529369 +0200 committer echevrier <[email protected]> 1636648987 +0100 Exchange Rate Oracle Showcase Add exchange-oracle crate and implement get_exchange_rate for coinGecko Clean according review Call update market data (Exchange rate) from enclave Rename pallet market to teeracle cargo fmt Set MARKET_DATA_UPDATE_INTERVAL to 24h [GA] bump node artifact Changes from review Set substrate-fixed version to 0.5.6 (before update to new substrate) Changes from review Rebase Changes due to review Rebase to get PR #498 to rework code according review * Rebase * cargo clippy and catch error in execute_update_market to prevent that the thread is stopped * settings for demo * update CI because this branch will not be merged in master, but in exchange_rate_oracle branch * changes from review * changes from review * Remove reference to light client for oracle * Set update interval for exchange range to 24h * Changes review * Changes review * Cargo clippy * Changes from review: improve variable names Co-authored-by: echevrier <[email protected]>

Add exchange-oracle crate and implement get_exchange_rate for coinGecko Clean according review Call update market data (Exchange rate) from enclave Rename pallet market to teeracle cargo fmt Set MARKET_DATA_UPDATE_INTERVAL to 24h [GA] bump node artifact Changes from review Set substrate-fixed version to 0.5.6 (before update to new substrate) Changes from review Rebase Changes due to review Rebase to get PR #498 to rework code according review * Rebase * cargo clippy and catch error in execute_update_market to prevent that the thread is stopped * settings for demo * update CI because this branch will not be merged in master, but in exchange_rate_oracle branch * changes from review * changes from review * Remove reference to light client for oracle * Set update interval for exchange range to 24h * Changes review * Changes review * Cargo clippy * Changes from review: improve variable names Co-authored-by: echevrier <[email protected]> Cargo test fix

murerfel self-assigned this Nov 9, 2021

murerfel commented Nov 9, 2021

View reviewed changes

murerfel requested review from clangenb and haerdib November 9, 2021 09:20

murerfel commented Nov 9, 2021

View reviewed changes

murerfel force-pushed the feature/fm-extrinsics-factory-nonce-cache branch from 11a7a2e to 40d8f29 Compare November 9, 2021 09:54

haerdib reviewed Nov 9, 2021

View reviewed changes

Base automatically changed from feature/fm-separate-send-xts-send-blocks to master November 9, 2021 11:07

murerfel mentioned this pull request Nov 10, 2021

Add exchange-oracle crate and implement get_exchange_rate for coinGecko #442

Merged

Felix Müller added 2 commits November 10, 2021 10:00

Extract the extrinsics composition to a new extrinsics factory crate

9735c4d

Also introduce a nonce cache in the process

PR fixes, comments and use /std feature explicitly for all dependencies

709bb97

murerfel force-pushed the feature/fm-extrinsics-factory-nonce-cache branch from 40d8f29 to 709bb97 Compare November 10, 2021 09:04

murerfel mentioned this pull request Nov 10, 2021

Extract the top pool operation execution into separate modules #500

Merged

1 task

fix cargo test for extrinsics factory

90f0c76

clangenb reviewed Nov 10, 2021

View reviewed changes

minor fix for comment

e9fc1af

murerfel requested review from clangenb and haerdib November 10, 2021 13:41

clangenb approved these changes Nov 10, 2021

View reviewed changes

murerfel merged commit e959c9c into master Nov 10, 2021

murerfel deleted the feature/fm-extrinsics-factory-nonce-cache branch November 10, 2021 13:49


		pub type NonceValue = u32;

		/// Nonce type (newtype wrapper for u64)

Extract a new extrinsics factory and nonce cache crate #498

Extract a new extrinsics factory and nonce cache crate #498

Uh oh!

Conversation

murerfel commented Nov 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clangenb Nov 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clangenb left a comment

Choose a reason for hiding this comment

Uh oh!

clangenb Nov 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clangenb left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

murerfel commented Nov 9, 2021 •

edited

Loading

clangenb Nov 10, 2021 •

edited

Loading

clangenb Nov 10, 2021 •

edited

Loading