Skip to content

Conversation

@jstarry
Copy link

@jstarry jstarry commented Dec 2, 2024

Problem

StaticMeta::compute_budget_limits is the only trait method on StaticMeta which is not a simple getter function. In fact, it's actually a fairly expensive trait method given that it has to do extra sanitization and computation for the transaction's default compute limit. This design limits the flexibility of the sanitize_and_convert_to_compute_budget_limits method (e.g. it's not possible to change it to accept an iterator of instructions for computing the default compute unit limit).

Summary of Changes

  • Replaced StaticMeta::compute_budget_limits with StaticMeta::compute_budget_instruction_details forcing the caller to explicitly sanitize the result if needed.
  • Increased visibility of ComputeBudgetInstructionDetails to pub

Fixes #

@jstarry jstarry requested a review from a team as a code owner December 2, 2024 09:14
@jstarry jstarry requested review from apfitzge and tao-stones and removed request for apfitzge December 2, 2024 09:14
Copy link

@apfitzge apfitzge left a comment

Choose a reason for hiding this comment

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

I personally like this interface more. There are a few places where we do not need the compute budget limits in full; just the CU price/limit. which we could refactor visibility on.

Specifically for re-iterating instructions, I think we should avoid that if we can achieve the same outcome with a few cached u16s.

Please let @tao-stones review, since he has mainly designed this interface

@tao-stones
Copy link

Exposes ComputeBudgetInstructionDetails does provides flexibility, tho I am not fully convinced that *_details to be exposed outside of crate. Totally open to change crate public APIs, but keep details internal gives RuntimeTransaction crate options to optimize without impact on callsite, for example, combining signature_details with compute_budget_details to instruction_details. Just want to raise it to hear your guys opinions about what should be considered as internal data.

@jstarry
Copy link
Author

jstarry commented Dec 3, 2024

I personally see the runtime tx crate as an internal crate anyways so I'm not concerned with what's publicly exposed. But in any case, ComputeBudgetInstructionDetails is a direct representation of the requested compute budget limits from a transaction so I think it makes a lot of sense to have this info be accessible in the static meta interface.

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.

Can't find strong reason to not make it public, given the understanding RuntimeTransaction is a internal crate. (tho not clear how to enforce it). Small nit to potentially reduce number of files followup PR touches.

transaction
.compute_budget_limits(&bank.feature_set)
.compute_budget_instruction_details()
.sanitize_and_convert_to_compute_budget_limits()

Choose a reason for hiding this comment

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

nit: if can add _feature_set: &FeatureSet to sanitize_and_convert_to_compute_budget_limits() as placeholder parameter, next PRs that's going to use feature_set won't have to touch these files again.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah I considered that but it just felt strange to modify that function when it wasn't related to the purpose of the PR

@jstarry jstarry merged commit 3a74e6d into anza-xyz:master Dec 3, 2024
40 checks passed
@jstarry jstarry deleted the refactor/static-meta-req-cb branch December 3, 2024 12:42
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.

4 participants