fix: correct logic for ctx signing start #568
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the logic for determining when context signing should be enabled in the finality provider service. The change corrects conditional statements that control whether signing operations use additional context information.
- Inverts comparison logic from
ContextSigningHeight > currentHeighttocurrentHeight >= ContextSigningHeight - Removes explicit setting of
ContextSigningHeightto max uint64 value in test configurations - Simplifies conditional logic in the proof-of-possession creation method
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| itest/babylon/babylon_test_manager.go | Removes explicit max uint64 assignment for context signing height |
| finality-provider/service/rand_committer.go | Fixes comparison logic for context signing activation |
| finality-provider/service/fp_test_helper.go | Corrects conditional logic for context-aware signing |
| finality-provider/service/finality_submitter.go | Updates comparison operator for context signing threshold |
| finality-provider/service/eots_manager_adapter.go | Fixes both commit and finality signature context logic |
| finality-provider/service/app.go | Simplifies proof-of-possession context signing condition |
| bsn/rollup/e2e/rollup_test_manager.go | Removes max uint64 context signing height assignment |
| bsn/cosmos/e2e/bcd_test_manager.go | Removes max uint64 context signing height assignment |
| // nextHeight-1 might underflow if the nextHeight is 0 | ||
| if (nextHeight == 0 && app.config.ContextSigningHeight > 0) || | ||
| (nextHeight > 0 && app.config.ContextSigningHeight > nextHeight-1) { | ||
| if nextHeight >= app.config.ContextSigningHeight { |
There was a problem hiding this comment.
The simplified condition may not handle the edge case where nextHeight is 0 and ContextSigningHeight is 0. The original logic explicitly handled nextHeight == 0 as a special case, but this new condition would evaluate to true when both values are 0, which may not be the intended behavior.
KonradStaniec
left a comment
There was a problem hiding this comment.
fixed condition looks good 👍 it would be great to add some tests catching this (can be done in subsequent prs though)
No description provided.