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

Gas & Fee #1327

Merged
merged 54 commits into from
Sep 6, 2023
Merged

Gas & Fee #1327

merged 54 commits into from
Sep 6, 2023

Conversation

grarco
Copy link
Collaborator

@grarco grarco commented May 2, 2023

Describe your changes

Closes #1010.
Closes #1473.
Closes #1489.
Closes #1597.

This PR finalizes the first version of the gas & fee system:

  • 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
  • Removes fixed fees added in Testnet fees #962
  • Implements variable fees
  • Implements fee payment to the actual block proposer
  • Adds a dry-run-wrapper client command to estimate gas cost of a tx
  • Extend the BlockSpaceAllocator to also allocate block gas
  • Adds an optional masp unshielding section to the Tx for fee payment
  • Implements disposable keypairs for wrapper signing and fee payment
  • Disable GAS_LIMIT_RESOLUTION
  • Expects a valid block proposer with every block
  • Testing

Indicate on which release or other PRs this topic is based on

v0.21.1

Checklist before merging to draft

  • I have added a changelog
  • Git history is in acceptable state

@grarco
Copy link
Collaborator Author

grarco commented May 3, 2023

pls update wasm

@grarco grarco force-pushed the grarco/variable-fees branch 2 times, most recently from 8c21790 to aa1196b Compare May 5, 2023 21:11
@grarco
Copy link
Collaborator Author

grarco commented May 5, 2023

pls spawn devnet [devnet-0.15-gas-fee,3,heliaxdev@1be9883,ON]

@cwgoes cwgoes mentioned this pull request May 18, 2023
10 tasks
@grarco grarco force-pushed the grarco/variable-fees branch 3 times, most recently from 27492b8 to 201f90d Compare May 31, 2023 12:15
@grarco grarco force-pushed the grarco/variable-fees branch from 201f90d to b752e7d Compare June 5, 2023 01:37
@grarco grarco force-pushed the grarco/variable-fees branch 3 times, most recently from 3f75477 to f270cca Compare June 16, 2023 13:35
@grarco grarco mentioned this pull request Jun 28, 2023
@grarco grarco force-pushed the grarco/variable-fees branch 4 times, most recently from 201f90d to fcd3c27 Compare July 5, 2023 11:21
@grarco grarco changed the title Variable fees Variable gas fees Jul 5, 2023
@grarco grarco force-pushed the grarco/variable-fees branch 2 times, most recently from 35879c5 to b63e370 Compare July 10, 2023 21:12
@grarco grarco force-pushed the grarco/variable-fees branch 9 times, most recently from 09b9b4e to 8fdcf8f Compare July 16, 2023 11:48
grarco added a commit that referenced this pull request Aug 24, 2023
@grarco grarco force-pushed the grarco/variable-fees branch from d0ae535 to 1bd14ea Compare August 24, 2023 11:27
grarco added a commit that referenced this pull request Aug 24, 2023
@grarco grarco force-pushed the grarco/variable-fees branch from 1bd14ea to 0aaaa66 Compare August 24, 2023 11:56
@grarco grarco force-pushed the grarco/variable-fees branch from 0aaaa66 to af95690 Compare August 24, 2023 14:41
Fraccaman added a commit that referenced this pull request Aug 25, 2023
* origin/grarco/variable-fees:
  changelog: #1327
  [ci] wasm checksums update
  Keep write log changes before fee unshield failure in `charge_fee`
  Changes `dev` feature to `testing`
  Adds check on masp gas payer
  Fixes broken fee optimization logic in cli. Fixes fee unshielding logic in ledger
  Saves optional disposable keypair to wallet
  Moves disposable signing key generation to wallet module
  Improves fee-related logs in the client
  Renames fees cli arguments
  Fixes gas in tests
  Renames fee payers cli arguments
  Refactors `get_tx_fee` to avoid using `Uint`
  Fixes `fee_amount` parsing
  Reduces the scale of the gas sub-units
  Adds gas and fees protocol parameters to query response
  Updates visibility of functions
  Adds fee unshielding integration test. Updates other tests
  Adds disposable signer e2e test. Updates other tests
  Updates gas meters in tests
  Updates e2e tests
  Misc adjustments for testing
  Adds disposable wrapper signer. Adjusts tx construction for fees
  Sdk functions to return the optional `Epoch` for fee unshielding
  Handles wrapper's signer separately
  Renames `gas_payer` to `fee_payer`
  Reworks wasm compilation step. Accepts `floats` wasm feature
  Dry-run wrapper tx
  Updates gas meter objects initializations
  Helper storage functions to construct fee unshielding tx in protocol
  `ShellParams` carries a generic wl_storage. Reworks `dispatch_tx` to charge fees and adds related functions
  Adds spare gas for each tx in the storage queue
  Updates gas computation with multipliers
  Removes masp fees + gas for the vp
  Fixes `clean-wasm` step of wasm Makefile
  Updates wasm vps with gas and host sig verification
  Gas metering for sig verification
  Renames gas function and adds exposed function for signatures' verification
  Exposes wasm functions for consuming gas
  Whitelisted gas in wasm
  Adds benchmarks crate for gas
  `process_proposal` validates gas & fees
  `prepare_proposal` validates gas & fee. Extracts validation to a separate method
  Renames `BlockAllocator` and makes it track gas too
  Updates unit tests for mandatory block proposer
  Reviews `ErrorCodes`. Adds fee validation function
  Always expects a block proposer. Moves `WrapperTx` handling inside `dispatch_tx`
  Adds new CLI arguments for gas & fee
  Adds core methods for fee unshielding. Brings back `GasLimit` to `u64`
  Adds a precommit write log to `WriteLog`. Adjusts gas accounting with multipliers
  Adds `split_borrow` method to `WriteLogAndStorage` trait
  Adds gas and fee protocol parameters
  Reworks `gas` module. Introduces `GasMetering` trait and gas sub-units
@Fraccaman Fraccaman mentioned this pull request Aug 25, 2023
Copy link
Member

Choose a reason for hiding this comment

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

it looks like this file is not being used - we must have accidentally revived it at some point

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah yes, could be my fault since the base of this pr is very old, I might have kept it by mistake (should be correctly removed in the draft branch though)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, actually it's still there...

Copy link
Member

Choose a reason for hiding this comment

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

This was renamed from block_space_alloc but the original is not deleted (same for the nested mods)

@@ -25,7 +25,7 @@ const UNTRUSTED_WASM_FEATURES: WasmFeatures = WasmFeatures {
relaxed_simd: false,
threads: false,
tail_call: false,
floats: false,
floats: true,
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we want to allow floats in wasm as it's allowed to give non deterministic results for NaNs. It's not needed for the gas, is it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah yes, sorry I should have left a note on this one. So basically I reworked the way we call fetch_or_compile and compile_and_fetch and also the implementation of fetch_or_compile. This is because compile_or_fetch is eventually called in any case (either the code being the actual code or only the hash of it). So I basically rewrote it to call fetch_or_compile in any case (this ease the gas accounting operation that must be done in any case and can therefore be done in this function only) and then let this function call compile_or_fetch in turn.

Now, in the process of doing this, I also moved the call to validate_untrusted_wasm because, at the moment, we are validating the code only when we receive an hash but I think we should do the opposite. When we get an hash, if the precompiled module is not in cache, we end up reading the code from storage. But I think there should be no need to validate code placed in storage (validation should happen when we write the code to storage which I think we are not doing at the moment). If instead of an hash we get some code, we need to validate it before running it (because if the tx is not whitelisted it would only be discarded by the vps, so after it's run).

Now, to the point of the floats. In the compilation of the ibc transaction we emit some f64 operations (I checked it with wasm2wat on the code emitted using both build-wasm-scripts and build-wasm-scripts-docker from different releases, even in the current version of main these instructions are there). The rearrangement of the compilation steps I mentioned before brought up this thing which, usually, doesn't manifest itself because the precompiled wasm module happens to always be in cache and so skips the validation part. To double check this, in the main branch, I removed the load from cache and forced the compilation of the module every time and indeed, the validation step rejected the ibc transaction because of forbidden floating point operations.

I had a look at the ibc code and it looks that these f64 operations where coming from one of ibc dependencies, in particular from a single insert in an HashMap which, for some strange reasons, triggered the generation of f64 operations in wasm, so I simply allowed floats to speed up the development process since I think I cannot solve this issue by myself

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see. The change makes sense, thanks!

I had a look at tx_ibc (I left the symbols in the wasm [by removing RUSTFLAGS='-C link-arg=-s' from makefile]) and found that the f64 usage comes from serde_json and it's compiled into a number parses even if floats are not used :/ It's being called from e.g. send_transfer_execute imported from ibc-rs. If we want to remove the usage of floats we have to use a fork (serde-rs/json#567). There's one from Near https://github.com/nearprotocol/json, it's a bit behind so might need to rebase on later version.

cc @yito88

Copy link
Member

Choose a reason for hiding this comment

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

Fraccaman added a commit that referenced this pull request Sep 6, 2023
* origin/grarco/variable-fees:
  changelog: #1327
  [ci] wasm checksums update
  Keep write log changes before fee unshield failure in `charge_fee`
  Changes `dev` feature to `testing`
  Adds check on masp gas payer
  Fixes broken fee optimization logic in cli. Fixes fee unshielding logic in ledger
  Saves optional disposable keypair to wallet
  Moves disposable signing key generation to wallet module
  Improves fee-related logs in the client
  Renames fees cli arguments
  Fixes gas in tests
  Renames fee payers cli arguments
  Refactors `get_tx_fee` to avoid using `Uint`
  Fixes `fee_amount` parsing
  Reduces the scale of the gas sub-units
  Adds gas and fees protocol parameters to query response
  Updates visibility of functions
  Adds fee unshielding integration test. Updates other tests
  Adds disposable signer e2e test. Updates other tests
  Updates gas meters in tests
  Updates e2e tests
  Misc adjustments for testing
  Adds disposable wrapper signer. Adjusts tx construction for fees
  Sdk functions to return the optional `Epoch` for fee unshielding
  Handles wrapper's signer separately
  Renames `gas_payer` to `fee_payer`
  Reworks wasm compilation step. Accepts `floats` wasm feature
  Dry-run wrapper tx
  Updates gas meter objects initializations
  Helper storage functions to construct fee unshielding tx in protocol
  `ShellParams` carries a generic wl_storage. Reworks `dispatch_tx` to charge fees and adds related functions
  Adds spare gas for each tx in the storage queue
  Updates gas computation with multipliers
  Removes masp fees + gas for the vp
  Fixes `clean-wasm` step of wasm Makefile
  Updates wasm vps with gas and host sig verification
  Gas metering for sig verification
  Renames gas function and adds exposed function for signatures' verification
  Exposes wasm functions for consuming gas
  Whitelisted gas in wasm
  Adds benchmarks crate for gas
  `process_proposal` validates gas & fees
  `prepare_proposal` validates gas & fee. Extracts validation to a separate method
  Renames `BlockAllocator` and makes it track gas too
  Updates unit tests for mandatory block proposer
  Reviews `ErrorCodes`. Adds fee validation function
  Always expects a block proposer. Moves `WrapperTx` handling inside `dispatch_tx`
  Adds new CLI arguments for gas & fee
  Adds core methods for fee unshielding. Brings back `GasLimit` to `u64`
  Adds a precommit write log to `WriteLog`. Adjusts gas accounting with multipliers
  Adds `split_borrow` method to `WriteLogAndStorage` trait
  Adds gas and fee protocol parameters
  Reworks `gas` module. Introduces `GasMetering` trait and gas sub-units
Fraccaman added a commit that referenced this pull request Sep 6, 2023
* origin/grarco/variable-fees:
  changelog: #1327
  [ci] wasm checksums update
  Keep write log changes before fee unshield failure in `charge_fee`
  Changes `dev` feature to `testing`
  Adds check on masp gas payer
  Fixes broken fee optimization logic in cli. Fixes fee unshielding logic in ledger
  Saves optional disposable keypair to wallet
  Moves disposable signing key generation to wallet module
  Improves fee-related logs in the client
  Renames fees cli arguments
  Fixes gas in tests
  Renames fee payers cli arguments
  Refactors `get_tx_fee` to avoid using `Uint`
  Fixes `fee_amount` parsing
  Reduces the scale of the gas sub-units
  Adds gas and fees protocol parameters to query response
  Updates visibility of functions
  Adds fee unshielding integration test. Updates other tests
  Adds disposable signer e2e test. Updates other tests
  Updates gas meters in tests
  Updates e2e tests
  Misc adjustments for testing
  Adds disposable wrapper signer. Adjusts tx construction for fees
  Sdk functions to return the optional `Epoch` for fee unshielding
  Handles wrapper's signer separately
  Renames `gas_payer` to `fee_payer`
  Reworks wasm compilation step. Accepts `floats` wasm feature
  Dry-run wrapper tx
  Updates gas meter objects initializations
  Helper storage functions to construct fee unshielding tx in protocol
  `ShellParams` carries a generic wl_storage. Reworks `dispatch_tx` to charge fees and adds related functions
  Adds spare gas for each tx in the storage queue
  Updates gas computation with multipliers
  Removes masp fees + gas for the vp
  Fixes `clean-wasm` step of wasm Makefile
  Updates wasm vps with gas and host sig verification
  Gas metering for sig verification
  Renames gas function and adds exposed function for signatures' verification
  Exposes wasm functions for consuming gas
  Whitelisted gas in wasm
  Adds benchmarks crate for gas
  `process_proposal` validates gas & fees
  `prepare_proposal` validates gas & fee. Extracts validation to a separate method
  Renames `BlockAllocator` and makes it track gas too
  Updates unit tests for mandatory block proposer
  Reviews `ErrorCodes`. Adds fee validation function
  Always expects a block proposer. Moves `WrapperTx` handling inside `dispatch_tx`
  Adds new CLI arguments for gas & fee
  Adds core methods for fee unshielding. Brings back `GasLimit` to `u64`
  Adds a precommit write log to `WriteLog`. Adjusts gas accounting with multipliers
  Adds `split_borrow` method to `WriteLogAndStorage` trait
  Adds gas and fee protocol parameters
  Reworks `gas` module. Introduces `GasMetering` trait and gas sub-units
@grarco grarco force-pushed the grarco/variable-fees branch from 12a91cd to 5b36a37 Compare September 6, 2023 13:56
@Fraccaman Fraccaman merged commit 37753b0 into main Sep 6, 2023
@Fraccaman Fraccaman deleted the grarco/variable-fees branch September 6, 2023 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants