Change type of Label in DealProposal to be []byte, rather than String

[laudiacay]

Background
Also see filecoin-project/specs-actors#1248 for important relevant discussion.

The DealProposal Label is not guaranteed to be UTF-8, but is assumed to be in some client implementations. This is causing trouble for said implementations. People suggested either enforcing UTF-8 (this is more difficult) or switching the type to be a byte array that can be interpreted as needed. Discussion settled on the latter.

Proposal
Change the type of the Label field to be a []byte, rather than a string. Draft PR with these changes inside is here: filecoin-project/specs-actors#1496.

[kaitlin-beegle]

Hi @laudiacay! It looks like work remains ongoing for this proposal, which is great.

Once these details are set, a FIP draft can be written according to the guidance in FIP0001 and pushed to the repo. I will then work through the process of facilitating community acceptance of the FIP draft.

If you have any questions about this process, please feel free to ask!

[laudiacay]

filecoin-project/specs-actors#1248 Work done!

[arajasek]

This needs to get nudged along if we want it in the next network upgrade -- I believe it's currently impl complete, but in the pending next branch of specs-actors.

[BlocksOnAChain]

@kaitlin-beegle can we move/do we have all the needed elements to move this for the V15 upgrade since it’s in the "code complete " state from our side?
CC: @jennijuju @laudiacay @arajasek

[kaitlin-beegle]

@BlocksOnAChain you bet we do! Thanks for flagging that this is technically complete. We can move this item to last call status this week. I expect it will be approved without issue, but diligence requires that the last call period last for at least two weeks.

I would expect that this item will be approved for v15 incorporation by Friday, January 21, but that implementation teams should prepare to add the change in their calibnet upgrades.

[kaitlin-beegle]

After connecting with @laudiacay and @arajasek, it was confirmed that this FIP is no longer ready for 'last call' status. This FIP proposal will instead be moved back to the discussion forum, where it can be reconsidered for future upgrades.

@laudiacay, it's my understanding that there are two technical paths that we can take to move this forward: pursuing a more technically complex implementation within the existing market modules, or waiting until markets are decoupled from implementations. Any idea yet which is a better path?

[anorth]

@kaitlin-beegle @arajasek @laudiacay I don't understand this workflow at all. This is about the proposal, not the implementation. The proposal draft, minimal as it is, seems good. In terms of the proposal, we seemed ready to dub it "last call". As far as I understand it, the implementers ~all think it's a good idea so could probably go as far as "accepted" at the next call.

The implementation timeline is at most marginally relevant to this. The description of "accepted" state says

This FIP is in the hands of the Filecoin implementation developers. Their process for deciding whether to encode it into their clients as part of a consensus upgrade is not part of the FIP process.

Implementation work happens mostly after the FIP is ratified. The FIP provides the mandate to commit that work. We should not hold back a proposal merely because we can't ship the code in the next release train: that's all backwards.

Is there something wrong with the proposal itself, or is it just hard to implement for one or more implementations? If wrong, can we have that discussion in the PR, or in the tracking issue? If it's just too hard to land in our next upgrade, no problem! Land the FIP, implementers can discuss when and how to realise it in a subsequent upgrade.

This is a FIP that's been drafted, partially implemented, is in all senses active. We should land the PR, either in "draft" or "last call". It deserves a full-on issue for discussion, and to signal that status (or, possibly, to discuss abandoning it).

[kaitlin-beegle]

Hi @anorth, good questions; thanks for raising.

Perhaps it would be helpful if @arajasek or @laudiacay could provide more details about the implementation challenge, and whether or not it will affect the spec. My apologies if my decision to not consider this 'last call' was premature, but it seemed from my (brief) conversations with Claudia and Aayush that this was not ready 'as-is'. If we need to reconsider, we certainly can.

More importantly (in my opinion) is this point about landing FIPs separate from the upgrade process. As you know, FIPs require a lot of guidance and coordination as they're written, and the process is still very manual. During the v14 network upgrade (in October), we unintentionally synced up 'last call' status with the network upgrade because of 1) the high number of FIPs, 2) team capacities to implement these FIPs, 3) the coordination work required to move these from 'last call' to 'accepted', and 4) the quick turn-around time on FIP0026.

Timelines on this upgrade have been similarly 'synced' with the upgrade timeline, but not intentionally so. It's more a result of work capacity issues and the prioritization around Snap Deals. Perhaps this was a mistake. Regardless, as we work on updates to FIP0001, I think we ought to consider whether or not this is actually harmful or not.

I'd like to put more thought into this and will open up a thread in the discussion forum. We may have crafted the FIP process to model the EIP process, but I think it's worth reconsidering this element. For the last two upgrade cycles, 'accepted' status has de facto meant network incorporation. I understand the appeal of separating out FIPs from actual network implementation, but I think there is a larger legitimacy debt inherent to 'accepting' 15, 20, or 30 FIPs, that we have no imminent capacity to incorporate on-chain.

[jennijuju]

Was reached out in DM and thought probably best to comment in public for transparency

In general i agree with @anorth - The FIP in question is ready for Last Call (then Accepted once the Last Call phase passes) as the protocol change needed is well defined, so does the spec-actor impl spec.

The lotus team's concern with rushing this to be Final in ntwk v15 was - we thought more consideration needs be taken on how to introduce change into node/market implementations, without breaking existing storage markets in ecosystem (i.e: how do we prevent the PSD messages thats in the mpool that are sent before the upgrade epoch but land after; how do we communicate with projects like Estuary to incorporate this change). And we dont think we will be able to deliver a proper solution within the ntwk v15 timeline.

All that being said, I think it's safe to move the FIP to last call and the core devs can think more about the implementation later on and hopefully get it accepted and finalized in an upcoming upgrade.

(looking forward to discuss more on how we can potentially decouple last call status from actually network upgrades comm / imply network upgrades in a different discussion!)

[jennijuju]

I’m also curious about what the Venus team things as they also have a running market implementation

[arajasek]

@anorth Sorry about the slow response here. I think we're in something of a middle-ground here. The chief reason this FIP isn't going live in v15 is definitely just because of the integration complexity in the clients.

However, we might need to revisit the proposal itself in order to make this possible. There's a good chance the only clean way to do this (without "turning off deals" on Filecoin for some period) will be a two-step process in which both types are briefly supported before string is deprecated. Some thought needs to go into how we're gonna make this happen.

Whether that uncertainty means that this FIP does or doesn't belong in Last Call, however, is not something I have an opinion on.

[jennijuju]

we are revisiting this proposal for network v16 given as we are switching to built in actor that’s written in rust, and the String type provided by Rust’s standard library a growable, mutable, owned, UTF-8 encoded string type. However, atm the deal proposal label may be not UTF8 encoded - this may cause problems.

Hence cross posting from #318

@arajasek and @Stebalien (core developers from lotus and fvm team) are proposing to change the Label to be a Union type, something like

struct Label {
  label_string string
  label_bytes []byte
}

We apply the following rules:

• Assert that exactly one of label_string and label_bytes is set.
• De/serialize this object as though it were just one field -- "If label_string is set, serialize it as a string, if not, serialize it as bytes" (and the reverse for decoding). This is NOT the default CBOR encoding of such a struct, custom encoding must be written.
• Assert that if label_string is set, it is valid UTF-8. This check would automatically happen in a language like (safe) Rust, and can be enforced at the CBOR level for other languages.

An analagous type can be seen here, with its encoding found here

—- end

Eventually in a later upgrade, we want to switch over to only accepting bytes for this field, however, we have to do this gradually to make sure the filecoin storage market is not disrupted due to the state migration that’s required by this type change.

Ecosystem impact:
Given the client implementation needs to update and incorporate this change, for the applications that are built on top of the implementation like lotus, and have the following characteristics maybe impacted:

• use non uft-8 encoded string for deal proposal label.
• tooling that is built dependent on deal proposal CID, especially the ones thats being published on chain.

[arajasek]

So here's a decision tree that may help reason about this.

Decision 1: Should we do anything at all?

If no, the impact is that behaviour in implementations and tooling in many other languages (Rust, JavaScript, etc) is unpredictable and risk-prone. In particular, the current plan around introducing Rust-based builtin actors as "canonical" will have to introduce unsafe Rust (or some other hack) to support this.

Based on that, I am inclined to say the answer to Decision 1 is YES (though the matter isn't necessarily entirely closed -- happy to hear arguments that an answer of NO is fine).

Decision 2: Can we do the simple solution?

The obvious answer is to convert all Labels to bytes, and use bytes moving forward. This was the original FIP-0027, which was accepted into the Chocolate upgrade, but dropped at the last minute because of the integration complications described here. In short, we could not think of a way to introduce a simple conversion to bytes without breaking dealmaking on Filecoin for at least a few hours. We may have been wrong in that assessment, challenges to the scenarios described in the linked complication descriptions is very welcome.

If we were correct that FIP-0027 as originally written leads to deal-making breaking for some extended period, we need to decide whether that's okay. We may, as a community, be okay with that outcome. The original proposal also requires a much larger refactor of client and market impls, and ecosystem tooling, but that can be handled.

I am personally inclined to say that we should not be breaking deal-making on Filecoin for hours, though. If we agree, that brings us to the next decision.

Decision 3: What's the best solution that doesn't damage dealmaking and tooling?

This brings us to the proposed modification to FIP-0027. We believe this will have no noticeable impact to dealmaking, while also avoiding issues in other languages (and, in particular, in the new Rust-based builtin actors).

It may still be the case that the proposed modification is either incorrect for some reason, or needlessly complicated. Feedback to correct / simplify is very welcome!