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

[BUGFIX][CLI] verify-bytecode-meter returns module ticks #16900

Merged
merged 3 commits into from
Mar 28, 2024
Merged

Conversation

amnn
Copy link
Contributor

@amnn amnn commented Mar 27, 2024

Description

We were returning the ticks used for the last function as the verifier ticks, which is not correct. Returning the ticks used for the last function is also not particularly useful, but I will fix that in a follow-up PR.

Test Plan

sui-framework/deepbook$ cargo run --bin sui -- \
  client verify-bytecode-meter
Total number of linter warnings suppressed: 3 (filtered categories: 2)
Running bytecode verifier for 7 modules
╭──────────────────────────────────╮
│ Module will pass metering check! │
├────────┬────────────┬────────────┤
│        │ Module     │ Function   │
│ Max    │ 16000000   │ 16000000   │
│ Used   │ 5869980    │ 7040       │
╰────────┴────────────┴────────────╯

Stack


If your changes are not user-facing and do not break anything, you can skip the following section. Otherwise, please briefly describe what has changed under the Release Notes section.

Type of Change (Check all that apply)

  • protocol change
  • user-visible impact
  • breaking change for a client SDKs
  • breaking change for FNs (FN binary must upgrade)
  • breaking change for validators or node operators (must upgrade binaries)
  • breaking change for on-chain data layout
  • necessitate either a data wipe or data migration

Release notes

Fixes a bug where sui client verify-bytecode-meter incorrectly returned the ticks used by the last function to be verified as the ticks used by the last module verified.

## Description

Adding a couple of features to the `verify-bytecode-meter` command:

- Specify a `--module` (as bytecode) to verify.
- Specify a protocol version to verify at.

Also switches the `--package` path from a positional parameter to a
flag, for symmetry with the new `--module` flag.

## Test Plan:

```
sui-framework/deepbook$ cargo run --bin sui -- \
  client verify-bytecode-meter
sui-framework/deepbook$ cargo run --bin sui -- \
  client verify-bytecode-meter --package .
sui-framework/deepbook$ cargo run --bin sui -- \
  move build
sui-framework/deepbook$ cargo run --bin sui -- \
  client verify-bytecode-meter --module build/DeepBook/bytecode_modules/clob.mv
sui-framework/deepbook$ cargo run --bin sui -- \ # Errors
  client verify-bytecode-meter --package '.'   \
  --module build/DeepBook/bytecode_modules/clob.mv
```

## Release Notes

 - Adds a `--module`/`-m` flag to `sui client bytecode-verify-meter` to
   verify some module bytecode.
 - Replaces the positional, optional package path parameter for this
   command with `--package`/`-p`.
@amnn amnn requested review from dariorussi, stefan-mysten and a team March 27, 2024 10:03
@amnn amnn self-assigned this Mar 27, 2024
Copy link

vercel bot commented Mar 27, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
explorer ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 28, 2024 0:58am
multisig-toolkit ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 28, 2024 0:58am
mysten-ui ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 28, 2024 0:58am
sui-core ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 28, 2024 0:58am
sui-kiosk ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 28, 2024 0:58am
sui-typescript-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 28, 2024 0:58am

We were returning the ticks used for the last function as the verifier
ticks, which is not correct.  Returning the ticks used for the last
function is also not particularly useful, but I will fix that in a
follow-up PR.

Test Plan:

```
sui-framework/deepbook$ cargo run --bin sui -- \
  client verify-bytecode-meter
Total number of linter warnings suppressed: 3 (filtered categories: 2)
Running bytecode verifier for 7 modules
╭──────────────────────────────────╮
│ Module will pass metering check! │
├────────┬────────────┬────────────┤
│        │ Module     │ Function   │
│ Max    │ 16000000   │ 16000000   │
│ Used   │ 5869980    │ 7040       │
╰────────┴────────────┴────────────╯
```
Base automatically changed from amnn/cli-verify-mod to main March 28, 2024 15:02
@amnn amnn merged commit a6cdd43 into main Mar 28, 2024
45 checks passed
@amnn amnn deleted the amnn/verify-fix-mod branch March 28, 2024 15:02
amnn added a commit that referenced this pull request Apr 4, 2024
## Description

Pulling the `Meter` trait definition out of each version of the verifier
into a common place. This is so that this trait can be used as part of
the execution layer interface.

The overall motivation is so that we can supply a `Meter` implementation
that captures more data when run from the CLI, to improve the quality of
output from `sui client verify-bytecode-meter`.

## Test Plan

```
sui$ cargo build --bin sui
sui$ cargo simtest
sui$ env SUI_SKIP_SIMTESTS=1 cargo nextest run
```

## Stack

- #16899 
- #16900
amnn added a commit that referenced this pull request Apr 4, 2024
## Description

Supersede `Verifier::meter_compiled_modules_with_overrides` by:

- Allowing `meter_compiled_modules` and `meter_compiled_bytes` to accept
a custom meter.
- Extracting the generation of `VerifierConfig` out to the
`sui-protocol-config` crate.
- Splitting up `VerifierConfig` into itself and `MeterConfig`.
- Adding a `Verifier::meter` function for creating the meter that is
used during signing.
- Removed the meter as a field of each `Verifier`.

This means that we can now pass a different meter type when we don't
want to enforce metering, and no longer need to modify the config
in-place to achieve the same result.

To make this work, I needed to enable the use of a `&mut dyn Meter`
trait object to instantiate what was previously a `&mut impl Meter`
parameter everywhere. To do this, I created a forwarding trait
implementation of `Meter` for `&mut dyn Meter` and added an extra
relaxation of `+ ?Sized` to all the `&mut impl Meter` parameters to
allow the trait object to instantiate them.

This is in preparation for passing a custom meter into this new
parameter when instantiated by the CLI to capture all information about
scopes during metering.

Today we don't do that which means:

- We only report back information about the last module and function we
verified.
- We can incorrectly report that everything is fine, (package would pass
verification) even if there was some previous module or function that
exceeded the stated limits.

## Test Plan

```
sui$ cargo build --bin sui
sui$ cargo simtest
sui$ env SUI_SKIP_SIMTESTS=1 cargo nextest run
external-crates$ ./tests.sh
deepbook$ cargo run --bin sui -p sui -- client verify-bytecode-meter
```

## Stack

- #16899 
- #16900 
- #16903
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.

2 participants