shredder introduce shred builder / new api

[puhtaytow]

Problem

Current Shredder API forces tests to pass all parameters explicitly, even when these are irrelevant. This makes unit tests unnecessairly verbose and harder to read/ work with.

• High boilerplate for simple tests
• Repeated setup of ReedSolomonCache and ProcessShredsStats, Keypair, and Entry data
• Manual handling of next_shred_index/next_code_index, even if not needed
• Tests must provide version, reference_tick, parent_slot in scenario where defaults would suffice

Summary of Changes

Introduce ShredBuilder, a type-state builder with fluent api to construct shreds for tests in ergonomic and flexible way:

• Data sources: random bytes, provided bytes and transactions
• Provides defaults: parent_slot = slot - 1, version = 0, reference_tick = 0, start_index = 0
• Optional overrides: with_parent_slot, with_version, with_reference_tick, with_start_index, with_invalid_index (specific failure tests)
• Internalizes Keypair, ReedSolomonCache and ProcessShredsStats out of box
• Returns an iterator of shreds for convinience
• Backward compatible in case of extending the fluent api with new setters

No changes to production shredding logic.

[puhtaytow]

@alexpyattaev if you could look when you have a minute and leave early comments 🙏

[alexpyattaev]

can we add tests for these as well and/or blend them into existing tests?

[alexpyattaev]

especially need test for this one, as well as doc-comments explaining each

[alexpyattaev]

Overall looking really nice, thank you!

[steviez]

Can you please update the PR description with the motivation for this PR. Namely, what is the problem with the current code and how does this PR address that problem / make things better

[puhtaytow]

Can you please update the PR description with the motivation for this PR. Namely, what is the problem with the current code and how does this PR address that problem / make things better

Sure, it wasn't there at the first point as i made the draft just so Alex could put early comments.
It is updated now, if you could read and tell me if something doesn't make sense then please 🙏

--edit

Please keep in mind as its still WiP.

[steviez]

Sure, it wasn't there at the first point as i made the draft just so Alex could put early comments

Ahh ok, I still got some notifications for it which is why I commented

It is updated now, if you could read and tell me if something doesn't make sense then please

Thank you !

Please keep in mind as its still WiP.

🫡

[alexpyattaev]

Why do we need this? Seems like the exact pattern we wanted to avoid no?

[puhtaytow]

It seemed for a moment like good way, but its clearly not. I'm workin on it 🙏

[steviez]

Current Shredder API forces tests to pass all parameters explicitly, even when these are irrelevant. This makes unit tests unnecessairly verbose and harder to read/ work with.

I agree with the sentiment that having to specify all the arguments in test code where we often use defaults is annoying.

That being said, I also typically try to avoid modifying "production" code for the sake of supporting "test" code. For example,

Provides defaults: parent_slot = slot - 1, version = 0, reference_tick = 0, start_index = 0

This changes makes test more manageable at the cost of making it easier to screw up production code. As is, the verbose code requires the caller to pass those parameters which requires them to think about it. This is good for the production code

As an alternative, what if we introduced a wrapper like TestShredder that held the "extra" state and a production Shredder internally.

Semi-related - I think it probably makes sense to sense to finish other cleanup (like making merkle_root a Hash instead of Option<Hash>) first

[puhtaytow]

what if we introduced a wrapper like TestShredder that held the "extra" state and a production Shredder internally.

I think it probably makes sense to sense to finish other cleanup (like making merkle_root a Hash instead of Option) first

Sure, ill switch to the cleanup and lets see what Alex think about the suggested alternative. From my perspective, any good solution is good :)

Obviously thank you for the input as always 🙏

[steviez]

Sure, ill switch to the cleanup and lets see what Alex think about the suggested alternative

Cool, I imagine the cleanup should be pretty straight forward and less stuff to possibly support with whatever we land on here

Obviously thank you for the input as always 🙏

🤝

[alexpyattaev]

As for API design we can definitely have a function that requires all arguments to be provided for prod, and only use the builder in tests.

[puhtaytow]

I'm also thinking if this new test api, could for example replace wrappers like this..

agave/ledger/benches/make_shreds_from_entries.rs

Lines 50 to 71 in 0b83420

	fn make_shreds_from_entries<R: Rng>(
	rng: &mut R,
	shredder: &Shredder,
	keypair: &Keypair,
	entries: &[Entry],
	is_last_in_slot: bool,
	chained_merkle_root: Option<Hash>,
	reed_solomon_cache: &ReedSolomonCache,
	stats: &mut ProcessShredsStats,
	) -> (Vec<Shred>, Vec<Shred>) {
	let (data, code) = shredder.entries_to_merkle_shreds_for_tests(
	keypair,
	entries,
	is_last_in_slot,
	chained_merkle_root,
	rng.gen_range(0..2_000), // next_shred_index
	rng.gen_range(0..2_000), // next_code_index
	reed_solomon_cache,
	stats,
	);
	(black_box(data), black_box(code))
	}

We could add optional rng as param and allow this way randomly generated indexes for benches, and remove this helper.

Just an idea.