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

[Protocol3] Batched proof verification #283

Merged
merged 20 commits into from
Jul 25, 2019
Merged

[Protocol3] Batched proof verification #283

merged 20 commits into from
Jul 25, 2019

Conversation

Brechtpd
Copy link
Contributor

Reduces the cost of verifying a proof from 550,000 gas/proof to about 200,000 gas/proof.

# Conflicts:
#	packages/loopring_v3/ABI/version30/IExchange.abi
#	packages/loopring_v3/contracts/iface/IExchange.sol
#	packages/loopring_v3/test/testExchangeBlocks.ts
@Brechtpd Brechtpd marked this pull request as ready for review July 22, 2019 19:21
# Conflicts:
#	packages/lightcone_v2.js/src/lib/wallet/ethereum/walletAccount.ts
#	packages/lightcone_v2.js/src/sign/exchange.ts
#	packages/lightcone_v2.js/src/wallet/metaMask.ts
@Brechtpd
Copy link
Contributor Author

@dong77 Ready for review.

Unfortunately this PR has an extremely messy diff because of the automatic formatting that is now done when committing.

@dong77
Copy link
Contributor

dong77 commented Jul 23, 2019

@dong77 Ready for review.

Unfortunately this PR has an extremely messy diff because of the automatic formatting that is now done when committing.

@Brechtpd no worries. @letsgoustc and myself will probably need a week to review this PR. Please be patient.

dong77
dong77 previously approved these changes Jul 25, 2019
require(circuit.enabled == true, "ALREADY_DISABLED");
circuit.enabled = false;

emit CircuitDisabled(blockType, onchainDataAvailability, blockSize, blockVersion);
Copy link
Contributor

Choose a reason for hiding this comment

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

break into multiple lines.

circuit.registered = true;
circuit.enabled = true;

emit CircuitRegistered(blockType, onchainDataAvailability, blockSize, blockVersion);
Copy link
Contributor

Choose a reason for hiding this comment

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

break into multiple lines.

ExchangeData.State storage S,
uint blockIdx,
uint256[8] memory proof
uint[] memory blockIndices,
Copy link
Contributor

Choose a reason for hiding this comment

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

should we check the max length of blockIndies?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no hard limit (except for the Ethereum gas limit/block). There's also a good chance verifying a proof will get even cheaper in Istanbul so it's up to the operator to decide how many proofs can be successfully verified.

letsgoustc
letsgoustc previously approved these changes Jul 25, 2019
@dong77 dong77 dismissed stale reviews from letsgoustc and themself via ea5ba1e July 25, 2019 13:26
@dong77 dong77 self-requested a review July 25, 2019 13:27
@dong77 dong77 merged commit 5c36b12 into master Jul 25, 2019
@dong77 dong77 deleted the batch-verification branch September 18, 2019 04:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants