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

added -walletallowsymboliclink (default false) #11343

Closed
wants to merge 1 commit into from

Conversation

isghe
Copy link
Contributor

@isghe isghe commented Sep 15, 2017

When is set to 1, permits to use symbolic link as a wallet.

When is set to 1, permits to use symbolic link as a wallet.
@luke-jr
Copy link
Member

luke-jr commented Sep 15, 2017

I don't know if this is a good idea. We don't allow wallets to be symlinks or outside the datadir for good reason (they're not self-sufficient files).

@esotericnonsense
Copy link
Contributor

#10885 contains some further information on this.

Preferred option there seems to be allowing a seperate walletdir (that would include bdb state).

@wtogami
Copy link
Contributor

wtogami commented Sep 15, 2017

FWIW I used my wallet.dat as a symlink to a separate encrypted partition for years now.

@gmaxwell
Copy link
Contributor

If the wallet.dat is on another filesystem from the database/ directory this will result in corruption in the event of unclean shutoffs.

@d3zd3z
Copy link

d3zd3z commented Sep 15, 2017

So, the argument is that it is better that my wallet be on an unencrypted filesystem than a chance something might get corrupt?

@d3zd3z
Copy link

d3zd3z commented Sep 15, 2017

I'm not sure I'm seeing how being on the same filesystem would prevent corruption. In my particular case, though, they are on the same filesystem, but the wallet goes through encfs to an underlying encrypted file on the filesystem.

@TheBlueMatt
Copy link
Contributor

If you want to do this you should instead just put your datadir on your encrypted partition and symlink chainstate/ and blocks/ onto your unencrypted partition. This will leave a few (smaller) files on your encrypted partition, but that should be fine. Worst case if anything blow up the thing you lose is your blocks/chainstate, which you can re-sync, instead of your wallet. This should maybe be noted in doc/files.md.

@isghe
Copy link
Contributor Author

isghe commented Sep 15, 2017

I think we should suppose that, who is using symbolic links, knows the risks on the way using them.

  1. In multi wallet configuration, the risk to load more than one time the same wallet;
  2. The risk that the target file is in another filesystem (as @gmaxwell told before).

Anyway introducing this limitation (rejecting symbolic links) introduce a big incompatibility with previous versions, didn't cited in release-noted.md.

For example, without symbolic links, I must change and reconsider the way I am handling my wallets, maybe using hard links, saving elsewhere, where the hard link is pointing to; that's sounds very ugly job.

Thanks for the attentions :-)

@jnewbery
Copy link
Contributor

I think we should suppose that, who is using symbolic links, knows the risks on the way using them.

This isn't a problem with symbolic links per se, it's that it's not safe to use them for the .dat file:

  • bitcoind can't distinguish whether multiple symlinks refer to the same file
  • it's not safe for the .dat file to be stored on a separate file system from the database directory

Both can lead to wallet file corruption, and therefore loss of funds. I wouldn't expect users to know this. We should prevent them from doing something that's this dangerous.

I think a better solution is to allow the user to specify an entirely separate directory for all the wallet files.

@luke-jr
Copy link
Member

luke-jr commented Sep 15, 2017

Symlinked wallets have never been supported. It was a bug that we didn't error immediately before.

Note that you won't benefit from the encrypted filesystem wrapper in your example either: all the data will be written to the database/ directory before wallet.dat...

@achow101
Copy link
Member

So, the argument is that it is better that my wallet be on an unencrypted filesystem than a chance something might get corrupt?

No.

The wallet database creates extra temporary files in the location of where Bitcoin Core thinks your wallet is (i.e. the symlink location). If your wallet is unencrypted (i.e. not using Bitcoin Core's wallet encryption), then those temporary files will be written to your unencrypted disk anyways and they may contain your unencrypted private keys. So keeping your wallet file on an encrypted disk is not helping as your private keys are unencrypted to the database software and may be written to an unencrypted disk while the wallet is in use.

I'm not sure I'm seeing how being on the same filesystem would prevent corruption. In my particular case, though, they are on the same filesystem, but the wallet goes through encfs to an underlying encrypted file on the filesystem.

Using symbolic linking may imply that you are attempting to use the same wallet file with different instances of Bitcoin Core (using an encrypted filesystem and symlinking is only one use case) which have different datadirs. If you were to shut down Bitcoin Core and the wallet database is not closed cleanly, there will be left over wallet database files in the datadir (i.e. not with the actual wallet file). If you then try to open it with another instance of Bitcoin Core with a different datadir, since the wallet.dat file was closed uncleanly and because the other datadir does not have the necessary wallet database files, you can have your wallet file corrupted.

@esotericnonsense
Copy link
Contributor

esotericnonsense commented Sep 15, 2017

Referencing gmaxwell's comment and jnewbery's above I also agree that allowing a seperate wallet directory that includes both wallet.dat and the BDB logs would be a far better solution. This folder could be specified or symlinked.

luke-jr's comment about the database directory being unencrypted also came up on IRC in a related discussion.

I believe that the relative paths are in wallet/db.cpp. In wallet/wallet.cpp GetDataDir() could be replaced with a GetWalletDir that defaults to GetDataDir but can be overridden.

I don't have a requirement for this particular feature but it doesn't look like too much of a pain to change.

@Polve
Copy link

Polve commented Sep 15, 2017

If you want to do this you should instead just put your datadir on your encrypted partition and symlink chainstate/ and blocks/ onto your unencrypted partition

How this would solve the corruption problems in the event of unclean shutoffs gmax is talking about?

@achow101
Copy link
Member

How this would solve the corruption problems in the event of unclean shutoffs gmax is talking about?

All of the relevant database files needed for the wallet.dat file would be present in the datadir on the encrypted disk instead of separated on different disks. They would actually be found in the same folder together as they are supposed to be.

@Polve
Copy link

Polve commented Sep 15, 2017

All of the relevant database files needed for the wallet.dat file would be present in the datadir on the encrypted disk instead of separated on different disks

wouldn't this be the same case where only the .dat file is symlinked to another folder/partition?
(that's my usual use case, like @wtogami, on an encrypted partition)

@achow101
Copy link
Member

achow101 commented Sep 15, 2017

wouldn't this be the same case where only the .dat file is symlinked to another folder/partition?
(that's my usual use case, like @wtogami, on an encrypted partition)

No it is not. When only the wallet file is symlinked, the wallet database files are created in the Bitcoin Core datadir which contains the symlink. They are not created in the folder containing the actual file itself. When you point the datadir and symlink the chainstate and blocks folder, the wallet database files will be in the same folder as the wallet file itself.

Here's some poor ascii art:

What happens when you symlink wallet.dat

Datadir on unencrypted disk | Folder on encrypted disk 
--------------------------- | -----------------------
 wallet database temp files |
 symlink to wallet.dat file | -------> actual wallet.dat file 
              blocks folder |
          chainstate folder |

What happens when you symlink other datadir stuff

Folder on unencrypted disk | Datadir on encrypted disk 
-------------------------- | -----------------------
                           | wallet database temp files 
                           | actual wallet.dat file
      actual blocks folder | <----- symlink to blocks folder 
  actual chainstate folder | <----- symlink chainstate folder 

@isghe
Copy link
Contributor Author

isghe commented Sep 15, 2017

My point of view is that who is using symbolic links, knows what it is doing:

I know that:

  1. I am not using multi wallet;
  2. If I will use multi wallet, I'll take care not passing the same files to bitcoin-core;
  3. the original file is not on another file system;
  4. I am not generating dynamically new addresses;
  5. I can lose the original wallet.dat (that's why I make regular backups);

Supposing that using symbolic links, will not affect the global Bitcoin, but eventually only my local system, why should bitcoin-core take care, about the good or wrong way, I am using symbolic links?
A warning message shouldn't be enough?

Thanks :-)

@jnewbery
Copy link
Contributor

The fact that it is safe for you to use in no way implies that it's safe in general to use. Contributors to this project consider the needs of all users, not just those who are as sophisticated and knowledgeable as you.

@isghe
Copy link
Contributor Author

isghe commented Sep 15, 2017

@jnewbery didn't you consider that the possible workarounds, a user can adopt, to go over this limitation (rejecting symbolic links), would be more dangerous, than giving to the user the choice to use it or not at his own risk?

@jnewbery
Copy link
Contributor

didn't you consider...

Yes, and my personal judgement is that taking previously undocumented behaviour which could result in loss of funds and turning it into a documented option is irresponsible.

@isghe
Copy link
Contributor Author

isghe commented Sep 15, 2017

@jnewbery interesting point of view, I'll think on it. Thanks :-)

@achow101
Copy link
Member

My point of view is that who is using symbolic links, knows what it is doing:

But do you really? Do you know the other BDB temporary files created are not in the same directory as the actual wallet file when you use symbolic links? Those files are actually in your datadir with the symlink, not the actual wallet file. Do you know that by using symbolic links you are at more risk of corrupting your wallet and losing your money? Do you know that keeping an unencrypted wallet on an encrypted drive and then symlinking it is completely pointless since your unencrypted keys will probably be written to the unencrypted drive anyways due to the BDB temp files?

There are a lot of things that most people are not going to know or consider, even if they know what they are doing with symbolic links. Just because you are technical enough to use symlinks does not mean that you fully understand how Bitcoin Core will work with this symlinks and any consequences of that.

@isghe
Copy link
Contributor Author

isghe commented Sep 15, 2017

@achow101 I think I wrote very well I am not symlinking to another file system.

@achow101
Copy link
Member

I brought up the file systems thing for others who were talking about encrypted file systems. The other points still stand as they don't depend on other file systems.

@laanwj
Copy link
Member

laanwj commented Sep 16, 2017

NACK. My preferred approach would be: Wallets should be in their own directory #11348

@isghe
Copy link
Contributor Author

isghe commented Sep 20, 2017

I don't understand why the limitation no wallet.dat as symbolic link introduced in v.0.15, is not declared in https://github.com/bitcoin/bitcoin/blob/v0.15.0/doc/release-notes.md
Thanks

@sipa
Copy link
Member

sipa commented Sep 21, 2017

@isghe It's not in the release notes because it was not supported functionality, and I don't think anyone was aware of users relying on this.

In any case, NACK. This is a footgun. I prefer #11348.

@maflcko
Copy link
Member

maflcko commented Sep 22, 2017

This pull points to the wrong branch (we only merge on the master branch). Also, there is disagreement whether this is the right direction.

@sylvandb
Copy link

Symbolic links have nothing to do with specific applications and have been system supported functionality for decades. Bitcoin core intentionally preventing that functionality is EGOCENTRIC, RUDE and as it wasn't documented in the release notes, INCONSIDERATE.

Every reason cited above in the threads on this topic justifies nothing more than a warning which the user should be free to bypass at their own risk.

You have totally destroyed years of my wallet management without even a mention in the release notes and left me wondering what other bombs you have left me to discover.

Thanks so very much.

@maflcko
Copy link
Member

maflcko commented Nov 15, 2017

If you are happy to accept loss of funds, you are very welcome to remove the fs::is_symlink(wallet_path) check in init.cpp. After all, this is open source. 🎉

@ryanofsky
Copy link
Contributor

@sylvandb, we have some changes queued up which should allow more flexibility. I have #11687 and there is also #11466. Right this second I am working on an improvement to #11687 that will actually reallow symlinks and address lukejr's concerns in that PR.

@ryanofsky
Copy link
Contributor

By the way, I was also surprised that there didn't seem to be a mention of symlink change in the release notes. That does seem like an unfortunate oversight. I wish more prs would update documentation and code together.

@sylvandb
Copy link

@MarcoFalke already done. How do you propose to address my other concern about potential for "other bombs" left in the code or the loss of trust caused by hidden, incompatible changes?

@ryanofsky that's good news.

All, usually I try to leave a constructive idea with a gripe. Sorry for the first post where I forgot to do that, so here it is:

The correct approach to resolve symlink problems and protect users does require to detect them and handle appropriately. In this case I suspect that would be to de-reference to the real path and use that instead of the path containing the symlink. However because core writes a bunch of stuff with canned filenames to accompany the wallet file, those names also need to change because the symlink may be selecting one file from a directory containing multiple wallet files.

@maflcko
Copy link
Member

maflcko commented Nov 15, 2017

@sylvandb I (and others) are paying close attention to every pull and commit to see if it might break some use case or interface. We will then mention that change not only in the list of changes in the release notes, but also devote a section or short sentence to it. However, there is only a limited amount of people that care about the release notes and oversights happen from time to time. This is an example of such oversight, sadly.

@maflcko
Copy link
Member

maflcko commented Nov 15, 2017

Also, when it comes to the wallet it is hard to reason about the different ways people use the wallet. This makes it tedious to review new wallet features, which often ends in rejection of wallet features and limiting the usability. Rather err on the save side, than letting inexperienced users lose funds.

@isghe
Copy link
Contributor Author

isghe commented Nov 16, 2017

I merged this patch -walletallowsymboliclink to version v0.15.1 in branch https://github.com/isghe/bitcoin/tree/isghe_0.15.1.

For my purposes it just works. Anyway take care the risks using symbolic link, not only with bitcoin, but also with a text editor…

Thanks, Isidoro

@luke-jr
Copy link
Member

luke-jr commented Nov 16, 2017

There's no comparison. Text files are single standalone files.

@isghe
Copy link
Contributor Author

isghe commented Nov 17, 2017

I don't know any other software inhibiting symbolic links.
Why is it necessary in bitcoin, and why it is not described in release note?

@sipa
Copy link
Member

sipa commented Nov 17, 2017

Because Bitcoin Core (BDB) creates temporary database journal files in the same directory as the wallet files. If those database files end up on a different filesystem, and a crash happens, there is no more guarantee the file is in sync with the journals. When people move files around (which often happens) between directories, it's even harder to reason about consistency.

You're free to use any patch you like, but the conditions under which it's safe to do so are too complicated to explain in release notes. We should just explain it is not supported. And it not being mentioned in release notes was indeed an oversight - we should fix that.

@isghe
Copy link
Contributor Author

isghe commented Nov 17, 2017

so, please add in release note, something similar to: "for security reasons we don't allow anymore symbolic links to wallet.dat files"

@luke-jr
Copy link
Member

luke-jr commented Nov 17, 2017

It was never supported, and never safe.

@sipa
Copy link
Member

sipa commented Nov 17, 2017

@luke-jr I agree with that, but still it makes sense to communicate that this situation is now detected.

@sylvandb
Copy link

@luke-jr of course it was supported. Not explicitly, but implicitly, likely the same way core supports running on an SSD versus a mechanical hard disk or possibly even an AMD vs. Intel processor.

There are a lot of relative safety arguments for SSD vs. mechanical, and there are unique failure cases for a symlink vs. real file in the directory.

In my case, a symlink is safer because it allows me to keep my wallet files together where they all have daily encrypted backup with a history going back several years. Changing a symlink to change wallets is a lot less error prone and risky than copying a wallet file into the active location and copying it back out some time later.

So now my choice is to run my own patched version of core, or take increased risk with my wallet files.

@Polve
Copy link

Polve commented Nov 18, 2017

My (forced) choice was to not upgrade.

@meshcollider
Copy link
Contributor

@Polve as mentioned above, hopefully the next major release (0.16.0) should support having wallet files outside the data directory (see #11466 which is basically ready to merge) in a way which is much safer than symlinking, apologies for the inconvenience caused until then :)

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

Successfully merging this pull request may close these issues.

None yet