Skip to content

Fix Password for lnd instance.#7

Merged
rockstardev merged 1 commit intobtcpayserver:masterfrom
ziggie1984:fix-lnd-wallet-pw
Oct 15, 2024
Merged

Fix Password for lnd instance.#7
rockstardev merged 1 commit intobtcpayserver:masterfrom
ziggie1984:fix-lnd-wallet-pw

Conversation

@ziggie1984
Copy link
Copy Markdown

@ziggie1984 ziggie1984 commented Oct 2, 2024

Fixes the password creation for the LND daemon.

Tho I think this would not be backwards compatible.

@warioishere
Copy link
Copy Markdown

the question is, how is this going to be implemented. Adding the corrected code, will break starting all older btcpay setups from starting up as the wrong password hellorockstar\n is already set

@ziggie1984
Copy link
Copy Markdown
Author

Ok changed the script, we will now try it without the line feed, it it fails we try the old way with the line feed.

@ziggie1984
Copy link
Copy Markdown
Author

ziggie1984 commented Oct 2, 2024

People trying to recover the rootkey via chantools, you need to use this version if your btcpay pw was created with the line feed character at the end.

https://github.com/ziggie1984/chantools/tree/btcpay-pw-fix

@warioishere
Copy link
Copy Markdown

thanks man, remem

People trying to recover the rootkey via chantools, you need to use this version if your btcpay pw was created with the line feed character at the end.

https://github.com/ziggie1984/chantools/tree/btcpay-pw-fix

I can confirm that this works with the newline broke passwort

@coveralls
Copy link
Copy Markdown

coveralls commented Oct 2, 2024

Pull Request Test Coverage Report for Build 11314695340

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 83 unchanged lines in 18 files lost coverage.
  • Overall coverage increased (+0.6%) to 49.826%

Files with Coverage Reduction New Missed Lines %
htlcswitch/linkfailure.go 2 51.72%
channeldb/forwarding_package.go 2 71.79%
lnrpc/signrpc/signer_grpc.pb.go 2 59.73%
log.go 2 93.2%
brontide/noise.go 2 86.57%
rpcserver.go 2 71.12%
channeldb/channel.go 2 71.66%
brontide/conn.go 3 53.28%
contractcourt/chain_arbitrator.go 3 79.17%
contractcourt/breach_resolver.go 3 88.33%
Totals Coverage Status
Change from base Build 9827632250: 0.6%
Covered Lines: 93641
Relevant Lines: 187935

💛 - Coveralls

Copy link
Copy Markdown
Member

@rockstardev rockstardev left a comment

Choose a reason for hiding this comment

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

@ziggie1984 thank you for doing this - I will be bumping LND to 1.18.3 in next 2 days, want to include this fix so we deploy it before BTCPay 2.0 is released so everyone can update to it.

If this is done properly we can also resolve this issue: btcpayserver/btcpayserver#5612

Let me know what you think how it's best to approach this and we'll sync for release.

@warioishere
Copy link
Copy Markdown

@rockstardev you said you gonna release lnd 0.18.3 soon, as far as I know, lnd 0.18.4 will be released soon which resolves incompatibilty with bitcoincore 28, dont know if you heard about that, but if you guys upgrade to bitcoincore 28 afterwards, no lnd node will start up anymore, and you need to release lnd 0.18.4 afterwards to fix this. You might save yourself some time and hazzle
@ziggie1984 is there a rawly estimate arrival time of lnd 0.18.4?

greetings

@ziggie1984 ziggie1984 force-pushed the fix-lnd-wallet-pw branch 2 times, most recently from 2cc6d02 to cc16568 Compare October 13, 2024 12:58
@ziggie1984
Copy link
Copy Markdown
Author

Ok changed it, tested locally and it worked.

@warioishere
Copy link
Copy Markdown

Ok changed it, tested locally and it worked.

will test this on my instance also this evening or maybe if not latest tomorrow evening

Copy link
Copy Markdown
Member

@rockstardev rockstardev left a comment

Choose a reason for hiding this comment

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

LGTM, you crushed it with this PR @ziggie1984

@rockstardev rockstardev merged commit 8742a9a into btcpayserver:master Oct 15, 2024
@rockstardev
Copy link
Copy Markdown
Member

rockstardev commented Oct 15, 2024

I've published btcpayserver/lnd:v0.18.3-beta container, so if you want to test @warioishere, you can use it... I'll open PR on our docker repository shortly.

https://hub.docker.com/layers/btcpayserver/lnd/v0.18.3-beta/images/sha256-e7449187b058440e4746f948bd095584d67bb82133c93737204e86c3aeff664a?context=repo

@rockstardev
Copy link
Copy Markdown
Member

Worked perfectly on my server. However, I've noticed that admin_macaroon ends up in logs:

image

so I've removed the echo $change_password_response line.

https://github.com/btcpayserver/lnd/pull/7/files#diff-98d30299b31fde380e773b8a332bf4cd3eb58421dfcda488c376551470b6cc26R82

Publishing new container for 0.18.3-beta on Docker Hub that should work for all, deleted the old one.

@rockstardev
Copy link
Copy Markdown
Member

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.

4 participants