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

[Observer] Add bootstrap command to generate observer networking key #2087

Merged
merged 8 commits into from Mar 20, 2022

Conversation

peterargue
Copy link
Contributor

@peterargue peterargue commented Feb 28, 2022

Closes: #2087

This adds a new command to generate the networking key used by observers on the public libp2p network.

Note: this key uses a different format from the normal staked networking key.

@codecov-commenter
Copy link

Codecov Report

Merging #2087 (2170a69) into master (0bd50cf) will decrease coverage by 0.33%.
The diff coverage is 63.63%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2087      +/-   ##
==========================================
- Coverage   57.63%   57.29%   -0.34%     
==========================================
  Files         631      635       +4     
  Lines       36233    36578     +345     
==========================================
+ Hits        20884    20959      +75     
- Misses      12732    12984     +252     
- Partials     2617     2635      +18     
Flag Coverage Δ
unittests 57.29% <63.63%> (-0.34%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
cmd/bootstrap/cmd/util.go 60.00% <50.00%> (-0.79%) ⬇️
cmd/bootstrap/cmd/observer_network_key.go 66.66% <66.66%> (ø)
ledger/common/hash/hash.go 18.18% <0.00%> (-21.82%) ⬇️
state/protocol/badger/validity.go 46.25% <0.00%> (-19.83%) ⬇️
consensus/hotstuff/eventloop/event_loop.go 68.29% <0.00%> (-6.10%) ⬇️
engine/common/rpc/convert/convert.go 25.00% <0.00%> (-5.55%) ⬇️
cmd/bootstrap/run/cluster_qc.go 53.84% <0.00%> (-5.25%) ⬇️
...s/hotstuff/votecollector/staking_vote_processor.go 89.47% <0.00%> (-3.51%) ⬇️
fvm/handler/contract.go 75.32% <0.00%> (-2.60%) ⬇️
cmd/bootstrap/transit/cmd/push.go 16.66% <0.00%> (-1.52%) ⬇️
... and 22 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0bd50cf...2170a69. Read the comment docs.

@peterargue peterargue force-pushed the petera/observer-networ-key-boostrap-util branch from 2170a69 to dcfdc60 Compare March 3, 2022 22:23
cmd/bootstrap/README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@synzhu synzhu left a comment

Choose a reason for hiding this comment

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

Overall good, a few small comments

cmd/bootstrap/cmd/observer_network_key.go Outdated Show resolved Hide resolved
cmd/bootstrap/cmd/observer_network_key.go Outdated Show resolved Hide resolved
cmd/bootstrap/README.md Outdated Show resolved Hide resolved
@peterargue peterargue requested a review from m4ksio March 9, 2022 00:56
@peterargue peterargue force-pushed the petera/observer-networ-key-boostrap-util branch from 96f75d4 to 35fdc9d Compare March 18, 2022 21:13
log.Info().Msg("generated network key")

// hex encode and write to file
output := make([]byte, hex.EncodedLen(networkKey.Size()))
Copy link
Contributor

Choose a reason for hiding this comment

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

Is .Size() returning encoded length?
I'm not sure if we need really to do this pre-allocation, can't we just encode whatever we get?
This might lead to some subtle bugs in the future

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fair enough. I updated to use the actual slice len. the preallocation is needed. that's actually what the hex library does for EncodeToString except without the cast

}

// generate unstaked networking private key
networkKey, err := utils.GenerateUnstakedNetworkingKey(flagNetworkSeed)
Copy link
Contributor

Choose a reason for hiding this comment

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

What are "observers" exactly? are these a sub-type of unstaked access nodes?
I'm asking to make sure GenerateUnstakedNetworkingKey is what we need and not GenerateNetworkingKey.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Observer is the new name for unstaked access node.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah okay 😅 It makes sense to use that function then.

@peterargue
Copy link
Contributor Author

bors merge

@bors
Copy link
Contributor

bors bot commented Mar 20, 2022

@bors bors bot merged commit cf277c2 into master Mar 20, 2022
@bors bors bot deleted the petera/observer-networ-key-boostrap-util branch March 20, 2022 21:29
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.

None yet

5 participants