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: adds orchestrator address to validator in staking module #112

Conversation

rach-id
Copy link
Member

@rach-id rach-id commented Apr 13, 2022

adds orchestrator address to validator in staking module

@rach-id rach-id changed the title adds orchestrator address to validator in staking module feat: adds orchestrator address to validator in staking module Apr 13, 2022
@rach-id rach-id marked this pull request as draft April 13, 2022 15:28
@rach-id
Copy link
Member Author

rach-id commented Apr 14, 2022

Adds to: celestiaorg/celestia-app#283

Copy link
Member Author

@rach-id rach-id left a comment

Choose a reason for hiding this comment

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

Can we merge this PR to that branch ?
Fixing the rest of the tests + adding new ones + adding checks would result in a really huge PR.
I would prefer to keep it incremental.
Let's merge this even if the CI is not passing to have easier reviews afterwards.

@rach-id
Copy link
Member Author

rach-id commented May 3, 2022

@evan-forbes Can you take a look please ?

@rach-id rach-id marked this pull request as ready for review May 3, 2022 10:53
Copy link
Member

@evan-forbes evan-forbes left a comment

Choose a reason for hiding this comment

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

this makes sense I think, and I'm find with approving even with failed CI as long as its not the default branch.

We also need to start thinking about the scenario where we don't start from genesis, and instead we hardfork/upgrade to include the QGB, and what state migrations would be required.

@rach-id
Copy link
Member Author

rach-id commented May 3, 2022

Some thoughts around starting the QGB.

Messaging compatibility

In the scenario where we don't start from genesis, I guess it is still alright.
Because, we're only adding new fields to the struct, not editing/removing any. Thus, on the messaging side, it should be alright.

Around starting the QGB on an existing chain

For an already running chain, to start the QGB, we only need the current valset to have orchestrator/Ethereum addresses. There can be two ways to do it.

Hard fork the chain

Add a new parameter specifying the height we want to start the QGB on. When we start the QGB, any validator who doesn't have an Ethereum/orchestrator address setup, they get kicked from the valset (we could add some validator set sanitization mechanism).
Then, that height parameter can be setup using governance.

Social coordination

Ask the validators to update their software and set up the Orchestrator/Ethereum address gently. Then, we start the QGB once all the current valset validators are ready (we give them a week or so, to be ready).

After starting the QGB

Post the starting ceremony, we could have a new check on the staking module, verifying that no validator can move to the current validator set if it doesn't have an orchestrator/Ethereum address set.
Also, maybe other checks when we add QGB slashing, like checking if they have some stake etc.

Questions

  1. When slashing. Would we target the validator staked amount ? Or something else ?
  2. This probably would only be done on testnet. Because, starting incentivized testnet/mainnet, I assume the QGB would be ready to be started on genesis. The way we start the QGB in the upcoming testnet wouldn't make much difference, right ?

@rach-id
Copy link
Member Author

rach-id commented May 3, 2022

@evan-forbes Does it make sense to create some issue where we discuss this ?

@evan-forbes
Copy link
Member

evan-forbes commented May 3, 2022

Does it make sense to create some issue where we discuss this?

let's discuss synchronously, and then create issues

there's a specific way to migrate state during a hardfork/upgrade using the sdk, and we will want to use that

@rach-id
Copy link
Member Author

rach-id commented May 3, 2022

Sounds good 👍 I will merge this PR now.

@rach-id
Copy link
Member Author

rach-id commented May 3, 2022

@evan-forbes I don't have permissions to merge :( Can you please?

@rach-id rach-id changed the title feat: adds orchestrator address to validator in staking module feat: adds orchestrator address to validator in staking struct May 3, 2022
@rach-id rach-id changed the title feat: adds orchestrator address to validator in staking struct feat: adds orchestrator address to validator in staking module May 3, 2022
@evan-forbes evan-forbes merged commit 545ccd2 into celestiaorg:rachid/orchestrator-address May 3, 2022
@rach-id rach-id deleted the adds_orchestrator_address_to_validator_init branch May 3, 2022 13:56
)

// EthAddress Regular EthAddress
type EthAddress struct {
Copy link
Member Author

Choose a reason for hiding this comment

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

To be changed with Geth implementation

rach-id added a commit to rach-id/cosmos-sdk that referenced this pull request May 9, 2022
…tiaorg#112)

* adds orchestrator address to validator initialization in staking module

* fix msg edit  validator

* todo

* todo
rach-id added a commit to rach-id/cosmos-sdk that referenced this pull request Jul 27, 2022
…tiaorg#112)

* adds orchestrator address to validator initialization in staking module

* fix msg edit  validator

* todo

* todo
rach-id added a commit to rach-id/cosmos-sdk that referenced this pull request Aug 4, 2022
…tiaorg#112)

* adds orchestrator address to validator initialization in staking module

* fix msg edit  validator

* todo

* todo
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants