[move-vm] allow multi-module publication #8555
Conversation
meng-xu-cs
force-pushed
the
multi-pub
branch
7 times, most recently
from
fc7144d
to
2854b88
Compare
meng-xu-cs
requested review from
sblackshear,
tnowacki,
shazqadeer,
vgao1996,
bob-wilson and
runtian-zhou
|
|
||
| data_store.publish_module(&module_id, module) | ||
| // NOTE: we want to (informally) argue that all modules pass the linking check before being |
There was a problem hiding this comment.
I haven't had time to digest this in detail yet, but there are two tests that would greatly increase my confidence:
- publish a module
Athat containsfriend Bin a vec that does not also containB - publish list
[A, B]whereBcreates cyclic dependencies, but only ifAis published first
There was a problem hiding this comment.
Yes, test case coming up
There was a problem hiding this comment.
Tests added in Move CLI testsuite, under dir multi_module_publish.
We cannot use functional-tests or e2e-tests yet because we don't have support for multi-module publishing at the TransactionPayload level (TransactionPayload::Module(<single-module-here>)). So Move CLI becomes really helpful in this case.
meng-xu-cs
force-pushed
the
multi-pub
branch
5 times, most recently
from
8a5f4d4
to
472ba86
Compare
| ); | ||
| if let Err(err) = res { | ||
| // TODO (mengxu): explain publish errors in multi-module publishing | ||
| println!("Invalid multi-module publishing: {}", err); |
There was a problem hiding this comment.
This requires some non-trivial changes to the explain_publish_error function. In order not to mess-up with this PR, I prefer to have a separate PR to address this TODO item.
language/tools/move-cli/src/main.rs
Outdated
| @@ -96,6 +96,9 @@ pub enum SandboxCommand { | |||
| /// Set this flag to ignore breaking changes checks and publish anyway | |||
| #[structopt(long = "ignore-breaking-changes")] | |||
| ignore_breaking_changes: bool, | |||
| /// fix publishign order | |||
There was a problem hiding this comment.
compile with no additional arguments compiles everything in the current directory, whereas specifying a list of files will compile only those files. I think it might make sense to file that same convention here, especially since sandbox publish with no other arguments already publishes everything. We could do this by using modules: Vec<String> as an anonymous argument instead of adding override-ordering.
There was a problem hiding this comment.
Yup, that would help us eliminate an additional argument. The only issue is that we could have a single Move file containing multiple modules, if we specify at the file granularity, we lose two capabilities
- the capability of publishing a subset of the modules in the same file,
- the capability of publishing the modules at a different ordering.
These issue can be addressed with the override-ordering flag, but at the cost of one extra flag in the command line
meng-xu-cs
force-pushed
the
multi-pub
branch
2 times, most recently
from
b9a8dab
to
254f48f
Compare
|
Sorry that it took me a while to address the comments. The PR is brought up to date again so feel free to take another look @sblackshear |
|
/land |
The first commit updates the Move VM API
This commit allows the Move VM to take a vector of modules (for lacking
of a better name, this vector of modules is called a bundle for now) and
publish them together.
Modules in the bundle vector must follow a topological order, i.e., if
there is a dependency relation (usage or friend) between two modules
A and B, via either 1) B uses A or 2) A declares B as a friend. Then,
A must appear before B in the bundle vector in order to be published
together. Putting B in front of A in this case will lead to a
MISSING_DEPENDENCY error or other linker errors.
All modules in the bundle will have to pass the bytecode verifier and
will be linker-checked with existing modules and all other modules in
the same bundle. Either all modules in the bundle are published/updated
or none is published/updated. Publication of the whole bundle fails as
long as one module in the bundle fail the bytecode verifier or the
linker checks (i.e., dependency and cyclic dependency check).
There is also a very informal argument in the code comments on why
the current implementation might seem plausible. Please check that
as well. I am hoping to come up with a much stronger argument (or
even paper proof) why this is correct, but I haven't reached there yet.
The second commit adds support for multi-module publishing the Move CLI
The
move-cli sandbox publishcommand now accepts a new optionalcommand line argument -m/--override-ordering that allows manual
specification of which compiled modules to be published and in what
order.
Specifying any of the
-m/--override-orderingflag will invoke themulti-module publishing flow in the Move VM (compared with the
traditional one-module-at-a-time flow).
For example:
sandbox publish -m A -m Bwill prepare a module vector[A, B] to be sent for publication while
-m B -m Awill create a vector[B, A]. And depending on the relationship between module A and B, it is
possible that only one particular ordering can be allowed. For more
detailed use cases, see the test case
multi_module_publish.It is possible to request publication of a subset of the compiled modules. For
example, if three modules A, B, C are compiled and only
-m A -m Bisspecified, then only A and B will be sent to Move VM for publication.
It is also possible to specify one module multiple times, e.g.,
-m A -m A. The Move CLI will not complain about it but the Move VMwill as it checks for no duplication in the vector of modules.
It is, however, not possible to specify a non-compiled module (e.g.,
module D which is not compiled) for publication. An error will be raised
in this case.
Motivation
Partially address #8512
Have you read the Contributing Guidelines on pull requests?
Yes
Test Plan
CI for existing code
new test cases being added to Move CLI, under
multi_module_publishdir.