Fix KMS-005: Convert AckData to use addresses for both player and dealer

[mcclurejt]

• adds test to ensure ProveEquivocation does not always revert

[claude]

Claude finished @mcclurejt's task in 3m 56s —— View job

---

Code Review — Fix KMS-005: AckData Uses Addresses for Player and Dealer

• Gather context and understand the PR
• Review core changes (types, crypto, merkle)
• Review DKG/reshare protocol changes
• Review node changes
• Review tests and test utilities
• Review contract changes
• Compile final review

---

Summary

This PR correctly replaces the int64 node ID based identifiers in AckData with common.Address values throughout — the right fix for KMS-005. The consolidation of CreateAcknowledgement into pkg/crypto and the epoch encoding fix (32 bytes → 8 bytes) from prior review feedback are both confirmed and correct. Several issues remain.

---

Bugs / Issues

1. CreateTestAcknowledgements still uses synthetic addresses (pkg/testutil/helpers.go:52)

The function signature still accepts dealerID int64 and converts it with common.BigToAddress(new(big.Int).SetInt64(dealerID)), producing addresses like 0x0000…0063. All integration test callsites (e.g. merkle_ack_integration_test.go:111) continue to pass raw integers. See inline comment for a suggested fix. Fix this →

2. Latent LeafIndex = 0 bug in VerifyOperatorBroadcast (pkg/node/node.go:2045)

proof := &merkle.MerkleProof{
    Leaf:  leafHash,
    Proof: broadcast.MerkleProof,
    // LeafIndex defaults to 0
}

When Phase 7 wires in the actual merkle.VerifyProof call, this will silently pass only for the operator whose address sorts first in SortAcknowledgements, and fail for everyone else. The correct index must be found by locating n.OperatorAddress in the sorted slice before constructing the proof struct. This is already filed as an inline comment. Fix this →

3. Merkle proof is not cryptographically verified (pkg/node/node.go:2053-2055)

if len(proof.Proof) == 0 {
    return fmt.Errorf("merkle proof is empty")
}

The proof is only checked for non-emptiness, not verified against the root. The comment says "Phase 7 will add on-chain root from contract" — that's fine as a phased approach, but the function should be clearly marked incomplete (e.g. a TODO(phase-7) comment) so it doesn't get shipped as-is. Without this check, a dealer can send any non-nil proof and pass verification.

---

Code Quality

4. Field-order asymmetry between signing message and merkle leaf

buildAcknowledgementSigningMessage packs dealer || player || epoch || ..., while HashAcknowledgementForMerkle packs player || dealer || epoch || .... The comment in types.go:82 documents the signing order; the Solidity contract documents the leaf order. Both are correct for their purpose. A short inline comment in each function (e.g. "Note: dealer precedes player here, matching types.Acknowledgement.Signature doc") would prevent future confusion for contributors implementing new verification paths.

5. SendDKGShare and SendReshareShare are near-identical (pkg/transport/client.go)

The two functions differ only in the path string (/dkg/share vs /reshare/share) and SessionTimestamp field source, but share 30+ lines of retry logic. Not a blocker, but extracting a private sendShare(path string, ...) helper would improve maintainability. Fix this →

---

Testing

6. Integration test still passes synthetic dealer address (internal/tests/integration/merkle_ack_integration_test.go:111)

acks := testutil.CreateTestAcknowledgements(t, 3, 5, 99) // epoch=5, dealerID=99

Blocked on issue #1 above.

7. TestAcknowledgementSorting does not verify ordering correctness (pkg/merkle/merkle_test.go:156)

The test verifies determinism across two calls to SortAcknowledgements, but never asserts the actual sort order is ascending. Two calls of an identity function would also pass. A simple assertion like require.True(t, sorted[i].PlayerAddress.Hex() <= sorted[i+1].PlayerAddress.Hex()) for each consecutive pair would catch regressions. Fix this →

---

Positives

• The epoch encoding fix (8 bytes, matching abi.encodePacked(uint64)) is confirmed correct.
• HashAcknowledgement wrapper has been eliminated; crypto.HashAcknowledgementForMerkle is now the single canonical implementation used everywhere.
• CreateAcknowledgement is canonically in pkg/crypto, removing the dkg/reshare duplication.
• The BroadcastCommitmentsWithProofs fix in transport/client.go (sorting acks before iterating to find the correct leafIndex) is correct and important — without this, the leaf index would have been wrong for any non-trivial sort order.
• The new test_ProveEquivocation_Success Solidity test correctly exercises the full equivocation proof path end-to-end, confirming the on-chain encoding and OZ merkle verification work as expected.

The base branch was changed.

[anupsv]

lgtm