Remove the undocumented, unused array type from the portable storage specification

[kayabaNerve]

This is currently unused, so it shouldn't be an issue to remove. It also likely isn't safe to adopt as it has unclear, potentially not actually implemented, behavior. Removing it now reduces the scope of the codec and prevents introducing usage. This not only promotes some ideal of safety (highly debatable if this is actually safer or just a nit) but makes it easier to safely reimplement the format (as seen in several PRs over the years due to the memory consumption of the original implementation).

Pleasantly removes the of make_recursive_variant, which is documented to not be portable. While obviously, it's sufficiently portable that hasn't been an issue, it's nice to note.

I removed all references to the constant and updated the code to compile. There may be some functions that are now unreachable and can also be removed. I did not personally run the full test suite and presume the CI will handle that.

cc @jeffro256 as the person I primarily want to poke for review of this.

[vtnerd]

This should prevent array of arrays, which is annoying but easy enough to workaround. Afaik, only ZeroMQ has vector<vector<...>>, but that uses a separate serialization mechanism.

The test suite for my serialization replacement actually tests for this case, and verifies that epee array of arrays does indeed write+read correctly.

[vtnerd]

As a quick note - I may have to add this removed flag back in that serialization PR. The problem is that the interface (in my serialization PR) doesn't explicitly block array of arrays (because ZeroMQ already uses it), so we should need a way to indicate vector<vector<...>> in the epee format.

[kayabaNerve]

EPEE already doesn't support array of arrays:

monero/contrib/epee/include/storages/portable_storage_from_json.h

Line 191 in 8e9ab96

ASSERT_MES_AND_THROW("array of array not suppoerted yet :( sorry");

I'd say this is just further justification for remove it so that it's not present as a potential.

If your serialization PR does restore it, we'd also need it properly implemented, completely. As for Monero currently represents vector<vector<...>>, the best reference is the output indexes within get_blocks.bin, a struct of the form { "output_indices": [ {"indices": [ { "indices": [...] } ] } ] } to represent Blocks(Block(Transaction)).

I'd also argue against the complexity of variably-typed arrays of arrays moving forward (as this definition would have assigned each sub-array its own type, right?).

[vtnerd]

Yes, vector<vector<...>> is unimplemented for json and epee readers, but I recall it working for the json and epee writers (been a while since I wrote that PR). In the epee writer, each sub-array has a new type+length tag. So technically there is a reference implementation, and the scheme works.

If your serialization PR does restore it, we'd also need it properly implemented, completely. As for Monero currently represents vector<vector<...>>, the best reference is the output indexes within get_blocks.bin, a struct of the form { "output_indices": [ {"indices": [ { "indices": [...] } ] } ] } to represent Blocks(Block(Transaction)).

The new serialization PR has tests for json and epee vector<vector<vector<uint64>>>, with different array lengths for internal/sub arrays to ensure they can be properly stored+read. The checks pass for epee+json readers+writers, with a msgpack implementation also written (in LWS), but not listed in the Monero PR.

I'd also argue against the complexity of variably-typed arrays of arrays moving forward (as this definition would have assigned each sub-array its own type, right?).

It was a little tricky to implement, but not too bad. Certainly not worth banning permanently. I would just write the code for it in Rust (assuming thats what prompted this PR), and be done with it. Msgpack is loads easier in this situation as everything requires a tag regardless of situation.

[vtnerd]

The alternative is just ban vector<vector<...>> in my serialization PR, but seems kind of a waste to do that (since its way more efficient to use that syntax).

[kayabaNerve]

I'd hesitate to call it a reference implementation if reading isn't implemented 😅

Not re: Rust alone. epee is incredibly frustrating to work with externally. If this is to be specified (as in the current documentation) and properly implemented, I'll manage. If this is a half-baked implementation which needs development (implementation of the reader), testing, and review, and is unnecessary surface so we can take the chance to shrink epee, I'd rather that.

If your upcoming PR has already put in all the work, and this will be used, I'm fine dropping this PR. I primarily wanted this out of its halfway state and didn't want to implement this myself.

Distinct question of if this PR should be merged pending yours however.

[vtnerd]

I'd hesitate to call it a reference implementation if reading isn't implemented 😅

Agreed it's questionable - without a reader the whole scheme could've been broken in that situation. It appears they got it correct on the writer side.

Not re: Rust alone. epee is incredibly frustrating to work with externally. If this is to be specified (as in the current documentation) and properly implemented, I'll manage. If this is a half-baked implementation which needs development (implementation of the reader), testing, and review, and is unnecessary surface so we can take the chance to shrink epee, I'd rather that.

Agreed, I'd prefer Msgpack or CBOR. Either would be easier on everyone, and fast enough.

The goal of the new serialization was to eventually introduce Msgpack to ZeroMQ RPC+PUB and HTTP RPC. There's lots of internal type changes to make it all work seamless, but it's doable. The P2P layer would be converted/merged to the new system last, as I'm more hesitant to unleash the new epee reader on such critical traffic.

If your upcoming PR has already put in all the work, and this will be used, I'm fine dropping this PR. I primarily wanted this out of its halfway state and didn't want to implement this myself.

Yes, although given how that PR has stalled it might be worth moving forward with this one. The only problem is that I would have to drop it or re-introduce it, not sure which is better.

[kayabaNerve]

This PR is ~10 lines. Seems easy enough to rebase your PR with the first added commit being "Revert removal of array type"? 😅

But I won't pressure you here. I trust and defer to your judgement :)

Regarding another format, I now have sunk cost on EPEE. How dare you suggest the future be better for other people after my pain in the present? /s

Would love to see a properly specified protocol used, with existing implementations, so long as it isn't rolled out too quickly and doesn't break current clients of course. But yeah, def a lot of issues with the status quo...

We can also open the question of if EPEE should have this limitation and new RPC types (completely breaking) should migrate to msgpack (or whatever) with vector<vector<_>>. I won't be so annoying as to demand such separation however. Just whatever makes sense to you.

[jeffro256]

I definitely would like to support arrays of arrays as a method for off-chain serialization at some point in the future. Though I'm pretty sure @vtnerd's serialization suite wipes out all of this DOM code (the array_entry_t, array_entry, storage_entry, section types, etc) anyways. We also know that arrays of arrays are currently not allowed, period. As @vtnerd mentioned, arrays of arrays are not out of the question for future p2p protocols. If we remove this bit now, I figure the only benefit is temporary readability; future alternative implementations of epee might need to support arrays of arrays. I guess I'm not opposed to the change though.