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

feat(rln): trustless root onchain #36

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

rymnc
Copy link
Contributor

@rymnc rymnc commented Feb 22, 2024

Description

Strips BinaryIMT of the restriction with storage. Root is now calculated in memory, and has parity with zerokit.
This way, we have cheap registrations and the root access is free, win-win!

Describe the changes made in your pull request here.

Checklist

Ensure you completed all of the steps below before submitting your pull request:

  • Added natspec comments?
  • Ran forge snapshot?
  • Ran pnpm lint?
  • Ran forge test?
  • Ran pnpm verify?

@rymnc rymnc self-assigned this Feb 22, 2024
@rymnc
Copy link
Contributor Author

rymnc commented Feb 22, 2024

@alrevuelta new deployment - 5ef581e

@rymnc rymnc requested a review from alrevuelta February 22, 2024 19:41
/// @dev Computes the root of the tree given the leaves.
/// @param self: Tree data.
/// @param leaves: Leaves in the tree
function calcRoot(
Copy link

@alrevuelta alrevuelta Feb 23, 2024

Choose a reason for hiding this comment

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

is this taken from somewhere? can we add unit tests?

does it work for eg 10.000 memberships? even view functions have limits so I suspect with a high amount of leaves.length this wont work, since too many hashes to do.

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 a test for 20 memberships as well as the gas snapshot has been committed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've linked the original implementation before modifying it to not use storage

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this function is not meant to be called by other contracts, and will only be resolved at the node level through eth_call's

@@ -278,4 +281,15 @@ abstract contract RlnBase {
}
return commitments;
}

function root() public view returns (uint256) {

Choose a reason for hiding this comment

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

related to https://github.com/vacp2p/rln-contract/pull/36/files#r1500330402

does this work with hundred or thousand of leafs? calcRoot has to do tons of hashes.

even view functions have limits so I suspect with a high amount of leaves.length this wont work, since too many hashes to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if it's above 500M gas then it will be rejected by popular rpc providers, otherwise it's okay.

Choose a reason for hiding this comment

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

500M seems too high.

https://community.infura.io/t/gas-limit-on-eth-call/1115
Says: 19 981 536

https://docs.infura.io/api/networks/ethereum/json-rpc-methods/eth_estimategas

To prevent abuse of the API, the gas parameter in this eth_estimateGas method and in eth_call is capped at 10x (1000%) the current block gas limit. You can recreate this behavior in your local test environment (besu, geth, or other client) via the rpc.gascap command-line option.

and from geth 50 000 000

From geth
--rpc.gascap value (default: 50000000)

How many leafs do we need to hit this limit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@rymnc rymnc Feb 23, 2024

Choose a reason for hiding this comment

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

caps out at 255 leaves (100M gas) (127 leaves @ 50M gas), not ideal, but perhaps we can split the computation and make the calcRoot function accept an array of prefilled nodes of the tree, and make multiple rpc calls to calcRoot like pagination

Choose a reason for hiding this comment

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

ow yeap quite small tree which makes it not practical. It could be interesting to do so, but perhaps with that it loses the purpose. I mean, with this a client has to keep calling back and forth, downloading the leafs, keeping some local state, which is error prone and makes me wonder if its practical. Of course its more gas efficient than the LazyIMT approach, so we should asses what matters more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

applications which will have smaller trees can probably use this - and have the benefits that this approach brings. will think of another approach how we can consume less gas and make the least amount of rpc calls.

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.

2 participants