No need to chain:

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

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

...

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

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

Ok, makes sense!

I think is must be either ISMINE_SPENDABLE or ISMINE_WATCH_SOLVABLE?

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.

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

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

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

Assert complete error message? cc @jnewbery

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.

Same as above.

Same as above.

Same as above.

Same as above.

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?

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.

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

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

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

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.

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.

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();

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

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

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

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

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

Needs rebase and replace with "assert_raises_rpc_error"

Could declare as const UniValue& to avoid copying params.

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

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