Fix removing voting power during expiry #218

KonradStaniec · 2024-10-22T12:09:11Z

fixes: #176

Currently our unbonding during expiry is not in line with on demand unbonding and slashing time lock i.e

when unbonding on demand happens, voting power of the staker is removed immediately but his unbonding output is still locked for UnbondingTime blocks
when slashing happens, voting power of the staker is removed immediately but his change output is still locked for UnbondingTime
in contrast, in expiry case, voting power is removed at last possible moment i.e CheckpointFInalizationTimeout blocks before expiration.

This pr:

changes expiry case to remove voting power of the staker UnbondingTime blocks before expiry. With this change, UnbondingTime becomes time that stake is still locked on BTC chain even though it does not have voting power on Babylon
introduces MaxUnbondingTime, which is defined as StakingTime - CheckpointFinalizationTimeout, this is necessary to avoid creating delegation which will never gain voting power, as they are expired during creation.

SebastianElvis · 2024-10-23T00:49:53Z

proto/babylon/btcstaking/v1/tx.proto

@@ -108,7 +108,7 @@ message MsgCreateBTCDelegation {
  // - unbonding transaction, time lock spending path
  // - staking slashing transaction, change output
  // - unbonding slashing transaction, change output
-  // It must be smaller than math.MaxUInt16 and larger that max(MinUnbondingTime, CheckpointFinalizationTimeout)
+  // It must be smaller than (staking_time - CheckpointFinalizationTimeout) and larger that max(MinUnbondingTime, CheckpointFinalizationTimeout)


Suggested change

// It must be smaller than (staking_time - CheckpointFinalizationTimeout) and larger that max(MinUnbondingTime, CheckpointFinalizationTimeout)

// It must be smaller than (staking_time - CheckpointFinalizationTimeout) and larger than max(MinUnbondingTime, CheckpointFinalizationTimeout)

Also is "be smaller than (staking_time - CheckpointFinalizationTimeout)" part enforced in ValidateParsedMessageAgainstTheParams?

SebastianElvis · 2024-10-23T00:53:28Z

x/btcstaking/keeper/inclusion_proof.go

 // VerifyInclusionProofAndGetHeight verifies the inclusion proof of the given staking tx
-// and returns the inclusion height
+// and returns the inclusion height as well as endheigh


Suggested change

// and returns the inclusion height as well as endheigh

// and returns the start height and end height

SebastianElvis · 2024-10-23T00:57:27Z

proto/babylon/btcstaking/v1/tx.proto

@@ -108,7 +108,7 @@ message MsgCreateBTCDelegation {
  // - unbonding transaction, time lock spending path
  // - staking slashing transaction, change output
  // - unbonding slashing transaction, change output
-  // It must be smaller than math.MaxUInt16 and larger that max(MinUnbondingTime, CheckpointFinalizationTimeout)
+  // It must be smaller than (staking_time - CheckpointFinalizationTimeout) and larger that max(MinUnbondingTime, CheckpointFinalizationTimeout)


Also is "be smaller than (staking_time - CheckpointFinalizationTimeout)" part enforced in ValidateParsedMessageAgainstTheParams?

SebastianElvis · 2024-10-23T01:03:10Z

x/btcstaking/keeper/inclusion_proof.go

+	// ensure staking tx's timelock has more than unbonding BTC blocks left
+	if btcTip.Height+unbondingTime >= endHeight {
+		return nil, types.ErrInvalidStakingTx.Wrapf("staking tx's timelock has no more than unbonding(=%d) blocks left", unbondingTime)
 	}


I'm actually not sure why we should remove BTC delegation's voting power unbondingTime before the timelock ends, rather than w before the timelock ends.

The reasong of removing a BTC delegation when there is w block left is that anything happens in BTC will be forwarded to Babylon within w BTC blocks. This is the security assumption of the system, and we have the vigilante monitor checking whether this w assumption has been broken or not. So as long as this assumption holds, the current approach of handling unbonding due to timelock is secure. I do not see the benefit of removing a BTC delegation when there are unbondingTime blocks left in the timelock, where unbondingTime > w.

In fact, any rational BTC staker will choose w+1 as unbonding time, as long as we don't use a big min_unbonding_time_blocks > w. IIRC min_unbonding_time_blocks is introduced due to the phase-1 system, but in phase-2 there is little benefit to have min_unbonding_time_blocks bigger than w.

So there are two things or (even three things) at play here:

we had min_unbonding_time_blocks in phase-2 even before phase-1. Main rationale for it was that , we wanted to have control over unbonding time.

we decided to keep it as min_unbonding_time_blocks, not exact unbonding_time_blocks to make it possible for self delegating finality providers to chose larger unbonding time to inspire trust in stakers i.e our prevention mechanism against selective slashing works only if finality provider have something at stake. If everybody have the same unbonding time then it is possible that:

finality provider self delegates a lot, to gather trust

finality provider gets lots of delegations

finality provider unbonds self delegated stake

the stakers seeing that will do the same

but due to finality provider making first move, his self-delegated stake will be unbonded first

finality provider selectively slash all stakers that did not manage to unbond
So yep, most of the stakers will chose unbonding time as min_unbonding_time_blocks+1, but self delegating finality providers may choose larger time.

As for:

I do not see the benefit of removing a BTC delegation when there are unbondingTime blocks left in the timelock, where unbondingTime > w.

There are two benefits:

system consistency - in any other unbonding (either due to unbonding on demand, or slashing) we remove voting power unbonding_time_blocks before staker is unlocked on BTC. At this point, expiry case is only exception to this rule.

(something I vaguely remember we discussed in hong-kong, should describe it better in ticket) Imagine finality provider have a lot of delegations nearing expiry (10000 or more), now finality provider double votes. We have 10000 slashing transactions to send to BTC and process.
Now:

if we remove voting power only w blocks before expiry this means we could have only w blocks to do that

if we remove voting power unbonding_time_blocks before expiry, this means we have unbonding_time_blocks which is possibly larger value and more time to absorb this many slashing transaction.

we decided to keep it as min_unbonding_time_blocks, not exact unbonding_time_blocks to make it possible for self delegating finality providers to chose larger unbonding time to inspire trust in stakers i.e our prevention mechanism against selective slashing works only if finality provider have something at stake. If everybody have the same unbonding time then it is possible that:

Good point on FP gathering trust by having self delegations with long unbonding time. Although this is a bit new to me -- is there any relevant discussion or documented somewhere?

system consistency - in any other unbonding (either due to unbonding on demand, or slashing) we remove voting power unbonding_time_blocks before staker is unlocked on BTC. At this point, expiry case is only exception to this rule.

Tbh I'm not sure about the end goal of having such consistency. One downside of having this is that by specifying a bigger unbonding period, its staking period also gets shorter, which does not seem expected.

We have 10000 slashing transactions to send to BTC and process.
Now:
if we remove voting power only w blocks before expiry this means we could have only w blocks to do that
if we remove voting power unbonding_time_blocks before expiry, this means we have unbonding_time_blocks which is > possibly larger value and more time to absorb this many slashing transaction.

Yes, but this only makes BTC delegations with big unbonding time secure, but not necessarily all BTC delegations. In fact, if this hypothetical scenario is possible, then we will have to choose a substantially bigger value than w to secure the system, and expire BTC delegations when less than that amount of BTC blocks left. This can be min_unbonding_time_blocks or some new parameters, but not necessarily the staker-specified unbonding_time

Good point on FP gathering trust by having self delegations with long unbonding time. Although this is a bit new to me -- is there any relevant discussion or documented somewhere?

Ahh this is something from long ago, I do not think we have any write up. Though that was the main reason, why we have UnboningTime chosen by user, and MinUnbondingTime as system parameter, and not system mandated UnbondingTime

One downside of having this is that by specifying a bigger unbonding period, its staking period also gets shorter, which does not seem expected.

Agree this is a bit weird, but this is chosen by user and most users will chose lowest possible unbonding time.
And this is actually in line with consistent definition of UnbondingTime after this change i.e time during which stake is locked on BTC, but do not have voting power on Babylon.

Yes, but this only makes BTC delegations with big unbonding time secure, but not necessarily all BTC delegations. In fact, if this hypothetical scenario is possible, then we will have to choose a substantially bigger value than w to secure the system, and expire BTC delegations when less than that amount of BTC blocks left. This can be min_unbonding_time_blocks or some new parameters, but not necessarily the staker-specified unbonding_time

Tbh I imagine we will start with min_unbonding_time_blocks set to something around the value we have on phase-1 so it will be considerably larger than w (which makes sense, as w is actually bare minimum required, and relaying on minimum is risky), like 1000blocks.

As for new parameters, why introduce some other parameters when approach in this pr (in combination with existing parameters):

enable flexible unbonding time for who ever want it (most probably self-delegating finality providers)

makes definition of the user chosen UnbondingTime consistent across different transactions and flows

make is possible to define min_unbonding_time_blocks which will be large enough, to cover for cases when there is a lot of slashing transactions that must hit BTC network.

OK now I remember why the unbonding time is w but not something much bigger. This is because Babylon has BTC timestamping where the chain is expected to be BTC timestamped within ~10 hours. This is much shorter than w BTC blocks. Any equivocation after the w unbonding period won't break safety at all, so it doesn't matter whether these BTC stake can be slashed. So any unbonding period no shorter than w achieves the same (slashable) safety guarantee.

But you are right that the unbonding time is now relevant to the staker security property, due to the another attack vector of selective slashing. Basically the community needs to have sufficient time to react when a FP massively unbonds its self staked BTC, because it can do selective slashing without its own BTC being slashed. In this case, I agree that we should allow BTC stakers to choose its own unbonding time.

Do you think it's necessary to apply this unbonding time to the timelock expiry as well? Basically once staking tx is on BTC, everyone can know when a BTC delegation will expire. The community can see how many BTC blocks left for the timelock path of self staked BTC. When to remove their voting power does not seem to matter.

Another question is whether we need to enforce a min_unbonding_time_blocks that is much bigger than w. According to the above analysis it does not seem to be the case. For slashable safety, we have w. For staker security FPs can choose to gain trust by having very big unbonding time.

Any equivocation after the w unbonding period won't break safety at all, so it doesn't matter whether these BTC stake can be slashed. So any unbonding period no shorter than w achieves the same (slashable) safety guarantee.

Could you expand on this ?

Due to this discussion, I kind of remembered why we wanted to have this larger unbonding time. Currently there are few problems with our slashing:

We cannnot fee bump our slashing transactions

Every delegation -> one slashing transaction

FPs do not have limits on delegations

So, now imagine de-generate case of finality provider who created 100k super small self-delegations and waits for the last possible moment to double sign. If we remove voting power w before the timelock ends then we have only w blocks to send all those slashing transactions to BTC, but if we remove voting power unbonding_time blocks before we have unbonding_time blocks, which may be much larger. (the same dynamic plays when we have unbonding on demand i.e fp double vote and unbonds all delegations, we have unbonding_time blocks to slash them)

So unbonding_time effectively controls how much time we have to send slashing transactions to BTC in the worst case scenario.

So given the the goal of having unbonding_time larger than w(so min_unbonding_time_blocks parameter) imo it should cover both cases:

expiry case

unbonding on demand
in same fashion.

Another question all together is that maybe we should have limit on number of delegations per finality provider (I think we should) but I remember there was some push back against this.

KonradStaniec · 2024-10-24T10:36:49Z

According to discussion with @SebastianElvis it is enough for voting power to expire max(w, min_unbonding_time) blocks before timelock finishes. Modified pr according to this discussion.

SebastianElvis

Thanks for all the discussion!

SebastianElvis · 2024-10-24T23:37:05Z

x/btcstaking/keeper/inclusion_proof.go

 	inclusionProof *types.ParsedProofOfInclusion,
-) (uint32, error) {
-	btccParams := k.btccKeeper.GetParams(ctx)
+) (*delegationTimeRangeInfo, error) {


How about

Suggested change

) (*delegationTimeRangeInfo, error) {

) (uint32, uint32, error) {

as this object is unpacked to two uint32 later anyway?

So this may be a personal preference, but:

I really dislike returning more that 2 values from one function, it makes is hard to read and call.

(uint32, uint32, error) does not tell you much what those uint32 represent while in returned struct those numbers have proper names startHeight uint32 which makes life easier for the caller

SebastianElvis · 2024-10-24T23:39:08Z

x/btcstaking/keeper/msg_server.go

 	// add this BTC delegation, and emit corresponding events
-	if err := ms.AddBTCDelegation(ctx, newBTCDel); err != nil {
+	if err := ms.AddBTCDelegation(ctx, newBTCDel, paramsValidationResult.MinUnbondingTime); err != nil {
 		panic(fmt.Errorf("failed to add BTC delegation that has passed verification: %w", err))
 	}


Tbh it's a bit cleaner to not have the MinUnbondingTime as a parameter in this function, but if we retrieve it again from DB inside AddBTCDelegation then it's more DB footprint 😅

agree it is a bit cleaner, but imo it is not that much cleaner to be worth two additional database calls 😅

KonradStaniec requested a review from SebastianElvis October 22, 2024 12:16

KonradStaniec added the consensus breaking change modifies `appHash` of the application label Oct 22, 2024

KonradStaniec marked this pull request as ready for review October 22, 2024 12:19

KonradStaniec requested a review from a team as a code owner October 22, 2024 12:19

KonradStaniec requested review from RafilxTenfen and removed request for a team October 22, 2024 12:19

SebastianElvis reviewed Oct 23, 2024

View reviewed changes

SebastianElvis approved these changes Oct 24, 2024

View reviewed changes

KonradStaniec added 6 commits October 25, 2024 10:21

Fix removing voting power during expiry

9c5beca

Add change log entry

dbd16e4

Update change log

8e30303

expire stake max(w, min_unbonding_time) before timelock ends

7dae2e6

refactor

65bcd08

fix changelog after rebase

f7c0cee

KonradStaniec force-pushed the konradstaniec/fix-expiry-unbonding branch from 936c0c8 to f7c0cee Compare October 25, 2024 08:24

KonradStaniec merged commit 0caa9be into main Oct 25, 2024
20 checks passed

KonradStaniec deleted the konradstaniec/fix-expiry-unbonding branch October 25, 2024 09:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix removing voting power during expiry #218

Fix removing voting power during expiry #218

KonradStaniec commented Oct 22, 2024 •

edited

Loading

SebastianElvis Oct 23, 2024

SebastianElvis Oct 23, 2024

SebastianElvis Oct 23, 2024 •

edited

Loading

SebastianElvis Oct 23, 2024

SebastianElvis Oct 23, 2024

SebastianElvis Oct 23, 2024 •

edited

Loading

KonradStaniec Oct 23, 2024

SebastianElvis Oct 23, 2024

KonradStaniec Oct 23, 2024

SebastianElvis Oct 24, 2024 •

edited

Loading

KonradStaniec Oct 24, 2024

KonradStaniec commented Oct 24, 2024

SebastianElvis left a comment

SebastianElvis Oct 24, 2024

KonradStaniec Oct 25, 2024

SebastianElvis Oct 24, 2024

KonradStaniec Oct 25, 2024

	// It must be smaller than (staking_time - CheckpointFinalizationTimeout) and larger that max(MinUnbondingTime, CheckpointFinalizationTimeout)
	// It must be smaller than (staking_time - CheckpointFinalizationTimeout) and larger than max(MinUnbondingTime, CheckpointFinalizationTimeout)

	// and returns the inclusion height as well as endheigh
	// and returns the start height and end height

	) (*delegationTimeRangeInfo, error) {
	) (uint32, uint32, error) {

Fix removing voting power during expiry #218

Fix removing voting power during expiry #218

Conversation

KonradStaniec commented Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SebastianElvis Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SebastianElvis Oct 23, 2024 • edited Loading

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

SebastianElvis Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KonradStaniec commented Oct 24, 2024

SebastianElvis left a 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

KonradStaniec commented Oct 22, 2024 •

edited

Loading

SebastianElvis Oct 23, 2024 •

edited

Loading

SebastianElvis Oct 23, 2024 •

edited

Loading

SebastianElvis Oct 24, 2024 •

edited

Loading