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

fix(f3): test API during inactive-F3 modes and make consistent and safe #12781

Merged
merged 5 commits into from
Dec 13, 2024

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Dec 13, 2024

Closes: #12772

  • Added an itest to test all of the F3 API method returns when both explicitly disabled F3 and F3 not properly started
  • Fixed the GetManifest panic
  • Made error messages more consistent (go-f3 has error messages that start with "F3 but we have some "f3)
    • GetManifest now copies the "no known network manifest" error message that GetF3PowerTable returns (from inside go-f3) during the not-running state
    • client.F3GetOrRenewParticipationTicket now returns nil as the zero value of api.F3ParticipationTicket which is a slice, like the inner lf3/leaser.getOrRenewParticipationTicket returns, so we don't get nil in one case and [] in another

One minor special case is F3GetECPowerTable returns a sensible value when F3 isn't ready, so it's only when it's properly disabled that it gives an error. This seems reasonable even if it feels mildly inconsistent.

@rvagg rvagg requested a review from masih December 13, 2024 07:23
@rvagg rvagg added the skip/changelog This change does not require CHANGELOG.md update label Dec 13, 2024
api/api_errors.go Outdated Show resolved Hide resolved
chain/lf3/f3.go Outdated Show resolved Hide resolved
@rvagg
Copy link
Member Author

rvagg commented Dec 13, 2024

@masih can you have a look at

lotus/itests/f3_test.go

Lines 355 to 357 in 2666c3e

m, err := n.F3GetManifest(e.testCtx)
require.NoError(e.t, err)
return newManifest.Equal(m)
please - this is now failing. Previous to this PR, F3GetManifest is only going to error if F3 is fully disabled, so I'm not sure what this error check was for. Am I safe to remove the error check and just return false when the manifest is nil? That would be equivalent to the existing behaviour but I'm not sure whether I'm exposing something of interest in the process of doing this.

@masih
Copy link
Member

masih commented Dec 13, 2024

is only going to error if F3 is fully disabled, so I'm not sure what this error check was for. Am I safe to remove the error check and just return false when the manifest is nil?

Yes, returning false on either error or mismatching Manifest should be fine.

Looks like the cause of tests failing is mismatching manifest though.

@rvagg rvagg force-pushed the rvagg/f3-api-consistency branch from 2666c3e to 3631bf6 Compare December 13, 2024 11:35
@rvagg
Copy link
Member Author

rvagg commented Dec 13, 2024

Ah, got it, I accidentally introduced a new error condition in GetManifest of "can't get latest certificate certificate-zero so can't return you a manifest" .. which isn't appropriate if we have a manifest but haven't quite started. Nice to have test coverage for it!

@rvagg rvagg merged commit 7f2068e into master Dec 13, 2024
83 checks passed
@rvagg rvagg deleted the rvagg/f3-api-consistency branch December 13, 2024 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip/changelog This change does not require CHANGELOG.md update
Projects
Status: 🎉 Done
Development

Successfully merging this pull request may close these issues.

F3#GetManifest can panic
2 participants