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

Problem: long-running IAVL indexing would happen during upgrade #854

Merged
merged 1 commit into from
Sep 5, 2022

Conversation

tomtau
Copy link
Contributor

@tomtau tomtau commented Sep 2, 2022

Solution: created a custom 0.44.8 fork that cherry-picks
the new version of IAVL

@tomtau
Copy link
Contributor Author

tomtau commented Sep 2, 2022

/runsim

@github-actions
Copy link

github-actions bot commented Sep 2, 2022

Simulation tests started and triggered by /runsim.
Will update here when it succeeds or fails.
Can further check progress here

@tomtau
Copy link
Contributor Author

tomtau commented Sep 2, 2022

@yihuang hm, with old version of gomod2nix, it runs for a long time and crashed:

INFO[2283] Fetching failed, retrying with different rev format  goPackagePath=git.apache.org/thrift.git rev=/v0.12.0
ERRO[2283] Fetching failed                               goPackagePath=git.apache.org/thrift.git
INFO[2283] Received finished job                         current=960 total=960
panic: exit status 1

goroutine 1 [running]:
main.main()

for a new version, it has a different schema

@codecov
Copy link

codecov bot commented Sep 2, 2022

Codecov Report

Merging #854 (400d7ec) into release/v3 (efbc1b6) will decrease coverage by 9.13%.
The diff coverage is n/a.

❗ Current head 400d7ec differs from pull request most recent head 08f1525. Consider uploading reports for the commit 08f1525 to get more accurate results

@@              Coverage Diff              @@
##           release/v3    #854      +/-   ##
=============================================
- Coverage       18.82%   9.69%   -9.14%     
=============================================
  Files              69      36      -33     
  Lines            7972    5764    -2208     
=============================================
- Hits             1501     559     -942     
+ Misses           5962    5116     -846     
+ Partials          509      89     -420     
Flag Coverage Δ
integration_tests ?
unit_tests 9.69% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
app/encoding.go 0.00% <0.00%> (-100.00%) ⬇️
x/nft/types/denom.go 0.00% <0.00%> (-100.00%) ⬇️
app/app.go 1.31% <0.00%> (-88.69%) ⬇️
x/nft/keeper/msg_server.go 0.00% <0.00%> (-75.31%) ⬇️
x/nft/types/keys.go 0.00% <0.00%> (-58.98%) ⬇️
x/nft/types/nft.go 0.00% <0.00%> (-52.95%) ⬇️
x/nft/types/tx.pb.go 0.00% <0.00%> (-22.78%) ⬇️
x/nft/types/nft.pb.go 0.60% <0.00%> (-21.99%) ⬇️
x/nft/types/genesis.go 0.00% <0.00%> (-9.10%) ⬇️
x/nft/types/query.pb.go 0.77% <0.00%> (-8.66%) ⬇️
... and 50 more

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

go.mod Outdated
// TODO: remove when ibc-go upgrades cosmos-sdk version
replace github.com/cosmos/cosmos-sdk => github.com/cosmos/cosmos-sdk v0.44.6
// needed due to a breaking change in 0.44.6
replace github.com/cosmos/cosmos-sdk => github.com/crypto-org-chain/cosmos-sdk v0.44.8-patch
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

@yihuang yihuang Sep 2, 2022

Choose a reason for hiding this comment

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

cosmos/cosmos-sdk#13133
the rollback cmd also need this one to really work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the rollback was primarily cherry-picked because there were some conflicts for the IAVL upgrade commit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to cherry-pick those commits from the rollback fix PR -- I also had to cherry-pick cosmos/cosmos-sdk@3bdbaf1 because otherwise Application interface won't have CommitMultiStore()
cosmos/cosmos-sdk@v0.44.6...crypto-org-chain:cosmos-sdk:v0.44.8-patch3
cosmos/cosmos-sdk@release/v0.44.x...crypto-org-chain:cosmos-sdk:v0.44.8-patch3

@github-actions
Copy link

github-actions bot commented Sep 2, 2022

/runsim simulation test has succeeded 🎉
Can further check here

@yihuang
Copy link
Collaborator

yihuang commented Sep 2, 2022

@yihuang hm, with old version of gomod2nix, it runs for a long time and crashed:

INFO[2283] Fetching failed, retrying with different rev format  goPackagePath=git.apache.org/thrift.git rev=/v0.12.0
ERRO[2283] Fetching failed                               goPackagePath=git.apache.org/thrift.git
INFO[2283] Received finished job                         current=960 total=960
panic: exit status 1

goroutine 1 [running]:
main.main()

for a new version, it has a different schema

better to update to new version I think, the old one is terrible.

@tomtau
Copy link
Contributor Author

tomtau commented Sep 2, 2022

@yihuang I tried to update it with niv: ab9e35a is there something else needed?
when I updated it, it got:

nix shell -f ./default.nix run-integration-tests --extra-experimental-features nix-command --command run-integration-tests
[265](https://github.com/crypto-org-chain/chain-main/runs/8151437117?check_suite_focus=true#step:5:266)
error: attribute 'fetch' missing
[266](https://github.com/crypto-org-chain/chain-main/runs/8151437117?check_suite_focus=true#step:5:267)

[267](https://github.com/crypto-org-chain/chain-main/runs/8151437117?check_suite_focus=true#step:5:268)
       at /nix/store/cx86n1a6mjyfaydchhwl2qiz2mvs216q-gomod2nix-src/builder/default.nix:54:28:
[268](https://github.com/crypto-org-chain/chain-main/runs/8151437117?check_suite_focus=true#step:5:269)

[269](https://github.com/crypto-org-chain/chain-main/runs/8151437117?check_suite_focus=true#step:5:270)
           53|                 src = fetchgit {
[270](https://github.com/crypto-org-chain/chain-main/runs/8151437117?check_suite_focus=true#step:5:271)
           54|                   inherit (meta.fetch) url sha256 rev;
[271](https://github.com/crypto-org-chain/chain-main/runs/8151437117?check_suite_focus=true#step:5:272)
             |                            ^
[272](https://github.com/crypto-org-chain/chain-main/runs/8151437117?check_suite_focus=true#step:5:273)
           55|                   fetchSubmodules = true;
[273](https://github.com/crypto-org-chain/chain-main/runs/8151437117?check_suite_focus=true#step:5:274)
(use '--show-trace' to show detailed location information)
[274](https://github.com/crypto-org-chain/chain-main/runs/8151437117?check_suite_focus=true#step:5:275)
make: *** [Makefile:156: nix-integration-test] Error 1

and updating nixpkgs got:

error: builder for '/nix/store/5hakgzzsa3nmzl7vjc3agj58kiwarrqh-python3.9-hdwallets-0.1.2.drv' failed with exit code 2;
[619](https://github.com/crypto-org-chain/chain-main/runs/8151941869?check_suite_focus=true#step:5:620)
       last 10 log lines:
[620](https://github.com/crypto-org-chain/chain-main/runs/8151941869?check_suite_focus=true#step:5:621)
       >   File "<frozen importlib._bootstrap>", line 1030, in _gcd_import
[621](https://github.com/crypto-org-chain/chain-main/runs/8151941869?check_suite_focus=true#step:5:622)
       >   File "<frozen importlib._bootstrap>", line 1007, in _find_and_load
[622](https://github.com/crypto-org-chain/chain-main/runs/8151941869?check_suite_focus=true#step:5:623)
       >   File "<frozen importlib._bootstrap>", line 972, in _find_and_load_unlocked
[623](https://github.com/crypto-org-chain/chain-main/runs/8151941869?check_suite_focus=true#step:5:624)
       >   File "<frozen importlib._bootstrap>", line 228, in _call_with_frames_removed
[624](https://github.com/crypto-org-chain/chain-main/runs/8151941869?check_suite_focus=true#step:5:625)
       >   File "<frozen importlib._bootstrap>", line 1030, in _gcd_import
[625](https://github.com/crypto-org-chain/chain-main/runs/8151941869?check_suite_focus=true#step:5:626)
       >   File "<frozen importlib._bootstrap>", line 1007, in _find_and_load
[626](https://github.com/crypto-org-chain/chain-main/runs/8151941869?check_suite_focus=true#step:5:627)
       >   File "<frozen importlib._bootstrap>", line 984, in _find_and_load_unlocked
[627](https://github.com/crypto-org-chain/chain-main/runs/8151941869?check_suite_focus=true#step:5:628)
       > ModuleNotFoundError: No module named 'poetry'
[628](https://github.com/crypto-org-chain/chain-main/runs/8151941869?check_suite_focus=true#step:5:629)
       > 
[629](https://github.com/crypto-org-chain/chain-main/runs/8151941869?check_suite_focus=true#step:5:630)
       > 
[630](https://github.com/crypto-org-chain/chain-main/runs/8151941869?check_suite_focus=true#step:5:631)
       For full logs, run 'nix log /nix/store/5hakgzzsa3nmzl7vjc3agj58kiwarrqh-python3.9-hdwallets-0.1.2.drv'.

@yihuang
Copy link
Collaborator

yihuang commented Sep 2, 2022

@yihuang I tried to update it with niv: ab9e35a is there something else needed?

let me open a separate PR to update it.
#855

@tomtau
Copy link
Contributor Author

tomtau commented Sep 2, 2022

@yihuang rebased and it works now, but upgrade fails -- not sure if it's due to the index being computed

@yihuang
Copy link
Collaborator

yihuang commented Sep 2, 2022

@yihuang rebased and it works now, but upgrade fails -- not sure if it's due to the index being computed

don't find error messages, looks like one of the random chain halt cases, trying rerun.

@yihuang
Copy link
Collaborator

yihuang commented Sep 2, 2022

succeed.

@tomtau tomtau force-pushed the patch/iavl branch 2 times, most recently from 44be8f3 to d6c3f1f Compare September 5, 2022 02:21
Solution: created a custom 0.44.8 fork that cherry-picks
the new version of IAVL
@tomtau
Copy link
Contributor Author

tomtau commented Sep 5, 2022

/runsim

@github-actions
Copy link

github-actions bot commented Sep 5, 2022

Simulation tests started and triggered by /runsim.
Will update here when it succeeds or fails.
Can further check progress here

@tomtau tomtau requested a review from yihuang September 5, 2022 03:16
@tomtau
Copy link
Contributor Author

tomtau commented Sep 5, 2022

@yihuang I added the rollback fix: given it could take a long time for the IAVL upgrade and there could be an app hash mismatch due to the dependency upgrade, it's good to have the rollback command working

@github-actions
Copy link

github-actions bot commented Sep 5, 2022

/runsim simulation test has succeeded 🎉
Can further check here

Copy link
Collaborator

@yihuang yihuang left a comment

Choose a reason for hiding this comment

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

LGTM

NOTE: The IAVL v0.19 upgrade introduces a new index that improves performance,
but when the node is started up for the first time, it may take a while
for the index to be initialized (a few minutes or hours depending on the node
configuration and underlying hardware).
Copy link
Collaborator

Choose a reason for hiding this comment

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

I copied this paragraph to cronos v0.8.1 release note ;D

@tomtau tomtau merged commit 688805c into crypto-org-chain:release/v3 Sep 5, 2022
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.

2 participants