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

[v2][node] Construct relay client from relay address from chain #931

Merged
merged 1 commit into from
Dec 2, 2024

Conversation

ian-shim
Copy link
Contributor

Why are these changes needed?

Node creates relay client with relay address from chain

Checks

  • I've made sure the tests are passing. Note that there might be a few flaky tests, in that case, please comment that they are not relevant.
  • I've checked the new test coverage and the coverage percentage didn't drop.
  • Testing Strategy
    • Unit tests
    • Integration tests
    • This PR is not tested :(

@ian-shim ian-shim marked this pull request as ready for review November 22, 2024 23:47
@@ -24,3 +26,7 @@ func (m *ReadOnlyMap[K, V]) Keys() []K {
func (m *ReadOnlyMap[K, V]) Len() int {
return len(m.data)
}

func (m *ReadOnlyMap[K, V]) Equal(data map[K]V) bool {
return reflect.DeepEqual(m.data, data)
Copy link
Contributor

Choose a reason for hiding this comment

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

Reflection may have perf issue, so may be not a good choice for non testing use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point

@@ -161,7 +161,7 @@ func (b *BlobHeader) GetEncodingParams(blobParams *core.BlobVersionParameters) (
}, nil
}

type RelayKey uint16
type RelayKey = uint16
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the consideration of changing to alias? The current one provides type safety which is often desired

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's mostly to interoperate with chainio library which still lives in core and can't import v2 due to import cycles.
So chainio library uses uint16 instead of RelayKey type, and I updated this to type alias to convert the output from chainio without an explicit conversion method

continue
}

relayClient, err := clients.NewRelayClient(&clients.RelayClientConfig{
Copy link
Contributor

Choose a reason for hiding this comment

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

Relay url may be updated much more frequently than protocol params, should it be in a different interval/thread?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I agree in terms of relative frequency, but I still think the frequency for new relays being added and getting used would be in order of hours if not days.

Copy link
Contributor

@cody-littley cody-littley left a comment

Choose a reason for hiding this comment

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

Perhaps this isn't the correct PR to add signing of GetChunks() requests via auth.SignGetChunksRequest(keys *core.KeyPair, request *pb.GetChunksRequest) []byte, just pointing out that its something we still need to do. We can still test without this by setting the flag AuthenticationDisabled=true, but that's not something we will want to deploy to mainnet.

@@ -34,7 +34,8 @@ type RawBundles struct {
}

func (n *Node) DownloadBundles(ctx context.Context, batch *corev2.Batch, operatorState *core.OperatorState) ([]*corev2.BlobShard, []*RawBundles, error) {
if n.RelayClient == nil {
relayClient, ok := n.RelayClient.Load().(clients.RelayClient)
Copy link
Contributor

Choose a reason for hiding this comment

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

will this panic if Load() returns nil?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. In that case, relayClient will be nil and ok will be false

@ian-shim ian-shim merged commit f3a9c52 into Layr-Labs:master Dec 2, 2024
6 checks passed
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.

3 participants