From e33ac1d45090891bbb38ad96817a96f6106c8583 Mon Sep 17 00:00:00 2001 From: seelabs Date: Fri, 9 Aug 2019 13:03:27 -0700 Subject: [PATCH] Add PayChan to recipient's owner directory --- src/ripple/app/tx/impl/PayChan.cpp | 25 ++++- src/ripple/protocol/Feature.h | 2 + src/ripple/protocol/impl/Feature.cpp | 2 + src/ripple/protocol/impl/LedgerFormats.cpp | 1 + src/test/app/PayChan_test.cpp | 121 +++++++++++++++++++++ src/test/rpc/AccountTx_test.cpp | 4 +- 6 files changed, 152 insertions(+), 3 deletions(-) diff --git a/src/ripple/app/tx/impl/PayChan.cpp b/src/ripple/app/tx/impl/PayChan.cpp index 5be551db34..3f9027b05c 100644 --- a/src/ripple/app/tx/impl/PayChan.cpp +++ b/src/ripple/app/tx/impl/PayChan.cpp @@ -135,6 +135,19 @@ closeChannel ( auto const page = (*slep)[sfOwnerNode]; if (! view.dirRemove(keylet::ownerDir(src), page, key, true)) { + JLOG (j.fatal()) << "Could not remove paychan from src owner directory"; + return tefBAD_LEDGER; + } + } + + // Remove PayChan from recipient's owner directory, if present. + if (auto const page = (*slep)[~sfDestinationNode]; + page && view.rules().enabled(fixPayChanRecipientOwnerDir)) + { + auto const dst = (*slep)[sfDestination]; + if (!view.dirRemove(keylet::ownerDir(dst), *page, key, true)) + { + JLOG (j.fatal()) << "Could not remove paychan from dst owner directory"; return tefBAD_LEDGER; } } @@ -245,13 +258,23 @@ PayChanCreate::doApply() // Add PayChan to owner directory { - auto page = dirAdd (ctx_.view(), keylet::ownerDir(account), slep->key(), + auto const page = dirAdd (ctx_.view(), keylet::ownerDir(account), slep->key(), false, describeOwnerDir (account), ctx_.app.journal ("View")); if (!page) return tecDIR_FULL; (*slep)[sfOwnerNode] = *page; } + // Add PayChan to the recipient's owner directory + if (ctx_.view().rules().enabled(fixPayChanRecipientOwnerDir)) + { + auto const page = dirAdd(ctx_.view(), keylet::ownerDir(dst), slep->key(), + false, describeOwnerDir(dst), 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]; adjustOwnerCount(ctx_.view(), sle, 1, ctx_.journal); diff --git a/src/ripple/protocol/Feature.h b/src/ripple/protocol/Feature.h index 6109cd8978..3b13e90230 100644 --- a/src/ripple/protocol/Feature.h +++ b/src/ripple/protocol/Feature.h @@ -86,6 +86,7 @@ class FeatureCollections "fixTakerDryOfferRemoval", "fixMasterKeyAsRegularKey", "fixCheckThreading", + "fixPayChanRecipientOwnerDir", }; std::vector features; @@ -373,6 +374,7 @@ extern uint256 const featureMultiSignReserve; extern uint256 const fixTakerDryOfferRemoval; extern uint256 const fixMasterKeyAsRegularKey; extern uint256 const fixCheckThreading; +extern uint256 const fixPayChanRecipientOwnerDir; } // ripple diff --git a/src/ripple/protocol/impl/Feature.cpp b/src/ripple/protocol/impl/Feature.cpp index b6a4990901..43a5d43363 100644 --- a/src/ripple/protocol/impl/Feature.cpp +++ b/src/ripple/protocol/impl/Feature.cpp @@ -119,6 +119,7 @@ detail::supportedAmendments () "fixTakerDryOfferRemoval", "fixMasterKeyAsRegularKey", "fixCheckThreading", + "fixPayChanRecipientOwnerDir", }; return supported; } @@ -177,5 +178,6 @@ uint256 const featureMultiSignReserve = *getRegisteredFeature("MultiSignReserve" uint256 const fixTakerDryOfferRemoval = *getRegisteredFeature("fixTakerDryOfferRemoval"); uint256 const fixMasterKeyAsRegularKey = *getRegisteredFeature("fixMasterKeyAsRegularKey"); uint256 const fixCheckThreading = *getRegisteredFeature("fixCheckThreading"); +uint256 const fixPayChanRecipientOwnerDir = *getRegisteredFeature("fixPayChanRecipientOwnerDir"); } // ripple diff --git a/src/ripple/protocol/impl/LedgerFormats.cpp b/src/ripple/protocol/impl/LedgerFormats.cpp index 527135c954..2f86a5abc4 100644 --- a/src/ripple/protocol/impl/LedgerFormats.cpp +++ b/src/ripple/protocol/impl/LedgerFormats.cpp @@ -181,6 +181,7 @@ LedgerFormats::LedgerFormats () { sfOwnerNode, soeREQUIRED }, { sfPreviousTxnID, soeREQUIRED }, { sfPreviousTxnLgrSeq, soeREQUIRED }, + { sfDestinationNode, soeOPTIONAL }, }, commonFields); diff --git a/src/test/app/PayChan_test.cpp b/src/test/app/PayChan_test.cpp index 01453dc90b..5a7f7b36c5 100644 --- a/src/test/app/PayChan_test.cpp +++ b/src/test/app/PayChan_test.cpp @@ -18,6 +18,7 @@ //============================================================================== #include +#include #include #include #include @@ -46,6 +47,20 @@ struct PayChan_test : public beast::unit_test::suite return k.key; } + + static + std::pair> + channelKeyAndSle(ReadView const& view, + jtx::Account const& account, + jtx::Account const& dst) + { + auto const sle = view.read (keylet::account (account)); + if (!sle) + return {}; + auto const k = keylet::payChan (account, dst, (*sle)[sfSequence] - 1); + return {k.key, view.read(k)}; + } + static Buffer signClaimAuth (PublicKey const& pk, SecretKey const& sk, @@ -1136,6 +1151,111 @@ struct PayChan_test : public beast::unit_test::suite } } + void + testMetaAndOwnership() + { + testcase("Metadata & Ownership"); + + using namespace jtx; + using namespace std::literals::chrono_literals; + + auto const alice = Account("alice"); + auto const bob = Account("bob"); + auto const settleDelay = 100s; + auto const pk = alice.pk(); + + auto inOwnerDir = [](ReadView const& view, + Account const& acc, + std::shared_ptr const& chan) -> bool { + ripple::Dir const ownerDir(view, keylet::ownerDir(acc.id())); + return std::find(ownerDir.begin(), ownerDir.end(), chan) != ownerDir.end(); + }; + + auto ownerDirCount = [](ReadView const& view, + Account const& acc) -> std::size_t + { + ripple::Dir const ownerDir(view, keylet::ownerDir(acc.id())); + return std::distance(ownerDir.begin(), ownerDir.end()); + }; + + { + // Test without adding the paychan to the recipient's owner + // directory + Env env( + *this, supported_amendments() - fixPayChanRecipientOwnerDir); + env.fund (XRP (10000), alice, bob); + env(create(alice, bob, XRP(1000), settleDelay, pk)); + env.close(); + auto const [chan, chanSle] = channelKeyAndSle(*env.current(), alice, bob); + BEAST_EXPECT(inOwnerDir(*env.current(), alice, chanSle)); + BEAST_EXPECT(ownerDirCount(*env.current(), alice) == 1); + BEAST_EXPECT(!inOwnerDir(*env.current(), bob, chanSle)); + BEAST_EXPECT(ownerDirCount(*env.current(), bob) == 0); + // close the channel + env(claim(bob, chan), txflags(tfClose)); + BEAST_EXPECT(!channelExists(*env.current(), chan)); + BEAST_EXPECT(!inOwnerDir(*env.current(), alice, chanSle)); + BEAST_EXPECT(ownerDirCount(*env.current(), alice) == 0); + BEAST_EXPECT(!inOwnerDir(*env.current(), bob, chanSle)); + BEAST_EXPECT(ownerDirCount(*env.current(), bob) == 0); + } + + { + // Test with adding the paychan to the recipient's owner directory + Env env(*this, supported_amendments()); + env.fund (XRP (10000), alice, bob); + env(create(alice, bob, XRP(1000), settleDelay, pk)); + env.close(); + auto const [chan, chanSle] = channelKeyAndSle(*env.current(), alice, bob); + BEAST_EXPECT(inOwnerDir(*env.current(), alice, chanSle)); + BEAST_EXPECT(ownerDirCount(*env.current(), alice) == 1); + BEAST_EXPECT(inOwnerDir(*env.current(), bob, chanSle)); + BEAST_EXPECT(ownerDirCount(*env.current(), bob) == 1); + // close the channel + env(claim(bob, chan), txflags(tfClose)); + BEAST_EXPECT(!channelExists(*env.current(), chan)); + BEAST_EXPECT(!inOwnerDir(*env.current(), alice, chanSle)); + BEAST_EXPECT(ownerDirCount(*env.current(), alice) == 0); + BEAST_EXPECT(!inOwnerDir(*env.current(), bob, chanSle)); + BEAST_EXPECT(ownerDirCount(*env.current(), bob) == 0); + } + + { + // Test removing paychans created before adding to the recipient's + // owner directory + Env env( + *this, supported_amendments() - fixPayChanRecipientOwnerDir); + env.fund (XRP (10000), alice, bob); + // create the channel before the amendment activates + env(create(alice, bob, XRP(1000), settleDelay, pk)); + env.close(); + auto const [chan, chanSle] = channelKeyAndSle(*env.current(), alice, bob); + BEAST_EXPECT(inOwnerDir(*env.current(), alice, chanSle)); + BEAST_EXPECT(ownerDirCount(*env.current(), alice) == 1); + BEAST_EXPECT(!inOwnerDir(*env.current(), bob, chanSle)); + BEAST_EXPECT(ownerDirCount(*env.current(), bob) == 0); + env.enableFeature(fixPayChanRecipientOwnerDir); + env.close(); + BEAST_EXPECT( + env.current()->rules().enabled(fixPayChanRecipientOwnerDir)); + // These checks look redundant, but if you don't `close` after the + // `create` these checks will fail. I believe this is due to the + // create running with one set of amendments initially, then with a + // different set with the ledger closes (tho I haven't dug into it) + BEAST_EXPECT(inOwnerDir(*env.current(), alice, chanSle)); + BEAST_EXPECT(!inOwnerDir(*env.current(), bob, chanSle)); + BEAST_EXPECT(ownerDirCount(*env.current(), bob) == 0); + + // close the channel after the amendment activates + env(claim(bob, chan), txflags(tfClose)); + BEAST_EXPECT(!channelExists(*env.current(), chan)); + BEAST_EXPECT(!inOwnerDir(*env.current(), alice, chanSle)); + BEAST_EXPECT(ownerDirCount(*env.current(), alice) == 0); + BEAST_EXPECT(!inOwnerDir(*env.current(), bob, chanSle)); + BEAST_EXPECT(ownerDirCount(*env.current(), bob) == 0); + } + } + void run () override { @@ -1152,6 +1272,7 @@ struct PayChan_test : public beast::unit_test::suite testRPC (); testOptionalFields (); testMalformedPK (); + testMetaAndOwnership (); } }; diff --git a/src/test/rpc/AccountTx_test.cpp b/src/test/rpc/AccountTx_test.cpp index 493d29eb4d..3b20a67f2d 100644 --- a/src/test/rpc/AccountTx_test.cpp +++ b/src/test/rpc/AccountTx_test.cpp @@ -419,9 +419,9 @@ class AccountTx_test : public beast::unit_test::suite { 2, jss::CheckCash, {}, {jss::Check}, {jss::AccountRoot, jss::AccountRoot, jss::DirectoryNode, jss::DirectoryNode}}, { 3, jss::CheckCreate, {jss::Check}, {}, {jss::AccountRoot, jss::AccountRoot, jss::DirectoryNode, jss::DirectoryNode}}, { 4, jss::CheckCreate, {jss::Check}, {}, {jss::AccountRoot, jss::AccountRoot, jss::DirectoryNode, jss::DirectoryNode}}, - { 5, jss::PaymentChannelClaim, {}, {jss::PayChannel}, {jss::AccountRoot, jss::AccountRoot, jss::DirectoryNode}}, + { 5, jss::PaymentChannelClaim, {}, {jss::PayChannel}, {jss::AccountRoot, jss::AccountRoot, jss::DirectoryNode, jss::DirectoryNode}}, { 6, jss::PaymentChannelFund, {}, {}, {jss::AccountRoot, jss::PayChannel }}, - { 7, jss::PaymentChannelCreate, {jss::PayChannel}, {}, {jss::AccountRoot, jss::AccountRoot, jss::DirectoryNode}}, + { 7, jss::PaymentChannelCreate, {jss::PayChannel}, {}, {jss::AccountRoot, jss::AccountRoot, jss::DirectoryNode, jss::DirectoryNode}}, { 8, jss::EscrowCancel, {}, {jss::Escrow}, {jss::AccountRoot, jss::DirectoryNode}}, { 9, jss::EscrowFinish, {}, {jss::Escrow}, {jss::AccountRoot, jss::DirectoryNode}}, { 10, jss::EscrowCreate, {jss::Escrow}, {}, {jss::AccountRoot, jss::DirectoryNode}},