Skip to content
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

Fix ed25519 program owner on testnet #23219

Closed

Conversation

jackcmay
Copy link
Contributor

Problem
Testnet's ed25519 program account is owned by the system program and not the native loader. This causes an assertion during snapshot load because the program is expected to exist and be owned by the native loader, if this is not the case then the program is re-added but during snapshot load the bank is frozen and thus panics.

Summary of Changes
Re-add the ed25519 program via a feature, this will reset the account's owner to the native loader.

Fixes #

@jackcmay jackcmay requested a review from t-nelson February 17, 2022 22:26
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!

I'm still trying to figure out how we got here 🤕

@jackcmay jackcmay requested a review from jstarry February 17, 2022 23:29
@codecov
Copy link

codecov bot commented Feb 18, 2022

Codecov Report

Merging #23219 (86db54a) into v1.9 (0fdbec9) will decrease coverage by 0.0%.
The diff coverage is 92.8%.

@@            Coverage Diff            @@
##             v1.9   #23219     +/-   ##
=========================================
- Coverage    81.7%    81.6%   -0.1%     
=========================================
  Files         529      529             
  Lines      147946   147951      +5     
=========================================
- Hits       120882   120857     -25     
- Misses      27064    27094     +30     

@t-nelson
Copy link
Contributor

:shipit: ?

@jstarry
Copy link
Member

jstarry commented Feb 18, 2022

I'm not sure this is the direction we want to take. The ed25519 program is owned by the system program because that's how precompiles are added. Maybe that was unintentional?

@t-nelson
Copy link
Contributor

It's divergent from how the account was created on devnet and MB

@jstarry
Copy link
Member

jstarry commented Feb 18, 2022

It's divergent from how the account was created on devnet and MB

Yeah, I'm aware of that. I think there might be a better way to handle the snapshot issue. Will dig in a bit

@jackcmay
Copy link
Contributor Author

True, pre-compiles are added as owned by the system account, but I don't think that was intentional and of the two precompiles, only the ed program is owned by the system program and only on testnet for some reason. I can't think of a reason to not make it consistent across nets and precompiles by switching the owner of the ed program to the native loader.

@jstarry
Copy link
Member

jstarry commented Feb 18, 2022

On mainnet*, ed25519 is a builtin program with account data including the string "ed25519_program" which doesn't apply to precompiles either. So even if we fix the owner, it's still going to be different on mainnet because it was added as a builtin program there already.

@jackcmay
Copy link
Contributor Author

You mean on mainnet?

@t-nelson
Copy link
Contributor

Mainnet-beta and devnet are consistent. I'm in favor of the shortest path to aligning testnet with them

@jackcmay jackcmay force-pushed the fix-ed25519-program-owner-2 branch from e4c9a4a to 389ebcf Compare February 18, 2022 07:30
@jackcmay
Copy link
Contributor Author

I think we need two release to fix this issue, one that sets the owner to native loader on testnet current changes in this pr, and another that adds back the ed25519 program to the builtin list like you had it in your last change.

Copy link
Member

@jstarry jstarry left a comment

Choose a reason for hiding this comment

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

This PR will break mainnet-beta/devnet again, failing test here: jackcmay#363

@jackcmay
Copy link
Contributor Author

Yeah, that would be fixed by the second release before taking v1.9 to mainnet

@jstarry
Copy link
Member

jstarry commented Feb 18, 2022

We don't need multiple releases, I have a different fix here: #23233

@jackcmay
Copy link
Contributor Author

Closed in lieu of a better fix here: #23233

@jackcmay jackcmay closed this Feb 18, 2022
@jackcmay jackcmay deleted the fix-ed25519-program-owner-2 branch April 20, 2022 22:48
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.

3 participants