Fix for lockunspent returning true even for non-existent outputs #3574

pull aalness wants to merge 1 commits into bitcoin:master from aalness:lockunspent changing 3 files +72 −14
  1. aalness commented at 11:50 PM on January 22, 2014: contributor

    lockunspent will now validate all of the provided outputs are both unspent and belong to the local wallet.

    The operation is now also atomic.

    Fix for #2667

  2. laanwj commented at 8:23 AM on January 23, 2014: member

    @aalness Removed the offtopic replies here, sorry for continuing them. I was lost in a maze of twisty github issues all alike...

    Edit: I also took the liberty to clarify your pull request title somewhat.

  3. in src/rpcwallet.cpp:None in 4420f91015 outdated 
    1792 | @@ -1791,7 +1793,20 @@ Value lockunspent(const Array& params, bool fHelp)
1793 |              throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, vout must be positive");
1794 |  
1795 |          COutPoint outpt(uint256(txid), nOutput);
1796 | +        if (!pwalletMain->IsLockableCoin(outpt)) {
1797 | +            throw JSONRPCError(RPC_INVALID_PARAMETER,
1798 | +                               "Invalid parameter, expected unspent output belonging to this wallet");
    Diapolo commented at 8:42 AM on January 23, 2014:

    Can you use 4-space indentation here also below throw please.

  4. Diapolo commented at 8:40 PM on January 23, 2014: none

    Does this change require any changes in the GUI coin-control code or do we directly benefit from this change?

  5. aalness commented at 8:55 PM on January 23, 2014: contributor

    Good question. At first read, it looks like the coin-control GUI could theoretically by-pass this new check and lock an unlockable coin. This could be confusing to an RPC user. eg. GUI user locks an unlockable coin, RPC user sees it in the output for listlockunspent and when they try to unlock it via lockunspent it fails due to the coin being unlockable.

  6. laanwj commented at 9:17 AM on January 24, 2014: member

    But does the GUI allow it? I think it's pretty locked down in that regard, showing only coins that can be spent (and thus locked). But I may be wrong.

  7. cozz commented at 5:25 PM on January 24, 2014: contributor

    Havent tested, but I agree with @laanwj. You check 4 conditions (mapwallet output.n !ispent ismine), coin control calls AvailableCoins, checking the same but even more conditions. So it should never be possible to lock an unlockable coin in the GUI.

    I also suggest:

    • The condition when unlocking a coin could simply be, if it is already locked or not. It should always be possible to unlock a coin, if the coin appears in listlockunspent.
    • rename your function IsLockableCoin to IsAvailableCoin. This way the function can also be used in the future for other stuff.
    • make this function IsAvailableCoin very similar to wallet->AvailableCoins, so that it is easy to maintain (I mean the conditions if(!FinalTx...) etc.)
    • put the function below AvailableCoins

    This would be consistent with the GUI, checking the same conditions.

  8. aalness commented at 9:01 PM on January 24, 2014: contributor

    Ah, good feedback @cozz. Will make those changes. Thanks.

  9. Fix for: https://github.com/bitcoin/bitcoin/issues/2667
    lockunspent will now validate all of the provided outputs are
both unspent and belong to the local wallet.

The operation is now also atomic.
    74ad586824
  10. aalness commented at 12:51 AM on January 25, 2014: contributor

    @cozz I incorporated your feedback but note the semantic implication:

    lockunspent lock=true for an already locked coin would now throw an error. lockunspent unlock=true for an already unlocked coin would now throw an error.

    Previously, the behavior was:

    lockunspent lock=true for an already locked coin returns true. lockunspent unlock=true for an already unlocked coin would return true.

    I think a case could be made for either behavior but I want to point it out in case this violates a convention I'm unaware of. Or if it's particularly distasteful to anyone.

  11. cozz commented at 2:42 AM on January 25, 2014: contributor

    @aalness yes, maybe in the case "lockunspent lock=true for an already locked coin", show a different error message like "coin already locked". But throw an error is fine with me.

    Code looks good, not sure what the core devs opinion is on this. So lets wait for another opinion here...

  12. in src/wallet.cpp:None in 74ad586824 
    1084 | +    return IsAvailableCoin(coin, output.hash, output.n, NULL);
1085 | +}
1086 | +
1087 | +bool CWallet::IsAvailableCoin(const CWalletTx& coin, uint256 hash, unsigned int n, const CCoinControl *coinControl) const
1088 | +{
1089 | +    // It's assumed the caller has tested coin.IsAvailable().
    laanwj commented at 11:39 AM on January 25, 2014:

    So now we have coin.IsAvailable as well as CWallet::IsAvailableCoin? I think that's a bit confusing.

  13. cozz commented at 6:25 PM on January 25, 2014: contributor

    @laanwj yes, so maybe not touch AvailableCoins at all. And just introduce 1 new function IsAvailableCoin, duplicating all the necessary logic. Just like IsLockableCoin before. Thats what I meant initially. Would also be easier to review and to compare. What do you think?

  14. aalness commented at 10:35 PM on January 25, 2014: contributor

    @laanwj yeah, that's fair. It bothered me slightly too, although, it seemed to be a bit of an existing convention to call a CWalletTx a "coin" or "pcoin". Do you think it would be less confusing to simply change the variable to "wtx"? Can you think of a better way to describe a transaction in this state, as opposed to "available"? Or would you just prefer that I remove the method altogether? @cozz I do like the property that IsAvailableCoin and AvailableCoins now share the same exact code. It seems like a stronger guarantee that the GUI and listunspent stay consistent with lockunspent.

  15. luke-jr commented at 5:05 PM on February 21, 2014: member

    @aalness Needs rebase

  16. BitcoinPullTester commented at 4:18 PM on June 23, 2014: none

    Automatic sanity-testing: FAILED MERGE, see http://jenkins.bluematt.me/pull-tester/p3574_74ad5868245790c1f6c712c6afaa0d76398d2d7d/ for test log.

    This pull does not merge cleanly onto current master This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

  17. laanwj commented at 2:45 PM on June 24, 2014: member

    Closing this pull as it appears to be inactive. If you intend to resume working on this, let me know.

  18. laanwj closed this on Jun 24, 2014
  19. laanwj referenced this in commit 99bc0b428b on Nov 16, 2017
  20. PastaPastaPasta referenced this in commit 1eb2d52c3a on Jan 17, 2020
  21. PastaPastaPasta referenced this in commit 78046932d9 on Jan 22, 2020
  22. PastaPastaPasta referenced this in commit 37713b81c5 on Jan 22, 2020
  23. PastaPastaPasta referenced this in commit 63da304fb7 on Jan 29, 2020
  24. PastaPastaPasta referenced this in commit 4433160703 on Jan 29, 2020
  25. PastaPastaPasta referenced this in commit a76a3b72db on Jan 29, 2020
  26. DrahtBot locked this on Sep 8, 2021
Contributors
aalnesslaanwjDiapolocozzluke-jrBitcoinPullTester
Linked (view graph)
#2667 lockunspent returns true even for non-existent outputs#11087 Diagnose unsuitable outputs in lockunspent().
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 18:16 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me