Equivalent proofs tracker #6456

ssd04 · 2024-09-11T18:40:11Z

Reasoning behind the pull request

Added proofs pool holder component
removed equivalent proofs for spos worker

Testing procedure

TBD

Pre-requisites

Based on the Contributing Guidelines the PR author and the reviewers must check the following requirements are met:

was the PR targeted to the correct branch?
if this is a larger feature that probably needs more than one PR, is there a feat branch created?
if this is a feat branch merging, do all satellite projects have a proper tag inside go.mod?

sstanculeanu · 2024-09-13T08:15:42Z

consensus/broadcast/delayedBroadcast.go

 	dbb.mutHeadersCache.Unlock()

-	aggSig, bitmap := headerHandler.GetPreviousAggregatedSignatureAndBitmap()
+	proof := headerHandler.GetPreviousProof()


note for further PRs(probably for myself): this method should be either called from interceptor, either the logic should be moved on the header interceptor, as the header will be broadcast on a new topic

added TODO for this

sstanculeanu · 2024-09-13T08:16:35Z

consensus/interface.go

+type EquivalentProofsPool interface {
+	AddNotarizedProof(headerProof data.HeaderProofHandler) error
+	GetNotarizedProof(shardID uint32, headerHash []byte) (data.HeaderProofHandler, error)
+	GetAllNotarizedProofs(shardID uint32) (map[string]data.HeaderProofHandler, error)


looks like this is not used.. will it be needed in the future?

it was used at some point, not used anymore, it's deleted now

sstanculeanu · 2024-09-13T08:18:06Z

consensus/spos/bls/subroundBlock.go

 	}

-	prevBlockProof := sr.Blockchain().GetCurrentHeaderProof()
+	prevBlockProof, err := sr.worker.GetEquivalentProof(sr.GetData())


the worker will have all the equivalent proofs logic removed(maybe in a different PR). The proof for the prev block should be requested from the newly added equivalentProofsTracker

kept initially the worker functions, deleted them now

sstanculeanu · 2024-09-13T08:26:49Z

consensus/spos/bls/subroundBlock.go

-func isProofEmpty(proof data.HeaderProof) bool {
-	return len(proof.AggregatedSignature) == 0 || len(proof.PubKeysBitmap) == 0
+func isProofEmpty(proof data.HeaderProofHandler) bool {
+	return len(proof.GetAggregatedSignature()) == 0 || len(proof.GetPubKeysBitmap()) == 0


I would also add here a check for len of header hash

sstanculeanu · 2024-09-13T09:26:28Z

consensus/spos/bls/subroundBlock.go


-	proof := sr.Blockchain().GetCurrentHeaderProof()
-	if !isProofEmpty(proof) {
+	proof, err := sr.worker.GetEquivalentProof(sr.GetData())


I think this can be removed completely L564-576.. reason: it should be saved into the newly added equivalentProofsTracker and this will be done from the interceptor

will be done in a further PR, maybe just save it into equivalentProofsTracker for this one

sstanculeanu · 2024-09-13T09:38:15Z

dataRetriever/dataPool/proofsCache/proofsPool.go

+// NewProofsPool creates a new proofs pool component
+func NewProofsPool() *proofsPool {
+	return &proofsPool{
+		cache: make(map[uint32]*proofsCache),


why do we need to save them by shard?

a node in shard X should have proofs only for X and Meta

i've added the option to save by shardID in a more general way; indeed, currently i use the proofs only for current shard, i haven't covered the cross shard or meta yet; will analyse this for a next PR

sstanculeanu · 2024-09-13T09:40:42Z

process/block/metablock.go

 		if mp.enableEpochsHandler.IsFlagEnabledInEpoch(common.EquivalentMessagesFlag, shardHdr.GetEpoch()) {
-			_, shardData.PubKeysBitmap = shardHdr.GetPreviousAggregatedSignatureAndBitmap()
+			proof := shardHdr.GetPreviousProof()
+			shardData.PubKeysBitmap = proof.GetPubKeysBitmap()


check proof against nil before this?

sstanculeanu · 2024-09-13T09:41:21Z

process/headerCheck/headerSignatureVerify.go

 func (hsv *HeaderSigVerifier) getPrevHeaderInfo(currentHeader data.HeaderHandler) (data.HeaderHandler, []byte, []byte, []byte, error) {
-	sig, bitmap := currentHeader.GetPreviousAggregatedSignatureAndBitmap()
+	previousProof := currentHeader.GetPreviousProof()
+	sig, bitmap := previousProof.GetAggregatedSignature(), previousProof.GetPubKeysBitmap()


check previousProof against nil before this

sstanculeanu · 2024-09-13T09:42:01Z

process/peer/process.go

 	if vs.enableEpochsHandler.IsFlagEnabledInEpoch(common.EquivalentMessagesFlag, previousHeader.GetEpoch()) {
-		_, bitmap = previousHeader.GetPreviousAggregatedSignatureAndBitmap()
+		proof := previousHeader.GetPreviousProof()
+		bitmap = proof.GetPubKeysBitmap()


check proof against nil

sstanculeanu · 2024-09-13T09:44:08Z

testscommon/dataRetriever/proofsPoolStub.go

+)
+
+// ProofsPoolStub -
+type ProofsPoolStub struct {


rename to mock.. file as well.. can use scripts/generators/mockGenerator.sh

consensus/spos/worker.go

dataRetriever/dataPool/proofsCache/proofsPool.go

Co-authored-by: Sorin Stanculeanu <[email protected]>

AdoAdoAdo · 2024-09-18T14:14:08Z

consensus/broadcast/delayedBroadcast.go

 	dbb.mutHeadersCache.Unlock()

-	aggSig, bitmap := headerHandler.GetPreviousAggregatedSignatureAndBitmap()
+	// TODO: should be handled from interceptor


This method was previously used by validators in consensus to find out if the leader propagates the block with final data on, and if not to do the propagation themselves in the predefined order.

This won't be needed anymore as the block in its final form is propagated at the beginning of the round, and if not propagated, then there is nothing to do.

So after equivalent proofs activation, this method will not be needed.

AdoAdoAdo · 2024-09-19T07:53:49Z

consensus/spos/bls/subroundBlock.go


-func isProofEmpty(proof data.HeaderProof) bool {
-	return len(proof.AggregatedSignature) == 0 || len(proof.PubKeysBitmap) == 0
+func isProofEmpty(proof data.HeaderProofHandler) bool {


this should only be an extra validation right?
I mean we only add proofs to the proof tracker if the proof is valid, so if it is already present, then it should have already been validated.

AdoAdoAdo · 2024-09-19T08:48:27Z

consensus/spos/bls/subroundBlock.go

 	}

-	proof := sr.Blockchain().GetCurrentHeaderProof()
+	proof, err := sr.EquivalentProofsPool().GetProof(sr.ShardCoordinator().SelfId(), sr.GetData())


I don't think this method - saveProofForPreviousHeaderIfNeeded is required here, as it is already saved on the regular interceptor.

indeed, should not be needed anymore

AdoAdoAdo · 2024-09-19T09:07:30Z

consensus/spos/bls/subroundEndRound.go


 	// TODO: check if this is the best approach. Perhaps we don't want to relay only on the first received message
-	if sr.worker.HasEquivalentMessage(sr.GetData()) {
+	if sr.EquivalentProofsPool().HasProof(sr.ShardCoordinator().SelfId(), sr.GetData()) {


I think we need to adapt the usage of shouldSendFinalInfo, as it should not stop the leader to commit the block if the proof is already received from someone else.

Leader should not send the info, true, but should still continue to commit the block. (otherwise he will try to sync it afterwards)

AdoAdoAdo · 2024-09-19T09:22:54Z

consensus/spos/bls/subroundEndRound.go

 	if sr.EnableEpochsHandler().IsFlagEnabledInEpoch(common.EquivalentMessagesFlag, sr.Header.GetEpoch()) {
-		sr.worker.SetValidEquivalentProof(sr.GetData(), proof)
-		sr.Blockchain().SetCurrentHeaderProof(proof)
+		err = sr.EquivalentProofsPool().AddProof(proof)


it seems after equivalent proofs activation doEndRoundJobByLeader will be called by all nodes in consensus group, so if shouldSendFinalInfo fails (they received from someone the proof), all consensus group participants that did not send the proof will no longer finalize the block during consensus, but will try to sync it afterwards.

AdoAdoAdo · 2024-09-19T09:54:18Z

consensus/spos/bls/subroundEndRound.go

-			AggregatedSignature: cnsDta.AggregateSignature,
+		proof := &block.HeaderProof{
 			PubKeysBitmap:       cnsDta.PubKeysBitmap,
+			AggregatedSignature: cnsDta.AggregateSignature,


The saving of the proof here maybe not always required, only if it was created by the participant, otherwise it should already be available in the proofs tracker.

AdoAdoAdo · 2024-09-19T12:01:57Z

process/block/metablock.go

 		shardData.PubKeysBitmap = shardHdr.GetPubKeysBitmap()
 		if mp.enableEpochsHandler.IsFlagEnabledInEpoch(common.EquivalentMessagesFlag, shardHdr.GetEpoch()) {
-			_, shardData.PubKeysBitmap = shardHdr.GetPreviousAggregatedSignatureAndBitmap()
+			proof := shardHdr.GetPreviousProof()


The data for a notarized shard header should be related to that shard header not the previous.
here the proof for the header should be used not the one included in the header, which is for the previous one.

Also

AdoAdoAdo · 2024-09-19T12:04:14Z

process/headerCheck/headerSignatureVerify.go

+		hasProof = len(previousAggregatedSignature) > 0 && len(previousBitmap) > 0
+
+		if len(previousBitmap) > 0 {
+			hasLeaderSignature = previousBitmap[0]&1 != 0


this is no longer valid
with fixed ordering, the leader can have any index in the bitmap, but at the same time it is no longer needed to have its signature in the proof.

AdoAdoAdo

The changes can be taken on the next PR

ssd04 added 4 commits September 9, 2024 21:49

added proofs tracker

11f7ffb

fix consensus unit tests

5d86307

moves proofs accumulator to data pool

bf8a938

adapted mocks and unit tests fixing

f25d57f

ssd04 self-assigned this Sep 11, 2024

ssd04 added 8 commits September 12, 2024 09:40

Merge branch 'feat/equivalent-messages' into equivalent-proofs-tracker

541d170

fix consensus unit tests

a1ebf33

update equivalent messages debugger

c52089f

add todo comment

ff82998

fix unit tests + added missing comments

a7e735b

fix consensus integration tests

fe022ea

fix consensus integration tests - fix linter

f9ea993

added todo for proof check

66d40f1

ssd04 marked this pull request as ready for review September 12, 2024 13:31

Merge branch 'feat/equivalent-messages' into equivalent-proofs-tracker

7a0e55b

ssd04 added type:feature New feature or request area:consensus interested:protocol complexity:medium labels Sep 12, 2024

update core version

631cbc8

sstanculeanu self-requested a review September 13, 2024 07:44

sstanculeanu reviewed Sep 13, 2024

View reviewed changes

ssd04 added 4 commits September 13, 2024 16:15

fixes after review: removed equivalent messages handling from worker

41cf564

fix factory and integration tests

2a6e05b

fix linter issues

340b17f

Merge branch 'feat/equivalent-messages' into equivalent-proofs-tracker

7095589

sstanculeanu reviewed Sep 18, 2024

View reviewed changes

consensus/spos/worker.go Outdated Show resolved Hide resolved

dataRetriever/dataPool/proofsCache/proofsPool.go Outdated Show resolved Hide resolved

ssd04 and others added 3 commits September 18, 2024 11:01

Update consensus/spos/worker.go

e292002

Co-authored-by: Sorin Stanculeanu <[email protected]>

Update dataRetriever/dataPool/proofsCache/proofsPool.go

4354927

Co-authored-by: Sorin Stanculeanu <[email protected]>

fixes after review

61cd94f

sstanculeanu approved these changes Sep 18, 2024

View reviewed changes

AdoAdoAdo reviewed Sep 19, 2024

View reviewed changes

AdoAdoAdo approved these changes Sep 19, 2024

View reviewed changes

AdoAdoAdo merged commit dd4e4d4 into feat/equivalent-messages Sep 19, 2024

AdoAdoAdo deleted the equivalent-proofs-tracker branch September 19, 2024 12:15

Equivalent proofs tracker #6456

Equivalent proofs tracker #6456

Uh oh!

Conversation

ssd04 commented Sep 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reasoning behind the pull request

Testing procedure

Pre-requisites

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ssd04 Sep 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AdoAdoAdo left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

ssd04 commented Sep 11, 2024 •

edited

Loading

ssd04 Sep 13, 2024 •

edited

Loading