Add a query version bit.

[noaov1]

This change is 

[dorimedini-starkware]

Reviewed 6 of 10 files at r1, 4 of 4 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @elintul, @giladchase, and @noaov1)

---

crates/blockifier/src/transaction/account_transaction.rs line 160 at r1 (raw file):

        let mut version = self.version();
        if simulate {
            let query_version_base = BigUint::from(2_u8).pow(QUERY_VERSION_BASE_BIT);

can this be constant?
or at least lazy_static?

Code quote:

BigUint::from(2_u8).pow(QUERY_VERSION_BASE_BIT);

---

crates/blockifier/src/transaction/account_transactions_test.rs line 103 at r2 (raw file):

    );

    let account_tx = AccountTransaction::DeployAccount(deploy_account_tx);

can you add a test where the simulate bit is true?
enough to parametrize an existing test... it shouldn't effect the test itself IIUC

---

crates/blockifier/src/transaction/transactions_test.rs line 1121 at r2 (raw file):

    let account_tx = AccountTransaction::Invoke(InvokeTransaction::V1(invoke_tx));

    account_tx.execute(state, block_context, true, true, true).unwrap();

anything to check on the resulting execution info? is the version written there somewhere?
also, please add assert!(!tx_execution_info.is_reverted())

[noaov1]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware, @elintul, and @giladchase)

---

crates/blockifier/src/transaction/account_transaction.rs line 160 at r1 (raw file):

Previously, dorimedini-starkware wrote…

can this be constant?
or at least lazy_static?

It can be lazy_static but that requires adding a dependency in the Cargo.toml (once_cell).
I wanted to avoid that in this PR (added a TODO above the constant) but can do it if you think it's necessary.

---

crates/blockifier/src/transaction/account_transactions_test.rs line 103 at r2 (raw file):

Previously, dorimedini-starkware wrote…

can you add a test where the simulate bit is true?
enough to parametrize an existing test... it shouldn't effect the test itself IIUC

Done

---

crates/blockifier/src/transaction/transactions_test.rs line 1121 at r2 (raw file):

Previously, dorimedini-starkware wrote…

anything to check on the resulting execution info? is the version written there somewhere?
also, please add assert!(!tx_execution_info.is_reverted())

I tested that the output of the syscall contains the correct version (see expected_tx_info).
Done.
Pre-PR.

[giladchase]

Reviewable status: 7 of 10 files reviewed, 4 unresolved discussions (waiting on @dorimedini-starkware, @elintul, and @noaov1)

---

crates/blockifier/src/transaction/account_transaction.rs line 160 at r1 (raw file):

Previously, dorimedini-starkware wrote…

can this be constant?
or at least lazy_static?

+1
But plz use once_cell::sync::Lazy since lazy_static is deprecated

---

crates/blockifier/src/transaction/account_transaction.rs line 153 at r2 (raw file):

                InvokeTransaction::V0(_) => TransactionVersion(StarkFelt::from(0_u8)),
                InvokeTransaction::V1(_) => TransactionVersion(StarkFelt::from(1_u8)),
            },

Export to SN API? That is, add aversion() method to InvokeTransaction that'll basically be identical to the one in DeclareTransaction (not sure if it's possible to do this without duplicating the version() logic)

Code quote:

            Self::Invoke(tx) => match tx {
                InvokeTransaction::V0(_) => TransactionVersion(StarkFelt::from(0_u8)),
                InvokeTransaction::V1(_) => TransactionVersion(StarkFelt::from(1_u8)),
            },

---

crates/blockifier/src/transaction/account_transaction.rs line 211 at r3 (raw file):

            }
            false => version,
        };

IMO one of those rare cases where if-else is better (also shorter) than match

Suggestion:

        let version = if simulate {
            let simulate_version_base = BigUint::from(2_u8).pow(SIMULATE_VERSION_BASE_BIT);
            let adjusted_version = felt_to_biguint(version.0) - simulate_version_base;
            TransactionVersion(
                biguint_to_felt(adjusted_version).expect("The version should be a field element."),
            )
        } else {
            version
        };

[dorimedini-starkware]

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @elintul and @noaov1)

[giladchase]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @elintul and @noaov1)

---

crates/blockifier/src/transaction/account_transaction.rs line 458 at r3 (raw file):

        mut execution_context: EntryPointExecutionContext,
        validate: bool,
        simulate: bool,

If we're already passing multiple context args everywhere, we might as well initialize AccountTransactionExecutionContext "at the root level" and pass it around (is it immutable?).
@noaov1 Is that possible?

This aligns well with our refactoring plans.
(I'm talking about a follow-up PR here, this shouldn't hold up the current one.)

WDYT @noaov1 @dorimedini-starkware @elintul ?

Code quote:

        validate: bool,
        simulate: bool,

[dorimedini-starkware]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @elintul and @noaov1)

---

crates/blockifier/src/transaction/account_transaction.rs line 458 at r3 (raw file):

Previously, giladchase wrote…

If we're already passing multiple context args everywhere, we might as well initialize AccountTransactionExecutionContext "at the root level" and pass it around (is it immutable?).
@noaov1 Is that possible?

This aligns well with our refactoring plans.
(I'm talking about a follow-up PR here, this shouldn't hold up the current one.)

WDYT @noaov1 @dorimedini-starkware @elintul ?

you are planning to remove get_account_transaction_context()?
what refactor are you talking about?
not in context to have an opinion here

[giladchase]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @elintul and @noaov1)

---

crates/blockifier/src/transaction/account_transaction.rs line 458 at r3 (raw file):

you are planning to remove get_account_transaction_context()?

Yep, having to initialize it at the onset of every function isn't ideal (duplication/boilerplate).

what refactor are you talking about?

Haven't settled on anything yet, at the very least we can split it into smaller ctx objects (this is a non-trivial task since it is passed around a lot).

Anyhow, this doesn't have to be agreed upon at this PR, i just mentioned it to provide additional context.
In other words, i think passing around the instance is preferable to passing a bunch of boolean args for it initialization, even if the refactor i mentioned doesn't end up happening.

---

crates/blockifier/src/transaction/test_utils.rs line 88 at r3 (raw file):

        account_balance,
        ContractClassV0::from_file(TEST_CONTRACT_CAIRO0_PATH).into(),
    )

Let's add a create_cairo0_account_tx_test_state wrapper which has a hardcoded TEST_CONTRACT_CAIRO0_PATH, and similarly for the CAIRO1 usage you added

Code quote:

    create_account_tx_test_state(
        ContractClassV0::from_file(ACCOUNT_CONTRACT_CAIRO0_PATH).into(),
        TEST_ACCOUNT_CONTRACT_CLASS_HASH,
        TEST_ACCOUNT_CONTRACT_ADDRESS,
        test_erc20_account_balance_key(),
        account_balance,
        ContractClassV0::from_file(TEST_CONTRACT_CAIRO0_PATH).into(),
    )

---

crates/blockifier/src/transaction/transaction_utils.rs line 68 at r3 (raw file):

}

// TODO: Convert to a `TryFrom` cast and put in starknet-api (In StarkFelt).

Ditto for the second comment

Suggestion:

// TODO: Convert to a `TryFrom` cast and put in starknet-api (In StarkFelt), gated behind `num-bigint` feature (for the sake of no-std users).

---

crates/blockifier/src/transaction/transactions_test.rs line 1130 at r3 (raw file):

            .concat()
            .into(),
    );

can you use calldata! here?

Code quote:

    let execute_calldata = vec![
        stark_felt!(TEST_CONTRACT_ADDRESS), // Contract address.
        entry_point_selector.0,             // EP selector.
        stark_felt!(calldata_len as u64),   // Calldata length.
    ];
    let execute_calldata = Calldata(
        [execute_calldata, expected_block_info.to_vec(), expected_tx_info, expected_call_info]
            .concat()
            .into(),
    );

[dorimedini-starkware]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @elintul and @noaov1)