Multi-spork client access support #230
Conversation
WalkthroughThe recent changes enhance the system to handle cross-spork access by implementing a multi-spork client in the Changes
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- bootstrap/bootstrap.go (2 hunks)
- config/config.go (4 hunks)
- models/cross-spork_client.go (1 hunks)
- tests/helpers.go (1 hunks)
- tests/integration_test.go (1 hunks)
Files skipped from review due to trivial changes (1)
- tests/helpers.go
Additional comments not posted (1)
tests/integration_test.go (1)
58-70: Ensure that theAccessNodeHostfield is correctly utilized in all relevant parts of the test setup.
There was a problem hiding this comment.
Actionable comments posted: 3
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- bootstrap/bootstrap.go (2 hunks)
- models/cross-spork_client.go (1 hunks)
- models/cross-spork_client_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- bootstrap/bootstrap.go
Additional comments not posted (6)
models/cross-spork_client_test.go (6)
13-14: Ensure proper error handling inNewCrossSporkClientinitialization.Verification successful
The error handling in the initialization of
NewCrossSporkClientappears to be robust. The function is designed to return an error, which is appropriately checked in the test file usingrequire.NoError(t, err). This ensures that the test will fail if an error occurs during the client initialization. Additionally, the usage inbootstrap.goalso captures the error, allowing for proper handling or propagation in the application logic.
models/cross-spork_client_test.go: Error checked withrequire.NoError(t, err)bootstrap/bootstrap.go: Error captured and needs to be handled or propagatedOverall, the implementation and usage of
NewCrossSporkClientalign with good practices in Go for error handling.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that error handling is robust during the initialization of `CrossSporkClient`. # Test: Search for error handling patterns in `NewCrossSporkClient` usage. rg --type go $'NewCrossSporkClient'Length of output: 465
16-20: Check the logic for adding sporks inAddSporkmethod.Verification successful
The
AddSporkmethod in theCrossSporkClientclass has been successfully located and reviewed. The method checks if a spork with the givenlastHeightalready exists in thesporkHostsmap. If it does, an error is returned, indicating that the provided last height already exists. This effectively handles the edge case of duplicate heights. If the height does not exist, a new client is created for the given host, and it is added to thesporkHostsmap.Given this implementation, the method does handle the specific edge case of duplicate heights appropriately by preventing the addition of a spork with a height that is already registered.
Conclusion:
TheAddSporkmethod correctly handles the edge case of duplicate heights as per the implementation in theCrossSporkClientclass. No issues were found regarding this specific edge case handling.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the logic for adding sporks handles edge cases correctly. # Test: Search for `AddSpork` method usage and ensure it handles duplicate heights and other edge cases. rg --type go $'AddSpork'Length of output: 363
Script:
#!/bin/bash # Extract the implementation of `AddSpork` method to verify edge case handling rg --type go -A 10 'func (c \*CrossSporkClient) AddSpork' models/cross-spork_client.goLength of output: 88
Script:
#!/bin/bash # Confirm the presence of the `AddSpork` method in the expected file cat models/cross-spork_client.goLength of output: 3849
22-23: Validate the client retrieval logic ingetClientForHeight.
28-29: Ensure error messages are informative and helpful inGetBlockByHeight.Verification successful
The error handling associated with the
GetBlockByHeightmethod across various files in the codebase appears to be adequately informative and context-specific. Here are the findings:
- In
models/cross-spork_client_test.go, the error messages include specific client hosts, which can be very useful for identifying issues related to specific network nodes or configurations.- In
bootstrap/bootstrap.go, the error is enhanced with a clear message indicating a failure to retrieve a block at a specified height, which is crucial for debugging issues related to blockchain state or height mismatches.- In
services/ingestion/subscriber.go, the error message explicitly includes the block height that could not be retrieved, directly pointing out the potential data availability issue.These implementations suggest that the error messages are designed to provide the necessary context to diagnose issues effectively. Therefore, the original review comment concerning the informativeness of the error messages in
GetBlockByHeightis addressed satisfactorily in the codebase.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that error messages in `GetBlockByHeight` provide enough context to be useful. # Test: Search for `GetBlockByHeight` usage and check the associated error handling. rg --type go $'GetBlockByHeight'Length of output: 660
Script:
#!/bin/bash # Extract the implementation of GetBlockByHeight and its error handling rg --type go -C 10 'func (.*CrossSporkClient.*GetBlockByHeight' models/cross-spork_client.go rg --type go -C 5 'GetBlockByHeight' services/ingestion/subscriber.go models/cross-spork_client_test.go bootstrap/bootstrap.goLength of output: 2830
32-33: Check that the client selection logic for specific heights is accurate inGetBlockByHeight.Verification successful
The implementation of
GetBlockByHeightin theCrossSporkClientclass reveals a clear mechanism for client selection based on the block height. The functiongetClientForHeightis used to determine the appropriate client for a given height. This function iterates through sorted height boundaries and selects the client corresponding to the last upper height limit that is greater than or equal to the requested height. This logic ensures that the correct client is used based on the block height, aligning with the expectations set in the review comment.Given this information, the client selection logic for specific heights in
GetBlockByHeightis accurately implemented as per the requirements.
- The logic is implemented in
models/cross-spork_client.gowithin thegetClientForHeightmethod, which is called byGetBlockByHeight.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the client selection logic for specific heights in `GetBlockByHeight` is accurate. # Test: Search for `GetBlockByHeight` usage and ensure it selects the correct client based on height. rg --type go $'GetBlockByHeight'Length of output: 660
Script:
#!/bin/bash # Description: Extract the implementation of `GetBlockByHeight` to verify client selection logic based on height. # Extract the function implementation from `cross-spork_client.go` rg --type go --context 20 'func \(c \*CrossSporkClient\) GetBlockByHeight' models/cross-spork_client.goLength of output: 1188
36-37: Verify that the client selection for lower heights is handled correctly inGetBlockByHeight.Verification successful
The review comment asked to verify the handling of client selection for lower heights in the
GetBlockByHeightmethod. From the provided script outputs, we can see the implementation details incross-spork_client.goand the test cases incross-spork_client_test.go.Analysis of Implementation:
The
CrossSporkClientclass incross-spork_client.gohas a methodgetClientForHeightwhich determines the appropriate client based on the block height. It iterates through the spork boundaries in descending order and selects the client that matches the height criteria. This method is used inGetBlockByHeightto fetch the block from the correct spork client based on the height.Analysis of Test Cases:
The test cases in
cross-spork_client_test.gospecifically check the client selection logic:
- For height 300, it expects to use the current spork client (
test1.com).- For height 150, it expects to use the client from the second spork (
test2.com).- For height 50, it expects to use the client from the third spork (
test3.com).- It also tests the boundary condition at height 200, expecting to use the client from the third spork (
test3.com).These tests align with the implementation logic in
cross-spork_client.go, confirming that the client selection for lower heights (and other heights) is handled as expected.Conclusion:
The implementation and tests confirm that the client selection for lower heights in
GetBlockByHeightis handled correctly. The test cases cover various scenarios including lower heights, ensuring that the correct client is selected based on the spork boundaries.Final Response:
The verification of the client selection for lower heights in
GetBlockByHeightconfirms that the method correctly handles different heights according to the spork boundaries. The associated test cases incross-spork_client_test.goadequately test this functionality, ensuring the correct client is used based on the block height.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the client selection for lower heights in `GetBlockByHeight` is handled correctly. # Test: Search for `GetBlockByHeight` usage and ensure it handles lower heights appropriately. rg --type go $'GetBlockByHeight'Length of output: 660
Script:
#!/bin/bash # Inspect the implementation of GetBlockByHeight in cross-spork_client.go cat models/cross-spork_client.go # Check for any specific logic in cross-spork_client_test.go related to client selection based on height cat models/cross-spork_client_test.goLength of output: 4975
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- models/cross-spork_client_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- models/cross-spork_client_test.go
config/config.go
Outdated
| var streamTimeout int | ||
| var initHeight uint64 | ||
|
|
||
| // parse from flags | ||
| flag.StringVar(&cfg.DatabaseDir, "database-dir", "./db", "Path to the directory for the database") | ||
| flag.StringVar(&cfg.RPCHost, "rpc-host", "", "Host for the RPC API server") | ||
| flag.IntVar(&cfg.RPCPort, "rpc-port", 8545, "Port for the RPC API server") | ||
| flag.StringVar(&cfg.AccessNodeGRPCHost, "access-node-grpc-host", "localhost:3569", "Host to the flow access node gRPC API") | ||
| flag.StringVar(&cfg.AccessNodeHost, "access-node-grpc-host", "localhost:3569", "Host to the flow access node gRPC API") | ||
| flag.StringVar(&accessSporkHosts, "access-node-spork-hosts", "", `Previous spork AN hosts, defined following the schema: {latest height}@{host} as comma separated list (e.g. "[email protected],[email protected]")`) |
There was a problem hiding this comment.
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.
I can make it that way yeah
There was a problem hiding this comment.
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.
it seems like there are 2 modes for the clients:
- current
- historic
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()
models/cross-spork_client.go
Outdated
| return err | ||
| } | ||
|
|
||
| c.sporkHosts[lastHeight] = client |
There was a problem hiding this comment.
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.
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.
I don't have access to this method through the SDK
There was a problem hiding this comment.
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.
Actionable comments posted: 2
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (4)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sumtests/go.modis excluded by!**/*.modtests/go.sumis excluded by!**/*.sum
Files selected for processing (1)
- services/ingestion/subscriber_test.go (1 hunks)
| client. | ||
| On("SubscribeEventsByBlockHeight", mock.Anything, mock.Anything, mock.Anything). | ||
| Return(func() (<-chan flowGo.BlockEvents, <-chan error, error) { | ||
| events := make(chan flowGo.BlockEvents) | ||
|
|
||
| for i := startHeight; i <= endHeight; i++ { | ||
| events <- flowGo.BlockEvents{ | ||
| BlockHeight: i, | ||
| } | ||
| } | ||
|
|
||
| return events, make(chan error), nil | ||
| }) |
There was a problem hiding this comment.
Consider adding a close operation for the events channel to prevent potential resource leaks.
+ close(events)This change ensures that the channel is properly closed after all events have been sent, preventing goroutines from hanging indefinitely.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| client. | |
| On("SubscribeEventsByBlockHeight", mock.Anything, mock.Anything, mock.Anything). | |
| Return(func() (<-chan flowGo.BlockEvents, <-chan error, error) { | |
| events := make(chan flowGo.BlockEvents) | |
| for i := startHeight; i <= endHeight; i++ { | |
| events <- flowGo.BlockEvents{ | |
| BlockHeight: i, | |
| } | |
| } | |
| return events, make(chan error), nil | |
| }) | |
| client. | |
| On("SubscribeEventsByBlockHeight", mock.Anything, mock.Anything, mock.Anything). | |
| Return(func() (<-chan flowGo.BlockEvents, <-chan error, error) { | |
| events := make(chan flowGo.BlockEvents) | |
| for i := startHeight; i <= endHeight; i++ { | |
| events <- flowGo.BlockEvents{ | |
| BlockHeight: i, | |
| } | |
| } | |
| close(events) | |
| return events, make(chan error), nil | |
| }) |
| for ev := range events { | ||
| require.NoError(t, ev.Err) | ||
|
|
||
| // this makes sure all the event heights are sequential | ||
| eventHeight := ev.Events.CadenceHeight() | ||
| require.Equal(t, prevHeight+1, eventHeight) | ||
| prevHeight = eventHeight | ||
| } |
There was a problem hiding this comment.
Consider handling potential errors from the event channel directly within the loop.
+ if ev.Err != nil {
+ t.Errorf("Error processing event: %v", ev.Err)
+ break
+ }
- require.NoError(t, ev.Err)This change improves the robustness of the test by handling errors immediately when they occur, allowing for cleaner error reporting and the ability to stop processing upon encountering an error.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| for ev := range events { | |
| require.NoError(t, ev.Err) | |
| // this makes sure all the event heights are sequential | |
| eventHeight := ev.Events.CadenceHeight() | |
| require.Equal(t, prevHeight+1, eventHeight) | |
| prevHeight = eventHeight | |
| } | |
| for ev := range events { | |
| if ev.Err != nil { | |
| t.Errorf("Error processing event: %v", ev.Err) | |
| break | |
| } | |
| // this makes sure all the event heights are sequential | |
| eventHeight := ev.Events.CadenceHeight() | |
| require.Equal(t, prevHeight+1, eventHeight) | |
| prevHeight = eventHeight | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- services/ingestion/subscriber_test.go (1 hunks)
Additional comments not posted (2)
services/ingestion/subscriber_test.go (2)
43-80: LGTM! The mock client setup function is well-structured and effectively simulates different block heights.
83-125: LGTM! The test function effectively simulates the behavior of the blockchain client across different sporks and checks for sequential event heights.
| } | ||
| }() | ||
|
|
||
| return events, make(chan error), nil |
There was a problem hiding this comment.
Consider closing the error channel to prevent potential resource leaks.
+ close(errChan)This change ensures that the error channel is properly closed after all events have been sent, preventing goroutines from hanging indefinitely.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| return events, make(chan error), nil | |
| return events, make(chan error), nil | |
| close(errChan) |
| for ev := range events { | ||
| if prevHeight == endHeight { | ||
| require.ErrorIs(t, ev.Err, models.ErrDisconnected) | ||
| break | ||
| } |
There was a problem hiding this comment.
Consider handling potential errors from the event channel directly within the loop.
+ if ev.Err != nil {
+ t.Errorf("Error processing event: %v", ev.Err)
+ break
+ }
- require.NoError(t, ev.Err)This change improves the robustness of the test by handling errors immediately when they occur, allowing for cleaner error reporting and the ability to stop processing upon encountering an error.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| for ev := range events { | |
| if prevHeight == endHeight { | |
| require.ErrorIs(t, ev.Err, models.ErrDisconnected) | |
| break | |
| } | |
| for ev := range events { | |
| if ev.Err != nil { | |
| t.Errorf("Error processing event: %v", ev.Err) | |
| break | |
| } | |
| if prevHeight == endHeight { | |
| require.ErrorIs(t, ev.Err, models.ErrDisconnected) | |
| break | |
| } |
bootstrap/bootstrap.go
Outdated
| return fmt.Errorf("failed to create client connection for host: %s, with error: %w", cfg.AccessNodeHost, err) | ||
| } | ||
|
|
||
| client, err := models.NewCrossSporkClient(grpcClient, logger) |
There was a problem hiding this comment.
I believe we could encapsulate the instantiation logic for CrossSporkClient, with a method that accepts cfg.AccessNodeHost & cfg.AccessNodePreviousSporkHosts and does the necessary creation of clients.
Maybe not something addressable in this PR, but a note to consider.
models/cross-spork_client.go
Outdated
|
|
||
| // NewCrossSporkClient creates a new instance of the client, it accepts the | ||
| // host to the current spork AN API. | ||
| func NewCrossSporkClient(currentSporkClient access.Client, logger zerolog.Logger) (*CrossSporkClient, error) { |
There was a problem hiding this comment.
There isn't any statement in this methods' body that can cause an error. If we choose to move the instantiation of current spork client and past sport clients in this method, then it makes sense to keep it.
models/cross-spork_client.go
Outdated
| @@ -0,0 +1,142 @@ | |||
| package models | |||
There was a problem hiding this comment.
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 the models package, but rather the services package. What do you think?
There was a problem hiding this comment.
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.
Oh, I see. This didn't even cross my mind 😇
# Conflicts: # config/config.go # go.mod # go.sum # tests/go.mod # tests/go.sum
|
@m-Peter changed the factory like you said in this PR. |
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (4)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sumtests/go.modis excluded by!**/*.modtests/go.sumis excluded by!**/*.sum
Files selected for processing (5)
- bootstrap/bootstrap.go (6 hunks)
- config/config.go (4 hunks)
- services/ingestion/subscriber.go (1 hunks)
- services/ingestion/subscriber_test.go (1 hunks)
- services/requester/cross-spork_client.go (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- bootstrap/bootstrap.go
- config/config.go
- services/ingestion/subscriber_test.go
Additional comments not posted (17)
services/requester/cross-spork_client.go (10)
23-28: Struct definition looks good.
30-50: Function definition looks good.
52-72: Function definition looks good.
74-77: Function definition looks good.
79-103: Function definition looks good.
105-111: Function definition looks good.
113-124: Function definition looks good.
126-133: Function definition looks good.
135-144: Function definition looks good.
146-155: Function definition looks good.services/ingestion/subscriber.go (7)
29-34: Struct definition looks good.
37-39: Function definition looks good.
42-92: Function definition looks good.
94-156: Function definition looks good.
158-215: Function definition looks good.
217-240: Function definition looks good.
21-26: Interface definition looks good.
| } | ||
|
|
||
| // getSporkBoundaries will return descending order of spork height boundaries | ||
| func (c *CrossSporkClient) getSporkBoundariesDesc() []uint64 { |
There was a problem hiding this comment.
Minor: We can move this to NewCrossSporkClient and cache it, as it is not expected to change after initialization.
There was a problem hiding this comment.
good point, I moved it into factory.
|
@sideninja Looks good 👏 |
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- services/requester/cross-spork_client.go (1 hunks)
- tests/integration_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- tests/integration_test.go
Additional comments not posted (7)
services/requester/cross-spork_client.go (7)
31-57: Initialization ofCrossSporkClientlooks robust and well-structured.
60-80: MethodaddSporkcorrectly handles adding new spork clients and checks for duplicates.
82-85: MethodIsPastSporkefficiently checks if a height is within past sporks.
87-110: MethodgetClientForHeightcorrectly selects the appropriate client based on the given height.
113-124: MethodGetLatestHeightForSporkcorrectly retrieves the latest block height for a given spork.
126-133: MethodGetBlockHeaderByHeightcorrectly fetches the block header for a specific height.
146-155: MethodSubscribeEventsByBlockHeightcorrectly subscribes to events based on the start height.
# Conflicts: # go.mod # go.sum # tests/go.mod # tests/go.sum
Closes: #229
Description
This PR adds support for connecting to multiple AN hosts configured by the height boundary, which can be used to support multi-spork access.
We can use the new
--access-node-spork-hostsflag to configure multiple hosts. This is optional, for the current spork we still use the--access-node-grpc-hostflag.For contributor use:
masterbranchFiles changedin the Github PR explorerSummary by CodeRabbit
New Features
Enhancements
Configuration Changes
AccessNodeGRPCHosttoAccessNodeHost.AccessNodePreviousSporkHoststo manage spork hosts.Bug Fixes