Generalized Proofs

[Sidu28]

No description provided.

[ChaoticWalrus]

potential bug: shouldn't this happen before the above? I think you are double-dipping if this line in particular isn't before the if-else block

[Sidu28]

Ah yeah this is missing a check.
Initially, I had it before the if/else statement. This was my original design: I imagined that risc0 always gets its fee first and we do not subtract it from "proven amounts". So essentially it would take longer for the podowner to "pay off" the proven sum.

G seems to disagree here, ie, if there is already a merkle proven balance, risc0 may not get a fee for proving it (probably a one time thing).

[Sidu28]

I've fixed, take a look. I think I still like the idea of separating out the fee before the if/else block

[ChaoticWalrus]

I think you may have misinterpreted my recommendation.
All I wanted to do was move the line that deducts the fee from the proven amount to above the if-else block, as then the fee would be paid to Risc0 correctly in all cases and your accounting would be fixed, since it was essentially just not accounting for the fee before (i.e. it was giving the "fee" amount both to the user and to Risc0).
The current version you have definitely looks wrong to me. why would you subtract from sumOfPartialWithdrawalsClaimedViaMerkleProvenGwei before removing the fee amount from provenPartialWithdrawalSumGwei?
will follow up with a comment of exactly what I'm suggesting.

[ChaoticWalrus]

I think it should look like:

        // subtract out proof service fee, to be paid at the end of operations
        uint64 remainingSumGwei = verifiedPartialWithdrawalBatch.provenPartialWithdrawalSumGwei - feeGwei;
        // subtract out partial withdrawals that may have been claimed via Merkle proofs, sending any remainder to user
        if (remainingSumGwei > sumOfPartialWithdrawalsClaimedViaMerkleProvenGwei) {
            remainingSumGwei -= sumOfPartialWithdrawalsClaimedViaMerkleProvenGwei;
            sumOfPartialWithdrawalsClaimedViaMerkleProvenGwei = 0;
            _sendETH_AsDelayedWithdrawal(podOwner, remainingSumGwei);
        } else {
            sumOfPartialWithdrawalsClaimedViaMerkleProvenGwei -= remainingSumGwei;
        }

        //send proof service their fee
        AddressUpgradeable.sendValue(payable(feeRecipient), feeGwei);

    }

do you see any problems with this or something equivalent?

[Sidu28]

This was my initial design exactly. G's design suggestion that we subtract the fee after, thus the current implementation: #346 (comment)

[ChaoticWalrus]

not to get too pedantic about what the code looked like before, but perhaps G was trying to point out the same bug as I did, as it appears it was present at the time as well.

I feel very confident that the present form is incorrect, and believe my suggestion is the simplest way to keep this accounting consistent + follow proper code conventions like CEI

[Sidu28]

ok wait I think I'm in agreement with you here - G's suggestion is that in the check, remainingSum should include the fee, i.e., don't subtract it before. Here's the state of the file when he made the comment. Sorry if im completely muddling this but this seems to be what you're suggesting now? I think either way the accounting wouldn't be necessarily correct its just whether or not we want to pay risc0 their fee from for that very first proof where sumOfPartialWithdrawalsClaimedViaMerkleProvenGwei is nonzero, which is probably what G was envisioning here

[ChaoticWalrus]

ahhh OK.
I would think Risc0 would want to get paid regardless, as they are doing work/computation here -- doesn't really feel fair for them to not pay them, tbh.

Ah, but I guess there's a risk where the funds to pay the fee aren't actually in the EigenPod in the event that the proven amount doesn't exceed sumOfPartialWithdrawalsClaimedViaMerkleProvenGwei ?
was that a worry before / reason for the require check specifically checking that the proven amount exceeds sumOfPartialWithdrawalsClaimedViaMerkleProvenGwei + feeGwei? Because now that check actually makes sense to me lol -- I hadn't considered this risk before.
perhaps we could just add a check that address(this).balance >= feeGwei ?
then I think we are OK in all instances, the system seems flexible, and Risc0 gets paid any time a proof is submitted successfully (which seems proper to me, at least).

[ChaoticWalrus]

again seems like this should probably be just another pausing flag to me.

I also think in this function is the more appropriate place to check this parameter (instead of also in the EigenPod), as it is at the start of the chain of calls.

[Sidu28]

We check it in _processPartialWithdrawal() which is called in _verifyAndProcessWithdrawal()l, which is in turn called in verifyAndProcessWithdrawals() which is called directly from the pod - that's why I added it as a separate check there. But I think we should make it a pausing flag for _processPartialWithdrawal() to standardize it a bit.

[ChaoticWalrus]

feels like it might be more appropriate to pause at the proofServiceCallback level?
I think doing so would be the same, but just cause calls to revert earlier when things are paused.
Feeling like we can eliminate the proofServiceEnabled flag then as well.

[Sidu28]

Yeah good point, I will add it as a flag at the EPM level - I think Im misunderstanding your point about proofServiceEnabled checks in the EigenPod (see my response above).

[ChaoticWalrus]

I think I was viewing the proofServiceEnabled checks and the way you're using this PAUSED_EIGENPODS_FULFILL_PARTIAL_WITHDRAWAL_PROOF_REQUEST flag as redundant.
Perhaps I read wrong, but it seems like:
a user calls the proof service,
which calls EigenPodManager.proofServiceCallback,
which checks the proofServiceEnabled flag (AND now checks the (redundant?) PAUSED_EIGENPODS_FULFILL_PARTIAL_WITHDRAWAL_PROOF_REQUEST flag),
which calls EigenPod.fulfillPartialWithdrawalProofRequest, (which previously checked the (redundant?) PAUSED_EIGENPODS_FULFILL_PARTIAL_WITHDRAWAL_PROOF_REQUEST flag)

[ChaoticWalrus]

I see what you're saying about pausing Merkle proof-based partial withdrawals now though! but I think if you want that functionality to remain, it should be paused with a separate flag.
Definitely feels like either we should leave the functionality in and design a system where both proof types can be unpaused/enabled simultaneously (which I believe the accounting model I suggested satisfies) OR just get rid of Merkle-proofs in this upgrade.
The proofServiceEnabled flag current doubles as a partialWithdrawalsViaMerkleProofsDisabled flag, which makes the whole thing effectively just an "upgrade switch" -- I feel like if we're doing that, we might as well just not have the flag or Merkle based partial withdrawal proofs at all!

[ChaoticWalrus]

The bones of this look good; thanks for taking it on.
I feel like there might be a bug to fix, think some cleanup can be done, and I'd like to see coverage added for the non-reverting cases, but it's headed in the right direction 👍

[ChaoticWalrus]

@Sidu28 if we leave in the ability to prove partial withdrawals via Merkle proofs, then I believe the _processPartialWithdrawal function needs an added check that the withdrawalTimestamp is after the the mostRecentWithdrawalTimestamp, as otherwise (as long as if either Merkle proofs and zk proofs are possible at the same time, or a transition is made Merkle->zk->Merkle) a user can zk prove a withdrawal and then Merkle-proof it afterwords, to "double dip".

This is definitely also a mark in favor of just fully removing Merkle-proving of partial withdrawals in this upgrade (as it's an example of the added complexity / potential dangers of having both proof methods active).

[Sidu28]

@Sidu28 if we leave in the ability to prove partial withdrawals via Merkle proofs, then I believe the _processPartialWithdrawal function needs an added check that the withdrawalTimestamp is after the the mostRecentWithdrawalTimestamp, as otherwise (as long as if either Merkle proofs and zk proofs are possible at the same time, or a transition is made Merkle->zk->Merkle) a user can zk prove a withdrawal and then Merkle-proof it afterwords, to "double dip".

This is definitely also a mark in favor of just fully removing Merkle-proving of partial withdrawals in this upgrade (as it's an example of the added complexity / potential dangers of having both proof methods active).

Discussed offline! We both agree that the current check being made here should suffice in allowing a switch back to merkle proving.

[ChaoticWalrus]

Closing this PR as stale.