Encapsulate coin balances within a new CMoney type.

maaku commented at 4:54 pm on April 18, 2014: contributor

Provides code parity between bitcoin and side chains which use a different type for representing and performing arithmetic on coin balances, e.g. Freicoin’s use of the GMP library’s arbitrary-precision rational number type for increased divisibility and interest calculations. This greatly reduces the friction in sharing code and submitting code upstream. Also organizes related functionality such as FormatMoney and ParseMoney into a single class framework.

in src/qt/coincontroldialog.cpp: in 41015bb2ed outdated

730@@ -731,7 +731,7 @@ void CoinControlDialog::updateView()
731 
732             // amount
733             itemOutput->setText(COLUMN_AMOUNT, BitcoinUnits::format(nDisplayUnit, out.tx->vout[out.i].nValue));
734-            itemOutput->setText(COLUMN_AMOUNT_INT64, strPad(QString::number(out.tx->vout[out.i].nValue), 15, " ")); // padding so that sorting works correctly
735+            itemOutput->setText(COLUMN_AMOUNT_INT64, strPad(QString::number(EncodeDouble(out.tx->vout[out.i].nValue.ToDouble())), 20, " ")); // padding so that sorting works correctly

laanwj commented at 5:00 pm on April 18, 2014:

Please don’t use double conversions in places that currently use integer arithmetic. We want to avoid using doubles wherever possible.

maaku commented at 7:15 pm on April 18, 2014:

This code necessarily has to have some sort of inexact conversion. That’s rather the point – we shouldn’t be assuming that money calculations are integer arithmetic! So assume that CMoney is implemented using some sort of exact rational arithmetic bignum library. How do we then write code that converts CMoney into an (compact!) ASCII-comparable string, in a way that is code-compatible regardless of the underlying integer type?

A reasonable compromise is what is implemented here – reduce, if necessary, to the precision of a double and then use the newly added EncodeDouble to convert that into a sortable uint64_t. Note that this is 100% consistent with current behavior in bitcoin - double provides 2^53 bits of precision, where MoneyRange constrains maximum value to less than 2^52.

I respect the gut-instinct “eww” attached to floating point conversion. But in this case it’s actually a reasonable compromise.

leofidus commented at 8:42 pm on April 18, 2014:

But introducing double conversion is problematic for all coins that use more than 53 bits of precision, increasing the friction in sharing code and submitting code upstream. I think there are more coins which use integer representation with values beyond 2^53 than there are coins using alternative datatypes.

gmaxwell commented at 9:04 pm on April 18, 2014:

The only metric of “more coins (existing)” that I think we should care about here is how much useful code and fixes their contributors have submitted upstream. By that metric no such other coins exist, AFAIK.

Really I don’t care for this whole pull as a compatibility with other forks thing: Its interesting upstream because it increases type-safety for values.

leofidus commented at 9:37 pm on April 18, 2014:

I agree that the change has value for Bitcoin, I’m simply saying that because interoperability is the stated goal of the PR it looks preferable not to break existing interoperability.

It seems more reasonable to me to use the much simpler

0out.tx->vout[out.i].nValue.ToInt64()

instead of

0EncodeDouble(out.tx->vout[out.i].nValue.ToDouble())

in Bitcoin and use the more complicated version only in implementations which actually implement sub-satoshi precision. That would also remove the entire EncodeDouble function from Bitcoin code, reducing the overall code size.

maaku commented at 1:47 am on April 19, 2014:

@gmaxwell, it’s hard for there to be meaningful contributions back when most alts are stuck on ancient versions of the code base! But even without contribution back, minimizing the code difference between alts makes it easier for maintainers to rebase against newer versions of Bitcoin Core, which increases the security of the greater crypto currency ecosystem. But as you say, the greatest contribution to bitcoin specifically is enforcing some type safety for monetary values. @leofidus, I’m not sure you understand what this change does or the context in which the code executes, as using ToInt64() would not fix the problem and would in fact make it worse. That’s alright; let’s walk through it:

The original line of code above fills in a hidden column with the left-padded string representation of the output’s value, in units of 1e-8 bitcoin. So for example, three outputs with the values 5btc, 0.2btc, and 1mbtc would have the following values for this field:

0000000500000000
1000000020000000
2000000000100000

When you request a numeric sort based on the output value field, what it actually does is do a string sort over this hidden field, which is why the left-padding is necessary. (I’ve used ‘0’ as the padding character just for clarity’s sake on Github, whereas the code actually uses ’ ’ [space].)

Now there are two Bitcoin-specific assumptions here: (1) monetary values are integers, and (2) no output will ever have more than 15 digits of absolute precision. These are not universally valid assumptions, and either or both of them might be invalidated on a side chain. Side chains don’t exist yet so we must look to alts for examples: Dogecoin and Freicoin invalidate one and both, respectfully.

One of the goals of this pull request is to minimize the number of lines of code which are sensitive to these assumptions, and to make the format assumptions explicit when it is necessary (e.g. ToInt64()). So if we do not assume that CMoney is implemented with an integer, and that the dynamic range of monetary values may be more than 15 decimal digits on an absolute scale, how do we still carry out this string-sorting hack for the GUI tables?

What the updated code does is use another cool little hack: it turns out that because IEEE floating point is designed in a particular way, you can sort non-negative floats or doubles as if they were integers and get the same result. There’s a little bit of bit-flipping magic that makes this work for negative values as well, and that’s what EncodeDouble does. So the new code does this:

It converts from whatever the native CMoney format is to 64-bit IEEE floating point, in the process rounding to approximately 16 decimal digits of precision - this is safe to do for bitcoin values in non-consensus code, since MoneyRange restrictions ensure this is a 1:1 function with stable round-trip transformations, at least without FPU bugs or weird, atypical, non-default compiler optimizations.
It uses the new utility function EncodeDouble to map the double to uint64_t in another 1:1 transformation, doing some bit-flipping magic so for any non-NaN floating point values a and b, if a < b then EncodeDouble(a) < EncodeDouble(b).
It then does the exact same print-integer-with-left-padding trick to fill the column, except it pads to 20 bytes instead of 15 to capture the entire range of uint64_t.

For the above values, it results in the following columns:

013960540253593796608
113939505956204314624
213904980397738950656

It’s less human readable, but it’s a hidden field and an implementation detail. Note that the values are still in descending order. The end result is that no matter the internal representation of CMoney, the hidden column will be filled with a twenty-character string representation of the value, which preserves sorting order for up to 16 decimal digits of relative precision. This is in every way better than the current implementation, and fully generic - it does not care what internal representation is used.

maaku commented at 5:01 pm on April 18, 2014: contributor

To be clear, there are a lot of changed lines across 64 files. But that is rather the point of this pull request - any side chain that changes the type for representing money from int64_t to something else has to change all these lines, thereby making it more difficult to share patches or rebase to keep the side chain up to date.

One of the few user visible changes that result from this is the use of strings instead of integers or doubles in the JSON-RPC.

in src/core.h: in 41015bb2ed outdated

164@@ -158,7 +165,7 @@ class CTxOut
165         // need a CTxIn of at least 148 bytes to spend,
166         // so dust is a txout less than 546 satoshis 
167         // with default nMinRelayTxFee.
168-        return ((nValue*1000)/(3*((int)GetSerializeSize(SER_DISK,0)+148)) < nMinRelayTxFee);
169+        return nValue.ToDouble() * (1000.0/3.0) / (GetSerializeSize(SER_DISK,0)+148) < nMinRelayTxFee.ToDouble();

laanwj commented at 5:01 pm on April 18, 2014:

Same here

maaku commented at 7:17 pm on April 18, 2014:

Whoops, this was left over from a previous version of the patch that didn’t have operator/() implemented for CMoney. There’s some talk on IRC about getting rid of operator*() and operator/(), so the problem still remains however. This code will get reworked as part of that, if possible.

maaku commented at 2:06 am on April 19, 2014:

Actually I’m going to push back on this one. Now that I’ve eliminated division of monetary values, this is not so trivial to implement. You have your choice of ToInt64() and ToDouble(), and assuming no bias about the underlying format I would argue that ToDouble() is the correct choice here - keep as many bits of precision for the comparison as possible.

sipa commented at 3:47 pm on May 23, 2014:

I have little problem with using floating-point for policy rules like this.

in src/money.cpp: in 41015bb2ed outdated

70+    else if (fPlus && n > 0)
71+        str.insert((unsigned int)0, 1, '+');
72+    return str;
73+}
74+
75+const CMoney operator/(const CMoney& a, const CMoney& b)

laanwj commented at 5:05 pm on April 18, 2014:

I think it is better to keep the behavior consistent with uint64_t here.

maaku commented at 7:23 pm on April 18, 2014:

Consistent in what way? With division resulting in truncation of sub-satoshi value? That doesn’t make sense when CMoney is changed to use some divisible representation.

I’ve audited the current code base. Nowhere is there consensus critical division going on, and I believe only the user-visible effects would be rounding in the IsDust calculation (see above).

laanwj commented at 5:07 pm on April 18, 2014: member

Couldn’t you accomplish mostly the same (but easier to review that behaviour is unchanged) with typedef uint64_t CMoney ?

Sure, for other chains the definition of CMoney may be more complex, but that’s not our problem.

maaku commented at 6:06 pm on April 18, 2014: contributor

@laanwj, no all the code which assumes CMoney is an integer would have to change (and there’s quite a bit of that). This pull request only allows a specific subset of arithmetic functionality, and requires explicit casts (with rounding modes) to treat the money value as anything else. Explicit is better than implicit, especially when the point is to maintain code compatibility across implementations with different underlying types.

sipa commented at 8:41 pm on April 18, 2014: member

I’m fine with a type to encapsulate the representation and arithmetic of monetary amounts, but does it really need to deal with conversion from/to human readable format too?

Especially since it’s depended on by core, that feels like functionality that belongs on a higher level. Leaving that in util makes the impact of the patch smaller too.

gavinandresen commented at 9:24 pm on April 18, 2014: contributor

NACK from me. I don’t think this passes the cost-to-review-for-correctness versus potential-benefit-for-Bitcoin test.

maaku commented at 2:03 am on April 19, 2014: contributor

@sipa util.h is loaded by core.cpp. I have no problem reverting that change, if that is the consensus decision. I just thought it was cleaner this way.

maaku commented at 4:32 am on April 19, 2014: contributor

@gavinandresen is there anything I could do along the lines of additional tests to make it easier to be confident that it’s correct?

laanwj commented at 6:36 am on April 19, 2014: member

How far do we want to go to facilitate altcoins? Well, I’m fine if it is a matter of renaming types, but not if it gives us more maintenance or testing overhead.

Hence I’d suggest splitting this up.

I like the idea of using a “money” type. Changing this throughout the source code will result in a lot of line changes, but is easy to review. Defining a typedef for uint64_t would be the straightforward way. This will already do most of the work of making code easier to port over without adding code.
Changing around the edge-cases where CMoney is treated as int: we don’t need to change these in Bitcoin. If it’s a small change in the GUI code, I’m fine with it if it makes it easier to use the GUI for altcoins. But in the core/ consensus code: keep the code as-is. Altcoins most likely don’t want to literally take changes there over anyway.

I also agree with @sipa. Don’t put formatting on the number type. I’m in favor of separating data and operations on it.

maaku commented at 7:54 am on April 19, 2014: contributor

@laanwj I think you are underestimating the importance of semantic explicitness and type safety here. A simple typedef int64_t CMoney would leave a half-dozen lines of code scattered around the project which have in-built assumptions about the integer properties of the money type, and worse leave open the possibility of new hidden dependencies being introduced undetected. That’s the key aspect of this pull request, and really the only thing which makes it interesting: adding a semantically explicit, generic, and type checked API as an interface to the underlying money type. I could split it into separate commits if you it is deemed necessary for review.

Regarding formatting, I’m not sure I understand. Aren’t FormatMoney and ParseMoney the exceptions in the code base? Most other types have a ToString method and use serialization methods for reading data structures out of streams – ParseMoney isn’t the same thing exactly, but the analogy is close. Having toString() and static fromString() methods is generally good C++ conventions, and a common pattern in frameworks like Qt.

Maybe it does belong in util.cpp. I just want to understand the reasoning better.

sipa commented at 8:57 am on April 19, 2014: member

A ToString() method is useful for debugging, but I don’t think it should be used for interaction with humans. Even for conversion of data types like CKey or CKeyId there is a separate module (base58) for conversion to the human form.

We’re not quite there yet, but I like to see the code evolve to a point where we can split off a “core data structures” library, which only contains definitions for structures used by the protocol and their serializations. That includes serialize, core, protocol, just the data types from script (not verification/signing), and if committed UTXOs become normative, also part of coins. No logging, no fee policy constants, and preferably no dependencies at all.

laanwj commented at 11:11 am on April 19, 2014: member

@maaku I’ll only accept this if the introduced money type has exactly the same behavior as int64_t. Defining division to be different, for example, or rounding, is not acceptable. If you insist on using a class that’s possible (and can also be straightforward), but it’s somewhat harder to verify than a simple typedef.

Having a few lines scattered around the code that assume that the money type is integer is fine. For bitcoin this will never change. I’m sorry to say, but making it easier for altcoins to merge our code changes is not high on my priority list. I’m okay with renaming types but not anything that has more risk or stresses our already very limited testing and review capacity.

sipa commented at 9:30 pm on April 19, 2014: member

I would be very happy if multiple altcoins would do things like changing the money type :D

maaku commented at 11:32 pm on April 21, 2014: contributor

The debugging/user-string distinction makes sense.

I reverted FormatMoney() and ParseMoney() back into util.cpp, squashed the followon commits, and broke the original commit in two: the first commit does the int64_t -> CMoney change, the second adds type safety by making CMoney a separate class.

maaku commented at 7:36 pm on April 22, 2014: contributor

I have added documentation about EncodeDouble, and moved the CompressAmount / DecompressAmount code to money.cpp, where it is used by a new CCompressedMoney type, as per comments from sipa. I have also added some basic tests of the EncodeDouble / DecodeDouble routines.

Use a typedef for monetary values b6943646c6

Encapsulate coin balances within a new CMoney type

Provides increased type safety of monetary calculations in bitcoin. Also brings code parity between bitcoin and side chains which use a different type for representing and performing arithmetic on coin balances, e.g. Freicoin's use of the GMP library's arbitrary-precision rational number type. This greatly reduces the friction in sharing code and submitting code upsream.

fc1cd73d0f

sipa commented at 3:29 pm on May 23, 2014: member

I still don’t like invoking rounding code (which, imho, is human-machine conversion stuff, not consensus code, even if it’s some special mode for that purpose) from within core data structures. Can’t you add an IMPLEMENT_SERIALIZE to CMoney itself, which in Bitcoin’s implementation just serializes as int64_t, and READWRITE(nAmount) as before in CTxOut?

in src/core.h: in fc1cd73d0f

14@@ -15,8 +15,8 @@
15 class CTransaction;
16 
17 /** No amount larger than this (in satoshi) is valid */
18-static const int64_t MAX_MONEY = 21000000 * COIN;
19-inline bool MoneyRange(int64_t nValue) { return (nValue >= 0 && nValue <= MAX_MONEY); }
20+static const CMoney MAX_MONEY = 21000000 * COIN;
21+inline bool MoneyRange(const CMoney& nValue) { return (nValue >= 0 && nValue <= MAX_MONEY); }

sipa commented at 3:46 pm on May 23, 2014:

Might it make sense to turn this into a CMoney::CheckRange() ?

maaku commented at 5:00 pm on May 23, 2014:

Good suggestion. It’ll increase the review burden, but is much cleaner.

maaku commented at 5:00 pm on May 23, 2014: contributor

There is no rounding code. I stripped all that out; for Bitcoin, conversion to either int64_t or double is lossless and round-trip (although the consensus risk for double is higher due to poor FPU or compiler optimization, and should not be used in consensus code). The ROUND_X modes are present for future-proofing purposes, to ensure that if/when CMoney is changed from integer to some other numeric type, the then lossy conversions to int64_t or double are performed correctly. ROUND_SIGNAL, which is what is used in the consensus code, is a non-rounding mode with the semantics: “do not round under any circumstances, if the value is not lovelessly and round-trip serializable into the target type, throw an exception.” Actual rounding modes are only specified for human-machine interface code (e.g. GUI, RPC api) and non-consensus code (e.g. transaction priority).

About serialization, the issue is that how it gets serialized could depend on context. In particular, serialization to int64_t will always be done in some cases, such as bitcoin-compatible nVersion=1 transactions for pegging. So when serializing an output you will need to know nVersion and sometimes inclusion height of the transaction in order to make a decision about serialization format.

So anyway, that’s why CTxOut needs to be the way it is, but maybe there is use for native serialization in some contexts.. I haven’t thought too much about that. It wouldn’t be difficult to add an IMPLEMENT_SERIALIZE to CMoney. The reason it is not there is so that if someone tries such a READWRITE(nValue), they’ll get an error and realize they have to think about these things ;)

in src/main.cpp: in fc1cd73d0f

780@@ -781,12 +781,12 @@ bool CheckTransaction(const CTransaction& tx, CValidationState &state)
781     return true;
782 }
783 
784-int64_t GetMinFee(const CTransaction& tx, unsigned int nBytes, bool fAllowFree, enum GetMinFee_mode mode)
785+CMoney GetMinFee(const CTransaction& tx, unsigned int nBytes, bool fAllowFree, enum GetMinFee_mode mode)

gavinandresen commented at 5:14 pm on May 23, 2014:

See #3959 and commit 3aa50118ca8cc8972d1c6b6dffe10963ceb86b7d where I represent fees with a CFeeRate class. It is not type-correct to express fees (which are money-per-transaction-size) as CMoney.

maaku commented at 5:22 pm on May 23, 2014:

Thanks for the heads-up. I’ll avoid basing pull-requests on top of pull-requests, but as soon as #3959 gets merged I’ll merge accordingly.

sipa commented at 5:14 pm on May 23, 2014: member

On Fri, May 23, 2014 at 7:00 PM, Mark Friedenbach notifications@github.comwrote:

There is no rounding code. I stripped all that out; for Bitcoin, conversion to either int64_t or double is lossless and round-trip. The ROUND_X modes are present for future-proofing purposes, to ensure that if/when CMoney is changed from integer to some other numeric type, the then lossy conversions to int64_t or double are performed correctly. ROUND_SIGNAL, which is what is used in the consensus code, is a non-rounding mode with the semantics: “do not round under any circumstances, if the value is not lovelessly and round-trip serializable into the target type, throw an exception.” Actual rounding modes are only specified for human-machine interface code (e.g. GUI, RPC api) and non-consensus code (e.g. transaction priority).

Yes, that’s my problem. We’re mixing code that should utility stuff into consensus core headers. It’s just engineering, but IMHO the one purpose of CMoney should be to encapsulate the arithmetic semantics and serialization. But in the actual one place where serialization of values is done, you bypass the serialization that could be encapsulated there.

Saying that serializing a CTxOut requires invoking some utility to forcefully cast the encapsulated money type to an int64 and then serializing that is for me pretty much undoing the single nicest advantage CMoney could have.

About serialization, the issue is that how it gets serialized could depend on context. In particular, serialization to int64_t will always be done in some cases, such as bitcoin-compatible nVersion=1 transactions for pegging. So when serializing an output you will need to know nVersion and sometimes inclusion height of the transaction in order to make a decision about serialization format.

So pass the CTxOut’s or CTransactions nVersion to the serialization code (it has an nVersion argument in all its arguments!), and have CMoney use that to decide how to serialize.

So anyway, that’s why CTxOut needs to be the way it is, but maybe there is use for native serialization in some contexts.. I haven’t thought too much about that. It wouldn’t be difficult to add an IMPLEMENT_SERIALIZE to CMoney. The reason it is not there is so that if someone tries such a READWRITE(nValue), they’ll get an error and realize they have to think about these things ;)

Ok then use a separate wrapper like CMoneyCompressor for that, which perhaps takes the CTransaction’s nVersion as an argument.

I’m just saying: if we’re introducing a type for encapsulating money semantics in a separate module, let’s get all responsibilities in there.

maaku commented at 5:25 pm on May 23, 2014: contributor

Ok a CMoneyCompressor-like class (CTxOutValueSerializer?) seems like a reasonable compromise. A little messy because it’ll need the transaction metadata inside the txout serializer, but I’ll see if I can make it work in a clean way.

sipa commented at 5:30 pm on May 23, 2014: member

Hmm, yeah agree. Putting responsibility about knowledge of transaction versions in money.* isn’t very clean. But the inherent problem is that they can’t be cleanly separated if they’re dependent on each other. Right now, however, this is not a problem in Bitcoin, and my reason for liking this is modularization - not future compatibility with things that Bitcoin itself are unlikely to do.

laanwj commented at 5:38 pm on May 23, 2014: member

To be honest I’m still not enthusiastic to merge this at all. As I’ve said before, it sounds like a overdesign for something that we don’t need in bitcoin.

gavinandresen commented at 6:22 pm on May 23, 2014: contributor

I agree with @laanwj – NACK on merging this from me, doesn’t pass my cost-to-review versus benefit-to-the-codebase test.

maaku commented at 6:44 pm on May 23, 2014: contributor

@laanwj @gavinandresen This solves actual ongoing maintenance problems being encountered by people using the bitcoin code base today, and is good software engineering for bitcoin. If your only opinion is it lacking in your own review prioritization criteria, that’s fine. But please save the NACK for code that you actively want to prevent from being adopted in bitcoin, lest that word lose all meaning. Or am I misunderstanding your intent?

This patch may not fix any of your own problems or pet issues, but it has significant impact on mine and others. It is solving real-world maintenance headaches that are resulting in unnecessarily divergent and less-frequently updated code bases and provides type safety to help prevent future consensus-risk logic errors.

gmaxwell commented at 7:25 pm on May 23, 2014: contributor

@maaku So if the codebases were less diverged we might expect to see you working upstream more? :)

maaku commented at 7:29 pm on May 23, 2014: contributor

@gmaxwell, that’s the whole point! On May 23, 2014 12:25 PM, “Gregory Maxwell” notifications@github.com wrote:

@maaku https://github.com/maaku So if the codebases were less diverged we might expect to see you working upstream more? :)

— Reply to this email directly or view it on GitHubhttps://github.com/bitcoin/bitcoin/pull/4067#issuecomment-44050739 .

petertodd commented at 10:18 am on May 24, 2014: contributor

Speaking as a maintainer of a alto implementation, strong NACK. I’d hate to find out we accidentally added a consensus critical difference from standard into semantics that mattered; changing alto implementations to in turn match those semantics would be difficult and convoluted, especially on stuff like python where each level of abstraction has a significant performance cost. Why change what isn’t broken for the sake of non-bitcoin projects?

Now making a money_value_t typedef is fine and useful for type safety, but only because that doesn’t change behaviour.

petertodd commented at 10:19 am on May 24, 2014: contributor

(s/alto/alt/, stupid android keyboard)

laanwj commented at 10:28 am on May 24, 2014: member

@petertodd It seems we agree. I’ve said so too - a typedef is fine with me. Let’s pull the first commit only, which does just that.

petertodd commented at 10:36 am on May 24, 2014: contributor

@laanwj ACK on a typedef.

Its worth remembering with optional changes like this that if we create a fork accidentally miners alone end up losing something like $50k/hour, and the reputation of Bitcoin will take a hit. (particularly given the reasons for making this change) I’ve already spoken to mining pools that stick with 0.8 because they consider the 0.9 changes to be risky; no sense giving them even more reasons to worry.

sipa commented at 10:44 am on May 24, 2014: member

There are two very different questions here.

One question is whether this is a net improvement to the codebase - independent of the risk. In my opinion, it is, if it actually encapsulated certain responsibilities out of other code. If it helps forked codebases contribute upstream, that’s a nice extra benefit.

However, If there is any doubt at all that this might introduce consensus changes, I don’t think anyone would even think about merging. If not enough people consider that proven, it will have to wait. If people consider it not worth the review time, it may not be merged at all ever - that’s just cost vs. benefits.

Anyway, ACK on a typedef. Let’s see where we go from there.

jtimon commented at 11:37 am on May 24, 2014: contributor

The typedef doesn’t solve the problem of the code making assumptions about the type being an int64 (which is the encapsulation/readability improvement that would directly benefit bitcoin). But merging a typedef would be simpler and it would help a little bit with forked codebases. Another PR replacing the typedef with a full class should be easier to read once the typedef is merged. So, yes, it probably makes sense to separate this PR in two given the fact that the first part is much easier to accept.

maaku commented at 2:25 pm on May 24, 2014: contributor

@petertodd or anyone else, please point to a single line of code that introduces consensus risk. This patch in no way changes the user- or network-visible behavior of the client It was very carefully constructed with precisely that goal in mind. CMoney is still int64, just wrapped in a class! You’ll see in the PR history that I’ve removed features such as operator/() which were safe but less obviously so.

Making CMoney a typedef has zero type safety benefits. Sorry, but that’s how C++ works. It will be treated exactly the same by the compiler as before. Merging just the first commit is better than nothing, and helps me considerably. But all of the direct type safety benefits to bitcoin are in the 2nd commit, and so I strongly recommend reviewing it.

petertodd commented at 2:45 pm on May 24, 2014: contributor

@maaku In Bitcoin even subtle compiler bugs lead to consensus problems; any change is risky. For me to properly review the CMoney class - specifically the idea that it’s “just” int64 wrapped in a class - would require me to get out the C++ standard and actually check that there isn’t any obscure gotchas.

Again, this is a change primarily for the benefit of other currencies, not Bitcoin. Why should we take that risk just so much smaller alts - or commercial ventures with non-public business plans in the case of sidechains - can benefit? (note how you’ve included rounding modes, yet they aren’t used in the Bitcoin code) We’re the ones with tens of thousands of dollars an hour downtime costs.

leofidus commented at 3:17 pm on May 24, 2014: none

Since some altcoins seem to benefit from this change, maybe they can merge this first and incorperate it in their next client version. After some production experience, it might be easier to accept this change here.

jtimon commented at 4:49 pm on May 24, 2014: contributor

By encapsulating functionality this increases the quality, maintainability, readability and future extensibility of bitcoin. “Improve bitcoin’s divisibility would be a trivial hardfork if it’s required in the future” I have read and written this sentence many times and this PR would actually make it true. About “any change is risky therefore any change is bad by default”, maybe we shouldn’t had refactored anything and just left almost everything on the main “module” where it used to be, maybe we shouldn’t have decoupled the initial GUI stuff from the rest of the code, maybe we shouldn’t separate the wallet from the core to avoid “consensus risks”. Good software engineering practices? Oh, no, those are risky… Sorry for the sarcasm. Can we go back to concrete criticisms of the changes instead of arguing with vague “any change is risky” arguments? @leofidus In the case of Freicoin, the main problem is to rebase the few technical changes it has on top of every new bitcoin release. If Freicoin accepts this change but bitcoin doesn’t, the rebase won’t get any simpler, probably worse. Maybe if bitcoin accepts the typedef, Freicoin can deploy the class version before bitcoin does, but I’m not sure that will be very useful since Freicoin would actually implement another version of the class, not the one equivalent to int64 that bitcoin would use.

maaku commented at 5:17 pm on May 24, 2014: contributor

@jtimon @leofidus Actually Freicoin has effectively had this change since the first code was written almost two years ago, although we use the GMP mpq_class directly instead of encapsulating it in CMoney, but the result is the same. Any upstream (bitcoin) change which deals with monetary values – which is just about everything except network code – results in a merge conflict and requires manual intervention. Likewise any Freicoin changes we want to move upstream require manual editing and re-testing, which is a barrier to all-volunteer efforts.

petertodd commented at 1:32 pm on May 25, 2014: contributor

@jtimon > “Improve bitcoin’s divisibility would be a trivial hardfork if it’s required in the future” I have read and written this sentence many times and this PR would actually make it true.

By the time Bitcoin is worth the 6x or so orders of magnitude more required to make a divisibility hardfork necessary, changing CMoney would be the least of our worries with regard to a divisibility upgrade.

As for “good software engineering practices”, consensus-critical code has very different good practices than normal code. Separating the wallet code - which shouldn’t change consensus behavior - from the consensus critical code was a good idea, albeit one that needed to be very carefully evaluated. Making a CMoney class might be a good idea, but there’s no need to rush it; a simple typedef is a conservative change that probably will have no impacts. I understand that you want a much less conservative change for your own reasons - Freicoin - but there’s simply no reason why the rest of the Bitcoin community should take on risk for your benefit.

Remember that the fact that Freicoin has done this change a long while ago does not help in testing the actual concern, which is that a CMoney class may have different consensus critical behavior to int64. To test that you’d have to actually test the transition period between having int64 and CMoney; Freicoin never did that.

laanwj commented at 1:58 pm on May 25, 2014: member

I agree that defining a money type is somewhat ‘better software engineering’ when looked at it some ways. But I see the future of Bitcoin Core as just Bitcoin Core, not ‘Altcoin Core’.

Eventually I want to enshrine the Bitcoin-specific consensus code in a library that as is as small and independent as possible and completely specific to Bitcoin. Altcoins can create their own consensus libraries. I expect there to be little or no code sharing between these. The consensus code does not need to be general.

For this goal I think inside the consensus code it is good to have more low-level concreteness, not more abstractions. Recently we already went from the CBigNum type to well-defined, concrete uint256/uint160/CScriptNum types.

However in the rest of the code that interfaces with client applications (RPC), or the user, or provides some other interface, a more abstract interface that isolates from the specific representation of money inside the system is useful.

jtimon commented at 7:27 pm on May 25, 2014: contributor

Since it seems that nobody would oppose to the innocuous typedef change, I think that probably it would be wise to separate that (which is not that interesting IMO but could be easily accepted) from the class approach to encapsulate serialization and arithmetic, which is clearly much more controversial. I also think that the second commit/PR would be easier to review if it was separated in smaller and more clear commits.

Please tell me if I’m going off-topic or I should move this to the mailing list, I’m trying to understand the reasoning better since I feel it can help me with my own pull requests. Although I don’t understand @laanwj’s argument that an abstraction on top of the low-level well-defined int64 type is risky or undesirable in consensus code, I can understand how changes that could potentially accept consensus are risky and therefore we must be more conservative about them, even if I disagree with the “this is completely useless for bitcoin and only useful for altchains” claim.

On the other hand, I don’t understand the “review-costs” arguments. If someone doesn’t have time to review certain change or doesn’t think it’s a priority to do so, that’s completely understandable. Not reviewing a PR for this reason and leaving it in limbo is one thing. But justifying a NACK to “cut review costs” seems at the very least premature to me. Maybe I’m missing something, but ideally NACKs should be in the form of: “This should never be merged becase A and B break X, Y and Z”, “This would be more acceptable by also implementing tests X, Y and Z” or “this PR doesn’t seem complete without also implementing A, B and C”. The more concrete the objections are, the less frustrating it is for the developer trying to contribute and the more educational for the contributor or in general to other potential contributors reading the pull requests with the objective of educating themselves on how to properly try to contribute.

Again, my apologies if this is not the place for these later observations.

leofidus commented at 7:44 pm on May 25, 2014: none

Am I understanding this correctly that leaving consensus-code unmodified (or typedef’ed) and introducing CMoney in the rest of the codebase would be a compromise which would help everyone (bitcoin has no fork risk but better modularization, freicoin has less work merging changes)? I can at least say that that version would benefit Dogecoin, which would mean that you would also get more help from us testing it.

laanwj commented at 8:28 pm on May 25, 2014: member

@jtimon It’s not so much about ‘review costs’, because review will almost certainly not find all bugs/inconsistencies. It is the costs of some unexpected change causing a fork. Years after Satoshi left people are still finding small unexpected cases in the consensus code.

Just that no one can point them out from glancing at the code doesn’t mean that no problems exist. Making the code (maybe) a bit more readable and consistent is just not worth that. I hope you understand. If the potential benefits don’t outweigh the potential trouble no one wants to take the risk to merge it, and a NACK is in order. That’s better than keeping it in limbo forever, IMO. @leofidus Yes if this could be done only outside the consensus code, it would be acceptable. That’d require an interface layer between the consensus code and the rest, which would be a good thing anyway. It is aligned with the goal of making a consensus library.

BitcoinPullTester commented at 11:12 am on June 23, 2014: none

Automatic sanity-testing: FAILED MERGE, see http://jenkins.bluematt.me/pull-tester/p4067_fc1cd73d0ff73e26da2548193c630b3b8a4d630b/ for test log.

This pull does not merge cleanly onto current master 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.

laanwj added the label Improvement on Jul 31, 2014

laanwj commented at 9:15 am on August 19, 2014: member

Closing this in favor of #4234.

laanwj closed this on Aug 19, 2014

jtimon commented at 0:32 am on October 2, 2014: contributor

After #4234 has been merged, a rebased version of this should be much easier to review.

laanwj commented at 8:01 am on October 2, 2014: member

@jtimon You could save yourself the trouble. As I’ve said multiple times (also above here), I’m willing to go as far as type-defing the money type. That’s what we did, so as far as I’m concerned this issue is now closed.

jtimon commented at 6:43 pm on October 2, 2014: contributor

Maybe this goes too far, but wouldn’t be nice to have a class and convert the functions in utilmoneystr to methods of that class? Maybe just moving that to amount.o would be enough?

sipa commented at 6:49 pm on October 2, 2014: member

At least please keep core data structure definition and human interaction separate. So no, don’t move any utilmoneystr to amount.o or CAmount - that would imply having everything using any core datastructure at all depend on that conversion code.

laanwj commented at 6:56 pm on October 2, 2014: member

I’m for keeping the data type and operations such as formatting on it decoupled. Especially as formatting could be done in different ways, depending on the context, it’s not an innate property of the type, and indeed not necessary by all clients.

DrahtBot locked this on Sep 8, 2021

Encapsulate coin balances within a new CMoney type. #4067