Skip to content

[test][node] Cover join stage-1 becoming the new leftmost node - #142

Merged
thep2p merged 2 commits into
thep2p/90-join-stage1-no-right-neighbor-testfrom
thep2p/90-join-stage1-new-leftmost-node-test
Sep 30, 2026
Merged

thep2p merged 2 commits into
thep2p/90-join-stage1-no-right-neighbor-testfrom
thep2p/90-join-stage1-new-leftmost-node-test

Conversation

@thep2p

@thep2p thep2p commented Sep 14, 2026

Copy link
Copy Markdown
Owner

Summary

  • Add test_join_stage1_link_level0_new_leftmost_node_sends_single_link_request: the mirror of the no-right-neighbor test, covering introducer.id() > u.id(), where u becomes the new leftmost node reachable from introducer. This is the scenario the direction-selection fix (in the orchestration PR earlier in this stack) exists to handle correctly, so it's the test that actually proves that fix.
  • Two small carried-over fixes caught during final verification against the reference implementation, both one-line: BaseNode::search_by_id's own span was missing its parent: &self.span (the pre-existing synchronous search method, missed when the span-parentage PR was split out earlier in this stack), and Core::search_by_id's doc comment was missing the paragraph explaining why the relay chain's self-terminating fallback is only sound when the caller picks the search direction so the first hop already satisfies it, the doc-side explanation of the same invariant the orchestration PR's direction-selection logic depends on.

This completes #90's required test coverage: normal two-sided link, one-node-graph edge case, and no-known-neighbor (both sides) are now all covered.

Test plan

  • cargo fmt --check
  • cargo clippy --all-targets -- -D warnings
  • cargo clippy --all-targets --all-features -- -D warnings
  • cargo test
  • cargo doc --no-deps

@thep2p
thep2p added this pull request to stack #143 September 14, 2026 20:23
@thep2p
thep2p force-pushed the thep2p/90-join-stage1-new-leftmost-node-test branch from fe11b97 to f26479b Compare September 15, 2026 17:27
@thep2p
thep2p force-pushed the thep2p/90-join-stage1-new-leftmost-node-test branch from f26479b to 14e3226 Compare September 15, 2026 17:50
@thep2p
thep2p force-pushed the thep2p/90-join-stage1-new-leftmost-node-test branch from 14e3226 to 63378d8 Compare September 15, 2026 21:09
@thep2p
thep2p force-pushed the thep2p/90-join-stage1-new-leftmost-node-test branch from 63378d8 to 554f4d7 Compare September 27, 2026 05:17
@thep2p
thep2p force-pushed the thep2p/90-join-stage1-new-leftmost-node-test branch from 554f4d7 to 8096337 Compare September 30, 2026 00:52
@thep2p
thep2p force-pushed the thep2p/90-join-stage1-new-leftmost-node-test branch 2 times, most recently from 753eed7 to 74709d3 Compare September 30, 2026 18:58
@thep2p
thep2p force-pushed the thep2p/90-join-stage1-new-leftmost-node-test branch from 74709d3 to afe9cb3 Compare September 30, 2026 21:18

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟢 Approval recommended

The test accurately covers the intended edge case, and the documentation matches the implementation.

Review effort: Balanced
Findings: None

What changed in this PR

Adds coverage for stage-1 joining when the node becomes the new leftmost node and documents the search-direction invariant.

Changes:

  • Tests the leftward search and single-link-request path.
  • Documents requirements for safe search fallback behavior.
File Description
src/​node/​base_node.rs Adds the new-leftmost-node join test.
src/​node/​core.rs Clarifies the directional search invariant.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@thep2p

thep2p commented Sep 30, 2026

Copy link
Copy Markdown
Owner Author

@claude review

@github-actions

github-actions Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

Claude finished @thep2p's task —— View job


Review: PR #142 — join stage-1 new leftmost node test

  • Read changed files (base_node.rs, core.rs)
  • Analyze the new test for correctness
  • Review doc/span fixes
  • Post review verdict

Verdict: Approve. The test is correct, a faithful mirror of the existing no-right-neighbor test, and the two carried-over fixes are sound.

Test correctness (base_node.rs:2956-3116)

I traced the new test against join_stage1_link_level0 (base_node.rs:424-494) and every assertion lines up:

  • introducer > own_id selects search_direction = Left (base_node.rs:433-434) → the SearchByIdRequest direction assertion (Direction::Left) is right.
  • The GetNeighborOp reuses search_direction (base_node.rs:466) → the Direction::Left assertion on the neighbor query is right.
  • GetLinkOp to z carries dir = search_direction = Left (base_node.rs:469), i.e. this node proposes itself for z's left slot → the new req.dir == Direction::Left assertion is correct, and a nice addition over the sibling test which omits it.
  • The SetLinkOp reply carries dir: Right (the mirror, via dir.opposite() at base_node.rs:346), so it lands on this node's right slot → the final Right == Some(z_id) / Left == None assertions are right, matching "u becomes the new leftmost node."

Nonce-cell plumbing, poll_until_some sequencing, and the link_request_count == 1 guard all mirror the established pattern in the no-right-neighbor test.

Minor observations (non-blocking)

  • Neighbor reply direction: the delivered RetNeighborOp uses direction: Direction::Left (base_node.rs:~3045), matching the query. The sibling test uses Right. Consistent with the mirror — good.
  • Dead binding mem_vec: let mem_vec = random_membership_vector(); is passed into BaseCore::new, so it's used — not dead. Fine.
  • The z_identity's membership vector is random and never asserted on; that's acceptable since stage-1 level-0 linking doesn't gate on the mem vec. Matches the sibling.

core.rs doc fix (core.rs:28-39)

The reworded fallback description and the new paragraph explaining the direction-selection invariant are accurate: they correctly state the Aspnes & Shah fallback is only sound when the caller's own id already satisfies the direction relation, that relayed hops have this by construction, and that the first externally-seeded hop must pick req.direction to preserve it — exactly what join_stage1_link_level0's direction selection enforces. No behavior change.

Note

I couldn't run cargo fmt / clippy / test here (cargo isn't in my allowed tools), so I verified by code inspection only. CI should confirm the test-plan checks.
• thep2p/90-join-stage1-new-leftmost-node-test

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Test is correct and a faithful mirror of the existing no-right-neighbor test. Traced every assertion against join_stage1_link_level0: direction selection (introducer > own_id -> Left), the GetLinkOp dir=Left proposal for z's left slot, and the SetLinkOp mirror landing z on this node's right slot all line up, matching 'u becomes the new leftmost node'. The added req.dir assertion is a nice improvement over the sibling test. The core.rs doc fixes (fallback wording + direction-selection invariant paragraph) are accurate and behavior-preserving. Verified by inspection; CI covers fmt/clippy/test/doc.

@thep2p
thep2p merged commit 5f89509 into main Sep 30, 2026
6 checks passed
@thep2p
thep2p deleted the thep2p/90-join-stage1-new-leftmost-node-test branch September 30, 2026 23:32
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.

2 participants