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.

Sign up for GitHub

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

Jump to bottom

native: prevent improper Neo cache invalidation #3279

Merged
merged 2 commits into from
Dec 21, 2023
Merged

native: prevent improper Neo cache invalidation #3279

merged 2 commits into from
Dec 21, 2023

Conversation

AnnaShaleva
Copy link
Member

Close #3253.

@AnnaShaleva
native: never set Neo's newEpoch* cache values to nil
d7d850a 
We have cache update mechanism (Neo's cache votesChanged flag), it must
be used for current epoch and new epoch cached values update. And the
cached current/new epoch values themselves must always contain valid
information for the current/new epoch. These cached values must only be
changed once per epoch, never set them to nil.

This commit prevents CN node panic described in #3253 when dBFT tries
to retrieve new epoch validators with some votes modifications made
before at the same dBFT epoch.

Close #3253.

Signed-off-by: Anna Shaleva <[email protected]>
@codecov Codecov
Copy link

codecov bot commented Dec 21, 2023 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (de98b39) 85.30% compared to head (47aefad) 85.31%.

Additional details and impacted files 
@@           Coverage Diff           @@
##           master    #3279   +/-   ##
=======================================
  Coverage   85.30%   85.31%           
=======================================
  Files         327      327           
  Lines       44153    44147    -6     
=======================================
- Hits        37664    37662    -2     
+ Misses       5001     4998    -3     
+ Partials     1488     1487    -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@AnnaShaleva
consensus: remove unnecessary call to ComputeNextBlockValidators
47aefad 
dBFT doesn't use validators got from this call to GetValidators callback,
because NeoGo doesn't properly set WithGetConsensusAddress, and thus
this call can be safely skipped. Instead, NeoGo fills NextConsensus field
by itself in NewBlockFromContext callback.

This commit technically doesn't perform any functional changes and doesn't
affect the problem described in #3253 in any way. This commit is just a
removal of the code that was never used by NeoGo library.

This commit is a direct consequence of nspcc-dev/dbft#84.

Signed-off-by: Anna Shaleva <[email protected]>
@AnnaShaleva
Copy link
Member Author

@fyfyrchik, welcome to review if you'd like to!

@AnnaShaleva
Copy link
Member Author

Tested on latest mainnet with WatchOnly consensus node, problem is solved (I was managed to reproduce it earlier with the current master and WatchOnly consensus node):

2023-12-21T22:11:48.440+0300	INFO	received PrepareResponse	{"validator": 4}
2023-12-21T22:11:48.453+0300	INFO	received PrepareResponse	{"validator": 3}
2023-12-21T22:11:48.453+0300	INFO	received PrepareResponse	{"validator": 1}
2023-12-21T22:11:48.466+0300	INFO	received Commit	{"validator": 2}
2023-12-21T22:11:48.470+0300	INFO	received Commit	{"validator": 6}
2023-12-21T22:11:48.487+0300	INFO	received Commit	{"validator": 1}
2023-12-21T22:11:48.487+0300	INFO	received Commit	{"validator": 3}
2023-12-21T22:11:48.499+0300	INFO	received Commit	{"validator": 5}
2023-12-21T22:11:48.500+0300	INFO	approving block	{"height": 4609122, "hash": "d483f259eaf132cc8dbfc165f86571ac04b7f2fedc9a532ac3e6ed0efc555f78", "tx_count": 0, "merkle": "0000000000000000000000000000000000000000000000000000000000000000", "prev": "14c55ef890c45a5b32847b516618388b0b2b2bef237cb5c4db9819c799705a12"}
2023-12-21T22:11:48.519+0300	INFO	initializing dbft	{"height": 4609123, "view": 0, "index": -1, "role": "WatchOnly"}
2023-12-21T22:11:49.432+0300	INFO	persisted to disk	{"blocks": 1, "keys": 105, "headerHeight": 4609122, "blockHeight": 4609122, "took": "5.222417ms"}
2023-12-21T22:11:58.439+0300	INFO	persisted to disk	{"blocks": 0, "keys": 2, "headerHeight": 4609122, "blockHeight": 4609122, "took": "8.014464ms"}
2023-12-21T22:12:04.734+0300	INFO	received PrepareRequest	{"validator": 1, "tx": 0}
2023-12-21T22:12:04.741+0300	INFO	received PrepareResponse	{"validator": 5}
2023-12-21T22:12:04.743+0300	INFO	received PrepareResponse	{"validator": 4}
2023-12-21T22:12:04.766+0300	INFO	received PrepareResponse	{"validator": 3}
2023-12-21T22:12:04.838+0300	INFO	received PrepareResponse	{"validator": 6}
2023-12-21T22:12:04.840+0300	INFO	received Commit	{"validator": 6}
2023-12-21T22:12:04.989+0300	INFO	received PrepareResponse	{"validator": 2}
2023-12-21T22:12:04.993+0300	INFO	received Commit	{"validator": 2}
2023-12-21T22:12:05.328+0300	INFO	received Commit	{"validator": 1}
2023-12-21T22:12:05.371+0300	INFO	received Commit	{"validator": 4}
2023-12-21T22:12:05.436+0300	INFO	received Commit	{"validator": 3}
2023-12-21T22:12:05.437+0300	INFO	approving block	{"height": 4609123, "hash": "b47be61aa27797ed18f055898c5f9166db57697c4141fb7c2d182d4e2482f082", "tx_count": 0, "merkle": "0000000000000000000000000000000000000000000000000000000000000000", "prev": "d483f259eaf132cc8dbfc165f86571ac04b7f2fedc9a532ac3e6ed0efc555f78"}
2023-12-21T22:12:05.444+0300	INFO	initializing dbft	{"height": 4609124, "view": 0, "index": -1, "role": "WatchOnly"}
2023-12-21T22:12:06.439+0300	INFO	persisted to disk	{"blocks": 1, "keys": 24, "headerHeight": 4609123, "blockHeight": 4609123, "took": "4.420163ms"}
2023-12-21T22:12:20.514+0300	INFO	received PrepareRequest	{"validator": 2, "tx": 1}
2023-12-21T22:12:20.530+0300	INFO	received PrepareResponse	{"validator": 6}
2023-12-21T22:12:20.668+0300	INFO	received PrepareResponse	{"validator": 5}
2023-12-21T22:12:20.880+0300	INFO	received PrepareResponse	{"validator": 4}
2023-12-21T22:12:20.933+0300	INFO	received Commit	{"validator": 2}
2023-12-21T22:12:20.961+0300	INFO	received Commit	{"validator": 5}
2023-12-21T22:12:21.012+0300	INFO	received PrepareResponse	{"validator": 3}
2023-12-21T22:12:21.034+0300	INFO	received Commit	{"validator": 6}
2023-12-21T22:12:21.157+0300	INFO	received Commit	{"validator": 3}
2023-12-21T22:12:21.836+0300	INFO	received PrepareResponse	{"validator": 1}
2023-12-21T22:12:21.846+0300	INFO	received Commit	{"validator": 4}
2023-12-21T22:12:21.846+0300	INFO	approving block	{"height": 4609124, "hash": "2a7693f27422e9e3280a504bd2b260d2cea5ffd86603492d3d25a7331a2c7074", "tx_count": 1, "merkle": "e0169ce1628891a087e6b7c402223101ee32c4bdd756241ac2028d4e1ee09d5f", "prev": "b47be61aa27797ed18f055898c5f9166db57697c4141fb7c2d182d4e2482f082"}
2023-12-21T22:12:21.871+0300	INFO	initializing dbft	{"height": 4609125, "view": 0, "index": -1, "role": "WatchOnly"}
2023-12-21T22:12:22.461+0300	INFO	persisted to disk	{"blocks": 1, "keys": 109, "headerHeight": 4609124, "blockHeight": 4609124, "took": "15.580952ms"}
2023-12-21T22:12:36.832+0300	INFO	received PrepareResponse	{"validator": 4}
2023-12-21T22:12:36.852+0300	INFO	received PrepareRequest	{"validator": 3, "tx": 2}
2023-12-21T22:12:36.853+0300	INFO	received PrepareResponse	{"validator": 2}
2023-12-21T22:12:36.874+0300	INFO	received PrepareResponse	{"validator": 6}
2023-12-21T22:12:36.875+0300	INFO	received PrepareResponse	{"validator": 5}
2023-12-21T22:12:36.883+0300	INFO	received PrepareResponse	{"validator": 1}
2023-12-21T22:12:36.910+0300	INFO	received Commit	{"validator": 6}
2023-12-21T22:12:36.982+0300	INFO	received Commit	{"validator": 2}
2023-12-21T22:12:36.993+0300	INFO	received PrepareResponse	{"validator": 0}
2023-12-21T22:12:37.028+0300	INFO	received Commit	{"validator": 5}
2023-12-21T22:12:37.100+0300	INFO	received Commit	{"validator": 0}
2023-12-21T22:12:37.101+0300	INFO	received Commit	{"validator": 4}
2023-12-21T22:12:37.101+0300	INFO	approving block	{"height": 4609125, "hash": "368c985c0ab4b8fd56f52a89014c05e4a71c2942c2a26423f4ae65abfe24a538", "tx_count": 2, "merkle": "44e2f75d82fc97d32fdc16cd729c2fd6b0f991dd4a4de66dd3ae7d9fc1ccd061", "prev": "2a7693f27422e9e3280a504bd2b260d2cea5ffd86603492d3d25a7331a2c7074"}
2023-12-21T22:12:37.130+0300	INFO	initializing dbft	{"height": 4609126, "view": 0, "index": -1, "role": "WatchOnly"}
2023-12-21T22:12:37.459+0300	INFO	persisted to disk	{"blocks": 1, "keys": 122, "headerHeight": 4609125, "blockHeight": 4609125, "took": "5.171221ms"}
2023-12-21T22:12:42.461+0300	INFO	persisted to disk	{"blocks": 0, "keys": 2, "headerHeight": 4609125, "blockHeight": 4609125, "took": "3.733067ms"}
2023-12-21T22:12:44.464+0300	INFO	persisted to disk	{"blocks": 0, "keys": 2, "headerHeight": 4609125, "blockHeight": 4609125, "took": "5.589899ms"}
2023-12-21T22:12:52.258+0300	INFO	received PrepareRequest	{"validator": 4, "tx": 1}
2023-12-21T22:12:52.259+0300	INFO	received PrepareResponse	{"validator": 6}
2023-12-21T22:12:52.286+0300	INFO	received PrepareResponse	{"validator": 3}
2023-12-21T22:12:52.307+0300	INFO	received PrepareResponse	{"validator": 1}
2023-12-21T22:12:52.381+0300	INFO	received PrepareResponse	{"validator": 0}
2023-12-21T22:12:52.413+0300	INFO	received Commit	{"validator": 6}
2023-12-21T22:12:52.792+0300	INFO	received Commit	{"validator": 1}
2023-12-21T22:12:52.837+0300	INFO	received Commit	{"validator": 4}
2023-12-21T22:12:52.923+0300	INFO	received Commit	{"validator": 3}
2023-12-21T22:12:53.035+0300	INFO	received Commit	{"validator": 0}
2023-12-21T22:12:53.035+0300	INFO	approving block	{"height": 4609126, "hash": "c637d326b3a2fc29ef66eedc6bb51b104212cec4f7d26584120e900455c5e044", "tx_count": 1, "merkle": "5f0e34e7ae614902641bdc4b406cce4953f9c65dff08ffdd2a9ea64fb247b4c2", "prev": "368c985c0ab4b8fd56f52a89014c05e4a71c2942c2a26423f4ae65abfe24a538"}
2023-12-21T22:12:53.053+0300	INFO	initializing dbft	{"height": 4609127, "view": 0, "index": -1, "role": "WatchOnly"}

States match the current mainnet up to:

anna@kiwi:~/Documents/GitProjects/nspcc-dev/neo-go$ go run scripts/compare-states/compare-states.go http://localhost:10332 http://seed1.neo.org:10332
at 0: 58a5157b7e99eeabf631291f1747ec8eb12ab89461cda888492b17a301de81e8 vs 58a5157b7e99eeabf631291f1747ec8eb12ab89461cda888492b17a301de81e8
at 4609122: 3aa0be66bed8452b70ae6dd0b5126122d80e4b1cc2a578d9e79bf7de1abaf9be vs 3aa0be66bed8452b70ae6dd0b5126122d80e4b1cc2a578d9e79bf7de1abaf9be

So @roman-khimov, can be safely merged and\or rolled on mainnet CN.

@roman-khimov roman-khimov merged commit a872c20 into master Dec 21, 2023
18 of 19 checks passed
@roman-khimov roman-khimov deleted the fix-next-vals-panic branch December 21, 2023 19:17
@fyfyrchik fyfyrchik mentioned this pull request Apr 8, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roman-khimov roman-khimov roman-khimov approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CN panic: unexpected external call to newEpochNextValidators cache
2 participants
@AnnaShaleva @roman-khimov