Use a typedef for monetary values #4234

pull maaku wants to merge 1 commits into bitcoin:master from maaku:money-typedef changing 62 files +397 −356
  1. maaku commented at 8:56 PM on May 26, 2014: contributor

    Replaces int64_t with a typedef in all monetary contexts.

    See #4067 for a more thorough description of this patch and associated context.

  2. sipa commented at 9:31 PM on May 26, 2014: member

    ACK.

    To review, git show --word-diff-regex='[[:alnum:]]+|[^[:space:]]' may be useful.

  3. laanwj commented at 9:50 AM on May 27, 2014: member

    Looks good to me

  4. laanwj commented at 9:56 AM on May 27, 2014: member

    I get quite a few compile errors in the Qt GUI. I think you're missing some casts here and there:

    http://www.hastebin.com/mamaxumafo.vbs

    Mind that qint64 and int64_t are defined differently (long long versus long on AMD64), so converting between then needs casts.

  5. luke-jr commented at 11:36 AM on May 27, 2014: member

    Maybe it should be CMoneyAmount or CSatoshis? CMoney doesn't seem to explain what it is (the whole program is arguably "money" to less-experienced people)

  6. gmaxwell commented at 11:39 AM on May 27, 2014: contributor

    Money sounds fine to me, "CCoins" might be better, alas. I'm not keen on using the base unit word there. If people wanted to change it CValue might be another alternative.

  7. laanwj commented at 11:47 AM on May 27, 2014: member

    CCoins sounds like a list of 'coins' which already have a clear-cut definition in the source code (alas, indeed), and CValue is very general (is it economic value, or some parameter value, or...). CSatoshis would make it specific to Bitcoin, which is what this is trying to avoid in the first place. But let's not bikeshed this too much. I think CMoney is fine.

  8. luke-jr commented at 11:50 AM on May 27, 2014: member

    CCoins already is used for a collection of (specific) coins, isn't it?

  9. gmaxwell commented at 11:53 AM on May 27, 2014: contributor

    That was the "alas".

  10. maaku commented at 8:39 PM on May 30, 2014: contributor

    I think a "coin" should include the entire txout including the scriptPubKey. But yeah, that ship has long-since sailed. @laanwj Your pastbin has expired, although I think I know what is going on with the qint64 and can figure it out. I'm not getting any errors or even warnings on my system however (clang on OS X 10.9.3) so I'm not sure if I'd catch them all. What compile and platform are you using?

  11. laanwj commented at 3:48 PM on June 1, 2014: member

    @maaku Ubuntu 14.04 64-bit

  12. maaku commented at 8:06 AM on June 6, 2014: contributor

    Fixed the compilation issue on Ubuntu 14.04 with a static cast. Also rebased against current master branch.

  13. jgarzik commented at 4:57 PM on July 29, 2014: contributor

    Needs rebase.

    This was ACK'd then sat. Shall we get this moving again? It is the sort of change that should be merged or dropped, as it is unfair to keep asking @maaku to keep rebasing it otherwise.

  14. sipa commented at 11:51 PM on July 29, 2014: member

    Yes please.

  15. jtimon commented at 12:54 AM on July 30, 2014: contributor

    Back to bike-shedding I like CAmount, seems less general than CValue. But, yes, this was separated from #4067 because the typedef only was acked there. Maybe it got forgotten due to the failed merge of the pull-tester...

  16. laanwj commented at 8:43 AM on July 30, 2014: member

    I tend to focus on pulls that fix bugs, add tests or change functionality in a useful way, so 'all over the place' changes like this tend to end up in the back burner.

    Last time it introduced a build problem for me. As that's fixed, and when this is rebased, I'm fine with merging it.

  17. maaku commented at 4:20 PM on July 30, 2014: contributor

    Ok, sounds like there is some consensus developing. I'll rebase ASAP. (Rebasing is a non-trivial process for this PR as it involves manually checking the entire diff history; hopefully I'll have it up this afternoon.)

  18. sipa commented at 1:04 AM on July 31, 2014: member

    To join the shed: CMoney or CAmount are fine to me. So are Money or Amount.

  19. laanwj added the label Improvement on Jul 31, 2014
  20. jtimon commented at 1:30 PM on July 31, 2014: contributor

    This is a version with replacing CMoney with Amount: https://github.com/jtimon/bitcoin/tree/typedef Still without rebase though.

  21. jtimon commented at 5:47 PM on July 31, 2014: contributor

    And here's another version after the rebase (by the way, no, it wasn't trivial): https://github.com/jtimon/bitcoin/tree/typedef2

  22. laanwj commented at 7:59 PM on July 31, 2014: member

    Rebased version using Amount looks good to me (prefer it to Money/CMoney).

  23. maaku commented at 8:10 PM on July 31, 2014: contributor

    I'm reviewing as well, comparing with my own rebase.

    I don't mind changing the name, but only if we get consensus. Amount is way too generic IMHO. Amount of what? Fees? We have a separate class (CFeeRate) for that. CMoney/Money isn't ideal, but I don't see a better, succinct option...

  24. laanwj commented at 8:18 PM on July 31, 2014: member

    Well it's better than Value. We should stop bike-shedding here and simply decide. Otherwise we'll still have this open in a month. @jtimon likes Amount, @sipa likes Amount, I like Amount. So that's our consensus.

    To be entirely clear, just add a comment what it is about in amount.h where you define the type.

  25. sipa commented at 9:20 PM on July 31, 2014: member

    @jtimon Looks good at first glace; care to do a pullreq, so we get pulltester feedback etc?

    I'm very aware that this is a change that will pretty much instantly become non-mergable, so given that there seems reasonable support for it, let's move.

  26. jtimon commented at 12:19 AM on August 1, 2014: contributor

    I conserved maaku's commit, I thought he could fetch my branch and push here, but if that's simpler I'll open a new PR (#4614).

  27. maaku commented at 3:31 PM on August 1, 2014: contributor

    So there were a dozen or so locations where int64_t/qint64 was still being used. Those have been fixed in the latest rebase, just pushed.

  28. in src/miner.cpp:None in 981a908de6 outdated 
      38 | @@ -39,7 +39,7 @@ class COrphan
  39 |      CFeeRate feeRate;
  40 |      double dPriority;
  41 |  
  42 | -    COrphan(const CTransaction* ptxIn) : ptx(ptxIn), feeRate(0), dPriority(0)
  43 | +    COrphan(const CTransaction* ptxIn) : ptx(ptxIn), dPriority(0), feeRate(0)
    sipa commented at 8:13 PM on August 1, 2014:

    Why change the initialization order?

    maaku commented at 5:13 AM on August 2, 2014:

    That... is a fine question. I swear at one point during the rebase I was getting out-of-order initialization warnings, so I switched it. But looking at the diff, the prior ordering is obviously correct. Thank you for catching it.

    I've pushed a new squashed commit that reverses that one hunk.

  29. sipa commented at 8:15 PM on August 1, 2014: member

    Untested ACK (reviewed with above word diff command).

  30. in src/qt/overviewpage.h:None in cff3419562 outdated 
      35 | @@ -34,8 +36,8 @@ class OverviewPage : public QWidget
  36 |      void showOutOfSyncWarning(bool fShow);
  37 |  
  38 |  public slots:
  39 | -    void setBalance(qint64 balance, qint64 unconfirmedBalance, qint64 immatureBalance,
  40 | -                    qint64 watchOnlyBalance, qint64 watchUnconfBalance, qint64 watchImmatureBalance);
  41 | +    void setBalance(const CAmount& balance, const CAmount& unconfirmedBalance, const CAmount& immatureBalance,
  42 | +                    const CAmount& watchOnlyBalance, const CAmount& watchUnconfBalance, const CAmount& watchImmatureBalance);
    laanwj commented at 3:27 PM on August 4, 2014:

    BTW we have many signal/slot definitions like this:

    connect(model, SIGNAL(balanceChanged(qint64, qint64, qint64, qint64, qint64, qint64)), this, SLOT(setBalance(qint64, qint64, qint64, qint64, qint64, qint64)));

    I'm fairly sure that these need to be updated to use CAmount as well. No need to add & or const in these SIGNAL() and SLOT() definitions (they are ignored), but the type needs to match.

    Also the CAmount type likely has to be registered to Qt at some point to be able to pass it in this way (see http://qt-project.org/doc/qt-4.8/custom-types.html).

    As long as CAmount is the same type as qint64 I imagine it will keep working, but it seems brittle.

  31. laanwj commented at 1:05 PM on August 15, 2014: member

    It cannot be merged like this. One thing needs to be done here, either:

    • Register the CAmount type with qt, and use that type name in the signal/slot definitions
    • Change the qt slots/signals back to qint64

    Connecting the signals/slots with amounts as qint64 but having them as CAmount in the .h file is difficult to read, and may break randomly.

  32. jtimon commented at 1:26 PM on August 15, 2014: contributor

    I think the first option is better.

  33. laanwj commented at 9:09 AM on August 26, 2014: member

    @maaku this is quickly going out of sync with the code again, do you intend to address the last nit so that this can be merged?

  34. laanwj commented at 9:26 AM on September 25, 2014: member

    Lots of conflicts already...

    UU src/coins.cpp
UU src/coins.h
UU src/core.cpp
UU src/main.h
UU src/miner.cpp
UU src/qt/transactionfilterproxy.h
UU src/qt/walletmodel.cpp
UU src/qt/walletmodel.h
UU src/rpcrawtransaction.cpp
UU src/rpcserver.cpp
UU src/rpcwallet.cpp
UU src/txmempool.cpp
UU src/util.cpp
UU src/util.h
  35. maaku force-pushed on Sep 26, 2014
  36. BitcoinPullTester commented at 9:27 PM on September 26, 2014: none

    Automatic sanity-testing: FAILED BUILD/TEST, see http://jenkins.bluematt.me/pull-tester/p4234_57c858d8c179a14487a6a85f5dbbc2b087d21e30/ for binaries and test log.

    This could happen for one of several reasons:

    1. It chanages paths in makefile.linux-mingw or otherwise changes build scripts in a way that made them incompatible with the automated testing scripts (please tweak those patches in qa/pull-tester)
    2. It adds/modifies tests which test network rules (thanks for doing that), which conflicts with a patch applied at test time
    3. It does not build on either Linux i386 or Win32 (via MinGW cross compile)
    4. The test suite fails on either Linux i386 or Win32
    5. The block test-cases failed (lookup the first bNN identifier which failed in https://github.com/TheBlueMatt/test-scripts/blob/master/FullBlockTestGenerator.java)

    If you believe this to be in error, please ping BlueMatt on freenode or TheBlueMatt here.

    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.

  37. maaku force-pushed on Sep 26, 2014
  38. BitcoinPullTester commented at 9:48 PM on September 26, 2014: none

    Automatic sanity-testing: FAILED BUILD/TEST, see http://jenkins.bluematt.me/pull-tester/p4234_a8df9965b68498292a1cc588b4ebdc6622b79499/ for binaries and test log.

    This could happen for one of several reasons:

    1. It chanages paths in makefile.linux-mingw or otherwise changes build scripts in a way that made them incompatible with the automated testing scripts (please tweak those patches in qa/pull-tester)
    2. It adds/modifies tests which test network rules (thanks for doing that), which conflicts with a patch applied at test time
    3. It does not build on either Linux i386 or Win32 (via MinGW cross compile)
    4. The test suite fails on either Linux i386 or Win32
    5. The block test-cases failed (lookup the first bNN identifier which failed in https://github.com/TheBlueMatt/test-scripts/blob/master/FullBlockTestGenerator.java)

    If you believe this to be in error, please ping BlueMatt on freenode or TheBlueMatt here.

    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.

  39. Use a typedef for monetary values a372168e77
  40. maaku force-pushed on Sep 26, 2014
  41. BitcoinPullTester commented at 11:43 PM on September 26, 2014: none

    Automatic sanity-testing: FAILED BUILD/TEST, see http://jenkins.bluematt.me/pull-tester/p4234_a372168e77a8a195613a02983f2589252698bf0f/ for binaries and test log.

    This could happen for one of several reasons:

    1. It chanages paths in makefile.linux-mingw or otherwise changes build scripts in a way that made them incompatible with the automated testing scripts (please tweak those patches in qa/pull-tester)
    2. It adds/modifies tests which test network rules (thanks for doing that), which conflicts with a patch applied at test time
    3. It does not build on either Linux i386 or Win32 (via MinGW cross compile)
    4. The test suite fails on either Linux i386 or Win32
    5. The block test-cases failed (lookup the first bNN identifier which failed in https://github.com/TheBlueMatt/test-scripts/blob/master/FullBlockTestGenerator.java)

    If you believe this to be in error, please ping BlueMatt on freenode or TheBlueMatt here.

    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.

  42. BitcoinPullTester commented at 12:11 AM on September 27, 2014: none

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

  43. sipa commented at 8:56 AM on September 27, 2014: member

    ACK. Verified type replacement + headers changes only.

  44. in src/amount.h:None in a372168e77 
       8 | +
   9 | +#include <stdint.h>
  10 | +
  11 | +typedef int64_t CAmount;
  12 | +
  13 | +#endif
    Diapolo commented at 11:59 AM on September 27, 2014:

    Should be #endif // BITCOIN_AMOUNT_H!

  45. Diapolo commented at 12:01 PM on September 27, 2014: none

    @laanwj Is a qint64 always an int64_t, Is that a sane assumption?

  46. laanwj commented at 1:45 PM on September 27, 2014: member

    @Diapolo That's not an assumption that you can make. Only that both are 64 bit and can be cast to each other, but they may be (and are in practice) defined differently.

  47. sipa commented at 5:42 PM on September 27, 2014: member

    Let's not hold this up for stylistic reasons that can be trivially fixed later; it will very quickly start conflicting again probably...

  48. maaku commented at 6:16 PM on September 27, 2014: contributor

    Wladimir made an in-line comment about that already. The right and proper thing to do is to abstract the interface between the two, and to register / provide the necessary hooks for Qt to handle CAmount natively in slot definitions.

    I'll be honest though, I'm not going to have time to write that in the next 1-2 weeks. Given how quickly this PR atrophies, it's probably better to do that as a separate PR.

  49. jtimon commented at 6:29 PM on September 27, 2014: contributor

    Yep, ACK, let's merge this before it needs rebase again.

  50. sipa commented at 6:44 PM on September 27, 2014: member

    @laanwj Is that SLOT typing issue a blocker? If it's not, I think we can hold up other pull requests for a short while while this is being reviewed/merged, but not longer.

  51. laanwj commented at 7:32 AM on September 29, 2014: member

    @sipa The point is that I am not sure how Qt handles types, ie, CAmount that are not directly registered to the system but used nevertheless in slot/signal descriptions. It does some magic especially when serializing for cross-thread messages, and it may be that this only works by accident.

    Has anyone actually tested the wallet GUI with this? That amounts still work correctly when receiving and sending? That update signals still come through properly?

  52. jtimon commented at 8:14 PM on September 30, 2014: contributor

    Can we "abstract the interface between the two, and to register / provide the necessary hooks for Qt to handle CAmount natively in slot definitions" after merging this? The GUI seems to be working just fine with the new type.

  53. sipa commented at 8:23 PM on September 30, 2014: member

    @jtimon My question whether the lack of slot hooks was a blocker. @laanwj asked whether someone actually tested it. It seems you did. So, all good?

  54. jtimon commented at 11:35 PM on September 30, 2014: contributor

    I just opened it in main mode (with no funds) and in regtest mode (where the mined 50s are ok). If you want me to do some other specific testing I'm happy to do it since I really want this to be merged soon, while it still doesn't require yet another rebase.

  55. laanwj commented at 7:42 AM on October 1, 2014: member

    Thanks for testing @jtimon .

    I don't like merging this in good faith. What if it breaks only on some platforms? Having wrong amounts would be the greatest screw-up possible in a GUI. So I'm going to do some testing myself and research the qt documentation on what to do here first.

  56. laanwj commented at 9:27 AM on October 1, 2014: member

    OK - I've verified all three signals involving CAmount:

    • WalletModel::balanceChanged(CAmount,CAmount,CAmount,CAmount,CAmount,CAmount)
    • WalletView::incomingTransaction(QString,int,CAmount,QString,QString)
    • OptionsModel::transactionFeeChanged(CAmount)

    I've also added code to make it possible to send CAmount over queued connections (just two lines).

    Fixes are here https://github.com/laanwj/bitcoin/commit/c122f5528c882efc8aebe31fd4d84612175f66aa

    Note: after merging this, be sure to start building from scratch - I had some strange issues with setting the fee in the options that went away with a clean build.

  57. laanwj merged this on Oct 1, 2014
  58. laanwj closed this on Oct 1, 2014
  59. laanwj referenced this in commit 3fd192f8b4 on Oct 1, 2014
  60. laanwj commented at 9:40 AM on October 1, 2014: member

    Merged via 3fd192f8b4

  61. in src/util.h:None in a372168e77 
      13 | @@ -14,6 +14,7 @@
  14 |  #include "config/bitcoin-config.h"
  15 |  #endif
  16 |  
  17 | +#include "amount.h"
    Diapolo commented at 12:39 PM on October 1, 2014:

    @jtimon Why was this included here? It isn't even used in util!

    jtimon commented at 6:14 PM on October 1, 2014:

    I don't know, a small mistake by @maaku I guess. Can you correct it?

    maaku commented at 6:47 PM on October 1, 2014:

    Yeah that can be removed. It's a leftover from when utilmoneystr was part of util. Should have been removed in the rebase.

  62. maaku referenced this in commit 4104bc7aa8 on Feb 23, 2015
  63. maaku referenced this in commit 28dc2efeaa on Feb 25, 2015
  64. maaku referenced this in commit 3e28331d33 on Mar 10, 2015
  65. DrahtBot locked this on Sep 8, 2021
Contributors
maakusipalaanwjluke-jrgmaxwelljgarzikjtimonBitcoinPullTesterDiapolo
Labels
Refactoring
Linked (view graph)
#4067 Encapsulate coin balances within a new CMoney type.#4614 Use a typedef for monetary values#5464 Use CAmount instead of int64_t
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-03 09:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me