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

feat: implement ticket based F3 participation lease #12531

Merged
merged 6 commits into from
Oct 8, 2024

Conversation

masih
Copy link
Member

@masih masih commented Sep 30, 2024

Related Issues

Proposed Changes

Implemented enhanced ticket-based participation system for F3 consensus in F3Participate. This update introduces a new design where participation tickets grant a temporary lease, allowing storage providers to sign as part of the F3 consensus mechanism. This design ensures that tickets are checked for validity and issuer alignment, handling errors
robustly. If there's an issuer mismatch, the system advises miners to retry with the existing ticket. If the ticket is invalid or expired, miners are directed to obtain a new ticket via F3GetOrRenewParticipationTicket.

Additional Info

Checklist

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

@masih masih force-pushed the masih/f3-participation-token branch 6 times, most recently from da27be4 to 9316263 Compare October 1, 2024 15:00
@masih masih marked this pull request as ready for review October 1, 2024 15:03
@masih masih requested review from Stebalien and Kubuxu October 1, 2024 15:04
api/api_full.go Outdated Show resolved Hide resolved
@masih masih force-pushed the masih/f3-participation-token branch 3 times, most recently from e107151 to 40f1170 Compare October 1, 2024 20:44
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.

Some code nits and a few comments, but I'm a bit brain-dead from debugging tests all day so I'll have to leave the full review for tomorrow (hopefully @Kubuxu can get to it before I do).

api/api_errors.go Outdated Show resolved Hide resolved
chain/lf3/f3.go Outdated Show resolved Hide resolved
api/api_full.go Outdated Show resolved Hide resolved
api/api_full.go Outdated Show resolved Hide resolved
node/impl/full/f3.go Outdated Show resolved Hide resolved
@masih masih force-pushed the masih/f3-participation-token branch 3 times, most recently from bc978d4 to 63a1d03 Compare October 2, 2024 08:39
@masih masih changed the title feat: implement token based F3 participation lease feat: implement ticket based F3 participation lease Oct 2, 2024
@masih masih force-pushed the masih/f3-participation-token branch from 63a1d03 to e34e47c Compare October 2, 2024 08:41
api/api_full.go Outdated Show resolved Hide resolved
@Kubuxu
Copy link
Contributor

Kubuxu commented Oct 3, 2024

I think I would like to see the ability of the Miner to set the maximum term of the lease. Fundamentally, the Miner is in control of what they give out.

@masih masih force-pushed the masih/f3-participation-token branch 2 times, most recently from 80b65c0 to 9a3932b Compare October 4, 2024 10:35
@masih
Copy link
Member Author

masih commented Oct 4, 2024

the ability of the Miner to set the maximum term

Added, where the miner sets the validity term, as well as an upper maximum dictated by node for safety.

@masih masih force-pushed the masih/f3-participation-token branch from 9a3932b to 017d8c3 Compare October 4, 2024 11:17
@masih masih force-pushed the masih/f3-participation-token branch from 017d8c3 to 3d12ca5 Compare October 4, 2024 15:20
@masih masih requested a review from Stebalien October 4, 2024 15:33
chain/lf3/participation_lease.go Outdated Show resolved Hide resolved
node/modules/storageminer.go Show resolved Hide resolved
node/modules/storageminer.go Show resolved Hide resolved
node/modules/storageminer.go Outdated Show resolved Hide resolved
node/modules/storageminer.go Outdated Show resolved Hide resolved
node/modules/storageminer.go Show resolved Hide resolved
@masih masih requested a review from Stebalien October 7, 2024 09:57
@masih masih force-pushed the masih/f3-participation-token branch 2 times, most recently from b36ec70 to 47f6258 Compare October 7, 2024 11:39
node/modules/storageminer.go Outdated Show resolved Hide resolved
node/modules/storageminer.go Outdated Show resolved Hide resolved
node/modules/storageminer.go Show resolved Hide resolved
node/modules/storageminer.go Show resolved Hide resolved
node/modules/storageminer.go Outdated Show resolved Hide resolved
node/modules/storageminer.go Outdated Show resolved Hide resolved
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.

LGTM modulo a few nits that you're free to ignore, and some failing lints.

@masih masih force-pushed the masih/f3-participation-token branch from ec3b3f0 to 3dcdb91 Compare October 7, 2024 16:44
@masih masih force-pushed the masih/f3-participation-token branch from 3dcdb91 to e7b8b5a Compare October 7, 2024 16:56
masih and others added 4 commits October 7, 2024 13:46
Implemented enhanced ticket-based participation system for F3 consensus
in `F3Participate`. This update introduces a new design where
participation tickets grant a temporary lease, allowing storage
providers to sign as part of the F3 consensus mechanism. This design ensures that
tickets are checked for validity and issuer alignment, handling errors
robustly. If there's an issuer mismatch, the system advises miners to
retry with the existing ticket. If the ticket is invalid or expired,
miners are directed to obtain a new ticket via
`F3GetOrRenewParticipationTicket`.

Fixes filecoin-project/go-f3#599
To avoid potential of deadlock in case f3Participator is used from
multiple goroutines use throw-away timers at the price of higher GC.

Also use the cancel function in context explicitly in a unified stop
hook that awaits the participation to end before exiting.
Require the start instance of a participation to never decrease if there
 is an existing lease by the miner.
@Stebalien Stebalien force-pushed the masih/f3-participation-token branch 2 times, most recently from 523dff4 to cd82c4e Compare October 7, 2024 23:29
That way we don't re-use leases across networks. It's a bit racy (we ask
for the manifest before we ask for the current progress) but it should
be fine because at least we won't create a lease for the new network
with a future instance.

There's still an ABA problem if we rapidly switch back and forth between
two networks but... let's just not do that? At least for the mainnet
switchover, that won't be an issue because we enforce a 900 epoch
silence period.

I have to say, I'm not happy about this. But... we can probably just
hard-code it in the future once we get rid of the dynamic manifest.
Back off and get a fresh token if F3 is not ready.
@masih masih force-pushed the masih/f3-participation-token branch from 034b073 to 73359d6 Compare October 8, 2024 08:35
@masih masih mentioned this pull request Oct 8, 2024
@masih masih merged commit a0d5292 into master Oct 8, 2024
83 checks passed
@masih masih deleted the masih/f3-participation-token branch October 8, 2024 08:57
rjan90 pushed a commit that referenced this pull request Oct 8, 2024
* Implement ticket based F3 participation lease

Implemented enhanced ticket-based participation system for F3 consensus
in `F3Participate`. This update introduces a new design where
participation tickets grant a temporary lease, allowing storage
providers to sign as part of the F3 consensus mechanism. This design ensures that
tickets are checked for validity and issuer alignment, handling errors
robustly. If there's an issuer mismatch, the system advises miners to
retry with the existing ticket. If the ticket is invalid or expired,
miners are directed to obtain a new ticket via
`F3GetOrRenewParticipationTicket`.

Fixes filecoin-project/go-f3#599

* Use fresh timer every time for F3 backoffs

To avoid potential of deadlock in case f3Participator is used from
multiple goroutines use throw-away timers at the price of higher GC.

Also use the cancel function in context explicitly in a unified stop
hook that awaits the participation to end before exiting.

* Strictly require start instance to never decrease

Require the start instance of a participation to never decrease if there
 is an existing lease by the miner.

* feat(f3): update go-f3 to 0.7.0 and adapt for changes to the API

* feat(f3): Include the network name in the lease

That way we don't re-use leases across networks. It's a bit racy (we ask
for the manifest before we ask for the current progress) but it should
be fine because at least we won't create a lease for the new network
with a future instance.

There's still an ABA problem if we rapidly switch back and forth between
two networks but... let's just not do that? At least for the mainnet
switchover, that won't be an issue because we enforce a 900 epoch
silence period.

I have to say, I'm not happy about this. But... we can probably just
hard-code it in the future once we get rid of the dynamic manifest.

* Handle not ready error gracefully in participator

Back off and get a fresh token if F3 is not ready.

---------

Co-authored-by: Steven Allen <[email protected]>
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.

Change Lotus Miner Participation API to avoid equivocations
3 participants