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

Allow users to have multiple wallets in bitcoind #1334

Merged
merged 3 commits into from
Mar 9, 2020

Conversation

araspitzu
Copy link
Contributor

@araspitzu araspitzu commented Feb 27, 2020

In this PR we force the usage of the default bitcoind wallet (named ""). This is necessary to allow the user to have multiple bitcoind wallets because when there are more than one we need to specify which wallet to use in the RPC calls. Fixes #1330.

Note for reviewers: the extra path in the bitcoin RPC URL identifies the wallet to be used and can be used in all the others RPC too. By default bitcoind has an unnamed wallet "" and eclair always expect to have that, users can add new wallets but should never rename or remove the default wallet (maybe we can add a few lines in the README for this).

…allows the users to have multiple bitcoin wallets in bitcoind.
@araspitzu araspitzu marked this pull request as ready for review February 27, 2020 16:09
@sstone sstone self-requested a review February 28, 2020 14:30
@sstone
Copy link
Member

sstone commented Mar 2, 2020

I was able to reproduce #1330 on testnet and check that this PR fixes it, but as mentioned it's worth adding a few lines to our documentation.

@t-bast
Copy link
Member

t-bast commented Mar 3, 2020

Open question: if we do that, why don't we allow the user to specify which wallet to use?
They could create a wallet for lightning, provide its name in eclair.conf and we'd use that.
And by default if they don't provide anything, we'd use the unnamed one.

@araspitzu
Copy link
Contributor Author

Open question: if we do that, why don't we allow the user to specify which wallet to use?
They could create a wallet for lightning, provide its name in eclair.conf and we'd use that.
And by default if they don't provide anything, we'd use the unnamed one.

Why not :) as long as we can provide additional features without damaging the 0-conf setup i think we can add it.

@araspitzu
Copy link
Contributor Author

Note for reviewers: at commit 44e009e this PR reads the bitcoin core wallet name from the configuration. Under some circumstances this introduces a new way the users can hurt themselves: between the fundTransaction and rollback operation we lock and unlock outpoints, if a user inadvertently restarts eclair before the rollback with a different walletname than it won't work and the outputs won't be unlocked. I think it's an acceptable risk (perhaps we can add a mention in the readme) because in general the users should not change bitcoind/wallet while there are pending operations in eclair.

Copy link
Member

@t-bast t-bast left a comment

Choose a reason for hiding this comment

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

LGTM, but I haven't tested E2E (I'll let sstone re-do the test he did on testnet).

Under some circumstances this introduces a new way the users can hurt themselves

I think this is fine. Users that do that can't complain if bad things happen afterwards IMO, they have to know what they're doing.

@araspitzu araspitzu force-pushed the use_default_bitcoind_wallet branch from 44e009e to 411fb84 Compare March 6, 2020 10:51
@araspitzu
Copy link
Contributor Author

On a second thought there are some other cases where the users can hurt themselves and even lose funds (think of swapping wallet and shutdown scripts usage), as a precaution i reverted the changes to implement only the original scope of this PR.

Copy link
Member

@sstone sstone left a comment

Choose a reason for hiding this comment

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

LGTM, just a wording change in our README.

Co-Authored-By: Fabrice Drouin <sstone@users.noreply.github.com>
@araspitzu araspitzu merged commit 8b64e23 into master Mar 9, 2020
@araspitzu araspitzu deleted the use_default_bitcoind_wallet branch March 9, 2020 09:32
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.

fatal error: bitcoind must have wallet support enabled when Bitcoin Core have multiple wallets
3 participants