split inner to owned and ref variants

[Shourya742]

closes: #2180

companion stratum-mining/sv2-apps#616

[plebhash]

recently @Shourya742 asked this over Discord:

At a high level, we're moving away from the old Inner enum, which previously had both Ref and Owned variants. We still have Inner, but now it only represents the Ref variant, and we've introduced InnerOwned to represent the Owned variant. This is purely an internal implementation detail of binary_sv2. Inner can be converted into the owned variant, similar to how the old Static variant worked. That's the main idea behind all the changes in binary_sv2.

In parsers_sv2 and the protocol-specific message crates, we now support both the owned and the previous reference-based variants with lifetimes, along with conversions from the reference type to the owned type. The handlers also have owned variants now. So far, all of that feels good.

The part I'm unsure about is channels_sv2, since we need to be careful about the API we expose to users.

My initial thought was that if channels_sv2 needs to store a value, it should require the owned variant; otherwise, it can just work with the reference-based type. The catch is that, in our sv2-apps implementation, we almost always convert reference types to 'static so they can be used in async Rust. So while the public API could be generic over reference-based types, internally we immediately turn them into 'static anyway.

That feels like an implementation detail driven by our heavy use of async Rust, but as a library we probably shouldn't be opinionated about how users structure their code.

Right now, I've made everything in channels_sv2 use the owned types, essentially saying, "these types own their memory, so the library is free to store them or do whatever it needs." I'm not sure if that's the right direction, though.

The other option would be a hybrid approach where channels_sv2 supports both owned and borrowed variants, but that seems like it would introduce a lot of duplication (unless we abstract it behind a trait).

Curious to hear your thoughts on which direction makes more sense.

it took me a while to find bandwidth to look at this carefully, but I was finally able to do it today

first, thanks @Shourya742 for the detailed explanation... the reasoning feels very clear and I can get a very good understanding of the problem at hand

overall, PR looks solid too, and I'm excited for the much leaner codebase we're achieving as a result

---

now, looking at channels_sv2 crate... I always like to think of it as something that's designed to work as a state-machine that represents how the Sv2 channels evolve over the app lifetime

conversely, main purpose of buffer_sv2 is the ability to "recycle" the same (relatively small) amount of Heap memory in a sustainable way (i.e. without fragmentation or over-consumption as time goes on) for millions and millions of messages across the app's lifetime

so the only part of the message above that I would push back is:

but as a library we probably shouldn't be opinionated about how users structure their code.

while this principle definitely holds true for crates such as parsers_sv2 and handlers_sv2 (and it's good to be careful to avoid overlooking it), if we were to provide ref-based APIs on channels_sv2, the channel state lifetime would be essentially holding buffer space hostage, which defeats the purpose of buffer_sv2 in the first place

so for the case of channels_sv2 crate, I wouldn't necessarily frame this design decision as being opinionated... it's simply a constraint of the broader design space

perhaps a good way to summarize this into a rule-of-thumb:

as frames arrive over the wire:

• if the message can be dropped immediately after being processed (e.g.: Sniffer use-case), then reference-based message type is the right approach, because it allows the underlying BufferPool slots to be recycled (as intended), while keeping the global app execution fast and efficient (no cpu/memory "wasted" copying/cloning stuff around)
• if the message needs to persist in memory for a long time (e.g.: Job Factory where NewTemplate message acts as JobOrigin of ExtendedJob, for which we might want to reconstruct a block/solution later), then we should convert it into the Owned variant so we can free-up the underlying BufferPool as soon as possible (while accepting the price for this extra copying/cloning, which is the right trade-off for this kind of scenario)

I'm sure none of this will not come as a surprise to you, as I vaguely recall that we've had similar discussions in dev calls in the past (and your mental model on these kinds of concepts is usually way sharper than mine 😅)

nevertheless, it's a good exercise to re-visit this point now so we can confidently move on in this direction without looking back later

---

TLDR: with regards to channels_sv2, this PR is already in the right direction

[plebhash]

I'm proof-reading my message above and I realized that it's maybe too centered around buffer_sv2

and given the main sentence that I was trying to push back against is:

but as a library we probably shouldn't be opinionated about how users structure their code.

perhaps one could argue that using channels_sv2 without buffer_sv2 is a valid use-case, and therefore the sentence above is true.

but I think the core of my rationale remains, while buffer_sv2 is just one possible instance of the real constraint:

channel state will always outlive whatever buffer a frame was decoded from, so borrowing a message that can be stored somewhere else is either impossible (if that memory is reused, the compiler won't allow it) or just indirect ownership with bad ergonomics (a lifetime parameter leaked into every type that holds a channel) and terrible consequences with regards to memory fragmentation

so TLDR: taking *Owned at the channels_sv2 boundary is basically a domain constraint, not really opinionated API design