Test that tick slot hashes update the recent blockhash queue

[jstarry]

Problem

There seems to be a common misconception that skipped slots do not impact the recent blockhash queue. There is no test to observe how this works.

Summary of Changes

• Added test to observe how tick slots impact the age of blockhash queue hash entries
• Added Bank::get_hash_age for use in above test
• Moved the Default implementation for BlockhashQueue into the blockhash_queue module because I can never find it
• Factored out new confirm_slot_entries function from blockstore_processor::confirm_slot_entries to make it easier to test the full verification flow for entries

Fixes #

[jstarry]

Note that this implies that using "last valid block height" instead of "last valid slot" for transaction expiration is actually incorrect because it assumes that skipped slots don't affect transaction expiration. This test shows that hashes for skipped slots are also added to the recent blockhash queue and so slots should be used for transaction expiration. Using block height for detecting expiration results in clients waiting longer than they need to (cc @t-nelson @CriesofCarrots @mvines)

[codecov]

Codecov Report

Merging #24242 (37c2cd6) into master (2da4e3e) will increase coverage by 0.1%.
The diff coverage is 79.1%.

@@            Coverage Diff            @@
##           master   #24242     +/-   ##
=========================================
+ Coverage    81.8%    81.9%   +0.1%     
=========================================
  Files         581      580      -1     
  Lines      158312   161329   +3017     
=========================================
+ Hits       129518   132268   +2750     
- Misses      28794    29061    +267     

[ryoqun]

(wow, is this really? it's big news to me... i'll get to review on this later...)

[jstarry]

(wow, is this really? it's big news to me... i'll get to review on this later...)

Yeah I was quite surprised too, let me know if you want to me to walk through how I reached this conclusion. I'll let you dig in first :)

[CriesofCarrots]

Oh. Wow.
I was about to type something about "how do we undeprecate a field, ie last_valid_slot", but just saw this: #23949 (comment) Is it doable?

[mvines]

lol wut.

doesn't this mean that if 150 slots are skipped in a row, there does not exist a valid blockhash for future votes?

[t-nelson]

tx forwards are a single hop. txs expire after 150 blocks. what's next?!

nice find! think it'll be enough to stop putting new ticks in or will there be side effect?

[ryoqun]

hmm, i think i found a counter-example?: https://explorer.solana.com/tx/5ecnMQYQhZEsup2YPr4Vii3skuvhx7VPXTXVVioVv96PhrkXwZhjbgpEzS8pnq4TdcRXDyuCQnga7wLG5GsDjJwX

this transaction is included in slot 117,746,642 with recent_blockhash Du5KNR1zXVucMpPzJuPRvt9XXHwMTXsaCWJQQqUe7GRG, which in turn refers to the slot 117,746,491

$ pry
[1] pry(main)> 117746642 - 117746491
=> 151 # greater than MAX_PROCESSING_AGE!

then, let's see block heights:

$ ./target/release/solana block 117746491 2> /dev/null | head
Slot: 117746491
Parent Slot: 117746490
Blockhash: Du5KNR1zXVucMpPzJuPRvt9XXHwMTXsaCWJQQqUe7GRG
Previous Blockhash: AzVaX69Z3YdCnuonpMfY48REUU8cJHbf7oyFuASYptTF
Block Time: 2022-01-25T17:45:38+09:00
Block Height: 106258160

$ ./target/release/solana block 117746642 2> /dev/null | head
Slot: 117746642
Parent Slot: 117746641
Blockhash: Czw8eVGqVpz843x3QBRbm4mQ78ZQAmHuTeFNGvv1Bj1X
Previous Blockhash: E6ii2Dg49Csb4u6dHdjZSJvfdmE25Kn92v5apk5rVJit
Block Time: 2022-01-25T17:47:01+09:00
Block Height: 106258298

$ pry
[1] pry(main)> 106258298 - 106258160
=> 138 # under MAX_PROCESSING_AGE!

[jstarry]

You have to find a counter example greater than 151 because we allow processing transactions that are 151 slots behind the current block. I have a test to show this strange off-by-one behavior here:

solana/runtime/src/blockhash_queue.rs

Lines 250 to 252 in d54ec40

	// Note that the "age" of a hash in the queue is 0-indexed. So when processing
	// transactions in block 50, the hash for block 49 has an age of 0 despite
	// being one block in the past.

[jstarry]

Any objections to merging this? I have a fix lined up here: #24389

[ryoqun]

lgtm!

[ryoqun]

loves these tiny clean-ups.