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

p2p: add human friendly names to peers #624

Merged
merged 5 commits into from
May 28, 2022

Conversation

xenowits
Copy link
Contributor

  1. Add human friendly names to peers
  2. Replace logs with the peer names

category: feature
ticket: #585

@xenowits xenowits marked this pull request as draft May 27, 2022 08:32
@xenowits xenowits linked an issue May 27, 2022 that may be closed by this pull request
@codecov
Copy link

codecov bot commented May 27, 2022

Codecov Report

Merging #624 (9f837cc) into main (6869873) will decrease coverage by 0.55%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##             main     #624      +/-   ##
==========================================
- Coverage   56.89%   56.33%   -0.56%     
==========================================
  Files          96       97       +1     
  Lines        9126     9145      +19     
==========================================
- Hits         5192     5152      -40     
- Misses       3195     3249      +54     
- Partials      739      744       +5     
Impacted Files Coverage Δ
core/consensus/component.go 63.88% <0.00%> (+0.44%) ⬆️
p2p/peer.go 50.00% <ø> (ø)
cluster/operator.go 60.97% <40.00%> (-6.77%) ⬇️
cluster/name.go 100.00% <100.00%> (ø)
core/qbft/qbft.go 72.11% <0.00%> (-9.16%) ⬇️
app/app.go 59.89% <0.00%> (-2.32%) ⬇️
core/dutydb/memory.go 77.56% <0.00%> (-1.93%) ⬇️

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 6869873...9f837cc. Read the comment docs.

// randomName returns a deterministic name for an ecdsa public key. The name consists of a noun
// and an adjective separated by a hyphen. The noun is calculated using PublicKey's X coordinate
// while the adjective is calculated using PublicKey's Y coordinate.
func randomName(pk ecdsa.PublicKey) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

lets move this function to its own file maybe name.go? and split out the lists to global variables defined one word per line.

Comment on lines 112 to 122
res := big.NewInt(0)

// calculate the index of the adjective using X % ADJ_LEN
adjLen := big.NewInt(int64(len(adjectives)))
res.Rem(pk.X, adjLen)
adjIdx := res.Uint64()

// similarly, calculate the index of the noun using Y % NOUN_LEN
nounLen := big.NewInt(int64(len(nouns)))
nounIdx := res.Rem(pk.Y, nounLen).Uint64()

Copy link
Contributor

@corverroos corverroos May 27, 2022

Choose a reason for hiding this comment

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

could be a bit more consistent, adjIdx calculated the same as nounIdx. Also please be very careful with big.Int, it has a bad mutable API. Prefer to use it in a immutable way, by only using one big.Int once and creating new ones with each calculation. You mutate res multiple times.


        // calculate the index of the adjective using X % ADJ_LEN
	adjLen := big.NewInt(int64(len(adjectives)))
	adjIdx := new(big.Int).Rem(pk.X, adjLen).Uint64()

	// similarly, calculate the index of the noun using Y % NOUN_LEN
	nounLen := big.NewInt(int64(len(nouns)))
	nounIdx := new(big.Int).Rem(pk.Y, nounLen).Uint64()


second, err := op.getName()
assert.NoError(t, err)
assert.True(t, strings.Contains(second, "-"))
Copy link
Contributor

Choose a reason for hiding this comment

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

add an assertion for the actual name require.Equal(t, first, "foo-bar")

Also please prefer require to assert

@@ -41,6 +41,9 @@ type Peer struct {
// Index is the order of this node in the cluster.
// This is only applicable to charon nodes, not relays.
Index int

// Name represents a human friendly name for the peer.
Name string
Copy link
Contributor

Choose a reason for hiding this comment

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

this isn't set... or used...

@xenowits xenowits added the merge when ready Indicates bulldozer bot may merge when all checks pass label May 28, 2022
@xenowits xenowits marked this pull request as ready for review May 28, 2022 09:53
@obol-bulldozer obol-bulldozer bot merged commit 112ad29 into main May 28, 2022
@obol-bulldozer obol-bulldozer bot deleted the xenowits/sapiens-friendly-names branch May 28, 2022 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge when ready Indicates bulldozer bot may merge when all checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants