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

aalness · 2014-01-22T23:50:39Z

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

laanwj · 2014-01-23T08:23:24Z

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

Diapolo · 2014-01-23T08:42:45Z

src/rpcwallet.cpp

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

Diapolo · 2014-01-23T20:40:55Z

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

aalness · 2014-01-23T20:55:41Z

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.

laanwj · 2014-01-24T09:17:37Z

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.

cozz · 2014-01-24T17:25:41Z

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.

aalness · 2014-01-24T21:01:59Z

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

lockunspent will now validate all of the provided outputs are both unspent and belong to the local wallet. The operation is now also atomic.

aalness · 2014-01-25T00:51:47Z

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

cozz · 2014-01-25T02:42:06Z

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

laanwj · 2014-01-25T11:39:33Z

src/wallet.cpp

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

cozz · 2014-01-25T18:25:12Z

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

aalness · 2014-01-25T22:35:48Z

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

luke-jr · 2014-02-21T17:05:20Z

@aalness Needs rebase

BitcoinPullTester · 2014-06-23T16:18:42Z

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.

laanwj · 2014-06-24T14:45:55Z

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

28f8b66 Diagnose unsuitable outputs in lockunspent(). (Eelis) Pull request description: Fixes #2667. This is a simplified version of pull request #3574, which was abandoned by its author. I added some tests as well. Tree-SHA512: e63e00dec8b1b232079380183805cb0b0b18c78ea6bea769837949aab984689d7f68b2ccfe66b1873517b040b9e616ce0eb058575c3d4382aa8c26eebcf1f14e

28f8b66 Diagnose unsuitable outputs in lockunspent(). (Eelis) Pull request description: Fixes dashpay#2667. This is a simplified version of pull request dashpay#3574, which was abandoned by its author. I added some tests as well. Tree-SHA512: e63e00dec8b1b232079380183805cb0b0b18c78ea6bea769837949aab984689d7f68b2ccfe66b1873517b040b9e616ce0eb058575c3d4382aa8c26eebcf1f14e

Diapolo reviewed Jan 23, 2014
View reviewed changes

src/rpcwallet.cpp Outdated

Copy link

Diapolo Jan 23, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Fix for: bitcoin#2667

74ad586

lockunspent will now validate all of the provided outputs are both unspent and belong to the local wallet. The operation is now also atomic.

laanwj reviewed Jan 25, 2014
View reviewed changes

src/wallet.cpp

Copy link

Member

laanwj Jan 25, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

laanwj closed this Jun 24, 2014

BitonicEelis mentioned this pull request Aug 18, 2017

Diagnose unsuitable outputs in lockunspent(). #11087

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

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

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

Uh oh!

Conversation

aalness commented Jan 22, 2014

Uh oh!

laanwj commented Jan 23, 2014

Uh oh!

Diapolo Jan 23, 2014

Choose a reason for hiding this comment

Uh oh!

Diapolo commented Jan 23, 2014

Uh oh!

aalness commented Jan 23, 2014

Uh oh!

laanwj commented Jan 24, 2014

Uh oh!

cozz commented Jan 24, 2014

Uh oh!

aalness commented Jan 24, 2014

Uh oh!

aalness commented Jan 25, 2014

Uh oh!

cozz commented Jan 25, 2014

Uh oh!

laanwj Jan 25, 2014

Choose a reason for hiding this comment

Uh oh!

cozz commented Jan 25, 2014

Uh oh!

aalness commented Jan 25, 2014

Uh oh!

luke-jr commented Feb 21, 2014

Uh oh!

BitcoinPullTester commented Jun 23, 2014

Uh oh!

laanwj commented Jun 24, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants