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

Update the state machine to support the universal nonce approach #468

Closed
3 tasks done
Tracked by #467
rach-id opened this issue Jun 5, 2022 · 3 comments
Closed
3 tasks done
Tracked by #467

Update the state machine to support the universal nonce approach #468

rach-id opened this issue Jun 5, 2022 · 3 comments
Assignees

Comments

@rach-id
Copy link
Member

rach-id commented Jun 5, 2022

As discussed under #464, changes will need to be done to the state machine to accommodate the universal nonce approach.
These changes include:

  • Updating the abci.endBlock to post valset requests with the correct nonce
  • Add checks on the valset/data commitment confirms checking the nonces
  • Add keepers/handlers tests checking the correctness of the nonces
@rach-id
Copy link
Member Author

rach-id commented Jun 11, 2022

To implement universal nonce inside the state machine, there are two approaches:

Replace nonce with universal_nonce

This approach consists of replacing the nonce field in attestations and requests to universal_nonce and index them using it.

Pros

Easy to implement

Cons

The problem with this approach is that the queries will be inconsistent. For example, we have 10 valsets, if I query for valset 5, there is a chance the request will fail as a data commitment will have universal nonce 5.
Thus, anyone querying the QGB would need to have prior knowledge of the universal nonce concept. I think this is not a good thing, as universal nonce is an internal mechanism used to introduce synchrony, not a user facing thing.

Also, the following issue should be investigated #483 to see if this implementation will expose the node to potential attack vectors.

Add an extra universal_nonce field

This consists of keeping the nonce field as it is, and adding a new field called universal_nonce.

Pros

This would keep the logical separation between valsets and data commitments and allow an easier interaction with the QGB module by users.

Cons

  • Introduce more state
  • Would require choosing an approach of the following when it comes to indexing attestations by universal nonce:
    • Add a mapping: universal_nonce => type {valset, data commitment}, nonce to retrieve the correct attestation based on a universal_nonce
    • Add a second index (as done in some other projects) where attestations will be indexed both by nonce and universal_nonce. This leads to two questions:
      • When indexing by two keys, will the data be duplicated? The following statement will be used to persist:
      store.Set(nonceKey, k.cdc.MustMarshal(&valset))
      store.Set(universalNonceKey, k.cdc.MustMarshal(&valset))
      The answer would require good knowledge of how the cosmos-sdk stores state internally. Because, if the data will be duplicated, it will be better to implement the above mapping instead.
      • Will add it as soon as I remember it :(

Personal opinion

I am more inclined towards the extra universal_nonce field. However, will need to investigate how indexing works internally.

@rach-id
Copy link
Member Author

rach-id commented Jun 11, 2022

@evan-forbes When you have time please, your input is so valuable.

@rach-id
Copy link
Member Author

rach-id commented Jun 15, 2022

We decided to move forwards with replacing nonce with universal_nonce and defining an Attestation interface to contain either data commitments or valsets.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

No branches or pull requests

1 participant