This repository was archived by the owner on Jan 22, 2025. It is now read-only.
Fix builtin handling on epoch boundaries #23256
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This comment was marked as spam.
This comment was marked as spam.
jstarry
force-pushed
the
fix-builtin-epoch-boundary
branch
from
6791174 to
a3beb92
Compare
jstarry
force-pushed
the
fix-builtin-epoch-boundary
branch
from
a3beb92 to
ee078d1
Compare
Codecov Report
@@ Coverage Diff @@
## master #23256 +/- ##
===========================================
+ Coverage 69.7% 81.3% +11.5%
===========================================
Files 36 571 +535
Lines 2220 155510 +153290
Branches 314 0 -314
===========================================
+ Hits 1549 126498 +124949
- Misses 559 29012 +28453
+ Partials 112 0 -112 |
|
Looks fine, I would take out the CI workaround, that is something that should probably be handled by devops instead of a per-pr special case. |
jstarry
force-pushed
the
fix-builtin-epoch-boundary
branch
from
ee078d1 to
a1bbfe9
Compare
mergify bot
pushed a commit
that referenced
this pull request
Feb 22, 2022
(cherry picked from commit bcda74f) # Conflicts: # runtime/src/bank.rs # runtime/src/genesis_utils.rs
mergify bot
added a commit
that referenced
this pull request
Feb 22, 2022
* Fix builtin handling on epoch boundaries (#23256) (cherry picked from commit bcda74f) # Conflicts: # runtime/src/bank.rs # runtime/src/genesis_utils.rs * fix conflicts Co-authored-by: Justin Starry <[email protected]>
jeffwashington
pushed a commit
to jeffwashington/solana
that referenced
this pull request
Mar 4, 2022
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
On epoch boundaries only newly activated features should lead to changes to builtin programs. But in #23233, builtin addition/removal transitions were combined such that if the removal feature id was not activated, the builtin would be actively re-added instead of being skipped.
This repeated addition of a builtin can cause problems because on testnet when the built-in for ed25519 is re-added, the account becomes native loader owned and the "ed25519_program" string is stored in its data buffer. This results in a hash mismatch because on testnet, the ed25519 program id is a system owned account with an empty data buffer.
Summary of Changes
Ensure that builtins are only "re-added" on epoch boundaries if the feature used for adding the builtin was newly activated. Keep the behavior of re-adding builtins during snapshot loading to ensure that bank's have the correct list of builtin programs until a builtin is removed.
Fixes #23253