Skip to content

Conversation

@iulianpascalau
Copy link
Contributor

@iulianpascalau iulianpascalau commented Aug 30, 2023

Reasoning behind the pull request

  • in the case of full history observers, if the epoch is wrongly provided, the node does not search the current active storers like the normal nodes. Default the search first call if the epoch search fails.

Proposed changes

  • extended the search for the full history resolver in case the epoch is wrongly provided

Testing procedure

  • standard system test
  • on a local testnet, in the middle of epoch x stop all validators and the observers that are not full history so the only remaining nodes running should be the full history observers - they won't sync new blocks because the chain is stopped. Connect a random observer to the network that is not in full history mode and with the flag StartInEpoch set to true. The node should be able to finish the bootstrapping phase and sync the last block of the full history node. Repeat this process with the rc/v1.6.0 branch and the random observer node will not be able to finish the bootstrapping phase, it will always error with an error like:
ERROR[2023-08-30 10:20:29.392]   bootstrapDataComponentsFactory create() failed: bootstrap process has failed: time is out 

after about 1 minute after it was started.

Pre-requisites

Based on the Contributing Guidelines the PR author and the reviewers must check the following requirements are met:

  • was the PR targeted to the correct branch?
  • if this is a larger feature that probably needs more than one PR, is there a feat branch created?
  • if this is a feat branch merging, do all satellite projects have a proper tag inside go.mod?

sstanculeanu
sstanculeanu previously approved these changes Aug 30, 2023
andreibancioiu
andreibancioiu previously approved these changes Aug 30, 2023
val, err := resolver.getFromStorage(testKey, testEpoch)
assert.Nil(t, err)
assert.Equal(t, testValue, val)
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Formatting / newline needed (and below).

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 generally prefer inner tests not separated by a new line. We should agree in a future clean code meeting if we will add empty lines or not in this particular case as to slowly align all existing code

assert.Nil(t, err)
assert.Equal(t, 0, len(val))
})
t.Run("get from epoch returned error and will default to search first", func(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

"will fallback to search first".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

@codecov
Copy link

codecov bot commented Aug 30, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.01% ⚠️

Comparison is base (79669bb) 80.10% compared to head (3ace731) 80.10%.
Report is 1 commits behind head on rc/v1.6.0.

❗ Current head 3ace731 differs from pull request most recent head 210e4f0. Consider uploading reports for the commit 210e4f0 to get more accurate results

Additional details and impacted files
@@              Coverage Diff              @@
##           rc/v1.6.0    #5544      +/-   ##
=============================================
- Coverage      80.10%   80.10%   -0.01%     
=============================================
  Files            708      708              
  Lines          93709    93712       +3     
=============================================
+ Hits           75063    75064       +1     
- Misses         13300    13303       +3     
+ Partials        5346     5345       -1     
Files Changed Coverage Δ
dataRetriever/resolvers/baseFullHistoryResolver.go 100.00% <100.00%> (+100.00%) ⬆️

... and 2 files with indirect coverage changes

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

Copy link
Contributor

@gabi-vuls gabi-vuls left a comment

Choose a reason for hiding this comment

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

Normal allin test: v1.5.13-dev-config-8c685c8e00 -> extend-full-history-resolv-0739badb6e

--- Specific errors ---

block hash does not match 7756
wrong nonce in block 2769
miniblocks does not match 0
num miniblocks does not match 0
miniblock hash does not match 0
block bodies does not match 0
receipts hash missmatch 0

/------/

--- Statistics ---

Nr. of all ERRORS: 0
Nr. of all WARNS: 247
Nr. of new ERRORS: 0
Nr. of new WARNS: 2
Nr. of PANICS: 0

/------/

@gabi-vuls gabi-vuls merged commit ab9721e into rc/v1.6.0 Aug 31, 2023
@gabi-vuls gabi-vuls deleted the extend-full-history-resolver-search branch August 31, 2023 16:52
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.

5 participants