Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(starknet_gateway): use has tx from address to skip validate #3218

Open
wants to merge 1 commit into
base: arni/skip_validate/gateway/remove_spawn_blocking
Choose a base branch
from

Conversation

ArniStarkware
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link
Contributor Author

ArniStarkware commented Jan 9, 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 January 9, 2025 12:27
@ArniStarkware ArniStarkware force-pushed the arni/skip_validate/deploy_account_exists/impl branch from 6e131e2 to 8f9b3c3 Compare January 9, 2025 14:24
@ArniStarkware ArniStarkware force-pushed the arni/skip_validate/deploy_account_exists/gateway_side branch 2 times, most recently from bc7634b to 5bffee1 Compare January 9, 2025 15:32
@ArniStarkware ArniStarkware force-pushed the arni/skip_validate/deploy_account_exists/impl branch 2 times, most recently from 326cbcb to 29ea006 Compare January 9, 2025 17:54
Copy link
Contributor

@Yael-Starkware Yael-Starkware left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @ArniStarkware)


crates/starknet_gateway/src/stateful_transaction_validator.rs line 138 at r1 (raw file):

                    // not find it?
                    return false;
                }

I think we should return the error, as any other error.

Code quote:

                Err(MempoolClientError::ClientError(_err)) => {
                    warn!("A client error occurred");
                    // Should we simply take a strict approach and in this case say the mempool did
                    // not find it?
                    return false;
                }

crates/starknet_gateway/src/stateful_transaction_validator.rs line 141 at r1 (raw file):

                Err(MempoolClientError::MempoolError(_err)) => {
                    panic!("No such error can occur on 'has_tx_from_address'")
                }

not sure I'd assert it, since the mempool implementation can change over time.

Code quote:

                Err(MempoolClientError::MempoolError(_err)) => {
                    panic!("No such error can occur on 'has_tx_from_address'")
                }

crates/starknet_gateway/src/gateway.rs line 147 at r1 (raw file):

        let skip_validate =
            skip_stateful_validations(&executable_tx, account_nonce, self.mempool_client.clone())
                .await;

why did you extract the skip validate logic from run_validate?

Code quote:

        let skip_validate =
            skip_stateful_validations(&executable_tx, account_nonce, self.mempool_client.clone())
                .await;

Copy link
Contributor

@Yael-Starkware Yael-Starkware 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, 4 unresolved discussions (waiting on @ArniStarkware)


crates/starknet_gateway/src/stateful_transaction_validator_test.rs line 155 at r1 (raw file):

        should_skip_validate
    );
}

lets add a test where has_tx_from_address() returns false

@ArniStarkware ArniStarkware force-pushed the arni/skip_validate/deploy_account_exists/gateway_side branch from 5bffee1 to 367229c Compare January 12, 2025 13:40
@ArniStarkware ArniStarkware changed the base branch from arni/skip_validate/deploy_account_exists/impl to arni/skip_validate/gateway/remove_spawn_blocking January 12, 2025 13:40
Copy link

Artifacts upload workflows:

@ArniStarkware ArniStarkware changed the title feat(starknet_gateway): use deploy account exists to skip validate feat(starknet_gateway): use has tx from address to skip validate Jan 12, 2025
@ArniStarkware ArniStarkware force-pushed the arni/skip_validate/gateway/remove_spawn_blocking branch from 9308f18 to c4f026f Compare January 12, 2025 15:34
@ArniStarkware ArniStarkware force-pushed the arni/skip_validate/deploy_account_exists/gateway_side branch from 367229c to 6fca32e Compare January 12, 2025 15:34
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 4 files reviewed, 5 unresolved discussions (waiting on @Yael-Starkware)


crates/starknet_gateway/src/gateway.rs line 147 at r1 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

why did you extract the skip validate logic from run_validate?

Done. Reverted.


crates/starknet_gateway/src/stateful_transaction_validator.rs line 138 at r1 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

I think we should return the error, as any other error.

Still a WIP.


crates/starknet_gateway/src/stateful_transaction_validator.rs line 141 at r1 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

not sure I'd assert it, since the mempool implementation can change over time.

Done.Still a WIP.


crates/starknet_gateway/src/stateful_transaction_validator_test.rs line 155 at r1 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

lets add a test where has_tx_from_address() returns false

Done.


-- commits line 2 at r2:

Suggestion:

367229c: feat(starknet_gateway): use has tx from address to skip validate

@ArniStarkware ArniStarkware force-pushed the arni/skip_validate/gateway/remove_spawn_blocking branch from c4f026f to 7b0854c Compare January 12, 2025 17:54
@ArniStarkware ArniStarkware force-pushed the arni/skip_validate/deploy_account_exists/gateway_side branch from 6fca32e to 85d86a9 Compare January 12, 2025 18:27
Copy link
Contributor

@Yael-Starkware Yael-Starkware 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 r3, all commit messages.
Reviewable status: 2 of 4 files reviewed, 3 unresolved discussions (waiting on @ArniStarkware)


crates/starknet_gateway/src/stateful_transaction_validator.rs line 138 at r1 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Still a WIP.

I didn't realize it is so complicated.
maybe it is good enough to just return unexpected error.
lets talk f2f


crates/starknet_gateway/src/stateful_transaction_validator.rs line 141 at r1 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Done.Still a WIP.

same

Copy link
Contributor

@Yael-Starkware Yael-Starkware 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 4 files reviewed, 3 unresolved discussions (waiting on @ArniStarkware)


crates/starknet_gateway/src/stateful_transaction_validator.rs line 138 at r1 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

I didn't realize it is so complicated.
maybe it is good enough to just return unexpected error.
lets talk f2f

after discussing f2f,
we decided to return here unexpected error and in a different PR fix the mempool_client_result_to_gw_spec_result return value in case of P2pPropagatorClientError, which will enable us to use it here.

@ArniStarkware ArniStarkware force-pushed the arni/skip_validate/gateway/remove_spawn_blocking branch from 7b0854c to 7af75d6 Compare January 13, 2025 15:13
@ArniStarkware ArniStarkware force-pushed the arni/skip_validate/deploy_account_exists/gateway_side branch from 85d86a9 to fabe1b6 Compare January 13, 2025 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants