Add gas logic for move call and module publish #119

lxfind · 2022-01-04T17:09:52Z

This PR adds gas deduction logic for move call orders and module publish orders. I will add gas logic for transfer orders separately. The gas cost model is arbitrary at the moment. We will need to come up with a proper gas model latter.
Here is how it works:

When an order is received (i.e. in handle_order), we check that the gas object exists and meets the minimum requirements: has enough balance to cover budget in the case of move call, and meets minimum balance requirement.
When a confirmation order is received, we pass the gas object over to the Move adapter for execution. In the case of Move call, Move VM will calculate how much gas was used after successful execution. Note that there should be an exchange rate between Move gas and FastX gas, as they may not be 1:1. In the case of module publish, we use the size in bytes of the modules to compute the gas cost. In both cases, we update the gas object's balance and write into it back to the temporary store.
All gas related logic is added to gas.rs.
Also added a succeeded field to the TemporaryStore, because we needed to do invariant checks that all mutable inputs must be written. This is only true when the transaction is successful.
Added tests in both adapter and authority tests to cover all cases.

sblackshear · 2022-01-04T18:49:14Z

Also added a succeeded field to the TemporaryStore, because we needed to do invariant checks that all mutable inputs must be written. This is only true when the transaction is successful.

Good find! FWIW I think the "all mutable inputs must be written" is too heavy-handed even in the success case. For example, one can write legitimate Move code like the following that violates the rule:

public fun main(o: &mut Obj, flag: bool) {
  if (flag) {
    o.f = 16
  }
}

sblackshear · 2022-01-04T18:57:40Z

Note that there should be an exchange rate between Move gas and FastX gas, as they may not be 1:1

I might misunderstand something, but I think we will not need this. The Move VM lets you use a custom "gas schedule" that specifies the cost for each instruction. If we set these costs to whatever is appropriate for FastX, I think that should be all we need?

lxfind · 2022-01-04T19:08:38Z

I might misunderstand something, but I think we will not need this. The Move VM lets you use a custom "gas schedule" that specifies the cost for each instruction. If we set these costs to whatever is appropriate for FastX, I think that should be all we need?

Do we want to do that though? It seems convenient to just reuse Move's metering, given that we use a strictly subset of their bytecode, with identical semantics.

huitseeker

For ensuring the code paths we write all make sense over time, would it be safer to make all calls with a super high gas budget MAX_GAS_BUDGETS, instead of passing None where a gas budget should be?

fastpay_core/src/authority/temporary_store.rs

fastpay_core/src/unit_tests/authority_tests.rs

fastx_types/src/gas.rs

lxfind · 2022-01-04T20:32:58Z

All comments addressed:

Removed the heavy invariant check in temporary store
Added ok_or_gas_error! in gas.rs
Defined MAX_GAS in test files

fastpay_core/src/authority/temporary_store.rs

sblackshear · 2022-01-04T19:00:57Z

fastx_types/src/gas.rs

+            Ok(())
+        }
+        OrderKind::Publish(publish) => {
+            assert_eq!(publish.gas_payment.0, gas_object.id());


We should probably use debug_assert's here.

Curious when should we use debug_assert vs assert? From C++ world we use debug assert for things that are expensive to assert. In this case it seems pretty cheap.

Good question for @huitseeker. I think what we've been doing elsewhere in fastnft is using debug_assert for any internal invariant violation (for both efficiency and liveness reasons). But of course whether or not to leave asserts (even cheap ones) on in prod is a deeply philosophical issue...

I'm probably not telling you anything new, but debug_assert statements are elided in non-debug builds, so their cost is precisely zero.

As to assert statements, being macros, they're programmatically easy to track, intercept and instrument after the fact, so that keeping their performance cost in check does not seem to be a big issue.

What I am worried by, however, is that they create panic code paths, which in production environments where a lot of inputs can be controlled by the adversary, and where cascading node failures lead to total system failure by definition, often prompt developers to reach for expletives.

So for each of those asserts I'd like to ask: is there any way at all that this panic could be recovered from?

I have a longer spiel on error management, I'll be happy to devote a doc / crypto meetup to it.

I agree that no input should be able to trigger an assertion (errors should be used in that case).
And no (proper use of) assertions should be recoverable.
Assertions should be used only in cases that indicate our implementation has a bug that would lead to inconsistent data down the road if continuing. Our production code will have bugs. So ideally I would love to be able to see the stack trace of the assertions in prod and be aware of it when it actually get hit, instead of debugging inconsistent data store in the system if we let the code to continue.
So my preference is:

For anything that could happen given certain input, we use errors

For anything that our implementation guarantees won't happen (e.g. a few lines back something is already checked, and latter on we assert on it), if it's really cheap (like a single equality check) we use assert, otherwise we use debug_assert.

Curious what you think.

fastx_types/src/gas.rs

sblackshear · 2022-01-04T21:33:53Z

Do we want to do that though? It seems convenient to just reuse Move's metering, given that we use a strictly subset of their bytecode, with identical semantics.

Ah, I think we're on the same page? I am indeed suggesting that we reuse the metering (which is cost-agnostic), but provide custom instruction costs (which the VM supports via taking a parameter of type CostTable.

Totally ok with using the default CostTable for now--just pointing out that

we can (and will probably want to) change this eventually
doing so is (I think) an good alternative to having separate notions of Move gas and FastX gas + converting between them

lxfind · 2022-01-04T21:50:54Z

Ah, I think we're on the same page? I am indeed suggesting that we reuse the metering (which is cost-agnostic), but provide custom instruction costs (which the VM supports via taking a parameter of type CostTable.

Cool! Looks like we only need to customize the GasConstants.

lxfind · 2022-01-04T22:36:48Z

Removed the conversion between move gas and fastx gas.
Added TryFrom<&Object> on GasCoin.

gdanezis · 2022-01-05T15:33:52Z

Good find! FWIW I think the "all mutable inputs must be written" is too heavy-handed even in the success case. For example, one can write legitimate Move code like the following that violates the rule:

Hey, all mutable objects need to at least have their sequence number incremented and TransactionDigest changed. So from the point of view of the authority they are all mutated.

gdanezis

Nice one -- a few comments on TODOs for follow-up issues.

gdanezis · 2022-01-05T15:44:57Z

fastpay_core/src/authority.rs

@@ -112,6 +114,11 @@ impl AuthorityState {
                FastPayError::IncorrectSigner
            );

+            if idx + 1 == input_object_cnt {


May be it would be slightly more clear if we add a get_gas_object_ref to the order, and then we compare this ref to the object_ref supplied. We will simply forget that the last object is special.

The orders are actually already structured this way, so I think all we should need to do is refactor the code above for resolving input ObjectRef's to objects a bit.

The orders are actually already structured this way, so I think all we should need to do is refactor the code above for resolving input ObjectRef's to objects a bit.

Not sure what you mean, but the Transfer order currently don't have a gas object yet. (in fact, this "last object" check works for transfer order today only because all of our objects are gas objects, so it's incomplete hack.) So everything will work properly only after I add gas support for Transfer order.

fastx_types/src/gas.rs

gdanezis · 2022-01-05T15:54:26Z

fastx_types/src/gas.rs

+}
+
+pub fn calculate_module_publish_gas(module_bytes: &[Vec<u8>]) -> u64 {
+    // TODO: Figure out module publish gas formula.


Should we parametrize this by the global table of Gas cost, with something like Gas_per_byte * bytes_stored. We probably also need to charge for gas for non module objects stored for Calls, and maybe reclaim some gas when objects are deleted?

lxfind · 2022-01-05T18:09:42Z

Addressed the following comments (pushed as a second commit on top of the first one):

Added back the debug_assert in temporary_store regarding mutable references must be written or deleted.
Added gas logic for both object creation and object deletion (refund in this case).
Added TODOs regrading keeping track the gas so that we could distribute them latter.
Changed the gas budget parameter to execute() always a number instead of Option. If we want a high budget, just give a high budget.
Added gas deduction in the case of Move execution failure.

huitseeker

Just nits! lgtm

huitseeker · 2022-01-05T20:03:39Z

fastx_types/src/object.rs

+    pub fn as_move(&self) -> Option<&MoveObject> {
+        use Data::*;
+        match self {
+            Move(m) => Some(m),
+            Module(_) => None,
+        }
+    }
+
+    pub fn as_move_mut(&mut self) -> Option<&mut MoveObject> {
+        use Data::*;
+        match self {
+            Move(m) => Some(m),
+            Module(_) => None,
+        }
+    }
+


Nit: This may confuse folks used to AsRef<T> and AsMut<T> implementations. Inserting a try_ somehwere in the naming pattern might help.

huitseeker · 2022-01-05T20:05:26Z

fastx_types/src/gas.rs

+macro_rules! ok_or_gas_error {
+    ($cond:expr, $e:expr) => {
+        if !($cond) {
+            Err(FastPayError::InsufficientGas { error: $e })
+        } else {
+            Ok(())
+        }
+    };
+}


Nit: I would have left the boolean-to-Result logic, which people are used to, and just macro-ed the error, but YMMV.

huitseeker · 2022-01-05T20:07:14Z

fastpay_core/src/unit_tests/authority_tests.rs

@@ -194,13 +199,23 @@ fn make_dependent_module(m: &CompiledModule) -> CompiledModule {
    dependent_module
 }

+fn check_gas_object(


[cfg(test)] would be good to make sure we don't trigger panics through an errant call to this.

This commit introduces a new command to fetch multiple blocks at once.

lxfind requested review from huitseeker, patrickkuo, sblackshear, gdanezis and oxade January 4, 2022 17:09

lxfind mentioned this pull request Jan 4, 2022

[fastx adapter] add gas fee logic #45

Closed

huitseeker reviewed Jan 4, 2022

View reviewed changes

fastpay_core/src/authority/temporary_store.rs Outdated Show resolved Hide resolved

sblackshear reviewed Jan 4, 2022

View reviewed changes

Add gas logic for move call and module publish

a12c60c

gdanezis approved these changes Jan 5, 2022

View reviewed changes

huitseeker approved these changes Jan 5, 2022

View reviewed changes

Address comments

1367045

lxfind merged commit 0f7cde3 into MystenLabs:main Jan 5, 2022

lxfind mentioned this pull request Jan 6, 2022

Add gas logic for transfer orders #126

Merged

mwtian pushed a commit that referenced this pull request Sep 12, 2022

[get_collections] introduce GetBlocks command (#119)

f46254a

This commit introduces a new command to fetch multiple blocks at once.

mwtian pushed a commit to mwtian/sui that referenced this pull request Sep 29, 2022

[get_collections] introduce GetBlocks command (MystenLabs#119)

6e7ab08

This commit introduces a new command to fetch multiple blocks at once.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add gas logic for move call and module publish #119

Add gas logic for move call and module publish #119

lxfind commented Jan 4, 2022

sblackshear commented Jan 4, 2022

sblackshear commented Jan 4, 2022

lxfind commented Jan 4, 2022

huitseeker left a comment

lxfind commented Jan 4, 2022

sblackshear Jan 4, 2022

lxfind Jan 4, 2022

sblackshear Jan 5, 2022

huitseeker Jan 5, 2022

lxfind Jan 5, 2022

sblackshear commented Jan 4, 2022

lxfind commented Jan 4, 2022

lxfind commented Jan 4, 2022

gdanezis commented Jan 5, 2022

gdanezis left a comment

gdanezis Jan 5, 2022

sblackshear Jan 5, 2022

lxfind Jan 5, 2022

gdanezis Jan 5, 2022

lxfind commented Jan 5, 2022

huitseeker left a comment

huitseeker Jan 5, 2022

huitseeker Jan 5, 2022

huitseeker Jan 5, 2022

Add gas logic for move call and module publish #119

Add gas logic for move call and module publish #119

Conversation

lxfind commented Jan 4, 2022

sblackshear commented Jan 4, 2022

sblackshear commented Jan 4, 2022

lxfind commented Jan 4, 2022

huitseeker left a comment

Choose a reason for hiding this comment

lxfind commented Jan 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sblackshear commented Jan 4, 2022

lxfind commented Jan 4, 2022

lxfind commented Jan 4, 2022

gdanezis commented Jan 5, 2022

gdanezis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lxfind commented Jan 5, 2022

huitseeker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment