FIP-0084: Remove ProveCommit and dependencies #1540
Conversation
| } | ||
|
|
||
| #[test] | ||
| fn invalid_proof_rejected() { |
There was a problem hiding this comment.
@anorth I'm more sure about the other tests but could you confirm that we are not throwing away any valuable test coverage here for prove_commit3?
There was a problem hiding this comment.
With existing tests in prove_commit2_failures_test.rs plus #1547 I think we're good.
There was a problem hiding this comment.
This is looking pretty good, a nice cleanup.
It seems there's some technical debt left behind in the tests. I think you're right to avoid doing it in this PR, but I would appreciate you documenting what needs to be done in an issue and ideally following up to do some of it.
| @@ -207,7 +208,7 @@ impl Default for ExpectInvocation { | |||
| exit_code: ExitCode::OK, | |||
| return_value: None, | |||
| subinvocs: None, | |||
| events: vec![], | |||
| events: None, | |||
There was a problem hiding this comment.
How about making the default Some(vec[]) so that opting out is explicit?
There was a problem hiding this comment.
I don't see why events should break this symmetry compared to subinvocs for example. Developing new events -> testing new development -> writing explicit checks for event expectations. The default should not expose this complexity. If I'm wrong about this then I think by symmetry we've gotten it wrong for all the other optional fields.
There was a problem hiding this comment.
I think we have it "wrong" for all the other optional fields, but the optionals are pragmatic. I think we can do better. I'll file an issue about it. Your call what to do here for now, but if it's only a small addition, I'd add the events.
| @@ -207,7 +208,7 @@ impl Default for ExpectInvocation { | |||
| exit_code: ExitCode::OK, | |||
| return_value: None, | |||
| subinvocs: None, | |||
| events: vec![], | |||
| events: None, | |||
There was a problem hiding this comment.
I think we have it "wrong" for all the other optional fields, but the optionals are pragmatic. I think we can do better. I'll file an issue about it. Your call what to do here for now, but if it's only a small addition, I'd add the events.
| } | ||
|
|
||
| #[test] | ||
| fn invalid_proof_rejected() { |
There was a problem hiding this comment.
With existing tests in prove_commit2_failures_test.rs plus #1547 I think we're good.
| // To ensure passed in configuration is internally consistent check that cfg deals are a subset | ||
| // of precommitted deal ids |
There was a problem hiding this comment.
Can you explain why it's a subset, and why below we can safely get(i) of configured_deals? So it's a subset, but strictly has a copy of the deals referenced in the prefix of deal_ids. Why?
There was a problem hiding this comment.
or ... is it simply that we want to take configured_deals and assign them a deal_id so we pluck them out in order, and then fill the rest up with defaults?
There was a problem hiding this comment.
I guess I'm struggling to see why there's additional deal_ids involved here, what are the test cases that involve a different number of deal_ids vs what's in the config?
There was a problem hiding this comment.
or ... is it simply that we want to take configured_deals and assign them a deal_id so we pluck them out in order, and then fill the rest up with defaults?
Yes this is it.
I guess I'm struggling to see why there's additional deal_ids involved here
Yeah you and me both 😄
I don't know if there are any tests that configure more deal ids than deal specs in ProveCommitCfgs but I doubt they're there. However the two configs are completely logically separate so it is technically possible and I wanted to handle just in case.
I could alternatively look into failing the test if the configs don't have the same number of deals but if I hit any failure cases I'd want to just cleanup the mess with #1545.
| @@ -79,6 +78,7 @@ pub struct State { | |||
| /// Claimed power for each miner. | |||
| pub claims: Cid, // Map, HAMT[address]Claim | |||
|
|
|||
| // Deprecated as of FIP 0084 | |||
| pub proof_validation_batch: Option<Cid>, | |||
There was a problem hiding this comment.
do we expect this to be empty after the upgrade? is there a period where some may get stuck in here across the upgrade epoch boundary, or are they run within the same epoch?
There was a problem hiding this comment.
Good question. They are always pulled out in the same epoch so there shouldn't be any trouble. I should double check cron logic in case null blocks mess with this.
| ) -> ProofsByAddress { | ||
| if state.proof_validation_batch.is_none() { | ||
| return ProofsByAddress::default(); | ||
| fn check_proofs_invariants(state: &State, acc: &MessageAccumulator) { |
There was a problem hiding this comment.
what makes use of check_state_invariants() across the actors? Is this called by some external tool? I know about the regular go-state-types ones that are run via lotus, but are these run anywhere with any regularity? Or can they be run manually across real state?
There was a problem hiding this comment.
These are only ever run in actors tests unfortunately. There used to be one set which was used both in actors tests and live tests of mainnet state. Ever since we moved to rust actors we've been maintaining twice the code to get the same value. We've never taken the time to figure out a reasonable solution.
I think it might be slightly less crazy to figure out how to run rust functions on a golang blockstore and then wire this into the lotus-shed command that checks invariants. Since the FVM must be doing this anyway it can't require completely new code.
There was a problem hiding this comment.
The integration tests will check invariants across actors.
Yes it's huge technical debt that there are two versions of this. I consider the ones that are developed alongside the actors and run in the tests to be the canonical. The Lotus team, however, consider the version in go-state-types, which runs against mainnet state, to be canonical. I agree with @ZenGround0's suggested path. This isn't captured in an issue anywhere AFAIK, perhaps since it's not quite clear in which repo it belongs.
There was a problem hiding this comment.
seems fine to me, from my level of knowledge; a few questions and comments inline but nothing blocking
#1548 (comment) is a comment relating to this PR; I'm mildly uncomfortable introducing an Option for events, even if that does make it on par with some other validation properties there. It would be better if it was much lower effort to add events by executing, eyeballing and backfilling.
It does seem like a large ask to add events here though, a lot of work's gone into this, well done @ZenGround0
I'm the odd one out on this (@anorth said the same) so it's likely I'm thinking about it wrong. However I feel worse about executing, eyeballing and backfilling if I'm understanding that approach right. To me it seems that approach encourages using the tests to learn about the system since you would see how the tests fail and then make them match the implementation. But to me this feels backwards. Instead I would rather engineers make changes to the system, predict the new behavior with a test, check that their assumptions are correct. This forces people to check their model of what should be happening against the system. I get that we reduce coverage of the exact exec trace this way, but we make the development of precise and good tests easier. I guess I would rather have tests that don't break when the system changes in a way that is irrelevant for the intent of the test. Especially when the trade off is that good tests with precise intents are easier to write. I would rather that lazy people write weak quiet tests (just set everything to None) than slightly stronger noisy tests (blindly copy whatever the system is doing without thinking about whether its relevant to the test). This is all a remnant of me being scarred by experiences (aka go-filecoin 🙃 ) where tests were so coupled to implementations that it became very hard to make simple changes and roughly impossible to make big changes. In reality that's probably never going to happen here so I wouldn't block a PR that changed events back to required. I think having #1548 alongside integration tests is probably a good tradeoff. |
removed in #1540 but still needed to activate preseals
left-over from #1540
removed in #1540 but still needed to activate preseals
removed in #1540 but still needed to activate preseals
removed in #1540 but still needed to activate preseals
* fix: restore ConfirmSectorProofsValid removed in #1540 but still needed to activate preseals * changes as per review * make rustfmt --------- Co-authored-by: aarshkshah1992 <[email protected]>
* Remove ProveCommit and dependencies * Remove gas param * Fixup power tests * Remove deprecated harness methods and redirect to prove commit3 * Use new harness names * Remove tests exercising deprecated behavior of prove commit * fmt * Get tests running locally * Cleanup * Fix many miner tests * Fix the fix * More linter and fmt fixes * fmt * clippy * Fix power tests * Fix itests and miner power dependent tests * fmt * Remove and cleanup prove_commit tests * All tests passing * Clippy * Review Response * Finish review response * Remove low value test method * Update prefactor tests to correct names --------- Co-authored-by: zenground0 <[email protected]>
* fix: restore ConfirmSectorProofsValid removed in #1540 but still needed to activate preseals * changes as per review * make rustfmt --------- Co-authored-by: aarshkshah1992 <[email protected]>
Closes #1277