PDPVerifier compatibility broken after extsload refactor

[rvagg]

Specifically, we need to conform to IPDPProvingSchedule to make a PDP server (Curio) happy:

1. PDPVerifier stores a listener address for each data set (FilecoinWarmStorageService) as instructed by the client
2. Curio retrieves this listener address via pdpVerifier.GetDataSetListener(dataSetId)
3. Curio then binds this listener address as an IPDPProvingSchedule interface
4. Curio calls these methods on the listener:

• getMaxProvingPeriod() - to know the proving period duration
• challengeWindow() - to know the challenge window size
• nextChallengeWindowStart(setId) - to calculate timing
• initChallengeWindowStart() - for initialisation
• getChallengesPerProof() - to know how many challenges needed

We currently need to provide the writable FilecoinWarmStorageService address as the listener (we tell Curio this is the recordKeeper for our data set when we make one), and it gets calls from PDPVerifier, so it has to be our writable contract. Unfortunately, these methods got moved over to FilecoinWarmStorageServiceStateView in our refactor, so they're no longer available and we no longer conform to IPDPProvingSchedule and Curio can't figure out what to do.

So we have to come up with a way to make this all work again.

• Put those methods back and somehow bind that interface so we know we're not going to break in future, and just accept these are necessary bloat?
• Pass two contract addresses to Curio, read/write? (this would be very trusting of Curio to accept but we are already talking about whitelisting).
• Expose a getViewAddress() method or similar on FilecoinWarmStorageService and teach Curio to use that to find its IPDPProvingSchedule (this would be neat for the SDK too so we don't have to hardcode two addresses).
• Some other creative solution.

Additionally, this exposes an error condition that has no way of feeding back to the client and as far as the client is concerned it succeeds. PDPVerifier says "yep, all good, you have a data set now with that listener", but Curio realises too late that it can't talk to this listener to figure out what it needs.

I think we could achieve a basic check in PDPVerifier?

      if (listenerAddr != address(0)) {
          try IPDPProvingSchedule(listenerAddr).getMaxProvingPeriod() returns (uint64) {
              // goodo, this smells right
          } catch {
              revert("Listener must implement IPDPProvingSchedule");
          }
      }

Curio could probably also do the same thing before it even gets there, but I think that IPDPProvingSchedule is a PDPVerifier opinion so a check in there makes sense.

[wjmelements]

somehow bind that interface so we know we're not going to break in future

The normal way to do this is

import {IPDPProvingSchedule} from "@pdp/IPDPProvingSchedule.sol";
contract FilecoinWarmStorageService is IPDPProvingSchedule {

[wjmelements]

Pass two contract addresses to Curio, read/write?

This is my preferred solution

Expose a getViewAddress() method or similar on FilecoinWarmStorageService and teach Curio to use that to find its IPDPProvingSchedule (this would be neat for the SDK too so we don't have to hardcode two addresses).

There could be many view contracts for the same service contract. The view contract already links the other way:

contract FilecoinWarmStorageServiceStateView {
    ...
    FilecoinWarmStorageService public immutable service;

In the event they are the same, this interface could return address(self).

[wjmelements]

I think we could achieve a basic check in PDPVerifier?

  if (listenerAddr != address(0)) {
      try IPDPProvingSchedule(listenerAddr).getMaxProvingPeriod() returns (uint64) {
          // goodo, this smells right
      } catch {
          revert("Listener must implement IPDPProvingSchedule");
      }
  }

Solidity already reverts in this situation because it tests RETURNDATASIZE. Catching and reverting as you do here allows for a custom error message.

pragma solidity ^0.8.13;

import {Test} from "forge-std/Test.sol";

interface Important {
    function getMaxProvingPeriod() external view returns (uint64);
}
contract Typing {
    function tryThis(Important important) external view returns (uint64) {
        return important.getMaxProvingPeriod();
    }
}

contract TypingTest is Test {

    function test_nonConformant() public {
        Typing typing = new Typing();

        vm.expectRevert();
        typing.tryThis(Important(address(this)));
    }
}

[wjmelements]

getChallengesPerProof

I don't think this method exists anymore.

[wjmelements]

getChallengesPerProof

I don't think this method exists anymore.

I removed it here: https://github.com/FilOzone/filecoin-services/pull/100/files#diff-3c6b40a4c618b442226b6ec0ce0e4937e701306ef505c10918f2e86f4e1fd943L597

I will put it back, into the view contract

[wjmelements]

I have started work on branch IPDPProvingSchedule, which has the goal of getting the View contract to implement IPDPProvingSchedule. It is blocked in part because some methods marked pure in @pdp/IPDPProvingSchedule are actually view. I'll make a separate PR to pdp for that.

[wjmelements]

I have started work on branch IPDPProvingSchedule, which has the goal of getting the View contract to implement IPDPProvingSchedule. It is blocked in part because some methods marked pure in @pdp/IPDPProvingSchedule are actually view. I'll make a separate PR to pdp for that.

FilOzone/pdp#186
But I will also try for a solution that makes getMaxProvingPeriod and challengeWindow pure instead of view.

[wjmelements]

After changing the interface methods to view and updating the code generation to distinguish pure methods, I get

Error (2527): Function declared as pure, but this expression (potentially) reads from the environment or state and thus requires "view".
  --> src/FilecoinWarmStorageServiceStateView.sol:32:16:
   |
32 |         return service.getChallengesPerProof();
   |                ^^^^^^^

Error (2527): Function declared as pure, but this expression (potentially) reads from the environment or state and thus requires "view".
  --> src/FilecoinWarmStorageServiceStateView.sol:48:16:
   |
48 |         return service.getDataSetSizeInBytes(leafCount);
   |                ^^^^^^^
make: *** [build] Error 1

This is because service is an immutable variable and immutable variables cannot be read in pure functions. Because the immutable is ignored, these functions are indeed pure so I am going to update the code generation to this:

return FilecoinWarmStorageServiceStateInternalLibrary.getDataSetSizeInBytes(leafCount)

That will keep the function pure and allow us to implement the interface.