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: The unlucky transactions are not shown in json rpc #458

Merged
merged 16 commits into from
May 10, 2022

Conversation

yihuang
Copy link
Collaborator

@yihuang yihuang commented May 6, 2022

Closes: #455

  • add fix-unlucky-tx command
  • test in integration test
$ cronosd fix-unlucky-tx 1 106 --home data/cronos_777-1/node0

👮🏻👮🏻👮🏻 !!!! REFERENCE THE PROBLEM YOUR ARE SOLVING IN THE PR TITLE AND DESCRIBE YOUR SOLUTION HERE !!!! DO NOT FORGET !!!! 👮🏻👮🏻👮🏻

PR Checklist:

  • Have you read the CONTRIBUTING.md?
  • Does your PR follow the C4 patch requirements?
  • Have you rebased your work on top of the latest master?
  • Have you checked your code compiles? (make)
  • Have you included tests for any non-trivial functionality?
  • Have you checked your code passes the unit tests? (make test)
  • Have you checked your code formatting is correct? (go fmt)
  • Have you checked your basic code style is fine? (golangci-lint run)
  • If you added any dependencies, have you checked they do not contain any known vulnerabilities? (go list -json -m all | nancy sleuth)
  • If your changes affect the client infrastructure, have you run the integration test?
  • If your changes affect public APIs, does your PR follow the C4 evolution of public contracts?
  • If your code changes public APIs, have you incremented the crate version numbers and documented your changes in the CHANGELOG.md?
  • If you are contributing for the first time, please read the agreement in CONTRIBUTING.md now and add a comment to this pull request stating that your PR is in accordance with the Developer's Certificate of Origin.

Thank you for your code, it's appreciated! :)

CHANGELOG.md Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved

replace github.com/tharsis/ethermint => github.com/yihuang/ethermint v0.7.2-cronos-9.0.20220427040028-1d16a8af7dfc
replace github.com/tharsis/ethermint => github.com/crypto-org-chain/ethermint v0.7.2-cronos-15
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the commits are identical, just move to crypto-org-chain fork and make a tag.
https://github.com/crypto-org-chain/ethermint/compare/1d16a8af7dfc...v0.7.2-cronos-15?body=&expand=1

Copy link
Contributor

@tomtau tomtau left a comment

Choose a reason for hiding this comment

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

not sure how long the "find from 0 to the upgrade height" process will take, but if too long, maybe good to later add a constant that contains all problematic block heights, so one can directly iterate over those?

Closes: crypto-org-chain#455
- add patch-tx-result command
- test in integration test
@yihuang yihuang marked this pull request as ready for review May 6, 2022 02:51
@yihuang yihuang requested a review from a team as a code owner May 6, 2022 02:51
@yihuang yihuang requested review from calvinaco and leejw51crypto and removed request for a team May 6, 2022 02:51
@yihuang
Copy link
Collaborator Author

yihuang commented May 6, 2022

not sure how long the "find from 0 to the upgrade height" process will take, but if too long, maybe good to later add a constant that contains all problematic block heights, so one can directly iterate over those?

I'm trying in a mainnet node, will see results later.

scripts/devnet.yaml Outdated Show resolved Hide resolved
scripts/devnet.yaml Outdated Show resolved Hide resolved
x/cronos/rpc/api.go Outdated Show resolved Hide resolved
go.mod Show resolved Hide resolved
@yihuang
Copy link
Collaborator Author

yihuang commented May 6, 2022

not sure how long the "find from 0 to the upgrade height" process will take, but if too long, maybe good to later add a constant that contains all problematic block heights, so one can directly iterate over those?

It takes almost 2hours to run the following command on a mainnet rocksdb node.:

$ cronosd fix-unlucky-tx 10000 20000 --home /chain/.cronosd/

it patched 13 transactions.
it seems indeed too slow to run, still doing more analysis on performance.

It takes almost 8 minutes to call LoadHeight, most of the time is spent on this.

@yihuang
Copy link
Collaborator Author

yihuang commented May 6, 2022

We can use app options to enable lazy loading of the cms, let's see how much that improve.

@yihuang
Copy link
Collaborator Author

yihuang commented May 6, 2022

Speed:

Iterated Blocks Patched Blocks Duration Block Range
6274 2647 3h26m 2280346 - 2286619
10000 17 1m41s 1 - 10000
10000 0 1m14s 1 - 10000 (already patched)

Replay block is the most costly part, the duration is almost proportional to the number of patched blocks, it takes several seconds per patched block.
The time is spent on the tx execution itself, LoadHeight is pretty fast after using the "lazy load" trick, so I guess there's no easy optimization on that.
I've tried on several known problematic blocks, the result looks alright.

@yihuang yihuang requested review from tomtau and thomas-nguy May 9, 2022 02:24
@thomas-nguy
Copy link
Collaborator

thomas-nguy commented May 9, 2022

I see,

Do you know it takes for a single patch? (you can try using the command start=x, end=x+1)

if the command can take as input a file with a list of all "problematic" height, maybe it could be faster.

Assuming that it takes 1min for 10000 blocks and that the current height in mainnet is 2682549, it can already
save few hours

@yihuang
Copy link
Collaborator Author

yihuang commented May 9, 2022

I see,

Do you know it takes for a single patch? (you can try using the command start=x, end=x+1)

if the command can take as input a file with a list of all "problematic" height, maybe it could be faster.

Assuming that it takes 1min for 10000 blocks and that the current height in mainnet is 2682549, it can already save few hours

The time spent on iterate normal blocks are negligible compared to the time to patch blocks, it takes around 4 hours to iterate all the blocks without doing the patch, it takes around 5 seconds to patch a block, so I guess it'll take days to patch all the problematic blocks. We can run it on a offline node first, then share the patched db to the other nodes.

@yihuang
Copy link
Collaborator Author

yihuang commented May 9, 2022

if the command can take as input a file with a list of all "problematic" height, maybe it could be faster.

it might make sense to decompose the command into:

cronosd fix-unlucky-tx dump-block-numbers
cronosd fix-unlucky-tx patch-blocks --blocks /path/to/block-numbers-file
cronosd fix-unlucky-tx patch-blocks --block-start $START --block-end $END

@lgtm-com
Copy link

lgtm-com bot commented May 9, 2022

This pull request introduces 1 alert when merging 0575a36 into ca00b0f - view on LGTM.com

new alerts:

  • 1 for Useless assignment to local variable

yihuang and others added 3 commits May 9, 2022 14:43
Co-authored-by: Thomas Nguy <81727899+thomas-nguy@users.noreply.github.com>
Copy link
Collaborator

@thomas-nguy thomas-nguy left a comment

Choose a reason for hiding this comment

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

looks good

@yihuang
Copy link
Collaborator Author

yihuang commented May 9, 2022

$ Prepare the test chain data
$ ./build/cronosd fix-unlucky-tx --home $CRONOS_HOME --end-block 46
patched 0xfdf21b2554785ba2e54107c1b2259dde930d618cb87bd0419cff69284ac1bb70 15 1
patched 0x64878d500003daf5d1c1888a818317e4ad88c21bebde9c409a84baf994ed142b 18 1
patched 0x53a6af6feed9b82ff34620ae17961a797ca28882f586dfed19639a42ca678232 21 1
patched 0xa5cdd666b6e488ffeb3ecd08fdc74942c3ab18ac85d5977df39ae6a56db32060 25 1
patched 0x71b1be75b5c1376ad57f801cbae037f550a9fe2c4938dd6fd5bd0e784c528bd0 28 1
patched 0x6af3a405d36dea8020e89ced537bfd73d94ce8e32ad69e6e5f085f897f5fa8d7 31 1
patched 0xdcf2f5164856966af2b4ef4caf97b24893422cb98fc6d4f97870a6c0b033befe 34 1
patched 0xaccdc7f87a52dd1a7de6d90942e9ac84949f425934e2dedc7e767f52f45efd8f 38 1
patched 0xa89283364e23cfeea7e2360850b5b7c546f61183a40a76571b2e5afc021a4e93 41 1
patched 0x6e3e73c119b4e943ea57266048eca6d77b22d2fec99f002d91547cdb62c00235 44 1


$ Reprepare the test chain data
$ ./build/cronosd fix-unlucky-tx --home $CRONOS_HOME --print-block-numbers --end-block 46
15 1
18 1
22 1
25 1
28 1
31 1
34 1
38 1
41 1
44 1
$ ./build/cronosd fix-unlucky-tx --home $CRONOS_HOME --print-block-numbers --end-block 46 | cut -d ' ' -f 1 | tee /tmp/blocks
15
18
22
25
28
31
34
38
41
44
$ ./build/cronosd fix-unlucky-tx --home $CRONOS_HOME --blocks-file /tmp/blocks --dry-run
... the tx execution results ...
$ ./build/cronosd fix-unlucky-tx --home $CRONOS_HOME --blocks-file /tmp/blocks
patched 0xfdf21b2554785ba2e54107c1b2259dde930d618cb87bd0419cff69284ac1bb70 15 1
patched 0x64878d500003daf5d1c1888a818317e4ad88c21bebde9c409a84baf994ed142b 18 1
patched 0x53a6af6feed9b82ff34620ae17961a797ca28882f586dfed19639a42ca678232 22 1
patched 0xa5cdd666b6e488ffeb3ecd08fdc74942c3ab18ac85d5977df39ae6a56db32060 25 1
patched 0x71b1be75b5c1376ad57f801cbae037f550a9fe2c4938dd6fd5bd0e784c528bd0 28 1
patched 0x6af3a405d36dea8020e89ced537bfd73d94ce8e32ad69e6e5f085f897f5fa8d7 31 1
patched 0xdcf2f5164856966af2b4ef4caf97b24893422cb98fc6d4f97870a6c0b033befe 34 1
patched 0xaccdc7f87a52dd1a7de6d90942e9ac84949f425934e2dedc7e767f52f45efd8f 38 1
patched 0xa89283364e23cfeea7e2360850b5b7c546f61183a40a76571b2e5afc021a4e93 41 1
patched 0x6e3e73c119b4e943ea57266048eca6d77b22d2fec99f002d91547cdb62c00235 44 1

Manual local test result, looks alright.

Copy link
Contributor

@tomtau tomtau left a comment

Choose a reason for hiding this comment

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

I guess it'll take days to patch all the problematic blocks.

will it be possible for the command to have a progress tracker / ETA for completion?

@yihuang
Copy link
Collaborator Author

yihuang commented May 10, 2022

I guess it'll take days to patch all the problematic blocks.

will it be possible for the command to have a progress tracker / ETA for completion?

I think the patch time for each block should be almost constant, we dump the block numbers need to patch in advance, then we can calculate how much time is left according to the latest patched block height.

@yihuang yihuang merged commit 47c90c6 into crypto-org-chain:release/v0.6.x May 10, 2022
@yihuang yihuang deleted the full-replay-tx branch May 10, 2022 04:33
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.

3 participants