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

Add ProofReplicaUpgrade2 #9250

Closed
jennijuju opened this issue Sep 2, 2022 · 15 comments
Closed

Add ProofReplicaUpgrade2 #9250

jennijuju opened this issue Sep 2, 2022 · 15 comments

Comments

@jennijuju
Copy link
Member

introduced by https://github.com/filecoin-project/FIPs/blob/master/FIPS/fip-0041.md

lotus should start to call PreCommitSectorBatch2 instead of PreCommitSectorBatch1.

Local sector precommit info might need to be migrated to the new param.

@jennijuju jennijuju added P1 P1: Must be resolved effort/hours Effort: Hours labels Sep 2, 2022
@jennijuju jennijuju added this to the Network v17 milestone Sep 2, 2022
@jennijuju jennijuju moved this to Opportunistic in Network nv17 Sep 2, 2022
@jennijuju jennijuju moved this from Opportunistic to Todo / In Scope in Network nv17 Sep 2, 2022
@arajasek
Copy link
Contributor

I don't think this should be scoped as part of nv17. Doing so means we would have to switch on network version to start using the new PreCommitSectorBatch2 method only after the network upgrade.

I would propose delaying this issue until after the nv17 upgrade, at which point we simply always call PreCommitSectorBatch2 (and ProveReplicaUpdates2).

@Kubuxu please chime in with thoughts.

@jennijuju
Copy link
Member Author

jennijuju commented Sep 10, 2022

Agree we don’t HAVE to. If we don’t do it in this release, then it will at least take 3 upgrades for us to fully drop batch 1 function in actir. Doing it now we can drop it in next upgrade.

@Kubuxu
Copy link
Contributor

Kubuxu commented Sep 16, 2022

It would be preferred if it could be removed in the next upgrade (but probably won't happen, FEVM) or one after that.
This also affects ProofReplicaUpgrade2.

@jennijuju jennijuju added P2 P2: Should be resolved and removed P1 P1: Must be resolved labels Sep 16, 2022
@jennijuju
Copy link
Member Author

thank you! lower it to P2

@jennijuju jennijuju changed the title Add PreCommitSectorBatch2 Add PreCommitSectorBatch2/ProofReplicaUpgrade2 Sep 16, 2022
@arajasek
Copy link
Contributor

Agree we don’t HAVE to. If we don’t do it in this release, then it will at least take 3 upgrades for us to fully drop batch 1 function in actir. Doing it now we can drop it in next upgrade.

Yeah, the problem is just that doing it now is strictly harder than doing it after (because after the upgrade we can just switch without nv guards).

@jennijuju
Copy link
Member Author

I am convinced this is not mandatory for t his upgrade (for now 😉 ), moving this to opportunistic for now.

@jennijuju jennijuju moved this from Todo / In Scope to Opportunistic in Network nv17 Sep 20, 2022
@anorth
Copy link
Member

anorth commented Sep 20, 2022

I concur you don't need to implement these before nv17, but disagree that this means it'll take 3 upgrades to drop the old ones.

The old methods will be dropped in nv18, if I have anything to say about it. Lotus will have implemented the new methods then, and your nv18 release will be mandatory.

@jennijuju
Copy link
Member Author

jennijuju commented Sep 20, 2022

@anorth i think we will need

  • one upgrade to introduce the new method - nv17
  • lotus introduce the new method in a feature release, feature release is optional
  • before nv18 upgrade
    • it is possible to have users send out PCSB message using nv17 lotus release
    • these messages will fail if we drop PCSB & PRU on v10 actor
  • it's much safer/less error prune for lotus users if we drop these methods in nv19, after we get everyone users nv18 lotus release, which sends PCSB&PRU2
  • in lotus we can also save time on implementing retry & recover mechanism for the failure mentioned in the point above

@anorth
Copy link
Member

anorth commented Sep 20, 2022

it is possible to have users send out PCSB message using nv17 lotus release

It's extremely unlikely this is done within hours of the upgrade and sit around in the message pool for that long. There are not terrible effects if these message fail. It's not worth the complexity to handle these messages. You should not implement any special retry/recovery above whatever general methods exist for retrying a failed high-level operation. Just declare the upgrade mandatory X hours before the network upgrade.

@arajasek
Copy link
Contributor

Yeah, I concur with @anorth here. I think we:

  • continue sending old messages in Lotus's nv17 release (v1.18.0)
  • switch to sending the new messages in the next Lotus release (v1.19.0)
  • announce the deprecation of the old methods in nv18 (actors v10)
  • tell folks that if they haven't upgraded to the Lotus v1.19 series they will experience a change in behaviour when they upgrade to the next mandatory release (v1.20.0)
  • reiterate that message when we ship v1.20.0

@Kubuxu
Copy link
Contributor

Kubuxu commented Sep 21, 2022

Thanks @arajasek, the ability to do this staggered deployment is why I have decided to pursue this FIP. At the point that people are running v1.20 they will be sending new messages. I don't think we need to consider messages that were sent with v1.18 when people have to run v1.20 for the upgrade, although it might be worth it to reiterate that people should not run v1.18 right up to the line.

Do we have any data on how the update pickup percentage looks before a network upgrade?

@jennijuju jennijuju removed the P2 P2: Should be resolved label Sep 24, 2022
@arajasek
Copy link
Contributor

arajasek commented Oct 5, 2022

Yeah, this FIP is great for allowing such changes to happen!

We don't have great data on the update % leading up to the network upgrade (we could get some in a month as we lead into v17). Anecdotally, it does appear that a majority of users upgrade in the 24 hours before the upgrade.

@arajasek
Copy link
Contributor

arajasek commented Oct 5, 2022

Descoping from v17 since it's a no-op for the upgrade.

@arajasek arajasek removed this from the Network v17 milestone Oct 5, 2022
@rjan90 rjan90 added this to the LM-Tech-Debt milestone Mar 31, 2023
@rjan90
Copy link
Contributor

rjan90 commented Aug 19, 2023

The switch to using PreCommitSectorBatch2 has been merged here: #11142.

@rjan90 rjan90 changed the title Add PreCommitSectorBatch2/ProofReplicaUpgrade2 Add ProofReplicaUpgrade2 Sep 19, 2023
@rjan90
Copy link
Contributor

rjan90 commented Apr 14, 2024

Closing this issue as ProveReplicaUpdates2 is being removed in the coming network upgrade (nv22), and ProveReplicaUpdates3 (method 35) has been introduced and implemented: #11831

@rjan90 rjan90 closed this as completed Apr 14, 2024
@github-project-automation github-project-automation bot moved this from Opportunistic to Done in Network nv17 Apr 14, 2024
@github-project-automation github-project-automation bot moved this to 👀 In Review in Lotus-Miner-V2 Apr 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 👀 In Review
Status: Done
Development

No branches or pull requests

5 participants