sui icon indicating copy to clipboard operation
sui copied to clipboard

[move] Option to verify on-chain dependencies on publish

Open amnn opened this issue 2 years ago • 2 comments

Adds the option to verify a Move package's dependencies against their on-chain counterparts on publish.

Test Plan

Newly added tests:

sui-source-validation$ cargo nextest run

Or by running sui client publish --verify-dependencies ....

Supersedes #4975

This PR adds the following changes (see last commit) relative to #4975:

All outstanding comments addressed and some additional things noticed after adding tests for the happy path and every error case:

Fixing Correctness Issues:

  • Make compatible with simtest.
  • Don't take CompiledPackage by value to verify it, take it by reference.
  • Change logic around addresses -- the check for visiting addresses was handled per-module which was not correct -- an address corresponds to a package, and can have multiple modules, and we should visit all of them. The check can be simplified to skipping the verification for the zero address (the current package).
  • Don't key modules solely by their package name and module name when identifying missing modules -- need to include their address (or at least replace package name with address) (this is fixed by changing the structure of the main loop body, see last bullet in "Coding Style/Idioms").

Behavioural changes:

  • Format errors using thiserror.
  • Add flag to PublishCommand to opt-in to verification (temporary while we stabilise it, to allow people including/especially Movey team to try it out).
  • Report LocalDependencyNotFound instead of ModuleCountMismatch, to match OnChainDependencyNotFound.

Improving Performance:

  • Avoid the linear scan through verified dependencies.
  • Avoid converting Symbol to String, use as_ref when a &str is required.
  • Avoid unnecessary clones and muts.
  • Use Map Entry APIs to avoid an extra lookup when testing for containment and adding an empty value.

Coding Style/Idioms:

  • Use let/else or map_err where possible.
  • Favour explicit field names for errors, rather than documenting field names in comments.
  • Get rid of unnecessary dependencies.
  • Implement the main body of verification as two loops, consuming a dictionary of local modules and a dictionary of on-chain modules, which produces an error if there is an unmatched entry in either by the end of the loop.

Stacked on #6246 (first two commits)

amnn avatar Nov 21 '22 23:11 amnn

Thoughts on making dep verification the default? Seems like a good security measure

sblackshear avatar Nov 22 '22 03:11 sblackshear

Yes, we definitely should enable it as the default. My plan was to have a couple of devnet versions where it was available but not always on, and advertise it so people try it.

If it generally works well, (low false positive rate is the main thing I'm looking for, and a zero false negative rate, but that should be a given), we can invert the behaviour of the flag to --skip-dependency-verification.

This should unblock us to let the Movey team (and anyone else who is keen) try the feature out while we build out the docs and supporting tools, which @oxade is going to make a start on, on top of this PR).

amnn avatar Nov 22 '22 07:11 amnn