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

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

Merged
merged 32 commits into from
Oct 30, 2023
Merged
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
9834392
Charge for input signature recovery (WIP)
bvrooman Oct 20, 2023
c2b6b01
Update tests
bvrooman Oct 22, 2023
8f8f119
Update CHANGELOG.md
bvrooman Oct 23, 2023
78bac01
Merge branch 'master' into bvrooman/feat/charge-for-input-signature-r…
bvrooman Oct 23, 2023
f3dd93d
Propagate gas_costs
bvrooman Oct 23, 2023
1978ef9
Update fee.rs
bvrooman Oct 23, 2023
880f217
Unique witnesses
bvrooman Oct 24, 2023
da91cb4
clippy
bvrooman Oct 24, 2023
da4f93b
Update unique witnesses and tests
bvrooman Oct 24, 2023
9a9661e
Add signed_inputs_with_inque_witnesses
bvrooman Oct 24, 2023
942cc6e
Add multiple message input tests
bvrooman Oct 24, 2023
670dba3
Clippy
bvrooman Oct 24, 2023
d9acf65
Calculate gas_used_by_signature_checks on Chargeable
bvrooman Oct 26, 2023
dbc7425
Increase test coverage
bvrooman Oct 26, 2023
c0625c7
Typo
bvrooman Oct 26, 2023
dfbea1d
Update tests
bvrooman Oct 26, 2023
87f0893
Revert inputs changes
bvrooman Oct 26, 2023
ca9753f
Merge branch 'master' into bvrooman/feat/charge-for-input-signature-r…
bvrooman Oct 26, 2023
bc3e060
Clippy knows best
bvrooman Oct 26, 2023
2e1b06a
More clippy
bvrooman Oct 26, 2023
5c2ec9c
Update contract_root gas cost based on benchmarks
bvrooman Oct 27, 2023
b83516c
Remove duplicated code
xgreenx Oct 27, 2023
c0a1180
Update max fee calculation
bvrooman Oct 27, 2023
125d598
Merge branch 'bvrooman/feat/charge-for-input-signature-recovery' of h…
bvrooman Oct 27, 2023
43ae1f1
Update contract_root to dependent cost
bvrooman Oct 27, 2023
496278d
Merge branch 'master' into bvrooman/feat/charge-for-input-signature-r…
Oct 27, 2023
035cf28
Update tests
bvrooman Oct 28, 2023
1448a3a
Merge branch 'bvrooman/feat/charge-for-input-signature-recovery' of h…
bvrooman Oct 28, 2023
b80af0f
Update gas.rs
bvrooman Oct 28, 2023
759aa57
Update checked_transaction.rs
bvrooman Oct 28, 2023
0bd7c3e
Update dependent gas resolve
bvrooman Oct 30, 2023
cdb0bee
Update tests
bvrooman Oct 30, 2023
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).

#### Breaking

- [#613](https://github.com/FuelLabs/fuel-vm/pull/613): Transaction fees now include the cost of signature verification for each input. For signed inputs, the cost of an EC recovery is charged. For predicate inputs, the cost of a BMT root of bytecode is charged.
- [#607](https://github.com/FuelLabs/fuel-vm/pull/607): The `Interpreter` expects the third generic argument during type definition that specifies the implementer of the `EcalHandler` trait for `ecal` opcode.
- [#609](https://github.com/FuelLabs/fuel-vm/pull/609): Checked transactions (`Create`, `Script`, and `Mint`) now enforce a maximum size. The maximum size is specified by `MAX_TRANSACTION_SIZE` in the transaction parameters, under consensus parameters. Checking a transaction above this size raises `CheckError::TransactionSizeLimitExceeded`.

Expand Down
5 changes: 5 additions & 0 deletions fuel-tx/src/transaction/consensus_parameters/gas.rs
Original file line number Diff line number Diff line change
Expand Up @@ -317,6 +317,9 @@ pub struct GasCostsValues {
pub smo: DependentCost,
pub srwq: DependentCost,
pub swwq: DependentCost,

// Non-opcode relative costs
pub contract_root: Word,
}

/// Dependent cost is a cost that depends on the number of units.
Expand Down Expand Up @@ -457,6 +460,7 @@ impl GasCostsValues {
smo: DependentCost::free(),
srwq: DependentCost::free(),
swwq: DependentCost::free(),
contract_root: 0,
}
}

Expand Down Expand Up @@ -569,6 +573,7 @@ impl GasCostsValues {
smo: DependentCost::unit(),
srwq: DependentCost::unit(),
swwq: DependentCost::unit(),
contract_root: 1,
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -161,5 +161,6 @@ pub fn default_gas_costs() -> GasCostsValues {
base: 44,
dep_per_unit: 5,
},
contract_root: 3024932,
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

}
}
142 changes: 122 additions & 20 deletions fuel-tx/src/transaction/fee.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,18 @@
use crate::FeeParameters;
use crate::{
field,
input::{
coin::CoinSigned,
message::{
MessageCoinSigned,
MessageDataSigned,
},
},
FeeParameters,
GasCosts,
Input,
};
use fuel_asm::Word;
use hashbrown::HashSet;

#[derive(Debug, Default, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
Expand Down Expand Up @@ -66,6 +79,7 @@ impl TransactionFee {
pub fn checked_from_values(
params: &FeeParameters,
metered_bytes: Word,
gas_used_by_signature_checks: Word,
gas_used_by_predicates: Word,
gas_limit: Word,
gas_price: Word,
Expand All @@ -74,7 +88,9 @@ impl TransactionFee {

// TODO: use native div_ceil once stabilized out from nightly
let bytes_gas = params.gas_per_byte.checked_mul(metered_bytes)?;
let min_gas = bytes_gas.checked_add(gas_used_by_predicates)?;
let min_gas = bytes_gas
.checked_add(gas_used_by_signature_checks)?
.checked_add(gas_used_by_predicates)?;
let max_gas = bytes_gas.checked_add(gas_limit)?;

let max_gas_to_pay = max_gas.checked_mul(gas_price).and_then(|total| {
Expand Down Expand Up @@ -113,18 +129,24 @@ 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>(
gas_costs: &GasCosts,
Copy link
Collaborator

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

params: &FeeParameters,
tx: &T,
) -> Option<Self> {
) -> Option<Self>
where
T: Chargeable + field::Inputs,
{
let metered_bytes = tx.metered_bytes_size() as Word;
let gas_used_by_signature_checks = tx.gas_used_by_signature_checks(gas_costs);
let gas_used_by_predicates = tx.gas_used_by_predicates();
let gas_limit = tx.limit();
let gas_price = tx.price();

Self::checked_from_values(
params,
metered_bytes,
gas_used_by_signature_checks,
gas_used_by_predicates,
gas_limit,
gas_price,
Expand All @@ -144,7 +166,58 @@ pub trait Chargeable {
fn metered_bytes_size(&self) -> usize;

/// Used for accounting purposes when charging for predicates.
fn gas_used_by_predicates(&self) -> Word;
fn gas_used_by_predicates(&self) -> Word
where
Self: field::Inputs,
{
let mut cumulative_predicate_gas: Word = 0;
for input in self.inputs() {
if let Some(predicate_gas_used) = input.predicate_gas_used() {
cumulative_predicate_gas =
cumulative_predicate_gas.saturating_add(predicate_gas_used);
}
}
cumulative_predicate_gas
}

fn gas_used_by_signature_checks(&self, gas_costs: &GasCosts) -> Word
where
Self: field::Inputs,
{
let mut witness_cache: HashSet<u8> = HashSet::new();
self.inputs()
.iter()
.filter(|input| match input {
// Include signed inputs of unique witness indices
Input::CoinSigned(CoinSigned { witness_index, .. })
| Input::MessageCoinSigned(MessageCoinSigned { witness_index, .. })
| Input::MessageDataSigned(MessageDataSigned { witness_index, .. })
if !witness_cache.contains(witness_index) =>
{
witness_cache.insert(*witness_index);
true
}
// Include all predicates
Input::CoinPredicate(_)
| Input::MessageCoinPredicate(_)
| Input::MessageDataPredicate(_) => true,
// Ignore all other inputs
_ => false,
})
.map(|input| match input {
// Charge EC recovery cost for signed inputs
Input::CoinSigned(_)
| Input::MessageCoinSigned(_)
| Input::MessageDataSigned(_) => gas_costs.ecr1,
// Charge the cost of the contract root for predicate inputs
Input::CoinPredicate(_)
| Input::MessageCoinPredicate(_)
| Input::MessageDataPredicate(_) => gas_costs.contract_root,
// Charge nothing for all other inputs
_ => 0,
})
.fold(0, |acc, cost| acc.saturating_add(cost))
}
}

#[cfg(test)]
Expand All @@ -159,68 +232,87 @@ mod tests {
.with_gas_per_byte(2)
.with_gas_price_factor(3);

fn gas_to_fee(gas: u64, gas_price: Word) -> f64 {
let fee = gas * gas_price;
fee as f64 / PARAMS.gas_price_factor as f64
}

#[test]
fn base_fee_is_calculated_correctly() {
Copy link
Member

Choose a reason for hiding this comment

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

Might be worth considering adding multi-sig tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

let metered_bytes = 5;
let gas_used_by_signature_checks = 12;
let gas_used_by_predicates = 7;
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");

let expected = PARAMS.gas_per_byte * metered_bytes + gas_limit;
let expected = expected * gas_price;
let expected = expected as f64 / PARAMS.gas_price_factor as f64;
let expected = expected.ceil() as Word;
let expected_max_gas = PARAMS.gas_per_byte * metered_bytes + gas_limit;
let expected_max_fee = gas_to_fee(expected_max_gas, gas_price).ceil() as Word;
let expected_min_gas = PARAMS.gas_per_byte * metered_bytes
+ gas_used_by_signature_checks
+ gas_limit;
let expected_min_fee = gas_to_fee(expected_min_gas, gas_price).ceil() as Word;

assert_eq!(expected, fee.max_fee);
assert_eq!(expected, fee.min_fee);
assert_eq!(expected_max_fee, fee.max_fee);
assert_eq!(expected_min_fee, fee.min_fee);
}

#[test]
fn base_fee_ceils() {
let metered_bytes = 5;
let gas_used_by_signature_checks = 12;
let gas_used_by_predicates = 7;
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");

let expected = PARAMS.gas_per_byte * metered_bytes + gas_limit;
let expected = expected * gas_price;
let expected = expected as f64 / PARAMS.gas_price_factor as f64;
let truncated = expected as Word;
let expected = expected.ceil() as Word;

assert_ne!(truncated, expected);
assert_eq!(expected, fee.max_fee);
assert_eq!(expected, fee.min_fee);
let expected_max_gas = PARAMS.gas_per_byte * metered_bytes + gas_limit;
let expected_max_fee = gas_to_fee(expected_max_gas, gas_price);
let truncated = expected_max_fee as Word;
let expected_max_fee = expected_max_fee.ceil() as Word;
assert_ne!(truncated, expected_max_fee);
assert_eq!(expected_max_fee, fee.max_fee);

let expected_min_gas = PARAMS.gas_per_byte * metered_bytes
+ gas_used_by_signature_checks
+ gas_limit;
let expected_min_fee = gas_to_fee(expected_min_gas, gas_price);
let truncated = expected_max_fee as Word;
let expected_min_fee = expected_min_fee.ceil() as Word;
assert_ne!(truncated, expected_min_fee);
assert_eq!(expected_min_fee, fee.min_fee);
}

#[test]
fn base_fee_zeroes() {
let metered_bytes = 5;
let gas_used_by_signature_checks = 12;
let gas_used_by_predicates = 7;
let gas_limit = 7;
let gas_price = 0;

let fee = TransactionFee::checked_from_values(
&PARAMS,
metered_bytes,
gas_used_by_signature_checks,
gas_used_by_predicates,
gas_limit,
gas_price,
Expand All @@ -236,13 +328,15 @@ mod tests {
#[test]
fn base_fee_wont_overflow_on_bytes() {
let metered_bytes = Word::MAX;
let gas_used_by_signature_checks = 12;
let gas_used_by_predicates = 7;
let gas_limit = 7;
let gas_price = 11;

let overflow = TransactionFee::checked_from_values(
&PARAMS,
metered_bytes,
gas_used_by_signature_checks,
gas_used_by_predicates,
gas_limit,
gas_price,
Expand All @@ -255,13 +349,15 @@ mod tests {
#[test]
fn base_fee_wont_overflow_on_gas_used_by_predicates() {
let metered_bytes = 5;
let gas_used_by_signature_checks = 12;
let gas_used_by_predicates = Word::MAX;
let gas_limit = 7;
let gas_price = 11;

let overflow = TransactionFee::checked_from_values(
&PARAMS,
metered_bytes,
gas_used_by_signature_checks,
gas_used_by_predicates,
gas_limit,
gas_price,
Expand All @@ -274,13 +370,15 @@ mod tests {
#[test]
fn base_fee_wont_overflow_on_limit() {
let metered_bytes = 5;
let gas_used_by_signature_checks = 12;
let gas_used_by_predicates = 7;
let gas_limit = Word::MAX;
let gas_price = 11;

let overflow = TransactionFee::checked_from_values(
&PARAMS,
metered_bytes,
gas_used_by_signature_checks,
gas_used_by_predicates,
gas_limit,
gas_price,
Expand All @@ -293,13 +391,15 @@ mod tests {
#[test]
fn base_fee_wont_overflow_on_price() {
let metered_bytes = 5;
let gas_used_by_signature_checks = 12;
let gas_used_by_predicates = 7;
let gas_limit = 7;
let gas_price = Word::MAX;

let overflow = TransactionFee::checked_from_values(
&PARAMS,
metered_bytes,
gas_used_by_signature_checks,
gas_used_by_predicates,
gas_limit,
gas_price,
Expand All @@ -312,13 +412,15 @@ mod tests {
#[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,
Expand Down
11 changes: 0 additions & 11 deletions fuel-tx/src/transaction/types/create.rs
Original file line number Diff line number Diff line change
Expand Up @@ -172,17 +172,6 @@ impl Chargeable for Create {
// is defined. Witness data should still be excluded.
self.witnesses_offset()
}

fn gas_used_by_predicates(&self) -> Word {
let mut cumulative_predicate_gas: Word = 0;
for input in self.inputs() {
if let Some(predicate_gas_used) = input.predicate_gas_used() {
cumulative_predicate_gas =
cumulative_predicate_gas.saturating_add(predicate_gas_used);
}
}
cumulative_predicate_gas
}
}

impl FormatValidityChecks for Create {
Expand Down
11 changes: 0 additions & 11 deletions fuel-tx/src/transaction/types/script.rs
Original file line number Diff line number Diff line change
Expand Up @@ -135,17 +135,6 @@ impl Chargeable for Script {
// is defined. Witness data should still be excluded.
self.witnesses_offset()
}

fn gas_used_by_predicates(&self) -> Word {
let mut cumulative_predicate_gas: Word = 0;
for input in self.inputs() {
if let Some(predicate_gas_used) = input.predicate_gas_used() {
cumulative_predicate_gas =
cumulative_predicate_gas.saturating_add(predicate_gas_used);
}
}
cumulative_predicate_gas
}
}

impl FormatValidityChecks for Script {
Expand Down
Loading