-
Notifications
You must be signed in to change notification settings - Fork 85
Implement FIPs #993 SealerID #1683
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
base: master
Are you sure you want to change the base?
Conversation
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.
We should figure out what's up with replaying sectors. I think its a big issue but maybe I'm missing something. Should be a one line fix if I'm right. Overall looks good. I'll look over sealer closer on next review.
For testing the two actors
-
For sealer id it should be straightforward to follow the pattern we have with other small actors. Look at multisig actor for example. Generally we make an actorHarness structure which enforces expectations using the MockRuntime and put that in a util.rs. Then we call this harness in test specific ways.
-
Miner actor tests are some of the densest and most annoying code in the network but there are a lot of examples to follow from.
-
The first thing I would do is add new edge cases to prove_commit_niporep_test.rs. This is an integration test running a full dummy fvm which is a good target for these cases because it will exercise the true inter actor communication
-
The miner actor also has extensive actorHarness unit tests like the one I'm recommending for sealer_id. It looks like we never added any for ni_porep. Ideally this change would include ni porep calls that involved sealer id edge cases there too.
state.allocate_sector_numbers( | ||
rt.store(), | ||
¶ms.sector_numbers, | ||
CollisionPolicy::DenyCollisions, |
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.
Based on this it looks like we might have a different idea of the security concern of collisions. I think we shouldn't even configure this and always deny collisions as a matter of network security. I want to hear your thoughts though -- it looks like none of this was really specified in the draft FIP.
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.
This is matching the same exact logic in the miner actor, the allocate_sector_numbers is pretty much copy-paste of the miner one
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.
Ok that makes sense then
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.
I would either remove this over engineered collision policy thing in this copied code or extract the allocation function somewhere so miner and sealer can share it.
Also all of this SGTM thanks for keeping things simple |
Looked over sealer code and this looks about ready to go once tests are in. |
See filecoin-project/FIPs#993
sealer_id_actor
andsealer_id_verifier_signature
toProveCommitSectorsNIParams
Couple of liberties/departures from design in #993, to be discussed and reincorporated into the FIP:
AuthenticateMessage
mechanism, with a payload containing enough metadata to be useful for fevm-based sector market implementations.Couple asks: