Skip to content

Commit

Permalink
light client: Address panic in response to empty signed header (backp…
Browse files Browse the repository at this point in the history
…ort #577) (#700)

* light client: Address panic in response to empty signed header (#577)

Closes #575.

This at least addresses the panic in the light client, instead turning it into a temporary error (i.e. that the node does not yet have the signed header for the requisite height, which is true here).

I've tested this patch on the `v0.34.x` branch and it prevents the panic, but instead causes the E2E tests to hang indefinitely. As such, I've logged #576.

The code change is identical across `main`, `v0.37.x` and `v0.34.x`, so backports should be clean. I'll add changelog entries on the backport branches prior to merging them.

---

#### PR checklist

- [ ] Tests written/updated
- [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog)
- [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments

(cherry picked from commit ea2020ffc5c752b5631ee1b769dc5dd78624f1bc)

* Add changelog entry

Signed-off-by: Thane Thomson <[email protected]>

---------

Signed-off-by: Thane Thomson <[email protected]>
Co-authored-by: Thane Thomson <[email protected]>
  • Loading branch information
mergify[bot] and thanethomson authored Apr 12, 2023
1 parent 3a91d15 commit 302d18e
Show file tree
Hide file tree
Showing 3 changed files with 20 additions and 0 deletions.
6 changes: 6 additions & 0 deletions .changelog/unreleased/bug-fixes/575-fix-light-client-panic.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
- `[light]` Fixed an edge case where a light client would panic when attempting
to query a node that (1) has started from a non-zero height and (2) does
not yet have any data. The light client will now, correctly, not panic
_and_ keep the node in its list of providers in the same way it would if
it queried a node starting from height zero that does not yet have data
([\#575](https://github.com/cometbft/cometbft/issues/575))
9 changes: 9 additions & 0 deletions light/provider/http/http.go
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,15 @@ func (p *http) signedHeader(ctx context.Context, height *int64) (*types.SignedHe
commit, err := p.client.Commit(ctx, height)
switch {
case err == nil:
// See https://github.com/cometbft/cometbft/issues/575
// If the node is starting at a non-zero height, but does not yet
// have any blocks, it can return an empty signed header without
// returning an error.
if commit.SignedHeader.IsEmpty() {
// Technically this means that the provider still needs to
// catch up.
return nil, provider.ErrHeightTooHigh
}
return &commit.SignedHeader, nil

case regexpTooHigh.MatchString(err.Error()):
Expand Down
5 changes: 5 additions & 0 deletions types/light.go
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,11 @@ type SignedHeader struct {
Commit *Commit `json:"commit"`
}

// IsEmpty returns true if both the header and commit are nil.
func (sh SignedHeader) IsEmpty() bool {
return sh.Header == nil && sh.Commit == nil
}

// ValidateBasic does basic consistency checks and makes sure the header
// and commit are consistent.
//
Expand Down

0 comments on commit 302d18e

Please sign in to comment.