Does what it says on the tin. Has full validation of private key before trying to import. Includes logic to request passphrase if wallet is locked. Include progress bar when scanning transactions as can take a few mins.
Motivation was to make it easier for people to import vanity addresses. I wrote bitcoinvanity.appspot.com and so want to make importing key easier for users.
1st attempt at QT programming so could be improved I expect.
need to remove tick in menu and add space for comment with address
also need to link up with actual command so it does something!
Need more error checking and inform user of issues.
linked scanwalletfortransactions back to progess box
adding in code to unlock wallet if required
Conflicts:
src/qt/bitcoingui.cpp
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/1e84f9794d07e234344c71b177906b0b2dac543b for binaries and test log.
I don't think the importprivkey functionality should be exposed to normal users. It's really pretty dangerous. Instead, I would suggest implementing the sweepprivkey ("import" the key, flagged as "not part of my balance, but automatically move any funds seen to another address") functionality.
I'm not sure about this. It's a feature many people will like, and it certainly has legitimate uses. On the other hand, I don't like encouraging moving private keys around or vanity addresses...
I don't like encouraging moving private keys around either. They're called "private" for a reason.
I agree that "sweep" (and "sweep again if more coins are received on the swept address") is a much better way to go, and useful in more cases.
What makes importing private keys so dangerous? I suppose it's because it is a kind of trojan horse:
I suppose this could be worked around by marking the key as "foreign", not using it for change or showing the public key in the address book, and then either sweeping it or using it for spends only (while carefully monitoring other's transactions so that no double spends happen).
If people are importing private keys, then the private key is in two places. We'll have people emailing private keys too themselves, private keys on every other USB stick on the planet, and people posting angry bugs about bitcoin loosing their coins and you'd better compensate me!!!!!
Private key is private. Two people can keep a secret if one of them is dead, and all that.
On 29 November 2012 16:25, Wladimir J. van der Laan < notifications@github.com> wrote:
What makes importing private keys so dangerous? I suppose it's because it is a kind of trojan horse:
- Other people could be spending coins from it. This is not a scenario currently supported by the client, and result in non-consisent internal administration (wrong balances, double spends, etc).
- The client could potentially use the key as change address, after which the other person that have the key can steal a large part of your coins.
- The user could accidentally use the public key associated to the private key as public bitcoin address, ask others to send to it, after which someone else can steal the coins like in 2.
I suppose this could be worked around by marking the key as "foreign", not using it for change or showing the public key in the address book, and then either sweeping it or using it for spends only (while carefully monitoring other's transactions so that no double spends happen).
— Reply to this email directly or view it on GitHubhttps://github.com/bitcoin/bitcoin/pull/2050#issuecomment-10836678.
BTW your implementation looks very good, haven't looked in full detail, but on a high level you're doing everything as it's supposed it.
261 | @@ -260,6 +262,9 @@ void BitcoinGUI::createActions() 262 | encryptWalletAction = new QAction(QIcon(":/icons/lock_closed"), tr("&Encrypt Wallet..."), this); 263 | encryptWalletAction->setStatusTip(tr("Encrypt the private keys that belong to your wallet")); 264 | encryptWalletAction->setCheckable(true); 265 | + importPrivateKeyAction = new QAction(QIcon(":/icons/key"), tr("&Import Private Key..."), this); 266 | + importPrivateKeyAction->setStatusTip(tr("Import private key into wallet."));
Nit-pick: Can you please remove . at the end, to ensure all that status tip formats match.
853 | @@ -847,6 +854,8 @@ void BitcoinGUI::setEncryptionStatus(int status) 854 | encryptWalletAction->setChecked(false); 855 | changePassphraseAction->setEnabled(false); 856 | encryptWalletAction->setEnabled(true); 857 | +
Why 2 empty lines here and below again?
42 | + ui(new Ui::ImportPrivateKeyDialog) 43 | +{ 44 | + ui->setupUi(this); 45 | + QValidator *validator = new PrivateKeyValidator(this); 46 | + ui->privateKeyEdit1->setValidator(validator); 47 | + //QObject::connect(validator, SIGNAL(keyValidityChanged(bool)),
Is this a left-over or a to-do?
0 | @@ -0,0 +1,51 @@ 1 | +<?xml version="1.0" encoding="UTF-8"?>
Would it be possible to use the status bar in the main window for this? So no need to invent a whole new class.
In principe it would be better, I thought about it too, but the problem is that the entire UI is held up during import, so it would probably only be confusing right now.
Couldn't that import run in an own thread and just block usage of the status bar for other stuff, while doing that?
I'm sure it could, but that'd be a huge change to the core (and things like balance would be invalid for the duration of the rescan!), whereas this is a tractable UI feature addition. Even better would be to optimize adding keys so that a complete rescan is not needed (may be easier when the new wallet format arrives).
@n1bor I would like to only comment on some style stuff I saw. There are some places where you added unneeded line-breaks (2 in a row). I also think it's much nicer to use speaking names for GUI elements like labels and such (eg: transactionFeeLabel or okButton). Consider this feedback constructive and it's mostly nit-picking, but I'm known to have eagle eyes for such small things, sorry ;).
Diapolo - thanks for comments happy to tidy them all up! Just not going to do till someone posts on here that they are going to merge the pull. As currently looks like the the consensus is against including this in the client.
Automatic sanity-testing: FAILED MERGE, see http://jenkins.bluematt.me/pull-tester/1e84f9794d07e234344c71b177906b0b2dac543b for test log.
This pull does not merge cleanly onto current master
Please rebase.
Luke - happy to rebase, but only if you are going to pull into client.
Thought consensus was you did not want to encourage this sort of thing?
I'm personally much more in favor of exporting/importing entire wallets. It has much less risk for shooting yourself in the foot, and doesn't require micromanagement from the user. For that reason, it's something I also would object less against to put in the GUI. I intend to work on that soon.
Importing private keys really should include a sweep feature for safety. Creating a transaction immediately spending all coins associated with the imported key to a new address.
It sounds like consensus is mixed, though I'm a bit more friendly to privkey importing than other devs. But I don't want us to leave the poor pull requestor in limbo. We should ACK the general direction or close this.
I'm in favor of closing this. It may make sense to have a feature like this that is accessible through an expert mode (coin control comes very close to that), but if made generally available, it would lead to even more misunderstanding about wallets.
However, I wouldn't be opposed to having the functionality of #2592 accessible through the GUI. If a sweepprivkey gets implemented, that would seem safe as well.