Integrated multikey in consensus #4463

iulianpascalau · 2022-09-14T12:52:47Z

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)

integrated multikey components in consensus

Proposed Changes

integration

Testing procedure

N/A yet

# Conflicts: # integrationTests/testProcessorNode.go

codecov-commenter · 2022-09-14T13:04:08Z

Codecov Report

Base: 73.94% // Head: 73.89% // Decreases project coverage by -0.04% ⚠️

Coverage data is based on head (4a971d5) compared to base (d61698d).
Patch coverage: 65.04% of modified lines in pull request are covered.

Additional details and impacted files

@@                 Coverage Diff                  @@
##           feat/multisigner    #4463      +/-   ##
====================================================
- Coverage             73.94%   73.89%   -0.05%     
====================================================
  Files                   698      698              
  Lines                 88808    89033     +225     
====================================================
+ Hits                  65665    65789     +124     
- Misses                18207    18285      +78     
- Partials               4936     4959      +23

Impacted Files	Coverage Δ
factory/consensusComponents.go	`0.00% <0.00%> (ø)`
consensus/spos/bls/subroundStartRound.go	`69.94% <35.71%> (-2.83%)`	⬇️
consensus/spos/bls/subroundSignature.go	`64.62% <43.33%> (-13.34%)`	⬇️
consensus/spos/bls/subroundBlock.go	`75.85% <46.87%> (-2.76%)`	⬇️
consensus/spos/bls/subroundEndRound.go	`63.89% <58.92%> (+0.28%)`	⬆️
consensus/broadcast/metaChainMessenger.go	`59.55% <66.66%> (-0.70%)`	⬇️
consensus/spos/worker.go	`81.74% <71.42%> (-0.25%)`	⬇️
consensus/broadcast/shardChainMessenger.go	`56.25% <76.47%> (-0.42%)`	⬇️
consensus/broadcast/commonMessenger.go	`81.98% <87.09%> (+0.16%)`	⬆️
consensus/spos/roundConsensus.go	`95.09% <90.90%> (-1.50%)`	⬇️
... and 9 more

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

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

sstanculeanu · 2022-09-14T14:48:13Z

consensus/spos/bls/subroundSignature.go

+	if sr.IsNodeInConsensusGroup(sr.SelfPubKey()) {
+		selfJobDone = sr.IsSelfJobDone(sr.Current())
+	}
+	allInOneJobDone := sr.IsMultiKeyJobDone(sr.Current())


Suggested change

allInOneJobDone := sr.IsMultiKeyJobDone(sr.Current())

multiKeyJobDone := sr.IsMultiKeyJobDone(sr.Current())

?

sstanculeanu · 2022-09-14T15:00:41Z

consensus/spos/bls/subroundSignature.go

@@ -302,3 +310,71 @@ func (sr *subroundSignature) remainingTime() time.Duration {

 	return remainigTime
 }
+
+func (sr *subroundSignature) doSignatureJobForManagedKeys() bool {
+	isAllInOneLeader := sr.IsMultiKeyLeaderInCurrentRound()


Suggested change

isAllInOneLeader := sr.IsMultiKeyLeaderInCurrentRound()

isMultiKeyLeader := sr.IsMultiKeyLeaderInCurrentRound()

sstanculeanu · 2022-09-14T15:20:10Z

consensus/spos/consensusState.go

+	if cns.IsNodeInConsensusGroup(cns.SelfPubKey()) {
+		selfJobDone = cns.IsSelfJobDone(currentSubroundId)
+	}
+	allInOneJobDone := cns.IsMultiKeyJobDone(currentSubroundId)


Suggested change

allInOneJobDone := cns.IsMultiKeyJobDone(currentSubroundId)

multiKeyJobDone := cns.IsMultiKeyJobDone(currentSubroundId)

What about this approach?

multiKeyJobDone := true if cns.IsMultiKeyInConsensusGroup() { multiKeyJobDone := cns.IsMultiKeyJobDone(currentSubroundId) }

refactored for consistency reasons

sstanculeanu · 2022-09-14T15:32:44Z

consensus/spos/roundConsensus.go

+}
+
+// IncrementRoundsWithoutReceivedMessages increments the number of rounds without received messages on a provided public key
+// TODO(jls) add tests


removed TODO as tests already exist

sstanculeanu · 2022-09-14T15:38:22Z

heartbeat/sender/multikeyHeartbeatSender.go

@@ -164,7 +164,7 @@ func (sender *multikeyHeartbeatSender) sendMessageForKey(pkBytes []byte) error {
 		versionNumber,
 		name,
 		identity,
-		uint32(core.RegularPeer), // force all in one peers to be of type regular peers
+		uint32(core.RegularPeer), // force multi key handled peers to be of type regular peers


SebastianMarian · 2022-09-14T21:54:25Z

consensus/broadcast/commonMessenger.go

+	if err != nil {
+		log.Error("setup error in commonMessenger.broadcast - public key is managed but does not contain p2p sign info",
+			"pk", pkBytes, "error", err)
+		return


Thinking if it is better to call cm.messenger.Broadcast(topic, data) before return. Exactly as the approach used in getPrivateKey method.

thought about this. I think part of the messages will be discarded because it will broadcast data as an observer on topics on which data needs to be sent only by validators.

here you need to broadcast on behalf of someone else, but you cannot do it since you don't have the required p2p data, so I think it is fine to simply return. Otherwise the message would anyway fail on verification and be dropped.

SebastianMarian · 2022-09-14T22:35:39Z

consensus/spos/bls/subroundSignature.go

+	if sr.IsNodeInConsensusGroup(sr.SelfPubKey()) {
+		selfJobDone = sr.IsSelfJobDone(sr.Current())
+	}
+	allInOneJobDone := sr.IsMultiKeyJobDone(sr.Current())


What about this approach?

multiKeyJobDone := true if sr.IsMultiKeyInConsensusGroup() { multiKeyJobDone := sr.IsMultiKeyJobDone(sr.Current()) }

SebastianMarian · 2022-09-14T22:49:07Z

consensus/spos/consensusState.go

+	if cns.IsNodeInConsensusGroup(cns.SelfPubKey()) {
+		selfJobDone = cns.IsSelfJobDone(currentSubroundId)
+	}
+	allInOneJobDone := cns.IsMultiKeyJobDone(currentSubroundId)


What about this approach?

multiKeyJobDone := true if cns.IsMultiKeyInConsensusGroup() { multiKeyJobDone := cns.IsMultiKeyJobDone(currentSubroundId) }

AdoAdoAdo · 2022-09-15T08:41:45Z

consensus/broadcast/commonMessenger.go


 	return nil
 }

+func (cm *commonMessenger) getPrivateKey(message *consensus.Message) crypto.PrivateKey {
+	publicKey := message.PubKey
+	if !cm.keysHolder.IsKeyManagedByCurrentNode(publicKey) {


can we extract the key management logic out of the consensus?

ok, will try

AdoAdoAdo · 2022-09-15T09:13:06Z

consensus/broadcast/commonMessenger.go

+	if err != nil {
+		log.Error("setup error in commonMessenger.broadcast - public key is managed but does not contain p2p sign info",
+			"pk", pkBytes, "error", err)
+		return


here you need to broadcast on behalf of someone else, but you cannot do it since you don't have the required p2p data, so I think it is fine to simply return. Otherwise the message would anyway fail on verification and be dropped.

AdoAdoAdo · 2022-09-15T09:26:04Z

integrationTests/testInitializer.go


 // SyncDelay is used so that nodes have enough time to sync
-var SyncDelay = time.Second / 5
+var SyncDelay = time.Second * 2


this is a large increase
I think we need to investigate where the increase comes from.

AdoAdoAdo · 2022-09-15T09:37:48Z

consensus/spos/subround.go

@@ -204,6 +206,30 @@ func (sr *Subround) ConsensusChannel() chan bool {
 	return sr.consensusStateChangedChannel
 }

+// GetAssociatedPid returns the associated PeerID to the provided public key bytes
+func (sr *Subround) GetAssociatedPid(pkBytes []byte) core.PeerID {


This logic I think needs to be moved to a separate component, so consensus does not need to manage itself multiple peers, but delegate this to that component.

ok, will try

AdoAdoAdo · 2022-09-15T10:28:37Z

consensus/spos/consensusState.go

+// IsMultiKeyLeaderInCurrentRound method checks if one of the nodes which are controlled by this instance
+// is leader in the current round
+func (cns *ConsensusState) IsMultiKeyLeaderInCurrentRound() bool {
+	managedKeys := cns.keysHolder.GetManagedKeysByCurrentNode()


this seems to be a bit heavy on mem alocations especially for scenarios with multiple keys.

will try to refactor in order to make the code more efficient

AdoAdoAdo · 2022-09-15T10:29:44Z

consensus/spos/consensusState.go

+func (cns *ConsensusState) IsMultiKeyJobDone(currentSubroundId int) bool {
+	managedKeys := cns.keysHolder.GetManagedKeysByCurrentNode()
+	for pk := range managedKeys {
+		if !cns.IsNodeInConsensusGroup(pk) {


maybe move the filtering of keys in consensus group managed by current node in a separate component, then we can optimize here allocations and number of iterations.

ok, will try

AdoAdoAdo · 2022-09-15T10:31:48Z

consensus/spos/bls/subroundStartRound.go

@@ -168,11 +171,28 @@ func (sr *subroundStartRound) initCurrentRound() bool {

 	pubKeys := sr.ConsensusGroup()

+	numMultiKeysInConsensusGroup := 0


can you create a separate method for the added functionality here? L174-L188
then you can also add some unit tests

yes, will add

AdoAdoAdo · 2022-09-15T10:52:33Z

consensus/spos/bls/subroundSignature.go

 			return false
 		}

-		log.Debug("step 2: signature has been sent")
-	}
+		isSelfLeader := sr.IsSelfLeaderInCurrentRound()


can we do this on line 71 and then use it in the conditional and here.

AdoAdoAdo · 2022-09-15T11:15:10Z

consensus/spos/bls/subroundSignature.go

+		}
+
+		if !isAllInOneLeader {
+			cnsMsg := consensus.NewConsensusMessage(


this part looks very similar to L73-L118
can we extract some common functionality and use in both places?

ok, will try

refactored and eliminated duplicated code

sstanculeanu · 2022-09-16T16:05:51Z

consensus/spos/consensusState_test.go

+
+	keysHandler := &testscommon.KeysHandlerStub{}
+	cns := internalInitConsensusStateWithKeysHandler(keysHandler)
+	// managedKey := []byte("managed key")


commented code

sstanculeanu · 2022-09-16T16:06:25Z

consensus/spos/bls/subroundEndRound_test.go

+	t.Parallel()
+
+	container := mock.InitConsensusCore()
+	keysHolder := &testscommon.KeysHandlerStub{}


Suggested change

keysHolder := &testscommon.KeysHandlerStub{}

keysHandler := &testscommon.KeysHandlerStub{}

SebastianMarian · 2022-09-16T23:26:48Z

consensus/broadcast/commonMessenger_test.go

-			&testscommon.KeysHolderStub{},
+			&testscommon.KeysHandlerStub{
+				IsOriginalPublicKeyOfTheNodeCalled: func(pkBytes []byte) bool {
+					return bytes.Equal(pkBytesProvided, pkBytes)


You can use nodePkBytes you already defined and used in tests above and remove L268

SebastianMarian · 2022-09-16T23:30:38Z

consensus/spos/bls/blsWorker_test.go

 }

-func initConsensusStateWithKeysHolder(keysHolder consensus.KeysHolder) *spos.ConsensusState {
+func initConsensusStateWithKeysHandler(keysHolder consensus.KeysHandler) *spos.ConsensusState {


keysHandler

SebastianMarian · 2022-09-16T23:39:07Z

consensus/spos/bls/subroundSignature.go

 	}

 	return sr.doSignatureJobForManagedKeys()
 }

+func (sr *subroundSignature) createAndSendSignatureMessage(signatureShare []byte, pkBytes []byte) bool {


SebastianMarian · 2022-09-16T23:39:16Z

consensus/spos/bls/subroundSignature.go

+	return true
+}
+
+func (sr *subroundSignature) completeSignatureSubRound(pk string, shouldWaitForAllSigsAsync bool) bool {


SebastianMarian · 2022-09-16T23:44:50Z

consensus/spos/bls/subroundSignature.go

-			err = sr.BroadcastMessenger().BroadcastConsensusMessage(cnsMsg)
-			if err != nil {
-				log.Debug("doSignatureJob.BroadcastConsensusMessage", "error", err.Error())
+			numMultiKeysSignaturesSent++


This should be moved to L364, when it is sure that the signature has been sent after execution with success of method createAndSendSignatureMessage

done, thanks

SebastianMarian · 2022-09-17T00:08:36Z

keysManagement/keysHandler.go

+	return false
+}
+
+// UpdatePublicKeyLiveness update the provided public key liveness if the provided pid is not managed by the current node


is not managed? or is managed?

not managed. As you want to update the liveness of the pk when the master node did not participated in the consensus even if it should have.

ok, now I see the sense the "not managed" is used ihere

SebastianMarian · 2022-09-17T00:17:57Z

keysManagement/keysHandler.go

+
+// IsOriginalPublicKeyOfTheNode returns true if the provided public key bytes are the original ones used by the node
+func (handler *keysHandler) IsOriginalPublicKeyOfTheNode(pkBytes []byte) bool {
+	return false


return true ? to work correctly in single-sign mode until the implementation will be done?

SebastianMarian · 2022-09-17T00:18:51Z

testscommon/keysHandlerStub.go

+		return stub.IsOriginalPublicKeyOfTheNodeCalled(pkBytes)
+	}
+
+	return false


return true ?

done, simplified one mock creation

- integrated multikey in consensus

1790723

iulianpascalau self-assigned this Sep 14, 2022

iulianpascalau added type:feature New feature or request area:consensus complexity:high impact:high interested:protocol interested:validator size:large type:refactoring area:multikey labels Sep 14, 2022

Merge branch 'feat/multisigner' into consensus-multikey-integration

4a971d5

# Conflicts: # integrationTests/testProcessorNode.go

sstanculeanu self-requested a review September 14, 2022 13:05

SebastianMarian self-requested a review September 14, 2022 15:01

sstanculeanu reviewed Sep 14, 2022

View reviewed changes

SebastianMarian reviewed Sep 14, 2022

View reviewed changes

AdoAdoAdo self-requested a review September 15, 2022 07:35

- fix after review round 1

8ef064c

AdoAdoAdo reviewed Sep 15, 2022

View reviewed changes

SebastianMarian previously approved these changes Sep 16, 2022

View reviewed changes

- fix after review: major code refactoring

0538727

iulianpascalau dismissed SebastianMarian’s stale review via 0538727 September 16, 2022 15:02

sstanculeanu reviewed Sep 16, 2022

View reviewed changes

SebastianMarian reviewed Sep 17, 2022

View reviewed changes

- fix after review

ba20a63

sstanculeanu approved these changes Sep 19, 2022

View reviewed changes

SebastianMarian approved these changes Sep 19, 2022

View reviewed changes

AdoAdoAdo approved these changes Sep 20, 2022

View reviewed changes

iulianpascalau merged commit 7fe1d70 into feat/multisigner Sep 20, 2022

iulianpascalau deleted the consensus-multikey-integration branch September 20, 2022 09:30

	allInOneJobDone := sr.IsMultiKeyJobDone(sr.Current())
	multiKeyJobDone := sr.IsMultiKeyJobDone(sr.Current())

	isAllInOneLeader := sr.IsMultiKeyLeaderInCurrentRound()
	isMultiKeyLeader := sr.IsMultiKeyLeaderInCurrentRound()

	allInOneJobDone := cns.IsMultiKeyJobDone(currentSubroundId)
	multiKeyJobDone := cns.IsMultiKeyJobDone(currentSubroundId)

		@@ -168,11 +171,28 @@ func (sr *subroundStartRound) initCurrentRound() bool {

		pubKeys := sr.ConsensusGroup()

		numMultiKeysInConsensusGroup := 0

	keysHolder := &testscommon.KeysHandlerStub{}
	keysHandler := &testscommon.KeysHandlerStub{}

Integrated multikey in consensus #4463

Integrated multikey in consensus #4463

Conversation

iulianpascalau commented Sep 14, 2022

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 Sep 14, 2022

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

iulianpascalau Sep 15, 2022 • edited

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

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

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

Choose a reason for hiding this comment

iulianpascalau Sep 15, 2022 •

edited