Skip to content

fix: allow abandonment and consensual termination of underfunded data sets#520

Open
wjmelements wants to merge 4 commits into
mainfrom
test/underfunded-payer-scenarios
Open

fix: allow abandonment and consensual termination of underfunded data sets#520
wjmelements wants to merge 4 commits into
mainfrom
test/underfunded-payer-scenarios

Conversation

@wjmelements

Copy link
Copy Markdown
Contributor

Reviewer @rvagg
Fixes #519
Consensual termination and abandonment both attempt to set the endEpoch to block.number in order to allow immediate cleanup.
Because we cannot modify the payment rate or the lockup for underfunded data sets before termination (see FilOzone/filecoin-pay#290), we are unable to reduce the lockup period to ensure endEpoch is the current block.
Instead of reverting, we will release the fixed lockup after termination.

Changes

  • add three failing tests
  • catch modifyRailLockup exceptions in abandonment and consensual termination

@wjmelements wjmelements linked an issue Jun 10, 2026 that may be closed by this pull request
@wjmelements wjmelements requested a review from rvagg June 10, 2026 21:29
@FilOzzy FilOzzy added this to FOC Jun 10, 2026
@github-project-automation github-project-automation Bot moved this to 📌 Triage in FOC Jun 10, 2026
@wjmelements wjmelements added the bug Something isn't working label Jun 11, 2026
…ndonRails and updateStorageRates

Assisted-by: Claude:claude-sonnet-4-6
@BigLep BigLep moved this from 📌 Triage to 🔎 Awaiting review in FOC Jun 11, 2026
Comment on lines +298 to +304
// Try to zero the lockup period (requires a fully-settled account).
// For underfunded payers the period change is blocked.
try payments.modifyRailLockup(pdpRailId, 0, pending) {}
catch {
// Always release the fixed lockup
payments.modifyRailLockup(pdpRailId, DEFAULT_LOCKUP_PERIOD, pending);
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I don't think we should do this here, immedaiteTermination path implies that everyone is behaving properly and agreeing together, allowing an underfunded client here to proceed with immediate termination gives a bad actor privileges that I don't think they should get.

In the normal case I don't think we'll get here, as I said in #519 I think the only way we can get into the situation where this reverts is where the SP abandons the data set and doesn't discover underfunding with regular settles. The immediateTermination path suggests the SP is still engaged and willing to submit a msg to the chain for this data set.

I'd just leave a comment in here about why we're not doing a try/catch.

Suggested change
// Try to zero the lockup period (requires a fully-settled account).
// For underfunded payers the period change is blocked.
try payments.modifyRailLockup(pdpRailId, 0, pending) {}
catch {
// Always release the fixed lockup
payments.modifyRailLockup(pdpRailId, DEFAULT_LOCKUP_PERIOD, pending);
}
// No try/catch here to cover the underfunded-client case. immediateTermination implies
// the payer is solvent enough to consent to releasing the lifecycle reserve and paying the
// terminate fee from `pending` in one step.
payments.modifyRailLockup(pdpRailId, 0, pending);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This makes sense to me. I'll think about it a bit more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

Status: 🔎 Awaiting review

Development

Successfully merging this pull request may close these issues.

Support underfunded termination

4 participants