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

Merge QGB feature branch into main #742

Merged
merged 96 commits into from
Sep 23, 2022
Merged

Merge QGB feature branch into main #742

merged 96 commits into from
Sep 23, 2022

Conversation

rach-id
Copy link
Member

@rach-id rach-id commented Sep 20, 2022

This merge will add the QGB state machine to Celestia-app.

rach-id and others added 30 commits February 16, 2022 11:03
* removes dummy field from data commitment

* adds TODOs

* adds comments

* linter fixes

* linter fixes

* linter fixes

* linter fixes

* linter fixes

* CI fixes

* query rename
* adds valset confirm logic and queries

* adds valset confirm logic

* removes unnecessary parameter from qgb keeper

* removes unnecessary cli for valset confirm

* updates comments and removes unnecessary ones

* remove query command from module
* add_qgb_orchestrator_address_logic

* updates comment

* don't return giant struct for linting reason

Co-authored-by: CHAMI Rachid <[email protected]>

* update imports

Co-authored-by: Evan Forbes <[email protected]>
* adds msgs + query proto

* adds data commitment basic functions

* adds data commitment query skeleton + server emitting events

* linter changes

* adds data commitment keeper + basic queries

* persisting data commitment confirm on submission

* adds data commitment confirms by validator query

* adds verification for validator and ethereum address when handling data commitments confirm messages

* adds data commitment by range keeper + query

* adds comments

* adds staking keeper to qgb and GetOrchestratorValidator implementation

* adds delegate key query

* adds qgb module address config

* cosmetics

* removes unnecessary comment

* uses restricted version of staking keeper for QGB

* cosmetics

* removes config file
* add valset and params

* create new val set request if the voting power has changed enough at the end of the block

* comment out broken query method we don't strictly need yet

* delete some comments
* fixes test common foundation

* fixes valset confirm query + query by nonce test

* go mod

* remove unnecessary comment

* cosmetics

* adds query current valset test

* adds prefix range test

* adds current valset normalized power test

* adds GetDelegateKeys test

* adds NewMsgSetOrchestratorAddress test

* adds ethereum signer test

* adds genesis test

* adds valset power diff test

* adds valset sort test

* adds Valset Creation Upon Unbonding test

* adds abci tests

* adds valset handler tests

* adds no lint

* adds no lint
* fixes data commitment keeper iterator

* adds data commitment by commitment query

* cosmetics

* adds data commitment queries tests

* cosmetics

* adds data commitment message validation tests

* adds commented msg data commitment confirm test
* adds handler test

* adds events check for handler test
* use index prefix for commitments query

* adds test for indexed query

* linter
* adds overview and valset init ADRs

* cleanup

* adds Data commitment ADR init

* improves the valset adr

* cosmetics

* adds more details and removes unnecessary stuff

* cosmetics

* proto format + ADR updates

* updates proto files + ADR

* adds endblocker doc to adr

* typo
* initial orchestrator client

* update go mod and change the makefile so it stops breaking the ci

* copy and paste the needed code from the keystore lib
…reum/Orchestrator addresses to cosmos-sdk.staking.Validator (#276)

* initial orchestrator client

* format proto and add necessary queries

* add orchestrator logic

* and encoding of data commitments and validator set changes

* add orchestrator command

* add relayer

* add the orchestrator command

* proto formatting

* use correct name

Co-authored-by: CHAMI Rachid <[email protected]>

* use correct path to endpoint

Co-authored-by: CHAMI Rachid <[email protected]>

* get rid of GravityNonces struct

* use correct name

Co-authored-by: CHAMI Rachid <[email protected]>

* Adds QGB related ADRs (#250)

* adds overview and valset init ADRs

* cleanup

* adds Data commitment ADR init

* improves the valset adr

* cosmetics

* adds more details and removes unnecessary stuff

* cosmetics

* proto format + ADR updates

* updates proto files + ADR

* adds endblocker doc to adr

* typo

* regenerate proto

* go mod tidy

* use correct name

Co-authored-by: CHAMI Rachid <[email protected]>

* handle error

* begin refactor

* adds data commitments and valset orchestrator processors

* add the app client that queries for validator sets and data commitments

* add two new methods

* add query methods to appClient

* WIP: update orchestrator and relayer to use the appClient

* WIP: update orchestrator and relayer to use the appClient

* WIP: update orchestrator and relayer to use the appClient

* adds evm_client implementation

* add the nil clients for the evm and mockappclient

* add the new methods to the mock app client

* flesh out the mock app client

* flesh out app orchestrator test

* adds QueryLatestValset to appClient

* wip: adds orchestrator test

* adds orchestrator valset test

* moves orchestrator test utils to a separate file

* adds orchestrator valset processEvents error check in test

* rename ValidatorSignature field in DataCommitmentConfirm to ValidatorAddress

* fix valset orchestrator signature test

* fix last valset request in orchestrator

* fix last valset request in orchestrator common test

* remove unnecessary nonce increment for data commitment signing in orchestrator

* fix orchestrator data commitment test

* closes the mocked app client in orchestrator test

* add the mock evm client

* fix the command to orchestrate valsets properly

* get rid of zerologger

* use tmlog instead of zerolog

* remove extra err handling

* add comment to question nonce usage in evm client submit dataroot

* adds data commitment orchestrator test

* adds deploy command

* adds evmRpc flag to qgb config

* deubgging the qgb install

* add set orchestrator command

* removes set Ethereum/Orch address + fixes tests + general cleanup (#296)

* removing setting the address separatly and relying on the eth/orch addresses to be defined in sdk.staking.Validator

* add tx command for qgb module

* added proto generated files

* fixes tests

* adds todo

* todo

* pass the keyring backend and path to app client

* adds error check the newKeyedTransactorWithChainId

* rename QueryLastValset1 to QueryLastValsetRequests

* rename evmChainIDFlag typo

* rename QueryLastValsets

* adds todo

* update deploy command to use the right latest valset

* adds todo

* updated go.mod and go.sum

* update the single-node.sh script to initialize three accounts and create one validator with an orchestrator and ethereum address

* imports

* adds events testing main.go

* remove unused variable

* use sweexordious fork for cosmos sdk in the app

* update go.sum

* update .gitignore

* update lint.yml

* linter fixes

* new line

* linter

* todo

* linter checks

* linter checks

* linter checks

* adds mutexes for race conditions

Co-authored-by: CHAMI Rachid <[email protected]>
…ayer for testing (#314)

* temporary disables listening for valsets and query for latest valset instead

* app client listening for arbitrary events for valsets

* temporary disable signatures for testing

* temporarily disable threshold checks for relayer

* update count in end blocker

* fixes orchestrator listening to valset events

* fix orchestrator signatures + enables checks on msg_server + makes bridgeId a const

* fixes signature tests + adds new checks

* adds todo

* remove unnecessary comment

* adds todo

* adds todo

* moves the signer outside of the NewAppClient

* cosmetics

* updates singe node script to only create one validator

* uncomments the threshold for querying two third data commitment confirms

* linter checks + cosmetics fixes

* cosmetics

* removes todo
* adds valset request by nonce

* trigger CI

* revert: trigger CI

* update go.mod

* Revert "update go.mod"

This reverts commit 0007050.
* adds valset request by nonce

* add querier, broadcaster and updates orchestrator to use them

* partially fix the tests for the new design

* update the deployer for the new design

* update orchestrator to new design

* Adds querier and evm client update

* update relayer to new design

* fix query.proto

* update deploy_command to use new Querier

* formatting

* formatting

* go.sum

* revert go.mod change
* go mod tidy

* go mod tidy
* adds valset request by nonce

* add querier, broadcaster and updates orchestrator to use them

* partially fix the tests for the new design

* update the deployer for the new design

* update orchestrator to new design

* Adds querier and evm client update

* update relayer to new design

* fix query.proto

* update deploy_command to use new Querier

* formatting

* formatting

* go.sum

* adds query last unbonding height
* uses the uint64 instead of int64 in right places

* typo

* cosmetics
* adds last valset request before  height query

* regenerating correct query.pb.go

* adds check before querying next valset

* Update x/qgb/keeper/query_valset.go

Co-authored-by: John Adler <[email protected]>

* adds comment

Co-authored-by: John Adler <[email protected]>
* adds valset request by nonce

* add querier, broadcaster and updates orchestrator to use them

* partially fix the tests for the new design

* update the deployer for the new design

* update orchestrator to new design

* Adds querier and evm client update

* update relayer to new design

* fix query.proto

* update deploy_command to use new Querier

* formatting

* formatting

* go.sum

* adds query last unbonding height

* adds orchestrator valset replay

* adds orchestrator data commitment replay

* cosmetics

* puts valset signature catchup in a separate function

* puts data commitment signature catchup in a separate function

* Querier cosmetics

* go.sum

* rename querier and catchup functions

* adds genesis case to querylastvalset

* format + todo

* initialized tm logger in test

* update orchestrator code to use uint64 instead of int64

* better logging

* Update x/qgb/orchestrator/orchestrator_client.go

Co-authored-by: Evan Forbes <[email protected]>

* formats import

* rename addOldValsetAttestations and addOldDataCommitmentAttestations

* defering logging instead of repeating it on every return

* adds comment

* Update x/qgb/orchestrator/relayer.go

Co-authored-by: John Adler <[email protected]>

* Update x/qgb/orchestrator/querier.go

Co-authored-by: John Adler <[email protected]>

* remove unnecessary error

Co-authored-by: Evan Forbes <[email protected]>
Co-authored-by: John Adler <[email protected]>
* adds valset request by nonce

* add querier, broadcaster and updates orchestrator to use them

* partially fix the tests for the new design

* update the deployer for the new design

* update orchestrator to new design

* Adds querier and evm client update

* update relayer to new design

* fix query.proto

* update deploy_command to use new Querier

* formatting

* formatting

* go.sum

* adds query last unbonding height

* adds orchestrator valset replay

* adds orchestrator data commitment replay

* cosmetics

* puts valset signature catchup in a separate function

* puts data commitment signature catchup in a separate function

* Querier cosmetics

* go.sum

* rename querier and catchup functions

* adds genesis case to querylastvalset

* format + todo

* initialized tm logger in test

* update orchestrator code to use uint64 instead of int64

* better logging

* adds last valset request before  height query

* regenerating correct query.pb.go

* adds logs

* adds querier QueryLastValsetBeforeHeight and QueryDataCommitmentConfirmsByExactRange

* fixes relayer to correctly send commitments

* Update x/qgb/orchestrator/orchestrator_client.go

Co-authored-by: Evan Forbes <[email protected]>

* formats import

* rename addOldValsetAttestations and addOldDataCommitmentAttestations

* defering logging instead of repeating it on every return

* adds comment

* nit

* use the right function for data commitment to tm.hash

* better logging

* missing from merge

* add comments

Co-authored-by: Evan Forbes <[email protected]>
# Conflicts:
#	.gitignore
#	Makefile
#	app/app.go
#	go.mod
#	go.sum
@rach-id rach-id marked this pull request as ready for review September 20, 2022 11:53
@codecov-commenter
Copy link

codecov-commenter commented Sep 20, 2022

Codecov Report

Merging #742 (13a9db9) into main (e58c8a5) will decrease coverage by 23.13%.
The diff coverage is 5.58%.

@@             Coverage Diff             @@
##             main     #742       +/-   ##
===========================================
- Coverage   46.38%   23.24%   -23.14%     
===========================================
  Files          33       71       +38     
  Lines        3251     8796     +5545     
===========================================
+ Hits         1508     2045      +537     
- Misses       1625     6578     +4953     
- Partials      118      173       +55     
Impacted Files Coverage Δ
app/app.go 6.49% <0.00%> (ø)
x/payment/types/builder.go 40.10% <ø> (ø)
x/qgb/genesis.go 0.00% <0.00%> (ø)
x/qgb/handler.go 0.00% <ø> (ø)
x/qgb/keeper/keeper_data_commitment.go 0.00% <0.00%> (ø)
x/qgb/keeper/msg_server.go 0.00% <0.00%> (ø)
x/qgb/keeper/query_attestation.go 0.00% <0.00%> (ø)
x/qgb/keeper/query_general.go 0.00% <0.00%> (ø)
x/qgb/keeper/query_valset.go 0.00% <0.00%> (ø)
x/qgb/module.go 7.14% <0.00%> (ø)
... and 49 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@evan-forbes
Copy link
Member

evan-forbes commented Sep 20, 2022

I will update the QGB universal nonce ADR to make the review easier

side note, we need to straighten out all of the ADR numbers

@evan-forbes
Copy link
Member

imo, let's make as few changes to this PR as possible, since reviewing any of those changes would be difficult and not mesh with the git history. Instead if we have any gripes, we should create (a) follow up issue(s)

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.

super super super dope. glad we're finally getting this merged, and I'm really proud of us (almost 100% @sweexordious lol) for iterating and shipping so fast. Not only are we shipping a working version of the qgb, but one that has altered the original design dramatically to produce something significantly better.

Really big hats off, @sweexordious. Top shelf work.

docs/architecture/ADR-002-QGB-ValSet.md Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
proto/qgb/genesis.proto Show resolved Hide resolved
scripts/protocgen.sh Show resolved Hide resolved
Comment on lines -13 to +19
celestia-appd gentx validator 5000000000utia --keyring-backend="test" --chain-id $CHAINID
celestia-appd gentx validator 5000000000utia \
--keyring-backend="test" \
--chain-id $CHAINID \
--orchestrator-address $(celestia-appd keys show validator -a --keyring-backend="test") \
#da6ed55cb2894ac2c9c10209c09de8e8b9d109b910338d5bf3d747a7e1fc9eb9
--ethereum-address 0x966e6f22781EF6a6A82BBB4DB3df8E225DfD9488

Copy link
Member

Choose a reason for hiding this comment

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

@YazzyYaz just an fyi that this change requires validators to add two extra fields when they create a validator. This differs from the normal cosmos-sdk, so if we have docs for creating a validator, we will need to update them.

also @sweexordious we also want to make a note of this somewhere to make sure that we document this in our own docs.

Copy link
Member Author

Choose a reason for hiding this comment

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

We should decide on which docs do we want to write for now. Do we want to wait until the orchestrator/relayer are ready and document everything? or we want to update existing validator docs?

Choose a reason for hiding this comment

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

@evan-forbes does this apply to Mamaki users or the new Arabica testnet?

Copy link
Member

Choose a reason for hiding this comment

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

does this apply to Mamaki users or the new Arabica testnet?

this will apply to whichever chains will be running using v0.7.0 of celestia-app.

Choose a reason for hiding this comment

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

OK cc @sysrex @Bidon15 just a heads up for when version 0.7.0 of celestia-app is released, we will need to update the configs on running Arabica.

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Bumping our discussion,
What exact rc candidate of 0.7.0 do we want to deploy in arabica for celestia-app?

Copy link
Member

Choose a reason for hiding this comment

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

depends on celestiaorg/celestia-node#1147, but if we can't merge that, then I would recommend sticking to v0.7.0-rc-1

testutil/common.go Outdated Show resolved Hide resolved
testutil/common.go Show resolved Hide resolved
x/qgb/handler.go Show resolved Hide resolved
x/qgb/types/validator.go Show resolved Hide resolved
Copy link
Collaborator

@rootulp rootulp left a comment

Choose a reason for hiding this comment

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

I did a first pass review prior to Thursday's synchronous review. A lot of my feedback pertains to code comments / consistency / naming. They can be addressed in follow-up PRs if we want to minimize the changes in this PR.

docs/architecture/ADR-002-QGB-ValSet.md Outdated Show resolved Hide resolved
docs/architecture/ADR-002-QGB-ValSet.md Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
proto/qgb/query.proto Outdated Show resolved Hide resolved
proto/qgb/query.proto Outdated Show resolved Hide resolved
x/qgb/keeper/keeper_valset.go Outdated Show resolved Hide resolved
x/qgb/keeper/query_valset.go Outdated Show resolved Hide resolved
x/qgb/types/genesis.go Outdated Show resolved Hide resolved
x/qgb/types/valset.go Outdated Show resolved Hide resolved
x/qgb/types/valset_confirm.go Show resolved Hide resolved
@evan-forbes
Copy link
Member

if we don't want to create a cleanup issue @sweexordious with all the feedback here (sounds kinda tedious), we could create a different PR with all the feed back and squash it into this PR

@rach-id
Copy link
Member Author

rach-id commented Sep 20, 2022

@evan-forbes Yes, you're right. I will apply the changes on a separate PR and merge it here.

@evan-forbes
Copy link
Member

evan-forbes commented Sep 23, 2022

if we can't figure out this silly CI bug tmrw morning, I'm calling it as this thing is stupid and weirdly frustrating. We have more important things to do atm lol

we can fix this CI after we ship incentivized testnet

…of cosmos-proto/protoc-gen-gocosmos during proto gen
@rach-id
Copy link
Member Author

rach-id commented Sep 23, 2022

@evan-forbes this commit seems to fix it: 13a9db9
It updates tendermintdev/sdk-proto-gen to v0.7. For some reason, the latest is still pointing towards v0.6 which doesn't have go1.18 installed.

Also, it removes this

go get github.com/regen-network/cosmos-proto/protoc-gen-gocosmos@latest 2>/dev/null
from protocgen.sh. Is there a reason why we have? it seems that it's already imported in the go.mod file.

@evan-forbes
Copy link
Member

evan-forbes commented Sep 23, 2022

Is there a reason why we have? it seems that it's already imported in the go.mod file.

it does seem redundant, so I'm good with removing it.

great work debugging!

friendly reminder to double check that we merge and not squash 🙂

@rach-id
Copy link
Member Author

rach-id commented Sep 23, 2022

Finally mergiiiiing 🎉 🎉
Thanks a lot for the reviews and for all the help provided 👍 👍

@rach-id rach-id merged commit 53d688e into main Sep 23, 2022
@rach-id rach-id deleted the qgb-integration branch September 23, 2022 11:18
@rach-id rach-id restored the qgb-integration branch October 27, 2022 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants