Add Shard Family #3448

miguelportilla · 2020-06-10T18:11:52Z

The App Family utilizes a single shared Tree Node and Full Below cache for all shards. This can create a problem when acquiring a shard that shares an account state node that was recently cached from another shard operation. The new Shard Family class solves this issue by managing separate Tree Node and Full Below caches for each shard.

The shard acquire process in LedgerMaster and InboundLedgers was improved with considering of the acquire reason.

HowardHinnant · 2020-06-23T19:19:12Z

src/ripple/shamap/Family.h

@@ -34,35 +34,42 @@ class Family
 public:
    virtual ~Family() = default;



While we're in this area, I recommend deleting the two copy members (as you've done in the derived classes already). This will further require explicitly defaulting the default constructor.

HowardHinnant

Nice job.

Left one nitpicky suggestion.

MarcelRaschke

approved changes ✅

miguelportilla · 2020-06-23T22:20:14Z

Thank you, @MarcelRaschke

* Optimize parsing of compressed message headers * Enforce protocol-defined message size maxima * Update comments

*Add majority timer configuration *FIXES: XRPLF#3396

Slice should, eventually, be replaced by std::string_view<std::uint8_t> so begin adding some helpful functions to align its interface.

* Improve error reporting (more readable exception messages) * Reduce function complexity (split oversized function to smaller pieces) * Reduce code duplication * Reduce buffer copying

scottschurr

👍 Looks good. I left some comments for your consideration, but nothing that would need to block merging. The pointer handling is no riskier than it was before this pull request 😉

scottschurr · 2020-06-23T23:50:42Z

src/ripple/app/ledger/InboundLedger.h

@@ -142,8 +142,9 @@ class InboundLedger final : public PeerSet,



While digging around getting myself oriented in this code I noticed that TreeNodeCache.h includes two unnecessary lines. They don't hurt anything because the include guard works. But I was wondering if you would mind removing the spare lines in this pull request while you're vaguely in the area. The lines that could be removed are:

#include <ripple/shamap/TreeNodeCache.h>

and

class SHAMapAbstractNode;

Thanks for your consideration.

I have a suggestion that may slightly improve usability of enum class Reason declared on line 51.

Uses of this enum currently look like this:

InboundLedger::Reason::GENERIC

It's not hard to make this a little shorter and easier to read. Consider adding the following static constexpr member declarations:

// Retain the sturdiness of an enum class, but make the enum members // less verbose to use. static constexpr auto forHISTORY = Reason::HISTORY; static constexpr auto forSHARD = Reason::SHARD; static constexpr auto forGENERIC = Reason::GENERIC; static constexpr auto forCONSENSUS = Reason::CONSENSUS;

The old uses of the enum can remain, but they can also be shortened to this:

InboundLedger::forCONSENSUS

I find that a little easier to read. Just for your consideration. forGeneric reads a little oddly, but I think the style works well for the other three.

If you choose to do this I'd suggest also switching all of the old-style uses of the enum to the shorter style. This will encourage future users to adopt the shorter style.

scottschurr · 2020-06-24T00:10:55Z

src/ripple/app/ledger/impl/InboundLedger.cpp

            mHaveHeader = false;
            mHaveTransactions = false;
            mHaveState = false;
            mLedger.reset();
-            tryDB(*app_.shardFamily());
+
+            tryDB(app_.shardFamily()->db());


Application.h doesn't offer any assurances that a call to Application::shardFamily will not return a nullptr. If there's a reason we don't need to guard for that perhaps it deserves a comment, either here or in Application.h.

Shards remain an optional feature. If configured, the shard store pointer is created in the Application ctor and is valid for the Application's object lifetime. It is the responsibility of the caller to check the validity of the pointer. The Application ctor has a comment but documentation could be improved, I'll add some.

In the InboundLedger(s) classes, we can safely assume the shard store pointer to be valid when the reason is SHARD. The initiating code in LedgerMaster, (the only code to assign this reason) only does so if the shard store pointer is valid. We also assert in InboundLedgers::acquire on the reason being SHARD with an invalid shard store pointer.

scottschurr · 2020-06-24T01:15:31Z

src/ripple/app/ledger/impl/LedgerMaster.cpp

+            std::min(ledgerIndex, shardStore->lastLedgerSeq(shardIndex));
+    }
+
+    auto haveHash{getLedgerHashForHistory(ledgerIndex, reason)};


haveHash could be const.

scottschurr · 2020-06-24T01:15:37Z

src/ripple/app/ledger/impl/LedgerMaster.cpp

@@ -735,7 +735,18 @@ LedgerMaster::tryFill(Job& job, std::shared_ptr<Ledger const> ledger)
 void
 LedgerMaster::getFetchPack(LedgerIndex missing, InboundLedger::Reason reason)
 {
-    auto haveHash{getLedgerHashForHistory(missing + 1, reason)};
+    LedgerIndex ledgerIndex{missing + 1};


FWIW, ledgerIndex could be made const by turning the following if into a lambda. It may not be worth it. Your call.

Will do, I prefer const when possible.

scottschurr · 2020-06-24T01:34:14Z

src/ripple/nodestore/impl/Shard.cpp

                    // All ledgers have been acquired, shard is complete
-                    acquireInfo_.reset();


There's a comment referring to acquireInfo_ in Shard.h line 211 that...

// If the shard is complete, this member will be null.

Is that comment still accurate with the acquireInfo_.reset() calls removed here and on line 293? I don't know, but I thought I would ask.

Nice catch! The comment should read If the shard is final, this member will be null.

scottschurr · 2020-06-24T22:16:30Z

src/ripple/shamap/NodeFamily.h

+        return false;
+    }
+
+    std::shared_ptr<FullBelowCache> getFullBelowCache(std::uint32_t) override


See comment in Family.h about the ignored parameter.

scottschurr · 2020-06-24T22:16:54Z

src/ripple/shamap/NodeFamily.h

+        return fbCache_;
+    }
+
+    std::shared_ptr<TreeNodeCache> getTreeNodeCache(std::uint32_t) override


See comment in Family.h about the ignored parameter.

scottschurr · 2020-06-24T23:19:56Z

src/ripple/shamap/impl/SHAMap.cpp


    if (!child)
    {
+        auto const& childHash = parent->getChildHash(branch);


Good spotting, reducing the scope.

scottschurr · 2020-06-25T00:05:19Z

src/ripple/shamap/impl/ShardFamily.cpp

+    , tnTargetSize_(app.config().getValueFor(SizedItem::treeCacheSize))
+    , tnTargetAge_(app.config().getValueFor(SizedItem::treeCacheAge))
+{
+    assert(app.getShardStore());


If you hit this assert I think you already had undefined behavior, since you assigned the contents of a nullptr to a reference. So all bets were already off. The assert may or may not fire.

Since you're in a .cpp file, you can work around this with the following trick...

static NodeStore::Database& getShardStore (Application& app) { auto const dbPtr = app.getShardStore(); assert (dbPtr); return *dbPtr; } ShardFamily::ShardFamily(Application& app, CollectorManager& cm) : app_(app) , db_(getShardStore(app)) , cm_(cm) , j_(app.journal("ShardFamily")) , tnTargetSize_(app.config().getValueFor(SizedItem::treeCacheSize)) , tnTargetAge_(app.config().getValueFor(SizedItem::treeCacheAge)) { }

scottschurr · 2020-06-25T00:22:19Z

src/ripple/shamap/impl/ShardFamily.cpp

+{
+    JLOG(j_.error()) << "Missing node in ledger sequence " << seq;
+
+    // Prevent recursive invocation


I'm guessing you don't really mean "recursive" here. If it were recursive then I think you'd need a std:::recursive_mutex, which you're not using.

evaluations for validated ledger age.

* Document delete_batch, back_off_milliseconds, age_threshold_seconds. * Convert those time values to chrono types. * Fix bug that ignored age_threshold_seconds. * Add a "recovery buffer" to the config that gives the node a chance to recover before aborting online delete. * Add begin/end log messages around the SQL queries. * Add a new configuration section: [sqlite] to allow tuning the sqlite database operations. Ignored on full/large history servers. * Update documentation of [node_db] and [sqlite] in the rippled-example.cfg file. * Resolves XRPLF#3321

* This was always a stub and enabled no functional changes

Corrects the public_key parameter name in the comment. See XRPLF/xrpl-dev-portal#854 for context.

*The tecUNFUNDED code is actively used when attempting to create payment channels; the messages incorrectly list it as deprecated. Meanwhile, the tecUNFUNDED_ADD code actually is an unused legacy code, dating back to when there was a WalletAdd transactor. *Engine result messages are not part of the binary format and are documented as subject to change without notice, so this should not require an amendment nor a new API version. *Mark terLAST, terFUNDS_SPENT deprecated

* Fixes XRPLF#3269 * Work on a version 2 of the XRP Network API has begun. The new API returns the code `notSynced` in place of `noClosed`, `noCurrent`, and `noNetwork`. And `invalidParams` is returned in place of `lgrIdxInvalid`. * The version 2 API can be specified by adding "api_version" = 2 to your json request. The default version remains 1 (if unspecified), except for the command line interface which tracks version 2. * It can be re-enabled by setting ApiMaximumSupportedVersion to 2.

This commit, if merged, adds support to allow multiple indepedent nodes to produce a binary identical shard for a given range of ledgers. The advantage is that servers can use content-addressable storage, and can more efficiently retrieve shards by downloading from multiple peers at once and then verifying the integrity of a shard by cross-checking its checksum with the checksum other servers report.

miguelportilla force-pushed the acquire_family branch 4 times, most recently from d8d2c52 to 32e033a Compare June 12, 2020 14:04

miguelportilla requested a review from HowardHinnant June 12, 2020 17:19

miguelportilla assigned HowardHinnant Jun 12, 2020

miguelportilla requested a review from scottschurr June 12, 2020 17:20

miguelportilla assigned scottschurr Jun 12, 2020

HowardHinnant reviewed Jun 23, 2020

View reviewed changes

HowardHinnant approved these changes Jun 23, 2020

View reviewed changes

MarcelRaschke approved these changes Jun 23, 2020

View reviewed changes

gregtatcam and others added 4 commits June 24, 2020 13:51

Improve compression support:

11271c2

* Optimize parsing of compressed message headers * Enforce protocol-defined message size maxima * Update comments

Use base 10 for majority vote calculation:

ea0bcdd

*Add majority timer configuration *FIXES: XRPLF#3396

Improve Slice:

69c7523

Slice should, eventually, be replaced by std::string_view<std::uint8_t> so begin adding some helpful functions to align its interface.

Cleanup SHAMap and simplify interfaces:

9be4b3e

* Improve error reporting (more readable exception messages) * Reduce function complexity (split oversized function to smaller pieces) * Reduce code duplication * Reduce buffer copying

scottschurr approved these changes Jun 25, 2020

View reviewed changes

carlhua added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Jun 25, 2020

carlhua removed the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Jun 25, 2020

miguelportilla force-pushed the acquire_family branch from 7711db0 to 8a2a12c Compare June 25, 2020 17:26

mtrippled and others added 9 commits June 25, 2020 10:49

Make server_info report consistent with internal

230a2f8

evaluations for validated ledger age.

Update cmake so that rippled can build as a submodule

be0b23b

Fix a build issue caused by pip3 being unavailable

46bb011

Remove CryptoConditionsSuite amendment:

d12f92b

* This was always a stub and enabled no functional changes

crawl_shards comment fix

f006fbf

Corrects the public_key parameter name in the comment. See XRPLF/xrpl-dev-portal#854 for context.

Add comments to protobuf files

5563d59

Improve handling of empty buffer in varint parsing (RIPD-1683)

96c4da8

HowardHinnant and others added 5 commits June 25, 2020 10:49

Improve received node process

6acf35d

Use NuDB version 2.0.3

50ce9b2

Improve ledgermaster shard acquire

cc34b09

miguelportilla force-pushed the acquire_family branch from 8a2a12c to 65b43a6 Compare June 25, 2020 18:52

miguelportilla added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Jun 25, 2020

This was referenced Jun 25, 2020

Proposed 1.6.0-b8 #3459

Closed

Proposed 1.6.0-b8 #3461

Closed

Add Shard Family

d4f91d3

miguelportilla force-pushed the acquire_family branch from 65b43a6 to d4f91d3 Compare June 26, 2020 11:59

This was referenced Jun 26, 2020

Proposed 1.6.0-b8 #3462

Closed

Proposed 1.6.0-b8 #3483

Merged

manojsdoshi closed this in #3483 Jun 30, 2020

miguelportilla deleted the acquire_family branch August 11, 2020 17:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Shard Family #3448

Add Shard Family #3448

miguelportilla commented Jun 10, 2020 •

edited

Loading

HowardHinnant Jun 23, 2020

HowardHinnant left a comment

MarcelRaschke left a comment

miguelportilla commented Jun 23, 2020

scottschurr left a comment

scottschurr Jun 23, 2020

scottschurr Jun 24, 2020

scottschurr Jun 24, 2020

miguelportilla Jun 25, 2020

scottschurr Jun 24, 2020

scottschurr Jun 24, 2020

miguelportilla Jun 25, 2020 •

edited

Loading

scottschurr Jun 24, 2020

miguelportilla Jun 25, 2020 •

edited

Loading

scottschurr Jun 24, 2020

scottschurr Jun 24, 2020

scottschurr Jun 24, 2020

scottschurr Jun 25, 2020

scottschurr Jun 25, 2020

		@@ -34,35 +34,42 @@ class Family
		public:
		virtual ~Family() = default;

		@@ -142,8 +142,9 @@ class InboundLedger final : public PeerSet,

		// All ledgers have been acquired, shard is complete
		acquireInfo_.reset();

Add Shard Family #3448

Add Shard Family #3448

Conversation

miguelportilla commented Jun 10, 2020 • edited Loading

Choose a reason for hiding this comment

HowardHinnant left a comment

Choose a reason for hiding this comment

MarcelRaschke left a comment

Choose a reason for hiding this comment

miguelportilla commented Jun 23, 2020

scottschurr 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

miguelportilla Jun 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miguelportilla Jun 25, 2020 • 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

miguelportilla commented Jun 10, 2020 •

edited

Loading

miguelportilla Jun 25, 2020 •

edited

Loading

miguelportilla Jun 25, 2020 •

edited

Loading