Skip to content

Conversation

@dai1975
Copy link

@dai1975 dai1975 commented May 8, 2025

Change prover and verifier to use NonMembership proof when value is null

@dai1975 dai1975 requested a review from Copilot May 8, 2025 09:02
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates the prover and verifier implementations to use NonMembership proofs when provided with a null value.

  • In relay/prover.go, a new branch handles empty value cases by invoking VerifyNonMembership instead of VerifyMembership.
  • In light-clients/lcp/types/client_state.go, the verification logic is modified to only compute and compare the hash when the value is non-empty.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
relay/prover.go Adds NonMembership proof handling for empty values and updates the proof decoding pipeline.
light-clients/lcp/types/client_state.go Skips computing the hash for empty values during membership verification.
Comments suppressed due to low confidence (1)

relay/prover.go:345

  • [nitpick] The variable name 'cp' is ambiguous. Consider renaming it to 'commitmentProofs' to improve clarity.
var cp lcptypes.CommitmentProofs

@dai1975 dai1975 changed the title prover and verifier treat proof as for NonMembership when value is null NonMembership proof May 8, 2025
@dai1975 dai1975 requested review from bluele and siburu May 8, 2025 09:06
@dai1975 dai1975 marked this pull request as ready for review May 8, 2025 09:32
@siburu siburu requested a review from Copilot May 18, 2025 13:59
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates the logic for state proof verification to use a NonMembership proof when the provided value is empty.

  • In relay/prover.go, a new branch handles NonMembership proofs using elc.MsgVerifyNonMembership when value is null.
  • In light-clients/lcp/types/client_state.go, the code now computes the hashed value only when a value is provided during membership verification.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
relay/prover.go Added a new branch to request a NonMembership proof when the input value is empty.
light-clients/lcp/types/client_state.go Updated the value hashing logic to only execute for non-empty values, aligning with NonMembership proofs.

Comment on lines +355 to +356
res, err := pr.lcpServiceClient.VerifyNonMembership(ctx.Context(), &m)
if err != nil {
return nil, clienttypes.Height{}, fmt.Errorf("failed ELC's VerifyNonMembership: elc_client_id=%v msg=%v %w", pr.config.ElcClientId, m, err)
}
cp.Message = res.Message
cp.Signatures = [][]byte{res.Signature}
Copy link

Copilot AI May 18, 2025

Choose a reason for hiding this comment

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

[nitpick] Both the NonMembership and Membership branches assign cp.Message and cp.Signatures in an identical manner. Consider refactoring to reduce code duplication by extracting these assignments outside the conditional branches.

Suggested change
res, err := pr.lcpServiceClient.VerifyNonMembership(ctx.Context(), &m)
if err != nil {
return nil, clienttypes.Height{}, fmt.Errorf("failed ELC's VerifyNonMembership: elc_client_id=%v msg=%v %w", pr.config.ElcClientId, m, err)
}
cp.Message = res.Message
cp.Signatures = [][]byte{res.Signature}
var err error
res, err = pr.lcpServiceClient.VerifyNonMembership(ctx.Context(), &m)
if err != nil {
return nil, clienttypes.Height{}, fmt.Errorf("failed ELC's VerifyNonMembership: elc_client_id=%v msg=%v %w", pr.config.ElcClientId, m, err)
}

Copilot uses AI. Check for mistakes.
Copy link
Author

Choose a reason for hiding this comment

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

The return type of VerifyMembership and VerifyMembership is not compatible.
I think generics is needed to commonalize these code, it is a bit over spec at this situation.

@dai1975 dai1975 requested a review from Copilot May 19, 2025 05:56
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR modifies the prover and verifier logic to use NonMembership proofs when the provided value is null.

  • In relay/prover.go, a branch is added to call VerifyNonMembership and assign the corresponding proof and signature when value is empty.
  • In light-clients/lcp/types/client_state.go, the hash computation is conditioned on the value being non-empty to support NonMembership logic.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
relay/prover.go Added NonMembership branch and adjusted proof encoding for empty values.
light-clients/lcp/types/client_state.go Modified hash computation logic to conditionally handle empty values for non-membership.
Comments suppressed due to low confidence (2)

relay/prover.go:386

  • [nitpick] Consider renaming 'cpEnc' to 'encodedProofs' or another more descriptive name to improve clarity.
cpEnc, err := lcptypes.EthABIEncodeCommitmentProofs(&cp)

light-clients/lcp/types/client_state.go:236

  • Ensure that there are unit tests covering both the membership and non-membership verification paths, as the new conditional logic impacts how the hashed value is computed.
hashedValue := crypto.Keccak256Hash(value)

Daisuke Kanda and others added 3 commits June 5, 2025 02:01
Signed-off-by: Daisuke Kanda <daisuke.kanda@datachain.jp>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Signed-off-by: Daisuke Kanda <daisuke.kanda@datachain.jp>
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.

2 participants