feat(ADR-25): Remove voting power from jailed finality providers #65

gitferry · 2024-09-10T14:38:46Z

This PR partially closes #64 by jailing sluggish finality providers by not assigning voting power to them in the voting power table. In particular, this PR made the following changes:

Change the sluggish label of a finality provider to jailed.
Implement AfterSluggishFinalityProviderDetected hook in the x/btcstaking module to set the label of a finality provider to jailed.
Introduce EventJailedFinalityProvider event as EventPowerDistUpdate, which is emitted after a finality provider is jailed.
Introduce a new parameter jail_duration to x/finality (needs to handle upgrade in a separate PR).
Introduce a new field jailed_until to FinalityProviderSigningInfo in x/finality, which is calculated as the timestamp of the jailing block plus the jail_duration of the parameter.
Clear the signing info record of the jailed finality provider and set jailed_until=jailingBlock.Time.Add(params.JailDuration).
Within the BeginBlocker of the next block, upon EventJailedFinalityProvider, record the finality provider public key and do not assign voting power to it.
Modify relevant tests

maurolacy

LGTM. Just some questions, and suggestion for default jailing duration.

Agree with removing the distinction between sluggish and jailed, as they are basically the same state (i.e. sluggish => jailed).

I've seen that in some impls jailed and slashed are also, not exactly the same state, but implied (slashed => jailed). This can be done as well in a follow-up, as it might simplify the code paths, checks, etc. The only requirement would be, slashed => jailed with jailing duration = forever, i.e. you cannot unjail after slashing.

maurolacy · 2024-09-11T06:04:39Z

x/epoching/keeper/msg_server.go

@@ -242,7 +243,7 @@ func (ms msgServer) WrappedCancelUnbondingDelegation(goCtx context.Context, msg
 		)
 	}

-	blockHeight := uint64(ctx.BlockHeader().Height)
+	blockHeight := uint64(ctx.HeaderInfo().Height)


What is the difference here? Just curious.

ctx.BlockHeader() returns ctx.header which is deprecated. We should use HeaderInfo instead. See https://github.com/cosmos/cosmos-sdk/blob/698627016d64dbf33bea01837364c5a6e55a1ee3/types/context.go#L45

maurolacy · 2024-09-11T06:05:10Z

x/btcstaking/keeper/power_dist_change.go

@@ -150,15 +150,14 @@ func (k Keeper) ProcessAllPowerDistUpdateEvents(
 	ctx context.Context,
 	dc *types.VotingPowerDistCache,
 	events []*types.EventPowerDistUpdate,
-	maxActiveFps uint32,


Is this max being enforced somewhere else? Just curious.

Yep, here

babylon/x/btcstaking/keeper/power_dist_change.go

Line 75 in 0a90a5e

k.recordVotingPowerAndCache(ctx, dc, newDc, maxActiveFps)

.

maurolacy · 2024-09-11T06:07:48Z

x/finality/keeper/liveness.go

+		return fmt.Errorf("failed to emit sluggish finality provider detected event: %w", err)
+	}
+
+	finalitytypes.IncrementSluggishFinalityProviderCounter()


Checked and couldn't find the call to the corresponding Decrement call. Perhaps just remove this counter?

Nice spot! Decrement part will be called in the implementation of unjailing, happening in the next PR. Nevertheless, we probably should not use sluggish here anymore. Fixed in 4452192

maurolacy · 2024-09-11T06:11:40Z

x/finality/types/params.go

@@ -13,6 +14,7 @@ const (
 	DefaultSignedBlocksWindow = int64(100)
 	DefaultMinPubRand         = 100
 	DefaultFinalitySigTimeout = 3
+	DefaultJailDuration       = 60 * 10 * time.Second // 10 min


This looks like too little to me. 24 hours or so sounds reasonable. So that jailing:

Is a punishment in itself for being offline.

Gives some time to the operator to properly restore service / recover liveness.

This was copied from Cosmos SDK but yeah I agree that 1 day seems more reasonable. Fixed in 2caa4af

maurolacy · 2024-09-11T07:05:05Z

x/finality/types/metrics.go

@@ -63,10 +63,10 @@ func IncrementSluggishFinalityProviderCounter() {
 	)
 }

-// DecrementSluggishFinalityProviderCounter increments the counter for the sluggish
+// DecrementJailedFinalityProviderCounter increments the counter for the jailed


Suggested change

// DecrementJailedFinalityProviderCounter increments the counter for the jailed

// DecrementJailedFinalityProviderCounter decrements the counter for the jailed

proto/babylon/btcstaking/v1/events.proto

proto/babylon/finality/v1/finality.proto

RafilxTenfen

LGTM, only some suggestions

Lazar955

Nice work 🚀

SebastianElvis

Great work! Mostly questions

proto/babylon/btcstaking/v1/events.proto

x/btcstaking/keeper/hooks.go

SebastianElvis · 2024-09-12T01:53:30Z

x/finality/keeper/liveness.go

+	err := k.hooks.AfterSluggishFinalityProviderDetected(ctx, fpBtcPk)
+	if err != nil {
+		return err
+	}


How about we call k.BTCStakingKeeper.JailFinalityProvider directly? From what I see the AfterSluggishFinalityProviderDetected hook is only used here and its only functionality is to call JailFinalityProvider. Removing this hook relationship makes the code easier to understand

We would have slashing reward logic in the future. So better to have a hook where the incentive module can also implement it?

We can add the logic of slashing rewards into JailFinalityProvider, as removing FP voting power and slashing its reward always need to be done at the same time. No strong opinion on this though

Agreed that jailing and slashing rewards should happen at the same time, so we can achieve this by having both modules implement the same hook. Some discussion about why we decided to use a hook.

Sure. In fact x/finality module has access to x/incentive so it can invoke the incentive module to slash the reward. But no problem if you think this is better done in a hook

proto/babylon/finality/v1/finality.proto

x/finality/keeper/liveness.go

SebastianElvis · 2024-09-13T05:12:17Z

x/btcstaking/keeper/power_dist_change.go

+		fp, err := k.GetFinalityProvider(ctx, fpDistInfo.BtcPk.MustMarshal())
+		if err != nil {
+			panic(fmt.Errorf("the finality provider %s does not exist: %w", fp.BtcPk.MarshalHex(), err))
+		}
+		fpDistInfo.IsJailed = fp.IsJailed()


This will cause some overhead imo -- upon every new block computing the new voting power table will need to retrieve all FPs in the DB, leading to O(n) overhead in DB read where n is the number of FPs. This was not the case where the DB overhead is O(# of BTC delegations with state updates).

Since we will have MsgUnjail anyway, how about we do the state update using the event-based approach similar to the existing events? That is,

Add a field IsJailed to FinalityProviderDistInfo

Upon jailing a FP, push a jailed event to the event queue

Upon MsgUnjail is approved, push an unjailed event to the event queue

ProcessAllPowerDistUpdateEvents processes all the corresponding jailed/unjailed events and sets IsJailed for corresponding FinalityProviderDistInfo objects

Nice point! Chaned to setting IsJailed to FinalityProviderDistInfo upon event processing and will have UnjailEvent to revert the flag in the subsequent PR

SebastianElvis

Great work!

SebastianElvis · 2024-09-16T01:59:28Z

x/btcstaking/types/btcstaking.go

 		if fps[i].IsTimestamped && !fps[j].IsTimestamped {
 			return true
 		}
 		if !fps[i].IsTimestamped && fps[j].IsTimestamped {
 			return false
 		}
+		if !fps[i].IsJailed && fps[j].IsJailed {
+			return true
+		}
+		if fps[i].IsJailed && !fps[j].IsJailed {
+			return false
+		}


How about computing a bool value zeroVotingPower := fps[i].IsJailed || !fps[i].IsTimestamped and then do the comparison?

That would be cleaner. Fixed in a871d44

SebastianElvis · 2024-09-16T02:00:56Z

x/btcstaking/types/incentive.go

 		if !fp.IsTimestamped {
 			break
 		}
+		if fp.IsJailed {
+			break
+		}


Given all these new jailed/timestamped concepts, I started to feel TotalVotingPower is no longer an accurate name. How about we rename it to sth like TotalBTCStake? Not for this PR, just some thought

Good point. Tracked in #72. Will fix it in a separate PR

Closes #64. TODOs: - [x] #65 - [x] #74 - [x] babylonlabs-io/finality-provider#56 - [x] #81 We still need to update the docs, add CLI cmd, and e2e tests but can be done in separate PRs

gitferry added 7 commits September 10, 2024 22:37

change sluggish to jailed

9f1bde2

add proto type change and jailing logic

0a90a5e

fix test

890fab3

reset missed block counter and bit map

5aa71a5

add test jail finality provider

1d3334f

modify liveness test

7ec426d

add reject votes from jailed finality provider

fe7bac6

gitferry marked this pull request as ready for review September 11, 2024 05:03

gitferry requested a review from a team as a code owner September 11, 2024 05:03

gitferry requested review from KonradStaniec, RafilxTenfen and SebastianElvis September 11, 2024 05:03

maurolacy approved these changes Sep 11, 2024

View reviewed changes

maurolacy reviewed Sep 11, 2024

View reviewed changes

rename some sluggish to jailed

5b46ebb

gitferry force-pushed the gai/jail-sluggish-fps branch from 4452192 to 5b46ebb Compare September 11, 2024 07:06

increase default jail dur to 1 day

2caa4af

gitferry requested a review from Lazar955 September 11, 2024 07:32

RafilxTenfen reviewed Sep 11, 2024

View reviewed changes

proto/babylon/btcstaking/v1/events.proto Show resolved Hide resolved

RafilxTenfen reviewed Sep 11, 2024

View reviewed changes

proto/babylon/finality/v1/finality.proto Show resolved Hide resolved

RafilxTenfen reviewed Sep 11, 2024

View reviewed changes

pr comments

10f04f3

RafilxTenfen approved these changes Sep 11, 2024

View reviewed changes

Lazar955 approved these changes Sep 11, 2024

View reviewed changes

SebastianElvis reviewed Sep 12, 2024

View reviewed changes

fix jailed fp get voting power in next delegation

a1b253d

gitferry force-pushed the gai/jail-sluggish-fps branch from 8f9cf72 to a1b253d Compare September 12, 2024 09:55

SebastianElvis reviewed Sep 13, 2024

View reviewed changes

fix setting isJailed

3cdf04b

SebastianElvis approved these changes Sep 16, 2024

View reviewed changes

gitferry mentioned this pull request Sep 16, 2024

TotalVotingPower is not accurate in FinalityProviderDistInfo #72

Closed

pr comments

a871d44

gitferry merged commit 2308deb into feat/adr-25-jailing Sep 16, 2024
17 checks passed

gitferry deleted the gai/jail-sluggish-fps branch September 16, 2024 02:46

gitferry mentioned this pull request Sep 18, 2024

feat(ADR-025): Enable jailing and unjailing #80

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ADR-25): Remove voting power from jailed finality providers #65

feat(ADR-25): Remove voting power from jailed finality providers #65

gitferry commented Sep 10, 2024 •

edited

Loading

maurolacy left a comment •

edited

Loading

maurolacy Sep 11, 2024

gitferry Sep 11, 2024

maurolacy Sep 11, 2024

gitferry Sep 11, 2024

maurolacy Sep 11, 2024 •

edited

Loading

gitferry Sep 11, 2024

maurolacy Sep 11, 2024 •

edited

Loading

gitferry Sep 11, 2024

maurolacy Sep 11, 2024

RafilxTenfen left a comment

Lazar955 left a comment

SebastianElvis left a comment

SebastianElvis Sep 12, 2024

gitferry Sep 12, 2024 •

edited

Loading

SebastianElvis Sep 12, 2024

gitferry Sep 12, 2024

SebastianElvis Sep 13, 2024

SebastianElvis Sep 13, 2024

gitferry Sep 14, 2024

SebastianElvis left a comment

SebastianElvis Sep 16, 2024

gitferry Sep 16, 2024

SebastianElvis Sep 16, 2024

gitferry Sep 16, 2024

	// DecrementJailedFinalityProviderCounter increments the counter for the jailed
	// DecrementJailedFinalityProviderCounter decrements the counter for the jailed

feat(ADR-25): Remove voting power from jailed finality providers #65

feat(ADR-25): Remove voting power from jailed finality providers #65

Conversation

gitferry commented Sep 10, 2024 • edited Loading

maurolacy left a comment • 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

Choose a reason for hiding this comment

maurolacy Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maurolacy Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RafilxTenfen left a comment

Choose a reason for hiding this comment

Lazar955 left a comment

Choose a reason for hiding this comment

SebastianElvis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gitferry Sep 12, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

gitferry commented Sep 10, 2024 •

edited

Loading

maurolacy left a comment •

edited

Loading

maurolacy Sep 11, 2024 •

edited

Loading

maurolacy Sep 11, 2024 •

edited

Loading

gitferry Sep 12, 2024 •

edited

Loading