Add C++ wrapper class in minisketch.h header

[sipa]

This provides a safer API for use within C++ code. It's just a headers-only wrapper around the C API, so it does not introduce any further binary compatibility concerns.

[naumenkogs]

Great, thanks!

[naumenkogs]

Doesn't compile for me:

In file included from src/test-exhaust.cpp:7:
src/../include/minisketch.h:272:9: error: missing 'typename' prior to dependent type name 'std::enable_if<std::is_convertible<typename
      std::remove_pointer<decltype(obj.data())>::type (*)[], const unsigned char (*)[]>::value && std::is_convertible<decltype(obj.size()),
      std::size_t>::value, std::nullptr_t>::type'
        std::enable_if<
        ^~~~~~~~~~~~~~~
        typename 
1 error generated.

[sipa]

@naumenkogs Oops, forgot to test that function. Fixed.

[naumenkogs]

How to safely use it? I remember minisketch_create could return nullptr in some cases.

[sipa]

I added an operator bool.

So you can use

Minisketch minisketch = Minisketch::CreateFP(...);
if (!minisketch) { ... }

[naumenkogs]

Why does this have to return anything if it's called on a sketch instance?

[sipa]

It's so that you can chain operations. E.g. auto vec = Minisketch::CreateFP(...).Deserialize(v).Add(x).Add(y).Merge(z).Serialize();

[naumenkogs]

I assume this is going to be safe even if the instance evaluates to false due to implementation issue nullptr?

[sipa]

I added a Minisketch::SupportedImplementation() to detect ahead of time which configurations are legal. You shouldn't call Minisketch::CreateFP() or the Minisketch constructor for unsupported combinations.

[jnewbery]

Strong concept ACK. Using an RAII object as the interface is far safer.

[jnewbery]

must have a size equal to the maximum allowed elements

This doesn't seem exactly true. I think minisketch_decode will return -1 if result.size() is too small, but it's allowed to be larger (and will be shrunk at the end by result.resize(). Perhaps the comment should be "must have a size at least as large as the maximum allowed elements"?

For my own understanding: is this requirement to avoid having to reallocate the vector, which would mean this would no longer be noexcept?

[sipa]

This doesn't seem exactly true. I think minisketch_decode will return -1 if result.size() is too small, but it's allowed to be larger (and will be shrunk at the end by result.resize()

Yes, that's correct. But I don't see how that's in conflict with the comment.

Perhaps the comment should be "must have a size at least as large as the maximum allowed elements"?

No, that'd be wrong. If you make it larger than the maximum number of elements you want, you may get more.

For my own understanding: is this requirement to avoid having to reallocate the vector, which would mean this would no longer be noexcept?

No, just to avoid redundancy. The size of what you want is already passed by the size of the vector you provide, no need to provide it a second time.

[sipa]

Oh, I think I see your confusion. It is intended to mean "You should pass in a vector with a size equal to the maximum number of elements you the caller allow it to return". "Allowed" is maybe not the best term here?

[jnewbery]

Sorry, yes I was confused by who was allowing who! I'd usually expect the interface to document what the client is allowed to do.

I don't have a great suggestion for changing this. Perhaps something like "Decode up to result.size() elements from the sketch. Will fail if result is not large enough to hold all sketch elements." but I'm not sure if that captures everything.

[sipa]

I've changed the comments a bit.

[naumenkogs]

I'm not an expert on C++ wrappers, but I've been using this new code for weeks now in Erlay, and the API seems correct.
Also, I have reviewed the code.
ACK.

[jnewbery]

I've never seen preprocessor directives nested like this (and didn't realise that whitespace was permitted between the # and the directive). This makes it so much clearer to see the scope of the #ifs. Really nice 👍

[sipa]

We do have a few examples in the Bitcoin Core codebase, try:

git grep '^# ' $(find -name '*.[ch]' -o -name '*.cpp')

[jnewbery]

Light review ACK a570de2

[jnewbery]

New comments are clearer to me. Thanks!