Read registry_address from block with REQUEST_CONFIRMATIONS_REQUIRED #8309

sorpaas · 2018-04-04T14:14:46Z

Use REQUEST_CONFIRMATIONS_REQUIRED instead of the latest block. Fix one TODO item in code comment.

debris · 2018-04-05T10:55:07Z

secret_store/src/listener/service_contract.rs

-					self.data.write().contract_address = service_contract_addr;
+				if let Some(block_hash) = get_confirmed_block_hash(&*client, REQUEST_CONFIRMATIONS_REQUIRED) {
+					// update contract address from registry
+					let service_contract_addr = client.registry_address(self.name.clone(), BlockId::Hash(block_hash)).unwrap_or_default();


I'm not sure whether it will work with not archive nodes. cc @tomusdrw

right, the state might be pruned get_confirmed_block_hash could return the oldest ancestor possible in that case, but then the call to reigistry_address will race with the pruning. We really need some way to block pruning while we inspect state by using some kind of extended BlockId.

debris · 2018-04-05T10:57:22Z

this should be also reviewed by @svyatonik

rphmeier · 2018-04-05T11:12:09Z

Using a constant number of confirmations is a little tenuous. Can we add a TODO to do this based on consensus finality where possible instead?

sorpaas · 2018-04-05T12:11:41Z

@rphmeier There were originally a TODO item and I was trying to fix that. Looks like this becomes complicated if pruning is enabled, and I don't think I have a way to fix this...

I think the original TODO comment should be enough as a holder for anyone looks into this. So I'm going to close this PR if that's okay.

svyatonik · 2018-04-05T13:16:03Z

@debris @rphmeier Thanks for pointing to this. This code should work only if we're synchronized (!sync.status().is_syncing()) and have a full trust (chain_info().security_level().is_full()) => I guess that's kind of a guarantee that we're dealing with the most up-to-date state (at least that's how updater works). Also, please correct me if I'm wrong (that's what I'm not 100% sure about), but pruning leaves some recent states and since we're dealing with block latest_block - 3, it's state is also not pruned (I believe that recent_states.length > 3). So this means that we have some time to work with this state (until it'll be pruned). So the problem would only arise if we're trying to deal with this state for too long. And in case if state is pruned - trying to read from contract at this state would lead to an error. Am I correct? If so, I wouldn't consider this as a serious problem && I'm for letting this PR merged-in.

@rphmeier I guess number of confirmation is an universal decision (and afair our UI and I guess most of DApps use the similar approach). Thanks for pointing that there's another way, though. @sorpaas could you please add TODO for this.

@sorpaas Thanks for helping with this TODO :) It is a little bigger than just fixing service_contract.rs, though :) And while changing the acl_storage.rs in the same way is arguable (and I wouldn't recommend to do this), changing the client.registry_address call in the key_server_set.rs is desirable. Could you please also change this in the similar way? Or just move original TODO to that file, instead or removing it?

sorpaas · 2018-04-05T13:29:39Z

@svyatonik Okay. Will do!

rphmeier · 2018-04-05T14:17:22Z

@svyatonik

trying to read from contract at this state would lead to an error

yeah. since it's only 3 blocks behind the head of the chain (minimum pruning we allow for now is 8) this is probably not a problem, but it's an assumption that can be broken well outside of this module. Although if this is triggered while we're importing many blocks and not just at the head of the chain, there is a pretty bad race condition.

The finality stuff isn't useful to most DApps since they're on the mainnet, so their only choice is to rely on confirmations until CasperFFG. But where possible, it's more correct and safer to use finality. It's not something the codebase supports yet but it will soon.

svyatonik · 2018-04-06T07:09:00Z

secret_store/src/helpers.rs

@@ -0,0 +1,12 @@
+use ethcore::client::{Client, BlockChainClient, BlockId};


Could you please add license preamble to the beginning of the file?

Or maybe just make this function a member of TrustedClient? Both have similar purpose.

@svyatonik I added license preamble. Looks like for cases using get_confirmed_block_hash we need to directly dealt with a Client reference but not TrustedClient. So just thought this would be less complicated if we don't do some refactoring.

And regarding license preamble, I realized that they still say 2015-2017, do we need to change all of them to 2015-2018?

Don't think so - guess it will be enough for new file only. Thanks!

Okay. Got it!

svyatonik · 2018-04-06T07:10:02Z

Also please do not forget to add TODO about using finality when possible (see my big comment above). Thanks!

sorpaas · 2018-04-06T07:27:06Z

@svyatonik Sorry I missed that!

svyatonik

LGTM

Read registry_address from block with REQUEST_CONFIRMATIONS_REQUIRED

8a4bf55

sorpaas added the A0-pleasereview 🤓 Pull request needs code review. label Apr 4, 2018

sorpaas added this to the 1.11 milestone Apr 4, 2018

5chdn added the M4-core ⛓ Core client code / Rust. label Apr 5, 2018

debris reviewed Apr 5, 2018

View reviewed changes

debris requested a review from svyatonik April 5, 2018 10:57

sorpaas closed this Apr 5, 2018

svyatonik reopened this Apr 5, 2018

Require confirmation blocks in key_server_set

d05b090

svyatonik reviewed Apr 6, 2018

View reviewed changes

sorpaas added 2 commits April 6, 2018 15:18

Add license preamble

66ee62f

TODO item for constant confirmation required number

a78b94b

Change license year in helpers.rs to 2015-2018

dbfa67b

svyatonik approved these changes Apr 6, 2018

View reviewed changes

svyatonik added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Apr 6, 2018

5chdn merged commit d7a7f03 into openethereum:master Apr 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Read registry_address from block with REQUEST_CONFIRMATIONS_REQUIRED #8309

Read registry_address from block with REQUEST_CONFIRMATIONS_REQUIRED #8309

sorpaas commented Apr 4, 2018

debris Apr 5, 2018

rphmeier Apr 5, 2018 •

edited

Loading

debris commented Apr 5, 2018

rphmeier commented Apr 5, 2018

sorpaas commented Apr 5, 2018

svyatonik commented Apr 5, 2018

sorpaas commented Apr 5, 2018

rphmeier commented Apr 5, 2018 •

edited

Loading

svyatonik Apr 6, 2018

svyatonik Apr 6, 2018

sorpaas Apr 6, 2018

svyatonik Apr 6, 2018

sorpaas Apr 6, 2018

svyatonik commented Apr 6, 2018

sorpaas commented Apr 6, 2018

svyatonik left a comment

		@@ -0,0 +1,12 @@
		use ethcore::client::{Client, BlockChainClient, BlockId};

Read registry_address from block with REQUEST_CONFIRMATIONS_REQUIRED #8309

Read registry_address from block with REQUEST_CONFIRMATIONS_REQUIRED #8309

Conversation

sorpaas commented Apr 4, 2018

debris Apr 5, 2018

Choose a reason for hiding this comment

rphmeier Apr 5, 2018 • edited Loading

Choose a reason for hiding this comment

debris commented Apr 5, 2018

rphmeier commented Apr 5, 2018

sorpaas commented Apr 5, 2018

svyatonik commented Apr 5, 2018

sorpaas commented Apr 5, 2018

rphmeier commented Apr 5, 2018 • edited Loading

svyatonik Apr 6, 2018

Choose a reason for hiding this comment

svyatonik Apr 6, 2018

Choose a reason for hiding this comment

sorpaas Apr 6, 2018

Choose a reason for hiding this comment

svyatonik Apr 6, 2018

Choose a reason for hiding this comment

sorpaas Apr 6, 2018

Choose a reason for hiding this comment

svyatonik commented Apr 6, 2018

sorpaas commented Apr 6, 2018

svyatonik left a comment

Choose a reason for hiding this comment

rphmeier Apr 5, 2018 •

edited

Loading

rphmeier commented Apr 5, 2018 •

edited

Loading