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

chore: update triplewz/poseidon #12602

Merged
merged 1 commit into from
Oct 16, 2024
Merged

chore: update triplewz/poseidon #12602

merged 1 commit into from
Oct 16, 2024

Conversation

magik6k
Copy link
Contributor

@magik6k magik6k commented Oct 15, 2024

Related Issues

triplewz/poseidon#2

Proposed Changes

Curio pulls in the new version of the Poseidon library which switched to generic fr.Element. We need that to support SnapDeal decode in pure Go, and can't use the older version.

Additional Info

Checklist

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

@rjan90 rjan90 requested a review from Stebalien October 15, 2024 17:19
@LexLuthr LexLuthr added the skip/changelog This change does not require CHANGELOG.md update label Oct 15, 2024
Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

I'd prefer a released version, but this seems reasonable. From what I can tell, this is only used in sealing, we have a separate flow to check the proofs. That means this change should be covered by the itests.

But just to double check: @magik6k have you tested the lotus-miner sealing flow and/or confirmed that it's tested (enough) in CI? I now some proof types aren't generally tested because they're too slow to test in CI.

go.mod Show resolved Hide resolved
storage/sealer/commitment/commr.go Show resolved Hide resolved
@magik6k
Copy link
Contributor Author

magik6k commented Oct 16, 2024

From what I can tell, this is only used in sealing, we have a separate flow to check the proofs. That means this change should be covered by the itests.

It's only used with "External PC2 binary" mode of sealing, which IIUC isn't adopted pretty much anywhere (that was used for brownfield supraseal pc2, which while faster wasn't very stable). We also use the new version of this library in Curio for many other parts of the proving system, and it definitely works fine there.

@rjan90
Copy link
Contributor

rjan90 commented Oct 16, 2024

It's only used with "External PC2 binary" mode of sealing, which IIUC isn't adopted pretty much anywhere (that was used for brownfield supraseal pc2, which while faster wasn't very stable).

Can confirm that the "External PC2 binary" mode of sealing is not well adopted, because it has the downside of only working with CC-sectors currently: and switching between external PC2 binary / internal PC modes depending if it is data or not in the sectors, would be a very painful experience.

@Stebalien Stebalien merged commit 61ca37b into master Oct 16, 2024
98 of 102 checks passed
@Stebalien Stebalien deleted the chore/update-poseidon branch October 16, 2024 15:13
rjan90 pushed a commit that referenced this pull request Oct 16, 2024
rjan90 pushed a commit that referenced this pull request Oct 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip/changelog This change does not require CHANGELOG.md update
Projects
Status: ☑️ Done (Archive)
Development

Successfully merging this pull request may close these issues.

4 participants