broadcast: use block id from snapshot as the CMR for initial blocks

[AshwinSekar]

Problem

When we produce the first block after a snapshot, we might not have the shreds of the parent available to set as the chained merkle root.

Now that SIMD-0333 (#11355) is active on master, all banks will have a block id.

Summary of Changes

Use the parent bank's block id instead.

[AshwinSekar]

I've checked this before, but good to double check that there's no race that can cause this to fail:

If parent_block_id is our leader bank, we've set the block_id here

agave/turbine/src/broadcast_stage/standard_broadcast_run.rs

Line 424 in b6abf60

broadcast_utils::set_block_id_and_send(

Leader slots are done sequentially so there's no way that this expect can fail.

If this is a non-leader bank, it must be frozen ( we don't build on non-frozen banks ). Block id is set when frozen here

agave/core/src/replay_stage.rs

Lines 3579 to 3582 in b6abf60

	debug_assert!(block_id.is_some() || is_leader_block);
	if block_id.is_some() {
	bank.set_block_id(block_id);
	}

Finally if this was a snapshot bank, SIMD-0333 guarantees that it was set

agave/runtime/src/snapshot_utils.rs

Line 573 in b6abf60

block_id: Some(bank_fields.block_id),

agave/runtime/src/bank.rs

Line 2018 in b6abf60

block_id: RwLock::new(fields.block_id),

[bw-solana]

when did we start serializing this into the snapshot? Want to make sure there isn't some set of honest validators we could overlap with (e.g. FD or older version) that are producing snapshots without this

Edit: Looks like we just started serializing into snapshot last month.. does that mean 4.1 would be incompatible with 4.0 in some cases?

[AshwinSekar]

in 4.0 we added support to read the block id field #9644
and then in 4.1 we added support to write the block id field (but still keep else Hash::default() logic) #11355

So now in 4.2 I believe we can remove the else Hash::default() logic and hard unwrap and still have adjacent version compatibility

[bw-solana]

my concern was around some 4.0 produced snapshot not being able to be processed by 4.1 code + this change.

But if I'm understanding, it sounds like we will just populate Default hash in that case and not fail the expect

[AshwinSekar]

yeah sorry, I realized that master is not on 4.2 so you're correct this is problematic if we merge in 4.1.

The snapshot still allows for None in v4.1 for v4.0 compatibility

agave/runtime/src/bank.rs

Line 493 in b6abf60

pub(crate) block_id: Option<Hash>, // Option wrapper can be removed in version after v4.1

We can wait to merge this change until after the branch cut

[bw-solana]

I believe test_slot_interrupt is broken. Need to modify the test code to set the block ID for parent banks before handling the broadcast receive results

[bw-solana]

The test changes look a little gross, but whatever. Seems like we could just write the block ID directly - doesn't seem any worse than directly writing the tick height to max before recursing parents

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.2%. Comparing base (e25cdff) to head (1305d76).

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #12354   +/-   ##
=======================================
  Coverage    83.2%    83.2%           
=======================================
  Files         849      849           
  Lines      322523   322547   +24     
=======================================
+ Hits       268495   268519   +24     
  Misses      54028    54028           

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.