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

stabilization-equivalent-proofs #6686

Open
wants to merge 41 commits into
base: feat/equivalent-messages
Choose a base branch
from

Conversation

AdoAdoAdo
Copy link
Contributor

@AdoAdoAdo AdoAdoAdo commented Dec 19, 2024

Reasoning behind the pull request

  • Bugfixes and changes while stabilizing the new consensus (v2) based on equivalent proofs

Proposed changes

  • change from epoch start trigger to generic epoch notifier to trigger the transition to new consensus - simpler and covers more cases
  • in case of generic epoch notifier the validators need to also need to trigger the transition correctly
  • early exit from consensus v1 operation if a transition to v2 happens.
  • adapt fork detector probable highest nonce computation
  • fix intercepted equivalent proof checks
  • adapt and fix tests

Testing procedure

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?

@AdoAdoAdo AdoAdoAdo changed the title use EpochNotifier instead of EpochStartNotifier to transition to the … stabilization-equivalent-proofs Dec 20, 2024
@AdoAdoAdo AdoAdoAdo self-assigned this Dec 20, 2024
@@ -57,6 +56,13 @@ type SubroundsHandler struct {
currentConsensusType consensusStateMachineType
}

func (s *SubroundsHandler) EpochConfirmed(epoch uint32, _ uint64) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

missing comment

@@ -17,6 +17,7 @@ func TestEpochChangeWithNodesShufflingAndRater(t *testing.T) {
t.Skip("this is not a short test")
}

_ = logger.SetLogLevel("*:DEBUG")
Copy link
Collaborator

@sstanculeanu sstanculeanu Jan 10, 2025

Choose a reason for hiding this comment

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

not to forget removing this

@@ -203,12 +203,16 @@ func TestSyncWorksInShard_EmptyBlocksNoForks_With_EquivalentProofs(t *testing.T)
t.Skip("this is not a short test")
}

_ = logger.SetLogLevel("*:DEBUG,process:TRACE,consensus:TRACE")
Copy link
Collaborator

@sstanculeanu sstanculeanu Jan 10, 2025

Choose a reason for hiding this comment

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

not to forget removing this

@@ -56,6 +56,7 @@ func (idv *interceptedDataVerifier) Verify(interceptedData process.InterceptedDa

err := interceptedData.CheckValidity()
if err != nil {
log.Debug("Intercepted data is invalid", "hash", interceptedData.Hash(), "err", err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we'll need a special handle here, as interceptedData.Hash() will be the header hash, thus receiving an invalid proof for the first time will lead to always returning ErrInvalidInterceptedData for further proofs(even valid ones)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good find, will fix

@@ -384,6 +384,7 @@ type ForkDetector interface {
GetNotarizedHeaderHash(nonce uint64) []byte
ResetProbableHighestNonce()
SetFinalToLastCheckpoint()
// ReceivedProof()
Copy link
Collaborator

@sstanculeanu sstanculeanu Jan 10, 2025

Choose a reason for hiding this comment

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

not to forget removing this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@@ -161,6 +162,10 @@ func (boot *baseBootstrap) requestedHeaderHash() []byte {
return boot.headerhash
}

func (boot *baseBootstrap) processReceivedProof(headerProof data.HeaderProofHandler) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

not to forget the implementation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -52,13 +52,13 @@ export GENESIS_STAKE_TYPE="direct" #'delegated' or 'direct' as in direct stake
export OBSERVERS_ANTIFLOOD_DISABLE=0

# Shard structure
export SHARDCOUNT=2
export SHARD_VALIDATORCOUNT=3
export SHARDCOUNT=1
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should be reverted

func (s *SubroundsHandler) EpochConfirmed(epoch uint32, _ uint64) {
err := s.initSubroundsForEpoch(epoch)
if err != nil {
log.Error("SubroundsHandler.EpochStartAction: cannot initialize subrounds", "error", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
log.Error("SubroundsHandler.EpochStartAction: cannot initialize subrounds", "error", err)
log.Error("SubroundsHandler.EpochConfirmed: cannot initialize subrounds", "error", err)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Comment on lines 2825 to 2828
if tpn.EnableEpochsHandler.IsFlagEnabledInEpoch(common.EquivalentMessagesFlag, blockHeader.GetEpoch()) {

// first block after genesis does not have the previous proof, as well as first block after epoch change where the flag gets activated
shouldAddProof := blockHeader.GetNonce() > 1 && !common.IsEpochChangeBlockForFlagActivation(blockHeader, tpn.EnableEpochsHandler, common.EquivalentMessagesFlag)
Copy link
Contributor

Choose a reason for hiding this comment

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

use here common.ShouldBlockHavePrevProof?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@AdoAdoAdo AdoAdoAdo marked this pull request as ready for review January 14, 2025 13:25
sstanculeanu
sstanculeanu previously approved these changes Jan 14, 2025
ssd04
ssd04 previously approved these changes Jan 14, 2025
@AdoAdoAdo AdoAdoAdo dismissed stale reviews from ssd04 and sstanculeanu via 25276b1 January 15, 2025 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants