CAmount: Small improvements related to CAmount

[jtimon]

if (info.amount != 0) is more readable than if (info.amount). Also if CAmount was a class instead of a typedef, the conditional operator cannot be overloaded.
The wallet should be using CAmount instead of int64_t.

[laanwj]

ACK on the change to src/wallet.h.

As discussed on IRC, I don't agree readability being a sufficient reason to change the others as it's very subjective.

[maaku]

ACK. If/when CAmount is made into its own class for type safety and sidechain extensibility reasons, implicit boolean conversion will no longer be desirable. The alternative != 0 construction is just as readable, correct, and achieves future proofing.

[jtimon]

The code in jtimon@710d428 doesn't compile giving the error:

In file included from ./amount.h:9:0,
                 from qt/guiutil.h:8,
                 from qt/guiutil.cpp:5:
./serialize.h: In instantiation of ‘void SerReadWrite(Stream&, const T&, int, int, CSerActionSerialize) [with Stream = CSizeComputer; T = TOperableAmount<long int>]’:
./primitives/transaction.h:120:9:   required from ‘void CTxOut::SerializationOp(Stream&, Operation, int, int) [with Stream = CSizeComputer; Operation = CSerActionSerialize]’
./primitives/transaction.h:116:5:   required from here
./serialize.h:765:40: warning: ISO C++ says that these are ambiguous, even though the worst conversion for the first is better than the worst conversion for the second: [enabled by default]
     ::Serialize(s, obj, nType, nVersion);
                                        ^
./serialize.h:498:13: note: candidate 1: void Serialize(Stream&, const T&, long int, int) [with Stream = CSizeComputer; T = TOperableAmount<long int>]
 inline void Serialize(Stream& os, const T& a, long nType, int nVersion)
             ^
In file included from ./amount.h:9:0,
                 from qt/guiutil.h:8,
                 from qt/guiutil.cpp:5:
./serialize.h:162:39: note: candidate 2: void Serialize(Stream&, bool, int, int) [with Stream = CSizeComputer]
 template<typename Stream> inline void Serialize(Stream& s, bool a, int, int=0)    { char f=a; WRITEDATA(

So it seems boolean conversion is not compatible with our serialization templates.
I used some code for the "safe bool operator" from here http://www.artima.com/cppsource/safebool3.html
Coincidentally the section "Knowing When to Say No" gives some additional reasons not to use them. It says "Before you go ahead, please consider that it's imperative to understand that this idiom should only be used where there is a reasonably unambiguous notion of validity for objects of a class".
And I wonder if this is true for CAmount. Here it's being assumed that the null value is 0 for CAmount. In CTxOut::SetNull() and CTxOut::IsNull(), "-1" is used for the "null value" of the CAmount CTxOut::nValue instead.

I can separate the two commits in different PRs if it's needed.

[petertodd]

Why are we changing consensus critical code for the sake of altcoins/sidechains? If there is no clear reason to change it, leave that code alone. Frankly we already have a huge number of changes coming down very quickly for v0.10 - I know I personally am finding it hard to find time for reviewing it all. Even bringing up the topic of alt-coins otherwise is loudly discouraged... Yet here you're making dangerous changes to Bitcoin Core just so you'll save money on programmers.

As it is, it's not even clear why your sidechain code would even benefit, and of course, that's currently non-opensource software that benefits no-one other than a secretive commercial company. You know, I personally am working on what you guys would call a sidechains implementation, and as I want to offer new functionality over Bitcoin it doesn't share any code at all with Bitcoin. Nor do I ask to have Bitcoin Core changed for the sake of my client's commercial venture.

Of course, maybe I'm biased - I do spend almost all my time on Bitcoin Core reviewing, writing unit tests, and increasing code coverage rather than breaking things...

[petertodd]

And before you reply complaining about this exact pull-req... Keep in mind how utterly tone-deaf this stuff is, for instance saying:

If/when CAmount is made into its own class for type safety and sidechain extensibility reason

Especially as we've already handled the type safety issue elsewhere by making it a typedef, and more importantly, this is consensus critical code that nearly never needs to change at all.

[maaku]

Yet here you're making dangerous changes to Bitcoin Core just so you'll save money on programmers.

Seriously Peter? Changing if (integer) to if (integer != 0) is "dangerous"?

Especially as we've already handled the type safety issue elsewhere by making it a typedef

I don't want to sidetrack this discussion into a rehash of #4037, but changing from int64_t to CAmount achieved almost no type safety benefits.

[petertodd]

Seriously Peter? Changing if (integer) to if (integer != 0) is
"dangerous"?

Like I said, I'm referring to the impact of this work in totality; not specifically this one single change. I think you missed the intent above, which was why you're arguing we need changes: ultimately for the sake of altcoins and sidechains more than Bitcoin Core's needs.

Also every line changed does incur precious review effort; arguing against does too, but that can pay dividends in the future.

Anyway, I don't recall seeing either of you doing much work with the consensus critical code before, especially in testing it - reimplementing it is a good learning experience as well. You'll find your enthusiasm for changing it goes down greatly once you get more experience working with it.

[maaku]

Arguing against changes for the sake of arguing wastes everybody's time. Is this change harmful? No. The C++ standard states that implicit conversion of integers to boolean values is performed by comparing against zero, which is done explicitly here. It solves problems for people who are working with other changes to bitcoin's codebase. The change is innocuous and makes someone's life easier. Why bring politics into the development process?

As to your ad hominum attacks, I suggest you take that elsewhere.

[gmaxwell]

Incidentally, this currently has nothing to do with sidechains. (And only commenting here because PT is oddly posting all around the internet saying it does, which makes no sense, and people are asking me... and answering once here is more efficient.)

Jtimon has been doing code structure cleanups in Bitcoin core of this kind since something like March.

The type changes stuff was done a while back and is justified for increased analyizability of the code (e.g. allowing for later improvements to type-safey of the sort that would have prevented the sighash_single bug). It was also easy to prove as safe.

@petertodd you also ACKed the typedef conversion of the money type in #4067 yourself.

The wallet.h change is obviously needed for consistency. The rest I don't have an opinion on.

[jtimon]

I'm pretty sure that these "dangerous changes" to "consensus critical code" (wallet.h, qt/guiutil.cpp and qt/receiverequestdialog.cpp no less) actually result in an identical build.
You are right that the number of people able to review consensus code is limited (in fact apparently the number people able to distinguish which code is consensus-critical and what changes will actually result in a different binary is even smaller than I thought).
As I argued in #5153, the solution is to make the code more accessible to more people, not "never changing the critical code at all". Someone should really write that tool for "identical-binary checking" it will be very useful.
Anyway, this seems a broader discussion that probably belongs to the mailing list. These changes are trivial (and thus not dangerous) and they're not changing consensus code but rather wallet and gui code.
The why is rather simple too: move a little bit more towards type safety in amounts, a work that is not complete. It does benefit altcoins like freicoin saving it a couple of additional commits in the next rebase on top of bitcoin? Yes, so what?
Freicoin's technical choice of maximizing the common codebase with Bitcoin benefits both projects by not fragmenting development unnecessarily. As an argument in favor of a change, benefiting codebases built on top of bitcoin may be a weak argument, but it should never be an argument against the change either.

[petertodd]

On 7 December 2014 11:46:56 GMT+00:00, Gregory Maxwell notifications@github.com wrote:

Incidentally, this has nothing to do with sidechains.

Mark's ACK above which specifically mentions sidechains as a reason to make this change; as has been advocated elsewhere by him and the pull-req author. (elsewhere altcoins have been brought up as the reason to make these changes too)

You'll note how I did not NACK this parch specifically; I simply want to be sure the overall plan is to continue the overall criteria for changing code in Bitcoin remains whether or not it will benefit Bitcoin users rather than niche special interests.

Mark: I work with the Bitcoin Core codebase as well in the context of altcoins, as does @btcdrak, and neither of us advocate changing it for the sake of that work regardless of how convenient it would make it for our niche use.

[gmaxwell]

I simply want to be sure the overall plan is to continue the overall criteria for changing code in Bitcoin remains whether or not it will benefit Bitcoin users

Absolutely.

[jgarzik]

Bitcoin benefits from more typesafe, more modular code.

[petertodd]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On 7 December 2014 12:00:25 GMT+00:00, "Jorge Timón" notifications@github.com wrote:

I'm pretty sure that these "dangerous changes" to "consensus critical
code" (wallet.h, qt/guiutil.cpp and qt/receiverequestdialog.cpp no
less) actually result in an identical build.
You are right that the number of people able to review consensus code
is limited (in fact apparently the number people able to distinguish
which code is consensus-critical and what changes will actually result
in a different binary is even smaller than I thought).

Like I said, this exact pull req I was not specifically arguing against. (which is a utACK BTW) Rather the next step being advocated above as the followup - change consensus critical code by changing CAmount - is the issue.

Obviously if that hadn't been brought up I'd have said nothing at all...

As I argued in #5153, the solution is to make the code more accessible
to more people, not "never changing the critical code at all". Someone
should really write that tool for "identical-binary checking" it will
be very useful.

Please do - I'll put up 250mBTC for that.
-----BEGIN PGP SIGNATURE-----
Version: APG v1.1.1

iQFQBAEBCAA6BQJUhEsUMxxQZXRlciBUb2RkIChsb3cgc2VjdXJpdHkga2V5KSA8
cGV0ZUBwZXRlcnRvZGQub3JnPgAKCRAZnIM7qOfwhaK/B/9Atp/zh7KPP7/NNJ8d
dfBL8EiHqnyammsa4ZPq+fTGkw0ZhFuQn1Kbxr/xOSeRHtTshal0Et32WwSXRyCr
ZM3Pdnl8qzQJz4DGQQDacWAnAcOZMbP9SHCLuTFbxLWF8PoGXHuJT9s3LSmEkPo5
lwWppE13haMVPIXZY9Mw7s2Ypn2vw/2Q0wP8g8W5gmZN3dHPfQRY0ACzwz9E0nef
t9rTkMd0dqGJPAY0KTqGeQ11TxiQSuZ/y20kv98KTP0oNV0gDf1tUdwvOOWaxKt2
jMy++IDTbGyj70Dh8CiRIam81JCs3TLVBiOpLjGyx/xTUKRuPaNUkt2d1K6pZ4hT
pOIJ
=5Kg9
-----END PGP SIGNATURE-----

[sipa]

I completely agree that the criterion for inclusion in Bitcoin Core should whether it benefits Bitcoin, and should be weighed against the risks it introduces for Bitcoin. And readable, robust, auditable, transparent, and more flexible code is a benefit too.

Regarding this pull request in particular: it continues a transformation of the code to abstract the handling of monetary amounts. IMHO, low benefit, but given that it's trivial and not touching consensus code, also low risk.

Regarding changing the type of monetary amounts: that's an idea that started a long time ago (see #4067, #4234, #4614, #5117, and #5308), and has had plenty of discussion. I believe the bottom line was that changing the type would make things clearer, but a separate class wouldn't outweigh the risks. Do we need to have that discussion again? (I don't want to use the "this was discussed before, we should never ever come back to it!" argument, but it does seem very late to bring this up).

Regarding motivations and conflict of interest: you can't ask more than people being honest about why they want a change.

[btcdrak]

@petertodd @maaku Clearly these kind of changes were specifically proposed to benefit altcoins and sidechains back in April 2014 and there were objections (as well as other comments in that ticket).

Refactoring of Bitcoin Core should be beneficial to Bitcoin Core first and foremost. If it has additional benefits for 3rd party, that's great, but should not be the primary intention for it.

Someone should really write that tool for "identical-binary checking" it will be very useful.

I will also pledge 250mBTC towards such a tool.

[gmaxwell]

What purpose are you commenting here for? I'm having trouble extracting it from your message.

The stuff your're linking to was a prior approach. What was adopted was a recommendation by wumpus (and supported by many, incl. petertodd). The whole motivation for it here is making things cleaner and more explicitly typed. So I am unable to tell what view your message is intended to express.

[jtimon]

I do plan to submit more changes related to CAmount but since a class was not wanted I am precisely trying to isolate the changes that could easily be accepted (as I thought these trivial changes would be).
I believe these changes are beneficial for Bitcoin or I wouldn't propose them here. Other people might disagree and I have stepped back in the past (like not proposing a pow class anymore). About other people maintaining codebases on top of Bitcoin, I suggest them to propose their changes to Bitcoin if they think they would improve Bitcoin (by improving its modularity and type safety, for example).
But that has to be judged on a case by case basis, not on a person by person basis.
This is not proposing to turn CAmount into a class (which is something I would agree with but was rejected), and although it's moving towards the type safety that you would achieve doing so, it cannot be judged as the same thing.
But if one thinks that a change being convenient for altcoins or sidechains maintenance is irrelevant for its acceptance shouldn't be insisting so much on it as if it were a reason not to do it.
I think it's a weak advantage for this codebase but still an advantage in itself. Many people disagree. Fair enough, I'll not use it as an argument. But it is very frustrating to hear "this will benefit a dependent codebase" as an argument against a change.
It would be also interesting to understand better what was so wrong about using a class instead of a typedef. I think many of the concerns had to do just with changing many different parts of the code at once. I'm precisely separating the changes to find out what will be accepted and what not. Maintaining this tiny patch independently won't be a huge pain if it turns out that people think it doesn't benefit bitcoin/master.
So the question is only "does replacing if(CAmount) with if(CAmount != 0) make the code more unambiguous, type-safe and/or clear?".
My answer is yes to all 3 but if this is so controversial I can leave that commit for other codebases, no problem.

[btcdrak]

@gmaxwell My concerns are general - I am concerned that OP is still advocating for changes for non-Bitcoin Core purposes (i.e. for the benefit of altcoins). Relocating code for modularity aside, I worry about the amount of consensus critical code being touched without express need to do so.

I feel there is an elephant in the room, so let me address it at the risk of being wrong: I'm objecting to making changes specifically so that altcoins and sidechains maintenance is easier, as it means we must touch consensus critical code. The only reason to touch consensus code IMO is to either fix bugs or specifically change the behaviour on purpose.

I am not against altcoins or sidechains, I am against doing things to bitcoin with the express purpose to benefit them.

It's not like we're proposing a features that update consensus on purpose e.g. a new opcode for 2-way pegging (which obviously benefits altchains), if it benefits to Bitcoin's usecase. Such a proposal is a relatively small feature addition that that doesn't affect great swaths of consensus critical code with all the associated risks of introducing weird edge-case bugs in consensus; instead OP is repeatedly proposing to touch consensus critical code to assist altcoin maintenance which is not only unnecessary, but downright dangerous.

[jtimon]

I feel there is an elephant in the room, so let me address it at the risk of being wrong: I'm objecting to making changes specifically so that altcoins and sidechains maintenance is easier, as it means we must touch consensus critical code.

We all agree on that, it has to be better for Bitcoin in general. IMO not because touching consensus should be avoided at all costs but because it what makes sense. Let's stop arguing about this, please.

The only reason to touch consensus code IMO is to either fix bugs or specifically change the behaviour on purpose.

This is a very different thing. "Not doing X only because of A", is very different from "Not doing X unless it is because of B".
Improving modularity, reducing ambiguity, improving readability of code and maintenance in general are good things for Bitcoin's codebase.
We can argue in each case if the benefits outweigh the "risks" or not.
In particular, this PR does NOT touch consensus critical code and it contains trivial changes with no risks at all.
But if we want to discuss a general policy I don't think this is right place to do so.

OP is repeatedly proposing to touch consensus critical code to assist altcoin maintenance which is not only unnecessary, but downright dangerous.

If your opinion is that my contributions are unnecessary and downright dangerous I obviously disagree.
You're free to do what you can to block them one by one, but you will have to give different reasons for each different change. I'm sorry if you're "generally concerned" about my contributions but I don't think anybody should accept that as an argument against any of the tiny commits in this PR.

Please, let's talk about the PR or move to another forum like the mailing list. I hope that it doesn't become a conversation about concrete people again. We should trust good and easy-to-review changes rather than people's reputation or pgp-signed commits.
It is really disappointing that people prefer to talk about my intentions rather than being objective and unbiased about the advantages and risks of the changes themselves (which again, are not consensus critical nor dangerous).

[gmaxwell]

@btcdrak Please point to any changes you see to concensus code which you believe are misplaced. I'm still at a loss as to what you're going on about. This PR does none of the things you're talking about.

[laanwj]

Rather the next step being advocated above as the followup - change consensus critical code by changing CAmount - is the issue.

Agreed. I went along with the CAmount change in the consensus code under the following conditions:

• CAmount would be a typedef for uint64_t, not a class or anything more complex
• The binary would be unchanged. I checked this. There is AFAIK no tool for this that I know of but an exact sequence of steps I published that can be used, so that's an easy 250mBTC for anyone that can do a search

This pull is simply a continuation of that into some silly GUI and wallet code, so I don't think it should be controversial.

[jtimon]

I leave the "if(x) -> if(x !=0)" controversial part here.
The uncontroversial commit and another one that just completes #4234 (without going as far as offering #4067-like type-safety) can be found in #5464.
Both PRs/commits have been rebased on top of 0.10.

[jtimon]

Closed #5464 and put back here as suggested by @laanwj .

[laanwj]

Yes, my point is that we wrap up the CAmount-related changes here, and review and check them, to get that over with. It has become clear that no one is looking forward to having a new 'oh, we forgot CAmount in this place' pull every few days.
So - please be sure you caught them all now.

[jtimon]

I haven't found anything else. As said I plan to maintain a #4037-like patch but I'm not there yet. Here's what I have so far: https://github.com/jtimon/bitcoin/compare/noamount
If you think I should get anything else from there I'm happy to do so.

Maybe "Use FormatMoney() in CTxOut::ToString()"? Although, honestly I would just show Satoshis there instead of Bitcoins.
I didn't have the courage to propose the AmountToString() and AmountToDouble() functions...

[jtimon]

Needs rebase but...closing.