Support UNLs with future effective dates: (UNL v2) #3619

ximinez · 2020-10-05T21:37:53Z

High Level Overview of Change

Creates a version 2 of the UNL file format allowing publishers to
pre-publish the next UNL while the current one is still valid.
Version 1 of the UNL file format is still valid and backward
compatible.
Also causes rippled to lock down if it has no valid UNLs, similar to
being amendment blocked, except reversible.
Resolves Allow prepublishing of a UNL with a future activation date #3548
Resolves Validator List Expiration Improvements #3470

Context of Change

Type of Change

New feature (non-breaking change which adds functionality)

Test Plan

Set up an internal site similar to https://vl.ripple.com. Publish a version 2 UNL file on that site, as described in the spec. (Replace blob and signature with a blobs_v2 array containing one or more blob/signature pairs.) Include one or more VLs in the array containing future effective dates (using Ripple-epoch time value). Verify that nodes switch to the new VL at the appropriate time.

Future Tasks

Some relatively minor deviations and additions were made from the spec in the development process. I'll update the spec ASAP.

This change is

nbougalis

Partial review, some quick comments. Will follow-up.

nbougalis · 2020-10-08T04:52:40Z

src/ripple/app/misc/NetworkOPs.cpp

+NetworkOPsImp::setExpiredValidatorList()
+{
+    expiredValidatorList_ = true;
+    setMode(OperatingMode::TRACKING);


I don't think TRACKING is the right mode here. If we're tracking we think we agree with the network but if we have no validator list, then we can't possibly be convinced of that fact.

I think CONNECTED is probably better.

Yes, that makes sense. I was paralleling amendment blocked, but this is a good place for a difference.

nbougalis · 2020-10-08T04:55:16Z

src/ripple/app/misc/ValidatorList.h

+void
+hash_append(Hasher& h, std::map<std::size_t, ValidatorBlobInfo> const& blobs)
+{
+    for (auto const& [_, item] : blobs)


Maybe use std::ignore here?

std::ignore doesn't work with structured bindings. (I'd love if we got a standard "ignore" variable like _ in other languages).

Oh right, it's only for std::tie 🤦‍♂️

Yeah, I definitely wish there was a more elegant way to do this...

nbougalis · 2020-10-08T05:54:23Z

src/ripple/app/main/Application.cpp

@@ -2143,6 +2143,12 @@ ApplicationImp::serverOkay(std::string& reason)
        return false;
    }

+    if (getOPs().isExpiredValidatorList())


Gramatically, isValidatorListExpired is better. But before you change anything, either name raises questions: if I have multiple lists, one of which is expired will this trigger? The name of the function suggests no. The reason we set suggests yes.

Hm.

hasExpiredValidatorList

isAnyValidatorListExpired

needUpdatedUNL

isConsensusBlocked

Any preferences or suggestions?

Until we decide on a better name, I'm going to use isUNLBlocked.

nbougalis · 2020-10-08T05:56:09Z

src/ripple/app/misc/ValidatorList.h

 #include <ripple/protocol/PublicKey.h>
 #include <boost/iterator/counting_iterator.hpp>
 #include <boost/range/adaptors.hpp>
 #include <mutex>
 #include <numeric>
 #include <shared_mutex>

+namespace protocol {
+// predeclaration
+class TMValidatorList;


My spidey sense is tingling...

I only did this so I could reference these classes in some function declarations below without needing to #include the entire protocol/messages.h. If there's a better way, I'm all ears.

nbougalis · 2020-10-08T05:58:46Z

src/ripple/app/misc/ValidatorList.h

        std::size_t sequence;
        TimeKeeper::time_point expiration;
+        TimeKeeper::time_point effective;


effective is not very helpful. I think we should rename to something like:

TimeKeeper::time_point validFrom; TimeKeeper::time_point validUntil;

validFrom and validUntil are used internally for the logic within rippled (in the C++ code). Validator list creators/maintainers use effective and expiration.

ximinez · 2020-10-15T15:53:42Z

Rebased on to 1.7.0-b3

src/ripple/app/misc/impl/ValidatorList.cpp

undertome · 2020-10-19T17:02:06Z

The node crashes for me when I provide validators under the validators config section.

src/ripple/app/misc/impl/ValidatorList.cpp

src/ripple/app/misc/NetworkOPs.cpp

src/ripple/app/misc/ValidatorList.h

src/test/jtx/TrustedPublisherServer.h

src/test/net/DatabaseDownloader_test.cpp

src/test/rpc/ShardArchiveHandler_test.cpp

nbougalis

👍 pending resolutions of a couple of things below and @seelabs's comments.

nbougalis · 2020-10-30T06:02:02Z

src/ripple/app/misc/ValidatorList.h

+    static constexpr std::uint32_t supportedListVersions[]{1, 2};
+    // In the initial release, to prevent potential abuse and attacks, any VL
+    // collection with more than 5 entries will be considered malformed.
+    static constexpr std::size_t MAX_SUPPORTED_BLOBS = 5;


I don't know that we need to support that many lists at once, to be honest. I think 2 would be fine, but it's at your discretion.

I figure 2 is probably going to be what we're most likely to see, but I wanted to leave a little wiggle room in case someone comes up with some interesting use cases we haven't considered.

nbougalis · 2020-11-15T11:23:09Z

src/ripple/app/misc/NetworkOPs.cpp

@@ -1561,6 +1579,13 @@ NetworkOPsImp::setAmendmentBlocked()
    setMode(OperatingMode::TRACKING);


While we're here, this should probably change too: TRACKING means "convinced we agree with the network" per the documentation. If we are amendment blocked we might transiently agree with the network, but the fact that we are actually UNABLE to process transactions the network is able to generate suggests that this should be CONNECTED instead.

Good point. Done.

nbougalis · 2020-11-15T11:23:42Z

src/ripple/app/misc/NetworkOPs.cpp

-    if ((om > OperatingMode::TRACKING) && amendmentBlocked_)
+    if ((om > OperatingMode::CONNECTED) && isUNLBlocked())
+        om = OperatingMode::CONNECTED;
+    else if ((om > OperatingMode::TRACKING) && isAmendmentBlocked())
        om = OperatingMode::TRACKING;


Same here... CONNECTED seems more appropriate.

* Discovered some bugs and inconsistencies doing multi-node testing.

* Fixes broken Release build unit tests

* Put and keep node in CONNECTED state instead of TRACKING if a VL expires. * Rename effective and expiration to validFrom and validTo respectively. * Renamed the "ExpiredValidatorList" set of functions to "UNLBlocked", though the name may change again later.

* Remove unnecessary `local` variable processing a local validator list. * Simplify VL protocol message size checking. * Simplify and consolidate VL version number handling. * Log requests for invalid VL version number.

* Fix a couple of asserts.

* Rename `isFunctionallyBlocked` to `isBlocked` * Moved a bunch of functions out of their class definition for readability * Removed some unhelpful comments * Added some helpful comments * Changed the unique_lock in ValidatorList to a lock_guard since it's never unlocked * Cleaned up some naming and formatting * Split `buildFileData` into two overloads for readability

ximinez · 2020-12-16T16:47:32Z

I forgot to mention this yesterday, but that last push included both rebasing onto 1.7.0-b8 and the latest round of feedback from @seelabs.

* Amendment blocked servers will never get past the "connected" state. * This necessitated reordering a few checks to return the appropriate error codes.

seelabs

Thanks for resolving those comments. LGTM 👍

* We could have gotten the VL from another source, so a peer sending it to us might have a lower "known sequence" value, or none (0).

intelliot · 2023-07-31T18:33:31Z

Merged on Jan 8, 2021 via 4b9d3ca

ckniffen · 2023-07-31T18:51:52Z

While effective date is documented there is no notes on XRPL.org that you need to specify version as 2.

ximinez · 2023-07-31T21:46:02Z

While effective date is documented there is no notes on XRPL.org that you need to specify version as 2.

Strictly speaking, I think a version 1 blob with an effective date would work fine. It would not be best practice because the current VL blob wouldn't be included with it.

ximinez requested review from seelabs and natenichols October 5, 2020 21:37

ximinez assigned seelabs Oct 5, 2020

ximinez assigned natenichols Oct 5, 2020

ximinez requested review from undertome and removed request for natenichols October 6, 2020 15:45

ximinez assigned undertome and unassigned natenichols Oct 6, 2020

nbougalis reviewed Oct 13, 2020

View reviewed changes

ximinez force-pushed the FutureUNL branch from f504e49 to 6ebf80b Compare October 15, 2020 15:47

undertome reviewed Oct 19, 2020

View reviewed changes