Use validator set precompiles in Attestations #1248

m-chrzan · 2019-10-08T13:47:20Z

Description

We recently added precompiles to access the validator set on-chain, allowing for constant time access to a validator's address at a particular index in the set. We can use this to correctly and efficiently implement attestation requests.

Tested

Unit tests still pass after adding new mock functions to MockValidators.

Related issues

Fixes Attestation Users SBAT use the new precompile #1169

asaj · 2019-10-08T17:49:58Z

packages/protocol/contracts/identity/Attestations.sol

@@ -642,16 +642,21 @@ contract Attestations is IAttestations, Ownable, Initializable, UsingRegistry, R
    internal
  {
    IRandom random = IRandom(registry.getAddressForOrDie(RANDOM_REGISTRY_ID));
+    IValidators validators = IValidators(


Do we need to call into the Validators smart contract for this, or can this contract call the precompile directly?

We could. I was imagining having Validators expose a wrapper around the precompile to minimize complexity (with assembly in other contracts), though this does increase gas costs.

Agree with wanting to cut gas costs, but there is also a small technical problem I just ran into, that the current solution kind of nicely circumvents.

If we use the precompile directly, we can't do ganache tests if we don't modify ethereumjs-vm to handle this precompile (which is not trivial since ganache has no concept of a validator set). By calling into the precompile wrapper in Validators, we can mock Validators and still test this function.

As we discussed offline, I think it's probably best to call the precompile directly in a privaet function, and to have a TestAttestations contract that inherits from Attestations and mocks out the call to the precompile

Consider defining a UsingPrecompiles contract, e.g. https://github.com/celo-org/celo-monorepo/pull/1292/files#diff-0cffec14a25e6876b387a826c4b4946d

It would be nice to add some support for this to ganache. If not, we can't call any part of core contracts that use validators.

Maybe define a list of addresses to serve as validators when launching it

packages/protocol/contracts/identity/Attestations.sol

codecov · 2019-10-14T17:01:48Z

Codecov Report

Merging #1248 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master   #1248   +/-   ##
======================================
  Coverage    67.1%   67.1%           
======================================
  Files         266     266           
  Lines        7776    7776           
  Branches      512     512           
======================================
  Hits         5218    5218           
  Misses       2456    2456           
  Partials      102     102

Flag	Coverage Δ
#mobile	`67.1% <ø> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 21f8cdc...31905d2. Read the comment docs.

mcortesi · 2019-10-16T15:14:07Z

@m-chrzan I think there's a conflict with @asaj PR about PoS. Since the second one, add a UsingPrecompiles contract which seems to achieve the same we want here.

asaj

Just some small nits

asaj · 2019-10-16T17:24:02Z

packages/protocol/contracts/governance/test/MockValidators.sol

@@ -23,6 +23,14 @@ contract MockValidators is IValidators {
    return validators;
  }

+  function numberValidatorsInCurrentSet() external view returns (uint256) {


Why do we need this in MockValidators?

Same as the IValidators issue below.

asaj · 2019-10-16T17:24:23Z

packages/protocol/contracts/governance/interfaces/IValidators.sol

@@ -5,4 +5,6 @@ interface IValidators {
  function isVoting(address) external view returns (bool);
  function isValidating(address) external view returns (bool);
  function getValidators() external view returns (address[] memory);
+  function validatorAddressFromCurrentSet(uint256) external view returns (address);


Do we need this in the IValidators interface?

Leftover from before moving the precompile into Attestations.

So remove it?

asaj · 2019-10-16T17:25:31Z

packages/protocol/contracts/identity/test/TestAttestations.sol

+ * couldn't test `request` with the current ganache local testnet.
+ */
+contract TestAttestations is Attestations {
+  address[] private __testValidators;


Why __ prefix? One underscore would be more consistent with what we do elsewhere

That was precisely my reasoning - since we use _ sometimes, since this is a mock function that should never collide with an actual function, __ would avoid such potential collisions. Of course we don't want to fall down the rabbit hole of ___, ____, ..., in the future, but I can't imagine us needing to go down another level.

Do we expect collisions? I feel like a single _ is fine

* master: (37 commits) [Wallet] Add support for social wallet (Safeguards) import (#1414) [Wallet] Implement new backup flows including social backup (#1399) Use validator set precompiles in Attestations (#1248) E2E Attestations test + various e2e improvements (#1417) Update Footer (#1331) [Wallet] New camera permission flow (#1398) [Wallet] Enable push notifications on iOS (#1389) [Wallet] Add Celo Lite toggle (UI only, zeroSync on/off in other PR) (#1369) Document npm inter-repo dependencies instructions (#1370) [wallet]Add more documentation on ZeroSync mode (#1367) [wallet]Add documentation for jndcrash (#1364) Point end-to-end tests back to master (#1372) Alfajores changes & comment on unlocking accounts (#1297) Reconfigure terraform local configuration during init to allow multiple envs (#773) Implement proof-of-stake changes (#1177) [Celotool] Update blockchain-api deploy script to automatically update faucet address (#1347) Allow most recently reporting oracle to report again (#1288) [wallet]Add documentation for ZeroSync mode (#1361) Fix Metadata registration during contract deploy (#1346) [Wallet] Enable firebase on iOS (#1344) ...

* master: (128 commits) [Wallet] Add support for social wallet (Safeguards) import (#1414) [Wallet] Implement new backup flows including social backup (#1399) Use validator set precompiles in Attestations (#1248) E2E Attestations test + various e2e improvements (#1417) Update Footer (#1331) [Wallet] New camera permission flow (#1398) [Wallet] Enable push notifications on iOS (#1389) [Wallet] Add Celo Lite toggle (UI only, zeroSync on/off in other PR) (#1369) Document npm inter-repo dependencies instructions (#1370) [wallet]Add more documentation on ZeroSync mode (#1367) [wallet]Add documentation for jndcrash (#1364) Point end-to-end tests back to master (#1372) Alfajores changes & comment on unlocking accounts (#1297) Reconfigure terraform local configuration during init to allow multiple envs (#773) Implement proof-of-stake changes (#1177) [Celotool] Update blockchain-api deploy script to automatically update faucet address (#1347) Allow most recently reporting oracle to report again (#1288) [wallet]Add documentation for ZeroSync mode (#1361) Fix Metadata registration during contract deploy (#1346) [Wallet] Enable firebase on iOS (#1344) ...

* master: (37 commits) [Wallet] Add support for social wallet (Safeguards) import (#1414) [Wallet] Implement new backup flows including social backup (#1399) Use validator set precompiles in Attestations (#1248) E2E Attestations test + various e2e improvements (#1417) Update Footer (#1331) [Wallet] New camera permission flow (#1398) [Wallet] Enable push notifications on iOS (#1389) [Wallet] Add Celo Lite toggle (UI only, zeroSync on/off in other PR) (#1369) Document npm inter-repo dependencies instructions (#1370) [wallet]Add more documentation on ZeroSync mode (#1367) [wallet]Add documentation for jndcrash (#1364) Point end-to-end tests back to master (#1372) Alfajores changes & comment on unlocking accounts (#1297) Reconfigure terraform local configuration during init to allow multiple envs (#773) Implement proof-of-stake changes (#1177) [Celotool] Update blockchain-api deploy script to automatically update faucet address (#1347) Allow most recently reporting oracle to report again (#1288) [wallet]Add documentation for ZeroSync mode (#1361) Fix Metadata registration during contract deploy (#1346) [Wallet] Enable firebase on iOS (#1344) ...

Use validator set precompiles in Attestations

5c204df

m-chrzan requested a review from asaj as a code owner October 8, 2019 13:47

m-chrzan assigned nambrot, mcortesi and asaj Oct 8, 2019

asaj reviewed Oct 8, 2019

View reviewed changes

asaj assigned m-chrzan and unassigned asaj Oct 8, 2019

nambrot removed their assignment Oct 10, 2019

Merge branch 'master' into m-chrzan/use-validator-precompile

ee13557

m-chrzan assigned asaj Oct 14, 2019

asaj removed their assignment Oct 14, 2019

mcortesi approved these changes Oct 15, 2019

View reviewed changes

mcortesi self-requested a review October 15, 2019 09:16

mcortesi removed their assignment Oct 15, 2019

m-chrzan added 4 commits October 16, 2019 12:12

Use precompiles in Attestations directly

d3da2f0

Merge branch 'master' into m-chrzan/use-validator-precompile

b8e9211

Document use of test contract

fb9aba1

Merge branch 'master' into m-chrzan/use-validator-precompile

8701010

m-chrzan assigned nambrot, mcortesi and asaj and unassigned m-chrzan Oct 16, 2019

Move precompile usage to separate contract

ec08cdd

asaj reviewed Oct 16, 2019

View reviewed changes

asaj assigned m-chrzan and unassigned asaj Oct 16, 2019

m-chrzan unassigned mcortesi Oct 16, 2019

m-chrzan assigned mcortesi Oct 16, 2019

Remove superflous methods

535b8e1

m-chrzan requested review from cmcewen and jmrossy as code owners October 17, 2019 20:10

Merge branch 'master' into m-chrzan/use-validator-precompile

ce76b44

nambrot force-pushed the m-chrzan/use-validator-precompile branch from c4d6c2c to ce76b44 Compare October 17, 2019 20:15

nambrot assigned asaj and unassigned nambrot Oct 17, 2019

asaj approved these changes Oct 17, 2019

View reviewed changes

asaj assigned nambrot and unassigned asaj Oct 17, 2019

mcortesi approved these changes Oct 21, 2019

View reviewed changes

mcortesi unassigned nambrot Oct 21, 2019

Merge branch 'master' into m-chrzan/use-validator-precompile

31905d2

m-chrzan merged commit 273fc82 into master Oct 22, 2019

tkporter pushed a commit that referenced this pull request Oct 24, 2019

Use validator set precompiles in Attestations (#1248)

132cc83

cmcewen deleted the m-chrzan/use-validator-precompile branch December 2, 2019 18:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use validator set precompiles in Attestations #1248

Use validator set precompiles in Attestations #1248

m-chrzan commented Oct 8, 2019

asaj Oct 8, 2019

m-chrzan Oct 12, 2019

m-chrzan Oct 14, 2019

asaj Oct 14, 2019

asaj Oct 14, 2019

mcortesi Oct 15, 2019

codecov bot commented Oct 14, 2019 •

edited

Loading

mcortesi commented Oct 16, 2019

asaj left a comment

asaj Oct 16, 2019

m-chrzan Oct 16, 2019

asaj Oct 17, 2019

asaj Oct 16, 2019

m-chrzan Oct 16, 2019

asaj Oct 17, 2019

asaj Oct 16, 2019

m-chrzan Oct 16, 2019

asaj Oct 17, 2019

Use validator set precompiles in Attestations #1248

Use validator set precompiles in Attestations #1248

Conversation

m-chrzan commented Oct 8, 2019

Description

Tested

Related issues

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 14, 2019 • edited Loading

Codecov Report

mcortesi commented Oct 16, 2019

asaj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 14, 2019 •

edited

Loading