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

Dump UpgradeProof to file before upgrade #8116

Closed
wants to merge 2 commits into from

Conversation

AdityaSripal
Copy link
Member

Description

closes: #XXXX


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

// ProofInfo contains the upgraded client and consensus state along with the associated proofs
// This will be useful for relayers who want to upgrade IBC clients of this chain on other counterparty chains
type ProofInfo struct {
ClientState *codectypes.Any `json:"client_state"`
Copy link
Member Author

Choose a reason for hiding this comment

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

These use the types that are required by upgrade client msg so relayers have to do no converting

@AdityaSripal AdityaSripal marked this pull request as draft December 8, 2020 19:35
@codecov
Copy link

codecov bot commented Dec 8, 2020

Codecov Report

Merging #8116 (ee08ae5) into master (f9dc082) will decrease coverage by 0.09%.
The diff coverage is 6.45%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8116      +/-   ##
==========================================
- Coverage   61.88%   61.78%   -0.10%     
==========================================
  Files         609      609              
  Lines       35096    35158      +62     
==========================================
+ Hits        21718    21722       +4     
- Misses      11096    11154      +58     
  Partials     2282     2282              
Impacted Files Coverage Δ
x/upgrade/keeper/keeper.go 45.40% <0.00%> (-22.71%) ⬇️
x/upgrade/abci.go 94.11% <100.00%> (+0.78%) ⬆️

@colin-axner colin-axner mentioned this pull request Dec 8, 2020
4 tasks
Copy link
Contributor

@colin-axner colin-axner left a comment

Choose a reason for hiding this comment

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

This is a great!! Didn't realize producing proofs with the keeper was possible

return nil, err
}
clientRes := queryableMs.Query(abci.RequestQuery{
Path: "store/upgrade/key",
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: use module name const instead of upgrade

return nil, err
}
consStateRes := queryableMs.Query(abci.RequestQuery{
Path: "store/upgrade/key",
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

if err != nil {
return nil, err
}
clientRes := queryableMs.Query(abci.RequestQuery{
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we check if the abci response is valid (no wrapped err)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto with the others

@@ -265,6 +271,98 @@ func (k Keeper) GetUpgradeInfoPath() (string, error) {
return filepath.Join(upgradeInfoFileDir, UpgradeInfoFileName), nil
}

// CreateUpgradeProofInfo creates the proof info struct with all necessary information
// for relayers to upgrade counterparty's IBC clients.
func (k Keeper) CreateUpgradeProofInfo(ctx sdk.Context, height int64) (*types.ProofInfo, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit, can we split the components of this PR into private funcs?

  • Get client state and pack (any)
  • get client state proof and check err
  • get cons state and pack (any)
  • get cons state proof and check err

// GetUpgradeProofPath returns the upgrade proof file path
func (k Keeper) GetUpgradeProofPath() (string, error) {
upgradeInfoFileDir := path.Join(k.getHomeDir(), "data")
err := tmos.EnsureDir(upgradeInfoFileDir, os.ModePerm)
Copy link
Collaborator

Choose a reason for hiding this comment

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

q: what's this tendermint dep?

if err != nil {
return err
}
return ioutil.WriteFile(upgradeProofFilePath, info, 0600)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd add a comment on why 0600 was chosen instead of 0644

@github-actions
Copy link
Contributor

github-actions bot commented Feb 6, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale label Feb 6, 2021
@cwgoes
Copy link
Contributor

cwgoes commented Feb 8, 2021

Bump, should we close this, or is it still useful for some reason?

@colin-axner
Copy link
Contributor

Bump, should we close this, or is it still useful for some reason?

We can close this. Not necessary since it turns out the full node will continue to serve query requests with proofs during an upgrade

@colin-axner colin-axner closed this Feb 8, 2021
@tac0turtle tac0turtle deleted the aditya/dump-upgrade-proof branch March 25, 2022 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants