Add static_assert to prevent VARINT(<signed value>) #9753

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.

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

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.

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

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

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 https://github.com/theuni/bitcoin/commit/9829c54de2725037ee0702306cbaa99fc9aa1826 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.");
    }
};

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

Squashed dd094d6c0aede5d9e4e22caa441cdacbc95beede -> d1df497083df4116ae3bf2915cea0da6a13fa274 (varint-assert.1 -> varint-assert.2)

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

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)

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

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.

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)

@laanwj. Implemented your suggestion in af20ce5a90aadd56d504291c754e0c60ca1f8788. 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.

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.

Please let me know if af20ce5a90aadd56d504291c754e0c60ca1f8788 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.

Still need feedback on whether to merge af20ce5a90aadd56d504291c754e0c60ca1f8788 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.

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.

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

Rebased 8950418e538d02c2eb8b0ea76b9838e867e91feb -> 46e649b8ff6e7a66957fc269a04ef4117bd34d7a (pr/varint-assert.3 -> pr/varint-assert.4) due to conflict with #10195 Rebased 46e649b8ff6e7a66957fc269a04ef4117bd34d7a -> 649bfb00bfb738df1b2c41c3d0c982f430c85495 (pr/varint-assert.4 -> pr/varint-assert.5) due to conflict with #10760 Rebased 649bfb00bfb738df1b2c41c3d0c982f430c85495 -> 499d95e278f34790660a2b9baf5525e0def1485a (pr/varint-assert.5 -> pr/varint-assert.6) due to conflict with #12683

utACK 499d95e278f34790660a2b9baf5525e0def1485a