From 37cb4cdbe3ef5bfd5372effbddec5348102a96f0 Mon Sep 17 00:00:00 2001 From: Denis Angell Date: Thu, 3 Sep 2026 16:27:45 +0000 Subject: [PATCH] fix: Recycle the escrow reserve in EscrowCancel and EscrowFinish (#8142) --- .../tx/transactors/escrow/EscrowCancel.cpp | 9 +- .../tx/transactors/escrow/EscrowFinish.cpp | 17 ++-- src/test/app/EscrowToken_test.cpp | 95 +++++++++++++++++++ src/test/app/Sponsor_test.cpp | 21 ++-- 4 files changed, 119 insertions(+), 23 deletions(-) diff --git a/src/libxrpl/tx/transactors/escrow/EscrowCancel.cpp b/src/libxrpl/tx/transactors/escrow/EscrowCancel.cpp index 21e6bd2c30..32b56b909f 100644 --- a/src/libxrpl/tx/transactors/escrow/EscrowCancel.cpp +++ b/src/libxrpl/tx/transactors/escrow/EscrowCancel.cpp @@ -169,6 +169,12 @@ EscrowCancel::doApply() auto const sle = ctx_.view().peek(keylet::account(account)); STAmount const amount = slep->getFieldAmount(sfAmount); + // The return can re-create a holding the owner deleted while the escrow + // was pending; the removed escrow must not be counted against its reserve. + bool const recycleReserve = ctx_.view().rules().enabled(fixCleanup3_4_0); + if (recycleReserve) + decreaseOwnerCountForObject(ctx_.view(), sle, slep, 1, ctx_.journal); + // Transfer amount back to the owner if (isXRP(amount)) { @@ -212,7 +218,8 @@ EscrowCancel::doApply() } } - decreaseOwnerCountForObject(ctx_.view(), sle, slep, 1, ctx_.journal); + if (!recycleReserve) + decreaseOwnerCountForObject(ctx_.view(), sle, slep, 1, ctx_.journal); // Remove escrow from ledger ctx_.view().erase(slep); diff --git a/src/libxrpl/tx/transactors/escrow/EscrowFinish.cpp b/src/libxrpl/tx/transactors/escrow/EscrowFinish.cpp index aa352d5e98..09219b0bf1 100644 --- a/src/libxrpl/tx/transactors/escrow/EscrowFinish.cpp +++ b/src/libxrpl/tx/transactors/escrow/EscrowFinish.cpp @@ -343,14 +343,12 @@ EscrowFinish::doApply() } } - // With the Sponsor amendment, release the escrow reserve before delivery. - // Token delivery can auto-create a destination holding, and the same - // sponsor (or the same account, for a self-escrow) may cover both the - // escrow being removed and the holding being created. Without the - // amendment, keep the legacy order: releasing early changes the reserve - // arithmetic for self-escrows and would break consensus if not gated. - bool const sponsorEnabled = ctx_.view().rules().enabled(featureSponsor); - if (sponsorEnabled) + // Delivery can auto-create the destination's holding; the removed escrow + // must not be counted against its reserve. The two share a reserve payer + // for a self-escrow, or when one sponsor covers both. + bool const recycleReserve = + ctx_.view().rules().enabled(featureSponsor) || ctx_.view().rules().enabled(fixCleanup3_4_0); + if (recycleReserve) decreaseOwnerCountForObject(ctx_.view(), account, slep, 1, ctx_.journal); STAmount const amount = slep->getFieldAmount(sfAmount); @@ -402,8 +400,7 @@ EscrowFinish::doApply() ctx_.view().update(sled); - // Adjust source owner count (legacy position, pre-Sponsor) - if (!sponsorEnabled) + if (!recycleReserve) decreaseOwnerCountForObject(ctx_.view(), account, slep, 1, ctx_.journal); // Remove escrow from ledger diff --git a/src/test/app/EscrowToken_test.cpp b/src/test/app/EscrowToken_test.cpp index 72db63bd3f..3a2bc14183 100644 --- a/src/test/app/EscrowToken_test.cpp +++ b/src/test/app/EscrowToken_test.cpp @@ -951,6 +951,99 @@ struct EscrowToken_test : public beast::unit_test::Suite } } + void + testIOUCancelReserveRecycle(FeatureBitset features) + { + testcase("IOU Cancel Reserve Recycle"); + using namespace jtx; + using namespace std::literals; + + // Escrowing the whole IOU balance lets the owner delete the now-zero + // trust line, so cancelling has to re-create it: one object destroyed, + // one created, and the reserve requirement unchanged. + Env env{*this, features}; + bool const fixEnabled = env.current()->rules().enabled(fixCleanup3_4_0); + + auto const baseFee = env.current()->fees().base; + auto const alice = Account("alice"); + auto const bob = Account("bob"); + auto const gw = Account("gw"); + auto const usd = gw["USD"]; + + env.fund(XRP(10'000), alice, bob, gw); + env.close(); + + env(fset(gw, asfAllowTrustLineLocking)); + env.close(); + + env.trust(usd(10'000), alice); + env.close(); + + env(pay(gw, alice, usd(10'000))); + env.close(); + BEAST_EXPECT(env.ownerCount(alice) == 1); + + auto const cancelAfter = env.now() + 100s; + auto const seq = env.seq(alice); + env(escrow::create(alice, bob, usd(10'000)), + escrow::kFinishTime(env.now() + 1s), + escrow::kCancelTime(cancelAfter), + Fee(baseFee)); + env.close(); + BEAST_EXPECT(env.ownerCount(alice) == 2); + + auto const trustLineKey = keylet::trustLine(alice.id(), gw.id(), usd.currency); + env(trust(alice, usd(0))); + env.close(); + BEAST_EXPECT(!env.current()->exists(trustLineKey)); + BEAST_EXPECT(env.ownerCount(alice) == 1); + + // Leave alice holding the reserve for exactly one owned object. That + // is the escrow now and the re-created trust line after the cancel. + auto const oneObject = env.current()->fees().accountReserve(1, 1); + auto const twoObjects = env.current()->fees().accountReserve(2, 1); + auto const balance = env.balance(alice).value().xrp(); + auto const feeCushion = baseFee.drops() * 20; + env(pay(alice, bob, drops(balance.drops() - oneObject.drops() - feeCushion))); + env.close(); + BEAST_EXPECT(env.balance(alice).value().xrp() >= oneObject); + BEAST_EXPECT(env.balance(alice).value().xrp() < twoObjects); + + for (; env.now() < cancelAfter; env.close()) + { + } + env.close(); + env.close(); + + auto const expectedResult = fixEnabled ? Ter(tesSUCCESS) : Ter(tecNO_LINE_INSUF_RESERVE); + env(escrow::cancel(alice, alice, seq), Fee(baseFee), expectedResult); + env.close(); + + auto const escrowKey = keylet::escrow(alice.id(), SeqProxy::rawSequence(seq)); + if (fixEnabled) + { + BEAST_EXPECT(!env.le(escrowKey)); + BEAST_EXPECT(env.current()->exists(trustLineKey)); + BEAST_EXPECT(env.balance(alice, usd) == usd(10'000)); + BEAST_EXPECT(env.ownerCount(alice) == 1); + } + else + { + // The tec keeps the escrow, so one more owner reserve lets the + // retry through. + BEAST_EXPECT(env.le(escrowKey) != nullptr); + BEAST_EXPECT(!env.current()->exists(trustLineKey)); + BEAST_EXPECT(env.ownerCount(alice) == 1); + + env(pay(bob, alice, drops(twoObjects.drops() - oneObject.drops()))); + env.close(); + env(escrow::cancel(alice, alice, seq), Fee(baseFee), Ter(tesSUCCESS)); + env.close(); + BEAST_EXPECT(!env.le(escrowKey)); + BEAST_EXPECT(env.balance(alice, usd) == usd(10'000)); + } + } + void testIOUBalances(FeatureBitset features) { @@ -4250,6 +4343,8 @@ public: } testMPTSplitEscrowTransferFee(all - fixCleanup3_4_0); testMPTSplitEscrowTransferFee(all); + testIOUCancelReserveRecycle(all - fixCleanup3_4_0); + testIOUCancelReserveRecycle(all); } }; diff --git a/src/test/app/Sponsor_test.cpp b/src/test/app/Sponsor_test.cpp index 593edfe0a4..9f07e9bdfe 100644 --- a/src/test/app/Sponsor_test.cpp +++ b/src/test/app/Sponsor_test.cpp @@ -5467,13 +5467,12 @@ public: using namespace test::jtx; using namespace std::chrono_literals; - // Finishing a self-escrow (source == destination) whose trust line - // was deleted while the escrow was outstanding auto-creates the line, - // and the outcome of that reserve check depends on whether the escrow - // reserve is released before delivery (Sponsor) or after (legacy). - // With the source's balance in the one-increment window - // [reserve(1), reserve(2)), the legacy order requires reserve(2) and - // fails, while the Sponsor order requires reserve(1) and succeeds. + // Finishing a self-escrow (source == destination) whose trust line was + // deleted while the escrow was outstanding auto-creates the line. With + // the source's balance in the one-increment window + // [reserve(1), reserve(2)), the finish succeeds only when the escrow + // reserve is released before delivery, which either featureSponsor or + // fixCleanup3_4_0 does. auto runTest = [&](FeatureBitset features, TER expected) { Account const alice("alice"); Account const gw("gw"); @@ -5538,11 +5537,9 @@ public: } }; - // Pre-amendment: legacy order — the escrow still counts against the - // reserve while the auto-created line is checked. - runTest(testableAmendments() - featureSponsor, tecNO_LINE_INSUF_RESERVE); - - // Post-amendment: the escrow reserve is recycled into the new line. + runTest(testableAmendments() - featureSponsor - fixCleanup3_4_0, tecNO_LINE_INSUF_RESERVE); + runTest(testableAmendments() - featureSponsor, tesSUCCESS); + runTest(testableAmendments() - fixCleanup3_4_0, tesSUCCESS); runTest(testableAmendments(), tesSUCCESS); }