-
Notifications
You must be signed in to change notification settings - Fork 11.2k
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
[genesis] add logic for extracting genesis modules for FastX framework #72
Conversation
@@ -17,3 +19,13 @@ pub mod messages; | |||
pub mod object; | |||
pub mod serialize; | |||
pub mod storage; | |||
|
|||
/// 0x1-- account address where Move stdlib modules are stored | |||
pub const MOVE_STDLIB_ADDRESS: AccountAddress = AccountAddress::new([ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had to move these to avoid cyclic deps between the verifier and framework crates.
|
||
fn get_framework_modules_(include_examples: bool, verify: bool) -> Result<Vec<CompiledModule>> { | ||
// TODO: prune unused deps from Move stdlib instead of using an explicit denylist. | ||
// The manually curated list are modules that do not pass the FastX verifier |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lxfind: the need for this denylist is a good indicator that your bytecode verifier passes are working :)--all these modules use global storage operators and (obviously, since ID
doesn't exist in the Move stdlib) the key
attribute without an ID
field.
@@ -249,24 +249,23 @@ impl Authority for AuthorityState { | |||
.map_err(|_| FastPayError::MoveExecutionFailure)?; | |||
} | |||
OrderKind::Publish(m) => { | |||
// Fake the gas payment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to charge for gas here--otherwise, a failed publish attempt is free.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to reflect on what failed means: there is a transaction that just fails because it is invalid, versus one that fails but costs are charged. Right I feel we do not make enough of a difference between these cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, agreed that we need consistent definitions/terminology here. Some attempts, eager to hear your thoughts:
- An invalid order will not be signed by any honest authority
- An invalid certificate will not be processed by any honest authority (i.e., will have no effect on the persistent state of that authority)
- A valid order will be signed by any honest authority, and thus will eventually be signed by enough honest authorities to form a certificate (under the usual assumptions)
- A valid, but failed certificate will be processed by any honest authority and change its persistent state only to (1) charge for gas, and update the certificate store. Non-gas inputs will still be spendable by future orders
- A valid, but successful certificate will be processed by any honest authority and change its persistent state to charge for gas, update the certificate store, and reflect the effects of executing the order "payload" (i.e., a module publish, move call, or transfer). All inputs are consumed and cannot be spent by future orders.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some reasons (full list in overleaf spec) for invalid orders include:
- Inputs not in
LockMap
- Gas fee input is insufficient to pay for the cost of processing the transaction
- Invalid signature from sender
Some reasons for invalid certificates include:
- Invalid signatures from authorities
- The order embedded in the certificate is invalid (should only happen due to dishonest authorities or bugs)
Some reason for failed certificate processing include (this is the biggest category):
- Function pointer in a call transaction does not correspond to a function published on-chain
- Input object types do not match the type signature of the function invoked
- Executing the function exceeded its gas budget
- Executing the function encountered a Move
abort
@@ -249,24 +249,23 @@ impl Authority for AuthorityState { | |||
.map_err(|_| FastPayError::MoveExecutionFailure)?; | |||
} | |||
OrderKind::Publish(m) => { | |||
// Fake the gas payment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to reflect on what failed means: there is a transaction that just fails because it is invalid, versus one that fails but costs are charged. Right I feel we do not make enough of a difference between these cases.
- Add code in `framework/lib.rs` to build and verify the FastX framework Move modules + their dependencies in the Move stdlib and hand back a list of `CompiledModule`s. Add test to ensure that this code always works on the current set of framework modules. - Add genesis.rs, which calls this code and wraps each module in an `Object` with a freshly generated ID - Add an authority test that creates a genesis state with a module in it, then successfully processes a new `publish` order containing a module that depends on genesis one. - Fix bugs in `publish` flow revealed by attempting to write this test. A couple of issues came up during this that will need to be fixed later. One is MystenLabs/sui#69; it basically stops us from calling the Move linker.
framework/lib.rs
to build and verify the FastX framework Move modules + their dependencies in the Move stdlib and hand back a list ofCompiledModule
s. Add test to ensure that this code always works on the current set of framework modules.Object
with a freshly generated IDM
in it, then successfully processes a newpublish
order containing a module that depends onM
.publish
flow revealed by attempting to write this test.A couple of major issues came up during this that will need to be fixed later. The easier, but less high-pri one is: #69; it basically stops us from calling the Move linke. The naster, and much more high-pri one (it stops us from creating a genesis with modules that depend on each other) is #71.