Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: new command phase1 stake to BTC delegation #90

Open
wants to merge 41 commits into
base: main
Choose a base branch
from

Conversation

RafilxTenfen
Copy link
Contributor

@RafilxTenfen RafilxTenfen commented Nov 5, 2024

  • add new command to create BTC delegation from BTC staking transaction stakercli daemon stake-from-phase1
  • refractory of e2e test manager
  • split e2e test files and test manager funcs

@RafilxTenfen RafilxTenfen self-assigned this Nov 5, 2024
staker/stakerapp.go Outdated Show resolved Hide resolved
stakerservice/client/rpcclient.go Outdated Show resolved Hide resolved
stakerservice/service.go Outdated Show resolved Hide resolved
@RafilxTenfen RafilxTenfen marked this pull request as ready for review November 22, 2024 20:40
@RafilxTenfen RafilxTenfen requested review from KonradStaniec, a team, Lazar955 and samricotta and removed request for a team November 22, 2024 20:40
@RafilxTenfen RafilxTenfen changed the title WIP: migrate stk to phase2 feat: new command phase1 stake to BTC delegation Nov 22, 2024
staker/stakerapp.go Outdated Show resolved Hide resolved
staker/stakerapp.go Outdated Show resolved Hide resolved
@@ -1,6 +1,6 @@
// Code generated by protoc-gen-go. DO NOT EDIT.
// versions:
// protoc-gen-go v1.33.0
// protoc-gen-go v1.35.1
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure why this updated, just run proto-gen

Comment on lines 1551 to 1563
// eventually tx is send to babylon
storedTx, err := app.waitForTrackedTransactionState(stkTxHash, proto.TransactionState_SENT_TO_BABYLON, time.Second, 20)
if err != nil {
utils.PushOrQuit(
cmd.errChan,
err,
app.quit,
)
app.logger.WithFields(logrus.Fields{
"stakingTxHash": stkTxHash,
}).Debugf("BTC delegation waited for too long to become active, check the status manually")
continue
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not fully convinced it is a good idea to retrieve the consumer delegation tx hash this way

@RafilxTenfen
Copy link
Contributor Author

RafilxTenfen commented Nov 26, 2024

One last thing is that I noticed some flankiness in the integration test
for the same code:

The failed one indicates that some datagen function from babylon to insert the covenant sig is not behaving as expected

2024-11-26T02:20:08.9985192Z     manager.go:1159: 
2024-11-26T02:20:08.9986075Z         	Error Trace:	/home/runner/work/btc-staker/btc-staker/itest/manager.go:1159
2024-11-26T02:20:08.9987678Z         	            				/home/runner/work/btc-staker/btc-staker/itest/e2e_test.go:977
2024-11-26T02:20:08.9988437Z         	Error:      	Received unexpected error:
2024-11-26T02:20:08.9990075Z         	            	invalid tx: txToSign inpunt index must point to output with provided script
2024-11-26T02:20:08.9990856Z         	Test:       	TestStakeFromPhase1
2024-11-26T02:20:08.9991493Z time="2024-11-26T02:17:42Z" level=info msg="Received shutdown signal. Stopping..."

The check that fails

	if txToSign.TxIn[0].PreviousOutPoint.Index != fundingOutputIdx {
		return fmt.Errorf("txToSign inpunt index must point to output with provided script")
	}

txToSign is the BTC staking tx, could it be due to the way that I create the staking tx with Phase1 commands?

}

bestBlockHeight := app.currentBestBlockHeight.Load()
if err := app.waitForStakingTransactionConfirmation(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tbh in this handler I would

  • return error if transaction is not confirmed on BTC already (less state to hold on to)
  • do the same thing that function handlePreApprovalCmd does but with inclusion proof . Maybe you can even reuse this function somehow. Then this new staking info will be saved in db only after send to Babylon succeeds.

This way we will avoid this waiting go rutine:

			go func() {
				// eventually tx is send to babylon and notifies the cmd request
				storedTx, err := app.waitForTrackedTransactionState(stkTxHash, proto.TransactionState_SENT_TO_BABYLON, time.Second, 20)

and we won't need to store delegation transaction txhash in db as it could be returned immediately from handlePreApprovalCmd function

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct, it became much simpler! Thanks
One thing is that I would still keep the BTC delegation tx hash in the TrackedTransaction I believe it helps to track the transaction in the other side

Let me know if you want to be removed

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing is that I would still keep the BTC delegation tx hash in the TrackedTransaction I believe it helps to track the transaction in the other side

hmm makes sense. Lets leave it then 👍

@KonradStaniec
Copy link
Collaborator

One last thing is that I noticed some flankiness in the integration test
for the same code:

Run fail https://github.com/babylonlabs-io/btc-staker/actions/runs/12022509602/job/33514852376?pr=90
Run Pass https://github.com/babylonlabs-io/btc-staker/actions/runs/12022509602/job/33515185226?pr=90
The failed one indicates that some datagen function from babylon to insert the covenant sig is not behaving as expected

Ahh I see, one of the test functions assumess that staking output index is zero:

func GenCovenantAdaptorSigs(
	covenantSKs []*btcec.PrivateKey,
	fpPKs []*btcec.PublicKey,
	fundingTx *wire.MsgTx,
	pkScriptPath []byte,
	slashingTx *bstypes.BTCSlashingTx,
) ([]*bstypes.CovenantAdaptorSignatures, error) {
	covenantSigs := []*bstypes.CovenantAdaptorSignatures{}
	for _, covenantSK := range covenantSKs {
		covMemberSigs := &bstypes.CovenantAdaptorSignatures{
			CovPk:       bbn.NewBIP340PubKeyFromBTCPK(covenantSK.PubKey()),
			AdaptorSigs: [][]byte{},
		}
		for _, fpPK := range fpPKs {
			encKey, err := asig.NewEncryptionKeyFromBTCPK(fpPK)
			if err != nil {
				return nil, err
			}
			covenantSig, err := slashingTx.EncSign(fundingTx, 0, pkScriptPath, covenantSK, encKey)
			if err != nil {
				return nil, err
			}
			covMemberSigs.AdaptorSigs = append(covMemberSigs.AdaptorSigs, covenantSig.MustMarshal())
		}
		covenantSigs = append(covenantSigs, covMemberSigs)
	}

	return covenantSigs, nil
}

but in test you create staking tx by:

	resFundRawStkTx, err := rpcBtc.FundRawTransaction(&tx, btcjson.FundRawTransactionOpts{
		FeeRate: btcjson.Float64(0.02),
	}, btcjson.Bool(false))

and FundRawTransaction by default does not make promises that your staking output will be at 0 index. So it can add change output at 0 position and staking output at index 1.

Simple fix is to set ChangePosition in FundRawTransactionOpts to 1 ,this way your staking output will always be at position 0 and change output at position 1 (though imo ideally functions in test would not make such assumptions 😅

Copy link
Collaborator

@KonradStaniec KonradStaniec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍 thanks for tackling it !

Only few cleanup comments

Required: true,
},
cli.Uint64Flag{
Name: txInclusionHeightFlag,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wodner whether this param is necessary i.e we have staking tx hash, we can retrieve staking transaction from the btc chain and check its height and based on that chose the param.

Non-blocking comment it can be tackled as future improvement imo.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had modified to get the Block Height from the tx hash in the CLI command, but that also added one more call to service, it was done this way to avoid sending the whole global params to the service app

proto/transaction.proto Outdated Show resolved Hide resolved
staker/stakerapp.go Outdated Show resolved Hide resolved
staker/stakerapp.go Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants