Ignore global cache for patched accounts #9752
Conversation
grbIzl
added
A0-pleasereview 🤓
|
Please review @andresilva @cheme :) |
ethcore/src/state/mod.rs
Outdated
| Ok(self.require(a, false)?.reset_code_and_storage(code, storage)) | ||
| let mut account = self.require(a, false)?; | ||
| account.switch_to_patched(); | ||
| Ok(account.reset_code_and_storage(code, storage)) |
There was a problem hiding this comment.
I got a question regarding 'reset_code_and_storage' : currently it does not touch account fields 'original_storage_cache', shouldn't we (I am not 100% sure but it seems safer) ?
In fact the PR seems to try to ignore lru storage cache since storage is ignored in private_tx, but to get eip-1283 running we probably want to keep trace of original_storage_value, I am starting to wonder how it will behave.
There was a problem hiding this comment.
My understanding of eip-1283 is gas optimization of SSTORE calls. For this case original cache may be required. In terms of this PR I fixed caching logic for SLOAD, which doesn't use original cache.
But again it's only my understanding. I don't know, how original cache can be further evolved. So it would be great, if @sorpaas can comment on this.
There was a problem hiding this comment.
I imagine plugging the encrypted state as a replacement for the trieDB storage could make it run ok, but there may be simpler way to do it.
Unrelated question, about those private contract, what are the limitation, it seems logic that they cannot change the state of other contracts (kinda break the confidentiality), but are they allowed to call external contract?
ethcore/src/state/account.rs
Outdated
| @@ -263,6 +281,10 @@ impl Account { | |||
| if let Some(value) = self.storage_changes.get(key) { | |||
| return Some(value.clone()) | |||
| } | |||
| //Use only in memory state for the patched account | |||
| if self.patched_account { | |||
There was a problem hiding this comment.
That is probably true for others function ('storage_at' will query the triedb for instance), do we need this kind of guard for them too?
I wonder if it wouldn't be ok to use the storage cache and original cache value (init them in 'reset_code_and_storage') and just prevent querying the triedb at 'get_and_cache_storage' ?
There was a problem hiding this comment.
Actually it is the guard for storage_at, because it directly calls cached_storage_at:
pub fn storage_at(&self, db: &HashDB<KeccakHasher, DBValue>, key: &H256) -> TrieResult<H256> {
if let Some(value) = self.cached_storage_at(key) {
return Ok(value);
}
Self::get_and_cache_storage(
&self.storage_root,
&mut self.storage_cache.borrow_mut(),
db,
key)
}
There was a problem hiding this comment.
I think 'Self::get_and_cache_storage' will be call due to this function returning 'None' value on missing key, then it will query the triedb (from the 'storage_at' code).
ethcore/src/state/account.rs
Outdated
| @@ -206,6 +214,16 @@ impl Account { | |||
| self.storage_changes = storage; | |||
| } | |||
|
|
|||
| /// Swtich account to the patched mode | |||
| pub fn switch_to_patched(&mut self) { | |||
| self.patched_account = true; | |||
There was a problem hiding this comment.
Reseting some account values here could make sense. Maybe this function could be combine with 'patch_account' somehow to avoid strange account state if future code forgot to call 'patch_account'.
There was a problem hiding this comment.
I was thinking heavily on how to achieve this, because I also don't like the this kind of dangling state for account. Didn't find a good solution. This information (if account is patched) is required both in state and account, but the external code works with the state only. So in such constraints this was the only suitable solution (IMO).
Meanwhile 'forget' to patch account is pretty difficult, the more dangerous situation may appear, if someone starts to use patched accounts as regular ones. Here I tried to prevent this with asserts in commit methods.
There was a problem hiding this comment.
I think I am missing some code/context, I only find 'switch_to_patched' function call in 'patch_account' account function, so I was simply thinking of removing this fn and make the bool switch in account fn 'reset_code_and_storage' (which seems to be also called at a single place just after switch_to_patched).
There was a problem hiding this comment.
@grbIzl Before we proceed on this, I wonder whether a simpler change would work:
- In
state::account::Account::reset_code_and_storage, in addition to what we have right now, addself.storage_root = KECCAK_NULL_RLP.
This should make it so that the patched account does not have any additional storage inherit from non-patched address. And it doesn't introduce additional complications!
My understanding of private transaction is limited, so please let me know if I get this wrong. I also have one question:
- When we "patch" an account for private transaction, do we need to care about its nonce and balance? I didn't find current code that deals with this.
ethcore/src/state/account.rs
Outdated
| @@ -263,6 +281,10 @@ impl Account { | |||
| if let Some(value) = self.storage_changes.get(key) { | |||
| return Some(value.clone()) | |||
| } | |||
| //Use only in memory state for the patched account | |||
ethcore/src/state/account.rs
Outdated
| @@ -474,6 +496,7 @@ impl Account { | |||
|
|
|||
| /// Commit the `storage_changes` to the backing DB and update `storage_root`. | |||
| pub fn commit_storage(&mut self, trie_factory: &TrieFactory, db: &mut HashDB<KeccakHasher, DBValue>) -> TrieResult<()> { | |||
| assert!(!self.patched_account, "Patched account should not be committed"); | |||
There was a problem hiding this comment.
Why are we sure here that patched account won't be committed?
|
@sorpaas Thanks for help! Yes, resetting storage root does the trick. I reverted my changes in account logic. As for your question: we have a regular smart contract - "wrapper" for the private contract, that stores private contract's state and code (encrypted). We call this wrapper "public contract". For the execution, we temporarily patch public contract's state and code with decrypted state and code from private contract and run transaction on such patched state. Nonce and balance during execution taken from the public contract, they are in a way "shared" between private and public contracts. So we don't need to care about them for this code. Do you see any issues with such approach? |
|
@grbIzl Thanks for the explanation. Sounds good! I don't see any issues at least for now. :) |
There was a problem hiding this comment.
Sorry I missed this, we also need to add (also in state::account::Account::reset_code_and_storage):
if self.storage_root != KECCAK_NULL_RLP {
self.original_storage_cache = Some((self.storage_root, Self::empty_storage_cache()));
}
This avoids patched account breaking its own checkpointing storage fetch. This particular situation would probably never really matter, but nonetheless I think it would be good to get them correct, for completeness.
There was a problem hiding this comment.
LGTM (original_storage_at will always call cached_storage_at so it seems fine regarding my previous interrogations). Just a litle concern about the fact that we rely on this particular state of the code that may break with future code change, but I cannot think of any good test case to prevent it :)
For private transactions we patch the state in order to apply encrypted data upon it. Such state is not commited and used only in order to calculate the results of the private transaction. In terms of this PR patched state was introduced explicitly for the state and the account in order to work with cache properly. For such patched accounts only local cache should be used (as it was changed during "patching").
Closes #9494