Skip to content

Use BuildResult for init proof signing#28

Merged
GalRogozinski merged 1 commit into
mainfrom
init-proof-root
Mar 23, 2026
Merged

Use BuildResult for init proof signing#28
GalRogozinski merged 1 commit into
mainfrom
init-proof-root

Conversation

@nkryuchkov
Copy link
Copy Markdown
Contributor

Summary

Fix the init-path proof signing mismatch in dkg-spec.

Operator.Init() was signing ceremony proofs with proof.MarshalSSZ(), while both BuildResult() and VerifyCeremonyProof() use proof.HashTreeRoot(). That meant anyone following the stubbed Init() implementation as the operator reference would produce proofs that fail verification.

This change refactors Operator.Init() to call BuildResult() directly, so init/reshare/resign all share the same result-building and proof-signing path.

Changes

  • Refactor Operator.Init() to delegate to BuildResult()
  • Remove duplicate init proof-signing logic
  • Add a regression test showing:
    • HashTreeRoot()-signed proofs verify
    • MarshalSSZ()-signed proofs do not verify

@nkryuchkov
Copy link
Copy Markdown
Contributor Author

@greptileai please review this PR

Copy link
Copy Markdown

@y0sher y0sher left a comment

Choose a reason for hiding this comment

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

small comment.

Comment thread operator.go
Copy link
Copy Markdown

@iurii-ssv iurii-ssv left a comment

Choose a reason for hiding this comment

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

Looks reasonable, but we really need repo maintainers to approve before merging

Copy link
Copy Markdown

@ljuba-ssv ljuba-ssv left a comment

Choose a reason for hiding this comment

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

Correct fix — eliminates the duplicate code path by delegating to BuildResult(). All three operator paths (Init, Reshare, Resign) now share the same signing logic. Regression test is well-designed. Net -29 lines.

Copy link
Copy Markdown

@MatheusFranco99 MatheusFranco99 left a comment

Choose a reason for hiding this comment

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

LGTM!
One small thing: I see that BuildResult is tested, but not Operator.Init itself, idk if it's relevant though

@nkryuchkov nkryuchkov requested a review from y0sher March 23, 2026 14:17
@GalRogozinski GalRogozinski enabled auto-merge (squash) March 23, 2026 14:27
@nkryuchkov
Copy link
Copy Markdown
Contributor Author

LGTM! One small thing: I see that BuildResult is tested, but not Operator.Init itself, idk if it's relevant though

I didn’t add a direct Operator.Init test because Init() is still a stub around the ceremony and doesn’t produce a real share/validator pubkey on its own. Since this change is specifically about the proof-signing path, I covered that at the shared implementation point (BuildResult) and with TestVerifyCeremonyProofSignaturePath, which checks the exact MarshalSSZ() vs HashTreeRoot() mismatch.

@GalRogozinski GalRogozinski merged commit 41d4052 into main Mar 23, 2026
4 checks passed
@GalRogozinski GalRogozinski deleted the init-proof-root branch March 23, 2026 14:57
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.

6 participants