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

db: return Miniheader with zero values rather than nil from FindLatestMiniheader when no headers are found #1809

Merged
merged 2 commits into from
Apr 12, 2021

Conversation

kyriediculous
Copy link
Contributor

@kyriediculous kyriediculous commented Mar 23, 2021

What does this pull request do? Explain your changes. (required)
Instead of returning nil from db.FindLatestMiniHeader when no entries are found in the database we return a zero-value to avoid unexpected behaviour whereby no error and no value is returned.

This behaviour is similar to the underlying ethClient.HeaderByNumber implementation.

note
Consider changing blockWatch.Miniheader.Number to an int64 or big.Int instead of a pointer reference *big.Int to avoid explicit initialisation of this field.

&blockwatch.MiniHeader{
  Number: big.NewInt(0)
}

Specific updates (required)

  • Added a test case
  • Return zero value when no results found in FindLatestMiniHeader
  • fixed other test cases for functions that use FindLatestMiniHeader
  • Changed the check for blocksElapsed to be <=0 to avoid verbose logging when no blocks have elapsed.

How did you test each of these updates (required)
Ran unit tests

Does this pull request close any open issues?
Fixes #1803
Fixes #1780

Checklist:

@kyriediculous kyriediculous marked this pull request as ready for review March 23, 2021 16:46
@kyriediculous kyriediculous requested a review from yondonfu March 23, 2021 16:46
Copy link
Member

@yondonfu yondonfu left a comment

Choose a reason for hiding this comment

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

Looks like there are multiple places in block_watcher.go that depend on FindLatestMiniHeader() returning a nil header when there are no headers in the DB. Here is one and here is another. There may be more. Currently, I think returning the zero value for a header in FindLatestMiniHeader() might cause breakage in those areas.

Could be worth weighing continuing to return a nil header and just adding a check for that in the code path that was causing a nil pointer error.

CHANGELOG_PENDING.md Outdated Show resolved Hide resolved
@kyriediculous
Copy link
Contributor Author

Reverted to old behaviour and added a nil check. Also added a quick one character fix for #1780.

Tested on rinkeby and checked if there were other cases we should add a nil check but didn't see any immeadiatly. Will double-check.

eth/blockwatch/block_watcher.go Outdated Show resolved Hide resolved
eth/blockwatch/block_watcher.go Outdated Show resolved Hide resolved
Copy link
Member

@yondonfu yondonfu left a comment

Choose a reason for hiding this comment

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

LGTM after squashing

CHANGELOG_PENDING.md Outdated Show resolved Hide resolved
@kyriediculous kyriediculous force-pushed the nv/blockwatch-nil branch 3 times, most recently from b492bdd to 7cc6a6f Compare April 11, 2021 21:21
Copy link
Member

@yondonfu yondonfu left a comment

Choose a reason for hiding this comment

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

One comment on changelog, but LGTM after fixing.

CHANGELOG_PENDING.md Outdated Show resolved Hide resolved
@kyriediculous
Copy link
Contributor Author

This one okay to merge ?

@yondonfu
Copy link
Member

Yep and already approved.

@kyriediculous kyriediculous merged commit e1e54ef into master Apr 12, 2021
@kyriediculous kyriediculous deleted the nv/blockwatch-nil branch April 12, 2021 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants