Multikey peer authentication sender integration #4394

sstanculeanu · 2022-08-24T15:03:06Z

Description of the reasoning behind the pull request (what feature was missing / how the problem was manifesting itself / what was the motive behind the refactoring)

peer authentication sender is not compatible with the multikey system

Proposed Changes

integrate keysHolder into newly added component multikeyPeerAuthenticationSender in order to support multikey
updated keysHolder with the needed methods
factory which will create the proper one(multikeyPeerAuthenticationSender or peerAuthenticationSender) will come in a further PR

Testing procedure

system test hbv2

codecov-commenter · 2022-08-28T16:57:31Z

Codecov Report

Merging #4394 (8b91a00) into feat/multisigner (75dc784) will increase coverage by 0.01%.
The diff coverage is 84.72%.

@@                 Coverage Diff                  @@
##           feat/multisigner    #4394      +/-   ##
====================================================
+ Coverage             73.69%   73.71%   +0.01%     
====================================================
  Files                   681      683       +2     
  Lines                 87093    87268     +175     
====================================================
+ Hits                  64187    64333     +146     
- Misses                18051    18072      +21     
- Partials               4855     4863       +8

Impacted Files	Coverage Δ
p2p/crypto/p2pSigner.go	`84.61% <60.00%> (-5.87%)`	⬇️
...artbeat/sender/multikeyPeerAuthenticationSender.go	`78.33% <78.33%> (ø)`
heartbeat/sender/peerAuthenticationSender.go	`93.67% <90.00%> (+0.03%)`	⬆️
heartbeat/sender/commonPeerAuthenticationSender.go	`90.90% <90.90%> (ø)`
keysManagement/keysHolder.go	`100.00% <100.00%> (ø)`
keysManagement/peerInfo.go	`100.00% <100.00%> (ø)`
...t/processor/peerAuthenticationRequestsProcessor.go	`94.40% <0.00%> (-2.40%)`	⬇️
p2p/libp2p/netMessenger.go	`77.55% <0.00%> (+0.25%)`	⬆️
storage/txcache/txListBySenderMap.go	`100.00% <0.00%> (+2.50%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

iulianpascalau · 2022-08-29T08:36:48Z

keysManagement/interface.go


 // P2PIdentityGenerator defines the operations of an entity that can generate P2P identities
 type P2PIdentityGenerator interface {
 	CreateRandomP2PIdentity() ([]byte, core.PeerID, error)
 	IsInterfaceNil() bool
 }
+
+// KeysHolder defines the operations of an entity that holds virtual identities for a node


this has to be moved where it is used: example: heartbeat package

iulianpascalau · 2022-08-29T08:40:55Z

keysManagement/peerInfo.go

@@ -15,6 +16,10 @@ type peerInfo struct {
 	nodeName           string
 	nodeIdentity       string

+	mutPeerAuthenticationData  sync.RWMutex


can remove this sync.RWMutex and use instead the mutChangeableData instance for both isValidator and nextPeerAuthenticationTime

iulianpascalau · 2022-08-29T08:41:47Z

keysManagement/keysHolder.go

@@ -285,6 +286,54 @@ func (holder *virtualPeersHolder) IsPidManagedByCurrentNode(pid core.PeerID) boo
 	return found
 }

+// IsKeyValidator returns true if the key validator status was set to true


missing unit tests for these 4 functions

iulianpascalau · 2022-08-29T08:50:54Z

heartbeat/sender/multikeyPeerAuthenticationSender.go

+
+	data, isHardforkTriggered, err := sender.prepareMessage([]byte(pk), sk)
+	if err != nil {
+		setTimeErr := sender.keysHolder.SetNextPeerAuthenticationTime(pkBytes, currentTimeStamp.Add(sender.timeBetweenSendsWhenError))


in order to simplify the logic, we can make the SetNextPeerAuthenticationTime not output an error, and just ignore the call if the set method did not find the provided public key bytes

iulianpascalau · 2022-08-29T08:51:34Z

heartbeat/sender/multikeyPeerAuthenticationSender.go

+	if isHardforkTriggered {
+		nextTimeStamp := currentTimeStamp.Add(sender.computeRandomDuration(sender.hardforkTimeBetweenSends))
+		setTimeErr := sender.keysHolder.SetNextPeerAuthenticationTime(pkBytes, nextTimeStamp)
+		if setTimeErr != nil {


also here, we can simplify the logic

iulianpascalau · 2022-08-29T08:51:58Z

heartbeat/sender/multikeyPeerAuthenticationSender.go

+			return fmt.Errorf("%w while seting next peer authentication time", setTimeErr)
+		}
+
+		setValidatorErr := sender.keysHolder.SetValidatorState(pkBytes, true)


this function can also be simplified

iulianpascalau · 2022-08-29T08:53:04Z

heartbeat/sender/multikeyPeerAuthenticationSender.go

+	}
+	isValidatorNow, shardID := sender.getIsValidatorStatusAndShardID(pkBytes)
+	isHardforkSource := sender.isHardforkSource(pkBytes)
+	oldIsValidator, err := sender.keysHolder.IsKeyValidator(pkBytes)


this can output only false if the provided key does not belong to this node

iulianpascalau · 2022-08-29T08:57:42Z

heartbeat/sender/multikeyPeerAuthenticationSender.go

+}
+
+func (sender *multikeyPeerAuthenticationSender) sendData(index int, pkBytes []byte, data []byte, isHardforkTriggered bool) {
+	go func() {


as seen in the next heartbeat sender implementation, we can drop the usage of the go async call and the usage of the index value and just start the call with a fixed time.Sleep call.
This will prevent launching more go routines & should simplify the testing procedure with virtually no drawback.

iulianpascalau · 2022-08-29T09:02:38Z

heartbeat/sender/multikeyPeerAuthenticationSender.go

+		Pubkey: pkBytes,
+	}
+
+	hardforkPayload, isTriggered := sender.getHardforkPayload()


this function contains a lot of duplicated code with the original peerAuthenticationSender implementation. Can we extract duplicated code as seen in the heartbeat sender future implementation?

iulianpascalau · 2022-08-29T09:03:49Z

heartbeat/sender/multikeyPeerAuthenticationSender_test.go

+		senderInstance, _ := newMultikeyPeerAuthenticationSender(args)
+		senderInstance.Execute()
+
+		time.Sleep(time.Second * 2) // allow the go routines to finish


if we remove the go routines all these waits should be removed.

bogdan-rosianu · 2022-08-30T08:26:25Z

heartbeat/sender/commonPeerAuthenticationSender.go

+	p2pSkBytes []byte,
+	pidBytes []byte,
+) ([]byte, bool, error) {
+


remove first empty line

bogdan-rosianu · 2022-08-30T08:57:21Z

heartbeat/sender/multikeyPeerAuthenticationSender.go

+
+	p2pSk, pid, err := sender.keysHolder.GetP2PIdentity(pkBytes)
+	if err != nil {
+		log.Error("could not get identity for pk", "pk", hex.EncodeToString(pkBytes))


log the error as well

bogdan-rosianu · 2022-08-30T08:57:37Z

heartbeat/sender/multikeyPeerAuthenticationSender.go

+
+	nextTimeToCheck, err := sender.keysHolder.GetNextPeerAuthenticationTime(pkBytes)
+	if err != nil {
+		log.Error("could not get next peer authentication time for pk", "pk", hex.EncodeToString(pkBytes))


log the error as well

ccorcoveanu · 2022-08-30T09:19:57Z

heartbeat/interface.go

@@ -16,8 +16,10 @@ import (
 // P2PMessenger defines a subset of the p2p.Messenger interface
 type P2PMessenger interface {
 	Broadcast(topic string, buff []byte)
+	BroadcastWithSk(topic string, buff []byte, pid core.PeerID, skBytes []byte)


Can we raname this into BroadcastUsingSk (or BroadcastSigningWithSk)? I know that the naming convention follows libp2p's PublishWithSk but I find it very misleading, I really thought we were sending the sk over the wire. (SendableData having an SK member doesn't help either :)) @iulianpascalau thoughts?)

right, let's rename to BroadcastWithPrivateKey

ccorcoveanu · 2022-08-30T09:20:44Z

heartbeat/interface.go

 	ID() core.PeerID
 	Sign(payload []byte) ([]byte, error)
+	SignWithPrivateKey(skBytes []byte, payload []byte) ([]byte, error)


Maybe also follow the naming convention above (either abbreviation of secret key, or private key)

This reverts commit 037596c.

This reverts commit 60fd880.

integrated multikey peer authentication

2d84dca

sstanculeanu added area:heartbeat complexity:trivial impact:low interested:protocol type:refactoring area:multikey labels Aug 24, 2022

sstanculeanu self-assigned this Aug 24, 2022

sstanculeanu added the status:work-in-progress Work In Progress label Aug 24, 2022

sstanculeanu and others added 3 commits August 24, 2022 18:41

fixed tests

0802272

Merge branch 'feat/multisigner' into integrate_peerauthentication_sender

1e51c1a

fixed build

56e4148

sstanculeanu marked this pull request as ready for review August 24, 2022 15:54

sstanculeanu removed the status:work-in-progress Work In Progress label Aug 24, 2022

sstanculeanu requested a review from iulianpascalau August 24, 2022 15:57

Merge branch 'feat/multisigner' into integrate_peerauthentication_sender

1541f0b

iulianpascalau reviewed Aug 29, 2022

View reviewed changes

fixes after review

8c36f42

bogdan-rosianu self-requested a review August 30, 2022 08:19

bogdan-rosianu reviewed Aug 30, 2022

View reviewed changes

fix after review

8b91a00

ccorcoveanu reviewed Aug 30, 2022

View reviewed changes

renamed xxxWithSk to xxxWithPrivateKey

7823d6d

iulianpascalau previously approved these changes Aug 30, 2022

View reviewed changes

renamed xxxWithPrivateKey/xxxWithSk to xxxUsingPrivateKey

704702e

sstanculeanu dismissed iulianpascalau’s stale review via 704702e August 30, 2022 16:54

ccorcoveanu previously approved these changes Aug 30, 2022

View reviewed changes

added multikeyHeartbeatSender + extracted common functionality

60fd880

sstanculeanu dismissed ccorcoveanu’s stale review via 60fd880 August 30, 2022 17:02

sstanculeanu added 3 commits August 30, 2022 20:03

fix after rebase

037596c

Revert "fix after rebase"

ef6f76a

This reverts commit 037596c.

Revert "added multikeyHeartbeatSender + extracted common functionality"

495176b

This reverts commit 60fd880.

ccorcoveanu approved these changes Aug 30, 2022

View reviewed changes

iulianpascalau approved these changes Aug 31, 2022

View reviewed changes

bogdan-rosianu approved these changes Aug 31, 2022

View reviewed changes

sstanculeanu merged commit a0130de into feat/multisigner Aug 31, 2022

sstanculeanu deleted the integrate_peerauthentication_sender branch August 31, 2022 13:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multikey peer authentication sender integration #4394

Multikey peer authentication sender integration #4394

sstanculeanu commented Aug 24, 2022 •

edited

codecov-commenter commented Aug 28, 2022 •

edited

iulianpascalau Aug 29, 2022

iulianpascalau Aug 29, 2022

iulianpascalau Aug 29, 2022

iulianpascalau Aug 29, 2022

iulianpascalau Aug 29, 2022

iulianpascalau Aug 29, 2022

iulianpascalau Aug 29, 2022

iulianpascalau Aug 29, 2022

iulianpascalau Aug 29, 2022

iulianpascalau Aug 29, 2022

bogdan-rosianu Aug 30, 2022

bogdan-rosianu Aug 30, 2022

bogdan-rosianu Aug 30, 2022

ccorcoveanu Aug 30, 2022

iulianpascalau Aug 30, 2022

sstanculeanu Aug 30, 2022

ccorcoveanu Aug 30, 2022

Multikey peer authentication sender integration #4394

Multikey peer authentication sender integration #4394

Conversation

sstanculeanu commented Aug 24, 2022 • edited

Description of the reasoning behind the pull request (what feature was missing / how the problem was manifesting itself / what was the motive behind the refactoring)

Proposed Changes

Testing procedure

codecov-commenter commented Aug 28, 2022 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sstanculeanu commented Aug 24, 2022 •

edited

codecov-commenter commented Aug 28, 2022 •

edited