Skip to content

Commit

Permalink
Improve directory insertion & deletion (RIPD-1353, RIPD-1488):
Browse files Browse the repository at this point in the history 
This commit introduces the "SortedDirectories" amendment, which
addresses two distinct issues:

First, it corrects a technical flaw that could, in some edge cases,
prevent an empty intermediate page from being deleted.

Second, it sorts directory entries within a page (other than order
book page entries, which remain strictly FIFO). This makes insert
operations deterministic, instead of pseudo-random and reliant on
temporal ordering.

Lastly, it removes the ability to perform a "soft delete" where
the page number of the item to delete need not be known if the
item is in the first 20 pages, and enforces a maximum limit to
the number of pages that a directory can span.
  • Loading branch information
@nbougalis @seelabs
nbougalis authored and seelabs committed Jul 31, 2017
1 parent 3666948 commit 463b154
Show file tree
Hide file tree
Showing 24 changed files with 1,156 additions and 499 deletions.
4 changes: 4 additions & 0 deletions Builds/VisualStudio2015/RippleD.vcxproj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2117,6 +2117,10 @@
<ExcludedFromBuild Condition="'$(Configuration)|$(Platform)'=='debug|x64'">True</ExcludedFromBuild>
<ExcludedFromBuild Condition="'$(Configuration)|$(Platform)'=='release|x64'">True</ExcludedFromBuild>
</ClCompile>
<ClCompile Include="..\..\src\ripple\ledger\impl\ApplyView.cpp">
<ExcludedFromBuild Condition="'$(Configuration)|$(Platform)'=='debug|x64'">True</ExcludedFromBuild>
<ExcludedFromBuild Condition="'$(Configuration)|$(Platform)'=='release|x64'">True</ExcludedFromBuild>
</ClCompile>
<ClCompile Include="..\..\src\ripple\ledger\impl\ApplyViewBase.cpp">
<ExcludedFromBuild Condition="'$(Configuration)|$(Platform)'=='debug|x64'">True</ExcludedFromBuild>
<ExcludedFromBuild Condition="'$(Configuration)|$(Platform)'=='release|x64'">True</ExcludedFromBuild>
Expand Down
3 changes: 3 additions & 0 deletions Builds/VisualStudio2015/RippleD.vcxproj.filters
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2760,6 +2760,9 @@
<ClCompile Include="..\..\src\ripple\ledger\impl\ApplyStateTable.cpp">
<Filter>ripple\ledger\impl</Filter>
</ClCompile>
<ClCompile Include="..\..\src\ripple\ledger\impl\ApplyView.cpp">
<Filter>ripple\ledger\impl</Filter>
</ClCompile>
<ClCompile Include="..\..\src\ripple\ledger\impl\ApplyViewBase.cpp">
<Filter>ripple\ledger\impl</Filter>
</ClCompile>
Expand Down
3 changes: 2 additions & 1 deletion src/ripple/app/main/Amendments.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,8 @@ supportedAmendments ()
{ "86E83A7D2ECE3AD5FA87AB2195AE015C950469ABF0B72EAACED318F74886AE90 CryptoConditionsSuite" },
{ "42EEA5E28A97824821D4EF97081FE36A54E9593C6E4F20CBAE098C69D2E072DC fix1373" },
{ "DC9CA96AEA1DCF83E527D1AFC916EFAF5D27388ECA4060A88817C1238CAEE0BF EnforceInvariants" },
{ "3012E8230864E95A58C60FD61430D7E1B4D3353195F2981DC12B0C7C0950FFAC FlowCross" }
{ "3012E8230864E95A58C60FD61430D7E1B4D3353195F2981DC12B0C7C0950FFAC FlowCross" },
{ "CC5ABAE4F3EC92E94A59B1908C2BE82D2228B6485C00AFF8F22DF930D89C194E SortedDirectories" }
};
}

Expand Down
2 changes: 1 addition & 1 deletion src/ripple/app/tx/impl/CancelTicket.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ CancelTicket::doApply ()

auto viewJ = ctx_.app.journal ("View");
TER const result = dirDelete (ctx_.view (), false, hint,
getOwnerDirIndex (ticket_owner), ticketId, false, (hint == 0), viewJ);
keylet::ownerDir (ticket_owner), ticketId, false, (hint == 0), viewJ);

adjustOwnerCount(view(), view().peek(
keylet::account(ticket_owner)), -1, viewJ);
Expand Down
112 changes: 56 additions & 56 deletions src/ripple/app/tx/impl/CreateOffer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1291,74 +1291,74 @@ CreateOffer::applyGuts (Sandbox& sb, Sandbox& sbCancel)
// We need to place the remainder of the offer into its order book.
auto const offer_index = getOfferIndex (account_, uSequence);

std::uint64_t uOwnerNode;

// Add offer to owner's directory.
std::tie(result, std::ignore) = dirAdd(sb, uOwnerNode,
keylet::ownerDir (account_), offer_index,
describeOwnerDir (account_), viewJ);
auto const ownerNode = dirAdd(sb, keylet::ownerDir (account_),
offer_index, false, describeOwnerDir (account_), viewJ);

if (result == tesSUCCESS)
if (!ownerNode)
{
// Update owner count.
adjustOwnerCount(sb, sleCreator, 1, viewJ);

JLOG (j_.trace()) <<
"adding to book: " << to_string (saTakerPays.issue ()) <<
" : " << to_string (saTakerGets.issue ());
JLOG (j_.debug()) <<
"final result: failed to add offer to owner's directory";
return { tecDIR_FULL, true };
}

Book const book { saTakerPays.issue(), saTakerGets.issue() };
std::uint64_t uBookNode;
bool isNewBook;
// Update owner count.
adjustOwnerCount(sb, sleCreator, 1, viewJ);

// Add offer to order book, using the original rate
// before any crossing occured.
auto dir = keylet::quality (keylet::book (book), uRate);
JLOG (j_.trace()) <<
"adding to book: " << to_string (saTakerPays.issue ()) <<
" : " << to_string (saTakerGets.issue ());

std::tie(result, isNewBook) = dirAdd (sb, uBookNode,
dir, offer_index, [&](SLE::ref sle)
{
sle->setFieldH160 (sfTakerPaysCurrency,
saTakerPays.issue().currency);
sle->setFieldH160 (sfTakerPaysIssuer,
saTakerPays.issue().account);
sle->setFieldH160 (sfTakerGetsCurrency,
saTakerGets.issue().currency);
sle->setFieldH160 (sfTakerGetsIssuer,
saTakerGets.issue().account);
sle->setFieldU64 (sfExchangeRate, uRate);
}, viewJ);

if (result == tesSUCCESS)
{
auto sleOffer = std::make_shared<SLE>(ltOFFER, offer_index);
sleOffer->setAccountID (sfAccount, account_);
sleOffer->setFieldU32 (sfSequence, uSequence);
sleOffer->setFieldH256 (sfBookDirectory, dir.key);
sleOffer->setFieldAmount (sfTakerPays, saTakerPays);
sleOffer->setFieldAmount (sfTakerGets, saTakerGets);
sleOffer->setFieldU64 (sfOwnerNode, uOwnerNode);
sleOffer->setFieldU64 (sfBookNode, uBookNode);
if (expiration)
sleOffer->setFieldU32 (sfExpiration, *expiration);
if (bPassive)
sleOffer->setFlag (lsfPassive);
if (bSell)
sleOffer->setFlag (lsfSell);
sb.insert(sleOffer);
Book const book { saTakerPays.issue(), saTakerGets.issue() };

if (isNewBook)
ctx_.app.getOrderBookDB().addOrderBook(book);
}
}
// Add offer to order book, using the original rate
// before any crossing occured.
auto dir = keylet::quality (keylet::book (book), uRate);
bool const bookExisted = static_cast<bool>(sb.peek (dir));

if (result != tesSUCCESS)
auto const bookNode = dirAdd (sb, dir, offer_index, true,
[&](SLE::ref sle)
{
sle->setFieldH160 (sfTakerPaysCurrency,
saTakerPays.issue().currency);
sle->setFieldH160 (sfTakerPaysIssuer,
saTakerPays.issue().account);
sle->setFieldH160 (sfTakerGetsCurrency,
saTakerGets.issue().currency);
sle->setFieldH160 (sfTakerGetsIssuer,
saTakerGets.issue().account);
sle->setFieldU64 (sfExchangeRate, uRate);
}, viewJ);

if (!bookNode)
{
JLOG (j_.debug()) <<
"final result: " << transToken (result);
"final result: failed to add offer to book";
return { tecDIR_FULL, true };
}

return { result, true };
auto sleOffer = std::make_shared<SLE>(ltOFFER, offer_index);
sleOffer->setAccountID (sfAccount, account_);
sleOffer->setFieldU32 (sfSequence, uSequence);
sleOffer->setFieldH256 (sfBookDirectory, dir.key);
sleOffer->setFieldAmount (sfTakerPays, saTakerPays);
sleOffer->setFieldAmount (sfTakerGets, saTakerGets);
sleOffer->setFieldU64 (sfOwnerNode, *ownerNode);
sleOffer->setFieldU64 (sfBookNode, *bookNode);
if (expiration)
sleOffer->setFieldU32 (sfExpiration, *expiration);
if (bPassive)
sleOffer->setFlag (lsfPassive);
if (bSell)
sleOffer->setFlag (lsfSell);
sb.insert(sleOffer);

if (!bookExisted)
ctx_.app.getOrderBookDB().addOrderBook(book);

JLOG (j_.debug()) << "final result: success";

return { tesSUCCESS, true };
}

TER
Expand Down
23 changes: 10 additions & 13 deletions src/ripple/app/tx/impl/CreateTicket.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -99,27 +99,24 @@ CreateTicket::doApply ()
sleTicket->setAccountID (sfTarget, target_account);
}

std::uint64_t hint;

auto viewJ = ctx_.app.journal ("View");

auto result = dirAdd(view(), hint, keylet::ownerDir (account_),
sleTicket->key(), describeOwnerDir (account_), viewJ);
auto const page = dirAdd(view(), keylet::ownerDir (account_),
sleTicket->key(), false, describeOwnerDir (account_), viewJ);

JLOG(j_.trace()) <<
"Creating ticket " << to_string (sleTicket->key()) <<
": " << transHuman (result.first);
": " << (page ? "success" : "failure");

if (result.first == tesSUCCESS)
{
sleTicket->setFieldU64(sfOwnerNode, hint);
if (!page)
return tecDIR_FULL;

// If we succeeded, the new entry counts agains the
// creator's reserve.
adjustOwnerCount(view(), sle, 1, viewJ);
}
sleTicket->setFieldU64(sfOwnerNode, *page);

return result.first;
// If we succeeded, the new entry counts agains the
// creator's reserve.
adjustOwnerCount(view(), sle, 1, viewJ);
return tesSUCCESS;
}

}
16 changes: 7 additions & 9 deletions src/ripple/app/tx/impl/Escrow.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -255,13 +255,11 @@ EscrowCreate::doApply()

// Add escrow to owner directory
{
uint64_t page;
auto result = dirAdd(ctx_.view(), page,
keylet::ownerDir(account), slep->key(),
describeOwnerDir(account), ctx_.app.journal ("View"));
if (! isTesSuccess(result.first))
return result.first;
(*slep)[sfOwnerNode] = page;
auto page = dirAdd(ctx_.view(), keylet::ownerDir(account), slep->key(),
false, describeOwnerDir(account), ctx_.app.journal ("View"));
if (!page)
return tecDIR_FULL;
(*slep)[sfOwnerNode] = *page;
}

// Deduct owner's balance, increment owner count
Expand Down Expand Up @@ -431,7 +429,7 @@ EscrowFinish::doApply()
{
auto const page = (*slep)[sfOwnerNode];
TER const ter = dirDelete(ctx_.view(), true,
page, keylet::ownerDir(account).key,
page, keylet::ownerDir(account),
k.key, false, page == 0, ctx_.app.journal ("View"));
if (! isTesSuccess(ter))
return ter;
Expand Down Expand Up @@ -497,7 +495,7 @@ EscrowCancel::doApply()
{
auto const page = (*slep)[sfOwnerNode];
TER const ter = dirDelete(ctx_.view(), true,
page, keylet::ownerDir(account).key,
page, keylet::ownerDir(account),
k.key, false, page == 0, ctx_.app.journal ("View"));
if (! isTesSuccess(ter))
return ter;
Expand Down
14 changes: 6 additions & 8 deletions src/ripple/app/tx/impl/PayChan.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ closeChannel (
// Remove PayChan from owner directory
{
auto const page = (*slep)[sfOwnerNode];
TER const ter = dirDelete (view, true, page, keylet::ownerDir (src).key,
TER const ter = dirDelete (view, true, page, keylet::ownerDir (src),
key, false, page == 0, j);
if (!isTesSuccess (ter))
return ter;
Expand Down Expand Up @@ -239,13 +239,11 @@ PayChanCreate::doApply()

// Add PayChan to owner directory
{
uint64_t page;
auto result = dirAdd (ctx_.view (), page, keylet::ownerDir (account),
slep->key (), describeOwnerDir (account),
ctx_.app.journal ("View"));
if (!isTesSuccess (result.first))
return result.first;
(*slep)[sfOwnerNode] = page;
auto page = dirAdd (ctx_.view(), keylet::ownerDir(account), slep->key(),
false, describeOwnerDir (account), ctx_.app.journal ("View"));
if (!page)
return tecDIR_FULL;
(*slep)[sfOwnerNode] = *page;
}

// Deduct owner's balance, increment owner count
Expand Down
25 changes: 11 additions & 14 deletions src/ripple/app/tx/impl/SetSignerList.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -236,24 +236,21 @@ SetSignerList::replaceSignerList ()

auto viewJ = ctx_.app.journal ("View");
// Add the signer list to the account's directory.
std::uint64_t hint;

auto result = dirAdd(ctx_.view (), hint, ownerDirKeylet,
signerListKeylet.key, describeOwnerDir (account_), viewJ);
auto page = dirAdd(ctx_.view (), ownerDirKeylet,
signerListKeylet.key, false, describeOwnerDir (account_), viewJ);

JLOG(j_.trace()) << "Create signer list for account " <<
toBase58(account_) << ": " << transHuman (result.first);
toBase58(account_) << ": " << (page ? "success" : "failure");

if (result.first == tesSUCCESS)
{
signerList->setFieldU64 (sfOwnerNode, hint);
if (!page)
return tecDIR_FULL;

// If we succeeded, the new entry counts against the
// creator's reserve.
adjustOwnerCount(view(), sle, addedOwnerCount, viewJ);
}
signerList->setFieldU64 (sfOwnerNode, *page);

return result.first;
// If we succeeded, the new entry counts against the
// creator's reserve.
adjustOwnerCount(view(), sle, addedOwnerCount, viewJ);
return tesSUCCESS;
}

TER
Expand Down Expand Up @@ -293,7 +290,7 @@ SetSignerList::removeSignersFromLedger (Keylet const& accountKeylet,

auto viewJ = ctx_.app.journal ("View");
TER const result = dirDelete(ctx_.view(), false, hint,
ownerDirKeylet.key, signerListKeylet.key, false, (hint == 0), viewJ);
ownerDirKeylet, signerListKeylet.key, false, (hint == 0), viewJ);

if (result == tesSUCCESS)
adjustOwnerCount(view(),
Expand Down
Loading

0 comments on commit 463b154

Please sign in to comment.