feat: Adds validator qgb checks #137

rach-id · 2022-05-05T22:29:24Z

Adds to #114

…g and editing

…ecks

…ecks # Conflicts: # client/flags/flags.go # simapp/simd/cmd/testnet.go # x/staking/simulation/operations.go

rach-id

🤔

rach-id · 2022-05-07T08:12:51Z

simapp/helpers/test_helpers.go

@@ -14,7 +14,7 @@ import (

 // SimAppChainID hardcoded chainID for simulation
 const (
-	DefaultGenTxGas = 1000000
+	DefaultGenTxGas = 4000000 // An increase is needed when adding checks to staking/msg_server.go


a 2.6x increase in gas for couple fields and few checks doesn't look so right to me. What do you think?

rach-id · 2022-05-07T08:17:45Z

@evan-forbes When you have time, please take a look on: https://github.com/celestiaorg/cosmos-sdk/runs/6333164562?check_suite_focus=true why it is failing. I can't understand why the app hash changes while running the same app.

adlerjohn · 2022-05-08T16:56:50Z

The app hash isn't just different, it's not even valid hex!

"\xceX\xfb|\\|\x97F\x10\xc6\xf5\x12i7\xd6\x17S\x98\r\xc9\x130\x03R\xc1\x97\xad\xb7R\x01\xb8\xfa"

adlerjohn · 2022-05-08T17:03:36Z

x/staking/keeper/msg_server.go

 	if msg.Orchestrator != "" {
+		_, err := k.validateOrchestratorAddress(ctx, msg.Orchestrator)


Are you sure you want to error when a validator exists if you're trying to edit it?

We don't want to have duplicate orchestrator or Ethereum address. If some other validatore has that address, we shouldnt continue. No?

For inserting a new validator, yes. But editing a validator (not sure what that even means?) requires the validator to exist prior to being edited, no?

We look first if the validator exists by its address. Then, we start checking if the new orchestrator address has been given to some other validator.
So, that one is looking for a validator by orchestrator address. If any other validator has that orchestrator address, we stop.
Or I am missing something

I think this makes sense, we want to throw an error if someone is trying to update their vaildator with an invalid orchestrator address. If we don't, then we won't be able to verify their attestations, and while it wouldn't be protocol breaking, I think it would be a terrible UX, as they'd have to debug why they can't submit signatures.

we're also checking that the validator exists on line 143

rach-id

The latest commits are for debugging purposes (mainly commenting code to find the problematic change). The history will be cleaned with a force push.

rach-id · 2022-05-09T10:21:12Z

x/staking/simulation/operations.go

@@ -146,7 +148,9 @@ func SimulateMsgCreateValidator(ak types.AccountKeeper, bk types.BankKeeper, k k
 			simtypes.RandomDecAmount(r, maxCommission),
 		)

-		ethAddr, _ := types.NewEthAddress("0x91DEd26b5f38B065FC0204c7929Da6b2A21277Cd")
+		ethPrivateKey, _ := crypto.GenerateKey()


This is probably what's making the app hash fail. Investigating more.

…mulation to have a deterministic way of creating it

rach-id · 2022-05-09T14:04:56Z

@evan-forbes If you agree on the eth address fix. we can merge.

evan-forbes · 2022-05-09T13:58:00Z

x/staking/simulation/operations.go

-		ethAddr, _ := types.NewEthAddress("0x" + fmt.Sprintf("%X", doubleOrchAddressBytes[:(types.ETHContractAddressLen/2)-2]))
+		ethAddr, _ := types.NewEthAddress("0x" + fmt.Sprintf("%X", doubleOrchAddressBytes[:(types.ETHContractAddressLen/2)-1]))


evan is confused why we are making doubleOrchAddress in the first place?

and perhaps we should not be using the golang's hex formatting tooling, and instead be using geth's

Great catch. fix coming.

and perhaps we should not be using the golang's hex formatting tooling, and instead be using geth's

It is the same. Just creating a hex string from a bytes array

check now again please.

evan-forbes

After a quick sync with @sweexordious, the main issue is simply that we are randomly generating the address each time, which cause the non-determinism issue. It also doesn't help that the gravity bridge code uses their own custom type for an eth address instead of using a wrapper around geth's Address type.

@sweexordious is currently writing an issue or two with a bit more context. With those reminders in place, I'm comfortable with merging, as we might not even use this method to keep track of orchestrator/eth addresses, so making another huge PR(s) just to properly fix the tests seems wasteful atm.

great work debugging @sweexordious !!

rach-id · 2022-05-09T14:47:12Z

Issues created: #143 and #144

* first pass on tests fixes * fixes the rest of unit tests * remove unnecessary comments * uses default eth address when starting sim network * cosmetics * comments failing test * comments failing test * adds orchestrator/ethereum address checks for validators when creating and editing * uses correct error codes for eth/orch address errors * uncomments test and fixes it from commit 434b308 * adds zero eth address check when creating/updating validator * attempts to fix duplicate eth address in sim network * revert squashed changes * uses unwrapped context for msg_server orch/eth validation * increase DefaultGenTxGas to accomodate new qgb validator changes * scaffolds an ethereum address from orchestrator address in staking simulation to have a deterministic way of creating it * fix eth address creation from orch address in operations * increase DefaultGenTxGas * increase DefaultGenTxGas * simpler way of creating an eth address from orch address in operations

rach-id added 11 commits May 3, 2022 18:40

first pass on tests fixes

0e9a2db

fixes the rest of unit tests

49fadc4

remove unnecessary comments

c3fe6aa

uses default eth address when starting sim network

168a6fe

cosmetics

af04769

comments failing test

bbf69d4

comments failing test

a910030

adds orchestrator/ethereum address checks for validators when creatin…

756f041

…g and editing

uses correct error codes for eth/orch address errors

e70bbd8

uncomments test and fixes it from commit 434b308

f53f9d6

Merge branch 'fix_tests_after_qgb_changes' into adds_validator_qgb_ch…

edf6b87

…ecks

rach-id added enhancement New feature or request C: QGB labels May 5, 2022

rach-id self-assigned this May 5, 2022

rach-id marked this pull request as draft May 5, 2022 22:29

rach-id changed the title ~~Adds validator qgb checks~~ feat: WIP Adds validator qgb checks May 5, 2022

rach-id added 2 commits May 6, 2022 11:29

adds zero eth address check when creating/updating validator

8568eac

attempts to fix duplicate eth address in sim network

b6fbf3a

rach-id force-pushed the adds_validator_qgb_checks branch from 44be9c7 to b6fbf3a Compare May 6, 2022 12:46

rach-id added 2 commits May 6, 2022 17:44

Merge branch 'rachid/orchestrator-address' into adds_validator_qgb_ch…

a10f734

…ecks # Conflicts: # client/flags/flags.go # simapp/simd/cmd/testnet.go # x/staking/simulation/operations.go

revert squashed changes

3dbbe6c

rach-id requested a review from evan-forbes May 6, 2022 15:49

rach-id marked this pull request as ready for review May 6, 2022 15:49

rach-id changed the title ~~feat: WIP Adds validator qgb checks~~ feat: Adds validator qgb checks May 6, 2022

uses unwrapped context for msg_server orch/eth validation

a547c1b

rach-id force-pushed the adds_validator_qgb_checks branch 3 times, most recently from 580e41c to 0ff3aed Compare May 7, 2022 07:50

rach-id commented May 7, 2022

View reviewed changes

adlerjohn reviewed May 8, 2022

View reviewed changes

rach-id commented May 9, 2022

View reviewed changes

rach-id added 2 commits May 9, 2022 13:39

increase DefaultGenTxGas to accomodate new qgb validator changes

03e8b0c

scaffolds an ethereum address from orchestrator address in staking si…

fd79550

…mulation to have a deterministic way of creating it

rach-id force-pushed the adds_validator_qgb_checks branch from a40e31b to fd79550 Compare May 9, 2022 11:40

rach-id added 3 commits May 9, 2022 13:47

fix eth address creation from orch address in operations

a799b09

increase DefaultGenTxGas

fa29549

increase DefaultGenTxGas

308853d

evan-forbes reviewed May 9, 2022

View reviewed changes

simpler way of creating an eth address from orch address in operations

176d296

evan-forbes approved these changes May 9, 2022

View reviewed changes

rach-id mentioned this pull request May 9, 2022

Create validator Ethereum address during simulation tests in a correct way #144

Open

rach-id merged commit d9dc2cd into celestiaorg:rachid/orchestrator-address May 9, 2022

rach-id deleted the adds_validator_qgb_checks branch May 9, 2022 14:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Adds validator qgb checks #137

feat: Adds validator qgb checks #137

rach-id commented May 5, 2022

rach-id left a comment

rach-id May 7, 2022 •

edited

Loading

rach-id commented May 7, 2022

adlerjohn commented May 8, 2022

adlerjohn May 8, 2022

rach-id May 8, 2022

adlerjohn May 8, 2022

rach-id May 9, 2022 •

edited

Loading

evan-forbes May 9, 2022 •

edited

Loading

rach-id left a comment

rach-id May 9, 2022

rach-id commented May 9, 2022

evan-forbes May 9, 2022

evan-forbes May 9, 2022

rach-id May 9, 2022

rach-id May 9, 2022

rach-id May 9, 2022

evan-forbes left a comment •

edited

Loading

rach-id commented May 9, 2022

		if msg.Orchestrator != "" {
		_, err := k.validateOrchestratorAddress(ctx, msg.Orchestrator)

		ethAddr, _ := types.NewEthAddress("0x" + fmt.Sprintf("%X", doubleOrchAddressBytes[:(types.ETHContractAddressLen/2)-2]))
		ethAddr, _ := types.NewEthAddress("0x" + fmt.Sprintf("%X", doubleOrchAddressBytes[:(types.ETHContractAddressLen/2)-1]))

feat: Adds validator qgb checks #137

feat: Adds validator qgb checks #137

Conversation

rach-id commented May 5, 2022

rach-id left a comment

Choose a reason for hiding this comment

rach-id May 7, 2022 • edited Loading

Choose a reason for hiding this comment

rach-id commented May 7, 2022

adlerjohn commented May 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rach-id May 9, 2022 • edited Loading

Choose a reason for hiding this comment

evan-forbes May 9, 2022 • edited Loading

Choose a reason for hiding this comment

rach-id left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rach-id commented May 9, 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

evan-forbes left a comment • edited Loading

Choose a reason for hiding this comment

rach-id commented May 9, 2022

rach-id May 7, 2022 •

edited

Loading

rach-id May 9, 2022 •

edited

Loading

evan-forbes May 9, 2022 •

edited

Loading

evan-forbes left a comment •

edited

Loading