Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Multi-spork client access support #230
Multi-spork client access support #230
Changes from 13 commits
fc36a20
0eaa3d4
e47ad92
b887c37
cf5e6ed
4ca5093
ca26af2
b291868
21ced97
b42c16a
a7ee4db
cbb71e4
f6fdfbc
aee635c
8479fcb
d068c59
1874e82
156026d
5d7a81e
44078d8
819a84d
969e9b5
0e08f32
ccfb9e4
8dba9c2
b6a680a
b182d07
d55ac59
1556554
183496a
c37a942
375b36c
15ebaff
4ea7f9a
d207e5b
56d6d19
fad7b9e
9a3ce74
67aceef
873113f
10094b2
ffac9c3
0949ca4
45e49dc
689797c
6e4dca5
02d85ad
db9f8d7
de05844
3f910aa
03b4666
752af31
ac66503
b133733
e2b9adb
3cadf70
e600315
68d9dff
f1a1df5
1269289
14d972f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does
latest height
mean last height? is it sealed or finalized? It may make sense to define this as the first height in the spork since that's available in the sporks.json file as well as via the API.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can make it that way yeah
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually on the second thought, I'm not sure it makes sense to do it that way, since then I cannot know if a height is in the current spork or previous, let me show with an example, if I provide one previous spork that starts at height 100, and I want to check for height 150, I can not know whether thats from previous spork or current spork, this would then require for current spork client to also be defined with starting height, which I wanted to to avoid. We could just subtract 1 from whats in sporks.json no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems like there are 2 modes for the clients:
In both cases, you can lookup the first block available using
GetNodeVersionInfo()
.In the historic case, you can lookup the last block available using
GetLatestBlockHeader()
. In some cases (infra bugs) this does not match[next spork start height] - 1
because the AN is missing some blocks. We always backfill when this happens, but something to be aware of.When I originally wrote this comment, I was thinking the first block would be easier for the operator to get, but I think you're right that they can just derive it from to next spork. Another benefit of using the last height is you could validate that the height matches the response from
GetLatestBlockHeader()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: Can we rename the file to
cross_spork_client.go
. Right now there's mixed-
&_
.Another note: The functionality of this
CrossSporkClient
doesn't make me feel like it belongs to themodels
package, but rather theservices
package. What do you think?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm using
-
and_
mixed because the_
is instead of a space, whereas-
is actually how you write "cross-spork" in english.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see. This didn't even cross my mind 😇
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's a good idea to validate the range to make sure there is a contiguous range of blocks. You can use
GetNodeVersionInfo()
to get the first block (NodeRootBlockHeight
is the first block the node has):https://github.com/onflow/flow-go/blob/1d581d472b7dcc40cd8952da497422c910ebd99b/access/api.go#L259-L266
GetLatestBlockHeader()
to get the latest sealed block.Then you can search all nodes to check for gaps
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good idea, didn't know it exists. In my mind if you wrongly configured it would err out when accessing, but this is better for sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have access to this method through the SDK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can issue this up as a follow up PR since I believe it will require updating Go SDK first and then updating here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#231