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

[Access] Add spork and node root block heights to GetNodeVersionInfo #4690

Merged
merged 5 commits into from
Sep 15, 2023

Conversation

peterargue
Copy link
Contributor

Closes: #4620

After spork transitions, it's useful for clients to have a way to programmatically identify the lowest height that an access node has in it's protocol db. This allows clients to know which blocks they can get from that node, and which they need to use another node, like a historic access node.

This PR updates the GetNodeVersionInfo endpoint to include the

  • SporkRootBlockHeight -> the root height for the spork network
  • NodeRootBlockHeight -> the lowest sealed block the node has in its protocol database

Updates were made to both the gRPC and REST apis.

@@ -145,6 +148,16 @@ func New(params Params) (*Backend, error) {
}
}

sporkRootBlockHeight, err := params.State.Params().SporkRootBlockHeight()
Copy link
Member

Choose a reason for hiding this comment

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

I wonder where do we get this data from? Did we already implemented the snapshot generation which includes the spork root block?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Params.SporkRootBlockHeight is included in the mainnet23 root-protocol-state-snapshot.json. It's extracted from the root snapshot loaded on boot, then written into the db here:
https://github.com/onflow/flow-go/blob/master/state/protocol/badger/state.go#L556-L563

Copy link
Contributor

@koko1123 koko1123 left a comment

Choose a reason for hiding this comment

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

looks good to me

@@ -76,6 +76,9 @@ type Backend struct {
collections storage.Collections
executionReceipts storage.ExecutionReceipts
connFactory connection.ConnectionFactory

sporkRootBlockHeight uint64
Copy link
Contributor

Choose a reason for hiding this comment

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

I have one question here. Could we get these parameters directly from state in GetNodeVersionInfo, the same as for stateParams := b.state.Params() and sporkID, err := stateParams.SporkID(), or do you have another idea behind this?

Copy link
Contributor Author

@peterargue peterargue Sep 14, 2023

Choose a reason for hiding this comment

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

Yea, we could. I was trying to avoid the db lookups since this data is static. We could cache the whole result since it should never change. I'll look into adding that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated in 24898eb

Copy link
Contributor

@Guitarheroua Guitarheroua left a comment

Choose a reason for hiding this comment

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

Looks good, just have one question in the comments.

Base automatically changed from petera/refactor-rpc-backend to master September 14, 2023 18:24
@codecov-commenter
Copy link

codecov-commenter commented Sep 14, 2023

Codecov Report

Patch coverage has no change and project coverage change: -5.06% ⚠️

Comparison is base (ec512fc) 55.46% compared to head (24898eb) 50.41%.
Report is 5 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4690      +/-   ##
==========================================
- Coverage   55.46%   50.41%   -5.06%     
==========================================
  Files         922      310     -612     
  Lines       85754    33086   -52668     
==========================================
- Hits        47563    16680   -30883     
+ Misses      34573    15132   -19441     
+ Partials     3618     1274    -2344     
Flag Coverage Δ
unittests 50.41% <ø> (-5.06%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 617 files with indirect coverage changes

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

@peterargue peterargue added this pull request to the merge queue Sep 15, 2023
Merged via the queue into master with commit c708e77 Sep 15, 2023
36 checks passed
@peterargue peterargue deleted the petera/4620-add-block-heights-node-info branch September 15, 2023 00:58
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.

Add root heights to GetNodeVersionInfo
5 participants