Skip to content
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

Whitelist gas accounting #1242

Closed
wants to merge 23 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .changelog/unreleased/1242-whitelist-gas-accounting.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
- Implemented a first gas metering system based on the
runtime cost of whitelisted transactions and VPs.
([#1242](https://github.com/anoma/namada/pull/1242))
181 changes: 178 additions & 3 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ resolver = "2"

members = [
"apps",
"benches",
"core",
"proof_of_stake",
"shared",
Expand Down
11 changes: 8 additions & 3 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ wasm_templates := wasm/tx_template wasm/vp_template
audit-ignores += RUSTSEC-2021-0076

build:
$(cargo) build
$(cargo) build --workspace --exclude namada_benchmarks

build-test:
$(cargo) +$(nightly) build --tests -Z unstable-options
Expand All @@ -36,7 +36,8 @@ package: build-release

check-wasm = $(cargo) check --target wasm32-unknown-unknown --manifest-path $(wasm)/Cargo.toml
check:
$(cargo) check && \
$(cargo) check --workspace --exclude namada_benchmarks && \
$(cargo) +$(nightly) check --benches -Z unstable-options && \
make -C $(wasms) check && \
make -C $(wasms_for_tests) check && \
$(foreach wasm,$(wasm_templates),$(check-wasm) && ) true
Expand All @@ -45,6 +46,7 @@ check-abcipp:
$(cargo) +$(nightly) check \
--workspace \
--exclude namada_tests \
--exclude namada_benchmarks \
--all-targets \
--no-default-features \
--features "abcipp ibc-mocks-abcipp testing" \
Expand Down Expand Up @@ -213,6 +215,9 @@ watch:
clean:
$(cargo) clean

bench:
$(cargo) +$(nightly) bench -Z unstable-options

build-doc:
$(cargo) doc --no-deps

Expand Down Expand Up @@ -266,4 +271,4 @@ test-miri:
MIRIFLAGS="-Zmiri-disable-isolation" $(cargo) +$(nightly) miri test


.PHONY : build check build-release clippy install run-ledger run-gossip reset-ledger test test-debug fmt watch clean build-doc doc build-wasm-scripts-docker debug-wasm-scripts-docker build-wasm-scripts debug-wasm-scripts clean-wasm-scripts dev-deps test-miri test-unit test-unit-abcipp clippy-abcipp
.PHONY : build check build-release clippy install run-ledger run-gossip reset-ledger test test-debug fmt watch clean build-doc doc build-wasm-scripts-docker debug-wasm-scripts-docker build-wasm-scripts debug-wasm-scripts clean-wasm-scripts dev-deps test-miri test-unit test-unit-abcipp clippy-abcipp bench
36 changes: 18 additions & 18 deletions apps/src/lib/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1702,12 +1702,12 @@ pub mod args {
const EXPIRATION_OPT: ArgOpt<DateTimeUtc> = arg_opt("expiration");
const FORCE: ArgFlag = flag("force");
const DONT_PREFETCH_WASM: ArgFlag = flag("dont-prefetch-wasm");
const GAS_AMOUNT: ArgDefault<token::Amount> =
arg_default("gas-amount", DefaultFn(|| token::Amount::from(0)));
const GAS_LIMIT: ArgDefault<token::Amount> =
arg_default("gas-limit", DefaultFn(|| token::Amount::from(0)));
const GAS_TOKEN: ArgDefaultFromCtx<WalletAddress> =
arg_default_from_ctx("gas-token", DefaultFn(|| "NAM".into()));
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)));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const FEE_TOKEN: ArgDefaultFromCtx<WalletAddress> =
arg_default_from_ctx("fee-token", DefaultFn(|| "NAM".into()));
const GENESIS_PATH: Arg<PathBuf> = arg("genesis-path");
const GENESIS_VALIDATOR: ArgOpt<String> = arg("genesis-validator").opt();
const HALT_ACTION: ArgFlag = flag("halt");
Expand Down Expand Up @@ -3024,7 +3024,7 @@ pub mod args {
/// If any new account is initialized by the tx, use the given alias to
/// save it in the wallet.
pub initialized_account_alias: Option<String>,
/// The amount being payed to include the transaction
/// The amount being payed (for gas unit) to include the transaction
pub fee_amount: token::Amount,
/// The token in which the fee is being paid
pub fee_token: WalletAddress,
Expand Down Expand Up @@ -3066,15 +3066,15 @@ pub mod args {
initialized, the alias will be the prefix of each new \
address joined with a number.",
))
.arg(GAS_AMOUNT.def().about(
"The amount being paid for the inclusion of this transaction",
.arg(FEE_AMOUNT.def().about(
"The amount being paid, per gas unit, for the inclusion of \
this transaction",
))
.arg(FEE_TOKEN.def().about("The token for paying the gas"))
.arg(GAS_LIMIT.def().about(
"The multiplier of the gas limit resolution defining the \
maximum amount of gas needed to run transaction",
))
.arg(GAS_TOKEN.def().about("The token for paying the gas"))
.arg(
GAS_LIMIT.def().about(
"The maximum amount of gas needed to run transaction",
),
)
.arg(EXPIRATION_OPT.def().about(
"The expiration datetime of the transaction, after which the \
tx won't be accepted anymore. All of these examples are \
Expand Down Expand Up @@ -3109,9 +3109,9 @@ pub mod args {
let broadcast_only = BROADCAST_ONLY.parse(matches);
let ledger_address = LEDGER_ADDRESS_DEFAULT.parse(matches);
let initialized_account_alias = ALIAS_OPT.parse(matches);
let fee_amount = GAS_AMOUNT.parse(matches);
let fee_token = GAS_TOKEN.parse(matches);
let gas_limit = GAS_LIMIT.parse(matches).into();
let fee_amount = FEE_AMOUNT.parse(matches);
let fee_token = FEE_TOKEN.parse(matches);
let gas_limit = GAS_LIMIT.parse(matches);
let expiration = EXPIRATION_OPT.parse(matches);

let signing_key = SIGNING_KEY_OPT.parse(matches);
Expand Down
Loading