perf(validatePayment): loop over proving periods #267

parzival1821 · 2025-10-08T08:58:18Z

Related Issue

Closes 252 ( Once completed)

Current Progress

I've implemented the updated looping algorithm in validatePayment as described in the issue

Replaced the existing loop with new algorithm
Convert provenPeriods to a bitmap
Remove provenThisPeriod

What I Need Feedback On

Before I continue with the rest of the issue, I'd like to get feedback on my implementation of the looping algorithm.

Since there were no existing tests for this specific part of the code, I want to make sure my approach is correct before implementing the remaining parts of the issue. Also I would love to help you guys write some tests for the same if required

Questions for Maintainers

Does the new looping logic match what you had in mind?
Are there any edge cases I should consider?
Any suggestions for improvement before I move forward?

Changes Made

Modified FilecoinWarmStorageService.sol to implement the new loop structure

Next Steps (after approval)

Once you confirm this approach is correct, I'll:

Complete the remaining tasks in perf(validatePayment): loop over proving periods instead of epochs #252
Request a full review

Note: This is a WIP/Draft PR specifically for early feedback on the algorithm implementation.
(Also, just a heads-up, I changed my username from dev-n-dough to parzival1821

parzival1821 · 2025-10-08T09:01:40Z

@wjmelements @rjan90 Would love to get your thoughts on this draft PR

parzival1821 · 2025-10-08T11:03:02Z

Some workflow checks are failing, sorry about that, let me just fix them real quick

service_contracts/foundry.lock

service_contracts/src/FilecoinWarmStorageService.sol

wjmelements · 2025-10-09T17:42:17Z

Please add some tests to check the edge cases, verifying that the IValidator.validatePayment behavior is the same as before.

parzival1821 · 2025-10-12T01:25:21Z

I am fixing some issues, will soon start writing the tests. Will push my commits soon

wjmelements · 2025-10-16T03:01:18Z

Are you able to finish this week? We are planning to code freeze this repo ~~Friday~~ and we want to include this change.

parzival1821 · 2025-10-16T09:29:56Z

@wjmelements I am actively working on this issue and will try my best to finish it this week

wjmelements · 2025-10-16T15:27:25Z

Looks like you did a bad merge and reverted a bunch of our recent changes by mistake.

parzival1821 · 2025-10-17T07:01:29Z

Yes I think something went wrong with the rebasing, I am trying to fix that

rjan90 · 2025-10-17T10:23:09Z

@parzival1821 Some smaller Lint Check failures to fix up here

parzival1821 · 2025-10-17T10:52:28Z

Working on the tests now, will push soon

rjan90 · 2025-10-17T11:05:35Z

Great! Let us know when it is ready for another round of review - we will try to get these changes across the line today

parzival1821 · 2025-10-17T19:18:29Z

Hi @rjan90 @wjmelements I have added the tests and made a few changes to the algorithm. One thing which is kind of unusual is that in the tests, is that I am using uint256 activationEpoch = block.number , but upon using vm.roll , the block.number is changing as expected, but the activationEpoch variable is also getting updated. I will further look into this, but for now I have assigned uint256 activationEpoch = 1 since block number in foundry test starts from 1. The tests are passing and the algo is working as expected

Please review my changes and lmk is any changes/improvement is required!

wjmelements · 2025-10-17T20:06:32Z

I am using uint256 activationEpoch = block.number , but upon using vm.roll , the block.number is changing as expected, but the activationEpoch variable is also getting updated. I will further look into this, but for now I have assigned uint256 activationEpoch = 1 since block number in foundry test starts from 1. The tests are passing and the algo is working as expected

In forge tests, use vm.getBlockNumber() instead of block.number

.gitignore

service_contracts/.gitignore

service_contracts/lib/forge-std

service_contracts/src/FilecoinWarmStorageService.sol

wjmelements · 2025-10-21T20:50:04Z

service_contracts/lib/openzeppelin-contracts

you still haven't reverted your changes to the submodules

My bad, fixed it now

service_contracts/src/FilecoinWarmStorageService.sol

wjmelements · 2025-10-22T19:25:08Z

service_contracts/src/FilecoinWarmStorageService.sol

        }

        uint256 startingPeriod = getProvingPeriodForEpoch(dataSetId, fromEpoch + 1);
-        uint256 endingPeriod = getProvingPeriodForEpoch(dataSetId, toEpoch);


I changed it so we only calculate endingPeriod if we will use it

wjmelements · 2025-10-22T19:25:41Z

service_contracts/src/FilecoinWarmStorageService.sol

    }

-    function _calcPeriodDeadline(uint256 dataSetId, uint256 periodId) private view returns (uint256) {
-        return provingActivationEpoch[dataSetId] + (periodId + 1) * maxProvingPeriod;


I pass the activationEpoch from the parent function instead of looking it up again with the dataSetId

wjmelements · 2025-10-22T19:26:26Z

service_contracts/src/FilecoinWarmStorageService.sol

            for (uint256 period = startingPeriod + 1; period < endingPeriod; period++) {
                if (_isPeriodProven(dataSetId, period, currentPeriod)) {
                    provenEpochCount += maxProvingPeriod;
-                    lastProvenEpoch = _calcPeriodDeadline(dataSetId, period);


Instead I calculate this once, from the ending period deadline

wjmelements · 2025-10-22T19:29:07Z

service_contracts/src/FilecoinWarmStorageService.sol

-        uint256 provenEpochCount,
-        uint256 lastProvenEpoch,


I remove these parameters since they are the outvars

In the case where lastProvenEpoch is unchanged, we revert so it is unimportant to initialize it

In the case where lastProvenEpoch is unchanged, we revert so it is unimportant to initialize it

This isn't quite true because we don't revert. But the uninitialized outvar is unused in that case.

Still I think we should be able to settle the rail over periods of inactivity.

#321
c448c41

parzival1821 · 2025-10-23T06:35:20Z

Thank you very much @wjmelements @rjan90 for guiding me through this PR. Would love to keep contributing to FilOzone and the Filecoin ecosystem as a whole!

@rvagg

Reviewer @rvagg It is only important to block forward progress through the current proving period. Currently we don't allow forward progress through unsettled periods without proofs. This could be a problem if there was a rail unproven for a long time but proofs were later resumed. Though much less likely after #258 and #267, we would not want it to be possible for the gap to exceed the block gas limit. Also important to recall that the `fromEpoch` is an exclusive index while `toEpoch` is an inclusive index. #### Changes * allow to settleUpTo previous period when unproven * define settledUpTo in previously irrelevant case

FilOzzy added this to FS Oct 8, 2025

github-project-automation bot moved this to 📌 Triage in FS Oct 8, 2025

rjan90 moved this from 📌 Triage to 🔎 Awaiting review in FS Oct 8, 2025

rjan90 linked an issue Oct 8, 2025 that may be closed by this pull request

perf(validatePayment): loop over proving periods instead of epochs #252

Closed

rjan90 added this to the M4: Filecoin Service Liftoff milestone Oct 8, 2025

rjan90 assigned parzival1821 Oct 8, 2025

wjmelements requested changes Oct 8, 2025

View reviewed changes

github-project-automation bot moved this from 🔎 Awaiting review to ⌨️ In Progress in FS Oct 8, 2025

parzival1821 force-pushed the validate-payment-loop branch from bf33be6 to 54d5f4d Compare October 9, 2025 10:52

parzival1821 requested a review from wjmelements October 9, 2025 11:23

parzival1821 force-pushed the validate-payment-loop branch from ec22cb9 to ab96798 Compare October 17, 2025 08:01

wjmelements reviewed Oct 17, 2025

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

wjmelements reviewed Oct 17, 2025

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

wjmelements reviewed Oct 17, 2025

View reviewed changes

service_contracts/.gitignore Outdated Show resolved Hide resolved

wjmelements reviewed Oct 17, 2025

View reviewed changes

service_contracts/lib/forge-std Show resolved Hide resolved

wjmelements reviewed Oct 17, 2025

View reviewed changes

service_contracts/src/FilecoinWarmStorageService.sol Outdated Show resolved Hide resolved

wjmelements reviewed Oct 17, 2025

View reviewed changes

service_contracts/src/FilecoinWarmStorageService.sol Outdated Show resolved Hide resolved

parzival1821 and others added 9 commits October 21, 2025 23:46

make required changes

f724dea

implemented required changes

e3bddea

revert submodules

3d78a36

fix failing test,formatting, and some minor code quality improvements

8ffc573

fixed a doc mistype in Errors.sol

78eeb8a

fix documentation and reverse accidental submodule updation

df87fa7

cache activatonEpoch in validatePayment

7ddb183

updated a few comments

34bdc4d

revert changes to forge-std, it now matches main

01555fe

parzival1821 force-pushed the validate-payment-loop branch from 17eee48 to 01555fe Compare October 21, 2025 18:26

wjmelements reviewed Oct 21, 2025

View reviewed changes

Revert openzeppelin-contracts submodule to match main

bc6b81a

wjmelements reviewed Oct 22, 2025

View reviewed changes

service_contracts/src/FilecoinWarmStorageService.sol Outdated Show resolved Hide resolved

Update service_contracts/src/FilecoinWarmStorageService.sol

85b716c

wjmelements reviewed Oct 22, 2025

View reviewed changes

service_contracts/src/FilecoinWarmStorageService.sol Outdated Show resolved Hide resolved

wjmelements added 3 commits October 22, 2025 14:06

simplify

8df221a

doc

6d5889f

rm unused var

abe64da

wjmelements approved these changes Oct 22, 2025

View reviewed changes

github-project-automation bot moved this from 🔎 Awaiting review to ✔️ Approved by reviewer in FS Oct 22, 2025

wjmelements merged commit af644df into FilOzone:main Oct 22, 2025
6 checks passed

github-project-automation bot moved this from ✔️ Approved by reviewer to 🎉 Done in FS Oct 22, 2025

wjmelements reviewed Oct 22, 2025

View reviewed changes

wjmelements mentioned this pull request Oct 22, 2025

feat(IValidator): Allow Forward Progress Despite Missing Proofs #321

Merged

rjan90 mentioned this pull request Oct 27, 2025

build: prep FWSS v1.0.0 release #332

Merged

perf(validatePayment): loop over proving periods #267

perf(validatePayment): loop over proving periods #267

Uh oh!

Conversation

parzival1821 commented Oct 8, 2025

Related Issue

Current Progress

What I Need Feedback On

Questions for Maintainers

Changes Made

Next Steps (after approval)

Uh oh!

parzival1821 commented Oct 8, 2025

Uh oh!

parzival1821 commented Oct 8, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wjmelements commented Oct 9, 2025

Uh oh!

parzival1821 commented Oct 12, 2025

Uh oh!

wjmelements commented Oct 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

parzival1821 commented Oct 16, 2025

Uh oh!

wjmelements commented Oct 16, 2025

Uh oh!

parzival1821 commented Oct 17, 2025

Uh oh!

rjan90 commented Oct 17, 2025

Uh oh!

parzival1821 commented Oct 17, 2025

Uh oh!

rjan90 commented Oct 17, 2025

Uh oh!

parzival1821 commented Oct 17, 2025

Uh oh!

wjmelements commented Oct 17, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parzival1821 commented Oct 23, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wjmelements commented Oct 16, 2025 •

edited

Loading