Skip to content

Fix turbine to sign last fec set if last entry in slot #6887

Merged
maheshr merged 6 commits intoanza-xyz:masterfrom
maheshr:fix_turbine_to_sign_last_fec_set_if_last_slot_in_entry
Aug 29, 2025
Merged

Fix turbine to sign last fec set if last entry in slot #6887
maheshr merged 6 commits intoanza-xyz:masterfrom
maheshr:fix_turbine_to_sign_last_fec_set_if_last_slot_in_entry

Conversation

@maheshr
Copy link
Copy Markdown

@maheshr maheshr commented Jul 8, 2025

Problem

Addressing issue #6698
Turbine creates more resigned shreds than necessary

Summary of Changes

Changed make_shreds_from_data to have only last fec_set signed if last in slot. Updated unit tests to only check fec_set when last in slot.

Ran Agave on test-net with and without my changes. Inspected several metrics (gen_data_time, gen_coding_time, data_bytes, padding_bytes, num_coding_shreds, num_data_shreds, num_merkle_data_shreds, num_merkle_coding_shreds) to ensure there was no performance degradation. Inspected num_repaired on another Anza validator to ensure that there were no unusual repair requests around the times the validator with my changes was leader.

@maheshr maheshr requested a review from gregcusack July 8, 2025 21:43
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jul 9, 2025

Codecov Report

❌ Patch coverage is 72.18045% with 37 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.0%. Comparing base (785455b) to head (2866349).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##           master    #6887     +/-   ##
=========================================
- Coverage    83.1%    83.0%   -0.1%     
=========================================
  Files         812      812             
  Lines      356900   356979     +79     
=========================================
+ Hits       296612   296639     +27     
- Misses      60288    60340     +52     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread ledger/src/shred.rs Outdated
@alexpyattaev
Copy link
Copy Markdown

Would it be possible to add a test that creates varied (random) amounts of data for a slot via shredder API, makes shreds, and then confirms that exactly 64 last shreds are resigned, no more and no less?

@maheshr
Copy link
Copy Markdown
Author

maheshr commented Aug 22, 2025

Would it be possible to add a test that creates varied (random) amounts of data for a slot via shredder API, makes shreds, and then confirms that exactly 64 last shreds are resigned, no more and no less?

Hey Alex - There are several existing tests that check if the shreds are last in slot. I modified them to only sign the last fec set. See test_make_shreds_from_data, test_make_shreds_from_data_paranoid, and test_make_shreds_from_data_rand. They use run_make_shreds_from_data and I made the change there to only check if the last batch is signed if data is last in slot. Let know if that doesn't solve for test you've asked for. Thanks!

@maheshr maheshr marked this pull request as ready for review August 22, 2025 04:38
@maheshr maheshr requested a review from alexpyattaev August 22, 2025 04:39
Comment thread ledger/src/shred/merkle.rs Outdated
@alexpyattaev alexpyattaev changed the title Fix turbine to sign last fec set if last slot in entry Fix turbine to sign last fec set if last entry in slot Aug 22, 2025
Copy link
Copy Markdown

@gregcusack gregcusack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a few initial thoughts

Comment thread ledger/src/shred/merkle.rs
Comment thread ledger/src/shred/merkle.rs Outdated
Comment thread ledger/src/shred.rs
Copy link
Copy Markdown

@alexpyattaev alexpyattaev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM thank you!

@alexpyattaev
Copy link
Copy Markdown

Hey Alex - There are several existing tests that check if the shreds are last in slot. I modified them to only sign the last fec set. See test_make_shreds_from_data, test_make_shreds_from_data_paranoid, and test_make_shreds_from_data_rand. They use run_make_shreds_from_data and I made the change there to only check if the last batch is signed if data is last in slot. Let know if that doesn't solve for test you've asked for. Thanks!

Yeah those should do fine.

Copy link
Copy Markdown

@gregcusack gregcusack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm! thank you!

let resigned = chained && is_last_in_slot;

// only sign last batch if it is chained and is the last in slot
// let resigned = chained && is_last_in_slot;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: looks like this was left in. can be deleted:

// let resigned = chained && is_last_in_slot;

^ you can do this in a follow up PR

@maheshr maheshr merged commit 5a609c8 into anza-xyz:master Aug 29, 2025
43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants