/bitcoin

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Add listunspent() test for spendable/unspendable UTXO #7822

Merged
merged 2 commits into from Apr 19, 2016

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
Tests
Projects
None yet
Milestone
No milestone
3 participants
@joaopaulofonseca @MarcoFalke @laanwj
@joaopaulofonseca
Contributor

joaopaulofonseca commented Apr 6, 2016 •
Edited 1 time

This PR adds a test on wallet.py covering the behavior of spendable/unspendable value for UTXO, when calling RPC listunspent.

This value should be different for address or private key import.

@joaopaulofonseca joaopaulofonseca referenced this pull request Apr 6, 2016

Closed

Add index to wallet UTXO #7823

@laanwj laanwj added the Tests label Apr 6, 2016

@joaopaulofonseca joaopaulofonseca referenced this pull request in uphold/bitcoin Apr 6, 2016

Closed

Added test to listunspent RPC #7

@MarcoFalke MarcoFalke commented on an outdated diff Apr 6, 2016

View full outdated diff
qa/rpc-tests/listunspent.py
+ # since the only available UTXO was spent, there sould be none after the transaction
+ assert_equal(len(self.nodes[0].listunspent()), 0)
+
+ # mine one more block to generate some more UTXOs
+ self.nodes[0].generate(1)
+ self.sync_all()
+
+ # list unspents after the transaction
+ list_unspent_after = self.nodes[0].listunspent()
+
+ # retrieve that transaction and check if the transaction inputs are not among the unspents
+ rawtx = self.nodes[0].getrawtransaction(txid, 1)
+ for txin in rawtx["vin"]:
+ for utxo in list_unspent_after:
+ if (txin["txid"] == utxo["txid"] and txin["vout"] == utxo["vout"]):
+ raise AssertionError("Fount used output on UTXOs list")
@MarcoFalke

MarcoFalke Apr 6, 2016

Member

Nit: typo

@MarcoFalke MarcoFalke commented on an outdated diff Apr 6, 2016

View full outdated diff
qa/rpc-tests/listunspent.py
+ assert_equal(len(self.nodes[1].listunspent()), 0)
+
+ # import address from another node
+ self.nodes[1].importaddress(send_to_address)
+
+ # making sure this address is watch-only
+ assert(self.nodes[1].validateaddress(send_to_address)["iswatchonly"])
+
+ # there should be only one unspendable unspent, from the imported address
+ assert_equal(len(self.nodes[1].listunspent()), 1)
+ assert(not self.nodes[1].listunspent()[0]["spendable"])
+
+ try:
+ self.nodes[1].sendtoaddress(self.nodes[0].getnewaddress(), 1)
+ except JSONRPCException:
+ print("Impossible to send coins. Available output is unspendable.")
@MarcoFalke

MarcoFalke Apr 6, 2016

Member

You may want to try assert_raises instead.

@MarcoFalke MarcoFalke commented on an outdated diff Apr 6, 2016

View full outdated diff
qa/rpc-tests/listunspent.py
+ #######################################################################
+ # Test case 3 #
+ # Import private key and check for updated unspents (now spendable) #
+ #######################################################################
+
+ # import private key, so this node can spend the available UTXO
+ priv_key = self.nodes[2].dumpprivkey(send_to_address)
+ self.nodes[1].importprivkey(priv_key)
+
+ # there should be only one spendable unspent, after importing private key
+ assert_equal(len(self.nodes[1].listunspent()), 1)
+ assert(self.nodes[1].listunspent()[0]["spendable"])
+
+ try:
+ self.nodes[1].sendtoaddress(self.nodes[0].getnewaddress(), 0.1)
+ except JSONRPCException, e:
@MarcoFalke

MarcoFalke Apr 6, 2016

Member

Please use py3 syntax here: as e:

@MarcoFalke MarcoFalke and 1 other commented on an outdated diff Apr 6, 2016

View full outdated diff
qa/rpc-tests/listunspent.py
+ # Test case 3 #
+ # Import private key and check for updated unspents (now spendable) #
+ #######################################################################
+
+ # import private key, so this node can spend the available UTXO
+ priv_key = self.nodes[2].dumpprivkey(send_to_address)
+ self.nodes[1].importprivkey(priv_key)
+
+ # there should be only one spendable unspent, after importing private key
+ assert_equal(len(self.nodes[1].listunspent()), 1)
+ assert(self.nodes[1].listunspent()[0]["spendable"])
+
+ try:
+ self.nodes[1].sendtoaddress(self.nodes[0].getnewaddress(), 0.1)
+ except JSONRPCException, e:
+ raise AssertionError("Not possible to send coins. Possible unsufficient funds.")
@MarcoFalke

MarcoFalke Apr 6, 2016

Member

Nit: No need to shadow the original exception. You can just remove the try-except block altogether and only leave the sendtoaddress

@joaopaulofonseca

joaopaulofonseca Apr 6, 2016

Contributor

Thanks for the tips. Fixed and rebased.
@MarcoFalke
Member

MarcoFalke commented Apr 6, 2016

Thanks, Concept ACK.

@joaopaulofonseca joaopaulofonseca changed the title from [qa] Add test to RPC listunspent to Add test to RPC listunspent Apr 7, 2016

@laanwj
Owner

laanwj commented Apr 14, 2016

I'm a bit divided whether I should propose to add this into the wallet test, which already uses listunspent. This currently adds some startup/teardown overhead (test takes ~9 seconds) just to test one call.
I don't feel strongly about that though.
What do you think @MarcoFalke?
@MarcoFalke
Member

MarcoFalke commented Apr 15, 2016

startup/teardown overhead

I think such overhead is rather small. (Note that disablewallet.py runs in less than a second)

The overhead probably is caused by setting up the bidirectional connections and sync_all() calls.

I have not yet looked closely at the test (I try to avoid large diffs right now in the /qa subfolder as each patch may or may not silently conflict with the python3 refactoring work.)

@jpdffonseca Would you mind to take a look if this can be neatly squashed into the wallet.py test, as suggested by @laanwj ?
@joaopaulofonseca
Contributor

joaopaulofonseca commented Apr 15, 2016

@MarcoFalke and @laanwj yes that makes sense. I will try to squash this verifications into wallet.py.

@joaopaulofonseca joaopaulofonseca changed the title from Add test to RPC listunspent to Add wallet test to check spendable/unspendable UTXO on RPC listunspent Apr 18, 2016

@joaopaulofonseca
Contributor

joaopaulofonseca commented Apr 18, 2016 •
Edited 1 time

@MarcoFalke, after checking that wallet.py already covers some of the tests for listuspent(), I discarded the separate test file and merged the tests regarding the import of bitcoin addresses and private keys into wallet.py. The test execution time should remain the same.

@joaopaulofonseca joaopaulofonseca changed the title from Add wallet test to check spendable/unspendable UTXO on RPC listunspent to Add listunspent test for spendable/unspendable UTXO Apr 18, 2016

@joaopaulofonseca joaopaulofonseca changed the title from Add listunspent test for spendable/unspendable UTXO to Add listunspent() test for spendable/unspendable UTXO Apr 18, 2016

@MarcoFalke
Member

MarcoFalke commented Apr 18, 2016

Thanks. Looks better now.

utACK 3b2eedc

@MarcoFalke MarcoFalke and 2 others commented on an outdated diff Apr 18, 2016

qa/rpc-tests/wallet.py
@@ -6,6 +6,27 @@
from test_framework.test_framework import BitcoinTestFramework
from test_framework.util import *
+def check_array_result(object_array, to_match, expected):
@MarcoFalke

MarcoFalke Apr 18, 2016

Member

If you copied this function from elsewhere, wouldn't it make sense to move it instead to test_framework/util.py, so the utility function can be used in other tests as well?

@joaopaulofonseca

joaopaulofonseca Apr 18, 2016

Contributor

Indeed.. It is used by multiple tests. I think it comes in handy for current and future test cases. If you guys agree I guess this could be moved to test_framework/util.py to be reused whenever needed..

@MarcoFalke

MarcoFalke Apr 18, 2016

Member

Sure, go ahead.

@laanwj

laanwj Apr 19, 2016

Owner

Do you intend to do so in this pull? If so I'll wait with merging.

@joaopaulofonseca

joaopaulofonseca Apr 19, 2016

Contributor

Yes, since we are on this topic.
@joaopaulofonseca
Contributor

joaopaulofonseca commented Apr 19, 2016

Moved assert_array_result() to test_framework/util.py.

I noticed that the previous method on receivedby.py had a bug when using the flag should_not_find.
If the flag is set to True, the validation wouldn't work correctly when the values passed as to_match are indeed found inside object_array, and no assertion is raised.
@laanwj
Owner

laanwj commented Apr 19, 2016

utACK 5d217de

@laanwj laanwj merged commit 5d217de into bitcoin:master Apr 19, 2016
1 check passed

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Apr 19, 2016

@laanwj
Merge #7822: Add listunspent() test for spendable/unspendable UTXO 
5d217de Add test to check spendable and unspendable UTXO on RPC listunspent (Joao Fonseca)
fa942c7 Move method to check matches within arrays on util.py (Joao Fonseca)
This commit was signed with a verified signature.
@laanwj laanwj Wladimir J. van der Laan
GPG key ID: 74810B012346C9A6 Learn about signing commits
0e6fd5e

@fixe fixe deleted the uphold:support/add-test-listunspent branch Apr 19, 2016

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Apr 19, 2016

@joaopaulofonseca @MarcoFalke
Add listunspent() test for spendable/unspendable UTXO 
Github-Pull: #7822
Rebased-From: fa942c7 5d217de
6862627

zander added a commit to zander/bitcoinclassic that referenced this pull request Jun 16, 2016

@joaopaulofonseca @zander
Add listunspent() test for spendable/unspendable UTXO 
Github-Pull: #7822
Rebased-From: fa942c7 5d217de
3d92bdd

nomnombtc added a commit to nomnombtc/bitcoin that referenced this pull request Nov 11, 2016

@joaopaulofonseca @nomnombtc
Add listunspent() test for spendable/unspendable UTXO 
nomnombtc: kept the part where it touches bip125 check in
listtransactions.py commented out.

Github-Pull: #7822
Rebased-From: fa942c7 5d217de
5c020a5

nomnombtc added a commit to nomnombtc/bitcoin that referenced this pull request Nov 12, 2016

@joaopaulofonseca @nomnombtc
Add listunspent() test for spendable/unspendable UTXO 
nomnombtc: kept the part where it touches bip125 check in
listtransactions.py commented out.

Github-Pull: #7822
Rebased-From: fa942c7 5d217de
47fd3b6

nomnombtc added a commit to nomnombtc/bitcoin that referenced this pull request Nov 13, 2016

@joaopaulofonseca @nomnombtc
Add listunspent() test for spendable/unspendable UTXO 
nomnombtc: kept the part where it touches bip125 check in
listtransactions.py commented out.

Github-Pull: #7822
Rebased-From: fa942c7 5d217de
9811199

sickpig added a commit to sickpig/BitcoinUnlimited that referenced this pull request Nov 14, 2016

@joaopaulofonseca @sickpig
Add listunspent() test for spendable/unspendable UTXO 
Github-Pull: #7822
Rebased-From: fa942c7 5d217de
This commit was signed with a verified signature.
@sickpig sickpig Andrea Suisani
GPG key ID: A9CA1D0EE3789741 Learn about signing commits
f5f418a

@dagurval dagurval referenced this pull request in bitcoinxt/bitcoinxt Sep 2, 2017

Merged

keypool test bump + detach wallet from miner #258

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