Simplify fault design #67

ZenGround0 · 2024-10-23T17:25:00Z

No description provided.

magik6k · 2024-10-23T17:44:59Z

contracts/src/SimplePDPService.sol

@@ -170,9 +164,14 @@ contract SimplePDPService is PDPListener, PDPRecordKeeper, Initializable, UUPSUp
            uint256 periodsSkipped = 1;
            if (provingDeadlines[proofSetId] < block.number) {
                periodsSkipped = 1 + ((block.number - provingDeadlines[proofSetId]) / getMaxProvingPeriod());
+                provingDeadlines[proofSetId] = block.number + getMaxProvingPeriod(); // reset deadline


Shouldn't that also be provingDeadlines[proofSetId] = provingDeadlines[proofSetId] + getMaxProvingPeriod(); to keep the deadline in a fixed window?

The current way the proving window will keep sliding backwards as I have to compute and submit the proof some time before the deadline

SGTM but we need to catch up multiple windows if we missed multiple otherwise you need to call this a bunch. So you'll need to add periodsSkipped I think

Can't PR now but fee lfree to push

Some discussion about policy options here, it wants a bit of thought.

The current way the proving window will keep sliding backwards as I have to compute and submit the proof some time before the deadline

Note the line you commented on is only the unproven case, so it only slides until a valid proof is submitted. But note also there's no window or equivalent here that makes the deadline really matter: the proof can be submitted any time in the 24hr window (after the short challenge delay). So I don't think the scheduling matters as much as it does for Window PoST.

anorth

This would benefit from some tests demonstrating the behaviour, especially around when the proving deadline moves.

anorth · 2024-10-28T20:36:00Z

contracts/src/SimplePDPService.sol

@@ -170,9 +164,14 @@ contract SimplePDPService is PDPListener, PDPRecordKeeper, Initializable, UUPSUp
            uint256 periodsSkipped = 1;
            if (provingDeadlines[proofSetId] < block.number) {
                periodsSkipped = 1 + ((block.number - provingDeadlines[proofSetId]) / getMaxProvingPeriod());
+                provingDeadlines[proofSetId] = block.number + getMaxProvingPeriod(); // reset deadline


The current way the proving window will keep sliding backwards as I have to compute and submit the proof some time before the deadline

Note the line you commented on is only the unproven case, so it only slides until a valid proof is submitted. But note also there's no window or equivalent here that makes the deadline really matter: the proof can be submitted any time in the 24hr window (after the short challenge delay). So I don't think the scheduling matters as much as it does for Window PoST.

anorth · 2024-10-28T20:36:33Z

contracts/src/SimplePDPService.sol

            }
            emit FaultRecord(FaultType.SKIPPED, periodsSkipped);
-        }
+
+        } 


Need to update provingDeadlines[proofSetId] if there was a successful proof, too.

ZenGround0 · 2024-11-18T09:45:32Z

Because things were getting too complicated I did a small redesign here. This should now close
#71 #66 and #65

anorth

I think this is very close.

The grinding that I see remaining could only be exercised with integration tests, which we lack. I suggest following up to add some integration tests, with some of them explicitly constructed from an "SP trying to cheat" frame.

anorth · 2024-11-18T19:31:34Z

contracts/src/SimplePDPService.sol

+        uint256 periodsSkipped = (block.number - provingDeadlines[proofSetId]) / getMaxProvingPeriod();
+        uint256 faultPeriods = periodsSkipped;
+        if (!provenThisPeriod[proofSetId]) { 
+            // include previous unproven period 
+            faultPeriods += 1;
+        }
+        if (faultPeriods > 0) {
+            emit FaultRecord(faultPeriods);
        }
+        provingDeadlines[proofSetId] = provingDeadlines[proofSetId] + getMaxProvingPeriod()*periodsSkipped; 


Nit: I think this code could also handle the on-time case, with something like periodsSkipped = max(0, (block.number - provingDeadlines[proofSetId])) / getMaxProvingPeriod(). The rest is basically duplicate.

anorth · 2024-11-18T19:31:35Z

contracts/src/SimplePDPService.sol

+        // Can only get here if calling nextProvingPeriod multiple times within the same proving period
+        uint256 prevDeadline = provingDeadlines[proofSetId] - getMaxProvingPeriod();
+        if (block.number <= prevDeadline) {
+            return;


I think this needs to revert. As is, a provider could grind challenges by calling verifier.nextProvingPeriod once each epoch after a successful proof until the deadline is reached.

In general, I think any call to this method must either (1) advance the proving period (possibly with fault), or (2) revert.

This makes me think that, in the PDP Verifier contract, the method name nextProvingPeriod is not quite right. That contract doesn't know anything about proving periods. It could instead be newChallenge or something. However since that's the method actually invoked by SPs, I can see the motivation for a name that represents their expected workflow.

Thanks for the catch on the revert.

Yeah I'm still not happy with the name but there is some sort of proving period thing that the pdp verifier knows about. Namely when batched adds and removes get added into the proofset. I think this name is probably ok for now even though pdp service has a modified idea of proving period

contracts/test/SimplePDPService.t.sol

anorth · 2024-11-20T20:04:12Z

test/PDPVerifier.t.sol

-        initializeData = abi.encodeWithSelector(SimplePDPService.initialize.selector, address(pdpVerifier));
-        MyERC1967Proxy listenerProxy = new MyERC1967Proxy(address(listenerImpl), initializeData);
-        listener = SimplePDPService(address(listenerProxy));
+        listener = new TestingRecordKeeperService();


ZenGround0 requested review from anorth and magik6k October 23, 2024 17:25

magik6k reviewed Oct 23, 2024

View reviewed changes

anorth reviewed Oct 28, 2024

View reviewed changes

anorth reviewed Nov 18, 2024

View reviewed changes

ZenGround0 changed the title ~~Fixing up bug caught in delayed review~~ Simplify fault design Nov 19, 2024

ZenGround0 mentioned this pull request Nov 19, 2024

Setup Integration tests exercising PDPVerifier and PDPService contracts #74

Open

ZenGround0 added 7 commits November 20, 2024 20:32

Delayed review response

7aac663

Redesign faults

d358c3a

Fixup

7738ff8

Add the bulk of tests

7a41348

Found bug

89f4d2c

Added test

e0b6c83

Double call to NPP reverts

f3e854f

ZenGround0 force-pushed the fix/pdp-service-deadline-update branch from 9605d2a to f3e854f Compare November 20, 2024 15:06

ZenGround0 requested review from anorth and aarshkshah1992 November 20, 2024 15:07

anorth approved these changes Nov 20, 2024

View reviewed changes

ZenGround0 merged commit 19993af into main Nov 21, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify fault design #67

Simplify fault design #67

ZenGround0 commented Oct 23, 2024

magik6k Oct 23, 2024

ZenGround0 Oct 23, 2024

ZenGround0 Oct 23, 2024

anorth Oct 24, 2024

anorth Oct 28, 2024

anorth left a comment

anorth Oct 28, 2024

anorth Oct 28, 2024

ZenGround0 commented Nov 18, 2024

anorth left a comment

anorth Nov 18, 2024

anorth Nov 18, 2024

anorth Nov 18, 2024

ZenGround0 Nov 20, 2024

anorth Nov 20, 2024

Simplify fault design #67

Simplify fault design #67

Conversation

ZenGround0 commented Oct 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anorth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZenGround0 commented Nov 18, 2024

anorth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment