send personhood to node template #37

brenzi · 2023-09-11T06:23:49Z

meaningless demo just calling template pallet do_something with a user-chosen index if has personhood

very general demonstration of trustlessly reading state from one chain and processing data and committing with an extrinsic into another chain: unlinkable XCM demo.

brenzi · 2023-09-11T06:24:08Z

trouble sending an extrinsic to target B
https://gist.github.com/brenzi/530be00e9de0dc9bf16d33a3ea624025#file-gistfile1-txt-L152-L156
the encoded call is correct, but the extrinsic factory returns an empty slice which isn't handled gracefully

edit: slice wasn't empty, but inside enclave the debug rendering is null. the problem was too little funds to pay for fees

brenzi · 2023-09-11T06:59:57Z

maybe try to align pallet indices in node-template with other nodes:

		System: frame_system = 0,
		Timestamp: pallet_timestamp = 3,
		Aura: pallet_aura = 23,
		Grandpa: pallet_grandpa = 25,
		Balances: pallet_balances = 10,
		TransactionPayment: pallet_transaction_payment = 11,
		Sudo: pallet_sudo = 5,
		// Include the custom logic from the pallet-template in the runtime.
		TemplateModule: pallet_template = 7,

see integritee-network/worker#1445

edit: does not fix the empty extrinsic error

brenzi · 2023-09-11T18:07:30Z

core-primitives/types/src/parentchain.rs

+#[cfg(feature = "std")]
+impl Display for ParentchainId {
+	fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
+		let message = match self {
+			ParentchainId::Integritee => "L1:Integritee",
+			ParentchainId::TargetA => "L1:Encointer",
+			ParentchainId::TargetB => "L1:NodeTemplate",
+		};
+		write!(f, "{}", message)
+	}
+}
+


I'd like this to be upstreamed and customizeable. but it should work inside enclave too and should replace all log prefixes

Display is part of core, so it can work inside the enclave too.

I am unsure if it makes sense to replace the general log prefixes. It is not the same as in the substrate runtime, where we know that the encointer logger target is always in the runtime. Here it can be any crate in a fairly complex setup, so we should keep the crate path in in the log IMO.

pardon, I didn't mean the pallet log prefixes. I meant the ones where we deliberately printed TargetA/B/Integritee before.
crate path in the log is fine, but the parentchain should become part of the prefix or be therwise prepended to the log message where it makes sense

brenzi · 2023-09-11T18:09:20Z

service/src/main.rs

+fn print_events<R, H>(events: Vec<EventRecord<R, H>>, parentchain_id: ParentchainId)
+where
+	R: Debug,
+{
 	for evr in &events {
-		debug!("Decoded: phase = {:?}, event = {:?}", evr.phase, evr.event);
-		match &evr.event {
-			RuntimeEvent::Balances(be) => {
-				info!("[+] Received balances event");
-				debug!("{:?}", be);
-				match &be {
-					pallet_balances::Event::Transfer {
-						from: transactor,
-						to: dest,
-						amount: value,
-					} => {
-						debug!("    Transactor:  {:?}", transactor.to_ss58check());
-						debug!("    Destination: {:?}", dest.to_ss58check());
-						debug!("    Value:       {:?}", value);
-					},
-					_ => {
-						trace!("Ignoring unsupported balances event");
-					},
-				}
-			},
-			_ => ()
+		if evr.phase == ApplyExtrinsic(0) {
+			// not interested in intrinsics
+			continue
 		}
+		let re = Regex::new(r"\s[0-9a-f]*\s\(").unwrap();
+		let event_str = re
+			.replace_all(format!("{:?}", evr.event).as_str(), "(")
+			.replace("RuntimeEvent::", "")
+			.replace("Event::", "");
+		println!("[{}] Event: {}", parentchain_id, event_str);


we were lucky. no need to map it all, we can just use the debug printed event which is nicely human readable already. some string magic helps to keeps lines short

As you mention, regex is always black magic, so I would be very happy to see a function here :)

// format the event to keep the lines short fn format_event(event: &str) -> String // or maybe fn format_event<'a>(event: &'a str) -> &'a str

brenzi · 2023-09-11T18:10:14Z

service/src/main.rs

@@ -690,7 +710,7 @@ fn init_target_parentchain<E>(
 fn init_parentchain<E>(
 	enclave: &Arc<E>,
 	node_api: &ParentchainApi,
-	tee_account_id: &AccountId32,
+	tee_account_id: Option<AccountId32>,


the idea is: if None, that means that no extrinsics need ever to be sent to this parentchain

I think the idea is good, but it should not be documented as a PR comment ;)

Same as before, this can also be an Option<&AccountId32>.

brenzi · 2023-09-11T18:12:25Z

service/src/account_funding.rs

+	debug!("Existential deposit is = {:?}", existential_deposit);
+	let balance_transfer_fee = api
+		.get_fee_details(
+			api.balance_transfer_allow_death(
+				MultiAddress::Address32([0u8; 32]),
+				existential_deposit,
+			)
+			.encode()
+			.into(),
+			None,
+		)
+		.unwrap()
+		.unwrap()
+		.inclusion_fee
+		.unwrap()
+		.inclusion_fee();
+	debug!("Balance Transfer Fee is = {:?}", balance_transfer_fee);
+
+	let min_required_funds = existential_deposit
+		.saturating_mul(EXISTENTIAL_DEPOSIT_FACTOR_FOR_INIT_FUNDS)
+		.saturating_add(balance_transfer_fee.saturating_mul(100));


this should probably be upstreamed as it is more robust than the previous solution

I think that the .saturating_mul(EXISTENTIAL_DEPOSIT_FACTOR_FOR_INIT_FUNDS) is not a good way to determine the min_required_funds. It has no connection to any onchain metric - it is just an arbitrary value set by us. I think it is better to remove it and increase the factor for the balance_transfer_fee, i.e.:

// ensure that we can send 10_000 transfers before the account falls below the existential deposit. let min_required_funds = existential_deposit .saturating_add(balance_transfer_fee.saturating_mul(10_000));

Then we have set a value for min_required_fund that we can relate to an actual onchain significance.

brenzi · 2023-09-12T07:10:25Z

Resulting screencast video

clangenb

Nice, this looks good! Only some details.

clangenb · 2023-10-03T11:55:08Z

cli/src/personhood_oracle/commands/issue_node_template_xt.rs

+				error!("{}", &error_msg);
+				let inner_error_msg: String =
+					Decode::decode(&mut rpc_return_value.value.as_slice())
+						.expect("Failed to decode node tempölate xt issuing RPC error msg");


clangenb · 2023-10-03T11:55:22Z

cli/src/personhood_oracle/commands/issue_node_template_xt.rs

+
+impl IssueNodeTemplateXtCmd {
+	pub fn run(&self, cli: &Cli) {
+		//todo!();


What is this todo about?

clangenb · 2023-10-03T11:58:41Z

cli/src/personhood_oracle/commands/issue_node_template_xt.rs

+		let rpc_params = rpc_params
+			.into_iter()
+			.map(|p| itp_utils::hex::hex_encode(p.as_slice()))
+			.collect();


uii, if this is necessary for our rpc api, then we have to improve it, but it indeed seems that we use a similar approach in the worker.

clangenb · 2023-10-03T12:00:40Z

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

+				trace!(
+					"encoded extrinsic with nonce {}: 0x{}",
+					nonce_value,
+					hex::encode(xt.clone())


I'd remove this trace as it introduces a clone

the idea here is to format the encoded call exactly the way it could be copy-pasted and inspected with js/apps. So I'd rather leave it as is - unless you have a way to achieve exactly the same without the clone

clangenb · 2023-10-03T12:00:57Z

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

-					.expect("A previously encoded extrinsic has valid codec; qed.")
+				let oe = OpaqueExtrinsic::from_bytes(&xt)
+					.expect("A previously encoded extrinsic has valid codec; qed.");
+				trace!("opaque extrinsic: {:?}", oe);


Isn't this the log that is empty?

clangenb · 2023-10-03T12:28:42Z

service/src/main.rs

@@ -690,7 +710,7 @@ fn init_target_parentchain<E>(
 fn init_parentchain<E>(
 	enclave: &Arc<E>,
 	node_api: &ParentchainApi,
-	tee_account_id: &AccountId32,
+	tee_account_id: Option<AccountId32>,


I think the idea is good, but it should not be documented as a PR comment ;)

clangenb · 2023-10-03T12:33:55Z

service/src/main.rs

 		}
 	}
 }

 fn init_target_parentchain<E>(
 	enclave: &Arc<E>,
-	tee_account_id: &AccountId32,
+	tee_account_id: Option<AccountId32>,


This might be a bit too nitpicky, but as we only use ref account below, this can be an Option<&AccountId>

clangenb · 2023-10-03T12:35:00Z

service/src/main.rs

@@ -690,7 +710,7 @@ fn init_target_parentchain<E>(
 fn init_parentchain<E>(
 	enclave: &Arc<E>,
 	node_api: &ParentchainApi,
-	tee_account_id: &AccountId32,
+	tee_account_id: Option<AccountId32>,


Same as before, this can also be an Option<&AccountId32>.

clangenb · 2023-10-03T12:40:34Z

service/src/ocall_bridge/worker_on_chain_ocall.rs

@@ -129,6 +129,7 @@ where
 				extrinsics.len(),
 				parentchain_id
 			);
+			trace!("extrinsics: {:?}", extrinsics);


I think we spam too many extrinsic logs. :)

clangenb · 2023-10-03T12:46:32Z

service/src/main.rs

+fn print_events<R, H>(events: Vec<EventRecord<R, H>>, parentchain_id: ParentchainId)
+where
+	R: Debug,
+{
 	for evr in &events {
-		debug!("Decoded: phase = {:?}, event = {:?}", evr.phase, evr.event);
-		match &evr.event {
-			RuntimeEvent::Balances(be) => {
-				info!("[+] Received balances event");
-				debug!("{:?}", be);
-				match &be {
-					pallet_balances::Event::Transfer {
-						from: transactor,
-						to: dest,
-						amount: value,
-					} => {
-						debug!("    Transactor:  {:?}", transactor.to_ss58check());
-						debug!("    Destination: {:?}", dest.to_ss58check());
-						debug!("    Value:       {:?}", value);
-					},
-					_ => {
-						trace!("Ignoring unsupported balances event");
-					},
-				}
-			},
-			_ => ()
+		if evr.phase == ApplyExtrinsic(0) {
+			// not interested in intrinsics
+			continue
 		}
+		let re = Regex::new(r"\s[0-9a-f]*\s\(").unwrap();
+		let event_str = re
+			.replace_all(format!("{:?}", evr.event).as_str(), "(")
+			.replace("RuntimeEvent::", "")
+			.replace("Event::", "");
+		println!("[{}] Event: {}", parentchain_id, event_str);


As you mention, regex is always black magic, so I would be very happy to see a function here :)

// format the event to keep the lines short fn format_event(event: &str) -> String // or maybe fn format_event<'a>(event: &'a str) -> &'a str

Co-authored-by: clangenb <[email protected]>

brenzi · 2023-10-04T13:24:16Z

generally, I think we should revisit your review comments when we upstream the cherries here. I'd rather not polish this PR because it is meant to just be a demo. I'd rather merge upstream into this repo to clean it up. otherwise we do work twice

brenzi added 2 commits September 10, 2023 21:26

system init works

5dbd7bf

implemented, but do_something extrinsic isn't accepted

52ca3b6

brenzi added 4 commits September 11, 2023 11:44

demo works

8b31434

tune log levels for demo

87f596a

demoed like this for screencast

6da0d0e

bash snippets for screencast

784cdd0

brenzi requested a review from clangenb September 11, 2023 18:05

brenzi commented Sep 11, 2023

View reviewed changes

brenzi mentioned this pull request Sep 12, 2023

print_events: customize per parentchain Runtime integritee-network/worker#1446

Closed

clangenb reviewed Oct 3, 2023

View reviewed changes

Update enclave-runtime/src/rpc/worker_api_direct.rs

6a518bc

Co-authored-by: clangenb <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

send personhood to node template #37

send personhood to node template #37

brenzi commented Sep 11, 2023 •

edited

Loading

brenzi commented Sep 11, 2023 •

edited

Loading

brenzi commented Sep 11, 2023 •

edited

Loading

brenzi Sep 11, 2023

clangenb Oct 3, 2023

brenzi Oct 4, 2023

brenzi Sep 11, 2023

clangenb Oct 3, 2023

brenzi Sep 11, 2023

clangenb Oct 3, 2023

clangenb Oct 3, 2023

brenzi Sep 11, 2023

clangenb Oct 3, 2023

brenzi commented Sep 12, 2023 •

edited

Loading

clangenb left a comment

clangenb Oct 3, 2023

clangenb Oct 3, 2023

clangenb Oct 3, 2023

clangenb Oct 3, 2023

brenzi Oct 4, 2023

clangenb Oct 3, 2023

brenzi Oct 4, 2023

clangenb Oct 3, 2023

clangenb Oct 3, 2023

clangenb Oct 3, 2023

clangenb Oct 3, 2023

clangenb Oct 3, 2023

brenzi commented Oct 4, 2023

send personhood to node template #37

Are you sure you want to change the base?

send personhood to node template #37

Conversation

brenzi commented Sep 11, 2023 • edited Loading

brenzi commented Sep 11, 2023 • edited Loading

brenzi commented Sep 11, 2023 • 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

brenzi commented Sep 12, 2023 • edited Loading

clangenb 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

brenzi commented Oct 4, 2023

brenzi commented Sep 11, 2023 •

edited

Loading

brenzi commented Sep 11, 2023 •

edited

Loading

brenzi commented Sep 11, 2023 •

edited

Loading

brenzi commented Sep 12, 2023 •

edited

Loading