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

fix(chain): do not return error on get_gc_stop_height #3144

Merged
merged 1 commit into from
Aug 12, 2020

Conversation

bowenwang1996
Copy link
Collaborator

Currently when we do not have enough blocks for garbage collection, get_gc_stop_height will return a DBNotFoundErr and the caller handles the error. This caused an incorrect handling of error in process_block and we will reject valid blocks if we do not have enough data for garbage collection, which can happen after a state sync. This PR changes get_gc_stop_height to return genesis height when we do not have enough data for garbage collection and therefore we don't need to worry about error handling at call site.

Test plan

  • test_process_block_after_state_sync

@gitpod-io
Copy link

gitpod-io bot commented Aug 12, 2020

@bowenwang1996 bowenwang1996 force-pushed the fix-gc-stop-height-bug branch from 07222d6 to 37afd2e Compare August 12, 2020 00:49
@codecov
Copy link

codecov bot commented Aug 12, 2020

Codecov Report

Merging #3144 into master will decrease coverage by 0.17%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3144      +/-   ##
==========================================
- Coverage   87.49%   87.31%   -0.18%     
==========================================
  Files         212      212              
  Lines       41463    41424      -39     
==========================================
- Hits        36277    36169     -108     
- Misses       5186     5255      +69     
Impacted Files Coverage Δ
chain/chain/src/types.rs 92.30% <ø> (ø)
chain/chain/src/chain.rs 88.72% <100.00%> (+0.23%) ⬆️
chain/chain/src/test_utils.rs 91.85% <100.00%> (-0.92%) ⬇️
chain/client/tests/process_blocks.rs 90.82% <100.00%> (+0.52%) ⬆️
neard/src/runtime.rs 86.42% <100.00%> (-0.10%) ⬇️
neard/tests/stake_nodes.rs 85.66% <0.00%> (-12.11%) ⬇️
chain/client/src/types.rs 63.75% <0.00%> (-3.75%) ⬇️
chain/client/src/test_utils.rs 81.32% <0.00%> (-3.71%) ⬇️
chain/client/src/sync.rs 86.21% <0.00%> (-2.28%) ⬇️
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 888da94...37afd2e. Read the comment docs.

@bowenwang1996 bowenwang1996 merged commit dcf6258 into master Aug 12, 2020
@bowenwang1996 bowenwang1996 deleted the fix-gc-stop-height-bug branch August 12, 2020 01:51
bowenwang1996 added a commit that referenced this pull request Aug 12, 2020
Currently when we do not have enough blocks for garbage collection, `get_gc_stop_height` will return a `DBNotFoundErr` and the caller handles the error. This caused an incorrect handling of error in `process_block` and we will reject valid blocks if we do not have enough data for garbage collection, which can happen after a state sync. This PR changes `get_gc_stop_height` to return genesis height when we do not have enough data for garbage collection and therefore we don't need to worry about error handling at call site.

Test plan
---------
* `test_process_block_after_state_sync`
bowenwang1996 added a commit that referenced this pull request Aug 13, 2020
Currently when we do not have enough blocks for garbage collection, `get_gc_stop_height` will return a `DBNotFoundErr` and the caller handles the error. This caused an incorrect handling of error in `process_block` and we will reject valid blocks if we do not have enough data for garbage collection, which can happen after a state sync. This PR changes `get_gc_stop_height` to return genesis height when we do not have enough data for garbage collection and therefore we don't need to worry about error handling at call site.

Test plan
---------
* `test_process_block_after_state_sync`
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