Fix native invoke writable privileges

[jackcmay]

Problem

Caller write privileges are not created correctly for native cpi. The caller_write_privileges vector should match up with message.account_keys because they will be indexed the same.

Summary of Changes

• Remove incorrect write privilege value insertion
• Match up write privileges vector with the accounts vector built from message.account_keys

Fix sourced from: #18616, thanks @jstarry !

Fixes #18629

[jstarry]

Looking now.. But first, can you also add a feature switch? There are two places where this will change behavior:

• Reduce payer balance needed to deploy programs #19645 (I'll update this to piggy back off the new feature switch for this PR)
• Add address map program #19473 (will have its own feature switch that will need to be dependant on the feature switch in this PR)

Pull request has been modified.

Pull request has been modified.

[codecov]

Codecov Report

Merging #19750 (85fa41e) into master (36f870a) will decrease coverage by 0.0%.
The diff coverage is 88.4%.

@@            Coverage Diff            @@
##           master   #19750     +/-   ##
=========================================
- Coverage    82.5%    82.5%   -0.1%     
=========================================
  Files         476      476             
  Lines      132822   132964    +142     
=========================================
+ Hits       109708   109792     +84     
- Misses      23114    23172     +58     

[Lichtso]

This all boils down to this, right?

let caller_keyed_accounts = invoke_context.get_keyed_accounts()?;
let mut caller_write_privileges = Vec::with_capacity(message.account_keys.len());
if invoke_context.is_feature_active(&fix_write_privs::id()) {
    for key in message.account_keys.iter() {
        let index = caller_keyed_accounts
            .iter()
            .position(|keyed_account| keyed_account.unsigned_key() == key)
            .ok_or(InstructionError::MissingAccount)?;
        caller_write_privileges.push(caller_keyed_accounts[index].is_writable());
    }
} else {
    caller_write_privileges.push(false);
    for index in keyed_account_indices.iter() {
        caller_write_privileges.push(caller_keyed_accounts[*index].is_writable());
    }
    // caller_write_privileges.insert(0, false);
}

And because message.account_keys is derived from caller_keyed_accounts and keyed_account_indices like this:

let callee_keyed_accounts = keyed_account_indices
    .iter()
    .map(|index| keyed_account_at_index(caller_keyed_accounts, *index))
    .collect::<Result<Vec<&KeyedAccount>, InstructionError>>()?;
let (message, callee_program_id, _) = Self::create_message(
    &instruction,
    &callee_keyed_accounts,
    signers,
    &invoke_context,
)?;

The only real difference remaining is the:
caller_write_privileges.push(false); or caller_write_privileges.insert(0, false); if fix_write_privs is off.

@jackcmay Do we have a test to highlight the difference? Because I have my doubts that this change actually changes anything (except for the first entry and thus everything being shifted by one).

[Lichtso]

Found the new test: message_processor::tests::test_native_invoke

And the flaw in my reasoning:
let message = Message::new(&[instruction.clone()], None);
The message.account_keys only depend on instruction not on the caller_keyed_accounts.

But then the question is: Why do we even pass keyed_account_indices into native_invoke only for it to possibly diverge?
Why not only derive keyed_account_indices_reordered from message.account_keys?
And aren't the callee_keyed_accounts possibly wrong as well?

[jackcmay]

We may not need to pass keyed_account_indices, I'll take a look at that

[Lichtso]

Here, I refactored it already but still waiting for CI tests:
https://github.com/Lichtso/solana/tree/refactor/cpi_syscall_account_reuse

I think InstructionProcessor::create_message() could handle gathering the callee_keyed_accounts and caller_write_privileges both in the CPI syscall and in native_invoke.

[jackcmay]

That would be nice to consolidate

[Lichtso]

That would be nice to consolidate

Addressed in #19762