Use IsMine to validate custom change address #11184

dooglus · 2017-08-28T23:07:28Z

... so it recogizes SegWit change addresses.

Fixes #11137.

promag · 2017-08-28T23:28:30Z

src/qt/sendcoinsdialog.cpp

IMO remove the comment, the code is more expressive.

Move { to this line.

promag · 2017-08-28T23:29:50Z

src/qt/walletmodel.cpp

const CTxDestination& dest.

Where can I find guidelines about where to put the whitespace? I went with the existing layout. See how the line I replaced had (const CKeyID &address)?

This is mentioned in the developer notes. Though, you could always just run clang format (also mentioned in the developer notes)

promag · 2017-08-28T23:30:08Z

src/qt/walletmodel.h

Sort include.

promag · 2017-08-28T23:30:30Z

src/qt/walletmodel.h

const CTxDestination& dest.

For the record, another option, that doesn't require to include in this header script/ismine.h, is bool isSpendable(const CKeyID& address).

I don't see bool isSpendable(const CKeyID& address) anywhere. And the point is that we want to be able to check the spendability of general addresses, not only CKeyIDs.

maflcko

utACK 89daeb84915d7fd21230771ed266dde1f7d7afda

maflcko · 2017-08-28T23:35:39Z

src/qt/walletmodel.cpp

function names are PascalCase.

Check src/qt/walletmodel.h - they're all lowerThenUpper

Yes but it's outdated.

So what's the plan? Shouldn't we go through the code fixing all the outdated style at once rather than fixing one at a time?

I mean, I can change just the one I touched if that's preferable.

maflcko · 2017-08-28T23:40:30Z

src/qt/sendcoinsdialog.cpp

if you feel like it, you might add a const CTxDestination dest = addr.Get(); before that line and then use if (!bool(model->IsMine(dest) & ISMINE_SPENDABLE)).

Though, it shouldn't make a difference.

promag · 2017-08-29T00:13:07Z

BTW, squash and reword, commit message is too long.

dooglus · 2017-08-29T00:18:48Z

Addressed @MarcoFalke's nit re. masking out ISMINE_SPENDABLE.

Squashed.

Reworded commit message.

promag · 2017-08-29T00:21:21Z

Thanks @dooglus but could you remove period. Suggestion Use IsMine to validate custom change address. Update PR title accordingly.

sipa · 2017-08-29T00:34:08Z

src/qt/sendcoinsdialog.cpp

The bool(..) is unnecessary.

dooglus · 2017-08-29T00:38:50Z

Removed period from commit message

Removed bool

Squashed again

dooglus · 2017-08-29T00:39:45Z

Replaced commit message with @promag's suggested one

dooglus · 2017-08-29T00:49:11Z

It appears that calling the function IsMine instead of isMine caused an error:

  CXX      qt/qt_libbitcoinqt_a-walletmodeltransaction.o
qt/walletmodel.cpp:566:12: error: too many arguments to function call, expected 1, have 2; did you mean '::IsMine'?
    return IsMine(*wallet, dest);
           ^~~~~~
           ::IsMine
./script/ismine.h:39:12: note: '::IsMine' declared here
isminetype IsMine(const CKeyStore& keystore, const CTxDestination& dest, SigVersion = SIGVERSION_BASE);
           ^

I'll call it 'IsSpendable()` instead.

maflcko · 2017-08-29T09:07:27Z

utACK 848ef973d7ecb054e0d6d6952fd69a0816f3b7cd

promag · 2017-08-29T09:13:58Z

@dooglus i believe you only had to call like ::IsMine(). Either way please update PR title too, and elaborate more about the motivation in the description. Ty.

meshcollider · 2017-08-29T11:37:59Z

@promag title is still correct I believe, because it still uses IsMine() to check

promag · 2017-08-29T16:48:05Z

@meshcollider it's too long 🙄.

dooglus · 2017-08-29T18:47:53Z

I finally understood what you meant about IsSpendable(), so did it that way. IsSpendable() calls IsMine(), so the commit log is correct. I've used the same string for the PR title.

sipa · 2017-08-29T19:43:11Z

Should change to a watch-only address be permitted?

dooglus · 2017-08-29T20:29:39Z

Change to any address is permitted. The warning is for if you do not have control of the address.

I'd like to see a warning if I accidentally try to send change to a watchonly address. Because maybe I accidentally copied the wrong address and am about to lose my coins.

promag · 2017-08-29T20:42:19Z

should change to a watch-only address be permitted?

@sipa follow up discussion? For now ISMINE_SPENDABLE is clear and does prevents possible problems that ISMINE_WATCH_ONLY could have.

jonasschnelli · 2017-08-29T21:15:26Z

src/qt/sendcoinsdialog.cpp

Wouldn't if (!model->IsSpendable(addr.Get())) { be simpler?

Yes it would, and that's how I had it until @MarcoFalke made this comment.

Is there a style guide that tells me how such things should be? Lots of people seem to have ideas about what's right, but I'd like to see something in writing, as it were.

dest can be used in a few places below, but as I mentioned above this is just personal preference for one style over another. Just use what you feel makes more sense, no need to discuss.

Good point. I use it in the few places below now too.

jonasschnelli · 2017-08-29T21:15:39Z

utACK 848ef973d7ecb054e0d6d6952fd69a0816f3b7cd

meshcollider · 2017-08-30T10:21:38Z

utACK c41224d

promag · 2017-09-04T14:51:26Z

utACK c41224d.

practicalswift · 2017-09-04T21:51:31Z

Concept ACK! Solves also the issue discussed in my PR #10972 which I've now closed.

laanwj · 2017-09-05T22:48:44Z

Ouch, did no one notice that this was filed against 0.15? Had to revert it, because otherwise it would mess up the release process for 0.15.0.
We need a new PR for master.

a1ea1cf qt: Use IsMine to validate custom change address (Chris Moore) Pull request description: Fixes #11137 Closes #11184 (which was accidentally opened against 0.15 branch) Tree-SHA512: a20a59b4f36c1471a9c84bcc7c69048576d1f413104c299a7ed9ba221f28eddf93d727fca2926420ea5d0dd9aba582924f26a5acd44d995039b7202c73eb53bc

a1ea1cf qt: Use IsMine to validate custom change address (Chris Moore) Pull request description: Fixes bitcoin#11137 Closes bitcoin#11184 (which was accidentally opened against 0.15 branch) Tree-SHA512: a20a59b4f36c1471a9c84bcc7c69048576d1f413104c299a7ed9ba221f28eddf93d727fca2926420ea5d0dd9aba582924f26a5acd44d995039b7202c73eb53bc

promag reviewed Aug 28, 2017

View reviewed changes

maflcko reviewed Aug 28, 2017

View reviewed changes

dooglus force-pushed the change_ismine_0.15 branch 2 times, most recently from 434c2cf to b49064c Compare August 29, 2017 00:17

sipa reviewed Aug 29, 2017

View reviewed changes

src/qt/sendcoinsdialog.cpp Outdated

Copy link

Member

sipa Aug 29, 2017

Choose a reason for hiding this comment

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

The bool(..) is unnecessary.

dooglus force-pushed the change_ismine_0.15 branch from b49064c to cc827dd Compare August 29, 2017 00:37

dooglus force-pushed the change_ismine_0.15 branch from cc827dd to bddab7f Compare August 29, 2017 00:39

dooglus force-pushed the change_ismine_0.15 branch from bddab7f to 1143d1e Compare August 29, 2017 00:42

dooglus force-pushed the change_ismine_0.15 branch from 1143d1e to 848ef97 Compare August 29, 2017 00:51

fanquake added the Wallet label Aug 29, 2017

dooglus changed the title ~~Use IsMine() to check whether we own the custom change address...~~ Use IsMine to validate custom change address Aug 29, 2017

jonasschnelli reviewed Aug 29, 2017

View reviewed changes

Use IsMine to validate custom change address

c41224d

dooglus force-pushed the change_ismine_0.15 branch from 848ef97 to c41224d Compare August 30, 2017 07:43

TheBlueMatt mentioned this pull request Sep 4, 2017

[Qt] Check return value of addr.GetKeyID(keyid) on custom change address change #10972

Closed

laanwj added this to the 0.15.1 milestone Sep 5, 2017

laanwj added the Needs backport label Sep 5, 2017

laanwj merged commit c41224d into bitcoin:0.15 Sep 5, 2017

maflcko mentioned this pull request Sep 5, 2017

qt: Use IsMine to validate custom change address #11247

Merged

maflcko removed the Needs backport label Sep 5, 2017

maflcko removed this from the 0.15.1 milestone Sep 5, 2017

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

Use IsMine to validate custom change address #11184

Use IsMine to validate custom change address #11184

Uh oh!

Conversation

dooglus commented Aug 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag Aug 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag commented Aug 29, 2017

Uh oh!

dooglus commented Aug 29, 2017

Uh oh!

promag commented Aug 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dooglus commented Aug 29, 2017

Uh oh!

dooglus commented Aug 29, 2017

Uh oh!

dooglus commented Aug 29, 2017

Uh oh!

maflcko commented Aug 29, 2017

Uh oh!

promag commented Aug 29, 2017

Uh oh!

meshcollider commented Aug 29, 2017

Uh oh!

promag commented Aug 29, 2017

Uh oh!

dooglus commented Aug 29, 2017

Uh oh!

sipa commented Aug 29, 2017

Uh oh!

dooglus commented Aug 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Aug 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

promag Aug 28, 2017 •

edited

Loading

promag commented Aug 29, 2017 •

edited

Loading

dooglus commented Aug 29, 2017 •

edited

Loading

practicalswift commented Sep 4, 2017 •

edited

Loading

laanwj commented Sep 5, 2017 •

edited

Loading