Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prefer ledger by branch #2300

Closed
wants to merge 38 commits into from

Conversation

@bachase
Copy link
Contributor

commented Dec 15, 2017

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

This comment has been minimized.

Copy link
Contributor Author

commented Dec 15, 2017

@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

This comment has been minimized.

Copy link
Contributor Author

commented Dec 15, 2017

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

@seelabs

This comment has been minimized.

Copy link
Contributor

commented Dec 15, 2017

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

This comment has been minimized.

Copy link
Contributor

commented Dec 15, 2017

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

@seelabs

This comment has been minimized.

Copy link
Contributor

commented Dec 15, 2017

@HowardHinnant That's clang 5.0

@HowardHinnant

This comment has been minimized.

Copy link
Contributor

commented Dec 15, 2017

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

This comment has been minimized.

Copy link
Collaborator

commented Dec 15, 2017

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

This comment has been minimized.

Copy link
Contributor Author

commented Dec 15, 2017

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

This comment has been minimized.

Copy link

commented Dec 15, 2017

Codecov Report

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

Impacted file tree graph

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

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

This comment has been minimized.

Copy link
@seelabs

seelabs Dec 15, 2017

Contributor

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

@JoelKatz

This comment has been minimized.

Copy link
Member

commented Dec 15, 2017

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

This comment has been minimized.

Copy link
Contributor Author

commented Dec 18, 2017

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

This comment has been minimized.

Copy link
Contributor Author

commented Dec 18, 2017

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

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

This comment has been minimized.

Copy link
@HowardHinnant

HowardHinnant Dec 18, 2017

Contributor

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);
@code
node->branchSupport = node->tipSupport
+ sum_(child : node->children) child->branchSupport

This comment has been minimized.

Copy link
@seelabs

seelabs Dec 18, 2017

Contributor

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;

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

This comment has been minimized.

Copy link
@seelabs

seelabs Dec 18, 2017

Contributor

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).

This comment has been minimized.

Copy link
@bachase

bachase Dec 20, 2017

Author Contributor

I'll remove it.

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

This comment has been minimized.

Copy link
@seelabs

seelabs Dec 18, 2017

Contributor

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?

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

This comment has been minimized.

Copy link
@seelabs

seelabs Dec 18, 2017

Contributor

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

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

This comment has been minimized.

Copy link
@seelabs

seelabs Dec 18, 2017

Contributor

Nit: formatting

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

This comment has been minimized.

Copy link
@seelabs

seelabs Dec 18, 2017

Contributor

formatting: auto const&


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

This comment has been minimized.

Copy link
@seelabs

seelabs Dec 18, 2017

Contributor

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

This comment has been minimized.

Copy link
@bachase

bachase Dec 20, 2017

Author Contributor

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

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

This comment has been minimized.

Copy link
@seelabs

seelabs Dec 18, 2017

Contributor

auto const

// becomes abcd->ef->...

// Create oldSuffix node that takes over loc
std::unique_ptr<Node> newNode{std::make_unique<Node>(oldSuffix)};

This comment has been minimized.

Copy link
@seelabs

seelabs Dec 18, 2017

Contributor

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

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

This comment has been minimized.

Copy link
@seelabs

seelabs Dec 18, 2017

Contributor

than -> then


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

This comment has been minimized.

Copy link
@bachase

bachase Dec 20, 2017

Author Contributor

@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

This comment has been minimized.

Copy link
Contributor Author

commented Dec 22, 2017

@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
Copy link
Member

left a comment

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

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

This comment has been minimized.

Copy link
@nbougalis

nbougalis Dec 26, 2017

Member

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

This comment has been minimized.

Copy link
@bachase

bachase Dec 27, 2017

Author Contributor

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.


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

This comment has been minimized.

Copy link
@nbougalis

nbougalis Dec 26, 2017

Member

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.

This comment has been minimized.

Copy link
@bachase

bachase Dec 27, 2017

Author Contributor

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

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

This comment has been minimized.

Copy link
@nbougalis

nbougalis Dec 26, 2017

Member

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;?

This comment has been minimized.

Copy link
@bachase

bachase Dec 27, 2017

Author Contributor

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.

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

This comment has been minimized.

Copy link
@nbougalis

nbougalis Dec 26, 2017

Member

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

This comment has been minimized.

Copy link
@bachase

bachase Dec 27, 2017

Author Contributor

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

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

This comment has been minimized.

Copy link
@nbougalis

nbougalis Dec 26, 2017

Member

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.

This comment has been minimized.

Copy link
@bachase

bachase Dec 27, 2017

Author Contributor

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

/** 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

This comment has been minimized.

Copy link
@nbougalis

nbougalis Dec 26, 2017

Member

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());
getPreferred(Ledger const& currLedger, Seq minValidSeq)
{
std::pair<Seq, ID> preferred = getPreferred(currLedger);
if(preferred.first >= minValidSeq && preferred.second != ID{})

This comment has been minimized.

Copy link
@nbougalis

nbougalis Dec 26, 2017

Member

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.

This comment has been minimized.

Copy link
@bachase

bachase Dec 27, 2017

Author Contributor

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

This comment has been minimized.

Copy link
@nbougalis

nbougalis Dec 27, 2017

Member

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.

This comment has been minimized.

Copy link
@bachase

bachase Dec 28, 2017

Author Contributor

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

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

This comment has been minimized.

Copy link
@nbougalis

nbougalis Dec 26, 2017

Member

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

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

This comment has been minimized.

Copy link
@nbougalis

nbougalis Dec 26, 2017

Member

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

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

This comment has been minimized.

Copy link
@nbougalis

nbougalis Dec 26, 2017

Member

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

This comment has been minimized.

Copy link
@bachase

bachase Dec 27, 2017

Author Contributor

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
Copy link
Contributor

left a comment

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.

using Seq = typename Ledger::Seq;
using ID = typename Ledger::ID;

/// Represents a span of ancestry of a ledger

This comment has been minimized.

Copy link
@seelabs

seelabs Jan 2, 2018

Contributor

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).


bool
empty() const
{

This comment has been minimized.

Copy link
@seelabs

seelabs Jan 2, 2018

Contributor

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

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

This comment has been minimized.

Copy link
@seelabs

seelabs Jan 2, 2018

Contributor

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.

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

This comment has been minimized.

Copy link
@seelabs

seelabs Jan 2, 2018

Contributor

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


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

This comment has been minimized.

Copy link
@seelabs

seelabs Jan 2, 2018

Contributor

Need to handle empty spans here.

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

This comment has been minimized.

Copy link
@seelabs

seelabs Jan 2, 2018

Contributor

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.


Json::Value
getJson() const
{

This comment has been minimized.

Copy link
@seelabs

seelabs Jan 2, 2018

Contributor

Handle empty span.

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

This comment has been minimized.

Copy link
@seelabs

seelabs Jan 2, 2018

Contributor

What does loc stand for?

This comment has been minimized.

Copy link
@bachase

bachase Jan 8, 2018

Author Contributor

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?

This comment has been minimized.

Copy link
@seelabs

seelabs Jan 8, 2018

Contributor

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.

// There is always a place to insert
assert(loc);

Span lTmp{ledger};

This comment has been minimized.

Copy link
@seelabs

seelabs Jan 2, 2018

Contributor

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

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

This comment has been minimized.

Copy link
@seelabs

seelabs Jan 2, 2018

Contributor

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

  1. This isn't generic code. We know it's a std::vector, so std::swap is fine, we don't need the using trick.
  2. 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.
  3. 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

This comment has been minimized.

Copy link
Contributor Author

commented Jan 8, 2018

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

seelabs approved these changes Jan 8, 2018

Copy link
Contributor

left a comment

👍

bachase and others added some commits Dec 20, 2017

[FOLD] Change full validation sequence number invariant:
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.
[FOLD] Address PR comments:
Eliminate empty() span possibility.
[FOLD] Enforce increasing sequence invariant on all validations:
Require all validations, full or partial, to be for a sequence number
larger than any unexpired validations already issued by that node.
[FOLD] Use uncommitted support in preferred branch:
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.
[FOLD] Reduce log warnings:
Re-orders the comparison in mismatch of RCLValidations to avoid trying
to lookup the ID of the sequence 0 ledger.
[FOLD] Add SpanTip:
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 bachase force-pushed the bachase:prefer-branch branch from aecab26 to ea18089 Jan 31, 2018

@bachase

This comment has been minimized.

Copy link
Contributor Author

commented Jan 31, 2018

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.
{
}

// The sequence number the tip ledger

This comment has been minimized.

Copy link
@wilsonianb

wilsonianb Feb 2, 2018

-> of the

@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

This comment has been minimized.

Copy link
@wilsonianb

wilsonianb Feb 2, 2018

should that say tip instead of preferred?

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

This comment has been minimized.

Copy link
@wilsonianb

wilsonianb Feb 2, 2018

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

This comment has been minimized.

Copy link
@bachase

bachase Feb 2, 2018

Author Contributor

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.


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

This comment has been minimized.

Copy link
@wilsonianb

wilsonianb Feb 2, 2018

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

This comment has been minimized.

Copy link
@bachase

bachase Feb 2, 2018

Author Contributor

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.

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

std::vector<Trigger> triggers = {

This comment has been minimized.

Copy link
@wilsonianb

wilsonianb Feb 2, 2018

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

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

This comment has been minimized.

Copy link
@wilsonianb

wilsonianb Feb 2, 2018

differing id -> partial?

});
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

This comment has been minimized.

Copy link
@wilsonianb

wilsonianb Feb 2, 2018

-> known

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

This comment has been minimized.

Copy link
@wilsonianb

wilsonianb Feb 2, 2018

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


bool shouldRelay = false;

This comment has been minimized.

Copy link
@wilsonianb

wilsonianb Feb 2, 2018

nit: remove line

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

This comment has been minimized.

Copy link
@wilsonianb

wilsonianb Feb 2, 2018

-> at or past?

beast::Journal j_;
};

/** Generic validations adaptor classs for RCL

This comment has been minimized.

Copy link
@wilsonianb

wilsonianb Feb 2, 2018

-> class

@ripplelabs-jenkins

This comment has been minimized.

Copy link
Collaborator

commented on 2a97f83 Feb 2, 2018

Jenkins Build Summary

Built from this commit

Built at 20180202 - 19:17:20

Test Results

Build Type Result Status
coverage 999 cases, 0 failed, t: 577s PASS
clang.debug.unity 999 cases, 0 failed, t: 448s PASS
clang.debug.nounity 997 cases, 0 failed, t: 352s PASS
gcc.debug.unity 999 cases, 0 failed, t: 398s PASS
gcc.debug.nounity 997 cases, 0 failed, t: 664s PASS
clang.release.unity 998 cases, 0 failed, t: 470s PASS
gcc.release.unity 998 cases, 0 failed, t: 509s PASS

bachase added some commits Feb 3, 2018

[FOLD] Fallback to acquiring ledgers:
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
Copy link

left a comment

LGTM 👍
Great work!

@bachase bachase referenced this pull request Feb 3, 2018

Merged

Proposed 0.90.0-b6 #2370

@bachase bachase added the Passed label Feb 3, 2018

@ripplelabs-jenkins

This comment has been minimized.

Copy link
Collaborator

commented on f0bdfe1 Feb 3, 2018

Jenkins Build Summary

Built from this commit

Built at 20180203 - 02:49:14

Test Results

Build Type Result Status
coverage 999 cases, 0 failed, t: 580s PASS
clang.debug.unity 999 cases, 0 failed, t: 445s PASS
clang.debug.nounity 997 cases, 0 failed, t: 354s PASS
gcc.debug.unity 999 cases, 0 failed, t: 394s PASS
clang.release.unity 998 cases, 0 failed, t: 477s PASS
gcc.debug.nounity 997 cases, 0 failed, t: 789s PASS
gcc.release.unity 998 cases, 0 failed, t: 506s PASS
@bachase

This comment has been minimized.

Copy link
Contributor Author

commented Feb 5, 2018

Merged in 94c6a2a

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.