-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Log more call context during errors #6918
Conversation
chain/vm/runtime.go
Outdated
@@ -391,7 +391,7 @@ func (rt *Runtime) Send(to address.Address, method abi.MethodNum, m cbor.Marshal | |||
if err.IsFatal() { | |||
panic(err) | |||
} | |||
log.Warnf("vmctx send failed: to: %s, method: %d: ret: %d, err: %s", to, method, ret, err) | |||
log.Warnf("vmctx send failed: from: %s to: %s, method: %d: ret: %d, err: %s", rt.Receiver(), to, method, ret, 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.
What does this ret
look like when printed? Is that coherent?
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.
Since this is currently on latest code we should check the mainnet logs and find out :)
I think %x will probably be better so we can get hex of cbor bytes and we could make that change here
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.
Looking more closely internalSend
should only return a non empty ret value in the case send returns a non empty ret value. This will only return non empty ret if shimCall
returns non empty ret which can only happen if shimCall does not return an error (either from marshalling ret or an abort from within the actor method being shimmed). This means printing ret should be strictly redundant since it is always empty.
There are only a few types of errors I see on mainnet but they all print []
for ret so that supports this line of reasoning.
d46f99b
to
e09a25c
Compare
This carries forward some nice work from @zgfzgf in #4954
Logging call context from Abortf as in the original PR turns out to be redundant with the logging from shimCall because all Abortf calls panic up to shimCall immediately. Logging shimCall failures is not redundant with internal and vm send actor error logging in the edge case where an internal send errors but the error is dropped which can occur in the case of programmer error in calls from reward and cron.