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

fix(dot/digest): verify if next epoch already contains some definition #2472

Merged
merged 22 commits into from
Apr 14, 2022
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
18bf1d7
fix: verify if next epoch already contains some definition
EclesioMeloJunior Apr 7, 2022
5ca05d0
chore: upgrade mockery version
EclesioMeloJunior Apr 7, 2022
44c3892
chore: remove duplicated code with generics
EclesioMeloJunior Apr 7, 2022
d69c694
chore: improve test comment
EclesioMeloJunior Apr 7, 2022
27bd2e8
chore: improve test comment
EclesioMeloJunior Apr 7, 2022
bbddab4
chore: improve test comment
EclesioMeloJunior Apr 7, 2022
71309cf
chore: improve test comment
EclesioMeloJunior Apr 7, 2022
58edb99
chore: remove the go:generate from the `EpochState`'s comment
EclesioMeloJunior Apr 7, 2022
47c8836
chore: use functions to define mocks in the test cases
EclesioMeloJunior Apr 8, 2022
931e8a0
chore: split into two different functions and check for existence bef…
EclesioMeloJunior Apr 12, 2022
73e3c33
chore: fix the ci lint warns
EclesioMeloJunior Apr 12, 2022
38ddcaa
chore: use a more descriptive name
EclesioMeloJunior Apr 12, 2022
dfd06d2
chore: unexport `Data` field on epoch_test.go
EclesioMeloJunior Apr 13, 2022
89a119f
chore: addressing last nits
EclesioMeloJunior Apr 13, 2022
f4cc36a
chore: rename to a better name
EclesioMeloJunior Apr 13, 2022
71ee157
chore: fix comments
EclesioMeloJunior Apr 13, 2022
cdd29ea
chore: addressing comments
EclesioMeloJunior Apr 13, 2022
d231f5a
Merge branch 'development' into eclesio/babe-check-already-persisted
EclesioMeloJunior Apr 13, 2022
16f35dc
chore: update mockery verison
EclesioMeloJunior Apr 13, 2022
1149c1f
chore: update devnet mocks as well
EclesioMeloJunior Apr 13, 2022
ac0bdd4
chore: improve comments
EclesioMeloJunior Apr 14, 2022
1fd682d
chore: improve comments
EclesioMeloJunior Apr 14, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 7 additions & 34 deletions dot/digest/digest.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import (
"errors"
"fmt"

"github.com/ChainSafe/gossamer/dot/state"
"github.com/ChainSafe/gossamer/dot/types"
"github.com/ChainSafe/gossamer/internal/log"
"github.com/ChainSafe/gossamer/lib/services"
Expand All @@ -17,8 +16,6 @@ import (

var (
_ services.Service = &Handler{}

ErrDefineNextEpoch = errors.New("cannot define next epoch data and config")
)

// Handler is used to handle consensus messages and relevant authority updates to BABE and GRANDPA
Expand Down Expand Up @@ -242,9 +239,14 @@ func (h *Handler) handleBlockFinalisation(ctx context.Context) {
continue
}

err := h.persistBABEDigestsForNextEpoch(&info.Header)
err := h.epochState.FinalizeBABENextEpochData(&info.Header)
if err != nil {
h.logger.Errorf("failed to persist babe next epoch data: %s", err)
}

err = h.epochState.FinalizeBABENextConfigData(&info.Header)
if err != nil {
h.logger.Errorf("failed to store babe next epoch digest: %s", err)
h.logger.Errorf("failed to persist babe next epoch config: %s", err)
}

err = h.handleGrandpaChangesOnFinalization(info.Header.Number)
Expand All @@ -257,35 +259,6 @@ func (h *Handler) handleBlockFinalisation(ctx context.Context) {
}
}

// persistBABEDigestsForNextEpoch is called only when a block is finalised
// and defines the correct next epoch data and next config data.
func (h *Handler) persistBABEDigestsForNextEpoch(finalizedHeader *types.Header) error {
currEpoch, err := h.epochState.GetEpochForBlock(finalizedHeader)
if err != nil {
return fmt.Errorf("cannot get epoch for block %d (%s): %w",
finalizedHeader.Number, finalizedHeader.Hash(), err)
}

nextEpoch := currEpoch + 1
err = h.epochState.FinalizeBABENextEpochData(nextEpoch)
if err != nil && !errors.Is(err, state.ErrEpochNotInMemory) {
return fmt.Errorf("cannot finalize babe next epoch data for block number %d (%s): %w",
finalizedHeader.Number, finalizedHeader.Hash(), err)
}

err = h.epochState.FinalizeBABENextConfigData(nextEpoch)
if err == nil {
return nil
} else if errors.Is(err, state.ErrEpochNotInMemory) {
return fmt.Errorf("%w: %s", ErrDefineNextEpoch, err)
}

// the epoch state does not contains any information about the next epoch
return fmt.Errorf("cannot finalize babe next config data for block number %d (%s): %w",
finalizedHeader.Number, finalizedHeader.Hash(), err)

}

func (h *Handler) handleGrandpaChangesOnImport(num uint) error {
resume := h.grandpaResume
if resume != nil && num >= resume.atBlock {
Expand Down
6 changes: 4 additions & 2 deletions dot/digest/interface.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ type BlockState interface {
FreeFinalisedNotifierChannel(ch chan *types.FinalisationInfo)
}

//go:generate mockgen -destination=mock_epoch_state_test.go -package $GOPACKAGE . EpochState
EclesioMeloJunior marked this conversation as resolved.
Show resolved Hide resolved

// EpochState is the interface for state.EpochState
type EpochState interface {
GetEpochForBlock(header *types.Header) (uint64, error)
Expand All @@ -26,8 +28,8 @@ type EpochState interface {

StoreBABENextEpochData(epoch uint64, hash common.Hash, nextEpochData types.NextEpochData)
StoreBABENextConfigData(epoch uint64, hash common.Hash, nextEpochData types.NextConfigData)
FinalizeBABENextEpochData(epoch uint64) error
FinalizeBABENextConfigData(epoch uint64) error
FinalizeBABENextEpochData(finalizedHeader *types.Header) error
FinalizeBABENextConfigData(finalizedHeader *types.Header) error
}

// GrandpaState is the interface for the state.GrandpaState
Expand Down
131 changes: 131 additions & 0 deletions dot/digest/mock_epoch_state_test.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

59 changes: 49 additions & 10 deletions dot/state/epoch.go
Original file line number Diff line number Diff line change
Expand Up @@ -540,11 +540,29 @@ func (s *EpochState) StoreBABENextConfigData(epoch uint64, hash common.Hash, nex
// getting the set of hashes from the received epoch and for each hash
// check if the header is in the database then it's been finalized and
// thus we can also set the corresponding EpochData in the database
func (s *EpochState) FinalizeBABENextEpochData(epoch uint64) error {
func (s *EpochState) FinalizeBABENextEpochData(finalizedHeader *types.Header) error {
s.nextEpochDataLock.Lock()
defer s.nextEpochDataLock.Unlock()

finalizedNextEpochData, err := lookupForNextEpochPersistedHash(s.nextEpochData, s, epoch)
finalizedBlockEpoch, err := s.GetEpochForBlock(finalizedHeader)
if err != nil {
return fmt.Errorf("cannot get epoch for block %d (%s): %w",
finalizedHeader.Number, finalizedHeader.Hash(), err)
}

nextEpoch := finalizedBlockEpoch + 1

epochInDatabase, err := s.getEpochDataInDatabase(nextEpoch)
EclesioMeloJunior marked this conversation as resolved.
Show resolved Hide resolved
if err != nil && !errors.Is(err, chaindb.ErrKeyNotFound) {
return fmt.Errorf("cannot check if next epoch data is already defined for epoch %d: %w", nextEpoch, err)
}

// config already defined we don't need to lookup in the map
EclesioMeloJunior marked this conversation as resolved.
Show resolved Hide resolved
if epochInDatabase != nil {
return nil
}

finalizedNextEpochData, err := findFinalizedHeaderForEpoch(s.nextEpochData, s, nextEpoch)
if err != nil {
return fmt.Errorf("cannot find next epoch data: %w", err)
}
Expand All @@ -554,14 +572,14 @@ func (s *EpochState) FinalizeBABENextEpochData(epoch uint64) error {
return fmt.Errorf("cannot transform epoch data: %w", err)
}

err = s.SetEpochData(epoch, ed)
err = s.SetEpochData(nextEpoch, ed)
if err != nil {
return fmt.Errorf("cannot set epoch data: %w", err)
}

// remove previous epochs from the memory
for e := range s.nextEpochData {
if e <= epoch {
if e <= nextEpoch {
delete(s.nextEpochData, e)
}
}
Expand All @@ -573,35 +591,56 @@ func (s *EpochState) FinalizeBABENextEpochData(epoch uint64) error {
// getting the set of hashes from the received epoch and for each hash
// check if the header is in the database then it's been finalized and
// thus we can also set the corresponding NextConfigData in the database
func (s *EpochState) FinalizeBABENextConfigData(epoch uint64) error {
func (s *EpochState) FinalizeBABENextConfigData(finalizedHeader *types.Header) error {
s.nextConfigDataLock.Lock()
defer s.nextConfigDataLock.Unlock()

finalizedNextConfigData, err := lookupForNextEpochPersistedHash(s.nextConfigData, s, epoch)
finalizedBlockEpoch, err := s.GetEpochForBlock(finalizedHeader)
if err != nil {
return fmt.Errorf("cannot get epoch for block %d (%s): %w",
finalizedHeader.Number, finalizedHeader.Hash(), err)
}

nextEpoch := finalizedBlockEpoch + 1

configInDatabase, err := s.getConfigDataInDatabase(nextEpoch)
EclesioMeloJunior marked this conversation as resolved.
Show resolved Hide resolved
if err != nil && !errors.Is(err, chaindb.ErrKeyNotFound) {
return fmt.Errorf("cannot check if next epoch config is already defined for epoch %d: %w", nextEpoch, err)
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Have you forgotten to cover the case when errors.Is(err, chaindb.ErrKeyNotFound) or is it deliberate?

Copy link
Member Author

@EclesioMeloJunior EclesioMeloJunior Apr 13, 2022

Choose a reason for hiding this comment

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

This is deliberate, I call the getConfigDataInDatabase and if I got the chaindb.ErrKeyNotFound error I don't need to return it since this error tells me that a key is not present in the database so we need to lookup in the memory and persist that data

// config already defined we don't need to lookup in the map
if configInDatabase != nil {
return nil
}

// not every epoch will have `ConfigData`
finalizedNextConfigData, err := findFinalizedHeaderForEpoch(s.nextConfigData, s, nextEpoch)
if errors.Is(err, ErrEpochNotInMemory) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we likely to come across ErrEpochNotInMemory a lot?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure but, in this case, we will always use the previous epoch config data.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a logger.Debugf inside this if condition

return nil
} else if err != nil {
return fmt.Errorf("cannot find next config data: %w", err)
}

cd := finalizedNextConfigData.ToConfigData()
err = s.SetConfigData(epoch, cd)
err = s.SetConfigData(nextEpoch, cd)
if err != nil {
return fmt.Errorf("cannot set config data: %w", err)
}

// remove previous epochs from the memory
for e := range s.nextConfigData {
if e <= epoch {
if e <= nextEpoch {
delete(s.nextConfigData, e)
}
}

return nil
}

// lookupForNextEpochPersistedHash given a specific epoch (the key) will go through the hashes looking
// findFinalizedHeaderForEpoch given a specific epoch (the key) will go through the hashes looking
// for a database persisted hash (belonging to the finalized chain)
// which contains the right configuration to be persisted and safely used
EclesioMeloJunior marked this conversation as resolved.
Show resolved Hide resolved
func lookupForNextEpochPersistedHash[T types.NextConfigData | types.NextEpochData](
func findFinalizedHeaderForEpoch[T types.NextConfigData | types.NextEpochData](
nextEpochMap map[uint64]map[common.Hash]T, es *EpochState, epoch uint64) (next *T, err error) {
hashes, has := nextEpochMap[epoch]
if !has {
Expand Down
Loading