ACK.

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

Looks good to me

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.

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)

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.

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.

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

That was the "alas".

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?

@maaku Ubuntu 14.04 64-bit

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

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.

Yes please.

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

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.

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

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

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

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

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

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

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.

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

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

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.

Untested ACK (reviewed with above word diff command).

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.

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.

ACK. Verified type replacement + headers changes only.

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

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

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

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.

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

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

@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?

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.

@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?

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.

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.

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 laanwj@c122f55

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.

Merged via 3fd192f