Separate block confirmations - #501

Merged
haerdib merged 19 commits into
masterfrom
separate-block-confirmations
Nov 11, 2021
Merged

Separate block confirmations#501
haerdib merged 19 commits into
masterfrom
separate-block-confirmations

Conversation

@haerdib

@haerdibhaerdib commented Nov 10, 2021•
edited
Loading

Copy link
Copy Markdown
Contributor

Seems to work quite well: For every parentchain block we get 20 sidechain block events.
grafik

Introduces the following changes:

  • exchanges call- and block confirmation with parentchain and sidechain confirmations
  • removes IpfsHash Update Event (was removed completely from pallet teerex)
  • updates substrate version to the same of the integritee-node (https://github.com/paritytech/substrate.git?branch=master#b391b82954ad95a927a921035e3017c4a0aad516), wasn't compiling otherwise
  • cleans up shielding call execution call (at least a little)
  • temporarily introduces a new file for Merkel root calculation. This can be removed once we update to a substrate commit >= 23rd September

closes#457

@haerdib
haerdibforce-pushed the separate-block-confirmations branch from 9e45e6a to 386000bCompareNovember 10, 2021 12:49
@haerdibhaerdib self-assigned this Nov 10, 2021
haerdib
haerdib commented Nov 10, 2021
View reviewed changes
Comment threadcore-primitives/stf-executor/src/executor.rs
account: AccountId,
amount: Amount,
shard: &ShardIdentifier,
calls: &mut Vec<OpaqueCall>,

@haerdibhaerdibNov 10, 2021•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Removed this input parameter, as there will never(!) be a new OpaqueCall introduced during Stf::execute of a shielding extrsinic. (New OpaqueCall is only issued during execute when calling "unshield")

@murerfelmurerfelNov 10, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for clarifying this ☺️

@clangenbclangenbNov 11, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Haha, good catch. 🛩️

@haerdib
haerdib requested review from clangenb and murerfel and removed request for clangenbNovember 10, 2021 13:24
murerfel
murerfel reviewed Nov 10, 2021
View reviewed changes
Comment threadenclave-runtime/src/lib.rs Outdated
haerdib
haerdib commented Nov 10, 2021
View reviewed changes
Comment threadenclave-runtime/Cargo.toml
sp-utils = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master" }
sp-version = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master" }
sp-application-crypto = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master" }
#beefy-merkle-tree = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master", features = "keccak" }

@haerdibhaerdibNov 10, 2021•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

That's the ugly part here: substrate actually has a no-std compatible merkle tree primitives crate: https://github.com/paritytech/substrate/tree/master/frame/beefy-mmr/primitives

However, this was introduced on 23rd of September. We are not yet on this substrate commit. Can you agree with this temporary solution and to remove it once we are on this commit? If yes, I'll create an appropriate issue ofc.

@murerfelmurerfelNov 10, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If it's well documented and even has a corresponding issue, that's totally fine for me 👍

@clangenbclangenbNov 11, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it's fine too. 👍

Comment threadenclave-runtime/src/beefy_merkle_tree.rs
@@ -0,0 +1,229 @@
// This file is part of Substrate.

@haerdibhaerdibNov 10, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

This file was copy pasted from https://github.com/paritytech/substrate/blob/master/frame/beefy-mmr/primitives/src/lib.rs. Can be removed once we are above commit paritytech/substrate@65ac6f3

@haerdib
haerdibforce-pushed the separate-block-confirmations branch from f1773ad to 5e49a19CompareNovember 10, 2021 14:04
murerfel
murerfel reviewed Nov 10, 2021
View reviewed changes
Comment threadenclave-runtime/src/lib.rs Outdated
Comment on lines +1105 to +1109
let xt_block = [TEEREX_MODULE, BLOCK_CONFIRMED];
let opaque_call =
OpaqueCall::from_tuple(&(xt_block, shard, block_hash, state_hash_new.encode()));
let opaque_call = proposed_sidechain_block_extrinsic(shard, block_hash);

@murerfelmurerfelNov 10, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

so we now use the sidechain block hash instead of the state hash for this extrinsic?

@haerdibhaerdibNov 10, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Not exactly. State hash has been removed from the extrinsic as it didn't have any use case at all (#457)

Comment threadenclave-runtime/src/lib.rs
Comment on lines +1113 to +1045
/// Creates a proposed_sidechain_block extrinsic for a given shard id and sidechain block hash.
fn proposed_sidechain_block_extrinsic(shard_id: ShardIdentifier, block_hash: H256) -> OpaqueCall {
OpaqueCall::from_tuple(&([TEEREX_MODULE, PROPOSED_SIDECHAIN_BLOCK], shard_id, block_hash))
}

@murerfelmurerfelNov 10, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Also here, I'd prefer the create_ in front. In a future refactoring, we could put these into the extrinsics factory component or something similar?

@haerdibhaerdibNov 10, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Already changed. :) Yes, definitely - but I think the whole lib.rs (just like in PR #500 but also for parentchain syncing) should be refactored (but that's your call).. I didn't want to start something that will be changed in the future anyway. If you have a place they belong to already, I'll gladly move them.

Comment threadenclave-runtime/src/tests.rs Outdated
Comment on lines +426 to +452
fn empty_extrinsic_vec_gives_zero_merkle_root() {
// given
let block_hash = H256::from([1; 32]);
let extrinsics = Vec::new();
let expected_call =
([TEEREX_MODULE, PROCESSED_PARENTCHAIN_BLOCK], block_hash, H256::default()).encode();

// when
let call = crate::processed_parentchain_block_extrinsic(block_hash, extrinsics);

// then
assert_eq!(call.0, expected_call);
}

fn some_extrinsics_vec_give_non_zero_merkle_root() {
// given
let block_hash = H256::from([1; 32]);
let extrinsics = vec![H256::from([4; 32]), H256::from([9; 32])];
let zero_root_call =
([TEEREX_MODULE, PROCESSED_PARENTCHAIN_BLOCK], block_hash, H256::default()).encode();

// when
let call = crate::processed_parentchain_block_extrinsic(block_hash, extrinsics);

// then
assert_ne!(call.0, zero_root_call);
}

@murerfelmurerfelNov 10, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Always nice to have tests for the changes you make 🥳

Comment threadenclave-runtime/src/lib.rs
Comment on lines +1169 to +1099
fn hash_of<T: Encode>(xt: T) -> H256 {
blake2_256(&xt.encode()).into()

@murerfelmurerfelNov 10, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is a function that we have duplicated multiple times I believe. We should at some point put it in a primitives crate where it can be properly shared.

@haerdibhaerdibNov 10, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Aye.. But I didn't want to touch too much code, in fear of the rebase conflicts 😨

@clangenbclangenbNov 11, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we should simply implement hash on the extrinsic in the api-client. 😄

murerfel
murerfel approved these changes Nov 10, 2021
View reviewed changes

@murerfelmurerfel left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM - and if the checks pass, it should be fine ☺️ thanks for your perseverance when it comes to good comments 🚀

clangenb
clangenb approved these changes Nov 11, 2021
View reviewed changes

@clangenbclangenb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me. 👍

Comment threadcore-primitives/stf-executor/src/executor.rs
account: AccountId,
amount: Amount,
shard: &ShardIdentifier,
calls: &mut Vec<OpaqueCall>,

@clangenbclangenbNov 11, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Haha, good catch. 🛩️

Comment threadenclave-runtime/Cargo.toml
sp-utils = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master" }
sp-version = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master" }
sp-application-crypto = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master" }
#beefy-merkle-tree = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master", features = "keccak" }

@clangenbclangenbNov 11, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it's fine too. 👍

Comment threadenclave-runtime/src/lib.rs
if let Err(e) = validator.submit_simple_header(
validator.num_relays(),
signed_block.block.header().clone(),
block.header().clone(),

@clangenbclangenbNov 11, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nice optimization.

Comment threadenclave-runtime/src/lib.rs
Comment on lines +1169 to +1099
fn hash_of<T: Encode>(xt: T) -> H256 {
blake2_256(&xt.encode()).into()

@clangenbclangenbNov 11, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we should simply implement hash on the extrinsic in the api-client. 😄

@haerdib
haerdib merged commit e08bf0c into masterNov 11, 2021
@haerdib
haerdib deleted the separate-block-confirmations branch November 11, 2021 07:33
@haerdibhaerdib mentioned this pull request Nov 12, 2021
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@murerfelmurerfelmurerfel approved these changes
@clangenbclangenbclangenb approved these changes

Assignees

@haerdibhaerdib

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Clarify Block & Call Confirmation

3 participants

@haerdib@murerfel@clangenb
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
Separate block confirmations - #501
Merged
haerdib merged 19 commits into
masterfrom 
separate-block-confirmations
Nov 11, 2021
Merged
Separate block confirmations#501
haerdib merged 19 commits into
masterfrom 
separate-block-confirmations
Conversation
@haerdib
@haerdibhaerdib
commented
Nov 10, 2021•

edited

Loading

Copy link


Copy Markdown

Contributor
Seems to work quite well: For every parentchain block we get 20 sidechain block events.
grafik
Introduces the following changes:
  • exchanges call- and block confirmation with parentchain and sidechain confirmations
  • removes IpfsHash Update Event (was removed completely from pallet teerex)
  • updates substrate version to the same of the integritee-node (https://github.com/paritytech/substrate.git?branch=master#b391b82954ad95a927a921035e3017c4a0aad516), wasn't compiling otherwise
  • cleans up shielding call execution call (at least a little)
  • temporarily introduces a new file for Merkel root calculation. This can be removed once we update to a substrate commit >= 23rd September
closes#457
@haerdib
haerdibforce-pushed
the
separate-block-confirmations
branch
from
9e45e6a to
386000bCompareNovember 10, 2021 12:49
@haerdibhaerdib
self-assigned this
Nov 10, 2021
haerdib
haerdib
commented
Nov 10, 2021
View reviewed changes
Comment threadcore-primitives/stf-executor/src/executor.rs
 account: AccountId,
 amount: Amount,
 shard: &ShardIdentifier,
 calls: &mut Vec<OpaqueCall>,
@haerdibhaerdibNov 10, 2021•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Removed this input parameter, as there will never(!) be a new OpaqueCall introduced during Stf::execute of a shielding extrsinic. (New OpaqueCall is only issued during execute when calling "unshield")
@murerfelmurerfelNov 10, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Thanks for clarifying this ☺️
@clangenbclangenbNov 11, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Haha, good catch. 🛩️
@haerdib
haerdib
requested review from
clangenb and murerfel
and removed request for
clangenbNovember 10, 2021 13:24
murerfel
murerfel
reviewed
Nov 10, 2021
View reviewed changes
Comment threadenclave-runtime/src/lib.rs
Outdated

haerdib
haerdib
commented
Nov 10, 2021
View reviewed changes
Comment threadenclave-runtime/Cargo.toml
sp-utils = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master" }
sp-version = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master" }
sp-application-crypto = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master" }
#beefy-merkle-tree = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master", features = "keccak" }
@haerdibhaerdibNov 10, 2021•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

That's the ugly part here: substrate actually has a no-std compatible merkle tree primitives crate: https://github.com/paritytech/substrate/tree/master/frame/beefy-mmr/primitives
However, this was introduced on 23rd of September. We are not yet on this substrate commit. Can you agree with this temporary solution and to remove it once we are on this commit? If yes, I'll create an appropriate issue ofc.
@murerfelmurerfelNov 10, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

If it's well documented and even has a corresponding issue, that's totally fine for me 👍
@clangenbclangenbNov 11, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I think it's fine too. 👍
Comment threadenclave-runtime/src/beefy_merkle_tree.rs
@@ -0,0 +1,229 @@
// This file is part of Substrate.
@haerdibhaerdibNov 10, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

This file was copy pasted from https://github.com/paritytech/substrate/blob/master/frame/beefy-mmr/primitives/src/lib.rs. Can be removed once we are above commit paritytech/substrate@65ac6f3
@haerdib
haerdibforce-pushed
the
separate-block-confirmations
branch
from
f1773ad to
5e49a19CompareNovember 10, 2021 14:04
murerfel
murerfel
reviewed
Nov 10, 2021
View reviewed changes
Comment threadenclave-runtime/src/lib.rs
Outdated


Comment on lines

+1105
 to 
+1109

	let xt_block = [TEEREX_MODULE, BLOCK_CONFIRMED];
	let opaque_call =
 OpaqueCall::from_tuple(&(xt_block, shard, block_hash, state_hash_new.encode()));
	let opaque_call = proposed_sidechain_block_extrinsic(shard, block_hash);
@murerfelmurerfelNov 10, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

so we now use the sidechain block hash instead of the state hash for this extrinsic?
@haerdibhaerdibNov 10, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Not exactly. State hash has been removed from the extrinsic as it didn't have any use case at all (#457)
Comment threadenclave-runtime/src/lib.rs

Comment on lines

+1113
 to 
+1045

/// Creates a proposed_sidechain_block extrinsic for a given shard id and sidechain block hash.
fn proposed_sidechain_block_extrinsic(shard_id: ShardIdentifier, block_hash: H256) -> OpaqueCall {
	OpaqueCall::from_tuple(&([TEEREX_MODULE, PROPOSED_SIDECHAIN_BLOCK], shard_id, block_hash))
}
@murerfelmurerfelNov 10, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Also here, I'd prefer the create_ in front. In a future refactoring, we could put these into the extrinsics factory component or something similar?
@haerdibhaerdibNov 10, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Already changed. :) Yes, definitely - but I think the whole lib.rs (just like in PR #500 but also for parentchain syncing) should be refactored (but that's your call).. I didn't want to start something that will be changed in the future anyway. If you have a place they belong to already, I'll gladly move them.
Comment threadenclave-runtime/src/tests.rs
Outdated


Comment on lines

+426
 to 
+452

fn empty_extrinsic_vec_gives_zero_merkle_root() {
	// given
	let block_hash = H256::from([1; 32]);
	let extrinsics = Vec::new();
	let expected_call =
 ([TEEREX_MODULE, PROCESSED_PARENTCHAIN_BLOCK], block_hash, H256::default()).encode();

	// when
	let call = crate::processed_parentchain_block_extrinsic(block_hash, extrinsics);

	// then
	assert_eq!(call.0, expected_call);
}

fn some_extrinsics_vec_give_non_zero_merkle_root() {
	// given
	let block_hash = H256::from([1; 32]);
	let extrinsics = vec![H256::from([4; 32]), H256::from([9; 32])];
	let zero_root_call =
 ([TEEREX_MODULE, PROCESSED_PARENTCHAIN_BLOCK], block_hash, H256::default()).encode();

	// when
	let call = crate::processed_parentchain_block_extrinsic(block_hash, extrinsics);

	// then
	assert_ne!(call.0, zero_root_call);
}
@murerfelmurerfelNov 10, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Always nice to have tests for the changes you make 🥳
Comment threadenclave-runtime/src/lib.rs

Comment on lines

+1169
 to 
+1099

fn hash_of<T: Encode>(xt: T) -> H256 {
	blake2_256(&xt.encode()).into()
@murerfelmurerfelNov 10, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

This is a function that we have duplicated multiple times I believe. We should at some point put it in a primitives crate where it can be properly shared.
@haerdibhaerdibNov 10, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Aye.. But I didn't want to touch too much code, in fear of the rebase conflicts 😨
@clangenbclangenbNov 11, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I think we should simply implement hash on the extrinsic in the api-client. 😄
murerfel
murerfel
approved these changes
Nov 10, 2021
View reviewed changes
@murerfelmurerfel
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM - and if the checks pass, it should be fine ☺️ thanks for your perseverance when it comes to good comments 🚀
clangenb
clangenb
approved these changes
Nov 11, 2021
View reviewed changes
@clangenbclangenb
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Looks good to me. 👍
Comment threadcore-primitives/stf-executor/src/executor.rs
 account: AccountId,
 amount: Amount,
 shard: &ShardIdentifier,
 calls: &mut Vec<OpaqueCall>,
@clangenbclangenbNov 11, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Haha, good catch. 🛩️
Comment threadenclave-runtime/Cargo.toml
sp-utils = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master" }
sp-version = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master" }
sp-application-crypto = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master" }
#beefy-merkle-tree = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master", features = "keccak" }
@clangenbclangenbNov 11, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I think it's fine too. 👍
Comment threadenclave-runtime/src/lib.rs
 if let Err(e) = validator.submit_simple_header(
 validator.num_relays(),
signed_block.block.header().clone(),
 block.header().clone(),
@clangenbclangenbNov 11, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

nice optimization.
Comment threadenclave-runtime/src/lib.rs

Comment on lines

+1169
 to 
+1099

fn hash_of<T: Encode>(xt: T) -> H256 {
	blake2_256(&xt.encode()).into()
@clangenbclangenbNov 11, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I think we should simply implement hash on the extrinsic in the api-client. 😄
@haerdib
haerdib
merged commit e08bf0c
into
masterNov 11, 2021
@haerdib
haerdib
deleted the

separate-block-confirmations

branch
November 11, 2021 07:33
@haerdibhaerdib
mentioned this pull request
Nov 12, 2021
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@murerfelmurerfelmurerfel approved these changes
@clangenbclangenbclangenb approved these changes

Assignees

@haerdibhaerdib

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Clarify Block & Call Confirmation


3 participants

@haerdib@murerfel@clangenb

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Separate block confirmations - #501
Merged
haerdib merged 19 commits into
masterfrom 
separate-block-confirmations
Nov 11, 2021
Merged
Separate block confirmations#501
haerdib merged 19 commits into
masterfrom 
separate-block-confirmations
Conversation
@haerdib
@haerdibhaerdib
commented
Nov 10, 2021•

edited

Loading

Copy link


Copy Markdown

Contributor
Seems to work quite well: For every parentchain block we get 20 sidechain block events.
grafik
Introduces the following changes:
  • exchanges call- and block confirmation with parentchain and sidechain confirmations
  • removes IpfsHash Update Event (was removed completely from pallet teerex)
  • updates substrate version to the same of the integritee-node (https://github.com/paritytech/substrate.git?branch=master#b391b82954ad95a927a921035e3017c4a0aad516), wasn't compiling otherwise
  • cleans up shielding call execution call (at least a little)
  • temporarily introduces a new file for Merkel root calculation. This can be removed once we update to a substrate commit >= 23rd September
closes#457
@haerdib
haerdibforce-pushed
the
separate-block-confirmations
branch
from
9e45e6a to
386000bCompareNovember 10, 2021 12:49
@haerdibhaerdib
self-assigned this
Nov 10, 2021
haerdib
haerdib
commented
Nov 10, 2021
View reviewed changes
Comment threadcore-primitives/stf-executor/src/executor.rs
 account: AccountId,
 amount: Amount,
 shard: &ShardIdentifier,
 calls: &mut Vec<OpaqueCall>,
@haerdibhaerdibNov 10, 2021•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Removed this input parameter, as there will never(!) be a new OpaqueCall introduced during Stf::execute of a shielding extrsinic. (New OpaqueCall is only issued during execute when calling "unshield")
@murerfelmurerfelNov 10, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Thanks for clarifying this ☺️
@clangenbclangenbNov 11, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Haha, good catch. 🛩️
@haerdib
haerdib
requested review from
clangenb and murerfel
and removed request for
clangenbNovember 10, 2021 13:24
murerfel
murerfel
reviewed
Nov 10, 2021
View reviewed changes
Comment threadenclave-runtime/src/lib.rs
Outdated

haerdib
haerdib
commented
Nov 10, 2021
View reviewed changes
Comment threadenclave-runtime/Cargo.toml
sp-utils = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master" }
sp-version = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master" }
sp-application-crypto = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master" }
#beefy-merkle-tree = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master", features = "keccak" }
@haerdibhaerdibNov 10, 2021•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

That's the ugly part here: substrate actually has a no-std compatible merkle tree primitives crate: https://github.com/paritytech/substrate/tree/master/frame/beefy-mmr/primitives
However, this was introduced on 23rd of September. We are not yet on this substrate commit. Can you agree with this temporary solution and to remove it once we are on this commit? If yes, I'll create an appropriate issue ofc.
@murerfelmurerfelNov 10, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

If it's well documented and even has a corresponding issue, that's totally fine for me 👍
@clangenbclangenbNov 11, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I think it's fine too. 👍
Comment threadenclave-runtime/src/beefy_merkle_tree.rs
@@ -0,0 +1,229 @@
// This file is part of Substrate.
@haerdibhaerdibNov 10, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

This file was copy pasted from https://github.com/paritytech/substrate/blob/master/frame/beefy-mmr/primitives/src/lib.rs. Can be removed once we are above commit paritytech/substrate@65ac6f3
@haerdib
haerdibforce-pushed
the
separate-block-confirmations
branch
from
f1773ad to
5e49a19CompareNovember 10, 2021 14:04
murerfel
murerfel
reviewed
Nov 10, 2021
View reviewed changes
Comment threadenclave-runtime/src/lib.rs
Outdated


Comment on lines

+1105
 to 
+1109

	let xt_block = [TEEREX_MODULE, BLOCK_CONFIRMED];
	let opaque_call =
 OpaqueCall::from_tuple(&(xt_block, shard, block_hash, state_hash_new.encode()));
	let opaque_call = proposed_sidechain_block_extrinsic(shard, block_hash);
@murerfelmurerfelNov 10, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

so we now use the sidechain block hash instead of the state hash for this extrinsic?
@haerdibhaerdibNov 10, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Not exactly. State hash has been removed from the extrinsic as it didn't have any use case at all (#457)
Comment threadenclave-runtime/src/lib.rs

Comment on lines

+1113
 to 
+1045

/// Creates a proposed_sidechain_block extrinsic for a given shard id and sidechain block hash.
fn proposed_sidechain_block_extrinsic(shard_id: ShardIdentifier, block_hash: H256) -> OpaqueCall {
	OpaqueCall::from_tuple(&([TEEREX_MODULE, PROPOSED_SIDECHAIN_BLOCK], shard_id, block_hash))
}
@murerfelmurerfelNov 10, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Also here, I'd prefer the create_ in front. In a future refactoring, we could put these into the extrinsics factory component or something similar?
@haerdibhaerdibNov 10, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Already changed. :) Yes, definitely - but I think the whole lib.rs (just like in PR #500 but also for parentchain syncing) should be refactored (but that's your call).. I didn't want to start something that will be changed in the future anyway. If you have a place they belong to already, I'll gladly move them.
Comment threadenclave-runtime/src/tests.rs
Outdated


Comment on lines

+426
 to 
+452

fn empty_extrinsic_vec_gives_zero_merkle_root() {
	// given
	let block_hash = H256::from([1; 32]);
	let extrinsics = Vec::new();
	let expected_call =
 ([TEEREX_MODULE, PROCESSED_PARENTCHAIN_BLOCK], block_hash, H256::default()).encode();

	// when
	let call = crate::processed_parentchain_block_extrinsic(block_hash, extrinsics);

	// then
	assert_eq!(call.0, expected_call);
}

fn some_extrinsics_vec_give_non_zero_merkle_root() {
	// given
	let block_hash = H256::from([1; 32]);
	let extrinsics = vec![H256::from([4; 32]), H256::from([9; 32])];
	let zero_root_call =
 ([TEEREX_MODULE, PROCESSED_PARENTCHAIN_BLOCK], block_hash, H256::default()).encode();

	// when
	let call = crate::processed_parentchain_block_extrinsic(block_hash, extrinsics);

	// then
	assert_ne!(call.0, zero_root_call);
}
@murerfelmurerfelNov 10, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Always nice to have tests for the changes you make 🥳
Comment threadenclave-runtime/src/lib.rs

Comment on lines

+1169
 to 
+1099

fn hash_of<T: Encode>(xt: T) -> H256 {
	blake2_256(&xt.encode()).into()
@murerfelmurerfelNov 10, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

This is a function that we have duplicated multiple times I believe. We should at some point put it in a primitives crate where it can be properly shared.
@haerdibhaerdibNov 10, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Aye.. But I didn't want to touch too much code, in fear of the rebase conflicts 😨
@clangenbclangenbNov 11, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I think we should simply implement hash on the extrinsic in the api-client. 😄
murerfel
murerfel
approved these changes
Nov 10, 2021
View reviewed changes
@murerfelmurerfel
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM - and if the checks pass, it should be fine ☺️ thanks for your perseverance when it comes to good comments 🚀
clangenb
clangenb
approved these changes
Nov 11, 2021
View reviewed changes
@clangenbclangenb
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Looks good to me. 👍
Comment threadcore-primitives/stf-executor/src/executor.rs
 account: AccountId,
 amount: Amount,
 shard: &ShardIdentifier,
 calls: &mut Vec<OpaqueCall>,
@clangenbclangenbNov 11, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Haha, good catch. 🛩️
Comment threadenclave-runtime/Cargo.toml
sp-utils = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master" }
sp-version = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master" }
sp-application-crypto = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master" }
#beefy-merkle-tree = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master", features = "keccak" }
@clangenbclangenbNov 11, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I think it's fine too. 👍
Comment threadenclave-runtime/src/lib.rs
 if let Err(e) = validator.submit_simple_header(
 validator.num_relays(),
signed_block.block.header().clone(),
 block.header().clone(),
@clangenbclangenbNov 11, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

nice optimization.
Comment threadenclave-runtime/src/lib.rs

Comment on lines

+1169
 to 
+1099

fn hash_of<T: Encode>(xt: T) -> H256 {
	blake2_256(&xt.encode()).into()
@clangenbclangenbNov 11, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I think we should simply implement hash on the extrinsic in the api-client. 😄
@haerdib
haerdib
merged commit e08bf0c
into
masterNov 11, 2021
@haerdib
haerdib
deleted the

separate-block-confirmations

branch
November 11, 2021 07:33
@haerdibhaerdib
mentioned this pull request
Nov 12, 2021
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@murerfelmurerfelmurerfel approved these changes
@clangenbclangenbclangenb approved these changes

Assignees

@haerdibhaerdib

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Clarify Block & Call Confirmation


3 participants

@haerdib@murerfel@clangenb

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Separate block confirmations - #501
Merged
haerdib merged 19 commits into
masterfrom 
separate-block-confirmations
Nov 11, 2021
Merged
Separate block confirmations#501
haerdib merged 19 commits into
masterfrom 
separate-block-confirmations
Conversation
@haerdib
@haerdibhaerdib
commented
Nov 10, 2021•

edited

Loading

Copy link


Copy Markdown

Contributor
Seems to work quite well: For every parentchain block we get 20 sidechain block events.
grafik
Introduces the following changes:
  • exchanges call- and block confirmation with parentchain and sidechain confirmations
  • removes IpfsHash Update Event (was removed completely from pallet teerex)
  • updates substrate version to the same of the integritee-node (https://github.com/paritytech/substrate.git?branch=master#b391b82954ad95a927a921035e3017c4a0aad516), wasn't compiling otherwise
  • cleans up shielding call execution call (at least a little)
  • temporarily introduces a new file for Merkel root calculation. This can be removed once we update to a substrate commit >= 23rd September
closes#457
@haerdib
haerdibforce-pushed
the
separate-block-confirmations
branch
from
9e45e6a to
386000bCompareNovember 10, 2021 12:49
@haerdibhaerdib
self-assigned this
Nov 10, 2021
haerdib
haerdib
commented
Nov 10, 2021
View reviewed changes
Comment threadcore-primitives/stf-executor/src/executor.rs
 account: AccountId,
 amount: Amount,
 shard: &ShardIdentifier,
 calls: &mut Vec<OpaqueCall>,
@haerdibhaerdibNov 10, 2021•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Removed this input parameter, as there will never(!) be a new OpaqueCall introduced during Stf::execute of a shielding extrsinic. (New OpaqueCall is only issued during execute when calling "unshield")
@murerfelmurerfelNov 10, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Thanks for clarifying this ☺️
@clangenbclangenbNov 11, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Haha, good catch. 🛩️
@haerdib
haerdib
requested review from
clangenb and murerfel
and removed request for
clangenbNovember 10, 2021 13:24
murerfel
murerfel
reviewed
Nov 10, 2021
View reviewed changes
Comment threadenclave-runtime/src/lib.rs
Outdated

haerdib
haerdib
commented
Nov 10, 2021
View reviewed changes
Comment threadenclave-runtime/Cargo.toml
sp-utils = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master" }
sp-version = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master" }
sp-application-crypto = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master" }
#beefy-merkle-tree = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master", features = "keccak" }
@haerdibhaerdibNov 10, 2021•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

That's the ugly part here: substrate actually has a no-std compatible merkle tree primitives crate: https://github.com/paritytech/substrate/tree/master/frame/beefy-mmr/primitives
However, this was introduced on 23rd of September. We are not yet on this substrate commit. Can you agree with this temporary solution and to remove it once we are on this commit? If yes, I'll create an appropriate issue ofc.
@murerfelmurerfelNov 10, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

If it's well documented and even has a corresponding issue, that's totally fine for me 👍
@clangenbclangenbNov 11, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I think it's fine too. 👍
Comment threadenclave-runtime/src/beefy_merkle_tree.rs
@@ -0,0 +1,229 @@
// This file is part of Substrate.
@haerdibhaerdibNov 10, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

This file was copy pasted from https://github.com/paritytech/substrate/blob/master/frame/beefy-mmr/primitives/src/lib.rs. Can be removed once we are above commit paritytech/substrate@65ac6f3
@haerdib
haerdibforce-pushed
the
separate-block-confirmations
branch
from
f1773ad to
5e49a19CompareNovember 10, 2021 14:04
murerfel
murerfel
reviewed
Nov 10, 2021
View reviewed changes
Comment threadenclave-runtime/src/lib.rs
Outdated


Comment on lines

+1105
 to 
+1109

	let xt_block = [TEEREX_MODULE, BLOCK_CONFIRMED];
	let opaque_call =
 OpaqueCall::from_tuple(&(xt_block, shard, block_hash, state_hash_new.encode()));
	let opaque_call = proposed_sidechain_block_extrinsic(shard, block_hash);
@murerfelmurerfelNov 10, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

so we now use the sidechain block hash instead of the state hash for this extrinsic?
@haerdibhaerdibNov 10, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Not exactly. State hash has been removed from the extrinsic as it didn't have any use case at all (#457)
Comment threadenclave-runtime/src/lib.rs

Comment on lines

+1113
 to 
+1045

/// Creates a proposed_sidechain_block extrinsic for a given shard id and sidechain block hash.
fn proposed_sidechain_block_extrinsic(shard_id: ShardIdentifier, block_hash: H256) -> OpaqueCall {
	OpaqueCall::from_tuple(&([TEEREX_MODULE, PROPOSED_SIDECHAIN_BLOCK], shard_id, block_hash))
}
@murerfelmurerfelNov 10, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Also here, I'd prefer the create_ in front. In a future refactoring, we could put these into the extrinsics factory component or something similar?
@haerdibhaerdibNov 10, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Already changed. :) Yes, definitely - but I think the whole lib.rs (just like in PR #500 but also for parentchain syncing) should be refactored (but that's your call).. I didn't want to start something that will be changed in the future anyway. If you have a place they belong to already, I'll gladly move them.
Comment threadenclave-runtime/src/tests.rs
Outdated


Comment on lines

+426
 to 
+452

fn empty_extrinsic_vec_gives_zero_merkle_root() {
	// given
	let block_hash = H256::from([1; 32]);
	let extrinsics = Vec::new();
	let expected_call =
 ([TEEREX_MODULE, PROCESSED_PARENTCHAIN_BLOCK], block_hash, H256::default()).encode();

	// when
	let call = crate::processed_parentchain_block_extrinsic(block_hash, extrinsics);

	// then
	assert_eq!(call.0, expected_call);
}

fn some_extrinsics_vec_give_non_zero_merkle_root() {
	// given
	let block_hash = H256::from([1; 32]);
	let extrinsics = vec![H256::from([4; 32]), H256::from([9; 32])];
	let zero_root_call =
 ([TEEREX_MODULE, PROCESSED_PARENTCHAIN_BLOCK], block_hash, H256::default()).encode();

	// when
	let call = crate::processed_parentchain_block_extrinsic(block_hash, extrinsics);

	// then
	assert_ne!(call.0, zero_root_call);
}
@murerfelmurerfelNov 10, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Always nice to have tests for the changes you make 🥳
Comment threadenclave-runtime/src/lib.rs

Comment on lines

+1169
 to 
+1099

fn hash_of<T: Encode>(xt: T) -> H256 {
	blake2_256(&xt.encode()).into()
@murerfelmurerfelNov 10, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

This is a function that we have duplicated multiple times I believe. We should at some point put it in a primitives crate where it can be properly shared.
@haerdibhaerdibNov 10, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Aye.. But I didn't want to touch too much code, in fear of the rebase conflicts 😨
@clangenbclangenbNov 11, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I think we should simply implement hash on the extrinsic in the api-client. 😄
murerfel
murerfel
approved these changes
Nov 10, 2021
View reviewed changes
@murerfelmurerfel
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM - and if the checks pass, it should be fine ☺️ thanks for your perseverance when it comes to good comments 🚀
clangenb
clangenb
approved these changes
Nov 11, 2021
View reviewed changes
@clangenbclangenb
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Looks good to me. 👍
Comment threadcore-primitives/stf-executor/src/executor.rs
 account: AccountId,
 amount: Amount,
 shard: &ShardIdentifier,
 calls: &mut Vec<OpaqueCall>,
@clangenbclangenbNov 11, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Haha, good catch. 🛩️
Comment threadenclave-runtime/Cargo.toml
sp-utils = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master" }
sp-version = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master" }
sp-application-crypto = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master" }
#beefy-merkle-tree = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master", features = "keccak" }
@clangenbclangenbNov 11, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I think it's fine too. 👍
Comment threadenclave-runtime/src/lib.rs
 if let Err(e) = validator.submit_simple_header(
 validator.num_relays(),
signed_block.block.header().clone(),
 block.header().clone(),
@clangenbclangenbNov 11, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

nice optimization.
Comment threadenclave-runtime/src/lib.rs

Comment on lines

+1169
 to 
+1099

fn hash_of<T: Encode>(xt: T) -> H256 {
	blake2_256(&xt.encode()).into()
@clangenbclangenbNov 11, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I think we should simply implement hash on the extrinsic in the api-client. 😄
@haerdib
haerdib
merged commit e08bf0c
into
masterNov 11, 2021
@haerdib
haerdib
deleted the

separate-block-confirmations

branch
November 11, 2021 07:33
@haerdibhaerdib
mentioned this pull request
Nov 12, 2021
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@murerfelmurerfelmurerfel approved these changes
@clangenbclangenbclangenb approved these changes

Assignees

@haerdibhaerdib

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Clarify Block & Call Confirmation


3 participants

@haerdib@murerfel@clangenb

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
Separate block confirmations - #501
Merged
haerdib merged 19 commits into
masterfrom 
separate-block-confirmations
Nov 11, 2021
Merged
Separate block confirmations#501
haerdib merged 19 commits into
masterfrom 
separate-block-confirmations
Conversation
@haerdib
@haerdibhaerdib
commented
Nov 10, 2021•

edited

Loading

Copy link


Copy Markdown

Contributor
Seems to work quite well: For every parentchain block we get 20 sidechain block events.
grafik
Introduces the following changes:
  • exchanges call- and block confirmation with parentchain and sidechain confirmations
  • removes IpfsHash Update Event (was removed completely from pallet teerex)
  • updates substrate version to the same of the integritee-node (https://github.com/paritytech/substrate.git?branch=master#b391b82954ad95a927a921035e3017c4a0aad516), wasn't compiling otherwise
  • cleans up shielding call execution call (at least a little)
  • temporarily introduces a new file for Merkel root calculation. This can be removed once we update to a substrate commit >= 23rd September
closes#457
@haerdib
haerdibforce-pushed
the
separate-block-confirmations
branch
from
9e45e6a to
386000bCompareNovember 10, 2021 12:49
@haerdibhaerdib
self-assigned this
Nov 10, 2021
haerdib
haerdib
commented
Nov 10, 2021
View reviewed changes
Comment threadcore-primitives/stf-executor/src/executor.rs
 account: AccountId,
 amount: Amount,
 shard: &ShardIdentifier,
 calls: &mut Vec<OpaqueCall>,
@haerdibhaerdibNov 10, 2021•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Removed this input parameter, as there will never(!) be a new OpaqueCall introduced during Stf::execute of a shielding extrsinic. (New OpaqueCall is only issued during execute when calling "unshield")
@murerfelmurerfelNov 10, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Thanks for clarifying this ☺️
@clangenbclangenbNov 11, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Haha, good catch. 🛩️
@haerdib
haerdib
requested review from
clangenb and murerfel
and removed request for
clangenbNovember 10, 2021 13:24
murerfel
murerfel
reviewed
Nov 10, 2021
View reviewed changes
Comment threadenclave-runtime/src/lib.rs
Outdated

haerdib
haerdib
commented
Nov 10, 2021
View reviewed changes
Comment threadenclave-runtime/Cargo.toml
sp-utils = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master" }
sp-version = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master" }
sp-application-crypto = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master" }
#beefy-merkle-tree = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master", features = "keccak" }
@haerdibhaerdibNov 10, 2021•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

That's the ugly part here: substrate actually has a no-std compatible merkle tree primitives crate: https://github.com/paritytech/substrate/tree/master/frame/beefy-mmr/primitives
However, this was introduced on 23rd of September. We are not yet on this substrate commit. Can you agree with this temporary solution and to remove it once we are on this commit? If yes, I'll create an appropriate issue ofc.
@murerfelmurerfelNov 10, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

If it's well documented and even has a corresponding issue, that's totally fine for me 👍
@clangenbclangenbNov 11, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I think it's fine too. 👍
Comment threadenclave-runtime/src/beefy_merkle_tree.rs
@@ -0,0 +1,229 @@
// This file is part of Substrate.
@haerdibhaerdibNov 10, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

This file was copy pasted from https://github.com/paritytech/substrate/blob/master/frame/beefy-mmr/primitives/src/lib.rs. Can be removed once we are above commit paritytech/substrate@65ac6f3
@haerdib
haerdibforce-pushed
the
separate-block-confirmations
branch
from
f1773ad to
5e49a19CompareNovember 10, 2021 14:04
murerfel
murerfel
reviewed
Nov 10, 2021
View reviewed changes
Comment threadenclave-runtime/src/lib.rs
Outdated


Comment on lines

+1105
 to 
+1109

	let xt_block = [TEEREX_MODULE, BLOCK_CONFIRMED];
	let opaque_call =
 OpaqueCall::from_tuple(&(xt_block, shard, block_hash, state_hash_new.encode()));
	let opaque_call = proposed_sidechain_block_extrinsic(shard, block_hash);
@murerfelmurerfelNov 10, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

so we now use the sidechain block hash instead of the state hash for this extrinsic?
@haerdibhaerdibNov 10, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Not exactly. State hash has been removed from the extrinsic as it didn't have any use case at all (#457)
Comment threadenclave-runtime/src/lib.rs

Comment on lines

+1113
 to 
+1045

/// Creates a proposed_sidechain_block extrinsic for a given shard id and sidechain block hash.
fn proposed_sidechain_block_extrinsic(shard_id: ShardIdentifier, block_hash: H256) -> OpaqueCall {
	OpaqueCall::from_tuple(&([TEEREX_MODULE, PROPOSED_SIDECHAIN_BLOCK], shard_id, block_hash))
}
@murerfelmurerfelNov 10, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Also here, I'd prefer the create_ in front. In a future refactoring, we could put these into the extrinsics factory component or something similar?
@haerdibhaerdibNov 10, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Already changed. :) Yes, definitely - but I think the whole lib.rs (just like in PR #500 but also for parentchain syncing) should be refactored (but that's your call).. I didn't want to start something that will be changed in the future anyway. If you have a place they belong to already, I'll gladly move them.
Comment threadenclave-runtime/src/tests.rs
Outdated


Comment on lines

+426
 to 
+452

fn empty_extrinsic_vec_gives_zero_merkle_root() {
	// given
	let block_hash = H256::from([1; 32]);
	let extrinsics = Vec::new();
	let expected_call =
 ([TEEREX_MODULE, PROCESSED_PARENTCHAIN_BLOCK], block_hash, H256::default()).encode();

	// when
	let call = crate::processed_parentchain_block_extrinsic(block_hash, extrinsics);

	// then
	assert_eq!(call.0, expected_call);
}

fn some_extrinsics_vec_give_non_zero_merkle_root() {
	// given
	let block_hash = H256::from([1; 32]);
	let extrinsics = vec![H256::from([4; 32]), H256::from([9; 32])];
	let zero_root_call =
 ([TEEREX_MODULE, PROCESSED_PARENTCHAIN_BLOCK], block_hash, H256::default()).encode();

	// when
	let call = crate::processed_parentchain_block_extrinsic(block_hash, extrinsics);

	// then
	assert_ne!(call.0, zero_root_call);
}
@murerfelmurerfelNov 10, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Always nice to have tests for the changes you make 🥳
Comment threadenclave-runtime/src/lib.rs

Comment on lines

+1169
 to 
+1099

fn hash_of<T: Encode>(xt: T) -> H256 {
	blake2_256(&xt.encode()).into()
@murerfelmurerfelNov 10, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

This is a function that we have duplicated multiple times I believe. We should at some point put it in a primitives crate where it can be properly shared.
@haerdibhaerdibNov 10, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Aye.. But I didn't want to touch too much code, in fear of the rebase conflicts 😨
@clangenbclangenbNov 11, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I think we should simply implement hash on the extrinsic in the api-client. 😄
murerfel
murerfel
approved these changes
Nov 10, 2021
View reviewed changes
@murerfelmurerfel
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM - and if the checks pass, it should be fine ☺️ thanks for your perseverance when it comes to good comments 🚀
clangenb
clangenb
approved these changes
Nov 11, 2021
View reviewed changes
@clangenbclangenb
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Looks good to me. 👍
Comment threadcore-primitives/stf-executor/src/executor.rs
 account: AccountId,
 amount: Amount,
 shard: &ShardIdentifier,
 calls: &mut Vec<OpaqueCall>,
@clangenbclangenbNov 11, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Haha, good catch. 🛩️
Comment threadenclave-runtime/Cargo.toml
sp-utils = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master" }
sp-version = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master" }
sp-application-crypto = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master" }
#beefy-merkle-tree = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master", features = "keccak" }
@clangenbclangenbNov 11, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I think it's fine too. 👍
Comment threadenclave-runtime/src/lib.rs
 if let Err(e) = validator.submit_simple_header(
 validator.num_relays(),
signed_block.block.header().clone(),
 block.header().clone(),
@clangenbclangenbNov 11, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

nice optimization.
Comment threadenclave-runtime/src/lib.rs

Comment on lines

+1169
 to 
+1099

fn hash_of<T: Encode>(xt: T) -> H256 {
	blake2_256(&xt.encode()).into()
@clangenbclangenbNov 11, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I think we should simply implement hash on the extrinsic in the api-client. 😄
@haerdib
haerdib
merged commit e08bf0c
into
masterNov 11, 2021
@haerdib
haerdib
deleted the

separate-block-confirmations

branch
November 11, 2021 07:33
@haerdibhaerdib
mentioned this pull request
Nov 12, 2021
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@murerfelmurerfelmurerfel approved these changes
@clangenbclangenbclangenb approved these changes

Assignees

@haerdibhaerdib

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Clarify Block & Call Confirmation


3 participants

@haerdib@murerfel@clangenb

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Separate block confirmations - #501
Merged
haerdib merged 19 commits into
masterfrom 
separate-block-confirmations
Nov 11, 2021
Merged
Separate block confirmations#501
haerdib merged 19 commits into
masterfrom 
separate-block-confirmations
Conversation
@haerdib
@haerdibhaerdib
commented
Nov 10, 2021•

edited

Loading

Copy link


Copy Markdown

Contributor
Seems to work quite well: For every parentchain block we get 20 sidechain block events.
grafik
Introduces the following changes:
  • exchanges call- and block confirmation with parentchain and sidechain confirmations
  • removes IpfsHash Update Event (was removed completely from pallet teerex)
  • updates substrate version to the same of the integritee-node (https://github.com/paritytech/substrate.git?branch=master#b391b82954ad95a927a921035e3017c4a0aad516), wasn't compiling otherwise
  • cleans up shielding call execution call (at least a little)
  • temporarily introduces a new file for Merkel root calculation. This can be removed once we update to a substrate commit >= 23rd September
closes#457
@haerdib
haerdibforce-pushed
the
separate-block-confirmations
branch
from
9e45e6a to
386000bCompareNovember 10, 2021 12:49
@haerdibhaerdib
self-assigned this
Nov 10, 2021
haerdib
haerdib
commented
Nov 10, 2021
View reviewed changes
Comment threadcore-primitives/stf-executor/src/executor.rs
 account: AccountId,
 amount: Amount,
 shard: &ShardIdentifier,
 calls: &mut Vec<OpaqueCall>,
@haerdibhaerdibNov 10, 2021•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Removed this input parameter, as there will never(!) be a new OpaqueCall introduced during Stf::execute of a shielding extrsinic. (New OpaqueCall is only issued during execute when calling "unshield")
@murerfelmurerfelNov 10, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Thanks for clarifying this ☺️
@clangenbclangenbNov 11, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Haha, good catch. 🛩️
@haerdib
haerdib
requested review from
clangenb and murerfel
and removed request for
clangenbNovember 10, 2021 13:24
murerfel
murerfel
reviewed
Nov 10, 2021
View reviewed changes
Comment threadenclave-runtime/src/lib.rs
Outdated

haerdib
haerdib
commented
Nov 10, 2021
View reviewed changes
Comment threadenclave-runtime/Cargo.toml
sp-utils = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master" }
sp-version = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master" }
sp-application-crypto = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master" }
#beefy-merkle-tree = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master", features = "keccak" }
@haerdibhaerdibNov 10, 2021•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

That's the ugly part here: substrate actually has a no-std compatible merkle tree primitives crate: https://github.com/paritytech/substrate/tree/master/frame/beefy-mmr/primitives
However, this was introduced on 23rd of September. We are not yet on this substrate commit. Can you agree with this temporary solution and to remove it once we are on this commit? If yes, I'll create an appropriate issue ofc.
@murerfelmurerfelNov 10, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

If it's well documented and even has a corresponding issue, that's totally fine for me 👍
@clangenbclangenbNov 11, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I think it's fine too. 👍
Comment threadenclave-runtime/src/beefy_merkle_tree.rs
@@ -0,0 +1,229 @@
// This file is part of Substrate.
@haerdibhaerdibNov 10, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

This file was copy pasted from https://github.com/paritytech/substrate/blob/master/frame/beefy-mmr/primitives/src/lib.rs. Can be removed once we are above commit paritytech/substrate@65ac6f3
@haerdib
haerdibforce-pushed
the
separate-block-confirmations
branch
from
f1773ad to
5e49a19CompareNovember 10, 2021 14:04
murerfel
murerfel
reviewed
Nov 10, 2021
View reviewed changes
Comment threadenclave-runtime/src/lib.rs
Outdated


Comment on lines

+1105
 to 
+1109

	let xt_block = [TEEREX_MODULE, BLOCK_CONFIRMED];
	let opaque_call =
 OpaqueCall::from_tuple(&(xt_block, shard, block_hash, state_hash_new.encode()));
	let opaque_call = proposed_sidechain_block_extrinsic(shard, block_hash);
@murerfelmurerfelNov 10, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

so we now use the sidechain block hash instead of the state hash for this extrinsic?
@haerdibhaerdibNov 10, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Not exactly. State hash has been removed from the extrinsic as it didn't have any use case at all (#457)
Comment threadenclave-runtime/src/lib.rs

Comment on lines

+1113
 to 
+1045

/// Creates a proposed_sidechain_block extrinsic for a given shard id and sidechain block hash.
fn proposed_sidechain_block_extrinsic(shard_id: ShardIdentifier, block_hash: H256) -> OpaqueCall {
	OpaqueCall::from_tuple(&([TEEREX_MODULE, PROPOSED_SIDECHAIN_BLOCK], shard_id, block_hash))
}
@murerfelmurerfelNov 10, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Also here, I'd prefer the create_ in front. In a future refactoring, we could put these into the extrinsics factory component or something similar?
@haerdibhaerdibNov 10, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Already changed. :) Yes, definitely - but I think the whole lib.rs (just like in PR #500 but also for parentchain syncing) should be refactored (but that's your call).. I didn't want to start something that will be changed in the future anyway. If you have a place they belong to already, I'll gladly move them.
Comment threadenclave-runtime/src/tests.rs
Outdated


Comment on lines

+426
 to 
+452

fn empty_extrinsic_vec_gives_zero_merkle_root() {
	// given
	let block_hash = H256::from([1; 32]);
	let extrinsics = Vec::new();
	let expected_call =
 ([TEEREX_MODULE, PROCESSED_PARENTCHAIN_BLOCK], block_hash, H256::default()).encode();

	// when
	let call = crate::processed_parentchain_block_extrinsic(block_hash, extrinsics);

	// then
	assert_eq!(call.0, expected_call);
}

fn some_extrinsics_vec_give_non_zero_merkle_root() {
	// given
	let block_hash = H256::from([1; 32]);
	let extrinsics = vec![H256::from([4; 32]), H256::from([9; 32])];
	let zero_root_call =
 ([TEEREX_MODULE, PROCESSED_PARENTCHAIN_BLOCK], block_hash, H256::default()).encode();

	// when
	let call = crate::processed_parentchain_block_extrinsic(block_hash, extrinsics);

	// then
	assert_ne!(call.0, zero_root_call);
}
@murerfelmurerfelNov 10, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Always nice to have tests for the changes you make 🥳
Comment threadenclave-runtime/src/lib.rs

Comment on lines

+1169
 to 
+1099

fn hash_of<T: Encode>(xt: T) -> H256 {
	blake2_256(&xt.encode()).into()
@murerfelmurerfelNov 10, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

This is a function that we have duplicated multiple times I believe. We should at some point put it in a primitives crate where it can be properly shared.
@haerdibhaerdibNov 10, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Aye.. But I didn't want to touch too much code, in fear of the rebase conflicts 😨
@clangenbclangenbNov 11, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I think we should simply implement hash on the extrinsic in the api-client. 😄
murerfel
murerfel
approved these changes
Nov 10, 2021
View reviewed changes
@murerfelmurerfel
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM - and if the checks pass, it should be fine ☺️ thanks for your perseverance when it comes to good comments 🚀
clangenb
clangenb
approved these changes
Nov 11, 2021
View reviewed changes
@clangenbclangenb
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Looks good to me. 👍
Comment threadcore-primitives/stf-executor/src/executor.rs
 account: AccountId,
 amount: Amount,
 shard: &ShardIdentifier,
 calls: &mut Vec<OpaqueCall>,
@clangenbclangenbNov 11, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Haha, good catch. 🛩️
Comment threadenclave-runtime/Cargo.toml
sp-utils = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master" }
sp-version = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master" }
sp-application-crypto = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master" }
#beefy-merkle-tree = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master", features = "keccak" }
@clangenbclangenbNov 11, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I think it's fine too. 👍
Comment threadenclave-runtime/src/lib.rs
 if let Err(e) = validator.submit_simple_header(
 validator.num_relays(),
signed_block.block.header().clone(),
 block.header().clone(),
@clangenbclangenbNov 11, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

nice optimization.
Comment threadenclave-runtime/src/lib.rs

Comment on lines

+1169
 to 
+1099

fn hash_of<T: Encode>(xt: T) -> H256 {
	blake2_256(&xt.encode()).into()
@clangenbclangenbNov 11, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I think we should simply implement hash on the extrinsic in the api-client. 😄
@haerdib
haerdib
merged commit e08bf0c
into
masterNov 11, 2021
@haerdib
haerdib
deleted the

separate-block-confirmations

branch
November 11, 2021 07:33
@haerdibhaerdib
mentioned this pull request
Nov 12, 2021
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@murerfelmurerfelmurerfel approved these changes
@clangenbclangenbclangenb approved these changes

Assignees

@haerdibhaerdib

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Clarify Block & Call Confirmation


3 participants

@haerdib@murerfel@clangenb

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Separate block confirmations - #501
Merged
haerdib merged 19 commits into
masterfrom 
separate-block-confirmations
Nov 11, 2021
Merged
Separate block confirmations#501
haerdib merged 19 commits into
masterfrom 
separate-block-confirmations
Conversation
@haerdib
@haerdibhaerdib
commented
Nov 10, 2021•

edited

Loading

Copy link


Copy Markdown

Contributor
Seems to work quite well: For every parentchain block we get 20 sidechain block events.
grafik
Introduces the following changes:
  • exchanges call- and block confirmation with parentchain and sidechain confirmations
  • removes IpfsHash Update Event (was removed completely from pallet teerex)
  • updates substrate version to the same of the integritee-node (https://github.com/paritytech/substrate.git?branch=master#b391b82954ad95a927a921035e3017c4a0aad516), wasn't compiling otherwise
  • cleans up shielding call execution call (at least a little)
  • temporarily introduces a new file for Merkel root calculation. This can be removed once we update to a substrate commit >= 23rd September
closes#457
@haerdib
haerdibforce-pushed
the
separate-block-confirmations
branch
from
9e45e6a to
386000bCompareNovember 10, 2021 12:49
@haerdibhaerdib
self-assigned this
Nov 10, 2021
haerdib
haerdib
commented
Nov 10, 2021
View reviewed changes
Comment threadcore-primitives/stf-executor/src/executor.rs
 account: AccountId,
 amount: Amount,
 shard: &ShardIdentifier,
 calls: &mut Vec<OpaqueCall>,
@haerdibhaerdibNov 10, 2021•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Removed this input parameter, as there will never(!) be a new OpaqueCall introduced during Stf::execute of a shielding extrsinic. (New OpaqueCall is only issued during execute when calling "unshield")
@murerfelmurerfelNov 10, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Thanks for clarifying this ☺️
@clangenbclangenbNov 11, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Haha, good catch. 🛩️
@haerdib
haerdib
requested review from
clangenb and murerfel
and removed request for
clangenbNovember 10, 2021 13:24
murerfel
murerfel
reviewed
Nov 10, 2021
View reviewed changes
Comment threadenclave-runtime/src/lib.rs
Outdated

haerdib
haerdib
commented
Nov 10, 2021
View reviewed changes
Comment threadenclave-runtime/Cargo.toml
sp-utils = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master" }
sp-version = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master" }
sp-application-crypto = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master" }
#beefy-merkle-tree = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master", features = "keccak" }
@haerdibhaerdibNov 10, 2021•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

That's the ugly part here: substrate actually has a no-std compatible merkle tree primitives crate: https://github.com/paritytech/substrate/tree/master/frame/beefy-mmr/primitives
However, this was introduced on 23rd of September. We are not yet on this substrate commit. Can you agree with this temporary solution and to remove it once we are on this commit? If yes, I'll create an appropriate issue ofc.
@murerfelmurerfelNov 10, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

If it's well documented and even has a corresponding issue, that's totally fine for me 👍
@clangenbclangenbNov 11, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I think it's fine too. 👍
Comment threadenclave-runtime/src/beefy_merkle_tree.rs
@@ -0,0 +1,229 @@
// This file is part of Substrate.
@haerdibhaerdibNov 10, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

This file was copy pasted from https://github.com/paritytech/substrate/blob/master/frame/beefy-mmr/primitives/src/lib.rs. Can be removed once we are above commit paritytech/substrate@65ac6f3
@haerdib
haerdibforce-pushed
the
separate-block-confirmations
branch
from
f1773ad to
5e49a19CompareNovember 10, 2021 14:04
murerfel
murerfel
reviewed
Nov 10, 2021
View reviewed changes
Comment threadenclave-runtime/src/lib.rs
Outdated


Comment on lines

+1105
 to 
+1109

	let xt_block = [TEEREX_MODULE, BLOCK_CONFIRMED];
	let opaque_call =
 OpaqueCall::from_tuple(&(xt_block, shard, block_hash, state_hash_new.encode()));
	let opaque_call = proposed_sidechain_block_extrinsic(shard, block_hash);
@murerfelmurerfelNov 10, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

so we now use the sidechain block hash instead of the state hash for this extrinsic?
@haerdibhaerdibNov 10, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Not exactly. State hash has been removed from the extrinsic as it didn't have any use case at all (#457)
Comment threadenclave-runtime/src/lib.rs

Comment on lines

+1113
 to 
+1045

/// Creates a proposed_sidechain_block extrinsic for a given shard id and sidechain block hash.
fn proposed_sidechain_block_extrinsic(shard_id: ShardIdentifier, block_hash: H256) -> OpaqueCall {
	OpaqueCall::from_tuple(&([TEEREX_MODULE, PROPOSED_SIDECHAIN_BLOCK], shard_id, block_hash))
}
@murerfelmurerfelNov 10, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Also here, I'd prefer the create_ in front. In a future refactoring, we could put these into the extrinsics factory component or something similar?
@haerdibhaerdibNov 10, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Already changed. :) Yes, definitely - but I think the whole lib.rs (just like in PR #500 but also for parentchain syncing) should be refactored (but that's your call).. I didn't want to start something that will be changed in the future anyway. If you have a place they belong to already, I'll gladly move them.
Comment threadenclave-runtime/src/tests.rs
Outdated


Comment on lines

+426
 to 
+452

fn empty_extrinsic_vec_gives_zero_merkle_root() {
	// given
	let block_hash = H256::from([1; 32]);
	let extrinsics = Vec::new();
	let expected_call =
 ([TEEREX_MODULE, PROCESSED_PARENTCHAIN_BLOCK], block_hash, H256::default()).encode();

	// when
	let call = crate::processed_parentchain_block_extrinsic(block_hash, extrinsics);

	// then
	assert_eq!(call.0, expected_call);
}

fn some_extrinsics_vec_give_non_zero_merkle_root() {
	// given
	let block_hash = H256::from([1; 32]);
	let extrinsics = vec![H256::from([4; 32]), H256::from([9; 32])];
	let zero_root_call =
 ([TEEREX_MODULE, PROCESSED_PARENTCHAIN_BLOCK], block_hash, H256::default()).encode();

	// when
	let call = crate::processed_parentchain_block_extrinsic(block_hash, extrinsics);

	// then
	assert_ne!(call.0, zero_root_call);
}
@murerfelmurerfelNov 10, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Always nice to have tests for the changes you make 🥳
Comment threadenclave-runtime/src/lib.rs

Comment on lines

+1169
 to 
+1099

fn hash_of<T: Encode>(xt: T) -> H256 {
	blake2_256(&xt.encode()).into()
@murerfelmurerfelNov 10, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

This is a function that we have duplicated multiple times I believe. We should at some point put it in a primitives crate where it can be properly shared.
@haerdibhaerdibNov 10, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Aye.. But I didn't want to touch too much code, in fear of the rebase conflicts 😨
@clangenbclangenbNov 11, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I think we should simply implement hash on the extrinsic in the api-client. 😄
murerfel
murerfel
approved these changes
Nov 10, 2021
View reviewed changes
@murerfelmurerfel
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM - and if the checks pass, it should be fine ☺️ thanks for your perseverance when it comes to good comments 🚀
clangenb
clangenb
approved these changes
Nov 11, 2021
View reviewed changes
@clangenbclangenb
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Looks good to me. 👍
Comment threadcore-primitives/stf-executor/src/executor.rs
 account: AccountId,
 amount: Amount,
 shard: &ShardIdentifier,
 calls: &mut Vec<OpaqueCall>,
@clangenbclangenbNov 11, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Haha, good catch. 🛩️
Comment threadenclave-runtime/Cargo.toml
sp-utils = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master" }
sp-version = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master" }
sp-application-crypto = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master" }
#beefy-merkle-tree = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master", features = "keccak" }
@clangenbclangenbNov 11, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I think it's fine too. 👍
Comment threadenclave-runtime/src/lib.rs
 if let Err(e) = validator.submit_simple_header(
 validator.num_relays(),
signed_block.block.header().clone(),
 block.header().clone(),
@clangenbclangenbNov 11, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

nice optimization.
Comment threadenclave-runtime/src/lib.rs

Comment on lines

+1169
 to 
+1099

fn hash_of<T: Encode>(xt: T) -> H256 {
	blake2_256(&xt.encode()).into()
@clangenbclangenbNov 11, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I think we should simply implement hash on the extrinsic in the api-client. 😄
@haerdib
haerdib
merged commit e08bf0c
into
masterNov 11, 2021
@haerdib
haerdib
deleted the

separate-block-confirmations

branch
November 11, 2021 07:33
@haerdibhaerdib
mentioned this pull request
Nov 12, 2021
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@murerfelmurerfelmurerfel approved these changes
@clangenbclangenbclangenb approved these changes

Assignees

@haerdibhaerdib

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Clarify Block & Call Confirmation


3 participants

@haerdib@murerfel@clangenb

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
Separate block confirmations - #501
Merged
haerdib merged 19 commits into
masterfrom 
separate-block-confirmations
Nov 11, 2021
Merged
Separate block confirmations#501
haerdib merged 19 commits into
masterfrom 
separate-block-confirmations
Conversation
@haerdib
@haerdibhaerdib
commented
Nov 10, 2021•

edited

Loading

Copy link


Copy Markdown

Contributor
Seems to work quite well: For every parentchain block we get 20 sidechain block events.
grafik
Introduces the following changes:
  • exchanges call- and block confirmation with parentchain and sidechain confirmations
  • removes IpfsHash Update Event (was removed completely from pallet teerex)
  • updates substrate version to the same of the integritee-node (https://github.com/paritytech/substrate.git?branch=master#b391b82954ad95a927a921035e3017c4a0aad516), wasn't compiling otherwise
  • cleans up shielding call execution call (at least a little)
  • temporarily introduces a new file for Merkel root calculation. This can be removed once we update to a substrate commit >= 23rd September
closes#457
@haerdib
haerdibforce-pushed
the
separate-block-confirmations
branch
from
9e45e6a to
386000bCompareNovember 10, 2021 12:49
@haerdibhaerdib
self-assigned this
Nov 10, 2021
haerdib
haerdib
commented
Nov 10, 2021
View reviewed changes
Comment threadcore-primitives/stf-executor/src/executor.rs
 account: AccountId,
 amount: Amount,
 shard: &ShardIdentifier,
 calls: &mut Vec<OpaqueCall>,
@haerdibhaerdibNov 10, 2021•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Removed this input parameter, as there will never(!) be a new OpaqueCall introduced during Stf::execute of a shielding extrsinic. (New OpaqueCall is only issued during execute when calling "unshield")
@murerfelmurerfelNov 10, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Thanks for clarifying this ☺️
@clangenbclangenbNov 11, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Haha, good catch. 🛩️
@haerdib
haerdib
requested review from
clangenb and murerfel
and removed request for
clangenbNovember 10, 2021 13:24
murerfel
murerfel
reviewed
Nov 10, 2021
View reviewed changes
Comment threadenclave-runtime/src/lib.rs
Outdated

haerdib
haerdib
commented
Nov 10, 2021
View reviewed changes
Comment threadenclave-runtime/Cargo.toml
sp-utils = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master" }
sp-version = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master" }
sp-application-crypto = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master" }
#beefy-merkle-tree = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master", features = "keccak" }
@haerdibhaerdibNov 10, 2021•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

That's the ugly part here: substrate actually has a no-std compatible merkle tree primitives crate: https://github.com/paritytech/substrate/tree/master/frame/beefy-mmr/primitives
However, this was introduced on 23rd of September. We are not yet on this substrate commit. Can you agree with this temporary solution and to remove it once we are on this commit? If yes, I'll create an appropriate issue ofc.
@murerfelmurerfelNov 10, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

If it's well documented and even has a corresponding issue, that's totally fine for me 👍
@clangenbclangenbNov 11, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I think it's fine too. 👍
Comment threadenclave-runtime/src/beefy_merkle_tree.rs
@@ -0,0 +1,229 @@
// This file is part of Substrate.
@haerdibhaerdibNov 10, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

This file was copy pasted from https://github.com/paritytech/substrate/blob/master/frame/beefy-mmr/primitives/src/lib.rs. Can be removed once we are above commit paritytech/substrate@65ac6f3
@haerdib
haerdibforce-pushed
the
separate-block-confirmations
branch
from
f1773ad to
5e49a19CompareNovember 10, 2021 14:04
murerfel
murerfel
reviewed
Nov 10, 2021
View reviewed changes
Comment threadenclave-runtime/src/lib.rs
Outdated


Comment on lines

+1105
 to 
+1109

	let xt_block = [TEEREX_MODULE, BLOCK_CONFIRMED];
	let opaque_call =
 OpaqueCall::from_tuple(&(xt_block, shard, block_hash, state_hash_new.encode()));
	let opaque_call = proposed_sidechain_block_extrinsic(shard, block_hash);
@murerfelmurerfelNov 10, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

so we now use the sidechain block hash instead of the state hash for this extrinsic?
@haerdibhaerdibNov 10, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Not exactly. State hash has been removed from the extrinsic as it didn't have any use case at all (#457)
Comment threadenclave-runtime/src/lib.rs

Comment on lines

+1113
 to 
+1045

/// Creates a proposed_sidechain_block extrinsic for a given shard id and sidechain block hash.
fn proposed_sidechain_block_extrinsic(shard_id: ShardIdentifier, block_hash: H256) -> OpaqueCall {
	OpaqueCall::from_tuple(&([TEEREX_MODULE, PROPOSED_SIDECHAIN_BLOCK], shard_id, block_hash))
}
@murerfelmurerfelNov 10, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Also here, I'd prefer the create_ in front. In a future refactoring, we could put these into the extrinsics factory component or something similar?
@haerdibhaerdibNov 10, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Already changed. :) Yes, definitely - but I think the whole lib.rs (just like in PR #500 but also for parentchain syncing) should be refactored (but that's your call).. I didn't want to start something that will be changed in the future anyway. If you have a place they belong to already, I'll gladly move them.
Comment threadenclave-runtime/src/tests.rs
Outdated


Comment on lines

+426
 to 
+452

fn empty_extrinsic_vec_gives_zero_merkle_root() {
	// given
	let block_hash = H256::from([1; 32]);
	let extrinsics = Vec::new();
	let expected_call =
 ([TEEREX_MODULE, PROCESSED_PARENTCHAIN_BLOCK], block_hash, H256::default()).encode();

	// when
	let call = crate::processed_parentchain_block_extrinsic(block_hash, extrinsics);

	// then
	assert_eq!(call.0, expected_call);
}

fn some_extrinsics_vec_give_non_zero_merkle_root() {
	// given
	let block_hash = H256::from([1; 32]);
	let extrinsics = vec![H256::from([4; 32]), H256::from([9; 32])];
	let zero_root_call =
 ([TEEREX_MODULE, PROCESSED_PARENTCHAIN_BLOCK], block_hash, H256::default()).encode();

	// when
	let call = crate::processed_parentchain_block_extrinsic(block_hash, extrinsics);

	// then
	assert_ne!(call.0, zero_root_call);
}
@murerfelmurerfelNov 10, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Always nice to have tests for the changes you make 🥳
Comment threadenclave-runtime/src/lib.rs

Comment on lines

+1169
 to 
+1099

fn hash_of<T: Encode>(xt: T) -> H256 {
	blake2_256(&xt.encode()).into()
@murerfelmurerfelNov 10, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

This is a function that we have duplicated multiple times I believe. We should at some point put it in a primitives crate where it can be properly shared.
@haerdibhaerdibNov 10, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Aye.. But I didn't want to touch too much code, in fear of the rebase conflicts 😨
@clangenbclangenbNov 11, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I think we should simply implement hash on the extrinsic in the api-client. 😄
murerfel
murerfel
approved these changes
Nov 10, 2021
View reviewed changes
@murerfelmurerfel
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM - and if the checks pass, it should be fine ☺️ thanks for your perseverance when it comes to good comments 🚀
clangenb
clangenb
approved these changes
Nov 11, 2021
View reviewed changes
@clangenbclangenb
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Looks good to me. 👍
Comment threadcore-primitives/stf-executor/src/executor.rs
 account: AccountId,
 amount: Amount,
 shard: &ShardIdentifier,
 calls: &mut Vec<OpaqueCall>,
@clangenbclangenbNov 11, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Haha, good catch. 🛩️
Comment threadenclave-runtime/Cargo.toml
sp-utils = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master" }
sp-version = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master" }
sp-application-crypto = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master" }
#beefy-merkle-tree = { version = "4.0.0-dev", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "master", features = "keccak" }
@clangenbclangenbNov 11, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I think it's fine too. 👍
Comment threadenclave-runtime/src/lib.rs
 if let Err(e) = validator.submit_simple_header(
 validator.num_relays(),
signed_block.block.header().clone(),
 block.header().clone(),
@clangenbclangenbNov 11, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

nice optimization.
Comment threadenclave-runtime/src/lib.rs

Comment on lines

+1169
 to 
+1099

fn hash_of<T: Encode>(xt: T) -> H256 {
	blake2_256(&xt.encode()).into()
@clangenbclangenbNov 11, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I think we should simply implement hash on the extrinsic in the api-client. 😄
@haerdib
haerdib
merged commit e08bf0c
into
masterNov 11, 2021
@haerdib
haerdib
deleted the

separate-block-confirmations

branch
November 11, 2021 07:33
@haerdibhaerdib
mentioned this pull request
Nov 12, 2021
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@murerfelmurerfelmurerfel approved these changes
@clangenbclangenbclangenb approved these changes

Assignees

@haerdibhaerdib

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Clarify Block & Call Confirmation


3 participants

@haerdib@murerfel@clangenb