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

RuntimeTransaction::try_new_from_sanitized_transaction #2674

Closed

Conversation

apfitzge
Copy link

Problem

  • Want to make transition of processing functions easier by allowing simpler translation from an already created SanitizedTransaction

Summary of Changes

  • add try_from_sanitized_transaction

Fixes #

@apfitzge apfitzge marked this pull request as ready for review August 20, 2024 17:28
@apfitzge apfitzge self-assigned this Aug 23, 2024
@apfitzge apfitzge force-pushed the runtime_transaction_direct_create branch from c6d5228 to 8ddddf4 Compare September 30, 2024 19:42
Copy link

@tao-stones tao-stones left a comment

Choose a reason for hiding this comment

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

What is the use case of this try_new? RuntimeTransaction<> encapsulates sanitizing transactions within. Wondering what situation makes sense to expose it? I mean if it can't follow lifetime of RuntimeTransaction<SanitizedVersionedTransaction> -> RuntimeTransaction<SanitizedTransaction>, I wonder if there are logical messiness on call site?

@apfitzge
Copy link
Author

apfitzge commented Oct 2, 2024

What is the use case of this try_new? RuntimeTransaction<> encapsulates sanitizing transactions within. Wondering what situation makes sense to expose it? I mean if it can't follow lifetime of RuntimeTransaction -> RuntimeTransaction, I wonder if there are logical messiness on call site?

  • Many tests directly create SanitizedTransaction from Transaction (in Use RuntimeTransaction #3041 I add a test-only call to this for easier replacement).
  • blockstore_processor calls Bank::verify_transaction which takes VersionedTransaction and returns SanitizedTransaction without ever using SanitizedVersionedTransaction. This will be updated in Use RuntimeTransaction #3041 to create RuntimeTransaction<SanitizedTransaction>.

I'd actually put pressure on the other way.
There's only one place I'm aware we even go to SanitizedVersionedTransaction as an intermediate step, which is in legacy BankingStage.
That is going away already, and at least until new transaction type is widely used I don't think we plan to have this use an intermediate RuntimeTransaction.

@apfitzge apfitzge closed this Oct 2, 2024
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.

2 participants