-
Notifications
You must be signed in to change notification settings - Fork 13
Feed F3 from EC chain and bootstrap #341
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
Conversation
Signed-off-by: Jakub Sztandera <oss@kubuxu.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #341 +/- ##
==========================================
- Coverage 82.92% 82.89% -0.04%
==========================================
Files 15 15
Lines 1687 1695 +8
==========================================
+ Hits 1399 1405 +6
- Misses 168 169 +1
- Partials 120 121 +1
|
Signed-off-by: Jakub Sztandera <oss@kubuxu.com>
|
Fuzz test failed on commit ee91787. To troubleshoot locally, download the seed corpus using GitHub CLI by running: gh run download 9506669726 -n testdataAleternatively, download directly from here. |
Signed-off-by: Jakub Sztandera <oss@kubuxu.com> Signed-off-by: Jakub Sztandera <oss@kubuxu.com>
|
Fuzz test failed on commit cd30c14. To troubleshoot locally, download the seed corpus using GitHub CLI by running: gh run download 9519124339 -n testdataAleternatively, download directly from here. |
Stebalien
left a comment
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.
Code LGTM except that one thing about the certificate store.
| powerEntries, err = h.client.certstore.GetPowerTable(h.runningCtx, instance) | ||
| if err != nil { | ||
| // this fires every round, is this correct? | ||
| h.log.Infof("failed getting power table from certstore: %v, falling back to EC", err) |
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 never see an error here as long as we've already inserted the certificate for the previous instance. I wonder if we have an off-by-one somewhere? Or are we not inserting the cert?
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.
It is always off by one in the error message
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.
What's the specific message and/or can I reproduce it somehow?
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.
OH! It's from here:
Lines 314 to 317 in cd30c14
| next, _, err := h.GetCommitteeForInstance(instance + 1) | |
| if err != nil { | |
| return xerrors.Errorf("getting commitee for next instance %d: %w", instance+1, err) | |
| } |
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 one: https://github.com/filecoin-project/go-f3/blob/main/certstore/certstore.go#L282
You can reproduce by running:
./f3 manifest gen and then ./f3 run --id=0 and ./f3 run --id=1 in two different terminals.
Co-authored-by: Steven Allen <steven@stebalien.com>
No description provided.