From 302d18e4068361140966ae3cd1f42c35f6a33fab Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Wed, 12 Apr 2023 08:17:13 -0400 Subject: [PATCH] light client: Address panic in response to empty signed header (backport #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 --------- Signed-off-by: Thane Thomson Co-authored-by: Thane Thomson --- .../unreleased/bug-fixes/575-fix-light-client-panic.md | 6 ++++++ light/provider/http/http.go | 9 +++++++++ types/light.go | 5 +++++ 3 files changed, 20 insertions(+) create mode 100644 .changelog/unreleased/bug-fixes/575-fix-light-client-panic.md diff --git a/.changelog/unreleased/bug-fixes/575-fix-light-client-panic.md b/.changelog/unreleased/bug-fixes/575-fix-light-client-panic.md new file mode 100644 index 000000000..0ec55b923 --- /dev/null +++ b/.changelog/unreleased/bug-fixes/575-fix-light-client-panic.md @@ -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)) \ No newline at end of file diff --git a/light/provider/http/http.go b/light/provider/http/http.go index 9fb01dd96..b73f7bd45 100644 --- a/light/provider/http/http.go +++ b/light/provider/http/http.go @@ -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()): diff --git a/types/light.go b/types/light.go index 31fdc620f..e3ef1f63d 100644 --- a/types/light.go +++ b/types/light.go @@ -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. //