Add static_assert to prevent VARINT(<signed value>)

[ryanofsky]

Using VARINT with signed types is dangerous because negative values will appear to serialize correctly, but then deserialize as positive values mod 128.

This commit changes the VARINT macro to trigger a compile error by default if called with an signed value, and it updates existing broken uses of VARINT to pass a special flag that lets them keep working with no changes in behavior.

There is some discussion about this issue here: #9693 (comment). I think another good change along these lines would be to make GetSizeOfVarInt and WriteVarInt throw exceptions if they are passed numbers less than 0 to serialize. But unlike this change, that would be a change in runtime behavior, and need more consideration.

[fanquake]

Failed on OSX:

qt/qt_bitcoin_qt-bitcoin.o `test -f 'qt/bitcoin.cpp' || echo './'`qt/bitcoin.cpp
In file included from qt/bitcoin.cpp:9:
In file included from ./qt/bitcoingui.h:12:
In file included from ./amount.h:9:
./serialize.h:311:16: error: constexpr function's return type 'void' is not a literal type
constexpr void CheckVarIntMode()
               ^
1 error generated.
make[1]: *** [qt/qt_bitcoin_qt-bitcoin.o] Error 1

[maflcko]

constexpr function's return type 'void' is not a literal type

If this is c++14 code, I wonder why the other builds succeeded, given that we are at c++11.

[CryptAxe]

Are the other systems using g++ > version 5?

[laanwj]

Do we explicitly request compilation for stdc++11? Would make sense to do so, as the default can (and has) changed.

[theuni]

This is a weird one.

g++ accepts constexpr void foo() even when using -std=c++11 -pedantic -Wall -Wextra. I assume that's a bug, but I'm not certain without looking at the language spec itself. That's the case in 4.8-5.1, anyway.

Not much we can do about that, as far as I can tell. Nothing I tried made g++ complain about it.

@laanwj I agree. It turned out to be unrelated to this issue, but I whipped up theuni@9829c54 anyway. Will PR it.

@ryanofsky You can just use a struct with a constexpr ctor instead:

template<VarIntMode Mode, typename I>
struct CheckVarIntMode
{
    constexpr CheckVarIntMode() {
        static_assert(Mode != VarIntMode::DEFAULT || std::is_unsigned<I>::value, "Unsigned type required with mode DEFAULT.");
        static_assert(Mode != VarIntMode::BROKEN_SIGNED || std::is_signed<I>::value, "Signed type required with mode BROKEN_SIGNED.");
    }
};

[ryanofsky]

Thanks @theuni, that's a clever workaround. Added in dd094d6.

Squashed dd094d6 -> d1df497 (varint-assert.1 -> varint-assert.2)

[ryanofsky]

Rebased d1df497 -> 8950418 (pr/varint-assert.2 -> pr/varint-assert.3) because of conflict with renamed nVersion variable in #8808.

[theuni]

Concept ACK. Though i'd prefer to just introduce a new macro rather than a default arg:
VARINT(foo, VarIntMode::NONNEGATIVE_SIGNED) -> VARINT_NONNEGATIVE_SIGNED(foo)

[laanwj]

This seems to be in limbo - @ryanofsky can you please reply to @theuni's comment?

[ryanofsky]

Though i'd prefer to just introduce a new macro rather than a default arg

I guess I prefer the opposite. I think it's good if you can look at code like:

ss << VARINT(i);
ss << VARINT(i, VarIntMode::DEFAULT);
ss << VARINT(i, VarIntMode::NONNEGATIVE_SIGNED);

and know that all 3 encodings of i are going to use the same underlying serialization machinery, and that you can understand the differences just by looking up the VarIntMode enum, skipping all the macro definition goop.

An interface like:

ss << VARINT(i);
ss << VARINT_DEFAULT(i);
ss << VARINT_NONNEGATIVE_SIGNED(i);

just seems more opaque. The implementation would also be a little longer and more duplicative.

[laanwj]

ss << VARINT(i);
ss << VARINT(i, VarIntMode::DEFAULT);
ss << VARINT(i, VarIntMode::NONNEGATIVE_SIGNED);

FWIW I don't like the repetition of VarInt here, the VarIntMode:: could be inside the macro instead of outside it to just have

ss << VARINT(i);
ss << VARINT(i, DEFAULT);
ss << VARINT(i, NONNEGATIVE_SIGNED);

But I wouldn't mind explicit VARUINT_U and VARINT_I or even VARUINT and VARSINT either...
(there's no need to make things longer and longer and more verbose)

[ryanofsky]

@laanwj. Implemented your suggestion in af20ce5. Let me know if you think I should squash it.

As mentioned before though, I prefer current change because VARINT() is a simple, dumb, single macro and regular VARINT(x) cases work exactly the same as before. The only verbose case is the VarIntMode::NONNEGATIVE_SIGNED one and IMO that is verbose in a good way, because if you are reading the code you can tell that VarIntMode::NONNEGATIVE_SIGNED is just a plain c++ value, not more macro magic goop, and you can click "VarIntMode" in your IDE to go to the definition of the enum, and see documentation and details about the encoding.

[laanwj]

Thanks.

As mentioned before though, I prefer current change because VARINT() is a simple, dumb, single macro and regular VARINT(x) cases work exactly the same as before.

I agree in general, but for this specific case this just seems overdone.
We don't ever expect more varintmodes than those two, so going all the way with specifying a long enum on every single use seems overkill, especially as we're already using a macro.
But that's just my opinion.

[ryanofsky]

Please let me know if af20ce5 is ok and does what you want then. I think both the implementation and the resulting interface are gross, but if this is actually what you want, I'll squash it.

[ryanofsky]

Still need feedback on whether to merge af20ce5 into this PR. I don't love the macro magic it adds, and I don't see much benefit in the change since only weird legacy code should ever need to use NONNEGATIVE_SIGNED (normal code should just use VARINT(value)). But I'm fine with it if it would help move things forward.

[theuni]

I would still prefer a separate macro, so that makes 3 differing opinions here :)
But as they all do the same thing, and we could argue about syntactic sugar until the end of time, utACK to either approach here.

[laanwj]

I would still prefer a separate macro, so that makes 3 differing opinions here :)

Seperate macros was also one of my preferences, FWIW:

But I wouldn't mind explicit VARUINT_U and VARINT_I or even VARUINT and VARSINT either...
(there's no need to make things longer and longer and more verbose)

I just don't like the overly verbose construction of passing a scoped enum into everything.

Edit: then again, I'm also not intending to hold this up indefinitely based on that, so utACK

[ryanofsky]

Rebased 8950418 -> 46e649b (pr/varint-assert.3 -> pr/varint-assert.4) due to conflict with #10195
Rebased 46e649b -> 649bfb0 (pr/varint-assert.4 -> pr/varint-assert.5) due to conflict with #10760
Rebased 649bfb0 -> 499d95e (pr/varint-assert.5 -> pr/varint-assert.6) due to conflict with #12683

[sipa]

utACK 499d95e