MF2-C01: preserve post-Finalize closing marker across challenge cycle

[philanton]

Findings addressed

C-01: post-Finalize submission gate erased across challenge cycle

When SubmitState() signs a TransitionTypeFinalize state, it updates the channel status to ChannelStatusClosing. CheckActiveChannel() then excludes Closing channels because it only returns home channels with status <= ChannelStatusOpen, preventing further user-initiated state submissions.

However, the event handlers can later regress the local lifecycle status. HandleHomeChannelChallenged() unconditionally sets channel.Status = core.ChannelStatusChallenged, even if the channel was already marked ChannelStatusClosing because the node signed a newer finalized state. If a later ChannelCheckpointed event is processed, HandleHomeChannelCheckpointed() changes ChannelStatusChallenged back to ChannelStatusOpen.

This allows the Closing marker to be erased by delayed onchain events. For example, a user can checkpoint state N, obtain signed states N + 1 and N + 2, challenge onchain with N + 1, and immediately request a finalized offchain state N + 3. The node marks the channel Closing. When the ChannelChallenged event for N + 1 is processed, the DB status becomes Challenged. When the user checkpoints N + 2 onchain, the ChannelCheckpointed event moves the DB status back to Open.

At that point, the database again contains an active/open channel even though the latest offchain state signed by the node is finalized. The original post-finalize path can become reachable again: SubmitState() can accept a new transition after the finalized state because CheckActiveChannel() sees an Open channel, and StateAdvancerV1.ValidateAdvancement() can advance from the finalized state into a new epoch. This undermines the remediation for the critical post-finalize funds-theft issue.

Fix. On ChannelCheckpointed, when clearing Challenged, derive the post-Finalize state from channel_states via a new HasSignedFinalize lookup. Restore Closing instead of Open when a node-signed Finalize row exists for the channel. The status field carries onchain reality while the dispute is live; the binding offchain Finalize fact is shadowed in channel_states and reapplied on resolution. No new ChannelStatus enum value, no DB migration, no API/SDK fan-out.

A second vector through the same regression — HandleHomeChannelCreated replaying over an initialized channel (reorg / indexer restart re-fires Created over a current Closing and erases the Finalize marker) — is guarded by a Status >= ChannelStatusOpen short-circuit with a warning log.

L-02: stale ChallengeExpiresAt after challenge resolution

HandleHomeChannelCheckpointed() clears a challenged home channel by changing channel.Status from core.ChannelStatusChallenged to core.ChannelStatusOpen, but it does not clear channel.ChallengeExpiresAt. Since HandleHomeChannelChallenged() persists channel.ChallengeExpiresAt when a challenge begins, a later checkpoint that resolves the challenge can leave the local database with an open channel that still reports a stale challenge expiry.

This does not appear to block normal state submission, because CheckOpenChannel() uses the channel status rather than ChallengeExpiresAt when deciding whether a home channel is open. However, the stale timestamp is exposed through the channel API and can mislead clients, dashboards, or operators into believing an open channel still has an active or historical challenge deadline attached to it.

Fix. HandleHomeChannelCheckpointed() now sets channel.ChallengeExpiresAt = nil alongside the status flip when clearing Challenged. HandleHomeChannelClosed() mirrors the same clear on terminal close so a lingering expiry never trails a closed channel.

I-01: misleading HandleNodeBalanceUpdated doc comment

The comment for HandleNodeBalanceUpdated() states that the handler updates the user's staked balance for the specified blockchain. The implementation actually calls SetNodeBalance() with event.BlockchainID, event.Asset, and event.Balance, which stores the node's onchain liquidity metric under node_balance. This can mislead maintainers into thinking the event affects user staking state, while it only updates observability data for node liquidity.

Fix. Corrected the doc comment on HandleNodeBalanceUpdated to describe the actual semantics: it upserts the node's onchain liquidity for the given blockchain/asset pair, not user staking state.

Test plan

• go vet ./...
• go test ./...
• New unit test TestHandleHomeChannelCheckpointed_FromChallengedWithSignedFinalize exercises the C-01 audit scenario: status restored to Closing when a signed Finalize exists.
• New integration test TestDBStore_HasSignedFinalize covers the storage primitive (present / absent / present-but-unsigned).
• Existing wasChallenged-path tests updated to mock the new lookup and assert ChallengeExpiresAt == nil after resolution (L-02 coverage).

Summary by CodeRabbit

• Bug Fixes
  • Improved handling of challenged channels when disputes are resolved through checkpoints; channels now correctly transition to "Closing" if a finalization has been signed, or "Open" otherwise.
  • Fixed channel state tracking to properly clear challenge expiry information when channels transition to closed or reopened states.

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR extends challenge-resolution logic with a query to detect node-signed Finalize states. When checkpointed events clear a Challenged status, the handler now restores channels to Closing (if Finalize exists) or Open, and explicitly clears challenge-expiry timestamps during all terminal-state transitions (Closed, Closing, Open).

Changes

Challenge Finalize State Detection and Cleanup

Layer / File(s)	Summary
Interface contracts for HasSignedFinalize pkg/core/interface.go, nitronode/store/database/interface.go, pkg/blockchain/evm/channel_hub_reactor.go	Define the new HasSignedFinalize(channelID string) (bool, error) method across public store interfaces to query whether a node-signed Finalize state exists.
Database implementation of HasSignedFinalize nitronode/store/database/state.go, nitronode/store/database/db_store_test.go	Implement HasSignedFinalize in DBStore by counting Finalize-transition rows, with integration test validating the query.
Event handler challenge resolution logic nitronode/event_handlers/service.go	Update HandleHomeChannelCheckpointed to consult HasSignedFinalize when restoring from Challenged status (Closing if finalized, Open otherwise), clear ChallengeExpiresAt, and update related documentation; also clear ChallengeExpiresAt in home/escrow close handlers.
Event handler tests with challenge clearing assertions nitronode/event_handlers/service_test.go	Expand test suite with assertions on ChallengeExpiresAt cleanup, mock HasSignedFinalize expectations for status-restoration paths, add regression test for signed-Finalize checkpoint handling, and update escrow-finalization tests.
Test doubles for HasSignedFinalize nitronode/event_handlers/testing.go, pkg/blockchain/evm/channel_hub_reactor_test.go	Implement HasSignedFinalize mock methods in test stores to support realistic test scenarios.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• layer-3/nitrolite#720: Introduces ChallengeExpiresAt field and sets it when handling challenged events; this PR clears and uses that same field during downstream checkpoint/close transitions.
• layer-3/nitrolite#754: Updates escrow challenge-expiry clearing on cooperative finalization in Solidity; this PR mirrors that cleanup in Go event handlers for both home and escrow channels.

Suggested reviewers

• ihsraham
• nksazonov
• dimast-x

---

🐰 A finalize state once signed remains forever known,
Yet challenges may cloud the truth till checkpoints are shown.
When dispute clears, we peek at what's been locked away,
And restore the channel's heart to Closing—or Open—today! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 38.46% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'MF2-C01: preserve post-Finalize closing marker across challenge cycle' directly and accurately summarizes the main change: implementing logic to preserve the post-Finalize Closing status when challenges are resolved via Checkpointed events, addressing audit finding MF2-C01.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/nitronode-mf2-c01

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[nksazonov]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

🧹 Nitpick comments (4)

nitronode/store/database/db_store_test.go (1)

1128-1202: ⚡ Quick win

Consider adding test coverage for unsigned Finalize edge case.

The test covers the happy path (node-signed Finalize exists → true) and the negative case (no Finalize → false). If you adopt the recommendation to explicitly check node_sig IS NOT NULL in the query, consider adding a test case that stores a Finalize state without a node signature and verifies the method returns false. This would ensure the defensive check works as intended.

🧪 Example test case

t.Run("False when unsigned Finalize exists", func(t *testing.T) {
	db, cleanup := SetupTestDB(t)
	defer cleanup()
	store := NewDBStore(db)
	setupChannel(t, store)

	// Store Finalize without node signature
	storeState(t, store, 7, core.TransitionTypeFinalize, false)

	got, err := store.HasSignedFinalize(homeChannelID)
	require.NoError(t, err)
	assert.False(t, got, "unsigned Finalize should not be detected")
})

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@nitronode/store/database/db_store_test.go` around lines 1128 - 1202, Add a
test case to TestDBStore_HasSignedFinalize that verifies an unsigned Finalize is
not counted: use the existing setupChannel helper and the storeState helper to
store a Finalize (core.TransitionTypeFinalize) with hasNodeSig=false, call
HasSignedFinalize(homeChannelID) and assert it returns false (no error); this
ensures the DBStore.HasSignedFinalize implementation correctly requires a
non-null node signature.

nitronode/store/database/state.go (2)

264-264: 💤 Low value

Remove unnecessary Limit(1) before Count.

The Limit(1) clause has no effect on Count() in SQL—Count still processes all matching rows. This line is misleading and can be removed for clarity.

♻️ Proposed cleanup

 	err := s.db.Model(&State{}).
 		Where("home_channel_id = ? AND transition_type = ?",
 			strings.ToLower(channelID), uint8(core.TransitionTypeFinalize)).
-		Limit(1).
 		Count(&count).Error

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@nitronode/store/database/state.go` at line 264, Remove the unnecessary
Limit(1) call that precedes a Count() on the DB query in state.go; locate the
query chain that calls Limit(1).Count(...) (within the method using the
store/database state) and delete the Limit(1) invocation so that the Count call
is not misleading and accurately reflects that it counts all matching rows.

---

253-270: ⚡ Quick win

Consider explicitly checking node_sig IS NOT NULL for defensive programming.

The implementation relies on the invariant stated in the doc comment (line 257-258) that Finalize rows are only written after the node signs. However, for an audit-finding fix, it would be safer to explicitly check node_sig IS NOT NULL in the WHERE clause rather than relying on an application-level invariant that could be violated through bugs, data migrations, or manual database changes.

🛡️ Proposed defensive query

 func (s *DBStore) HasSignedFinalize(channelID string) (bool, error) {
 	var count int64
 	err := s.db.Model(&State{}).
-		Where("home_channel_id = ? AND transition_type = ?",
-			strings.ToLower(channelID), uint8(core.TransitionTypeFinalize)).
+		Where("home_channel_id = ? AND transition_type = ? AND node_sig IS NOT NULL",
+			strings.ToLower(channelID), uint8(core.TransitionTypeFinalize)).
 		Limit(1).
 		Count(&count).Error

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@nitronode/store/database/state.go` around lines 253 - 270, HasSignedFinalize
currently counts Finalize rows relying on the invariant that node_sig is set;
update the DB query in HasSignedFinalize (on the State model) to explicitly
require node_sig IS NOT NULL in the WHERE clause in addition to home_channel_id
= strings.ToLower(channelID) and transition_type =
uint8(core.TransitionTypeFinalize), so the count only includes rows with a
non-null node_sig; keep the Count(&count) logic and existing error handling.

pkg/blockchain/evm/channel_hub_reactor.go (1)

79-81: 💤 Low value

Consider aligning the doc comment with the other interfaces.

The doc comment for HasSignedFinalize in this interface is shorter than in pkg/core/interface.go and nitronode/store/database/interface.go, which both explain the use case (detecting post-Finalize lifecycle when status is overwritten by challenge). Adding this context here would improve consistency and help future maintainers understand the purpose.

📝 Suggested doc comment alignment

-	// HasSignedFinalize reports whether a node-signed Finalize state exists for the given
-	// home channel.
+	// HasSignedFinalize reports whether a node-signed Finalize state exists for the given
+	// home channel. Used by event handlers to detect the post-Finalize lifecycle when the
+	// channel status field has been temporarily overwritten by an on-chain challenge.
 	HasSignedFinalize(channelID string) (bool, error)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/blockchain/evm/channel_hub_reactor.go` around lines 79 - 81, Update the
doc comment for HasSignedFinalize to match the other interfaces by adding the
use-case description: explain that HasSignedFinalize(channelID string) reports
whether a node-signed Finalize state exists for the given home channel and is
used to detect the post-Finalize lifecycle when a channel's status may be
overwritten by a later challenge; keep the signature and return types unchanged
and ensure the comment wording mirrors the explanatory intent used in the other
interface declarations.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@nitronode/store/database/db_store_test.go`:
- Around line 1128-1202: Add a test case to TestDBStore_HasSignedFinalize that
verifies an unsigned Finalize is not counted: use the existing setupChannel
helper and the storeState helper to store a Finalize
(core.TransitionTypeFinalize) with hasNodeSig=false, call
HasSignedFinalize(homeChannelID) and assert it returns false (no error); this
ensures the DBStore.HasSignedFinalize implementation correctly requires a
non-null node signature.

In `@nitronode/store/database/state.go`:
- Line 264: Remove the unnecessary Limit(1) call that precedes a Count() on the
DB query in state.go; locate the query chain that calls Limit(1).Count(...)
(within the method using the store/database state) and delete the Limit(1)
invocation so that the Count call is not misleading and accurately reflects that
it counts all matching rows.
- Around line 253-270: HasSignedFinalize currently counts Finalize rows relying
on the invariant that node_sig is set; update the DB query in HasSignedFinalize
(on the State model) to explicitly require node_sig IS NOT NULL in the WHERE
clause in addition to home_channel_id = strings.ToLower(channelID) and
transition_type = uint8(core.TransitionTypeFinalize), so the count only includes
rows with a non-null node_sig; keep the Count(&count) logic and existing error
handling.

In `@pkg/blockchain/evm/channel_hub_reactor.go`:
- Around line 79-81: Update the doc comment for HasSignedFinalize to match the
other interfaces by adding the use-case description: explain that
HasSignedFinalize(channelID string) reports whether a node-signed Finalize state
exists for the given home channel and is used to detect the post-Finalize
lifecycle when a channel's status may be overwritten by a later challenge; keep
the signature and return types unchanged and ensure the comment wording mirrors
the explanatory intent used in the other interface declarations.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e32fe25a-36d3-4b9d-9141-22a1e0eb2f86

📥 Commits

Reviewing files that changed from the base of the PR and between 6f38d21 and 8619685.

📒 Files selected for processing (9)

• nitronode/event_handlers/service.go
• nitronode/event_handlers/service_test.go
• nitronode/event_handlers/testing.go
• nitronode/store/database/db_store_test.go
• nitronode/store/database/interface.go
• nitronode/store/database/state.go
• pkg/blockchain/evm/channel_hub_reactor.go
• pkg/blockchain/evm/channel_hub_reactor_test.go
• pkg/core/interface.go

[nksazonov]

Solid fix for the core Closing → Challenged → Open regression — the HasSignedFinalize lookup in HandleHomeChannelCheckpointed is the right mechanism. Two concerns require attention before landing: a gap in query defensiveness (see inline comments) and a structurally identical regression path via event replay that this fix does not cover.

---

Out-of-diff: HandleHomeChannelCreated can regress Closing → Open via event replay (service.go:66)

HandleHomeChannelCreated unconditionally sets channel.Status = core.ChannelStatusOpen. If the HomeChannelCreated on-chain event is replayed — chain reorg, indexer restart, or block reprocessing — a channel already in ChannelStatusClosing is reset to Open, re-arming CheckActiveChannel and the submission gate. This is structurally the same regression path this PR closes in HandleHomeChannelCheckpointed. The system already treats replay as a real concern (UpdateStateSigsIfMissing is explicitly idempotent). The same HasSignedFinalize guard, or a simpler if channel.Status >= core.ChannelStatusClosing { return nil } early-out, should be applied here.

---

Out-of-diff: backfillOffChainHeadNodeSig runs unconditionally after restoring Closing (service.go:158–159)

backfillOffChainHeadNodeSig is called in the wasChallenged branch regardless of the HasSignedFinalize result. In practice this is safe — the Finalize state is the head and already has NodeSig != nil, so the function exits early. But the intent is non-obvious: a reader must trace two functions to confirm no post-Finalize state ever receives a node signature here. The finalized variable is already in scope; gating the call on !finalized makes the invariant explicit and eliminates the silent dependency on the Finalize always being the head.

[philanton]

Thanks for catching the HandleHomeChannelCreated replay path — same regression vector through a different door (Void→Open is supposed to be one-shot; reorg/indexer-restart re-fires Created over a current Closing and erases the Finalize marker). Guarded with Status >= ChannelStatusOpen + warning log in f4a32bc, with a table-driven test across Open/Challenged/Closing/Closed.

On gating backfillOffChainHeadNodeSig on !finalized: I'd leave it. The function's invariant is "node-sign the head if missing", not "node-sign unless we're in Closing" — folding the Finalize semantics into the caller spreads the lifecycle concern. The existing head.NodeSig != nil early-return already covers the Finalize-as-head case explicitly, and gating on finalized would still call GetLastStateByChannelID in the non-finalized branch, so the perf delta is zero.

[nksazonov]

Great work!

[ihsraham]

Approving after clarification. The future C-01 path this PR targets appears covered.

Given the only current environment is stress, and stress does not contain the Open + signed Finalize edge state, I do not see a migration or repair blocker here.

The Open + signed Finalize case can stay as a P2 hardening follow-up for corrupted or future persistent state.