feat: implement of btc-staker improvement 107 #134

wnjoon · 2025-02-11T09:25:51Z

This PR includes the implementation for the btc-staker improvement related to issue #107 .

The removal of the watch-staking endpoint and the post-approval flow were approved in the previous PR(#131). This PR focuses on 'reducing the amount of state stored in the database'.

The details of the implementation are as follows:

Reduce the amount of state held in the Database

Previously, btc-staker had the following structural shortcomings due to the inclusion of watch-staking endpoints and post-approval flows:

It required maintaining status information for both Babylon and Bitcoin, even though certain information could be directly queried from these chains. This led to unnecessary duplication in the database.
The logic became overly complex, as follow-up processing had to account for both post-approval and pre-approval scenarios when restarting the program.

In the previous PR(#131), the watch-staking endpoint and post-approval flow were removed, simplifying the database structure. Additionally, information that can be directly queried from the chain (e.g., transaction progress status) is no longer stored in the database.

The existing database structure is as follows:

The Watched field is not shown as it was already removed in a previous PR.

type StoredTransaction struct {
    StoredTransactionIdx      uint64
    StakingTx                 *wire.MsgTx
    StakerAddress              string
    StakingOutputIndex        uint32
    StakingTxConfirmationInfo *BtcConfirmationInfo
    StakingTime               uint16
    FinalityProvidersBtcPks   []*btcec.PublicKey
    Pop                       *ProofOfPossession
    State                      proto.TransactionState
    UnbondingTxData            *UnbondingStoreData
    BabylonBTCDelegationTxHash string
}

The reduced db looks like this:

type StoredTransaction struct {
	StoredTransactionIdx uint64
	StakingTx            *wire.MsgTx
	StakerAddress        string
}

The deleted fields can be retrieved directly from Babylon or Bitcoin when needed. Additionally, some fields are no longer used due to the removal of the post-approval flow.

wnjoon · 2025-02-11T09:30:05Z

Commit Summary 0233d1a

Removed state, fpkey, and pop fields from StoredTransaction
Added a function QueryBTCDelegation to query transaction information from the Babylon chain.

@KonradStaniec
Let’s review additional fields that can be removed. If you have any suggestions, please feel free to share them.

KonradStaniec · 2025-02-12T12:39:23Z

staker/stakerapp.go

+	if err != nil {
+		return nil, nil, fmt.Errorf("cannot spend staking output. Error converting fpBtcPkList to btcPkList: %w", err)
+	}
+
 	spendStakeTxInfo, err := createSpendStakeTxFromStoredTx(


this functions has inside the follwing checks:

if storedtx.StakingTxConfirmedOnBtc() && !storedtx.UnbondingTxConfirmedOnBtc() {

and storedtx.StakingTxConfirmedOnBtc() was depended on the old post approval flow.

I think now we must remove this check.

This also touches:

func (app *App) WithdrawableTransactions(limit, offset uint64) (*stakerdb.StoredTransactionQueryResult, error) {

function i.e this function currently returns results based on this field.

I think now delegation is withdrawable if:

the staking transaction is still on BTC and its timelock has expired

the unbonding transaction is still on BTC and its timelock has expired

I modified the createSpendStakeTxFromStoredTx function and its calling part to align with your suggestion.

fixed 380b429

this functions has inside the follwing checks:

if storedtx.StakingTxConfirmedOnBtc() && !storedtx.UnbondingTxConfirmedOnBtc() {

and storedtx.StakingTxConfirmedOnBtc() was depended on the old post approval flow.

I think now we must remove this check.

This also touches:

func (app *App) WithdrawableTransactions(limit, offset uint64) (*stakerdb.StoredTransactionQueryResult, error) {

function i.e this function currently returns results based on this field.

I think now delegation is withdrawable if:

the staking transaction is still on BTC and its timelock has expired

the unbonding transaction is still on BTC and its timelock has expired

We need to discuss the WithdrawableTransactions function.

Currently, this function queries all transactions stored in the database and checks whether StakingTxConfirmedOnBtc() or UnbondingTxConfirmedOnBtc() is used. Based on this, it sets the corresponding values and then verifies whether timeLockExpired is used.

Due to this structure, we had to maintain StakingTxConfirmationInfo and UnbondingStoreData.UnbondingTxConfirmationInfo in the database. Additionally, we also store the StakingTime and UnbondingTime fields in the database.

If these fields are removed, we would need to call the RPC for each transaction to fetch the required values, which seems highly inefficient. Moreover, adding an RPC call inside the QueryStoredTransactions function does not seem structurally ideal.

If querying all withdrawable transactions is not necessary, it might be a better approach to implement a function that checks whether specific requested transactions are withdrawable. Please share your thoughts on this.

However, with this commit, spent transactions are removed from the database, meaning only actively staked transactions remain. This might reduce the overall burden on the database. I will proceed with this approach and provide further comments after testing.

If querying all withdrawable transactions is not necessary, it might be a better approach to implement a function that checks whether specific requested transactions are withdrawable. Please share your thoughts on this.

Tbh I feel this function is hard requiremnt for the users i.e If I am a user and have multiple delegations (each in different stage) I should have way to check which of those I can withdraw to my wallet.

In the future such functionality can be automatic based on this information i.e

periodically query all withdrawable transactions

and withdraw them to user wallet

If these fields are removed, we would need to call the RPC for each transaction to fetch the required values, which seems highly inefficient. Moreover, adding an RPC call inside the QueryStoredTransactions function does not seem structurally ideal.

ahh agreed on that. this means we need to fill StakingTxConfirmedOnBtc() field in db, after tx delegation is active on Babylon

fixed cb94a22

@KonradStaniec
I modified WithdrawableTransaction to retrieve the necessary data via RPC instead of the database.
As a result, since the database is no longer used, I created it as a separate function rather than passing a query statement to QueryStoredTransactions. After retrieving the status from the Babylon chain, I then obtained the confirmation information from Bitcoin based on the transaction status.

KonradStaniec · 2025-02-12T12:45:18Z

staker/stakerapp.go

+	if unbondingTxStatus == walletcontroller.TxNotFound {
+		// no unbonding tx on chain and staking output already spent, most probably
+		// staking transaction has been withdrawn, update state in db
+		return nil


I wonder what would be best handling of this 🤔 maybe we need to add some operations to prune db from spent staking transactions to not have them in db forever or maybe we need to add some bool field to db, like withdrawn bool wdyt ?

I think it would be a good idea to remove that transaction content from the database. Otherwise, functions that query the entire database, such as QueryStoredTransactions, will retrieve unnecessary items.

fixed 380b429

KonradStaniec · 2025-02-12T12:49:06Z

stakerservice/service.go

 	}

 	details := storedTxToStakingDetails(storedTx)
+	details.StakingState = di.BtcDelegation.GetStatusDesc()


those details are also filed in withdrawableTransactions endpoint

fixed 380b429

KonradStaniec · 2025-02-12T12:51:54Z

staker/stakerapp.go

 	stakingTxHash *chainhash.Hash,
 ) {
 	defer app.wg.Done()
 	defer waitEv.Cancel()
-	unbondingTxHash := unbondingData.UnbondingTx.TxHash()
+	// unbondingTxHash := unbondingData.UnbondingTx.TxHash()


commented out code ?

fixed 380b429

… and etc

wnjoon · 2025-02-13T02:49:14Z

Commit Summary 380b429

Removed db fields: StakingOutputIndex, BabylonBTCDelegationTxHash
Refactoring db field: Take out UnbondingTxConfirmationInfo from UnbondingTxData
Added a function to remove transactions from the database and applied it to the logic where a transaction is spent.

wnjoon · 2025-02-13T12:14:31Z

Commit Summary cb94a22

Removed db fields related to confirmation
Changed the logic of the WithdrawableTransaction function to retrieve data via RPC instead of the database.

At this stage, the database will look like this:

type StoredTransaction struct {
	StoredTransactionIdx uint64
	StakingTx            *wire.MsgTx
	StakerAddress        string
}

wnjoon · 2025-02-14T05:27:30Z

Commit Summary 8628a24

Updated protobuf
Removed unused functions
Added comments
Wrapped errors using fmt.Errorf based on the code I worked on

KonradStaniec · 2025-02-14T07:04:36Z

staker/stakerapp.go

+
+	var totalTxCount int
+	// add stored transactions to slice
+	if err := app.txTracker.ScanTrackedTransactions(func(tx *stakerdb.StoredTransaction) error {


I think here we need to:

first return all the hashes in db (similarity like in checkTransactionsStatus)

and then iterate over all the hashes to check the status on Babylon and BTC

Otherwise we will take the read lock on db for a long time given all the queries we are making.

You are right.
I will try modifying the function by applying offset and limit as described below.

Previously, I returned storedTransaction from ScanTrackedTransactions and processed it directly in RPC, so I didn’t feel the need for offset and limit.

I will change the logic as follows:

Apply offset and limit to the StoredTransactions function to first retrieve the transaction list.

Process the retrieved transactions one by one based on their status.

fixed 2c2adee

KonradStaniec · 2025-02-14T07:07:43Z

proto/transaction.proto

 // Contains information about btc confirmation
 message BTCConfirmationInfo {
    uint32 block_height = 1;
    bytes block_hash = 2;
 }

-message CovenantSig {


I think BTCConfirmationInfo is not longer used

fixed 2c2adee

KonradStaniec · 2025-02-14T07:19:16Z

stakerservice/service.go

 		"btc_delegation_from_btc_staking_tx": rpc.NewRPCFunc(s.btcDelegationFromBtcStakingTx, "stakerAddress,btcStkTxHash,tag,covenantPksHex,covenantQuorum"),
 		"staking_details":                    rpc.NewRPCFunc(s.stakingDetails, "stakingTxHash"),
 		"spend_stake":                        rpc.NewRPCFunc(s.spendStake, "stakingTxHash"),
 		"list_staking_transactions":          rpc.NewRPCFunc(s.listStakingTransactions, "offset,limit"),
 		"unbond_staking":                     rpc.NewRPCFunc(s.unbondStaking, "stakingTxHash"),
-		"withdrawable_transactions":          rpc.NewRPCFunc(s.withdrawableTransactions, "offset,limit"),
+		"withdrawable_transactions":          rpc.NewRPCFunc(s.withdrawableTransactions, ""),


why do you think pagination does not make sense here ?

fixed 2c2adee

wnjoon · 2025-02-14T09:00:25Z

Commit Summary 2c2adee

Updated WithdrawableTransactions logic
Removed unused protobuf field

KonradStaniec

Looks good ! thanks for this contribution !

* feat: implement of btc-staker improvement 107 (#134) * feat: remove watch-staking endpoint, post-approval flow * feat: remove db fields - state, fpkey and pop * feat: update CHANGELOG * feat: remove stakingoutputIdx, btcTxHash. refactoring UnbondingTxData and etc * feat: remove confirmation fields and modify WithdrawableTransactions * feat: cleanup codes including comments, error wrapping etc. * feat: change WithdrawableTransactions logic and set pagination * Konradstaniec/bump changelog (#141) Co-authored-by: RafilxTenfen <[email protected]> --------- Co-authored-by: wonjoon <[email protected]> Co-authored-by: RafilxTenfen <[email protected]>

wnjoon added 2 commits February 11, 2025 10:08

feat: remove watch-staking endpoint, post-approval flow

2745d54

feat: remove db fields - state, fpkey and pop

0233d1a

wnjoon marked this pull request as ready for review February 11, 2025 09:33

feat: update CHANGELOG

f3883f4

KonradStaniec reviewed Feb 12, 2025

View reviewed changes

feat: remove stakingoutputIdx, btcTxHash. refactoring UnbondingTxData…

380b429

… and etc

feat: remove confirmation fields and modify WithdrawableTransactions

cb94a22

feat: cleanup codes including comments, error wrapping etc.

8628a24

KonradStaniec reviewed Feb 14, 2025

View reviewed changes

feat: change WithdrawableTransactions logic and set pagination

2c2adee

wnjoon requested a review from KonradStaniec February 16, 2025 22:14

KonradStaniec approved these changes Feb 17, 2025

View reviewed changes

KonradStaniec merged commit 1101a57 into babylonlabs-io:feat/b-harvest-improve-btc-staker Feb 18, 2025
9 of 11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement of btc-staker improvement 107 #134

feat: implement of btc-staker improvement 107 #134

wnjoon commented Feb 11, 2025 •

edited

Loading

wnjoon commented Feb 11, 2025 •

edited

Loading

KonradStaniec Feb 12, 2025

wnjoon Feb 13, 2025 •

edited

Loading

wnjoon Feb 13, 2025 •

edited

Loading

KonradStaniec Feb 13, 2025

wnjoon Feb 13, 2025

KonradStaniec Feb 12, 2025

wnjoon Feb 13, 2025

wnjoon Feb 13, 2025

KonradStaniec Feb 12, 2025

wnjoon Feb 13, 2025

KonradStaniec Feb 12, 2025

wnjoon Feb 13, 2025

wnjoon commented Feb 13, 2025 •

edited

Loading

wnjoon commented Feb 13, 2025 •

edited

Loading

wnjoon commented Feb 14, 2025

KonradStaniec Feb 14, 2025

wnjoon Feb 14, 2025 •

edited

Loading

wnjoon Feb 14, 2025

KonradStaniec Feb 14, 2025

wnjoon Feb 14, 2025

KonradStaniec Feb 14, 2025

wnjoon Feb 14, 2025

wnjoon commented Feb 14, 2025

KonradStaniec left a comment

feat: implement of btc-staker improvement 107 #134

feat: implement of btc-staker improvement 107 #134

Conversation

wnjoon commented Feb 11, 2025 • edited Loading

Reduce the amount of state held in the Database

wnjoon commented Feb 11, 2025 • edited Loading

Choose a reason for hiding this comment

wnjoon Feb 13, 2025 • edited Loading

Choose a reason for hiding this comment

wnjoon Feb 13, 2025 • 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

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

wnjoon commented Feb 13, 2025 • edited Loading

wnjoon commented Feb 13, 2025 • edited Loading

wnjoon commented Feb 14, 2025

Choose a reason for hiding this comment

wnjoon Feb 14, 2025 • 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

wnjoon commented Feb 14, 2025

KonradStaniec left a comment

Choose a reason for hiding this comment

wnjoon commented Feb 11, 2025 •

edited

Loading

wnjoon commented Feb 11, 2025 •

edited

Loading

wnjoon Feb 13, 2025 •

edited

Loading

wnjoon Feb 13, 2025 •

edited

Loading

wnjoon commented Feb 13, 2025 •

edited

Loading

wnjoon commented Feb 13, 2025 •

edited

Loading

wnjoon Feb 14, 2025 •

edited

Loading