Too many bitcoins allowed as amount. (Issue #2401) #2679

vhf commented at 10:19 PM on May 21, 2013: contributor

trivial change to the GUI: cannot send over 21000000btc as proposed in #2401 Using magic number doesn't seem right. Could we factor this out, together with https://github.com/bitcoin/bitcoin/blob/master/src/bitcoinrpc.cpp#L96 ? And what about BitcoinUnits::parse() as well ?

in src/qt/bitcoinamountfield.cpp:None in b8d14e1b97 outdated

  59 | @@ -60,7 +60,9 @@ bool BitcoinAmountField::validate()
  60 |      bool valid = true;
  61 |      if (amount->value() == 0.0)
  62 |          valid = false;
  63 | -    if (valid && !BitcoinUnits::parse(currentUnit, text(), 0))
  64 | +    else if (valid && !BitcoinUnits::parse(currentUnit, text(), 0))

laanwj commented at 3:45 PM on May 22, 2013:

as you changed this to else if, you can leave out the if (valid ... on this line

vhf commented at 3:50 PM on May 22, 2013:

Absolutely.

super3 commented at 7:59 AM on May 29, 2013: contributor

Can you mash these into one commit?

vhf commented at 8:36 AM on May 29, 2013: contributor

@super3 : Sure. Done.

in src/qt/bitcoinamountfield.cpp:None in ebf6bbda48 outdated

  59 | @@ -60,7 +60,9 @@ bool BitcoinAmountField::validate()
  60 |      bool valid = true;
  61 |      if (amount->value() == 0.0)
  62 |          valid = false;
  63 | -    if (valid && !BitcoinUnits::parse(currentUnit, text(), 0))
  64 | +    else if (!BitcoinUnits::parse(currentUnit, text(), 0))
  65 | +        valid = false;
  66 | +    else if (amount->value() > 21000000)

laanwj commented at 6:20 AM on May 30, 2013:

I don't think this works with the other units? uBTC / mBTC / ...
We shouldn't hardcode a magic number here. In principle this is MAX_MONEY constant but unfortunately this is not available in the GUI code. Maybe add a BitcoinUnits::maxAmount(unit).

vhf commented at 11:47 AM on May 30, 2013:

Thanks for reviewing @laanwj !

Should have thought about that, thanks ! Fixed.
Agreed about the magic number, as said in initial PR. What could we do about it ? Move this constant and possibly others out of main.h to a new header file, which could then be used there, here, and in bitcoinrpc.cpp where the same magic number is also used ?

I updated my PR. Unfortunately when squashing my new commit with this PR this thread gets hidden, sorry about this.

laanwj commented at 12:01 PM on May 30, 2013:

I'm fine with the current solution of putting a magic number in BitcoinUnits. At least everything is together there. It's not a number we'll change often anyway, to say at least :-)

in src/qt/bitcoinunits.cpp:None in 2ad7a6c3a7 outdated

  68 | +    switch(unit)
  69 | +    {
  70 | +    case BTC:  return 21000000;
  71 | +    case mBTC: return 21000000000;
  72 | +    case uBTC: return 21000000000000;
  73 | +    default:   return 21000000;

laanwj commented at 12:00 PM on May 30, 2013:

Please put 0 here :) If we ever end up with an invalid unit, that makes sure the number is always invalid.

vhf commented at 12:07 PM on May 30, 2013:

Yep, done.

laanwj commented at 12:58 PM on May 30, 2013: member

Works for me (TM)

ACK

Suffice commented at 10:37 PM on May 31, 2013: none

This is the one of the most arbitrary and needless things I've seen to be implemented. It's so incredibly unlikely that anyone will ever have even 10 million, let alone all 21 million. What's the point in setting a hard input limit to that? Or any limit for that matter? If someone wants to try to send a bazillion bitcoins, let them. They should have the freedom to try that, no harm will come of it. Altcoin clients with higher limits will then be easier to set up as well.

vhf commented at 10:53 PM on May 31, 2013: contributor

What's the point [...] They should have the freedom to try that, no harm will come of it.

Well, there you have your reason. They should have the freedom to try, and have the chance to spot where it went wrong by seeing a highlighted field. I don't really see a 21'000'000 limit on a field supposed to be limited to 21'000'000 as arbitrary.

As I stated before, this feature is not of first importance. But it's not arbitrary. And I count on altcoin clients' dev to be smart enough to grep 210 -R * (and all other btc-specific numbers, there are plenty) and replace this value here and there. There were two occurrences, now there's three, no big deal.

It's just some trivial changes in response to a two month old issue. If the main devs were seeing this as completely pointless, I suspect they would have closed #2401 by now. If not, now would be a good time to do it. :)

Suffice commented at 11:11 PM on May 31, 2013: none

I think I misunderstood what this was to do slightly. So long as a person can still functionally type in 99,999,999,999 I'm fine with it. (But still personally think there shouldn't be an input limit.) Yet an unobtrusive notice that they exceeded the limit is an alright feature.

Perhaps something more useful would be to show a highlighted cautionary field if a person is trying to send more than certain percentage of what is in their balance. Or some feature like that. Many have accidentally sent 10x or more of what they meant to send because of some extra digits, or miscalculated denominations. Most understand about what percentage of their balance they want to send, and they can then check that to be sure that it's not excessive.

vhf commented at 11:19 PM on May 31, 2013: contributor

No input limit is not an option, not possibly doable. At the moment, the field is limited to 8 digit btc amounts, meaning 99'999'999 is fine but 100'000'000 is not. What this PR does is limiting to 21'000'000, because unlike 99'999'999, 21'000'000 is not arbitrary.

Your suggestion goes way further. You could open an issue, but it will probably raise much more discussion than this PR. The percentage should at least be configurable, but here is not the place to discuss this.

Too many bitcoins allowed in amount. (#2401)

Using magic number doesn't seem right. Could we factor this out, together with https://github.com/bitcoin/bitcoin/blob/master/src/bitcoinrpc.cpp#L96 ?
And what about BitcoinUnits::parse() as well ?

a35e268da4

BitcoinPullTester commented at 10:15 PM on June 2, 2013: none

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

gavinandresen referenced this in commit 2e01ec3207 on Jun 25, 2013

gavinandresen merged this on Jun 25, 2013

gavinandresen closed this on Jun 25, 2013

vhf deleted the branch on Jun 25, 2013

bitcoin locked this on Sep 8, 2021