Inner instruction indexes don't increment for precompiles

[jstarry]

Problem

In the transaction's metadata we store a record of the inner instructions that were called by a program. Those are matched up to transaction instructions using the index field but transaction instructions that are precompiles don't cause the inner ix index to be incremented.

In the following example, the inner instructions index should be 7, not 6.

https://explorer.solana.com/tx/4oDFDuBTKxth5kyfnpzTNEUnUjYWgEFByYELKb9P1VhEp4QpYE9qWy9Eyn1x5rM5MCDcrKuwrjoJQ7rNQt3fYeK3?cluster=testnet

Example response

{
  "jsonrpc": "2.0",
  "result": {
    "blockTime": 1650901869,
    "meta": {
      "err": null,
      "fee": 10000,
      "innerInstructions": [
        {
          "index": 6,
          "instructions": [
            {
              "parsed": {
                "info": {
                  "destination": "9w61eejrbNTC9CddP5vUVe3QYyQaPKaruP4cguKkmPzZ",
                  "lamports": 5000,
                  "source": "EJUKLLjBMhFnkonfn7wcThnHyDewmhVmG9sEuVP9cvF8"
                },
                "type": "transfer"
              },
              "program": "system",
              "programId": "11111111111111111111111111111111"
            },
            {
              "accounts": [],
              "data": "2kik7JJwphwg1LCJHPyAGRmL9vgKTmkg17wxo45JiAkyn8SruXyzePByitozMpWpeWpJdd4gsBGrf4sxLAXqYhxLsocu2Cjv82mDLaYj21JkkoZysk2pzV1KcYCgNU7",
              "programId": "eeLSJgWzzxrqKv1UxtRVVH8FX3qCQWUs9QuAjJpETGU"
            },
            {
              "accounts": [],
              "data": "LmxsQLBdTJBNF",
              "programId": "eeLSJgWzzxrqKv1UxtRVVH8FX3qCQWUs9QuAjJpETGU"
            }
          ]
        }
      ],
    },
    "slot": 129964372,
    "transaction": {
      "message": {
        "accountKeys": [],
        "addressTableLookups": null,
        "instructions": [
          {
            "accounts": [], // 0
            "data": "16TYTJ8fLSxF",
            "programId": "ComputeBudget111111111111111111111111111111"
          },
          {
            "accounts": [], // 1
            "data": "7YXqSw",
            "programId": "ComputeBudget111111111111111111111111111111"
          },
          {
            "accounts": [], // 2
            "data": "16TYTJ8fLSxF",
            "programId": "ComputeBudget111111111111111111111111111111"
          },
          {
            "accounts": [], // 3
            "data": "7YXqSw",
            "programId": "ComputeBudget111111111111111111111111111111"
          },
          {
            "accounts": [], // 4
            "data": "16TYTJ8fLSxF",
            "programId": "ComputeBudget111111111111111111111111111111"
          },
          {
            "accounts": [], // 5
            "data": "7YXqSw",
            "programId": "ComputeBudget111111111111111111111111111111"
          },
          {
            "accounts": [ // 6
              "KeccakSecp256k11111111111111111111111111111"
            ],
            "data": "2CgVohjrBkqeYWvA",
            "programId": "KeccakSecp256k11111111111111111111111111111"
          },
          {
            "accounts": [ // 7
              "Sysvar1nstructions1111111111111111111111111",
              "EJUKLLjBMhFnkonfn7wcThnHyDewmhVmG9sEuVP9cvF8",
              "9w61eejrbNTC9CddP5vUVe3QYyQaPKaruP4cguKkmPzZ",
              "Hb47E8THZhVs7P4hSWCHsk175qk5vTp2mmqdcRD4pQNb",
              "11111111111111111111111111111111",
              "eeLSJgWzzxrqKv1UxtRVVH8FX3qCQWUs9QuAjJpETGU",
              "6pgKoyyTfRMfrsq8xz3RvmwBUL2GpEe7Z7uBuJE7KYvt",
              "9N3nkyUfmWTvfYvYJi1xVYHW1pyScPsAByccpsoP27S8",
              "3EZXVf4fDhLhrLEyyv6uKkicVnUm5omCzzy5aG6Wnfym",
              "TokenkegQfeZyiNwAJbNbGKPFXCWuBvf9Ss623VQ5DA"
            ],
            "data": "3BtaPdCKZxzHJhbrChGuWjc3HGVAzkKZ8QYHPojpKWpd39PCEyPqqEDf29cV1gD2UUiBVRjTnaPXfWLUmr7M3RaPJCW3xVLpUd4bzYMwEwYKUowVXETpWe4SJT7Dy1nzH6Kb5JJ77rydYA8HhEFCs6ckng7Tk5G2SGDB16LammV7LmtEz3BaX9AJDWYhNHHYCj4iaSf33r7QQ29KrLMYwTk754eJ49Ez59wLnKJf",
            "programId": "eeLSJgWzzxrqKv1UxtRVVH8FX3qCQWUs9QuAjJpETGU"
          }
        ],
        "recentBlockhash": "EvRxMd4LeYQxkPQvnTmN7reY3ndQBhtHVgFW5otbAssz"
      },
      "signatures": [
        "4oDFDuBTKxth5kyfnpzTNEUnUjYWgEFByYELKb9P1VhEp4QpYE9qWy9Eyn1x5rM5MCDcrKuwrjoJQ7rNQt3fYeK3"
      ]
    }
  },
  "id": "ef6086fc-0179-48d5-973e-3301119c4491"
}

Proposed Solution

Ensure that the inner ix index is incremented for precompiles as well

[Lichtso]

I guess this continue statement is too early.

            if invoke_context
                .feature_set
                .is_active(&prevent_calling_precompiles_as_programs::id())
                && is_precompile(program_id, |id| invoke_context.feature_set.is_active(id))
            {
                // Precompiled programs don't have an instruction processor
                continue;
            }

And thus is record_instruction not called for precompiles:

                self.transaction_context
                    .record_instruction(InstructionContext::new(
                        nesting_level,
                        program_indices,
                        instruction_accounts,
                        instruction_data,
                    ));

@jstarry Does this also occur if prevent_calling_precompiles_as_programs is disabled?

[jstarry]

I guess this continue statement is too early.

Yup, that's my read as well

@jstarry Does this also occur if prevent_calling_precompiles_as_programs is disabled?

I don't think so. But that feature is enabled on all clusters now anyways

[Lichtso]

Last functional change appears to be #19930, the continue was added back in 1.8

[jstarry]

I don't think so. But that feature is enabled on all clusters now anyways

My mistake, it's not enabled on mainnet-beta yet. And as your new PR shows, the missing call to TransactionContext::push for precompiles is the issue so without the feature activated, the index is correct.

[Lichtso]

The feature gates here are going to be tricky as this is already somewhat of a mess.

prevent_calling_precompiles_as_programs should not be activated on MNB as that would corrupt the instruction record, right?
The patch I prepared depends on record_instruction_in_transaction_context_push, which is not active anywhere yet. So the patch could be backported to simply go under that feature gate as well.

[jstarry]

prevent_calling_precompiles_as_programs should not be activated on MNB as that would corrupt the instruction record, right?

Yeah, I think that's best. I added a note here so that jack is aware: #24746 (comment)

The patch I prepared depends on record_instruction_in_transaction_context_push, which is not active anywhere yet. So the patch could be backported to simply go under that feature gate as well.

I don't know anything about that feature so I'll take a look at your fix and respond in a bit

[jstarry]

I just realized that this bug breaks the behavior of the get_processed_sibling_instruction syscall because the indexing will be wrong. So we cannot enable the prevent_calling_precompiles_as_programs feature on mainnet until this is fixed. @Lichtso do you agree?

[Lichtso]

Yes, the bug in prevent_calling_precompiles_as_programs breaks the indices in the instruction record and get_processed_sibling_instruction().

Therefore, prevent_calling_precompiles_as_programs should not be enabled on MNB before record_instruction_in_transaction_context_push, as that should override its behavior and fix the bug.

[CriesofCarrots]

Okay, so to be clear, the add_get_processed_sibling_instruction_syscall feature is okay to be activated on mainnet-beta at any time, but record_instruction_in_transaction_context_push must be activated before prevent_calling_precompiles_as_programs?

[jstarry]

Okay, so to be clear, the add_get_processed_sibling_instruction_syscall feature is okay to be activated on mainnet-beta at any time, but record_instruction_in_transaction_context_push must be activated before prevent_calling_precompiles_as_programs?

Correct, see this comment on the prevent_calling_precompiles_as_programs feature tracker issue

[Lichtso]

Also, #24743 needs to be back-ported before record_instruction_in_transaction_context_push is activated.