chore(submitter): remove dumpPrivKey #15

Lazar955 · 2024-08-15T14:54:41Z

This pull request removes the usage of the dumpPrivKey RPC method from the submitter and end-to-end (E2E) tests. The dumpPrivKey method is specific to legacy wallets and is no longer supported in the latest versions of bitcoind. Eliminating its usage is a necessary step for upgrading to newer versions of bitcoind, as its deprecation would otherwise hinder the upgrade process.

Updates the version of bitcoind to v27.0

References issue.

- Upgrade bitcoind version - Use descriptors to import priv key

submitter/relayer/utils.go

Lazar955 · 2024-08-15T20:37:36Z

e2etest/bitcoind_node_setup.go

-	// on app level. Descriptor wallets do not allow dumping private keys.
-	buff, _, err := h.m.ExecBitcoindCliCmd(h.t, []string{"createwallet", walletName, "false", "false", passphrase, "false", "false"})
+	// last arg is true which indicates we are using descriptor wallets they do not allow dumping private keys.
+	buff, _, err := h.m.ExecBitcoindCliCmd(h.t, []string{"createwallet", walletName, "false", "false", passphrase, "false", "true"})


Legacy wallets were "false", true for descriptor wallets

e2etest/test_manager.go

submitter/relayer/relayer.go

e2etest/test_manager.go

submitter/relayer/relayer.go

KonradStaniec · 2024-08-16T11:21:35Z

submitter/relayer/relayer.go

@@ -386,44 +386,54 @@ func (rl *Relayer) buildTxWithData(
 	rl.logger.Debugf("Building a BTC tx using %v with data %x", utxo.TxID.String(), data)
 	tx := wire.NewMsgTx(wire.TxVersion)

-	outPoint := wire.NewOutPoint(utxo.TxID, utxo.Vout)


the utxo *types.UTXO argument to the function is probably not needed now as all intput to the transaction as selected by rl.BTCWallet.FundRawTransaction function.

Given that we probably need two versions of this function:

first version for the first transaction, in which we allow wallet to chose inputs automatically

second version for the second transaction, in which we are able to specifiy input manually to use the transaction chaining

Not sure how FundRawTransaction would simplify things as we cannot control the inputs used by FundRawTransaction. As such, for (1), how do we know the value of the output is sufficient to fund the second tx for chaining?

If I understand our chaining mechanism correctly, want we want to do is:

send first transaction with pretty low fee rate, lets call it Tx1

then send the second transaction (Tx2)with higher fee rate and with input that is Tx1 change output (this is necessary to achieve the chaining)

Imo FundRawTransaction can help here a lot and simplify stuff as:

For the Tx1 we can just create unsigned and unfunded transaction with one output, and send it to FundRawTransaction, it will automatically choses inputs and add one change output which will have correct value to obey specified fee rate.

Then we can create Tx2 with one input (change output from Tx1 ) and one output and send it to FundRawTransaction. Now based on specified fee rate, endpoint will attach or not other inputs and will attach one change output to Tx2 . This way chaining will be preserved, and second tx will obey specified fee rate

Oh, I didn't realize in (2) we can inject one input from the change output of tx1. If the case, then it indeed improves a lot. Sounds good to me!

e2etest/test_manager.go

RafilxTenfen

PR looks good, ~~but what about dumpPrivKeyAndSignTx at the processCheckpoints case~~

My bad, tested the wrong branch locally

RafilxTenfen · 2024-08-16T11:43:41Z

e2etest/container/config.go

@@ -10,7 +10,7 @@ type ImageConfig struct {
 //nolint:deadcode
 const (
 	dockerBitcoindRepository = "lncm/bitcoind"
-	dockerBitcoindVersionTag = "v24.0.1"
+	dockerBitcoindVersionTag = "v26.1"


RafilxTenfen

Code changes looks good to me ^^

gitferry

Signing part lgtm! As per off-line discussion, my major concern is whether FundRawTransaction can handle the chaining without introducing much complexity. Maybe this part can be improved in a separate pr to not block here

gitferry · 2024-08-16T13:17:59Z

submitter/relayer/relayer.go

-	// add unlocking script into the input of the tx
-	tx, err = completeTxIn(tx, segwit, wif.PrivKey, utxo)
+
+	signedTx, _, err := rl.BTCWallet.SignRawTransactionWithWallet(tx)


Do we need to check the second output is true?

yep we should check this flag and retrun error if it is false.

This flags indicate whether all inputs were signed, from caller perspective it means that if it is false transaction cannot be sent to the BTC.

gitferry · 2024-08-17T01:30:43Z

submitter/relayer/relayer.go

@@ -386,44 +386,54 @@ func (rl *Relayer) buildTxWithData(
 	rl.logger.Debugf("Building a BTC tx using %v with data %x", utxo.TxID.String(), data)
 	tx := wire.NewMsgTx(wire.TxVersion)

-	outPoint := wire.NewOutPoint(utxo.TxID, utxo.Vout)


Not sure how FundRawTransaction would simplify things as we cannot control the inputs used by FundRawTransaction. As such, for (1), how do we know the value of the output is sufficient to fund the second tx for chaining?

KonradStaniec

We minor nits. Before merging lets wait for @gitferry approval as he is most familiar with this logic.

KonradStaniec · 2024-08-20T05:39:10Z

submitter/relayer/relayer.go

-	rl.logger.Debugf("Got a change address %v", changeAddr.String())
-	changeScript, err := txscript.PayToAddrScript(changeAddr)
+	feeRate := btcutil.Amount(rl.getFeeRate()).ToBTC()
+	changePosition := 1


I would make this a constant and the top of the file and use it to refer to change outputs wherever necessary, because now we have a lot magic numbers in the file e.g Transaction.TxOut[1]

KonradStaniec · 2024-08-20T05:42:28Z

submitter/relayer/relayer.go

-	// add unlocking script into the input of the tx
-	tx, err = completeTxIn(tx, segwit, wif.PrivKey, utxo)
+
+	signedTx, _, err := rl.BTCWallet.SignRawTransactionWithWallet(tx)


yep we should check this flag and retrun error if it is false.

This flags indicate whether all inputs were signed, from caller perspective it means that if it is false transaction cannot be sent to the BTC.

KonradStaniec · 2024-08-20T05:50:40Z

submitter/relayer/relayer.go

 	}
-	change := utxo.Amount - txFee
-	tx.AddTxOut(wire.NewTxOut(int64(change), changeScript))
+	change := changeAmount - txFee


imo this is not needed and you can use changeAmount instead

KonradStaniec · 2024-08-20T05:52:51Z

submitter/relayer/utils.go

 		return 0, err
 	}
+
 	btcTx, err := btcutil.NewTxFromBytes(txBytes.Bytes())


here instead of first serializing transaction, you can use btcutil.NewTx(tx) constructor, and remove serialization step

KonradStaniec · 2024-08-20T05:59:42Z

submitter/relayer/relayer.go

 }

 // resendSecondTxOfCheckpointToBTC resends the second tx of the checkpoint with bumpedFee
 func (rl *Relayer) resendSecondTxOfCheckpointToBTC(tx2 *types.BtcTxInfo, bumpedFee btcutil.Amount) (*types.BtcTxInfo, error) {
 	// set output value of the second tx to be the balance minus the bumped fee
 	// if the bumped fee is higher than the balance, then set the bumped fee to
 	// be equal to the balance to ensure the output value is not negative
-	balance := tx2.Utxo.Amount
+	balance := btcutil.Amount(tx2.Tx.TxOut[1].Value)
+
 	if bumpedFee > balance {
 		rl.logger.Debugf("the bumped fee %v Satoshis for the second tx is more than UTXO amount %v Satoshis",
 			bumpedFee, balance)
 		bumpedFee = balance


this is a bit debatable, as if we set bumpedFee = balance and later have tx2.Tx.TxOut[1].Value = int64(balance - bumpedFee) this means we will end up with output with value 0 that will be rejected by bitcoind as dust output.
This probably not for this pr, but some todo here for future fix would be good.

gitferry

Sorry for the delay. Some quick comments:

gitferry · 2024-08-20T09:00:38Z

submitter/relayer/relayer.go

 // ChainTwoTxAndSend consumes one utxo and build two chaining txs:
 // the second tx consumes the output of the first tx


Suggested change

// ChainTwoTxAndSend consumes one utxo and build two chaining txs:

// the second tx consumes the output of the first tx

// ChainTwoTxAndSend builds two chaining txs with the given data

// the second tx consumes the output of the first tx

gitferry · 2024-08-20T09:07:24Z

submitter/relayer/relayer.go

-	bumpedFee := newTx1Fee + newTx2Fee - ckptInfo.Tx1.Fee
-
-	return bumpedFee.MulF64(float64(rl.config.ResubmitFeeMultiplier))
+	return ckptInfo.Tx2.Fee.MulF64(rl.config.ResubmitFeeMultiplier)


why we don't want to estimate the fee here?

We've changed how we estimate fee, by using the fee value from the response of FundRawTransaction. This produces a different value (a bit higher fee) than we used to, which means when we estimate the fee here it wouldn't be the same way as we did, and in local tests resulting in a lower calculated bumpedFee needing a higher ResubmitFeeMultiplier. So I see it either as estimating fee ourselves or using the response from FundRawTransaction. Open to any other inputs, I might be missing something

This makes sense. Thanks for the explanation!

gitferry · 2024-08-20T09:08:35Z

submitter/relayer/relayer.go

 	// set output value of the second tx to be the balance minus the bumped fee
 	// if the bumped fee is higher than the balance, then set the bumped fee to
 	// be equal to the balance to ensure the output value is not negative


This seems outdated

gitferry · 2024-08-20T09:31:42Z

submitter/relayer/relayer.go

-	balance := tx2.Utxo.Amount
+	balance := btcutil.Amount(tx2.Tx.TxOut[changePosition].Value)
+
+	// todo: revise this as this means we will end up with output with value 0 that will be rejected by bitcoind as dust output.


can we make it an issue for reminder?

gitferry · 2024-08-20T09:33:55Z

submitter/relayer/relayer.go

 // buildTxWithData builds a tx with data inserted as OP_RETURN
 // note that OP_RETURN is set as the first output of the tx (index 0)
 // and the rest of the balance is sent to a new change address
 // as the second output with index 1


this doc string needs update

gitferry · 2024-08-20T09:35:19Z

submitter/relayer/relayer.go

-	tx.AddTxIn(txIn)
+	if firstTx != nil {
+		txID := firstTx.TxHash()
+		outPoint := wire.NewOutPoint(&txID, 1)


can we make 1 a self-explained constant?

gitferry · 2024-08-20T09:44:02Z

submitter/relayer/relayer.go

 		return nil, err
 	}

+	change := changeAmount - txFee


should we also check the change is higher than dust?

gitferry · 2024-08-20T09:49:37Z

submitter/relayer/relayer.go

 		return nil, err
 	}

+	change := changeAmount - txFee
+
 	rl.logger.Debugf("Successfully composed a BTC tx with balance of input: %v, "+


We probably need to remove the balance of input as we don't specify it anymore. And we need to make it a single line as otherwise the values are not well translated

gitferry

Great work! LGTM other than comments above

Lazar955 added 5 commits August 15, 2024 10:57

wip: removes dump priv key

ba9bd67

cleanup

bb8ad7e

remove dumpPrivKey from e2e

3928f78

- Upgrade bitcoind version - Use descriptors to import priv key

regen mocks

effbdee

linting

24dc4c7

Lazar955 commented Aug 15, 2024

View reviewed changes

submitter/relayer/utils.go Outdated Show resolved Hide resolved

Lazar955 added 2 commits August 15, 2024 22:26

cleanup

6ecb235

hygiene

1a905da

Lazar955 commented Aug 15, 2024

View reviewed changes

cleanup comments

9ff87a5

Lazar955 commented Aug 16, 2024

View reviewed changes

e2etest/test_manager.go Outdated Show resolved Hide resolved

hygiene

37d6109

Lazar955 marked this pull request as ready for review August 16, 2024 09:55

Lazar955 requested review from gitferry, KonradStaniec and RafilxTenfen August 16, 2024 09:55

KonradStaniec reviewed Aug 16, 2024

View reviewed changes

cleanup, use changePosition

a0d6c40

RafilxTenfen reviewed Aug 16, 2024

View reviewed changes

RafilxTenfen approved these changes Aug 16, 2024

View reviewed changes

gitferry reviewed Aug 17, 2024

View reviewed changes

Lazar955 added 2 commits August 19, 2024 13:38

tx chaining

30a7716

changes bump fee calculation

dcd74c2

KonradStaniec approved these changes Aug 20, 2024

View reviewed changes

pr comments, bump bitcoind to v27

f617220

Lazar955 requested a review from gitferry August 20, 2024 09:02

gitferry reviewed Aug 20, 2024

View reviewed changes

Lazar955 mentioned this pull request Aug 20, 2024

Refactor resendSecondTxOfCheckpointToBTC #16

Open

gitferry approved these changes Aug 20, 2024

View reviewed changes

pr comments fix

9d7a961

Lazar955 merged commit e9a1e24 into dev Aug 20, 2024
8 checks passed

Lazar955 deleted the lazar/remove-dump-privkey branch August 20, 2024 14:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(submitter): remove dumpPrivKey #15

chore(submitter): remove dumpPrivKey #15

Lazar955 commented Aug 15, 2024 •

edited

Loading

Lazar955 Aug 15, 2024

KonradStaniec Aug 16, 2024

gitferry Aug 17, 2024

KonradStaniec Aug 19, 2024

gitferry Aug 19, 2024

RafilxTenfen left a comment •

edited

Loading

RafilxTenfen Aug 16, 2024

RafilxTenfen left a comment •

edited

Loading

gitferry left a comment •

edited

Loading

gitferry Aug 16, 2024

KonradStaniec Aug 20, 2024

gitferry Aug 17, 2024

KonradStaniec left a comment

KonradStaniec Aug 20, 2024

KonradStaniec Aug 20, 2024

KonradStaniec Aug 20, 2024

KonradStaniec Aug 20, 2024

KonradStaniec Aug 20, 2024

gitferry left a comment

gitferry Aug 20, 2024

gitferry Aug 20, 2024

Lazar955 Aug 20, 2024

gitferry Aug 20, 2024

gitferry Aug 20, 2024

gitferry Aug 20, 2024

gitferry Aug 20, 2024

gitferry Aug 20, 2024

gitferry Aug 20, 2024

gitferry Aug 20, 2024

gitferry left a comment

		// ChainTwoTxAndSend consumes one utxo and build two chaining txs:
		// the second tx consumes the output of the first tx

chore(submitter): remove dumpPrivKey #15

chore(submitter): remove dumpPrivKey #15

Conversation

Lazar955 commented Aug 15, 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

RafilxTenfen left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RafilxTenfen left a comment • edited Loading

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

KonradStaniec 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

gitferry 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

gitferry left a comment

Choose a reason for hiding this comment

Lazar955 commented Aug 15, 2024 •

edited

Loading

RafilxTenfen left a comment •

edited

Loading

RafilxTenfen left a comment •

edited

Loading

gitferry left a comment •

edited

Loading