Enforce no duplicate slots from incoming connections #4944

Bronek · 2024-03-08T20:43:06Z

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

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 · 2024-03-11T22:18:39Z

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 ⚠️

Additional details and impacted files

@@            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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Bronek · 2024-03-12T11:51:36Z

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

scottschurr

👍 LGTM! Thanks for adding the unit test.

src/test/peerfinder/PeerFinder_test.cpp

gregtatcam

LGTM

Bronek · 2024-03-12T19:53:46Z

This is ready to merge (but might want to delay merging after #4947 , so we have working Windows build as well)

scottschurr

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.

Changes look good. 👍

Bronek · 2024-03-12T20:31:12Z

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.

* Price Oracle (XLS-47d): (XRPLF#4789) (XRPLF#4789) Implement native support for Price Oracles. A Price Oracle is used to bring real-world data, such as market prices, onto the blockchain, enabling dApps to access and utilize information that resides outside the blockchain. Add Price Oracle functionality: - OracleSet: create or update the Oracle object - OracleDelete: delete the Oracle object To support this functionality add: - New RPC method, `get_aggregate_price`, to calculate aggregate price for a token pair of the specified oracles - `ltOracle` object The `ltOracle` object maintains: - Oracle Owner's account - Oracle's metadata - Up to ten token pairs with the scaled price - The last update time the token pairs were updated Add Oracle unit-tests * fix compile error on gcc 13: (XRPLF#4932) The compilation fails due to an issue in the initializer list of an optional argument, which holds a vector of pairs. The code compiles correctly on earlier gcc versions, but fails on gcc 13. * Set version to 2.2.0-b1 * Remove default ctors from SecretKey and PublicKey: (XRPLF#4607) * It is now an invariant that all constructed Public Keys are valid, non-empty and contain 33 bytes of data. * Additionally, the memory footprint of the PublicKey class is reduced. The size_ data member is declared as static. * Distinguish and identify the PublisherList retrieved from the local config file, versus the ones obtained from other validators. * Fixes XRPLF#2942 * Fast base58 codec: (XRPLF#4327) This algorithm is about an order of magnitude faster than the existing algorithm (about 10x faster for encoding and about 15x faster for decoding - including the double hash for the checksum). The algorithms use gcc's int128 (fast MS version will have to wait, in the meantime MS falls back to the slow code). * feat: add user version of `feature` RPC (XRPLF#4781) * uses same formatting as admin RPC * hides potentially sensitive data * build: add STCurrency.h to xrpl_core to fix clio build (XRPLF#4939) * Embed patched recipe for RocksDB 6.29.5 (XRPLF#4947) * fix: order book update variable swap: (XRPLF#4890) This is likely the result of a typo when the code was simplified. * Fix workflows (XRPLF#4948) The problem was `CONAN_USERNAME` environment variable, which Conan 1.x uses as the default user in package references. * Upgrade to xxhash 0.8.2 as a Conan requirement, enable SIMD hashing (XRPLF#4893) We are currently using old version 0.6.2 of `xxhash`, as a verbatim copy and paste of its header file `xxhash.h`. Switch to the more recent version 0.8.2. Since this version is in Conan Center (and properly protects its ABI by keeping the state object incomplete), add it as a Conan requirement. Switch to the SIMD instructions (in the new `XXH3` family) supported by the new version. * Update remaining actions (XRPLF#4949) Downgrade {upload,download}-artifact action to v3 because of unreliability with v4. * Install more public headers (XRPLF#4940) Fixes some mistakes in XRPLF#4885 * test: Env unit test RPC errors return a unique result: (XRPLF#4877) * telENV_RPC_FAILED is a new code, reserved exclusively for unit tests when RPC fails. This will make those types of errors distinct and easier to test for when expected and/or diagnose when not. * Output RPC command result when result is not expected. * Fix workflows (XRPLF#4951) - Update container for Doxygen workflow. Matches Linux workflow, with newer GLIBC version required by newer actions. - Fixes macOS workflow to install and configure Conan correctly. Still fails on tests, but that does not seem attributable to the workflow. * perf: improve `account_tx` SQL query: (XRPLF#4955) The witness server makes heavily use of the `account_tx` RPC command. Perf testing showed that the SQL query used by `account_tx` became unacceptably slow when the DB was large and there was a `marker` parameter. The plan for the query showed only indexed reads. This appears to be an issue with the internal SQLite optimizer. This patch rewrote the query to use `UNION` instead of `OR` and significantly improves performance. See RXI-896 and RIPD-1847 for more details. * `fixEmptyDID`: fix amendment to handle empty DID edge case: (XRPLF#4950) This amendment fixes an edge case where an empty DID object can be created. It adds an additional check to ensure that DIDs are non-empty when created, and returns a `tecEMPTY_DID` error if the DID would be empty. * Enforce no duplicate slots from incoming connections: (XRPLF#4944) 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 behavior (when updating slot state), as a result of an apparently matching remote_endpoint in slots_ being used by a different connection. * Remove zaphod.alloy.ee hub from default server list: (XRPLF#4903) Remove the zaphod.alloy.ee hubs from the bootstrap and default configuration after 5 years. It has been an honor to run these servers, but it is now time for another entity to step into this role. The zaphod servers will be taken offline in a phased manner keeping all those who have peering arrangements informed. These would be the preferred attributes of a boostrap set of hubs: 1. Commitment to run the hubs for a minimum of 2 years 2. Highly available 3. Geographically dispersed 4. Secure and up to date 5. Committed to ensure that peering information is kept private * Write improved `forAllApiVersions` used in NetworkOPs (XRPLF#4833) * Don't reach consensus as quickly if no other proposals seen: (XRPLF#4763) This fixes a case where a peer can desync under a certain timing circumstance--if it reaches a certain point in consensus before it receives proposals. This was noticed under high transaction volumes. Namely, when we arrive at the point of deciding whether consensus is reached after minimum establish phase duration but before having received any proposals. This could be caused by finishing the previous round slightly faster and/or having some delay in receiving proposals. Existing behavior arrives at consensus immediately after the minimum establish duration with no proposals. This causes us to desync because we then close a non-validated ledger. The change in this PR causes us to wait for a configured threshold before making the decision to arrive at consensus with no proposals. This allows validators to catch up and for brief delays in receiving proposals to be absorbed. There should be no drawback since, with no proposals coming in, we needn't be in a huge rush to jump ahead. * fixXChainRewardRounding: round reward shares down: (XRPLF#4933) When calculating reward shares, the amount should always be rounded down. If the `fixUniversalNumber` amendment is not active, this works correctly. If it is not active, then the amount is incorrectly rounded up. This patch introduces an amendment so it will be rounded down. * Remove unused files * Remove packaging scripts * Consolidate external libraries * Simplify protobuf generation * Rename .hpp to .h * Format formerly .hpp files * Rewrite includes $ find src/ripple/ src/test/ -type f -exec sed -i 's:include\s*["<]ripple/$.*$\.h$pp$\?[">]:include <ripple/\1.h>:' {} + * Fix source lists * Add markers around source lists * fix: improper handling of large synthetic AMM offers: A large synthetic offer was not handled correctly in the payment engine. This patch fixes that issue and introduces a new invariant check while processing synthetic offers. * Set version to 2.1.1 * chore: change Github Action triggers for build/test jobs (XRPLF#4956) Github Actions for the build/test jobs (nix.yml, mac.yml, windows.yml) will only run on branches that build packages (develop, release, master), and branches with names starting with "ci/". This is intended as a compromise between disabling CI jobs on personal forks entirely, and having the jobs run as a free-for-all. Note that it will not affect PR jobs at all. * Address compiler warnings * Fix search for protoc * chore: Default validator-keys-tool to master branch: (XRPLF#4943) * master is the default branch for that project. There's no point in using develop. * Remove unused lambdas from MultiApiJson_test * fix Conan component reference typo * Set version to 2.2.0-b2 * bump version * 2.2.3 * 2.2.4 * 2.2.5 --------- Co-authored-by: Gregory Tsipenyuk <[email protected]> Co-authored-by: seelabs <[email protected]> Co-authored-by: Chenna Keshava B S <[email protected]> Co-authored-by: Mayukha Vadari <[email protected]> Co-authored-by: John Freeman <[email protected]> Co-authored-by: Bronek Kozicki <[email protected]> Co-authored-by: Ed Hennis <[email protected]> Co-authored-by: Olek <[email protected]> Co-authored-by: Alloy Networks <[email protected]> Co-authored-by: Mark Travis <[email protected]> Co-authored-by: Gregory Tsipenyuk <[email protected]>

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 behavior (when updating slot state), as a result of an apparently matching remote_endpoint in slots_ being used by a different connection.

RIPD-1848 Enforce no duplicate slots from incoming connections

e6c0b03

Bronek force-pushed the bugfix/unique_incoming_connections branch from b33fbb3 to e6c0b03 Compare March 8, 2024 21:02

Bronek marked this pull request as ready for review March 11, 2024 22:18

RIPD-1848 Add unit test

25cdc6c

Bronek force-pushed the bugfix/unique_incoming_connections branch from a0aa87e to 25cdc6c Compare March 11, 2024 22:19

Bronek requested review from seelabs and scottschurr March 11, 2024 22:20

scottschurr approved these changes Mar 12, 2024

View reviewed changes

Bronek requested a review from gregtatcam March 12, 2024 18:15

thejohnfreeman requested changes Mar 12, 2024

View reviewed changes

src/test/peerfinder/PeerFinder_test.cpp Outdated Show resolved Hide resolved

gregtatcam reviewed Mar 12, 2024

View reviewed changes

src/test/peerfinder/PeerFinder_test.cpp Outdated Show resolved Hide resolved

src/test/peerfinder/PeerFinder_test.cpp Outdated Show resolved Hide resolved

Minor cleanup

051408f

Bronek requested review from thejohnfreeman, gregtatcam and scottschurr March 12, 2024 19:17

gregtatcam reviewed Mar 12, 2024

View reviewed changes

src/test/peerfinder/PeerFinder_test.cpp Outdated Show resolved Hide resolved

Bronek requested a review from gregtatcam March 12, 2024 19:18

thejohnfreeman approved these changes Mar 12, 2024

View reviewed changes

gregtatcam approved these changes Mar 12, 2024

View reviewed changes

Remove unnecessary loop from test

5a7f9e4

gregtatcam approved these changes Mar 12, 2024

View reviewed changes

scottschurr approved these changes Mar 12, 2024

View reviewed changes

Bronek added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Mar 13, 2024

Bronek self-assigned this Mar 13, 2024

Bronek added the Perf impact not expected Change is not expected to improve nor harm performance. label Mar 13, 2024

Merge branch 'develop' into bugfix/unique_incoming_connections

bd5a65c

Bronek added 2 commits March 13, 2024 21:16

Merge branch 'develop' into bugfix/unique_incoming_connections

fc12935

Merge branch 'develop' into bugfix/unique_incoming_connections

a795455

Bronek added the Bug label Mar 20, 2024

Bronek and others added 2 commits March 21, 2024 03:33

Merge branch 'develop' into bugfix/unique_incoming_connections

86daed1

Merge branch 'develop' into bugfix/unique_incoming_connections

11f06fa

seelabs merged commit 64e4687 into XRPLF:develop Mar 22, 2024
17 checks passed

Bronek added a commit to Bronek/rippled that referenced this pull request Jul 2, 2024

Revert PR XRPLF#4944

8558c3b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enforce no duplicate slots from incoming connections #4944

Enforce no duplicate slots from incoming connections #4944

Bronek commented Mar 8, 2024 •

edited

Loading

codecov-commenter commented Mar 11, 2024 •

edited

Loading

Bronek commented Mar 12, 2024

scottschurr left a comment

gregtatcam left a comment

Bronek commented Mar 12, 2024

scottschurr left a comment

Bronek commented Mar 12, 2024

Enforce no duplicate slots from incoming connections #4944

Enforce no duplicate slots from incoming connections #4944

Conversation

Bronek commented Mar 8, 2024 • edited Loading

High Level Overview of Change

Context of Change

Type of Change

Future Tasks

codecov-commenter commented Mar 11, 2024 • edited Loading

Codecov Report

Bronek commented Mar 12, 2024

scottschurr left a comment

Choose a reason for hiding this comment

gregtatcam left a comment

Choose a reason for hiding this comment

Bronek commented Mar 12, 2024

scottschurr left a comment

Choose a reason for hiding this comment

Bronek commented Mar 12, 2024

Bronek commented Mar 8, 2024 •

edited

Loading

codecov-commenter commented Mar 11, 2024 •

edited

Loading