Use a typedef for monetary values

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.

sipa commented at 9:31 pm on May 26, 2014: member

ACK.

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

laanwj commented at 9:50 am on May 27, 2014: member

Looks good to me

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.

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)

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.

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.

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?

gmaxwell commented at 11:53 am on May 27, 2014: contributor

That was the “alas”.

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?

laanwj commented at 3:48 pm on June 1, 2014: member

@maaku Ubuntu 14.04 64-bit

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.

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.

sipa commented at 11:51 pm on July 29, 2014: member

Yes please.

jtimon commented at 0: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…

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.

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.)

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.

laanwj added the label Improvement on Jul 31, 2014

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.

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

laanwj commented at 7:59 pm on July 31, 2014: member

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

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…

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.

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.

jtimon commented at 0: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).

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.

in src/miner.cpp: 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.

sipa commented at 8:15 pm on August 1, 2014: member

Untested ACK (reviewed with above word diff command).

in src/qt/overviewpage.h: 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:

0connect(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.

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.

jtimon commented at 1:26 pm on August 15, 2014: contributor

I think the first option is better.

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?

laanwj commented at 9:26 am on September 25, 2014: member

Lots of conflicts already…

 0UU src/coins.cpp
 1UU src/coins.h
 2UU src/core.cpp
 3UU src/main.h
 4UU src/miner.cpp
 5UU src/qt/transactionfilterproxy.h
 6UU src/qt/walletmodel.cpp
 7UU src/qt/walletmodel.h
 8UU src/rpcrawtransaction.cpp
 9UU src/rpcserver.cpp
10UU src/rpcwallet.cpp
11UU src/txmempool.cpp
12UU src/util.cpp
13UU src/util.h

maaku force-pushed on Sep 26, 2014

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:

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)
It adds/modifies tests which test network rules (thanks for doing that), which conflicts with a patch applied at test time
It does not build on either Linux i386 or Win32 (via MinGW cross compile)
The test suite fails on either Linux i386 or Win32
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.

maaku force-pushed on Sep 26, 2014

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:

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)
It adds/modifies tests which test network rules (thanks for doing that), which conflicts with a patch applied at test time
It does not build on either Linux i386 or Win32 (via MinGW cross compile)
The test suite fails on either Linux i386 or Win32
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.

Use a typedef for monetary values a372168e77

maaku force-pushed on Sep 26, 2014

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:

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)
It adds/modifies tests which test network rules (thanks for doing that), which conflicts with a patch applied at test time
It does not build on either Linux i386 or Win32 (via MinGW cross compile)
The test suite fails on either Linux i386 or Win32
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.

BitcoinPullTester commented at 0: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.

sipa commented at 8:56 am on September 27, 2014: member

ACK. Verified type replacement + headers changes only.

in src/amount.h: 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!

Diapolo commented at 12:01 pm on September 27, 2014: none

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

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.

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…

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.

jtimon commented at 6:29 pm on September 27, 2014: contributor

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

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.

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?

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.

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?

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.

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.

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.

laanwj merged this on Oct 1, 2014

laanwj closed this on Oct 1, 2014

laanwj referenced this in commit 3fd192f8b4 on Oct 1, 2014

laanwj commented at 9:40 am on October 1, 2014: member

Merged via 3fd192f8b4

in src/util.h: 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.

maaku referenced this in commit 4104bc7aa8 on Feb 23, 2015

maaku referenced this in commit 28dc2efeaa on Feb 25, 2015

maaku referenced this in commit 3e28331d33 on Mar 10, 2015

DrahtBot locked this on Sep 8, 2021

Use a typedef for monetary values #4234