near / nearcore

Reference client for NEAR Protocol
https://near.org
GNU General Public License v3.0
2.31k stars 615 forks source link

fix: tentative fix for test gc_after_sync1 #11816

Closed Trisfald closed 1 month ago

Trisfald commented 1 month ago

Quoting the test description:

Spin up one validating node and one nonvalidating node stop the nonvalidating node in the second epoch and restart it in the fourth epoch to trigger state sync Check that after 10 epochs the node has properly garbage collected blocks.

After 10 epochs the test checks that the 6 epochs are garbage collected. The problem is: one particular block (89 - the last block of epoch before state sync) is explicitly requested by state sync so it will be returned by the RPC call.

This PR changes the test to expect the presence of block 89. If instead the block shouldn't be there, it's a bug in state sync / GC.

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 71.76%. Comparing base (0557fa4) to head (e2392be). Report is 4 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #11816 +/- ## ========================================== + Coverage 71.72% 71.76% +0.03% ========================================== Files 796 796 Lines 162984 163196 +212 Branches 162984 163196 +212 ========================================== + Hits 116897 117112 +215 + Misses 41037 41033 -4 - Partials 5050 5051 +1 ``` | [Flag](https://app.codecov.io/gh/near/nearcore/pull/11816/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near) | Coverage Δ | | |---|---|---| | [backward-compatibility](https://app.codecov.io/gh/near/nearcore/pull/11816/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near) | `0.23% <ø> (-0.01%)` | :arrow_down: | | [db-migration](https://app.codecov.io/gh/near/nearcore/pull/11816/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near) | `0.23% <ø> (-0.01%)` | :arrow_down: | | [genesis-check](https://app.codecov.io/gh/near/nearcore/pull/11816/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near) | `1.35% <ø> (-0.01%)` | :arrow_down: | | [integration-tests](https://app.codecov.io/gh/near/nearcore/pull/11816/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near) | `37.83% <ø> (+0.01%)` | :arrow_up: | | [linux](https://app.codecov.io/gh/near/nearcore/pull/11816/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near) | `71.36% <ø> (+0.01%)` | :arrow_up: | | [linux-nightly](https://app.codecov.io/gh/near/nearcore/pull/11816/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near) | `71.34% <ø> (+0.01%)` | :arrow_up: | | [macos](https://app.codecov.io/gh/near/nearcore/pull/11816/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near) | `54.62% <ø> (+1.65%)` | :arrow_up: | | [pytests](https://app.codecov.io/gh/near/nearcore/pull/11816/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near) | `1.62% <ø> (+0.03%)` | :arrow_up: | | [sanity-checks](https://app.codecov.io/gh/near/nearcore/pull/11816/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near) | `1.42% <ø> (+0.03%)` | :arrow_up: | | [unittests](https://app.codecov.io/gh/near/nearcore/pull/11816/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near) | `66.21% <ø> (+0.05%)` | :arrow_up: | | [upgradability](https://app.codecov.io/gh/near/nearcore/pull/11816/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near) | `0.28% <ø> (-0.01%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Trisfald commented 1 month ago

@VanBarbascu I added you as reviewer, if you can have a look if the updated test assertions are good.

TL;DR - for state sync to work starting at epoch 4 (height 90) it has to fetch the last block of previous epoch (height 89) and that's why block 89 is not GC'd in this scenario

Longarithm commented 1 month ago

I suspect this is because block is staying in the cache - and cache is not cleaned up properly, as stated in https://github.com/near/nearcore/blob/9932e95c5c49262189bc4737366ce46bb2b953c8/chain/chain/src/store/mod.rs#L1245-L1259. Should we rather clean up block-related caches for the sake of data consistency?

shreyan-gupta commented 1 month ago

Had a quick chat with Razvan and ideally the block should have been GC'd as we only care about the last block of the previous epoch for state sync purposes and no other block. So the error is a genuine error.

Trisfald commented 1 month ago

Thanks, then I'll close this PR since the error is genuine and we should fix it.