Implemented update of `balance_root` and `state_root` for inputs and outputs #789

xgreenx · 2022-11-25T17:24:54Z

The change actualizes the balance_root and state_root for inputs before execution and outputs after execution. It only preparation of the code to do those updates with the current dummy algorithm. The correct algorithm for state calculation should be implemented in #627 and #626.

Also, it is preparation for updating tx_pointer. Because it requires an additional field in the database, it should be done in a separate PR.

Minor changes:

Created refs folder in the fuel-executor with a wrapper to minimize the amount of code.
Added tx_status to the ExecutionResult, preparation for the Move writing transaction status db writing into txpool and add full status to subscription #772 and more advanced testing.
Moved tx_pool::TransactionStatus to fuel-core-interfaces.

CC @bvrooman This change should answer your question in the related issues.

xgreenx · 2022-11-25T17:35:30Z

fuel-core/src/executor.rs

-                        return Err(Error::InvalidTransactionOutcome {
-                            transaction_id: tx.id(),
-                        })
+    // TODO: Maybe we need move it to `fuel-vm`? O_o Because other `Outputs` are processed there


I'm unsure about this part. fuel-vm already updates outputs for all utxos. Maybe better to move it there or extract all updates here.

fuel-vm could set the balance and state roots, but it doesn't manage utxo id's or utxo state directly. This is mainly to enable the client to do bulk optimization for utxo lookups and validation across many txs at once in parallel.

edit: however in the case of contract output utxo ids, the vm should be able to calculate those as well without interacting with any db state, so moving this to the VM may be the right call (and would probably make fraud proof setup easier).

xgreenx · 2022-11-25T17:37:41Z

fuel-core/src/executor.rs

+            .outputs_mut()
+            .last_mut()
+        {
+            *to = Address::try_from(contract_id.as_ref()).unwrap();


@Voxelot I added this test but not sure what our expectation is. Maybe we want to reject transactions that try sending tokens to the contract via outputs(not the script body).

Hmm yeah, we could do this but it would add extra overhead to every transfer to check if the output address is a contract id.

cc @adlerjohn

Voxelot · 2022-11-29T22:58:18Z

fuel-block-executor/src/refs/contract.rs

+/// The wrapper around `contract_id` to simplify work with `Contract` in the database.
+pub struct ContractRef<'a, Database> {
+    database: Database,
+    contract_id: &'a ContractId,


do we really need to lifetime reference this? the ContractId implements copy and should be fairly cheap to own here 🤔

I added this type to simplify usage without affecting performance. Because the type is public and used inside of another crate, the compiler will not optimize usage of this struct, and it will add a smaaaaaaaaaaaaaall overhead(to copy 32 bytes)=)

If this lifetime causes pain to your eyes, I will remove it=D

While copying does incur a small overhead, dereferencing is also not free from a cache standpoint. I also think the simpler struct definition we get may be worth the cost of copying the Bytes32 .

I don't know why we are afraid of lifetimes=) But if two people don't like it, I will replace it with four copies.

I know it's more efficient to use the lifetimes, but my preference is to try and keep the code as simple as possible until it's proven to be a bottleneck.

Rather than a question of lifetimes, I think (if I'm understanding Brandon's original comment correctly) it's more of a question about when to use references vs. when to use copies. Personally, I think IDs (similar to ints) are something you should be able to copy without much fear of overhead. But I can also understand that in a performance-critical application, avoiding copies is a good general goal. (But there is the potential cost of accumulating cache misses if this is in any sort of loop!)

…outputs

bvrooman · 2022-12-02T16:32:23Z

fuel-core/src/executor.rs

+        let expected_utxo_id = |contract: &ContractRef<&mut Database>| {
+            let maybe_utxo_id = contract.utxo()?.map(|utxo| utxo.into_owned());
            let expected_utxo_id = if self.config.utxo_validation {
                maybe_utxo_id
-                    .ok_or(Error::ContractUtxoMissing(*contract_id))?
-                    .into_owned()
+                    .ok_or_else(|| Error::ContractUtxoMissing(*contract.contract_id()))?
            } else {
-                maybe_utxo_id.unwrap_or_default().into_owned()
+                maybe_utxo_id.unwrap_or_default()
            };
            Result::<_, Error>::Ok(expected_utxo_id)
        };


Instead of a lambda, can expected_utxo_id be replaced by a getter on the ContractRef interface? If that is practical, I think it would make the readability a bit better and consistent:

let mut contract = ContractRef::new(&mut *db, contract_id); *utxo_id = contract.utxo_id()?; *balance_root = contract.balance_root()?; *state_root = contract.state_root()?;

I tried to avoid any business logic inside of those wrappers=)

Good point. I am not strongly opinionated either way. As usual with software, there are pros and cons to either approach!

bvrooman · 2022-12-02T16:35:47Z

fuel-core/src/executor.rs

@@ -2449,6 +2574,281 @@ mod tests {
        assert_eq!(coin.status, CoinStatus::Spent);
    }

+    #[test]


Just an aside comment: now that we have so many lines of tests in this file, it may be worth extracting the test suite to its own file.

We will, but in a separate PR. executor.rs should live in the fuel-executor, and during this refactoring, we will definitely split the logic across several files and the same with tests.

Voxelot · 2022-12-06T02:52:55Z

fuel-core/src/executor.rs

            // Needed to convince the compiler that tx is taken by ref here
            #[allow(clippy::needless_borrow)]


Does this clippy annotation still apply?

No, we can remove

Voxelot · 2022-12-06T02:56:51Z

fuel-core/src/executor.rs

            *header.height(),
            &tx_id,
            tx_db_transaction.deref_mut(),
            vm_result.tx().inputs(),
            vm_result.tx().outputs(),
        )?;
+        self.compute_outputs(


It looks like we're persisting the outputs before we compute their state/balance roots etc. It seems like this should've been caught in tests if we're checking that the balance / state roots on contract outputs are set and persisted correctly 🤔 .

We have a test but without storage check=( persist_output_utxos persist only outputs related to utxo=) It is the expected ordering of methods. Added a comment to make it clear

Voxelot · 2022-12-06T03:03:13Z

fuel-core/src/executor.rs

+        } = executor.execute(ExecutionBlock::Production(block)).unwrap();
+
+        let empty_state = Bytes32::from(*empty_sum());
+        let executed_tx = block.transactions()[2].as_script().unwrap();


we should also be testing:

that the tx in the db matches the tx in the block

that utxos in the db match the expected state, since those are distinct from the output types on the tx

related: https://github.com/FuelLabs/fuel-core/pull/789/files#r1040374277

Voxelot

blocking the previous approval due to bug in utxo persistence

Added check of the storage into unit test. Added comment to highlight the ordering of operations.

not a bug

Voxelot · 2022-12-06T03:31:54Z

Ahh that's right, these output calculations aren't persisted to the utxo state set. They are just metadata on the tx itself. Thanks for clarifying.

ControlCplusControlV · 2022-12-06T03:39:05Z

fuel-core-interfaces/src/executor.rs

@@ -50,6 +51,14 @@ pub struct ExecutionResult {
    /// The list of skipped transactions with corresponding errors. Those transactions were
    /// not included in the block and didn't affect the state of the blockchain.
    pub skipped_transactions: Vec<(Transaction, Error)>,
+    /// The status of the transactions execution included into the block.


[nit] I think this sentence might need to be adjusted along the lines of "The status of executed transactions included in the block"

ControlCplusControlV

LGTM

xgreenx requested a review from a team November 25, 2022 17:24

xgreenx self-assigned this Nov 25, 2022

xgreenx commented Nov 25, 2022

View reviewed changes

Voxelot reviewed Nov 29, 2022

View reviewed changes

xgreenx force-pushed the feature/move-tabels-to-interfaces branch from c69eee9 to d2a9dd0 Compare November 30, 2022 23:43

Base automatically changed from feature/move-tabels-to-interfaces to master December 1, 2022 00:45

xgreenx added 5 commits December 2, 2022 01:48

Moved all tablets into fuel-core-interfaces/src/db.

4c760e0

Return back balances.rs

2bc95aa

Implemented update of balance_root and state_root for inputs and …

540ba7e

…outputs

Make clippy happy

d421e47

Make CI happy

17b3214

xgreenx force-pushed the feature/update-state-and-balances-root branch from bac9d86 to 17b3214 Compare December 2, 2022 02:02

Forgot to commit after rebase

2b347ce

bvrooman previously approved these changes Dec 2, 2022

View reviewed changes

bvrooman requested a review from a team December 2, 2022 16:40

Applied suggestions from the review

b6ff9c7

xgreenx dismissed bvrooman’s stale review via b6ff9c7 December 2, 2022 17:21

Make clippy happy

d8bb31f

bvrooman previously approved these changes Dec 2, 2022

View reviewed changes

bvrooman requested a review from a team December 2, 2022 20:03

xgreenx requested a review from Voxelot December 3, 2022 15:38

xgreenx added 2 commits December 5, 2022 21:00

Merge branch 'master' into feature/update-state-and-balances-root

ad8ac89

Merge branch 'master' into feature/update-state-and-balances-root

1e0619e

Voxelot reviewed Dec 6, 2022

View reviewed changes

Voxelot previously requested changes Dec 6, 2022

View reviewed changes

xgreenx added 2 commits December 6, 2022 03:14

Merge branch 'master' into feature/update-state-and-balances-root

d45db74

Fix TODO after merge of the genesis change.

854ca9b

Added check of the storage into unit test. Added comment to highlight the ordering of operations.

xgreenx dismissed bvrooman’s stale review via 854ca9b December 6, 2022 03:22

Voxelot approved these changes Dec 6, 2022

View reviewed changes

ControlCplusControlV reviewed Dec 6, 2022

View reviewed changes

ControlCplusControlV approved these changes Dec 6, 2022

View reviewed changes

xgreenx merged commit bad93d1 into master Dec 6, 2022

xgreenx deleted the feature/update-state-and-balances-root branch December 6, 2022 11:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented update of `balance_root` and `state_root` for inputs and outputs #789

Implemented update of `balance_root` and `state_root` for inputs and outputs #789

xgreenx commented Nov 25, 2022 •

edited

Loading

xgreenx Nov 25, 2022

Voxelot Nov 29, 2022 •

edited

Loading

xgreenx Nov 25, 2022

Voxelot Dec 6, 2022 •

edited

Loading

Voxelot Nov 29, 2022

xgreenx Dec 2, 2022

bvrooman Dec 2, 2022

xgreenx Dec 2, 2022

Voxelot Dec 2, 2022

bvrooman Dec 2, 2022 •

edited

Loading

bvrooman Dec 2, 2022

xgreenx Dec 2, 2022

bvrooman Dec 2, 2022

bvrooman Dec 2, 2022

xgreenx Dec 2, 2022

Voxelot Dec 6, 2022

xgreenx Dec 6, 2022

Voxelot Dec 6, 2022 •

edited

Loading

xgreenx Dec 6, 2022 •

edited

Loading

Voxelot Dec 6, 2022 •

edited

Loading

Voxelot left a comment •

edited

Loading

Voxelot commented Dec 6, 2022

ControlCplusControlV Dec 6, 2022

ControlCplusControlV left a comment

		// Needed to convince the compiler that tx is taken by ref here
		#[allow(clippy::needless_borrow)]

Implemented update of balance_root and state_root for inputs and outputs #789

Implemented update of balance_root and state_root for inputs and outputs #789

Conversation

xgreenx commented Nov 25, 2022 • edited Loading

Choose a reason for hiding this comment

Voxelot Nov 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Voxelot Dec 6, 2022 • 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 Dec 2, 2022 • 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

Voxelot Dec 6, 2022 • edited Loading

Choose a reason for hiding this comment

xgreenx Dec 6, 2022 • edited Loading

Choose a reason for hiding this comment

Voxelot Dec 6, 2022 • edited Loading

Choose a reason for hiding this comment

Voxelot left a comment • edited Loading

Choose a reason for hiding this comment

Voxelot commented Dec 6, 2022

Choose a reason for hiding this comment

ControlCplusControlV left a comment

Choose a reason for hiding this comment

Implemented update of `balance_root` and `state_root` for inputs and outputs #789

Implemented update of `balance_root` and `state_root` for inputs and outputs #789

xgreenx commented Nov 25, 2022 •

edited

Loading

Voxelot Nov 29, 2022 •

edited

Loading

Voxelot Dec 6, 2022 •

edited

Loading

bvrooman Dec 2, 2022 •

edited

Loading

Voxelot Dec 6, 2022 •

edited

Loading

xgreenx Dec 6, 2022 •

edited

Loading

Voxelot Dec 6, 2022 •

edited

Loading

Voxelot left a comment •

edited

Loading