Renaming of unclear SB and PB variable names #605

haerdib · 2021-12-30T12:27:03Z

some clean up of names:
- SB -> SidechainBlock
- PB -> ParentchainBlock
- xxxT -> xxxTrait

haerdib

When implementing out of sync detection the clean up got a little out of hand. So I'm doing a separate PR for the name clean up

haerdib · 2021-12-30T12:28:26Z

core-primitives/types/src/lib.rs

 	pub cyphertext: Vec<u8>,
 }

-pub type IpfsHash = [u8; 46];


Seemed strange to me that some types were defined below struct implementations, so I moved them up.

haerdib · 2021-12-30T12:30:47Z

sidechain/consensus/aura/src/lib.rs

+	ValidateerFetcher: ValidateerFetch + GetStorageVerified,
 	P: Pair,
-	B: ParentchainBlock<Hash = H256>,
+	ParentchainHeader: ParentchainHeaderTrait<Hash = H256>,


As only the header is used in this function, I removed the Block input enforcement.

I think that's fine, yes 👍

murerfel

I really appreciate the work you've put into this re-naming ☺️ I would even go one step further, and clearly distinguish SignedSidechainBlock from SidechainBlock - right now we still mix the two and the type parameter SidechainBlock is used for both.

murerfel · 2022-01-10T07:05:41Z

sidechain/consensus/aura/src/block_importer.rs

-	SB: SignedBlockT<Public = Authority::Public> + 'static,
-	SB::Block: BlockT<ShardIdentifier = H256>,
+	ParentchainBlock: ParentchainBlockTrait<Hash = H256>,
+	SidechainBlock: SignedBlockTrait<Public = Authority::Public> + 'static,


This type should probably be named SignedSidechainBlock? It has confused me a couple of times, because we sometimes use SB for the 'regular' sidechain block, and sometimes for the signed sidechain block.

I agree here, confused me too.

murerfel · 2022-01-10T07:08:54Z

sidechain/consensus/aura/src/lib.rs

+	ValidateerFetcher: ValidateerFetch + GetStorageVerified,
 	P: Pair,
-	B: ParentchainBlock<Hash = H256>,
+	ParentchainHeader: ParentchainHeaderTrait<Hash = H256>,


I think that's fine, yes 👍

murerfel · 2022-01-10T07:12:30Z

sidechain/consensus/common/src/error.rs

-	#[error("Bad parentchain block (Hash={0}). Reason: {1}")]
-	BadSidechainBlock(BlockHash, String),
+	BadParentchainBlock(ParentchainBlockHash, String),
+	#[error("Bad sidechain block (Hash={0}). Reason: {1}")]


good catch!

clangenb · 2022-01-10T07:43:18Z

sidechain/consensus/aura/src/block_importer.rs

-	SB: SignedBlockT<Public = Authority::Public> + 'static,
-	SB::Block: BlockT<ShardIdentifier = H256>,
+	ParentchainBlock: ParentchainBlockTrait<Hash = H256>,
+	SidechainBlock: SignedBlockTrait<Public = Authority::Public> + 'static,


I agree here, confused me too.

clangenb · 2022-01-10T07:51:39Z

sidechain/consensus/aura/src/block_importer.rs

+	ParentchainBlockImportTrigger: TriggerParentchainBlockImport<SignedParentchainBlockGeneric<ParentchainBlock>>
+		+ Send
+		+ Sync,
 {


In occasions like this I get unsure, whether it would be better to use well-defined acronyms as trait parameters (more consistently used than the ones before) after all because you loose the overview:

of opening and closing <

when it extends to multiple lines.

I think it would be reasonable to use acronyms for very frequently used traits, like ParentchainBlock, SidechainBlock, SignedSidechainBlock

What do you think about that?

I'm fine with that, but if we decided on something, we should put it into action immediately, otherwise it will get lost again.

Would an alias not also solve this problem? In this case an alias for TriggerParentchainBlockImport<SignedParentchainBlockGeneric<ParentchainBlock>> ?

Discussion result: We will continue using the long, but descriptive names for now.

TriggerParentchainBlockImport<SignedParentchainBlockGeneric>

The problem with that is, that TriggerParentchainBlockImport is a trait, and trait aliasing is not yet resolved: rust-lang/rust#41517

I think it's safer to leave as is for now. I removed the extra "Generic" though, as Traits are now annotated with Trait

…ppropriate

…ock Where necessary

…nBlock

murerfel

LGTM 👍

murerfel

LGTM (still 😄 ) 👍

clangenb

LGTM

* Make mu-ra and untrusted worker url queriable (integritee-network#595) * extract request_keys() to separate file * remove providr input, add dummy getter function * add node_api worker_for_shard call * fix error message * add primitives cache and rpc call * fix tests * add primitives-cache to workspace * fix unit tests * remove obsolete .yml provider from request-keys cmd * remove provider_addr from CI py scripts * fix reported worker address * improve usability of rpc-client * make it work * fix rebase error * add some delay * update local setup script * remove ugly async worker url, replace with enclave getter function * some steps towards a working exmaple.. * add peer_updater * fix unit test * fix some test clippy warnings * fix function name * fix client mu ra url * fix comment * fix comment * rename state_sync to appropriate request keys * fix comments and add missing _size to untrusted_worker_addr * update cargo.lock after rebase * fix typos * rename store_peers to set_peers * fix comment * move set_primitives to primitves cache repository * return read guard instead of primittves clone * rename config worker_rpc_port to trusted_worker_port * remove obsolete Error enum from request_keys.rs * fix unit tests * move thread spawning back into watch fn * rename worker-rpc-port to trusted-worker-port * readd external worker address * fix unit tests * fix unit test * add external addresses, optional port input and unit tests * update test names * [cli.yml] update shorts * fix local setup configs * change untrusted worker port to w * [sidechain] detect out of sync error (integritee-network#606) * inital commit * remove unwrap_err from assert_matches * Update substrate sp-core to version 4.1.0-dev (integritee-network#612) Co-authored-by: Gaudenz Kessler <[email protected]> * Renaming of unspecific SB and PB variable names (integritee-network#605) * some clean up & add hanlde import error * remove logic changes * fix unit tests * [aura block importer] rename SidechainBlock to SignedSidechainBlock * fix rebase errors * [aura mock] rename xxT import to xxTrait * [aura verifier] rename SidechainBlock to SignedSidechainblock where appropriate * fix rebase errors * [aura] rename Sidechainblock to SignedSidechainBlock * rename SB & PB to full written version and adapt to SignedSidechainBlock Where necessary * [sidechain block imported] remove extra generic from SignedParentchainBlock * some further SB & PB clean up * rename B & SB to SidechainBlock & SignedSidechainBlock * some further renaming * completely remove SB * rename all left over PBs * remove rebase error & rename to SignedSidechainBlock * rename to SignedSidechainBlock * Sidechain peer fetch blocks - RPC client/server (integritee-network#580) * WIP: RPC call to fetch sidechain blocks * WIP: sidechain peer fetch crate with RPC server and client impl * WIP: test for RPC peer sidechain block fetching * fix unit test * remove obsolete comment * fix rebase error * cargo fmt * fix tests * fix Fixme + add som Send+Sync to errors * update add_block_to_batch to return error. Otherwise silent fail * small comment fixes * make some comments better understandable * remove FIXME comment * remove new lines * fix trailing comments * [peer-fetch] change order or crates in .toml * [sidechain storage] fix error message of HeaderAncestryMismatch * [sidechain storage] exchange match statement with ok_or * [sidechain storage] use temp-dir in tests * [sidechain storage] remove extra genesis block check * fix rebase errors * remove untrstued url, replace with untrusted peer fetcher * [sidechain storage] fix comment * update delete_last_block description comment * [sidechain storage] fix comment grammer * move FetchUntrustedPeers trait to the top * [FetchBlocksFromPeer] extend description comment * update cargo.lock * rename get_blocks_following to get_blocks_after * rename get_blocks_following to get_blocks_after * rename all leftover "blocks_following" to "block_after" Co-authored-by: Bigna Härdi <[email protected]> * [Sidechain] Peer block fetching o-call implementation (integritee-network#619) * introduce o-call for fetching sidechain blocks from peer * re-name api-client-extensions to node-api-extensions Sub-task of integritee-network#567 * add direct call rpc doc (integritee-network#620) * add some doc * add some structure to the links * restructure rpc interface * Update docs/README.md Co-authored-by: gaudenzkessler <[email protected]> * adapt readmes according to review comments Co-authored-by: gaudenzkessler <[email protected]> * [Sidechain] Peer sync and block production suspension (integritee-network#618) Peer syncer implementation (not in use yet) and block production suspender (also not in use yet) * update to most recent teaclave commit (integritee-network#624) * Sync state from fellow validateer (integritee-network#615) * rename request_keys to sync_state * rename request_key_prov to request_state_prov * rename request_keys.rs to sync_state.rs * restructure key and state provisioning server * some refactoring * add TlsServer struct * add test file * rename key_provision_server to state_provisioning_server * add unit test * update unit test * introduce mockable key handler struct * shielding key success * remove clippy warnings * fix test * add unit tests for KeyHandler * rename to prepare for state inclusion * rename seal_handler * add shard as argument to sync state * some more renaming * add shard read & write process * [SealHandler] add unit tests & fix state * update networking test to include state * add default shard * add some documentation * remove ugly for loop * move authentications to separate file * update comment * remove obsolete, never ending loop * add error logs * remove extra phantom field * add sgx feature flag * remove global variables from test * add join handle to test * add some more logging info * Change tokio runtime to use 2 worker threads. Gossiping spawns new tokio tasks. (integritee-network#626) * Add state update sequence (integritee-network#632) * add bock_import_sequence.svg * move block_import.svg to docs/diagramms * update diagramm * add block import sequence * RPC call to get metadata from sgx-runtime (integritee-network#642) * RPC call to get metadata from sgx-runtime - rcp call - print sgx metadata cli * Change from review: Metadata is already encoded * Change from review Co-authored-by: echevrier <[email protected]> * bump substrate to commit 59649dd (integritee-network#645) * update .tomls to new substrate versions * cargo update * RawEvent -> Event * remove default from Accountid * RawEvent -> Event * cargo update enclave-runtime * fix bump errors * remove unused patch * finaly compiling * update sgx-runtime and substrate-api-client to github * remove integritee-node-runtime patch * cargo update -p std-std --precise 59649dd * update Github Actions integritee node * remove bh-config * fix clippy * fix cargo test * update spec version * update substrate-api-client * update sgx-runtime source * update substrate * adjust node version values * detect new game * solve merge conflicts * update sgx-runtime * fix some things * cargo format Co-authored-by: gaudenzkessler <[email protected]> Co-authored-by: Gaudenz Kessler <[email protected]> Co-authored-by: Felix Müller <[email protected]> Co-authored-by: echevrier <[email protected]> Co-authored-by: echevrier <[email protected]>

* Make mu-ra and untrusted worker url queriable (integritee-network#595) * extract request_keys() to separate file * remove providr input, add dummy getter function * add node_api worker_for_shard call * fix error message * add primitives cache and rpc call * fix tests * add primitives-cache to workspace * fix unit tests * remove obsolete .yml provider from request-keys cmd * remove provider_addr from CI py scripts * fix reported worker address * improve usability of rpc-client * make it work * fix rebase error * add some delay * update local setup script * remove ugly async worker url, replace with enclave getter function * some steps towards a working exmaple.. * add peer_updater * fix unit test * fix some test clippy warnings * fix function name * fix client mu ra url * fix comment * fix comment * rename state_sync to appropriate request keys * fix comments and add missing _size to untrusted_worker_addr * update cargo.lock after rebase * fix typos * rename store_peers to set_peers * fix comment * move set_primitives to primitves cache repository * return read guard instead of primittves clone * rename config worker_rpc_port to trusted_worker_port * remove obsolete Error enum from request_keys.rs * fix unit tests * move thread spawning back into watch fn * rename worker-rpc-port to trusted-worker-port * readd external worker address * fix unit tests * fix unit test * add external addresses, optional port input and unit tests * update test names * [cli.yml] update shorts * fix local setup configs * change untrusted worker port to w * [sidechain] detect out of sync error (integritee-network#606) * inital commit * remove unwrap_err from assert_matches * Update substrate sp-core to version 4.1.0-dev (integritee-network#612) Co-authored-by: Gaudenz Kessler <[email protected]> * Renaming of unspecific SB and PB variable names (integritee-network#605) * some clean up & add hanlde import error * remove logic changes * fix unit tests * [aura block importer] rename SidechainBlock to SignedSidechainBlock * fix rebase errors * [aura mock] rename xxT import to xxTrait * [aura verifier] rename SidechainBlock to SignedSidechainblock where appropriate * fix rebase errors * [aura] rename Sidechainblock to SignedSidechainBlock * rename SB & PB to full written version and adapt to SignedSidechainBlock Where necessary * [sidechain block imported] remove extra generic from SignedParentchainBlock * some further SB & PB clean up * rename B & SB to SidechainBlock & SignedSidechainBlock * some further renaming * completely remove SB * rename all left over PBs * remove rebase error & rename to SignedSidechainBlock * rename to SignedSidechainBlock * Sidechain peer fetch blocks - RPC client/server (integritee-network#580) * WIP: RPC call to fetch sidechain blocks * WIP: sidechain peer fetch crate with RPC server and client impl * WIP: test for RPC peer sidechain block fetching * fix unit test * remove obsolete comment * fix rebase error * cargo fmt * fix tests * fix Fixme + add som Send+Sync to errors * update add_block_to_batch to return error. Otherwise silent fail * small comment fixes * make some comments better understandable * remove FIXME comment * remove new lines * fix trailing comments * [peer-fetch] change order or crates in .toml * [sidechain storage] fix error message of HeaderAncestryMismatch * [sidechain storage] exchange match statement with ok_or * [sidechain storage] use temp-dir in tests * [sidechain storage] remove extra genesis block check * fix rebase errors * remove untrstued url, replace with untrusted peer fetcher * [sidechain storage] fix comment * update delete_last_block description comment * [sidechain storage] fix comment grammer * move FetchUntrustedPeers trait to the top * [FetchBlocksFromPeer] extend description comment * update cargo.lock * rename get_blocks_following to get_blocks_after * rename get_blocks_following to get_blocks_after * rename all leftover "blocks_following" to "block_after" Co-authored-by: Bigna Härdi <[email protected]> * [Sidechain] Peer block fetching o-call implementation (integritee-network#619) * introduce o-call for fetching sidechain blocks from peer * re-name api-client-extensions to node-api-extensions Sub-task of integritee-network#567 * add direct call rpc doc (integritee-network#620) * add some doc * add some structure to the links * restructure rpc interface * Update docs/README.md Co-authored-by: gaudenzkessler <[email protected]> * adapt readmes according to review comments Co-authored-by: gaudenzkessler <[email protected]> * [Sidechain] Peer sync and block production suspension (integritee-network#618) Peer syncer implementation (not in use yet) and block production suspender (also not in use yet) * update to most recent teaclave commit (integritee-network#624) * Sync state from fellow validateer (integritee-network#615) * rename request_keys to sync_state * rename request_key_prov to request_state_prov * rename request_keys.rs to sync_state.rs * restructure key and state provisioning server * some refactoring * add TlsServer struct * add test file * rename key_provision_server to state_provisioning_server * add unit test * update unit test * introduce mockable key handler struct * shielding key success * remove clippy warnings * fix test * add unit tests for KeyHandler * rename to prepare for state inclusion * rename seal_handler * add shard as argument to sync state * some more renaming * add shard read & write process * [SealHandler] add unit tests & fix state * update networking test to include state * add default shard * add some documentation * remove ugly for loop * move authentications to separate file * update comment * remove obsolete, never ending loop * add error logs * remove extra phantom field * add sgx feature flag * remove global variables from test * add join handle to test * add some more logging info * Change tokio runtime to use 2 worker threads. Gossiping spawns new tokio tasks. (integritee-network#626) * Add state update sequence (integritee-network#632) * add bock_import_sequence.svg * move block_import.svg to docs/diagramms * update diagramm * add block import sequence * RPC call to get metadata from sgx-runtime (integritee-network#642) * RPC call to get metadata from sgx-runtime - rcp call - print sgx metadata cli * Change from review: Metadata is already encoded * Change from review Co-authored-by: echevrier <[email protected]> * bump substrate to commit 59649dd (integritee-network#645) * update .tomls to new substrate versions * cargo update * RawEvent -> Event * remove default from Accountid * RawEvent -> Event * cargo update enclave-runtime * fix bump errors * remove unused patch * finaly compiling * update sgx-runtime and substrate-api-client to github * remove integritee-node-runtime patch * cargo update -p std-std --precise 59649dd * update Github Actions integritee node * remove bh-config * fix clippy * fix cargo test * update spec version * update substrate-api-client * update sgx-runtime source * update substrate * adjust node version values * detect new game * solve merge conflicts * update sgx-runtime * fix some things * cargo format Co-authored-by: haerdib <[email protected]> Co-authored-by: Gaudenz Kessler <[email protected]> Co-authored-by: Felix Müller <[email protected]> Co-authored-by: Bigna Härdi <[email protected]> Co-authored-by: echevrier <[email protected]> Co-authored-by: echevrier <[email protected]>

haerdib mentioned this pull request Dec 30, 2021

Sync state by retrieving sidechain blocks from storage of a different, given worker #567

Closed

9 tasks

haerdib changed the title ~~[sidechain] allow import error handling~~ [sidechain] name clean up Dec 30, 2021

haerdib commented Dec 30, 2021

View reviewed changes

haerdib requested a review from clangenb December 30, 2021 12:40

haerdib self-assigned this Dec 30, 2021

haerdib requested a review from murerfel January 10, 2022 06:46

murerfel reviewed Jan 10, 2022

View reviewed changes

clangenb reviewed Jan 10, 2022

View reviewed changes

haerdib force-pushed the bh-detect-out-of-sync-error branch from 0a5c47f to 8eddcc6 Compare January 10, 2022 09:17

haerdib added 10 commits January 10, 2022 11:43

some clean up & add hanlde import error

8f196d3

remove logic changes

d642b70

fix unit tests

096b69d

[aura block importer] rename SidechainBlock to SignedSidechainBlock

af51db8

fix rebase errors

3c9d6b7

[aura mock] rename xxT import to xxTrait

6480f69

[aura verifier] rename SidechainBlock to SignedSidechainblock where a…

70f8030

…ppropriate

fix rebase errors

2e1ee8a

[aura] rename Sidechainblock to SignedSidechainBlock

a24a358

rename SB & PB to full written version and adapt to SignedSidechainBl…

60c7146

…ock Where necessary

haerdib force-pushed the bh-detect-out-of-sync-error branch from 55cd6cd to 60c7146 Compare January 10, 2022 10:43

[sidechain block imported] remove extra generic from SignedParentchai…

0d36119

…nBlock

murerfel approved these changes Jan 10, 2022

View reviewed changes

clangenb approved these changes Jan 10, 2022

View reviewed changes

haerdib added 7 commits January 10, 2022 12:16

some further SB & PB clean up

f89ccc3

rename B & SB to SidechainBlock & SignedSidechainBlock

685cf5e

some further renaming

8b0fbc5

completely remove SB

165bbc8

rename all left over PBs

bc3c3bb

remove rebase error & rename to SignedSidechainBlock

0fe77dc

rename to SignedSidechainBlock

80e9221

haerdib changed the title ~~[sidechain] name clean up~~ Renaming of unclear SB and PB variable names Jan 10, 2022

haerdib requested review from clangenb and murerfel January 10, 2022 12:09

murerfel approved these changes Jan 10, 2022

View reviewed changes

clangenb approved these changes Jan 10, 2022

View reviewed changes

haerdib merged commit 6c2cb6e into master Jan 10, 2022

haerdib deleted the bh-detect-out-of-sync-error branch January 10, 2022 14:58

Uh oh!

Renaming of unclear SB and PB variable names #605

Renaming of unclear SB and PB variable names #605

Uh oh!

Conversation

haerdib commented Dec 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

haerdib left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

murerfel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

murerfel left a comment

Choose a reason for hiding this comment

Uh oh!

murerfel left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clangenb left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

haerdib commented Dec 30, 2021 •

edited

Loading

murerfel left a comment •

edited

Loading