Implement threshold for the intersection of lists of validators

[Bronek]

High Level Overview of Change

Improve UNL security by allowing validators to set a minimum number of UNL publishers to agree on validators

Context of Change

In the current system, every node builds an internal list of validators to use, by performing union of UNLs received from all configured UNL publishers (validator lists), where the authenticity of each single UNL is verified by a manifest with a known master key.

This union is a weak security spot of the UNL publication system, as it allows a single UNL publisher to add an arbitrary number of validators, which will be subsequently used by all nodes trusting this UNL publisher. Currently the risk is managed by recommending only the use of two UNL publishers: vl.xrplf.org and vl.ripple.com . With this union approach, a hypothetical addition of a third (or more) UNL publisher would only increase the attack surface, in this way (counterintuitively) degrading the UNL system security.

This PR is replacing union with a configurable intersection threshold between UNL publishers, which by default is calculated as:

threshold =  size(validator_list_keys) < 3
  ? 1
  : floor(size(validator_list_keys) / 2) + 1

This means that for a default 2 UNL publishers, the default threshold will be 1. Since "intersection of 1 sets" is not an intersection (it's an union) this makes the default behaviour (for one or two UNL publishers) identical to the current union system.

If a node configuration contains a third UNL publisher, the default threshold will be calculated as floor(3 / 2) + 1, that is 1 + 1 that is 2. This means that, for 3 validator lists, the node will only use validators which are present on 2 or more lists (and will silently ignore validators which are on one list only).

If the configured validator list contains 4 or 5 publishers, the default threshold will be 3. If the configured validator list contains 6 or 7 publishers, the default threshold will be 4 etc.

This PR adds an optional configuration option [validator_list_threshold] to validators.txt which can be explicitly set to the minimum number of the lists on which a validator must be listed in order to be used (this number must not be greater than the size of [validator_list_keys]). If it is not set, or set to 0, the value will be calculated at startup from the size of [validator_list_keys] as explained above.

The actually used threshold value will be also displayed in the output of validators RPC method (alongside with the used list of validators, and the content of each configured validator list, as is the current output).

This change is not altering the list of the default UNL publishers. The intent of this PR is to make it safe to extend this list in the future.

---

The security problems of the union UNL publication system were originally described by @mDuo13 and the idea to use intersection of validator lists as a solution came from @ximinez

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

API Impact

Add new value validator_list_threshold to the output from validators RPC method

Add new (optional) section [validator_list_threshold] to validators file.

• Public API: New feature (new methods and/or new fields)
• Public API: Breaking change (in general, breaking changes should only impact the next api_version)
• libxrpl change (any change that may affect libxrpl or dependents of libxrpl)
• Peer protocol change (must be backward compatible or bump the peer protocol version)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.9%. Comparing base (0ec17b6) to head (1dc99fa).

Additional details and impacted files

@@            Coverage Diff            @@
##           develop   #5112     +/-   ##
=========================================
- Coverage     77.9%   77.9%   -0.0%     
=========================================
  Files          782     782             
  Lines        66622   66676     +54     
  Branches      8136    8157     +21     
=========================================
+ Hits         51902   51928     +26     
- Misses       14720   14748     +28     

Files with missing lines	Coverage Δ
src/xrpld/app/main/Application.cpp	68.9% <100.0%> (+<0.1%)	⬆️
src/xrpld/app/misc/ValidatorList.h	100.0% <ø> (ø)
src/xrpld/app/misc/detail/ValidatorList.cpp	86.9% <100.0%> (+0.5%)	⬆️
src/xrpld/core/Config.h	85.7% <ø> (ø)
src/xrpld/core/ConfigSections.h	100.0% <ø> (ø)
src/xrpld/core/detail/Config.cpp	74.7% <100.0%> (+1.2%)	⬆️

... and 6 files with indirect coverage changes

---- 🚨 Try these New Features:

• Flaky Tests Detection - Detect and resolve failed and flaky tests
• JS Bundle Analysis - Avoid shipping oversized bundles

[bachase]

Mostly style/nits so far. Will do another pass after I better understand how the unavailable lists is being considered.

[bachase]

Any reason to not explicitly require callers to specify, even if its an optional?

[Bronek]

Good question, and admittedly I am not 100% certain that this is the right way, so I will present both arguments for and against making a change here (to remove the default)

1. for - in rippled proper (as opposed to unit tests), we call this function in one place only ApplicationImp::setup and it is important that, in this one location, we definitely do provide this parameter, as parsed from the configuration.

2. against - we do recommend using default in configuration, for a good reason. It makes sense to follow this advice, in the most concise way possible (that is, not providing the parameter at all) also in unit tests etc. where we call this function outside of rippled proper

3. against - making it a required parameter would cause relatively large churn in unit tests

[feature/validator_list_threshold] % git diff --stat
 src/test/app/Manifest_test.cpp          |  2 +-
 src/test/app/ValidatorList_test.cpp     | 66 +++++++++++++++++++++++++++++++++++++-----------------------------
 src/test/app/ValidatorSite_test.cpp     |  2 +-
 src/test/consensus/NegativeUNL_test.cpp |  2 +-
 src/xrpld/app/misc/ValidatorList.h      |  2 +-
 5 files changed, 41 insertions(+), 33 deletions(-)

Again, I am not 100% convinced that points 2. and 3. do outweigh point 1. , and (since it's not much work) if you think that 1. (or some other argument) dictates to remove the default value, I'd be happy to do it.

[bachase]

I don't have a super strong point of view. Trust your judgement (or defer to any other reviewers)

[mov-bx-0xb800]

Good catch, however:

The use of Union of X sets is still valid, however as stated by OP, an attack surface opens up with a malicious publisher. There may be cases where an operator would want to use the Union of X sets instead of the intersect. I recommend allowing the use of both Union and Intersect, where Intersect is the default as to the clear benefits of it.

---

The intent of this PR is to make it safe to extend this list in the future.

Agreed, this would welcome an influx of (reputable) UNL publishers to greater network relevance. I must argue that standards are still to be applied with the addition of new(er) publishers to the default lists (validators-example.txt). Maintainers and trustees of rippled should not add any publishers or author to this list unless:

• a proven track record on the network is present and backed.
• author is recognized, present, relevant and of sound mind within the "community" of network users and network operators.

This so called standard should not be ambiguous or overreaching in its limited scope. The consequence of such will result in extreme ends.

[Bronek]

Good catch, however:

The use of Union of X sets is still valid, however as stated by OP, an attack surface opens up with a malicious publisher. There may be cases where an operator would want to use the Union of X sets instead of the intersect. I recommend allowing the use of both Union and Intersect, where Intersect is the default as to the clear benefits of it.

The use of union is definitely allowed and this PR won't change it, it's just not the default. The user only needs to configure the "intersection of 1", which is functionally equivalent to union.

[validator_list_threshold]
1

The intent of this PR is to make it safe to extend this list in the future.

Agreed, this would welcome an influx of (reputable) UNL publishers to greater network relevance. I must argue that standards are still to be applied with the addition of new(er) publishers to the default lists (validators-example.txt). Maintainers and trustees of rippled should not add any publishers or author to this list unless:

• a proven track record on the network is present and backed.
• author is recognized, present, relevant and of sound mind within the "community" of network users and network operators.

This so called standard should not be ambiguous or overreaching in its limited scope. The consequence of such will result in extreme ends.

Agree, but this is outside of the scope of this PR.

[bachase]

👍 Great job with the unit tests and the careful thought into how to incrementally improve UNL management.