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

Remove unnecessary unwrap from simulate_transaction_unchecked() #35375

Merged
merged 1 commit into from
Mar 1, 2024

Conversation

pgarg66
Copy link
Contributor

@pgarg66 pgarg66 commented Feb 29, 2024

Problem

simulate_transaction_unchecked() does unnecessary unwrap(), that can be handled more gracefully.

Summary of Changes

Replace unwrap() with better handling.

Fixes #

@pgarg66 pgarg66 marked this pull request as ready for review February 29, 2024 17:09
execution_results
.pop()
.unwrap_or(TransactionExecutionResult::NotExecuted(
TransactionError::InvalidProgramForExecution,
Copy link
Contributor

Choose a reason for hiding this comment

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

i guess this is about as generic a TransactionError as we have 🤔

prolly fine for now since this is isolated to simulation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, the other one we could use was AccountInUse. Nothing generic otherwise.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does TransactionExecutionResult::NotExecuted trigger a replay?

Copy link
Contributor

Choose a reason for hiding this comment

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

assuming by "replay" you mean that the tx is retryable, no. when a tx makes it to execute, it's getting fees taken and committed in a block. even if it were, this logic is in simulate, which won't be committed anyway

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant retryable yes. And I forgot that this is simulation and thus a failure is not repeated. So seems alright to me.

Copy link

codecov bot commented Feb 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.7%. Comparing base (6ee3bb9) to head (7a1c830).
Report is 21 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff            @@
##           master   #35375    +/-   ##
========================================
  Coverage    81.7%    81.7%            
========================================
  Files         834      834            
  Lines      224299   224819   +520     
========================================
+ Hits       183361   183831   +470     
- Misses      40938    40988    +50     

@pgarg66 pgarg66 requested a review from t-nelson March 1, 2024 00:15
Copy link
Contributor

@t-nelson t-nelson left a comment

Choose a reason for hiding this comment

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

-2 unwrap()s! 🥳

@pgarg66 pgarg66 merged commit cb260f1 into solana-labs:master Mar 1, 2024
35 checks passed
@pgarg66 pgarg66 deleted the remove-unwrap branch March 1, 2024 21:37
@pgarg66 pgarg66 added v1.18 PRs that should be backported to v1.18 v1.17 PRs that should be backported to v1.17 labels Mar 1, 2024
Copy link
Contributor

mergify bot commented Mar 1, 2024

Backports to the stable branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule.

Copy link
Contributor

mergify bot commented Mar 1, 2024

Backports to the beta branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis.

mergify bot pushed a commit that referenced this pull request Mar 1, 2024
…5375)

Remove unnecessary unwrap from simulate_transaction_unchecked()

(cherry picked from commit cb260f1)
mergify bot pushed a commit that referenced this pull request Mar 1, 2024
…5375)

Remove unnecessary unwrap from simulate_transaction_unchecked()

(cherry picked from commit cb260f1)
willhickey pushed a commit that referenced this pull request Mar 3, 2024
…()` (backport of #35375) (#35397)

Remove unnecessary unwrap from `simulate_transaction_unchecked()` (#35375)

Remove unnecessary unwrap from simulate_transaction_unchecked()

(cherry picked from commit cb260f1)

Co-authored-by: Pankaj Garg <[email protected]>
pgarg66 added a commit to pgarg66/solana that referenced this pull request Mar 4, 2024
…()` (backport of solana-labs#35375)

Remove unnecessary unwrap from `simulate_transaction_unchecked()` (solana-labs#35375)

Remove unnecessary unwrap from simulate_transaction_unchecked()

(cherry picked from commit cb260f1)

Co-authored-by: Pankaj Garg <[email protected]>
pgarg66 added a commit to pgarg66/solana that referenced this pull request Mar 5, 2024
…()` (backport of solana-labs#35375)

Remove unnecessary unwrap from `simulate_transaction_unchecked()` (solana-labs#35375)

Remove unnecessary unwrap from simulate_transaction_unchecked()

(cherry picked from commit cb260f1)

Co-authored-by: Pankaj Garg <[email protected]>
pgarg66 added a commit to pgarg66/solana that referenced this pull request Mar 6, 2024
…()` (backport of solana-labs#35375)

Remove unnecessary unwrap from `simulate_transaction_unchecked()` (solana-labs#35375)

Remove unnecessary unwrap from simulate_transaction_unchecked()

(cherry picked from commit cb260f1)

Co-authored-by: Pankaj Garg <[email protected]>
pgarg66 added a commit to anza-xyz/agave that referenced this pull request Mar 9, 2024
…()` (backport of solana-labs#35375) (#65)

Remove unnecessary unwrap from `simulate_transaction_unchecked()` (solana-labs#35375)

Remove unnecessary unwrap from simulate_transaction_unchecked()

(cherry picked from commit cb260f1)

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
willhickey pushed a commit that referenced this pull request Mar 9, 2024
…()` (backport of #35375) (#65)

Remove unnecessary unwrap from `simulate_transaction_unchecked()` (#35375)

Remove unnecessary unwrap from simulate_transaction_unchecked()

(cherry picked from commit cb260f1)

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
godmodegalactus added a commit to blockworks-foundation/solana that referenced this pull request Apr 16, 2024
Changing the defaults for ScanConfig

v1.17: ignore mio audit report (solana-labs#76)

ci: ignore mio audit

v1.17: Display error message while loading geyser plugins (solana-labs#33990) (solana-labs#97)

Display error message while laoding geyser plugins (solana-labs#33990)

Co-authored-by: galactus <[email protected]>

v1.17: Remove unnecessary unwrap from `simulate_transaction_unchecked()` (backport of solana-labs#35375) (solana-labs#65)

Remove unnecessary unwrap from `simulate_transaction_unchecked()` (solana-labs#35375)

Remove unnecessary unwrap from simulate_transaction_unchecked()

(cherry picked from commit cb260f1)

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v1.17 PRs that should be backported to v1.17 v1.18 PRs that should be backported to v1.18
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants