PeerAuthenticationResolver #3749
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dataRetriever/factory/resolverscontainer/baseResolversContainerFactory.go
Show resolved
Hide resolved
| } | ||
|
|
||
| sort.Slice(validatorsPKs, func(i, j int) bool { | ||
| return bytes.Compare(validatorsPKs[i], validatorsPKs[j]) < 0 |
There was a problem hiding this comment.
👍 this sort by the bls pks should make the distribution among shards quite even
| dataSlice := make([][]byte, 0, res.maxNumOfPeerAuthenticationInResponse) | ||
| for _, pk := range pksChunk { | ||
| peerAuth, tmpErr := res.fetchPeerAuthenticationAsByteSlice(pk) | ||
| if tmpErr != nil { |
There was a problem hiding this comment.
I do not think we should add the errors here. It will just clutter the node in normal circumstances when some peer authentication messages are missing. Instead, I would return an error if the data slice is empty, outside this for loop
| sort.Slice(keys, func(i, j int) bool { | ||
| return bytes.Compare(keys[i], keys[j]) < 0 | ||
| }) | ||
|
|
||
| for _, key := range keys { |
There was a problem hiding this comment.
didn't realized how this would have actually end up implemented.
This is way too sub-optimal to be left as it is.
We should use a get operation here and send the full public key in the request until we figure out how to optimally resolve this.
iulianpascalau
previously approved these changes
Feb 7, 2022
dataRetriever/errors.go
Outdated
Comment on lines
230
to
231
| // ErrNotFound signals that a data is missing | ||
| var ErrNotFound = errors.New("data not found") |
There was a problem hiding this comment.
Suggested change
| // ErrNotFound signals that a data is missing | |
| var ErrNotFound = errors.New("data not found") | |
| // ErrPeerAuthsNotFound signals that no Peer Authentications has been provided | |
| var ErrPeerAuthsNotFound = errors.New("no peer authentications") |
| ) | ||
| } | ||
|
|
||
| // ProcessReceivedMessage will be the callback func from the p2p.Messenger and will be called each time a new message was received |
There was a problem hiding this comment.
Suggested change
| // ProcessReceivedMessage will be the callback func from the p2p.Messenger and will be called each time a new message was received | |
| // ProcessReceivedMessage represents the callback func from the p2p.Messenger that is called each time a new message is received |
| }) | ||
| } | ||
|
|
||
| func Test_peerAuthenticationResolver_ProcessReceivedMessage(t *testing.T) { |
There was a problem hiding this comment.
Suggested change
| func Test_peerAuthenticationResolver_ProcessReceivedMessage(t *testing.T) { | |
| func TestPeerAuthenticationResolver_ProcessReceivedMessage(t *testing.T) { |
| }) | ||
| } | ||
|
|
||
| func Test_peerAuthenticationResolver_RequestShouldError(t *testing.T) { |
There was a problem hiding this comment.
Suggested change
| func Test_peerAuthenticationResolver_RequestShouldError(t *testing.T) { | |
| func TestPeerAuthenticationResolver_RequestShouldError(t *testing.T) { |
|
|
||
| } | ||
|
|
||
| func Test_peerAuthenticationResolver_RequestShouldWork(t *testing.T) { |
There was a problem hiding this comment.
Suggested change
| func Test_peerAuthenticationResolver_RequestShouldWork(t *testing.T) { | |
| func TestPeerAuthenticationResolver_RequestShouldWork(t *testing.T) { |
dragos-rebegea
approved these changes
Feb 8, 2022
iulianpascalau
approved these changes
Feb 8, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.