Fixed setaccount accepting foreign address #4985

ghost commented at 10:43 PM on September 25, 2014: none

Fixed issue #4209 where using setaccount with a foreign address causes the address to be added to your receiving addresses.

Fixed setaccount accepting foreign address

Fixed issue https://github.com/bitcoin/bitcoin/issues/4209 where using setaccount with a foreign address causes the address to be added to your receiving addresses.

ed0a6f1d0e

Fixed a parenthesis error

I forgot a parenthesis at the end of my if statement, which caused the code to fail.

91f0339d3e

Reworked ownership detection, added error message

Included the 'current unused key' detection in the ownership check of the address, and added a throw error if the address is foreign.

42f79d681b

Changed a test to THROW

Trying out an idea which should fix a failing test with the RPC command setaccount. I am not so sure about these tests, so if this is no good just let me know.

b19f1766ab

Fixed comment typo for ownership test

The comment previously said "... if the address if yours." and I have fixed the type to say "... if the address is yours.

362327f288

in src/rpcwallet.cpp:None in 91f0339d3e outdated

 237 | @@ -238,8 +238,10 @@ Value setaccount(const Array& params, bool fHelp)
 238 |              GetAccountAddress(strOldAccount, true);
 239 |      }
 240 |  
 241 | -    pwalletMain->SetAddressBook(address.Get(), strAccount, "receive");
 242 | -
 243 | +    // Only add the account if the address if yours.
 244 | +    if (IsMine(*pwalletMain, address.Get()))

laanwj commented at 7:38 AM on September 26, 2014:

You're on the right track. However IMO you should do the ismine check before the code above here, to make sure the "Detect when changing the account of an address that is the 'unused current key' of another account" isn't done either for a foreign address. Also it'd be nice to throw an error throw JSONRPCError(...), instead of silently failing.

unknown commented at 1:39 PM on September 26, 2014:

Okay, thanks for the feedback. I will update it.

BitcoinPullTester commented at 6:24 PM on September 26, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4985_362327f2881dd76b87bdef363beed3ad72cd7a7a/ for binaries and test log. 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.

ghost commented at 10:28 PM on September 28, 2014: none

@laanwj is this pull request ready yet do you think? Just wondering if it is up to par.

laanwj commented at 11:02 AM on September 29, 2014: member

Looks good to me now, untested ACK

ghost commented at 11:39 PM on September 29, 2014: none

Does ACK stand for Amsterdam Compiler Kit? If so, could I try to test it and give you the results?

sipa commented at 11:39 PM on September 29, 2014: member

"ACK" means "I agree with merging".

sipa commented at 11:40 PM on September 29, 2014: member

utACK (which means "I agree with merging, but did not test anything")

ghost commented at 11:42 PM on September 29, 2014: none

Ah okay, thank you for the clarification @sipa , I wasn't sure.

ghost commented at 9:16 PM on September 30, 2014: none

I compiled my fork and tested the new feature with bitcoind -regtest -daemon. It is working as expected as you can see on this pastebin of my commands and output.

TheBlueMatt commented at 11:50 PM on September 30, 2014: member

There is no longer a success test for setaccount in the tester. Can you add one?

ghost commented at 12:58 AM on October 1, 2014: none

@TheBlueMatt I'd be glad to try and add that. You said "there is no longer a success test" Was the test removed when I did the following:

-BOOST_CHECK_NO_THROW(CallRPC("setaccount 1D1ZrZNe3JUo7ZycKEYQQiQAWd9y54F4XZ nullaccount"));
+BOOST_CHECK_THROW(CallRPC("setaccount 1D1ZrZNe3JUo7ZycKEYQQiQAWd9y54F4XZ nullaccount"), runtime_error);

Or was it something else?

TheBlueMatt commented at 2:04 AM on October 1, 2014: member

Yes, I'm referring to that test (specifically, I no longer see any test which verifies that setaccount returns true, so changing setaccount to always throw would not be caught by tests).

in src/rpcwallet.cpp:None in 362327f288 outdated

 248 | +        pwalletMain->SetAddressBook(address.Get(), strAccount, "receive");
 249 |      }
 250 | -
 251 | -    pwalletMain->SetAddressBook(address.Get(), strAccount, "receive");
 252 | +    else
 253 | +        throw JSONRPCError(RPC_MISC_ERROR, "Foreign Bitcoin address");

laanwj commented at 7:00 AM on October 1, 2014:

I'd suggest a different error message here, for example "setaccount can only be used with own address". I'm not sure what a user would make of "Foreign Bitcoin address".

unknown commented at 1:26 PM on October 1, 2014:

Okay, that does sound like a more descriptive message. I'll change it.

ghost commented at 1:23 PM on October 1, 2014: none

Okay I have a thought about this a while: for the test to return true the address must belong to the user. Is there a wallet with addresses used for development? If not, I am thinking I could just make a new address. Then, in the code, I would just import the public/private keys before using setaccount on the address. I was also thinking of using a regtest address, but I noticed that the other tests use a real bitcoin address.

Improved the error message for IsMine check 4ef0e3d64f

laanwj commented at 1:40 PM on October 1, 2014: member

Then, in the code, I would just import the public/private keys before using setaccount on the address.

You could just generate an address and setaccount that.

ghost commented at 1:49 PM on October 1, 2014: none

You could just generate an address and setaccount that.

I had thought of that, and that does seem simpler to me. I am wondering, however, if that would just be needlessly using up addresses. Is that an issue or not?

ghost commented at 1:59 PM on October 1, 2014: none

Actually, to find out, I could just look at what other tests have done which require an owned address.

ghost commented at 2:12 PM on October 1, 2014: none

I think I got it now. We'll see.

Added setaccount success test with demo address 247ff7b2db

ghost commented at 2:38 PM on October 1, 2014: none

I will revisit this later today. I see that when I moved the demo address to another account it messed with some other tests.

Created new address for setaccount success test f71553c217

ghost commented at 8:13 PM on October 1, 2014: none

Looks good now. Give me feedback if you like. I'll actually compile and test this again tonight.

in src/test/rpc_wallet_tests.cpp:None in f71553c217 outdated

  79 | @@ -80,11 +80,15 @@ BOOST_AUTO_TEST_CASE(rpc_wallet)
  80 |  		walletdb.WriteAccount(strAccount, account);
  81 |  	});
  82 |  
  83 | -
  84 | +    CPubKey setaccountDemoPubkey = pwalletMain->GenerateNewKey();

TheBlueMatt commented at 5:15 AM on October 2, 2014:

Whats with the indentation on this line?

TheBlueMatt commented at 5:16 AM on October 2, 2014: member

Indentation, squash, then utACK.

Changed mixed indentation to four spaces cfe104b76f

laanwj commented at 5:22 PM on October 2, 2014: member

@ericshawlinux it's fine, I'll handle it from here

laanwj referenced this in commit 20a4b69360 on Oct 2, 2014

laanwj commented at 5:33 PM on October 2, 2014: member

Merged via 20a4b69

Squashed everything into two commits: a functional change and an indentation commit
Changed the indentation commit to a clang-reformat, so that's done now

laanwj closed this on Oct 2, 2014

ghost commented at 5:39 PM on October 2, 2014: none

Okay, thanks :)

MarcoFalke locked this on Sep 8, 2021