multi: add Bitcoin Cash #944

buck54321 · 2021-01-27T02:12:27Z

Add server and client support for Bitcoin Cash.

The primary challenge is that Bitcoin Cash uses a special address encoding called Cash Address that encodes a pubkey hash using Bech32, but does not signify segwit. Another challenge is that the Bitcoin Cash uses a different signature hash algorithm.

Bitcoin Cash also doesn't implement estimatesmartfee. The closest thing they have is estimatefee, and their version takes no confirmation target argument. I'll look more into how the algo works.

Similar to Litecoin, Bitcoin has not yet implemented the new sendrawtransaction argument signature, and still uses the older allowHighFees bool argument.

To solve these challenges, I've made liberal use of the github.com/gcash/bchd and github.com/gcash/bchutil packages, which appears to be under active maintenance. Regardless, the functionality we use is likely to be battle-tested at this point, so I think we're good to go.

IMPORTANT: Bitcoin Cash uses the same data directory, ~/.bitcoin, and the same executable names, bitcoind and bitcoin-cli, as Bitcoin Core. This makes life very difficult for both developers and users.

DO NOT start Bitcoin Cash without specifying a custom -datadir. IF YOU FAIL to specify a custom data directory, your Bitcoin Core data will be destroyed and YOU'LL HAVE TO RESYNC BITCOIN.

Avoid overwriting your Bitcoin block data by adding networkactive=1 to your .bitcoin/bitcoin.conf. 1 is the default value, but Bitcoin Cash doesn't support the setting, so will error if you forget to specify a custom datadir.

Many of our simnet scripts rely on our executables being in PATH. For dcrdex developers, it is expected that the executables (or symlinks) are named bitcoincashd and bitcoincash-cli. The live tests on the server backend also expect your mainnet directory to be at ~/.bch.

buck54321 · 2021-01-29T02:38:03Z

client/asset/interface.go

+type AuditInfo struct {
 	// Recipient is the string-encoded recipient address.
-	Recipient() string
+	Recipient string
 	// Expiration is the unix timestamp of the contract time lock expiration.
-	Expiration() time.Time
+	Expiration time.Time
 	// Coin is the coin that contains the contract.
-	Coin() Coin
+	Coin Coin
 	// Contract is the contract script.
-	Contract() dex.Bytes
+	Contract dex.Bytes
 	// SecretHash is the contract's secret hash.
-	SecretHash() dex.Bytes
+	SecretHash dex.Bytes
 }


I needed to modify the Recipient outside of client/asset/btc, so making this a field was a solution, but I didn't see any reason that any of these needed to be methods.

buck54321 · 2021-01-29T02:47:14Z

dex/networks/btc/script.go

-	if err != nil {
-		return nil, fmt.Errorf("error decoding sender address %s: %w", sender, err)
-	}
+func MakeContract(rAddr, sAddr btcutil.Address, secretHash []byte, lockTime int64, segwit bool, chainParams *chaincfg.Params) ([]byte, error) {


Changing these arguments from string to btcutil.Address was needed to make things work with the new customizable AddressDecoder. It also appears to be a simplification when the changes are taken as a whole.

buck54321 · 2021-01-29T02:48:52Z

server/asset/dcr/dcr_test.go

-	if contract.RefundAddress() != swap.refund.String() {
-		t.Fatalf("case 13 - wrong recipient. wanted '%s' got '%s'", contract.RefundAddress(), swap.refund.String())


The RefundAddress method remained unused, so I didn't replicate it as a field. Can always add it later.

chappjc · 2021-02-08T15:26:21Z

client/asset/btc/btc.go

@@ -361,6 +382,10 @@ type ExchangeWallet struct {
 	useSplitTx        bool
 	useLegacyBalance  bool
 	segwit            bool
+	legacyRawFeeLimit bool
+	signNonSegwit     TxInSigner
+	estimateFee       func(RPCClient, uint64) (uint64, error)


Just curious why you went the way of a function rather than a switch or bool as we've been doing for other asset differences.

The bch rpc for estimatefee is not the same as the legacy bitcoind estimatefee rpc. The bitcoin RPC at least took a confirmation target argument. Since it's not a difference between legacy/non-legacy, I figured a custom function is better.

chappjc · 2021-02-08T15:29:22Z

server/asset/btc/btc.go

 	// Verify contract and set refundAddress and swapAddress.
-	err = btc.auditContract(contract)
-	if err != nil {
-		return nil, err
-	}
-	return contract, nil
+	return btc.auditContract(output)


Big thumbs up for the much clearer semantics here.

chappjc · 2021-02-08T17:31:54Z

Just flagged this work for the 0.3 milestone, but I don't see why it can't be merged with the 0.2 release as an experimental and/or in-progress feature, perhaps just without full support for production mainnet use.

JoeGruffins

Unable to run tests or set up a wallet with this error:

error creating bch wallet: error getting wallet balance for bch: rawrequest error: -32601: Method not found

livetest.go:160: error getting available: rawrequest error: -32601: Method not found

Could you add the info about what to name executables and being careful not to erase bitcoin's datadir to a README.md in the dex/testing/bch folder?

buck54321 · 2021-02-12T09:59:36Z

Should be fixed now @JoeGruffins . Lost something in a rebase.

JoeGruffins

Working well on simnet.

chappjc

Reads great, but still testing. Just a few nits for now.

I'm happy to see the deinterfacing of both AuditInfo and Contract as it simplifies things and makes code dealing with those much more transparent.

chappjc · 2021-02-24T14:52:56Z

dex/networks/bch/params.go

+	// TestNet3Params are the clone parameters for testnet.
+	TestNet3Params = btc.ReadCloneParams(&btc.CloneParams{
+		PubKeyHashAddrID: bchchaincfg.TestNet3Params.LegacyPubKeyHashAddrID,
+		ScriptHashAddrID: bchchaincfg.TestNet3Params.LegacyScriptHashAddrID,
+		Bech32HRPSegwit:  "bchtest",
+		CoinbaseMaturity: 100,
+		Net:              0xf4f3e5f4,
+	})


Just reiterating what I mentioned in matrix chat - we might consider making testnet4 our preferred testnet.

https://bitcoincashresearch.org/t/testnet4-and-scalenet/148
https://gitlab.com/bitcoin-cash-node/bitcoin-cash-node/-/merge_requests/739#note_399231913

The main testnet for BCH, testnet3, has gotten bloated. People have used it for testing 32 MB block propagation for weeks on end. As a result, testnet3 now takes a long time to sync and is kinda annoying to work with. I think the problem here is that we’ve had two separate use cases for a testnet which don’t mesh well. I think we would be better served if we replaced testnet3 with two separate nets.
testnet4 is optimized for convenience. It’s intended to be kept lightweight and quick to sync. Its main purpose is for compatibility testing and feature testing. ...

Also nice about testnet4 is that it has unique default ports like 28333

Explorer: though https://tbch4.loping.net/

So I started switching everything to testnet 4, but quickly ran into some hurdles because we're using the bchd chaincfg.Params in a few places, and no such Params exist for testnet4. The clone params are easy and I know exactly which fields are being used there, but I can't say the same for the bchd paths, so we can't use the partial params like we do with clone params.

Can still do it, but punting for now. We should consider opening a PR at bchd with the new parameters too.

Sounds good. Was really just taking notes on testnet4 here.

chappjc · 2021-02-24T14:56:02Z

dex/networks/bch/params.go

+		ScriptHashAddrID: bchchaincfg.TestNet3Params.LegacyScriptHashAddrID,
+		Bech32HRPSegwit:  "bchtest",
+		CoinbaseMaturity: 100,
+		Net:              0xf4f3e5f4,


Net becomes 0xe2b7daaf on testnet4
https://gitlab.com/bitcoin-cash-node/bitcoin-cash-node/-/blob/a5e1d83f308f0428b401816b113e9cf3fd1ecb03/src/chainparams.cpp#L478

chappjc · 2021-02-24T15:02:32Z

dex/networks/bch/params.go

+		PubKeyHashAddrID: bchchaincfg.TestNet3Params.LegacyPubKeyHashAddrID,
+		ScriptHashAddrID: bchchaincfg.TestNet3Params.LegacyScriptHashAddrID,
+		Bech32HRPSegwit:  "bchtest",
+		CoinbaseMaturity: 100,


These four appear same on testnet4, https://gitlab.com/bitcoin-cash-node/bitcoin-cash-node/-/blob/a5e1d83f308f0428b401816b113e9cf3fd1ecb03/src/chainparams.cpp#L499-500 (6f and c4)

client/asset/btc/btc.go

go.mod

server/asset/bch/live_test.go

client/asset/btc/btc.go

chappjc · 2021-02-25T20:39:33Z

New conflicts with fee-estimate merged.

The primary challenge is that Bitcoin Cash uses a special address encoding called Cash Address that encodes a pubkey hash using Bech32, but does not signify segwit. Another challenge is that the Bitcoin Cash uses a different signature hash algorithm. Bitcoin Cash also doesn't implement estimatesmartfee. The closest thing they have is estimatefee, and their version takes no confirmation target argument. I'll look more into how the algo works. Similar to Litecoin, Bitcoin has not yet implemented the new sendrawtransaction argument signature, and still uses the older allowHighFees bool arguement. To solve these challenges, I've made liberal use of the github.com/gcash/bchd and github.com/gcash/bchutil packages, which appears to be under active maintenance. Regardless, the functionality we use is likely to be battle-tested at this point, so I think we're good to go. IMPORTANT: Bitcoin Cash uses the same data directory, ~/.bitcoin, and the same executable names, bitcoind and bitcoin-cli. This makes life very difficult for both developers and users. 1. DO NOT start Bitcoin Cash without specifying a custom -datadir. IF YOU FAIL to specify a custom data directory, your Bitcoin Core data will be destroyed and YOU'LL HAVE TO RESYNC BITCOIN. 2. Many of our simnet scripts rely on our executables being in PATH. For dcrdex developement, it is expected that the executables (or symlinks) are named bitcoincashd and bitcoincash-cli.

chappjc · 2021-03-04T17:28:30Z

client/asset/btc/btc.go

+	var rpcErr *dcrjson.RPCError
+	if errors.As(err, &rpcErr) && rpcErr.Code == dcrjson.ErrRPCMethodNotFound.Code {


Sorry to circle back on this, but I believe we should be using the constants in btcjson, although clearly we'd need to assert with dcrjson.RPCError because of our using dcr's rpcclient now. Either that or we just make a package level const with the expected value of ErrRPCMethodNotFound.Code to spare us importing btcjson just for one number. Just that the error codes between dcr and btc's rpc servers aren't necessarily the same, and I've noted commits with bitcoin where they've actually changed the codes and I'm sure we didn't change them too.

chappjc · 2021-03-04T17:34:31Z

server/asset/bch/live_test.go

 		if err != nil {
 			fmt.Printf("Connect failed: %v", err)
 			return 1
 		}
+		defer wg.Wait()


Look like it's going to start waiting before cancelling now when m.Run() completes because this defer is on the top of the defer stack. When looking at the previous revision where I had requested a change, I had envisioned moving the ctx, cancel := context.WithCancel(context.Background()) down to right before dexAsset.Connect(ctx) because ctx isn't used before that, thus allowing the cancel(); wg.Wait() to go in the same closure again, just down here. If Connect failed, you'd still have to cancel manually before return 1, or just leak the context because who cares TestMain is about to terminate.

buck54321 force-pushed the bch branch from 877bb35 to b5d9ecc Compare January 29, 2021 02:36

buck54321 commented Jan 29, 2021

View reviewed changes

buck54321 force-pushed the bch branch from b5d9ecc to b3a701d Compare January 29, 2021 02:52

buck54321 marked this pull request as ready for review January 29, 2021 02:55

chappjc reviewed Feb 8, 2021

View reviewed changes

chappjc added this to the 0.3 milestone Feb 8, 2021

buck54321 force-pushed the bch branch 3 times, most recently from b3a701d to a121002 Compare February 10, 2021 23:10

JoeGruffins reviewed Feb 12, 2021

View reviewed changes

JoeGruffins approved these changes Feb 15, 2021

View reviewed changes

JoeGruffins mentioned this pull request Feb 18, 2021

multi: Add Ethereum server asset. #979

Merged

chappjc requested changes Feb 24, 2021

View reviewed changes

chappjc reviewed Feb 24, 2021

View reviewed changes

server/asset/bch/live_test.go Outdated Show resolved Hide resolved

chappjc reviewed Feb 24, 2021

View reviewed changes

client/asset/btc/btc.go Show resolved Hide resolved

buck54321 added 6 commits March 1, 2021 13:14

cleanup

7672e70

smarten up simnet-setup.sh

f2b4f5f

support pubkey addresses. better clone config

9d839b7

missing doc

88799d1

whoops on legacy balance

7800378

buck54321 force-pushed the bch branch from 3314f00 to 2f20ae3 Compare March 1, 2021 20:21

review followup

c9c30c1

buck54321 force-pushed the bch branch from 2f20ae3 to c9c30c1 Compare March 1, 2021 20:48

chappjc reviewed Mar 4, 2021

View reviewed changes

chappjc suggestions

512cc79

chappjc approved these changes Mar 5, 2021

View reviewed changes

chappjc merged commit 542ed9b into decred:master Mar 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

multi: add Bitcoin Cash #944

multi: add Bitcoin Cash #944

buck54321 commented Jan 27, 2021 •

edited

Loading

buck54321 Jan 29, 2021

buck54321 Jan 29, 2021

buck54321 Jan 29, 2021

chappjc Feb 8, 2021

buck54321 Feb 10, 2021 •

edited

Loading

chappjc Feb 8, 2021

chappjc commented Feb 8, 2021

JoeGruffins left a comment •

edited

Loading

buck54321 commented Feb 12, 2021

JoeGruffins left a comment

chappjc left a comment

chappjc Feb 24, 2021

buck54321 Mar 1, 2021

chappjc Mar 1, 2021

chappjc Feb 24, 2021

chappjc Feb 24, 2021 •

edited

Loading

chappjc commented Feb 25, 2021

chappjc Mar 4, 2021 •

edited

Loading

chappjc Mar 4, 2021 •

edited

Loading

		if contract.RefundAddress() != swap.refund.String() {
		t.Fatalf("case 13 - wrong recipient. wanted '%s' got '%s'", contract.RefundAddress(), swap.refund.String())

		var rpcErr *dcrjson.RPCError
		if errors.As(err, &rpcErr) && rpcErr.Code == dcrjson.ErrRPCMethodNotFound.Code {

multi: add Bitcoin Cash #944

multi: add Bitcoin Cash #944

Conversation

buck54321 commented Jan 27, 2021 • 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

buck54321 Feb 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chappjc commented Feb 8, 2021

JoeGruffins left a comment • edited Loading

Choose a reason for hiding this comment

buck54321 commented Feb 12, 2021

JoeGruffins left a comment

Choose a reason for hiding this comment

chappjc 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

chappjc Feb 24, 2021 • edited Loading

Choose a reason for hiding this comment

chappjc commented Feb 25, 2021

chappjc Mar 4, 2021 • edited Loading

Choose a reason for hiding this comment

chappjc Mar 4, 2021 • edited Loading

Choose a reason for hiding this comment

buck54321 commented Jan 27, 2021 •

edited

Loading

buck54321 Feb 10, 2021 •

edited

Loading

JoeGruffins left a comment •

edited

Loading

chappjc Feb 24, 2021 •

edited

Loading

chappjc Mar 4, 2021 •

edited

Loading

chappjc Mar 4, 2021 •

edited

Loading