Support transaction policies #623

xgreenx · 2023-11-04T22:46:13Z

Closes #576 Closes FuelLabs/fuel-core#1398

Added support for transaction policies. The Script and Create
transactions received a new field, policies. Policies allow the addition
of some limits to the transaction to protect the user or specify some details regarding execution.

This change makes the GasPrice and Maturity fields optional, allowing to save space in the future. Also, this will enable us to support multidimensional prices later.

Along with this change, we introduced two new policies:

WitnessLimit - allows the limitation of the maximum size of witnesses in bytes for the contract. Because of the changes in the gas calculation model(the blockchain also charges the user for the witness data), the user should protect himself from the block producer or third parties blowing up witness data and draining the user's funds.
MaxFee - allows the upper bound for the maximum fee that users agree to pay for the transaction.

This change brings the following modification to the gas model:

The GasLimit only limits the script execution. Previously, the GasLimit also limited the predicate execution time, but it is not valid anymore. So, it is not possible to use the GasLimit for transaction cost limitations. A new MaxFee policy is a way to do that. The GasLimit field was removed from the Create transaction because it only relates to the script execution(which the Create transaction doesn't have).
The blockchain changes the user for the size of witness data(before it was free). There is no separate price for the storage, so it uses gas to charge the user. It affects the min_gas and min_fee calculation.
A new WhitnessLimit also impacts the max_gas and max_fee calculation along with the GasLimit(in the case of Create transaction only WitnessLimit affects the max_gas and max_fee).
The minimal gas also charges the user for transaction ID calculation.

The change has the following modification to the layout transaction:

The Create transaction doesn't have the GasLimit field anymore. Because the Create transaction doesn't have any script to execute
The Create and Script transactions don't have explicit maturity and gas_price fields. Instead, these fields can be set via a new policies field.
The Create and Script transactions have a new policies field with a unique canonical serialization and deserialization for optimal space consumption.

Other breaking changes caused by the change:

Each transaction requires setting the GasPrice policy.
Previously, GasLimit should be less than the MAX_GAS_PER_TX constant. After removing this field from the Create transaction, it is impossible to require it. Instead, it requires that max_gas <= MAX_GAS_PER_TX for any transaction. Consequently, any Script transaction that uses MAX_GAS_PER_TX as a GasLimit will always fail because of a new rule. Setting the estimated gas usage instead solves the problem.
If the max_fee > policies.max_fee, then transaction will be rejected.
If the witnessses_size > policies.witness_limit, then transaction will be rejected.
GTF opcode changed its hardcoded constants for fields. It should be updated according to the values from the specification on the Sway side.

Updated transaction validity rules according to policies. Updated fee calculation and refund logic.

Fixed all tests.

fuel-asm/src/args.rs

fuel-tx/src/tests/valid_cases/transaction.rs

fuel-tx/src/transaction.rs

fuel-tx/src/transaction/policies.rs

fuel-tx/src/transaction/fee.rs

fuel-vm/src/interpreter/metadata.rs

xgreenx · 2023-11-07T11:52:46Z

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

    pub fn free() -> Self {
        Self {
            base: 0,
-            dep_per_unit: 0,
+            dep_per_unit: u64::MAX,


It will produce zero-price

Adding a comment explaining that would be nice. Although, I'm also considering using 0 to make them free anyway in #625

xgreenx · 2023-11-07T12:00:06Z

fuel-tx/src/transaction/validity.rs

+    let max_gas = tx.max_gas(gas_costs, fee_params);
+    if max_gas > tx_params.max_gas_per_tx {
+        Err(CheckError::TransactionMaxGasExceeded)?
+    }


The specification says GasLimit > MAX_GAS_PER_TX, while in real life should limit the total transaction cost.

I've created a specification PR to actualize it.

But it is a huge breaking change because SDK right now uses MAX_GAS_PER_TX as a GasLimit -> the transaction always fails because of this check.

xgreenx · 2023-11-07T12:25:58Z

fuel-vm/src/interpreter/internal/message_tests.rs

+#[test_case(0, 0, 1 => Some(96))]
+#[test_case(88, 0, 1 => Some(184))]
+#[test_case(0, 1, 2 => Some(168))]
+#[test_case(0, 2, 3 => Some(240))]
+#[test_case(0, 1, 3 => Some(168))]
+#[test_case(44, 2, 3 => Some(284))]


Because we removed two fields, we need to subtract 16.

fuel-tx/src/transaction/fee.rs

xgreenx · 2023-11-07T12:43:57Z

fuel-tx/src/transaction/fee.rs

-
-#[cfg(test)]
-#[allow(clippy::cast_possible_truncation)]
-mod tests {


It is tested in the fuel_vm/src/checked_transaction.rs

# Conflicts: # .github/workflows/ci.yml # fuel-tx/src/transaction/fee.rs

fuel-tx/src/tests/valid_cases/transaction.rs

fuel-tx/src/transaction/policies.rs

fuel-tx/src/builder.rs

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

fuel-tx/src/transaction/policies.rs

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

fuel-vm/src/interpreter/memory/tests.rs

fuel-vm/src/tests/predicate.rs

Voxelot · 2023-11-08T09:51:18Z

fuel-tx/src/builder.rs

@@ -129,11 +116,10 @@ pub struct TransactionBuilder<Tx> {
 impl TransactionBuilder<Script> {
    pub fn script(script: Vec<u8>, script_data: Vec<u8>) -> Self {
        let tx = Script {
-            gas_price: Default::default(),
            gas_limit: Default::default(),


Should we also rename this to script_gas_limit to more closely match the spec?

Voxelot · 2023-11-08T09:51:56Z

fuel-tx/src/builder.rs

@@ -313,8 +297,11 @@ impl<Tx: Buildable> TransactionBuilder<Tx> {
        self
    }

-    pub fn gas_limit(&mut self, gas_limit: Word) -> &mut Self {
-        self.tx.set_gas_limit(gas_limit);
+    pub fn gas_limit(&mut self, gas_limit: Word) -> &mut Self


Suggested change

pub fn gas_limit(&mut self, gas_limit: Word) -> &mut Self

pub fn script_gas_limit(&mut self, gas_limit: Word) -> &mut Self

Voxelot · 2023-11-08T09:53:23Z

fuel-tx/src/tests/valid_cases/transaction.rs

+}
+
+#[test]
+fn script_set_witness_limit_for_non_empty_witness_fails() {


Suggested change

fn script_set_witness_limit_for_non_empty_witness_fails() {

fn script_set_witness_limit_less_than_witness_data_size_fails() {

Voxelot · 2023-11-08T09:54:23Z

fuel-tx/src/tests/valid_cases/transaction.rs

+}
+
+#[test]
+fn create_set_witness_limit_for_non_empty_witness_fails() {


Suggested change

fn create_set_witness_limit_for_non_empty_witness_fails() {

fn create_set_witness_limit_less_than_witness_data_size_fails() {

Voxelot · 2023-11-08T09:56:37Z

fuel-tx/src/transaction.rs

@@ -17,6 +39,7 @@ use itertools::Itertools;

 mod fee;
 mod metadata;
+pub mod policies;


[nit] Should pub mods be sorted into a separate section from private mods?

… feature/transaction-policy

bvrooman · 2023-11-08T09:49:31Z

fuel-tx/src/transaction/policies.rs

+            PolicyType::GasPrice => 0,
+            PolicyType::WitnessLimit => 1,
+            PolicyType::Maturity => 2,
+            PolicyType::MaxFee => 3,


You can use the inverse operation of how you defined the PolicyType to find the index:

Suggested change

PolicyType::GasPrice => 0,

PolicyType::WitnessLimit => 1,

PolicyType::Maturity => 2,

PolicyType::MaxFee => 3,

PolicyType::GasPrice => PoliciesBits::GasPrice.trailing_zeros(),

PolicyType::WitnessLimit => PoliciesBits::WitnessLimit.trailing_zeros(),

PolicyType::Maturity => PoliciesBits::Maturity.trailing_zeros(),

PolicyType::MaxFee => PoliciesBits::MaxFee.trailing_zeros(),

or:

Suggested change

PolicyType::GasPrice => 0,

PolicyType::WitnessLimit => 1,

PolicyType::Maturity => 2,

PolicyType::MaxFee => 3,

self.bit().trailinig_zeroes()

The idea was to write the implementation manually to control values because, in the future, we may remove GasPrice or any other policy.

As long as they don't get out of sync 👍

bvrooman · 2023-11-08T09:53:36Z

fuel-tx/src/transaction/policies.rs

+/// The total number of policies.
+pub const POLICIES_NUMBER: usize = PoliciesBits::all().bits().count_ones() as usize;
+
+/// It is a container for managing policies.


Suggested change

/// It is a container for managing policies.

/// Container for managing policies.

bvrooman · 2023-11-08T11:48:34Z

fuel-vm/src/error.rs

-    /// The transaction doesn't contain enough gas to evaluate all predicates
-    #[display(fmt = "Insufficient gas available for all predicates")]
-    CumulativePredicateGasExceededTxGasLimit,
+    /// The transaction `max_gas` more than global gas limit.


Suggested change

/// The transaction `max_gas` more than global gas limit.

/// The transaction's `max_gas` is greater than the global gas limit.

or

Suggested change

/// The transaction `max_gas` more than global gas limit.

/// The transaction policy value for `MaxGas` is greater than the global gas limit.

bvrooman · 2023-11-09T06:35:45Z

I proposed some changes in #630 - mostly for the purpose of allowing us to encapsulate policy definitions and checks better. It would be nice to have this logic centralized rather than spread out, because it means a reduction in overall complexity: Having one place to define this logic would mean better maintainability/less duplication and easier extensibility. But I am simply suggesting one approach - if you have an idea on how you would like to do this, we can review that too now or at a later date after this PR is in.

# Conflicts: # CHANGELOG.md # Cargo.toml

Imports FuelLabs/fuel-vm#623 --------- Co-authored-by: Brandon Vrooman <[email protected]> Co-authored-by: Brandon Vrooman <[email protected]> Co-authored-by: hal3e <[email protected]>

Findings after implementation FuelLabs/fuel-vm#623

Imports FuelLabs/fuel-vm#623 --------- Co-authored-by: Brandon Vrooman <[email protected]> Co-authored-by: Brandon Vrooman <[email protected]> Co-authored-by: hal3e <[email protected]>

xgreenx added 4 commits November 3, 2023 00:35

Removed subtraction of predicate used gas from the gas limit.

b8c05e5

Updated values of teh GTF

0b6d296

Implemented Policies structure.

4ad331f

Updated transaction validity rules according to policies. Updated fee calculation and refund logic.

Applied change for VM.

044b3a1

Fixed all tests.

xgreenx requested a review from a team November 4, 2023 22:46

xgreenx self-assigned this Nov 4, 2023

MitchTurner reviewed Nov 4, 2023

View reviewed changes

fuel-asm/src/args.rs Show resolved Hide resolved

MitchTurner reviewed Nov 5, 2023

View reviewed changes

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

MitchTurner reviewed Nov 5, 2023

View reviewed changes

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

Voxelot reviewed Nov 6, 2023

View reviewed changes

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

xgreenx added 2 commits November 6, 2023 22:43

Minor nits

17242eb

Fix test in fuel-core

9d78719

This was referenced Nov 7, 2023

Transaction policies support FuelLabs/fuel-core#1472

Merged

Some clarification after implementation FuelLabs/fuel-specs#533

Merged

MitchTurner reviewed Nov 7, 2023

View reviewed changes

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

xgreenx commented Nov 7, 2023

View reviewed changes

xgreenx added 2 commits November 7, 2023 12:47

Merge branch 'master' into feature/transaction-policy

482deaf

# Conflicts: # .github/workflows/ci.yml # fuel-tx/src/transaction/fee.rs

Fixed review ocmments

db58054

xgreenx marked this pull request as ready for review November 7, 2023 13:09

xgreenx requested a review from a team November 7, 2023 13:09

MitchTurner reviewed Nov 7, 2023

View reviewed changes

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

MitchTurner reviewed Nov 7, 2023

View reviewed changes

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

MitchTurner reviewed Nov 7, 2023

View reviewed changes

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

Dentosal reviewed Nov 7, 2023

View reviewed changes

fuel-tx/src/builder.rs Show resolved Hide resolved

MitchTurner reviewed Nov 7, 2023

View reviewed changes

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

Dentosal reviewed Nov 7, 2023

View reviewed changes

fuel-tx/src/transaction/policies.rs Show resolved Hide resolved

Dentosal reviewed Nov 7, 2023

View reviewed changes

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

Dentosal reviewed Nov 7, 2023

View reviewed changes

fuel-vm/src/interpreter/memory/tests.rs Outdated Show resolved Hide resolved

Dentosal reviewed Nov 7, 2023

View reviewed changes

fuel-vm/src/tests/predicate.rs Outdated Show resolved Hide resolved

Voxelot reviewed Nov 8, 2023

View reviewed changes

xgreenx added 4 commits November 8, 2023 10:23

Renamed gas_limit to script_gas_limit

3efa7dd

Merge remote-tracking branch 'origin/feature/transaction-policy' into…

9a488a6

… feature/transaction-policy

Renamed gas_limit to script_gas_limit

089fdd9

Use correct values for policies GTF

69c08ef

xgreenx force-pushed the feature/transaction-policy branch from 15efde3 to 69c08ef Compare November 8, 2023 11:19

bvrooman reviewed Nov 8, 2023

View reviewed changes

bvrooman mentioned this pull request Nov 8, 2023

dev: Encapsulate per-policy logic for validity and checking #630

Closed

Address comments

e8d7bcc

MitchTurner previously approved these changes Nov 8, 2023

View reviewed changes

xgreenx and others added 2 commits November 9, 2023 07:58

Merge branch 'master' into feature/transaction-policy

379869e

Merge branch 'master' into feature/transaction-policy

25e0a5e

# Conflicts: # CHANGELOG.md # Cargo.toml

xgreenx dismissed MitchTurner’s stale review via 25e0a5e November 9, 2023 09:56

xgreenx enabled auto-merge November 9, 2023 09:59

bvrooman approved these changes Nov 9, 2023

View reviewed changes

xgreenx added this pull request to the merge queue Nov 9, 2023

Merged via the queue into master with commit 61c6992 Nov 9, 2023
37 checks passed

xgreenx deleted the feature/transaction-policy branch November 9, 2023 10:09

xgreenx restored the feature/transaction-policy branch November 9, 2023 10:53

xgreenx deleted the feature/transaction-policy branch November 9, 2023 10:55

xgreenx mentioned this pull request Nov 13, 2023

Release v0.42.0 #635

Merged

xgreenx added a commit to FuelLabs/fuel-specs that referenced this pull request Nov 30, 2023

Some clarification after implementation (#533)

0904457

Findings after implementation FuelLabs/fuel-vm#623

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support transaction policies #623

Support transaction policies #623

xgreenx commented Nov 4, 2023 •

edited

Loading

xgreenx Nov 7, 2023

Dentosal Nov 7, 2023

xgreenx Nov 7, 2023

xgreenx Nov 7, 2023

xgreenx Nov 7, 2023

Voxelot Nov 8, 2023

Voxelot Nov 8, 2023

Voxelot Nov 8, 2023

Voxelot Nov 8, 2023

Voxelot Nov 8, 2023 •

edited

Loading

bvrooman Nov 8, 2023

xgreenx Nov 8, 2023

bvrooman Nov 9, 2023

bvrooman Nov 8, 2023

bvrooman Nov 8, 2023

bvrooman commented Nov 9, 2023 •

edited

Loading

	pub fn gas_limit(&mut self, gas_limit: Word) -> &mut Self
	pub fn script_gas_limit(&mut self, gas_limit: Word) -> &mut Self

	fn script_set_witness_limit_for_non_empty_witness_fails() {
	fn script_set_witness_limit_less_than_witness_data_size_fails() {

	fn create_set_witness_limit_for_non_empty_witness_fails() {
	fn create_set_witness_limit_less_than_witness_data_size_fails() {

	/// It is a container for managing policies.
	/// Container for managing policies.

	/// The transaction `max_gas` more than global gas limit.
	/// The transaction's `max_gas` is greater than the global gas limit.

	/// The transaction `max_gas` more than global gas limit.
	/// The transaction policy value for `MaxGas` is greater than the global gas limit.

Support transaction policies #623

Support transaction policies #623

Conversation

xgreenx commented Nov 4, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Voxelot Nov 8, 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

bvrooman commented Nov 9, 2023 • edited Loading

xgreenx commented Nov 4, 2023 •

edited

Loading

Voxelot Nov 8, 2023 •

edited

Loading

bvrooman commented Nov 9, 2023 •

edited

Loading