Refactor ledger replay logic (RIPD-1547) #2477

bachase · 2018-04-11T13:45:52Z

This extracts the ledger replay logic into a standalone function for future reuse.

@ximinez I'm tagging you to review the middle commit, which is the change we had discussed to use a ReadView for the TxQ update.

sublimator · 2018-04-11T13:51:25Z

You used to be able to start a testnet seeded from a json ledger dump. I tried this a month or so ago and it no longer worked. It would go from say ledger n to 2 ... rather than n+1 ... With issues arising from that ...

ripplelabs-jenkins · 2018-04-11T14:41:44Z

Jenkins Build Summary

Built from this commit

Built at 20180416 - 14:32:25

Test Results

Build Type	Log	Result	Status
docs	logfile	1 cases, 0 failed, t: 2s	PASS ✅
clang.debug.unity	logfile	1022 cases, 0 failed, t: 251s	PASS ✅
coverage	logfile	1022 cases, 0 failed, t: 619s	PASS ✅
clang.debug.nounity	logfile	1020 cases, 0 failed, t: 247s	PASS ✅
clang.debug.unity, PARALLEL_TESTS=false	logfile	1022 cases, 0 failed, t: 488s	PASS ✅
gcc.debug.nounity	logfile	1020 cases, 0 failed, t: 103s	PASS ✅
gcc.debug.unity	logfile	1022 cases, 0 failed, t: 244s	PASS ✅
clang.release.unity	logfile	1021 cases, 0 failed, t: 329s	PASS ✅
msvc.debug	logfile	1018 cases, 0 failed, t: 565s	PASS ✅
gcc.release.unity	logfile	1021 cases, 0 failed, t: 349s	PASS ✅
gcc.debug.unity -Dstatic=true	logfile	1022 cases, 0 failed, t: 243s	PASS ✅
msvc.debug, PROJECT_NAME=rippled_classic	logfile	1018 cases, 0 failed, t: 1179s	PASS ✅
msvc.release	logfile	1017 cases, 0 failed, t: 506s	PASS ✅

scottschurr · 2018-04-11T20:56:39Z

I'm not sure what's going on with the Travis and AppVeyor CI failures. It looks like there's some sort of collision going between this branch, which is based on -b3, and -b4, which removed BeastConfig.h. The specific failure is:

/home/travis/build/ripple/rippled/src/ripple/app/ledger/impl/BuildLedger.cpp:20:25: fatal error: BeastConfig.h: No such file or directory

Maybe rebasing to -b4 would help? I dunno...

bachase · 2018-04-12T13:11:20Z

Rebased on 1.0.0-b4. 🤦‍♂️ for missing that initially.

codecov-io · 2018-04-12T13:47:49Z

Codecov Report

Merging #2477 into develop will increase coverage by 0.02%.
The diff coverage is 84.11%.

@@            Coverage Diff             @@
##           develop   #2477      +/-   ##
==========================================
+ Coverage    70.67%   70.7%   +0.02%     
==========================================
  Files          696     699       +3     
  Lines        54436   54454      +18     
==========================================
+ Hits         38473   38500      +27     
+ Misses       15963   15954       -9

Impacted Files	Coverage Δ
src/ripple/app/ledger/LedgerMaster.h	`100% <ø> (+16.66%)`	⬆️
src/ripple/app/consensus/RCLConsensus.h	`80% <ø> (ø)`	⬆️
src/ripple/app/misc/TxQ.h	`97.05% <ø> (ø)`	⬆️
src/ripple/app/misc/impl/TxQ.cpp	`95.75% <ø> (ø)`	⬆️
src/ripple/app/misc/NetworkOPs.cpp	`63.64% <0%> (-0.05%)`	⬇️
src/ripple/app/main/Application.cpp	`58.55% <0%> (+0.38%)`	⬆️
src/ripple/app/ledger/LedgerReplay.h	`100% <100%> (ø)`
src/ripple/app/ledger/impl/LedgerReplay.cpp	`100% <100%> (ø)`
src/ripple/app/ledger/impl/BuildLedger.cpp	`86.11% <86.11%> (ø)`
src/ripple/app/consensus/RCLConsensus.cpp	`65.34% <92.85%> (-1.89%)`	⬇️
... and 6 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 1eece9b...4b778ea. Read the comment docs.

HowardHinnant

Nice job.

nbougalis · 2018-04-12T20:10:59Z

src/ripple/app/ledger/LedgerReplay.h

+
+}  // namespace ripple
+
+#endif


pico-nit: newline here.

nbougalis

Nice. Left minor comments; nothing major.

nbougalis · 2018-04-12T20:20:31Z

src/ripple/app/consensus/RCLConsensus.cpp

-                applyTransaction(
-                    app_, accum, *tx.second, false, tapNO_CHECK_SIGN, j_);
+            assert(replayData->parent()->info().hash == previousLedger.id());
+            return buildLedger(*replayData, tapNO_CHECK_SIGN, app_, j_);
        }
        else


I'd remove the else here. You have a return in the if part of the branch.

nbougalis · 2018-04-12T20:32:31Z

src/ripple/app/ledger/LedgerReplay.h

+{
+    std::shared_ptr<Ledger const> parent_;
+    std::shared_ptr<Ledger const> replay_;
+    std::map<int, std::shared_ptr<STTx const>> orderedTxns_;


The key should be std::uint32_t (the type of sfTransactionIndex).

nbougalis · 2018-04-12T20:34:27Z

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

+
+/** Apply a set of consensus transactions to a ledger.
+
+  Typically the txFilter is used to reject transactions


There no longer is a txFilter argument passed to this function, so this comment is out of date.

scottschurr

👍 Nice cleanup! I especially like that you can pass a ReadView to TxQ::processClosedLedger. I left a few thoughts to consider, but spotted no real mistakes.

scottschurr · 2018-04-12T19:20:54Z

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

+/** Apply a set of consensus transactions to a ledger.
+
+  Typically the txFilter is used to reject transactions
+  that already accepted in the prior ledger.


Stale comment? txFilter is no longer an argument.

scottschurr · 2018-04-12T19:43:51Z

src/ripple/app/consensus/RCLConsensus.cpp

-        assert(!accum.open());
-        if (replay)
+    std::shared_ptr<Ledger> buildLCL = [&]() {
+        auto replayData = ledgerMaster_.releaseReplay();


auto const?

scottschurr · 2018-04-12T20:05:36Z

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

+{
+    auto buildLCL = std::make_shared<Ledger>(*parent, closeTime);
+
+    auto const v2_enabled = buildLCL->rules().enabled(featureSHAMapV2);


I'd go ahead and declare v2_enabled as bool. And if you brace initialize it then narrowing is disabled. So if someone changes the return type of enabled() the compiler will catch them.

scottschurr · 2018-04-12T20:09:49Z

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

+
+        int asf = buildLCL->stateMap().flushDirty(
+            hotACCOUNT_NODE, buildLCL->info().seq);
+        int tmf = buildLCL->txMap().flushDirty(


int consts?

scottschurr · 2018-04-12T20:31:03Z

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

+    for (auto const& item : replay_->txMap())
+    {
+        auto txPair = replay_->txRead(item.key());
+        auto txIndex = (*txPair.second)[sfTransactionIndex];


auto const?

scottschurr · 2018-04-12T20:33:36Z

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

+    {
+        auto txPair = replay_->txRead(item.key());
+        auto txIndex = (*txPair.second)[sfTransactionIndex];
+        orderedTxns_.emplace(txIndex, txPair.first);


This may be a hair more efficient if we move the shared_ptr in txPair.first. Consider...

orderedTxns_.emplace(txIndex, std::move (txPair.first));

The auto makes that possible optimization invisible in the text, eh? Tradeoffs....

scottschurr · 2018-04-12T20:50:00Z

src/ripple/app/main/Application.cpp

-                auto txID = item.key();
-                auto txPair = replayLedger->txRead (txID);
-                auto txIndex = (*txPair.second)[sfTransactionIndex];
+                std::shared_ptr<STTx const> const & tx = it.second;


Pico nit: space between const and & is atypical?

scottschurr · 2018-04-12T20:56:39Z

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

+    auto buildLCL = std::make_shared<Ledger>(*parent, closeTime);
+
+    auto const v2_enabled = buildLCL->rules().enabled(featureSHAMapV2);
+    auto v2_transition = false;


Looks like v2_transition is unused?

scottschurr · 2018-04-12T21:14:46Z

src/test/app/LedgerReplay_test.cpp

+        auto lastClosedParent =
+            ledgerMaster.getLedgerByHash(lastClosed->info().parentHash);
+
+        auto replayed = buildLedger(


lastClosed, lastClosedParent, and replayed can all be const I think...

scottschurr

👍 Looks great to me! Thanks.

Also switch to use ReadView for TxQ updates.

ximinez

👍 I did a light review of the entire PR. Looks good. I'm still surprised the TxQ change actually works. 😀

ximinez · 2018-04-27T21:21:26Z

src/test/app/LedgerReplay_test.cpp

+            env.app(),
+            env.journal);
+
+        BEAST_EXPECT(replayed->info().hash == lastClosed->info().hash);


I like this test.

seelabs · 2018-05-16T02:32:06Z

In 1.1.0-b1

bachase assigned HowardHinnant, ximinez and scottschurr Apr 11, 2018

bachase requested review from HowardHinnant, ximinez and scottschurr April 11, 2018 13:45

bachase force-pushed the replay branch from 8d4a19d to c3e528c Compare April 12, 2018 13:08

HowardHinnant approved these changes Apr 12, 2018

View reviewed changes

nbougalis reviewed Apr 12, 2018

View reviewed changes

nbougalis approved these changes Apr 12, 2018

View reviewed changes

scottschurr approved these changes Apr 12, 2018

View reviewed changes

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

Refactor ledger replay logic (RIPD-1547):

4b778ea

Also switch to use ReadView for TxQ updates.

bachase force-pushed the replay branch from a8dd448 to 4b778ea Compare April 16, 2018 13:15

ximinez approved these changes Apr 27, 2018

View reviewed changes

seelabs closed this May 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor ledger replay logic (RIPD-1547) #2477

Refactor ledger replay logic (RIPD-1547) #2477

bachase commented Apr 11, 2018

sublimator commented Apr 11, 2018 via email

ripplelabs-jenkins commented Apr 11, 2018 •

edited

Loading

scottschurr commented Apr 11, 2018

bachase commented Apr 12, 2018

codecov-io commented Apr 12, 2018 •

edited

Loading

HowardHinnant left a comment

nbougalis Apr 12, 2018

nbougalis left a comment

nbougalis Apr 12, 2018

nbougalis Apr 12, 2018

nbougalis Apr 12, 2018

scottschurr left a comment

scottschurr Apr 12, 2018

scottschurr Apr 12, 2018

scottschurr Apr 12, 2018

scottschurr Apr 12, 2018

scottschurr Apr 12, 2018

scottschurr Apr 12, 2018

scottschurr Apr 12, 2018

scottschurr Apr 12, 2018

scottschurr Apr 12, 2018

scottschurr left a comment

ximinez left a comment

ximinez Apr 27, 2018

seelabs commented May 16, 2018


		/** Apply a set of consensus transactions to a ledger.

		Typically the txFilter is used to reject transactions

Refactor ledger replay logic (RIPD-1547) #2477

Refactor ledger replay logic (RIPD-1547) #2477

Conversation

bachase commented Apr 11, 2018

sublimator commented Apr 11, 2018 via email

ripplelabs-jenkins commented Apr 11, 2018 • edited Loading

Jenkins Build Summary

Test Results

scottschurr commented Apr 11, 2018

bachase commented Apr 12, 2018

codecov-io commented Apr 12, 2018 • edited Loading

Codecov Report

HowardHinnant left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbougalis 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottschurr left a comment

Choose a reason for hiding this comment

ximinez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seelabs commented May 16, 2018

ripplelabs-jenkins commented Apr 11, 2018 •

edited

Loading

codecov-io commented Apr 12, 2018 •

edited

Loading