Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug] Ring address ordering makes valid subsets beings rejected #536

Merged
merged 3 commits into from
May 17, 2024

Conversation

red-0ne
Copy link
Contributor

@red-0ne red-0ne commented May 16, 2024

Summary

This PR fixes the RelayRequest signature verification process that fails when delegateeGatewayAddresses ordering differs at on-chain verification time.

Issue

The RelayRequest verification step checks that the ring addresses used to sign the request are contained within the expected ring at proof verification time. For this we use ringSig.Ring().Equal(expectedRing).

The actual implementation of Equal [1] imposes that the request ring pubkeys must be in the same order as the expected one.

Using Equal in the following (requestRingAddrs, expectedRingAddrs) examples would yield undesired results in some cases:

* Equals(AB, ABC) == true  // desired
* Equals(BC, ABC) == false // undesired
* Equals(AC, ABC) == false // undesired

Given our delegation and undelegation use cases with their dynamic aspect, we want the (BC, ABC) and (AC, ABC) cases to yield true.

[1] https://github.com/noot/ring-go/blob/master/ring.go#L23

Type of change

Select one or more:

  • New feature, functionality or library
  • Bug fix
  • Code health or cleanup
  • Documentation
  • Other (specify)

Testing

Local Testing (only if making code changes)

  • Unit Tests: make go_develop_and_test
  • LocalNet E2E Tests: make test_e2e
  • See quickstart guide for instructions

PR Testing (only if making code changes)

  • DevNet E2E Tests: Add the devnet-test-e2e label to the PR.
    • THIS IS VERY EXPENSIVE, so only do it after all the reviews are complete.
    • Optionally run make trigger_ci if you want to re-trigger tests without any code changes
    • If tests fail, try re-running failed tests only using the GitHub UI as shown here

Sanity Checklist

  • I have tested my changes using the available tooling
  • I have commented my code
  • I have performed a self-review of my own code; both comments & source code
  • I create and reference any new tickets, if applicable
  • I have left TODOs throughout the codebase, if applicable

@red-0ne red-0ne added the bug Something isn't working label May 16, 2024
@red-0ne red-0ne added this to the Shannon Public TestNet milestone May 16, 2024
@red-0ne red-0ne self-assigned this May 16, 2024
Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

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

left some comments with suggested namings since it was a little confusing for me so long after looking at this quote. PTAL

pkg/crypto/rings/client.go Outdated Show resolved Hide resolved
pkg/crypto/rings/client.go Outdated Show resolved Hide resolved
pkg/crypto/rings/client.go Outdated Show resolved Hide resolved
pkg/crypto/rings/client.go Outdated Show resolved Hide resolved
pkg/crypto/rings/client.go Outdated Show resolved Hide resolved

ringPoints := make(map[string]ringtypes.Point, len(points))
for _, point := range points {
ringPoints[string(point.Encode())] = point
Copy link
Member

Choose a reason for hiding this comment

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

It's not super clear to me what string(point.Encode()). Is it like a hash or something else?

Trying to think if there's a better name to make this clear.

@red-0ne red-0ne requested a review from Olshansk May 17, 2024 01:15
Copy link

The CI will now also run the e2e tests on devnet, which increases the time it takes to complete all CI checks. If you just created a pull request, you might need to push another commit to produce a container image DevNet can utilize to spin up infrastructure. You can use make trigger_ci to push an empty commit.

@github-actions github-actions bot added devnet push-image CI related - pushes images to ghcr.io labels May 17, 2024
@red-0ne red-0ne merged commit b12f5e1 into main May 17, 2024
10 checks passed
@Olshansk Olshansk deleted the fix/ring-equality branch May 29, 2024 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working devnet devnet-test-e2e push-image CI related - pushes images to ghcr.io
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

2 participants