Bug fixes staking v4 #3993

mariusmihaic · 2022-04-15T13:28:30Z

What's new

Added StorageHandlerArgs struct placeholder for all arguments required to create either a shard or a meta storage handler, which now also contains NodesCoordinatorRegistryFactory
StakingDataProvider now subscribes to EpochNotifier. This is used so that after staking v4, we expect newly staked nodes to be added in list = AuctionList, instead of list = NewList
In stakingToPeer, all newly staked and unjailed nodes are moved to auction list starting with staking v4 init

Bugfixes

nodeCoordinator.NodesCoordinatorToRegistry func now takes as input param epoch, so that it knows which marshaller to use depending on the epoch
In saveNodesCoordinatorRegistry from epochStart/bootStrap, nodes config is saved using NodesCoordinatorRegistryFactory, instead of hard coded json marshaller
Split unStakeNonEligibleNodesWithNotEnoughFunds in separate functions : fillStakingDataForNonEligible + unStakeNodesWithNotEnoughFunds/ unStakeNodesWithNotEnoughFundsWithStakingV4 depending on epoch
There was a bug in legacySystemSC after node restart. After restart, when EpochConfirmed is called with epoch =startInEpoch, this would cause maxNumNodesConfig to be = maxNumNodesConfig in startInEpoch, instead of latest config; fix is one line code, see L1366 + also added unit tests
Fixed saveState in indexHashedNodesCoordinatorRegistry.go to use the epoch to save the nodes config registry instead of flag. This is because we cannot rely on the flag after node restarts.
Fixed bug in shardValidatorsInfoMap -> GetValidator func where the getter used to return the pointer to the internal validator member instead of a copy.
Fixed inconsistency flags activation between staking.go and legacySystemSC.go/systemSC.go

…ve-unjailed-new-nodes-to-auction

…ve-unjailed-new-nodes-to-auction # Conflicts: # state/validatorsInfoMap_test.go

…ve-unjailed-new-nodes-to-auction # Conflicts: # factory/coreComponents.go # sharding/nodesCoordinator/indexHashedNodesCoordinator_test.go

…ve-unjailed-new-nodes-to-auction

sasurobert · 2022-05-02T13:27:42Z

process/scToProtocol/stakingToPeer.go

 	isValidator := account.GetList() == string(common.EligibleList) || account.GetList() == string(common.WaitingList)
 	if !stakingData.Jailed {
 		if stakingData.StakedNonce == nonce && !isValidator {
-			log.Debug("node is staked, changed status to new", "blsKey", blsPubKey)
-			account.SetListAndIndex(account.GetShardId(), string(common.NewList), uint32(stakingData.StakedNonce))
+			log.Debug(fmt.Sprintf("node is staked, changed status to %s list", newNodesList), "blsKey", blsPubKey)


Right, I missed it; could've used args.... Changed it

sasurobert · 2022-05-02T13:53:43Z

sharding/nodesCoordinator/nodesCoordinatorRegistryFactory.go

-	if ncf.flagStakingV4.IsSet() {
-		return ncf.createRegistryWithAuction(buff)
+	registry, err := ncf.createRegistryWithAuction(buff)
+	if err == nil {


it does not look good - it was a little better with flag check here directly.

Agree, but this change is actually a fix; since flag activation wouldn't work.
I've discovered a chicken-egg problem with this flag in case of node restart(also raised this problem to @iulianpascalau and @AdoAdoAdo) . After node restart, calling getLastBootstrapData was done with some dummy values and nodesCoordinatorRegistryFactory wouldn't have the flag correctly set, causing unmarshal error.

codecov · 2022-05-03T08:10:10Z

Codecov Report

Merging #3993 (1464814) into feat/liquid-staking (a1dad37) will decrease coverage by 0.01%.
The diff coverage is 82.64%.

❗ Current head 1464814 differs from pull request most recent head cee9d7e. Consider uploading reports for the commit cee9d7e to get more accurate results

@@                   Coverage Diff                   @@
##           feat/liquid-staking    #3993      +/-   ##
=======================================================
- Coverage                75.40%   75.38%   -0.02%     
=======================================================
  Files                      623      622       -1     
  Lines                    83343    83275      -68     
=======================================================
- Hits                     62845    62779      -66     
  Misses                   15773    15773              
+ Partials                  4725     4723       -2

Impacted Files	Coverage Δ
factory/bootstrapComponents.go	`78.26% <ø> (-0.12%)`	⬇️
factory/shardingFactory.go	`13.86% <0.00%> (-0.11%)`	⬇️
node/nodeRunner.go	`0.00% <0.00%> (ø)`
epochStart/metachain/systemSCs.go	`67.38% <32.14%> (-4.28%)`	⬇️
epochStart/bootstrap/baseStorageHandler.go	`55.55% <33.33%> (-5.64%)`	⬇️
state/validatorInfo.go	`18.36% <60.00%> (+4.73%)`	⬆️
epochStart/metachain/legacySystemSCs.go	`72.26% <73.33%> (+0.11%)`	⬆️
epochStart/bootstrap/shardStorageHandler.go	`88.67% <88.23%> (-0.43%)`	⬇️
epochStart/bootstrap/metaStorageHandler.go	`72.46% <100.00%> (+0.82%)`	⬆️
epochStart/bootstrap/process.go	`84.52% <100.00%> (+0.04%)`	⬆️
... and 27 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a1dad37...cee9d7e. Read the comment docs.

…des-to-auction

bogdan-rosianu · 2022-05-03T10:50:37Z

epochStart/bootstrap/process_test.go

@@ -90,8 +91,7 @@ func createMockEpochStartBootstrapArgs(
 ) ArgsEpochStartBootstrap {
 	generalCfg := testscommon.GetGeneralConfig()
 	nodesCoordinatorRegistryFactory, _ := nodesCoordinator.NewNodesCoordinatorRegistryFactory(
-		&testscommon.MarshalizerMock{},
-		&epochNotifier.EpochNotifierStub{},
+		&marshal.GogoProtoMarshalizer{},


is there a reason for using the protobuf here?

Yes, because MarshalizerMock uses json marshaller,.
NodesCoordinatorRegistryFactory uses 2 marshallers:

hard coded json marshaller

another marshaller which is expected to be != json (in our case, proto).

Decided to changed it to use this mock instead: NodesCoordinatorRegistryFactoryMock

bogdan-rosianu · 2022-05-03T10:55:48Z

epochStart/metachain/legacySystemSCs.go

@@ -321,7 +323,12 @@ func (s *legacySystemSCProcessor) unStakeNodesWithNotEnoughFunds(
 			continue
 		}

-		validatorInfo.SetList(string(common.LeavingList))
+		validatorLeaving := validatorInfo.ShallowClone()
+		validatorLeaving.SetList(string(common.LeavingList))


all the occurrences of this SetList cast the list to string. might change the function and the interface so the param is PeerType not string

Yes, but this is because we had to force ValidatorInfo DTO into an interface. This ValidatorInfo uses proto which auto-generates structs with List field of type string => GetList() will return by default string upon proto file generation.

If I change SetList, I would also have to change GetList, which is an auto-generated func by proto(returns string), and that is not possible

bogdan-rosianu · 2022-05-03T11:13:52Z

epochStart/metachain/systemSCs.go

+		log.Debug("unStake at end of epoch for node", "blsKey", blsKey)
+		err = s.unStakeOneNode(blsKey, epoch)
+		if err != nil {
+			return err


what happens if: we have 5 nodes in the nodesToUnStake. for the third node, unStakeOneNode returns err. the first 2 processed nodes remain unstaked and this can lead to inconsistencies.

Right, this would be pretty bad: execution would fail here and we wouldn't be able to also update delegation contracts (by calling updateDelegationContracts). This code is from legacy, but also updated for staking v4, so yes, it would lead the inconsistencies.

It will fail only if we make mistake in code

bogdan-rosianu · 2022-05-03T11:18:50Z

factory/blockProcessorCreator.go

@@ -712,7 +713,12 @@ func (pcf *processComponentsFactory) newMetaBlockProcessor(
 	}

 	// TODO: in case of changing the minimum node price, make sure to update the staking data provider
-	stakingDataProvider, err := metachainEpochStart.NewStakingDataProvider(systemVM, pcf.systemSCConfig.StakingSystemSCConfig.GenesisNodePrice)
+	stakingDataProvider, err := metachainEpochStart.NewStakingDataProvider(
+		systemVM,


could have created an args struct

Right, I was also thinking about it, but was not sure that is worth the refactor.
However, I've created args struct as your suggestion

bogdan-rosianu · 2022-05-03T11:25:03Z

process/scToProtocol/stakingToPeer.go

@@ -58,6 +59,8 @@ type stakingToPeer struct {
 	flagStaking                      atomic.Flag
 	validatorToDelegationEnableEpoch uint32
 	flagValidatorToDelegation        atomic.Flag
+	stakingV4InitEpoch               uint32
+	flagStakingV4                    atomic.Flag


rename to flagStakingV4Init

No, actually flagStakingV4 is correct in this component.
UpdateProtocol(body *block.Body, nonce uint64) is called whenever a new tx is processed in a block. If a new node is registerd or unjailed by a tx in epoch == stakingV4Init, we still want to save it with list == AuctionList

bogdan-rosianu · 2022-05-03T11:33:10Z

sharding/nodesCoordinator/indexHashedNodesCoordinatorRegistry.go

-func (ihnc *indexHashedNodesCoordinator) NodesCoordinatorToRegistry() NodesCoordinatorRegistryHandler {
-	if ihnc.flagStakingV4.IsSet() {
+func (ihnc *indexHashedNodesCoordinator) NodesCoordinatorToRegistry(epoch uint32) NodesCoordinatorRegistryHandler {
+	if epoch >= ihnc.stakingV4EnableEpoch {


could have used polymorphism

I honestly don't see any good solution for this problem using polymorphism, I actually think it would only increase the overhead. This is the purpose of this factory: to create registry data depending on the epoch

bogdan-rosianu · 2022-05-03T11:40:38Z

state/validatorInfo.go

@@ -109,6 +109,16 @@ func (vi *ValidatorInfo) SetTotalValidatorIgnoredSignatures(totalValidatorIgnore
 	vi.TotalValidatorIgnoredSignatures = totalValidatorIgnoredSignatures
 }

+// ShallowClone returns a clone of the object
+func (vi *ValidatorInfo) ShallowClone() ValidatorInfoHandler {


please rename the test TestValidatorInfo_IsInterfaceNile

I deleted that file. It had not benefit.

mariusmihaic added 10 commits April 15, 2022 16:27

FEAT: Move unjailed and new staked nodes to auction

c000cff

FIX: Check for no error

751d213

FEAT: Refactor code in stakingDataProvider and systemScs

cbe5cb1

FIX: Pointer bugs + refactor systemSCs.go

79d4fc4

FEAT: Unit tests for stakingDataProvider.go with staking v4

aa31e14

FIX: Small fixes

8af8559

FIX: Epoch flag name

1c1987c

FIX: Pass staking v4 epoch in nodes coord

2ce0098

Merge branch 'EN-11927-integration-tests-staking-v4' into EN-11957-mo…

6cf20ea

…ve-unjailed-new-nodes-to-auction

Merge branch 'EN-11927-integration-tests-staking-v4' into EN-11957-mo…

d8039c4

…ve-unjailed-new-nodes-to-auction

mariusmihaic self-assigned this Apr 20, 2022

mariusmihaic added 15 commits April 20, 2022 13:45

FIX: Delete error condition for maxNumNodes decrease

cb549f6

Merge branch 'EN-11927-integration-tests-staking-v4' into EN-11957-mo…

d60e2d0

…ve-unjailed-new-nodes-to-auction # Conflicts: # state/validatorsInfoMap_test.go

FEAT: One more unit test

cf4c2f4

FIX: Hot fix for chicken-egg problem in CreateNodesCoordinatorRegistry

e4cd7f2

FIX: Bug in storageHandler when saving nodes coord registry

098bb93

FIX: saveState in indexHashedNodesCoordinator.go

7ef95c4

Merge branch 'EN-11927-integration-tests-staking-v4' into EN-11957-mo…

d353ece

…ve-unjailed-new-nodes-to-auction # Conflicts: # factory/coreComponents.go # sharding/nodesCoordinator/indexHashedNodesCoordinator_test.go

Merge branch 'EN-11927-integration-tests-staking-v4' into EN-11957-mo…

b90f7c3

…ve-unjailed-new-nodes-to-auction

Merge branch 'EN-11927-integration-tests-staking-v4' into EN-11957-mo…

e109d6c

…ve-unjailed-new-nodes-to-auction

FIX: Bug in maxNumNodesUpdate in legacySystemSCs.go

1574f53

FIX: Do some fixes

0ade9ea

FIX: Add missed file

4366a7d

FIX: Gas schedule

4316271

FIX: Flags inconsistency between systemSCs.go and staking.go

8338304

FIX: Broken tests

7d507b1

mariusmihaic changed the title ~~Move unjailed and new staked nodes to auction~~ Bug fixes staking v4 May 2, 2022

mariusmihaic marked this pull request as ready for review May 2, 2022 13:01

sasurobert self-requested a review May 2, 2022 13:12

Base automatically changed from EN-11927-integration-tests-staking-v4 to feat/liquid-staking May 2, 2022 13:12

sasurobert reviewed May 2, 2022

View reviewed changes

mariusmihaic added 2 commits May 3, 2022 11:00

FIX: Broken tests

3d4d319

FIX: Review finding

3557a42

mariusmihaic added 3 commits May 3, 2022 11:16

FIX: Linter errors

7a3c479

Merge branch 'feat/liquid-staking' into EN-11957-move-unjailed-new-no…

91ca831

…des-to-auction

FIX: More tests

0a13853

bogdan-rosianu self-requested a review May 3, 2022 10:37

bogdan-rosianu reviewed May 3, 2022

View reviewed changes

FIX: Review findings

cee9d7e

bogdan-rosianu approved these changes May 3, 2022

View reviewed changes

sasurobert approved these changes May 3, 2022

View reviewed changes

mariusmihaic merged commit 97275d1 into feat/liquid-staking May 3, 2022

mariusmihaic deleted the EN-11957-move-unjailed-new-nodes-to-auction branch May 3, 2022 15:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug fixes staking v4 #3993

Bug fixes staking v4 #3993

mariusmihaic commented Apr 15, 2022 •

edited

sasurobert May 2, 2022

mariusmihaic May 3, 2022

sasurobert May 2, 2022

mariusmihaic May 3, 2022 •

edited

codecov bot commented May 3, 2022 •

edited

bogdan-rosianu May 3, 2022

mariusmihaic May 3, 2022

bogdan-rosianu May 3, 2022

mariusmihaic May 3, 2022 •

edited

bogdan-rosianu May 3, 2022

mariusmihaic May 3, 2022

sasurobert May 3, 2022

bogdan-rosianu May 3, 2022

mariusmihaic May 3, 2022

bogdan-rosianu May 3, 2022

mariusmihaic May 3, 2022

bogdan-rosianu May 3, 2022

mariusmihaic May 3, 2022

bogdan-rosianu May 3, 2022

mariusmihaic May 3, 2022

Bug fixes staking v4 #3993

Bug fixes staking v4 #3993

Conversation

mariusmihaic commented Apr 15, 2022 • edited

What's new

Bugfixes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariusmihaic May 3, 2022 • edited

Choose a reason for hiding this comment

codecov bot commented May 3, 2022 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariusmihaic May 3, 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

mariusmihaic commented Apr 15, 2022 •

edited

mariusmihaic May 3, 2022 •

edited

codecov bot commented May 3, 2022 •

edited

mariusmihaic May 3, 2022 •

edited