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

Add root storage #76

Merged
merged 3 commits into from
Oct 29, 2022
Merged

Add root storage #76

merged 3 commits into from
Oct 29, 2022

Conversation

lok52
Copy link
Collaborator

@lok52 lok52 commented Oct 12, 2022

Closes #62

Add separate root storage to relayer similar to roots mapping in ZkBobPool contract.
It allows more strict validation of users' transactions on relayer side reducing the possibility to drain relayer sending reverting transactions.
Logic flow is similar to nullifierSet, but root storage is built using HSET redis command, maintaining a mapping from a pool index to a corresponding root.

Applications states:

  • Initialization: on startup relayer fetches all transactions with corresponding calldata and starts building the storage from ground up.
    // Save root in confirmed state
    const root = this.state.getMerkleRoot()
    await this.state.roots.add({
    [newPoolIndex]: root,
    })
    }
  • Relayer received a transaction:
  1. Extract delta index used in proof construction and lookup a root by this index in confirmed and optimistic storages. If root is not found in any of storages then tx is invalid. If root is found but it is not equal to the root used in proof construction (we get it from public proof inputs) then tx is invalid.
    await checkAssertion(() => checkRoot(delta.transferIndex, root, pool.state.roots, pool.optimisticState.roots))
  2. Save the root in optimistic root storage.
  3. Check if tx is mined:
    3.1. Mined: remove root from optimistic storage and include it into confirmed storage.
    // Add root to confirmed state and remove from optimistic one
    const poolIndex = ((commitIndex + 1) * OUTPLUSONE).toString(10)
    logger.info('Adding root %s %s to PS', poolIndex, root)
    await pool.state.roots.add({ [poolIndex]: root })
    logger.info('Removing root %s %s from OS', poolIndex, root)
    await pool.optimisticState.roots.remove([poolIndex])

    3.2: Reverted: delete whole optimistic root storage
    logger.info('Clearing optimistic roots...')
    await pool.optimisticState.roots.clear()

@lok52 lok52 requested a review from akolotov October 12, 2022 13:11
@lok52 lok52 force-pushed the feature/root-validation branch from 3e5fb03 to 17af262 Compare October 12, 2022 13:12
@lok52 lok52 requested a review from r0wdy1 October 21, 2022 09:02
Copy link

@r0wdy1 r0wdy1 left a comment

Choose a reason for hiding this comment

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

Could you add a test for both good and bad cases?
This change seems to be rather difficult to test manually on staging environment, so it's better to have some test coverage

@lok52
Copy link
Collaborator Author

lok52 commented Oct 27, 2022

Could you add a test for both good and bad cases? This change seems to be rather difficult to test manually on staging environment, so it's better to have some test coverage

It is rather hard to implement such e2e test where multiple transactions will be in optimistic state and then make the first one fail. We need to run a local node in no-mining mode and manually send requests to mine blocks at specific points in time. I will try to make it, but it does not look straightforward for now.

@lok52 lok52 mentioned this pull request Oct 27, 2022
Copy link

@r0wdy1 r0wdy1 left a comment

Choose a reason for hiding this comment

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

looks good, although further testing is required

@akolotov
Copy link

@LyzeOfKiel please fix the merge conflict

@lok52
Copy link
Collaborator Author

lok52 commented Oct 28, 2022

Will fix conflicts after #78 will be merged in this branch

@akolotov
Copy link

Will fix conflicts after #78 will be merged in this branch

Could we merge this PR first and make a rebase for #78 so it will be targeted to devel?

@akolotov akolotov merged commit 8ee76d0 into devel Oct 29, 2022
@akolotov akolotov deleted the feature/root-validation branch October 29, 2022 12:26
akolotov added a commit that referenced this pull request Nov 1, 2022
This merge contains the following set of changes:
- Remove /proof_tx endpoint (#59)
- Add merkle root index validation (#60)
- Add endpoints for parameters hash (#65)
- Improvements in user tx validation (#69)
- Update ZkBobPool ABI (#71)
- Configuration ability fo DB files dir (#79)
- Bunch of improvements to robust transaction sending (#78)
- Maintenance of merkle root for more robust transaction verification (#76)
- Add fallback in root lookup method (#86)
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.

Relayer: Add more strict assertions on transaction proof verification {R7.11}
3 participants