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
add '*' default value for --access-node-ids flag for LN/SN nodes #1426
Conversation
Follow up to this ticket https://github.com/dapperlabs/flow-internal/issues/1594 |
Codecov Report
@@ Coverage Diff @@
## master #1426 +/- ##
==========================================
- Coverage 55.41% 55.39% -0.02%
==========================================
Files 512 512
Lines 31942 31964 +22
==========================================
+ Hits 17700 17707 +7
- Misses 11862 11873 +11
- Partials 2380 2384 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One suggestion to disallow this new configuration for Mainnet, otherwise LGTM
@@ -384,7 +365,7 @@ func prepareConsensusService(container testnet.ContainerConfig, i int, accessNod | |||
fmt.Sprintf("--chunk-alpha=1"), | |||
fmt.Sprintf("--emergency-sealing-active=false"), | |||
fmt.Sprintf("--insecure-access-api=false"), | |||
fmt.Sprintf("--access-node-ids=%s", accessNodeIDS), | |||
fmt.Sprint("--access-node-ids=*"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
cmd/collection/main.go
Outdated
if accessNodeIDS[0] == "*" { | ||
anIDS, err = common.DefaultAccessNodeIDS(node.State.Sealed()) | ||
if err != nil { | ||
return fmt.Errorf("failed to get default access node ids %w", err) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should explicitly disallow this configuration on Mainnet for now.
if accessNodeIDS[0] == "*" { | |
anIDS, err = common.DefaultAccessNodeIDS(node.State.Sealed()) | |
if err != nil { | |
return fmt.Errorf("failed to get default access node ids %w", err) | |
} | |
if accessNodeIDS[0] == "*" { | |
if node.ChainID == flow.Mainnet { | |
return fmt.Errorf("must specify preferred access node IDs for non-test networks") | |
} | |
anIDS, err = common.DefaultAccessNodeIDS(node.State.Sealed()) | |
if err != nil { | |
return fmt.Errorf("failed to get default access node ids %w", err) | |
} |
cmd/consensus/main.go
Outdated
|
||
// if * provided use all ids in protocol state by default | ||
if accessNodeIDS[0] == "*" { | ||
anIDS, err = common.DefaultAccessNodeIDS(node.State.Sealed()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here - disallow this configuration on mainnet
…ow/flow-go into khalil/1583-stake-node-util-func
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very helpful, thank!
cmd/collection/main.go
Outdated
@@ -146,7 +146,7 @@ func main() { | |||
|
|||
// epoch qc contract flags | |||
flags.BoolVar(&insecureAccessAPI, "insecure-access-api", false, "required if insecure GRPC connection should be used") | |||
flags.StringSliceVar(&accessNodeIDS, "access-node-ids", []string{}, fmt.Sprintf("array of access node ID's sorted in priority order where the first ID in this array will get the first connection attempt and each subsequent ID after serves as a fallback. minimum length %d", common.DefaultAccessNodeIDSMinimum)) | |||
flags.StringSliceVar(&accessNodeIDS, "access-node-ids", []string{}, fmt.Sprintf("array of access node ID's sorted in priority order where the first ID in this array will get the first connection attempt and each subsequent ID after serves as a fallback. Minimum length %d. Use '*' for all IDs in protocol state.", common.DefaultAccessNodeIDSMinimum)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
flags.StringSliceVar(&accessNodeIDS, "access-node-ids", []string{}, fmt.Sprintf("array of access node ID's sorted in priority order where the first ID in this array will get the first connection attempt and each subsequent ID after serves as a fallback. Minimum length %d. Use '*' for all IDs in protocol state.", common.DefaultAccessNodeIDSMinimum)) | |
flags.StringSliceVar(&accessNodeIDS, "access-node-ids", []string{}, fmt.Sprintf("array of access node IDs sorted in priority order where the first ID in this array will get the first connection attempt and each subsequent ID serves as a fallback. Minimum length %d. Use '*' for all IDs in protocol state.", common.DefaultAccessNodeIDSMinimum)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cmd/consensus/main.go
Outdated
@@ -145,7 +145,7 @@ func main() { | |||
flags.UintVar(&requiredApprovalsForSealConstruction, "required-construction-seal-approvals", sealing.DefaultRequiredApprovalsForSealConstruction, "minimum number of approvals that are required to construct a seal") | |||
flags.BoolVar(&emergencySealing, "emergency-sealing-active", sealing.DefaultEmergencySealingActive, "(de)activation of emergency sealing") | |||
flags.BoolVar(&insecureAccessAPI, "insecure-access-api", false, "required if insecure GRPC connection should be used") | |||
flags.StringSliceVar(&accessNodeIDS, "access-node-ids", []string{}, fmt.Sprintf("array of access node ID's sorted in priority order where the first ID in this array will get the first connection attempt and each subsequent ID after serves as a fallback. minimum length %d", common.DefaultAccessNodeIDSMinimum)) | |||
flags.StringSliceVar(&accessNodeIDS, "access-node-ids", []string{}, fmt.Sprintf("array of access node ID's sorted in priority order where the first ID in this array will get the first connection attempt and each subsequent ID after serves as a fallback. Minimum length %d. Use '*' for all IDs in protocol state.", common.DefaultAccessNodeIDSMinimum)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
flags.StringSliceVar(&accessNodeIDS, "access-node-ids", []string{}, fmt.Sprintf("array of access node ID's sorted in priority order where the first ID in this array will get the first connection attempt and each subsequent ID after serves as a fallback. Minimum length %d. Use '*' for all IDs in protocol state.", common.DefaultAccessNodeIDSMinimum)) | |
flags.StringSliceVar(&accessNodeIDS, "access-node-ids", []string{}, fmt.Sprintf("array of access node IDs sorted in priority order where the first ID in this array will get the first connection attempt and each subsequent ID serves as a fallback. Minimum length %d. Use '*' for all IDs in protocol state.", common.DefaultAccessNodeIDSMinimum)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
accessNodeIDS := make([]string, 0) | ||
for i, c := range containers { | ||
fmt.Printf("%d: %s", i+1, c.Identity().String()) | ||
if c.Unstaked { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're removing this, you might as well remove the boolean flag Unstaked
in integration/testnet/container.go
. It's obsolete anyway. /cc @vishalchangrani for double-check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -139,3 +128,29 @@ func convertAccessAddrFromState(address string, insecureAccessAPI bool) string { | |||
|
|||
return accessAddress.String() | |||
} | |||
|
|||
// DefaultAccessNodeIDS will return all the access node IDS in the protocol state for staked access nodes | |||
func DefaultAccessNodeIDS(snapshot protocol.Snapshot) ([]flow.Identifier, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- This should probably never be allowed to return 0 IDs.
- This can return < 2 IDs. Depending on whether we're in a test situation, and. whether the test is specifically exercising AN fallback mechanisms, this may or may not be a bad thing. But I would at the very least log a warning there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…obsolete struct field ContainerConfig.Unstaked
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎸
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a bunch!
Co-authored-by: Jordan Schalm <jordan@dapperlabs.com>
bors merge |
This PR adds support for '*' value that can be set for the --acess-node-ids flag on LN & SN nodes. When this value is set LN/SN nodes will use ids of all the access nodes in the protocol state.
i.e :
--access-node-ids=*