hotfix: Invalid minUnbondingTime for verifying inclusion proof #289
Conversation
gitferry
requested review from
KonradStaniec and
RafilxTenfen
and removed request for
a team
|
I am a little bit confused:
Why do we not use the latest parameter to update the voting power dist as well? |
x/btcstaking/keeper/msg_server.go
Outdated
| @@ -315,7 +314,6 @@ func (ms msgServer) AddBTCDelegationInclusionProof( | |||
| btcutil.NewTx(stakingTx), | |||
| btccParams.BtcConfirmationDepth, | |||
| btcDel.StakingTime, | |||
| minUnbondingTime, | |||
There was a problem hiding this comment.
I think @RafilxTenfen comment is valid here - #289 (comment), ie we should pass here the params.MinUnbondingTimeBlocks from the params used when creating the delegation, not retrieve latest one from the store inside the function
gitferry
force-pushed
the
hotfix/set-min-staking-time-for-unbonding
branch
from
56f7516
to
85689cb
Compare
|
@RafilxTenfen @KonradStaniec good point, thanks guys! I forgot we are using versioned params. Fixed in 85689cb. Added a note there and relevant test |
| @@ -215,14 +215,14 @@ func (ms msgServer) CreateBTCDelegation(goCtx context.Context, req *types.MsgCre | |||
| btcutil.NewTx(parsedMsg.StakingTx.Transaction), | |||
| btccParams.BtcConfirmationDepth, | |||
| uint32(parsedMsg.StakingTime), | |||
| vp.Params.MinStakingTimeBlocks, | |||
There was a problem hiding this comment.
could you double check the tests ?
I have changed this back locally to vp.Params.MinStakingTimeBlocks and run the tests, and they did not catch this.
There was a problem hiding this comment.
Oh that's because the tests are for VerifyInclusionProofAndGetHeight. Seems we need to have a special case for testing CreateBTCDelegation
There was a problem hiding this comment.
In d729190 I added a happy case to TestDoNotAllowDelegationWithoutFinalityProvider so that if you change vp.Params.MinStakingTimeBlocks, the test won't pass. But I don't think it's comprehensive enough. We can create an issue to add comprehensive test for CreateBTCDelegation
There was a problem hiding this comment.
I also noticed that we don't have a validation rule to ensure minStakingTime > minUnbondingTime in params. I think this rule is important. Otherwise, the verification will never pass if the user choose to stake with minStakingTime:
// ensure staking tx's timelock has more than unbonding BTC blocks left
if btcTip.Height+minUnbondingTime >= endHeight {
return nil, types.ErrInvalidStakingTx.
Wrapf("staking tx's timelock has no more than unbonding(=%d) blocks left", minUnbondingTime)
}
There was a problem hiding this comment.
But I don't think it's comprehensive enough. We can create an issue to add comprehensive test for CreateBTCDelegation
Sure, lets create issue for this 👍
The bug is caused due to setting `params.MinStakingTime` to the btcstaking module's keeper method `VerifyInclusionProofAndGetHeight` other than `params.MinUnbondingTime`. In the fix, we remove `minUnbondingTime` from the parameter list of `VerifyInclusionProofAndGetHeight` as it should retrieve the parameter within the method. This PR also added comprehensive tests for `VerifyInclusionProofAndGetHeight`
…289) (#290) The bug is caused due to setting `params.MinStakingTime` to the btcstaking module's keeper method `VerifyInclusionProofAndGetHeight` other than `params.MinUnbondingTime`. In the fix, we remove `minUnbondingTime` from the parameter list of `VerifyInclusionProofAndGetHeight` as it should retrieve the parameter within the method. This PR also added comprehensive tests for `VerifyInclusionProofAndGetHeight`
The bug is caused due to setting
params.MinStakingTimeto the btcstaking module's keeper methodVerifyInclusionProofAndGetHeightother thanparams.MinUnbondingTime. In the fix, we removeminUnbondingTimefrom the parameter list ofVerifyInclusionProofAndGetHeightas it should retrieve the parameter within the method.This PR also added comprehensive tests for
VerifyInclusionProofAndGetHeight