(OraklNode) Major update to use udp instead of tcp for communication among peers #1571
Conversation
WalkthroughWalkthroughThe changes involve a major refactor of the libp2p setup in the node application, transitioning from the Changes
Sequence Diagram(s)sequenceDiagram
participant Main as Main Function
participant Host as NewHost
participant Pubsub as MakePubsub
participant BootAPI as ConnectThroughBootApi
Main->>Host: Create new host with configurations
Host-->>Main: Return host instance
Main->>Pubsub: Set up pubsub with host
Pubsub-->>Main: Return pubsub instance
Main->>BootAPI: Connect through boot API
BootAPI-->>Main: Connection established
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: 9
Outside diff range and nitpick comments (2)
node/pkg/libp2p/setup/setup.go (1)
Line range hint
19-55: RefactorConnectThroughBootApito improve error handling and logging.- log.Error().Err(err).Msg("Error connecting to boot API") + log.Error().Err(err).Str("api_endpoint", apiEndpoint).Msg("Error connecting to boot API")This change adds more context to the error log, making it easier to diagnose issues.
node/pkg/boot/tests/peer_test.go (1)
134-134: Consider adding more host options for comprehensive testing.It might be beneficial to test with different configurations to ensure robustness.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
node/go.modis excluded by!**/*.mod,!**/*.mod
Files selected for processing (17)
- node/cmd/node/main.go (1 hunks)
- node/migrations/boot/000002_update_column.down.sql (1 hunks)
- node/migrations/boot/000002_update_column.up.sql (1 hunks)
- node/pkg/aggregator/main_test.go (1 hunks)
- node/pkg/boot/boot.go (3 hunks)
- node/pkg/boot/peer/controller.go (5 hunks)
- node/pkg/boot/peer/queries.go (1 hunks)
- node/pkg/boot/tests/main_test.go (1 hunks)
- node/pkg/boot/tests/peer_test.go (7 hunks)
- node/pkg/libp2p/setup/setup.go (2 hunks)
- node/pkg/libp2p/setup/type.go (1 hunks)
- node/pkg/libp2p/tests/libp2p_test.go (3 hunks)
- node/pkg/libp2p/utils/utils.go (4 hunks)
- node/pkg/reporter/main_test.go (1 hunks)
- node/script/test_connection/main.go (1 hunks)
- node/script/test_fetcher_and_aggregator/main.go (1 hunks)
- node/script/test_raft/main.go (2 hunks)
Additional comments not posted (29)
node/migrations/boot/000002_update_column.up.sql (3)
1-1: Verify the necessity of deleting all entries from thepeerstable.This operation will remove all existing data. Ensure that this is intended and that data integrity is maintained, possibly through backups or migration strategies.
2-2: The addition of theurlcolumn is correctly handled withIF NOT EXISTS, which prevents errors if the column already exists.
3-3: Ensure that dropping theip,port, andhost_idcolumns does not affect other parts of the application.This operation removes several columns. Confirm that no other parts of the application rely on these columns before proceeding.
node/pkg/boot/peer/queries.go (1)
4-4: The SQL query for inserting peers has been correctly updated to reflect the new database schema.node/migrations/boot/000002_update_column.down.sql (3)
1-1: Verify the necessity of deleting all entries from thepeerstable in the rollback script.This operation will remove all existing data. Ensure that this is intended and that data integrity is maintained, possibly through backups or migration strategies.
2-2: The removal of theurlcolumn is correctly handled withIF EXISTS, which prevents errors if the column does not exist.
3-5: The addition of theip,port, andhost_idcolumns is correctly handled withIF NOT EXISTS, which prevents errors if the columns already exist.node/pkg/libp2p/tests/libp2p_test.go (3)
13-13: TheTestMakeHostfunction correctly tests the new host creation with specific options. Good use of error handling and resource cleanup.
21-21: TheTestMakePubsubfunction correctly tests the pubsub creation using a new host. Proper error handling and resource cleanup are in place.
34-34: TheTestGetHostAddressfunction correctly tests the retrieval of a host's address. Proper error handling and resource cleanup are in place.node/script/test_connection/main.go (3)
26-26: Ensure proper error handling forNewHost.Verification successful
Ensure proper error handling for
NewHost.
- The search results confirm that error handling is consistently implemented after each
NewHostcall across the codebase.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that error handling for `NewHost` is consistent across all usages in the codebase. # Test: Search for the function usage. Expect: Consistent error handling. rg --type go $'NewHost'Length of output: 1924
Script:
#!/bin/bash # Description: Verify error handling for each instance of `NewHost` usage. # Test: Ensure error handling is present after each `NewHost` call. rg --type go --after-context=3 $'NewHost' | grep -E 'if err != nil|log\.Fatal|panic|return err'Length of output: 999
36-36: Ensure proper error handling forConnectThroughBootApi.Verification successful
Proper error handling for
ConnectThroughBootApiis implemented consistently across the codebase. Each instance checks for an error and logs an appropriate message.
node/script/test_raft/main.go: Logs a fatal error message and exits.node/script/test_fetcher_and_aggregator/main.go: Logs an error message and returns.node/script/test_connection/main.go: Logs a fatal error message and exits.node/cmd/node/main.go: Logs an error message and enters a select loop.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that error handling for `ConnectThroughBootApi` is consistent across all usages in the codebase. # Test: Search for the function usage. Expect: Consistent error handling. rg --type go $'ConnectThroughBootApi'Length of output: 478
Script:
#!/bin/bash # Description: Check error handling for each instance of `ConnectThroughBootApi`. # Check error handling in node/script/test_raft/main.go echo "Checking node/script/test_raft/main.go" rg --context 5 --type go $'ConnectThroughBootApi' node/script/test_raft/main.go # Check error handling in node/script/test_fetcher_and_aggregator/main.go echo "Checking node/script/test_fetcher_and_aggregator/main.go" rg --context 5 --type go $'ConnectThroughBootApi' node/script/test_fetcher_and_aggregator/main.go # Check error handling in node/script/test_connection/main.go echo "Checking node/script/test_connection/main.go" rg --context 5 --type go $'ConnectThroughBootApi' node/script/test_connection/main.go # Check error handling in node/cmd/node/main.go echo "Checking node/cmd/node/main.go" rg --context 5 --type go $'ConnectThroughBootApi' node/cmd/node/main.goLength of output: 1797
31-31: Ensure proper error handling forMakePubsub.Verification successful
The verification process confirms that error handling for
MakePubsubis consistently implemented across the codebase. Each instance ofMakePubsubis followed by appropriate error handling, either logging the error or returning it.
node/script/test_raft/main.go: Logs the error and exits.node/script/test_fetcher_and_aggregator/main.go: Logs the error and returns.node/script/test_connection/main.go: Logs the error and exits.node/pkg/reporter/main_test.go: Returns the error.node/pkg/libp2p/tests/libp2p_test.go: Logs the error in a test context.node/pkg/aggregator/main_test.go: Returns the error.node/cmd/node/main.go: Logs the error.No issues were found with the error handling for
MakePubsub.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that error handling for `MakePubsub` is consistent across all usages in the codebase. # Test: Search for the function usage. Expect: Consistent error handling. rg --type go $'MakePubsub'Length of output: 764
Script:
#!/bin/bash # Description: Verify error handling for `MakePubsub` across all usages in the codebase. # Check error handling in node/script/test_raft/main.go echo "Checking node/script/test_raft/main.go" rg -A 3 'MakePubsub' node/script/test_raft/main.go # Check error handling in node/script/test_fetcher_and_aggregator/main.go echo "Checking node/script/test_fetcher_and_aggregator/main.go" rg -A 3 'MakePubsub' node/script/test_fetcher_and_aggregator/main.go # Check error handling in node/script/test_connection/main.go echo "Checking node/script/test_connection/main.go" rg -A 3 'MakePubsub' node/script/test_connection/main.go # Check error handling in node/pkg/reporter/main_test.go echo "Checking node/pkg/reporter/main_test.go" rg -A 3 'MakePubsub' node/pkg/reporter/main_test.go # Check error handling in node/pkg/libp2p/tests/libp2p_test.go echo "Checking node/pkg/libp2p/tests/libp2p_test.go" rg -A 3 'MakePubsub' node/pkg/libp2p/tests/libp2p_test.go # Check error handling in node/pkg/aggregator/main_test.go echo "Checking node/pkg/aggregator/main_test.go" rg -A 3 'MakePubsub' node/pkg/aggregator/main_test.go # Check error handling in node/cmd/node/main.go echo "Checking node/cmd/node/main.go" rg -A 3 'MakePubsub' node/cmd/node/main.goLength of output: 2006
node/pkg/boot/tests/main_test.go (1)
49-49: Ensure the new URL field is correctly used in database queries.Verification successful
The
InsertPeerquery is correctly using theurlfield in all instances, includingmain_test.go,queries.go, andcontroller.go.
main_test.go:map[string]any{"url": "/ip4/100.78.175.63/tcp/10002/p2p/12D3KooWERrdEepSi8HPRNsfjj3Nd7XcxV9SJcHdovpPLyYUtuch"}queries.go:INSERT INTO peers (url) VALUES (@url) RETURNING *;controller.go:map[string]any{"url": payload.Url}Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the new URL field is correctly used in all relevant database queries. # Test: Search for the `InsertPeer` query usage. Expect: Correct usage of the URL field. rg --type go $'InsertPeer'Length of output: 554
Script:
#!/bin/bash # Description: Verify the correct usage of the URL field in the `InsertPeer` query within `controller.go`. # Test: Search for the `InsertPeer` query usage in `controller.go`. Expect: Correct usage of the URL field. rg --type go --context 5 $'InsertPeer' node/pkg/boot/peer/controller.goLength of output: 993
node/script/test_raft/main.go (1)
Line range hint
23-47: Ensure proper error handling and logging forNewHost,MakePubsub, andConnectThroughBootApi.Verification successful
The extracted code segments show that error handling and logging for
NewHost,MakePubsub, andConnectThroughBootApiare generally consistent across the codebase. Each instance checks for errors and logs them appropriately, using eitherlog.Fatal().Err(err).Msg(...)orlog.Error().Err(err).Msg(...), depending on the context.Here are the key observations:
- node/script/test_connection/main.go: Uses
log.Fatal().Err(err).Msg(...)for all three functions.- node/script/test_fetcher_and_aggregator/main.go: Uses
log.Error().Err(err).Msg(...)and returns from the function on error.- node/pkg/reporter/main_test.go: Returns the error directly without logging, which is typical for test files.
- node/pkg/libp2p/tests/libp2p_test.go: Uses
t.Errorfort.Fatalffor error handling, which is appropriate for test files.- node/pkg/boot/boot.go: Uses
log.Error().Err(err).Msg(...)and returns the error.- node/pkg/boot/peer/controller.go: Uses
log.Error().Err(err).Msg(...)and returns an HTTP status code on error.- node/pkg/boot/tests/peer_test.go: Uses
t.Fatalffor error handling in tests.- node/pkg/aggregator/main_test.go: Returns the error directly without logging, which is typical for test files.
- node/cmd/node/main.go: Uses
log.Error().Err(err).Msg(...)and handles the error appropriately.Given these observations, the error handling and logging practices are consistent and comprehensive across the codebase.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that error handling and logging for `NewHost`, `MakePubsub`, and `ConnectThroughBootApi` are consistent and comprehensive across all usages in the codebase. # Test: Search for the function usages. Expect: Consistent and comprehensive error handling and logging. rg --type go $'NewHost|MakePubsub|ConnectThroughBootApi'Length of output: 3140
Script:
#!/bin/bash # Extract relevant code segments from the identified files to review error handling and logging for `NewHost`, `MakePubsub`, and `ConnectThroughBootApi`. # Extract from node/script/test_connection/main.go echo "node/script/test_connection/main.go:" rg --context 5 'NewHost|MakePubsub|ConnectThroughBootApi' node/script/test_connection/main.go # Extract from node/script/test_fetcher_and_aggregator/main.go echo "node/script/test_fetcher_and_aggregator/main.go:" rg --context 5 'NewHost|MakePubsub|ConnectThroughBootApi' node/script/test_fetcher_and_aggregator/main.go # Extract from node/pkg/reporter/main_test.go echo "node/pkg/reporter/main_test.go:" rg --context 5 'NewHost|MakePubsub|ConnectThroughBootApi' node/pkg/reporter/main_test.go # Extract from node/pkg/libp2p/tests/libp2p_test.go echo "node/pkg/libp2p/tests/libp2p_test.go:" rg --context 5 'NewHost|MakePubsub|ConnectThroughBootApi' node/pkg/libp2p/tests/libp2p_test.go # Extract from node/pkg/boot/boot.go echo "node/pkg/boot/boot.go:" rg --context 5 'NewHost|MakePubsub|ConnectThroughBootApi' node/pkg/boot/boot.go # Extract from node/pkg/boot/peer/controller.go echo "node/pkg/boot/peer/controller.go:" rg --context 5 'NewHost|MakePubsub|ConnectThroughBootApi' node/pkg/boot/peer/controller.go # Extract from node/pkg/boot/tests/peer_test.go echo "node/pkg/boot/tests/peer_test.go:" rg --context 5 'NewHost|MakePubsub|ConnectThroughBootApi' node/pkg/boot/tests/peer_test.go # Extract from node/pkg/aggregator/main_test.go echo "node/pkg/aggregator/main_test.go:" rg --context 5 'NewHost|MakePubsub|ConnectThroughBootApi' node/pkg/aggregator/main_test.go # Extract from node/cmd/node/main.go echo "node/cmd/node/main.go:" rg --context 5 'NewHost|MakePubsub|ConnectThroughBootApi' node/cmd/node/main.goLength of output: 6070
node/pkg/boot/boot.go (1)
20-20: Validate the change in refresh interval.The change in
REFRESH_INTERVALfrom 60 seconds to 10 seconds could have significant implications on system performance and peer connectivity. Verify that this change is well-tested and justified:node/pkg/libp2p/setup/type.go (4)
21-27: TheHostConfigstruct andHostOptiontype are well-defined and follow best practices for configuration management in Go.Also applies to: 29-29
31-35: TheWithPortfunction correctly implements the option pattern, allowing dynamic configuration of theHostConfig.
37-41: TheWithPrivateKeyfunction is implemented correctly to set the private key in the host configuration.
43-47: TheWithSecretStringfunction effectively encapsulates the setting of a secret string in the host configuration.node/pkg/boot/peer/controller.go (2)
13-14: The updatedPeerModelandPeerInsertModelstructs correctly reflect the new database schema using a single URL field.Also applies to: 18-18
35-35: Theinsertfunction correctly uses the new URL field for database operations.node/pkg/aggregator/main_test.go (1)
60-60: Thesetupfunction correctly initializes a libp2p host with the new configuration options.node/pkg/reporter/main_test.go (1)
122-122: Thesetupfunction correctly initializes a libp2p host with the new configuration options.node/pkg/boot/tests/peer_test.go (5)
39-39: Good use of assertions to verify the URL after insertion.
88-88: Consistent error messages enhance maintainability.Also applies to: 92-92
98-98: Good practice to use dynamic URLs for peer synchronization tests.Also applies to: 102-102
139-139: Ensure consistent URL extraction and insertion across tests.Also applies to: 145-145, 156-156
27-27: Ensure the URL format is validated before insertion.
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)
- node/pkg/boot/peer/controller.go (5 hunks)
- node/pkg/libp2p/setup/type.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- node/pkg/boot/peer/controller.go
- node/pkg/libp2p/setup/type.go
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)
- node/pkg/boot/boot.go (3 hunks)
- node/pkg/boot/peer/controller.go (5 hunks)
Files skipped from review as they are similar to previous changes (2)
- node/pkg/boot/boot.go
- node/pkg/boot/peer/controller.go
* feat: implement peer count into raft pkg * fix: remove unused property * test: add cases to test based on feedback * (OraklNode) Default to udp - quic (#1573) * feat: default to udp - quic * test: prevent possible duplicate port err * test: fix port error * test: fix test err * fix: remove listen port, add missing `HOST_IP` reference * fix: rollback listen port * fix: update based on feedback
nick-bisonai
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (2)
node/pkg/libp2p/setup/setup.go (1)
Line range hint
19-64: TheConnectThroughBootApifunction is well-structured with clear error handling and logging. However, consider adding more detailed logging at each step to improve traceability during debugging.+ log.Debug().Msg("Attempting to extract connection URL") + log.Debug().Str("externalIp", externalIp).Msg("Using external IP") + log.Debug().Str("apiEndpoint", apiEndpoint).Msg("Using API endpoint")node/pkg/raft/types.go (1)
46-47: Introduction ofReplyHeartbeatMessagestruct with no fields. Consider adding relevant fields or documentation if this is intended to be a marker struct.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (14)
- node/.env.example (1 hunks)
- node/cmd/node/main.go (1 hunks)
- node/pkg/aggregator/main_test.go (2 hunks)
- node/pkg/boot/tests/peer_test.go (7 hunks)
- node/pkg/libp2p/setup/setup.go (2 hunks)
- node/pkg/libp2p/setup/type.go (1 hunks)
- node/pkg/libp2p/tests/libp2p_test.go (3 hunks)
- node/pkg/libp2p/utils/utils.go (4 hunks)
- node/pkg/raft/accessors.go (2 hunks)
- node/pkg/raft/raft.go (7 hunks)
- node/pkg/raft/types.go (4 hunks)
- node/pkg/reporter/main_test.go (2 hunks)
- node/pkg/utils/set/set.go (1 hunks)
- node/pkg/utils/tests/set_test.go (1 hunks)
Files skipped from review due to trivial changes (1)
- node/.env.example
Files skipped from review as they are similar to previous changes (6)
- node/cmd/node/main.go
- node/pkg/aggregator/main_test.go
- node/pkg/libp2p/setup/type.go
- node/pkg/libp2p/tests/libp2p_test.go
- node/pkg/libp2p/utils/utils.go
- node/pkg/reporter/main_test.go
Additional comments not posted (18)
node/pkg/utils/set/set.go (1)
3-26: The implementation of theSetdata structure looks solid and follows good Go practices.node/pkg/utils/tests/set_test.go (3)
10-22: The basic operations test covers all the primary functionalities of the Set. Good use of assertions to validate behavior.
24-30: The test for adding duplicates correctly checks that duplicates are not added to the set. This is crucial for the set's integrity.
32-37: Testing operations on an empty set is a good practice to ensure stability in edge cases. Well done.node/pkg/raft/accessors.go (1)
76-76: The methodSubscribersCountcorrectly utilizes theSizemethod of theSetdata structure to return the number of peers. This is a clean and efficient way to handle this functionality.node/pkg/raft/types.go (3)
9-9: Import ofsetpackage added to support new data structures.
24-24: Addition ofReplyHeartbeatMessageType to support new heartbeat reply functionality.
76-77: Addition ofPrevPeersandPeersfields using the newsetdata structure to manage peer states more efficiently.node/pkg/boot/tests/peer_test.go (5)
27-27: Updated test to use the newUrlfield inPeerInsertModel, aligning with database schema changes.
39-39: Assertion to check if the URL of the inserted peer matches the expected URL. Good practice to ensure data integrity.
88-88: Tests for extracting URLs from hosts. Ensures that the new utility functionExtractConnectionUrlis working as expected.Also applies to: 92-92
98-98: Tests for syncing peers using the new URL field. Validates the functionality of the updated sync logic.Also applies to: 102-102
134-134: Tests for peer insertion and refresh functionality using the new URL field. Good coverage of the new database schema changes.Also applies to: 139-139, 145-145, 156-156
node/pkg/raft/raft.go (5)
13-13: Import ofsetpackage to support new peer management features in the Raft implementation.
45-46: Initialization ofPrevPeersandPeersusing the newsetdata structure. This change supports dynamic peer management.
111-112: Implementation ofhandleReplyHeartbeatmethod to handle incoming heartbeat replies. This addition enhances the Raft protocol's robustness.Also applies to: 228-237
119-121: Logic to swapPeersandPrevPeersduring heartbeat handling. Ensures that peer state transitions are managed correctly.
274-290: MethodsendReplyHeartbeatto publish a heartbeat reply message. This method is crucial for maintaining the state consistency across the cluster.
Description
peersdb, store the whole connection url.Type of change
Please delete options that are not relevant.
Checklist before requesting a review
Deployment