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 exit code mismatch #1412

Merged
merged 2 commits into from
Feb 7, 2022
Merged

fix exit code mismatch #1412

merged 2 commits into from
Feb 7, 2022

Conversation

noot
Copy link
Contributor

@noot noot commented Feb 1, 2022

Summary of changes
Changes introduced in this pull request:

  • fix exit code mismatch causing receipt root mismatch on mainnet

Reference issue to close (if applicable)

Closes n/a

Other information and links

@lemmih
Copy link
Contributor

lemmih commented Feb 2, 2022

Are there some situations where we need to return SysErrForbidden and others where ErrForbidden is correct?

@elmattic
Copy link
Contributor

elmattic commented Feb 2, 2022

Are there some situations where we need to return SysErrForbidden and others where ErrForbidden is correct?

The rule of thumb is that SysErrXXX errors must be used in non-actor code only (so here the VM) while ErrXXX are actor code only.

@lemmih
Copy link
Contributor

lemmih commented Feb 2, 2022

I think we're using test-vectors that are no longer valid. Re-generating the test vectors from scratch changes the expected exit codes.

@lemmih
Copy link
Contributor

lemmih commented Feb 2, 2022

Here's the patch that changed the test vector: filecoin-project/specs-actors#1566

Note that they only updated the specs-actors repository and not the test-vectors repository.

@noot
Copy link
Contributor Author

noot commented Feb 2, 2022

@lemmih since we directly pull test-vectors as a submodule we need to either wait for the repo to be updated with the correct vectors to get CI to pass, or to fix it ourselves (which as you pointed out is cumbersome).

@lemmih
Copy link
Contributor

lemmih commented Feb 2, 2022

@lemmih since we directly pull test-vectors as a submodule we need to either wait for the repo to be updated with the correct vectors to get CI to pass, or to fix it ourselves (which as you pointed out is cumbersome).

As a short-term solution, I say we fork test-vectors and update the vectors ourselves.

@lemmih
Copy link
Contributor

lemmih commented Feb 3, 2022

Hi noot! I disabled the borked test vectors and pull in their replacement to the extra-vectors/ folder. Not all of the updated test vectors pass but I think we can tackle that in another PR.

@noot
Copy link
Contributor Author

noot commented Feb 3, 2022

@lemmih thanks! I can tackle the failing test in a follow up PR.

@noot noot merged commit 1fb4282 into main Feb 7, 2022
@noot noot deleted the noot/fix-exit-code branch February 7, 2022 18:28
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.

None yet

3 participants