Add precompiles to access validator set #441

m-chrzan · 2019-09-06T20:51:22Z

Description

Sister monorepo PR: celo-org/celo-monorepo#874

This PR adds two precompiled contracts that give the EVM access to the current validator set's information.

getValidator at 0xfa - takes as input an index, returns the address of the validator at that index. Reverts if out of bounds.
numberValidators at 0xf9 - returns the size of the current validator set.

Tested

Geth e2e tests in monorepo.

Related issues

Fixes Attestations SBAT access current validator set #360

Backwards compatibility

No breaking changes, only adds new precompiles.

kevjue · 2019-09-09T18:18:26Z

The code LGTM. However, seems like there are some unit tests that are failing. I would of expected that, since the evmcontext object (which now has a new field) is well used within the unit tests.

I'll approve once the unit tests pass.

asaj · 2019-09-09T19:48:16Z

Why not have a single precompile that returns the entire set? Seems like that would be more flexible going forward

m-chrzan · 2019-09-09T20:27:45Z

However, seems like there are some unit tests that are failing. I would of expected that, since the evmcontext object (which now has a new field) is well used within the unit tests.

Working through those right now.

Why not have a single precompile that returns the entire set? Seems like that would be more flexible going forward

I'd imagine we would set a higher gas cost (proportional to size of validator set) if we went with that approach. I wanted to allow contract authors to be able to optimize gas costs if they only need to read a few validator addresses (like our use case with requesting attestations.

kevjue

LGTM!

asaj · 2019-09-10T23:29:53Z

I'd imagine we would set a higher gas cost (proportional to size of validator set) if we went with that approach. I wanted to allow contract authors to be able to optimize gas costs if they only need to read a few validator addresses (like our use case with requesting attestations.

Should we include an isValidator precompile? I'm worried these precompiles are a bit too specific to our attestations use case, and won't generalize well (though we can always add more later on)

m-chrzan · 2019-09-11T20:43:49Z

Should we include an isValidator precompile?

I've actually been considering adding this as well recently. Feels like it could easily be a follow up PR though.

* Add precompiles to access validator set (#441) * set max gas to double of the charged gas for the 'intrinsic' smart contract calls (#472) * set max gas to double of the charged gas for the 'intrinsic' evm operations * addressed PR comments * addressed pr comment * Adds Prepared Certificates to ensure Istanbul liveness (#366) * Check message address against signature (#477) * Check signing validator's address matches msg address * Add comments about use of sig data in tests * Try fix Circle build test failures * Try fix Circle build test failures, take 2

* contract_comm/currency/currency.go * fixed the txn price-sorted min-heap * merge master (#490) * Add precompiles to access validator set (#441) * set max gas to double of the charged gas for the 'intrinsic' smart contract calls (#472) * set max gas to double of the charged gas for the 'intrinsic' evm operations * addressed PR comments * addressed pr comment * Adds Prepared Certificates to ensure Istanbul liveness (#366) * Check message address against signature (#477) * Check signing validator's address matches msg address * Add comments about use of sig data in tests * Try fix Circle build test failures * Try fix Circle build test failures, take 2

* Check message address against signature (#477) * Check signing validator's address matches msg address * Add comments about use of sig data in tests * Try fix Circle build test failures * Try fix Circle build test failures, take 2 * tx price heap fix (#471) * contract_comm/currency/currency.go * fixed the txn price-sorted min-heap * merge master (#490) * Add precompiles to access validator set (#441) * set max gas to double of the charged gas for the 'intrinsic' smart contract calls (#472) * set max gas to double of the charged gas for the 'intrinsic' evm operations * addressed PR comments * addressed pr comment * Adds Prepared Certificates to ensure Istanbul liveness (#366) * Check message address against signature (#477) * Check signing validator's address matches msg address * Add comments about use of sig data in tests * Try fix Circle build test failures * Try fix Circle build test failures, take 2 * added new option --use-in-memory-discovery-table (#479) * added new option --use-in-memory-discovery-table * merge master (#489) * Adds Prepared Certificates to ensure Istanbul liveness (#366) * Check message address against signature (#477) * Check signing validator's address matches msg address * Add comments about use of sig data in tests * Try fix Circle build test failures * Try fix Circle build test failures, take 2 * Allow v4/v5 on a bootnode simultaneously, allow mobile to use discv5 (#454) * changes for isolating celo networks * changes to get unit tests working * changes to add salt in the discovery packets * removed checking for the ip address when handling a reply * added ping-ip-from-packet option to bootnode * for handling expected replies, don't filter on expected sender ip address if --pingIPFromPacket is used * Add v4 flag * Add unhandled and quicken docker builds * Add salt & logs * Add v4 flag * Add PeerDiscovery to mobile node config * Remove logs * Remove hardcoded bootnodes * Add salt & turn on discv5 * Delete hardcoded eth bootnodes * Make Discoveryv5 configurable * Lint * Add comment to bootnode v4/v5 handling * Change PeerDiscovery -> NoDiscovery * Remove mobile geth no discovery * Reduce istanbul default timeout, cap exp backoff (#475) * Reduce istanbul default timeout, cap exp backoff * Ensure round 0 timeout factors in block period * Sanitize logs (#495) * Change registry lookup and infrastructure lookup error logs to debug level * Sanitize logs regarding registry deployment * Change empty abi logging and comment * Lower log level from error to warning for potentially outdated istanbul messages * Change back to an error message * Add input length checks for precompiled contracts (#476) * add input length checks * check exact input length. add a new error for input length. check input in a few more places * add tests that verify the input-length checks for contracts that don't require an evm instance * fix formatting * add comments to explain input length checks * run the formatter * e2e transfer test was failing because it passes in a transaction options object, making the input larger than 96 bytes * e2e tests have revealed that our precompiled contracts need to be tolerant of inputs that are longer than the bytes that are actually read

* Log on ValidatorElections * merge master (#496) * Check message address against signature (#477) * Check signing validator's address matches msg address * Add comments about use of sig data in tests * Try fix Circle build test failures * Try fix Circle build test failures, take 2 * tx price heap fix (#471) * contract_comm/currency/currency.go * fixed the txn price-sorted min-heap * merge master (#490) * Add precompiles to access validator set (#441) * set max gas to double of the charged gas for the 'intrinsic' smart contract calls (#472) * set max gas to double of the charged gas for the 'intrinsic' evm operations * addressed PR comments * addressed pr comment * Adds Prepared Certificates to ensure Istanbul liveness (#366) * Check message address against signature (#477) * Check signing validator's address matches msg address * Add comments about use of sig data in tests * Try fix Circle build test failures * Try fix Circle build test failures, take 2 * added new option --use-in-memory-discovery-table (#479) * added new option --use-in-memory-discovery-table * merge master (#489) * Adds Prepared Certificates to ensure Istanbul liveness (#366) * Check message address against signature (#477) * Check signing validator's address matches msg address * Add comments about use of sig data in tests * Try fix Circle build test failures * Try fix Circle build test failures, take 2 * Allow v4/v5 on a bootnode simultaneously, allow mobile to use discv5 (#454) * changes for isolating celo networks * changes to get unit tests working * changes to add salt in the discovery packets * removed checking for the ip address when handling a reply * added ping-ip-from-packet option to bootnode * for handling expected replies, don't filter on expected sender ip address if --pingIPFromPacket is used * Add v4 flag * Add unhandled and quicken docker builds * Add salt & logs * Add v4 flag * Add PeerDiscovery to mobile node config * Remove logs * Remove hardcoded bootnodes * Add salt & turn on discv5 * Delete hardcoded eth bootnodes * Make Discoveryv5 configurable * Lint * Add comment to bootnode v4/v5 handling * Change PeerDiscovery -> NoDiscovery * Remove mobile geth no discovery * Reduce istanbul default timeout, cap exp backoff (#475) * Reduce istanbul default timeout, cap exp backoff * Ensure round 0 timeout factors in block period * Sanitize logs (#495) * Change registry lookup and infrastructure lookup error logs to debug level * Sanitize logs regarding registry deployment * Change empty abi logging and comment * Lower log level from error to warning for potentially outdated istanbul messages * Change back to an error message * Add input length checks for precompiled contracts (#476) * add input length checks * check exact input length. add a new error for input length. check input in a few more places * add tests that verify the input-length checks for contracts that don't require an evm instance * fix formatting * add comments to explain input length checks * run the formatter * e2e transfer test was failing because it passes in a transaction options object, making the input larger than 96 bytes * e2e tests have revealed that our precompiled contracts need to be tolerant of inputs that are longer than the bytes that are actually read

m-chrzan added 2 commits August 16, 2019 12:06

Expose validator list from istanbul engine

3ede809

Return validator address from precompile

70ddd64

m-chrzan requested a review from asaj as a code owner September 6, 2019 20:51

Handle OOB, expose set size precompile

e5efa74

m-chrzan force-pushed the m-chrzan/validators-list branch from ab21a41 to e5efa74 Compare September 6, 2019 20:54

m-chrzan assigned asaj and kevjue Sep 6, 2019

Merge branch 'master' into m-chrzan/validators-list

37515a1

kevjue assigned m-chrzan and unassigned kevjue Sep 9, 2019

asaj removed their assignment Sep 9, 2019

Avoid nil pointer dereference in unit test

567dcc2

m-chrzan mentioned this pull request Sep 9, 2019

Expose and test validator set precompiles celo-org/celo-monorepo#874

Merged

m-chrzan assigned asaj and kevjue and unassigned m-chrzan Sep 9, 2019

kevjue assigned m-chrzan and unassigned kevjue Sep 9, 2019

kevjue approved these changes Sep 9, 2019

View reviewed changes

Merge branch 'master' into m-chrzan/validators-list

b7f56a6

asaj removed their assignment Sep 10, 2019

m-chrzan merged commit d4b48f3 into master Sep 13, 2019

cmcewen mentioned this pull request Oct 29, 2019

[Android] Onboarding Verification fails celo-org/celo-monorepo#1330

Closed

mcortesi deleted the m-chrzan/validators-list branch December 4, 2019 23:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add precompiles to access validator set #441

Add precompiles to access validator set #441

m-chrzan commented Sep 6, 2019

kevjue commented Sep 9, 2019

asaj commented Sep 9, 2019

m-chrzan commented Sep 9, 2019 •

edited

Loading

kevjue left a comment

asaj commented Sep 10, 2019

m-chrzan commented Sep 11, 2019

Add precompiles to access validator set #441

Add precompiles to access validator set #441

Conversation

m-chrzan commented Sep 6, 2019

Description

Tested

Related issues

Backwards compatibility

kevjue commented Sep 9, 2019

asaj commented Sep 9, 2019

m-chrzan commented Sep 9, 2019 • edited Loading

kevjue left a comment

Choose a reason for hiding this comment

asaj commented Sep 10, 2019

m-chrzan commented Sep 11, 2019

m-chrzan commented Sep 9, 2019 •

edited

Loading