QA: segwit.py: s/find_unspent/find_spendable

jtimon commented at 10:05 pm on December 11, 2017: contributor

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.

fanquake added the label Tests on Dec 11, 2017

sipa commented at 5:14 am on December 12, 2017: member

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 commented at 11:39 am on December 12, 2017: member

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 commented at 2:31 pm on December 12, 2017: contributor

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 commented at 6:10 pm on December 12, 2017: member

@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 commented at 11:29 pm on December 14, 2017: contributor

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.

in test/functional/segwit.py:72 in d01b06991a outdated

69@@ -70,7 +70,7 @@ def getutxo(txid):
70 
71 def find_unspent(node, min_value):
72     for utxo in node.listunspent():

MarcoFalke commented at 8:42 pm on December 15, 2017:

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

jtimon commented at 9:00 am on December 30, 2017:

Not sure I understand the suggestion.

MarcoFalke commented at 11:49 am on December 30, 2017:

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

in test/functional/segwit.py:75 in d01b06991a outdated

69@@ -70,7 +70,7 @@ def getutxo(txid):
70 
71 def find_unspent(node, min_value):
72     for utxo in node.listunspent():
73-        if utxo['amount'] >= min_value:
74+        if utxo['amount'] >= min_value and utxo['spendable']:
75             return utxo
76

MarcoFalke commented at 8:42 pm on December 15, 2017:

Should throw for clarity.

jtimon commented at 9:01 am on December 30, 2017:

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

promag commented at 9:05 am on December 30, 2017:

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

in test/functional/segwit.py:71 in d01b06991a outdated

69@@ -70,7 +70,7 @@ def getutxo(txid):
70 
71 def find_unspent(node, min_value):

MarcoFalke commented at 8:43 pm on December 15, 2017:

nit: Rename to find_spendable_output or something.

jtimon commented at 9:00 am on December 30, 2017:

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.

MarcoFalke commented at 11:48 am on December 30, 2017:

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.

MarcoFalke commented at 8:45 pm on December 15, 2017: member

utACK d01b06991a4e10b2cb95b25bfcddcddc58ed0738. Some style nits.

fanquake added this to the "In progress" column in a project

MarcoFalke commented at 5:33 am on December 30, 2017: member

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

jtimon commented at 9:05 am on December 30, 2017: contributor

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

MarcoFalke commented at 11:50 am on December 30, 2017: member

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

jtimon force-pushed on Jan 8, 2018

jtimon commented at 0:09 am on January 9, 2018: contributor

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?

jtimon force-pushed on Jan 9, 2018

in test/functional/segwit.py:76 in 7a80979eea outdated

74+def find_spendable_output(node, min_value):
75+    for utxo in node.listunspent(query_options={'minimumAmount': min_value}):
76+        if utxo['spendable']:
77             return utxo
78 
79+    raise AssertionError("Unspent output higher than %s not found." % min_value)

promag commented at 0:19 am on January 9, 2018:

Nit, equal or higher. Nit, remove ..

in test/functional/segwit.py:71 in 7a80979eea outdated

67@@ -68,11 +68,13 @@ def getutxo(txid):
68     utxo["txid"] = txid
69     return utxo
70 
71-def find_unspent(node, min_value):
72-    for utxo in node.listunspent():
73-        if utxo['amount'] >= min_value:
74+def find_spendable_output(node, min_value):

promag commented at 0:21 am on January 9, 2018:

Nit, s/output/utxo.

promag commented at 0:22 am on January 9, 2018: member

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

jtimon force-pushed on Jan 9, 2018

jtimon commented at 0:30 am on January 9, 2018: contributor

Force pushed to fix newest nits.

EDIT: and squashed too.

jtimon force-pushed on Jan 9, 2018

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

MarcoFalke commented at 11:57 pm on January 10, 2018: member

utACK 305c345cc16c21160f07459156b36bd71e86395e

MarcoFalke commented at 3:33 pm on January 25, 2018: member

Needs rebase

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

jtimon force-pushed on Jan 31, 2018

jtimon commented at 9:45 pm on January 31, 2018: contributor

Rebased

MarcoFalke commented at 9:51 pm on January 31, 2018: member

re-utACK 9bb59cf

jnewbery commented at 10:08 pm on January 31, 2018: member

Tested ACK 9bb59cf7baea0b33d97ef0c0eeee88a8b104be3a

MarcoFalke merged this on Feb 2, 2018

MarcoFalke closed this on Feb 2, 2018

MarcoFalke referenced this in commit 4cad91663d on Feb 2, 2018

jtimon deleted the branch on Feb 8, 2018

fanquake moved this from the "In progress" to the "Done" column in a project

DrahtBot locked this on Sep 8, 2021

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