Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Watch-Only Addresses: Separating Transaction Signing From Transaction Processing #2121

Closed
wants to merge 1 commit into from
@CodeShark

A major issue many have faced in using bitcoind to build applications is the lack of RPC support for tracking transactions and balances without having to know and keep associated private keys in the wallet. Oftentimes one might want to watch other people's transactions or to keep signing nodes behind tighter security while using separate relay nodes to service all nonsigning, non-key-generating application functionality such as sending payment and confirmation alerts.

In order to achieve these objectives, I have had to build a bunch of custom software - much of which duplicates functionality that is already present in the Satoshi Client. This pull request is an attempt at addressing some of these concerns without having to fundamentally restructure the client architecture. (Thanks, sipa!)

The proposal is to add another kind of object to the wallet database - a bitcoin address sans private key - which the client treats as if it were any other wallet adddress except for when it comes to signing and privkey export operations. This means RPC calls such as getreceivedbyaddress and listtransactions can be used on arbitrary bitcoin addresses.

I've added an RPC call:

importaddress <bitcoinaddress> [label] [rescan=true]

The address is added as a new type of serialized object in wallet.dat and loads into the key maps of the CKeyStore instances with the key set to the CKeyID and the secret set to an empty vector.

Please test it out and let me know what you think.

Cheers,

-Eric Lombrozo

@CodeShark

Sorry, guys - I changed the branch names and it closed the original pull request #2117

The pull request remains open but under a new name.

@CodeShark

A multiple wallet version of bitcoind is underway: #2124

That branch will eventually be merged with this one. In order to distinguish spendable wallets from watch-only wallets, a new member will be added to the CWallet class as well as a new corresponding value to the database file. The CWallet methods as well as RPC calls will then be able to distinguish between the two cases and exhibit the correct behavior.

@mikegogulski

OMG this is great!

@BitcoinPullTester

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/89f806c1d5e171ab48b29eeea8937fb2078e917a for binaries and test log.

@jgarzik
Owner

Neat. I'm surprised it was this easy.

Eric Lombrozo Separating Transaction Signing From Transaction Processing
==========================================================

A major issue many have faced in using bitcoind to build applications is the lack of RPC support for tracking transactions and balances without having to know and keep associated private keys in the wallet. Oftentimes I want to watch other people's transactions or to keep my signing nodes behind tighter security while using separate relay nodes to service all nonsigning, non-key-generating application functionality such as sending payment and confirmation alerts.

In order to achieve these objectives, I have had to build a bunch of custom software - much of which duplicates functionality that is already present in the Satoshi Client. This fork is an attempt at addressing some of these concerns without having to fundamentally restructure the client architecture. (Thanks, sipa!)

The proposal is to add another kind of object to the wallet database - a bitcoin address sans private key - which the client treats as if it were any other wallet adddress except for when it comes to signing and privkey export operations. This means RPC calls such as getreceivedbyaddress and listtransactions can be used on arbitrary bitcoin addresses.

I've added an RPC call:

	importaddress <bitcoinaddress> [label] [rescan=true]

The address is added as a new type of serialized object in wallet.dat and loads into the key maps of the CKeyStore instances with the key set to the CKeyID and the secret set to an empty vector.

Please test it out and let me know what you think.

Cheers,

-Eric Lombrozo
007a283
@sipa
Owner

Just a remark so we don't forget (I haven't checked whether it's possible with this implementation): it'd be nice to have a way to include arbitrary P2SH addresses as watch-only

@namecoin-qt namecoin-qt referenced this pull request from a commit in namecoin-qt/namecoin-qt
@namecoin-qt namecoin-qt Added importaddress RPC command from Codeshark's pull request #2121 79f8451
@jgarzik
Owner

Looks pretty good. Minor nit: "label" is deprecated in favor of "account." The help text must be changed. And if you are going to change the help text, go ahead and name the var strAccount as other, new code does.

Would like to see this merged.

@sipa
Owner

Yes, please rebase.

Regarding labels vs. accounts - I consider them separate things, which incidentally (and regrettably) overlap (if an address A has label L, payments to A will credit the account also called L). In this case, similarity with the existing API (importprivkey) is more important in any case, so I prefer seeing it called label.

@ryny24

is it possible to download and test this modification? I'd love to try it. Can't seem to select this branch

@kyledrake

I would love to see this rebased and merged! I'm going to be using this functionality for an upcoming new version of Coinpunk, to support client-side encryption for private keys, so that bitcoind becomes a listener, and cannot spend any of the user's money. This is a huge linchpin in my new version, and it enables this capability for people developing web services and offline clients that use a central bitcoind instance to keep up with the blockchain while they are offline. And of course cold storage wallets that generate the private keys on their own end.

I won't be able to release the new Coinpunk until this is released, so it would be amazing if this commit could get into the next release. I don't think it's too dangerous, just because I believe it's not handling anything that could potentially lose bitcoins, but I could be mistaken. But it would be awesome.

Let me know if I can help in any way. Thanks, you guys are awesome. :-)

@kiaya

I'd also love to see this make the next release. I have a project where I need to watch a large number of addresses without having the private keys on the server. I think the pull request would solve my biggest current development challenge.

@tstranex

Just thinking about an alternative to this: Set a wallet passphrase so that all the private keys are encrypted and then write a separate utility to overwrite the encrypted private keys with zeros in the wallet file. Since bitcoind is not going to access the keys anyway while they're encrypted, it shouldn't matter that they're actually zeros. So that will effectively produce watch-only addresses. However, it still wouldn't be possible to add new watch-only addresses via rpc, which this patch can do.

By the way, could a slightly cleaner implementation of this be to store watch-only addresses as normal keys (except where the encrypted private key is cleared) instead of adding the new "address" data type in the walletdb?

@kiaya

Thanks for this, tstranex. I hadn't considered that. I might look into this as a fall back. In my case, I'm using vanity addresses so actually don't need to create addresses via RPC.

@luke-jr

@CodeShark Needs rebase.

Although I don't think this makes sense to merge until we have HD wallets and it can be an entire read-only address-chain...

@kyledrake

I compiled and tested this code, and it seems to work fine. Does it collide with HD wallets? If not, I would really like to advocate for getting this in. I strongly believe that this code solves a critically urgent security problem.

Without this code, I'm concerned that we may get more services that put private keys on servers simply because they have no choice, which could lead to more major thefts, which could contribute to undermining the soundness of the Bitcoin ecosystem. The last thing Bitcoin needs right now is another high profile wallet theft fiasco.

This is a very important and very significant security upgrade, that I believe will dramatically lead to the improvement of security across the board, for many different types of applications. To provide analogies, this could be as important to bitcoind security as ProPolice was to buffer overflow prevention, or PBKDF2 was to password hashing.

Please consider including this in the next release of bitcoind. Do it for the people that have lost a lot of money to server wallet thefts. Do it for the children.

:wink:

@luke-jr

@kyledrake HD wallets should solve all of those issues, that's my point ;)

@kyledrake

@luke-jr I'm cool with HD Wallets, but HD Wallets are a long way away. This is a light, simple patch to solve a very big problem, that enables some major new infrastructure for bitcoin. I tried to rebase it and I got very close, I think someone with knowledge of the bitcoin source would very easily be able to rebase this. The biggest hurdle was the @sipa (I think?) refactor of keystore.cpp. The rest was just really simple merges.

@kyledrake

So here is my rebase attempt:

kyledrake@3171839

It fails to compile on this:

llvm-g++ -c -Wall -Wextra -Wformat -Wformat-security -Wno-unused-parameter -g -DMAC_OSX -DMSG_NOSIGNAL=0 -DBOOST_SPIRIT_THREADSAFE -DUSE_UPNP=1 -DUSE_IPV6=1 -I/Users/kyledrake/bitcoin/src/leveldb/include -I/Users/kyledrake/bitcoin/src/leveldb/helpers -DHAVE_BUILD_INFO -I"/Users/kyledrake/bitcoin/src" -I"/Users/kyledrake/bitcoin/src"/obj -I"/opt/local/include" -I"/opt/local/include/db48" -MMD -MF obj/keystore.d -o obj/keystore.o keystore.cpp
keystore.cpp: In member function ‘bool CBasicKeyStore::AddKey(const CKeyID&)’:
keystore.cpp:34: error: ‘CSecret’ was not declared in this scope
keystore.cpp:34: error: ‘make_pair’ was not declared in this scope
make: *** [obj/keystore.o] Error 1

Apparently CSecret has gone away.. any ideas on how to fix this?

@luke-jr

I think CSecret became CKey (and the old CKey become CPubKey).

HD wallets IIRC are planned for the next release.

@sipa
Owner

CKey and CSecret were merged. CPubKey is still CPubKey but inherited some of the pubkey-only functionality of the former CKey.

I consider this a very useful feature, and it's orthogonal to HD wallets. Sure, BIP32 specifies a derivation that can be useful for watch-only wallets, but it still needs to be implemented.

@kyledrake

@sipa Do you have any free time this week to look at this? I think it's a really super easy fix for you because you are very familiar with the code (and awesome). I think I got pretty close, I just don't know any C++ and I'm completely unfamiliar with the bitcoin keystore plumbing. http://i.imgur.com/xVyoSl.jpg

@runeksvendsen

FYI this patch compiles successfully against ec0004a.

I've tested importaddress with listtransactions and getbalance and it agrees with blockchain.info.

@kyledrake kyledrake referenced this pull request in gasteve/node-libcoin
Merged

importAddress for commands list #12

@BitcoinPullTester

Automatic sanity-testing: FAILED MERGE, see http://jenkins.bluematt.me/pull-tester/007a28374165f67d713a318bc37ef1286684cad5 for test log.

This pull does not merge cleanly onto current master
This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
Contact BlueMatt on freenode if something looks broken.

@kyledrake

For those of us in the bitcoind camp using an old unstable master so we can get importaddress, I have merged importaddress with @runeksvendsen's hint of ec0004a here: https://github.com/kyledrake/bitcoin/tree/importaddressupdate

Since I posted on this pull request, I have gotten emails from people using this commit in production, because there is no other alternative. Which makes me excited about its potential to improve bitcoin security, but also terrified because they are using it with unstable bitcoind. It would be really great if someone was able to look at this, I really doubt it would take more than a few hours for a seasoned member of the glorious, highly talented bitcoin development team to resolve the merge conflicts on it (@sipa? @jgarzik?).

@sipa
Owner

@kyledrake Have a look at #2861.

@meighti

It would be nice to add a feature in the code to easily or automatically LOCKUNSPENT all unspent coins of the imported watch-only address. Not to mention that those coins are not spendable without privkey.

In shell I run this:
bitcoind lockunspent false $(bitcoind listunspent 0 9999999 '["12c6DSiU4Rq3P4ZxziKxzrL5LmMBrzjrJX"]' |tr -d ' \n')

@jgarzik
Owner

Superceded by #2861, closing.

@jgarzik jgarzik closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 8, 2013
  1. @CodeShark

    Separating Transaction Signing From Transaction Processing

    Eric Lombrozo authored CodeShark committed
    ==========================================================
    
    A major issue many have faced in using bitcoind to build applications is the lack of RPC support for tracking transactions and balances without having to know and keep associated private keys in the wallet. Oftentimes I want to watch other people's transactions or to keep my signing nodes behind tighter security while using separate relay nodes to service all nonsigning, non-key-generating application functionality such as sending payment and confirmation alerts.
    
    In order to achieve these objectives, I have had to build a bunch of custom software - much of which duplicates functionality that is already present in the Satoshi Client. This fork is an attempt at addressing some of these concerns without having to fundamentally restructure the client architecture. (Thanks, sipa!)
    
    The proposal is to add another kind of object to the wallet database - a bitcoin address sans private key - which the client treats as if it were any other wallet adddress except for when it comes to signing and privkey export operations. This means RPC calls such as getreceivedbyaddress and listtransactions can be used on arbitrary bitcoin addresses.
    
    I've added an RPC call:
    
    	importaddress <bitcoinaddress> [label] [rescan=true]
    
    The address is added as a new type of serialized object in wallet.dat and loads into the key maps of the CKeyStore instances with the key set to the CKeyID and the secret set to an empty vector.
    
    Please test it out and let me know what you think.
    
    Cheers,
    
    -Eric Lombrozo
This page is out of date. Refresh to see the latest.
View
2  src/bitcoinrpc.cpp
@@ -247,6 +247,7 @@ static const CRPCCommand vRPCCommands[] =
{ "listsinceblock", &listsinceblock, false, false },
{ "dumpprivkey", &dumpprivkey, true, false },
{ "importprivkey", &importprivkey, false, false },
+ { "importaddress", &importaddress, false, false },
{ "listunspent", &listunspent, false, false },
{ "getrawtransaction", &getrawtransaction, false, false },
{ "createrawtransaction", &createrawtransaction, false, false },
@@ -1170,6 +1171,7 @@ Array RPCConvertValues(const std::string &strMethod, const std::vector<std::stri
if (strMethod == "lockunspent" && n > 0) ConvertTo<bool>(params[0]);
if (strMethod == "lockunspent" && n > 1) ConvertTo<Array>(params[1]);
if (strMethod == "importprivkey" && n > 2) ConvertTo<bool>(params[2]);
+ if (strMethod == "importaddress" && n > 2) ConvertTo<bool>(params[2]);
return params;
}
View
1  src/bitcoinrpc.h
@@ -137,6 +137,7 @@ extern json_spirit::Value addnode(const json_spirit::Array& params, bool fHelp);
extern json_spirit::Value getaddednodeinfo(const json_spirit::Array& params, bool fHelp);
extern json_spirit::Value dumpprivkey(const json_spirit::Array& params, bool fHelp); // in rpcdump.cpp
extern json_spirit::Value importprivkey(const json_spirit::Array& params, bool fHelp);
+extern json_spirit::Value importaddress(const json_spirit::Array& params, bool fHelp);
extern json_spirit::Value getgenerate(const json_spirit::Array& params, bool fHelp); // in rpcmining.cpp
extern json_spirit::Value setgenerate(const json_spirit::Array& params, bool fHelp);
View
28 src/keystore.cpp
@@ -26,6 +26,16 @@ bool CBasicKeyStore::AddKey(const CKey& key)
return true;
}
+bool CBasicKeyStore::AddKey(const CKeyID& address)
+{
+ // The key is watch-only. We don't have the secret.
+ {
+ LOCK(cs_KeyStore);
+ mapKeys[address] = make_pair(CSecret(), false);
+ }
+ return true;
+}
+
bool CBasicKeyStore::AddCScript(const CScript& redeemScript)
{
{
@@ -139,6 +149,24 @@ bool CCryptoKeyStore::AddKey(const CKey& key)
return true;
}
+bool CCryptoKeyStore::AddKey(const CKeyID& address)
+{
+ // The key is watch-only. We don't have the secret.
+ {
+ LOCK(cs_KeyStore);
+ if (!IsCrypted())
+ return CBasicKeyStore::AddKey(address);
+
+ if (IsLocked())
+ return false;
+
+ if (!SetCrypted())
+ return false;
+
+ mapCryptedKeys[address] = make_pair(CPubKey(), std::vector<unsigned char>());
+ }
+ return true;
+}
bool CCryptoKeyStore::AddCryptedKey(const CPubKey &vchPubKey, const std::vector<unsigned char> &vchCryptedSecret)
{
View
2  src/keystore.h
@@ -56,6 +56,7 @@ class CBasicKeyStore : public CKeyStore
public:
bool AddKey(const CKey& key);
+ bool AddKey(const CKeyID& address);
bool HaveKey(const CKeyID &address) const
{
bool result;
@@ -147,6 +148,7 @@ class CCryptoKeyStore : public CBasicKeyStore
virtual bool AddCryptedKey(const CPubKey &vchPubKey, const std::vector<unsigned char> &vchCryptedSecret);
bool AddKey(const CKey& key);
+ bool AddKey(const CKeyID& address);
bool HaveKey(const CKeyID &address) const
{
{
View
40 src/rpcdump.cpp
@@ -77,6 +77,46 @@ Value importprivkey(const Array& params, bool fHelp)
return Value::null;
}
+Value importaddress(const Array& params, bool fHelp)
+{
+ if (fHelp || params.size() < 1 || params.size() > 3)
+ throw runtime_error(
+ "importaddress <bitcoinaddress> [label] [rescan=true]\n"
+ "Adds an address that can be watched as if it were in your wallet but cannot be used to spend.");
+
+ CKeyID vchAddress;
+ CBitcoinAddress address(params[0].get_str());
+ if (!address.GetKeyID(vchAddress)) throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid Bitcoin address");
+
+ string strLabel = "";
+ if (params.size() > 1)
+ strLabel = params[1].get_str();
+
+ // Whether to perform rescan after import
+ bool fRescan = true;
+ if (params.size() > 2)
+ fRescan = params[2].get_bool();
+
+ {
+ LOCK2(cs_main, pwalletMain->cs_wallet);
+
+ pwalletMain->MarkDirty();
+ pwalletMain->SetAddressBookName(vchAddress, strLabel);
+
+ if (!pwalletMain->AddAddress(vchAddress))
+ throw JSONRPCError(RPC_WALLET_ERROR, "Error adding address to wallet");
+
+ if (fRescan)
+ {
+ pwalletMain->ScanForWalletTransactions(pindexGenesisBlock, true);
+ pwalletMain->ReacceptWalletTransactions();
+ }
+ }
+
+ return Value::null;
+}
+
+
Value dumpprivkey(const Array& params, bool fHelp)
{
if (fHelp || params.size() != 1)
View
11 src/wallet.cpp
@@ -55,6 +55,17 @@ bool CWallet::AddKey(const CKey& key)
return true;
}
+bool CWallet::AddAddress(const CKeyID& address)
+{
+ if (!CCryptoKeyStore::AddKey(address))
+ return false;
+ if (!fFileBacked)
+ return true;
+ if (!IsCrypted())
+ return CWalletDB(strWalletFile).WriteAddress(address);
+ return true;
+}
+
bool CWallet::AddCryptedKey(const CPubKey &vchPubKey, const vector<unsigned char> &vchCryptedSecret)
{
if (!CCryptoKeyStore::AddCryptedKey(vchPubKey, vchCryptedSecret))
View
4 src/wallet.h
@@ -137,8 +137,12 @@ class CWallet : public CCryptoKeyStore
CPubKey GenerateNewKey();
// Adds a key to the store, and saves it to disk.
bool AddKey(const CKey& key);
+ // Adds a watching address to the store, saves it to disk.
+ bool AddAddress(const CKeyID& address);
// Adds a key to the store, without saving it to disk (used by LoadWallet)
bool LoadKey(const CKey& key) { return CCryptoKeyStore::AddKey(key); }
+ // Adds a watching address to the store, without saving it to disk (used by LoadWallet)
+ bool LoadAddress(const CKeyID& address) { return CCryptoKeyStore::AddKey(address); }
bool LoadMinVersion(int nVersion) { nWalletVersion = nVersion; nWalletMaxVersion = std::max(nWalletMaxVersion, nVersion); return true; }
View
10 src/walletdb.cpp
@@ -369,6 +369,16 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue,
return false;
}
}
+ else if (strType == "address")
+ {
+ CKeyID address;
+ ssKey >> address;
+ if (!pwallet->LoadAddress(address))
+ {
+ strErr = "Error reading wallet database: LoadKey failed on address";
+ return false;
+ }
+ }
else if (strType == "orderposnext")
{
ssValue >> pwallet->nOrderPosNext;
View
6 src/walletdb.h
@@ -56,6 +56,12 @@ class CWalletDB : public CDB
return Write(std::make_pair(std::string("key"), vchPubKey.Raw()), vchPrivKey, false);
}
+ bool WriteAddress(const CKeyID& address)
+ {
+ nWalletDBUpdated++;
+ return Write(std::make_pair(std::string("address"), address), address, false);
+ }
+
bool WriteCryptedKey(const CPubKey& vchPubKey, const std::vector<unsigned char>& vchCryptedSecret, bool fEraseUnencryptedKey = true)
{
nWalletDBUpdated++;
Something went wrong with that request. Please try again.