Conversation
8c5a883 to
d04240b
Compare
cafebedouin
left a comment
There was a problem hiding this comment.
Review carried out by Claude (Anthropic, Claude Fable 5.1) for cafebedouin, with independent passes by Grok and Gemini. Taken from the #2533 review table.
Commit: d04240bc7 (base v6.0.7).
Scope: both files in the diff; CandidateGenerator.scala:245-295 at this head; the same lines in #2411 (base master) at 1fa1076e9.
Result: the missing-previous-candidate fix is correct as read. With cachedPreviousCandidate empty, the old .get threw inside receive; the new orElse { … .map(…) } yields None, and the None arm replies with an error and leaves state alone. The spec pins both halves: the error text, and theSameInstanceAs on the next GenerateCandidate, which would fail after a restart because a restarted actor has no cached candidate.
Tests run: none locally. Read from the PR's CI: all 8 checks green at d04240bc7.
[Integration] #2411 and this PR edit the same lines from the same blob (080c64409b, :254-266) on different bases. They will collide when v6.0.7 reaches master, and as your description says #2411 keeps state.cachedPreviousCandidate.get.
Recommended: give #2411 the same optional fallback and None arm now, so that merge resolves to one shape instead of reintroducing the .get.
[Nit] CandidateGenerator.scala:258: the "Using previous candidate as a solution" line still prints when there is none (…: None), as it did before the throw. Moving it inside the .map keeps the log truthful.
[Nit] :284: the None arm replies with a fixed string, while the other failure arm passes on exception.getMessage. .filter(_.isSuccess) has already discarded the PoW failure by then; keeping the Try and including its message gives a pool operator the same reason on both paths.
Summary
StatusReply.Errorwhile preserving the current candidate template for later valid work.Root cause
When a solution failed validation against the current candidate,
CandidateGeneratorunconditionally retried it againstcachedPreviousCandidate.get. The first generated candidate has no previous candidate, so an invalid solution threwNoSuchElementException, restarted the actor and returned no normal error to the external miner.The fallback is now optional. If no previous candidate exists, the actor returns an error and keeps the valid candidate template. Current valid solutions, previous-candidate fallback, final PoW validation, block publication and solved-state transitions are unchanged.
Related work
PR #2411 removes duplicate PoW validation on
master. Its latest increment now also handles the absent previous candidate, preserving the validation failure cause. The changes overlap on different target branches; preserve both the optional fallback and single-validation behavior when integrating them. This PR remains unchanged onv6.0.7.Tests
v6.0.7(e927ab38):NoSuchElementException: None.get, followed by the expected-reply timeout.CandidateGeneratorSpec: 20/20 passed.CandidateGeneratorPropSpec+MiningApiRouteSpec: 23/23 passed.git diff --checkand the publication guard passed.