-
Notifications
You must be signed in to change notification settings - Fork 818
Drop the Config builtin #5331
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
Drop the Config builtin #5331
Conversation
|
The Firedancer team maintains a line-for-line reimplementation of the |
This comment was marked as resolved.
This comment was marked as resolved.
979478b to
bb88b19
Compare
|
@tao-stones I've requested your review here, since the eviction of the Config program modifies the builtin cost tracker setup. The contributor warnings around the builtin tracker list explicitly mention how to treat existing builtins that are going to be migrated and those that are not going to be migrated. However, it does not say anything about builtins that have been migrated on all three clusters. Based on our previous conversations in SIMD 0170 and the codebase within Let me know if you feel otherwise! |
|
@topointon-jump FYI this PR touches the stake program, but it does not practically change anything about it. It merely updates it to use the Config program's client, rather than the builtin library from this repository. Same interface. |
tao-stones
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look good from builtin cost perspective. config will no longer be builtin, its default cost will be 200K CUs.
|
Looks correct to me, but have a general question about builtin migrations that is related. I see that we use |
Yes, exactly! Here: Lines 74 to 77 in f56845e
https://github.com/anza-xyz/agave/blob/master/fetch-core-bpf.sh |
a871577 to
a2c9ac1
Compare
joncinque
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me! Just a couple of small questions
This comment was marked as resolved.
This comment was marked as resolved.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5331 +/- ##
=========================================
- Coverage 83.3% 83.3% -0.1%
=========================================
Files 828 824 -4
Lines 373649 372824 -825
=========================================
- Hits 311592 310761 -831
- Misses 62057 62063 +6 🚀 New features to boost your workflow:
|
joncinque
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
|
The node has hummed over the epoch rollover and everything seems good. CI passing, comments resolved. Ready to merge. @tao-stones I re-requested you for the Fees codeowner. Thanks! |
tao-stones
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
vouch for fee
Problem
The Config program has been migrated to an on-chain BPF program. It's time to remove it from the monorepo.
Summary of Changes
Commit-by-commit, unwind the Config program's API being used around the validator, then finally drop the program entirely.