Enforce no duplicate slots from incoming connections

Bronek commented 6 months ago

High Level Overview of Change

We do not currently enforce that incoming peer connection does not have remote_endpoint which is already used (either by incoming or outgoing connection), hence already stored in slots_. If we happen to receive a connection from such a duplicate remote_endpoint, it will eventually result in a crash (when disconnecting) or weird behaviour (when updating slot state), as a result of an apparently matching remote_endpoint in slots_ being used by a different connection

Context of Change

This came from troubleshooting a crash of our node in livenet, as reported in RIPD-1848

Type of Change

[x] Bug fix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
[ ] Refactor (non-breaking change that only restructures code)
[ ] Performance (increase or change in throughput and/or latency)
[ ] Tests (you added tests for code that already exists, or your new feature included in this PR)
[ ] Documentation update
[ ] Chore (no impact to binary, e.g. .gitignore, formatting, dropping support for older tooling)
[ ] Release

Future Tasks

A different approach is also possible, where we do allow duplicate remote_endpoint and change the type of slots_ accordingly. This would be a more intrusive change

codecov-commenter commented 6 months ago

Codecov Report

Attention: Patch coverage is 85.96491% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 76.96%. Comparing base (ea9b1e3) to head (11f06fa).

Files	Patch %	Lines
src/test/peerfinder/PeerFinder_test.cpp	84.61%	2 Missing and 6 partials :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## develop #4944 +/- ## ========================================= Coverage 76.95% 76.96% ========================================= Files 1127 1127 Lines 131714 131771 +57 Branches 39530 39663 +133 ========================================= + Hits 101365 101420 +55 - Misses 24394 24468 +74 + Partials 5955 5883 -72 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Bronek commented 6 months ago

If the fix is reverted, then the newly added test will result in the same crash as reported (in the Release build)

% ./rippled -u PeerFinder
ripple.PeerFinder.PeerFinder backoff 1
Logic error: PeerFinder::Logic::remove(): remote_endpoint missing from slots_
[1]    950283 segmentation fault  ./rippled -u PeerFinder

Bronek commented 6 months ago

This is ready to merge (but might want to delay merging after https://github.com/XRPLF/rippled/pull/4947 , so we have working Windows build as well)

Bronek commented 6 months ago

The test_duplicateOutIn()test without the loop still causes the LogicError if the fix is removed. The test_duplicateInOut() without the loop does not fire the LogicError, but it also did not fire the LogicError with the loop either.

Yes, that's correct - new_outbound_slot already did have the required check, so this test also increases test coverage outside of its immediate focus.

XRPLF / rippled