Fix unlocking of an already spent output #12739

pull pedrobranco wants to merge 1 commits into bitcoin:master from uphold:bugfix/fix-lockunspent-deadlock changing 2 files +13 −3

pedrobranco commented at 6:53 PM on March 20, 2018: contributor

Fixes #12738.
promag commented at 6:57 PM on March 20, 2018: member

IMO when the UTXO is spent it should be considered unlocked no?
pedrobranco commented at 7:11 PM on March 20, 2018: contributor

IMO when the UTXO is spent it should be considered unlocked no?

Agree. Unlocking the unspent when relaying the transaction that spends it is preferable, but is a different scope of this PR since that approach is a breaking change.
promag commented at 9:50 PM on March 20, 2018: member

Not sure what you mean with breaking change.
fanquake added the label Wallet on Mar 20, 2018

in test/functional/wallet_basic.py:162 in 972d56970a outdated

 157 | @@ -158,6 +158,9 @@ def run_test(self):
 158 |          spent_0 = {"txid": node0utxos[0]["txid"], "vout": node0utxos[0]["vout"]}
 159 |          assert_raises_rpc_error(-8, "Invalid parameter, expected unspent output", self.nodes[0].lockunspent, False, [spent_0])
 160 |  
 161 | +        # Verify that a spent output can be unlocked
 162 | +        assert_equal(self.nodes[0].lockunspent, True, [spent_0], True)

Empact commented at 7:52 AM on March 21, 2018:

Seems like this is an improper use of assert_equal - basically it's meant to be use with arg1 == all other args, so the two Trues are redundant or mistaken.

https://github.com/bitcoin/bitcoin/blob/master/test/functional/test_framework/util.py#L36

pedrobranco force-pushed on Mar 21, 2018
pedrobranco commented at 10:40 AM on March 21, 2018: contributor

Not sure what you mean with breaking change.

If we unlock the output when sending the raw transaction, calling lockunspent true <unspents> will fail because they are already unlocked. It is not a breaking change for bitcoind, but for the ones that uses the same flow as the example here.
pedrobranco force-pushed on Mar 21, 2018
Fix lockunspent deadlock when unlocking already spent outputs b64bf76c71
pedrobranco force-pushed on Mar 21, 2018
luke-jr commented at 4:54 PM on June 12, 2018: member

lockunspent probably should throw an error if the referenced TXOs aren't unspent...
laanwj commented at 4:53 PM on June 24, 2018: member

Closing because #13160 was merged.
laanwj closed this on Jun 24, 2018
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#12738 Cannot unlock already spent output