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

bachase · 2018-02-16T20:57:01Z

When validators are removed or added via dynamic UNL change, these changes ensure a node updates the trust status of any existing validations.

The first set of changes switch to use NodeID to consistently refer to the sender of consensus proposals and validations even when using manifests.

ripplelabs-jenkins · 2018-02-16T23:03:53Z

Jenkins Build Summary

Built from this commit

Built at 20180301 - 05:36:53

Test Results

Build Type	Result	Status
coverage	1003 cases, 0 failed, t: 627s	PASS ✅
clang.debug.unity	1003 cases, 0 failed, t: 400s	PASS ✅
clang.debug.nounity	1001 cases, 0 failed, t: 355s	PASS ✅
gcc.debug.unity	1003 cases, 0 failed, t: 445s	PASS ✅
gcc.debug.nounity	1001 cases, 0 failed, t: 492s	PASS ✅
clang.release.unity	1002 cases, 0 failed, t: 482s	PASS ✅
gcc.release.unity	1002 cases, 0 failed, t: 516s	PASS ✅

bachase · 2018-02-21T00:55:22Z

Rebased on 0.90.0

wilsonianb · 2018-02-20T05:25:32Z

src/ripple/protocol/STValidation.h

+
+        @param ledgerHash The hash of the validated ledger
+        @param signTime When the validation is signed
+        @param raPub The current signing public key


I realize this isn't new but maybe change raPub to publicKey here since that's what's used in the definition, and I don't know what raPub means

wilsonianb · 2018-02-20T05:26:03Z

src/ripple/protocol/STValidation.h

+        @param ledgerHash The hash of the validated ledger
+        @param signTime When the validation is signed
+        @param raPub The current signing public key
+        @param nodeID ID corresponding to nodes public master key


nodes -> node's

wilsonianb · 2018-02-21T00:08:29Z

src/ripple/app/consensus/RCLValidations.cpp

@@ -172,7 +172,7 @@ RCLValidationsAdaptor::onStale(RCLValidation&& v)
 }

 void
-RCLValidationsAdaptor::flush(hash_map<PublicKey, RCLValidation>&& remaining)
+RCLValidationsAdaptor::flush(hash_map<NodeID, RCLValidation>&& remaining)


The docs should also be updated
https://github.com/bachase/rippled/blob/adef962292739c2e732d253c11cb168fa92e48c6/src/ripple/consensus/Validations.h#L260

wilsonianb · 2018-02-21T00:19:25Z

src/ripple/consensus/Validations.h

            if (!enforcer(now, val.seq(), parms_))
                return ValStatus::badSeq;

            // This validation is a repeat if we already have
-            // one with the same id for this key
-            auto const ret = byLedger_[val.ledgerID()].emplace(key, val);
+            // one with the same id and signing key for this node


would it also be a repeat if we already have one with the same id and a different signing key for this node?

I think repeatedID actually should have been removed as part of the prefer by branch work (which you suggested #2300 (comment)). If a node issues two validations with the same ID (using either the same or different signing keys), the seq enforcer above will ignore all but the first.

Agree?

How about (the unexpected case) when there's multiple validations with the same ledger id from the same validator but with different sequences?
We could just treat it normally (add the new validation assuming it has a higher sequence) and not need repeatedID.
I was mainly asking because I don't think this change to the comment is accurate.

I believe the ledger sequence number is part of what is hashed to create the ledger id, so you can't really have two validations for the same id but different sequence numbers.

I agree that the comment is now inaccurate.

Right, it would be a malformed validation, but that doesn't mean someone won't send one.

Good point.

I like

We could just treat it normally (add the new validation assuming it has a higher sequence) and not need repeatedID.

I don't see repeatedID really giving us much useful information; the increasing sequence number invariant is the essential one.

Actually, what would happen if a trusted validator only sends validations for a single id but with incrementing sequence numbers?
It seems like it would be as if they only validated that single id, but that validation never expires/is always current (assuming they keep resigning and updated the sign time). If we ever stopped keeping that ledger (via online delete?) we would keep re-acquiring it in order to update the ledger trie.
Would any of that be an issue?

If we were unable to acquire it, that validator's validation would not be added to the trie and effectively be ignored. Impact wise, I don't see this as different than a validator sending validations for random ledger ids that we can't acquire.

I'm wondering about if it's a valid ledger id that we can acquire.
If we haven't acquired it yet, and we have a prior validation with the same id but lower seq, we would ~~cancel and then restart the acquire attempt.~~ hit InboundLedgers multiple times for the same ledger
If/once we have acquired the ledger, we would have this entry in the trie with the real seq that persists until the bad validator stops sending the validations.

wilsonianb · 2018-02-21T05:04:19Z

src/ripple/protocol/STValidation.h

+
+    /** Construct a STValidation from a peer.
+
+        Construct an STValidation from serialized data previously shared by a


nit: either an -> a or a -> an above

wilsonianb · 2018-02-21T05:05:17Z

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

+            break;
+        newTrustedKeys.insert(val.second);
+
+        if (trustedKeys_.count(val.second) == 0)


You could change this to

if (trustedKeys_.erase(val.second) == 0)

and then you could remove the if(newTrustedKeys.count(k) == 0) check in the loop below

wilsonianb · 2018-02-21T05:05:59Z

src/ripple/app/misc/ValidatorList.h

-    void
-    onConsensusStart (
-        KeySet const& seenValidators);
+    TrustChanges


Do you think this warrants a name change to onConsensusStart?

Good call; updateTrustedKeys matches the doxygen comment.

Actually, that was already out of sync a bit based on these changes. updateTrusted is better.

wilsonianb · 2018-02-21T05:07:00Z

src/test/app/ValidatorList_test.cpp

+                        calcNodeID(*parseBase58<PublicKey>(
+                            TokenType::TOKEN_NODE_PUBLIC, valKey)));
+                    if(ins.second)
+                        initiallyAdded.insert(*ins.first);


I got confused by initiallyAdded. It seems to actually represent the second/subsequent set of added trusted keys.

seelabs · 2018-02-21T16:34:17Z

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

+            trustChanges.removed.insert(calcNodeID(k));
+
+    quorum_ = quorum;
+    std::swap(newTrustedKeys, trustedKeys_);


We don't really need to swap here, this is really just a way to quickly move newTrustedKeys into trustedKeys_. We can do that directly by putting newTrustedKeys into it's own scope and then trustedKeys_ = std::move(newTrustedKeys) here instead. Fine as-is though.

Your suggestion is clearer for anyone reading the code, so I will change.

seelabs · 2018-02-21T16:41:12Z

src/ripple/protocol/STValidation.h

+    */
+    STValidation(
+        SerialIter& sit,
+        std::function<NodeID(PublicKey const&)> const& lookupNodeID,


I don't love std::function if it can be avoided (it type erases and may allocate). This function is used to calculate a NodeID. One solution: we could make this parameter a template and then delegate to a private constructor that takes the NodeID directly. That way you don't have to put the much of an implementation in the header file.

The PublicKey that is the argument to the function requires initializing the STObject base class first, so I had trouble delegating to a private constructor. I think putting everything in the header and using a template isn't so bad in this case.

seelabs · 2018-02-21T16:42:01Z

src/test/app/RCLValidations_test.cpp

    void
-    run() override
+    testChangeTrusted()
+    {


Give the test a name with testcase

seelabs · 2018-02-21T16:42:21Z

src/test/app/RCLValidations_test.cpp

+    }
+
+    void
+    testRCLValidatedLedger()
    {


Give the test a name with testcase

seelabs · 2018-02-21T16:43:59Z

src/test/app/ValidatorList_test.cpp

+    static hash_set<NodeID>
+    asNodeIDs(std::initializer_list<PublicKey> const& pks)
+    {
+        hash_set<NodeID> res;


Nit: We know the final size of res. We could res.reserve(pks.size()); here. Tho I'm less concerned about this sort of thing in test code.

seelabs · 2018-02-21T16:46:11Z

src/test/consensus/Validations_test.cpp

+            trustedVals.clear();
+            harness.vals().trustChanged({}, {a.nodeID()});
+            // make acquiring ledger available
+            Ledger ledgerAB = h["ab"];


ledgerAB is an unused variable

wilsonianb · 2018-02-21T21:23:39Z

src/test/app/ValidatorList_test.cpp

@@ -530,7 +530,7 @@ class ValidatorList_test : public beast::unit_test::suite



This test could also be renamed to testUpdateTrusted

wilsonianb · 2018-02-21T21:49:07Z

src/ripple/protocol/STValidation.h

+
+        if (checkSignature && !isValid())
+        {
+            JLOG(debugLog().error()) << "Invalid validation" << getJson(0);


error: there are no arguments to 'debugLog' that depend on a template parameter, so a declaration of 'debugLog' must be available [-fpermissive] JLOG(debugLog().error()) << "Invalid validation" << getJson(0);

We need to include #include <ripple/basics/Log.h> in this file.

wilsonianb · 2018-02-21T21:54:07Z

src/ripple/consensus/Validations.h

-            auto const ret = byLedger_[val.ledgerID()].emplace(key, val);
-            if (!ret.second && ret.first->second.key() == val.key())
-                return ValStatus::repeatID;
+            // one with the same id and signing key for this node


Should the comment be removed or modified to say that we don't reject a validation with the same id as a previous one with a lower sequence?

Oops, forgot to remove.

seelabs · 2018-02-23T14:27:35Z

src/ripple/consensus/Validations.h

+            // one with the same id and signing key for this node
+            auto ret = byLedger_[val.ledgerID()].emplace(nodeID, val);
+            if (!ret.second)
+                ret.first->second = val;


I'd usually see code like this written as something like: byLedger_[ledgerID][nodeID] = val. What you have is better (and doesn't require val to be default constructable). Maybe leave a comment that we should use c++-17's insert_or_assign here when we move to c++-17. This both clarifies the intent here and reminds us to clean this up when we switch.

codecov-io · 2018-02-23T17:34:16Z

Codecov Report

Merging #2393 into develop will increase coverage by 0.01%.
The diff coverage is 87.42%.

@@             Coverage Diff             @@
##           develop    #2393      +/-   ##
===========================================
+ Coverage     70.3%   70.32%   +0.01%     
===========================================
  Files          703      703              
  Lines        53671    53715      +44     
===========================================
+ Hits         37735    37776      +41     
- Misses       15936    15939       +3

Impacted Files	Coverage Δ
src/ripple/app/misc/NetworkOPs.h	`100% <ø> (ø)`	⬆️
src/ripple/app/misc/ValidatorKeys.h	`100% <ø> (ø)`	⬆️
src/ripple/app/consensus/RCLConsensus.h	`80% <ø> (ø)`	⬆️
src/ripple/overlay/impl/PeerImp.cpp	`0% <0%> (ø)`	⬆️
src/ripple/app/consensus/RCLConsensus.cpp	`66.9% <100%> (-0.08%)`	⬇️
src/ripple/app/consensus/RCLValidations.cpp	`83.12% <100%> (+1.41%)`	⬆️
src/ripple/overlay/impl/OverlayImpl.cpp	`33.15% <100%> (+0.48%)`	⬆️
src/ripple/app/misc/impl/ValidatorKeys.cpp	`87.87% <100%> (+0.78%)`	⬆️
src/ripple/app/misc/NetworkOPs.cpp	`63.78% <100%> (+0.05%)`	⬆️
src/ripple/app/misc/ValidatorList.h	`100% <100%> (+1.69%)`	⬆️
... and 13 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 6230204...941935f. Read the comment docs.

seelabs

👍

wilsonianb

LGTM 👍

wilsonianb · 2018-02-23T22:09:26Z

src/test/app/ValidatorList_test.cpp

@@ -535,15 +546,18 @@ class ValidatorList_test : public beast::unit_test::suite
                cfgKeys.push_back (toBase58(
                    TokenType::TOKEN_NODE_PUBLIC, valKey));
                if (cfgKeys.size () <= n - 5)
-                    activeValidators.emplace (valKey);
+                    activeValidators.emplace (calcNodeID(valKey));
            }

            BEAST_EXPECT(trustedKeys->load (
                emptyLocalKey, cfgKeys, cfgPublishers));

            // onConsensusStart should make all available configured


stray onConsensusStart

Change the trust status of existing validations based when nodes are added or removed from the UNL.

bachase · 2018-02-27T13:13:41Z

Squashed.

mellery451 · 2018-03-02T23:48:50Z

Merged in 20defb4

bachase assigned seelabs and wilsonianb Feb 16, 2018

bachase requested review from seelabs and wilsonianb February 16, 2018 20:57

bachase force-pushed the update-unl branch from 71a6ef8 to adef962 Compare February 21, 2018 00:55

wilsonianb reviewed Feb 21, 2018

View reviewed changes

seelabs reviewed Feb 21, 2018

View reviewed changes

wilsonianb reviewed Feb 21, 2018

View reviewed changes

seelabs reviewed Feb 23, 2018

View reviewed changes

seelabs approved these changes Feb 26, 2018

View reviewed changes

wilsonianb approved these changes Feb 26, 2018

View reviewed changes

Update validations on UNL change (RIPD-1566):

941935f

Change the trust status of existing validations based when nodes are added or removed from the UNL.

bachase force-pushed the update-unl branch from d90d024 to 941935f Compare February 27, 2018 13:12

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

mellery451 closed this Mar 2, 2018


		/** Construct a STValidation from a peer.

		Construct an STValidation from serialized data previously shared by a

		@@ -530,7 +530,7 @@ class ValidatorList_test : public beast::unit_test::suite

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

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

Conversation

bachase commented Feb 16, 2018

ripplelabs-jenkins commented Feb 16, 2018 • edited Loading

Jenkins Build Summary

Test Results

bachase commented Feb 21, 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

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

wilsonianb Feb 21, 2018 • 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

seelabs Feb 21, 2018 • 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

codecov-io commented Feb 23, 2018 • edited Loading

Codecov Report

seelabs left a comment

Choose a reason for hiding this comment

wilsonianb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bachase commented Feb 27, 2018

mellery451 commented Mar 2, 2018

ripplelabs-jenkins commented Feb 16, 2018 •

edited

Loading

wilsonianb Feb 21, 2018 •

edited

Loading

seelabs Feb 21, 2018 •

edited

Loading

codecov-io commented Feb 23, 2018 •

edited

Loading