Skip to content

Conversation

rjan90
Copy link
Contributor

@rjan90 rjan90 commented Aug 13, 2025

chore: bump filecoin-proofs-api to v19.0

Breaking Change Note:

Users will need to update their calls from:

// Before
ClearCache(sectorSize, cacheDirPath)
ClearSyntheticProofs(sectorSize, cacheDirPath)

// After  
ClearCache(cacheDirPath)
ClearSyntheticProofs(cacheDirPath)

The changes are consistent across all layers and properly remove the unused sector_size parameter completely from the API.

chore: bump filecoin-proofs-api to v19.0
@Copilot Copilot AI review requested due to automatic review settings August 13, 2025 05:37
@github-project-automation github-project-automation bot moved this to 📌 Triage in FilOz Aug 13, 2025
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates the filecoin-proofs-api dependency from version 18.1 to 19.0, representing a major version bump that may include breaking changes.

  • Upgraded filecoin-proofs-api from v18.1 to v19.0

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

@rvagg
Copy link
Member

rvagg commented Aug 13, 2025

looks like we removed sector_size from a couple of calls that'll need to be updated here

fix: remove unused sector_size from cache function calls
@rjan90 rjan90 moved this from 📌 Triage to 🔎 Awaiting Review in FilOz Aug 13, 2025
@rjan90 rjan90 self-assigned this Aug 13, 2025
@rvagg
Copy link
Member

rvagg commented Aug 13, 2025

oh .. those sectorSize arguments go all the way up to the Go API, we should remove it there as well and this would become a breaking change.

…ntional unused variables

chore: prefix sector_size parameters with underscore to indicate intentional unused variables
@rjan90
Copy link
Contributor Author

rjan90 commented Aug 13, 2025

oh .. those sectorSize arguments go all the way up to the Go API, we should remove it there as well and this would become a breaking change.

Ah, just pushed underscoring the parameters, as I believed we still would need them for the Go-code. But I will look into removing them there

rjan90 added 2 commits August 13, 2025 10:07
BREAKING CHANGE: ClearCache and ClearSyntheticProofs no longer accept sectorSize parameter.
chore: rustfmt
@vmx
Copy link
Contributor

vmx commented Aug 13, 2025

See #513 for how I've planned it. Though a breaking change is even better if that's possible.

@rjan90
Copy link
Contributor Author

rjan90 commented Aug 13, 2025

@vmx I was also going down that route in the first couple of commits:

But the build checks were still failing after those changes. At which point I switched to remove the sectorSize from ClearCache and ClearSyntheticProof.

@vmx
Copy link
Contributor

vmx commented Aug 13, 2025

The error on that one was a timeout. I restarted it and it now passes.

@rvagg
Copy link
Member

rvagg commented Aug 14, 2025

I think we can just break these.

In Lotus they're isolated to here: https://github.com/filecoin-project/lotus/blob/master/storage/sealer/ffiwrapper/sealer_cgo.go

In Curio they're in https://github.com/filecoin-project/curio/blob/main/lib/ffi/sdr_funcs.go and one in https://github.com/filecoin-project/curio/blob/main/lib/ffi/snap_funcs.go (cc @LexLuthr).

Not having to have sector size at some of these callsites might afford some additional cleanup opportunities, but I haven't looked at it in any detail.

So as long as we're bumping a version here to signal breakage, and documenting it, adapting to it upstream shouldn't be hard.

@github-project-automation github-project-automation bot moved this from 🔎 Awaiting Review to ✔️ Approved by reviewer in FilOz Aug 14, 2025
@rjan90 rjan90 merged commit d5fdfdb into master Aug 14, 2025
5 checks passed
@rjan90 rjan90 deleted the phi/bump-proofs-api branch August 14, 2025 04:43
@github-project-automation github-project-automation bot moved this from ✔️ Approved by reviewer to 🎉 Done in FilOz Aug 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🎉 Done
Development

Successfully merging this pull request may close these issues.

3 participants