Whitelist gas accounting #1242

grarco · 2023-03-22T12:24:17Z

Addresses #1010.

This PR implements a first version of gas metering based on the runtime cost of the whitelisted txs and vps:

In storage table of gas costs (taken from the now modified checksums.json file)
Native vps charge gas only for the calls to the host exposed functions
Gas cost for a single signature verification (to account for multisig transactions)
Tx and block GasLimit checks in mempool_validate, prepare_proposal and process_proposal
Runtime tx gas check against the declared GasLimit in finalize_block
Removes MASP fees
Check that all the whitelisted txs/vps have an associated gas cost in checksums.py, and fn load_genesis_config
Unit tests

grarco · 2023-03-22T14:26:21Z

apps/src/lib/client/rpc.rs

@@ -1892,23 +1882,26 @@ pub async fn query_conversions(ctx: Context, args: args::QueryConversions) {
 }

 /// Query a conversion.
-pub async fn query_conversion(
-    client: HttpClient,
+pub async fn query_conversion<CLIENT>(


I needed these changes for benchmarks since I wanted to reuse some client functions of masp (specifically to construct shielded txs) but without a real client nor a running ledger. I suspect the SDK guys are doing something similar to this but I haven't has the time to check

yeah, it's exactly like that

grarco · 2023-03-24T16:01:13Z

pls update wasm

grarco · 2023-03-28T17:30:21Z

pls update wasm

grarco · 2023-03-30T12:10:21Z

pls update wasm

grarco · 2023-03-31T16:01:45Z

pls update wasm

grarco · 2023-03-31T16:38:29Z

apps/src/lib/cli.rs

+    const FEE_AMOUNT: ArgDefault<token::Amount> =
+        arg_default("fee-amount", DefaultFn(|| token::Amount::from(0)));
+    const GAS_LIMIT: ArgDefault<GasLimit> =
+        arg_default("gas-limit", DefaultFn(|| GasLimit::from(10)));


This default for the GasLimit doesn't really have much sense given how different the costs for different txs will be and the lack of a refunding mechanism (at the moment). It's there to ease the tx submission process in testnets but it will probably be better to remove it in future PRs or even in this one

Agreed. Does the client do gas estimation? A reasonable approach for now can be to estimate gas and add a little bit of a buffer on top, then set that as the gas limit.

At the moment I've added the possibility to dry-run the entire tx (wrapper + inner) to estimate the gas cost manually. Were you thinking about something that doesn't require an action from the user? Like the client automatically doing the estimation right before submitting a tx?

Yes, the latter - at least this should be possible as a default, which can be turned off by a flag manually specifying gas.

Date: Wed Feb 8 18:55:43 2023 +0100

Date: Mon Mar 20 15:16:06 2023 +0100

Date: Thu Mar 23 17:05:59 2023 +0100

Date: Fri Mar 24 18:50:17 2023 +0100

Date: Wed Mar 29 18:31:41 2023 +0200

Date: Wed Mar 29 18:49:23 2023 +0200

Date: Fri Apr 14 23:57:16 2023 +0200

Date: Sat May 13 17:59:50 2023 +0200

cwgoes

Mostly looks fine, a few comments.

cwgoes · 2023-06-19T14:27:55Z

apps/src/lib/cli.rs

+    const FEE_AMOUNT: ArgDefault<token::Amount> =
+        arg_default("fee-amount", DefaultFn(|| token::Amount::from(0)));
+    const GAS_LIMIT: ArgDefault<GasLimit> =
+        arg_default("gas-limit", DefaultFn(|| GasLimit::from(10)));


Yes, the latter - at least this should be possible as a default, which can be turned off by a flag manually specifying gas.

cwgoes · 2023-06-19T14:29:55Z

wasm/gas.json

@@ -0,0 +1,20 @@
+{


How are we generating this file, exactly?

At the moment manually. We first run the benchmarks, then we go through the html report produced and compute the gas cost based on the average execution time. The conversion should be something like 1ms~10 gas units. I didn't feel like automating this because we only need to run the benchmarks from time to time

cwgoes · 2023-06-19T14:35:18Z

apps/src/lib/node/ledger/shell/finalize_block.rs

+        let wrapper_fees = self.get_wrapper_tx_fees();
+        match balance.checked_sub(wrapper_fees) {
+            Some(amount) => {
+                self.wl_storage


Do we have a more... abstract version of this function, perhaps?

I feel like we often do logic with raw keys that should be abstracted.

Yes, I've improved this in #1327

cwgoes · 2023-06-19T14:37:05Z

core/src/ledger/gas.rs

+    /// Add gas cost. It will return error when the
+    /// consumed gas exceeds the provided transaction gas limit, but the state
+    /// will still be updated
+    fn add(&mut self, gas: u64) -> Result<()>;


Might be clearer to call this "consume" ?

grarco · 2023-08-10T13:53:40Z

Closed in favor of #1327

grarco force-pushed the grarco/whitelist-gas-accounting branch 2 times, most recently from 458c28b to 7ae6159 Compare March 22, 2023 13:35

grarco mentioned this pull request Mar 22, 2023

Gas & Fee #1010

Closed

4 tasks

grarco force-pushed the grarco/whitelist-gas-accounting branch from 7ae6159 to 66ab4d6 Compare March 22, 2023 14:06

grarco commented Mar 22, 2023

View reviewed changes

grarco force-pushed the grarco/whitelist-gas-accounting branch 6 times, most recently from 7103311 to c9ca5ad Compare March 24, 2023 15:33

grarco force-pushed the grarco/whitelist-gas-accounting branch 6 times, most recently from 2466237 to a0b5d93 Compare March 28, 2023 16:50

grarco force-pushed the grarco/whitelist-gas-accounting branch 2 times, most recently from dff51dd to d1843d2 Compare March 29, 2023 14:48

grarco force-pushed the grarco/whitelist-gas-accounting branch 3 times, most recently from 37b8279 to 823e5a7 Compare March 31, 2023 14:05

grarco added a commit that referenced this pull request Mar 31, 2023

changelog: add #1242

f5ee56d

grarco force-pushed the grarco/whitelist-gas-accounting branch from 09322e8 to f5ee56d Compare March 31, 2023 16:35

grarco commented Mar 31, 2023

View reviewed changes

grarco added a commit that referenced this pull request Mar 31, 2023

changelog: add #1242

d6e165e

grarco force-pushed the grarco/whitelist-gas-accounting branch from 8cf8135 to 9768a85 Compare May 20, 2023 17:34

grarco added a commit that referenced this pull request May 23, 2023

changelog: add #1242

fce81b4

grarco force-pushed the grarco/whitelist-gas-accounting branch from 9768a85 to fce81b4 Compare May 23, 2023 15:40

grarco and others added 23 commits June 5, 2023 10:15

Implements block gas

685a815

Date: Wed Feb 8 18:55:43 2023 +0100

Adds benchmarks

682d81b

Improves gas for native vps

30f9590

Date: Mon Mar 20 15:16:06 2023 +0100

Removes MASP shielded fees

0241b6f

Fixes gas for unit tests

0b906ef

Date: Thu Mar 23 17:05:59 2023 +0100

Corrects gas in e2e tests

964d224

Date: Fri Mar 24 18:50:17 2023 +0100

Updates cargo lock for benchmarks

39493a5

Date: Wed Mar 29 18:31:41 2023 +0200

Misc fixes to gas and checksum file

6b44bb2

Date: Wed Mar 29 18:49:23 2023 +0200

Misc fixes to docs and e2e tests

2384328

Date: Fri Apr 14 23:57:16 2023 +0200

Fixes benchmarks

44cf55f

Date: Sat May 13 17:59:50 2023 +0200

Removes mock gas meter in tx execution

b7bf157

Optimizes gas accounting for tx hash

36d81b3

Optimizes vp gas metering

3da5e80

Renames vp_eval parameters

5b8e41d

Fixes unit tests and wrong error handling

ec08858

Short circuit vp evaluation on error

c2570bf

Refactors gas meters api into trait

2165084

Moves compilation gas into fetch_or_compile

e1ac8d1

Fixes e2e tests

980ab09

Adjusts conditional compilation for fee payment

f02c6e7

Clippy + fmt

7b89045

[ci] wasm checksums update

6478fc3

changelog: add #1242

6349f96

grarco force-pushed the grarco/whitelist-gas-accounting branch from fce81b4 to 6349f96 Compare June 5, 2023 01:31

cwgoes reviewed Jun 19, 2023

View reviewed changes

grarco closed this Aug 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Whitelist gas accounting #1242

Whitelist gas accounting #1242

grarco commented Mar 22, 2023 •

edited

Loading

grarco Mar 22, 2023

tzemanovic Apr 28, 2023

grarco commented Mar 24, 2023

grarco commented Mar 28, 2023

grarco commented Mar 30, 2023

grarco commented Mar 31, 2023

grarco Mar 31, 2023

cwgoes Jun 16, 2023

grarco Jun 19, 2023

cwgoes Jun 19, 2023

cwgoes left a comment

cwgoes Jun 19, 2023

cwgoes Jun 19, 2023

grarco Jun 28, 2023

cwgoes Jun 19, 2023

grarco Jun 28, 2023

cwgoes Jun 19, 2023

grarco commented Aug 10, 2023

Whitelist gas accounting #1242

Whitelist gas accounting #1242

Conversation

grarco commented Mar 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grarco commented Mar 24, 2023

grarco commented Mar 28, 2023

grarco commented Mar 30, 2023

grarco commented Mar 31, 2023

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

cwgoes 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grarco commented Aug 10, 2023

grarco commented Mar 22, 2023 •

edited

Loading