refactor: (BitcoinRBF-Step2): some minimum code refactoring for Bitco…

…in RBF adoption (#3381)

* minimum code refactor for bitcoin RBF

* add changelog entry

* add unit test for FeeRateToSatPerByte

* make changelog descriptive; rename specialHandleFeeRate as GetFeeRateForRegnetAndTestnet; code simplification

* renaming sample function; make switch case in PostGasPrice to estimate fee rate according to Bitcoin network type

* remove redundant variable description

* make AddWithdrawTxOutputs a one-line call

* rename sample function BTCAddressP2WPKH

* remove unused test and comment

* include coin type to error message; make code cleaner

* return error if failed to get signer address; add BTCPayToAddrScript as a method of TSS PubKey

* Update zetaclient/chains/bitcoin/signer/signer.go

Replace 'Msgf' with 'Msg'

Co-authored-by: Dmitry S <[email protected]>

* remove redundant test functions; use testlog for unit test

* create observer in test suite; use testlog package

* add description to fee estimation formula

* use structured logs

* make AddTxInputs independent method

* add comments to explain function arguments; improve error wrapping; code simplification

* replace ifs with switch case; return original err without overwriting

* seems safe to remove panic recovery in FetchUTXOs

* move Telemetry update to the line before acquiring observer lock

* use testlog package

* use retry package for Bitcoin tx broadcasting; let SaveBroadcastedTx return error

* use named return values to make GetEstimatedFeeRate more readable

* move utxo unit tests to utxos.go and improved unit tests

* wrap RPC error in LoadLastBlockScanned

* move last scanned block to log field; use Opt function for test suite

* move values to log fields

* add unit test for FetchUTXOs

* add unit for SignWithdrawTx; use structured log

* avoid creating log field map and add log fields right on the logger

* fix client.GetEstimatedFeeRate

* Fix loadBroadcastedTxMap

* Fix SelectedUTXOs

* Fix log naming

* fix e2e logging

* Fix setPendingNonce

* don't use GasPriorityFee as it's always empty

* update changelog

---------

Co-authored-by: Dmitry S <[email protected]>

changelog.md

@@ -21,6 +21,7 @@
 * [3332](https://github.com/zeta-chain/node/pull/3332) - implement orchestrator V2. Move BTC observer-signer to V2
 * [3360](https://github.com/zeta-chain/node/pull/3360) - update protocol contract imports using consolidated path
 * [3349](https://github.com/zeta-chain/node/pull/3349) - implement new bitcoin rpc in zetaclient with improved performance and observability
+* [3381](https://github.com/zeta-chain/node/pull/3381) - split Bitcoin observer and signer into small files and organize outbound logic into reusable/testable functions; renaming, type unification, etc.
 * [3390](https://github.com/zeta-chain/node/pull/3390) - orchestrator V2: EVM observer-signer
 * [3426](https://github.com/zeta-chain/node/pull/3426) - use protocol contracts V2 with Bitcoin deposits
 * [3326](https://github.com/zeta-chain/node/pull/3326) - improve error messages for cctx status object

testutil/sample/crypto.go

@@ -8,14 +8,15 @@ import (
 	"strconv"
 	"testing"
 
-	"github.com/btcsuite/btcd/btcec/v2"
 	"github.com/btcsuite/btcd/btcutil"
 	"github.com/btcsuite/btcd/chaincfg"
 	"github.com/btcsuite/btcd/chaincfg/chainhash"
+	"github.com/btcsuite/btcd/txscript"
 	"github.com/cometbft/cometbft/crypto/secp256k1"
 	"github.com/cosmos/cosmos-sdk/crypto/keys/ed25519"
 	cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types"
 	sdk "github.com/cosmos/cosmos-sdk/types"
+	secp "github.com/decred/dcrd/dcrec/secp256k1/v4"
 	ethcommon "github.com/ethereum/go-ethereum/common"
 	ethtypes "github.com/ethereum/go-ethereum/core/types"
 	ethcrypto "github.com/ethereum/go-ethereum/crypto"
@@ -90,16 +91,24 @@ func EthAddressFromRand(r *rand.Rand) ethcommon.Address {
 	return ethcommon.BytesToAddress(sdk.AccAddress(PubKey(r).Address()).Bytes())
 }
 
-// BtcAddressP2WPKH returns a sample btc P2WPKH address
-func BtcAddressP2WPKH(t *testing.T, net *chaincfg.Params) string {
-	privateKey, err := btcec.NewPrivateKey()
+// BTCAddressP2WPKH returns a sample Bitcoin Pay-to-Witness-Public-Key-Hash (P2WPKH) address
+func BTCAddressP2WPKH(t *testing.T, r *rand.Rand, net *chaincfg.Params) *btcutil.AddressWitnessPubKeyHash {
+	privateKey, err := secp.GeneratePrivateKeyFromRand(r)
 	require.NoError(t, err)
 
 	pubKeyHash := btcutil.Hash160(privateKey.PubKey().SerializeCompressed())
 	addr, err := btcutil.NewAddressWitnessPubKeyHash(pubKeyHash, net)
 	require.NoError(t, err)
 
-	return addr.String()
+	return addr
+}
+
+// BtcAddressP2WPKH returns a pkscript for a sample btc P2WPKH address
+func BTCAddressP2WPKHScript(t *testing.T, r *rand.Rand, net *chaincfg.Params) []byte {
+	addr := BTCAddressP2WPKH(t, r, net)
+	script, err := txscript.PayToAddrScript(addr)
+	require.NoError(t, err)
+	return script
 }
 
 // SolanaPrivateKey returns a sample solana private key

x/crosschain/types/cctx_test.go

@@ -138,10 +138,11 @@ func Test_SetRevertOutboundValues(t *testing.T) {
 	})
 
 	t.Run("successfully set BTC revert address V1", func(t *testing.T) {
+		r := sample.Rand()
 		cctx := sample.CrossChainTx(t, "test")
 		cctx.InboundParams.SenderChainId = chains.BitcoinTestnet.ChainId
 		cctx.OutboundParams = cctx.OutboundParams[:1]
-		cctx.RevertOptions.RevertAddress = sample.BtcAddressP2WPKH(t, &chaincfg.TestNet3Params)
+		cctx.RevertOptions.RevertAddress = sample.BTCAddressP2WPKH(t, r, &chaincfg.TestNet3Params).String()
 
 		err := cctx.AddRevertOutbound(100)
 		require.NoError(t, err)

x/crosschain/types/revert_options_test.go

@@ -49,7 +49,8 @@ func TestRevertOptions_GetEVMRevertAddress(t *testing.T) {
 
 func TestRevertOptions_GetBTCRevertAddress(t *testing.T) {
 	t.Run("valid Bitcoin revert address", func(t *testing.T) {
-		addr := sample.BtcAddressP2WPKH(t, &chaincfg.TestNet3Params)
+		r := sample.Rand()
+		addr := sample.BTCAddressP2WPKH(t, r, &chaincfg.TestNet3Params).String()
 		actualAddr, valid := types.RevertOptions{
 			RevertAddress: addr,
 		}.GetBTCRevertAddress(chains.BitcoinTestnet.ChainId)

zetaclient/chains/bitcoin/client/client.go

@@ -38,6 +38,7 @@ import (
 	"github.com/rs/zerolog"
 	"github.com/tendermint/btcd/chaincfg"
 
+	pkgchains "github.com/zeta-chain/node/pkg/chains"
 	"github.com/zeta-chain/node/zetaclient/config"
 	"github.com/zeta-chain/node/zetaclient/logs"
 	"github.com/zeta-chain/node/zetaclient/metrics"
@@ -48,6 +49,7 @@ type Client struct {
 	hostURL    string
 	client     *http.Client
 	clientName string
+	isRegnet   bool
 	config     config.BTCConfig
 	params     chains.Params
 	logger     zerolog.Logger
@@ -81,14 +83,18 @@ func New(cfg config.BTCConfig, chainID int64, logger zerolog.Logger, opts ...Opt
 		return nil, errors.Wrap(err, "unable to resolve chain params")
 	}
 
-	clientName := fmt.Sprintf("btc:%d", chainID)
+	var (
+		clientName = fmt.Sprintf("btc:%d", chainID)
+		isRegnet   = pkgchains.IsBitcoinRegnet(chainID)
+	)
 
 	c := &Client{
 		hostURL:    normalizeHostURL(cfg.RPCHost, true),
 		client:     defaultHTTPClient(),
 		config:     cfg,
 		params:     params,
 		clientName: clientName,
+		isRegnet:   isRegnet,
 		logger: logger.With().
 			Str(logs.FieldModule, "btc_client").
 			Int64(logs.FieldChain, chainID).

zetaclient/chains/bitcoin/client/client_test.go

@@ -137,7 +137,7 @@ func TestClientLive(t *testing.T) {
 
 		require.NoError(t, err)
 		require.Len(t, inbounds, 1)
-		require.Equal(t, inbounds[0].Value, 0.0001)
+		require.Equal(t, inbounds[0].Value+inbounds[0].DepositorFee, 0.0001)
 		require.Equal(t, inbounds[0].ToAddress, "tb1qsa222mn2rhdq9cruxkz8p2teutvxuextx3ees2")
 
 		// the text memo is base64 std encoded string:DSRR1RmDCwWmxqY201/TMtsJdmA=
@@ -332,7 +332,7 @@ func TestClientLive(t *testing.T) {
 		require.NoError(t, err)
 
 		// go back whatever blocks as needed
-		endBlock := startBlock - 100
+		endBlock := startBlock - 1
 
 		// loop through mempool.space blocks backwards
 		for bn := startBlock; bn >= endBlock; {

zetaclient/chains/bitcoin/client/helpers.go

@@ -9,6 +9,16 @@ import (
 	"github.com/btcsuite/btcd/btcutil"
 	"github.com/btcsuite/btcd/chaincfg/chainhash"
 	"github.com/pkg/errors"
+
+	"github.com/zeta-chain/node/zetaclient/chains/bitcoin/common"
+)
+
+const (
+	// FeeRateRegnet is the hardcoded fee rate for regnet
+	FeeRateRegnet = 1
+
+	// maxBTCSupply is the maximum supply of Bitcoin
+	maxBTCSupply = 21000000.0
 )
 
 // GetBlockVerboseByStr alias for GetBlockVerbose
@@ -104,6 +114,30 @@ func (c *Client) GetRawTransactionResult(ctx context.Context,
 	}
 }
 
+// GetEstimatedFeeRate gets estimated smart fee rate (sat/vB) targeting given block confirmation
+func (c *Client) GetEstimatedFeeRate(ctx context.Context, confTarget int64) (satsPerByte int64, err error) {
+	// RPC 'EstimateSmartFee' is not available in regnet
+	if c.isRegnet {
+		return FeeRateRegnet, nil
+	}
+
+	feeResult, err := c.EstimateSmartFee(ctx, confTarget, &types.EstimateModeEconomical)
+	switch {
+	case err != nil:
+		return 0, errors.Wrap(err, "unable to estimate smart fee")
+	case feeResult.Errors != nil:
+		return 0, fmt.Errorf("fee result contains errors: %s", feeResult.Errors)
+	case feeResult.FeeRate == nil:
+		return 0, errors.New("nil fee rate")
+	}
+
+	feeRate := *feeResult.FeeRate
+	if feeRate <= 0 || feeRate >= maxBTCSupply {
+		return 0, fmt.Errorf("invalid fee rate: %f", feeRate)
+	}
+	return common.FeeRateToSatPerByte(feeRate), nil
+}
+
 // GetTransactionFeeAndRate gets the transaction fee and rate for a given tx result
 func (c *Client) GetTransactionFeeAndRate(ctx context.Context, rawResult *types.TxRawResult) (int64, int64, error) {
 	var (

zetaclient/chains/bitcoin/client/mockgen.go

@@ -45,6 +45,7 @@ type client interface {
 
 	SendRawTransaction(ctx context.Context, tx *wire.MsgTx, allowHighFees bool) (*hash.Hash, error)
 
+	GetEstimatedFeeRate(ctx context.Context, confTarget int64) (int64, error)
 	GetTransactionFeeAndRate(ctx context.Context, tx *types.TxRawResult) (int64, int64, error)
 	EstimateSmartFee(
 		ctx context.Context,

zetaclient/chains/bitcoin/common/fee.go

@@ -5,7 +5,6 @@ import (
 	"encoding/hex"
 	"fmt"
 	"math"
-	"math/big"
 
 	"github.com/btcsuite/btcd/blockchain"
 	"github.com/btcsuite/btcd/btcjson"
@@ -20,19 +19,20 @@ import (
 
 const (
 	// constants related to transaction size calculations
-	bytesPerKB           = 1000
-	bytesPerInput        = 41           // each input is 41 bytes
-	bytesPerOutputP2TR   = 43           // each P2TR output is 43 bytes
-	bytesPerOutputP2WSH  = 43           // each P2WSH output is 43 bytes
-	bytesPerOutputP2WPKH = 31           // each P2WPKH output is 31 bytes
-	bytesPerOutputP2SH   = 32           // each P2SH output is 32 bytes
-	bytesPerOutputP2PKH  = 34           // each P2PKH output is 34 bytes
-	bytesPerOutputAvg    = 37           // average size of all above types of outputs (36.6 bytes)
-	bytes1stWitness      = 110          // the 1st witness incurs about 110 bytes and it may vary
-	bytesPerWitness      = 108          // each additional witness incurs about 108 bytes and it may vary
-	OutboundBytesMin     = uint64(239)  // 239vB == EstimateSegWitTxSize(2, 2, toP2WPKH)
-	OutboundBytesMax     = uint64(1543) // 1543v == EstimateSegWitTxSize(21, 2, toP2TR)
-	OutboundBytesAvg     = uint64(245)  // 245vB is a suggested gas limit for zetacore
+	bytesPerInput        = 41          // each input is 41 bytes
+	bytesPerOutputP2TR   = 43          // each P2TR output is 43 bytes
+	bytesPerOutputP2WSH  = 43          // each P2WSH output is 43 bytes
+	bytesPerOutputP2WPKH = 31          // each P2WPKH output is 31 bytes
+	bytesPerOutputP2SH   = 32          // each P2SH output is 32 bytes
+	bytesPerOutputP2PKH  = 34          // each P2PKH output is 34 bytes
+	bytesPerOutputAvg    = 37          // average size of all above types of outputs (36.6 bytes)
+	bytes1stWitness      = 110         // the 1st witness incurs about 110 bytes and it may vary
+	bytesPerWitness      = 108         // each additional witness incurs about 108 bytes and it may vary
+	OutboundBytesMin     = int64(239)  // 239vB == EstimateOutboundSize(2, 2, toP2WPKH)
+	OutboundBytesMax     = int64(1543) // 1543v == EstimateOutboundSize(21, 2, toP2TR)
+
+	// bytesPerKB is the number of vB in a KB
+	bytesPerKB = 1000
 
 	// defaultDepositorFeeRate is the default fee rate for depositor fee, 20 sat/vB
 	defaultDepositorFeeRate = 20
@@ -49,6 +49,7 @@ var (
 	BtcOutboundBytesDepositor = OutboundSizeDepositor()
 
 	// BtcOutboundBytesWithdrawer is the outbound size incurred by the withdrawer: 177vB
+	// This will be the suggested gas limit used for zetacore
 	BtcOutboundBytesWithdrawer = OutboundSizeWithdrawer()
 
 	// DefaultDepositorFee is the default depositor fee is 0.00001360 BTC (20 * 68vB / 100000000)
@@ -67,34 +68,35 @@ type RPC interface {
 // DepositorFeeCalculator is a function type to calculate the Bitcoin depositor fee
 type DepositorFeeCalculator func(context.Context, RPC, *btcjson.TxRawResult, *chaincfg.Params) (float64, error)
 
-// FeeRateToSatPerByte converts a fee rate in BTC/KB to sat/byte.
-func FeeRateToSatPerByte(rate float64) *big.Int {
+// FeeRateToSatPerByte converts a fee rate from BTC/KB to sat/vB.
+func FeeRateToSatPerByte(rate float64) int64 {
+	satPerKB := rate * btcutil.SatoshiPerBitcoin
 	// #nosec G115 always in range
-	satPerKB := new(big.Int).SetInt64(int64(rate * btcutil.SatoshiPerBitcoin))
-	return new(big.Int).Div(satPerKB, big.NewInt(bytesPerKB))
+	return int64(satPerKB / bytesPerKB)
 }
 
 // WiredTxSize calculates the wired tx size in bytes
-func WiredTxSize(numInputs uint64, numOutputs uint64) uint64 {
+func WiredTxSize(numInputs uint64, numOutputs uint64) int64 {
 	// Version 4 bytes + LockTime 4 bytes + Serialized varint size for the
 	// number of transaction inputs and outputs.
 	// #nosec G115 always positive
-	return uint64(8 + wire.VarIntSerializeSize(numInputs) + wire.VarIntSerializeSize(numOutputs))
+	return int64(8 + wire.VarIntSerializeSize(numInputs) + wire.VarIntSerializeSize(numOutputs))
 }
 
 // EstimateOutboundSize estimates the size of an outbound in vBytes
-func EstimateOutboundSize(numInputs uint64, payees []btcutil.Address) (uint64, error) {
-	if numInputs == 0 {
+func EstimateOutboundSize(numInputs int64, payees []btcutil.Address) (int64, error) {
+	if numInputs <= 0 {
 		return 0, nil
 	}
 	// #nosec G115 always positive
 	numOutputs := 2 + uint64(len(payees))
-	bytesWiredTx := WiredTxSize(numInputs, numOutputs)
+	// #nosec G115 checked positive
+	bytesWiredTx := WiredTxSize(uint64(numInputs), numOutputs)
 	bytesInput := numInputs * bytesPerInput
-	bytesOutput := uint64(2) * bytesPerOutputP2WPKH // new nonce mark, change
+	bytesOutput := int64(2) * bytesPerOutputP2WPKH // new nonce mark, change
 
 	// calculate the size of the outputs to payees
-	bytesToPayees := uint64(0)
+	bytesToPayees := int64(0)
 	for _, to := range payees {
 		sizeOutput, err := GetOutputSizeByAddress(to)
 		if err != nil {
@@ -112,7 +114,7 @@ func EstimateOutboundSize(numInputs uint64, payees []btcutil.Address) (uint64, e
 }
 
 // GetOutputSizeByAddress returns the size of a tx output in bytes by the given address
-func GetOutputSizeByAddress(to btcutil.Address) (uint64, error) {
+func GetOutputSizeByAddress(to btcutil.Address) (int64, error) {
 	switch addr := to.(type) {
 	case *btcutil.AddressTaproot:
 		if addr == nil {
@@ -145,16 +147,16 @@ func GetOutputSizeByAddress(to btcutil.Address) (uint64, error) {
 }
 
 // OutboundSizeDepositor returns outbound size (68vB) incurred by the depositor
-func OutboundSizeDepositor() uint64 {
+func OutboundSizeDepositor() int64 {
 	return bytesPerInput + bytesPerWitness/blockchain.WitnessScaleFactor
 }
 
 // OutboundSizeWithdrawer returns outbound size (177vB) incurred by the withdrawer (1 input, 3 outputs)
-func OutboundSizeWithdrawer() uint64 {
+func OutboundSizeWithdrawer() int64 {
 	bytesWiredTx := WiredTxSize(1, 3)
-	bytesInput := uint64(1) * bytesPerInput         // nonce mark
-	bytesOutput := uint64(2) * bytesPerOutputP2WPKH // 2 P2WPKH outputs: new nonce mark, change
-	bytesOutput += bytesPerOutputAvg                // 1 output to withdrawer's address
+	bytesInput := int64(1) * bytesPerInput         // nonce mark
+	bytesOutput := int64(2) * bytesPerOutputP2WPKH // 2 P2WPKH outputs: new nonce mark, change
+	bytesOutput += bytesPerOutputAvg               // 1 output to withdrawer's address
 
 	return bytesWiredTx + bytesInput + bytesOutput + bytes1stWitness/blockchain.WitnessScaleFactor
 }
@@ -255,7 +257,7 @@ func CalcDepositorFee(
 
 // GetRecentFeeRate gets the highest fee rate from recent blocks
 // Note: this method should be used for testnet ONLY
-func GetRecentFeeRate(ctx context.Context, rpc RPC, netParams *chaincfg.Params) (uint64, error) {
+func GetRecentFeeRate(ctx context.Context, rpc RPC, netParams *chaincfg.Params) (int64, error) {
 	// should avoid using this method for mainnet
 	if netParams.Name == chaincfg.MainNetParams.Name {
 		return 0, errors.New("GetRecentFeeRate should not be used for mainnet")
@@ -295,6 +297,5 @@ func GetRecentFeeRate(ctx context.Context, rpc RPC, netParams *chaincfg.Params)
 		highestRate = defaultTestnetFeeRate
 	}
 
-	// #nosec G115 always in range
-	return uint64(highestRate), nil
+	return highestRate, nil
 }