feat: support skip_validate in mempool

[Yael-Starkware]

This change is 

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.61%. Comparing base (7c08d83) to head (acc6ff5).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #376      +/-   ##
==========================================
+ Coverage   82.89%   83.61%   +0.71%     
==========================================
  Files          37       37              
  Lines        1719     1733      +14     
  Branches     1719     1733      +14     
==========================================
+ Hits         1425     1449      +24     
+ Misses        217      207      -10     
  Partials       77       77              

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

[dafnamatsry]

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @ArniStarkware and @Yael-Starkware)

a discussion (no related file):
Please add @elintul or @giladchase

---

---

crates/mempool/src/transaction_pool.rs line 24 at r1 (raw file):

impl TransactionPool {
    pub fn insert(&mut self, tx: ThinTransaction, account_nonce: Nonce) -> MempoolResult<()> {

I think insert should do only and insert without any other checks.

Please move this check outside of the TransactionPool.

Code quote:

 pub fn insert

---

crates/mempool/src/transaction_pool.rs line 52 at r1 (raw file):

        tx: &ThinTransaction,
        account_nonce: Nonce,
    ) -> MempoolResult<()> {

Transaction pool is a data structure for transactions and as such should not implement the "business logic".
You can implement has_deploy_account_tx(sender_addrees) instead.

Code quote:

    pub fn account_deployed(
        &self,
        tx: &ThinTransaction,
        account_nonce: Nonce,
    ) -> MempoolResult<()> {

---

crates/mempool/src/transaction_pool.rs line 55 at r1 (raw file):

        // If tx nonce = 1 and account nonce = 0, the gateway have skipped validations and this
        // means that the account may not have been deployed yet and we need to check it now.
        if tx.nonce == Nonce(StarkFelt::ONE) && account_nonce == Nonce(StarkFelt::ZERO) {

Note that in the GW we have this nonce configurable. The same should be here.
Alternatively, you can add maybe a boolean as an input to the mempool to check for DeployAccount tx.

Code quote:

tx.nonce == Nonce(StarkFelt::ONE)

[elintul]

Reviewed all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @ArniStarkware and @Yael-Starkware)

---

crates/mempool/src/mempool_test.rs line 21 at r1 (raw file):

/// 2. add_tx_input!(tip: 1, tx_hash: StarkFelt::TWO, address: "0x3")
/// 3. add_tx_input!(tip:1 , tx_hash: StarkFelt::TWO)
macro_rules! add_tx_input {

Revert whitespace changes, make sure your formatter is configured correctly?

Code quote:

add_tx_input

[elintul]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @ArniStarkware, @dafnamatsry, and @Yael-Starkware)

---

crates/mempool/src/transaction_pool.rs line 24 at r1 (raw file):

Previously, dafnamatsry wrote…

I think insert should do only and insert without any other checks.

Please move this check outside of the TransactionPool.

I agree, this check it out of scope for the pool itself and is part of the mempool logic.

[elintul]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @ArniStarkware, @dafnamatsry, and @Yael-Starkware)

---

crates/mempool/src/transaction_pool.rs line 55 at r1 (raw file):

Previously, dafnamatsry wrote…

Note that in the GW we have this nonce configurable. The same should be here.
Alternatively, you can add maybe a boolean as an input to the mempool to check for DeployAccount tx.

Q: why is it configurable? Is there a (future) usecase where nonce > 1 for this feature?

[elintul]

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @ArniStarkware, @dafnamatsry, and @Yael-Starkware)

---

crates/mempool/src/transaction_pool.rs line 61 at r1 (raw file):

                .unwrap_or(&BTreeMap::new())
                .get(&Nonce(StarkFelt::ZERO))
                .ok_or(MempoolError::UndeployedAccount { sender_address: tx.sender_address })?;

This isn't an error that indicates a negative flow, why not return a boolean?

Code quote:

UndeployedAccount

[elintul]

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @ArniStarkware, @dafnamatsry, and @Yael-Starkware)

---

crates/mempool/src/transaction_pool.rs line 48 at r1 (raw file):

    }

    pub fn account_deployed(

Suggestion:

account_was_deployed

---

crates/mempool/src/transaction_pool.rs line 61 at r1 (raw file):

                .unwrap_or(&BTreeMap::new())
                .get(&Nonce(StarkFelt::ZERO))
                .ok_or(MempoolError::UndeployedAccount { sender_address: tx.sender_address })?;

Seems like AccountTransactionIndex is missing an API we need. :)
Please add get(&self, address: ContractAddress, nonce: Nonce) -> Option<&TransactionReference> to it in a separate PR.

Code quote:

            self.txs_by_account
                .0
                .get(&tx.sender_address)
                .unwrap_or(&BTreeMap::new())
                .get(&Nonce(StarkFelt::ZERO))
                .ok_or(MempoolError::UndeployedAccount { sender_address: tx.sender_address })?;

[elintul]

Reviewed 3 of 4 files at r1.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @ArniStarkware, @dafnamatsry, and @Yael-Starkware)

[elintul]

Reviewed 1 of 4 files at r1.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @ArniStarkware, @dafnamatsry, and @Yael-Starkware)

[Yael-Starkware]

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @ArniStarkware, @dafnamatsry, and @elintul)

---

crates/mempool/src/mempool_test.rs line 21 at r1 (raw file):

Previously, elintul (Elin) wrote…

Revert whitespace changes, make sure your formatter is configured correctly?

done.

---

crates/mempool/src/transaction_pool.rs line 24 at r1 (raw file):

Previously, elintul (Elin) wrote…

I agree, this check it out of scope for the pool itself and is part of the mempool logic.

done.

---

crates/mempool/src/transaction_pool.rs line 48 at r1 (raw file):

    }

    pub fn account_deployed(

changed the name to has_deploy_account_tx , since the account was actually not deployed yet.

---

crates/mempool/src/transaction_pool.rs line 52 at r1 (raw file):

Previously, dafnamatsry wrote…

Transaction pool is a data structure for transactions and as such should not implement the "business logic".
You can implement has_deploy_account_tx(sender_addrees) instead.

Done.

---

crates/mempool/src/transaction_pool.rs line 55 at r1 (raw file):

Previously, elintul (Elin) wrote…

Q: why is it configurable? Is there a (future) usecase where nonce > 1 for this feature?

discussed with @dafnamatsry and I think we can remove the configuration

---

crates/mempool/src/transaction_pool.rs line 61 at r1 (raw file):

Previously, elintul (Elin) wrote…

This isn't an error that indicates a negative flow, why not return a boolean?

it is a negative flow if invoke doesn't have a deploy account tx.

---

crates/mempool/src/transaction_pool.rs line 61 at r1 (raw file):

Previously, elintul (Elin) wrote…

Seems like AccountTransactionIndex is missing an API we need. :)
Please add get(&self, address: ContractAddress, nonce: Nonce) -> Option<&TransactionReference> to it in a separate PR.

added in a separate PR
https://reviewable.io/reviews/starkware-libs/mempool/402

[elintul]

Reviewed 1 of 3 files at r3, all commit messages.
Reviewable status: 2 of 5 files reviewed, 6 unresolved discussions (waiting on @ArniStarkware, @dafnamatsry, and @Yael-Starkware)

[elintul]

Reviewable status: 2 of 5 files reviewed, 6 unresolved discussions (waiting on @ArniStarkware, @dafnamatsry, and @Yael-Starkware)

---

crates/mempool/src/transaction_pool.rs line 61 at r1 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

it is a negative flow if invoke doesn't have a deploy account tx.

Does the user have something to do with the error, or do we simply match over Ok/Err in our internal business logic? If the latter, we should return a boolean from this method.

[dafnamatsry]

Reviewed 1 of 4 files at r2, 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @ArniStarkware, @elintul, and @Yael-Starkware)

---

crates/mempool/src/mempool.rs line 92 at r3 (raw file):

    }

    pub fn has_deploy_account_tx(

has implies that it returns a boolean. but it return a Result. Consider verify instead.
Plus, we do the verification only in a specific case, and the name should indicate that.

Code quote:

has_deploy_account_tx

---

crates/mempool/src/mempool.rs line 98 at r3 (raw file):

    ) -> MempoolResult<()> {
        // If tx nonce = 1 and account nonce = 0, the gateway have skipped validations and this
        // means that the account may not have been deployed yet and we need to check it now.

Suggestion:

        // If the tx nonce is 1 and the account nonce is 0, the account was not deployed yet, and the gateway has skipped validations.
        // In this case, we verify that a deploy account tx exists in the pool.

---

crates/mempool/src/mempool.rs line 100 at r3 (raw file):

        // means that the account may not have been deployed yet and we need to check it now.
        // TODO(Yael 8/7/2024): This is only relevant for invoke tx, check the type once available.
        if tx.nonce == Nonce(Felt::ONE) && account_nonce == Nonce(Felt::ZERO) {

Please move this check to a common crate (I think we should crate one), as it appears both here and in the GW.

(In a separate PR)

Code quote:

 tx.nonce == Nonce(Felt::ONE) && account_nonce == Nonce(Felt::ZERO)

[Yael-Starkware]

Reviewable status: 3 of 5 files reviewed, 10 unresolved discussions (waiting on @ArniStarkware, @dafnamatsry, @elintul, and @giladchase)

---

crates/mempool/src/mempool.rs line 83 at r3 (raw file):

        let tx = input.tx;
        let tx_reference = TransactionReference::new(&tx);
        self.has_deploy_account_tx(&tx, input.account.state.nonce)?;

@giladchase do you think this line should appear here or in add_tx() ?

Code quote:

has_deploy_account_tx

[elintul]

Reviewed 1 of 4 files at r2, all commit messages.
Reviewable status: 3 of 5 files reviewed, 10 unresolved discussions (waiting on @ArniStarkware, @dafnamatsry, and @giladchase)

[giladchase]

Reviewable status: 3 of 5 files reviewed, 9 unresolved discussions (waiting on @ArniStarkware, @dafnamatsry, @elintul, and @Yael-Starkware)

---

crates/mempool/src/mempool.rs line 83 at r3 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

@giladchase do you think this line should appear here or in add_tx() ?

add_tx, since insert doesn't need to know about this high-level check.
Also we don't want new, which also uses insert, to be exposed to this I think.

[elintul]

Reviewed 1 of 2 files at r4.
Reviewable status: 4 of 5 files reviewed, 10 unresolved discussions (waiting on @ArniStarkware, @dafnamatsry, @giladchase, and @Yael-Starkware)

---

crates/mempool/src/mempool.rs line 92 at r3 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

how is verify_tx?
other future checks could fit here too

I'd maybe go with should_insert.

---

crates/mempool/src/mempool_test.rs line 26 at r4 (raw file):

/// 1. add_tx_input!(tip: 1, tx_hash: 2, sender_address: 3_u8, tx_nonce: 4)
/// 2. add_tx_input!(tip: 1, tx_hash: 2, sender_address: 3_u8)
/// 3. add_tx_input!(tip: 1, tx_hash: 2)

Please revert whitespaces changes; make sure your formatter is well-defined.
Also, it there are changes needed to this macro, please separate them to another PR.

[Yael-Starkware]

Reviewable status: 4 of 5 files reviewed, 10 unresolved discussions (waiting on @ArniStarkware, @dafnamatsry, @elintul, and @giladchase)

---

crates/mempool/src/mempool_test.rs line 26 at r4 (raw file):

Previously, elintul (Elin) wrote…

Please revert whitespaces changes; make sure your formatter is well-defined.
Also, it there are changes needed to this macro, please separate them to another PR.

Done.

[elintul]

Reviewed all commit messages.
Reviewable status: 4 of 5 files reviewed, 10 unresolved discussions (waiting on @ArniStarkware, @dafnamatsry, and @giladchase)

[Yael-Starkware]

Reviewable status: 4 of 5 files reviewed, 8 unresolved discussions (waiting on @ArniStarkware, @dafnamatsry, @elintul, and @giladchase)

---

crates/mempool/src/mempool.rs line 83 at r3 (raw file):

Previously, giladchase wrote…

add_tx, since insert doesn't need to know about this high-level check.
Also we don't want new, which also uses insert, to be exposed to this I think.

done

---

crates/mempool/src/mempool.rs line 92 at r3 (raw file):

Previously, elintul (Elin) wrote…

I'd maybe go with should_insert.

done

---

crates/mempool/src/mempool.rs line 104 at r3 (raw file):

Previously, giladchase wrote…

I think it suffices to only check if the sender_address even exists in the pool.

If it exists, then it must be Nonce(0), since otherwise it wouldn't have passed has_deploy_account_tx?

nit-picking: if all we want to know is is_some then a containts_* -> bool might fit better.

nit-picking 2 (also maybe not directly related to this PR): why does get_by_address_and_nonce return option and the rest of the API of pool returns Errors?

done

---

crates/mempool/src/mempool_test.rs line 187 at r3 (raw file):

Previously, giladchase wrote…

Does this address have any special meaning? if not then you can use 1_u8 I think, because 0x1000 looks meaningful.

Done.

---

crates/mempool/src/transaction_pool.rs line 61 at r1 (raw file):

Previously, giladchase wrote…

IIUC the user has something to do, this error means that the user send invokes immediately while forgotting to deploy.
Which is probably a bug in the user's wallet (or more accurately the user is the wallet here?).

ack

[giladchase]

Reviewable status: 2 of 5 files reviewed, 7 unresolved discussions (waiting on @ArniStarkware, @dafnamatsry, @elintul, and @Yael-Starkware)

---

crates/mempool/src/mempool.rs line 100 at r3 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

Following our discussion a few days ago. @elintul and me were just discussing whether to pass a value form gateway that tells the mempool to verify the the deploy_account tx exists.

@giladchase wdyt? There are two options here:

1. check if tx_nonce=1 and account_nonce=0, if true , check for deploy account. as implemented here.
2. get an indication from the gateway the it needs to perform the check for deploy account.

leaning towards (2), less coupling, but i'm really not sure, we need to know what common for a mempool to do/know, Elin's looking into it

---

crates/mempool/src/mempool_test.rs line 187 at r3 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

Done.

Tnx, also move it as an inline const inside the test plz, since other tests are using different addresses then that (or in a separate PR make other tests all use this address)

[dafnamatsry]

Reviewed 2 of 3 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @ArniStarkware, @elintul, @giladchase, and @Yael-Starkware)

---

crates/mempool/src/mempool.rs line 100 at r3 (raw file):

Previously, giladchase wrote…

leaning towards (2), less coupling, but i'm really not sure, we need to know what common for a mempool to do/know, Elin's looking into it

leaning towards (2) as well.
Whatever we decide, can be done in a separate PR.

---

crates/mempool/src/mempool.rs line 104 at r3 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

done

@giladchase Why it has to be Nonce(0)? The mempool can accept future nonces... Or do you assume that if we have txs for this account, they passed validation so there is a deployed account for them?
In any way, @Yael-Starkware Please update the comment to explain that.

[Yael-Starkware]

Reviewable status: 4 of 5 files reviewed, 4 unresolved discussions (waiting on @ArniStarkware, @dafnamatsry, and @elintul)

---

crates/mempool/src/mempool.rs line 100 at r3 (raw file):

Previously, dafnamatsry wrote…

leaning towards (2) as well.
Whatever we decide, can be done in a separate PR.

ok will do in a separate PR, added a TODO there and will get right onto it.

---

crates/mempool/src/mempool.rs line 104 at r3 (raw file):

Previously, dafnamatsry wrote…

@giladchase Why it has to be Nonce(0)? The mempool can accept future nonces... Or do you assume that if we have txs for this account, they passed validation so there is a deployed account for them?
In any way, @Yael-Starkware Please update the comment to explain that.

changed the comment to explain it, the comment will be modified once I implement the todo.

---

crates/mempool/src/mempool_test.rs line 187 at r3 (raw file):

Previously, giladchase wrote…

Tnx, also move it as an inline const inside the test plz, since other tests are using different addresses then that (or in a separate PR make other tests all use this address)

will do in a separate PR. I already have another PR with a test using this const

[giladchase]

Reviewable status: 4 of 5 files reviewed, 4 unresolved discussions (waiting on @ArniStarkware, @dafnamatsry, @elintul, and @Yael-Starkware)

---

crates/mempool/src/mempool.rs line 104 at r3 (raw file):

Or do you assume that if we have txs for this account, they passed validation so there is a deployed account for them?

Yep

[Yael-Starkware]

Reviewable status: 2 of 5 files reviewed, 4 unresolved discussions (waiting on @ArniStarkware, @dafnamatsry, @elintul, and @giladchase)

---

crates/mempool/src/mempool.rs line 100 at r3 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

ok will do in a separate PR, added a TODO there and will get right onto it.

done in PR:
https://reviewable.io/reviews/starkware-libs/mempool/450#-

---

crates/mempool/src/mempool_test.rs line 187 at r3 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

will do in a separate PR. I already have another PR with a test using this const

Done.

[dafnamatsry]

Reviewed 3 of 3 files at r9, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ArniStarkware, @elintul, @giladchase, and @Yael-Starkware)

.

[dafnamatsry]

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

as decided

[elintul]

Moving this feature to the next MS; my team will take it.