Skip to content
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

client/asset/bch: native Bitcoin Cash SPV wallet #1635

Merged
merged 6 commits into from
Sep 16, 2022

Conversation

buck54321
Copy link
Member

@buck54321 buck54321 commented May 29, 2022

The gcash repo has been replaced by a branch on my fork until my PRs get merged on gcash. Here is the diff.

There aren't any new unit tests, because the stuff that might be unit tested is really just type translations.

Testing on simnet requires bchd and bchctl from gcash to be in PATH. The harness will start a bchd node if both are found.

client/asset/bch:
Add bchSpvWallet type, which implements btc.BTCWallet around
gcash/btcwallet. Most methods simply perform type translaction
from the gcash types to the btcsuite types. Wallet creation is
handled in-package. Wallet initalization is handled via a constructor
passed to the (now-exported) btc.OpenSPVWallet.

client/asset/btc:
ConfigOpts that are common to clone assets can be generated with
the new CommonConfigOpts, SPVConfigOpts, and RPCConfigOpts functions.
OpenSPVWallet has been exported and modified to accept a constructor
for a BTCWallet. Some spvWallet methods are now exported, including
startWallet -> Start and signTransaction -> SignTx.

Some re-organization to move methods that rely on *wallet.Wallet 
directly (rather than BTCWallet) to walletExtender. This includes 
(*spvWallet).startWallet → (walletExtender).Start as well as the wallet 
rescan functionality. 

A couple of concrete types needed to be swapped for interfaces. The 
*neutrino.ServerPeer is now a SPVPeer, so a wrapper was written 
for neutrino.ChainService to translate the type in the Peer method. 
Wallet block notifications are now generalized.

dex:
New ErrorCloser type handles a common pattern where itermediary
instances of whatever need to be shut down if some sequence of actions
doesn't complete successfully. Used in bch and btc.

dex/testing/btc:
Harness updated to allow running a bchd/bchwallet pair. These are
needed for the compact filters.

@chappjc
Copy link
Member

chappjc commented May 30, 2022

For gcash/neutrino, I think lightninglabs/neutrino#247 would be good too. It properly resolves the "always timeout" issue when publishing a transaction, where my previous commit to resolve that missed the mark.

@chappjc chappjc added this to the TBD milestone Jun 9, 2022
@buck54321
Copy link
Member Author

For gcash/neutrino, I think lightninglabs/neutrino#247 would be good too. It properly resolves the "always timeout" issue when publishing a transaction, where my previous commit to resolve that missed the mark.

Done

@buck54321 buck54321 marked this pull request as ready for review August 9, 2022 18:56
// only used for wallets with WalletDefinition.Seeded = true.
func (d *Driver) Exists(walletType, dataDir string, settings map[string]string, net dex.Network) (bool, error) {
if walletType != walletTypeSPV {
return false, fmt.Errorf("no Bitcoin wallet of type %q available", walletType)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe intended, but says "Bitcoin".

// AddPeers: addPeers,
ConnectPeers: connectPeers,
// // WARNING: PublishTransaction currently uses the entire duration
// // because if an external bug, but even if the resolved, a typical
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

even if the bug is resolved

// Depending on the network, we add some addpeers or a connect peer. On
// regtest, if the peers haven't been explicitly set, add the simnet harness
// alpha node as an additional peer so we don't have to type it in. On
// mainet and testnet3, add a known reliable persistent peer to be used in
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like testnet4 is used.

Comment on lines +200 to +215
case bchwire.TestNet, bchwire.SimNet: // plain "wire.TestNet" is regnet!
connectPeers = []string{"localhost:21577"}
}
Copy link
Member

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 get past "no peers" on testnet. I have a local node running and tried changing this to the port I set for it but still no peers. Any idea what I can try?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment here I think is to indicate that there is some ambiguity between testnet 1 and simnet. As you indicated, we're using bchwire.TestNet4. I've now added a addPeers = []string{"localhost:28333"} for that case, and was able to sync with bchd --testnet4 running locally. For Bitcoin, we added a testnet btcd node at dex-test.ssgen.io. Maybe we'll want to do the same for bch eventually.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be needed if cfilter peers are scarce. Hopefully not though

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh no, I was using -testnet instead of -testnet4. So... I should use testnet 4 right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh crap. We can't use testnet 4, can we?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty sure we sorted it out on Matrix, but we're on testnet4 so that's right. @JoeGruffins I guess the confusion for you was that you need bchd installed locally to have a node serving cfilters since bitcoin-cash-node isn't able.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still not finding peers after running bchd. This definitely works for others on testnet?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still not finding peers after running bchd. This definitely works for others on testnet?

Only when running local bchd. Like:

bchd --testnet4 --whitelist 127.0.0.0/8 --whitelist ::1 --banduration=20s

Testing on simnet requires bchd and bchctl from gcash to be in
PATH.

client/asset/bch:
Add bchSpvWallet type, which implements btc.BTCWallet around
gcash/btcwallet. Most methods simply perform type translaction
from the gcash types to the btcsuite types. Wallet creation is
handled in-package. Wallet initalization is handled via a constructor
passed to the (now-exported) btc.OpenSPVWallet.

client/asset/btc:
ConfigOpts that are common to clone assets can be generated with
the new CommonConfigOpts, SPVConfigOpts, and RPCConfigOpts functions.
OpenSPVWallet has been exported and modified to accept a constructor
for a BTCWallet. Some spvWallet methods are now exported, including
startWallet -> Start and signTransaction -> SignTx.

dex:
New ErrorCloser type handles a common pattern where itermediary
instances of whatever need to be shut down if some sequence of actions
doesn't complete successfully. Used in bch and btc.

dex/testing/btc:
Harness updated to allow running a bchd/bchwallet pair. These are
needed for the compact filters.
@chappjc
Copy link
Member

chappjc commented Sep 9, 2022

I think bchd 0.19 is forked off the network, at least on testnet4. Looks like an accidental hard fork to me

2022-09-09 17:52:56.110 [INF] SYNC: Processed 4500 blocks in 10.27s (4501 transactions, height 92501/112374 (82.32%), 2022-04-24 18:42:07 -0500 CDT, ~10 MiB cache)
2022-09-09 17:53:06.116 [INF] SYNC: Processed 3500 blocks in 10s (3507 transactions, height 96001/112374 (85.43%), 2022-05-19 01:03:46 -0500 CDT, ~10 MiB cache)
2022-09-09 17:53:16.774 [INF] SYNC: Processed 4000 blocks in 10.65s (4052 transactions, height 100001/112374 (88.99%), 2022-06-15 12:11:09 -0500 CDT, ~11 MiB cache)
2022-09-09 17:53:26.942 [INF] SYNC: Processed 4500 blocks in 10.16s (4706 transactions, height 104501/112374 (92.99%), 2022-07-17 00:20:00 -0500 CDT, ~11 MiB cache)
2022-09-09 17:53:37.309 [INF] SYNC: Processed 4000 blocks in 10.36s (4193 transactions, height 108501/112374 (96.55%), 2022-08-13 20:30:59 -0500 CDT, ~11 MiB cache)
2022-09-09 17:53:46.616 [INF] SYNC: Rejected block 000000001ad72b7fcc01b5c1beca6df36cc276d5b7bbdf8362b9d3c5d5306e04 from 199.192.18.235:28333 (outbound): serialized transaction is too small - got 99, min 100
2022-09-09 17:53:46.616 [INF] CHAN: Adding orphan block 000000000a8c216129f1abd8c3842e6e6bef6f3a5606f5e2e66ebba8d5c5ae45 with parent 000000001ad72b7fcc01b5c1beca6df36cc276d5b7bbdf8362b9d3c5d5306e04
2022-09-09 17:53:46.616 [INF] CHAN: Adding orphan block 000000000afc2e712b1711eae5a0be6b13ee3532b5e01088efbf8f1e32474dfa with parent 000000000a8c216129f1abd8c3842e6e6bef6f3a5606f5e2e66ebba8d5c5ae45
2022-09-09 17:53:46.616 [INF] CHAN: Adding orphan block 000000001ef73751e62359323a635135450c661668cad2e09a053f3ca923f5a8 with parent 000000000afc2e712b1711eae5a0be6b13ee3532b5e01088efbf8f1e32474dfa
2022-09-09 17:53:46.616 [INF] CHAN: Adding orphan block 0000000004cd010c1fba62e8e96964f43e9cc098047ab7eec3036b0f299a20e8 with parent 000000001ef73751e62359323a635135450c661668cad2e09a053f3ca923f5a8
2022-09-09 17:53:46.617 [INF] CHAN: Adding orphan block 000000000f907029b3498775eb4132e6f3f58989066f1b40f71b5bc6e50d7169 with parent 0000000004cd010c1fba62e8e96964f43e9cc098047ab7eec3036b0f299a20e8
2022-09-09 17:53:46.617 [INF] CHAN: Adding orphan block 0000000020f9c894ba81f93d2d2071aa887c26ed9f49fc380d209ddfc200f04f with parent 000000000f907029b3498775eb4132e6f3f58989066f1b40f71b5bc6e50d7169
2022-09-09 17:53:46.617 [INF] CHAN: Adding orphan block 0000000016ffdb0d2a99398f3dfdb144310383b28da2e52d52c175eeb3eeb44a with parent 0000000020f9c894ba81f93d2d2071aa887c26ed9f49fc380d209ddfc200f04f
2022-09-09 17:53:46.617 [INF] CHAN: Adding orphan block 0000000009d3dd4f9a19b10de4a82132e5d47b1b6d214cf688dcf3b855d9fe4f with parent 0000000016ffdb0d2a99398f3dfdb144310383b28da2e52d52c175eeb3eeb44a
2022-09-09 17:53:46.617 [INF] CHAN: Adding orphan block 00000000072a425550a93ea7950f308f45a104f9d0b3c5dfd5533fae333be1da with parent 0000000009d3dd4f9a19b10de4a82132e5d47b1b6d214cf688dcf3b855d9fe4f
<thousands>

I'll try mainnet, but the above is my a local bchd testnet4 node I just started fresh.

@chappjc
Copy link
Member

chappjc commented Sep 16, 2022

I think bchd 0.19 is forked off the network, at least on testnet4.

On this issue, it does not seem to prevent bchd from syncing with the fork at https://tbch4.loping.net/, so I guess it's not fatal. The invalid fork seems to be tracked by https://testnet4.imaginary.cash/

Copy link
Member

@chappjc chappjc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We really need this, and all of your code looks pristine, so we should move forward with it, but bchd and/or the bch-neutrino are really flaky in testing. I got into a situation with an active trade that took multiple restarts of both dexc and my local bchd, and multiple wallet Recover and Rescans to get it back in sync.

At some point dexc/neutrino or the one bchd peer got stuck:

2022-09-16 22:07:35.661 [WRN] CORE[bch][SPV][NTRNO]: Unable to fetch set of candidate checkpoints, trying again...
2022-09-16 22:07:48.662 [WRN] CORE[bch][SPV][NTRNO]: Unable to fetch set of candidate checkpoints, trying again...
2022-09-16 22:07:57.415 [DBG] CORE[dcr]: tip change: 993745 (00000000bf0208d4dec5e9265273c7034f46b6f0a2ddac1a4319d05a7744d571) => 993746 (000000006e2f32a0b01b1e0a93c213d225d608f02a3f96a3dfb3d3c53e3d04d2)
2022-09-16 22:07:57.415 [DBG] CORE[bch][SPV]: Performing cfilters scan for 2bb6fc26be13f4a6c5fe1600993eeef31c09d1594b8265169840760f50edb452:0 from height 113250
2022-09-16 22:07:57.416 [DBG] CORE[bch][SPV]: Performing cfilters scan for 2bb6fc26be13f4a6c5fe1600993eeef31c09d1594b8265169840760f50edb452:0 from height 113250
2022-09-16 22:08:01.663 [WRN] CORE[bch][SPV][NTRNO]: Unable to fetch set of candidate checkpoints, trying again...
2022-09-16 22:08:14.664 [WRN] CORE[bch][SPV][NTRNO]: Unable to fetch set of candidate checkpoints, trying again...
2022-09-16 22:08:27.666 [WRN] CORE[bch][SPV][NTRNO]: Unable to fetch set of candidate checkpoints, trying again...
2022-09-16 22:08:30.953 [DBG] CORE: Unsubscribing from the dcr_bch order book for dex-test.ssgen.io:7232
2022-09-16 22:08:40.668 [WRN] CORE[bch][SPV][NTRNO]: Unable to fetch set of candidate checkpoints, trying again...
2022-09-16 22:08:53.670 [WRN] CORE[bch][SPV][NTRNO]: Unable to fetch set of candidate checkpoints, trying again...
2022-09-16 22:09:06.672 [WRN] CORE[bch][SPV][NTRNO]: Unable to fetch set of candidate checkpoints, trying again...
2022-09-16 22:09:19.673 [WRN] CORE[bch][SPV][NTRNO]: Unable to fetch set of candidate checkpoints, trying again...
^C2022-09-16 22:09:24.462 [INF] DEXC: Attempting to logout...
You have active orders. Shutting down now may result in failed swaps and account penalization.
Do you want to quit anyway? ('yes' to quit, or enter to abort shutdown): y
2022-09-16 22:09:25.487 [INF] DEXC: Shutting down...
2022-09-16 22:09:25.487 [INF] CORE[DB]: Backing up database...
2022-09-16 22:09:25.487 [DBG] CORE: notify: |POKE| (conn) Server disconnect - dex-test.ssgen.io:7232 is disconnected
2022-09-16 22:09:25.487 [DBG] CORE[wss://dex-test.ssgen.io:7232/ws]: Sending close 1000 (normal) message.
2022-09-16 22:09:25.487 [INF] WEB: Web server off
2022-09-16 22:09:25.488 [WRN] CORE: Shutting down with active order d815f3cbcb71bd9e71382d47eba857d72ede2495b5dea3516b77ae0855373d5f in status executed.
2022-09-16 22:09:25.488 [WRN] CORE: Shutting down with active order 958cd74322f9baabb0970057d95a012e9a8995ae9e1cbd981cfd0b7171080686 in status executed.
2022-09-16 22:09:25.488 [INF] CORE: Locking DCR wallet
2022-09-16 22:09:25.488 [INF] CORE[dcr][SPV]: Unloading wallet
2022-09-16 22:09:25.489 [INF] CORE[dcr][SPV]: SPV wallet closed
2022-09-16 22:09:25.489 [INF] CORE: Locking BCH wallet
2022-09-16 22:09:25.489 [INF] CORE[bch][SPV]: Unloading wallet
2022-09-16 22:09:32.675 [WRN] CORE[bch][SPV][NTRNO]: Unable to fetch set of candidate checkpoints, trying again...
2022-09-16 22:09:32.676 [INF] CORE[bch][SPV]: SPV wallet closed
2022-09-16 22:09:32.676 [INF] CORE: DEX client core off
2022-09-16 22:09:32.676 [INF] DEXC: Exiting dexc main.

On restart of dexc, it continued with those Unable to fetch set of candidate checkpoints, trying again...

So then I tried another restart, but same. Then I tried a Rescan, which didn't seem to work right, and then a Recover.

Restarted my bchd twice, then restarted dexc, before it was able to start serving to neutrino again. There was nothing in bchd logs to indicate an issue however. I did notice that sometimes on bchd startup it would be lacking messages like [INF] PEER: Growing size of checkpoint cache from 52 to 54 block hashes, and if it did have them it seemed to be alright.

@chappjc chappjc merged commit eea605d into decred:master Sep 16, 2022
@chappjc
Copy link
Member

chappjc commented Sep 16, 2022

Oh all that weirdness in my last comment started when I noticed an order funded by a split tx timed out during placement, because it didn't propagate because either neutrino or bchd choked at some point prior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants