feat: implement PDP V0 proof fees #82

aarshkshah1992 · 2024-12-04T14:47:41Z

Implements PDP proof fees as flushed out and documented here.

The Proof fees are proportional to the number of challenges and size of the proof set being proved. They're burnt and are paid by the SP once every proving period when they call provePossesion.

However, if the gas fee for a call to provePossesion is >= 5 % of daily estimated storage reward for an SP, then no proof fee is charged.

Please see the linked doc for more details.

Makefile

src/Log2.sol

ZenGround0

Looking good I'll do a more thorough review tomorrow.

ZenGround0 · 2024-12-04T15:12:52Z

src/Fees.sol

+    uint256 constant SYBIL_FEE = ONE_FIL / 10;
+
+    // assume 1FIL = $5 for now -> we can change this to use an oracle in PDP V1
+    uint256 constant FIL_USD_PRICE = 5;


I know we've gone over this but noting the fragility of this approach again given the reality of volatility. If FIL sits at $10 USD this reduces fee by half.

@ZenGround0

See discussion here

We can either ship this now with this price for PDP V0 and revisit in PDP V1 etc once the on chain payments/FWS story becomes clearer

Change this to use an oracle right now that gives us a real time price feed and live with the complexity.

What are your thoughts ?

recaping here that we decided to move forward implementing a call to a price oracle already in v0!

[not relevant since we are going for the price oracle, just putting this here for clarity: in the current case, if the fil price is higher than what is set up here, the fee is going to be higher than expected (i.e. if FIL gets to 10 and price is set to 5 in the contract, fee would be 2x the expected value)]

Have you considered using a stablecoin as payment for PDP? We'd love to work with you to figure out how to make this happen!

@usdfil we plan build a payment pathway with fungible token support - but that will be the next milestone & will be outside of the proof contract itself (out of the scope of this PR). It is better to follow #fil-pdp, and we would like to get your support when we start working on payments.

test/PDPVerifier.t.sol

src/PDPVerifier.sol

ZenGround0 · 2024-12-04T16:03:11Z

src/PDPVerifier.sol

        address listenerAddr = proofSetListener[setId];
        if (listenerAddr != address(0)) {
-            PDPListener(listenerAddr).posessionProven(setId, proofSetLeafCount[setId], seed, proofs.length);
+            PDPListener(listenerAddr).posessionProven(


nit: without a linter enforcing a particular style these editor style refactors are noisy and probably best to avoid. I would guess the style of the code is now less consistent since the style change here only shows up in places that you've edited.

src/PDPVerifier.sol

src/Fees.sol

lucaniz · 2024-12-05T14:14:57Z

src/Fees.sol

+    // 5% of daily reward
+    uint256 constant FIVE_PERCENT_DAILY_REWARD_ATTO_FIL =
+        (DAILY_TIB_STORAGE_REWARD_ATTO_FIL * 5) / 100;
+


Same comment as above holds here as well.

And also for 4% daily reward.

lucaniz · 2024-12-05T14:41:35Z

src/Fees.sol

        } else {
-            gasPrice = ONE_NANO_FIL;
+            uint256 calculatedProofFee = (PROOF_PRICE_ATTO_FIL *


I think we are calculating ProofFee the wrong way.

ProofPrice is supposed to be different than the one calculated above
See https://www.notion.so/filecoindev/Pricing-mechanism-for-PDPverifier-12adc41950c180ea9608cb419c369ba4?pvs=4#145dc41950c180f5be05f413ccf3b644

cc @irenegia

@lucaniz It's just a naming problem. I asked this above too. This function returns the total amount of "fee/price" that the prover has to pay.

Please see #82 (comment).

ZenGround0 · 2024-12-10T12:47:38Z

@aarshkshah1992 what's the deal with checking in the node modules but not the solidity libraries? When you cleaned up the repo you seemed to be keeping dependencies out. Is there a good reason to keep some dependencies and not others?

I'm also wondering if we can use a standard approach to dependencies. Using both node modules and git submodules seems like a lot of cognitive overhead. Is there anyway to avoid using node here?

aarshkshah1992 · 2024-12-10T12:56:31Z

@ZenGround0 Removed the node_modules/. We will install them as part of CI.

Unfortunately, as per Pyth docs -> https://docs.pyth.network/price-feeds/use-real-time-data/evm

we need to use node for Pyth. However, no biggie. Just run make install locally and it will all be taken care of you.

aarshkshah1992 · 2024-12-10T12:58:06Z

@ZenGround0 Please don't review it yet (I mean your call). Wanna go through all of the code once again and convince myself it's correct (also wanna test on Calib).

aarshkshah1992 · 2024-12-11T09:02:27Z

@ZenGround0 This is now ready for review.

lucaniz · 2024-12-11T13:02:28Z

src/Fees.sol

+    uint256 constant SYBIL_FEE = FIL_TO_ATTO_FIL / 10;
+
+    // 2 USD/Tib/month is the current reward earned by Storage Providers
+    uint256 constant MONTHLY_TIB_STORAGE_REWARD_USD = 2;


minor (feel free to ignore): I'd personally avoid the word "reward" to avoid misunderstading withblock reward. Maybe income? idk...

It would also help to say "estimated current reward"

lucaniz · 2024-12-11T13:04:02Z

src/Fees.sol

+    uint256 constant USD_DECIMALS = 1e18;
+
+    // 1 TiB in bytes (2^40)
+    uint256 constant TIB_IN_BYTES = 2 ** 40;


minor (again, feel free to ignore): i like more bytes_in_1TiB, but I guess it's personal taste

Lets keep it same for now.

ZenGround0

Reviewed Verifier, moving onto Fees, then tests.

src/Fees.sol

.github/workflows/makefile.yml

Makefile

src/PDPVerifier.sol

ZenGround0 · 2024-12-12T08:55:02Z

src/PDPVerifier.sol


    // Events
    event ProofSetCreated(uint256 indexed setId);
    event ProofSetDeleted(uint256 indexed setId, uint256 deletedLeafCount);
    event RootsAdded(uint256 indexed firstAdded);
    event RootsRemoved(uint256[] indexed rootIds);
+    event ProofFeePaid(uint256 indexed setId, uint256 fee);


src/Fees.sol

ZenGround0 · 2024-12-12T09:03:14Z

src/PDPVerifier.sol

@@ -174,6 +182,12 @@ contract PDPVerifier is Initializable, UUPSUpgradeable, OwnableUpgradeable {
        return nextChallengeEpoch[setId];
    }

+    // Returns the listener address for a proof set
+    function getProofSetListener(uint256 setId) public view returns (address) {


Confused about this as its already on main .

I see you merged main into this branch so it makes some sense. Weird that github UI sees this as a diff though seems almost like a github bug? Probably just advanced git edge cases.

Yeah really weird. I see the function exists only once in this file so this change has not added anything new here.

src/PDPVerifier.sol

ZenGround0 · 2024-12-12T09:32:19Z

src/PDPVerifier.sol

+
+        // Return the price and exponent representing USD per FIL
+        return (uint64(priceData.price), priceData.expo);
+    }


Thanks for getting this all so clean. It looks great.

ZenGround0 · 2024-12-12T09:50:06Z

src/Fees.sol

+    uint256 constant SYBIL_FEE = FIL_TO_ATTO_FIL / 10;
+
+    // 2 USD/Tib/month is the current reward earned by Storage Providers
+    uint256 constant MONTHLY_TIB_STORAGE_REWARD_USD = 2;


It would also help to say "estimated current reward"

ZenGround0 · 2024-12-12T10:04:00Z

src/Fees.sol

+        // Calculate reward per epoch per byte (in AttoFIL)
+        uint256 rewardPerEpochPerByte;
+        if (filUsdPriceExpo >= 0) {
+            rewardPerEpochPerByte = (MONTHLY_TIB_STORAGE_REWARD_USD * FIL_TO_ATTO_FIL) / (TIB_IN_BYTES * EPOCHS_PER_MONTH * filUsdPrice * (10 ** uint32(filUsdPriceExpo)));


Does pyth oracale have any notion of precision? The way things are structured here we appear to lose precision exponentially as price increases. Assuming filUsdPrice is an integer that needs not precision adjustments as we use it here and the pyth oracle can give us FIL price of $1,$2,$3,...,$9, $10, $20, $30,... $100, $200, $300. If this is the best we can do it is massively better than just estimating with a constant. But ideally we could get constant precision for any price.

Ah ok, looking at the docs I think it will typically return a negative exponent and an integer containing lots of precision. So in this case say FIL = 6.886768 then fiprice will be 6886768 and exponent will be -6 to move the decimal to the correct place. So in this case the precision will be accounted for.

~~Is this how you've seen it work @aarshkshah1992 ?~~

Yes what you are saying is exactly correct. So we need to handle both positive and negative exponents here.

I think I was just confusing myself imagining price being between 1 and 10 for some reason. You can make the price be 133 or 16666 just using the price integer. I'd be curious to see how much precision the oracle gives and what representation they typically choose. But I'm not concerned about this anymore.

@ZenGround0 I think the exponent is more useful for prices < 1 USD 😅

src/Fees.sol

ZenGround0 · 2024-12-12T10:13:45Z

src/Fees.sol

        }
-        uint256 numerator = PROOF_GAS_FLOOR * challengeCount * gasPrice;
-        uint256 denominator = ONE_PERCENT_DENOM;
-        return numerator / denominator;
    }



Since the last iteration the overall fee calculation has become so much cleaner thanks everybody for simplifying.

ZenGround0

While I've left a bunch of comments I don't think anything is strictly blocking. The most important thing is initializing the proofSetLastProvenEpoch state variable.

ZenGround0 · 2024-12-12T10:34:43Z

test/Fees.t.sol

+contract PDPFeesTest is Test {
+    uint256 constant epochs_per_day = 2880;
+
+    function computeRewardPerPeriod(uint64 filUsdPrice, int32 filUsdPriceExpo, uint256 rawSize) internal pure returns (uint256) {


nit: Since you are repeating code between tests and the main contract a pattern you might like to know about:

declare this function as a private / internal function in the verifier contract

In tests create a contract type that inherits from the verifier and can therefore use its private methods

Call the function on this contract as part of your testing here.

This is just a thought. I do like seeing reward per epoch per byte computed directly inline of the proof fee function in the contract so not clear exactly which is cleanest.

FYI I do this here if you're looking to copy the pattern: https://github.com/FilOzone/pdp/blob/main/test/PDPVerifier.t.sol#L850

Here I expose a testing version of heightFromIndex.

@ZenGround0 I want to keep it this way as I dont want to rely on the code being tested to test the code. This function helps calculates the fee as it should be and then we match that what's written by the code under test is the same.

ZenGround0 · 2024-12-12T11:08:29Z

test/Fees.t.sol

+
+        uint256 rewardPerPeriod = computeRewardPerPeriod(filUsdPrice, filUsdPriceExpo, rawSize);
+
+        uint256 gasLimitLeft = (rewardPerPeriod * PDPFees.GAS_LIMIT_LEFT_PERCENTAGE) / 100;


nit: put these as contract constants so you don't have to recompute

Not sure what you mean. These need to be computed on the fly as the rewardPerPeriod is not a constant.

ZenGround0 · 2024-12-12T11:10:30Z

test/Fees.t.sol

+        uint256 gasLimitLeft = (rewardPerPeriod * PDPFees.GAS_LIMIT_LEFT_PERCENTAGE) / 100;
+        uint256 gasLimitRight = (rewardPerPeriod * PDPFees.GAS_LIMIT_RIGHT_PERCENTAGE) / 100;
+
+        uint256 estimatedGasFee = (gasLimitLeft + gasLimitRight) / 2;


nit: would be nice to add to this case tests at the boundaries, i.e. estimatedGasFee = gasLimitLeft and estimatedGasFee = gasLimitRight - 1

ZenGround0 · 2024-12-12T11:43:49Z

test/Fees.t.sol

+        assertTrue(fee > 0, "Fee should be positive with negative exponent");
+    }
+
+    function testProofFeeWithGasFeeBoundLargeRawSize() public pure {


I don't understand the point of this test.

Just a test with the biggest data size (1e30 bytes) among all the tests. Dosent hurt.

ZenGround0 · 2024-12-12T11:48:57Z

src/Fees.sol

+        // Calculate reward per epoch per byte (in AttoFIL)
+        uint256 rewardPerEpochPerByte;
+        if (filUsdPriceExpo >= 0) {
+            rewardPerEpochPerByte = (MONTHLY_TIB_STORAGE_REWARD_USD * FIL_TO_ATTO_FIL) / (TIB_IN_BYTES * EPOCHS_PER_MONTH * filUsdPrice * (10 ** uint32(filUsdPriceExpo)));


I think I was just confusing myself imagining price being between 1 and 10 for some reason. You can make the price be 133 or 16666 just using the price integer. I'd be curious to see how much precision the oracle gives and what representation they typically choose. But I'm not concerned about this anymore.

ZenGround0 · 2024-12-12T11:51:42Z

test/PDPVerifier.t.sol


        // Test 1: Sending less than the required fee
-        uint256 insufficientFee = correctProofFee - 1;
+
+        uint256 correctFee = 388051072754688;


Thanks, overall better to have this than not I think. Please leave a comment stating why we need to to it this way. If there is any more development on proving code we will run into failures every time here so at least we should make it easy to fix them. Probably a good idea to leave instructions here for that too.

ZenGround0 · 2024-12-12T11:54:23Z

test/PDPVerifier.t.sol

@@ -693,7 +732,21 @@ contract PDPVerifierProofTest is Test, ProofBuilderHelper {

        // Submit proof successfully, advancing the proof set to a new challenge epoch.
        vm.mockCall(pdpVerifier.RANDOMNESS_PRECOMPILE(), abi.encode(challengeEpoch), abi.encode(challengeEpoch));
-        pdpVerifier.provePossession{value: PDPFees.proofFee(proofs.length)}(setId, proofs);
+        // Mock Pyth oracle call to return $5 USD/FIL
+        bytes memory pythCallData = abi.encodeWithSelector(


These code blocks are repeated many times. I think you could encapsulate call data generation into a helper function of price and expo.

aarshkshah1992 · 2024-12-12T13:13:35Z

@ZenGround0 Have addressed all your comments. Please can you take a look ?

aarshkshah1992 · 2024-12-12T13:26:09Z

@ZenGround0 Have addressed all your review comments. Please can you give it one more look ?

aarshkshah1992 · 2024-12-12T13:32:34Z

@ZenGround0 Merging as discussed offline

implement proof fees

29b0552

aarshkshah1992 requested a review from ZenGround0 December 4, 2024 14:47

aarshkshah1992 added 2 commits December 4, 2024 18:49

fix CI build

ce59d9f

fix CI test job

db58e19

ZenGround0 reviewed Dec 4, 2024

View reviewed changes

Makefile Outdated Show resolved Hide resolved

src/Log2.sol Outdated Show resolved Hide resolved

ZenGround0 reviewed Dec 4, 2024

View reviewed changes

aarshkshah1992 added 5 commits December 5, 2024 12:47

replace Log2 library with BitOps

69db776

remove logs

98b36e0

remove extraneous docs

c5b2209

fix test regression

0509eac

Merge branch 'main' into feat/impl-proof-fees-v0

b885e31

lucaniz reviewed Dec 5, 2024

View reviewed changes

src/Fees.sol Outdated Show resolved Hide resolved

lucaniz reviewed Dec 5, 2024

View reviewed changes

aarshkshah1992 closed this Dec 10, 2024

aarshkshah1992 reopened this Dec 10, 2024

PDP proof fees

5452fcd

aarshkshah1992 added 2 commits December 10, 2024 16:50

do not commit node modules

a63c60c

fix CI

97a499b

aarshkshah1992 marked this pull request as draft December 10, 2024 12:57

aarshkshah1992 added 5 commits December 11, 2024 10:10

revert changes to BitOps.sol

be192ab

revert extraneous changes

d5ae6a8

remove extra changes

8a03038

revert to base

973f2d8

final changes

0581af2

aarshkshah1992 self-assigned this Dec 11, 2024

aarshkshah1992 marked this pull request as ready for review December 11, 2024 09:02

lucaniz reviewed Dec 11, 2024

View reviewed changes

ZenGround0 reviewed Dec 12, 2024

View reviewed changes

aarshkshah1992 added 5 commits December 12, 2024 15:24

change name of reward

20544b0

changes as per review

9dc4806

changes as per review

f010919

line break

e63d0cd

changes as per review

80a71f5

ZenGround0 approved these changes Dec 12, 2024

View reviewed changes

aarshkshah1992 added 4 commits December 12, 2024 16:42

helper function for pythCallData in tests

1a6a32b

add doc for correctFee

d1a5701

add tests as per review

a192803

fix test

e03809d

add tests for LastProvenEpoch

49347b9

aarshkshah1992 merged commit cac5c17 into main Dec 12, 2024
1 check passed


		uint256 rewardPerPeriod = computeRewardPerPeriod(filUsdPrice, filUsdPriceExpo, rawSize);

		uint256 gasLimitLeft = (rewardPerPeriod * PDPFees.GAS_LIMIT_LEFT_PERCENTAGE) / 100;

feat: implement PDP V0 proof fees #82

feat: implement PDP V0 proof fees #82

Conversation

aarshkshah1992 commented Dec 4, 2024 • edited Loading

ZenGround0 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

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

Choose a reason for hiding this comment

ZenGround0 commented Dec 10, 2024 • edited Loading

aarshkshah1992 commented Dec 10, 2024

aarshkshah1992 commented Dec 10, 2024

aarshkshah1992 commented Dec 11, 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

ZenGround0 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

Choose a reason for hiding this comment

ZenGround0 Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

ZenGround0 Dec 12, 2024 • edited Loading

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

ZenGround0 Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

ZenGround0 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aarshkshah1992 commented Dec 12, 2024

aarshkshah1992 commented Dec 12, 2024

aarshkshah1992 commented Dec 12, 2024

aarshkshah1992 commented Dec 4, 2024 •

edited

Loading

ZenGround0 commented Dec 10, 2024 •

edited

Loading

ZenGround0 Dec 12, 2024 •

edited

Loading

ZenGround0 Dec 12, 2024 •

edited

Loading

ZenGround0 Dec 12, 2024 •

edited

Loading