Move a check from SendMoneyToDestination to SendMoney.

[domob1812]

Move the checks for the amount sent from SendMoneyToDestination to
SendMoney. These checks make sense for SendMoney itself, not only for
SendMoneyToDestination.

[laanwj]

There is already a balance check in SendMoney in https://github.com/bitcoin/bitcoin/blob/master/src/wallet.cpp#L1504

See also pull #4331, which also changes where and how balance checks are being done.

[domob1812]

Thanks for the quick reply. Nevertheless, I think that the checks are simply misplaced in SendMoneyToDestination, since they apply equally to SendMoney alone. Whether or not they belong there is another question. I think that the error message "Insufficient funds" is better suited than "This transaction requires a fee of at least..." for the case that "nValue > GetBalance()". Also, the check for negative nValue makes sense in my opinion. This case is not directly caught by the current check in SendMoney, although it presumably is in generic transaction checking code.

If you think that the checks are superfluous, then I can update the patch to just remove them instead of moving.

[laanwj]

Agreed that the check really belongs in SendMoney.
BTW: this would make SendMoneyToDestination a trivial function, maybe we can get rid of it completely.

Edit: Had the functions the wrong way around

[domob1812]

Yes, I thought about that myself. It seemed like a "too invasive" change for the initial proposal, but I'll gladly do it. So far, SendMoney isn't actually used anywhere by itself, only SendToDestination. The latter is used only from the "sendtoaddress" and "sendfrom" RPC commands in rpcwallet.cpp.

So if you don't want to keep SendMoney by itself just for the sake of it, the simplest solution would be to get rid of it entirely and move everything to SendMoneyToDestination. Or, alternatively, we could "inline" the trivial logic (construction of the output script from the address) and use SendMoney directly at both places.

What do you think? I would personally go for the first way, i. e., remove SendMoney and only keep SendMoneyToDestination unless there's some belief that SendMoney may be needed again in the future. This will clean up the code the most. If you agree with either of the solutions, I'll update the patch. Thanks for your input!

[laanwj]

Agreed. Let's combine the functions and rename SendMoneyToDestination to SendMoney.

For more advanced use-cases such as sending to a non-destination script it's easy enough to use CreateTransaction/CommitTransaction manually. This is, for example, what the GUI does because it can send to multiple recipients.

[domob1812]

Done. Did you imagine it like this?

[laanwj]

Yes, ACK

[BitcoinPullTester]

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4463_e832ab7754732af7fd46ad7f16eef973e238c357/ 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.