Opaque rt

[mvandeberg]

Closes open-orchard/koinos#235

Implement and test Opaque rt type and multihash basetype changes

[theoreticalbts]

Why is signature_data a variable_blob? Why not make it opaque< sig_block_data > / opaque< sig_transaction_data >?

[theoreticalbts]

active_block_data loses transactions and extensions fields. Is this intentional?

[theoreticalbts]

Todo list:

• Replace const with mutable
• Pass depth in from_binary for opaque
• When deserializing opaque from JSON, choose serialization based on whether JSON is string
• In opaque fields should start with _
• unbox() should throw an exception rather than return value
• Refactor locked to be method rather than field
• TODO: Review opaque type after refactor
• Remove #include <iostream> from varint.hpp

[mvandeberg]

Ready for re-review

[theoreticalbts]

I went ahead and added a state transition diagram as a comment. There are lots of cases in opaque<T> that have potential problems. Let's figure out some rules for addressing them.

I suggest the following:

• (1) Arrows specified in the diagram do what the diagram says.
• (2) An arrow that ends up at the current state should be a no-op (i.e. arrows are idempotent)
• (3) If you see an arrow that doesn't touch your box, throw an exception.

In the current code, rule (1) (the normal case) seems to work everywhere.

Rule (2) seems to work everywhere except calling lock() on Unboxed, where there's an unnecessary call to serialize().

Rule (3) is undefined-ish behavior in the current code: In two cases we put it into the "illegal" !_native && !_blob state, in the third state we dereference the optional.

[theoreticalbts]

Now that I look further into the code, I'm not sure the naming of the states in my comment matches up completely with the nomenclature in the code. For example the is_unboxed() function will return _native.has_value(), which will be true in both the Unboxed and Unlocked state. So we should probably rename either the is_unboxed() method or the Unboxed state.

[theoreticalbts]

I'm not a fan of defining operator variable_blob&. This apparently is C++ syntax which says if x is an opaque<T>, you can say variable_blob( x ) and it will invokes a method on the opaque class in defiance of all logic. That is opaque::to_variable_blob( x ) or x.to_variable_blob().

[theoreticalbts]

The get_blob() function returns a reference to the internal _blob member, which can be cleared by state changes. One of the following three mitigations should be deployed:

• (1) Add a comment admonishing the caller that the returned reference will be invalidated by changing the state or having the opaque go out of scope.
• (2) Change the return type to variable_blob (no reference)
• (3) Have the caller pass in a variable_blob for the data to be copied into

This also applies to get_native(), get_const_native(), get_native(), and the equality overrides (how many of those are there!?)

[theoreticalbts]

I'm not totally comfortable with the overridden assignment operator returning something other than *this. Are we sure the overridden assignment operator is correct?

[theoreticalbts]

I think this code may be actually become more readable if we get rid of is_unboxed() and is_locked() methods entirely. This solves the problem of conflicting with the names I picked for the states, but the main motivation is actually that if you see something like:

         if( !is_unboxed() ) throw pack::opaque_not_unboxed( "Opaque type not unboxed." );
         return &(*_native);

you have to think for a moment if you want is_unboxed() or is_unlocked() or maybe something else entirely. But it's quite clear if you instead say:

         if( !_native.has_value() ) throw pack::opaque_not_unboxed( "Opaque type not unboxed." );
         return &(*_native);

Then you can immediately see this is good defensive programming, you're checking the optional you're dereferencing before you dereference it.

[theoreticalbts]

We should probably unit test all 12 state transition cases. And also things like a = b = c where c is a variable_blob, if you insist on overriding the assignment operator.