Decompose consensus params #514

MitchTurner · 2023-07-19T16:24:12Z

The idea behind this PR was that ConsensusParameters was being passed to any function that needed a value in ConsensusParameters. We wanted the functions to define what they needed instead of giving them the whole kitchen sink.

The work included:

Removing ConsensusParams from all function signatures
Adding the required fields back to the function signatures
Grouping parameters that are used together into sub structs: e.g. TxParameters, FeeParameters
Fixing tests
Fixing tests
Fixing tests
Combining fields of large function signatures

MitchTurner · 2023-07-19T16:25:35Z

In addition to tests not passing, There are a number of function signatures I'd like to improve before merging this. I'll try to document what those changes are here so we can track them.

xgreenx · 2023-07-20T22:24:26Z

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

@@ -237,25 +242,29 @@ impl FormatValidityChecks for Create {
    fn check_without_signatures(
        &self,
        block_height: BlockHeight,
-        parameters: &ConsensusParameters,
+        tx_params: &TxParameters,


If we need a combination of the parameters, it is better to pass only ConsensusParameters. It is not a problem if some fields are not used. The API should be simple for the user and transparent regarding requirements(like you don't need to pass ConsensusParameters if you only need a ChainId).

For sure. I've combined them back together in some other places. Clippy just didn't catch this because it's at the limit (7). I can do the same here. Good catch.

xgreenx · 2023-07-24T11:44:56Z

fuel-tx/src/builder.rs

+    tx_params: TxParameters,
+    predicate_params: PredicateParameters,
+    script_params: ScriptParameters,
+    contract_params: ContractParameters,
+    fee_params: FeeParameters,
+    chain_id: ChainId,


Maybe it is still better to store all params like ConsensusParameters inside. In this case get_params will be useful too(it is commented now). In this case all new methods are still useable but you support old once too=)

xgreenx · 2023-07-24T11:46:38Z

fuel-tx/src/tests/offset.rs

@@ -466,7 +469,8 @@ fn iow_offset() {
            let bytes = tx.to_bytes();

            let mut tx_p = tx.clone();
-            tx_p.precompute(&ConsensusParameters::DEFAULT.chain_id)
+            let chain_id = ChainId::new(0);
+            tx_p.precompute(&chain_id)


I guess you can use Default::default like you did in other places=)

xgreenx · 2023-07-24T12:34:39Z

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

+            let chain_id = ChainId::new(0);
+
+            tx.sign_inputs(&secret, &chain_id);
+            keys.iter().for_each(|sk| tx.sign_inputs(sk, &chain_id));


The same here, let's use one approach everywhere either Default::default() or ChainId::default() or ChainId::new(0) or ChainId::DEFAULT=)

It applies to all places=)

xgreenx · 2023-07-24T12:38:34Z

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

@@ -648,7 +772,11 @@ fn transaction_with_duplicate_contract_inputs_is_invalid() {
        .add_output(o)
        .add_output(p)
        .finalize()
-        .check_without_signatures(Default::default(), &Default::default())
+        .check_without_signatures(


It seems we don't need to modify it=) You don't use chain_id

This is a trait method. It gets used in the other impls of this method.

But it sounds like you want chain_id as part of ConsensusParams so I think it will get removed in that case anyway.

xgreenx · 2023-07-24T12:43:51Z

fuel-tx/src/transaction/consensus_parameters.rs

+
+/// A collection of parameters for convenience
+#[derive(Debug, Clone, Copy)]
+pub struct ConsensusParams<'a> {


ChainId should be part of the ConsensusParams, because it is the same for the whole network and without it it is impossible to continue the network

It is true that the ConsensusParams is a combination of the other parameters and references them. But at the same time, this type can be the place that actually aggregates all of them and holds them during the lifecycle of the fuel-core. If we want to pass somewhere a reference to all params, we can pass &ConsensusParams. The ConsensusParams itself doesn't need to work with references to the Copy types.

It makes the API harder to use and creates situations like in several places of your PR(Where you create each parameter -> you create a ConsensusParams -> you pass it to the corresponding place).

I think we need to work with values instead of references, if you don't have any strong reason why we want to use references=)

It does make it harder to work with.

I had reasons I preferred references, but I'm not really convinced by them anymore, so I'll just get rid of the refs. Especially since all of them are Copy.

xgreenx · 2023-07-24T13:23:27Z

fuel-vm/src/checked_transaction.rs

-        params: &ConsensusParameters,
-        gas_costs: &GasCosts,
-    ) -> Result<Self, CheckError>;
+    fn check_predicates(self, params: CheckPredicateParams) -> Result<Self, CheckError>;


I prefer &CheckPredicateParams more than CheckPredicateParams because we usually store this configuration somewhere. Plus, downstream code decides whether it needs to clone or not.

xgreenx · 2023-07-24T13:25:23Z

fuel-vm/src/interpreter.rs

@@ -109,7 +112,14 @@ pub struct Interpreter<S, Tx = ()> {
    balances: RuntimeBalances,
    gas_costs: GasCosts,
    profiler: Profiler,
-    params: ConsensusParameters,
+    fee_params: FeeParameters,


I guess you want to use InterpreterParams here=)

xgreenx · 2023-07-24T13:26:22Z

fuel-vm/src/interpreter.rs

@@ -181,6 +191,12 @@ impl<S, Tx> Interpreter<S, Tx> {
    pub const fn profiler(&self) -> &Profiler {
        &self.profiler
    }
+
+    /// Modify the FeeParameters
+    pub fn with_fee_params(mut self, fee_params: FeeParameters) -> Self {


It should be specified during creation of the Interpreter

xgreenx · 2023-07-24T13:29:07Z

fuel-vm/src/interpreter/debug.rs

+    let tx_params = TxParameters::default();
+    let predicate_params = PredicateParameters::default();
+    let script_params = ScriptParameters::default();
+    let contract_params = ContractParameters::default();
+    let fee_params = FeeParameters::default();
+    let chain_id = ChainId::default();
+    let gas_costs = GasCosts::default();


It adds a lot of overhead=) if ConsensusParams works with values instead, you don't need to do it

I missed this one. Adding now.

xgreenx · 2023-07-24T13:30:45Z

fuel-vm/src/util.rs

-                self.params,
-                self.gas_costs.clone(),
-            );
+            let interpreter_params = InterpreterParams {


It is better if Self contains InterpreterParams instead=)

I added the ConsesensusParams to Self and used the into.

Dentosal · 2023-07-25T12:05:20Z

fuel-tx/src/transaction/consensus_parameters.rs

+    // TODO: This should be pub(crate) for the `fuel_tx` tests, but for some reason the
+    // tests can't see the fields with `pub(crate)`


Integration tests cannot see pub(crate) items, as they are outside the crate. Typically you'd do a public re-export behind test-helpers feature flag, but I don't see much point in not having this as a part of the public interface anyways.

Yeah. That's a good point. There are probably places outside of fuel-tx that could use this as well. I was referring to something in fuel-tx/src/tests, not in the integ tests.

I usually prefer getters/setters over accessing fields directly, but I'm not going to worry about it right now.

Dentosal

One todo and some commented out code should be cleaned up, but other than that LGTM

Dentosal · 2023-07-25T12:07:07Z

fuel-vm/src/transactor.rs

+    // /// Consensus parameters
+    // pub const fn params(&self) -> &ConsensusParameters {
+    //     self.interpreter.params()
+    // }
+    //


Commented-out code that likely should be removed

xgreenx reviewed Jul 20, 2023

View reviewed changes

MitchTurner changed the title ~~Draft: Decompose consensus params~~ Decompose consensus params Jul 20, 2023

xgreenx reviewed Jul 24, 2023

View reviewed changes

xgreenx assigned MitchTurner Jul 24, 2023

xgreenx requested a review from a team July 24, 2023 17:17

MitchTurner added 23 commits July 24, 2023 15:43

Break down ConsensusParameters

905441d

Remove ConsensusParameters from everywhere

0f25bc8

Use default for individual fields not for types

a1b10c2

Get tests compiling, but not passing

e42678a

Fix one test

6bdd9a0

Fix bug in transactor default

0554711

Fix bug in test helper

261b4cc

Fix fee test bug

3f30841

Include chain_id in tx builder to get chain_id to match

8171431

Get all tests passing

c17fd88

Partially appease Clippy-sama

d412263

Group params for into_checked

6de4203

Create param for Interpreter init

35a6790

Simplify MemoryClient signature

d134584

Appease Clippy-sama

a655e12

Move ConsensusParams back to original file

52bcef5

Use ConsensusParams in more places

02b7572

Bump version, only use default ChainId

5cf29da

Make ConsensusParams hold values instead of refs

2441d99

Consolidate ChainId and GasCosts into ConsensusParams

8e90661

Collect Interpreter parameters inside struct

1b44647

Fix tests

9cf95c2

Cleanup, appease Clippy-sama

cbb56e4

MitchTurner force-pushed the decompose-consensus-params branch from ab3db20 to cbb56e4 Compare July 24, 2023 22:46

MitchTurner requested a review from xgreenx July 24, 2023 22:50

MitchTurner and others added 2 commits July 24, 2023 16:00

Simplify test setups

6466901

Merge branch 'master' into decompose-consensus-params

415ab30

Dentosal reviewed Jul 25, 2023

View reviewed changes

Dentosal requested changes Jul 25, 2023

View reviewed changes

Remove comment and TODO

5021d87

MitchTurner requested a review from Dentosal July 25, 2023 16:37

Rename ConsensusParams back to ConsensusParameters

1d7350b

Dentosal approved these changes Jul 25, 2023

View reviewed changes

MitchTurner added this pull request to the merge queue Jul 25, 2023

Merged via the queue into master with commit e278038 Jul 25, 2023
30 checks passed

MitchTurner deleted the decompose-consensus-params branch July 25, 2023 17:09

This was referenced Jul 25, 2023

Make ConsensusParameters Serialize/Derserialize again #526

Merged

Decompose consensus params struct FuelLabs/fuel-core#1262

Merged

xgreenx mentioned this pull request Jul 26, 2023

Follow-up clean ups and fixed for the VM initialization #527

Merged

MitchTurner mentioned this pull request Jul 26, 2023

Reimplement with_params method for TxBuilder #530

Merged

xgreenx mentioned this pull request Aug 8, 2023

Release v0.36.0 #545

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Decompose consensus params #514

Decompose consensus params #514

MitchTurner commented Jul 19, 2023 •

edited

Loading

MitchTurner commented Jul 19, 2023

xgreenx Jul 20, 2023

MitchTurner Jul 20, 2023

xgreenx Jul 24, 2023

xgreenx Jul 24, 2023

MitchTurner Jul 24, 2023

xgreenx Jul 24, 2023

MitchTurner Jul 24, 2023

xgreenx Jul 24, 2023

MitchTurner Jul 24, 2023

xgreenx Jul 24, 2023

xgreenx Jul 24, 2023

MitchTurner Jul 24, 2023

xgreenx Jul 24, 2023

xgreenx Jul 24, 2023

xgreenx Jul 24, 2023

xgreenx Jul 24, 2023

MitchTurner Jul 24, 2023

xgreenx Jul 24, 2023

MitchTurner Jul 24, 2023

Dentosal Jul 25, 2023

MitchTurner Jul 25, 2023

Dentosal left a comment

Dentosal Jul 25, 2023

		// TODO: This should be pub(crate) for the `fuel_tx` tests, but for some reason the
		// tests can't see the fields with `pub(crate)`

Decompose consensus params #514

Decompose consensus params #514

Conversation

MitchTurner commented Jul 19, 2023 • edited Loading

MitchTurner commented Jul 19, 2023

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dentosal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MitchTurner commented Jul 19, 2023 •

edited

Loading