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

round down compute-unit-price to its nearest 1_000 microlamport#31469

Closed
tao-stones wants to merge 7 commits into
solana-labs:masterfrom
tao-stones:round_down_compute_unit_price
Closed

round down compute-unit-price to its nearest 1_000 microlamport#31469
tao-stones wants to merge 7 commits into
solana-labs:masterfrom
tao-stones:round_down_compute_unit_price

Conversation

@tao-stones
Copy link
Copy Markdown
Contributor

@tao-stones tao-stones commented May 3, 2023 •
edited
Loading

Problem

Rounding compute-unit-price down to nearest 1_000 micro-lamport, effectively making its minimum change to be 0.001 lamport.

Summary of Changes

  1. add feature gate
  2. round compute-unit-price down to nearest 1_000 micro-lamports if feature activated;
  3. add and update tests

Feature Gate Issue: #31453

@tao-stones tao-stones added the feature-gate Pull Request adds or modifies a runtime feature gate label May 3, 2023
@tao-stones
Copy link
Copy Markdown
Contributor Author

tao-stones commented May 3, 2023 •
edited
Loading

runtime/src/transaction_priority_details.rs now requires feature activation status in order to determine prioritization. PR #31549 adds a flag to packet to pass feature status.

@tao-stones tao-stones mentioned this pull request May 9, 2023
Merged
@tao-stones
Copy link
Copy Markdown
Contributor Author

tao-stones commented May 10, 2023

blocked by #31549

@tao-stones tao-stones force-pushed the round_down_compute_unit_price branch from f997009 to e69a2f6 Compare May 11, 2023 14:42
@tao-stones tao-stones force-pushed the round_down_compute_unit_price branch from e69a2f6 to bb9a700 Compare May 18, 2023 20:24
tao-stones
tao-stones commented May 18, 2023
View reviewed changes
Comment thread core/src/packet_deserializer.rs Outdated
@tao-stones tao-stones marked this pull request as ready for review May 18, 2023 20:35
@tao-stones tao-stones force-pushed the round_down_compute_unit_price branch 2 times, most recently from c053a45 to 2e08bdf Compare May 18, 2023 21:09
@tao-stones tao-stones marked this pull request as draft May 18, 2023 21:34
apfitzge
apfitzge reviewed May 18, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@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.

some initial comments, need to take another pass

Comment thread program-runtime/src/compute_budget.rs Outdated
Comment thread program-runtime/src/prioritization_fee.rs Outdated
@tao-stones tao-stones force-pushed the round_down_compute_unit_price branch from 26742c3 to 46db185 Compare May 18, 2023 23:04
@mvines
Copy link
Copy Markdown
Contributor

mvines commented May 18, 2023

Draft change itself looks fine to me. I feel like we need a SIMD for this though to fully justify why we want to do this, and to enable folks like Pyth to chime in on any impact it may have on their transactions and if/when they can make changes in their design to deal with it

@tao-stones
Copy link
Copy Markdown
Contributor Author

tao-stones commented May 19, 2023

Draft change itself looks fine to me. I feel like we need a SIMD for this though to fully justify why we want to do this, and to enable folks like Pyth to chime in on any impact it may have on their transactions and if/when they can make changes in their design to deal with it

Sounds good, I'll chat with David then open a SIMD today.

tao-stones
tao-stones commented May 19, 2023
View reviewed changes
Comment thread programs/sbf/tests/programs.rs Outdated
Copy link
Copy Markdown
Contributor Author

@tao-stones tao-stones May 19, 2023

Choose a reason for hiding this comment

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

these should all be true since bank has all features enabled at ln 3805

@codecov
Copy link
Copy Markdown

codecov Bot commented May 19, 2023 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c142ba1) 82.0% compared to head (c39e284) 82.0%.
Report is 1568 commits behind head on master.

Additional details and impacted files 
@@           Coverage Diff            @@
##           master   #31469    +/-   ##
========================================
  Coverage    82.0%    82.0%            
========================================
  Files         769      769            
  Lines      209137   209250   +113     
========================================
+ Hits       171587   171696   +109     
- Misses      37550    37554     +4

@tao-stones tao-stones mentioned this pull request May 23, 2023
Closed
@tao-stones tao-stones mentioned this pull request Jun 2, 2023
Closed
@tao-stones tao-stones force-pushed the round_down_compute_unit_price branch 2 times, most recently from dd8c403 to fe17691 Compare June 2, 2023 23:01
@tao-stones tao-stones marked this pull request as ready for review June 4, 2023 20:49
@tao-stones tao-stones changed the title round down compute-unit-price to its nearest lamport round down compute-unit-price to its nearest 1_000 microlamport Jun 4, 2023
@tao-stones tao-stones mentioned this pull request Jun 6, 2023
Closed
@tao-stones tao-stones force-pushed the round_down_compute_unit_price branch from fe17691 to 012c504 Compare June 7, 2023 21:11
@tao-stones
Copy link
Copy Markdown
Contributor Author

tao-stones commented Jun 7, 2023

@eugene-chen @SpaceMonkeyForever would you be able to review?

@eugene-chen
Copy link
Copy Markdown

eugene-chen commented Jun 7, 2023

I am definitely not qualified to review the code 😁

@tao-stones tao-stones force-pushed the round_down_compute_unit_price branch from 86ba4bf to 5ccd09a Compare June 8, 2023 18:22
apfitzge
apfitzge suggested changes Jun 8, 2023
View reviewed changes
Comment thread program-runtime/src/compute_budget.rs Outdated
Comment thread sdk/src/feature_set.rs Outdated
Comment thread program-runtime/src/compute_budget.rs Outdated
godmodegalactus
godmodegalactus reviewed Jun 9, 2023
View reviewed changes
Comment thread program-runtime/src/compute_budget.rs Outdated
@tao-stones tao-stones force-pushed the round_down_compute_unit_price branch from 5ccd09a to 5cf0e18 Compare June 9, 2023 16:49
apfitzge
apfitzge reviewed Jun 12, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@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.

No more comments on this one from me in terms of the implementation, but think we should get the SIMD (solana-foundation/solana-improvement-documents#50) accept prior to merging & moving forward - make sure this is the path we want to take forward.

@tao-stones tao-stones mentioned this pull request Jun 14, 2023
Merged
@tao-stones
Copy link
Copy Markdown
Contributor Author

tao-stones commented Jun 21, 2023

Think we had agreed on this PR, would love to have it merged before drifting too far away from master. Would appreciate review / approve SIMD 50 first.
tag @eugene-chen @mschneider @godmodegalactus @SpaceMonkeyForever
🙇🏼‍♂️

godmodegalactus
godmodegalactus reviewed Jun 21, 2023
View reviewed changes
Comment thread program-runtime/src/compute_budget.rs Outdated
@godmodegalactus
Copy link
Copy Markdown
Contributor

godmodegalactus commented Jun 21, 2023

Overall looks good to me. Performance impact should be insignificant overall but may increase a little the average CU consumed by compute budget program.

apfitzge
apfitzge reviewed Jun 21, 2023
View reviewed changes
Comment thread program-runtime/src/compute_budget.rs Outdated
@tao-stones tao-stones force-pushed the round_down_compute_unit_price branch from 5cf0e18 to c39e284 Compare June 21, 2023 22:41
@apfitzge
Copy link
Copy Markdown
Contributor

apfitzge commented Jun 22, 2023

approved, but still we should be waiting on the SIMD acceptance.

@tao-stones
Copy link
Copy Markdown
Contributor Author

tao-stones commented Jun 22, 2023

approved, but still we should be waiting on the SIMD acceptance.

🙇🏼

@mergify mergify Bot added community Community contribution need:merge-assist labels Jan 23, 2024
@willhickey
Copy link
Copy Markdown
Contributor

willhickey commented Mar 3, 2024

This repository is no longer in use. Please re-open this pull request in the agave repo: https://github.com/anza-xyz/agave

@willhickey willhickey closed this Mar 3, 2024
@apfitzge apfitzge mentioned this pull request Oct 1, 2024
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@mschneider mschneider Awaiting requested review from mschneider

@mvines mvines Awaiting requested review from mvines

2 more reviewers

@apfitzge apfitzge apfitzge approved these changes

@godmodegalactus godmodegalactus godmodegalactus approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

community Community contribution feature-gate Pull Request adds or modifies a runtime feature gate need:merge-assist

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@tao-stones @mvines @eugene-chen @SpaceMonkeyForever @godmodegalactus @apfitzge @willhickey