Make CostTracker aware of inflight transactions

[steviez]

Problem

When a leader is packing a Bank, transactions are added to the cost-tracker and then later updated or removed, depending on whether the transactions were committed or not. However, it is also possible for a Bank to get frozen while these transactions are "in-flight".

After the bank is frozen, the bank is sent over to CostUpdateService almost immediately. The result is that we've observed a Bank having its' cost details reported to metrics BEFORE the updates / removals were made to the CostTracker for these in-flight transactions. This causes a leader to submit metrics with an over-reported cost in comparison to what we calculate from replaying the block.

Taking over work done by @apfitzge in #398

Summary of Changes

• Add in-flight transaction count to CostTracker
• Increment / decrement the in-flight transaction count in BankingStage
• Make CostUpdateService wait (with a timeout) for in-flight transaction count to settle to zero

Fixes #366

[steviez]

Some alternatives that were considered:

• Andrew pursued an alternative option in CostTracker - wait for in-flight transactions before reporting #373 that would put a lock in Bank and have the updating / reporting thread synchronize through that
• Another possible alternative is holding the freeze lock longer. Currently, it is dropped here inside Consumer::execute_and_commit_transactions_locked():
  

  agave/core/src/banking_stage/consumer.rs

  Line 701 in 01314a4

  drop(freeze_lock);

  
  If we returned the held freeze lock to the caller (Consumer::process_and_record_transactions_with_pre_results()), we could process the cost updates/removals with the lock held (ie release at the end of this block):
  

  agave/core/src/banking_stage/consumer.rs

  Lines 497 to 531 in 01314a4

  	let mut execute_and_commit_transactions_output =
  	self.execute_and_commit_transactions_locked(bank, &batch);
  	// Once the accounts are new transactions can enter the pipeline to process them
  	let (_, unlock_us) = measure_us!(drop(batch));
  	let ExecuteAndCommitTransactionsOutput {
  	ref mut retryable_transaction_indexes,
  	ref execute_and_commit_timings,
  	ref commit_transactions_result,
  	..
  	} = execute_and_commit_transactions_output;
  	// Costs of all transactions are added to the cost_tracker before processing.
  	// To ensure accurate tracking of compute units, transactions that ultimately
  	// were not included in the block should have their cost removed.
  	QosService::remove_costs(
  	transaction_qos_cost_results.iter(),
  	commit_transactions_result.as_ref().ok(),
  	bank,
  	);
  	// once feature `apply_cost_tracker_during_replay` is activated, leader shall no longer
  	// adjust block with executed cost (a behavior more inline with bankless leader), it
  	// should use requested, or default `compute_unit_limit` as transaction's execution cost.
  	if !bank
  	.feature_set
  	.is_active(&feature_set::apply_cost_tracker_during_replay::id())
  	{
  	QosService::update_costs(
  	transaction_qos_cost_results.iter(),
  	commit_transactions_result.as_ref().ok(),
  	bank,
  	);
  	}

  
  It doesn't look like a lot of stuff to hold the lock for, but in any case, does seem a bit riskier than the approach this PR does

[apfitzge]

I don't think holding the freeze lock for longer will work, we'd also have to acquire the lock earlier. Right now we only hold freeze lock during (record, commit), but not execution.

I think that approach is riskier and not sure we should be backporting that.
A long execution, possibly due to bug?, would then prevent other threads from moving on to the next bank.

[apfitzge]

lgtm - let's get approval from second set of eyes before merging.

[steviez]

I don't think holding the freeze lock for longer will work, we'd also have to acquire the lock earlier. Right now we only hold freeze lock during (record, commit), but not execution.

Ah right, thank you. So yeah, VERY significantly extending the duration of holding that lock

I think that approach is riskier and not sure we should be backporting that. A long execution, possibly due to bug?, would then prevent other threads from moving on to the next bank.

Yep, 100% agreed on that approach being much riskier

[steviez]

lgtm - let's get approval from second set of eyes before merging.

@t-nelson - I think you've followed along with the issue this PR aims to address so adding you as a reviewer with Tao being OOO this week

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 96.29630% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 81.9%. Comparing base (182d27f) to head (c30bbbb).
Report is 6 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #437   +/-   ##
=======================================
  Coverage    81.8%    81.9%           
=======================================
  Files         842      842           
  Lines      228492   228518   +26     
=======================================
+ Hits       187104   187165   +61     
+ Misses      41388    41353   -35     

[bw-solana]

Overall LGTM. I think we could optimize the read locking on the cost reporting

[bw-solana]

LGTM

[steviez]

c30bbbb 😬

[apfitzge]

lgtm - how did I misss the report remove 🤦

[mergify]

Backports to the beta branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis.

[mergify]

Backports to the stable branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule.