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.

Sign up for GitHub

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

Jump to bottom

Add GMP precompile killswitch #2292

Merged
merged 12 commits into from May 22, 2023
Merged

Add GMP precompile killswitch #2292

merged 12 commits into from May 22, 2023

Conversation

notlesh
Copy link
Contributor

@notlesh notlesh commented May 9, 2023 •
edited

What does it do?

Adds an explicit killswitch to the GMP precompile. This defaults to enabled, so the precompile will not work without it being disabled first.

TODO:

  • Add call to wormhole_transfer_erc20 in tests to make sure it properly fails
  • Clean up execute_reverts_no_decode() mess
  • Add TS test which calls set_storage to show that this technique works to manage the killswitch

⚠️ Breaking Changes ⚠️

  • This adds a killswitch which defaults to disabled, so the GMP precompile will immediately stop working once this is deployed until a set_storage call is made to enable it.

@notlesh notlesh added B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D5-nicetohaveaudit⚠️ PR contains trivial changes to logic that should be properly reviewed. breaking Needs to be mentioned in breaking changes labels May 9, 2023
notlesh
notlesh commented May 11, 2023
View reviewed changes
tests/tests/test-precompile/test-precompile-wormhole.ts Outdated
context.polkadotApi.tx.system.setStorage([
[
ENABLED_FLAG_STORAGE_ADDRESS,
context.polkadotApi.registry.createType("Option<bool>", true).toHex()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please review 🙏

Copy link
Contributor Author

@notlesh notlesh May 11, 2023 •
edited

Choose a reason for hiding this comment

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

Based on https://docs.substrate.io/reference/scale-codec/, I think 0x0101 should be the same thing (Some(true))

@notlesh notlesh added the A0-pleasereview Pull request needs code review. label May 11, 2023
@github-actions
Copy link
Contributor

github-actions bot commented May 11, 2023 •
edited

Coverage generated "Fri May 19 21:40:40 UTC 2023":
https://s3.amazonaws.com/moonbeam-coverage/pulls/2292/html/index.html

Master coverage: 72.42%
Pull coverage: 72.36%

@notlesh notlesh added D1-audited👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited and removed D5-nicetohaveaudit⚠️ PR contains trivial changes to logic that should be properly reviewed. labels May 17, 2023
@timbrinded
Copy link
Contributor

Sorry it's taken me so long to take a look at this.
@notlesh Are we able to have a negative TS test case in here for when killswitch is enabled? Can be a copypaste of the positive steps but just without your freshly added sudo ext call. It'd be good to inscribe into a fixture how we expect the negative scenario to play out (even though the negative case is already covered by the rust test).

@notlesh
Copy link
Contributor Author

notlesh commented May 18, 2023

Are we able to have a negative TS test case in here for when killswitch is enabled?

I added such a test in c9635d0, but it really needs to check the output because there are plenty of other reasons it could revert. Do we have anything like execute_reverts() in our TS tests?

@timbrinded
Copy link
Contributor

if you have the txn hash you can just call getTransactionReceipt() and status will be false (reverted). In terms of extracting the revert reason you can use: extractRevertReason()
like this

@notlesh
Copy link
Contributor Author

notlesh commented May 18, 2023

if you have the txn hash you can just call getTransactionReceipt() and status will be false (reverted). In terms of extracting the revert reason you can use: extractRevertReason() like this

Thanks, fixed in ea08987

timbrinded
timbrinded reviewed May 19, 2023
View reviewed changes
tests/tests/test-precompile/test-precompile-wormhole.ts Outdated
@@ -209,6 +210,21 @@ describeDevMoonbeam(`Test local Wormhole`, (context) => {
.signAndSend(alith);
await context.createBlock();

// we also need to disable the killswitch by setting the 'enabled' flag to Some(true)
const ENABLED_FLAG_STORAGE_ADDRESS =
"0xb7f047395bba5df0367b45771c00de502551bba17abb82ef3498bab688e470b8";
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry to be tedious but can you derive this instead, so that it's clear what this is?

This is to help future generations having to work out where that key is from (and it's a pain to work backwards from a hash).

  const enabledFlagKey = u8aToHex(
            u8aConcat(xxhashAsU8a("gmp", 128), xxhashAsU8a("PrecompileEnabled", 128))
          );

ℹ️ both functions are available in the @polkadot/util repo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good suggestion. Added in 751af83. I also left the keys themselves for future generations 😁

@notlesh notlesh requested a review from timbrinded May 22, 2023 15:18
timbrinded
timbrinded approved these changes May 22, 2023
View reviewed changes
Copy link
Contributor

@timbrinded timbrinded left a comment

Choose a reason for hiding this comment

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

Looks great 👍

@notlesh notlesh merged commit 441c4f7 into master May 22, 2023
26 checks passed
@notlesh notlesh deleted the notlesh-add-gmp-killswitch branch May 22, 2023 17:15
timbrinded pushed a commit that referenced this pull request Jun 2, 2023
@notlesh @timbrinded
Add GMP precompile killswitch (#2292)
11c3654 
* Add GMP precompile killswitch

* Test revert values

* Don't decode revert message

* Properly encode revert string

* Test against encoded output

* Disable killswitch in wormhole test

* prettier

* Add basic TS test covering killswitch default

* Expect revert reason to match killswitch

* Don't share test suite with killswitch test

* Derive magic storage keys
@notlesh notlesh mentioned this pull request Jun 5, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timbrinded timbrinded timbrinded approved these changes

@crystalin crystalin Awaiting requested review from crystalin

Assignees
No one assigned
Labels
A0-pleasereview Pull request needs code review. B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes breaking Needs to be mentioned in breaking changes D1-audited👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@notlesh @timbrinded