-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor ledger replay logic (RIPD-1547) #2477
Conversation
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 ...
|
Jenkins Build SummaryBuilt from this commit Built at 20180416 - 14:32:25 Test Results
|
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
Maybe rebasing to -b4 would help? I dunno... |
Rebased on 1.0.0-b4. 🤦♂️ for missing that initially. |
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job.
|
||
} // namespace ripple | ||
|
||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pico-nit: newline here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. Left minor comments; nothing major.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd remove the else
here. You have a return
in the if
part of the branch.
src/ripple/app/ledger/LedgerReplay.h
Outdated
{ | ||
std::shared_ptr<Ledger const> parent_; | ||
std::shared_ptr<Ledger const> replay_; | ||
std::map<int, std::shared_ptr<STTx const>> orderedTxns_; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The key should be std::uint32_t
(the type of sfTransactionIndex
).
|
||
/** Apply a set of consensus transactions to a ledger. | ||
|
||
Typically the txFilter is used to reject transactions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There no longer is a txFilter
argument passed to this function, so this comment is out of date.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 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.
/** Apply a set of consensus transactions to a ledger. | ||
|
||
Typically the txFilter is used to reject transactions | ||
that already accepted in the prior ledger. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stale comment? txFilter
is no longer an argument.
assert(!accum.open()); | ||
if (replay) | ||
std::shared_ptr<Ledger> buildLCL = [&]() { | ||
auto replayData = ledgerMaster_.releaseReplay(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
auto const?
{ | ||
auto buildLCL = std::make_shared<Ledger>(*parent, closeTime); | ||
|
||
auto const v2_enabled = buildLCL->rules().enabled(featureSHAMapV2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
||
int asf = buildLCL->stateMap().flushDirty( | ||
hotACCOUNT_NODE, buildLCL->info().seq); | ||
int tmf = buildLCL->txMap().flushDirty( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
int consts?
for (auto const& item : replay_->txMap()) | ||
{ | ||
auto txPair = replay_->txRead(item.key()); | ||
auto txIndex = (*txPair.second)[sfTransactionIndex]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
auto const?
{ | ||
auto txPair = replay_->txRead(item.key()); | ||
auto txIndex = (*txPair.second)[sfTransactionIndex]; | ||
orderedTxns_.emplace(txIndex, txPair.first); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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....
src/ripple/app/main/Application.cpp
Outdated
auto txID = item.key(); | ||
auto txPair = replayLedger->txRead (txID); | ||
auto txIndex = (*txPair.second)[sfTransactionIndex]; | ||
std::shared_ptr<STTx const> const & tx = it.second; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pico nit: space between const
and &
is atypical?
auto buildLCL = std::make_shared<Ledger>(*parent, closeTime); | ||
|
||
auto const v2_enabled = buildLCL->rules().enabled(featureSHAMapV2); | ||
auto v2_transition = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like v2_transition
is unused?
src/test/app/LedgerReplay_test.cpp
Outdated
auto lastClosedParent = | ||
ledgerMaster.getLedgerByHash(lastClosed->info().parentHash); | ||
|
||
auto replayed = buildLedger( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lastClosed
, lastClosedParent
, and replayed
can all be const
I think...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks great to me! Thanks.
Also switch to use ReadView for TxQ updates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I did a light review of the entire PR. Looks good. I'm still surprised the TxQ
change actually works. 😀
env.app(), | ||
env.journal); | ||
|
||
BEAST_EXPECT(replayed->info().hash == lastClosed->info().hash); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this test.
In 1.1.0-b1 |
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 theTxQ
update.