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

feature gate to enable compute_budget::request_heap_frame on mainnetBeta #30077

Merged
merged 8 commits into from
Feb 3, 2023
Merged

feature gate to enable compute_budget::request_heap_frame on mainnetBeta #30077

merged 8 commits into from
Feb 3, 2023

Conversation

tao-stones
Copy link
Contributor

@tao-stones tao-stones commented Feb 2, 2023 •
edited
Loading

Problem

in Mainnet-beta, v1.14+ should by default keep v1.13 behavior in term of compute_budget::request_heap_frame instruction, which is transaction's prioritization fee will be ignored (eg, set to zero) if same transaction includes compute_budget::request_heap_frame instruction.

Summary of Changes

  • Add feature gate to mimic v1.13 behavior

Feature Gate Issue #30076

@tao-stones tao-stones requested review from brooksprumo, Lichtso and steviez and removed request for brooksprumo February 2, 2023 07:53
@tao-stones tao-stones added v1.14 v1.15 (abandoned) The v1.15 branch has been abandoned labels Feb 2, 2023
@tao-stones tao-stones mentioned this pull request Feb 2, 2023
Closed
@jackcmay
Copy link
Contributor

jackcmay commented Feb 2, 2023

Missing test coverage to exercise the two behaviors should also be added

@tao-stones tao-stones force-pushed the feature_gate_enable_request_heap_frame_ix branch 2 times, most recently from 4baa95d to 991b4ee Compare February 2, 2023 16:01
@tao-stones
Copy link
Contributor Author

Missing test coverage to exercise the two behaviors should also be added

tests added for calculate_fee() and process_instructions()

Lichtso
Lichtso reviewed Feb 2, 2023
View reviewed changes
runtime/src/bank.rs Outdated Show resolved Hide resolved
program-runtime/src/compute_budget.rs Show resolved Hide resolved
steviez
steviez reviewed Feb 2, 2023
View reviewed changes
Copy link
Contributor

@steviez steviez left a comment

Choose a reason for hiding this comment

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

Just a couple of nits

runtime/src/accounts.rs Show resolved Hide resolved
runtime/src/cost_model.rs Outdated Show resolved Hide resolved
runtime/src/bank.rs Outdated Show resolved Hide resolved
t-nelson
t-nelson reviewed Feb 2, 2023
View reviewed changes
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.

lgtm initial motivation-wise. just nits/rigor nags from me

program-runtime/src/compute_budget.rs Show resolved Hide resolved
runtime/src/accounts.rs Show resolved Hide resolved
runtime/src/bank.rs Outdated Show resolved Hide resolved
runtime/src/cost_model.rs Outdated Show resolved Hide resolved
runtime/src/cost_model.rs Outdated Show resolved Hide resolved
@tao-stones tao-stones force-pushed the feature_gate_enable_request_heap_frame_ix branch 2 times, most recently from 1f077de to 45fb223 Compare February 2, 2023 23:54
t-nelson
t-nelson reviewed Feb 3, 2023
View reviewed changes
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.

r+ @steviez approval

thanks!

steviez
steviez approved these changes Feb 3, 2023
View reviewed changes
Copy link
Contributor

@steviez steviez left a comment

Choose a reason for hiding this comment

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

LGTM, but it looks like a merge issue from the change to move tests out of bank.rs so you'll have to rebase + get approvals again

@tao-stones tao-stones force-pushed the feature_gate_enable_request_heap_frame_ix branch from 45fb223 to 826e1b0 Compare February 3, 2023 03:53
@tao-stones
Copy link
Contributor Author

LGTM, but it looks like a merge issue from the change to move tests out of bank.rs so you'll have to rebase + get approvals again

rebased. Heeding your advice, going to run the PR against the snapshot to confirm if it'd produce right hash. Will post result back here.

@tao-stones tao-stones requested a review from steviez February 3, 2023 05:30
@tao-stones tao-stones merged commit 4293f11 into solana-labs:master Feb 3, 2023
@mergify mergify bot mentioned this pull request Feb 3, 2023
Merged
mergify bot pushed a commit that referenced this pull request Feb 3, 2023
@tao-stones @mergify
feature gate to enable compute_budget::request_heap_frame on mainnetB…
6c908ea 
…eta (#30077)

(cherry picked from commit 4293f11)

# Conflicts:
#	program-runtime/src/compute_budget.rs
#	runtime/src/accounts.rs
#	runtime/src/bank.rs
#	runtime/src/bank/tests.rs
#	runtime/src/cost_model.rs
#	runtime/src/transaction_priority_details.rs
#	sdk/src/feature_set.rs
@mergify mergify bot mentioned this pull request Feb 3, 2023
Merged
mergify bot pushed a commit that referenced this pull request Feb 3, 2023
@tao-stones @mergify
feature gate to enable compute_budget::request_heap_frame on mainnetB…
b538412 
…eta (#30077)

(cherry picked from commit 4293f11)

# Conflicts:
#	programs/sbf/tests/programs.rs
#	runtime/src/accounts.rs
#	runtime/src/bank.rs
#	runtime/src/bank/tests.rs
#	sdk/src/feature_set.rs
@tao-stones tao-stones added the feature-gate Pull Request adds or modifies a runtime feature gate label Feb 3, 2023
tao-stones added a commit that referenced this pull request Feb 4, 2023
@tao-stones
feature gate to enable compute_budget::request_heap_frame on mainnetB…
0bb76eb 
…eta (#30077)

(cherry picked from commit 4293f11)

merge fix
tao-stones added a commit that referenced this pull request Feb 4, 2023
@mergify @tao-stones @mvines
feature gate to enable compute_budget::request_heap_frame on mainnetB…
e2b476e 
…eta (backport #30077) (#30117)

Co-authored-by: Tao Zhu <[email protected]>
Co-authored-by: Tao Zhu <[email protected]>
Co-authored-by: Michael Vines <[email protected]>
tao-stones added a commit that referenced this pull request Feb 4, 2023
@mergify @tao-stones
feature gate to enable compute_budget::request_heap_frame on mainnetB…
0cad9c5 
…eta (backport #30077) (#30118)

Co-authored-by: Tao Zhu <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@t-nelson t-nelson t-nelson left review comments

@Lichtso Lichtso Awaiting requested review from Lichtso

@steviez steviez Awaiting requested review from steviez

Assignees
No one assigned
Labels
feature-gate Pull Request adds or modifies a runtime feature gate v1.15 (abandoned) The v1.15 branch has been abandoned
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@tao-stones @jackcmay @Lichtso @t-nelson @steviez