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

Pausable precompiles #588

Merged
merged 2 commits into from
Sep 21, 2022
Merged

Pausable precompiles #588

merged 2 commits into from
Sep 21, 2022

Conversation

RomanHodulak
Copy link
Contributor

@RomanHodulak RomanHodulak commented Aug 19, 2022

Description

Introduces pausable precompiles for stopping the engine when it hits a precompiled function. This is useful for emergency cases.

Performance / NEAR gas cost considerations

Increases gas cost by a few Tgas because of additional storage read operation to load the few bytes of paused precompiles flags.

Testing

Every added module is covered by a set of unit tests covering every conceived test scenario.

Then there is a module in engine-tests containing a set of functional tests that checks several test scenarios on the wasm binary. For example, there is a test case hitting a paused precompile during a smart contract execution that expects the error ERR_PAUSED.

How should this be reviewed

You should consider the administrative engine smart contract methods this feature is going to be controlled by. Take a look at test scenarios to get an idea. Separately from that, there is the mechanism of ensuring calling a paused precompile throws an error, which you should observe.

Additional information

I have some thoughts considering the use-cases/usefulness of the engine smart contract API of this feature:

Engine smart contract method interface

The current interface is like this:

As an admin I want to pause modexp, its associated flag is 0b000000000010000. To pause it I need to run:

pause_precompiles(0b000000000010000)

The cause of the issue that made me pause modexp is resolved, now I run:

resume_precompiles(0b000000000010000)

Seems pretty inconvenient to need to know the right binary flag. Maybe it would be better to accept a list of stringified versions? So the previous example would run like this:

pause_precompiles(["MODEXP"])

resume_precompiles(["MODEXP"])

Also, when you hit invalid string, the error message could include a list of all possible precompile strings.

pause_precompiles(["MODXP"])

Invalid precompile "MODXP", pick one or more from this list: "MODEXP", "BLAKE2F", ...

Reading the current state from the engine smart contract

The paused_precompiles method returns u32, where each 1-bit identifies a paused precompile. Same as above, more human friendly would be to return a list:

paused_precompiles() -> Vec<String>

For example, outputs:

["MODEXP", "BLAKE2F", ...]

@joshuajbouw
Copy link
Contributor

joshuajbouw commented Aug 19, 2022

Make sure CI is working please.

engine/src/pausables.rs Outdated Show resolved Hide resolved
.github/workflows/tests.yml Outdated Show resolved Hide resolved
engine/src/pausables.rs Outdated Show resolved Hide resolved
engine/src/pausables.rs Outdated Show resolved Hide resolved
engine-standalone-storage/src/relayer_db/mod.rs Outdated Show resolved Hide resolved
engine-tests/src/tests/standalone/sanity.rs Outdated Show resolved Hide resolved
engine-types/src/lib.rs Outdated Show resolved Hide resolved
engine/src/engine.rs Outdated Show resolved Hide resolved
engine/src/lib.rs Outdated Show resolved Hide resolved
@RomanHodulak RomanHodulak force-pushed the pausable-precompiles branch 2 times, most recently from 734a5d6 to 6811ec0 Compare August 25, 2022 19:44
@mrLSD
Copy link
Member

mrLSD commented Sep 1, 2022

@RomanHodulak it looks like Gast cost increased?

@joshuajbouw
Copy link
Contributor

joshuajbouw commented Sep 2, 2022

Please fix CI @RomanHodulak.

Just note that gas increases are not ideal. If gas must be increased, its important to note in the PR why it is increasing / decreasing.

@RomanHodulak RomanHodulak force-pushed the pausable-precompiles branch 3 times, most recently from 6b44583 to 5381d5e Compare September 2, 2022 23:27
@mooori
Copy link

mooori commented Sep 3, 2022

As an admin I want to pause modexp, its associated flag is 0b000000000010000. To pause it I need to run:

pause_precompiles(0b000000000010000)

The cause of the issue that made me pause modexp is resolved, now I run:

resume_precompiles(0b000000000010000)

Seems pretty inconvenient to need to know the right binary flag.

The bitflags crate could be helpful here.

@joshuajbouw
Copy link
Contributor

As noted in call, be sure to update https://github.com/aurora-is-near/aurora-cli as well with the new methods.

engine/src/pausables.rs Outdated Show resolved Hide resolved
engine/src/pausables.rs Outdated Show resolved Hide resolved
@RomanHodulak RomanHodulak force-pushed the pausable-precompiles branch 2 times, most recently from 78f11c3 to 6a6a1d7 Compare September 10, 2022 17:56
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.

This looks like a well introduced feature which will be helpful. Great job!

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.

Nice work overall. I think there are some details in the API design that need a second look though.

engine-precompiles/src/lib.rs Outdated Show resolved Hide resolved
engine-standalone-storage/src/sync/mod.rs Outdated Show resolved Hide resolved
engine-tests/src/tests/pausable_precompiles.rs Outdated Show resolved Hide resolved
engine-tests/src/tests/pausable_precompiles.rs Outdated Show resolved Hide resolved
engine/src/engine.rs Show resolved Hide resolved
engine/src/pausables.rs Outdated Show resolved Hide resolved
engine/src/pausables.rs Outdated Show resolved Hide resolved
engine/src/pausables.rs Outdated Show resolved Hide resolved
engine/src/pausables.rs Outdated Show resolved Hide resolved
engine/src/pausables.rs Outdated Show resolved Hide resolved
@RomanHodulak RomanHodulak force-pushed the pausable-precompiles branch 3 times, most recently from c0f2e0c to b08652b Compare September 21, 2022 00:58
@RomanHodulak RomanHodulak force-pushed the pausable-precompiles branch 2 times, most recently from 812a9a3 to 3ab9d2b Compare September 21, 2022 01:38
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.

Looks good! Thanks for addressing my comments.

@joshuajbouw
Copy link
Contributor

Sorry I broke the mergabiity :(, can you fix this @RomanHodulak ?

…eated.

This fixes issue where CI fail when listing the wasm build directory.
…f permissions for such action.

Permissions are hardcoded for a single NEAR account.
@joshuajbouw joshuajbouw merged commit 48119ee into develop Sep 21, 2022
@joshuajbouw joshuajbouw deleted the pausable-precompiles branch September 21, 2022 15:05
birchmd pushed a commit that referenced this pull request Nov 15, 2022
@birchmd birchmd mentioned this pull request Nov 15, 2022
joshuajbouw added a commit that referenced this pull request Nov 25, 2022
* Chore(docs): Additional documentation for xcc gas values (#590)
* Feat(standalone): Conversion from standalone's TransactionKind to NormalizedEthTransaction (#586)
* Add Backstage metadata (#534)
* Add Backstage metadata
* Fix(aurora-engine-transactions): remove hex feaure from std (#600)
* Feat(tests): benchmark transaction Emufid2pv2UpxrZae4NyowF2N2ZHvYEPq16LsQc7Uoc6 (#599)
* Connector-tests: Add tests for the case where an account other than aurora-engine is used for ETH deposit (#598)
* [Docs] Eth Connector - extended documentation (#601)
* feat: add serde JSON to JSON serializable structs in parameters (#605)
* Pausable precompiles (#588)
* Test(engine): Increase unit test coverage (#614)
* Feat: allow xcc calls to perform any possible NEAR call (#610)
* Test(engine): Increase unit test coverage (#618)
* Refactor: Mark functions that create promises on NEAR as unsafe (#617)
* Fix(xcc): Only update the router contract version in storage if the deploy is successful (#616)
* Fix(CI): broken submodules checkout + Clippy warnings (#621)
* Chore(standalone): Upgrade rocksdb to v0.19. (#615)
* fix: remove sscache from CI (#626)
* Tiny refactoring by clippy suggestions (#625)
* Improvements in log macro (#630)
* Fix (engine): update SputnikVM dependency to avoid stack overflow on deeply nested EVM calls (#628)
* Fix(xcc): Ensure the xcc router attaches enough gas to the execute function (#622)
* Release 2.8.0 notes
* Build: Add reproducible build job. (#633)
* fix: bn 256 regression (#637)
* Update release notes
* Update README to exclude version (#623)
* Chore: Update to SputnikVM version v0.37.1-aurora. Includes some overflow-related fixes. (#638)
* fix: modexp underestimated gas
* chore: bump Cargo to 2.8
* chore: remove version from deployments in README.md

Co-authored-by: Alexey Lapitsky <lex@realisticgroup.com>
Co-authored-by: Joshua J. Bouw <joshua@aurora.dev>
Co-authored-by: Kirill <kirill@aurora.dev>
Co-authored-by: Evgeny Ukhanov <evgeny@aurora.dev>
Co-authored-by: Roman Hodulák <roman.hodulak@aurora.dev>
Co-authored-by: Dmitry Strokov <dmitry.strokov@aurora.dev>
Co-authored-by: Oleksandr Anyshchenko <aanischenko@gmail.com>
Co-authored-by: Oleksandr Anyshchenko <oleksandr.anyshchenko@aurora.dev>
This pull request was closed.
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.

5 participants