test(concurrency): test concurrent fee transfer when the sequencer is…

[meship-starkware]

… the sender

---

This change is 

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.68%. Comparing base (6e06e79) to head (8995b60).
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1963      +/-   ##
==========================================
+ Coverage   78.42%   78.68%   +0.25%     
==========================================
  Files          62       62              
  Lines        8913     8988      +75     
  Branches     8913     8988      +75     
==========================================
+ Hits         6990     7072      +82     
+ Misses       1476     1464      -12     
- Partials      447      452       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[meship-starkware]

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @avi-starkware and @noaov1)

---

crates/blockifier/src/transaction/account_transactions_test.rs line 1302 at r1 (raw file):

            [(sequencer_balance_key_low, sender_balance), (sequencer_balance_key_high, 0_u128)]
        {
            assert!(storage.contains_key(&(fee_token_address, seq_key)));

I think the first assertion is enough because it indicates that we run the sequential fee transfer, and a test already checks it. WDYT?

[avi-starkware]

crates/blockifier/src/transaction/account_transactions_test.rs line 1269 at r1 (raw file):

    block_context.block_info = block_info;

    // Create the account transaction accurding to the fee type.

Suggestion:

    // Create the account transaction according to the fee type.

[avi-starkware]

crates/blockifier/src/transaction/account_transactions_test.rs line 1302 at r1 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

I think the first assertion is enough because it indicates that we run the sequential fee transfer, and a test already checks it. WDYT?

You want to remove the second assertion?

Code snippet:

            assert_eq!(
                *storage.get(&(fee_token_address, seq_key)).unwrap(),
                stark_felt!(seq_value)
            );

[meship-starkware]

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @avi-starkware and @noaov1)

---

crates/blockifier/src/transaction/account_transactions_test.rs line 1302 at r1 (raw file):

Previously, avi-starkware wrote…

You want to remove the second assertion?

Yes.

[avi-starkware]

crates/blockifier/src/transaction/account_transactions_test.rs line 1302 at r1 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

Yes.

Actually, I think we should remove the first assertion. If we are able to compare the value at the key, then that key obviously exists.

[avi-starkware]

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @meship-starkware and @noaov1)

[meship-starkware]

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @avi-starkware and @noaov1)

---

crates/blockifier/src/transaction/account_transactions_test.rs line 1269 at r1 (raw file):

    block_context.block_info = block_info;

    // Create the account transaction accurding to the fee type.

Done.

[avi-starkware]

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @noaov1)

[noaov1]

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @meship-starkware)

---

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

    max_fee: Fee,
    #[values(FeeType::Eth, FeeType::Strk)] fee_type: FeeType,
) {

Suggestion:

#[case::tx_version_1(TransactionVersion::ONE, FeeType::Eth)]
#[case::tx_version_3(TransactionVersion::THREE, FeeType::Strk)]
fn test_concurrent_fee_transfer_when_sender_is_sequencer(
    #[case] version: TransactionVersion,
    #[case] fee_type: FeeType,
) {

---

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

    // Set the sequencer as the sender.
    block_info.sequencer_address = account_address;
    block_context.block_info = block_info;

Why is it needed?
Why not modifying directly the block context?

Suggestion:

    // Set the sequencer as the sender.
    block_context.block_info.sequencer_address = account_address;

---

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

            version: TransactionVersion::THREE
        })
    };

Note that it knows which resource bound to use according to the transaction version.
You need to declare and deploy the test contract.

Suggestion:

    let account_tx = account_invoke_tx(invoke_tx_args! {
            max_fee,
            sender_address: account_address,
            calldata: create_trivial_calldata(test_contract.get_instance_address(0)),
            version: TransactionVersion::ONE,
            resource_bounds: l1_resource_bounds(MAX_L1_GAS_AMOUNT, MAX_L1_GAS_PRICE),
            version
        })
    };

---

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

    let chain_info = &block_context.chain_info;
    let fee_token_address = block_context.chain_info.fee_token_address(&fee_type);
    let state = &mut test_state(chain_info, sender_balance, &[(account, 1)]);

Better to define it at the beginning (see other tests)

Code quote:

    let chain_info = &block_context.chain_info;
    let fee_token_address = block_context.chain_info.fee_token_address(&fee_type);
    let state = &mut test_state(chain_info, sender_balance, &[(account, 1)]);

---

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

        get_sequencer_balance_keys(&block_context);
    let mut transactional_state = TransactionalState::create_transactional(state);
    account_tx.execute_raw(&mut transactional_state, &block_context, true, false).unwrap();

Please define variables to the flags.
Why validate=false? Does it matter?

Suggestion:

    account_tx.execute(&mut state, &block_context, true, false).unwrap();

---

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

            );
        }
    }

Will reading directly from the state be enough? 🤔

Code quote:

    for storage in [
        transactional_cache.initial_reads.storage.clone(),
        transactional_cache.writes.storage.clone(),
    ] {
        for (seq_key, seq_value) in
            [(sequencer_balance_key_low, sender_balance), (sequencer_balance_key_high, 0_u128)]
        {
            assert_eq!(
                *storage.get(&(fee_token_address, seq_key)).unwrap(),
                stark_felt!(seq_value)
            );
        }
    }

[meship-starkware]

Reviewable status: 0 of 1 files reviewed, 4 unresolved discussions (waiting on @avi-starkware and @noaov1)

---

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

Previously, noaov1 (Noa Oved) wrote…

Why is it needed?
Why not modifying directly the block context?

Done.

---

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

Previously, noaov1 (Noa Oved) wrote…

Note that it knows which resource bound to use according to the transaction version.
You need to declare and deploy the test contract.

Done.

---

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

Previously, noaov1 (Noa Oved) wrote…

Please define variables to the flags.
Why validate=false? Does it matter?

No, it's a mistake. Validate should be true.
Done.

---

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

Previously, noaov1 (Noa Oved) wrote…

Will reading directly from the state be enough? 🤔

I don't think I understand the question.

[noaov1]

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @meship-starkware)

---

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

Previously, meship-starkware (Meshi Peled) wrote…

I don't think I understand the question.

I meant:

Code snippet:

for (seq_key, seq_value) in
            [(sequencer_balance_key_low, sender_balance), (sequencer_balance_key_high, 0_u128)]
        {
            assert_eq!(
                state.get_storage_at(fee_token_address, seq_key).unwrap(),
                stark_felt!(seq_value)
            );
        }

---

crates/blockifier/src/transaction/account_transactions_test.rs line 1278 at r3 (raw file):

    let (sequencer_balance_key_low, sequencer_balance_key_high) =
        get_sequencer_balance_keys(&block_context);
    // Create the account transaction according to the fee type.

Please remove

Code quote:

    // Create the account transaction according to the fee type.

[meship-starkware]

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @noaov1)

---

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

Previously, noaov1 (Noa Oved) wrote…

I meant:

Done.

[noaov1]

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @meship-starkware)

[noaov1]

Reviewed 1 of 1 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @meship-starkware)

---

crates/blockifier/src/transaction/account_transactions_test.rs line 1171 at r6 (raw file):

    // sequencer_balance_key_high.
    const STORAGE_WRITE_LOW: u128 = 100;
    const STORAGE_READ_LOW: u128 = 50;

Suggestion:

    const TRANSFER_AMOUNT: u128 = 100;
    const SEQUENCER_BALANCE_LOW_INITIAL u128 = 50;

---

crates/blockifier/src/transaction/account_transactions_test.rs line 1184 at r6 (raw file):

        calldata: create_trivial_calldata(test_contract.get_instance_address(0)),
        resource_bounds: l1_resource_bounds(MAX_L1_GAS_AMOUNT, MAX_L1_GAS_PRICE),
        version: TransactionVersion::THREE

Please add the version as an input parameter (to fit the fee type) or use version only (see my comment below)

Code quote:

        version: TransactionVersion::THREE

---

crates/blockifier/src/transaction/account_transactions_test.rs line 1265 at r6 (raw file):

#[rstest]
#[case::tx_version_1(TransactionVersion::ONE, FeeType::Eth)]
#[case::tx_version_3(TransactionVersion::THREE, FeeType::Strk)]

Can you deduce the fee type from the transaction directly?

Code quote:

#[case::tx_version_1(TransactionVersion::ONE, FeeType::Eth)]
#[case::tx_version_3(TransactionVersion::THREE, FeeType::Strk)]

---

crates/blockifier/src/transaction/account_transactions_test.rs line 1296 at r6 (raw file):

        account_tx.execute(&mut transactional_state, &block_context, charge_fee, validate).unwrap();
    assert!(!result.is_reverted());
    // Check that the sequencer balance was not changed.

Suggestion:

    // Check that the sequencer balance was updared (in this case, was not changed).

[meship-starkware]

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @noaov1)

---

crates/blockifier/src/transaction/account_transactions_test.rs line 1184 at r6 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Please add the version as an input parameter (to fit the fee type) or use version only (see my comment below)

Done.

[avi-starkware]

crates/blockifier/src/transaction/account_transactions_test.rs line 1247 at r7 (raw file):

    assert!(!result.is_reverted());

    // Check that the sequencer balance was updared (in this case, was not changed).

Suggestion:

    // Check that the sequencer balance was updated (in this case, was not changed).

[noaov1]

Reviewed 1 of 2 files at r7, 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @meship-starkware)

---

crates/blockifier/src/transaction/account_transactions_test.rs line 1173 at r8 (raw file):

    // sequencer_balance_key_high.
    const STORAGE_WRITE_LOW: u128 = 100;
    const STORAGE_READ_LOW: u128 = 50;

Are those constants needed now?

Code quote:

    const STORAGE_WRITE_LOW: u128 = 100;
    const STORAGE_READ_LOW: u128 = 50;

---

crates/blockifier/src/transaction/account_transactions_test.rs line 1181 at r8 (raw file):

    let test_contract = FeatureContract::TestContract(CairoVersion::Cairo0);
    let chain_info = &block_context.chain_info;
    let fee_type = if version == TransactionVersion::ONE { FeeType::Eth } else { FeeType::Strk };

Same below

Suggestion:

    let fee_type = &account_tx.fee_type();

---

crates/blockifier/src/transaction/account_transactions_test.rs line 1247 at r8 (raw file):

    assert!(!result.is_reverted());

    // Check that the sequencer balance was updated (in this case, was not changed).

Right?

Suggestion:

// Check that the sequencer balance was not updated.

---

crates/blockifier/src/transaction/account_transactions_test.rs line 1305 at r8 (raw file):

        account_tx.execute(&mut transactional_state, &block_context, charge_fee, validate).unwrap();
    assert!(!result.is_reverted());
    // Check that the sequencer balance was not changed.

Suggestion:

// Check that the sequencer balance was updated (in this case, was not changed).

[meship-starkware]

Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @noaov1)

---

crates/blockifier/src/transaction/account_transactions_test.rs line 1181 at r8 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Same below

Done.

---

crates/blockifier/src/transaction/account_transactions_test.rs line 1247 at r8 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Right?

Done.

[noaov1]

Reviewed 1 of 1 files at r9, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @meship-starkware)