From f3924b6b155658f2788737398bf79b899dc500d6 Mon Sep 17 00:00:00 2001 From: Jonathan Chappelow Date: Thu, 5 May 2022 08:19:59 -0500 Subject: [PATCH] client/asset: custom address stringer The exchange wallets had an address decoder function for converting a string to a btcutil.Address, which is required for assets like BCH with special address formats. However, it is incorrect to then use the btcutil.Address String method go get a human readable address. This adds a complementary helper function for converting from a btcutil.Address to a correct string. Normally this is simply using the Address as a Stringer, but for BCH it is the new EncodeCashAddress function, which powers the existing RecodeCashAddress. This updates the btc baseWallet methods to use the new stringAddr to achieve the reverse of decodeAddr. This ensures that the string address stored in the fundingCoins is correct for the asset. This also ensures the recipient address in AuditContract is correct. I believe the main issue resolved by this is providing the correct address to the node.privKeyForAddress method when signing. This also provides the correct string to the getaddressinfo RPC. It is not clear if bitcoincashd was previously ok with the incorrect address strings when handling RPCs. This also removes the unused (*auditInfo).Recipient method, and fixes the documentation for the other methods since there is no longer an asset.AuditInfo *interface*, just a struct. --- client/asset/bch/bch.go | 33 ++------------ client/asset/btc/btc.go | 86 ++++++++++++++++++++++------------- client/asset/btc/rpcclient.go | 9 +++- client/asset/dcr/dcr.go | 21 +++------ dex/networks/bch/cashaddr.go | 9 ++++ dex/networks/btc/clone.go | 5 ++ dex/networks/btc/script.go | 2 +- 7 files changed, 88 insertions(+), 77 deletions(-) diff --git a/client/asset/bch/bch.go b/client/asset/bch/bch.go index 5b01f96117..b2e8c47d51 100644 --- a/client/asset/bch/bch.go +++ b/client/asset/bch/bch.go @@ -159,9 +159,11 @@ func NewWallet(cfg *asset.WalletConfig, logger dex.Logger, network dex.Network) DefaultFallbackFee: defaultFee, Segwit: false, LegacyBalance: true, - // Bitcoin Cash uses the Cash Address encoding, which is Bech32, but - // not indicative of segwit. We provide a custom encoder. - AddressDecoder: dexbch.DecodeCashAddress, + // Bitcoin Cash uses the Cash Address encoding, which is Bech32, but not + // indicative of segwit. We provide a custom encoder and decode to go + // to/from a btcutil.Address and a string. + AddressDecoder: dexbch.DecodeCashAddress, + AddressStringer: dexbch.EncodeCashAddress, // Bitcoin Cash has a custom signature hash algorithm. Since they don't // have segwit, Bitcoin Cash implemented a variation of the withdrawn // BIP0062 that utilizes Shnorr signatures. @@ -192,31 +194,6 @@ type BCHWallet struct { *btc.ExchangeWalletFullNode } -// Address converts the Bitcoin base58-encoded address returned by the embedded -// ExchangeWallet into a Cash Address. -func (bch *BCHWallet) Address() (string, error) { - btcAddrStr, err := bch.ExchangeWalletFullNode.Address() - if err != nil { - return "", err - } - return dexbch.RecodeCashAddress(btcAddrStr, bch.Net()) -} - -// AuditContract modifies the *asset.Contract returned by the ExchangeWallet -// AuditContract method by converting the Recipient to the Cash Address -// encoding. -func (bch *BCHWallet) AuditContract(coinID, contract, txData dex.Bytes, rebroadcast bool) (*asset.AuditInfo, error) { // AuditInfo has address - ai, err := bch.ExchangeWalletFullNode.AuditContract(coinID, contract, txData, rebroadcast) - if err != nil { - return nil, err - } - ai.Recipient, err = dexbch.RecodeCashAddress(ai.Recipient, bch.Net()) - if err != nil { - return nil, err - } - return ai, nil -} - // rawTxSigner signs the transaction using Bitcoin Cash's custom signature // hash and signing algorithm. func rawTxInSigner(btcTx *wire.MsgTx, idx int, subScript []byte, hashType txscript.SigHashType, btcKey *btcec.PrivateKey, val uint64) ([]byte, error) { diff --git a/client/asset/btc/btc.go b/client/asset/btc/btc.go index 27c7f1de9e..c664ddc7dc 100644 --- a/client/asset/btc/btc.go +++ b/client/asset/btc/btc.go @@ -228,7 +228,11 @@ type BTCCloneCFG struct { // AddressDecoder is an optional argument that can decode an address string // into btcutil.Address. If AddressDecoder is not supplied, // btcutil.DecodeAddress will be used. - AddressDecoder dexbtc.AddressDecoder + AddressDecoder dexbtc.AddressDecoder // string => btcutil.Address + // AddressStringer is an optional argument that can encode a btcutil.Address + // into an address string. If AddressStringer is not supplied, the + // (btcutil.Address).String method will be used. + AddressStringer dexbtc.AddressStringer // btcutil.Address => string, may be an override or just the String method // BlockDeserializer can be used in place of (*wire.MsgBlock).Deserialize. BlockDeserializer func([]byte) (*wire.MsgBlock, error) // ArglessChangeAddrRPC can be true if the getrawchangeaddress takes no @@ -332,27 +336,19 @@ func (op *output) wireOutPoint() *wire.OutPoint { // auditInfo is information about a swap contract on that blockchain. type auditInfo struct { output *output - recipient btcutil.Address + recipient btcutil.Address // caution: use stringAddr, not the Stringer contract []byte secretHash []byte expiration time.Time } -// Recipient returns a base58 string for the contract's receiving address. Part -// of the asset.AuditInfo interface. -func (ci *auditInfo) Recipient() string { - return ci.recipient.String() -} - // Expiration returns the expiration time of the contract, which is the earliest -// time that a refund can be issued for an un-redeemed contract. Part of the -// asset.AuditInfo interface. +// time that a refund can be issued for an un-redeemed contract. func (ci *auditInfo) Expiration() time.Time { return ci.expiration } -// Coin returns the output as an asset.Coin. Part of the asset.AuditInfo -// interface. +// Coin returns the output as an asset.Coin. func (ci *auditInfo) Coin() asset.Coin { return ci.output } @@ -587,8 +583,9 @@ type baseWallet struct { useLegacyBalance bool segwit bool signNonSegwit TxInSigner - estimateFee func(RawRequester, uint64) (uint64, error) + estimateFee func(RawRequester, uint64) (uint64, error) // TODO: resolve the awkwardness of an RPC-oriented func in a generic framework decodeAddr dexbtc.AddressDecoder + stringAddr dexbtc.AddressStringer tipMtx sync.RWMutex currentTip *block @@ -773,6 +770,7 @@ func newRPCWallet(requester RawRequesterWithContext, cfg *BTCCloneCFG, walletCon requester: requester, segwit: cfg.Segwit, decodeAddr: btc.decodeAddr, + stringAddr: btc.stringAddr, deserializeBlock: blockDeserializer, arglessChangeAddrRPC: cfg.ArglessChangeAddrRPC, legacyRawSends: cfg.LegacyRawFeeLimit, @@ -824,6 +822,13 @@ func newUnconnectedWallet(cfg *BTCCloneCFG, walletCfg *WalletConfig) (*baseWalle addrDecoder = cfg.AddressDecoder } + addrStringer := cfg.AddressStringer + if cfg.AddressStringer == nil { + addrStringer = func(addr btcutil.Address, _ *chaincfg.Params) (string, error) { + return addr.String(), nil + } + } + nonSegwitSigner := rawTxInSig if cfg.NonSegwitSigner != nil { nonSegwitSigner = cfg.NonSegwitSigner @@ -848,6 +853,7 @@ func newUnconnectedWallet(cfg *BTCCloneCFG, walletCfg *WalletConfig) (*baseWalle signNonSegwit: nonSegwitSigner, estimateFee: cfg.FeeEstimator, decodeAddr: addrDecoder, + stringAddr: addrStringer, walletInfo: cfg.WalletInfo, } @@ -887,12 +893,6 @@ func (btc *baseWallet) Info() *asset.WalletInfo { return btc.walletInfo } -// Net returns the ExchangeWallet's *chaincfg.Params. This is not part of the -// asset.Wallet interface, but is provided as a convenience for embedding types. -func (btc *baseWallet) Net() *chaincfg.Params { - return btc.chainParams -} - // Connect connects the wallet to the RPC server. Satisfies the dex.Connector // interface. func (btc *baseWallet) Connect(ctx context.Context) (*sync.WaitGroup, error) { @@ -1705,6 +1705,10 @@ func (btc *baseWallet) split(value uint64, lots uint64, outputs []*output, if err != nil { return nil, false, fmt.Errorf("error creating split transaction address: %w", err) } + addrStr, err := btc.stringAddr(addr, btc.chainParams) + if err != nil { + return nil, false, fmt.Errorf("failed to stringify the change address: %w", err) + } reqFunds := calc.RequiredOrderFundsAlt(value, swapInputSize, lots, nfo.SwapSizeBase, nfo.SwapSize, bumpedMaxRate) @@ -1734,7 +1738,7 @@ func (btc *baseWallet) split(value uint64, lots uint64, outputs []*output, fundingCoins = map[outPoint]*utxo{op.pt: { txHash: op.txHash(), vout: op.vout(), - address: addr.String(), + address: addrStr, amount: reqFunds, }} @@ -2054,12 +2058,18 @@ func (btc *baseWallet) Swap(swaps *asset.Swaps) ([]asset.Receipt, asset.Coin, ui btc.log.Errorf("failed to lock change output: %v", err) } + addrStr, err := btc.stringAddr(changeAddr, btc.chainParams) + if err != nil { + btc.log.Errorf("Failed to stringify address %v (default encoding): %v", changeAddr, err) + addrStr = changeAddr.String() // may or may not be able to retrieve the private keys for the next swap! + } + // Log it as a fundingCoin, since it is expected that this will be // chained into further matches. btc.fundingCoins[change.pt] = &utxo{ txHash: change.txHash(), vout: change.vout(), - address: changeAddr.String(), + address: addrStr, amount: change.value, } } @@ -2217,9 +2227,9 @@ func (btc *baseWallet) convertAuditInfo(ai *asset.AuditInfo) (*auditInfo, error) } return &auditInfo{ - output: newOutput(txHash, vout, ai.Coin.Value()), // *output - recipient: recip, // btcutil.Address - contract: ai.Contract, // []byte + output: newOutput(txHash, vout, ai.Coin.Value()), // *output + recipient: recip, // btcutil.Address + contract: ai.Contract, // []byte secretHash: ai.SecretHash, // []byte expiration: ai.Expiration, // time.Time }, nil @@ -2348,9 +2358,15 @@ func (btc *baseWallet) AuditContract(coinID, contract, txData dex.Bytes, rebroad }() } + addrStr, err := btc.stringAddr(receiver, btc.chainParams) + if err != nil { + btc.log.Errorf("Failed to stringify receiver address %v (default): %v", receiver, err) + addrStr = receiver.String() // potentially misleading AuditInfo.Recipient + } + return &asset.AuditInfo{ Coin: newOutput(txHash, vout, uint64(txOut.Value)), - Recipient: receiver.String(), + Recipient: addrStr, Contract: contract, SecretHash: secretHash, Expiration: time.Unix(int64(stamp), 0).UTC(), @@ -2741,7 +2757,7 @@ func (btc *baseWallet) Address() (string, error) { if err != nil { return "", err } - return addr.String(), nil + return btc.stringAddr(addr, btc.chainParams) } // NewAddress returns a new address from the wallet. This satisfies the @@ -3245,8 +3261,9 @@ func (btc *baseWallet) signTxAndAddChange(baseTx *wire.MsgTx, addr btcutil.Addre // Add the change output. vSize0 := dexbtc.MsgTxVBytes(baseTx) baseTx.AddTxOut(changeOutput) - changeSize := dexbtc.MsgTxVBytes(baseTx) - vSize0 // may be dexbtc.P2WPKHOutputSize - btc.log.Debugf("Change output size = %d, addr = %s", changeSize, addr.String()) + changeSize := dexbtc.MsgTxVBytes(baseTx) - vSize0 // may be dexbtc.P2WPKHOutputSize + addrStr, _ := btc.stringAddr(addr, btc.chainParams) // just for logging + btc.log.Debugf("Change output size = %d, addr = %s", changeSize, addrStr) vSize += changeSize fee := feeRate * vSize @@ -3328,7 +3345,11 @@ func (btc *baseWallet) broadcastTx(signedTx *wire.MsgTx) error { // createSig creates and returns the serialized raw signature and compressed // pubkey for a transaction input signature. func (btc *baseWallet) createSig(tx *wire.MsgTx, idx int, pkScript []byte, addr btcutil.Address, val uint64) (sig, pubkey []byte, err error) { - privKey, err := btc.node.privKeyForAddress(addr.String()) + addrStr, err := btc.stringAddr(addr, btc.chainParams) + if err != nil { + return nil, nil, err + } + privKey, err := btc.node.privKeyForAddress(addrStr) if err != nil { return nil, nil, err } @@ -3343,8 +3364,11 @@ func (btc *baseWallet) createSig(tx *wire.MsgTx, idx int, pkScript []byte, addr // input and the pubkey associated with the address. func (btc *baseWallet) createWitnessSig(tx *wire.MsgTx, idx int, pkScript []byte, addr btcutil.Address, val uint64, sigHashes *txscript.TxSigHashes) (sig, pubkey []byte, err error) { - - privKey, err := btc.node.privKeyForAddress(addr.String()) + addrStr, err := btc.stringAddr(addr, btc.chainParams) + if err != nil { + return nil, nil, err + } + privKey, err := btc.node.privKeyForAddress(addrStr) if err != nil { return nil, nil, err } diff --git a/client/asset/btc/rpcclient.go b/client/asset/btc/rpcclient.go index b9485df491..8e827b17b2 100644 --- a/client/asset/btc/rpcclient.go +++ b/client/asset/btc/rpcclient.go @@ -74,6 +74,7 @@ type rpcCore struct { requester RawRequesterWithContext segwit bool decodeAddr dexbtc.AddressDecoder + stringAddr dexbtc.AddressStringer deserializeBlock func([]byte) (*wire.MsgBlock, error) arglessChangeAddrRPC bool legacyRawSends bool @@ -395,7 +396,7 @@ func (wc *rpcClient) address(aType string) (btcutil.Address, error) { if err != nil { return nil, err } - return wc.decodeAddr(addrStr, wc.chainParams) + return wc.decodeAddr(addrStr, wc.chainParams) // we should consider returning a string } // signTx attempts to have the wallet sign the transaction inputs. @@ -497,7 +498,11 @@ func (wc *rpcClient) GetWalletInfo() (*GetWalletInfoResult, error) { // getaddressinfo or validateaddress RPC command. func (wc *rpcClient) getAddressInfo(addr btcutil.Address, method string) (*GetAddressInfoResult, error) { ai := new(GetAddressInfoResult) - return ai, wc.call(method, anylist{addr.String()}, ai) + addrStr, err := wc.stringAddr(addr, wc.chainParams) + if err != nil { + return nil, err + } + return ai, wc.call(method, anylist{addrStr}, ai) } // ownsAddress indicates if an address belongs to the wallet. diff --git a/client/asset/dcr/dcr.go b/client/asset/dcr/dcr.go index 36d67b51c9..b2e72edad1 100644 --- a/client/asset/dcr/dcr.go +++ b/client/asset/dcr/dcr.go @@ -235,24 +235,16 @@ func (op *output) wireOutPoint() *wire.OutPoint { // auditInfo is information about a swap contract on the blockchain, not // necessarily created by this wallet, as would be returned from AuditContract. -// auditInfo satisfies the asset.AuditInfo interface. type auditInfo struct { output *output secretHash []byte contract []byte - recipient stdaddr.Address + recipient stdaddr.Address // unused? expiration time.Time } -// Recipient is a base58 string for the contract's receiving address. Part of -// the asset.AuditInfo interface. -func (ci *auditInfo) Recipient() string { - return ci.recipient.String() -} - // Expiration is the expiration time of the contract, which is the earliest time -// that a refund can be issued for an un-redeemed contract. Part of the -// asset.AuditInfo interface. +// that a refund can be issued for an un-redeemed contract. func (ci *auditInfo) Expiration() time.Time { return ci.expiration } @@ -262,8 +254,7 @@ func (ci *auditInfo) Contract() dex.Bytes { return ci.contract } -// Coin returns the output as an asset.Coin. Part of the asset.AuditInfo -// interface. +// Coin returns the output as an asset.Coin. func (ci *auditInfo) Coin() asset.Coin { return ci.output } @@ -291,9 +282,9 @@ func convertAuditInfo(ai *asset.AuditInfo, chainParams *chaincfg.Params) (*audit } return &auditInfo{ - output: op, // *output - recipient: recip, // btcutil.Address - contract: ai.Contract, // []byte + output: op, // *output + recipient: recip, // btcutil.Address + contract: ai.Contract, // []byte secretHash: ai.SecretHash, // []byte expiration: ai.Expiration, // time.Time }, nil diff --git a/dex/networks/bch/cashaddr.go b/dex/networks/bch/cashaddr.go index f4e579d3d2..d4e6fa98e5 100644 --- a/dex/networks/bch/cashaddr.go +++ b/dex/networks/bch/cashaddr.go @@ -19,8 +19,17 @@ func RecodeCashAddress(addr string, net *chaincfg.Params) (string, error) { if err != nil { return "", err } + return EncodeCashAddress(btcAddr, net) +} +// EncodeCashAddress converts a btcutil.Address that the BTC backend into a Cash +// Address string. For example, if a pkScript was decoded to a btcutil.Address, +// the correct address string would not be from the Address' String method, but +// from EncodeCashAddress. This is more direct than doing +// RecodeCashAddress(addr.String()), which needlessly decodes the string. +func EncodeCashAddress(btcAddr btcutil.Address, net *chaincfg.Params) (string, error) { var bchAddr bchutil.Address + var err error switch at := btcAddr.(type) { case *btcutil.AddressPubKeyHash: bchAddr, err = bchutil.NewAddressPubKeyHash(btcAddr.ScriptAddress(), convertParams(net)) diff --git a/dex/networks/btc/clone.go b/dex/networks/btc/clone.go index 08a1d2c62a..b4d8a8571a 100644 --- a/dex/networks/btc/clone.go +++ b/dex/networks/btc/clone.go @@ -12,6 +12,11 @@ import ( // AddressDecoder decodes a string address to a btcutil.Address. type AddressDecoder func(addr string, net *chaincfg.Params) (btcutil.Address, error) +// AddressStringer converts the btcutil.Address into a string address. This may +// be different from using (btcutil.Address).String when there is a special +// encoding such as a BCH "Cash Address". +type AddressStringer func(btcutil.Address, *chaincfg.Params) (string, error) + // ReadCloneParams translates a CloneParams into a btcsuite chaincfg.Params. func ReadCloneParams(cloneParams *CloneParams) *chaincfg.Params { return &chaincfg.Params{ diff --git a/dex/networks/btc/script.go b/dex/networks/btc/script.go index 0b8d0af795..73325fcb66 100644 --- a/dex/networks/btc/script.go +++ b/dex/networks/btc/script.go @@ -647,7 +647,7 @@ func InputInfo(pkScript, redeemScript []byte, chainParams *chaincfg.Params) (*Sp } if nonStandard { return &SpendInfo{ - // SigScriptSize and WitnessSizecannot be determined, leave zero. + // SigScriptSize and WitnessSize cannot be determined, leave zero. ScriptAddrs: scriptAddrs, ScriptType: scriptType, NonStandardScript: true,