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
Implement generational storage #87
Conversation
* Base precompile code between connectors * Handle errors and validate input * Use proper result
@@ -222,16 +222,16 @@ impl Engine { | |||
.unwrap_or_else(U256::zero) | |||
} | |||
|
|||
pub fn remove_storage(address: &Address, key: &H256) { | |||
sdk::remove_storage(&storage_to_key(address, key)); | |||
pub fn remove_storage(address: &Address, key: &H256, generation: u32) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about creating a separate struct:
AddressWithGeneration {
address: Address,
generation: u64,
}
You can use a different (and better) name for it.
This way interface for all this method will be a little bit cleaner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typically I do stuff like that if we get over 5 arguments. Here, I didn't see much of a point but, I get that grouping it together would be fine.
@@ -503,7 +522,8 @@ impl evm::backend::Backend for Engine { | |||
|
|||
/// Get storage value of address at index. | |||
fn storage(&self, address: Address, index: H256) -> H256 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not from this PR, but still
It is weird to use index
for this variable name here and key
elsewhere (see get_storage
/set_storage
), when they are referring to the same thing (have same semantics). I'd keep key
which I believe is what ethereum is calling this value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, SSTORE
and SLOAD
use the nomenclature key
.
let generation = Self::get_generation(&address); | ||
Engine::get_storage(&address, &index, generation) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't feel as efficient as possible. We are making two read access per read in the evm.
What about we have some cache created on the fly on each call. Since the most common scenarios is contracts reading from the same address this should be a noticeable improve. Generation is only read once per call per address.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can discuss how to do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it was kind of frustrating to really find a good way to do this and I was quite hesitant. I wanted to look into if we can do some early returns on some other parts. But, I'm not sure if that would be possible. The idea is to only read it once the entire time though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that caching would likely be a good idea. It's a little unfortunate that the type signature of fn storage
is &self
instead of &mut self
.
Maybe it's worth merging a version without caching and make an issue to do some optimization work in the future. This way we can get the core fix in faster and also spend more dedicated time on optimization. I think optimization should usually be a dedicated task so that measurements can be made as part of the process. E.g. we will know how much gas is saved by adding a caching layer, or whatever other clever solution we might come up with given more time to design something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with Michael, let's delay this optimisation for future PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In principle, we could defer optimizations. However, given that we are already at (or over) any reasonable gas use with the current contract, the impact of everything like this ought to at least be quantified. Adding a few gas-guzzling tweaks here and there may impact our bottom line soon enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made some changes to this which stores the value in state upon creation. That way if there are multiple storage reads, then it'll only do it once. I don't know if the storage read is going to be always required at least once.
eb569aa
to
9f11b69
Compare
* Validate register length in `read_input_arr20()` * Only read register length in `Engine::get_code_size` * Add `read_input_borsh()` * Ensure `method.args.len() == args_decoded.len()` * Ensure register size is 8 in `read_u64` * Use constant to specify the register ID used in `read_input()`
9437b33
to
77ba897
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I filed #105 to follow up on the performance question since it will not be addressed in this PR
@@ -1,3 +1,4 @@ | |||
#![feature(array_methods)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That could have been a Jetbrains Rust automatic addition.
} | ||
} | ||
Apply::Delete { address } => Engine::remove_account(&address), | ||
Apply::Delete { address } => { | ||
let generation = Self::get_generation(&address); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: if both match arms need generation
then we could call get_generation
before the match
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need the address from somewhere though that needs to be modified is the issue with that.
@@ -542,17 +563,20 @@ impl ApplyBackend for Engine { | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't work since remove_all_storage
is empty right now.
Not shown by github since this code was not touched in the PR (copy-pasting here):
if reset_storage {
Engine::remove_all_storage(&address)
}
Out of curiosity, when is the flat reset_storage
set to true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its a possible flag that can be passed back by the EVM. Technically, we don't wipe it, but I just have it in there for prosperity sake. Maybe there will eventually be a use for it. Lots of notes and discussion on that already. See engine.rs L#287
This should be good to go once tests pass. |
Let's merge #84 against this PR and make sure the test for self-destruct is passing after this test. |
@joshuajbouw Tests still failing |
As far as I was aware this was expected as it was fixed elsewhere? I'll double check if that was merged to the target branch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, Approving to merge into #84
Can someone merge this please? |
Merging into #84 and will fix tests there |
* Add tests for state check after selfdestruct * Aurora runner tracks storage usage to avoid underflow when storage is released in future transactions (#85) * Implement generational storage (#87) * Base precompile code between connectors (#73) * Base precompile code between connectors * Handle errors and validate input * Use proper result * Document `aurora encode-address` usage. * Cache cargo artifacts between CI runs. (#92) * Address comments from audit. (#86) * Validate register length in `read_input_arr20()` * Only read register length in `Engine::get_code_size` * Add `read_input_borsh()` * Ensure `method.args.len() == args_decoded.len()` * Ensure register size is 8 in `read_u64` * Use constant to specify the register ID used in `read_input()` * Reduce size of `cargo cache` in CI. (#95) * Define a `Wei` newtype for balances. (#96) * Fix evm-bully builds after recent refactoring. (#100) * Refactor `Engine::get_state` to return a `Result`. (#99) * Ensure that `Cargo.lock` in the repo is valid. (#101) * Remove unneeded nightly feature. (#102) * Implement BC generational storage. * fix address input * remove note * put key on the end of the storage key * remove pub from methods * Dispatch precompiles on the full address. (#107) * Support state migration on upgrade. (#103) * Implement the ETH connector. (#59) * Move when to call `set_generation` * Fix arg Co-authored-by: Marcelo Fornet <mfornet94@gmail.com> Co-authored-by: Arto Bendiken <arto@aurora.dev> Co-authored-by: Michael Birch <michael@near.org> Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com> Co-authored-by: Evgeny Ukhanov <mrlsd@ya.ru> * Fix layout of the key * Fix all tests (don't wipe the storage all the time) * Use correct generation in writing storage * Remove unnecessary references Co-authored-by: Michael Birch <michael@near.org> Co-authored-by: Joshua J. Bouw <dev@joshuajbouw.com> Co-authored-by: Arto Bendiken <arto@aurora.dev> Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com> Co-authored-by: Evgeny Ukhanov <mrlsd@ya.ru> Co-authored-by: Michael Birch <michael.birch@aurora.dev>
This implements the backwards compatible generational storage, which passes the tests.
How it works is that we have generational addresses, which start at 0, which increment by 1 every time a self destruct action, or more specifically, every time we
Apply::Delete
.This feature is backwards compatible with the current storage structure and shouldn't cause conflicting issues as our keys are either a length of 54 or 58. Prior, our account storage kept a length of 54; however, to give us access to new storage, we must provide a new key. That is where the key length of 58 comes into play as the extra 4 bytes will allow us to tack on the generation value
u32
.NOTE that the tests are -expectedly- failing. They were addressed in #85.