[Qt] Remove CAmount from BitcoinAmountField Q_PROPERTY #5117

Fixes a bug, currently the fee field in the optionsdialog is always empty.

Seems to be this change was made, when CAmount was introduced, nice you catched that.

NACK. This solution is hacky because CAmount is not guaranteed to be a uint64_t, and the type of setValue is actually a CAmount.

That's weird actually. Why doesn't CAmount work here? We do register the type with Qt, here https://github.com/bitcoin/bitcoin/blob/master/src/qt/bitcoin.cpp#L516

I cannot reproduce it here either. The fee field works fine w/ Qt 5.2.1. What version of Qt are you building with?

Agreed on the hack, but I dont know better. It works under qt5.3 for me too. Problem exists with 4.8. Do we still build linux-releases with qt4?

Yes, we do, so I guess we need to find a solution for qt4 (even if we wouldn't build with it ourselves anymore we should support it). In the worst case we could make this an #ifdef ... but I wonder if I'm not just doing something wrong in the registration?

I tried playing around with the MetaType-Registration, does not work. Updated now to an #if.

I don't see any difference on OSX with 4.8.6. Seems to work fine without the hack. Could you describe exactly what's fixed? I'm assuming I'd know the problem if I saw it.

What is the build/runtime environment you're using?

I compile archlinux, but I also tested compiling a release in virtualbox and then tested the resulting linux executable in a different linux virtualbox, same bug. Dont know about windows or osx.

The field is always empty when opening the dialog. You can enter a value and its stored correctly internally, but if you reopen the dialog its empty again.

Ok, I've reproduced in linux with qt4.8. I went pretty far down the rabbit hole and I believe I've found the core issue, but I can only wildly speculate as to the reason.

linux64: typedef long int int64_t; osx64: typedef long long int64_t; linux32: typedef long long int int64_t; win64: typedef long long int64_t; win32: typedef long long int64_t;

Only linux64 defines int64_t as a long. I would guess that everywhere else is working fine.

As a test, changing the typedef for CAmount from "int64_t" to "long long" (and fixing up a few casualties like json) allows the fee field to work again. Change it to "long" instead, which is the same size, and it breaks. So it seems as though what's in master will be broken on systems running qt4 where int64_t != long long.

<wild speculation>

My best guess from looking at the headers is that qt's variant in qt4 is that they use some rtti to determine convertible user-defined types, but they don't seem to ever store longs, they always store long long's (qint64 is also long long):

qt4:
...
            float f;
            qreal real;
            qlonglong ll;
            qulonglong ull;
            QObject *o;
            void *ptr;

qt5:
...
            long l;
            ulong ul;
            bool b;
            double d;
            float f;
            qreal real;
            qlonglong ll;
            qulonglong ull;

So when it attempts to convert out of the variant to our "long", it sees typeid's that don't match, refuses to convert, and sets the valid_out in AmountSpinBox::value to false.

</wild speculation>

I'm not sure if that's any help, since I can't explain exactly what the problem is, but it's definitely more than just a matter of qt4/qt5.

@theuni I dived into another part of the rabbit hole last night. The issue is twofold:

1. QDataWidgetMapper doesn't seem to accept the custom property types. No matter what, it doesn't seem to be able to pass a user type like CAmount from the model's data to the widget property. It just treats the result as invalid.
2. In OptionsModel::data with FEE we actually return a QVariant(qint64), not a QVariant.setValue(CAmount), so it's a wonder this works at all, and Qt5 finds a way to map this to the CAmount property type. To return the proper type it should be

        case Fee: {
            // Attention: Init() is called before payTxFee is set in AppInit2()!
            // To ensure we can change the fee on-the-fly update our QSetting when
            // opening OptionsDialog, which queries Fee via the mapper.
            if (!(payTxFee == CFeeRate(settings.value("nTransactionFee").toLongLong(), 1000)))
                settings.setValue("nTransactionFee", (qint64)payTxFee.GetFeePerK());
            // Todo: Consider to revert back to use just payTxFee here, if we don't want
            // -paytxfee to update our QSettings!
            QVariant v;
            CAmount amount = CAmount(settings.value("nTransactionFee").toLongLong());
            v.setValue(amount);
            return v; // v.type() will now be the user type for CAmount
        }

(a symmetrical change would be needed in setData, which still expects a qint64!)

(2) is easy to fix, but it doesn't make it any better, and breaks cozz's fix too, as it will have no idea how to convert a QVariant:CAmount to a qint64 property. (1) is more tricky.

A difference between Qt4 and Qt5 here seems to be that on Qt4, an actual UserType is registered for CAmount (which gets ID 129 or so). Qt5 understands it to be just a typedef (and gets ID 32:long / 4:longlong). This explains why it 'just works' on Qt5.

In retrospect I'd say @cozz's solution is the best for now, both for Qt4 and Qt5. It will break on compilation of the generated moc in case BitcoinAmountField::setValue() no longer takes a qint64, but that's a problem for altcoins where CAmount is not a qint64, not for us. Let's just add a comment referring to this issue why we did it this way.

... but then I realized something I should have realized before charging into rabbit holes : do we still need this setting at all after the 'smart fee' change, or do we need to deal with fee completely different now?

At least we now have a new an extra setting:

• -txconfirmtarget=n : create transactions that have enough fees (or priority) so they are likely to confirm within n blocks (default: 1). This setting is over-ridden by the -paytxfee option.

For the GUI maybe this should not be global at all, it would be nice to allow people to interactively determine their fee versus confirm target preference while sending the transaction.

@laanwj I started down the same path as well. I began by using QVariant::fromValue<CAmount>() and value.value<CAmount>() as necessary, but I never had any luck that way. I assumed that If I treated it as though it were an opaque class that wasn't compatible to an integral type I'd eventually be able to get the same "class" back out by doing all variant operations manually and in reverse, but I had no such luck. Sounds like your first point is probably the cause there.

Edit: Grr, github ate my tags. That was QVariant::fromValue<CAmount>() and value.value<CAmount>()

Updated now for both qt4/5 and linked to this issue.

I also suspect that because there is a weird difference between long and long long the mapping does not work. @laanwj About smartfee, I thought the same and was really trying to come up with something to bring -txconfirmtarget to the GUI, but I didnt. See full thoughts here: #4866