refactor: Add Cluster::isMember, a membership query that copies nothing - #8221
Conversation
Cluster::member returns std::optional<std::string>, copying the node's configured name out from under the mutex. Two production callers only ever test the result for engagement and throw the string away, and one of them is PeerImp::cluster(), which ten call sites in PeerImp use - several of them per message. Adds a bool-returning isMember alongside member and uses it at those two sites. The two callers that actually want the name, PeerImp's connection banner and ConnectAttempt's, are left alone. Covered by a case in the existing cluster suite asserting the two queries agree over a mixed population.
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean, mechanical refactor: adds Cluster::isMember mirroring the existing member lookup under the same lock but without copying the name, and updates the two boolean-only call sites (PeerImp::cluster(), OverlayImpl::onHandoff) plus adds a test case verifying isMember agrees with member. Implementation and call-site changes are correct and consistent with the existing locking/lookup pattern; no correctness, security, or consistency issues found in the added lines.
There was a problem hiding this comment.
🟢 Approval recommended
No unresolved issues remain, and membership consistency is covered by tests.
Pull request overview
Adds a non-copying cluster membership query and updates membership-only callers to use it.
Changes:
- Added
Cluster::isMember. - Updated peer and overlay membership checks.
- Added consistency test coverage.
File summaries
| File | Summary |
|---|---|
src/xrpld/overlay/detail/PeerImp.cpp |
Uses the efficient membership query. |
src/xrpld/overlay/detail/OverlayImpl.cpp |
Uses the query for handoff decisions. |
src/xrpld/overlay/detail/Cluster.cpp |
Implements locked membership lookup. |
src/xrpld/overlay/Cluster.h |
Declares and documents the new query. |
src/test/overlay/cluster_test.cpp |
Verifies consistency with member. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
godexsoft
left a comment
godexsoft
left a comment
There was a problem hiding this comment.
Small request, otherwise good to go 👍
Review feedback pointed out that the cluster/network sizes in the new "isMember agrees with member" test case were unnamed magic numbers. Pull them into overlapCount and clusterSize constants so the intent of each value is clear.
54040e7 to
0dc2c44
Compare
There was a problem hiding this comment.
Clean, minimal refactor: adds Cluster::isMember(PublicKey const&) mirroring the existing member() locking pattern but avoiding the string copy, and correctly swaps the two boolean-only call sites (PeerImp::cluster, OverlayImpl::onHandoff) to use it while leaving the two callers that need the name (PeerImp's banner, ConnectAttempt) on member(). New test case validates isMember agrees with member() across a mixed cluster/non-cluster population. No correctness, security, or consistency issues found in the changed lines.
bthomee
added
the
Ready to merge
The named constants added for the isMember test case use std::size_t, but the file never included the header that provides it. That tripped misc-include-cleaner in CI, which treats the warning as an error.
There was a problem hiding this comment.
Clean, low-risk refactor: adds Cluster::isMember mirroring the existing member locking/lookup pattern, updates the two call sites that only needed a boolean, and adds a test asserting isMember agrees with member. No behavioral or correctness issues found in the changed lines — the new method reuses the same mutex and underlying nodes_ container semantics as member, and the call-site swaps are mechanical (static_cast<bool>(member(...)) → isMember(...)).
High Level Overview of Change
Adds
Cluster::isMember(PublicKey const&) const, a bool-returning membership query, and switches thetwo production call sites that only test cluster membership for engagement -
PeerImp::cluster()andOverlayImpl::onHandoff- fromCluster::memberto it.Context of Change
Cluster::memberreturnsstd::optional<std::string>, copying the node's configured comment out fromunder
mutex_on every call.PeerImp::cluster()has 9 call sites inPeerImp.cpp, several of them onthe per-message path (transaction, validation, and proposal relay), and both it and
OverlayImpl::onHandoffdiscard the string immediately after testing it forbool.isMemberreplaces that withnodes_.contains(identity), no string copy, under the same lock. The twocallers that actually want the name -
PeerImp's connection banner andConnectAttempt's - are left onmember.Covered by a new case in the existing cluster suite (
src/test/overlay/cluster_test.cpp) assertingisMemberagrees withmemberacross a mixed cluster/non-cluster population of public keys.API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)