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 1, 2025

@ArniStarkware ArniStarkware marked this pull request as ready for review July 1, 2025 13:10
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: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @alonh5)


crates/apollo_gateway/src/stateful_transaction_validator.rs line 113 at r1 (raw file):

        // StatefulTransactionValidator and update it only once a new block is created.
        let latest_block_info = get_latest_block_info(state_reader_factory)?;
        let l2_gas_price = latest_block_info.gas_prices.strk_gas_prices.l2_gas_price;

Consider calling these lines outside this function (in process_tx or in a helper function)? because this function instantiates a blockifier stateful validator and the l2 gas price is not a part of it (?)
OR, change the func name

Code quote:

        let latest_block_info = get_latest_block_info(state_reader_factory)?;
        let l2_gas_price = latest_block_info.gas_prices.strk_gas_prices.l2_gas_price;

crates/apollo_gateway/src/stateful_transaction_validator.rs line 130 at r1 (raw file):

        );

        let blockifier_statefult_validator =

Suggestion:

blockifier_stateful_validator

@ArniStarkware ArniStarkware force-pushed the arni/gateway/reject_low_tx_gas_price/expose_price branch from b6aa95b to a1cf12f Compare July 1, 2025 15:56
@github-actions
Copy link

github-actions bot commented Jul 1, 2025

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: 0 of 3 files reviewed, all discussions resolved (waiting on @alonh5)


crates/apollo_gateway/src/stateful_transaction_validator.rs line 113 at r1 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

Consider calling these lines outside this function (in process_tx or in a helper function)? because this function instantiates a blockifier stateful validator and the l2 gas price is not a part of it (?)
OR, change the func name

Done.


crates/apollo_gateway/src/stateful_transaction_validator.rs line 130 at r1 (raw file):

        );

        let blockifier_statefult_validator =

stale

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 3 of 3 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @alonh5)

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:

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

alonh5
alonh5 previously requested changes Jul 2, 2025
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.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ArniStarkware)


a discussion (no related file):
Please add @ShahakShama and @dan-starkware as reviewers to this stack

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.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ArniStarkware and @dan-starkware)


crates/apollo_gateway/src/gateway.rs line 209 at r2 (raw file):

        // threshold.
        let _l2_gas_price =
            validator.block_context().block_info().gas_prices.strk_gas_prices.l2_gas_price;

You have the next_l2_gas_price inside the block header. Why not use it?

@ArniStarkware ArniStarkware force-pushed the arni/gateway/reject_low_tx_gas_price/expose_price branch from a1cf12f to 2d41454 Compare July 2, 2025 17:38
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: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @alonh5, @ayeletstarkware, @dan-starkware, and @ShahakShama)


crates/apollo_gateway/src/gateway.rs line 209 at r2 (raw file):

Previously, ShahakShama wrote…

You have the next_l2_gas_price inside the block header. Why not use it?

  1. I see that the gateway's state reader will always produce a SyncStateReader. The type of StateReaderFactory allows us to produce an RpcStateReader. It might no longer be supported, but a refactor is required for us to count on it at this point in the code. RpcStateReader does not have the field next_l2_gas_price IIUC.

  2. I don't want to make other blocking calls to the state to the state reader. There is a call to get_block_info. This call is blocking on an async action:

    let block = block_on(self.state_sync_client.get_block(self.block_number))

    Again, we can refactor the SyncStateReader to expose this interface, and hopefully do fewer blocking calls, but this requires a refactor.

  3. I didn't want to break the abstraction (a lot), so I used the existing executor.

Possible solution - create a wrapper to BlockInfo that also holds (an optional?) next_l2_gas_price.

Anyway - sounds like work for another day. Correct? Added a todo.

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: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @alonh5, @ayeletstarkware, @dan-starkware, and @ShahakShama)


a discussion (no related file):

Previously, alonh5 (Alon Haramati) wrote…

Please add @ShahakShama and @dan-starkware as reviewers to this stack

Now merging into main-v0.14.0. This comment is no longer relevant.

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.

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @alonh5, @ayeletstarkware, and @dan-starkware)

@ArniStarkware ArniStarkware changed the base branch from main-v0.14.0-testnet to graphite-base/7712 July 3, 2025 07:13
@ArniStarkware ArniStarkware force-pushed the arni/gateway/reject_low_tx_gas_price/expose_price branch from 2d41454 to a21723c Compare July 3, 2025 07:13
@ArniStarkware ArniStarkware changed the base branch from graphite-base/7712 to main-v0.14.0 July 3, 2025 07:13
@ArniStarkware
Copy link
Contributor Author

Folded the following PRs into this one:
#7717
#7720

@github-actions
Copy link

github-actions bot commented Jul 3, 2025

Benchmark movements: No major performance changes detected.

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 2 of 4 files at r4, 4 of 4 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @alonh5 and @dan-starkware)

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.

Dismissed @alonh5 from a discussion.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @alonh5 and @dan-starkware)

@ArniStarkware ArniStarkware dismissed alonh5’s stale review July 3, 2025 08:33

No longer relevant.

@ArniStarkware ArniStarkware added this pull request to the merge queue Jul 3, 2025
Merged via the queue into main-v0.14.0 with commit 3810b00 Jul 3, 2025
53 of 74 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jul 5, 2025
@ArniStarkware ArniStarkware deleted the arni/gateway/reject_low_tx_gas_price/expose_price branch September 11, 2025 13:22
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