Encapsulate coin balances within a new CMoney type.
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.
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.
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.
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.
@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.
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.
Closing this in favor of #4234.