Record instructions which are precompiles

[Lichtso]

Problem

See #24705

Summary of Changes

Don't just skip instructions when is_precompile by continue,
instead have them be recorded in the transaction_context.

Additionally feature gated behind record_instruction_in_transaction_context_push.

Fixes #24705
Updates Feature Gate: #24965

[jstarry]

Would it be too hacky to do self.transaction_context.record_precompile_ix() which directly does self.instruction_trace.push(vec![])? Looks like it would work ok here:

/// Convert from an InstructionTrace to InnerInstructionsList
pub fn inner_instructions_list_from_instruction_trace(
    instruction_trace: &InstructionTrace,
) -> InnerInstructionsList {
    instruction_trace
        .iter()
        .map(|inner_instructions_trace| {
            inner_instructions_trace
                .iter()
                .skip(1)
                .map(|instruction_context| {

[Lichtso]

Do you mean moving

                invoke_context
                    .transaction_context
                    .push(
                        program_indices,
                        &instruction_accounts,
                        &instruction.data,
                        true,
                    )
                    .and_then(|_| invoke_context.transaction_context.pop())

into a function TransactionContext::record_precompile_ix()?

Or to use TransactionContext::record_instruction() in the message_processor?
Because that is already on its way out.

Or, do you want to not actually record anything but sneak in fake entries in inner_instructions_list_from_instruction_trace()?
That would cause problems with the sibling instruction introspection I think.

[jstarry]

Or, do you want to not actually record anything but sneak in fake entries in inner_instructions_list_from_instruction_trace()?

Yeah, this.

That would cause problems with the sibling instruction introspection I think.

Good point, could we protect against this with an enum? Precompiles will always need special handling so might be worth doing something like this:

pub type InstructionTrace = Vec<InstructionTraceRecord>;

pub enum InstructionTraceRecord {
  Precompile { program_id: Pubkey },
  ProgramInstruction(Vec<InstructionContext>),
}

[jstarry]

I'm just hoping we can find a way to avoid adding too much complexity.. the precompile transition has had a lot of bugs from corner cases

[Lichtso]

The enum sounds like a lot more complexity. Keep in mind that in ABIv2 the instruction stack, trace and record are all the same thing. And it is designed to reflect the incoming message as close as possible.

[jstarry]

The enum sounds like a lot more complexity. Keep in mind that in ABIv2 the instruction stack, trace and record are all the same thing. And it is designed to reflect the incoming message as close as possible.

How do precompiles fit into ABIv2?

[Lichtso]

That is actually a good question as they were not taken into account yet.
So with the current implementation they would just sit there in the instruction trace (so other instructions can see them) but not be executed. That way the instruction trace stays in sync with the message as long as no error occurs.

[jstarry]

Got it, well I suggested the enum because I'd prefer avoiding trying to make a precompile act like an instruction because it's bound to introduce bugs. Using an enum forces us to handle precompiles explicitly.

[jstarry]

Looks good. A bit hard to follow but I think when these feature gates get cleaned up in the future this should get more simplified.

[jstarry]

Can you && this with the check that the prevent_calling_precompiles_as_programs feature is active? Technically there are no "precompiles" unless that feature is active. We should also ensure that the usage of is_precompile below also incorporates the feature gate check.

Pull request has been modified.

[mergify]

automerge label removed due to a CI failure