Fix access specifiers in gadget inheritance tree

[AntoineRondelet]

See: #394

As mentioned in the ticket linked above, the gadgets inheritances were inconsistent. Some gadgets like joinsplit or COMM_gadget had a private inheritance from libsnark::gadget and all others inherited publicly. So far the libsnark::gadget class is very small. Only one constructor (public) and 2 protected attributes. As such, public inheritance is not an issue. I think that this "public scopes by default" behavior is not desired. Instead, we should be strict with scopes and group attributes/methods by access specifier to control the interface of the classes in order to expose the strict necessary in the public interface of each class, and we should use private (or protected if needed) scopes by default and should modify this default behavior when using public scopes is justified. Likewise, we should specify scopes and access specifiers explicitly to crystalize the implementer's intention, instead of relying on default behavior. This allows to keep a good control over the children classes interfaces and forces to think about interfaces/API.
[EDIT:] Edited message further to discussions below.

[AntoineRondelet]

Also, most gadgets in libsnark (in gadgetlib1) inherit from gadget publicly, but there are inconsistencies, such as https://github.com/clearmatics/libsnark/blob/master/libsnark/gadgetlib1/gadgets/fields/exponentiation_gadget.hpp#L31 which follows the default inheritance behavior which is private in C++.

[dtebbs]

Regarding consistency and explicitly specifying inheritence type, this makes sense to me (although it is a shame that this cannot be checked autmatically AFAIK).

Regarding the type of inheritance, on reflection I'm a bit uneasy about changing it. Partly because libsnark::gadget members are already protected, and all libsnark gadgets use public inheritance, it's not clear to me that private fits with the original intention of libsnark. Two things concern me a bit.

a) libsnark::gadget currently has no interface so nothing is hiddn by using private. If, in the future, gadget did ever publicly expose methods, this would be with the intention that they are available on all gadgets. So it seems that visibility of methods and members on gadget is the right way of controlling their visibility on the gadget implementations.

b) Using private inheritance also breaks the "is a" relationship, which I'm also a bit concerned about.

So I wonder if we should just sort out the consistency and explicit visibility part in this PR, keep public inheritance for now and reflect on this a bit more. Let me know what you think.

[AntoineRondelet]

b) Using private inheritance also breaks the "is a" relationship, which I'm also a bit concerned about.

Good point.

So I wonder if we should just sort out the consistency and explicit visibility part in this PR, keep public inheritance for now and reflect on this a bit more. Let me know what you think.

Yes, I did this (see last commit). Happy to keep it public for now. Note, as flagged in my previous messages, that nothing really changes much if we switch to another type of inheritance (if we use protected, the protected members of the mother class (no public members/attributes beyond the constructor) remain protected, and if we use private, these become private, but we have no "friend" function that could have been broken.). The point you made about "is a" vs "has a" is important though. We'll need to fix a bunch of inconsistencies on the inheritance of gadgets in the libsnark code too to have a uniform behavior.

[dtebbs]

LGTM

[dtebbs]

@AntoineRondelet as discusses, I checked some of the classes in hash_stream.hpp. AFAICT, public seems to be technically correct. I pushed the change this branch - hope that doesn't conflict with any local changes on your end.

[AntoineRondelet]

@AntoineRondelet as discusses, I checked some of the classes in hash_stream.hpp. AFAICT, public seems to be technically correct. I pushed the change this branch - hope that doesn't conflict with any local changes on your end.

Thanks for having a look at these @dtebbs, no problem, don't have any pending changes here, so happy to merge.