test: add unit test for wallet watch-only methods involving PubKeys #16786

pull theStack wants to merge 1 commits into bitcoin:master from theStack:add_wallet_watchpubkey_unit_test changing 1 files +78 −0
  1. theStack commented at 8:35 PM on September 1, 2019: member

    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.

  2. fanquake added the label Tests on Sep 1, 2019
  3. fanquake requested review from achow101 on Sep 3, 2019
  4. fanquake requested review from instagibbs on Sep 3, 2019
  5. fanquake commented at 4:14 AM on September 3, 2019: member

    This is currently failing on Travis:

    wallet/test/wallet_tests.cpp(381): Entering test case "WatchOnlyPubKeys"
Assertion failed: lock cs_wallet not held in wallet/wallet.cpp:578; locks held:
unknown location(0): fatal error: in "wallet_tests/WatchOnlyPubKeys": signal: SIGABRT (application abort requested)
wallet/test/wallet_tests.cpp(357): last checkpoint
wallet/test/wallet_tests.cpp(381): Leaving test case "WatchOnlyPubKeys"; testing time: 138195us

    wallet/test/wallet_tests.cpp:362:5: error: calling function 'RemoveWatchOnly' requires holding mutex 'wallet.cs_wallet' exclusively [-Werror,-Wthread-safety-analysis]
    wallet.RemoveWatchOnly(p2pk);
    ^
1 error generated.

    You'll need to add some locking LOCK(wallet->cs_wallet);

  6. theStack commented at 6:58 AM on September 3, 2019: member

    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++).

  7. in src/wallet/test/wallet_tests.cpp:340 in 6a06b1d815 outdated 
     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
    instagibbs commented at 12:59 PM on September 3, 2019:

    s/cryptographically correct/a point on the curve/

    theStack commented at 10:27 PM on September 3, 2019:

    Done

  8. in src/wallet/test/wallet_tests.cpp:375 in 6a06b1d815 outdated 
     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());
    instagibbs commented at 1:01 PM on September 3, 2019:

    Would it just be simpler for this test to zero out the last 32/64 bytes? Is having the shuffled bytes testing anything important ?

    theStack commented at 2:40 PM on September 3, 2019:

    Good point -- no, the shuffling doesn't test anything import, for some reason I didn't want to set the PubKey coordinate(s) to a fixed value off the curve. Zeroing out is simpler and saves the loop.

  9. in src/wallet/test/wallet_tests.cpp:349 in 6a06b1d815 outdated 
     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
    instagibbs commented at 1:02 PM on September 3, 2019:

    s/add/added/

    theStack commented at 10:27 PM on September 3, 2019:

    Done

  10. in src/wallet/test/wallet_tests.cpp:354 in 6a06b1d815 outdated 
     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
    instagibbs commented at 1:03 PM on September 3, 2019:

    s/add/added/

    theStack commented at 10:28 PM on September 3, 2019:

    Done

  11. in src/wallet/test/wallet_tests.cpp:359 in 6a06b1d815 outdated 
     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));
    instagibbs commented at 1:04 PM on September 3, 2019:

    maybe also assert what found_pubkey ends up being in this situation

    theStack commented at 2:42 PM on September 3, 2019:

    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.

  12. instagibbs commented at 1:05 PM on September 3, 2019: member

    you should just squash the 2 commits together

  13. theStack force-pushed on Sep 3, 2019
  14. in src/wallet/test/wallet_tests.cpp:366 in c19bb7b313 outdated 
     361 | +    }
 362 | +
 363 | +    wallet.RemoveWatchOnly(p2pk);
 364 | +    BOOST_CHECK(!wallet.HaveWatchOnly(p2pk));
 365 | +
 366 | +    if (isPubKeyFullyValid)
    achow101 commented at 6:12 PM on September 3, 2019:

    nit: braces

    As above, you should also check that found_pubkey is the default CPubKey.

    theStack commented at 7:50 PM on September 3, 2019:

    Since 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.

  15. in src/wallet/test/wallet_tests.cpp:386 in c19bb7b313 outdated 
     381 | +BOOST_AUTO_TEST_CASE(WatchOnlyPubKeys)
 382 | +{
 383 | +    CKey key;
 384 | +    CPubKey pubkey;
 385 | +
 386 | +    assert(m_wallet.HaveWatchOnly() == false);
    achow101 commented at 6:13 PM on September 3, 2019:

    Use BOOST_CHECK instead of assert.

    theStack commented at 10:28 PM on September 3, 2019:

    Done

  16. in src/wallet/test/wallet_tests.cpp:391 in c19bb7b313 outdated 
     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);
    achow101 commented at 6:15 PM on September 3, 2019:

    Should use !IsCompressed rather than checking the size directly.

    theStack commented at 10:29 PM on September 3, 2019:

    Done

  17. theStack force-pushed on Sep 7, 2019
  18. fanquake requested review from achow101 on Sep 16, 2019
  19. fanquake requested review from instagibbs on Sep 16, 2019
  20. instagibbs approved
  21. instagibbs commented at 1:41 PM on September 16, 2019: member

    utACK

  22. in src/wallet/test/wallet_tests.cpp:354 in 3a219e8230 outdated 
     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();
    achow101 commented at 4:29 PM on September 16, 2019:

    nit: snake_case

    theStack commented at 6:16 AM on September 17, 2019:

    Done

  23. achow101 approved
  24. achow101 commented at 4:29 PM on September 16, 2019: member

    Code Review ACK 3a219e82304c7f62d75369d6ee55111f601cc892

  25. test: add unit test for wallet watch-only methods involving PubKeys
    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
    a57a1d42d5
  26. theStack force-pushed on Sep 16, 2019
  27. theStack commented at 6:18 AM on September 17, 2019: member

    @instagibbs @achow101 Thanks for your reviews!

  28. Sjors commented at 10:41 AM on September 19, 2019: member

    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?

  29. theStack commented at 1:44 PM on September 19, 2019: member

    @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 for importaddress 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");

  30. Sjors commented at 3:19 PM on September 19, 2019: member

    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.

  31. theStack commented at 5:51 PM on September 19, 2019: member

    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 020000000000000000000000000000000000000000000000000000000000000000 indeed throws. importaddress 21020000000000000000000000000000000000000000000000000000000000000000ac does 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.

  32. theStack commented at 1:58 PM on October 10, 2019: member

    Since there has been no activity for three weeks: is there anything else I can do for this PR?

  33. fanquake requested review from achow101 on Oct 10, 2019
  34. fanquake requested review from instagibbs on Oct 10, 2019
  35. fanquake requested review from Sjors on Oct 10, 2019
  37. Sjors approved
  38. Sjors commented at 3:10 PM on October 10, 2019: member

    re-ACK a57a1d4

  39. MarcoFalke referenced this in commit 59f0687fea on Oct 10, 2019
  40. MarcoFalke merged this on Oct 10, 2019
  41. MarcoFalke closed this on Oct 10, 2019
  42. sidhujag referenced this in commit 682b9d59e3 on Oct 11, 2019
  43. theStack deleted the branch on Dec 1, 2020
  44. PastaPastaPasta referenced this in commit 7fcccf9454 on Jun 27, 2021
  45. PastaPastaPasta referenced this in commit 46ad484153 on Jun 28, 2021
  46. PastaPastaPasta referenced this in commit 7be594d09d on Jun 29, 2021
  47. PastaPastaPasta referenced this in commit c8d9167207 on Sep 11, 2021
  48. PastaPastaPasta referenced this in commit bfc4661229 on Sep 11, 2021
  49. PastaPastaPasta referenced this in commit 050bef2bc1 on Sep 12, 2021
  50. PastaPastaPasta referenced this in commit 9dc067498e on Sep 12, 2021
  51. DrahtBot locked this on Feb 15, 2022
Contributors
theStackfanquakeinstagibbsachow101Sjors


achow101instagibbs

Labels
Tests
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-22 00:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me