Fix regression in ConnectAttempt

[Bronek]

High Level Overview of Change

Fix a regression bug from #5669

Context of Change

A regression was introduced in #5669 which would cause rippled to potentially dereference a disengaged std::optional when connecting to a peer. This would cause UB in release build and crash in debug.

The bug does not affect version 3.0.0 since #5669 was not planned for release in this version and got explicitly reverted.

Type of Change

• Bug fix (non-breaking change which fixes an issue)

[Tapanito]

Great catch! Thanks Bronek.

[vvysokikh1]

@Tapanito the semantics of optional and string seem to be really messed up here.
I'm a bit concerned that the change here fixes the crash but changes the meaning of the check (checking if string is empty vs checking if optional has value). Should both these conditions be checked here? Should it be purely std::string (without optional)?

[Tapanito]

@vvysokikh1 the previous code simply did a cast to bool, auto const result = overlay_.peerFinder().activate(slot_, publicKey, static_cast<bool>(member));, which I understand is checking if the member (std::optional) has a value.

[codecov]

Codecov Report

❌ Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.5%. Comparing base (a422855) to head (e9c9862).
⚠️ Report is 2 commits behind head on develop.

Files with missing lines	Patch %	Lines
src/xrpld/overlay/detail/ConnectAttempt.cpp	0.0%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##           develop   #5900     +/-   ##
=========================================
- Coverage     79.5%   79.5%   -0.0%     
=========================================
  Files          816     816             
  Lines        72179   72179             
  Branches      8280    8276      -4     
=========================================
- Hits         57367   57354     -13     
- Misses       14812   14825     +13     

Files with missing lines	Coverage Δ
src/xrpld/overlay/detail/ConnectAttempt.cpp	0.0% <0.0%> (ø)

... and 5 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Bronek]

@vvysokikh1 the previous code simply did a cast to bool, auto const result = overlay_.peerFinder().activate(slot_, publicKey, static_cast<bool>(member));, which I understand is checking if the member (std::optional) has a value.

The regression that's being fixed here is that #5669 did change the semantics, from checking an equivalent of member.has_value() (indirectly, via cast to bool) to empty() on a string, unconditionally dereferenced. Here we are bringing the old semantics back.

Here's the old code :

rippled/src/xrpld/overlay/detail/ConnectAttempt.cpp

Line 409 in ccb9f1e

slot_, publicKey, static_cast<bool>(member));

[vvysokikh1]

@vvysokikh1 the previous code simply did a cast to bool, auto const result = overlay_.peerFinder().activate(slot_, publicKey, static_cast<bool>(member));, which I understand is checking if the member (std::optional) has a value.

yes, and you changed that to check if the string is not empty. My question here - was that an intentional change or a mistake. If that was intentional, this fix is not sufficient. If by mistake - i can approve then

[Bronek]

@vvysokikh1 the previous code simply did a cast to bool, auto const result = overlay_.peerFinder().activate(slot_, publicKey, static_cast<bool>(member));, which I understand is checking if the member (std::optional) has a value.

yes, and you changed that to check if the string is not empty. My question here - was that an intentional change or a mistake. If that was intentional, this fix is not sufficient. If by mistake - i can approve then

FWIW I find nothing in #5669 to indicate that was intentional. There's no change in PeerfinderManager.cpp or in Cluster.cpp that would indicate any change of semantics to the return value of app_.cluster().member(publicKey); or its use in overlay_.peerFinder().activate(...) or explanation how that might be possibly related to SSL socket shutdown.

[Bronek]

@Tapanito the embarrassing part is that, when merging #5669 I did look at that part and did not read it carefully enough 🥹