wallet.cpp:ExtractPubKey() (see recent change in commit 798a589aff64b83a0844688a661f4bd987c3340c) which is however static and only indirectly available via the public methods AddWatchOnly(), LoadWatchOnly() and RemoveWatchOnly(). Since the first of those methods also stores the addresses to the disk, the second, simpler one was chosen which only operates in memory.
This is currently failing on Travis:
0wallet/test/wallet_tests.cpp(381): Entering test case "WatchOnlyPubKeys"
1Assertion failed: lock cs_wallet not held in wallet/wallet.cpp:578; locks held:
2unknown location(0): fatal error: in "wallet_tests/WatchOnlyPubKeys": signal: SIGABRT (application abort requested)
3wallet/test/wallet_tests.cpp(357): last checkpoint
4wallet/test/wallet_tests.cpp(381): Leaving test case "WatchOnlyPubKeys"; testing time: 138195us
0wallet/test/wallet_tests.cpp:362:5: error: calling function 'RemoveWatchOnly' requires holding mutex 'wallet.cs_wallet' exclusively [-Werror,-Wthread-safety-analysis]
1 wallet.RemoveWatchOnly(p2pk);
2 ^
31 error generated.
You’ll need to add some locking LOCK(wallet->cs_wallet);
This is currently failing on Travis: … You’ll need to add some locking
LOCK(wallet->cs_wallet);
Thanks, done (f74dc7ea05cf466077c6f46910ae4228373656e4). It took me quite a while to reproduce this warning, obviously it only appears compiling with clang (which makes sense after looking at src/threadsafety.h, where all the defines like e.g. EXCLUSIVE_LOCKS_REQUIRED are empty for g++).
333@@ -334,6 +334,82 @@ BOOST_AUTO_TEST_CASE(LoadReceiveRequests)
334 BOOST_CHECK_EQUAL(values[1], "val_rr1");
335 }
336
337+// Test some watch-only wallet methods by the procedure of loading (LoadWatchOnly),
338+// checking (HaveWatchOnly), getting (GetWatchPubKey) and removing (RemoveWatchOnly) a
339+// given PubKey, resp. its corresponding P2PK Script. Results of the the impact on
340+// the address -> PubKey map is dependent on whether the PubKey is cryptographically
cryptographically correct/a point on the curve/
369+// Cryptographically invalidate a PubKey whilst keeping length and first byte
370+static void PollutePubKey(CPubKey& pubkey)
371+{
372+ do {
373+ std::vector<unsigned char> pubkey_raw(pubkey.begin(), pubkey.end());
374+ std::random_shuffle(pubkey_raw.begin()+1, pubkey_raw.end());
343+{
344+ CScript p2pk = GetScriptForRawPubKey(add_pubkey);
345+ CKeyID add_address = add_pubkey.GetID();
346+ CPubKey found_pubkey;
347+
348+ // all Scripts (i.e. also all PubKeys) are add to the general watch-only set
348+ // all Scripts (i.e. also all PubKeys) are add to the general watch-only set
349+ BOOST_CHECK(!wallet.HaveWatchOnly(p2pk));
350+ wallet.LoadWatchOnly(p2pk);
351+ BOOST_CHECK(wallet.HaveWatchOnly(p2pk));
352+
353+ // only cryptographically valid PubKeys shall be add to the watch-only address -> PubKey map
354+ bool isPubKeyFullyValid = add_pubkey.IsFullyValid();
355+ if (isPubKeyFullyValid) {
356+ BOOST_CHECK(wallet.GetWatchPubKey(add_address, found_pubkey));
357+ BOOST_CHECK(found_pubkey == add_pubkey);
358+ } else {
359+ BOOST_CHECK(!wallet.GetWatchPubKey(add_address, found_pubkey));
found_pubkey ends up being in this situation
found_pubkey isn’t touched when no PubKey is found, but of course this fact can be tested as well by asserting it to be a newly constructed (and thus invalid) PubKey.
361+ }
362+
363+ wallet.RemoveWatchOnly(p2pk);
364+ BOOST_CHECK(!wallet.HaveWatchOnly(p2pk));
365+
366+ if (isPubKeyFullyValid)
nit: braces
As above, you should also check that found_pubkey is the default CPubKey.
GetWatchPubKey doesn’t modify the passed PubKey if it returns false, it would still have the same contents as before, so in this case the correct assertion would be found_pubkey == add_pubkey. To not confuse the reader (one could wrongly assume that this assert checks for an explicit modification by the function in this case as well), I add the comment // passed key is unchanged for those two cases.
381+BOOST_AUTO_TEST_CASE(WatchOnlyPubKeys)
382+{
383+ CKey key;
384+ CPubKey pubkey;
385+
386+ assert(m_wallet.HaveWatchOnly() == false);
BOOST_CHECK instead of assert.
386+ assert(m_wallet.HaveWatchOnly() == false);
387+
388+ // uncompressed valid PubKey
389+ key.MakeNewKey(false);
390+ pubkey = key.GetPubKey();
391+ assert(pubkey.size() == CPubKey::PUBLIC_KEY_SIZE);
!IsCompressed rather than checking the size directly.
349+ BOOST_CHECK(!wallet.HaveWatchOnly(p2pk));
350+ wallet.LoadWatchOnly(p2pk);
351+ BOOST_CHECK(wallet.HaveWatchOnly(p2pk));
352+
353+ // only PubKeys on the curve shall be added to the watch-only address -> PubKey map
354+ bool isPubKeyFullyValid = add_pubkey.IsFullyValid();
The motivation for this addition was to unit test the function
wallet.cpp:ExtractPubKey() (see recent change in commit
798a589aff64b83a0844688a661f4bd987c3340c) which is however static and only
indirectly available via the public methods AddWatchOnly(), LoadWatchOnly() and
RemoveWatchOnly(). Since the first of those methods also stores the addresses
to the disk, the second, simpler one was chosen which only operates in memory.
test: add missing wallet lock for test case WatchOnlyPubKeys
test: test case WatchOnlyPubKeys, suggested review changes by instagibbs
test: test case WatchOnlyPubKeys, suggested review changes by achow101
test: test case WatchOnlyPubKeys, s/isPubKeyFullyValid/is_pubkey_fully_valid
ACK a57a1d4
As an aside, in case anyone wants to test P2PK functionality (on testnet), here’s a fun tool: https://github.com/dianerey/bech32p2pkaddress (no need to provide inputs in the Python script, just use fundrawtransaction and signrawtransactionwithwallet).
When you import a pubkey using importpubkey, the wallet watches for P2PK transactions as well. It also happily imports invalid pubkeys to watch. The same goes for importaddress 21PUBKEYac (P2PK script). The wallet also happily signs raw transactions to such keys, which should make anyone appreciate checksums.
Although the above scenarios are rather contrived, should we have some RPC methods check if a pubkey is on a curve?
@Sjors: Thanks for the review!
When you import a pubkey using
importpubkey, the wallet watches for P2PK transactions as well. It also happily imports invalid pubkeys to watch. The same goes forimportaddress 21PUBKEYac(P2PK script). The wallet also happily signs raw transactions to such keys, which should make anyone appreciate checksums.
Are you sure that importpubkey imports invalid pubkeys? Can you give an example? Looking at wallet/rpcdump.cpp:importpubkey() there is a check if the given pubkey is on the curve, throwing an error if it isn’t:
if (!pubKey.IsFullyValid())
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Pubkey is not a valid public key");
Oops, I just changed one character in the pubkey hex thinking that would do the trick, but of course with a compressed pubkey such a tweaked point is most likely still on the curve.
importpubkey 020000000000000000000000000000000000000000000000000000000000000000 indeed throws. importaddress 21020000000000000000000000000000000000000000000000000000000000000000ac does work.
Oops, I just changed one character in the pubkey hex thinking that would do the trick, but of course with a compressed pubkey such a tweaked point is most likely still on the curve.
Okay :+1:
importpubkey 020000000000000000000000000000000000000000000000000000000000000000indeed throws.importaddress 21020000000000000000000000000000000000000000000000000000000000000000acdoes work.
Looking at the code, importaddress doesn’t try to interpret (or validate) the contents of the passed hex-encoded script in any way but rather passes it to the wallet directly via ImportScripts() and ImportScriptPubKeys(). The RPC manual says to this call “If you have the full public key, you should call importpubkey instead of this.”, so I guess it is okay that there is no on-the-curve check here for pubkeys contained in P2PK scripts.