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 Retrieval Isolation #1181

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

supriya-premkumar
Copy link

Why are these changes needed?

  • Updates operator socket format to host:v1DispersalPort;v1RetrievalPort;v2DispersalPort;V2RetrievalPort
  • Refactors parseOperatorSocket to handle v2 retrieval socket with strict port validation
  • Adds ValidatePort() to core utils
  • Adds stricter validation for operator sockets. Valid formats:
    • host:v1DispersalPort;v1RetrievalPort
    • host:v1DispersalPort;v1RetrievalPort;v2DispersalPort;V2RetrievalPort
  • Adds host validation supporting both FQDN and IP addr
  • Registers node for the new V2 retrieval service

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

@supriya-premkumar supriya-premkumar force-pushed the supriya-egda-819-v1v2-port-separation branch 3 times, most recently from 6da5a24 to f33b818 Compare January 29, 2025 10:28
@@ -252,19 +253,33 @@ func NewConfig(ctx *cli.Context) (*Config, error) {
return nil, fmt.Errorf("v2 dispersal port (NODE_V2_DISPERSAL_PORT) must be specified if v2 is enabled")
}
} else {
_, err = strconv.Atoi(v2DispersalPort)
// TODO: should we mandate v2Enabled == true
Copy link
Author

Choose a reason for hiding this comment

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

corner case: I think this and line:269 needs to check that v2Enabled==true This will avoid user errors of adding a v2 port in the flag without enabling v2.

cc: @ian-shim holding off on making this change to confirm intent.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that makes sense. Although we would need to update this logic when we update the flags that specify the mode

@supriya-premkumar supriya-premkumar force-pushed the supriya-egda-819-v1v2-port-separation branch from f33b818 to a34665a Compare January 29, 2025 18:54
- Updates operator socket format to host:v1DispersalPort;v1RetrievalPort;v2DispersalPort;V2RetrievalPort
- Refactors parseOperatorSocket to handle v2 retrieval socket with strict port validation
- Adds ValidatePort() to core utils
- Adds stricter validation for operator sockets. Valid formats:
    - host:v1DispersalPort;v1RetrievalPort
    - host:v1DispersalPort;v1RetrievalPort;v2DispersalPort;V2RetrievalPort
- Adds host validation supporting both FQDN and IP addr
- Registers node for the new V2 retrieval service
@supriya-premkumar supriya-premkumar force-pushed the supriya-egda-819-v1v2-port-separation branch from a34665a to c446a82 Compare January 29, 2025 21:40
}

type StakeAmount = *big.Int

func ParseOperatorSocket(socket string) (host string, dispersalPort string, retrievalPort string, v2DispersalPort string, err error) {
func ParseOperatorSocket(socket string) (host, v1DispersalPort, v1RetrievalPort, v2DispersalPort, v2RetrievalPort string, err error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe it's time to make OperatorSocket a struct and return the struct?
Number of return variables is getting big 🤣

@@ -252,19 +253,33 @@ func NewConfig(ctx *cli.Context) (*Config, error) {
return nil, fmt.Errorf("v2 dispersal port (NODE_V2_DISPERSAL_PORT) must be specified if v2 is enabled")
}
} else {
_, err = strconv.Atoi(v2DispersalPort)
// TODO: should we mandate v2Enabled == true
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that makes sense. Although we would need to update this logic when we update the flags that specify the mode

@@ -109,5 +109,34 @@ func RunServers(serverV1 *Server, serverV2 *ServerV2, config *node.Config, logge
}
}()

go func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

we should remove serverV2 from L102 and update the server options in L95

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