Update xdrpp to support non-option cyclic refs for new cyclic refs

[leighmcculloch]

Description

Explain in detail the additional functionality you would like to see in stellar-core.

Stop using optional-data (*) feature of XDR for cyclic references, and update our use of xdrpp so that we can support cyclic references without adding the 4-byte optional-data discriminant.

Explain why this feature is important

The XDR optional-data (*) is shorthand for a union and introduces a 4-byte prefix to any type/value that uses it.

Most of the time when we're introducing the optional-data onto a data type it is to leverage the side-effect that the C++ generated code will cause the value to be a pointer, which moves the value to be outline, so that the value does not have infinite size.

We aren't actually interested in the value being optional.

A side-effect is the XDR contains an extra 4-bytes of data that is wasted ledger space.

A side-effect is that the XDR definition communicates with every XDR parser / client that the field is optional, and clients must handle that optional case. In the Rust XDR lib this introduces an Option<> which introduces unnecessary branches and failure paths that shouldn't logically exist.

Describe the solution you'd like

xdrpp adds support for non-optional-data cyclic references with the introduction of -uptr that makes all union arms pointers anyway. We could update to that. @stanford-scs describes the solution in https://groups.google.com/g/stellar-dev/c/X0oRzJoIr10/m/v6TKvjvUAgAJ.

Describe alternatives you've considered

We had discussed removing some unnecessary cyclic references in the contract XDR as a way to get away from these cyclic reference issues, however @graydon pointed out yesterday in Discord that we can't remove them all, and we need the cyclic references. I'm opening this issue to track this problem now that we know there isn't this way around it.

Additional context

Some prior discussion in this thread starting at https://groups.google.com/g/stellar-dev/c/X0oRzJoIr10/m/2MclmECiAgAJ.

Note that using non-optional-data cyclic references is not going to cause significant downstream issues, because XDR libs in other languages already either treat all structured values as outline (Java), or treat most values where a cyclic ref would turn up as outline (Go), or can detect cyclic refs and make just cyclic refs outline (Rust).

[MonsieurNicolas]

Can you add an xdr example of what you're trying to do?

Fishing around in the above links, it looks like it's when there is already a discriminant outside:

    union SCVal switch (SCValType type)
    {
    ...
    case SCV_OBJECT:
        SCObject* obj;
    ... 
    };

In any case, this issue probably belongs to the xdrpp repo. I am not sure which part of the xdr spec specifies this.
I think maybe https://datatracker.ietf.org/doc/html/rfc4506.html#section-4.19 as there is this example in there:

         union stringlist switch (bool opted) {
         case TRUE:
            struct {
               string item<>;
               stringlist next;
            } element;
         case FALSE:
            void;
         };

As for the proposed solution

xdrpp adds support for non-optional-data cyclic references with the introduction of -uptr that makes all union arms pointers anyway. We could update to that. @stanford-scs describes the solution in https://groups.google.com/g/stellar-dev/c/X0oRzJoIr10/m/v6TKvjvUAgAJ.

it's unclear if we'd move to this version at this time (uptr did not yield memory savings that we were hoping for, while adding overhead, and C++20 seems to go crazy on symbol size).

[leighmcculloch]

Can you add an xdr example of what you're trying to do?

Fishing around in the above links, it looks like it's when there is already a discriminant outside:

    union SCVal switch (SCValType type)
    {
    ...
    case SCV_OBJECT:
        SCObject* obj;
    ... 
    };

Yes, whenever there is already a discriminant that prevents infinitely sized type there's no need to introduce yet another discriminant on the cyclic reference. I'd like the example you demonstrated to become:

    union SCVal switch (SCValType type)
    {
    ...
    case SCV_OBJECT:
        SCObject obj;
    ... 
    };

xdrpp adds support for non-optional-data cyclic references with the introduction of -uptr that makes all union arms pointers anyway. We could update to that. @stanford-scs describes the solution in https://groups.google.com/g/stellar-dev/c/X0oRzJoIr10/m/v6TKvjvUAgAJ.

it's unclear if we'd move to this version at this time (uptr did not yield memory savings that we were hoping for, while adding overhead, and C++20 seems to go crazy on symbol size).

@stanford-scs mentioned -uptr could be backported to C++17. @stanford-scs ?

@stanford-scs did also mention another solution that he described as adaptive. This is probably similar to how the Rust generator works where it identifies cyclic fields on types and outlines only those fields.

[MonsieurNicolas]

Let's not backport -uptr to C++17 just yet. We didn't (could not? @graydon ) do a proper eval of the C++20 version, and I don't know if it's what we need or if it's something more complicated like doing it only on certain fields (which we could do based off annotations btw if we can't automate the detection).

[graydon]

@leighmcculloch wait, is this even possible in the rust side? If you generate rust code for a recursive type without Box, it has to go through Option<> as well, no?

[leighmcculloch]

Xdrgen identifies cyclic paths and inserts a Box in the Rust generated code.