Skip to content

Conversation

@ArniStarkware
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link
Contributor Author

ArniStarkware commented Jul 2, 2025

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

@ArniStarkware ArniStarkware marked this pull request as ready for review July 2, 2025 09:03
@ArniStarkware ArniStarkware force-pushed the arni/gateway/reject_low_tx_gas_price/config branch from 39f2fe8 to cc35ba1 Compare July 2, 2025 10:12
@ArniStarkware ArniStarkware force-pushed the arni/gateway/reject_low_tx_gas_price/reject_low_l2_price branch from 80b990c to 31930dc Compare July 2, 2025 10:46
@ArniStarkware ArniStarkware force-pushed the arni/gateway/reject_low_tx_gas_price/config branch from cc35ba1 to 6c23d77 Compare July 2, 2025 10:46
@ArniStarkware ArniStarkware force-pushed the arni/gateway/reject_low_tx_gas_price/reject_low_l2_price branch from 31930dc to 89e5442 Compare July 2, 2025 10:59
@ArniStarkware ArniStarkware force-pushed the arni/gateway/reject_low_tx_gas_price/config branch from 6c23d77 to 55fbb6d Compare July 2, 2025 10:59
Copy link
Contributor

@ayeletstarkware ayeletstarkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 4 files at r1, all commit messages.
Reviewable status: 1 of 4 files reviewed, 1 unresolved discussion (waiting on @alonh5)


crates/apollo_gateway/src/config.rs line 176 at r2 (raw file):

    pub versioned_constants_overrides: VersionedConstantsOverrides,
    // Minimum gas price as percentage of threshold to accept transactions.
    pub min_gas_price_precentage: u8, // E.g., 80 to require 80% of threshold.

Consider using 0.8 instead of 80 to remove the need for percentage-to-ratio conversion.

Copy link
Contributor Author

@ArniStarkware ArniStarkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 of 4 files reviewed, 1 unresolved discussion (waiting on @alonh5 and @ayeletstarkware)


crates/apollo_gateway/src/config.rs line 176 at r2 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

Consider using 0.8 instead of 80 to remove the need for percentage-to-ratio conversion.

But then the type is a float.
We hate float.

Copy link
Contributor

@alonh5 alonh5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

PR looks good. Did we calculate what the percentage we want is? We should probably think of how much percentage the price could move in ~10 blocks, and have that be the threshold.

Reviewed 2 of 4 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ayeletstarkware)

Copy link
Contributor

@ayeletstarkware ayeletstarkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ArniStarkware)

@ArniStarkware ArniStarkware force-pushed the arni/gateway/reject_low_tx_gas_price/reject_low_l2_price branch from 89e5442 to 0bcb723 Compare July 2, 2025 13:41
@ArniStarkware ArniStarkware force-pushed the arni/gateway/reject_low_tx_gas_price/config branch from 55fbb6d to a71eb8f Compare July 2, 2025 13:41
@ArniStarkware
Copy link
Contributor Author

Apparently, the most the gas price changes between blocks is by a factor of 1/48. Turns out this implies 0.8 is the perfect factor.

If C=1-1/48, then C^11 < 0.8 < C^10. Passing it by @leo-starkware as well.

Copy link
Collaborator

@ShahakShama ShahakShama left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 4 of 4 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ArniStarkware)


crates/apollo_gateway/src/config.rs line 176 at r3 (raw file):

    pub versioned_constants_overrides: VersionedConstantsOverrides,
    // Minimum gas price as percentage of threshold to accept transactions.
    pub min_gas_price_percentage: u8, // E.g., 80 to require 80% of threshold.

Why not float?

Copy link
Contributor Author

@ArniStarkware ArniStarkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ArniStarkware)


crates/apollo_gateway/src/config.rs line 176 at r3 (raw file):

Previously, ShahakShama wrote…

Why not float?

https://reviewable.io/reviews/starkware-libs/sequencer/7720#-OU9o9IODkWi6zwWu9-K

Lior hates floats.

@ArniStarkware ArniStarkware force-pushed the arni/gateway/reject_low_tx_gas_price/reject_low_l2_price branch from 0bcb723 to b047f5b Compare July 2, 2025 17:38
@ArniStarkware ArniStarkware force-pushed the arni/gateway/reject_low_tx_gas_price/config branch from a71eb8f to ac6f956 Compare July 2, 2025 17:38
@ArniStarkware ArniStarkware force-pushed the arni/gateway/reject_low_tx_gas_price/config branch from ac6f956 to b01a6bd Compare July 3, 2025 07:13
@ArniStarkware ArniStarkware force-pushed the arni/gateway/reject_low_tx_gas_price/reject_low_l2_price branch from b047f5b to 843f3b1 Compare July 3, 2025 07:13
@ArniStarkware ArniStarkware deleted the arni/gateway/reject_low_tx_gas_price/config branch July 3, 2025 08:31
@github-actions github-actions bot locked and limited conversation to collaborators Jul 4, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants