filecoin-project / builtin-actors

The Filecoin built-in actors
Other
83 stars 79 forks source link

Discuss: Versioning and FVM M2 development #483

Closed raulk closed 1 year ago

raulk commented 2 years ago

Context

The FVM M2 milestone requires changes to these actors:

Furthermore, we propose bringing in EVM actors into the builtin-actors tree (#482).

We would like to figure out ways to support concurrent development of nv17 and FVM M2 (presumably nv18).

Proposal

ZenGround0 commented 2 years ago

Strong preference for developing directly on master. Linear commit history and one-thing-per-commit means that the worst case where we have to revert partial development on something that doesn't have time to land is not even that bad.

raulk commented 2 years ago

@ZenGround0 I didn't follow your line of reasoning. This isn't about merging strategy. This is about supporting concurrent development.

ZenGround0 commented 2 years ago

Yes I understand that. I was trying to understand the reason behind a develop branch which I think mostly exists to separate out changes that we think might not land. If that's the motivation it's not sufficient given the nice properties of the linear commit history imo. Maybe I'm missing stronger motivation.

Doing development on one branch is simpler when there is not too much going on and my intuition is that these two streams combined are not big enough to violate that heuristic. If you'd strongly prefer a development branch we can make it work too.

raulk commented 2 years ago

I was trying to understand the reason behind a develop branch which I think mostly exists to separate out changes that we think might not land.

That isn't the motivation. The motivation is to enable concurrent development of network versions that are pipelined in time. Merging all changes to master will imply that changes slated for nv18 would end up shipping in nv17 (if master represented nv17).

anorth commented 2 years ago

I think you're both right, but maybe with different contexts.

I concur with @ZenGround0 a strong preference for development of the code that we expect to ship soon happens on master. At the moment "soon" means for nv17 (actors v8 has already branched off for release in nv16). So Zen this means all the work you're preparing for would be on master.

We do then need a way to develop code that's targeting a release further in the future. A long-lived branch that's treated as a target for PRs is a good way of doing this. Maintainers need to continually rebase on master. I would caution against maintaining more than one of these, as the cost of conflict between these branches runs high. But either "M2" or "nv18" or even just "next" are ok labels for this.

All that said, @raulk, if changes to the init actor or account actor are ready in time for nv17, I would tend to just release them with that, following the principle of minimising work in progress. The closer that M2 comes to just flicking a switch to enable already-implemented behaviour, the better.

jennijuju commented 2 years ago

I think the ask for developing in a dev branch was partially from me.

reason being:

If we land on everything goes directly into master, then we need to establish stricter rules on the test coverage with each PR imho.

anorth commented 2 years ago

we need to establish stricter rules on the test coverage with each PR imho

We should anyway, so if master is intimidating enough to make it actually happen, so much the better!

arajasek commented 2 years ago

Yeah, I like just having master and a next (and a release/vX branch that preserves old releases and supports any quick changes needed there). My opinion (which is really just a mashup of @anorth and @ZenGround0's stated opinions) is:

This isn't really that different from the method we've used in specs-actors in the past, except that we are expecting more future-looking dev work happening in parallel, and so need some new place for that work (when absolutely necessary). A single next branch serves that purpose.

raulk commented 2 years ago

@arajasek sounds like the approach I suggested in the original post, but limiting concurrency to 1 in a next branch.

raulk commented 2 years ago

@anorth is also aligned with that approach.

Re: the suggestions of shipping FVM M2-related changes (init, system, and account actors) in nv17 if they're ready, I'm strongly opposed.

  1. There is no value in shipping unused features to the network. This relies on a fake assumption that they're secure even though they haven't been subjected to... usage.
  2. There's the risk that later phases of FVM M2 (incentivised testnet, auditing) will unveil vulnerabilities in code that we've shipped in nv17. We don't want to be in that situation.
jennijuju commented 2 years ago

Here is the latest proposal from the FVM perspective, assuming nv17 is for addressing selected non-FVM FIPs, an nv18 will be reserved for nv18.

FVM Milestone Network version Actors version builtin-actors branch
FVM M1 nv16 v8 release/v8
nv17 v9 master
FVM M2 EVM nv18 v10 next
FVM M2 Native (tbd) (tbd) next with feature flags    

with this proposal, question:

jennijuju commented 2 years ago

cc @lemmih - how does the forest used to manage this? any thoughts on the proposals above & how we should go forward?

lemmih commented 2 years ago

In short, we essentially didn't manage network versions so I have no insight to offer. :) I might be reading this discussion wrong but which branches to use is an internal matter. As long as you publish your crates and use semver, forest will be happy.

raulk commented 2 years ago

For the record, my immediate interest is coming out with an actionable item here that we all mildly agree on. That seems to be to target all "projected" (vs. confirmed) changes to a next branch. This outcome limits our abillity to project changes into the future beyond the next network version, which basically results in clipping our own wings to fly and achieve higher velocities and rates of innovation in the long-term. I do not agree with that outcome and develop/fvm-m2 would be a ton better, but this (in my view, defective) policy can be reversed very quickly when the time comes.

vyzo commented 2 years ago

M2 native should go in next with feature flags, not master imo.

raulk commented 2 years ago

Well spotted, @vyzo. I've also updated it in the original mapping table which includes ref-fvm version alignment.

image
arajasek commented 2 years ago

I might be reading this discussion wrong but which branches to use is an internal matter. As long as you publish your crates and use semver, forest will be happy.

@lemmih No such thing as an internal matter -- your (team's) input on things like this is very welcome, since it'll affect anyone building an implementation of Filecoin :)

anorth commented 2 years ago

Re: the suggestions of shipping FVM M2-related changes (init, system, and account actors) in nv17 if they're ready, I'm strongly opposed

I will note some arguments in favour of shipping code when it's ready, though we can defer a decision until we have a concrete case to evaluate.

jennijuju commented 1 year ago

we are set for m2.1 - open new issue when m2.2 comes if needed.