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

* init decoupling-post-worker code #1497

Conversation

lovel8
Copy link
Contributor

@lovel8 lovel8 commented Aug 20, 2021

Merge based on:
filecoin-project/lotus#6943

@cryptonemo
Copy link
Collaborator

Thanks for the PR! Having glanced at it very briefly, can someone describe the main objective and difference between the split PoSt API that's already available (started in #1278)? Just trying to get a better idea of the use case(s) and determine if there's overlap, as it appears there may be.

@lovel8
Copy link
Contributor Author

lovel8 commented Aug 23, 2021

Thanks for the PR! Having glanced at it very briefly, can someone describe the main objective and difference between the split PoSt API that's already available (started in #1278)? Just trying to get a better idea of the use case(s) and determine if there's overlap, as it appears there may be.

@jennijuju @cryptonemo
main objective:
Decoupling WinningPoSt and WindowPoSt Process from lotus-miner

difference:

  1. Modify the serialized return of the winningPoSt and windowPoSt proof results to support remote access over the network.
  2. Split the windowPoSt proof to support remote acquisition from multiple storage miners through the network.

@cryptonemo
Copy link
Collaborator

Thanks for the PR! Having glanced at it very briefly, can someone describe the main objective and difference between the split PoSt API that's already available (started in #1278)? Just trying to get a better idea of the use case(s) and determine if there's overlap, as it appears there may be.

@jennijuju @cryptonemo
main objective:
Decoupling WinningPoSt and WindowPoSt Process from lotus-miner

difference:

1. Modify the serialized return of the winningPoSt and windowPoSt proof results to support remote access over the network.

The existing API was designed to be distributed in the sense that vanilla proofs can be generated where the data is, then shipped back over the network to node(s) that generates the snark proof(s). I don't yet see how this differs from what's there. I see that you form the proofs into a string (which appears to be the main difference?), but the existing API allows for standard serialization of the vanilla proofs. A string type is not recommended for this use-case, particularly in an official API as there are no format guarantees. But the existing API doesn't constrain that, as it's just an API for getting the proofs and doesn't mind how they are shipped.

2. Split the windowPoSt proof to support remote acquisition from multiple storage miners through the network.

I'm still confused, as that was also the intent of the original distributed PoSt API (which is provided for both WinningPoSt and WindowPoSt). Again, I've only looked over the code briefly and was surprised by the amount of overlap, so am looking for specific differences that are required that aren't provided by the existing API.

Some more (perhaps related) info about this is here, but I was unable to locate more specific design specifications at the moment: #854

@lovel8
Copy link
Contributor Author

lovel8 commented Aug 25, 2021

Thanks for the PR! Having glanced at it very briefly, can someone describe the main objective and difference between the split PoSt API that's already available (started in #1278)? Just trying to get a better idea of the use case(s) and determine if there's overlap, as it appears there may be.

@jennijuju @cryptonemo
main objective:
Decoupling WinningPoSt and WindowPoSt Process from lotus-miner
difference:

1. Modify the serialized return of the winningPoSt and windowPoSt proof results to support remote access over the network.

The existing API was designed to be distributed in the sense that vanilla proofs can be generated where the data is, then shipped back over the network to node(s) that generates the snark proof(s). I don't yet see how this differs from what's there. I see that you form the proofs into a string (which appears to be the main difference?), but the existing API allows for standard serialization of the vanilla proofs. A string type is not recommended for this use-case, particularly in an official API as there are no format guarantees. But the existing API doesn't constrain that, as it's just an API for getting the proofs and doesn't mind how they are shipped.

2. Split the windowPoSt proof to support remote acquisition from multiple storage miners through the network.

I'm still confused, as that was also the intent of the original distributed PoSt API (which is provided for both WinningPoSt and WindowPoSt). Again, I've only looked over the code briefly and was surprised by the amount of overlap, so am looking for specific differences that are required that aren't provided by the existing API.

Some more (perhaps related) info about this is here, but I was unable to locate more specific design specifications at the moment: #854

I will take some time to verify whether the #1278 interface can meet the splitting requirements, and if it is feasible, we will directly use the existing interface

@lovel8 lovel8 force-pushed the feat/decoupling-post-worker branch from 7ff6f49 to f75af00 Compare August 27, 2021 06:34
@lovel8
Copy link
Contributor Author

lovel8 commented Aug 27, 2021

@cryptonemo @jennijuju The existing code has been reused and the verification passed!

@lovel8
Copy link
Contributor Author

lovel8 commented Sep 1, 2021

@cryptonemo When can this pull be merged?

@cryptonemo
Copy link
Collaborator

@cryptonemo When can this pull be merged?

What specifically are you referring to? All I see is a method that returns a string, which isn't needed in the API.

@cryptonemo
Copy link
Collaborator

Removing the verifies is also not an option. If you can re-factor it to something that makes sense for the use case and show via test that it's needed (since the existing tests do not require it), we can review that.

@lovel8 lovel8 force-pushed the feat/decoupling-post-worker branch 3 times, most recently from d5e9596 to 10ff1a3 Compare September 3, 2021 03:28
@lovel8
Copy link
Contributor Author

lovel8 commented Sep 3, 2021

@cryptonemo

  1. String serialization has been removed.
  2. After splitting partitions, partition_vanilla_proofs support Single partition proofs, But the verification interface is for all partitions. because the position of the challenge node is not offset,so single partition verification will fail.
    To support the verification of a single partition, we need to add a set of interfaces that support single partition certification and add offset parameters. Do you have any better suggestions for modification?

@lovel8 lovel8 force-pushed the feat/decoupling-post-worker branch 5 times, most recently from 0cab00a to 01ce54e Compare September 6, 2021 02:15
@lovel8
Copy link
Contributor Author

lovel8 commented Sep 6, 2021

@cryptonemo Please review that. or any suggestions

@lovel8 lovel8 force-pushed the feat/decoupling-post-worker branch 3 times, most recently from 31205ad to e8cfea2 Compare September 15, 2021 03:13
@lovel8
Copy link
Contributor Author

lovel8 commented Sep 15, 2021

@cryptonemo Api test code has been added

@lovel8
Copy link
Contributor Author

lovel8 commented Sep 15, 2021

ci/circleci: test_darwin failed: because the automatic running test environment did not prove parameters

@lovel8
Copy link
Contributor Author

lovel8 commented Sep 15, 2021

@cryptonemo Regarding the modification of the proof api, we think it is necessary after communication, otherwise the function will not be completed. Can you describe the PL suggestion here. Is it necessary to modify the original api?

@cryptonemo
Copy link
Collaborator

@cryptonemo Regarding the modification of the proof api, we think it is necessary after communication, otherwise the function will not be completed. Can you describe the PL suggestion here. Is it necessary to modify the original api?

I'm not ignoring this, and this generally looks better, but I'll have to review in more detail (likely next week).

@cryptonemo cryptonemo mentioned this pull request Sep 21, 2021
10 tasks
Copy link
Collaborator

@porcuquine porcuquine left a comment

Choose a reason for hiding this comment

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

GitHub review intercepted this comment and made it pending. It's minor, but worth noting.

@@ -497,3 +498,9 @@ where
Ok(res)
}
}

#[derive(Serialize, Deserialize)]
pub struct PubVanRsp {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Without expressing an opinion on the rest, may I ask that you avoid names like this? Just spell out what you mean (and make sure that makes sense). This will help keep the code base as legible as possible.

@cryptonemo
Copy link
Collaborator

Closing since this was replaced with #1526

@cryptonemo cryptonemo closed this Dec 6, 2021
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.

3 participants