Fix nonce fee_calculator overwrite

[CriesofCarrots]

Problem

The fee calculator in a nonce account will never update from its initial value, even as cluster tx fees change and the nonce is used. This is because prepare_if_nonce_account(), intended to advance the nonce on an InstructionError, always overwrites the fee_calculator with the account's original fee calculator.

Summary of Changes

• Plumb bank fee_calculator into prepare_if_nonce_account() so that it updates to the correct fee calculator
• Also, only update the account in the error case, since the account is already updated on success:

  solana/sdk/src/nonce/account.rs

  Line 66 in 4ef8f89

  self.set_state(&Versions::new_current(State::Initialized(new_data)))

Enables failing test in #10967

[mvines]

1.1 backport as well?

[CriesofCarrots]

1.1 backport as well?

I say yes, assuming there will be more 1.1 releases.

[mvines]

1.1 backport as well?

I say yes, assuming there will be more 1.1 releases.

Yep on demand as needed until we upgrade mainnet-beta to 1.2.

Are we going to need to gate the rollout of this PR? It looks like it might cause a bank hash difference with/without

[t-nelson]

lgtm! Nice catch 🙏

👍 v1.1 backport

[CriesofCarrots]

Are we going to need to gate the rollout of this PR? It looks like it might cause a bank hash difference with/without

It's actually fairly unlikely to cause a bank hash mismatch, since cluster fees aren't moving afaict. But it is technically possible.
Can you walk me through the gate process?

[mvines]

Are we going to need to gate the rollout of this PR? It looks like it might cause a bank hash difference with/without

It's actually fairly unlikely to cause a bank hash mismatch, since cluster fees aren't moving afaict. But it is technically possible.

Ah, ok. Phew. Yeah with the low TPS right now on mainnet-beta it's very unlikely that the fee would increase during an affected slot.

Can you walk me through the gate process?

Essentially for each operating_mode, we pick a slot or epoch when we want to switch over from the old to new behavior. Then for testnet/mainnet-beta, ensure everybody upgrades before that slot/epoch hits

[CriesofCarrots]

Ah, ok. Phew. Yeah with the low TPS right now on mainnet-beta it's very unlikely that the fee would increase during an affected slot.

Does this mean you are okay forgoing the gating? @mvines

[mvines]

Does this mean you are okay forgoing the gating? @mvines

Yeah, l think so. @t-nelson - can you coordinate the mainnet-beta rollout of this though please. Perhaps a new 1.1 release for next Monday

[codecov]

Codecov Report

Merging #10973 into master will decrease coverage by 0.0%.
The diff coverage is 96.1%.

@@            Coverage Diff            @@
##           master   #10973     +/-   ##
=========================================
- Coverage    82.1%    82.1%   -0.1%     
=========================================
  Files         318      318             
  Lines       72747    72804     +57     
=========================================
+ Hits        59782    59823     +41     
- Misses      12965    12981     +16     

[ryoqun]

@CriesofCarrots I think this causes diverged bank hash because patched validator and unpatched validator save different things into the AccountsDB if tx_result.is_err().

So, I guess this needs gated release, actually.

FYI: @jstarry @mvines @t-nelson

[CriesofCarrots]

Unless we're seeing any fee fluctuation, I don't think this saves anything different if tx_result.is_err(). I think the issue may actually be with the success case, because the blockhash returned from the blockhashes sysvar in system_instruction_processor (and retained in the nonce account as of this pr) may not be the same as that returned from the bank.last_blockhash_with_fee_calculator. @t-nelson , can you confirm?

[t-nelson]

@CriesofCarrots I believe they should match. The RecentBlockhashes sysvar is updated in Bank::new_from_parent() and we don't touch bank.blockhash_queue until the next slot boundary

[CriesofCarrots]

@t-nelson see test at CriesofCarrots@5239ed4
I believe there is one tick at the block boundary where that isn't true