This repository has been archived by the owner on Oct 2, 2023. It is now read-only.
sort CommRs by sector id #27
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
laser
force-pushed
the
feat/23-sort-commitments-by-sector-id
branch
from
0151d38
to
ddc1458
Compare
dignifiedquire
suggested changes
Sep 20, 2019
There was a problem hiding this comment.
It makes little sense to change this, unless you also change the ordering in rust-fil-proofs: https://github.com/filecoin-project/rust-fil-proofs/blob/master/filecoin-proofs/src/api/post.rs#L35-L45
|
@dignifiedquire part of my request was that I already have the sectors sorted by sector ID, no need to spend any more time sorting them by something else (especially since this will be happening inside the VM) |
|
Sure, but that’s a different PR. This change can merge separately and will have no effect until the upstream change is made.
Do you wan the other change merged first?
…On Fri, Sep 20, 2019, at 3:13 PM, Friedel Ziegelmayer wrote:
***@***.**** requested changes on this pull request.
It makes little sense to change this, unless you also change the ordering in rust-fil-proofs: https://github.com/filecoin-project/rust-fil-proofs/blob/master/filecoin-proofs/src/api/post.rs#L35-L45
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub <#27?email_source=notifications&email_token=AAGX6G7GJT6VE7WX4VSY37DQKVDI3A5CNFSM4IY3ETA2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCFPK4KY#pullrequestreview-291417643>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGX6G5S6KK623EDUMQKI7LQKVDI3ANCNFSM4IY3ETAQ>.
|
laser
changed the title
laser
force-pushed
the
feat/23-sort-commitments-by-sector-id
branch
from
ddc1458
to
b7c5745
Compare
laser
force-pushed
the
feat/23-sort-commitments-by-sector-id
branch
from
b7c5745
to
6153cd8
Compare
|
This PR has been stale for several weeks; closing. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #23
Why does this PR exist?
PoSt generation and verification require their input CommRs be provided in the same order. @whyrusleeping pointed out that we're sorting on replica-bytes and we could be sorting by sector id instead.
What's in this PR?
This PR updates the sorter to use sector id instead of bytes, and adds a JSON marshal/unmarshal method pair as per @anorth.