Forbid bogus account assignment to native_loaders

[ryoqun]

Problem

Transactions can assign account to NativeLoader1111 for no good reason.

Summary of Changes

Forbid doing so just like sysvars.

Obviously, this creates dos attack window when upgrading.

Note that, I'm intentionally omitting to gate this change. Considering recent our practices, this should be acceptable? (CC: @mvines )

Alternatively, if this is concerning too much, we can resort to adding account.executable to this existing condition:

solana/runtime/src/bank.rs

Line 1201 in 5b1aca1

already_genuine_program_exists = native_loader::check_id(&account.owner);

and to-be-pushed new capitalization calculation at #11927.

context

following up: #11750

blocking: #11927 (This is not a blocker anymore; as this requires proper gating, I've unblocked #11927).

[codecov]

Codecov Report

Merging #11928 into master will decrease coverage by 0.0%.
The diff coverage is 100.0%.

@@            Coverage Diff            @@
##           master   #11928     +/-   ##
=========================================
- Coverage    82.2%    82.1%   -0.1%     
=========================================
  Files         331      331             
  Lines       78062    78072     +10     
=========================================
- Hits        64184    64169     -15     
- Misses      13878    13903     +25     

[mvines]

Note that, I'm intentionally omitting to gate this change. Considering recent our practices, this should be acceptable?

😬 things just got very real over the weekend, I think the luxury of our past short-cuts aren't really viable anymore

[mvines]

Actually why bother with this? When we introduce a new BPFLoader, we'll just rewrite the account and preserve whatever lamports are in it

[ryoqun]

Actually why bother with this? When we introduce a new BPFLoader, we'll just rewrite the account and preserve whatever lamports are in it

Newer native loader program accounts (= accounts owned by NativeLoader1111111111111111111111111111111) is introduced by ensuring to exist at a certain epoch by checking the existence of account at the specific account address. (@jackcmay: Come to think of it, we can unconditionally and always overwrite them at the start of epoch X, maybe?)

(this paragraph is based on current impl with the above checking; unconditional overwrite will evade this entirely) So, we must be careful to discern whether to overwrite or not by checking already-existing account is bogus or genuine. The check requires executable bit in addition of just owner. (This is lacking in the current impl) And executable bit is needed unless we forbid arbitrary assignments to the native loader program. I want to treat them similarly like sysvars (there could be no bogus accounts owned by Sysvar1111111111111111111111111111111111111).

Unless we forbid, we must also consider native loader program accounts are executable or not when calculating capitalization to account for special handling for genuine program accounts: https://github.com/solana-labs/solana/pull/11927/files#diff-2099c5256db4eb5975c8834af38f6456R1577. On the other hand, sysvar is just enough with owner check.

Ultimately, I just want simplicity and consistency between specially-owned accounts (sysvars and native programs) Also, I think fewer abnormal situation will lead to less bugs.

Also, if forbidding for native loaders can be optional, why sysvar is forbidden btw? I thought lack of forbid of native loaders was just a oversight. :)

[mvines]

Also, if forbidding for native loaders can be optional, why sysvar is forbidden btw? I thought lack of forbid of native loaders was just a oversight. :)

It's certainly nicer to forbid transfers for completeness. But it doesn't seem strictly necessary, nobody will be able to realistically generate a privatekey for any of these well-known address, so we can be quite sure that they're either unallocated or somebody transferred some SOL to that address, which is effectively a token burn. So I feel like we can just overwrite what ever is in that account regardless of the current content.

I'm not opposed to disabling transfers here. I just don't feel strongly either

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[stale]

This stale pull request has been automatically closed. Thank you for your contributions.

[ryoqun]

• MUST [ ] needs tests for this

[ryoqun]

will be superseded by #13884