From c7c1b3cc3b7547504ae4d91ff6c41af18c3f264b Mon Sep 17 00:00:00 2001 From: Nik Bougalis Date: Fri, 25 Aug 2017 02:15:19 -0700 Subject: [PATCH] Track escrow in recipient's owner directory (RIPD-1523): Introduce "fix1523" which corrects a minor technical flaw with the original implementation of the escrow feature. When creating an escrow, the entry would only be tracked in the owner directory of the sender; as a result, an escrow recipient would not be able to detect incoming escrows without monitoring the ledger in real-time for transactions of interest or without the sender communicating this information out of band. With the fix in place, escrows where the recipient differs from the sender will be listed in the recipient's owner directory as well. --- src/ripple/app/main/Amendments.cpp | 3 +- src/ripple/app/tx/impl/Escrow.cpp | 38 ++++- src/ripple/ledger/Directory.h | 3 - src/ripple/ledger/impl/Directory.cpp | 32 ---- src/ripple/protocol/Feature.h | 46 ++--- src/ripple/protocol/SField.h | 1 + src/ripple/protocol/impl/Feature.cpp | 1 + src/ripple/protocol/impl/LedgerFormats.cpp | 3 +- src/ripple/protocol/impl/SField.cpp | 17 +- src/test/app/Escrow_test.cpp | 185 +++++++++++++++++++-- 10 files changed, 251 insertions(+), 78 deletions(-) diff --git a/src/ripple/app/main/Amendments.cpp b/src/ripple/app/main/Amendments.cpp index 7e8c5ac4c85..5407bad02d8 100644 --- a/src/ripple/app/main/Amendments.cpp +++ b/src/ripple/app/main/Amendments.cpp @@ -58,7 +58,8 @@ supportedAmendments () { "3012E8230864E95A58C60FD61430D7E1B4D3353195F2981DC12B0C7C0950FFAC FlowCross" }, { "CC5ABAE4F3EC92E94A59B1908C2BE82D2228B6485C00AFF8F22DF930D89C194E SortedDirectories" }, { "B4D44CC3111ADD964E846FC57760C8B50FFCD5A82C86A72756F6B058DDDF96AD fix1201" }, - { "6C92211186613F9647A89DFFBAB8F94C99D4C7E956D495270789128569177DA1 fix1512" } + { "6C92211186613F9647A89DFFBAB8F94C99D4C7E956D495270789128569177DA1 fix1512" }, + { "B9E739B8296B4A1BB29BE990B17D66E21B62A300A909F25AC55C22D6C72E1F9D fix1523" } }; } diff --git a/src/ripple/app/tx/impl/Escrow.cpp b/src/ripple/app/tx/impl/Escrow.cpp index 3b9abf0149b..5600993beab 100644 --- a/src/ripple/app/tx/impl/Escrow.cpp +++ b/src/ripple/app/tx/impl/Escrow.cpp @@ -253,7 +253,7 @@ EscrowCreate::doApply() ctx_.view().insert(slep); - // Add escrow to owner directory + // Add escrow to sender's owner directory { auto page = dirAdd(ctx_.view(), keylet::ownerDir(account), slep->key(), false, describeOwnerDir(account), ctx_.app.journal ("View")); @@ -262,6 +262,21 @@ EscrowCreate::doApply() (*slep)[sfOwnerNode] = *page; } + // If it's not a self-send, add escrow to recipient's owner directory. + if (ctx_.view ().rules().enabled(fix1523)) + { + auto const dest = ctx_.tx[sfDestination]; + + if (dest != ctx_.tx[sfAccount]) + { + auto page = dirAdd(ctx_.view(), keylet::ownerDir(dest), slep->key(), + false, describeOwnerDir(dest), ctx_.app.journal ("View")); + if (!page) + return tecDIR_FULL; + (*slep)[sfDestinationNode] = *page; + } + } + // Deduct owner's balance, increment owner count (*sle)[sfBalance] = (*sle)[sfBalance] - ctx_.tx[sfAmount]; (*sle)[sfOwnerCount] = (*sle)[sfOwnerCount] + 1; @@ -435,6 +450,16 @@ EscrowFinish::doApply() return ter; } + // Remove escrow from recipient's owner directory, if present. + if (ctx_.view ().rules().enabled(fix1523) && (*slep)[~sfDestinationNode]) + { + TER const ter = dirDelete(ctx_.view(), true, + (*slep)[sfDestinationNode], keylet::ownerDir((*slep)[sfDestination]), + k.key, false, false, ctx_.app.journal ("View")); + if (! isTesSuccess(ter)) + return ter; + } + // NOTE: These payments cannot be used to fund accounts // Fetch Destination SLE @@ -488,7 +513,6 @@ EscrowCancel::doApply() ctx_.view().info().parentCloseTime.time_since_epoch().count() <= (*slep)[sfCancelAfter]) return tecNO_PERMISSION; - AccountID const account = (*slep)[sfAccount]; // Remove escrow from owner directory @@ -501,6 +525,16 @@ EscrowCancel::doApply() return ter; } + // Remove escrow from recipient's owner directory, if present. + if (ctx_.view ().rules().enabled(fix1523) && (*slep)[~sfDestinationNode]) + { + TER const ter = dirDelete(ctx_.view(), true, + (*slep)[sfDestinationNode], keylet::ownerDir((*slep)[sfDestination]), + k.key, false, false, ctx_.app.journal ("View")); + if (! isTesSuccess(ter)) + return ter; + } + // Transfer amount back to owner, decrement owner count auto const sle = ctx_.view().peek( keylet::account(account)); diff --git a/src/ripple/ledger/Directory.h b/src/ripple/ledger/Directory.h index e2a1a381e7f..2ded082a971 100644 --- a/src/ripple/ledger/Directory.h +++ b/src/ripple/ledger/Directory.h @@ -44,9 +44,6 @@ class Dir const_iterator end() const; - - const_iterator - find(uint256 const& page_key, uint256 const& sle_key) const; }; class Dir::const_iterator diff --git a/src/ripple/ledger/impl/Directory.cpp b/src/ripple/ledger/impl/Directory.cpp index b52738336fc..d1b4b4c95b4 100644 --- a/src/ripple/ledger/impl/Directory.cpp +++ b/src/ripple/ledger/impl/Directory.cpp @@ -60,38 +60,6 @@ Dir::end() const -> return const_iterator(*view_, root_, root_); } -const_iterator -Dir::find(uint256 const& page_key, uint256 const& sle_key) const -{ - if (sle_ == nullptr) - return end(); - - auto it = const_iterator(*view_, root_, keylet::page(page_key, 0)); - if (root_.key == page_key) - { - it.sle_ = sle_; - it.indexes_ = indexes_; - } - else - { - it.sle_ = view_->read(it.page_); - if (it.sle_ == nullptr) - { - it.page_ = root_; - return it; - } - it.indexes_ = &it.sle_->getFieldV256(sfIndexes); - } - - it.it_ = std::find(std::begin(*it.indexes_), - std::end(*it.indexes_), sle_key); - if (it.it_ == std::end(*it.indexes_)) - return end(); - - it.index_ = *it.it_; - return it; -} - bool const_iterator::operator==(const_iterator const& other) const { diff --git a/src/ripple/protocol/Feature.h b/src/ripple/protocol/Feature.h index 4a70a88a72f..9f99a87ff01 100644 --- a/src/ripple/protocol/Feature.h +++ b/src/ripple/protocol/Feature.h @@ -49,27 +49,30 @@ namespace detail { class FeatureCollections { static constexpr char const* const featureNames[] = - {"MultiSign", - "Tickets", - "TrustSetAuth", - "FeeEscalation", - "OwnerPaysFee", - "CompareFlowV1V2", - "SHAMapV2", - "PayChan", - "Flow", - "CompareTakerFlowCross", - "FlowCross", - "CryptoConditions", - "TickSize", - "fix1368", - "Escrow", - "CryptoConditionsSuite", - "fix1373", - "EnforceInvariants", - "SortedDirectories", - "fix1201", - "fix1512"}; + { + "MultiSign", + "Tickets", + "TrustSetAuth", + "FeeEscalation", + "OwnerPaysFee", + "CompareFlowV1V2", + "SHAMapV2", + "PayChan", + "Flow", + "CompareTakerFlowCross", + "FlowCross", + "CryptoConditions", + "TickSize", + "fix1368", + "Escrow", + "CryptoConditionsSuite", + "fix1373", + "EnforceInvariants", + "SortedDirectories", + "fix1201", + "fix1512", + "fix1523" + }; std::vector features; boost::container::flat_map featureToIndex; @@ -164,6 +167,7 @@ extern uint256 const featureEnforceInvariants; extern uint256 const featureSortedDirectories; extern uint256 const fix1201; extern uint256 const fix1512; +extern uint256 const fix1523; } // ripple diff --git a/src/ripple/protocol/SField.h b/src/ripple/protocol/SField.h index 83205b086ab..232cb4a8532 100644 --- a/src/ripple/protocol/SField.h +++ b/src/ripple/protocol/SField.h @@ -387,6 +387,7 @@ extern SF_U64 const sfBaseFee; extern SF_U64 const sfExchangeRate; extern SF_U64 const sfLowNode; extern SF_U64 const sfHighNode; +extern SF_U64 const sfDestinationNode; // 128-bit extern SF_U128 const sfEmailHash; diff --git a/src/ripple/protocol/impl/Feature.cpp b/src/ripple/protocol/impl/Feature.cpp index dbda491a090..f7f10b1322e 100644 --- a/src/ripple/protocol/impl/Feature.cpp +++ b/src/ripple/protocol/impl/Feature.cpp @@ -116,5 +116,6 @@ uint256 const featureEnforceInvariants = *getRegisteredFeature("EnforceInvariant uint256 const featureSortedDirectories = *getRegisteredFeature("SortedDirectories"); uint256 const fix1201 = *getRegisteredFeature("fix1201"); uint256 const fix1512 = *getRegisteredFeature("fix1512"); +uint256 const fix1523 = *getRegisteredFeature("fix1523"); } // ripple diff --git a/src/ripple/protocol/impl/LedgerFormats.cpp b/src/ripple/protocol/impl/LedgerFormats.cpp index d900906c8d9..9dac61ba2fc 100644 --- a/src/ripple/protocol/impl/LedgerFormats.cpp +++ b/src/ripple/protocol/impl/LedgerFormats.cpp @@ -98,7 +98,8 @@ LedgerFormats::LedgerFormats () SOElement (sfDestinationTag, SOE_OPTIONAL) << SOElement (sfOwnerNode, SOE_REQUIRED) << SOElement (sfPreviousTxnID, SOE_REQUIRED) << - SOElement (sfPreviousTxnLgrSeq, SOE_REQUIRED); + SOElement (sfPreviousTxnLgrSeq, SOE_REQUIRED) << + SOElement (sfDestinationNode, SOE_OPTIONAL); add ("LedgerHashes", ltLEDGER_HASHES) << SOElement (sfFirstLedgerSequence, SOE_OPTIONAL) // Remove if we do a ledger restart diff --git a/src/ripple/protocol/impl/SField.cpp b/src/ripple/protocol/impl/SField.cpp index 691c1bb8dac..a73dcd950e8 100644 --- a/src/ripple/protocol/impl/SField.cpp +++ b/src/ripple/protocol/impl/SField.cpp @@ -132,14 +132,15 @@ SF_U32 const sfSignerListID = make::one(&sfSignerListID, SF_U32 const sfSettleDelay = make::one(&sfSettleDelay, STI_UINT32, 39, "SettleDelay"); // 64-bit integers -SF_U64 const sfIndexNext = make::one(&sfIndexNext, STI_UINT64, 1, "IndexNext"); -SF_U64 const sfIndexPrevious = make::one(&sfIndexPrevious, STI_UINT64, 2, "IndexPrevious"); -SF_U64 const sfBookNode = make::one(&sfBookNode, STI_UINT64, 3, "BookNode"); -SF_U64 const sfOwnerNode = make::one(&sfOwnerNode, STI_UINT64, 4, "OwnerNode"); -SF_U64 const sfBaseFee = make::one(&sfBaseFee, STI_UINT64, 5, "BaseFee"); -SF_U64 const sfExchangeRate = make::one(&sfExchangeRate, STI_UINT64, 6, "ExchangeRate"); -SF_U64 const sfLowNode = make::one(&sfLowNode, STI_UINT64, 7, "LowNode"); -SF_U64 const sfHighNode = make::one(&sfHighNode, STI_UINT64, 8, "HighNode"); +SF_U64 const sfIndexNext = make::one(&sfIndexNext, STI_UINT64, 1, "IndexNext"); +SF_U64 const sfIndexPrevious = make::one(&sfIndexPrevious, STI_UINT64, 2, "IndexPrevious"); +SF_U64 const sfBookNode = make::one(&sfBookNode, STI_UINT64, 3, "BookNode"); +SF_U64 const sfOwnerNode = make::one(&sfOwnerNode, STI_UINT64, 4, "OwnerNode"); +SF_U64 const sfBaseFee = make::one(&sfBaseFee, STI_UINT64, 5, "BaseFee"); +SF_U64 const sfExchangeRate = make::one(&sfExchangeRate, STI_UINT64, 6, "ExchangeRate"); +SF_U64 const sfLowNode = make::one(&sfLowNode, STI_UINT64, 7, "LowNode"); +SF_U64 const sfHighNode = make::one(&sfHighNode, STI_UINT64, 8, "HighNode"); +SF_U64 const sfDestinationNode = make::one(&sfDestinationNode, STI_UINT64, 9, "DestinationNode"); // 128-bit SF_U128 const sfEmailHash = make::one(&sfEmailHash, STI_HASH128, 1, "EmailHash"); diff --git a/src/test/app/Escrow_test.cpp b/src/test/app/Escrow_test.cpp index adb861a7717..f8df897d2ec 100644 --- a/src/test/app/Escrow_test.cpp +++ b/src/test/app/Escrow_test.cpp @@ -20,10 +20,13 @@ #include #include #include +#include #include #include #include #include +#include +#include namespace ripple { namespace test { @@ -120,6 +123,17 @@ struct Escrow_test : public beast::unit_test::suite return jv; } + static + Json::Value + lockup (jtx::Account const& account, jtx::Account const& to, + STAmount const& amount, NetClock::time_point const& expiry, + NetClock::time_point const& cancel) + { + Json::Value jv = lockup (account, to, amount, expiry); + jv["CancelAfter"] = cancel.time_since_epoch().count(); + return jv; + } + static Json::Value lockup (jtx::Account const& account, jtx::Account const& to, @@ -654,7 +668,7 @@ struct Escrow_test : public beast::unit_test::suite { return env.now() + d; }; env.fund(XRP(5000), "alice", "bob", "carol"); - std::array cb = + std::array cb = {{ 0xA2, 0x2B, 0x80, 0x20, 0x42, 0x4A, 0x70, 0x49, 0x49, 0x52, 0x92, 0x67, 0xB6, 0x21, 0xB3, 0xD7, 0x91, 0x19, 0xD7, 0x29, @@ -671,18 +685,169 @@ struct Escrow_test : public beast::unit_test::suite } void - testMeta() + testMetaAndOwnership() { - testcase ("Metadata"); - using namespace jtx; using namespace std::chrono; - Env env(*this, with_features(featureEscrow)); - env.fund(XRP(5000), "alice", "bob", "carol"); - env(condpay("alice", "carol", XRP(1000), makeSlice(cb1), env.now() + 1s)); - auto const m = env.meta(); - BEAST_EXPECT((*m)[sfTransactionResult] == tesSUCCESS); + auto const alice = Account("alice"); + auto const bruce = Account("bruce"); + auto const carol = Account("carol"); + + { + testcase ("Metadata & Ownership (without fix1523)"); + Env env(*this, with_features(featureEscrow)); + env.fund(XRP(5000), alice, bruce, carol); + auto const seq = env.seq(alice); + + env(lockup(alice, carol, XRP(1000), env.now() + 1s)); + + BEAST_EXPECT((*env.meta())[sfTransactionResult] == tesSUCCESS); + + auto const escrow = env.le(keylet::escrow(alice.id(), seq)); + BEAST_EXPECT(escrow); + + ripple::Dir aod (*env.current(), keylet::ownerDir(alice.id())); + BEAST_EXPECT(std::distance(aod.begin(), aod.end()) == 1); + BEAST_EXPECT(std::find(aod.begin(), aod.end(), escrow) != aod.end()); + + ripple::Dir cod (*env.current(), keylet::ownerDir(carol.id())); + BEAST_EXPECT(cod.begin() == cod.end()); + } + + { + testcase ("Metadata (with fix1523, to self)"); + + Env env(*this, with_features(featureEscrow, fix1523)); + env.fund(XRP(5000), alice, bruce, carol); + auto const aseq = env.seq(alice); + auto const bseq = env.seq(bruce); + + env(lockup(alice, alice, XRP(1000), env.now() + 1s, env.now() + 500s)); + BEAST_EXPECT((*env.meta())[sfTransactionResult] == tesSUCCESS); + env.close(5s); + auto const aa = env.le(keylet::escrow(alice.id(), aseq)); + BEAST_EXPECT(aa); + + { + ripple::Dir aod(*env.current(), keylet::ownerDir(alice.id())); + BEAST_EXPECT(std::distance(aod.begin(), aod.end()) == 1); + BEAST_EXPECT(std::find(aod.begin(), aod.end(), aa) != aod.end()); + } + + env(lockup(bruce, bruce, XRP(1000), env.now() + 1s, env.now() + 2s)); + BEAST_EXPECT((*env.meta())[sfTransactionResult] == tesSUCCESS); + env.close(5s); + auto const bb = env.le(keylet::escrow(bruce.id(), bseq)); + BEAST_EXPECT(bb); + + { + ripple::Dir bod(*env.current(), keylet::ownerDir(bruce.id())); + BEAST_EXPECT(std::distance(bod.begin(), bod.end()) == 1); + BEAST_EXPECT(std::find(bod.begin(), bod.end(), bb) != bod.end()); + } + + env.close(5s); + env(finish(alice, alice, aseq)); + { + BEAST_EXPECT(!env.le(keylet::escrow(alice.id(), aseq))); + BEAST_EXPECT((*env.meta())[sfTransactionResult] == tesSUCCESS); + + ripple::Dir aod(*env.current(), keylet::ownerDir(alice.id())); + BEAST_EXPECT(std::distance(aod.begin(), aod.end()) == 0); + BEAST_EXPECT(std::find(aod.begin(), aod.end(), aa) == aod.end()); + + ripple::Dir bod(*env.current(), keylet::ownerDir(bruce.id())); + BEAST_EXPECT(std::distance(bod.begin(), bod.end()) == 1); + BEAST_EXPECT(std::find(bod.begin(), bod.end(), bb) != bod.end()); + } + + env.close(5s); + env(cancel(bruce, bruce, bseq)); + { + BEAST_EXPECT(!env.le(keylet::escrow(bruce.id(), bseq))); + BEAST_EXPECT((*env.meta())[sfTransactionResult] == tesSUCCESS); + + ripple::Dir bod(*env.current(), keylet::ownerDir(bruce.id())); + BEAST_EXPECT(std::distance(bod.begin(), bod.end()) == 0); + BEAST_EXPECT(std::find(bod.begin(), bod.end(), bb) == bod.end()); + } + } + + { + testcase ("Metadata (with fix1523, to other)"); + + Env env(*this, with_features(featureEscrow, fix1523)); + env.fund(XRP(5000), alice, bruce, carol); + auto const aseq = env.seq(alice); + auto const bseq = env.seq(bruce); + + env(lockup(alice, bruce, XRP(1000), env.now() + 1s)); + BEAST_EXPECT((*env.meta())[sfTransactionResult] == tesSUCCESS); + env.close(5s); + env(lockup(bruce, carol, XRP(1000), env.now() + 1s, env.now() + 2s)); + BEAST_EXPECT((*env.meta())[sfTransactionResult] == tesSUCCESS); + env.close(5s); + + auto const ab = env.le(keylet::escrow(alice.id(), aseq)); + BEAST_EXPECT(ab); + + auto const bc = env.le(keylet::escrow(bruce.id(), bseq)); + BEAST_EXPECT(bc); + + { + ripple::Dir aod(*env.current(), keylet::ownerDir(alice.id())); + BEAST_EXPECT(std::distance(aod.begin(), aod.end()) == 1); + BEAST_EXPECT(std::find(aod.begin(), aod.end(), ab) != aod.end()); + + ripple::Dir bod(*env.current(), keylet::ownerDir(bruce.id())); + BEAST_EXPECT(std::distance(bod.begin(), bod.end()) == 2); + BEAST_EXPECT(std::find(bod.begin(), bod.end(), ab) != bod.end()); + BEAST_EXPECT(std::find(bod.begin(), bod.end(), bc) != bod.end()); + + ripple::Dir cod(*env.current(), keylet::ownerDir(carol.id())); + BEAST_EXPECT(std::distance(cod.begin(), cod.end()) == 1); + BEAST_EXPECT(std::find(cod.begin(), cod.end(), bc) != cod.end()); + } + + env.close(5s); + env(finish(alice, alice, aseq)); + { + BEAST_EXPECT(!env.le(keylet::escrow(alice.id(), aseq))); + BEAST_EXPECT(env.le(keylet::escrow(bruce.id(), bseq))); + + ripple::Dir aod(*env.current(), keylet::ownerDir(alice.id())); + BEAST_EXPECT(std::distance(aod.begin(), aod.end()) == 0); + BEAST_EXPECT(std::find(aod.begin(), aod.end(), ab) == aod.end()); + + ripple::Dir bod(*env.current(), keylet::ownerDir(bruce.id())); + BEAST_EXPECT(std::distance(bod.begin(), bod.end()) == 1); + BEAST_EXPECT(std::find(bod.begin(), bod.end(), ab) == bod.end()); + BEAST_EXPECT(std::find(bod.begin(), bod.end(), bc) != bod.end()); + + ripple::Dir cod(*env.current(), keylet::ownerDir(carol.id())); + BEAST_EXPECT(std::distance(cod.begin(), cod.end()) == 1); + } + + env.close(5s); + env(cancel(bruce, bruce, bseq)); + { + BEAST_EXPECT(!env.le(keylet::escrow(alice.id(), aseq))); + BEAST_EXPECT(!env.le(keylet::escrow(bruce.id(), bseq))); + + ripple::Dir aod(*env.current(), keylet::ownerDir(alice.id())); + BEAST_EXPECT(std::distance(aod.begin(), aod.end()) == 0); + BEAST_EXPECT(std::find(aod.begin(), aod.end(), ab) == aod.end()); + + ripple::Dir bod(*env.current(), keylet::ownerDir(bruce.id())); + BEAST_EXPECT(std::distance(bod.begin(), bod.end()) == 0); + BEAST_EXPECT(std::find(bod.begin(), bod.end(), ab) == bod.end()); + BEAST_EXPECT(std::find(bod.begin(), bod.end(), bc) == bod.end()); + + ripple::Dir cod(*env.current(), keylet::ownerDir(carol.id())); + BEAST_EXPECT(std::distance(cod.begin(), cod.end()) == 0); + } + } } void testConsequences() @@ -745,7 +910,7 @@ struct Escrow_test : public beast::unit_test::suite testFails(); testLockup(); testEscrowConditions(); - testMeta(); + testMetaAndOwnership(); testConsequences(); } };