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

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.

Sign up for GitHub

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

Jump to bottom

Merged

maflcko merged 1 commit into bitcoin:master from theStack:add_wallet_watchpubkey_unit_test

Oct 10, 2019

+78 −0

Contributor

theStack commented Sep 1, 2019

The motivation for this addition was to unit test the function wallet.cpp:ExtractPubKey() (see recent change in commit 798a589) 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.

fanquake added the Tests label

fanquake requested review from achow101 and instagibbs

September 3, 2019 04:05

Member

fanquake commented Sep 3, 2019

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);

Contributor Author

theStack commented Sep 3, 2019

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

instagibbs reviewed

View reviewed changes

src/wallet/test/wallet_tests.cpp Outdated Show resolved Hide resolved

src/wallet/test/wallet_tests.cpp Outdated Show resolved Hide resolved

src/wallet/test/wallet_tests.cpp Outdated Show resolved Hide resolved

src/wallet/test/wallet_tests.cpp Outdated Show resolved Hide resolved

src/wallet/test/wallet_tests.cpp Outdated

Member

instagibbs Sep 3, 2019

maybe also assert what found_pubkey ends up being in this situation

Contributor Author

theStack Sep 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.

Member

instagibbs commented Sep 3, 2019

you should just squash the 2 commits together

theStack force-pushed the add_wallet_watchpubkey_unit_test branch from f74dc7e to d0e1d33 Compare

September 3, 2019 14:17

achow101 reviewed

View reviewed changes

src/wallet/test/wallet_tests.cpp Outdated

Member

achow101 Sep 3, 2019

nit: braces

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

Contributor Author

theStack Sep 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.

src/wallet/test/wallet_tests.cpp Outdated Show resolved Hide resolved

src/wallet/test/wallet_tests.cpp Outdated Show resolved Hide resolved

theStack force-pushed the add_wallet_watchpubkey_unit_test branch from d613804 to 3a219e8 Compare

September 7, 2019 13:43

fanquake requested review from achow101 and instagibbs

September 16, 2019 08:43

instagibbs approved these changes

View reviewed changes

Member

instagibbs left a comment

utACK

achow101 approved these changes

View reviewed changes

Member

achow101 left a comment

Code Review ACK 3a219e82304c7f62d75369d6ee55111f601cc892

src/wallet/test/wallet_tests.cpp Outdated Show resolved Hide resolved


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

a57a1d4

The motivation for this addition was to unit test the function
wallet.cpp:ExtractPubKey() (see recent change in commit
798a589) 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

theStack force-pushed the add_wallet_watchpubkey_unit_test branch from 601698c to a57a1d4 Compare

September 16, 2019 21:24

Contributor Author

theStack commented Sep 17, 2019

@instagibbs @achow101 Thanks for your reviews!

Member

Sjors commented Sep 19, 2019 •

edited

Loading

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?

Contributor Author

theStack commented Sep 19, 2019

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

Member

Sjors commented Sep 19, 2019

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.

Contributor Author

theStack commented Sep 19, 2019

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 👍

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.

Contributor Author

theStack commented Oct 10, 2019

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

fanquake requested review from Sjors, achow101 and instagibbs

October 10, 2019 13:59

Member

instagibbs commented Oct 10, 2019

reACK a57a1d4

just squashed

Sjors approved these changes

View reviewed changes

Member

Sjors left a comment

re-ACK a57a1d4

maflcko pushed a commit that referenced this pull request


          Merge #16786: test: add unit test for wallet watch-only methods invol…

59f0687

…ving PubKeys

a57a1d4 test: add unit test for wallet watch-only methods involving PubKeys (Sebastian Falbesoner)

Pull request description:

  The motivation for this addition was to unit test the function `wallet.cpp:ExtractPubKey()` (see recent change in commit 798a589) 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.

ACKs for top commit:
  Sjors:
    ACK a57a1d4
  instagibbs:
    reACK a57a1d4
  Sjors:
    re-ACK a57a1d4

Tree-SHA512: 92a242204ab533022cd848662997372c41815b1265d07b3d96305697f801db29a5ba5668337faf4bea702bec1451972529afd6665927fb142aaf91700a338b26

maflcko merged commit a57a1d4 into bitcoin:master

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request


          Merge bitcoin#16786: test: add unit test for wallet watch-only method…

682b9d5

…s involving PubKeys

a57a1d4 test: add unit test for wallet watch-only methods involving PubKeys (Sebastian Falbesoner)

Pull request description:

  The motivation for this addition was to unit test the function `wallet.cpp:ExtractPubKey()` (see recent change in commit 798a589) 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.

ACKs for top commit:
  Sjors:
    ACK a57a1d4
  instagibbs:
    reACK bitcoin@a57a1d4
  Sjors:
    re-ACK a57a1d4

Tree-SHA512: 92a242204ab533022cd848662997372c41815b1265d07b3d96305697f801db29a5ba5668337faf4bea702bec1451972529afd6665927fb142aaf91700a338b26

Sjors mentioned this pull request

Introduce ScriptPubKeyMan interface and use it for key and script management (aka wallet boxes) #16341

Closed

MarkLTZ mentioned this pull request

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

theStack deleted the add_wallet_watchpubkey_unit_test branch

December 1, 2020 09:59

PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request


          Merge bitcoin#16786: test: add unit test for wallet watch-only method…

bfc4661

…s involving PubKeys

a57a1d4 test: add unit test for wallet watch-only methods involving PubKeys (Sebastian Falbesoner)

Pull request description:

  The motivation for this addition was to unit test the function `wallet.cpp:ExtractPubKey()` (see recent change in commit 798a589) 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.

ACKs for top commit:
  Sjors:
    ACK a57a1d4
  instagibbs:
    reACK bitcoin@a57a1d4
  Sjors:
    re-ACK a57a1d4

Tree-SHA512: 92a242204ab533022cd848662997372c41815b1265d07b3d96305697f801db29a5ba5668337faf4bea702bec1451972529afd6665927fb142aaf91700a338b26

bitcoin locked as resolved and limited conversation to collaborators

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels