Add expiration policy #871

AurelienFT · 2024-11-16T22:22:10Z

FuelLabs/fuel-specs#593

Spec PR : FuelLabs/fuel-specs#616

Checklist

Breaking changes are clearly marked as such in the PR description and changelog
New behavior is reflected in tests
If performance characteristic of an instruction change, update gas costs as well or make a follow-up PR for that
The specification matches the implemented behavior (link update PR if changes are needed)

Before requesting review

I have reviewed the code myself
I have created follow-up issues caused by this PR and linked them here

After merging, notify other teams

[Add or remove entries as needed]

Rust SDK
Sway compiler
Platform documentation (for out-of-organization contributors, the person merging the PR will do this)
Someone else?

MitchTurner

About time we added the expiration :) Both maturity and expiration are important for predicate writing.

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

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

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

fuel-vm/src/tests/validation.rs

fuel-tx/src/transaction.rs

Voxelot · 2024-11-18T05:50:51Z

We might want to beef up the tests around da_compression.rs to cover policy compression/decompression, as right now it is only testing randomly generated txs without any policies.

cc @Dentosal

MitchTurner

LGTM!

fuel-tx/src/tests/bytes.rs

Co-authored-by: Andrea Cerone <[email protected]>

rafal-ch · 2024-11-26T15:16:18Z

fuel-tx/src/transaction/policies.rs

@@ -305,6 +328,11 @@ impl Distribution<Policies> for Standard {
            policies.set(PolicyType::Maturity, Some(maturity as u64));
        }

+        if policies.get(PolicyType::Expiration).is_some() {


nit: I think we can use is_set() instead of get(...).is_some()

Nice. Changed it and for Maturity above also.

rafal-ch · 2024-11-26T15:23:45Z

fuel-vm/src/tests/validation.rs

+    let rng = &mut StdRng::seed_from_u64(2322u64);
+
+    // Given
+    const EXPIRATION: BlockHeight = BlockHeight::new(2);


Suggested change

const EXPIRATION: BlockHeight = BlockHeight::new(2);

const EXPIRATION: BlockHeight = BlockHeight::new(1);

to clearly show that transactions expire only when the current block height is higher than the specified expiry block (if the match, the transaction is still ok).

For the record, I'd also add a similar test fn transaction__execution__doesn_not_work_after_expiration() and use, for example:

const EXPIRATION: BlockHeight = BlockHeight::new(9); const BLOCK_HEIGHT: BlockHeight = BlockHeight::new(10);

I added a new test to test the current height. However it seems that for maturiy we only test correct case here (failed cases are test elsewhere) but I tried to make one here but finalize_checked is panicking on error.

xgreenx

I think we can also update the into_ready function to verify the expiration policy as well. Because this function exists to verify stuff that can change after verification.

xgreenx · 2024-11-27T22:52:34Z

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

+    // Given
+    let err = Transaction::script(
+        Default::default(),
+        vec![],
+        vec![],
+        Policies::new().with_expiration(999.into()).with_max_fee(0),
+        vec![],
+        vec![],
+        vec![],
+    )
+    // When
+    .check(block_height, &test_params())
+    .expect_err("Expected erroneous transaction");


Suggested change

// Given

let err = Transaction::script(

Default::default(),

vec![],

vec![],

Policies::new().with_expiration(999.into()).with_max_fee(0),

vec![],

vec![],

vec![],

)

// When

.check(block_height, &test_params())

.expect_err("Expected erroneous transaction");

// Given

let old_block_height = 999u32.into();

let err = Transaction::script(

Default::default(),

vec![],

vec![],

Policies::new().with_expiration(old_block_height).with_max_fee(0),

vec![],

vec![],

vec![],

)

// When

.check(block_height, &test_params())

.expect_err("Expected erroneous transaction");

xgreenx · 2024-11-27T22:53:11Z

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

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


Why we can't use the same pattern like in

TransactionBuilder::create(rng.gen(), rng.gen(), vec![]) // Given .expiration(block_height) .add_fee_input() .add_contract_created() .finalize() // When .check(block_height, &test_params()) // Then .expect("Failed to validate tx create");

?

The same about Create transaction

xgreenx · 2024-11-27T22:58:43Z

fuel-tx/src/transaction/validity.rs

@@ -358,6 +361,10 @@ where
        Err(ValidityError::TransactionMaturity)?;
    }

+    if tx.expiration() < block_height {


@dmihal @bajpai244 What do we expect when tx.expiration == block_height? Should it fail or should it pass?

AurelienFT added 3 commits November 16, 2024 23:21

Add expiration policy, some tests are still failing.

d1748bc

Fix tests

ac9d47b

Update changelog

d57fcd0

AurelienFT marked this pull request as ready for review November 17, 2024 13:05

AurelienFT requested review from xgreenx, Dentosal, MitchTurner and Voxelot as code owners November 17, 2024 13:05

AurelienFT mentioned this pull request Nov 17, 2024

Add policy expiration FuelLabs/fuel-specs#616

Open

5 tasks

AurelienFT requested a review from a team November 17, 2024 13:11

AurelienFT self-assigned this Nov 17, 2024

MitchTurner reviewed Nov 17, 2024

View reviewed changes

Update test and default expiration vlaue

ab81aaf

AurelienFT requested a review from MitchTurner November 18, 2024 09:51

MitchTurner previously approved these changes Nov 18, 2024

View reviewed changes

AurelienFT requested a review from a team November 19, 2024 13:34

AurelienFT mentioned this pull request Nov 21, 2024

(Merge after VM PR) Use expiration policy FuelLabs/fuel-core#2447

Open

9 tasks

acerone85 reviewed Nov 21, 2024

View reviewed changes

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

Update fuel-tx/src/tests/bytes.rs

d909d38

Co-authored-by: Andrea Cerone <[email protected]>

AurelienFT dismissed MitchTurner’s stale review via d909d38 November 21, 2024 17:45

AurelienFT requested a review from MitchTurner November 21, 2024 17:45

petertonysmith94 mentioned this pull request Nov 22, 2024

Support Policy Expiration FuelLabs/fuels-ts#3403

Open

rafal-ch reviewed Nov 26, 2024

View reviewed changes

Add test and use helper function to policy exists.

237e902

AurelienFT requested review from acerone85 and rafal-ch November 26, 2024 17:41

xgreenx reviewed Nov 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add expiration policy #871

Add expiration policy #871

AurelienFT commented Nov 16, 2024 •

edited

Loading

MitchTurner left a comment

Voxelot commented Nov 18, 2024 •

edited

Loading

MitchTurner left a comment

rafal-ch Nov 26, 2024

AurelienFT Nov 26, 2024

rafal-ch Nov 26, 2024

rafal-ch Nov 26, 2024

AurelienFT Nov 26, 2024

xgreenx left a comment

xgreenx Nov 27, 2024

xgreenx Nov 27, 2024

xgreenx Nov 27, 2024

	const EXPIRATION: BlockHeight = BlockHeight::new(2);
	const EXPIRATION: BlockHeight = BlockHeight::new(1);

Add expiration policy #871

Are you sure you want to change the base?

Add expiration policy #871

Conversation

AurelienFT commented Nov 16, 2024 • edited Loading

Checklist

Before requesting review

After merging, notify other teams

MitchTurner left a comment

Choose a reason for hiding this comment

Voxelot commented Nov 18, 2024 • edited Loading

MitchTurner left a 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

xgreenx left a 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

AurelienFT commented Nov 16, 2024 •

edited

Loading

Voxelot commented Nov 18, 2024 •

edited

Loading