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

deps: update boost to v1.0.0/filclient-commp-fix #95

Closed
wants to merge 2 commits into from

Conversation

rvagg
Copy link
Collaborator

@rvagg rvagg commented Aug 3, 2022

Ref: filecoin-project/boost#680

Fixes the CommP mismatch problems with the CAR being generated out of order for DAGs with certain (slightly) malformed DAG-PB blocks.

So this does two things - it pulls forward boost from a March pre-v1 commit to a branch based off v1.0.0, and that branch contains the two commits from filecoin-project/boost#675 that should address the CommP problems (for now, some larger work needs to be done, see filecoin-project/boost#673 (comment) for more info).

Since the deals with boost involve setting up a local libp2p server on the filclient side, and the CAR is generated on the client side as it's being sent over the wire to the storage provider, then fixing it on the client should be all that's required. The CommP mismatch comes because filclient pre-calculates a CommP using the standard car.SelectiveCar way of generating a CAR to CommP over, but then Boost uses a different way for its network transport, then the storage provider calculates the CommP of the bytes of the CAR it receives from the client's Boost libp2p server.

This update brings a few other dependencies along with it, it does leave a slight mismatch of filecoin-ffi between Boost and filclient (0.7.3-444-g5d00bb4 here, 0.7.3-449-g3f9ecec there), but since we use such a tiny amount of ffi here and it still compiles, that shouldn't be a problem I think.

An alternative would be to branch off filecoin-project/boost@cd38741e464d and add the two commits to that so we have an absolute minimal upgrade. Being a little more ambitious seems in order given how far out of sync we are with some of the deps here.

Ref: filecoin-project/boost#680

Fixes the CommP mismatch problems with the CAR being generated out of order
for DAGs with certain (slightly) malformed DAG-PB blocks.
@jacobheun
Copy link

If the dependency update to latest Boost is problematic right now given the large version change, we can cut a 1.0.1 tag in Boost with the commp patch to avoid the branch referencing.

@en0ma
Copy link
Contributor

en0ma commented Aug 4, 2022

If the dependency update to latest Boost is problematic right now given the large version change, we can cut a 1.0.1 tag in Boost with the commp patch to avoid the branch referencing.

I think this is a good idea. It will make it easy for Filclient to pull this fix in without the wide dep update. This will help Estuary-Boost integration mitigate the Commp issue right now, while we deal with getting the dependencies for Filclient up to date later.

@en0ma
Copy link
Contributor

en0ma commented Aug 4, 2022

@jacobheun filecoin-project/boost#675 has been merged, please let us know when a tag is released for it

@jacobheun
Copy link

@en0ma we cut a tag of the backport to 1.0.0, https://github.com/filecoin-project/boost/releases/tag/v1.0.1. 1.0.1 should help mitigate the dependency update problem for you in the short term.

@en0ma
Copy link
Contributor

en0ma commented Aug 4, 2022

@rvagg
Copy link
Collaborator Author

rvagg commented Aug 4, 2022

Updated this to use boost@v1.0.1.

@elijaharita
Copy link
Contributor

FYI filclient's boost was updated to v1.2.1 with the commp fix in another recently merged PR

@jacobheun
Copy link

FYI filclient's boost was updated to v1.2.1 with the commp fix in another recently merged PR

This can be closed then right? Given #90 includes the patch in Boost main.

FYI Boost is aiming to release 1.3.0 this week which will include the fix, so that will be usable instead of the commit being used in #90.

@elijaharita
Copy link
Contributor

@jacobheun thanks for the heads up! I'll pull in that version before next filclient release.

And yeah, this can be closed, I tagged this branch as v0.1.0 since we needed the fix for Estuary ASAP and we're getting dependency conflicts from the lotus update.

@elijaharita elijaharita closed this Aug 8, 2022
@jacobheun
Copy link

@jacobheun
Copy link

jacobheun commented Aug 31, 2022

@en0ma @elijaharita did this every get deployed to Estuary? Estuary's go mod looks like that's not the case and we're still seeing this issue come up.

@elijaharita
Copy link
Contributor

elijaharita commented Aug 31, 2022

@jacobheun it's not merged to master yet but the dev branch on estuary is on boost 1.3

@jacobheun
Copy link

Is there any ETA on when that will get deployed to prod or a place I can follow to know when that happens?

@rvagg rvagg deleted the rvagg/boost-update-custom-branch branch September 2, 2022 07:52
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