Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fvm: fail if implicit message fails #8734

Merged
merged 1 commit into from
May 27, 2022
Merged

fvm: fail if implicit message fails #8734

merged 1 commit into from
May 27, 2022

Conversation

arajasek
Copy link
Contributor

Related Issues

Resolves #8463

Proposed Changes

This is not strictly needed, since all existing callsites of ApplyImplicitMessage check for a non-zero exit code and fail anyway. But nice to make it explicit!

Additional Info

Checklist

Before you mark the PR ready for review, please make sure that:

  • All commits have a clear commit message.
  • The PR title is in the form of of <PR type>: <area>: <change being made>
    • example: fix: mempool: Introduce a cache for valid signatures
    • PR type: fix, feat, INTERFACE BREAKING CHANGE, CONSENSUS BREAKING, build, chore, ci, docs,perf, refactor, revert, style, test
    • area: api, chain, state, vm, data transfer, market, mempool, message, block production, multisig, networking, paychan, proving, sealing, wallet, deps
  • This PR has tests for new functionality or change in behaviour
  • If new user-facing features are introduced, clear usage guidelines and / or documentation updates should be included in https://lotus.filecoin.io or Discussion Tutorials.
  • CI is green

@arajasek arajasek requested a review from a team as a code owner May 26, 2022 22:34
@codecov
Copy link

codecov bot commented May 26, 2022

Codecov Report

Merging #8734 (e6993d5) into feat/nv16 (a081f8d) will decrease coverage by 0.01%.
The diff coverage is 50.00%.

Impacted file tree graph

@@              Coverage Diff              @@
##           feat/nv16    #8734      +/-   ##
=============================================
- Coverage      40.72%   40.70%   -0.02%     
=============================================
  Files            704      704              
  Lines          77228    77230       +2     
=============================================
- Hits           31452    31438      -14     
- Misses         40387    40399      +12     
- Partials        5389     5393       +4     
Impacted Files Coverage Δ
chain/vm/fvm.go 37.10% <50.00%> (-0.34%) ⬇️
chain/events/observer.go 73.79% <0.00%> (-6.21%) ⬇️
storage/wdpost_sched.go 77.45% <0.00%> (-3.93%) ⬇️
chain/events/events_height.go 68.90% <0.00%> (-2.53%) ⬇️
chain/sub/incoming.go 58.10% <0.00%> (-2.45%) ⬇️
chain/exchange/client.go 52.96% <0.00%> (-1.70%) ⬇️
miner/miner.go 56.72% <0.00%> (-1.32%) ⬇️
chain/sync.go 70.52% <0.00%> (-0.69%) ⬇️
chain/store/store.go 64.33% <0.00%> (-0.67%) ⬇️
storage/wdpost_changehandler.go 98.11% <0.00%> (-0.48%) ⬇️
... and 13 more

Copy link
Contributor

@magik6k magik6k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 thing to address

chain/vm/fvm.go Outdated
}

if ret.ExitCode != 0 {
return applyRet, fmt.Errorf("implicit message failed with exit code: %d", ret.ExitCode)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also print ret.ActorErr?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yessir.

@arajasek arajasek force-pushed the asr/implicit-fail branch from e6993d5 to 7ea661b Compare May 27, 2022 15:43
@arajasek arajasek merged commit 40d75d1 into feat/nv16 May 27, 2022
@arajasek arajasek deleted the asr/implicit-fail branch May 27, 2022 15:43
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants