QA: segwit.py: s/find_unspent/find_spendable_utxo/ #11869

jtimon · 2017-12-11T22:05:43Z

Separated from #8994
It was found out testing that PR but I think this fix should be done even without #8994 the fix is not necessary by luck. Unless I'm missing something.

sipa · 2017-12-12T05:14:42Z

I don't think it's obvious that a function with that name would only return spendable UTXOs. If there is a use case for it, perhaps make it a boolean argument require_spendable?

promag

I would say to extend the scope to add minimumSumAmount=min_value and maximumCount=1 to listunspent options BUT the result could be an unspendable unspent. I guess listunspent could have a new option to filter server side.

Also, callers assume this call always work, it should throw after the loop?

Agree with @sipa.

jtimon · 2017-12-12T14:31:25Z

Is there any use case where the function is used to return unspendable outputs? By defintion, if they're unspendable they're not part of the utxo, right?
I don't quite get why, but changing the genesis block of regtest (ie using the custom regtest proposed in #8994 ) makes this function return unspendable utxos and thus make segwit.py fail.
Or in other words, I don't quite get why the function never randomly returns an unspendable utxo and fails without #8994.

Perhaps this doesn't make sense without #8994, but I don't see the point in adding new parameters to the function that aren't used for any test.

Also, callers assume this call always work, it should throw after the loop?

Yeah, that makes sense to me.

sipa · 2017-12-12T18:10:22Z

@jtimon Ah, I see the confusion. Spendable here means you have the private keys and scripts necessary to spend them. Watch-only addresses can result in non-spendable UTXOs being listed.

jtimon · 2017-12-14T23:29:16Z

I see. So would it be fine to add the require_spendable argument and default to True?
I still don't see the point until some test actually needs to call it with False, but I don't mind adding it.

maflcko

utACK d01b06991a4e10b2cb95b25bfcddcddc58ed0738. Some style nits.

maflcko · 2017-12-15T20:42:12Z

test/functional/segwit.py

@@ -70,7 +70,7 @@ def getutxo(txid):

 def find_unspent(node, min_value):
    for utxo in node.listunspent():


nit: Can use query_options={"minimumAmount": min_value}

Not sure I understand the suggestion.

listunspent takes named arguments according to the documentation, so you could add the named argument and remove some code from the python script instead.

maflcko · 2017-12-15T20:42:40Z

test/functional/segwit.py

@@ -70,7 +70,7 @@ def getutxo(txid):

 def find_unspent(node, min_value):
    for utxo in node.listunspent():
-        if utxo['amount'] >= min_value:
+        if utxo['amount'] >= min_value and utxo['spendable']:
            return utxo



Should throw for clarity.

Yeah, @promag pointed this out already and I said I would do it. Then I ask a question that nobody answered.

I'd say either add the new arg or rename the function and add the missing throw.

maflcko · 2017-12-15T20:43:30Z

test/functional/segwit.py

@@ -70,7 +70,7 @@ def getutxo(txid):

 def find_unspent(node, min_value):


nit: Rename to find_spendable_output or something.

That would make the PR much more disruptive, is it worth it?
Also, if I add the require_spendable param, I don't think it makes sense to rename it to that.

much more disruptive.

It is used in less than 5 places.

Changing the function name when the functionality changes aims review and prevents silent merge conflicts/bugs.

maflcko · 2017-12-30T05:33:38Z

@jtimon Are you still working on this? Otherwise I will close

jtimon · 2017-12-30T09:05:48Z

@MarcoFalke I'm on vacation and I didn't saw your nits.
Does this have chances to be merge if I add the require_spendable argument and default to True?
IMO I would just do the throw thing and leave it as it is without require_spendable, but as said I don't strongly care, my main goal is making #8994 one commit shorter.

maflcko · 2017-12-30T11:50:45Z

@jtimon No rush. Feel free to revisit next year. ;)

jtimon · 2018-01-09T00:09:05Z

Hopefully fixed all nits.
Either I squash before merge or a ditch the last commit, opinions welcomed. I think I prefer the former but no strong opinion.
Rebased because why not?

promag

Nits, don't mind ignoring them. +1 squash.

promag · 2018-01-09T00:19:17Z

test/functional/segwit.py

            return utxo

+    raise AssertionError("Unspent output higher than %s not found." % min_value)


Nit, equal or higher.
Nit, remove ..

promag · 2018-01-09T00:21:07Z

test/functional/segwit.py

-def find_unspent(node, min_value):
-    for utxo in node.listunspent():
-        if utxo['amount'] >= min_value:
+def find_spendable_output(node, min_value):


Nit, s/output/utxo.

jtimon · 2018-01-09T00:30:52Z

Force pushed to fix newest nits.

EDIT: and squashed too.

maflcko · 2018-01-10T23:57:58Z

utACK 305c345cc16c21160f07459156b36bd71e86395e

maflcko · 2018-01-25T15:33:08Z

Needs rebase

jtimon · 2018-01-31T21:45:21Z

Rebased

maflcko · 2018-01-31T21:51:32Z

re-utACK 9bb59cf

jnewbery · 2018-01-31T22:08:26Z

Tested ACK 9bb59cf

9bb59cf QA: segwit.py: s/find_unspent/find_spendable_utxo/ (Jorge Timón) Pull request description: Separated from #8994 It was found out testing that PR but I think this fix should be done even without #8994 the fix is not necessary by luck. Unless I'm missing something. Tree-SHA512: ba1a970e674878ae2f27ee8bba372fa3b42dafff682374f50e5ddc9896fd8808fef2b2b70c7028f2a7b0dbea9f3e29a757e2cde98970f5761bf414455ba02c09

fanquake added the Tests label Dec 11, 2017

promag reviewed Dec 12, 2017

View reviewed changes

maflcko reviewed Dec 15, 2017

View reviewed changes

jtimon force-pushed the b16-segwit-test-fix branch from d01b069 to 528e39c Compare January 8, 2018 23:48

jtimon force-pushed the b16-segwit-test-fix branch from 538a07f to 7a80979 Compare January 9, 2018 00:12

promag reviewed Jan 9, 2018

View reviewed changes

jtimon force-pushed the b16-segwit-test-fix branch from 7a80979 to 31260af Compare January 9, 2018 00:30

jtimon force-pushed the b16-segwit-test-fix branch 2 times, most recently from ca2a458 to 305c345 Compare January 9, 2018 00:35

jtimon mentioned this pull request Jan 9, 2018

Testchains: Introduce custom chain whose constructor... #8994

Closed

12 tasks

jtimon changed the title ~~QA: segwit.py: find_unspent() only return utxo if utxo['spendable']~~ QA: segwit.py: s/find_unspent/find_spendable_utxo/ Jan 9, 2018

ajtowns mentioned this pull request Jan 16, 2018

[tests] Rename functional tests #11774

Merged

QA: segwit.py: s/find_unspent/find_spendable_utxo/

9bb59cf

jtimon force-pushed the b16-segwit-test-fix branch from 305c345 to 9bb59cf Compare January 31, 2018 21:40

maflcko merged commit 9bb59cf into bitcoin:master Feb 2, 2018

jtimon deleted the b16-segwit-test-fix branch February 8, 2018 21:01

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

		@@ -70,7 +70,7 @@ def getutxo(txid):

		def find_unspent(node, min_value):
		for utxo in node.listunspent():

		return utxo

		raise AssertionError("Unspent output higher than %s not found." % min_value)

QA: segwit.py: s/find_unspent/find_spendable_utxo/ #11869

QA: segwit.py: s/find_unspent/find_spendable_utxo/ #11869

Uh oh!

Conversation

jtimon commented Dec 11, 2017

Uh oh!

sipa commented Dec 12, 2017

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

jtimon commented Dec 12, 2017

Uh oh!

sipa commented Dec 12, 2017

Uh oh!

jtimon commented Dec 14, 2017

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Dec 30, 2017

Uh oh!

jtimon commented Dec 30, 2017

Uh oh!

maflcko commented Dec 30, 2017

Uh oh!

jtimon commented Jan 9, 2018

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtimon commented Jan 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Jan 10, 2018

Uh oh!

maflcko commented Jan 25, 2018

Uh oh!

jtimon commented Jan 31, 2018

Uh oh!

maflcko commented Jan 31, 2018

Uh oh!

jnewbery commented Jan 31, 2018

Uh oh!

Uh oh!

jtimon commented Jan 9, 2018 •

edited

Loading