feat: Charge for input signature verification (address recovery and predicate roots) #613

bvrooman · 2023-10-22T22:38:14Z

Related issues:

Charge transaction for signatures verification as part of min_fee #599

This PR adds support for charging users for recovering addresses from signatures of signed inputs. For each signed input (non-predicate in effect), we charge the cost of an EC recovery. For each predicate input, we calculate the gas cost based on the length of the bytecode. The calculation is based on the gas cost of calculating the Merkle root of contract bytecode, which is dependent on contract length.

This PR uses the gas price generated by the benchmark in this PR. This benchmark calculates a gas cost by timing contract root calculation using the maximum size for contracts at 17mb.

…ecovery

xgreenx · 2023-10-24T23:11:59Z

fuel-tx/src/transaction.rs

+        /// Returns all signed inputs. This excludes predicate inputs.
+        fn signed_inputs(&self) -> Vec<&Input> {
+            self.inputs()
+                .iter()
+                .filter_map(|input| input.witness_index().is_some().then_some(input))
+                .collect_vec()
+        }
+
+        /// Returns all signed inputs filtered by witness uniqueness.
+        fn signed_inputs_with_unique_witnesses(&self) -> Vec<&Input> {
+            self.signed_inputs()
+                .into_iter()
+                .sorted_unstable_by(|a, b| {
+                    Ord::cmp(&a.witness_index(), &b.witness_index())
+                })
+                .dedup()
+                .collect_vec()
+        }


It is better to not add one-time functions into the trait. We only need them in one place, plus we need to iterate over all inputs because you need to charge for Signed and for Predicate inputs

I understand your suggestion. Instead specializing the trait here, I can just iterate over the inputs at the time of calculating the fee, as you suggest in another comment.

Currently, this method is also used in tests to verify the fee for a transaction is correct. But maybe I can refactor the tests so that we can test the fees without duplicating the calculation logic, and we can keep the calculation isolated to the one place.

xgreenx · 2023-10-24T23:15:57Z

fuel-tx/src/transaction/fee.rs

        params: &FeeParameters,
        tx: &T,
    ) -> Option<Self> {
        let metered_bytes = tx.metered_bytes_size() as Word;
+        let unique_witnesses = tx.signed_inputs_with_unique_witnesses().len() as Word;


Because you need to find Signed and Predicate inputs, it is better to iterate over them here and calculate the number of unique witnesses, plus charge for predicates.

You can use one loop to cover all inputs. Plus, you can already implement charging for the predicates too, but with some dummy values that you will update later.

I will point out that the calculation for predicate gas is done by the tx:

tx.gas_used_by_predicates()

whereas the signature cost is done here.

Would it be wrong to have a

tx.gas_used_for_sign_verification()

method on the tx? And if yes, would it make sense to change the predicate calculation?

I don't really have an opinion yet, but seems strange that they are so different.

I think it may be beneficial to move this to the Chargeable trait, and allow the method to take in a reference to GasCosts. We calculate the gas cost when checking the transaction, but also in a number of tests to verify the fee. Ideally, we want to test the calculation used in production, rather than testing a recreation of the calculation that may become out of sync. This means we need to wrap it in a method that we can call on the tx.

xgreenx · 2023-10-24T23:19:26Z

fuel-tx/src/transaction/fee.rs

@@ -113,18 +120,23 @@ impl TransactionFee {
    /// Attempt to create a transaction fee from parameters and transaction internals
    ///
    /// Will return `None` if arithmetic overflow occurs.
-    pub fn checked_from_tx<T: Chargeable>(
+    pub fn checked_from_tx<T: Chargeable + Inputs>(
+        gas_costs: &GasCosts,


Regarding predicates, you can already add a new cost into GasCosts and add a new structure that we can use later to charge for the predicates.

Maybe you want to extend FeeParameters, for example, the price of the ecr1 can be duplicated there. The constructor can accept gas_costs, like FeeParameters::new(gas_costs).

We could add new fields for both the cost of signed input address recovery and the cost of predicate ownership check. The advantage of this is that we don't have to pass GasCosts around, which is a little bit simpler.

On the other hand, it is not clear to me why that should be the responsibility of FeeParameters when GasCosts already contains these values. FeeParameters is responsible for providing information about how to convert gas to fees, rather than storing any concrete gas costs themselves.

bvrooman · 2023-10-25T00:20:41Z

I will put this PR back into draft as I apply the following changes:

refactor calculation to use a single loop over the inputs, checking for both signed inputs and predicates at this time, rather than separately
refactor tests to avoid duplication of fee calculation (allowing us to centralize the fee calculation to only checked_from_tx)
adding dummy value for predicate costs
(maybe) refactoring FeeParameters

fuel-tx/src/transaction/fee.rs

MitchTurner · 2023-10-25T23:51:26Z

fuel-tx/src/transaction/fee.rs

+        let fee = gas * gas_price;
+        fee as f64 / PARAMS.gas_price_factor as f64
+    }
+
    #[test]
    fn base_fee_is_calculated_correctly() {


Might be worth considering adding multi-sig tests?

I added additional tests for multiple signed inputs. Is that what you mean by multi-sig tests?

…ecovery

fuel-tx/src/transaction/types/create.rs

xgreenx · 2023-10-27T14:06:44Z

fuel-vm/src/checked_transaction.rs

+        let fee = tx.metered_bytes_size() as u128
            * fee_params.gas_per_byte as u128
            * tx.price() as u128;
        let gas = tx.limit() as u128 * tx.price() as u128;
-        let total = bytes + gas;
+        let total = fee + gas;


max_fee it is min_fee + (GasLimit converted into the price). So you need to update it in the same way as for is_valid_min_fee=)

If I understand correctly:

max_fee = min_fee + gas_limit * gas_price

That would mean that max_fee is always greater than or equal to min_fee.

After making that change, some tests fail, such as:

#[test] fn base_fee_gas_limit_less_than_gas_used_by_predicates() { let metered_bytes = 5; let gas_used_by_signature_checks = 12; let gas_used_by_predicates = 8; let gas_limit = 7; let gas_price = 11; let fee = TransactionFee::checked_from_values( &PARAMS, metered_bytes, gas_used_by_signature_checks, gas_used_by_predicates, gas_limit, gas_price, ) .expect("failed to calculate fee"); assert!(fee.min_fee > fee.max_fee); }

where we test that the min_fee is greater than max_fee.

Is there a specification for min/max fee?

xgreenx · 2023-10-27T14:17:56Z

fuel-tx/src/transaction/consensus_parameters/gas/default_gas_costs.rs

@@ -161,5 +161,6 @@ pub fn default_gas_costs() -> GasCostsValues {
            base: 44,
            dep_per_unit: 5,
        },
+        contract_root: 3024932,


Hmm, it is a very huge value. Maybe it is better if we use DependentCost instead. Could you check how expensive it is in this case, please?

…ttps://github.com/FuelLabs/fuel-vm into bvrooman/feat/charge-for-input-signature-recovery

…ecovery

…ttps://github.com/FuelLabs/fuel-vm into bvrooman/feat/charge-for-input-signature-recovery

bvrooman · 2023-10-28T01:16:41Z

fuel-tx/src/transaction/fee.rs

+        let min_gas = bytes_gas
+            .checked_add(gas_used_by_signature_checks)?
+            .checked_add(gas_used_by_predicates)?;
+        let max_gas = min_gas.checked_add(gas_limit)?;


Fee is effectively calculated using the formula defined in the spec. Min fee and max fee are separated by the gas limit.

In effect, max_fee = min_fee + gas_limit * gas_price.

bvrooman · 2023-10-28T01:17:29Z

fuel-tx/src/transaction/fee.rs

-    #[test]
-    fn base_fee_gas_limit_less_than_gas_used_by_predicates() {
-        let metered_bytes = 5;
-        let gas_used_by_predicates = 8;
-        let gas_limit = 7;
-        let gas_price = 11;
-
-        let fee = TransactionFee::checked_from_values(
-            &PARAMS,
-            metered_bytes,
-            gas_used_by_predicates,
-            gas_limit,
-            gas_price,
-        )
-        .expect("failed to calculate fee");
-
-        assert!(fee.min_fee > fee.max_fee);
-    }


I am removing this test because it is now no longer possible for min_fee to be greater than max_fee.

xgreenx · 2023-10-30T13:04:15Z

fuel-tx/src/transaction/consensus_parameters/gas.rs

+    pub fn resolve(&self, units: Word) -> Word {
+        // Apply the linear transformation from units to cost:
+        // f(x) = base + mx
+        self.base + self.dep_per_unit.saturating_mul(units)


The formula is base + units/dep_pet_unit.

Suggested change

self.base + self.dep_per_unit.saturating_mul(units)

self.base + units.saturating_div(self.dep_per_unit)

How do we handle the case where dep_per_unit is 0, as with DependentCost::unit()? Is that only for testing?

In production, we don't have cases like this=) But in the tests it overflows

xgreenx · 2023-10-30T16:23:10Z

fuel-tx/src/transaction/consensus_parameters/gas/default_gas_costs.rs

+        contract_root: DependentCost {
+            base: 75,
+            dep_per_unit: 1,
+        },


I got other results from benchmarks=) But it's okay. We can finalize them later before the final gas prices update.

bvrooman added 12 commits October 20, 2023 14:30

Charge for input signature recovery (WIP)

9834392

Update tests

c2b6b01

Update CHANGELOG.md

8f8f119

Merge branch 'master' into bvrooman/feat/charge-for-input-signature-r…

78bac01

…ecovery

Propagate gas_costs

f3dd93d

Update fee.rs

1978ef9

Unique witnesses

880f217

clippy

da91cb4

Update unique witnesses and tests

da4f93b

Add signed_inputs_with_inque_witnesses

9a9661e

Add multiple message input tests

942cc6e

Clippy

670dba3

bvrooman marked this pull request as ready for review October 24, 2023 19:10

bvrooman self-assigned this Oct 24, 2023

bvrooman requested a review from a team October 24, 2023 22:16

xgreenx reviewed Oct 24, 2023

View reviewed changes

bvrooman marked this pull request as draft October 25, 2023 00:21

MitchTurner reviewed Oct 25, 2023

View reviewed changes

fuel-tx/src/transaction/fee.rs Outdated Show resolved Hide resolved

MitchTurner reviewed Oct 25, 2023

View reviewed changes

bvrooman added 9 commits October 25, 2023 20:47

Calculate gas_used_by_signature_checks on Chargeable

d9acf65

Increase test coverage

dbc7425

Typo

c0625c7

Update tests

dfbea1d

Revert inputs changes

87f0893

Merge branch 'master' into bvrooman/feat/charge-for-input-signature-r…

ca9753f

…ecovery

Clippy knows best

bc3e060

More clippy

2e1b06a

Update contract_root gas cost based on benchmarks

5c2ec9c

bvrooman changed the title ~~feat: Charge for input signature recovery~~ feat: Charge for input signature verification (address recovery and predicate roots) Oct 27, 2023

bvrooman marked this pull request as ready for review October 27, 2023 05:30

Remove duplicated code

b83516c

xgreenx reviewed Oct 27, 2023

View reviewed changes

bvrooman added 3 commits October 27, 2023 15:16

Update max fee calculation

c0a1180

Merge branch 'bvrooman/feat/charge-for-input-signature-recovery' of h…

125d598

…ttps://github.com/FuelLabs/fuel-vm into bvrooman/feat/charge-for-input-signature-recovery

Update contract_root to dependent cost

43ae1f1

bvrooman mentioned this pull request Oct 27, 2023

test: Add contract code root benchmark FuelLabs/fuel-core#1452

Merged

Brandon Vrooman and others added 3 commits October 27, 2023 19:49

Merge branch 'master' into bvrooman/feat/charge-for-input-signature-r…

496278d

…ecovery

Update tests

035cf28

Merge branch 'bvrooman/feat/charge-for-input-signature-recovery' of h…

1448a3a

…ttps://github.com/FuelLabs/fuel-vm into bvrooman/feat/charge-for-input-signature-recovery

bvrooman commented Oct 28, 2023

View reviewed changes

bvrooman added 2 commits October 27, 2023 21:20

Update gas.rs

b80af0f

Update checked_transaction.rs

759aa57

xgreenx reviewed Oct 30, 2023

View reviewed changes

bvrooman added 2 commits October 30, 2023 10:57

Update dependent gas resolve

0bd7c3e

Update tests

cdb0bee

xgreenx approved these changes Oct 30, 2023

View reviewed changes

bvrooman added this pull request to the merge queue Oct 30, 2023

Merged via the queue into master with commit 8880e58 Oct 30, 2023
37 checks passed

bvrooman deleted the bvrooman/feat/charge-for-input-signature-recovery branch October 30, 2023 18:32

xgreenx mentioned this pull request Oct 31, 2023

Release v0.40.0 #621

Merged

bvrooman mentioned this pull request Nov 1, 2023

Charge transaction for signatures verification as part of min_fee #599

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Charge for input signature verification (address recovery and predicate roots) #613

feat: Charge for input signature verification (address recovery and predicate roots) #613

bvrooman commented Oct 22, 2023 •

edited

Loading

xgreenx Oct 24, 2023

bvrooman Oct 25, 2023

xgreenx Oct 24, 2023

MitchTurner Oct 25, 2023

bvrooman Oct 26, 2023

xgreenx Oct 24, 2023

bvrooman Oct 25, 2023

bvrooman commented Oct 25, 2023 •

edited

Loading

MitchTurner Oct 25, 2023

bvrooman Oct 27, 2023

xgreenx Oct 27, 2023

bvrooman Oct 27, 2023 •

edited

Loading

xgreenx Oct 27, 2023

bvrooman Oct 28, 2023

bvrooman Oct 28, 2023

xgreenx Oct 30, 2023

bvrooman Oct 30, 2023 •

edited

Loading

xgreenx Oct 30, 2023

xgreenx Oct 30, 2023

	self.base + self.dep_per_unit.saturating_mul(units)
	self.base + units.saturating_div(self.dep_per_unit)

feat: Charge for input signature verification (address recovery and predicate roots) #613

feat: Charge for input signature verification (address recovery and predicate roots) #613

Conversation

bvrooman commented Oct 22, 2023 • edited Loading

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

bvrooman commented Oct 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvrooman Oct 27, 2023 • edited Loading

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

bvrooman Oct 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvrooman commented Oct 22, 2023 •

edited

Loading

bvrooman commented Oct 25, 2023 •

edited

Loading

bvrooman Oct 27, 2023 •

edited

Loading

bvrooman Oct 30, 2023 •

edited

Loading