feat(mempool): fix overflow bug in fee escalation flow; add tests

[elintul]

No description provided.

[lotem-starkware]

This change is 

[elintul]

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @ayeletstarkware, @giladchase, and @MohammadNassar1)

a discussion (no related file):
Depends on: #1445, for CI to pass.

---

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 13.37%. Comparing base (b0cfe82) to head (577e96b).
Report is 433 commits behind head on main.

❗ There is a different number of reports uploaded between BASE (b0cfe82) and HEAD (577e96b). Click for more details.

HEAD has 2 uploads less than BASE

Flag	BASE (b0cfe82)	HEAD (577e96b)
	3	1

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #1447       +/-   ##
===========================================
- Coverage   74.18%   13.37%   -60.82%     
===========================================
  Files         359       31      -328     
  Lines       36240     3087    -33153     
  Branches    36240     3087    -33153     
===========================================
- Hits        26886      413    -26473     
+ Misses       7220     2656     -4564     
+ Partials     2134       18     -2116     

Flag	Coverage Δ
	?

Flags with carried forward coverage won't be shown. Click here to find out more.

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

[MohammadNassar1]

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

---

crates/mempool/src/mempool.rs line 251 at r2 (raw file):

        let percentage = u128::from(self.fee_escalation_percentage);

        let Some(escalation_qualified_value) = (existing_value / 100)

Can you please document why it's okay to divide it before the multiplication?

Does it depend on the ranges of the gas/tip values?

What's the range of these values?

Code quote:

(existing_value / 100)

[elintul]

Reviewable status: 1 of 3 files reviewed, 1 unresolved discussion (waiting on @ayeletstarkware, @giladchase, and @MohammadNassar1)

---

crates/mempool/src/mempool.rs line 251 at r2 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

Can you please document why it's okay to divide it before the multiplication?

Does it depend on the ranges of the gas/tip values?

What's the range of these values?

Why wouldn't it be okay? Value is u128, clear from the signature. Not sure what information I can add with words.

[MohammadNassar1]

Reviewable status: 1 of 3 files reviewed, 1 unresolved discussion (waiting on @ayeletstarkware, @elintul, and @giladchase)

---

crates/mempool/src/mempool.rs line 251 at r2 (raw file):

Previously, elintul (Elin) wrote…

Why wouldn't it be okay? Value is u128, clear from the signature. Not sure what information I can add with words.

Typically, to avoid issues with integer division, we multiply first and then divide. However, in this case, we use a different approach.

This can cause problems when existing_values are less than 100..
Do you assume that tip/gas are above these values? is it worth mentioning?

Another Q that I asked above: what's the range of these values?

[elintul]

Reviewable status: 1 of 3 files reviewed, 1 unresolved discussion (waiting on @ayeletstarkware, @giladchase, and @MohammadNassar1)

---

crates/mempool/src/mempool.rs line 251 at r2 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

Typically, to avoid issues with integer division, we multiply first and then divide. However, in this case, we use a different approach.

This can cause problems when existing_values are less than 100..
Do you assume that tip/gas are above these values? is it worth mentioning?

Another Q that I asked above: what's the range of these values?

Okay good point, I'll give it some more thought; changed the order in the meantime in order not to delay this PR.
WDYM by range? Can you rephrase differently? I thought I answered: you know from the type.

[MohammadNassar1]

Reviewable status: 1 of 3 files reviewed, 1 unresolved discussion (waiting on @ayeletstarkware, @elintul, and @giladchase)

---

crates/mempool/src/mempool.rs line 251 at r2 (raw file):

Previously, elintul (Elin) wrote…

Okay good point, I'll give it some more thought; changed the order in the meantime in order not to delay this PR.
WDYM by range? Can you rephrase differently? I thought I answered: you know from the type.

Is it acceptable for the 'tip' or 'gas' values to be small (smaller than 100 in this case)? While the parameter type allows it, does the UX permit such small values?

[MohammadNassar1]

Reviewable status: 1 of 3 files reviewed, 2 unresolved discussions (waiting on @ayeletstarkware, @elintul, and @giladchase)

---

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

    for (tip, max_l2_gas_price) in initial_values {
        let existing_tx = tx!(tip: tip, max_l2_gas_price: max_l2_gas_price, tx_nonce: 1,
            sender_address: "0x0");

Why do you pass sender_address? I see you always use the default address.

Code quote:

sender_address: "0x0"

[elintul]

Reviewable status: 1 of 3 files reviewed, 2 unresolved discussions (waiting on @ayeletstarkware, @giladchase, and @MohammadNassar1)

---

crates/mempool/src/mempool.rs line 251 at r2 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

Is it acceptable for the 'tip' or 'gas' values to be small (smaller than 100 in this case)? While the parameter type allows it, does the UX permit such small values?

Not sure, but we should handle well all possible values.

[MohammadNassar1]

Reviewable status: 1 of 3 files reviewed, 1 unresolved discussion (waiting on @ayeletstarkware, @elintul, and @giladchase)

[MohammadNassar1]

Reviewable status: 1 of 3 files reviewed, 2 unresolved discussions (waiting on @ayeletstarkware, @elintul, and @giladchase)

---

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

        let escalation_factor = 100 + u128::from(self.fee_escalation_percentage);

        // TODO(Elin): add overflow tests; 2^127 existing with 10% increase should not overflow.

Do you want to keep the TODO?

Code quote:

// TODO(Elin): add overflow tests; 2^127 existing with 10% increase should not overflow.

[elintul]

Reviewable status: 1 of 3 files reviewed, 2 unresolved discussions (waiting on @ayeletstarkware, @giladchase, and @MohammadNassar1)

---

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

Previously, MohammadNassar1 (mohammad-starkware) wrote…

Why do you pass sender_address? I see you always use the default address.

Since it appears in the error message.

[MohammadNassar1]

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

---

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

Previously, elintul (Elin) wrote…

Since it appears in the error message.

Are you referring to the error in add_txs_and_verify_no_replacement? Since it's not part of the test, I believe you can use the default value.

But, feel free to disregard this comment and go ahead with merging the PR.

[elintul]

Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @ayeletstarkware, @giladchase, and @MohammadNassar1)

---

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

Previously, MohammadNassar1 (mohammad-starkware) wrote…

Are you referring to the error in add_txs_and_verify_no_replacement? Since it's not part of the test, I believe you can use the default value.

But, feel free to disregard this comment and go ahead with merging the PR.

Yeah, perhaps it isn't needed here, since the address is now inferred from the existing transaction.
Good catch, I'll go over all fee escalation tests and fix it.

[MohammadNassar1]

Reviewed 1 of 2 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ayeletstarkware, @elintul, and @giladchase)