cleanup unchained continuation

[puhtaytow]

Problem

There is still remaining parts of code that allows for unchained shreds. The unchained shreds are nomore in production.
Cleaning it all at once would result in hard to review PR, that is why taking iterative approach seem to be the way.

This PR is one of many, towards the unchained shreds removal.

Summary of Changes

• Removed option from chained_merkle_root param:

  • blockstore/setup_erasure_shreds_with_index_and_chained_merkle
  • blockstore/setup_erasure_shreds_with_index_and_chained_merkle_and_last_in_slot
  • ledger/benches/make_shreds_from_entries
  • ledger/shredder/entries_to_merkle_shreds_for_tests
  • ledger/shredder/make_merkle_shreds_from_entries
  • ledger/shredder/make_shreds_from_data_slice

• Removed chained switch param in:

  • ledger/sigverify_shreds/make_shreds
  • ledger/shred/make_merkle_shreds_for_tests
  • ledger/tests

• Removed last part of test test_check_last_fec_set in blockstore as suggested

[puhtaytow]

im not sure what to do with this test https://github.com/puhtaytow/agave/blob/25bbb89f961e5289b273af7180924728bab68d0b/ledger/src/blockstore.rs#L11747 it explicitly says that None variant has to be used, tagging @AshwinSekar as you added the comment there 🙏

can we just remove the test last part*? its blocking me from removing Option at setup_erasure_shreds_with_index_and_chained_merkle_and_last_in_slot, which is blocking further at entries_to_merkle_shreds_for_tests

[puhtaytow]

@alexpyattaev @steviez could you have a look when you find a moment? 🙏

[alexpyattaev]

im not sure what to do with this test https://github.com/puhtaytow/agave/blob/25bbb89f961e5289b273af7180924728bab68d0b/ledger/src/blockstore.rs#L11747 it explicitly says that None variant has to be used, tagging @AshwinSekar as you added the comment there 🙏

can we just remove the test last part*? its blocking me from removing Option at setup_erasure_shreds_with_index_and_chained_merkle_and_last_in_slot, which is blocking further at entries_to_merkle_shreds_for_tests

I would kill that part of the test.

[steviez]

The changes from this PR aren't too crazy, I think we can remove Option from this function and update all callers at least:

agave/ledger/src/shredder.rs

Line 72 in b984691

chained_merkle_root: Option<Hash>,

I haven't dug through the functions beneath that, but I'm inclined to think that removing Option throughout shouldn't be too bad

[puhtaytow]

Took your suggestions and now its cleaned up till shredder/make_merkle_shreds_from_entries, which already result in changes across 22 files.

Please have a look when you find a moment 🙏 if you think it should be splitted then let me know.

[steviez]

Wanna update this one too ?

agave/ledger/src/shred.rs

Lines 1014 to 1020 in f9294b1

	pub(super) fn make_merkle_shreds_for_tests<R: Rng>(
	rng: &mut R,
	slot: Slot,
	data_size: usize,
	chained: bool,
	is_last_in_slot: bool,
	) -> Result<Vec<merkle::Shred>, Error> {

Took your suggestions and now its cleaned up till shredder/make_merkle_shreds_from_entries, which already result in changes across 22 files.

Yep, we have touched a handful of files. However, all of the changes are of the same type / pretty easy to follow. So, I don't mind lumping them all into a PR like this.

On the other hand, the logic below shred::merkle::make_shreds_from_data() is deserving of more scrutiny. So, getting all of the mechanical changes in this PR should allow for a pretty small diff in the subsequent PR

[steviez]

note to other reviewers - there are other instances where we use Hash::default() so changing None to that here shouldn't be an issue.

No code changes being requested, just calling this out

[codecov-commenter]

Codecov Report

❌ Patch coverage is 94.44444% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.9%. Comparing base (25bbb89) to head (e3d4e4f).
⚠️ Report is 54 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##           master    #8079     +/-   ##
=========================================
- Coverage    82.9%    82.9%   -0.1%     
=========================================
  Files         823      823             
  Lines      360421   360393     -28     
=========================================
- Hits       299016   298949     -67     
- Misses      61405    61444     +39     

🚀 New features to boost your workflow:

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

[puhtaytow]

Wanna update this one too ?

Good point. Fixed.

the logic below shred::merkle::make_shreds_from_data() is deserving of more scrutiny. So, getting all of the mechanical changes in this PR should allow for a pretty small diff in the subsequent PR

Makes sense.

---

I fixed stuff that you addressed, lets wait what Ashwin would say about the last part of test. To my understanding we omitt this way testing not-retransmitted full one, but unchained shreds aren't real now, so it could only be a negative test? and question would be if its needed and if it is needed, then that might be good thing to address in the #8048

[alexpyattaev]

LGTM

[puhtaytow]

Thank you guys 🥇