Add Ethereum secp keys to validators #471

sug0 · 2022-09-15T13:23:23Z

Finalizes the implementation of the 8th item of issue no. #242

This PR adds Ethereum secp keys to genesis validators, and also finalizes the validation code for valset upd vote extensions.

We integrate changes from #371

Missing in this PR: Proof-of-stake VP changes, centered around verifying Ethereum secp keys in storage. This can be addressed in a future PR.

sug0 · 2022-09-16T09:46:05Z

still missing some VP stuff, will make this a draft PR again

sug0 · 2022-09-20T10:01:06Z

@james-chf feel free to leave a review, if you have time. what's blocking this PR right now are some changes to VPs, which will unify the native and wasm interfaces #318. after that is more or less done, in main, me/tomas/brent can implement the remaining stuff for the ledger secp keys pr #371, which is being tracked here as well. the remaining changes will be around the PoS VP, to verify eth secp keys.

apps/src/lib/config/genesis.rs

shared/src/types/key/secp256k1.rs

james-chf · 2022-09-26T18:57:05Z

This PR has code from another PR + some new Ethereum-bridge specific logic, this would be easier to review in separate stacked PRs (though not worth splitting out now)

I can't test this locally yet as I need to update my way of generating a devchain to work with the new Tendermint v0.38.x, but I'll do that soon. Then I can test this altogether (maybe after it has merged to eth-bridge-integration), still have a couple more things to look at on this PR

james-chf · 2022-09-26T16:52:16Z

shared/src/ledger/pos/storage.rs

+        &mut self,
+        address: &Self::Address,
+        value: &types::ValidatorEthKey<Self::PublicKey>,
+    ) {


I realize the rest of the API is similar to this currently, but it'd be good to return a Result type rather than .unwrap()ping, since self.write can error

yeah this is more of a general issue outside of this PR, idk if @tzemanovic wants to pitch in on this

In general, yes agree that it'd be better. We didn't worry about this too much for now in here as this is inside PosBase and these methods are only used by the protocol to init chain. If we return an error from, it would still crash further up the stack - the API of PoS crate could do some bigger refactors in general, but for now it's not a high priority

james-chf · 2022-09-26T16:55:19Z

shared/src/ledger/pos/vp.rs

+    fn read_validator_eth_cold_key(
+        &self,
+        key: &Self::Address,
+    ) -> Option<types::ValidatorEthKey<Self::PublicKey>> {
+        let value =
+            self.ctx.read_pre(&validator_eth_cold_key_key(key)).unwrap();
+        value.map(|value| decode(value).unwrap())
+    }
+
+    fn read_validator_eth_hot_key(
+        &self,
+        key: &Self::Address,
+    ) -> Option<types::ValidatorEthKey<Self::PublicKey>> {
+        let value = self.ctx.read_pre(&validator_eth_hot_key_key(key)).unwrap();
+        value.map(|value| decode(value).unwrap())
+    }


When should we expect read_validator_eth_cold_key/read_validator_eth_hot_key to return None instead of Some(...)? It feels like this API could directly return a ValidatorEthKey<Self::PublicKey> (or better, Result<ValidatorEthKey<Self::PublicKey>>), if it's the case that all validators must have Eth keys going forwards

it can happen if we are a fullnode, for instance

Even for a full node, we are reading the validator's key from blockchain state (self.ctx.read_pre) when executing this VP, so these two funcs should work the same for full nodes and validators? I guess returning an Option here follows the pattern of other methods here for the time being

at type level key: &Self::Address can be any address, so this would be None if the given address is not a validator address. If we know we're calling it with validator addresses only, we can expect that those are always Some

james-chf · 2022-09-26T17:33:20Z

shared/src/ledger/pos/mod.rs

+impl
+    namada_proof_of_stake::types::TryRefTo<
+        namada_proof_of_stake::types::EthAddress,
+    > for common::PublicKey
+{


How come we have this TryRefTo trait (impl TryRefTo<T> for U)? It looks like it's trying to achieve the same thing as impl TryInto<U> for &T which would be more standard, couldn't we use that?

me and tomas had a whole coding session where we tried (and successfully!) managed to implement TryFrom, but the code in the proof_of_stake crate looked hideous and unmaintainable with it, because of lifetime issues with references. in short, it's not worth using TryInto.

pretty sure this is the same reason why we have a RefTo trait lying around

Ah interesting, it'd be nice if we could use TryInto but I haven't see what the code looks like with using that, if it means lifetimes everywhere

yeah, we had to specify pretty funky bounds on the PoS traits that use an associated types for the public key to get it to work, so this was actually lot cleaner

still, same as the other thing, the way PoS traits are implemented could use a bigger refactor that would alleviate this

james-chf · 2022-09-26T18:25:08Z

apps/src/lib/wallet/store.rs

@@ -287,16 +289,28 @@ impl Store {
    ///
    /// Note that this removes the validator data.
    pub fn gen_validator_keys(
+        eth_bridge_keypair: Option<common::SecretKey>,


This ultimately needs to be a namada::types::key::secp256k1::SecretKey, we should refactor at some point to accept namada::types::key::secp256k1::SecretKey rather than common::SecretKey. In general, I think we have places where we are using common::{SecretKey, PublicKey} where we should be constraining to the a more specific type (as not all schemes make sense)? We should only use those enum types where the function can or should accept both imo

yeah agreed. it forces you to return Option or Return types, which makes APIs more convoluted to use in the end.

james-chf · 2022-09-26T18:46:12Z

apps/src/lib/node/ledger/shell/queries.rs

+    fn get_ethbridge_from_namada_addr(
+        &self,
+        validator: &Address,
+        epoch: Option<Epoch>,
+    ) -> Option<EthAddress> {


It would be clearer to return a Result here instead of Option - it looks like we return None in the case where read_validator_eth_hot_key returns None, so it sort of goes back to that. Would we ever expect this to return None, or would that indicate an error?

same issue as above, it can return None if we are a fullnode.

like we discussed in our call, the Shell<...> should take an extra parameterNodeTag to distinguish between different node kinds, and only implement methods like prepare_proposal e.g. if we are a NodeTag = Validator. this would also obviate the need to error out from methods like get_ethbridge_from_namada_addr, unless we actually got some storage reading error.

Yeah that sounds like a good approach, have opened #521

james-chf

lgtm to merge!

sug0 · 2022-10-13T08:03:48Z

pls update wasm

tzemanovic and others added 30 commits September 12, 2022 16:12

fixup! pos: add validator eth cold and hot keys

0918529

shared: add eth keys for validators

ebbc8fc

client: add eth keys for validators

250ee2e

TODO: update genesis

6893c20

config: add eth keys to genesis

92a0e22

client: add eth keys to pre_genesis

fd92541

wallet: add eth keys to ValidatorWallet

93179b7

proper hard-coded secp keys, remove outdated comments

87f70e4

shared: add conversion from secp256k1 pk to eth address

be2a295

WIP: add maps for eth address reverse lookup to native address

de84663

TODO: add eth hot key to validator keys in the wallet store

f083429

WIP: Merge eth hot and cold keys into single storage sub-key space

6b9de30

add eth keys to gen_genesis_validator test

e283819

WIP: adding secp key as a ValidatorKeys field

231a664

WIP: add missing ValidatorKeys field

8e27bba

WIP: gen eth keys

ade2415

fixup! WIP: gen eth keys

aa7e997

fixup! WIP: add maps for eth address reverse lookup to native address

5ba4781

write eth keys and addresses for genesis and on-chain created validators

d3143f3

fixup! WIP: gen eth keys

dc05733

fixup! WIP: gen eth keys

dac56f2

fix cli

5cbc47c

Fix wasm tests

175da50

Remove duplicate tiny-keccak dep

3950da6

Fix prepare and process proposal unit tests

7ca2f72

Run make fmt

e22a4e2

Remove duped EthAddress type from namada shared

aaad19f

Return an Ethereum bridge addr from a Namada validator addr

166e483

Add voting powers map to valset upd vexts

8fffeca

Add missing read_eth_key_addresses() to PosReadOnly

48e1fe9

sug0 added 5 commits September 15, 2022 16:38

Add new test utils

66c89db

Fix clippy checks

e0f88e0

Fixed signature of PoS eth key read methods

86a4d5f

Add epoched Ethereum keys

7e08fc0

Fix typo in secp key code

daab76d

sug0 marked this pull request as ready for review September 16, 2022 09:41

sug0 requested review from batconjurer and james-chf September 16, 2022 09:41

sug0 marked this pull request as draft September 16, 2022 09:46

batconjurer reviewed Sep 21, 2022

View reviewed changes

apps/src/lib/config/genesis.rs Show resolved Hide resolved

shared/src/types/key/secp256k1.rs Show resolved Hide resolved

sug0 added 2 commits September 21, 2022 10:18

Merge eth-bridge-integration

540eb4c

Load the right keys from ValidatorConfig

b2fc5c1

sug0 marked this pull request as ready for review September 21, 2022 09:48

sug0 requested review from batconjurer and tzemanovic September 21, 2022 09:52

james-chf mentioned this pull request Sep 27, 2022

CI for the eth-bridge-integration branch #394

Closed

james-chf reviewed Sep 27, 2022

View reviewed changes

Merge eth-bridge-integration

3411176

james-chf approved these changes Sep 27, 2022

View reviewed changes

Fraccaman force-pushed the eth-bridge-integration branch 2 times, most recently from 8d7b66f to c285b74 Compare October 6, 2022 10:49

batconjurer approved these changes Oct 12, 2022

View reviewed changes

Merge eth-bridge-integration

7374acd

[ci] wasm checksums update

b608c78

sug0 merged commit 9abe316 into eth-bridge-integration Oct 13, 2022

sug0 deleted the tiago/ethbridge/ledger-secp-keys branch October 13, 2022 08:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Ethereum secp keys to validators #471

Add Ethereum secp keys to validators #471

sug0 commented Sep 15, 2022 •

edited

Loading

sug0 commented Sep 16, 2022

sug0 commented Sep 20, 2022 •

edited

Loading

james-chf commented Sep 26, 2022

james-chf Sep 26, 2022

sug0 Sep 27, 2022

tzemanovic Sep 29, 2022

james-chf Sep 26, 2022

sug0 Sep 27, 2022

james-chf Sep 27, 2022

tzemanovic Sep 29, 2022

james-chf Sep 26, 2022

sug0 Sep 27, 2022 •

edited

Loading

sug0 Sep 27, 2022

james-chf Sep 27, 2022

tzemanovic Sep 29, 2022

tzemanovic Sep 29, 2022

james-chf Sep 26, 2022

sug0 Sep 27, 2022

james-chf Sep 26, 2022

sug0 Sep 27, 2022

sug0 Sep 27, 2022

james-chf Sep 27, 2022

james-chf left a comment

sug0 commented Oct 13, 2022

Add Ethereum secp keys to validators #471

Add Ethereum secp keys to validators #471

Conversation

sug0 commented Sep 15, 2022 • edited Loading

sug0 commented Sep 16, 2022

sug0 commented Sep 20, 2022 • edited Loading

james-chf commented Sep 26, 2022

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

sug0 Sep 27, 2022 • edited Loading

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

Choose a reason for hiding this comment

james-chf left a comment

Choose a reason for hiding this comment

sug0 commented Oct 13, 2022

sug0 commented Sep 15, 2022 •

edited

Loading

sug0 commented Sep 20, 2022 •

edited

Loading

sug0 Sep 27, 2022 •

edited

Loading