Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.

Refactor: Improve type safety and readability of transaction execution#22215

Merged
jstarry merged 4 commits intosolana-labs:masterfrom
jstarry:execute-transaction
Jan 5, 2022
Merged

Refactor: Improve type safety and readability of transaction execution#22215
jstarry merged 4 commits intosolana-labs:masterfrom
jstarry:execute-transaction

Conversation

@jstarry
Copy link
Copy Markdown
Contributor

@jstarry jstarry commented Jan 1, 2022 •
edited
Loading

Problem

  • Lack of type safety to differentiate between transaction failures that should be committed vs ignored (Bank::can_commit infamously handles this)
  • Confusing that NonceFull is used in both LoadedTransaction and TransactionExecutionResult
  • Bank::load_and_execute_transactions is 300 lines long and confusing to follow because it re-assigns process_result a lot

Summary of Changes

  • New enum TransactionExecutionDetails which aims to make it easier to differentiate between tx errors which should be committed or not. It also holds recorded log messages and inner instructions now to avoid some zipping of data structures.
  • New Bank::execute_loaded_transaction method to make Bank::load_and_execute_transactions easier to read

Fixes #

@jstarry jstarry force-pushed the execute-transaction branch from 9a0984a to facb06b Compare January 1, 2022 13:15
@jstarry jstarry force-pushed the execute-transaction branch from facb06b to f2c8a11 Compare January 1, 2022 21:51
@jstarry jstarry force-pushed the execute-transaction branch from f2c8a11 to 79f59be Compare January 2, 2022 00:07
@jstarry jstarry force-pushed the execute-transaction branch from 79f59be to a48f5c2 Compare January 2, 2022 06:29
@codecov
Copy link
Copy Markdown

codecov Bot commented Jan 2, 2022 •
edited
Loading

Codecov Report

Merging #22215 (de19eee) into master (ca8fef5) will decrease coverage by 0.0%.
The diff coverage is 95.5%.

❗ Current head de19eee differs from pull request most recent head 83906a9. Consider uploading reports for the commit 83906a9 to get more accurate results

@@            Coverage Diff            @@
##           master   #22215     +/-   ##
=========================================
- Coverage    81.1%    81.1%   -0.1%     
=========================================
  Files         523      523             
  Lines      146705   146742     +37     
=========================================
+ Hits       118995   119013     +18     
- Misses      27710    27729     +19

@jstarry jstarry requested a review from Lichtso January 2, 2022 12:14
@jstarry jstarry force-pushed the execute-transaction branch 10 times, most recently from b3b56a3 to d6fd639 Compare January 4, 2022 07:46
@jstarry jstarry force-pushed the execute-transaction branch from d6fd639 to de19eee Compare January 4, 2022 10:57
@jstarry
Copy link
Copy Markdown
Contributor Author

jstarry commented Jan 4, 2022

@Lichtso can you take another look?

Lichtso
Lichtso reviewed Jan 4, 2022
View reviewed changes
Comment thread runtime/src/bank.rs
Lichtso
Lichtso reviewed Jan 4, 2022
View reviewed changes
Comment thread runtime/src/bank.rs
Lichtso
Lichtso reviewed Jan 4, 2022
View reviewed changes
Comment thread programs/bpf/tests/programs.rs
Lichtso
Lichtso reviewed Jan 4, 2022
View reviewed changes
Comment thread runtime/src/bank.rs Outdated
@jstarry
Copy link
Copy Markdown
Contributor Author

jstarry commented Jan 4, 2022

Thanks for the feedback @Lichtso, did you finish your review pass? I've addressed your comments

Lichtso
Lichtso previously approved these changes Jan 4, 2022
View reviewed changes
@jstarry jstarry added the automerge Merge this Pull Request automatically once CI passes label Jan 4, 2022
@mergify mergify Bot removed the automerge Merge this Pull Request automatically once CI passes label Jan 4, 2022
@mergify
Copy link
Copy Markdown
Contributor

mergify Bot commented Jan 4, 2022

automerge label removed due to a CI failure

@jstarry jstarry force-pushed the execute-transaction branch from f412e9a to 83906a9 Compare January 4, 2022 22:11
@jstarry jstarry added the automerge Merge this Pull Request automatically once CI passes label Jan 4, 2022
@mergify mergify Bot dismissed Lichtso’s stale review January 4, 2022 22:12

Pull request has been modified.

@mergify
Copy link
Copy Markdown
Contributor

mergify Bot commented Jan 5, 2022

automerge label removed due to a CI failure

@mergify mergify Bot removed the automerge Merge this Pull Request automatically once CI passes label Jan 5, 2022
@jstarry jstarry merged commit 45458e7 into solana-labs:master Jan 5, 2022
@jstarry jstarry deleted the execute-transaction branch January 5, 2022 02:15
@jstarry jstarry added the v1.9 label Jan 5, 2022
mergify Bot pushed a commit that referenced this pull request Jan 5, 2022
@jstarry
Refactor: Improve type safety and readability of transaction execution (
d0a8af5 
#22215)

* Refactor Bank::load_and_execute_transactions

* Refactor: improve type safety of TransactionExecutionResult

* Add enum for extra type safety in execution results

* feedback

(cherry picked from commit 45458e7)

# Conflicts:
#	programs/bpf/benches/bpf_loader.rs
#	programs/bpf/tests/programs.rs
#	runtime/src/bank.rs
@mergify mergify Bot mentioned this pull request Jan 5, 2022
Merged
jstarry added a commit that referenced this pull request Jan 5, 2022
@jstarry
Refactor: Improve type safety and readability of transaction execution (
5432d88 
#22215)
jstarry added a commit that referenced this pull request Jan 5, 2022
@mergify @jstarry
Refactor: Improve type safety and readability of transaction executio…
8578429 
…n (backport #22215) (#22289)

* Refactor: Improve type safety and readability of transaction execution (#22215)

* resolve conflicts

Co-authored-by: Justin Starry <justin@solana.com>
@brooksprumo brooksprumo mentioned this pull request Jan 5, 2022
Closed
@CriesofCarrots CriesofCarrots mentioned this pull request Jan 15, 2022
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@Lichtso Lichtso Lichtso left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jstarry @Lichtso