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

Don't propagate Golang error stack information over the SwingSet bridge #7929

Closed
michaelfig opened this issue Jun 15, 2023 · 0 comments · Fixed by #7930
Closed

Don't propagate Golang error stack information over the SwingSet bridge #7929

michaelfig opened this issue Jun 15, 2023 · 0 comments · Fixed by #7930
Assignees
Labels
enhancement New feature or request vaults_triage DO NOT USE

Comments

@michaelfig
Copy link
Member

What is the Problem Being Solved?

During the PismoD network soft-patch, some validators diverged and fell out of consensus because they did not upgrade their node software at the same time as the 2/3+ validators did.

Comparing SwingSet transcripts between in- and out-of-consensus nodes, @mhofman found the divergence: a failed request from a SwingSet vat to a Golang module resulted in error messages that were sensitive to the soft-patch:

  • unpatched: insufficient funds [agoric-labs/cosmos-sdk@v0.45.11-alpha.agoric.1/x/bank/keeper/send.go:186] (note the version: alpha.agoric.1)
  • patched: insufficient funds [agoric-labs/cosmos-sdk@v0.45.11-alpha.agoric.1.1/x/bank/keeper/send.go:186] (note the extra "dot one": alpha.agoric.1 .1)

Description of the Design

A suspect idiom in our Golang code is using fmt.Errorf("... %w", ..., err) to create a fresh error based on err with some descriptive text. It turns out that %w renders cosmos-sdk’s attached error stack frame (which has the version information in it). Changing it to %s instead only propagates the error message.

However, we can still use %w for parts of the agd implementation that are not within Tendermint consensus (such as client code), where the stack information is useful for debugging.

Security Considerations

This issue can cause divergent behaviour between validators running a consensus-breaking soft-patch when it was not expected to be. This is a risk to availability, as such a soft-patch could potentially halt the chain. The mitigating factor is that clear communication with the validator community can help coordinate the necessary upgrades.

Scaling Considerations

n/a

Test Plan

Write unit tests demonstrating the absence and presence of stack strings with the %w and %s format specifiers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request vaults_triage DO NOT USE
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants