Merge branch 'develop' into api-changelog-delivermax

API-CHANGELOG.md

@@ -118,33 +118,20 @@ was released on Mar 14, 2023.
   removed from the [ledger subscription stream](https://xrpl.org/subscribe.html#ledger-stream), because it will no longer
   have any meaning.
 
-# In development
-
-Changes below this point are in development.
-
 ## API Version 2
 
-At the time of writing, this version is expected to be introduced in `rippled` version 2.0.
-
-Currently (prior to the release of 2.0), it is available as a "beta" version, meaning it can be enabled with a config setting in `rippled.cfg`:
-
-```
-[beta_rpc_api]
-1
-```
-
-Since `api_version` 2 is in "beta", breaking changes to V2 can currently be made at any time.
+API version 2 is introduced in `rippled` version 2.0. Users can request it explicitly by specifying `"api_version" : 2`.
 
 #### Removed methods
 
-In API version 2, the following methods are no longer available:
+In API version 2, the following methods are no longer available: (https://github.com/XRPLF/rippled/pull/4759)
 
 - `tx_history` - Instead, use other methods such as `account_tx` or `ledger` with the `transactions` field set to `true`.
 - `ledger_header` - Instead, use the `ledger` method.
 
 #### Modifications to JSON transaction element in V2
 
-In API version 2, JSON elements for transaction output have been changed and made consistent for all methods which output transactions:
+In API version 2, JSON elements for transaction output have been changed and made consistent for all methods which output transactions: (https://github.com/XRPLF/rippled/pull/4775)
 
 - JSON transaction element is named `tx_json`
 - Binary transaction element is named `tx_blob`
@@ -167,38 +154,33 @@ This change affects the following methods:
 - `subscribe` - Renamed transaction element from `transaction` to `tx_json`. Changed location of `hash` and added new elements
 - `sign`, `sign_for`, `submit` and `submit_multisigned` - Changed location of `hash` element.
 
-#### Modifications to account_info response in V2
+#### Modification to `Payment` transaction JSON schema
+
+- In `Payment` transaction type, JSON RPC field `Amount` is renamed to `DeliverMax`. To enable smooth client transition, `Amount` is still handled, as described below: (https://github.com/XRPLF/rippled/pull/4733)
+  - On JSON RPC input (e.g. `submit_multisigned` etc. methods), `Amount` is recognized as an alias to `DeliverMax` for both API version 1 and version 2 clients.
+  - On JSON RPC input, submitting both `Amount` and `DeliverMax` fields is allowed _only_ if they are identical; otherwise such input is rejected with `rpcINVALID_PARAMS` error.
+  - On JSON RPC output (e.g. `subscribe`, `account_tx` etc. methods), `DeliverMax` is present in both API version 1 and version 2.
+  - On JSON RPC output, `Amount` is only present in API version 1 and _not_ in version 2.
+
+#### Modifications to account_info response
 
-- `signer_lists` is returned in the root of the response. Previously, in API version 1, it was nested under `account_data`. (https://github.com/XRPLF/rippled/pull/3770)
+- `signer_lists` is returned in the root of the response. In API version 1, it was nested under `account_data`. (https://github.com/XRPLF/rippled/pull/3770)
 - When using an invalid `signer_lists` value, the API now returns an "invalidParams" error. (https://github.com/XRPLF/rippled/pull/4585)
   - (`signer_lists` must be a boolean. In API version 1, strings are accepted and may return a normal response - as if `signer_lists` were `true`.)
 
-#### Modifications to [account_tx](https://xrpl.org/account_tx.html#account_tx) response in V2
+#### Modifications to [account_tx](https://xrpl.org/account_tx.html#account_tx) response
 
-- Using `ledger_index_min`, `ledger_index_max`, and `ledger_index` returns `invalidParams` because if you use `ledger_index_min` or `ledger_index_max`, then it does not make sense to also specify `ledger_index`. Previously, in API version 1, no error was returned. (https://github.com/XRPLF/rippled/pull/4571)
+- Using `ledger_index_min`, `ledger_index_max`, and `ledger_index` returns `invalidParams` because if you use `ledger_index_min` or `ledger_index_max`, then it does not make sense to also specify `ledger_index`. In API version 1, no error was returned. (https://github.com/XRPLF/rippled/pull/4571)
   - The same applies for `ledger_index_min`, `ledger_index_max`, and `ledger_hash`. (https://github.com/XRPLF/rippled/issues/4545#issuecomment-1565065579)
 - Using a `ledger_index_min` or `ledger_index_max` beyond the range of ledgers that the server has:
   - returns `lgrIdxMalformed` in API version 2. (https://github.com/XRPLF/rippled/issues/4288)
-  - Previously, in API version 1, no error was returned.
-
-##### In progress
-
-- Attempting to use a non-boolean value (such as a string) for the `binary` or `forward` parameters returns `invalidParams` (`rpcINVALID_PARAMS`). Previously, in API version 1, no error was returned. (https://github.com/XRPLF/rippled/pull/4620)
-
-#### Modifications to [noripple_check](https://xrpl.org/noripple_check.html#noripple_check) response in V2
-
-##### In progress
+  - In API version 1, no error was returned.
 
-- Attempting to use a non-boolean value (such as a string) for the `transactions` parameter returns `invalidParams` (`rpcINVALID_PARAMS`). Previously, in API version 1, no error was returned. (https://github.com/XRPLF/rippled/pull/4620)
+- Attempting to use a non-boolean value (such as a string) for the `binary` or `forward` parameters returns `invalidParams` (`rpcINVALID_PARAMS`). In API version 1, no error was returned. (<https://github.com/XRPLF/rippled/pull/4620>)
 
-##### In progress
-
-- In `Payment` transaction type, JSON RPC field `Amount` is renamed to `DeliverMax`. To enable smooth client transition, `Amount` is still handled, as described below:
-  - On JSON RPC input (e.g. `submit_multisigned` etc. methods), `Amount` is recognized as an alias to `DeliverMax` for both API version 1 and version 2 clients.
-  - On JSON RPC input, submitting both `Amount` and `DeliverMax` fields is allowed _only_ if they are identical; otherwise such input is rejected with `rpcINVALID_PARAMS` error.
-  - On JSON RPC output (e.g. `subscribe`, `account_tx` etc. methods), `DeliverMax` is present in both API version 1 and version 2.
-  - On JSON RPC output, `Amount` is only present in API version 1 and _not_ in version 2.
+#### Modifications to [noripple_check](https://xrpl.org/noripple_check.html#noripple_check) response
 
+- Attempting to use a non-boolean value (such as a string) for the `transactions` parameter returns `invalidParams` (`rpcINVALID_PARAMS`). In API version 1, no error was returned. (<https://github.com/XRPLF/rippled/pull/4620>)
 
 # Unit tests for API changes
 

Builds/containers/gitlab-ci/push_to_artifactory.sh

@@ -20,7 +20,7 @@ RIPPLED_REPORTING_DBG_PKG=$(ls rippled-reporting-dbgsym_*.*deb)
 # TODO - where to upload src tgz?
 RIPPLED_SRC=$(ls rippled_*.orig.tar.gz)
 DEB_MATRIX=";deb.component=${COMPONENT};deb.architecture=amd64"
-for dist in buster bullseye bionic focal jammy; do
+for dist in bookworm buster bullseye bionic focal jammy; do
     DEB_MATRIX="${DEB_MATRIX};deb.distribution=${dist}"
 done
 echo "{ \"debs\": {" > "${TOPDIR}/files.info"

Builds/containers/gitlab-ci/smoketest.sh

@@ -62,9 +62,10 @@ else
     yum -y update
     if [ "${install_from}" = "repo" ] ; then
         pkgs=("yum-utils coreutils util-linux")
-        if [ "$ID" = "rocky" ]; then
-            pkgs="${pkgs[@]/coreutils}"
-        fi
+        case "$ID" in
+            rocky|almalinux)
+                pkgs="${pkgs[@]/coreutils}"
+        esac
         yum install -y $pkgs
         REPOFILE="/etc/yum.repos.d/artifactory.repo"
         echo "[Artifactory]" > ${REPOFILE}

src/ripple/consensus/Consensus.h

@@ -1759,15 +1759,27 @@ Consensus<Adaptor>::updateOurPositions(bool const share)
     else
     {
         int neededWeight;
+        // It's possible to be at a close time impasse (described below), so
+        // keep track of whether this round has taken a long time.
+        bool stuck = false;
 
         if (convergePercent_ < parms.avMID_CONSENSUS_TIME)
+        {
             neededWeight = parms.avINIT_CONSENSUS_PCT;
+        }
         else if (convergePercent_ < parms.avLATE_CONSENSUS_TIME)
+        {
             neededWeight = parms.avMID_CONSENSUS_PCT;
+        }
         else if (convergePercent_ < parms.avSTUCK_CONSENSUS_TIME)
+        {
             neededWeight = parms.avLATE_CONSENSUS_PCT;
+        }
         else
+        {
             neededWeight = parms.avSTUCK_CONSENSUS_PCT;
+            stuck = true;
+        }
 
         int participants = currPeerPositions_.size();
         if (mode_.get() == ConsensusMode::proposing)
@@ -1787,57 +1799,156 @@ Consensus<Adaptor>::updateOurPositions(bool const share)
                         << " nw:" << neededWeight << " thrV:" << threshVote
                         << " thrC:" << threshConsensus;
 
-        // An impasse is possible unless a validator pretends to change
-        // its close time vote. Imagine 5 validators. 3 have positions
-        // for close time t1, and 2 with t2. That's an impasse because
-        // 75% will never be met. However, if one of the validators voting
-        // for t2 switches to t1, then that will be 80% and sufficient
-        // to break the impasse. It's also OK for those agreeing
-        // with the 3 to pretend to vote for the one with 2, because
-        // that will never exceed the threshold of 75%, even with as
-        // few as 3 validators. The most it can achieve is 2/3.
-        for (auto& [t, v] : closeTimeVotes)
+        // Choose a close time and decide whether there is consensus for it.
+        //
+        // Close time is chosen based on the threshVote threshold
+        // calculated above. If a close time has votes equal to or greater than
+        // that threshold, then that is the best close time. If multiple
+        // close times have an equal number of votes, then choose the greatest
+        // of them. Ensure that our close time then matches that which meets
+        // the criteria. But if no close time meet the criteria, make no
+        // changes.
+        //
+        // This is implemented slightly differently for validators vs
+        // non-validators. For non-validators, it is sufficient to merely
+        // count the close times from all peer positions to determine
+        // the best. Validators, however, risk putting the network into an
+        // impasse unless they are able to change their own position without
+        // first having counted it towards the close time totals.
+        //
+        // Here's how the impasse could occur:
+        // Imagine 5 validators. 3 have close time t1, and 2 have t2.
+        // As consensus time increases, the threshVote threshold also increases.
+        // Once threshVote exceeds 60%, no members of either set of validators
+        // will change their close times.
+        //
+        // Avoiding the impasse means that validators should identify
+        // whether they currently have the best close time. First, choose
+        // the close time with the most votes. However, if multiple close times
+        // have the same number of votes, pick the latest of them.
+        // If the validator does not currently have the best close time,
+        // switch to it and increase the local vote tally for that better
+        // close time. This will result in consensus in the next iteration
+        // assuming that the peer messages propagate successfully.
+        // In this case the validators in set t1 will remain the same but
+        // those in t2 switch to t1.
+        //
+        // Another wrinkle, however, is that too many position changes
+        // from validators also has a destabilizing affect. Consensus can
+        // take longer if peers have to keep re-calculating positions,
+        // and mistakes can be made if peers settle on the wrong position.
+        // Therefore, avoiding an impasse should also minimize the likelihood
+        // of gratuitous change of position.
+        //
+        // The solution for validators is to first track whether it's
+        // possible that the network is at an impasse based on how much
+        // time this current consensus round has taken. This is reflected
+        // in the "stuck" boolean. When stuck, validators perform the
+        // above-described position change based solely on whether or not
+        // they agree with the best position, and ignore the threshVote
+        // criteria used for the earlier part of the phase.
+        //
+        // Determining whether there is close time consensus is very simple
+        // in comparison: if votes for the best close time meet or exceed
+        // threshConsensus, then we have close time consensus. Otherwise, not.
+
+        // First, find the best close time with which to agree: first criteria
+        // is the close time with the most votes. If a tie, the latest
+        // close time of those tied for the maximum number of votes.
+        std::multimap<int, NetClock::time_point> votesByCloseTime;
+        std::stringstream ss;
+        ss << "Close time calculation for ledger sequence "
+           << static_cast<std::uint32_t>(previousLedger_.seq()) + 1
+           << " Close times and vote count are as follows: ";
+        bool first = true;
+        for (auto const& [closeTime, voteCount] : closeTimeVotes)
         {
-            if (adaptor_.validating() &&
-                t != asCloseTime(result_->position.closeTime()))
-            {
-                JLOG(j_.debug()) << "Others have voted for a close time "
-                                    "different than ours. Adding our vote "
-                                    "to this one in case it is necessary "
-                                    "to break an impasse.";
-                ++v;
-            }
-            JLOG(j_.debug())
-                << "CCTime: seq "
-                << static_cast<std::uint32_t>(previousLedger_.seq()) + 1 << ": "
-                << t.time_since_epoch().count() << " has " << v << ", "
-                << threshVote << " required";
-
-            if (v >= threshVote)
+            if (first)
+                first = false;
+            else
+                ss << ',';
+            votesByCloseTime.insert({voteCount, closeTime});
+            ss << closeTime.time_since_epoch().count() << ':' << voteCount;
+        }
+        // These always gets populated because currPeerPositions_ is not
+        // empty to end up here, so at least 1 close time has at least 1 vote.
+        assert(!currPeerPositions_.empty());
+        std::optional<int> maxVote;
+        std::set<NetClock::time_point> maxCloseTimes;
+        // Highest vote getter is last. Track each close time that is tied
+        // with the highest.
+        for (auto rit = votesByCloseTime.crbegin();
+             rit != votesByCloseTime.crend();
+             ++rit)
+        {
+            int const voteCount = rit->first;
+            if (!maxVote.has_value())
+                maxVote = voteCount;
+            else if (voteCount < *maxVote)
+                break;
+            maxCloseTimes.insert(rit->second);
+        }
+        // The best close time is the latest close time of those that have
+        // the maximum number of votes.
+        NetClock::time_point const bestCloseTime = *maxCloseTimes.crbegin();
+        ss << ". The best close time has the most votes. If there is a tie, "
+              "choose the latest. This is "
+           << bestCloseTime.time_since_epoch().count() << "with " << *maxVote
+           << " votes. ";
+
+        // If we are a validator potentially at an impasse and our own close
+        // time is not the best, change our close time to match it and
+        // tally another vote for it.
+        if (adaptor_.validating() && stuck &&
+            consensusCloseTime != bestCloseTime)
+        {
+            consensusCloseTime = bestCloseTime;
+            ++*maxVote;
+            ss << " We are a validator. Consensus has taken "
+               << result_->roundTime.read().count()
+               << "ms. Previous round "
+                  "took "
+               << prevRoundTime_.count()
+               << "ms. Now changing our "
+                  "close time to "
+               << bestCloseTime.time_since_epoch().count()
+               << " that "
+                  "now has "
+               << *maxVote << " votes.";
+        }
+        // If the close time with the most votes also meets or exceeds the
+        // threshold to change our position, then change our position.
+        // Then check if we have met or exceeded the threshold for close
+        // time consensus.
+        //
+        // The 2nd check has been nested within the first historically.
+        // It's possible that this can be optimized by doing the
+        // 2nd check independently--this may make consensus happen faster in
+        // some cases. Then again, the trade-offs have not been modelled.
+        if (*maxVote >= threshVote)
+        {
+            consensusCloseTime = bestCloseTime;
+            ss << "Close time " << bestCloseTime.time_since_epoch().count()
+               << " has " << *maxVote << " votes, which is >= the threshold ("
+               << threshVote
+               << " to make that our position if it isn't already.";
+            if (*maxVote >= threshConsensus)
             {
-                // A close time has enough votes for us to try to agree
-                consensusCloseTime = t;
-                threshVote = v;
-
-                if (threshVote >= threshConsensus)
-                {
-                    haveCloseTimeConsensus_ = true;
-                    // Make sure that the winning close time is the one
-                    // that propagates to the rest of the function.
-                    break;
-                }
+                haveCloseTimeConsensus_ = true;
+                ss << " The maximum votes also >= the threshold ("
+                   << threshConsensus << ") for consensus.";
             }
         }
 
         if (!haveCloseTimeConsensus_)
         {
-            JLOG(j_.debug())
-                << "No CT consensus:"
-                << " Proposers:" << currPeerPositions_.size()
-                << " Mode:" << to_string(mode_.get())
-                << " Thresh:" << threshConsensus
-                << " Pos:" << consensusCloseTime.time_since_epoch().count();
+            ss << " No CT consensus:"
+               << " Proposers:" << currPeerPositions_.size()
+               << " Mode:" << to_string(mode_.get())
+               << " Thresh:" << threshConsensus
+               << " Pos:" << consensusCloseTime.time_since_epoch().count();
         }
+        JLOG(j_.debug()) << ss.str();
     }
 
     if (!ourNewSet &&

src/ripple/protocol/impl/BuildInfo.cpp

@@ -33,7 +33,7 @@ namespace BuildInfo {
 //  and follow the format described at http://semver.org/
 //------------------------------------------------------------------------------
 // clang-format off
-char const* const versionString = "2.0.0-b4"
+char const* const versionString = "2.0.0-rc2"
 // clang-format on
 
 #if defined(DEBUG) || defined(SANITIZER)