feat: improve performance BTC reward distribution #306

RafilxTenfen · 2024-11-28T02:17:31Z

Add new structures to track BTC delegation rewards and lazily distribute
Add new event BTCDelegationStatus_EXPIRED for BTC delegations that indicates a BTC delegation is no longer contributing to security
Important files diff for review
- proto/babylon/incentive/rewards.proto
- x/finality/keeper/power_dist_change.go
- x/incentive/keeper/btc_staking_gauge.go
- x/incentive/keeper/grpc_query.go
- x/incentive/keeper/msg_server.go
- x/incentive/keeper/reward_tracker.go
- x/incentive/keeper/reward_tracker_store.go
- x/btcstaking/keeper/btc_delegations.go
- x/btcstaking/keeper/msg_server.go
- x/btcstaking/types/btc_delegation.go

TODOs

e2e test for rewards

Benchmarks

Command to generate pprof go tool pprof -http=:8080 http://localhost:6060/debug/pprof/profile\?seconds\=300

Main branch that iterates over delegations to distribute rewards with 163k BTC delegations, took 15.52s (13.1%) of the time during the 300seconds interval used to generate the pprof

Current branch rafilx/improve-performance-btc-dist with 100k BTC delegations, took 6.73s (1.8%) of the time during the 300seconds interval

Note that the percentage of the time used that is ~7x lower should be considered instead of the time, the benchmark from main was running in the benchmark server 48 processors and 256GB of RAM while the one in the current branch I run locally with 24 processors and 96GB of RAM.... So the percentage of the time usage instead of seconds should be the one in comparison

Thanks @Lazar955 for help with benchmark
The function used to show the difference is the TallyBlocks because the RewardBTCStaking does not appear in the pprof of the current branch demonstrating that the time spent there is negligible to the overall period

…tion rewards

…improve-performance-btc-dist

…otal amount of satoshi staked as sdkmath.int

… need the total to create new delegation

…improve-performance-btc-dist

…distribution testing

…improve-performance-btc-dist

…c file

… (needed to create a new set of keys to store map of del => fp address)

…improve-performance-btc-dist

RafilxTenfen · 2024-12-17T02:07:51Z

It would be great to have some e2e test with pre-defined values i.e setup some scenario with lets ay 2 fps, each fp having 2 delegations with pre-defined values, known inflation, and split between fps and validators and calculate whether caluclated rewards for stakers makes sense.

I have added an integration tests with rewards, and there is also a fuzz test that verifies the proportion distributed FuzzRewardBTCStaking

but I will write an e2e test tomorrow with 2 active fps and 2 BTC delegations, it will be a bit difficult to estimate the rewards, but I think the goal would be to see the difference between the delegations amounts

app/app.go

proto/babylon/incentive/rewards.proto

testutil/coins/margin.go

testutil/incentives-helper/keeper.go

SebastianElvis · 2024-12-17T04:34:49Z

x/finality/keeper/power_dist_change.go

@@ -247,7 +261,11 @@ func (k Keeper) ProcessAllPowerDistUpdateEvents(

 		// if this finality provider is slashed, continue to avoid
 		// assigning delegation to it
-		if _, ok := slashedFPs[fpBTCPKHex]; ok {


No BtcDels field is no longer necessary. It is used for tracking the share of every BTC delegation under a given FP. Now this shall be replaced by storeBTCDelegationRewardsTracker store

SebastianElvis

That's a comprehensive PR 👍 Still parsing this. Some initial comments

Meanwhile, is it possible to consolidate some of the abstractions, e.g., in reward_tracker.go and reward_tracker_store.go? The current implementation is a bit hard to follow and some abstractions might not be necessary. See comments

SebastianElvis · 2024-12-17T04:41:18Z

x/incentive/keeper/reward_tracker_store.go

Do we consider using the new collections package for handling all these DB operations (like here)? It seems that some functions defined below are captured in the collections

I did not consider to use collections package, thanks for pointing it out
I will update

proto/babylon/incentive/rewards.proto

SebastianElvis · 2024-12-17T04:54:57Z

x/incentive/keeper/reward_tracker.go

+// sendAllBtcRewardsToGauge iterates over all the finality providers associated
+// with the delegator and withdraw the rewards available to the gauge.
+// This creates new periods for each delegation and finality provider.
+func (k Keeper) sendAllBtcRewardsToGauge(ctx context.Context, del sdk.AccAddress) error {
+	return k.iterBtcDelegationsByDelegator(ctx, del, func(del, fp sdk.AccAddress) error {
+		return k.btcDelegationModified(ctx, fp, del)
+	})
+}


Tbh this function is a bit hard to follow. In order to distribute rewards to each BTC delegator, it uses btcDelegationModified which is actually a "hook" function.

It iterates over all the delegations from one address and acts as if the btc delegation was modified (which was, since BTC rewards are being withdraw) it just doesn't modifies the amount staked

SebastianElvis · 2024-12-17T04:56:18Z

x/incentive/keeper/reward_tracker.go

+// CalculateBTCDelegationRewards calculates the rewards entitled for this delegation
+// from the starting period cumulative reward and the ending period received as parameter
+// It returns the amount of rewards without decimals (it removes the DecimalAccumulatedRewards).
+func (k Keeper) CalculateBTCDelegationRewards(ctx context.Context, fp, del sdk.AccAddress, endPeriod uint64) (sdk.Coins, error) {


This function is only used in CalculateBTCDelegationRewardsAndSendToGauge. Could we merge it into it?

We could, but it is very helpful to have a separate function that only calculates the rewards instead of calculating and sending them to the gauge

mostly for testing

SebastianElvis · 2024-12-17T05:04:23Z

x/incentive/keeper/reward_tracker.go

+// FpSlashed a slashed finality provider should withdraw all the rewards
+// available to it, by iterating over all the delegations for this FP
+// and sending to the gauge. After the rewards are removed, it should
+// delete every rewards tracker value in the store related to this slashed
+// finality provider.
+func (k Keeper) FpSlashed(ctx context.Context, fp sdk.AccAddress) error {


Do we need to do anything upon a slashed FP?

After a FP is slashed BTC delegators will unbond and withdraw anyway, and this will trigger all those lazy executions. So enforce withdrawal does not seem necessary.

Enforcing withdrawal might cause some edge case, e.g., a FP is slashed which triggers all withdrawal. Then if a BTC delegator wants to withdraw will the code panick since some keys in KV store are removed?

This is not strictly necessary, but the FP will not receive rewards anymore under that BTC pk, so we will basically stay with all of those structures inside the database, my idea here was to use everything at once and then prune it

Enforcing withdrawal might cause some edge case, e.g., a FP is slashed which triggers all withdrawal. Then if a BTC delegator wants to withdraw will the code panick since some keys in KV store are removed?

If the BTC delegator wants to withdraw after the FP is slashed, it will just not exists that FP,DEL record anymore and will not appear in the iterator, it should not panic

SebastianElvis · 2024-12-17T05:07:29Z

x/incentive/keeper/grpc_query.go

+		if err := k.sendAllBtcDelegationTypeToRewardsGauge(ctx, sType, address); err != nil {
+			return nil, status.Error(codes.Internal, err.Error())
+		}


RPC should not trigger state update at all. Otherwise it's possibel that someone queries this API and the queried node's state root is updated, then in the next block this node won't have a matching state root with any other node.

in the query the database does not commit the modified changes to the state
The same is done by the cosmos-sdk DelegationRewards query

SebastianElvis · 2024-12-17T05:11:50Z

x/finality/keeper/power_dist_change.go

 		// add all BTC delegations that are not unbonded to the new finality provider
 		for j := range dc.FinalityProviders[i].BtcDels {
 			btcDel := *dc.FinalityProviders[i].BtcDels[j]


Would recommend we remove any usage of BtcDels under FinalityProviderDistInfo. The original purpose of BtcDels is to track the share of every BTC delegator under the FP. Now this is replaced by KV stores under x/incentive and no need to do this anymore

Can this be done in another PR after this one

Could we do it in this PR or merge this PR to a feature branch? imo replacing BtcDels with the "virtual periods" is the main point of this feature so it's better to have it before merging this feature to main

hmm I also think we could remove those here as this is main value proposition of this pr. In my mind it should not be that hard i.e:

remove BtcDels []*BTCDelDistInfo from FinalityProviderDistInfo

Update FinalityProviderDistInfo.TotalBondedSat based on new activated and unbonded delegations. So with every new unbonding FinalityProviderDistInfo.TotalBondedSat = FinalityProviderDistInfo.TotalBondedSat - unbonding.value and with avery activated staking FinalityProviderDistInfo.TotalBondedSat = FinalityProviderDistInfo.TotalBondedSat + activated.value

With this change, it seems, our voting power event handling wil become optimal (we care only about the diff in voting power in each block)

SebastianElvis · 2024-12-17T05:14:40Z

x/finality/keeper/power_dist_change.go

@@ -326,6 +350,20 @@ func (k Keeper) ProcessAllPowerDistUpdateEvents(
 		}
 	}

+	k.processBtcDelegations(ctx, cacheFpByBtcPkHex, newActiveBtcDels, func(fp, del sdk.AccAddress, sats uint64) {


Can we do this inside the for _, event := range events loop? This loop was supposed to be the "event bus" that dispatches notifications to relevant modules, including x/incentive

we could if the events were sorted and never received more than once

ref #306 (comment)

One BTC delegation can generate 2 unbonding events

I see. Probably a better fix is then distinguishing between two types of unbonded events, but this is certainly out of this PR's scope.

Also, why is newActiveBtcDels needed given that a BTC del cannot be active twice?

gitferry

Great work! My first round of comments:

proto/babylon/incentive/rewards.proto

x/btcstaking/types/btc_delegation.go

gitferry · 2024-12-17T12:11:24Z

x/finality/keeper/power_dist_change.go

+	})
+
+	k.processBtcDelegations(ctx, cacheFpByBtcPkHex, newUnbondedBtcDels, func(fp, del sdk.AccAddress, sats uint64) {
+		err := k.IncentiveKeeper.BtcDelegationUnbonded(ctx, fp, del, sats)


I think this pr works because the unbonded delegation will not appear in the cache so that unbonding events will processed twice by the incentive module

x/finality/keeper/power_dist_change.go

…nt signatures

proto/babylon/incentive/rewards.proto

SebastianElvis · 2024-12-18T01:40:04Z

test/e2e/initialization/node.go

@@ -355,7 +355,7 @@ func (n *internalNode) initNodeConfigs(persistentPeers []string) error {
 	valConfig.P2P.ExternalAddress = fmt.Sprintf("%s:%d", n.moniker, 26656)
 	valConfig.RPC.ListenAddress = "tcp://0.0.0.0:26657"
 	valConfig.StateSync.Enable = false
-	valConfig.LogLevel = "debug"
+	valConfig.LogLevel = "info"


probably we can keep it at debug level for debugging purposes

SebastianElvis · 2024-12-18T01:42:38Z

x/finality/keeper/power_dist_change.go

 		// add all BTC delegations that are not unbonded to the new finality provider
 		for j := range dc.FinalityProviders[i].BtcDels {
 			btcDel := *dc.FinalityProviders[i].BtcDels[j]


Could we do it in this PR or merge this PR to a feature branch? imo replacing BtcDels with the "virtual periods" is the main point of this feature so it's better to have it before merging this feature to main

SebastianElvis · 2024-12-18T01:45:24Z

x/finality/keeper/power_dist_change.go

@@ -326,6 +350,20 @@ func (k Keeper) ProcessAllPowerDistUpdateEvents(
 		}
 	}

+	k.processBtcDelegations(ctx, cacheFpByBtcPkHex, newActiveBtcDels, func(fp, del sdk.AccAddress, sats uint64) {


I see. Probably a better fix is then distinguishing between two types of unbonded events, but this is certainly out of this PR's scope.

Also, why is newActiveBtcDels needed given that a BTC del cannot be active twice?

…improve-performance-btc-dist

… if btcstaking is active

RafilxTenfen added 3 commits November 27, 2024 12:21

chore: add todos

f55ea69

chore: add FinalityProviderRewards structure to help track BTC delega…

e338dbf

…tion rewards

chore: add BTC delegation rewards structure tracker

58d9451

RafilxTenfen self-assigned this Nov 29, 2024

RafilxTenfen added 26 commits November 29, 2024 08:35

Merge branch 'main' of github.com:babylonlabs-io/babylon into rafilx/…

467b5a3

…improve-performance-btc-dist

chore: add todos and descriptions for rewards tracker

598608c

chore: adding shape to FP reward tracker periods

530cd88

chore: modified to use same structure to store the amt staked

e79d0e1

chore: removed single value in store and used structure to keep the t…

0a08cce

…otal amount of satoshi staked as sdkmath.int

chore: same structure to store the amount creates weird case where we…

3f33dba

… need the total to create new delegation

Merge branch 'main' of github.com:babylonlabs-io/babylon into rafilx/…

9067255

…improve-performance-btc-dist

fix: add #306 to changelog

a4bc644

fix: failing test

8cc0400

fix: lint errcheck

482b860

fix: storage of fp current rewards and add test for AddDelegationSat

63f4b6d

chore: add test for subtract delegation sats

8fc5d9e

chore: add test for initilize fp

b3120e7

fix: precision points error and allowed 0.1% margin error in rewards …

ea15191

…distribution testing

Merge branch 'main' of github.com:babylonlabs-io/babylon into rafilx/…

013fc62

…improve-performance-btc-dist

chore: merge main

dcc06f3

fix: lint add errcheck

7ca2ceb

Merge branch 'main' of github.com:babylonlabs-io/babylon into rafilx/…

454b200

…improve-performance-btc-dist

chore: add details for structures

48882e2

chore: add implementation of unbond and slash fp

92fffdc

chore: add slash handle and split store set/get/iterator into specifi…

b9d9e09

…c file

chore: add rewards to gauge on reward withdraw and reward calculation…

d009991

… (needed to create a new set of keys to store map of del => fp address)

chore: add todo for init/export gen

f80d9f6

Merge branch 'main' of github.com:babylonlabs-io/babylon into rafilx/…

513b73d

…improve-performance-btc-dist

fix: unecessary coupling of signer PrivateSigner inside helpers

813a886

chore: add test for private func setBTCDelegatorToFP

e1bac01

RafilxTenfen added 4 commits December 16, 2024 17:06

chore: add integration test for early unbond

067232e

chore: reach expire BTC staking tx

a2a5c8e

chore: add test to check early unbonding + expire event

139781b

chore: add integration test for incentives and rewards

01479f3

RafilxTenfen requested a review from KonradStaniec December 17, 2024 02:07

SebastianElvis reviewed Dec 17, 2024

View reviewed changes

gitferry reviewed Dec 17, 2024

View reviewed changes

RafilxTenfen added 5 commits December 17, 2024 09:33

chore: rollback to private maccperms

e6d1884

chore: address pr comments

3504b67

chore: add e2e test to BTC rewards active btc delegations with covena…

b5f7794

…nt signatures

chore: stuck at e2e btc staking activation height

e0cab3a

chore: stuck at activated height

9d7fad6

SebastianElvis reviewed Dec 18, 2024

View reviewed changes

RafilxTenfen added 12 commits December 17, 2024 22:57

chore: add new status event BTCDelegationStatus_EXPIRED

dcee28e

chore: add new e2e test that currently fails to activate finality

b3decf7

Merge branch 'main' of github.com:babylonlabs-io/babylon into rafilx/…

27a13dd

…improve-performance-btc-dist

fix: set btc delegations to active prior to commit pub rand and check…

bdf7c15

… if btcstaking is active

chore: add check for rewards amounts

8d324dd

fix: lint and test

e9c8050

chore: set wallet name for delegations const

318993b

fix: covenant sigs from correct node

2087c8a

chore: correct usage of nodes

c9e8f8a

chore: add finality vote to another block

99acf6c

chore: add sort of node by name on creation

beffe5e

tryfix: check rewards seccond time

3c97ab1

RafilxTenfen mentioned this pull request Dec 18, 2024

chore: remove BTC delegation reward gauge #357

Open

RafilxTenfen added 2 commits December 18, 2024 17:02

tryfix: add fp voting every now and then

955c498

tryfix: add fp voting every block, fails due to acc sequence

5cd7bd7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: improve performance BTC reward distribution #306

feat: improve performance BTC reward distribution #306

RafilxTenfen commented Nov 28, 2024 •

edited

Loading

RafilxTenfen commented Dec 17, 2024

SebastianElvis Dec 17, 2024

SebastianElvis left a comment

SebastianElvis Dec 17, 2024

RafilxTenfen Dec 17, 2024

SebastianElvis Dec 17, 2024

RafilxTenfen Dec 18, 2024

SebastianElvis Dec 17, 2024

RafilxTenfen Dec 17, 2024

RafilxTenfen Dec 17, 2024

SebastianElvis Dec 17, 2024

RafilxTenfen Dec 17, 2024

SebastianElvis Dec 17, 2024

RafilxTenfen Dec 17, 2024

SebastianElvis Dec 17, 2024

RafilxTenfen Dec 17, 2024

SebastianElvis Dec 18, 2024

KonradStaniec Dec 18, 2024 •

edited

Loading

SebastianElvis Dec 17, 2024

RafilxTenfen Dec 17, 2024

SebastianElvis Dec 18, 2024

gitferry left a comment •

edited

Loading

gitferry Dec 17, 2024

SebastianElvis Dec 18, 2024

SebastianElvis Dec 18, 2024

SebastianElvis Dec 18, 2024

feat: improve performance BTC reward distribution #306

Are you sure you want to change the base?

feat: improve performance BTC reward distribution #306

Conversation

RafilxTenfen commented Nov 28, 2024 • edited Loading

TODOs

Benchmarks

RafilxTenfen commented Dec 17, 2024

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

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

KonradStaniec Dec 18, 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

gitferry 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

RafilxTenfen commented Nov 28, 2024 •

edited

Loading

KonradStaniec Dec 18, 2024 •

edited

Loading

gitferry left a comment •

edited

Loading