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

feat: ignore unavailable swap clients on create (#1815) #2004

Merged
merged 1 commit into from
Dec 2, 2020

Conversation

rsercano
Copy link
Collaborator

attempts to fix #1815

But as a followup I don't know if this affects next startups when L2 is setup properly @sangaman

@rsercano rsercano requested review from a user, sangaman and raladev November 24, 2020 09:52
@rsercano rsercano self-assigned this Nov 24, 2020
@ghost
Copy link

ghost commented Nov 24, 2020

Can we initialize all swap clients after the initial create now?

@sangaman
Copy link
Collaborator

Can we initialize all swap clients after the initial create now?

Yes, thanks to #1973 we should automatically initialize wallets on future xud unlocks, so requiring all wallets/swap clients to be online upon initial create isn't that big a deal any more. So this change would make sense to me now.

sangaman
sangaman previously approved these changes Nov 24, 2020
Copy link
Collaborator

@sangaman sangaman left a comment

Choose a reason for hiding this comment

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

The change is very straightforward to do and I think the use case makes at least some sense since wallets/swap clients that aren't initialized up front will be initialized on future unlock calls. It does mean, however, that a user could create an xud node and not have all swap clients initialized upon first run, if they want them to be initialized properly later they'll need to restart xud. But maybe that's a desirable tradeoff. The thinking originally of having this requirement was to prevent users from initializing xud without initializing all the swap clients it is using (if a swap client is enabled and configured, presumably we expect the user to want it to be initialized), the related issue changes that behavior.

Note that we still require swap clients to be online for restore calls, which makes sense to keep that restriction to help ensure we restore everything properly.

It'd be good to get a concept ACK from @kilrau about the above, but the code change here is fine.

@rsercano rsercano requested a review from kilrau November 24, 2020 13:41
@kilrau
Copy link
Contributor

kilrau commented Nov 24, 2020

Concept ACK

kilrau
kilrau previously approved these changes Nov 24, 2020
Copy link
Contributor

@raladev raladev left a comment

Choose a reason for hiding this comment

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

Steps:

  1. wait for neutrino sync
  2. stop container with lndbtc
  3. try to create xud wallet
  • I am still getting unavaliable err
native@ubuntu:~/xud$ ./bin/xucli create
You are creating an xud node key and underlying wallets. All will be secured by a single password provided below.

Enter a password: 
Re-enter password: 

Error: 13 INTERNAL: could not initialize lnd-BTC: 14 UNAVAILABLE: failed to connect to all addresses
native@ubuntu:~/xud$ ./bin/xucli getbalance
xud is locked, run 'xucli unlock', 'xucli create', or 'xucli restore' then try again

native@ubuntu:~/xud$ ./bin/xucli unlock
Enter master xud password: 
no xud node exists to unlock, try creating one with 'xucli create' or 'xucli restore'
native@ubuntu:~/xud$ 
  • also it would be good if u fix assertion for auto-post-create flow
24/11/2020 09:32:03.099 [LND-BTC] error: AssertionError [ERR_ASSERTION]: awaitWalletInit should not be called from a status besides Initialized or Unlocked
    at LndClient.<anonymous> (/home/native/xud/dist/lndclient/LndClient.js:369:38)
    at Generator.next (<anonymous>)
    at /home/native/xud/dist/lndclient/LndClient.js:27:71
    at new Promise (<anonymous>)
    at __awaiter (/home/native/xud/dist/lndclient/LndClient.js:23:12)
    at LndClient.awaitWalletInit (/home/native/xud/dist/lndclient/LndClient.js:348:42)
    at LndClient.<anonymous> (/home/native/xud/dist/lndclient/LndClient.js:413:30)
    at Generator.next (<anonymous>)
    at /home/native/xud/dist/lndclient/LndClient.js:27:71
    at new Promise (<anonymous>)

@rsercano
Copy link
Collaborator Author

@raladev are you sure you have the correct changeset? I have the exact same condition and don't get an error:

image
image

@rsercano rsercano requested a review from raladev November 25, 2020 10:45
@raladev
Copy link
Contributor

raladev commented Nov 25, 2020

checked once more, same result.
Screenshot from 2020-11-25 21-55-33
Screenshot from 2020-11-25 21-55-21

Yeap, im sure that I am in same changeset.

native@ubuntu:~/xud$ git log -1
commit f7b955b89236861c33226e14bc105ae77c372f24 (HEAD -> fix/create-node-ignore-unavailable-clients, origin/fix/create-node-ignore-unavailable-clients)
Author: rsercano <[email protected]>
Date:   Tue Nov 24 12:51:05 2020 +0300

    fix: ignores unavailable swap clients during create node (#1815)

U have a little bit differrent condition, in your case both lnd are not avaliable, in my case only lndbtc is not avaliable.

My steps:

  1. share lnd port through simnet.conf - https://docs.exchangeunion.com/development/developer-guide
  2. bash xud.sh
  3. wait for neutrino sync
  4. close utils
  5. docker rm -f simnet_xud_1
  6. docker stop simnet_lndbtc_1
  7. git clone https://github.com/ExchangeUnion/xud.git
  8. mkdir .xud & cd ./xud
  9. nano xud.conf - set pathes to lndbtc and lndltc certs
loglevel = "trace"
noencrypt = false

[http]
host = "localhost"
port = 8887

[lnd.BTC]
cltvdelta = 40
disable = false
host = "localhost"
nomacaroons = false
port = 30009
certpath = "/home/native/.xud-docker/simnet/data/lndbtc/tls.cert"
macaroonpath = "/home/native/.xud-docker/simnet/data/lndbtc/data/chain/bitcoin/simnet/admin.macaroon"

[lnd.LTC]
cltvdelta = 576
disable = false
host = "localhost"
nomacaroons = false
port = 31009
certpath = "/home/native/.xud-docker/simnet/data/lndltc/tls.cert"
macaroonpath = "/home/native/.xud-docker/simnet/data/lndltc/data/chain/litecoin/simnet/admin.macaroon"

[raiden]
disable = true

[connext]
disable = false
host = "localhost"
port = 25040

[debug]
testing = false
  1. cd ~/xud
  2. git checkout fix/create-node-ignore-unavailable-clients
  3. npm install & npm run compile & npm run compile:seedutil https://docs.exchangeunion.com/development/native-installation
  4. sudo ./bin/xud
  5. sudo ./bin/xucli create

@rsercano
Copy link
Collaborator Author

rsercano commented Nov 25, 2020 via email

@rsercano rsercano dismissed stale reviews from kilrau and sangaman via 0d39a5d November 26, 2020 07:57
@rsercano rsercano force-pushed the fix/create-node-ignore-unavailable-clients branch from f7b955b to 0d39a5d Compare November 26, 2020 07:57
@rsercano
Copy link
Collaborator Author

I changed how initWallets in SwapClientManager behave while initializing by removing throw error phrase, therefore it won't throw while initializing a wallet per swap client and create/restore commands should continue as is, if one of the clients is not available for initializing.

@raladev @sangaman

@rsercano rsercano requested review from sangaman and raladev and removed request for raladev November 26, 2020 07:59
@kilrau
Copy link
Contributor

kilrau commented Nov 27, 2020

Could you please give this another look? @sangaman

@sangaman
Copy link
Collaborator

sangaman commented Dec 2, 2020

This error 24/11/2020 09:32:03.099 [LND-BTC] error: AssertionError [ERR_ASSERTION]: awaitWalletInit should not be called from a status besides Initialized or Unlocked is unrelated to this PR - I'll look into it separately. It's a bit of an edge case but I can reproduce it by setting up a new node with an enabled lnd in the config that is unreachable. It's setting the state of that lnd to WaitingUnlock when it should really be disconnected, so it tries to unlock it anyway and hits that exception.

Sercan's workaround to remove the throw line is fine for now, but I think it would make sense to reenable it once the above is addressed.

@sangaman sangaman changed the title fix: ignores unavailable swap clients during create node (#1815) feat: ignore unavailable swap clients on create (#1815) Dec 2, 2020
@sangaman sangaman merged commit 360cca7 into master Dec 2, 2020
@sangaman sangaman deleted the fix/create-node-ignore-unavailable-clients branch December 2, 2020 07:54
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.

xucli doesn't work if L2 is not set up properly
4 participants