Remove bucketlist shadows, avoid resolving snapshots on publish #2205

marta-lokhova · 2019-07-19T21:49:24Z

This PR implements CAP-0025, and makes a few changes to how we publish and catchup. Main change is shadow removal from the bucketlist, which changes how buckets are merged (and hence, producing different buckets, which affects ledger header hash).

This change implies that on publish, it is no longer necessary to resolve future buckets. Moreover, it is not even necessary to store future buckets in half-merged state, since bucketlist can fully recover the merges by just using its currs and snaps.

This change also implements the upgrade mechanism. Specifically, upon the upgrade (which will be properly timed), in-progress merges are dropped, and new-style merges are started.

graydon

I'm going to need to revisit this (will try again tomorrow) but a preliminary scan looks plausible! I'm a little confused about the need to do the upgrade on a specific ledger boundary: why can't we just start doing new merges (after the upgrade) with the new logic? buckets all have their protocol version number written into them now so you can tell with a given input where it originated. I must not be getting something about this upgrade vs. the last one (which we didn't need to coordinate with a specific merge boundary).

src/catchup/ApplyBucketsWork.cpp

src/bucket/FutureBucket.cpp

src/bucket/BucketList.cpp

MonsieurNicolas

There is a quite a bit of churn in many places... I think I agree with what you told me offline that the better (and much simpler) solution might be to use bucket versions and just change FutureBucket::startMerge to not populate shadows if either input bucket is at protocol 12 or higher.

MonsieurNicolas · 2019-08-09T23:27:20Z

src/historywork/ResolveSnapshotWork.cpp

 }

 BasicWork::State
 ResolveSnapshotWork::onRun()
 {
+    if (mSnapshot->mLocalState.futuresAllClear())


why do we need to special case the "all clear" case?

Looks like futuresAllResolved already accounts for "all clear", so I think we don't need it. Removed.

MonsieurNicolas · 2019-08-09T23:31:16Z

src/herder/Upgrades.cpp

@@ -748,6 +752,13 @@ Upgrades::applyVersionUpgrade(AbstractLedgerTxn& ltx, uint32_t newVersion)
    {
        prepareLiabilities(ltx, header);
    }
+
+    if (header.current().ledgerVersion >= 12 && prevVersion < 12)


be consistent: use Bucket::FIRST_PROTOCOL_SHADOWS_REMOVED

MonsieurNicolas · 2019-08-09T23:31:34Z

src/history/HistoryArchive.cpp

+        if (clearFutureBuckets)
+        {
+            // Future buckets aren't needed in the archives
+            // post protocol 12.


use Bucket::FIRST_PROTOCOL_SHADOWS_REMOVED not 12

hmm, it's quite verbose that way. "Protocol 10" and "Protocol 11" are used consistently in the comments/tests throughout the codebase, I don't see how this is any different.

MonsieurNicolas · 2019-08-09T23:32:07Z

src/ledger/LedgerManager.h

@@ -106,7 +106,7 @@ class LedgerManager

    // return the HAS that corresponds to the last closed ledger as persisted in
    // the database
-    virtual HistoryArchiveState getLastClosedLedgerHAS() = 0;
+    virtual HistoryArchiveState getPersistentHAS() = 0;


why the rename?

there are two functions with very similar names, which are easy to confuse: HistoryManager::getLastClosedHistoryArchiveState and LedgerManager::getLastClosedLedgerHAS. The former returns HAS from an in-memory LCL, the latter queries the database for whatever last HAS was saved.

LastClosedLedger is used all over the code, calling it persistent makes it less clear on what it is

Oh I see now what you're saying. We should remove the method in HistoryManager instead

src/history/HistoryArchive.cpp

src/bucket/BucketList.cpp

src/catchup/ApplyBucketsWork.h

graydon

Out of time for today, but definitely liking the look of this! A few nits that are largely stylistic, plus one worrying thing concerning the semantic blurring happening between maxProtocolVersion and protocolVersion. Thanks for keeping at it, I'll try to return to it next week.

src/catchup/ApplyBucketsWork.cpp

src/history/HistoryArchive.cpp

src/ledger/LedgerManagerImpl.cpp

src/main/ApplicationUtils.cpp

src/bucket/BucketList.cpp

MonsieurNicolas

In general, I think there is an opportunity for simplification:
right now the logic to deal with new/old merge is scattered around the code by passing around booleans/state information, a better approach might be to see if we can instead derive the behavior from information we already have (such as presence or absence of shadows, or version in bucket).

We may be able to perform additional checks (invariants) by performing changes similar to what you did, but it may not be worth it; instead, it may be better to just have a new "checkInvariants" method at the bucket list level.

MonsieurNicolas · 2019-08-29T00:40:16Z

src/history/HistoryArchive.cpp

-    // We only ever write fully-resolved HASs to files, when making
-    // checkpoints. This may change in the future if we start publishing
-    // input-only HASs.
-    assert(futuresAllResolved());


shouldn't this check be performed conditionally? We still need to write those like that in protocol 11 and below

MonsieurNicolas · 2019-08-29T00:41:24Z

src/bucket/Bucket.cpp

+        if (!shadows.empty())
+        {
+            throw std::runtime_error(
+                "Protocol 12 or later does not support shadows");


12 or FIRST_PROTOCOL_SHADOWS_REMOVED ? pick one in the code base

MonsieurNicolas · 2019-08-29T00:42:20Z

src/bucket/Bucket.cpp

+    }
+    else
+    {
+        ++mc.mPreShadowRemovalProtocolMerges;


why do we need those counters? if it's for tests, maybe it's better to add them when you add tests

src/bucket/BucketList.cpp

MonsieurNicolas · 2019-09-03T22:55:49Z

src/catchup/ApplyBucketsWork.cpp

@@ -145,6 +149,35 @@ ApplyBucketsWork::startLevel()
 BasicWork::State
 ApplyBucketsWork::onRun()
 {
+    if (mResolveMerges && mDelayTimer)


maybe this should just be a different Work? I don't see why merges should be managed with Apply as they are unrelated.
Ideally what we can do is:

download & verify buckets

in parallel
a. resolve merges
b. apply buckets

so the way we can do this is simply:

download & verify buckets

start merges

apply buckets

wait for merges to be completed

MonsieurNicolas · 2019-09-03T23:02:01Z

src/catchup/CatchupWork.cpp

@@ -106,12 +107,27 @@ CatchupWork::alreadyHaveBucketsHistoryArchiveState(uint32_t atCheckpoint) const
 WorkSeqPtr
 CatchupWork::downloadApplyBuckets()
 {
+    // If first ledger we're going to replay is of older version (before shadow


(related to the other comment: we should probably move this out of the "apply buckets" method)

MonsieurNicolas · 2019-09-03T23:02:46Z

src/catchup/CatchupWork.cpp

@@ -106,12 +107,27 @@ CatchupWork::alreadyHaveBucketsHistoryArchiveState(uint32_t atCheckpoint) const
 WorkSeqPtr
 CatchupWork::downloadApplyBuckets()
 {
+    // If first ledger we're going to replay is of older version (before shadow
+    // removal), it'll perform old-style merges
+    auto ledgerVersion = mVerifiedLedgerRangeStart.header.ledgerVersion;


why not just always wait for merges regardless of protocol version? The danger of blocking the main thread exists also in the current version of the protocol, right?

src/history/HistoryArchive.cpp

MonsieurNicolas · 2019-09-03T23:46:23Z

src/history/HistoryManagerImpl.cpp

@@ -251,7 +242,18 @@ HistoryManagerImpl::maybeQueueHistoryCheckpoint()
 void
 HistoryManagerImpl::queueCurrentHistory()
 {
-    auto has = getLastClosedHistoryArchiveState();
+    // Subtle: depending on the protocol version,


related to my previous comment: I don't see why we need this here.
Basically: to queue things up we should never have to wait.

Later (actual publish), we can tell if we need to wait in ResolveSnapshotWork directly by for example seeing that we have shadows or not.

src/main/Config.cpp

marta-lokhova · 2019-09-12T02:57:47Z

Still got a version-related question/concern, to discuss offline

src/bucket/Bucket.cpp

MonsieurNicolas · 2019-09-12T23:40:28Z

src/history/HistoryArchive.cpp

+    {
+        auto& level = currentBuckets[i];
+        auto& prev = currentBuckets[i - 1];
+        Hash emptyHash;


const (emptyHash)

src/bucket/BucketList.cpp

graydon

Very close! Minor cosmetic changes for the most part, I reviewed 22/30 files, didn't get to tests yet. Will review tests this evening.

src/invariant/test/BucketListIsConsistentWithDatabaseTests.cpp

src/history/HistoryArchive.cpp

src/catchup/CatchupWork.cpp

src/bucket/BucketList.cpp

src/catchup/ApplyBucketsWork.cpp

graydon

A few more "comments, clarifications, slight reorganizations" in a few of the tests, but overall looking quite nearly done!

Thank you for all the patience and iteration: as you've seen every one of these bucket logic changes multiplies the overall subsystem complexity a lot, so I really appreciate that you've whittled this change down to its core and tested it thoroughly.

src/bucket/test/BucketTests.cpp

src/bucket/test/BucketManagerTests.cpp

src/history/test/HistoryTests.cpp

src/herder/test/UpgradesTests.cpp

…g catchup

graydon · 2019-09-16T22:12:01Z

@marta-lokhova Thanks for all the patient editing! Looks great. @MonsieurNicolas Anything left on your end?

MonsieurNicolas · 2019-09-16T23:14:56Z

r+ 119b651

…n_publish Remove bucketlist shadows, avoid resolving snapshots on publish Reviewed-by: MonsieurNicolas

eLmistiso approved these changes Jul 20, 2019

View reviewed changes

marta-lokhova force-pushed the avoid_resolving_buckets_on_publish branch from 503620b to 2b9cdfd Compare July 24, 2019 17:04

graydon reviewed Jul 25, 2019

View reviewed changes

src/catchup/ApplyBucketsWork.cpp Show resolved Hide resolved

src/catchup/ApplyBucketsWork.cpp Outdated Show resolved Hide resolved

src/bucket/FutureBucket.cpp Outdated Show resolved Hide resolved

src/bucket/BucketList.cpp Outdated Show resolved Hide resolved

MonsieurNicolas added this to In progress in v12.0.0 via automation Aug 9, 2019

MonsieurNicolas requested changes Aug 9, 2019

View reviewed changes

vogel reviewed Aug 14, 2019

View reviewed changes

src/history/HistoryArchive.cpp Outdated Show resolved Hide resolved

vogel reviewed Aug 14, 2019

View reviewed changes

src/bucket/BucketList.cpp Outdated Show resolved Hide resolved

vogel reviewed Aug 14, 2019

View reviewed changes

src/catchup/ApplyBucketsWork.h Outdated Show resolved Hide resolved

marta-lokhova force-pushed the avoid_resolving_buckets_on_publish branch 3 times, most recently from 8bb498a to 4eabb0e Compare August 23, 2019 22:39

graydon requested changes Aug 24, 2019

View reviewed changes

MonsieurNicolas requested changes Sep 3, 2019

View reviewed changes

MonsieurNicolas added the protocol label Sep 10, 2019

marta-lokhova force-pushed the avoid_resolving_buckets_on_publish branch from 4eabb0e to c4fd83a Compare September 12, 2019 02:56

marta-lokhova force-pushed the avoid_resolving_buckets_on_publish branch 2 times, most recently from 9dde0eb to a05acb0 Compare September 12, 2019 23:29

MonsieurNicolas reviewed Sep 13, 2019

View reviewed changes

graydon reviewed Sep 13, 2019

View reviewed changes

src/bucket/BucketList.cpp Outdated Show resolved Hide resolved

graydon reviewed Sep 13, 2019

View reviewed changes

src/bucket/BucketList.cpp Show resolved Hide resolved

graydon reviewed Sep 13, 2019

View reviewed changes

src/bucket/BucketList.cpp Show resolved Hide resolved

graydon requested changes Sep 13, 2019

View reviewed changes

marta-lokhova force-pushed the avoid_resolving_buckets_on_publish branch 2 times, most recently from c077255 to 3da7b56 Compare September 14, 2019 03:11

graydon requested changes Sep 14, 2019

View reviewed changes

marta-lokhova added 4 commits September 16, 2019 12:23

Bump protocol version

5dfb916

Stop enforcing futures resolvedness in HAS and add utility functions

52d53eb

Do not use shadows in new-style merges

6f0f424

Clear new-style future buckets for publish and validate buckets durin…

72181f1

…g catchup

marta-lokhova added 4 commits September 16, 2019 12:23

Wait for merges to resolve after bucket application

0153692

Test protocol 12 upgrade

901534e

Test publish and catchup after protocol upgrade

73a4e2e

Update BucketList tests to reflect shadow removal logic

119b651

marta-lokhova force-pushed the avoid_resolving_buckets_on_publish branch from 3da7b56 to 119b651 Compare September 16, 2019 19:25

graydon approved these changes Sep 16, 2019

View reviewed changes

latobarita added a commit that referenced this pull request Sep 16, 2019

Merge pull request #2205 from marta-lokhova/avoid_resolving_buckets_o…

1e4ebb5

…n_publish Remove bucketlist shadows, avoid resolving snapshots on publish Reviewed-by: MonsieurNicolas

latobarita merged commit 119b651 into stellar:master Sep 17, 2019

v12.0.0 automation moved this from In progress to Done Sep 17, 2019

This was referenced Sep 17, 2019

Fix rebuildLedgerFromBuckets command #2274

Merged

Consider removing shadows from BucketList #2175

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove bucketlist shadows, avoid resolving snapshots on publish #2205

Remove bucketlist shadows, avoid resolving snapshots on publish #2205

marta-lokhova commented Jul 19, 2019

graydon left a comment

MonsieurNicolas left a comment

MonsieurNicolas Aug 9, 2019

marta-lokhova Aug 19, 2019

MonsieurNicolas Aug 9, 2019

MonsieurNicolas Aug 9, 2019

marta-lokhova Aug 19, 2019

MonsieurNicolas Aug 9, 2019

marta-lokhova Aug 10, 2019

MonsieurNicolas Aug 10, 2019

MonsieurNicolas Aug 10, 2019

graydon left a comment

MonsieurNicolas left a comment

MonsieurNicolas Aug 29, 2019

MonsieurNicolas Aug 29, 2019

MonsieurNicolas Aug 29, 2019

MonsieurNicolas Sep 3, 2019

MonsieurNicolas Sep 3, 2019

MonsieurNicolas Sep 3, 2019

MonsieurNicolas Sep 3, 2019

marta-lokhova commented Sep 12, 2019

MonsieurNicolas Sep 12, 2019

graydon left a comment

graydon left a comment

graydon commented Sep 16, 2019

MonsieurNicolas commented Sep 16, 2019

Remove bucketlist shadows, avoid resolving snapshots on publish #2205

Remove bucketlist shadows, avoid resolving snapshots on publish #2205

Conversation

marta-lokhova commented Jul 19, 2019

graydon left a comment

Choose a reason for hiding this comment

MonsieurNicolas 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

graydon left a comment

Choose a reason for hiding this comment

MonsieurNicolas 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

marta-lokhova commented Sep 12, 2019

Choose a reason for hiding this comment

graydon left a comment

Choose a reason for hiding this comment

graydon left a comment

Choose a reason for hiding this comment

graydon commented Sep 16, 2019

MonsieurNicolas commented Sep 16, 2019