Allow a newly-started validator to participate #2167

mtrippled · 2017-07-11T09:33:40Z

in consensus.

bachase

The changes all look good. Left some style nits. https://github.com/bachase/rippled/commits/bc-recover has version rebased on the proposed 0.80.0-b1 and another commit with the style changes (feel free to ignore them).

Additionally, the commit message is formatted oddly in github; I believe we shoot for 80 character width.

It would be nice to write a unit test for these changes, but I don't see a nice way with the current framework.

bachase · 2017-07-11T19:01:53Z

src/ripple/app/misc/Validations.cpp

@@ -87,6 +87,16 @@ class ValidationsImp : public Validations
 private:
    bool addValidation (STValidation::ref val, std::string const& source) override
    {
+        if (val->getFieldU32 (sfLedgerSequence) <= app_.getMaxLedger())


Heads up that this will need to go to RCLValidations.cpp after rebasing on 0.80.0-b1

bachase · 2017-07-11T19:04:19Z

src/ripple/app/main/Application.cpp

@@ -1126,6 +1138,7 @@ bool ApplicationImp::setup()
            getWalletDB (), "PublisherManifests");

        m_networkOPs->setValidationKeys (valSecret, valPublic);
+        setMaxLedger();


Consider just inlining to the call site since it isn't reused.

I'd rather keep this broken out as a separate function because the calling function is so big already.

bachase · 2017-07-11T19:08:37Z

src/ripple/app/ledger/LedgerMaster.h

    std::atomic <std::uint32_t> mValidLedgerSign;
-    std::atomic <std::uint32_t> mValidLedgerSeq;
-    std::atomic <std::uint32_t> mBuildingLedgerSeq;
+    std::atomic <LedgerIndex> mValidLedgerSeq {0};


It looks like this is initialized in the constructor already (https://github.com/ripple/rippled/blob/develop/src/ripple/app/ledger/impl/LedgerMaster.cpp#L83). I actually prefer the inline initialization you have here. Consider treating the other atomics the same way.

bachase · 2017-07-11T19:23:47Z

src/ripple/app/ledger/LedgerMaster.h

@@ -343,6 +344,8 @@ class LedgerMaster

    std::uint32_t fetch_seq_;

+    LedgerIndex const max_ledger_difference_ {1000000};


Add a comment on the purpose?

I'd also like an explanation

bachase · 2017-07-11T19:25:26Z

src/ripple/app/main/Application.cpp

@@ -937,8 +940,15 @@ class ApplicationImp
            std::chrono::seconds {config_->getSize (siSweepInterval)});
    }

+    LedgerIndex getMaxLedger() override


Since this is only the maximum ledger persisted during the last run of the program and is not updated once we start running, consider renaming to getMinAllowedLedger.

I'm also in favor of a rename

Fixed. It's not minAllowed, it's minAllowed - 1 which doesn't flow off the tongue nicely. So I made it maxDisallowedLedger

mtrippled · 2017-07-11T20:31:10Z

@bachase I'll wait for David & Brandon's comments before incorporating the changes you suggested.

wilsonianb · 2017-07-12T21:04:52Z

src/ripple/app/ledger/LedgerMaster.h

@@ -343,6 +344,8 @@ class LedgerMaster

    std::uint32_t fetch_seq_;

+    LedgerIndex const max_ledger_difference_ {1000000};


I'd also like an explanation

wilsonianb · 2017-07-12T21:05:24Z

src/ripple/app/main/Application.cpp

@@ -937,8 +940,15 @@ class ApplicationImp
            std::chrono::seconds {config_->getSize (siSweepInterval)});
    }

+    LedgerIndex getMaxLedger() override


I'm also in favor of a rename

wilsonianb · 2017-07-12T21:06:04Z

src/ripple/app/main/Application.cpp

+    if (!seq)
+        maxLedger_= 0;
+    else
+        maxLedger_ = *seq;


maxLedger_ is already initialized to 0, so do we need the else? setMaxLedger is currently only being a called once, but even if we added subsequent calls, it might make sense to leave maxLedger_ with its previous value if we can't look it up.

Otherwise, this could be

maxLedger_ = seq ? *seq : 0;

wilsonianb · 2017-07-12T21:16:40Z

src/ripple/app/consensus/RCLConsensus.cpp

@@ -843,7 +843,8 @@ RCLConsensus::validate(RCLCxLedger const& ledger, bool proposing)
    v->setTrusted();
    // suppress it if we receive it - FIXME: wrong suppression
    app_.getHashRouter().addSuppression(signingHash);
-    app_.getValidations().addValidation(v, "local");
+    if (! app_.getValidations().addValidation(v, "local"))


I'm wondering if we can move the getMaxLedger check to earlier in this function. It seems like we only need to be checking our own validations, not all received validations.

Are you suggesting we only need to prevent ourselves from issuing the in the past validations, since that would usually prevent receiving them from others? If so, I like the idea. You could eliminate the check inside addValidation and instead just check when considering whether to validate the consensus ledger https://github.com/mtrippled/rippled/blob/22536534936b9508f8b4dfc54d5abae869579e8e/src/ripple/app/consensus/RCLConsensus.cpp#L438

Or even simpler, just make it the condition for validating when you start the round https://github.com/mtrippled/rippled/blob/22536534936b9508f8b4dfc54d5abae869579e8e/src/ripple/app/consensus/RCLConsensus.cpp#L917

Fixed: it's now just part of the condition to be a validator.

codecov-io · 2017-07-17T15:14:22Z

Codecov Report

Merging #2167 into develop will increase coverage by <.01%.
The diff coverage is 94.44%.

@@             Coverage Diff             @@
##           develop    #2167      +/-   ##
===========================================
+ Coverage    69.87%   69.87%   +<.01%     
===========================================
  Files          689      689              
  Lines        50587    50599      +12     
===========================================
+ Hits         35347    35358      +11     
- Misses       15240    15241       +1

Impacted Files	Coverage Δ
src/ripple/app/main/Application.h	`100% <ø> (ø)`	⬆️
src/ripple/app/ledger/LedgerMaster.h	`83.33% <ø> (ø)`	⬆️
src/ripple/app/ledger/impl/LedgerMaster.cpp	`46.25% <100%> (ø)`	⬆️
src/ripple/app/consensus/RCLConsensus.cpp	`64.6% <100%> (-0.09%)`	⬇️
src/ripple/app/main/Application.cpp	`62.4% <92.3%> (+0.49%)`	⬆️

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 4308b12...e07fb7f. Read the comment docs.

bachase

One last nit but otherwise 👍

bachase · 2017-07-17T15:25:58Z

src/ripple/app/main/Application.h

@@ -171,6 +172,8 @@ class Application : public beast::PropertyStream::Source

    /** Retrieve the "wallet database" */
    virtual DatabaseCon& getWalletDB () = 0;
+


Consider documenting getMaxDisallowedLedger since it is public.

wilsonianb

👍 LGTM

seelabs · 2017-07-31T22:51:49Z

In 0.80.0-b4

mtrippled requested review from bachase, JoelKatz and wilsonianb July 11, 2017 09:33

mtrippled force-pushed the recover branch from 96dfa9a to 2253653 Compare July 11, 2017 18:09

bachase approved these changes Jul 11, 2017

View reviewed changes

mtrippled closed this Jul 11, 2017

mtrippled reopened this Jul 11, 2017

mtrippled force-pushed the recover branch from f7b088d to 2253653 Compare July 12, 2017 00:45

wilsonianb reviewed Jul 12, 2017

View reviewed changes

mtrippled force-pushed the recover branch from 2253653 to 180af0a Compare July 17, 2017 12:18

mtrippled closed this Jul 17, 2017

mtrippled reopened this Jul 17, 2017

bachase approved these changes Jul 17, 2017

View reviewed changes

mtrippled force-pushed the recover branch from b7e8226 to f976bec Compare July 17, 2017 16:55

wilsonianb approved these changes Jul 19, 2017

View reviewed changes

mtrippled mentioned this pull request Jul 20, 2017

Proposed 0.80.0-b2 #2184

Merged

Allow a newly-started validator to participate in consensus.

e07fb7f

mtrippled force-pushed the recover branch from f976bec to e07fb7f Compare July 21, 2017 23:36

mtrippled removed the request for review from JoelKatz July 21, 2017 23:37

mtrippled added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Jul 31, 2017

seelabs closed this Jul 31, 2017

mtrippled deleted the recover branch October 16, 2017 18:37

bachase mentioned this pull request Dec 18, 2017

Prefer ledger by branch #2300

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow a newly-started validator to participate #2167

Allow a newly-started validator to participate #2167

mtrippled commented Jul 11, 2017

bachase left a comment

bachase Jul 11, 2017

bachase Jul 11, 2017

mtrippled Jul 17, 2017

bachase Jul 11, 2017

mtrippled Jul 17, 2017

bachase Jul 11, 2017

wilsonianb Jul 12, 2017

mtrippled Jul 17, 2017

bachase Jul 11, 2017

wilsonianb Jul 12, 2017

mtrippled Jul 17, 2017

mtrippled commented Jul 11, 2017

wilsonianb Jul 12, 2017

wilsonianb Jul 12, 2017

wilsonianb Jul 12, 2017 •

edited

Loading

mtrippled Jul 17, 2017

wilsonianb Jul 12, 2017

bachase Jul 13, 2017

bachase Jul 13, 2017

mtrippled Jul 17, 2017

codecov-io commented Jul 17, 2017 •

edited

Loading

bachase left a comment

bachase Jul 17, 2017

mtrippled Jul 17, 2017

wilsonianb left a comment

seelabs commented Jul 31, 2017

		@@ -343,6 +344,8 @@ class LedgerMaster

		std::uint32_t fetch_seq_;

		LedgerIndex const max_ledger_difference_ {1000000};

		@@ -171,6 +172,8 @@ class Application : public beast::PropertyStream::Source

		/** Retrieve the "wallet database" */
		virtual DatabaseCon& getWalletDB () = 0;

Allow a newly-started validator to participate #2167

Allow a newly-started validator to participate #2167

Conversation

mtrippled commented Jul 11, 2017

bachase 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

mtrippled commented Jul 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilsonianb Jul 12, 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

codecov-io commented Jul 17, 2017 • edited Loading

Codecov Report

bachase left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilsonianb left a comment

Choose a reason for hiding this comment

seelabs commented Jul 31, 2017

wilsonianb Jul 12, 2017 •

edited

Loading

codecov-io commented Jul 17, 2017 •

edited

Loading