bugFix/only-staked-nodes-to-validators #1771

sasurobert · 2020-05-20T13:27:22Z

Save only staked nodes to validators trie.
Add storage updates to smart contract results only for staking smart contract

iulianpascalau · 2020-05-20T13:42:27Z

process/scToProtocol/stakingToPeer.go

 	blsPubKey []byte,
 	nonce uint64,
 ) error {
+	if stakingData.StakedNonce == math.MaxUint64 {


ugly hardcoded thing. Will think about it.

yes - will try to get rid of this component and make the stakingSC to write directly into the validator trie - but there are a few issues right now with it. Like like reverting and journalizing of those elements.

SebastianMarian · 2020-05-20T14:16:58Z

vm/systemSmartContracts/auction_test.go

@@ -1702,6 +1701,7 @@ func TestAuctionStakingSC_ChangeRewardAddress(t *testing.T) {
 	changeRewardAddress(t, sc, stakerAddress, newRewardAddr, vmcommon.Ok)
 }

+/*


Maybe t.Skip with description instead commented code?

forgot t.skip :)

…nto bugFix/save-only-staked-nodes

iulianpascalau · 2020-05-21T08:08:25Z

vm/systemSmartContracts/auction.go

@@ -701,11 +715,23 @@ func (s *stakingAuctionSC) unStake(args *vmcommon.ContractCallInput) vmcommon.Re
 	}

 	for _, blsKey := range blsKeys {
+		if registrationData.NumStaked == 0 {
+			continue


why not break?

iulianpascalau · 2020-05-21T11:11:36Z

process/economics/economicsData.go

 	return ed.maxGasLimitPerBlock
 }

+// MaxGasLimitPerMetaBlock will return maximum gas limit allowed per block
+func (ed *EconomicsData) MaxGasLimitPerMetaBlock() uint64 {


why do we need this method for? MaxGasLimitPerBlock is enogh for the job.

SebastianMarian · 2020-05-21T10:37:08Z

vm/systemSmartContracts/auction.go

@@ -701,11 +715,23 @@ func (s *stakingAuctionSC) unStake(args *vmcommon.ContractCallInput) vmcommon.Re
 	}

 	for _, blsKey := range blsKeys {
+		if registrationData.NumStaked == 0 {
+			continue


SebastianMarian · 2020-05-21T10:41:24Z

vm/systemSmartContracts/auction.go

+
+	err = s.saveRegistrationData(args.CallerAddr, registrationData)
+	if err != nil {
+		return vmcommon.UserError


It is ok to return vmcommon.UserError, if saving not worked but unStake and registrationData.NumStaked -= 1 has been done ?

this error can be only marshalizing error. can't do anything if the final result cannot be saved to trie.

SebastianMarian · 2020-05-21T11:14:02Z

vm/systemSmartContracts/auction.go

@@ -1034,8 +1076,8 @@ func shuffleList(list []string, random []byte) {
 func isNumArgsCorrectToStake(args [][]byte) bool {
 	maxNodesToRun := big.NewInt(0).SetBytes(args[0]).Uint64()
 	areEnoughArgs := uint64(len(args)) >= 2*maxNodesToRun+1
-
-	return areEnoughArgs
+	areNotTooManyArgs := uint64(len(args)) <= 2*maxNodesToRun+1+2


use a var with a description for this: 2*maxNodesToRun+1+2

added comment.

SebastianMarian · 2020-05-21T12:14:08Z

genesis/process/disabled/feeHandler.go

@@ -22,7 +22,7 @@ func (fh *FeeHandler) MinGasPrice() uint64 {
 }

 // MaxGasLimitPerBlock return max uint64


Reformulate this comment :)

it really returns max uint64 as it is a disabled handler.

SebastianMarian · 2020-05-21T12:15:41Z

integrationTests/mock/feeHandlerStub.go

@@ -48,7 +48,7 @@ func (fhs *FeeHandlerStub) SetMinGasLimit(minGasLimit uint64) {
 }

 // MaxGasLimitPerBlock -
-func (fhs *FeeHandlerStub) MaxGasLimitPerBlock() uint64 {
+func (fhs *FeeHandlerStub) MaxGasLimitPerBlock(uint32) uint64 {
 	return fhs.MaxGasLimitPerBlockCalled()


change the mock method signature also

it is not used rigth now - in any test it is only one value. there is no need.

SebastianMarian · 2020-05-21T12:16:15Z

node/mock/txFeeHandlerStub.go

@@ -32,7 +32,7 @@ func (fhs *FeeHandlerStub) DeveloperPercentage() float64 {
 }

 // MaxGasLimitPerBlock -
-func (fhs *FeeHandlerStub) MaxGasLimitPerBlock() uint64 {
+func (fhs *FeeHandlerStub) MaxGasLimitPerBlock(uint32) uint64 {
 	return fhs.MaxGasLimitPerBlockCalled()


change the mock method signature also

it is not used rigth now - in any test it is only one value. there is no need.

SebastianMarian · 2020-05-21T12:19:06Z

process/economics/economicsData.go

@@ -108,6 +111,11 @@ func convertValues(economics *config.EconomicsConfig) (*EconomicsData, error) {
 		return nil, process.ErrInvalidMaxGasLimitPerBlock
 	}

+	maxGasLimitPerMetaBlock, err := strconv.ParseUint(economics.FeeSettings.MaxGasLimitPerMetaBlock, conversionBase, bitConversionSize)
+	if err != nil {
+		return nil, process.ErrInvalidMaxGasLimitPerBlock


ErrInvalidMaxGasLimitPerMetaBlock ?

no need to add one more.

SebastianMarian · 2020-05-21T12:23:06Z

process/economics/testEconomicsData.go

@@ -12,6 +12,7 @@ type TestEconomicsData struct {
 // SetMaxGasLimitPerBlock sets the maximum gas limit allowed per one block
 func (ted *TestEconomicsData) SetMaxGasLimitPerBlock(maxGasLimitPerBlock uint64) {


Maybe you can add another parameter to this method maxGasLimitPerMetaBlock uint64, so in this way we can test with different values if we want this. Or better to create another method to SetMaxGasLimitPerMetaBlock

it is not used rigth now - in any test it is only one value. there is no need. When there will be a test to especially change these values - it will be created.

SebastianMarian · 2020-05-21T12:25:18Z

process/mock/feeHandlerStub.go

@@ -48,7 +48,7 @@ func (fhs *FeeHandlerStub) SetMinGasLimit(minGasLimit uint64) {
 }

 // MaxGasLimitPerBlock -
-func (fhs *FeeHandlerStub) MaxGasLimitPerBlock() uint64 {
+func (fhs *FeeHandlerStub) MaxGasLimitPerBlock(uint32) uint64 {
 	return fhs.MaxGasLimitPerBlockCalled()


change the mock method signature also

it is not used rigth now - in any test it is only one value. there is no need.

SebastianMarian · 2020-05-21T12:26:18Z

process/smartContract/scQueryService.go

@@ -79,7 +79,7 @@ func (service *SCQueryService) createVMCallInput(query *process.SCQuery, gasPric
 		CallerAddr:  query.ScAddress,
 		CallValue:   big.NewInt(0),
 		GasPrice:    gasPrice,
-		GasProvided: service.economicsFee.MaxGasLimitPerBlock(),
+		GasProvided: service.economicsFee.MaxGasLimitPerBlock(0),


It is ok to be harded here to shard 0 ?

it is a query service it does not matter.

SebastianMarian · 2020-05-21T12:26:30Z

process/smartContract/scQueryService.go

@@ -134,7 +134,7 @@ func (service *SCQueryService) ComputeScCallGasLimit(tx *transaction.Transaction
 		return 0, err
 	}

-	gasConsumed := service.economicsFee.MaxGasLimitPerBlock() - vmOutput.GasRemaining
+	gasConsumed := service.economicsFee.MaxGasLimitPerBlock(0) - vmOutput.GasRemaining


It is ok to be harded here to shard 0 ?

it is a query service - it does not matter.

SebastianMarian · 2020-05-21T12:30:54Z

vm/systemSmartContracts/auction.go

-				registrationData.BlsPubKeys[len(registrationData.BlsPubKeys)-1] = nil
-				registrationData.BlsPubKeys = registrationData.BlsPubKeys[:len(registrationData.BlsPubKeys)-1]
+				lastIndex := len(registrationData.BlsPubKeys) - 1
+				registrationData.BlsPubKeys[i] = registrationData.BlsPubKeys[lastIndex]


It is not a problem that you have changed the order of the registrationData.BlsPubKeys after the call of this method?

no - the order is not used in any place.

LucianMincu

System tests passed.

sasurobert added 5 commits May 20, 2020 13:45

small fix - do not save nodes which only got registered.

5e16007

return error

f6b4f56

fixed staking vs registering of nodes

ef77cbe

small fixes and a TODO

e0a65b4

test not needed for now

ed3d816

iulianpascalau previously approved these changes May 20, 2020

View reviewed changes

SebastianMarian reviewed May 20, 2020

View reviewed changes

sasurobert added 3 commits May 20, 2020 17:38

Merge branch 'development' into bugFix/save-only-staked-nodes

de2d4d5

test not needed for now

e63a779

Merge remote-tracking branch 'origin/bugFix/save-only-staked-nodes' i…

fd66707

…nto bugFix/save-only-staked-nodes

sasurobert dismissed iulianpascalau’s stale review via fd66707 May 20, 2020 14:50

iulianpascalau previously approved these changes May 20, 2020

View reviewed changes

SebastianMarian previously approved these changes May 20, 2020

View reviewed changes

LucianMincu added 3 commits May 20, 2020 19:22

Merge branch 'development' into bugFix/save-only-staked-nodes

0f2399f

Merge branch 'development' into bugFix/save-only-staked-nodes

ddcf8d8

Merge branch 'development' into bugFix/save-only-staked-nodes

81d8e4e

miiu96 closed this May 21, 2020

miiu96 deleted the bugFix/save-only-staked-nodes branch May 21, 2020 06:08

AdoAdoAdo restored the bugFix/save-only-staked-nodes branch May 21, 2020 06:14

iulianpascalau reopened this May 21, 2020

sasurobert added 2 commits May 21, 2020 10:40

small fixes.

aacf29c

Merge remote-tracking branch 'origin/bugFix/save-only-staked-nodes' i…

c6e7e25

…nto bugFix/save-only-staked-nodes

sasurobert dismissed stale reviews from SebastianMarian and iulianpascalau via c6e7e25 May 21, 2020 07:55

sasurobert and others added 5 commits May 21, 2020 10:57

Merge branch 'development' into bugFix/save-only-staked-nodes

e902e59

Merge branch 'development' into bugFix/save-only-staked-nodes

c793e2a

Merge branch 'development' into bugFix/save-only-staked-nodes

21f8ce9

add gas limit per metablock.

8d311c9

Merge remote-tracking branch 'origin/bugFix/save-only-staked-nodes' i…

87d3fb6

…nto bugFix/save-only-staked-nodes

iulianpascalau reviewed May 21, 2020

View reviewed changes

fix integration tests.

c9a6aee

iulianpascalau reviewed May 21, 2020

View reviewed changes

sasurobert added 2 commits May 21, 2020 14:15

fix after review.

3f31c38

fix after review.

f878581

SebastianMarian reviewed May 21, 2020

View reviewed changes

comments added

0bcb308

iulianpascalau approved these changes May 21, 2020

View reviewed changes

Merge branch 'development' into bugFix/save-only-staked-nodes

ce2cfd2

SebastianMarian reviewed May 21, 2020

View reviewed changes

SebastianMarian approved these changes May 21, 2020

View reviewed changes

Merge branch 'development' into bugFix/save-only-staked-nodes

0ba9ad6

LucianMincu approved these changes May 21, 2020

View reviewed changes

Merge branch 'development' into bugFix/save-only-staked-nodes

b74f6b5

iulianpascalau merged commit 8b7eb59 into development May 21, 2020

iulianpascalau deleted the bugFix/save-only-staked-nodes branch May 21, 2020 14:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bugFix/only-staked-nodes-to-validators #1771

bugFix/only-staked-nodes-to-validators #1771

sasurobert commented May 20, 2020

iulianpascalau May 20, 2020

sasurobert May 20, 2020

SebastianMarian May 20, 2020

sasurobert May 20, 2020

iulianpascalau May 21, 2020

iulianpascalau May 21, 2020

sasurobert May 21, 2020

SebastianMarian May 21, 2020

sasurobert May 21, 2020

SebastianMarian May 21, 2020

sasurobert May 21, 2020

SebastianMarian May 21, 2020

sasurobert May 21, 2020

SebastianMarian May 21, 2020

sasurobert May 21, 2020

SebastianMarian May 21, 2020

sasurobert May 21, 2020

SebastianMarian May 21, 2020

sasurobert May 21, 2020

SebastianMarian May 21, 2020

sasurobert May 21, 2020

SebastianMarian May 21, 2020

sasurobert May 21, 2020

SebastianMarian May 21, 2020

sasurobert May 21, 2020

SebastianMarian May 21, 2020

sasurobert May 21, 2020

SebastianMarian May 21, 2020

sasurobert May 21, 2020

SebastianMarian May 21, 2020

sasurobert May 21, 2020

LucianMincu left a comment

		@@ -22,7 +22,7 @@ func (fh *FeeHandler) MinGasPrice() uint64 {
		}

		// MaxGasLimitPerBlock return max uint64

		@@ -12,6 +12,7 @@ type TestEconomicsData struct {
		// SetMaxGasLimitPerBlock sets the maximum gas limit allowed per one block
		func (ted *TestEconomicsData) SetMaxGasLimitPerBlock(maxGasLimitPerBlock uint64) {

bugFix/only-staked-nodes-to-validators #1771

bugFix/only-staked-nodes-to-validators #1771

Conversation

sasurobert commented May 20, 2020

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

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

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

LucianMincu left a comment

Choose a reason for hiding this comment