Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: chain: future proof the from & to address protocols #9515

Merged
merged 1 commit into from
Nov 8, 2022

Conversation

Stebalien
Copy link
Member

@Stebalien Stebalien commented Oct 19, 2022

This lets us add new address protocols to go-address without implicitly accepting them in messages on the network.

Checklist

Before you mark the PR ready for review, please make sure that:

  • Commits have a clear commit message.
  • PR title is in the form of of <PR type>: <area>: <change being made>
    • example: fix: mempool: Introduce a cache for valid signatures
    • PR type: fix, feat, build, chore, ci, docs, perf, refactor, revert, style, test
    • area, e.g. api, chain, state, market, mempool, multisig, networking, paych, proving, sealing, wallet, deps
  • New features have usage guidelines and / or documentation updates in
  • Tests exist for new functionality or change in behavior
  • CI is green

@Stebalien Stebalien requested a review from a team as a code owner October 19, 2022 04:44
@Stebalien Stebalien requested a review from arajasek October 19, 2022 04:45
@Stebalien Stebalien force-pushed the feat/future-proof-address-protocol branch 2 times, most recently from 4273f4d to b2280f4 Compare October 19, 2022 04:52
@Stebalien
Copy link
Member Author

I think I got all the cases we care about here.

Copy link
Member

@raulk raulk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a fan of this solution; I'd prefer to be more explicit.

How about we keep track of supported protocols per network version in go-address? There are several options, out of which one of these would work best:

  • Turn the Protocol alias into a newtype, and add a MinNetworkVersion() uint to it.
  • Add a top-level utility function SupportedInNetworkVersion(address.Protocol, version uint)

I'd prefer the first one, but there may be some type breakage.

@Stebalien
Copy link
Member Author

So, go-addresss doesn't understand (and shouldn't understand) network versions. It would be best to add a helper in lotus, but it's unclear where we'd do that.

@raulk
Copy link
Member

raulk commented Oct 19, 2022

@Stebalien I was afraid you'd say that. I think it should.

  • go-address has no utility beyond interacting with the Filecoin network.
  • As the Filecoin network evolves, so does go-address.
  • It's best to place change management and versioning logic in a single, colocated place.
  • Other dependents of go-address will also benefit from this library having self-awareness and knowledge of when each protocol was introduced.

What are downsides do you see in putting the change management logic here?

@Stebalien
Copy link
Member Author

Well, for one, that would introduce a cyclic module dependency between the state types and the address type. I agree we should manage this kind of information in one place, but that one place is the lotus repo. If we started adding version checks to the go-address repo, we'd need to start making changes in two places.

But yes, the current situation isn't great. We should a small package in lotus where we define the address constraints for an upgrade.

@raulk
Copy link
Member

raulk commented Oct 19, 2022

Well, for one, that would introduce a cyclic module dependency between the state types and the address type.

The network version is just a uint. There is no reason to make this module depend on network.Version from go-state-types. The orchestrator of both modules (e.g. Lotus) can trivially cast the type, if needed.

@raulk
Copy link
Member

raulk commented Oct 19, 2022

I still think constraints belong in go-address, and enforcement of those constraints obviously on the client of that module (Lotous). That way, an address protocol can declare "I don't make sense before nv18", and the client module would be in charge of enforcing it.

@Stebalien
Copy link
Member Author

This is coming down to a matter of preference and norms, and I'm strongly inclined to stick with the current norms of encoding all network policies in this repo and the builtin actors. All other repos provide functionality and types, but they don't encode decisions with respect to network versions (and, really, don't even care about them).

The final decision is up to @arajasek.

@Stebalien
Copy link
Member Author

Options:

  1. Add a ValidForNetworkVersion on the Address type (in the go-address module).
  2. Add a new package/function somewhere in lotus for checking per-version address rules.
  3. Keep this as-is with ad-hoc checks everywhere (matches current behavior but is, well, ad-hoc).

@arajasek
Copy link
Contributor

I definitely don't want go-address becoming aware of network versions and having the constraints defined there. I'd much rather leave go-address home to the implementations of various address protocols, and let clients use them as they see fit. That makes more sense to me, and fits the pattern better. As an example, our actors don't have any stated constraints about them -- one could deploy v8 code as network version 1 if they wanted to, it's up to clients to enforce that doesn't happen.

I don't like the ad-hoc checks either, because that will grow. I would actually propose adding this ValidForNetworkVersion to go-state-types. It's a bit funky, but that's where "what's appropriate for this network version" logic currently lives (eg. ActorsVersionForNetwork).

I don't feel strongly about that, though, so if you wanna just have the helper here in lotus, that's fine by me (but do have a method, and not just m.To.Protocol() > address.BLS.

@Stebalien Stebalien mentioned this pull request Nov 8, 2022
6 tasks
@Stebalien Stebalien force-pushed the feat/future-proof-address-protocol branch from b2280f4 to a69480b Compare November 8, 2022 20:21
This lets us add new address protocols to go-address without implicitly
accepting them in messages on the network.
@Stebalien Stebalien force-pushed the feat/future-proof-address-protocol branch from a69480b to c6f2710 Compare November 8, 2022 20:28
@arajasek arajasek merged commit bd4900a into master Nov 8, 2022
@arajasek arajasek deleted the feat/future-proof-address-protocol branch November 8, 2022 21:27
@simlecode
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants