Skip to content

audit: implement LEP-6 heal-op lifecycle and recheck handlers#120

Open
j-rafique wants to merge 1 commit intoLEP-6-shadow-scoringfrom
LEP-6-heal-op-lifecycle
Open

audit: implement LEP-6 heal-op lifecycle and recheck handlers#120
j-rafique wants to merge 1 commit intoLEP-6-shadow-scoringfrom
LEP-6-heal-op-lifecycle

Conversation

@j-rafique
Copy link
Copy Markdown
Contributor

  • Implement storage-truth heal-op lifecycle and recheck handlers
  • Scope LEP-6 heal op lifecycle to PR4

- Implement storage-truth heal-op lifecycle and recheck handlers
- Scope LEP-6 heal op lifecycle to PR4
@roomote-v0
Copy link
Copy Markdown

roomote-v0 Bot commented Apr 20, 2026

Rooviewer Clock   See task

Reviewed the heal-op lifecycle implementation. Found 3 issues:

  • Bug: ClaimHealComplete double-appends req.Details to healOp.Notes in the zero-verifier (single-node) path -- once before calling finalizeHealOp and once inside it
  • Cleanup: finalizeHealOp calls m.GetParams(ctx).WithDefaults() twice in the same block; can be read once into a local variable (also, GetParams already applies WithDefaults internally)
  • Performance: GetAllHealOps full KV scan runs twice per epoch end -- once in expireStorageTruthHealOpsAtEpochEnd and again in scheduleStorageTruthHealOpsAtEpochEnd

Mention @roomote in a comment to request specific changes to this pull request or fix all unresolved issues.

Comment on lines +106 to +110
healOp.Notes = appendStorageTruthNote(healOp.Notes, req.Details)

// Single-node networks may not have verifier assignments; finalize immediately.
if len(healOp.VerifierSupernodeAccounts) == 0 {
if err := m.finalizeHealOp(sdkCtx, healOp, true, req.HealManifestHash, req.Details); err != nil {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

req.Details is appended to healOp.Notes here on line 106, and then finalizeHealOp is called on line 110 with req.Details as the details parameter, which appends it a second time (line 246). On single-node networks (zero verifiers), this produces duplicated notes like "my details | my details". The test on line 246 of the test file uses require.Contains so it doesn't catch the duplication.

Suggested change
healOp.Notes = appendStorageTruthNote(healOp.Notes, req.Details)
// Single-node networks may not have verifier assignments; finalize immediately.
if len(healOp.VerifierSupernodeAccounts) == 0 {
if err := m.finalizeHealOp(sdkCtx, healOp, true, req.HealManifestHash, req.Details); err != nil {
healOp.Notes = appendStorageTruthNote(healOp.Notes, req.Details)
// Single-node networks may not have verifier assignments; finalize immediately.
if len(healOp.VerifierSupernodeAccounts) == 0 {
if err := m.finalizeHealOp(sdkCtx, healOp, true, req.HealManifestHash, ""); err != nil {

Fix it with Roo Code or mention @roomote and request a fix.

Comment on lines +258 to +264
if verified {
currentEpoch, err := deriveEpochAtHeight(ctx.BlockHeight(), m.GetParams(ctx).WithDefaults())
if err != nil {
return err
}
ticketState.LastHealEpoch = currentEpoch.EpochID
ticketState.ProbationUntilEpoch = currentEpoch.EpochID + uint64(m.GetParams(ctx).WithDefaults().StorageTruthProbationEpochs)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

m.GetParams(ctx).WithDefaults() is called twice in this block (lines 259 and 264), each hitting the KV store. Since GetParams already applies WithDefaults() internally, both the duplicate store read and the outer .WithDefaults() are redundant. Consider reading params once into a local variable.

Suggested change
if verified {
currentEpoch, err := deriveEpochAtHeight(ctx.BlockHeight(), m.GetParams(ctx).WithDefaults())
if err != nil {
return err
}
ticketState.LastHealEpoch = currentEpoch.EpochID
ticketState.ProbationUntilEpoch = currentEpoch.EpochID + uint64(m.GetParams(ctx).WithDefaults().StorageTruthProbationEpochs)
params := m.GetParams(ctx)
currentEpoch, err := deriveEpochAtHeight(ctx.BlockHeight(), params)
if err != nil {
return err
}
ticketState.LastHealEpoch = currentEpoch.EpochID
ticketState.ProbationUntilEpoch = currentEpoch.EpochID + uint64(params.StorageTruthProbationEpochs)

Fix it with Roo Code or mention @roomote and request a fix.

Comment on lines +15 to +20
func (k Keeper) ProcessStorageTruthHealOpsAtEpochEnd(ctx sdk.Context, epochID uint64, params types.Params) error {
if err := k.expireStorageTruthHealOpsAtEpochEnd(ctx, epochID); err != nil {
return err
}
return k.scheduleStorageTruthHealOpsAtEpochEnd(ctx, epochID, params)
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both expireStorageTruthHealOpsAtEpochEnd (line 23) and scheduleStorageTruthHealOpsAtEpochEnd (line 77) independently call GetAllHealOps, resulting in two full KV-store scans of every heal op per epoch end. As the number of heal ops accumulates over time, this becomes increasingly expensive. Consider fetching the list once in ProcessStorageTruthHealOpsAtEpochEnd and passing it to both sub-functions.

Fix it with Roo Code or mention @roomote and request a fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant