Repository navigation
fix(clientapi): decide "block not found" from the status code, not the text - #299
Merged
Merged
Conversation
…e text The check asked whether the rendered error contained the substring "404". The beacon node is addressed by slot, so on a transport failure go-eth2-client joins in the error from net/http, which carries the full request URL: failed to call GET endpoint Get "http://bn:5052/eth/v2/beacon/blocks/404": dial tcp ...: refused 79,447 of the first 15,000,000 slots contain "404" in decimal, about one in two hundred. For those slots an unreachable beacon node satisfied the check. The reverse also held: any status code passed as long as something laundered a "404" into the message, so a 500 naming slot 4040 read as not-found. Five call sites were affected, in two different shapes. RequestBeaconBlock and the three blob.go sites went through response404(err.Error()). RequestBeaconBlock took the missing-block branch and returned CreateMissingBlock(slot) with a nil error: a synthetic empty block was written and the epoch completed clean, indistinguishable downstream from a genuinely empty slot. The blob sites did the same more quietly, returning empty sidecars for a slot never fetched. RequestBlockRoot open-coded the same check inline, which is why it did not turn up alongside the others, and it is the damaging one. It returns a zero root on a match and calls log.Panicf otherwise. A zero root never equals the cached root, so reorg.go deletes that slot's block metrics and redownloads it. A transient beacon node failure during reorg handling, on a 404-shaped slot, therefore deleted correct data, and the redownload hit the first bug and wrote a fabricated empty block in its place. The two compounded. isNotFound asks the error for its status code instead. The slot, the port, the peer address and any hex root echoed back in a body can no longer be mistaken for a status. This changes behaviour, deliberately, in two places. In RequestBeaconBlock a transport failure on a 404-shaped slot now falls through to the retry loop and, if the node stays down, to the existing "unable to retrieve Beacon Block at slot %d" error, so an epoch that used to complete against a flapping node will now stop. In RequestBlockRoot the same failure now reaches the log.Panicf that every other slot already reached. Both make a silent wrong answer into a visible one. The panic in RequestBlockRoot is its own problem and is left alone here rather than folded into this fix. go-eth2-client declares Error() on the value receiver, so api.Error and *api.Error both satisfy the error interface. It returns the pointer form today; both are matched, because a switch to the value form would otherwise stop every genuine 404 being recognised and send every missing slot into a retry loop. The tests cover the statuses accepted and rejected, unwrapping through fmt.Errorf and errors.Join, the value form, and the six shapes that fooled the old check. One test makes a request that really fails against a closed loopback port and asserts the hand-built transport error used by the others still matches it, so those cannot drift into asserting against a fiction. Seven mutations were tried, including restoring the old substring implementation, and all seven were caught; an equivalent mutation was confirmed to survive. isNotFound is at 100%. Fixes #298
Collaborator
|
lgtm. the visible stop is fine by me. the old behavior was deleting correct rows and writing a fabricated empty block in their place, a stop is strictly better than that definitely lmao merges clean together with #297 in either order |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #298.
response404asked whether the rendered error contained the substring404. Because the beacon node is addressed by slot, a transport failure joins in the error fromnet/http, which carries the full request URL:About one slot in two hundred contains
404in decimal, so for those an unreachable node satisfied the check. The reverse held too: any status code passed as long as something laundered a404into the message.Five call sites, in two shapes
RequestBeaconBlockand the threeblob.gosites went throughresponse404(err.Error()).RequestBeaconBlocktook the missing-block branch and returnedCreateMissingBlock(slot)with a nil error, so a synthetic empty block was written and the epoch completed clean, indistinguishable downstream from a genuinely empty slot.RequestBlockRootopen-coded the same check inline, which is why it does not appear in a search for the helper, and it is the damaging one. It returns a zero root on a match. Its only caller is reorg handling, which treats a root mismatch as a signal toDeleteBlockMetricsand redownload. A zero root never matches, so a transient beacon node failure during reorg handling, on a 404-shaped slot, deletes correct data — and the redownload then hits the first bug and writes a fabricated empty block in its place. The two compound.This changes behaviour, deliberately
In
RequestBeaconBlock, a transport failure on a 404-shaped slot now falls through to the retry loop and, if the node stays down, to the existingunable to retrieve Beacon Block at slot %derror. An epoch that used to complete against a flapping node will now stop.In
RequestBlockRoot, the same failure now reaches thelog.Panicfthat every other slot already reached.Both turn a silent wrong answer into a visible one, which is the point. The
log.Panicfitself is a separate problem and is left alone here rather than folded into this fix — worth its own issue if you agree.On matching both the pointer and the value form
go-eth2-clientdeclaresError()on the value receiver, so bothapi.Errorand*api.Errorsatisfy the error interface. It returns the pointer today (http.go:181and:356). Both are matched, because a switch to the value form would otherwise stop every genuine 404 being recognised and send every missing slot into a retry loop — a worse failure than the one being fixed.Testing
Seven mutations were tried and all seven caught, including restoring the old substring implementation, so the regression is pinned rather than merely described. An equivalent mutation was confirmed to survive, which is what makes "all caught" mean anything.
One test makes a request that really fails against a closed loopback port and asserts that the hand-built transport error the other tests use still matches it, so they cannot drift into asserting against a fiction.
isNotFoundis at 100%. Full repogo build,go vetandgo testare green, and the package is clean under-shuffle=on -count=2 -race.goteth-v4has the identical bug on all five sites and is untouched here; the port is its own PR once this shape is agreed.