Diagnose unsuitable outputs in lockunspent(). #11087

pull BitonicEelis wants to merge 1 commits into bitcoin:master from BitonicEelis:lockunspent changing 2 files +60 −15
  1. BitonicEelis commented at 1:23 PM on August 18, 2017: contributor

    Fixes #2667.

    This is a simplified version of pull request #3574, which was abandoned by its author.

    I added some tests as well.

  2. in src/wallet/rpcwallet.cpp:2402 in fae526df06 outdated 
    2398 | @@ -2397,6 +2399,40 @@ UniValue lockunspent(const JSONRPCRequest& request)
2399 |  
2400 |          COutPoint outpt(uint256S(txid), nOutput);
2401 |  
2402 | +        const bool isLocked = pwallet->IsLockedCoin(outpt.hash, outpt.n);
    promag commented at 1:53 PM on August 18, 2017: 
    bool is_locked = ...;
  3. in src/wallet/rpcwallet.cpp:2404 in fae526df06 outdated 
    2398 | @@ -2397,6 +2399,40 @@ UniValue lockunspent(const JSONRPCRequest& request)
2399 |  
2400 |          COutPoint outpt(uint256S(txid), nOutput);
2401 |  
2402 | +        const bool isLocked = pwallet->IsLockedCoin(outpt.hash, outpt.n);
2403 | +
2404 | +        if (fUnlock)
    promag commented at 1:59 PM on August 18, 2017:

    No need to chain:

    if (fUnlock && !is_locked) {
    throw ...;
}

if (!fUnlock && is_locked) {
    throw ...;
}

...
    BitonicEelis commented at 2:43 PM on August 18, 2017:

    Thanks for the review! Are you absolutely sure that attempting to lock an already locked output should be an error?

    promag commented at 2:46 PM on August 18, 2017:

    IMO yes, it indicates it was locked by other client and as such it is not yours to use?

    BitonicEelis commented at 2:58 PM on August 18, 2017:

    Ok, makes sense!

  4. in src/wallet/rpcwallet.cpp:2423 in fae526df06 outdated 
    2418 | +            const CWalletTx& trans = it->second;
2419 | +
2420 | +            if (outpt.n >= trans.tx->vout.size())
2421 | +                throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, vout index out of bounds");
2422 | +
2423 | +            if (pwallet->IsMine(trans.tx->vout[outpt.n]) == ISMINE_NO)
    promag commented at 2:03 PM on August 18, 2017:

    I think is must be either ISMINE_SPENDABLE or ISMINE_WATCH_SOLVABLE?

    promag commented at 2:33 PM on August 18, 2017:

    Do we really have to test IsMine and all? In the wallet it's just a std::set<COutPoint>, so IMO is enough to check:

    • if the input is valid output
    • if the lock state is compatible with the argument.

    But if there is interest in doing these tests, then perform the cheapest first.

    BitonicEelis commented at 2:58 PM on August 18, 2017:

    I'll remove the check for now. It can always be added later.

  5. promag commented at 2:17 PM on August 18, 2017: member

    Read developer notes and update accordingly.

  6. laanwj added the label RPC/REST/ZMQ on Aug 18, 2017
  7. BitonicEelis force-pushed on Aug 18, 2017
  8. BitonicEelis commented at 2:59 PM on August 18, 2017: contributor

    Amended to:

    • comply with variable naming policy
    • reject attempts to lock already locked outputs
    • remove MINE check
  9. in src/wallet/rpcwallet.cpp:2404 in 764ab3a6d9 outdated 
    2398 | @@ -2397,6 +2399,38 @@ UniValue lockunspent(const JSONRPCRequest& request)
2399 |  
2400 |          COutPoint outpt(uint256S(txid), nOutput);
2401 |  
2402 | +        const bool is_locked = pwallet->IsLockedCoin(outpt.hash, outpt.n);
2403 | +
2404 | +        if (fUnlock && !is_locked)
    promag commented at 3:07 PM on August 18, 2017:

    Missing {} and { is one the same line. Same below. See developer notes.

    BitonicEelis commented at 3:14 PM on August 18, 2017:

    Will do! Thanks for caring about consistent style. :)

  10. BitonicEelis force-pushed on Aug 18, 2017
  11. BitonicEelis commented at 3:15 PM on August 18, 2017: contributor

    Amended to use curly brackets in accordance with developer notes.

  14. in test/functional/wallet.py:96 in 918b6db42b outdated 
      92 | @@ -93,11 +93,17 @@ def run_test(self):
  93 |          # Exercise locking of unspent outputs
  94 |          unspent_0 = self.nodes[2].listunspent()[0]
  95 |          unspent_0 = {"txid": unspent_0["txid"], "vout": unspent_0["vout"]}
  96 | +        assert_raises_jsonrpc(-8, "expected locked output", self.nodes[2].lockunspent, True, [unspent_0])
    promag commented at 11:51 PM on August 22, 2017:

    Assert complete error message? cc @jnewbery

    jnewbery commented at 4:26 PM on August 24, 2017:

    Personally, I don't think the complete error message needs to be asserted, as long as the string is long enough to disambiguate. But also fine to change this to the complete string.

  15. in test/functional/wallet.py:98 in 918b6db42b outdated 
      92 | @@ -93,11 +93,17 @@ def run_test(self):
  93 |          # Exercise locking of unspent outputs
  94 |          unspent_0 = self.nodes[2].listunspent()[0]
  95 |          unspent_0 = {"txid": unspent_0["txid"], "vout": unspent_0["vout"]}
  96 | +        assert_raises_jsonrpc(-8, "expected locked output", self.nodes[2].lockunspent, True, [unspent_0])
  97 |          self.nodes[2].lockunspent(False, [unspent_0])
  98 | +        assert_raises_jsonrpc(-8, "output already locked", self.nodes[2].lockunspent, False, [unspent_0])
    promag commented at 11:51 PM on August 22, 2017:

    Same as above.

  16. in test/functional/wallet.py:103 in 918b6db42b outdated 
      98 | +        assert_raises_jsonrpc(-8, "output already locked", self.nodes[2].lockunspent, False, [unspent_0])
  99 |          assert_raises_jsonrpc(-4, "Insufficient funds", self.nodes[2].sendtoaddress, self.nodes[2].getnewaddress(), 20)
 100 |          assert_equal([unspent_0], self.nodes[2].listlockunspent())
 101 |          self.nodes[2].lockunspent(True, [unspent_0])
 102 |          assert_equal(len(self.nodes[2].listlockunspent()), 0)
 103 | +        assert_raises_jsonrpc(-8, "unknown transaction",
    promag commented at 11:52 PM on August 22, 2017:

    Same as above.

  17. in test/functional/wallet.py:105 in 918b6db42b outdated 
     100 |          assert_equal([unspent_0], self.nodes[2].listlockunspent())
 101 |          self.nodes[2].lockunspent(True, [unspent_0])
 102 |          assert_equal(len(self.nodes[2].listlockunspent()), 0)
 103 | +        assert_raises_jsonrpc(-8, "unknown transaction",
 104 | +          self.nodes[2].lockunspent, False, [{"txid": "0000000000000000000000000000000000", "vout": 0}])
 105 | +        assert_raises_jsonrpc(-8, "vout index out of bounds",
    promag commented at 11:52 PM on August 22, 2017:

    Same as above.

  18. in test/functional/wallet.py:147 in 918b6db42b outdated 
     141 | @@ -136,6 +142,10 @@ def run_test(self):
 142 |          assert_equal(self.nodes[2].getbalance(), 94)
 143 |          assert_equal(self.nodes[2].getbalance("from1"), 94-21)
 144 |  
 145 | +        # Verify that a spent output cannot be locked anymore
 146 | +        spent_0 = {"txid" : node0utxos[0]["txid"], "vout" : node0utxos[0]["vout"]}
 147 | +        assert_raises_jsonrpc(-8, "expected unspent output", self.nodes[2].lockunspent, False, [spent_0])
    promag commented at 11:52 PM on August 22, 2017:

    Same as above.

  19. ryanofsky commented at 3:23 PM on August 25, 2017: member

    utACK 918b6db42b12d86a01bdb70a4fc50244aa2ce623

    You could modernize the code a little more: adding a range for loop instead of an index loop, and using UniValue directly instead of adding adding new find_value calls. Also I'm not sure if it's useful to make locking an already locked output or unlocked an already unlocked one errors. But overall, this change seems like an improvement and looks good to merge.

  20. in src/wallet/rpcwallet.cpp:2413 in 918b6db42b outdated 
    2408 | +        if (!fUnlock) {
2409 | +            if (is_locked) {
2410 | +                throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, output already locked");
2411 | +            }
2412 | +
2413 | +            if (pwallet->IsSpent(outpt.hash, outpt.n)) {
    jnewbery commented at 4:18 PM on August 25, 2017:

    Why are these three checks ("expected unspent", "unknown transaction" and "vout index out of bounds") only done when trying to lock? Isn't it equally invalid to try to unlock bad txouts? Can you place these three checks above the const bool is_locked = pwallet->IsLockedCoin(outpt.hash, outpt.n); line?

    Eelis commented at 9:57 AM on August 26, 2017:

    The idea was that in all three of those cases (already spent output, unknown transaction, and vout index out of bounds), IsLockedCoin should already have returned false, so checking !is_locked should be enough.

    jnewbery commented at 3:07 PM on August 28, 2017:

    I agree that the function will correctly return an error, but I think that it's better to return the more precise error if possible. expected unspent output, unknown transaction and vout index out of bounds are better to return than expected locked output (the last could infer that the outpoint exists but is unlocked).

    Eelis commented at 10:27 PM on August 28, 2017:

    Makes sense, I'll make the errors more precise.

  21. in src/wallet/rpcwallet.cpp:2431 in 918b6db42b outdated 
    2426 | +            }
2427 | +        }
2428 | +    }
2429 | +
2430 | +    // Atomically set (un)locked status for the outputs.
2431 | +    for (unsigned int idx = 0; idx < outputs.size(); idx++) {
    jnewbery commented at 4:19 PM on August 25, 2017:

    You could save reparsing the json by saving a vector of COutPoints in the first pass.

    Eelis commented at 9:41 PM on August 25, 2017:

    These loops don't do any json parsing, because the data is already in a UniValue. The find_value calls find the desired value in UniValue's internal std::vector of values.

    jnewbery commented at 3:04 PM on August 28, 2017:

    You're right. There's no reparsing the JSON, just rereading the string and reconstructing the COutPoint.

    I think either way is fine. Whichever you prefer.

  22. jnewbery commented at 4:24 PM on August 25, 2017: member

    Looks good, and great test coverage. A couple of nits inline.

    Maybe out of the scope of this PR, but this RPC returns true if called with false but no list of transactions. A user might expect that with that call all outputs have been locked, but in fact it's a no-op. I think that if (request.params.size() == 1 && !fUnlock) then we should return false or throw.

  23. BitonicEelis force-pushed on Aug 29, 2017
  24. BitonicEelis commented at 8:45 AM on August 29, 2017: contributor

    Amended to:

    • store COutPoints in vector instead of recomputing them
    • include "invalid parameter:" error prefix in expectations in test
    • make unlock errors more precise @ryanofsky Unfortunately, range-for cannot be used to iterate over a UniValue because it does not provide begin()/end().

    Thanks for the feedback!

  25. in src/wallet/rpcwallet.cpp:2402 in 82937c0421 outdated 
    2401 | +    outputs.reserve(output_params.size());
2402 | +
2403 | +    for (unsigned int idx = 0; idx < output_params.size(); idx++) {
2404 | +         const UniValue& param = output_params[idx];
2405 | +
2406 | +        if (!param.isObject()) {
    jnewbery commented at 6:36 PM on August 31, 2017:

    You can remove this check entirely. Type checking is done by the UniValue.get_xxx() functions, and returns almost an identical error to that given by this explicit check.

    With the explicit check:

    → bitcoin-cli -rpcwallet=w1 lockunspent true [\"string\"]
error code: -8
error message:
Invalid parameter, expected

    without:

    → bitcoin-cli -rpcwallet=w1 lockunspent true [\"string\"]
error code: -1
error message:
JSON value is not an object as expected

    The implicit get_xxx() check is used extensively in other RPCs.

    You can also combine the line below with above:

    const UniValue& o = request.params[1].get_obj();
  26. in src/wallet/rpcwallet.cpp:2460 in 82937c0421 outdated 
    2408 | -        const UniValue& o = output.get_obj();
2409 | +        }
2410 | +
2411 | +        const UniValue& o = param.get_obj();
2412 |  
2413 |          RPCTypeCheckObj(o,
    jnewbery commented at 6:37 PM on August 31, 2017:

    In contrast to my comment above, it makes sense to keep these. RPCTypeCheckObj provides slightly more information (both the type expected and type received)

  27. in test/functional/wallet.py:155 in 82937c0421 outdated 
     149 | @@ -144,6 +150,10 @@ def run_test(self):
 150 |          assert_equal(self.nodes[2].getbalance(), 94)
 151 |          assert_equal(self.nodes[2].getbalance("from1"), 94-21)
 152 |  
 153 | +        # Verify that a spent output cannot be locked anymore
 154 | +        spent_0 = {"txid": node0utxos[0]["txid"], "vout": node0utxos[0]["vout"]}
 155 | +        assert_raises_jsonrpc(-8, "Invalid parameter, expected unspent output", self.nodes[2].lockunspent, False, [spent_0])
    jnewbery commented at 6:38 PM on August 31, 2017:

    This is the bug. lockunspent() should be called on self.nodes[0], not self.nodes[2]

    BitonicEelis commented at 9:11 AM on September 1, 2017:

    Thanks! This one stumped me because for some reason when I ran the wallet tests locally, they didn't fail..

    jnewbery commented at 12:33 PM on September 1, 2017:

    Yes! I saw the same thing. Very odd. h/t @sdaftuar for spotting the bug.

  28. jnewbery commented at 6:38 PM on August 31, 2017: member

    Looks great! One bug in the test code and one nit in the RPC method.

  29. in test/functional/wallet.py:112 in 82937c0421 outdated 
     107 |          assert_raises_jsonrpc(-4, "Insufficient funds", self.nodes[2].sendtoaddress, self.nodes[2].getnewaddress(), 20)
 108 |          assert_equal([unspent_0], self.nodes[2].listlockunspent())
 109 |          self.nodes[2].lockunspent(True, [unspent_0])
 110 |          assert_equal(len(self.nodes[2].listlockunspent()), 0)
 111 | +        assert_raises_jsonrpc(-8, "Invalid parameter, unknown transaction",
 112 | +          self.nodes[2].lockunspent, False, [{"txid": "0000000000000000000000000000000000", "vout": 0}])
    jnewbery commented at 6:49 PM on August 31, 2017:

    I personally don't like this indentation style. It looks like something between a new line and a new code block.

    Would you mind aligning this continuation line with the opening parens:

        assert_raises_jsonrpc(-8, "Invalid parameter, unknown transaction",
                          self.nodes[2].lockunspent, False, [{"txid": "0000000000000000000000000000000000", "vout": 0}])

    same below

  30. BitonicEelis force-pushed on Sep 1, 2017
  31. BitonicEelis commented at 9:36 AM on September 1, 2017: contributor

    Amended to:

    • remove isObject check
    • indent continued function call lines to match opening parenthesis
    • call lockunspent on correct node in test
  32. jnewbery commented at 1:27 PM on September 1, 2017: member

    Tested ACK fe2c95bda22c21252709cc1088d7e61a8c8b1df8. Looks great. Thanks for being receptive to all my feedback!

  33. in test/functional/wallet.py:157 in fe2c95bda2 outdated 
     151 | @@ -144,6 +152,10 @@ def run_test(self):
 152 |          assert_equal(self.nodes[2].getbalance(), 94)
 153 |          assert_equal(self.nodes[2].getbalance("from1"), 94-21)
 154 |  
 155 | +        # Verify that a spent output cannot be locked anymore
 156 | +        spent_0 = {"txid": node0utxos[0]["txid"], "vout": node0utxos[0]["vout"]}
 157 | +        assert_raises_jsonrpc(-8, "Invalid parameter, expected unspent output", self.nodes[0].lockunspent, False, [spent_0])
    MarcoFalke commented at 7:10 PM on October 9, 2017:

    Needs rebase and replace with "assert_raises_rpc_error"

  34. in src/wallet/rpcwallet.cpp:2392 in fe2c95bda2 outdated 
    2393 | -    for (unsigned int idx = 0; idx < outputs.size(); idx++) {
2394 | -        const UniValue& output = outputs[idx];
2395 | -        if (!output.isObject())
2396 | -            throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, expected object");
2397 | -        const UniValue& o = output.get_obj();
2398 | +    const UniValue output_params = request.params[1];
    ryanofsky commented at 8:58 PM on October 10, 2017:

    Could declare as const UniValue& to avoid copying params.

  35. in src/wallet/rpcwallet.cpp:2408 in fe2c95bda2 outdated 
    2411 |                  {"vout", UniValueType(UniValue::VNUM)},
2412 |              });
2413 |  
2414 | -        std::string txid = find_value(o, "txid").get_str();
2415 | -        if (!IsHex(txid))
2416 | +        const std::string txid = find_value(o, "txid").get_str();
    ryanofsky commented at 9:02 PM on October 10, 2017:

    Could declare as const std::string& to avoid copying string.

  36. ryanofsky commented at 9:10 PM on October 10, 2017: member

    utACK fe2c95bda22c21252709cc1088d7e61a8c8b1df8. No significant changes since last review. Left minor comments (feel free to ignore them), but this does need to be rebased.

  37. Diagnose unsuitable outputs in lockunspent().
    Fixes #2667.
    28f8b66577
  38. BitonicEelis force-pushed on Oct 12, 2017
  39. BitonicEelis commented at 2:04 PM on October 12, 2017: contributor

    Amended/rebased to:

    • use assert_raises_rpc_error;
    • use references to avoid copying a UniValue and std::string, as suggested by ryanofsky.
  40. ryanofsky commented at 7:53 PM on October 12, 2017: member

    utACK 28f8b6657764c7746645a6e75dfb09ffc0597322. Only changes since last review are rebase & adding const refs.

  41. in src/wallet/rpcwallet.cpp:2508 in 28f8b66577 
    2518 | +        outputs.push_back(outpt);
2519 | +    }
2520 | +
2521 | +    // Atomically set (un)locked status for the outputs.
2522 | +    for (const COutPoint& outpt : outputs) {
2523 | +        if (fUnlock) pwallet->UnlockCoin(outpt);
    sipa commented at 11:45 PM on October 12, 2017:

    Coding style nit; if with else always needs braces/indentation (I know the original code you're rewriting wasn't following this convention, but in new code we try to).

  42. sipa commented at 11:51 PM on October 12, 2017: member

    utACK 28f8b6657764c7746645a6e75dfb09ffc0597322

  43. laanwj merged this on Nov 16, 2017
  44. laanwj closed this on Nov 16, 2017
  45. laanwj referenced this in commit 99bc0b428b on Nov 16, 2017
  46. BitonicEelis deleted the branch on Nov 16, 2017
  47. random-zebra referenced this in commit 328bad7a40 on Oct 28, 2019
  48. random-zebra referenced this in commit 8b4b6d3429 on Nov 1, 2019
  49. random-zebra referenced this in commit 5e4114f964 on Nov 4, 2019
  50. random-zebra referenced this in commit 19a116da1a on Nov 8, 2019
  51. furszy referenced this in commit bfbbb6b30c on Nov 19, 2019
  52. akshaynexus referenced this in commit 39f198de27 on Nov 19, 2019
  53. CaveSpectre11 referenced this in commit cada16967d on Dec 14, 2019
  54. PastaPastaPasta referenced this in commit 1eb2d52c3a on Jan 17, 2020
  55. PastaPastaPasta referenced this in commit 78046932d9 on Jan 22, 2020
  56. PastaPastaPasta referenced this in commit 37713b81c5 on Jan 22, 2020
  57. PastaPastaPasta referenced this in commit 63da304fb7 on Jan 29, 2020
  58. PastaPastaPasta referenced this in commit 4433160703 on Jan 29, 2020
  59. PastaPastaPasta referenced this in commit a76a3b72db on Jan 29, 2020
  60. wqking referenced this in commit c3a724e321 on May 31, 2020
  61. KolbyML referenced this in commit cfacd6f5e9 on Nov 21, 2020
  62. ckti referenced this in commit 1c5265e83a on Mar 28, 2021
  63. DrahtBot locked this on Sep 8, 2021
Contributors
BitonicEelispromagmeshcolliderlaanwjjnewberyryanofskyEelisMarcoFalkesipa
Labels
RPC/REST/ZMQ
Linked (view graph)
#2667 lockunspent returns true even for non-existent outputs#3574 Fix for lockunspent returning true even for non-existent outputs#12738 Cannot unlock already spent output
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-13 15:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me