Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Decentralized prover #97

Merged
merged 40 commits into from
Sep 10, 2024
Merged

Decentralized prover #97

merged 40 commits into from
Sep 10, 2024

Conversation

AllFi
Copy link

@AllFi AllFi commented Dec 8, 2023

The motivation behind this PR is to take a step forward towards enabling permissionless operators. To achieve this objective, the PR introduces the concept of a pending commitments queue.

In this PR, the following changes were implemented:

  • The pendingCommitments have been added to the pool contract. It is a queue that stores all commitments waiting to be added to the Merkle tree.
  • The transact function no longer updates merkle root and pool index or requires a tree proof and root after. Instead, it merely appends a new commitment to the pendingCommitments. This modification enables the submission of transactions in parallel since the ordering is now executed on-chain.
  • The appendDirectDeposits function was also modified accordingly.
  • A new method called proveTreeUpdate has been introduced. This method accepts a tree update proof and updates the Merkle root and pool index if the proof is valid. The operator who submits the proof receives a tree update fee.
  • The fee structure has been divided into a transact fee and a tree update fee. The operator who submits a transaction receives the transact fee, while the operator who calls proveTreeUpdate receives the tree update fee. To ensure motivation for submitting a tree update proof, a new state variable called minTreeUpdateFee has been introduced.
  • Additionally, a prover address has been added to the transact calldata to prevent operator frontrunning.
    Currently, there is no mechanism for the appendDirectDeposit function to guard against front-running attacks, making such attacks still possible in this case.
  • To address the race condition during proveTreeUpdate, the operator submitting the first transaction can specify a privileged prover who will have a grace period to submit the tree update proof. To support this, a new field, gracePeriod, has been added.
  • AllowListOperatorManager was implemented to support configuration with multiple operators.
  • The memo message size has been added to simplify the implementation of the extra data later.
  • Support for the new message prefix has been added. This is essential to support the new memo encryption scheme described in [Research] Replace ECDH with HKDF during shared secret encryption libzeropool-zkbob#14.

TODO:

  • Rename the transact function to allow the UI to distinguish between new and old calldata versions.
  • Add version in the transact calldata.
  • Remove forced exit?
  • A storage collision exists because the pool contract lacks a usable gap. Figure out how to fix it.
  • Add a gap for the future updates?
  • Update scripts.

Deploy:

  • Don't forget to call setTokenSeller after the upgrade

src/zkbob/manager/AllowListOperatorManager.sol Outdated Show resolved Hide resolved
src/zkbob/manager/AllowListOperatorManager.sol Outdated Show resolved Hide resolved
src/zkbob/manager/AllowListOperatorManager.sol Outdated Show resolved Hide resolved
src/zkbob/utils/PriorityQueue.sol Outdated Show resolved Hide resolved
src/zkbob/utils/PriorityQueue.sol Outdated Show resolved Hide resolved
src/zkbob/utils/PriorityQueue.sol Outdated Show resolved Hide resolved
src/zkbob/utils/PriorityQueue.sol Outdated Show resolved Hide resolved
src/zkbob/ZkBobPool.sol Outdated Show resolved Hide resolved
src/zkbob/ZkBobPool.sol Outdated Show resolved Hide resolved
src/zkbob/ZkBobPool.sol Show resolved Hide resolved
AllFi and others added 2 commits January 31, 2024 15:42
Co-authored-by: Kirill Fedoseev <kirill@blockscout.com>
Copy link
Collaborator

@k1rill-fedoseev k1rill-fedoseev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link

@EvgenKor EvgenKor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@AllFi
Copy link
Author

AllFi commented Feb 26, 2024

Outdated comment I have updated the `ZkBobPool.s.sol`, `Local.s.sol`, and `DeployZkBobPoolModules.s.sol` scripts to accommodate modifications in this PR. Additionally, I have added `DeployAllowListOperatorManager.s.sol` to independently deploy `AllowListOperatorManager`, as it is not deployed within `DeployZkBobPoolModules.s.sol`. `DeployZkBobPoolModulesTest.t.sol` includes a fork test that tests the upgrade of the USDC pool on Optimism using the `DeployZkBobPoolModules` script.

Modifications in ZkBobPool.s.sol include:

  • Deploying AllowListOperatorManager instead of MutableOperatorManager. The contract will be deployed with parameters zkBobRelayer, zkBobRelayerFeeReceiver, and allowListEnabled from Env.s.sol.
  • Setting accounting.kycProviderManager if defined in Env.s.sol.
  • Setting accounting limits to values identical to those in the DeployZkBobPoolModules.s.sol script.
  • Setting ZkBobPool.gracePeriod and ZkBobPool.minTreeUpdateFee.
  • Setting AllowListOperatorManager.owner to the owner specified in Env.s.sol, if defined

Modifications in DeployZkBobPoolModules.s.sol include:

  • Setting ZkBobPool.gracePeriod and ZkBobPool.minTreeUpdateFee.
  • Setting ZkBobPool.tokenSeller, as the previous value will be in __deprecatedGap2 after the implementation update.
  • Removing the increment of txCount

The ZkBobPool.s.sol might be used to deploy a new pool, a combination of DeployZkBobPoolModules.s.sol and DeployAllowListOperatorManager.s.sol might be used to upgrade the existing pool.

Notes:

  • The DeployZkBobPoolModules.s.sol script assumes that the proxyAdmin == pool.owner == deployer
  • The parameters gracePeriod and minTreeUpdateFee should be updated before deployment
  • ZkBobPool.redeemer is not configured by any of the mentioned scripts
Differences in storage layout between current pool implementation and migration/opt-BOB-to-USDC (USDC pool on Optimism)
OLD LAYOUT:
0) Ownable._owner
1) ZkBobAccounting.slot0
2) ZkBobAccounting.slot1
3) ZkBobAccoutning.tiers
4) ZkBobAccounting.snapshots
5) ZkBobAccounting.userStats
6) ZkBobPool.operatorManager
7) ZkBobPool.nullifiers
8) ZkBobPool.roots
9) ZkBobPool.all_messages_hash
10) ZkBobPool.accumulatedFee
11) ZkBobTokenSellerMixin.tokenSeller

NEW LAYOUT:
0) Ownable._owner                                  | SAME
1-3) ZkBobPool.__deprecatedGap                     | DEPRECATED
4) ZkBobPool.redeemer                              | REUSE
5) ZkBobPool.accounting AND ZkBobPool.pool_index   | REUSE
6) ZkBobPool.operatorManager                       | SAME
7) ZkBobPool.nullifiers                            | SAME
8) ZkBobPool.roots                                 | SAME
9) ZkBobPool.all_messages_hash                     | SAME
10) ZkBobPool.accumulatedFee                       | SAME
11) ZkBobPool.__deprecatedGap2                     | DEPRECATED
12-14) ZkBobPool.pendingCommitments                | NEW
15) ZkBobPool.gracePeriod AND minTreeUpdateFee     | NEW
16-65) ZkBobPool.__gap                             | NEW
66) ZkBobTokenSellerMixin.tokenSeller              | NEW

@AllFi AllFi mentioned this pull request Feb 26, 2024
@AllFi
Copy link
Author

AllFi commented Mar 22, 2024

  • Add assigned prover address to the transactV2 calldata

@AllFi
Copy link
Author

AllFi commented Aug 19, 2024

I believe this PR is ready for the final review.

Deployment plan:

  1. Set TOKEN_NUMERATOR to 1000 before the upgrade.
  2. Deploy the new pool implementation: ./script/deploy.sh ./script/scripts/NewZkBobPoolUSDCImpl.s.sol --tc DeployNewZkBobPoolUSDCImpl --broadcast --verify
  3. Set the proxies', provers', and operator manager owner addresses in the DeployAllowListOperatorManager.s.sol file.
  4. Deploy the new operator manager: ./script/deploy.sh ./script/scripts/DeployAllowListOperatorManager.s.sol --tc DeployAllowListOperatorManager --broadcast --verify
  5. In the MigrateDecentralization.s.sol file, set the newZkBobPoolImpl and newOperatorManager addresses. Also, configure the gracePeriod and minTreeUpdateFee values appropriately.
  6. Run the migration script: ./script/deploy.sh ./script/scripts/MigrateDecentralization.s.sol --tc MigrateDecentralization --sender 0xbe7D4E55D80fC3e67D80ebf988eB0E551cCA4eB7
  7. Use run-latest.json to prepare Safe proposal

Parameter Values:

  • gracePeriod: I believe 3 minutes is a good starting point. It is short enough to maintain protocol liveness in a worst-case scenario, yet long enough to provide an adequate window in a normal scenario.
  • minTreeUpdateFee: I computed the max, min, and mean transaction fees for the last 3 months for the OP-USDC pool. The results are as follows:
Mean: 0.16172255227554794 USD
Max: 20.095211035222057 USD
Min: 0.001998291024255525 USD

Considering these values, I think a fee of 0.1 USDC should cover the tree update cost in most cases. It's still relatively low, so the overall fee impact will remain minimal.

@AllFi AllFi marked this pull request as ready for review August 19, 2024 12:58
@k1rill-fedoseev k1rill-fedoseev merged commit 1677f1e into develop Sep 10, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants