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

Author a sibling block in case best block's slot is same as current slot #2827

Merged
merged 62 commits into from
Sep 26, 2022
Merged
Show file tree
Hide file tree
Changes from 58 commits
Commits
Show all changes
62 commits
Select commit Hold shift + click to select a range
5cd4263
temp
kishansagathiya Jul 18, 2022
f57fa84
temp
kishansagathiya Jul 18, 2022
ca4724f
Merge branch 'development' into kishan/fix/slot-number-must-increase
kishansagathiya Aug 2, 2022
12fde0b
temp
kishansagathiya Aug 3, 2022
ae2fa69
While authoring a new block, if we find that best block was authored in
kishansagathiya Aug 4, 2022
cc2ffaf
clean up
kishansagathiya Aug 4, 2022
cbc44d2
add a check for current slot is greater than slot in best block
kishansagathiya Aug 5, 2022
3de132f
test for err slot lagging
kishansagathiya Aug 16, 2022
a63a073
test
kishansagathiya Aug 17, 2022
aca3b68
fix test for errLaggingSlot
kishansagathiya Aug 17, 2022
9c9f47b
test that we create a fork if best block is authored in the same slot
kishansagathiya Aug 17, 2022
527f515
temp
kishansagathiya Aug 22, 2022
cecc308
fixed tests
kishansagathiya Aug 29, 2022
d022f43
Merge branch 'development' into kishan/fix/slot-number-must-increase
kishansagathiya Aug 29, 2022
540c09a
Author a sibling block in case best block's slot is same as current s…
kishansagathiya Sep 9, 2022
1edb86b
chore(dot): remove unneeded error functions (#2790)
qdm12 Sep 9, 2022
7fa9196
chore(wasmer): add helpers.go file with helper functions (#2749)
qdm12 Sep 9, 2022
64e3c03
Merge branch 'development' of github.com:ChainSafe/gossamer into deve…
kishansagathiya Sep 13, 2022
db03cbb
Merge branch 'development' into kishan/fix/slot-number-must-increase
kishansagathiya Sep 13, 2022
95a7a35
merge fix
kishansagathiya Sep 13, 2022
220f5ae
Merge branch 'development' of github.com:ChainSafe/gossamer into deve…
kishansagathiya Sep 14, 2022
37fa232
Merge branch 'development' into kishan/fix/slot-number-must-increase
kishansagathiya Sep 14, 2022
11471e8
intermediate
kishansagathiya Sep 15, 2022
e0109dc
it probably fixes the test
kishansagathiya Sep 15, 2022
93b3057
Merge branch 'development' into kishan/fix/slot-number-must-increase
kishansagathiya Sep 15, 2022
fdb1fd2
Use a real block import handler
kishansagathiya Sep 15, 2022
388678a
more fix
kishansagathiya Sep 16, 2022
0862104
improved createTestService
kishansagathiya Sep 16, 2022
c99b776
Merge branch 'development' into kishan/fix/slot-number-must-increase
kishansagathiya Sep 16, 2022
26f8769
reuse network mocks
kishansagathiya Sep 16, 2022
22d9b2c
more fix
kishansagathiya Sep 16, 2022
3e2aa7f
Update lib/babe/babe_integration_test.go
kishansagathiya Sep 19, 2022
1589dc9
Update lib/babe/babe_integration_test.go
kishansagathiya Sep 19, 2022
8ec533c
Update lib/babe/babe_integration_test.go
kishansagathiya Sep 19, 2022
8448863
Update lib/babe/babe.go
kishansagathiya Sep 19, 2022
ce9e195
Update lib/babe/babe_integration_test.go
kishansagathiya Sep 19, 2022
bcdf2b5
Update lib/babe/babe_integration_test.go
kishansagathiya Sep 19, 2022
9a9f26d
Update lib/babe/babe_integration_test.go
kishansagathiya Sep 19, 2022
b065fb8
Update lib/babe/babe_integration_test.go
kishansagathiya Sep 19, 2022
264db41
Update lib/babe/babe_integration_test.go
kishansagathiya Sep 19, 2022
24e69e6
Update dot/state/block_notify.go
kishansagathiya Sep 21, 2022
2fd728a
Update lib/babe/babe_integration_test.go
kishansagathiya Sep 21, 2022
31fa786
Merge branch 'development' into kishan/fix/slot-number-must-increase
kishansagathiya Sep 21, 2022
881eee0
Merge branch 'kishan/fix/slot-number-must-increase' of github.com:Cha…
kishansagathiya Sep 21, 2022
34800ee
Update lib/babe/babe.go
kishansagathiya Sep 21, 2022
8155d43
Update lib/babe/babe.go
kishansagathiya Sep 21, 2022
d0b2a1b
Update lib/babe/babe.go
kishansagathiya Sep 21, 2022
03db54d
Update lib/babe/babe_integration_test.go
kishansagathiya Sep 21, 2022
45b5fd3
Update lib/babe/babe_integration_test.go
kishansagathiya Sep 21, 2022
a71de07
Update lib/babe/errors.go
kishansagathiya Sep 21, 2022
37b41ca
fix
kishansagathiya Sep 21, 2022
532c617
split handleSlot function
kishansagathiya Sep 21, 2022
f56e154
Update lib/babe/babe_integration_test.go
kishansagathiya Sep 21, 2022
849ad82
Update lib/babe/babe_integration_test.go
kishansagathiya Sep 21, 2022
9ba1b58
separate mock interface
kishansagathiya Sep 22, 2022
8dbc936
Merge branch 'kishan/fix/slot-number-must-increase' of github.com:Cha…
kishansagathiya Sep 22, 2022
db736ba
some clean up
kishansagathiya Sep 22, 2022
3366ecc
Update lib/babe/babe_integration_test.go
kishansagathiya Sep 22, 2022
65f649e
added mock generate file
kishansagathiya Sep 26, 2022
e667450
Merge branch 'kishan/fix/slot-number-must-increase' of github.com:Cha…
kishansagathiya Sep 26, 2022
06313f1
fixed the mock generation command
kishansagathiya Sep 26, 2022
bb1a02c
Merge branch 'development' into kishan/fix/slot-number-must-increase
kishansagathiya Sep 26, 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
1 change: 1 addition & 0 deletions dot/services.go
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,7 @@ func (nb nodeBuilder) createBABEService(cfg *Config, st *state.Service, ks keyst
cs *core.Service, telemetryMailer telemetry.Client) (babe.ServiceIFace, error) {
return nb.createBABEServiceWithBuilder(cfg, st, ks, cs, telemetryMailer, babe.Builder{})
}

func (nodeBuilder) createBABEServiceWithBuilder(cfg *Config, st *state.Service, ks keystore.Keystore,
cs *core.Service, telemetryMailer telemetry.Client, newBabeService ServiceBuilder) (babe.
ServiceIFace, error) {
Expand Down
4 changes: 3 additions & 1 deletion dot/types/authority.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@ import (

// Authority struct to hold authority data
type Authority struct {
Key crypto.PublicKey
Key crypto.PublicKey
// Weight exists for potential improvements in the protocol and could
// have a use-case in the future. In polkadot all authorities have the weight = 1.
Weight uint64
}

Expand Down
47 changes: 40 additions & 7 deletions lib/babe/babe.go
Original file line number Diff line number Diff line change
Expand Up @@ -454,32 +454,65 @@ func (b *Service) handleEpoch(epoch uint64) (next uint64, err error) {
return next, nil
}

func (b *Service) handleSlot(epoch, slotNum uint64,
authorityIndex uint32,
preRuntimeDigest *types.PreRuntimeDigest,
) error {
func (b *Service) getParentForBlockAuthoring(slotNum uint64) (*types.Header, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

please add a unit test for this function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It gets tested TestService_HandleSlotWithSameSlot and TestService_HandleSlotWithLaggingSlot.

Copy link
Contributor

@qdm12 qdm12 Sep 22, 2022

Choose a reason for hiding this comment

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

These are integration tests, not unit tests. We want a unit test because:

  • it should cover all code paths (unlike integ tests)
  • it should be fast
  • it's easier to spot a bug with a unit test than through an integ test

Copy link
Contributor

Choose a reason for hiding this comment

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

@qdm12 please remember this was a PR that was already approved before. The lack of existing unit tests in the babe package is known. I don't think it makes sense to add scope to this PR now by just unit testing this function, and not handleSlot as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like new code added should be unit tested, but up to you if you feel it's not necessary. I wasn't referring to unit testing handleSlot, that's indeed out of scope.

parentHeader, err := b.blockState.BestBlockHeader()
if err != nil {
return err
return nil, fmt.Errorf("could not get best block header: %w", err)
}

if parentHeader == nil {
return errNilParentHeader
return nil, errNilParentHeader
}

atGenesisBlock := b.blockState.GenesisHash().Equal(parentHeader.Hash())
if !atGenesisBlock {
bestBlockSlotNum, err := b.blockState.GetSlotForBlock(parentHeader.Hash())
Comment on lines +468 to +469
Copy link
Contributor

Choose a reason for hiding this comment

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

nit maybe move the content of that if block to a separate function?

if err != nil {
return nil, fmt.Errorf("could not get slot for block: %w", err)
}

if bestBlockSlotNum > slotNum {
return nil, fmt.Errorf("%w: best block slot number is %d and got slot number %d",
errLaggingSlot, bestBlockSlotNum, slotNum)
}

if bestBlockSlotNum == slotNum {
// pick parent of best block instead to handle slot
newParentHeader, err := b.blockState.GetHeader(parentHeader.ParentHash)
if err != nil {
return nil, fmt.Errorf("could not get header: %w", err)
}
if newParentHeader == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit 🤔 can this ever happen?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It probably isn't supposed to happen, but I have seen it happen.

Copy link
Contributor

Choose a reason for hiding this comment

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

Uh should we create an issue for that? Sounds kinda strange we can't find the parent header right?

return nil, fmt.Errorf("%w: for block hash %s", errNilParentHeader, parentHeader.ParentHash)
}
parentHeader = newParentHeader
kishansagathiya marked this conversation as resolved.
Show resolved Hide resolved
}
}

// there is a chance that the best block header may change in the course of building the block,
// so let's copy it first.
parent, err := parentHeader.DeepCopy()
if err != nil {
return err
return nil, fmt.Errorf("could not create deep copy of parent header: %w", err)
}

return parent, nil
}

func (b *Service) handleSlot(epoch, slotNum uint64,
authorityIndex uint32,
preRuntimeDigest *types.PreRuntimeDigest,
) error {
currentSlot := Slot{
start: time.Now(),
duration: b.constants.slotDuration,
number: slotNum,
}

parent, err := b.getParentForBlockAuthoring(slotNum)
if err != nil {
return fmt.Errorf("could not get parent for claiming slot %d: %w", slotNum, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

the slotNum argument should be bundled in the error context by getParentForBlockAuthoring, not here. Also shouldn't it be for block authoring instead of for claiming slot?

Suggested change
return fmt.Errorf("could not get parent for claiming slot %d: %w", slotNum, err)
return fmt.Errorf("could not get parent for block authoring: %w", err)

}
b.storageState.Lock()
defer b.storageState.Unlock()

Expand Down
235 changes: 199 additions & 36 deletions lib/babe/babe_integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
package babe

import (
"path/filepath"
"testing"
"time"

Expand All @@ -15,16 +14,16 @@ import (
"github.com/ChainSafe/gossamer/dot/types"
"github.com/ChainSafe/gossamer/internal/log"
"github.com/ChainSafe/gossamer/lib/babe/mocks"
"github.com/ChainSafe/gossamer/lib/common"
"github.com/ChainSafe/gossamer/lib/crypto/sr25519"
"github.com/ChainSafe/gossamer/lib/runtime"
rtstorage "github.com/ChainSafe/gossamer/lib/runtime/storage"
"github.com/ChainSafe/gossamer/lib/runtime/wasmer"
"github.com/ChainSafe/gossamer/lib/trie"
"github.com/ChainSafe/gossamer/lib/utils"
"github.com/ChainSafe/gossamer/pkg/scale"
"github.com/golang/mock/gomock"

mock "github.com/stretchr/testify/mock"
"github.com/stretchr/testify/mock"
"github.com/stretchr/testify/require"
)

Expand Down Expand Up @@ -79,37 +78,32 @@ func createTestService(t *testing.T, cfg ServiceConfig) *Service {

cfg.Telemetry = telemetryMock

if cfg.TransactionState == nil {
cfg.TransactionState = state.NewTransactionState(telemetryMock)
}

testDatadirPath := t.TempDir()
require.NoError(t, err)

var dbSrv *state.Service
if cfg.BlockState == nil || cfg.StorageState == nil || cfg.EpochState == nil {
config := state.Config{
Path: testDatadirPath,
LogLevel: log.Info,
Telemetry: telemetryMock,
}
dbSrv = state.NewService(config)
dbSrv.UseMemDB()
config := state.Config{
Path: testDatadirPath,
LogLevel: log.Info,
Telemetry: telemetryMock,
}
dbSrv = state.NewService(config)
dbSrv.UseMemDB()

err = dbSrv.Initialise(&gen, &genHeader, &genTrie)
require.NoError(t, err)
err = dbSrv.Initialise(&gen, &genHeader, &genTrie)
require.NoError(t, err)

err = dbSrv.Start()
require.NoError(t, err)
err = dbSrv.Start()
require.NoError(t, err)

t.Cleanup(func() {
_ = dbSrv.Stop()
})
t.Cleanup(func() {
_ = dbSrv.Stop()
})

cfg.BlockState = dbSrv.Block
cfg.StorageState = dbSrv.Storage
cfg.EpochState = dbSrv.Epoch
}
cfg.BlockState = dbSrv.Block
cfg.StorageState = dbSrv.Storage
cfg.EpochState = dbSrv.Epoch
cfg.TransactionState = dbSrv.Transaction
kishansagathiya marked this conversation as resolved.
Show resolved Hide resolved

var rtCfg wasmer.Config
rtCfg.Storage = rtstorage.NewTrieState(&genTrie)
Expand All @@ -119,12 +113,7 @@ func createTestService(t *testing.T, cfg ServiceConfig) *Service {
require.NoError(t, err)

nodeStorage := runtime.NodeStorage{}
if dbSrv != nil {
nodeStorage.BaseDB = dbSrv.Base
} else {
nodeStorage.BaseDB, err = utils.SetupDatabase(filepath.Join(testDatadirPath, "offline_storage"), false)
require.NoError(t, err)
}
nodeStorage.BaseDB = dbSrv.Base

rtCfg.NodeStorage = nodeStorage
rt, err := wasmer.NewRuntimeFromGenesis(rtCfg)
Expand All @@ -135,6 +124,26 @@ func createTestService(t *testing.T, cfg ServiceConfig) *Service {
cfg.LogLvl = defaultTestLogLvl
babeService, err := NewService(&cfg)
require.NoError(t, err)

if cfg.BlockImportHandler == nil {
mockNetwork := mocks.NewMockNetwork(ctrl)
mockNetwork.EXPECT().GossipMessage(gomock.Any()).AnyTimes()

coreConfig := core.Config{
BlockState: dbSrv.Block,
EpochState: dbSrv.Epoch,
StorageState: storageState,
TransactionState: dbSrv.Transaction,
Runtime: rt,
Keystore: rtCfg.Keystore,
Network: mockNetwork,
CodeSubstitutedState: dbSrv.Base,
CodeSubstitutes: make(map[common.Hash]string),
}

babeService.blockImportHandler = NewTestService(t, &coreConfig)
}

return babeService
}

Expand Down Expand Up @@ -256,17 +265,15 @@ func TestService_GetAuthorityIndex(t *testing.T) {
}

func TestStartAndStop(t *testing.T) {
serviceConfig := ServiceConfig{}
bs := createTestService(t, serviceConfig)
bs := createTestService(t, ServiceConfig{})
qdm12 marked this conversation as resolved.
Show resolved Hide resolved
err := bs.Start()
require.NoError(t, err)
err = bs.Stop()
require.NoError(t, err)
}

func TestService_PauseAndResume(t *testing.T) {
serviceConfig := ServiceConfig{}
bs := createTestService(t, serviceConfig)
bs := createTestService(t, ServiceConfig{})
err := bs.Start()
require.NoError(t, err)
time.Sleep(time.Second)
Expand All @@ -292,3 +299,159 @@ func TestService_PauseAndResume(t *testing.T) {
err = bs.Stop()
require.NoError(t, err)
}

func TestService_HandleSlotWithLaggingSlot(t *testing.T) {
cfg := ServiceConfig{
Authority: true,
Lead: true,
}
babeService := createTestService(t, cfg)

err := babeService.Start()
require.NoError(t, err)
defer func() {
err = babeService.Stop()
require.NoError(t, err)
}()

// add a block
parentHash := babeService.blockState.GenesisHash()
rt, err := babeService.blockState.GetRuntime(nil)
require.NoError(t, err)

epochData, err := babeService.initiateEpoch(testEpochIndex)
require.NoError(t, err)

ext := runtime.NewTestExtrinsic(t, rt, parentHash, parentHash, 0, "System.remark", []byte{0xab, 0xcd})
block := createTestBlock(t, babeService, emptyHeader, [][]byte{common.MustHexToBytes(ext)},
1, testEpochIndex, epochData)

babeService.blockState.AddBlock(block)
time.Sleep(babeService.constants.slotDuration)

header, err := babeService.blockState.BestBlockHeader()
require.NoError(t, err)

bestBlockSlotNum, err := babeService.blockState.GetSlotForBlock(header.Hash())
require.NoError(t, err)

slotnum := uint64(1)
slot := Slot{
start: time.Now(),
duration: time.Second,
number: slotnum,
}
preRuntimeDigest, err := types.NewBabePrimaryPreDigest(
0, slot.number,
[sr25519.VRFOutputLength]byte{},
[sr25519.VRFProofLength]byte{},
).ToPreRuntimeDigest()

require.NoError(t, err)

err = babeService.handleSlot(
babeService.epochHandler.epochNumber,
bestBlockSlotNum-1,
babeService.epochHandler.epochData.authorityIndex,
preRuntimeDigest)

require.ErrorIs(t, err, errLaggingSlot)
}

func TestService_HandleSlotWithSameSlot(t *testing.T) {
alice := keyring.Alice().(*sr25519.Keypair)
bob := keyring.Bob().(*sr25519.Keypair)

// Create babe service for alice
cfgAlice := ServiceConfig{
Authority: true,
Lead: true,
Keypair: alice,
AuthData: []types.Authority{
{
Key: alice.Public().(*sr25519.PublicKey),
Weight: 1,
},
{
Key: bob.Public().(*sr25519.PublicKey),
Weight: 1,
},
},
}

// Create babe service for bob
cfgBob := ServiceConfig{
Authority: true,
Lead: true,
Keypair: bob,
AuthData: []types.Authority{
{
Key: alice.Public().(*sr25519.PublicKey),
Weight: 1,
},
{
Key: bob.Public().(*sr25519.PublicKey),
Weight: 1,
},
},
}
qdm12 marked this conversation as resolved.
Show resolved Hide resolved

babeServiceBob := createTestService(t, cfgBob)

err := babeServiceBob.Start()
require.NoError(t, err)
defer func() {
_ = babeServiceBob.Stop()
}()

// wait till bob creates a block
time.Sleep(babeServiceBob.constants.slotDuration)
require.NoError(t, err)

block, err := babeServiceBob.blockState.GetBlockByNumber(1)
require.NoError(t, err)

err = babeServiceBob.Stop()
require.NoError(t, err)

time.Sleep(babeServiceBob.constants.slotDuration)

babeServiceAlice := createTestService(t, cfgAlice)

// Add block created by Bob to Alice
err = babeServiceAlice.blockState.AddBlock(block)
require.NoError(t, err)

time.Sleep(babeServiceBob.constants.slotDuration)

bestBlockHeader, err := babeServiceAlice.blockState.BestBlockHeader()
require.NoError(t, err)
require.Equal(t, block.Header.Hash(), bestBlockHeader.Hash())

// If the slot we are claiming is the same as the slot of the best block header, test that we can
// still claim the slot without error.
bestBlockSlotNum, err := babeServiceAlice.blockState.GetSlotForBlock(bestBlockHeader.Hash())
require.NoError(t, err)

slot := Slot{
start: time.Now(),
duration: time.Second,
number: bestBlockSlotNum,
}
preRuntimeDigest, err := types.NewBabePrimaryPreDigest(
0, slot.number,
[sr25519.VRFOutputLength]byte{},
[sr25519.VRFProofLength]byte{},
).ToPreRuntimeDigest()
require.NoError(t, err)

// slot gets occupied even if it has been occupied by a block
// authored by someone else
err = babeServiceAlice.handleSlot(
testEpochIndex,
bestBlockSlotNum,
0,
preRuntimeDigest)
require.NoError(t, err)

}
Loading