search

decred / dcrdex

The Decred Decentralized Exchange (DEX), powered by atomic-swaps.
Other
186 stars 96 forks source link

zec: cannot connect to new rpc/external wallet if previously configured #2330

Closed chappjc closed 1 year ago

chappjc commented 1 year ago

Had a zcashd configured with dexc, but then restarted the simnet harness, and tried to have it connect again:

image

There seems to be no way to swap out the zcashd instance.

chappjc commented 1 year ago

...or perhaps not an issue because restarting seems to get it working. Reconfigure / settings change doesn't handle it though.

buck54321 commented 1 year ago

@chappjc. I think I may have located a couple of potential issues, but I'm failing to reproduce. Did the full error in your logs provide any more context?

chappjc commented 1 year ago

I'm afraid I've nuked the simnet datadir. But the key to reproduce is to swap out the configured zcashd with a new zcashd that does not have the accounts created yet.... while dexc is running, so that's why I realized this may not be an issue.

buck54321 commented 1 year ago

I'm trying my best, but not able to hit it. It seems like you're hitting it during reconfiguration. I've tried restarting the harness and reconfiguring to either the same node or a different node, and I'm not seeing the same error. Notably, because of a bug here, zec will always return restart = true from Reconfigure, which causes us to hit connectWalletResumeTrades here, which hits Connect and causes the accounts to be created in the ConnectFunc.

On the general problem of the user switching out the node with a running dexc, I can think of two scenarios worth discussing, One is when the user inserts a new node instance with identical configconnecturation but a new wallet. If they do this while dexc is running, this could cause problems. That said, there will always be ways to bork an RPC wallet. They should know better. The other scenario is that we're on simnet and we restart the harness. This is especially a Zcash problem because of the account creation requisite that is only run on connect. It also won't ever happen on mainnet or testnet. I don't know that we should ever expect to smoothly switch out an entire node under a running RPC wallet anyway. The fact that it doesn't crash harder is just lucky, and it may be worse for e.g. doge-delta, where the wallet seed is still random. But even with a deterministic wallet, I think we could run into problems with previously generated addresses that aren't yet known to the newly created node.

chappjc commented 1 year ago

Notably, because of a bug here, zec will always return restart = true from Reconfigure, which causes us to hit connectWalletResumeTrades here, which hits Connect and causes the accounts to be created in the ConnectFunc.

Found and fixing that bug in https://github.com/decred/dcrdex/pull/2326. And, umm... dang, maybe I think this can only be reproduced with that changed. Sorry.

I think we could run into problems with previously generated addresses that aren't yet known to the newly created node.

Yeah, definitely. If there were any active orders or trades, swapping out like this is a major issue. I don't think we should sweat this case (or this issue anymore, will close).