Conversation
WalkthroughThis change updates the comparison logic of the Changes
Sequence Diagram(s)sequenceDiagram
participant Test
participant PeerGenerator
participant SyncPeer
Test->>PeerGenerator: generate_peer(latency, accumulated_difficulty)
PeerGenerator->>SyncPeer: Create new SyncPeer with given parameters
Note over SyncPeer: accumulated_difficulty is set (default 1 if None)
Test->>SyncPeer: Invoke comparison (cmp) between peers
SyncPeer-->>Test: Return comparison result based on accumulated difficulty
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration 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: 0
🧹 Nitpick comments (1)
base_layer/core/src/base_node/sync/sync_peer.rs (1)
193-223: Added test to verify sorting by difficultyThe new test case effectively verifies that peers are correctly sorted by their accumulated difficulty in descending order (highest first). This is an important addition that directly validates the fix made to the
Ordimplementation.There's a small inaccuracy in the comment on line 207. It states "because difficulty is identical, it should sort by latency" but this test specifically uses different difficulty values to test sorting by difficulty, not latency.
- // Sort the list; because difficulty is identical, it should sort by latency + // Sort the list; it should sort by difficulty in descending order
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
base_layer/core/src/base_node/sync/sync_peer.rs(4 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
base_layer/core/src/base_node/sync/sync_peer.rs (2)
base_layer/core/src/base_node/state_machine_service/states/header_sync.rs (1)
new(50-71)base_layer/core/src/base_node/proto/chain_metadata.rs (1)
from(72-84)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: ci
- GitHub Check: cargo check with stable
- GitHub Check: Cucumber tests / Base Layer
🔇 Additional comments (2)
base_layer/core/src/base_node/sync/sync_peer.rs (2)
109-113: Corrected peer sorting logic to prioritize higher difficultyThe change fixes the
Ordimplementation to properly sort peers by their accumulated difficulty in descending order. This ensures that peers with higher accumulated difficulty (which typically have more up-to-date chains) are preferred during synchronization.Previously, the comparison was incorrectly causing peers with lower difficulty to be prioritized. This change aligns with the PR objective of fixing peer order during synchronization.
150-163: Enhanced test flexibility by adding difficulty parameterGood enhancement to the
generate_peerfunction by adding an optionalaccumulated_difficultyparameter with a default value. This allows test cases to create peers with specific difficulty values, which is essential for thorough testing of sorting behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
base_layer/core/src/base_node/sync/sync_peer.rs (1)
193-222: Added comprehensive test for difficulty-based peer sorting.The new test
it_sorts_by_poweffectively verifies that peers are sorted correctly based on their accumulated difficulty in descending order. This test ensures that the changes to theOrdimplementation work as intended.Consider using a more descriptive name for the test that clearly indicates it tests the descending order of peers by difficulty, such as
it_sorts_by_pow_in_descending_order.- fn it_sorts_by_pow() { + fn it_sorts_by_pow_in_descending_order() {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
base_layer/core/src/base_node/sync/sync_peer.rs(4 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
base_layer/core/src/base_node/sync/sync_peer.rs (2)
base_layer/core/src/base_node/state_machine_service/states/header_sync.rs (1)
new(50-71)base_layer/core/src/base_node/proto/chain_metadata.rs (1)
from(72-84)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: ci
🔇 Additional comments (4)
base_layer/core/src/base_node/sync/sync_peer.rs (4)
109-113: Fixed peer comparison logic to correctly prioritize peers with higher accumulated difficulty.The comparison logic has been revised to correctly compare peers by accumulated difficulty in descending order. This fix ensures that peers with higher accumulated difficulty (which likely have a more up-to-date blockchain) are prioritized during synchronization, which is the expected behavior for blockchain nodes.
150-158: LGTM: Added optional accumulated difficulty parameter to generate_peer function.The
generate_peerfunction has been enhanced to accept an optional accumulated difficulty parameter, defaulting to 1 if not provided. This improvement allows for creating test peers with specific difficulty values, which is useful for testing the sorting behavior.
161-161: LGTM: Updated ChainMetadata creation to use the provided accumulated_difficulty.The ChainMetadata object creation now correctly incorporates the peer's accumulated difficulty, which ensures that test peers accurately reflect the intended difficulty values for testing purposes.
173-179: LGTM: Updated generate_peer calls with the new parameter.All calls to
generate_peerhave been properly updated to include the newNoneparameter for accumulated difficulty, ensuring compatibility with the updated function signature.
Test Results (CI) 3 files 129 suites 42m 19s ⏱️ Results for commit 1e6782f. |
Test Results (Integration tests)36 tests 36 ✅ 15m 26s ⏱️ Results for commit 1e6782f. |
* development: (412 commits) chore: new release v1.13.1-pre.0 (tari-project#6898) fix: excess sig order in the tx tab (tari-project#6897) chore: ensure thread safety (tari-project#6896) fix: peer order (tari-project#6894) fix: use plain string for grpc address (tari-project#6881) fix: startup arg (tari-project#6889) feat: add num connections to network state (tari-project#6884) refactor: reduce logging to make it less noisy (tari-project#6882) feat: check coinbase count (tari-project#6880) chore(deps): bump dorny/test-reporter from 1 to 2 (tari-project#6883) chore: update readme (tari-project#6878) fix: libtor cli option (tari-project#6877) chore: new version v1.13.0-pre.0 (tari-project#6875) chore: reset network (tari-project#6874) fix: prune mode validation (tari-project#6873) perf: remove duplicate metadata signature verification (tari-project#6866) feat: remove static moneroD response (tari-project#6867) chore(ci): updates to pull version from workspace (tari-project#6868) chore(ci): binaries build continue-on-error when release (tari-project#6865) chore: handle seed words env var (tari-project#6855) ...
2 participants
Description
Fixes sync peer order to ensure it picks the best peer
Summary by CodeRabbit
New Features
Refactor
Tests