-
Notifications
You must be signed in to change notification settings - Fork 97
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
multi: add support for ZCash #1570
Conversation
buck54321
commented
Apr 7, 2022
•
edited
Loading
edited
324335e
to
0d2d75b
Compare
Might want to keep an eye on https://github.com/zcash/lightwalletd too for a possible built-in light wallet. It doesn't appear to be suitable as is, but not certain. |
b563c4c
to
e999610
Compare
Will review shorty. Wanna rebase away the first two commits that are already merged (the first one without the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, I was able to do a swap on simnet. Haven't tested with testnet yet. For some reason the dex client is constantly sending getbestblockhash
, getnetworkinfo
, and getblockchaininfo
calls to the zcash client when I'm running on simnet. It seems a bit excessive.. there are at least 2-3 calls every second.
serializeTx func(*wire.MsgTx) ([]byte, error) | ||
deserializeBlock func([]byte) (*wire.MsgBlock, error) | ||
hashTx func(*wire.MsgTx) *chainhash.Hash | ||
numericGetRawTxRPC bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could use a doc saying this rpc method has different args in zcash.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some thoughts on sign.go to start
client/asset/zec/sign.go
Outdated
// https://github.com/zcash/zips/blob/main/zip-0243.rst#specification | ||
// which extends https://zips.z.cash/zip-0143#specification | ||
// See also implementation @ | ||
// https://github.com/iqoption/zecutil/blob/master/sign.go |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sign.go file needs a license comment at the top. It's unfortunate that zecutil completely threw away bchutil's license, and double unfortunate that libzec-go is gpl. I think with some minor rewriting and using our own code style and idioms to make this properly ours we're good though. It's a short function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dex/networks/zec
Updated for NU5. I expect that ZCash won't be live on DEX when the upgrade goes live on May 31st, but we're ready-ish for the upgrade if it is. For a ZCash upgrade, an operator should be expected to shut down for the upgrade, since old version txs won't be accepted on the new chain, per this upgrade guide. I'm syncing testnet and then I'll search for some NU5 txs to use for tests. |
9d20bcc
to
2003811
Compare
I put some feelers in, and it seems to me like the calls to The I'm not seeing a lot of I do see some |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! 🚀 🔒 🎆
Just need a resolve.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow. Thats a lot of zcash. My eyes started to glaze over at tx.go
I've not been able to trade so far with :
2022-05-19 23:23:46.860 [ERR] CORE: notify: |ERROR| (order) Swap send error - Error encountered sending a swap output(s) worth 1.00000000 ZEC on order 117411dd - Order: 117411dd464a5b5f40e3fc40c32f5d45cf0fbc88d1fa38fca69ab9ec668b1090
2022-05-19 23:23:46.860 [ERR] CORE: 127.0.0.1:17273 tick: {swapMatches order 117411dd464a5b5f40e3fc40c32f5d45cf0fbc88d1fa38fca69ab9ec668b1090 - {error sending swap transaction: signing error: tx signing error: rawrequest (signrawtransaction) error: -22: TX decode failed, raw tx: 0500000001ce67eab2739d1db57d16dcc8457cd0f1028c424459a5052ef4d65ca59cf841150100000000ffffffff0100e1f5050000000017a914c83a74347ff867cade02a48197b39345b8635b018700000000}}
I also attempted to get the core simnet tests running but they failed. Can do in another pr though.
Noticed a couple things maybe unrelated to this pr.
If I start to make a ZEC wallet but I put in a password, it errors, which is fine. But if I close that box then I see this for the wallet:
And I can click on that box to unlock the wallet that isnt there.
Also, I was not able to pay the fee with bitcoin for the simnet tests. May be a problem with the simnet tests.
$ zcashd --version
Zcash Daemon version v5.0.0
client/asset/zec/sign.go
Outdated
|
||
func validateSigHashType(v txscript.SigHashType) bool { | ||
return v == txscript.SigHashAll || v == txscript.SigHashNone || v == txscript.SigHashSingle || | ||
v == txscript.SigHashAnyOneCanPay || v == 0x82 || v == 0x83 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are these last two hex?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't seem to figure it out, but I'm basing it on
Consensus rule: [NU5 onward] Any SIGHASH type encoding used in a version 5 transaction MUST be the
canonical encoding of one of the defined SIGHASH types, i.e. one of 0x01, 0x02, 0x03, 0x81, 0x82, or 0x83.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would that be the bit-or of SIGHASH_ANYONECANPAY with either SIGHASH_NONE or SIGHASH_SINGLE?
/** Signature hash types/flags */
enum
{
SIGHASH_ALL = 1,
SIGHASH_NONE = 2,
SIGHASH_SINGLE = 3,
SIGHASH_ANYONECANPAY = 0x80,
};
Now that I say that, I think we're missing 0x81 too (SIGHASH_ALL | SIGHASH_ANYONECANPAY)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, and just SigHashAnyOneCanPay isn't valid it seems. It's gotta be combined with one of the all/none/single options: https://github.com/zcash/zcash/blob/a45c2a2d4e3d387bf2dcb2e5808db2f6f7008799/src/script/interpreter.cpp#L1237-L1257
Oh! zcash 5.0 was released a week ago. https://github.com/zcash/zcash/releases/tag/v5.0.0 I've been working with 4.6 |
How about https://zcashblockexplorer.com/ for the mainnet explorer? No external JS, displays fine with no JS, seems to be reference deployment of https://github.com/nighthawk-apps/zcash-explorer, has an onion address, NU5 tx and UA support I don't see a good testnet option though. Maybe https://sochain.com/testnet/zec |
#1597 made some conflicts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This server log seems off: Blocks are too empty to calculate %!d(string=zec) median fees. Using no-competition rate.
Also, I can't seem to get refunds to happen. Is client checking the locktime correctly?
It looks like will need a couple more wallets for simnet tests just like doge. Would you like to add or shall I?
// Tx can only produce tx hashes for unshielded transactions. Tx can only create | ||
// signature hashes for unshielded version 5 transactions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe first Tx can only produce tx hashes for unshielded transactions.
is redundant.
ConsensusBranchHeartwood = [4]byte{0x0B, 0x23, 0xB9, 0xF5} // 903000 | ||
ConsensusBranchCanopy = [4]byte{0xA6, 0x75, 0xFF, 0xE9} // 1046400, tesnet: 1028500 | ||
ConsensusBranchNU5 = [4]byte{0xB4, 0xD0, 0xD6, 0xC2} // 1687104, testnet: 1842420 | ||
ConsensusBranchNU5 = [4]byte{0xB4, 0xD0, 0xD6, 0xC2} // 1687104, testnet: 1842420 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exported bytes probably better as a constant so that they cannot be mutated.
dex/networks/zec/tx.go
Outdated
copy(b[:32], prevoutsDigest[:]) | ||
copy(b[32:64], seqDigest[:]) | ||
copy(b[64:], outputsDigest[:]) | ||
return blake2bHash(b, []byte("ZTxIdTranspaHash")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could make some of these strings that are reused constants to avoid typos. I don't see any typos currently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. Just one silly conflict with the new txscript.NewTxSigHashes
function signature. Also @JoeGruffins review
dex/testing/dcrdex/harness.sh
Outdated
"base": "ZEC_simnet", | ||
"quote": "BTC_simnet", | ||
"lotSize": 100000000, | ||
"rateStep": 100000, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This rate step is too high to reflect realistic rates. No a big deal on simnet, just not realistic.
Currently it's 0.00293146 BTC/ZEC, but I can only use 0.00x
@JoeGruffins is right. It can never refund. It seems the reason is that the
|
I suppose we'll just have to pull the last 11 block headers to compute mediantime. The only RPCs I can see that might give it to us is BTW, refunds do work if I hack the expiry check:
|
6fa91e1
to
8dbf2c8
Compare
Implement block deserialization, tx deserialization/serialization and input signing for ZCash and generalize those functions in the client and server btc packages. Implemenation notes 1. zcashd does not support encrypted wallets. No passwords allowed. 2. After starting the harness, it takes a few minutes for beta to get caught up. 3. zcashd can take a very long time to get it's fee estimates primed.
All txs we create and sign must be version 5. Use new SIGHASH algos from ZIP-244. Move SIGHASH stuff to methods of dexzec.Tx. Add live test to scan testnet blocks looking for deserialization errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Latest updates look good. Refunds work now.
[DBG] CORE: Refundable match 4092328257640b9120a1384debe25b6b185acec21b24f6e1711cd4a1d47649d6 for order 72f905d3dfc7a504b7fff74e79d80318295aa8f472107d19c9c65d9e52d25dd5 (Maker)
[INF] CORE: Refunding zec contract bd12551965edd38fbc1aa1c8cfd5fdec42f48bae92bff6b15f4494adf36db9d6:0 for match 4092328257640b9120a1384debe25b6b185acec21b24f6e1711cd4a1d47649d6 (no valid counterswap received from Taker)