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

Implement generational storage with SELFDESTRUCT tests #84

Merged
merged 8 commits into from Jun 10, 2021

Conversation

mfornet
Copy link
Contributor

@mfornet mfornet commented May 13, 2021

Check state is removed after selfdestruct is called following this description:

https://gist.github.com/mfornet/a8ffcedfffd22612e34cf0b3c3f5c9e3

@joshuajbouw use this test, to check that reset state is working.


Also add a test (test_self_destruct_with_submit) where function that call selfdestruct is called using engine.submit rather than engine.call and it fails with:

`Result::unwrap()` on an `Err` value: InconsistentStateError(IntegerOverflow)'

This was found by chance. @birchmd @artob can you take a look at this.

@mfornet mfornet requested a review from artob as a code owner May 13, 2021 02:29
@artob artob changed the base branch from master to develop May 13, 2021 08:45
@joshuajbouw
Copy link
Contributor

joshuajbouw commented May 13, 2021

Hm, getting an error that I'm missing a file? Can you double-check that it's pushed?

Can see the issue here: https://github.com/aurora-is-near/aurora-engine/pull/84/checks?check_run_id=2572055068#step:6:1400

    pub fn load() -> Self {
        Self(solidity::ContractConstructor::compile_from_extended_json(
            "etc/eth-contracts/artifacts/contracts/StateTest.sol/SelfDestructFactory.json",
        ))
    }

@birchmd
Copy link
Member

birchmd commented May 13, 2021

Hm, getting an error that I'm missing a file?

Looks like it's supposed to be generated by yarn (see changes to Makefile). We'll just need to update the GitHub action to run that first.

@birchmd
Copy link
Member

birchmd commented May 13, 2021

By the way @mfornet is yarn (or hardhat or whatever) needed to compile this contract for some reason? If this existing solidity::compile function would work (as is used for the standard precompiles test) then I would prefer that to adding an additional tooling dependency.

Even if yarn is needed we could probably use a similar tactic to what I used with the standard precompiles contract to get rust to call yarn itself rather than have it go through the Makefile. This would be a better way than what I suggested about about changing the GitHub action.

@birchmd
Copy link
Member

birchmd commented May 13, 2021

called using engine.submit rather than engine.call and it fails

Noted. I'll look into it.

Update: I know what is throwing the error. It has to do with storage management in the near-vm-runner. You can see the error being created when you evict storage that somehow was not accounted for: logic.rs#L2125. I can make the error not happen by increasing current_storage_usage in the initial Context for AuroraRunner. I think what I should do is update the context after each run call. I'll submit a PR to do that.

@joshuajbouw
Copy link
Contributor

Got it now @birchmd

@birchmd
Copy link
Member

birchmd commented May 13, 2021

@mfornet the error you encountered with submit is fixed in #85

* 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>
@artob artob changed the title Add tests for state check after selfdestruct Implement generational storage with SELFDESTRUCT tests Jun 4, 2021
@artob
Copy link
Contributor

artob commented Jun 4, 2021

@joshuajbouw @mfornet Please resolve the conflicts now after the merge of #87.

src/precompiles/mod.rs Outdated Show resolved Hide resolved
@joshuajbouw
Copy link
Contributor

Just merged develop into this branch carefully as there were at least 20 file conflicts.

Copy link
Contributor

@joshuajbouw joshuajbouw left a comment

Choose a reason for hiding this comment

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

Generally fine, I still have to wait for the CI to go through its run, but my only complaint is that there is obviously quite a bit of feature creep going on here. Do be very careful with that going forward. Would be best to have each of those as separate PRs. Also, do thoroughly document all changes that had been made in the description.

@mfornet
Copy link
Contributor Author

mfornet commented Jun 9, 2021

Found a subtle bug in the way keys were created. Some tests are still failing (see CI), and is a regression introduced in PR #87, already checked on previous commit that it was working fine.

I was looking into it, but still haven't figured out the issue.

Simple test where it fails

  1. A standard ERC20 implementation (form OpenZeppelin) is deployed.
  2. Check balance from a user (it is 0)
  3. Mint 10 tokens for the user
  4. Check balance from a user (it is 10)

At step 3 the transaction fails.

src/engine.rs Outdated Show resolved Hide resolved
Copy link
Member

@birchmd birchmd left a comment

Choose a reason for hiding this comment

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

Great work team!

@mfornet mfornet merged commit 2bf9b25 into develop Jun 10, 2021
@mfornet mfornet deleted the test_self_destruct branch June 10, 2021 13:47
artob added a commit that referenced this pull request Jun 17, 2021
* Introduce precompiles for the ETH & ERC-20 connectors. (#51)
* Implement generational storage with `SELFDESTRUCT` tests. (#84)
* Remove the dependency on Lunarity. (#115)
* Fix Clippy complaint with `+nightly`. (#117)
* Simplify the `sdk::read_u64` return type. (#118)
* Add an `is_used_proof` interface. (#120)
* Add an `evm-bully=yes` build to CI. (#121)
* Handle transaction gas limit properly. (#123)
* Fix u128 JSON parsing & tests in the ETH connector. (#125)
* Fix evm-bully builds. (#130)
* Add JSON custom error types. (#131)
* Don't burn NEP-141 on deposit. (#133)
* Fix needless borrows. (#135)
* Improve and refactor the ETH connector. (#136)
* Add a macro for logging. (#142)

Co-authored-by: Aleksey Kladov <aleksey@near.org>
Co-authored-by: Arto Bendiken <arto@aurora.dev>
Co-authored-by: Evgeny Ukhanov <evgeny@aurora.dev>
Co-authored-by: Frank Braun <frank@aurora.dev>
Co-authored-by: Joshua J. Bouw <joshua@aurora.dev>
Co-authored-by: Kirill <kirill@aurora.dev>
Co-authored-by: Marcelo Fornet <marcelo@aurora.dev>
Co-authored-by: Michael Birch <michael@aurora.dev>
artob added a commit that referenced this pull request Jun 17, 2021
* Introduce precompiles for the ETH & ERC-20 connectors. (#51)
* Implement generational storage with `SELFDESTRUCT` tests. (#84)
* Fix u128 JSON parsing & tests in the ETH connector. (#125)
* Add JSON custom error types. (#131)
* Don't burn NEP-141 on deposit. (#133)
* Fix needless borrows. (#135)
* Improve and refactor the ETH connector. (#136)
* Add a macro for logging. (#142)

Co-authored-by: Aleksey Kladov <aleksey@near.org>
Co-authored-by: Arto Bendiken <arto@aurora.dev>
Co-authored-by: Evgeny Ukhanov <evgeny@aurora.dev>
Co-authored-by: Frank Braun <frank@aurora.dev>
Co-authored-by: Joshua J. Bouw <joshua@aurora.dev>
Co-authored-by: Kirill <kirill@aurora.dev>
Co-authored-by: Marcelo Fornet <marcelo@aurora.dev>
Co-authored-by: Michael Birch <michael@aurora.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants