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: Randomness module #117

Merged
merged 8 commits into from
Dec 13, 2023
Merged

feat: Randomness module #117

merged 8 commits into from
Dec 13, 2023

Conversation

hacheigriega
Copy link
Member

@hacheigriega hacheigriega commented Dec 4, 2023

Motivation

This PR fully implements the randomness module, which will be used as a source of randomness in SEDA protocol.

An example of a query:

$ seda-chaind query randomness seed
block_height: "7"
seed: 3e97ad3fea6ec9211b279fdf2bc26dd6b730fb3bc8a7f3400363e6500e4a86af

Explanation of Changes

The implementation takes advantages of two CometBFT ABCI methods: PrepareProposal, which is run only by the block proposer while building a block, and ProcessProposal, which is run by all validators while validating a block. In PrepareProposal the proposer computes the VRF proof by running VRF_Prove(secret_key, alpha), where secret_key is the consensus key and alpha, or the input message, is previous_seed || current_block_timestamp. The block proposer then signs and prepends the NewSeed transaction containing the VRF proof pi and the VRF hash output beta to the list of transactions in the block proposal.

Upon receiving this nicely prepared proposal in ProcessProposal, the validators first check if the first transaction in the list is the NewSeed sent by the current block proposer. If the check fails, they reject the block. If the check succeeds, the validators then run VRF_Verify(public_key, pi, alpha) with the public key obtained from the validator store. The validator accepts or rejects the block based on the verification result.

  • Consensus key is now generated using secp256k1 instead of ed25519 so that it is compatible with our VRF library.
  • Added a wrapper around x/staking message server to create a regular account based on the consensus key during create-validator transaction. A regular account is required when the block proposer signs and sends a NewSeed transaction.
  • The process fails on average after a few hundred blocks due to a bug in the vrf-go repo. There seems to be something wrong with scalar arithmetics.

Testing

Tested on a local single-node chain deployed manually and a two-node chain in e2e testing. In a single-node setting, the only proposer gets stuck due to the VRF bug and blocks don't get produced after some point. In a multi-node setting, this problem is prevented by the next proposer successfully creating a valid block.

To run the e2e test, create .netrc file in the project root and add the credentials for GitHub private repository access:

machine github.com
        login <YOUR_USERNAME>
        password <YOUR_GITHUB_TOKEN>

Then run make test-e2e. (Currently not working due to a Cosmos SDK genesis JSON marshalling error.. Should be fixed soon. e2e works locally now but all CI tasks are failing due to issues with downloading the vrf-go repo, which is private.)

Related PRs and Issues

Closes #112

@hacheigriega hacheigriega marked this pull request as ready for review December 7, 2023 15:22
@hacheigriega hacheigriega requested a review from a team December 7, 2023 15:22
Copy link
Contributor

@gluax gluax left a comment

Choose a reason for hiding this comment

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

Comments on the .netrc stuff, otherwise LGTM.

app/app.go Show resolved Hide resolved
x/randomness/keeper/abci.go Outdated Show resolved Hide resolved
DEVELOPING.md Outdated
Comment on lines 163 to 170
To run end-to-end tests, you first need to create a file `.netrc` containing GitHub credentials in the project root. This enables an access to the private repositories during the Docker build process. The `.netrc` file should look as follows:
```bash
machine github.com
login <YOUR_USERNAME>
password <YOUR_GITHUB_TOKEN>
```

The e2e testing can now be run with the following command:
Copy link
Contributor

Choose a reason for hiding this comment

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

Two things:

  1. Do you know if this is secure? I.e., does no command print out the GitHub username/token from the container?
  2. Can we not use a .env file instead?

Either way, an example file would be nice, like example.netrc or e2e.example.env. Then in the md you can link to the example file :).

Copy link
Member Author

Choose a reason for hiding this comment

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

The build process doesn't print it out, and the container just contains the file. Probably not ideal, but it's not any less secure than other commonly suggested solutions.

@hacheigriega hacheigriega merged commit 74aad54 into main Dec 13, 2023
4 of 6 checks passed
@hacheigriega hacheigriega deleted the hy/vrf branch December 13, 2023 01:02
Comment on lines +89 to +92
// err = mempool.Insert(ctx, tx)
// if err != nil {
// return nil, err
// }
Copy link
Contributor

Choose a reason for hiding this comment

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

In this case we probably wouldn't need to worry about the mempool at all. CometBFT is responsible for selecting txns from the mempool and proposing them in a new block. PrepareProposal then receives the proposed txns.

We are also already directly including the new seed txn in the proposal by which point we'd have bypassed the mempool already.

}

// TO-DO run DefaultProposalHandler.ProcessProposalHandler first?
// TO-DO Validate()?
Copy link
Contributor

Choose a reason for hiding this comment

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

We definitely need to do some stateless checks here. We are using msg.Pi and msg.Beta below without checking their legitimacies first. If msg.Pi and/or msg.Beta is an empty string, hex.DecodeString will return an empty byte slice and no error. This would bypass the if err != nil {} checks and lead to a panic due to !bytes.Equal(beta, msgBeta).

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.

✨ Full implementation of Randomness module
3 participants