-
Notifications
You must be signed in to change notification settings - Fork 247
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
Initial version of incremental record commitment (take 2) #1201
Conversation
crates/subspace-archiving/src/archiver/incremental_record_commitments.rs
Outdated
Show resolved
Hide resolved
let bytes_before_next_record = ((self.processed_bytes / self.record_size) + 1) | ||
* self.record_size | ||
- self.processed_bytes; | ||
let padding_bytes = bytes_before_next_record % self.record_size; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bytes_before_next_record should be < record_size, why is the % op needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In your version yes, in the code that was there before no.
crates/subspace-archiving/src/archiver/incremental_record_commitments.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored in a different way, thanks for suggestions
let bytes_before_next_record = ((self.processed_bytes / self.record_size) + 1) | ||
* self.record_size | ||
- self.processed_bytes; | ||
let padding_bytes = bytes_before_next_record % self.record_size; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In your version yes, in the code that was there before no.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks
Similar, but not the same as #1161, please read #1161 for background.
In this iteration incremental commitment creation is simpler due to beginning of the segment never changing (see #1200). I also improved comments in the code a bit.
Resolves #1160
Code contributor checklist: