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

Ignore staking queue #3861

Merged
merged 23 commits into from Mar 18, 2022

Conversation

mariusmihaic
Copy link
Contributor

@mariusmihaic mariusmihaic commented Mar 2, 2022

  • Added staking v4 flag in staking.go which has been used to deactivate functions which will be obsolete since staking v4 is enabled. It has been mostly used to "inhibit" staking queue/ waiting list.
  • Added unit tests

@mariusmihaic mariusmihaic changed the title En 116622 ignore staking queue v1 Ignore staking queue v1 Mar 2, 2022
@mariusmihaic mariusmihaic changed the title Ignore staking queue v1 Ignore staking queue Mar 2, 2022
s.eei.AddReturnMessage(vm.ErrWaitingListDisabled.Error())
s.eei.Finish([]byte{0})

return vmcommon.Ok
Copy link
Contributor

Choose a reason for hiding this comment

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

if you have return message than return vmcommon.UserError, and do not add to Finish anything. as it is ignored

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for heads up, refactored it

@@ -1353,6 +1381,13 @@ func (s *stakingSC) getWaitingListIndex(args *vmcommon.ContractCallInput) vmcomm
}

func (s *stakingSC) getWaitingListSize(args *vmcommon.ContractCallInput) vmcommon.ReturnCode {
if s.flagStakingV4.IsSet() {
s.eei.AddReturnMessage(vm.ErrWaitingListDisabled.Error())
s.eei.Finish([]byte{0})
Copy link
Contributor

Choose a reason for hiding this comment

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

if you have return message than return vmcommon.UserError, and do not add to Finish anything. as it is ignored

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for heads up, refactored it

@mariusmihaic mariusmihaic self-assigned this Mar 8, 2022
Base automatically changed from EN-11662-filter-new-nodes to feat/liquid-staking March 9, 2022 17:28
@mariusmihaic mariusmihaic marked this pull request as ready for review March 11, 2022 14:12
@ssd04 ssd04 self-requested a review March 15, 2022 12:16
stakingSmartContract.EpochConfirmed(args.EpochConfig.EnableEpochs.StakingV4EnableEpoch, 0)

arguments := CreateVmContractCallInput()
arguments.Arguments = [][]byte{}
Copy link
Contributor

Choose a reason for hiding this comment

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

is this needed? i think it can be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, can be removed. I've put it there so I don't pass nil pointers

doStake(t, stakingSmartContract, stakingAccessAddress, addr, addr)
checkIsStaked(t, stakingSmartContract, addr, addr, vmcommon.Ok)
}
requireRegisteredNodes(t, stakingSmartContract, eei, 14, 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

here we have 14 staked nodes, but we MaxNumberOfNodesForStake = 4, or we are not taking this into account in this test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the expected behavior. Once staking v4 is enabled, we can stake any number of nodes, without limit.

On L1007 you can see that after trying to stake 10 nodes, we expect to have: 4 staked nodes + 6 waiting. (because MaxNumberOfNodesForStake = 4)

Here, on L1026 after flagStakingV4 is enabled, waiting queue is removed => all 14 nodes are staked

Copy link
Contributor

Choose a reason for hiding this comment

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

ok 👍 thanks

ssd04
ssd04 previously approved these changes Mar 16, 2022
@codecov
Copy link

codecov bot commented Mar 17, 2022

Codecov Report

Merging #3861 (0023066) into feat/liquid-staking (0b413f3) will decrease coverage by 0.49%.
The diff coverage is 74.57%.

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

@@                   Coverage Diff                   @@
##           feat/liquid-staking    #3861      +/-   ##
=======================================================
- Coverage                74.99%   74.49%   -0.50%     
=======================================================
  Files                      614      606       -8     
  Lines                    81639    80533    -1106     
=======================================================
- Hits                     61223    59991    -1232     
- Misses                   15755    15933     +178     
+ Partials                  4661     4609      -52     
Impacted Files Coverage Δ
dataRetriever/resolvers/headerResolver.go 92.81% <ø> (ø)
factory/apiResolverFactory.go 0.00% <0.00%> (-75.38%) ⬇️
genesis/process/shardGenesisBlockCreator.go 40.07% <0.00%> (-0.22%) ⬇️
node/nodeRunner.go 0.00% <0.00%> (ø)
p2p/libp2p/netMessenger.go 75.00% <ø> (-3.87%) ⬇️
process/block/metablock.go 60.19% <0.00%> (-0.04%) ⬇️
process/block/postprocess/intermediateResults.go 86.00% <ø> (+2.02%) ⬆️
...cess/block/preprocess/validatorInfoPreProcessor.go 53.24% <0.00%> (-0.71%) ⬇️
process/interceptors/multiDataInterceptor.go 90.14% <ø> (ø)
storage/pruning/pruningStorer.go 71.37% <0.00%> (-1.04%) ⬇️
... and 99 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 0b413f3...8badd35. Read the comment docs.

@mariusmihaic mariusmihaic force-pushed the EN-116622-ignore-staking-queue-v1 branch from 8badd35 to 779733d Compare March 17, 2022 08:50
s.eei.AddReturnMessage(err.Error())
return vmcommon.UserError
if !s.flagStakingV4.IsSet() {
addOneFromQueue := !s.flagCorrectLastUnjailed.IsSet() || s.canStakeIfOneRemoved()
Copy link
Contributor

Choose a reason for hiding this comment

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

this file will need one big cleanup - like stakingV2 separate file and stakingV4 separate. Add a task in JIRA and we will see later.

@mariusmihaic mariusmihaic merged commit 5a61ee6 into feat/liquid-staking Mar 18, 2022
@mariusmihaic mariusmihaic deleted the EN-116622-ignore-staking-queue-v1 branch March 18, 2022 11:29
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.

None yet

3 participants