Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix next source piece index calculation #3300

Merged
merged 4 commits into from
Dec 10, 2024
Merged

Fix next source piece index calculation #3300

merged 4 commits into from
Dec 10, 2024

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Dec 9, 2024

This PR fixes a bug in the next source piece calculation, where it returned the current piece index if it was a source piece. This led to incorrect object data being retrieved during fast object retrieval across pieces.

pub const fn next_multiple_of(self, rhs: u64) -> u64
Calculates the smallest value greater than or equal to self that is a multiple of rhs.

It also cleans up some of the related data handling code.

Code contributor checklist:

@teor2345 teor2345 added the bug Something isn't working label Dec 9, 2024
@teor2345 teor2345 self-assigned this Dec 9, 2024
@teor2345 teor2345 enabled auto-merge December 9, 2024 23:39
@@ -143,7 +143,7 @@ impl PieceIndex {
/// Returns the next source piece index
#[inline]
pub const fn next_source_index(&self) -> PieceIndex {
PieceIndex(self.0.next_multiple_of(Self::source_ratio()))
PieceIndex(self.0 + Self::source_ratio() - (self.0 % Self::source_ratio()))
Copy link
Member

Choose a reason for hiding this comment

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

I do not understand why subtraction is needed here, it'll always be 0.

Also the whole source_ratio doesn't really make sense because it only works for cases when numerator is 1. If we had 2/3 it would return 1, which would be wrong, so if this code was to be fixed, it'd be fixed to account for that.

Copy link
Contributor Author

@teor2345 teor2345 Dec 10, 2024

Choose a reason for hiding this comment

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

The subtraction is needed to calculate the next source piece after a parity piece.

Removing source_ratio() is a larger change which isn't needed to fix this bug, I'll push it in another PR.

Edit: see PR #3301.

Copy link
Member

Choose a reason for hiding this comment

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

source_ratio() is 2, source indices are always even numbers, hence you're always subtracting 0. How would you describe what this code is doing with regular English?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you start with a source piece index, add 2 to go to the next source piece index.
If you start with a parity piece index, add 2 to go to the next source/parity piece pair, then subtract 1 to go to the source piece index in that pair.

shared/subspace-data-retrieval/src/object_fetcher.rs Outdated Show resolved Hide resolved
@nazar-pc nazar-pc disabled auto-merge December 10, 2024 18:23
@nazar-pc nazar-pc merged commit d53af9e into main Dec 10, 2024
8 checks passed
@nazar-pc nazar-pc deleted the fix-multi-piece branch December 10, 2024 18:23
@nazar-pc
Copy link
Member

#3301 fixed my concerns

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants