Prefer ledger by branch #2300

bachase · 2017-12-15T21:26:42Z

These changes leverage the ancestry information of validated ledgers to better select a preferred working ledger for consensus. Additionally, the Validations class now tracks both full and partial validations. Partial validations are only used to determine the working ledger; full validations are required for any quorum related function. Validators are also now explicitly restricted to sending full validations with increasing ledger index, otherwise the validation is marked partial.

The algorithm for determining the preferred ledger is described in the LedgerTrie::getPreferred documentation. The LedgerTrie documentation describes the trie data structure used to support this calculation.

9949483 is a motivating consensus simulation
c6060d8 introduces the LedgerTrie data structure
The subsequent commits are meant to be manageable sized commits that transition to use the ledger trie for the preferred branch calculations.

febedc4 is from #2292

bachase · 2017-12-15T21:28:17Z

@HowardHinnant and @seelabs, could you focus on the LedgerTrie data structure commit in c6060d8? It should be standalone, although the use case comes in the later commits.

bachase · 2017-12-15T21:29:11Z

@JoelKatz and @wilsonianb , could you focus on the remaining commits that change how Validations stores validations and uses the ledger ancestry?

seelabs · 2017-12-15T21:46:55Z

I get some warnings:

src/ripple/beast/utility/Journal.h:272:5: warning: definition of implicit copy assignment operator for 'Journal' is deprecated because it has a user-declared destructor [-Wdeprecated]
src/ripple/beast/utility/Journal.h:272:5: warning: definition of implicit copy assignment operator for 'Journal' is deprecated because it has a user-declared destructor [-Wdeprecated]
src/ripple/beast/utility/Journal.h:272:5: warning: definition of implicit copy assignment operator for 'Journal' is deprecated because it has a user-declared destructor [-Wdeprecated]
src/test/csf/PeerGroup.h:50:5: warning: definition of implicit copy assignment operator for 'PeerGroup' is deprecated because it has a user-declared copy constructor [-Wdeprecated]
src/test/csf/PeerGroup.h:50:5: warning: definition of implicit copy assignment operator for 'PeerGroup' is deprecated because it has a user-declared copy constructor [-Wdeprecated]

HowardHinnant · 2017-12-15T21:51:19Z

@seelabs Excellent! I've been wondering when compilers would start warning about that. What compiler gave us these warnings?

seelabs · 2017-12-15T21:53:34Z

@HowardHinnant That's clang 5.0

HowardHinnant · 2017-12-15T21:57:26Z

Well, it's going to put me out of a job. This is one of the things I look for to bitch about during code reviews. ;-)

ripplelabs-jenkins · 2017-12-15T22:00:35Z

Jenkins Build Summary

Built from this commit

Built at 20180203 - 03:15:52

Test Results

Build Type	Result	Status
coverage	999 cases, 0 failed, t: 575s	PASS ✅
clang.debug.unity	999 cases, 0 failed, t: 445s	PASS ✅
clang.debug.nounity	997 cases, 0 failed, t: 406s	PASS ✅
gcc.debug.unity	999 cases, 0 failed, t: 398s	PASS ✅
gcc.debug.nounity	997 cases, 0 failed, t: 277s	PASS ✅
clang.release.unity	998 cases, 0 failed, t: 474s	PASS ✅
gcc.release.unity	998 cases, 0 failed, t: 511s	PASS ✅

bachase · 2017-12-15T22:09:28Z

Good catch @seelabs. Looks like I grabbed a stale version from #2292. I'll rebase after that gets in the next 0.90.0 beta. The other warnings I will fix.

codecov-io · 2017-12-15T22:18:24Z

Codecov Report

Merging #2300 into develop will increase coverage by 0.13%.
The diff coverage is 84.46%.

@@            Coverage Diff             @@
##           develop   #2300      +/-   ##
==========================================
+ Coverage    70.96%   71.1%   +0.13%     
==========================================
  Files          691     692       +1     
  Lines        51663   51639      -24     
==========================================
+ Hits         36661   36716      +55     
+ Misses       15002   14923      -79

Impacted Files	Coverage Δ
src/ripple/app/consensus/RCLConsensus.h	`80% <ø> (ø)`	⬆️
src/ripple/app/main/Application.h	`100% <ø> (ø)`	⬆️
src/ripple/app/misc/NetworkOPs.cpp	`63.49% <0%> (+0.72%)`	⬆️
src/ripple/consensus/Consensus.h	`90.06% <100%> (+1.25%)`	⬆️
src/ripple/app/main/Application.cpp	`60.79% <100%> (-0.17%)`	⬇️
src/ripple/app/consensus/RCLValidations.h	`81.48% <100%> (+4.2%)`	⬆️
src/ripple/app/consensus/RCLConsensus.cpp	`67.32% <36.36%> (+0.65%)`	⬆️
src/ripple/app/consensus/RCLValidations.cpp	`86.36% <82.75%> (+13.25%)`	⬆️
src/ripple/beast/utility/Journal.h	`97.1% <90%> (ø)`	⬆️
src/ripple/consensus/LedgerTrie.h	`90.4% <90.4%> (ø)`
... and 113 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 090d813...1d97908. Read the comment docs.

seelabs · 2017-12-15T22:24:00Z

src/ripple/consensus/LedgerTrie.h

+                            + sum_(child : node->children) child->branchSupport
+        @endcode
+
+    This is analagous to the merkle tree property in which a node's hash is


Nit: Found some misspellings (analagous -> analogous). Here's a commit with fixes for the ones I found: seelabs@5a3e182

JoelKatz · 2017-12-15T23:56:54Z

What is the rule for rejecting a validation because its sequence number is too low? Is the rule that the sequence must be greater than that for any non-expired validation from that same validator? And does the code now issue partial validations automatically if the sequence number is less than or equal to that of any non-expired full validation that server has issued?

bachase · 2017-12-18T13:55:35Z

Thanks for your fix @seelabs, I grabbed that and added a fix for PeerGroup warnings. The other warnings will be fixed in a future rebase.

bachase · 2017-12-18T14:14:33Z

@JoelKatz

There are two separate ways this is enforced--when issuing validations and when receiving validations. When issuing validations, a validator tracks the largest sequence number/ledger index of a full validation and requires any subsequent issuing of a full validation to be for a higher number. Otherwise, the validation becomes a partial validation. This also builds on #2167 so that on a restart, this largest number must exceed that of the largest persisted ledger.

Similarly, when receiving validations, a node maintains the largest full validation sequence number from every validator. If it receives a full validation with smaller sequence number than the prior full validation from that validator, the validation is ignored. This state is not currently persisted across restarts.

All of these changes are in 1e349e2.

As coded, expiring a validation does not change this behavior. I could change it such that the receiving node resets the largest seen full validation sequence number if the most recent validation from that validator expired. Alternatively, instead of rejecting a full validation that violates the increasing sequence number invariant, we could treat it as a partial instead (which is what the issuing node should've done anyway).

Note that this implementation does expire stale validations as before and will not use expired validations in the new ledger ancestry/preferred branch calculations.

HowardHinnant · 2017-12-18T22:07:44Z

src/ripple/consensus/LedgerTrie.h

+                    [child](std::unique_ptr<Node> const& curr) {
+                        return curr.get() == child;
+                    });
+            assert(it != children.end());


If I'm reading this correctly (and I may not be), child must be in children exactly once. If that's the case, then we could make the assert stronger:

assert(it == children.end()-1);

seelabs · 2017-12-18T21:52:33Z

src/ripple/consensus/LedgerTrie.h

+
+        @code
+        node->branchSupport = node->tipSupport
+                            + sum_(child : node->children) child->branchSupport


The function call with the range loop syntax in the pseudo-code is a bit weird. How about:

node->branchSupport = node->tipSupport; for (child : node->children) node->branchSupport += child->branchSupport;

seelabs · 2017-12-18T21:55:34Z

src/ripple/consensus/LedgerTrie.h

+
+/** Ancestry trie of ledgers
+
+    Combination of a compressed trie and merkle-ish tree that maintains


The analogy with a merkle-tree doesn't aid in understanding here. While this data types and merkle trees both have parents that contain functions of their children, the functions are very different (sums vs. hashes) and serve very different purposes (support vs. verification of contents).

I'll remove it.

seelabs · 2017-12-18T21:58:09Z

src/ripple/consensus/LedgerTrie.h

+       {
+          // The default ledger represents a ledger that prefixes all other
+          // ledgers, e.g. the genesis ledger
+          Ledger();


Creating a genesis ledger should be a rare, and should be an operation that stands out. Can we remove the default constructor and add a tagged constructor for the genesis ledger?

seelabs · 2017-12-18T21:59:07Z

src/ripple/consensus/LedgerTrie.h

+          Ledger();
+
+          Ledger(Ledger &);
+          Ledger& operator=(Ledger );


Nit: Ledger & -> Ledger& and operator= should take the param by const&

seelabs · 2017-12-18T22:00:03Z

src/ripple/consensus/LedgerTrie.h

+        std::pair<Seq, ID>
+        tip() const
+        {
+            Seq tipSeq{end_ -Seq{1}};


Nit: formatting

seelabs · 2017-12-18T22:11:48Z

src/ripple/consensus/LedgerTrie.h

+            if(!children.empty())
+            {
+                Json::Value &cs = (res["children"] = Json::arrayValue);
+                for(auto const & child : children)


formatting: auto const&

seelabs · 2017-12-18T22:12:13Z

src/ripple/consensus/LedgerTrie.h

+
+    // The root of the trie. The root is allowed to break the no-single child
+    // invariant.
+    std::unique_ptr<Node> root;


Instead of unique_ptr can this be a value type instead (i.e. Node)

I considered that, but I felt the uniformity of all Nodes accessed via a pointer like interface trumped the benefit of a value type.

seelabs · 2017-12-18T22:13:07Z

src/ripple/consensus/LedgerTrie.h

+            // Find the child with the longest ancestry match
+            for (std::unique_ptr<Node> const& child : curr->children)
+            {
+                auto childPos = child->span.diff(ledger);


auto const

seelabs · 2017-12-18T22:14:09Z

src/ripple/consensus/LedgerTrie.h

+            //    becomes abcd->ef->...
+
+            // Create oldSuffix node that takes over loc
+            std::unique_ptr<Node> newNode{std::make_unique<Node>(oldSuffix)};


Don't need to say the type twice: auto newNode = std::make_unique<Node>(oldSuffix).

seelabs · 2017-12-18T22:14:39Z

src/ripple/consensus/LedgerTrie.h

+        The preferred ledger for this sequence number is then the ledger
+        with relative majority of support, where prefixSupport can be given to
+        ANY ledger at that sequence number (including one not yet known). If no
+        such preferred ledger exists, than prior sequence preferred ledger is


than -> then

bachase · 2017-12-20T17:54:58Z

src/test/csf/ledgers.h

+
+    // This is required by the generic Consensus for now and should be
+    // migrated to the MakeGenesis approach above.
+    Ledger() : Ledger(MakeGenesis{})


@seelabs I liked your suggestion for making the tagged constructor, but generic Consensus still expects a default constructor. I would like to fix that, but would prefer to do it as a separate refactor, so this is needed in the interim.

bachase · 2017-12-22T13:21:32Z

@JoelKatz, I believe the latest commit 2e85e5a addresses your concerns regarding expiration and allowed full validation sequence numbers. The invariant is now: require a full validation to be for a ledger sequence/index larger than any non-expired full validations previously received from that validator. The invariant enforced earlier in this PR did not include the non-expired qualification.

nbougalis

This is fine. I'm just leaving some general commentary; up to you whether to adopt it or not.

nbougalis · 2017-12-25T22:22:43Z

src/ripple/app/consensus/RCLValidations.cpp

+    RCLValidations& vals,
+    std::shared_ptr<Ledger const> ledger,
+    LedgerIndex minValidSeq)
+{


Does this kind of wrapper function really add anything, over the caller doing the work?

Nothing really added; it was to hide the detail of wrapping a ripple::Ledger in a RCLValidatedLedger. I will reconsider just having the caller do that work.

nbougalis · 2017-12-25T22:27:59Z

src/ripple/app/consensus/RCLConsensus.cpp


-    if (netLgr != ledgerID)
+    if (netLgr != ledgerID && netLgr != uint256{})


Two comments: first, this change actually relies on semantics that we ought to deprecate: namely that the default constructor of a base_uint will zero-initialize. Note: base_uint currently has operator! which returns true if it's zero - maybe we ought to add operator bool that returns true if it's non-zero and false otherwise?

Second, I don't think this call to getPreferred can ever return a zero (although I may be wrong). If that is the case, then there's no point in even checking this.

Correct on the second point, this check is not needed here.

nbougalis · 2017-12-25T22:34:33Z

src/ripple/consensus/Validations.h

@@ -73,7 +73,43 @@ struct ValidationParms
    std::chrono::seconds validationSET_EXPIRES = std::chrono::minutes{10};


Just commentary; no change necessary: I dislike this naming scheme - this mixed lowercase-UPPERCASE_WITH_UNDERSCORES thing is unlike anything else in the code, and it stands out as a sore thumb. Is this really the best we can do?

Why not simply, for example, std::chrono::seconds setExpiry = 10m;?

Good question. This arose during prior refactoring when making these settings non-constant for use in simulations. #2084 (comment) was the discussion then and it seems I went with the most non-conforming style 😏 . I'm happy to change if you prefer consistency.

nbougalis · 2017-12-25T22:49:12Z

src/ripple/app/consensus/RCLValidations.cpp

+auto
+RCLValidatedLedger::seq() const -> Seq
+{
+    return ledger_ ? ledger_->info().seq : Seq{0};


Is this ternary here for the unique case when we construct an RCLValidateLedger for the genesis ledger? Seems sub-optimal.

Correct, this is for the genesis ledger. I can cache those values in the constructor to avoid the ternary logic.

nbougalis · 2017-12-25T23:06:52Z

src/ripple/consensus/LedgerTrie.h

+    @tparam Ledger A type representing a ledger and its history
+*/
+template <class Ledger>
+class LedgerTrie


I kind of already know the answer to this, but is there no implementation of a trie that we can use instead of writing or own? It's unfortunate that Boost doesn't have one.

I didn't come across one that met the use case here.

nbougalis · 2017-12-26T00:15:05Z

src/ripple/consensus/Validations.h

+    /** Try advancing the largest observed full validation ledger sequence
+
+        Try setting the largest full sequence observed, but return false if it
+        violates the invaraint that a full validation must be larger than all


Typo: invariant. Also this comment is somewhat confusing. This function might be better named as accept or just treated as bool operator()(time_point now, Seq s, ValidationParms const & p). The call site would look like:

const bool isFull = proposing && fullSeqEnforcer_.accept(stopwatch().now(), ledger.seq(), app_.getValidations().parms());

Or:

const bool isFull = proposing && fullSeqEnforcer_(stopwatch().now(), ledger.seq(), app_.getValidations().parms());

nbougalis · 2017-12-26T01:18:21Z

src/ripple/consensus/Validations.h

+    getPreferred(Ledger const& currLedger, Seq minValidSeq)
+    {
+        std::pair<Seq, ID> preferred = getPreferred(currLedger);
+        if(preferred.first >= minValidSeq && preferred.second != ID{})


Deprecated behavior in base_uint: assuming that default construction leaves the object in some fixed value. Note: we can decide that is the behavior we really want from base_uint and declare that it accepted. But we should still decide what to do with this code.

@nbougalis, if the plan is to deprecate the base_uint default constructor behavior, I can rewrite this to use boost::optional instead. I was tempted by this approach since a ledger hash of 0 already has a special meaning.

I'm fine with using zero (as it does have a special meaning, as you point out). I just want to make sure that if we remove the "zero init on default construction" this code won't accidentally break.

I'll make it explicit as ID{0}.

nbougalis · 2017-12-26T02:24:17Z

src/ripple/app/consensus/RCLValidations.cpp

+auto
+RCLValidatedLedger::seq() const -> Seq
+{
+    return ledger_ ? ledger_->info().seq : Seq{0};


Why a ternary here? Is it to handle the case where this instance refers to the genesis ledger?

nbougalis · 2017-12-26T02:34:05Z

src/ripple/consensus/LedgerTrie.h

+#include <algorithm>
+#include <memory>
+#include <vector>
+#include <ripple/json/json_value.h>


Pico-nitpick: reordering includes with the standard ones at the bottom will help make sure we don't hide missing header errors.

nbougalis · 2017-12-26T02:35:44Z

src/ripple/consensus/LedgerTrie.h

+    class Span
+    {
+        // The span is the half-open interval [start,end) of ledger_
+        Seq start_{0};


Could we use boost::icl? It's probably overkill for this, but I want to make sure we've considered it.

Yes, I considered for a bit, but decided the relatively simple requirements made avoiding the burden of readers having to get familiar with ICL worth it.

seelabs

Left some comments, in particular I think we either need to handle empty spans or explicitly document the precondition that a span is not empty for some functions.

seelabs · 2018-01-02T21:52:11Z

src/ripple/consensus/LedgerTrie.h

+    using Seq = typename Ledger::Seq;
+    using ID = typename Ledger::ID;
+
+    /// Represents a span of ancestry of a ledger


The Span and Node inner classes obscure the interface to LedgerTrie, especially since the functions are implemented in the class definition. Consider moving the implementations outside the class definition (and possibly moving the inner classes to a namespace so they are no longer inner classes).

seelabs · 2018-01-02T21:54:03Z

src/ripple/consensus/LedgerTrie.h

+
+        bool
+        empty() const
+        {


Under what circumstances do we expect an empty span? Some of the Span function do not work correctly on empty spans (flagged later on).

seelabs · 2018-01-02T21:55:25Z

src/ripple/consensus/LedgerTrie.h

+        //Return the ID of the ledger that starts this span
+        ID
+        startID() const
+        {


What should happen if the span is empty here? Consider either assert(!empty()); if it's a precondition that the span isn't empty, or handle the empty case.

seelabs · 2018-01-02T21:56:12Z

src/ripple/consensus/LedgerTrie.h

+        //  The Seq and ID of the end of the span
+        std::pair<Seq, ID>
+        tip() const
+        {


I don't think tip makes sense for an empty span. Maybe assert(!empty());?

seelabs · 2018-01-02T21:56:44Z

src/ripple/consensus/LedgerTrie.h

+
+        friend std::ostream&
+        operator<<(std::ostream& o, Span const& s)
+        {


Need to handle empty spans here.

seelabs · 2018-01-02T22:00:00Z

src/ripple/consensus/LedgerTrie.h

+                  will be dangling as a result of this call
+        */
+        void
+        erase(Node const* child)


If fine with this as-is. You're right, it's a private helper class. I'd love to think of a better name (delete would work if it wasn't reserved). At any rate, no change needed.

seelabs · 2018-01-02T22:00:31Z

src/ripple/consensus/LedgerTrie.h

+
+        Json::Value
+        getJson() const
+        {


Handle empty span.

seelabs · 2018-01-02T22:05:52Z

src/ripple/consensus/LedgerTrie.h

+    void
+    insert(Ledger const& ledger, std::uint32_t count = 1)
+    {
+        Node* loc;


What does loc stand for?

loc is the location where the location where the new ledger support will go; it might be a Node that matches the ledger exactly, or it might be the ancestor.

Any alternate name that you would suggest?

Got it. That's fine. Sometimes I'll put a comment next to a variable like that and keep the small name: Node* loc; // location of the new ledger support, but comments like that can clutter up the code too. I think it's fine as-is, I just didn't think of "location". Thanks for clarifying.

seelabs · 2018-01-02T22:06:29Z

src/ripple/consensus/LedgerTrie.h

+        // There is always a place to insert
+        assert(loc);
+
+        Span lTmp{ledger};


What does lTmp stand for? It's a span, l makes me expect a ledger.

seelabs · 2018-01-02T22:12:42Z

src/ripple/consensus/LedgerTrie.h

+            newNode->tipSupport = loc->tipSupport;
+            newNode->branchSupport = loc->branchSupport;
+            using std::swap;
+            swap(newNode->children, loc->children);


This is fine as-is, but I wanted to leave a couple notes:

This isn't generic code. We know it's a std::vector, so std::swap is fine, we don't need the using trick.

We know newNode->children is empty, so we could just move, we don't need to swap at all: nodeNode->children = std::move(loc->children); If you're concerned children may change type in the future, you could assert(loc->children.empty()) This would be my preferred solution.

I'm not suggesting this, but newNode->children = std::exchange(loc->children, {}) is also a candidate to replace this code (std::exchange is less well known so people would stumble over the code, and it's not worth the benefit on one less move (I think)).

bachase · 2018-01-08T15:52:50Z

Thanks for the great feedback @seelabs. I made the invariant explicit that a Span is never empty and added asserts in the various constructors. The empty span state was only used during insert to detect suffixes and I now do that using an optional to ensure the Span invariant is never broken.

seelabs

👍

Require a new full validation to be for a sequence number larger than any unexpired previously issued full validation sequence number from that validator. This is less strict than the earlier changeset which did not allow expiration of what is the largest issued full validation sequence number.

Eliminate empty() span possibility.

Require all validations, full or partial, to be for a sequence number larger than any unexpired validations already issued by that node.

Updates the preferred branch algorithm to use uncommitted rather than prefix support as the required margin for selecting a ledger. Uncommitted suppport is the number of validated ledgers a node has received whose sequence number is less than the sequence number of interest or less than the sequence number last validated by the node.

Re-orders the comparison in mismatch of RCLValidations to avoid trying to lookup the ID of the sequence 0 ledger.

Use the SpanTip class to allow finding the ID of the ancestor of a tip of ledger history. The Validations::getPreferred function can use this to more directly check if the current ledger is the parent of the preferred ledger.

bachase · 2018-01-31T16:01:11Z

Rebased on 0.90.0-b5 and added 3 new commits in response to @wilsonianb's latest comments.

Fix typos.
Only acquire ledgers we are not yet acquiring when calling updateTrie
Use the added SpanTip::ancestor function to check if the current ledger is the parent of the preferred ledger.

wilsonianb · 2018-01-31T18:02:27Z

src/ripple/consensus/LedgerTrie.h

+    {
+    }
+
+    // The sequence number the tip ledger


-> of the

wilsonianb · 2018-01-31T18:03:29Z

src/ripple/consensus/LedgerTrie.h

+        @return The ID of the ancestor with that sequence number
+
+        @note s must be less than or equal to the sequence number of the
+              preferred ledger


should that say tip instead of preferred?

wilsonianb · 2018-02-01T23:57:35Z

src/test/consensus/LedgerTrie_test.cpp

+                  / \
+               B(1)  C(1)
+              /  |   |
+             H   D   F(1)


nit: F(1) -> F
or add (1) to H and G

Actually, this does reflect the initial tip support, but the test ends up changing the values. I'll copy the tree to those spots and update the values.

wilsonianb · 2018-02-02T00:14:44Z

src/test/consensus/LedgerTrie_test.cpp

+
+            // However, if you validated a ledger with Seq 5, potentially on
+            // a different branch, you do not yet know if they chose abcd
+            // or abcf because of you, so abc remains preferred


Can you elaborate on this for me? I'm not grasping the role of largestIssued

The problem is if you fully validate some ledger L, there is no guarantee that the parent of L had majority support of all nodes in the previous round. The issue arises when you have Byzantine nodes and/or multiple UNLs, since you don't know about the nodes not in your UNL that might've relied on your validation to fully validate a ledger that you are no longer committed to. By treating all validators whose last validation is for sequence less than largestIssued, you wait to be sure that you can't jump to the wrong branch when a majority of your trusted peers actually did validate your current branch.

https://gist.github.com/bachase/6f92b3907baeb3e8cf3ffd5fd8df5f31 is a motivating example involving Byzantine nodes.

wilsonianb · 2018-02-02T01:30:25Z

src/test/consensus/Validations_test.cpp

-        BEAST_EXPECT(harness.stale()[0].ledgerID() == Ledger::ID{1});
+        using Trigger = std::function<void(TestValidations&)>;
+
+        std::vector<Trigger> triggers = {


I think getCurrentPublicKeys and currentTrusted can be added to this as triggers

wilsonianb · 2018-02-02T01:32:57Z

src/test/consensus/Validations_test.cpp

        Node a = harness.makeNode(), b = harness.makeNode(),
             c = harness.makeNode(), d = harness.makeNode();
-
        c.untrust();

        // first round a,b,c agree, d has differing id


differing id -> partial?

wilsonianb · 2018-02-02T03:56:35Z

src/ripple/consensus/Validations.h

            });
        return ret;
    }

    /** Get the set of known public keys associated with current validations

-        @return The set of of knowns keys for current trusted and untrusted
-                validations
+        @return The set of knows keys for current listed validators


wilsonianb · 2018-02-02T05:22:37Z

src/ripple/app/consensus/RCLValidations.cpp

-        // Only forward if current and trusted
-        return current && val->isTrusted();
+        if(j.error())
+            dmp(j.error(), "missing ledger sequence field");


If this is an untrusted or unlisted validation, masterKey will be boost::none in dmp

wilsonianb · 2018-02-02T05:23:32Z

src/ripple/app/consensus/RCLValidations.cpp


-    bool shouldRelay = false;



nit: remove line

wilsonianb · 2018-02-02T05:27:11Z

src/ripple/app/consensus/RCLValidations.cpp

-                           << toBase58(TokenType::TOKEN_NODE_PUBLIC, *pubKey)
-                           << " published multiple validations for ledger "
-                           << seq;
+            dmp(j.warn(), "already validated sequence past " + to_string(seq));


-> at or past?

wilsonianb · 2018-02-02T05:29:55Z

src/ripple/app/consensus/RCLValidations.h

+    beast::Journal j_;
+};
+
+/** Generic validations adaptor classs for RCL


When first starting up, a node may still be working to acquire the ledgers for validations it has received. When calculating the preferred ledger in this case, we should still rely on these validations over peer counts until we acquire our first validated ledger. This should fix the speed at which a newly started node syncs with the network.

wilsonianb

LGTM 👍
Great work!

bachase · 2018-02-05T17:02:48Z

Merged in 94c6a2a

bachase requested review from seelabs, wilsonianb, HowardHinnant and JoelKatz December 15, 2017 21:27

bachase assigned JoelKatz, seelabs, wilsonianb and HowardHinnant Dec 15, 2017

seelabs reviewed Dec 15, 2017

View reviewed changes

HowardHinnant reviewed Dec 18, 2017

View reviewed changes

seelabs reviewed Dec 18, 2017

View reviewed changes

bachase commented Dec 20, 2017

View reviewed changes

HowardHinnant approved these changes Dec 21, 2017

View reviewed changes

nbougalis approved these changes Dec 26, 2017

View reviewed changes

seelabs reviewed Jan 2, 2018

View reviewed changes

seelabs approved these changes Jan 8, 2018

View reviewed changes

seelabs and others added 17 commits January 31, 2018 08:50

[FOLD] Fix misspellings

9501278

[FOLD] Fix clang PeerGroup warnings

329ca88

[FOLD] Address PR suggestions

ad35ee2

[FOLD] Make a few memer functions const

cff3191

[FOLD] Address PR comments

fd58f28

[FOLD] Explicitly initialize zero IDs

2b2ba69

[FOLD] Address PR comments:

0b58ac4

Eliminate empty() span possibility.

[FOLD] Move support classes to namespace

7cdd0e7

[FOLD] Fix assert

961747f

[FOLD] Enforce increasing sequence invariant on all validations:

37c583d

Require all validations, full or partial, to be for a sequence number larger than any unexpired validations already issued by that node.

[FOLD] Reduce log warnings:

0ccf0a8

Re-orders the comparison in mismatch of RCLValidations to avoid trying to lookup the ID of the sequence 0 ledger.

[FOLD] Address PR comments

277134b

[FOLD] Fix typos

83d5d55

[FOLD] Only acquire ledgers we are not yet acquiring

944527d

[FOLD] Add SpanTip:

ea18089

Use the SpanTip class to allow finding the ID of the ancestor of a tip of ledger history. The Validations::getPreferred function can use this to more directly check if the current ledger is the parent of the preferred ledger.

bachase force-pushed the prefer-branch branch from aecab26 to ea18089 Compare January 31, 2018 15:58

wilsonianb reviewed Feb 2, 2018

View reviewed changes

bachase added 3 commits February 2, 2018 13:09

[FOLD] Address PR comments

2a97f83

[FOLD] Track seq and ID in acquiring ledgers

5fd14de

wilsonianb approved these changes Feb 3, 2018

View reviewed changes

bachase mentioned this pull request Feb 3, 2018

Proposed 0.90.0-b6 #2370

Merged

bachase added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Feb 3, 2018

bachase closed this Feb 5, 2018

bachase mentioned this pull request Feb 21, 2018

Update validations on UNL change (RIPD-1566) #2393

Closed


		/** Ancestry trie of ledgers

		Combination of a compressed trie and merkle-ish tree that maintains


		if (netLgr != ledgerID)
		if (netLgr != ledgerID && netLgr != uint256{})

		@@ -73,7 +73,43 @@ struct ValidationParms
		std::chrono::seconds validationSET_EXPIRES = std::chrono::minutes{10};

Prefer ledger by branch #2300

Prefer ledger by branch #2300

Conversation

bachase commented Dec 15, 2017 • edited Loading

bachase commented Dec 15, 2017 • edited Loading

bachase commented Dec 15, 2017 • edited Loading

seelabs commented Dec 15, 2017 • edited Loading

HowardHinnant commented Dec 15, 2017

seelabs commented Dec 15, 2017

HowardHinnant commented Dec 15, 2017

ripplelabs-jenkins commented Dec 15, 2017 • edited Loading

Jenkins Build Summary

Test Results

bachase commented Dec 15, 2017

codecov-io commented Dec 15, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

JoelKatz commented Dec 15, 2017

bachase commented Dec 18, 2017

bachase commented Dec 18, 2017 • edited Loading

HowardHinnant Dec 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bachase commented Dec 22, 2017

nbougalis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bachase Dec 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seelabs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bachase commented Jan 8, 2018

seelabs left a comment

Choose a reason for hiding this comment

bachase commented Jan 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bachase commented Dec 15, 2017 •

edited

Loading

bachase commented Dec 15, 2017 •

edited

Loading

bachase commented Dec 15, 2017 •

edited

Loading

seelabs commented Dec 15, 2017 •

edited

Loading

ripplelabs-jenkins commented Dec 15, 2017 •

edited

Loading

codecov-io commented Dec 15, 2017 •

edited

Loading

bachase commented Dec 18, 2017 •

edited

Loading

HowardHinnant Dec 18, 2017 •

edited

Loading

bachase Dec 27, 2017 •

edited

Loading