solana-labs / solana

Web-Scale Blockchain for fast, secure, scalable, decentralized apps and marketplaces.
https://solanalabs.com
Apache License 2.0
13.35k stars 4.35k forks source link

Apply transaction actual execution units to cost_tracker #24198

Closed tao-stones closed 2 years ago

tao-stones commented 2 years ago

Problem

To make cost tracking more accurate, can apply transactions actual execution units after successful execution.

Proposed Solution

Potentially can include actual execution units in

#[derive(Debug, Clone)]
pub struct TransactionExecutionDetails {
    pub status: Result<()>,
    pub log_messages: Option<Vec<String>>,
    pub inner_instructions: Option<InnerInstructionsList>,
    pub durable_nonce_fee: Option<DurableNonceFee>,
    pub return_data: Option<TransactionReturnData>,
}

So successfully executed Transaction can have that info ready for QosService::update_or_remove_transaction_costs()

Side note: The unsuccessfully executed transaction still consumes execution units, which will also be aggregated and reflected in estimated units.

buffalu commented 2 years ago

need to be careful about race condition where you accidentally build block too big, but this seems better

tao-stones commented 2 years ago

Regarding transaction's estimated and actual execution units:

  1. cost_model computes transaction's execution_cost by summing up all programs' units in transaction, including built-in programs (which are constants defined at block_cost_limits.rs), and BPF programs.
  2. the total estimated units are the sum of execution_cost and few other const cost, defined here.
  3. on the other hand, message_processor reports the BPF programs' units (include both succeeded or errored) at here, built-in programs are not included. (Or I am mistaken)

With these, my initial thought of bring actual units to banking_stage to apply to cost_tracker is: instead of returning each transaction's execution units from bank.execute_loaded_transaction (which would only include BPF programs), rather iterating each transaction's programs after execution to pick built-in units from constant def and BPF from execute_and_commit_timings.execute_timings.executeAccessoryTimings.per_program_timings to sum up the actual units.

The drawback is there will be one additional iteration of instructions per executed-and-recorded transactions.

What's your thoughts on this? @ckamm @carllin @jstarry

ckamm commented 2 years ago

I was hoping we could sum up the compute_units_consumed directly in MessageProcessor::process_message() and communicate them outward in ProcessedMessageInfo and further.

The direct path via the invoke context's data should also include built-in programs, since it reads the user-visible compute limit?

tao-stones commented 2 years ago

Builtin programs don't consume units at the moment (they may at some point), the accumulated execution units from process_message() are for BPF programs only. For the purpose of this issue, can do:

  1. pipe the sum of a transaction's BPF programs execution units to banking_stage, as actual_bpf_execution_units;
  2. update cost_tracker to track builtin and bpf programs separately.
  3. after execution, cost_tracker is updated by the diff between estimated and actual _bpfprograms units.
tao-stones commented 2 years ago

communicate them outward in ProcessedMessageInfo and further.

Mmm, looks like ProcessedMessageInfo is only for successful execution. To include failed execution, can compare ExecuteTimings before/after.