Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

move sidechain_importBlock rpc method to trusted rpc #1605

Merged

Conversation

kziemianek
Copy link
Contributor

The main goal of this PR is to move sidechain_importBlock from untrusted rpc endpoint to trusted. By doing so, there is no need to query for untrusted url anymore during peers update. It also:

  • removes enclave call.
  • removes rpc_methods trusted json-rpc method

Copy link
Contributor

@clangenb clangenb left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the upstream PR I have nothing to add LGTM!

Comment on lines -172 to +164
let worker_api_direct = DirectWorkerApi::new(enclave_url.clone().into());
match worker_api_direct.get_untrusted_worker_url() {
Ok(untrusted_worker_url) => {
peer_urls.push(untrusted_worker_url);
},
Err(e) => {
error!(
"Failed to get untrusted worker url (enclave: {}): {:?}",
enclave_url, e
);
},
}
peer_urls.push(enclave_url.into());
Copy link
Contributor

Choose a reason for hiding this comment

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

Just commenting for the records, we change the to enclave_url because the server is now running on the trusted side.

Comment on lines +111 to +112
let direct_client = DirectWorkerApi::new(url.clone());
if let Err(e) = direct_client.import_sidechain_blocks(encoded_blocks_cloned) {
Copy link
Contributor

@clangenb clangenb Jul 24, 2024

Choose a reason for hiding this comment

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

Nice code quality improvement :)

Comment on lines 39 to 41
let fetch_sidechain_blocks_module = BlockFetchServerModuleBuilder::new(sidechain_block_fetcher)
.build()
.map_err(|e| CallError::Failed(e.to_string().into()))?; // `to_string` necessary due to no all errors implementing Send + Sync.
Copy link
Contributor

Choose a reason for hiding this comment

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

hmmm, I guess the sidechain block fetcher should also be on the trusted side eventually.

@clangenb clangenb added A3-sidechain B1-releasenotes C1-low 📌 Does not elevate a release containing this beyond "low priority" E0-breaksnothing labels Jul 24, 2024
@clangenb clangenb merged commit e6761d3 into integritee-network:master Jul 24, 2024
32 of 36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A3-sidechain B1-releasenotes C1-low 📌 Does not elevate a release containing this beyond "low priority" E0-breaksnothing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants