From 6fa6a96e3abb0f857d1c1dbbd02974aa86a4ae32 Mon Sep 17 00:00:00 2001 From: tequ Date: Tue, 21 Oct 2025 13:17:53 +0900 Subject: [PATCH] Introduce `StartTime` in `CronSet` and improve next execution scheduling (#596) --- hook/sfcodes.h | 6 +- hook/tts.h | 4 +- src/ripple/app/tx/impl/Cron.cpp | 7 +- src/ripple/app/tx/impl/SetCron.cpp | 106 +++++++++++++++------ src/ripple/protocol/SField.h | 1 + src/ripple/protocol/impl/LedgerFormats.cpp | 1 + src/ripple/protocol/impl/SField.cpp | 1 + src/ripple/protocol/impl/TxFormats.cpp | 1 + src/test/app/Cron_test.cpp | 91 ++++++++++++------ src/test/app/SetHookTSH_test.cpp | 4 + src/test/jtx/cron.h | 15 +++ src/test/jtx/impl/cron.cpp | 6 ++ 12 files changed, 182 insertions(+), 61 deletions(-) diff --git a/hook/sfcodes.h b/hook/sfcodes.h index e7c36c535..242d14499 100644 --- a/hook/sfcodes.h +++ b/hook/sfcodes.h @@ -62,6 +62,9 @@ #define sfEmitGeneration ((2U << 16U) + 46U) #define sfLockCount ((2U << 16U) + 49U) #define sfFirstNFTokenSequence ((2U << 16U) + 50U) +#define sfStartTime ((2U << 16U) + 93U) +#define sfRepeatCount ((2U << 16U) + 94U) +#define sfDelaySeconds ((2U << 16U) + 95U) #define sfXahauActivationLgrSeq ((2U << 16U) + 96U) #define sfImportSequence ((2U << 16U) + 97U) #define sfRewardTime ((2U << 16U) + 98U) @@ -129,6 +132,7 @@ #define sfGovernanceFlags ((5U << 16U) + 99U) #define sfGovernanceMarks ((5U << 16U) + 98U) #define sfEmittedTxnID ((5U << 16U) + 97U) +#define sfCron ((5U << 16U) + 95U) #define sfAmount ((6U << 16U) + 1U) #define sfBalance ((6U << 16U) + 2U) #define sfLimitAmount ((6U << 16U) + 3U) @@ -236,4 +240,4 @@ #define sfActiveValidators ((15U << 16U) + 95U) #define sfImportVLKeys ((15U << 16U) + 94U) #define sfHookEmissions ((15U << 16U) + 93U) -#define sfAmounts ((15U << 16U) + 92U) \ No newline at end of file +#define sfAmounts ((15U << 16U) + 92U) diff --git a/hook/tts.h b/hook/tts.h index 0973fa189..75ce47470 100644 --- a/hook/tts.h +++ b/hook/tts.h @@ -31,6 +31,8 @@ #define ttURITOKEN_BUY 47 #define ttURITOKEN_CREATE_SELL_OFFER 48 #define ttURITOKEN_CANCEL_SELL_OFFER 49 +#define ttCRON 92 +#define ttCRON_SET 93 #define ttREMIT 95 #define ttGENESIS_MINT 96 #define ttIMPORT 97 @@ -40,4 +42,4 @@ #define ttFEE 101 #define ttUNL_MODIFY 102 #define ttEMIT_FAILURE 103 -#define ttUNL_REPORT 104 \ No newline at end of file +#define ttUNL_REPORT 104 diff --git a/src/ripple/app/tx/impl/Cron.cpp b/src/ripple/app/tx/impl/Cron.cpp index befcdd425..67826b408 100644 --- a/src/ripple/app/tx/impl/Cron.cpp +++ b/src/ripple/app/tx/impl/Cron.cpp @@ -121,11 +121,11 @@ Cron::doApply() uint32_t delay = sleCron->getFieldU32(sfDelaySeconds); uint32_t recur = sleCron->getFieldU32(sfRepeatCount); - uint32_t currentTime = view.parentCloseTime().time_since_epoch().count(); + uint32_t lastStartTime = sleCron->getFieldU32(sfStartTime); // do all this sanity checking before we modify the ledger... - uint32_t afterTime = currentTime + delay; - if (afterTime < currentTime) + uint32_t afterTime = lastStartTime + delay; + if (afterTime < lastStartTime) return tefINTERNAL; // in all circumstances the Cron object is deleted... @@ -163,6 +163,7 @@ Cron::doApply() sleCron->setFieldU64(sfOwnerNode, *page); sleCron->setFieldU32(sfDelaySeconds, delay); sleCron->setFieldU32(sfRepeatCount, recur - 1); + sleCron->setFieldU32(sfStartTime, afterTime); sleCron->setAccountID(sfOwner, id); sle->setFieldH256(sfCron, klCron.key); diff --git a/src/ripple/app/tx/impl/SetCron.cpp b/src/ripple/app/tx/impl/SetCron.cpp index 3e2e4e6f4..3325702a8 100644 --- a/src/ripple/app/tx/impl/SetCron.cpp +++ b/src/ripple/app/tx/impl/SetCron.cpp @@ -51,47 +51,74 @@ SetCron::preflight(PreflightContext const& ctx) return temINVALID_FLAG; } - // DelaySeconds (D), RepeatCount (R) - // DR - Set Cron with Delay and Repeat - // D- - Set Cron (once off) with Delay only (repat implicitly 0) - // -R - Invalid + // DelaySeconds (D), RepeatCount (R), StartTime (S) + // DRS - Set Cron with Delay and Repeat and StartTime + // DR- - Invalid(StartTime is required) + // D-S - Invalid (both DelaySeconds and RepeatCount are required) + // -RS - Invalid (both DelaySeconds and RepeatCount are required) + // --S - Onetime cron with StartTime only // -- - Clear any existing cron (succeeds even if there isn't one) / with // tfCronUnset flag set bool const hasDelay = tx.isFieldPresent(sfDelaySeconds); bool const hasRepeat = tx.isFieldPresent(sfRepeatCount); + bool const hasStartTime = tx.isFieldPresent(sfStartTime); if (tx.isFlag(tfCronUnset)) { - if (hasDelay || hasRepeat) + // delete operation + if (hasDelay || hasRepeat || hasStartTime) { JLOG(j.debug()) << "SetCron: tfCronUnset flag cannot be used with " - "DelaySeconds or RepeatCount."; + "DelaySeconds, RepeatCount or StartTime."; return temMALFORMED; } } else { - if (!hasDelay) + // create operation + + if (!hasStartTime) { - JLOG(j.debug()) << "SetCron: DelaySeconds must be " - "specified to create a cron."; + JLOG(j.debug()) + << "SetCron: StartTime is required. Use StartTime=0 for " + "immediate execution, or specify a future timestamp."; + return temMALFORMED; + } + + if ((!hasDelay && hasRepeat) || (hasDelay && !hasRepeat)) + { + JLOG(j.debug()) + << "SetCron: DelaySeconds and RepeatCount must both be present " + "for recurring crons, or both absent for one-off crons."; return temMALFORMED; } // check delay is not too high - auto delay = tx.getFieldU32(sfDelaySeconds); - if (delay > 31536000UL /* 365 days in seconds */) + if (hasDelay) { - JLOG(j.debug()) << "SetCron: DelaySeconds was too high. (max 365 " - "days in seconds)."; - return temMALFORMED; + auto delay = tx.getFieldU32(sfDelaySeconds); + if (delay > 31536000UL /* 365 days in seconds */) + { + JLOG(j.debug()) + << "SetCron: DelaySeconds was too high. (max 365 " + "days in seconds)."; + return temMALFORMED; + } } // check repeat is not too high if (hasRepeat) { auto recur = tx.getFieldU32(sfRepeatCount); + if (recur == 0) + { + JLOG(j.debug()) + << "SetCron: RepeatCount must be greater than 0." + "For one-time execution, omit DelaySeconds and " + "RepeatCount."; + return temMALFORMED; + } if (recur > 256) { JLOG(j.debug()) @@ -108,6 +135,30 @@ SetCron::preflight(PreflightContext const& ctx) TER SetCron::preclaim(PreclaimContext const& ctx) { + if (ctx.tx.isFieldPresent(sfStartTime) && + ctx.tx.getFieldU32(sfStartTime) != 0) + { + // StartTime 0 means the cron will execute immediately + + auto const startTime = ctx.tx.getFieldU32(sfStartTime); + auto const parentCloseTime = + ctx.view.parentCloseTime().time_since_epoch().count(); + + if (startTime < parentCloseTime) + { + JLOG(ctx.j.debug()) << "SetCron: StartTime must be in the future " + "(or 0 for immediate execution)"; + return tecEXPIRED; + } + + if (startTime > ctx.view.parentCloseTime().time_since_epoch().count() + + 365 * 24 * 60 * 60) + { + JLOG(ctx.j.debug()) << "SetCron: StartTime is too far in the " + "future (max 365 days)."; + return tecEXPIRED; + } + } return tesSUCCESS; } @@ -123,6 +174,7 @@ SetCron::doApply() // ledger. uint32_t delay{0}; uint32_t recur{0}; + uint32_t startTime{0}; if (!isDelete) { @@ -130,17 +182,14 @@ SetCron::doApply() delay = tx.getFieldU32(sfDelaySeconds); if (tx.isFieldPresent(sfRepeatCount)) recur = tx.getFieldU32(sfRepeatCount); + if (tx.isFieldPresent(sfStartTime)) + { + startTime = tx.getFieldU32(sfStartTime); + if (startTime == 0) + startTime = view.parentCloseTime().time_since_epoch().count(); + } } - uint32_t currentTime = view.parentCloseTime().time_since_epoch().count(); - - // do all this sanity checking before we modify the ledger... - // even for a delete operation this will fall through without incident - - uint32_t afterTime = currentTime + delay; - if (afterTime < currentTime) - return tefINTERNAL; - AccountID const& id = tx.getAccountID(sfAccount); auto sle = view.peek(keylet::account(id)); if (!sle) @@ -190,7 +239,7 @@ SetCron::doApply() // execution to here means we're creating a new Cron object and adding it to // the user's owner dir - Keylet klCron = keylet::cron(afterTime, id); + Keylet klCron = keylet::cron(startTime, id); std::shared_ptr sleCron = std::make_shared(klCron); @@ -214,6 +263,7 @@ SetCron::doApply() adjustOwnerCount(view, sle, 1, j_); // set the fields + sleCron->setFieldU32(sfStartTime, startTime); sleCron->setFieldU32(sfDelaySeconds, delay); sleCron->setFieldU32(sfRepeatCount, recur); sleCron->setAccountID(sfOwner, id); @@ -232,18 +282,18 @@ SetCron::calculateBaseFee(ReadView const& view, STTx const& tx) { auto const baseFee = Transactor::calculateBaseFee(view, tx); - auto const hasRepeat = tx.isFieldPresent(sfRepeatCount); - if (tx.isFlag(tfCronUnset)) // delete cron return baseFee; + auto const repeatCount = + tx.isFieldPresent(sfRepeatCount) ? tx.getFieldU32(sfRepeatCount) : 0; + // factor a cost based on the total number of txns expected // for RepeatCount of 0 we have this txn (SetCron) and the // single Cron txn (2). For a RepeatCount of 1 we have this txn, // the first time the cron executes, and the second time (3). - uint32_t const additionalExpectedExecutions = - hasRepeat ? tx.getFieldU32(sfRepeatCount) + 1 : 1; + uint32_t const additionalExpectedExecutions = 1 + repeatCount; auto const additionalFee = baseFee * additionalExpectedExecutions; if (baseFee + additionalFee < baseFee) diff --git a/src/ripple/protocol/SField.h b/src/ripple/protocol/SField.h index a75488310..2f9619de9 100644 --- a/src/ripple/protocol/SField.h +++ b/src/ripple/protocol/SField.h @@ -412,6 +412,7 @@ extern SF_UINT32 const sfImportSequence; extern SF_UINT32 const sfXahauActivationLgrSeq; extern SF_UINT32 const sfDelaySeconds; extern SF_UINT32 const sfRepeatCount; +extern SF_UINT32 const sfStartTime; // 64-bit integers (common) extern SF_UINT64 const sfIndexNext; diff --git a/src/ripple/protocol/impl/LedgerFormats.cpp b/src/ripple/protocol/impl/LedgerFormats.cpp index 78c3f61e8..83303e620 100644 --- a/src/ripple/protocol/impl/LedgerFormats.cpp +++ b/src/ripple/protocol/impl/LedgerFormats.cpp @@ -371,6 +371,7 @@ LedgerFormats::LedgerFormats() ltCRON, { {sfOwner, soeREQUIRED}, + {sfStartTime, soeREQUIRED}, {sfDelaySeconds, soeREQUIRED}, {sfRepeatCount, soeREQUIRED}, {sfOwnerNode, soeREQUIRED}, diff --git a/src/ripple/protocol/impl/SField.cpp b/src/ripple/protocol/impl/SField.cpp index 85b52ec2f..eeecaa126 100644 --- a/src/ripple/protocol/impl/SField.cpp +++ b/src/ripple/protocol/impl/SField.cpp @@ -157,6 +157,7 @@ CONSTRUCT_TYPED_SFIELD(sfLockCount, "LockCount", UINT32, CONSTRUCT_TYPED_SFIELD(sfFirstNFTokenSequence, "FirstNFTokenSequence", UINT32, 50); +CONSTRUCT_TYPED_SFIELD(sfStartTime, "StartTime", UINT32, 93); CONSTRUCT_TYPED_SFIELD(sfRepeatCount, "RepeatCount", UINT32, 94); CONSTRUCT_TYPED_SFIELD(sfDelaySeconds, "DelaySeconds", UINT32, 95); CONSTRUCT_TYPED_SFIELD(sfXahauActivationLgrSeq, "XahauActivationLgrSeq",UINT32, 96); diff --git a/src/ripple/protocol/impl/TxFormats.cpp b/src/ripple/protocol/impl/TxFormats.cpp index b0a995bb8..efcb44752 100644 --- a/src/ripple/protocol/impl/TxFormats.cpp +++ b/src/ripple/protocol/impl/TxFormats.cpp @@ -486,6 +486,7 @@ TxFormats::TxFormats() { {sfDelaySeconds, soeOPTIONAL}, {sfRepeatCount, soeOPTIONAL}, + {sfStartTime, soeOPTIONAL}, }, commonFields); } diff --git a/src/test/app/Cron_test.cpp b/src/test/app/Cron_test.cpp index a1f96ce46..6e8248183 100644 --- a/src/test/app/Cron_test.cpp +++ b/src/test/app/Cron_test.cpp @@ -48,9 +48,13 @@ struct Cron_test : public beast::unit_test::suite auto const expectResult = withCron ? ter(tesSUCCESS) : ter(temDISABLED); - auto tx = cron::set(alice); // CLAIM - env(cron::set(alice), cron::delay(100), fee(XRP(1)), expectResult); + env(cron::set(alice), + cron::startTime(0), + cron::repeat(100), + cron::delay(100), + fee(XRP(1)), + expectResult); env.close(); } } @@ -70,9 +74,10 @@ struct Cron_test : public beast::unit_test::suite env.fund(XRP(1000), alice); env.close(); - // create with RepeatCount + // create auto expected = baseFee * 2 + baseFee * 256; env(cron::set(alice), + cron::startTime(0), cron::delay(356 * 24 * 60 * 60), cron::repeat(256), fee(expected - 1), @@ -80,26 +85,13 @@ struct Cron_test : public beast::unit_test::suite env.close(); env(cron::set(alice), + cron::startTime(0), cron::delay(356 * 24 * 60 * 60), cron::repeat(256), fee(expected), ter(tesSUCCESS)); env.close(); - // create with no RepeatCount - expected = baseFee * 2; - env(cron::set(alice), - cron::delay(356 * 24 * 60 * 60), - fee(expected - 1), - ter(telINSUF_FEE_P)); - env.close(); - - env(cron::set(alice), - cron::delay(356 * 24 * 60 * 60), - fee(expected), - ter(tesSUCCESS)); - env.close(); - // delete expected = baseFee; env(cron::set(alice), @@ -143,30 +135,47 @@ struct Cron_test : public beast::unit_test::suite // temMALFORMED { - // Invalid both DelaySeconds and RepeatCount are not specified + // Invalid DelaySeconds and RepeatCount and StartTime are not + // specified env(cron::set(alice), ter(temMALFORMED)); - // Invalid DelaySeconds and RepeatCount combination - // (only RepeatCount specified) - env(cron::set(alice), cron::repeat(256), ter(temMALFORMED)); + // Invalid DelaySeconds and RepeatCount combination with StartTime + env(cron::set(alice), + cron::startTime(100), + cron::delay(356 * 24 * 60 * 60), + ter(temMALFORMED)); + env(cron::set(alice), + cron::startTime(100), + cron::repeat(256), + ter(temMALFORMED)); // Invalid DelaySeconds env(cron::set(alice), + cron::startTime(100), cron::delay(365 * 24 * 60 * 60 + 1), cron::repeat(256), ter(temMALFORMED)); // Invalid RepeatCount env(cron::set(alice), + cron::startTime(100), cron::delay(365 * 24 * 60 * 60), cron::repeat(257), ter(temMALFORMED)); - // Invalid tfCronUnset flag + // Invalid with tfCronUnset flag env(cron::set(alice), cron::delay(365 * 24 * 60 * 60), txflags(tfCronUnset), ter(temMALFORMED)); + env(cron::set(alice), + cron::repeat(100), + txflags(tfCronUnset), + ter(temMALFORMED)); + env(cron::set(alice), + cron::startTime(100), + txflags(tfCronUnset), + ter(temMALFORMED)); } } @@ -179,9 +188,25 @@ struct Cron_test : public beast::unit_test::suite auto const alice = Account("alice"); Env env{*this, features | featureCron}; + env.fund(XRP(1000), alice); + env.close(); - // there is no check in preclaim - BEAST_EXPECT(true); + // Past StartTime + env(cron::set(alice), + cron::startTime( + env.timeKeeper().now().time_since_epoch().count() - 1), + fee(XRP(1)), + ter(tecEXPIRED)); + env.close(); + + // Too far Future StartTime + env(cron::set(alice), + cron::startTime( + env.timeKeeper().now().time_since_epoch().count() + + 365 * 24 * 60 * 60 + 1), + fee(XRP(1)), + ter(tecEXPIRED)); + env.close(); } void @@ -199,7 +224,10 @@ struct Cron_test : public beast::unit_test::suite auto const aliceOwnerCount = ownerCount(env, alice); // create cron + auto parentCloseTime = + env.current()->parentCloseTime().time_since_epoch().count(); env(cron::set(alice), + cron::startTime(parentCloseTime + 356 * 24 * 60 * 60), cron::delay(356 * 24 * 60 * 60), cron::repeat(256), fee(XRP(1)), @@ -219,9 +247,15 @@ struct Cron_test : public beast::unit_test::suite BEAST_EXPECT( cronSle->getFieldU32(sfDelaySeconds) == 356 * 24 * 60 * 60); BEAST_EXPECT(cronSle->getFieldU32(sfRepeatCount) == 256); + BEAST_EXPECT( + cronSle->getFieldU32(sfStartTime) == + parentCloseTime + 356 * 24 * 60 * 60); // update cron + parentCloseTime = + env.current()->parentCloseTime().time_since_epoch().count(); env(cron::set(alice), + cron::startTime(0), cron::delay(100), cron::repeat(10), fee(XRP(1)), @@ -243,6 +277,7 @@ struct Cron_test : public beast::unit_test::suite BEAST_EXPECT(cronSle2); BEAST_EXPECT(cronSle2->getFieldU32(sfDelaySeconds) == 100); BEAST_EXPECT(cronSle2->getFieldU32(sfRepeatCount) == 10); + BEAST_EXPECT(cronSle2->getFieldU32(sfStartTime) == parentCloseTime); // delete cron env(cron::set(alice), @@ -289,6 +324,7 @@ struct Cron_test : public beast::unit_test::suite auto repeatCount = 10; env(cron::set(alice), + cron::startTime(baseTime + 100), cron::delay(100), cron::repeat(repeatCount), fee(XRP(1))); @@ -311,7 +347,7 @@ struct Cron_test : public beast::unit_test::suite } // close after 100 seconds passed - env.close(); + env.close(10s); auto txns = env.closed()->txs; auto size = std::distance(txns.begin(), txns.end()); @@ -348,8 +384,7 @@ struct Cron_test : public beast::unit_test::suite cronSle->getAccountID(sfOwner) == alice.id()); // set new base time - baseTime = - env.timeKeeper().now().time_since_epoch().count(); + baseTime = baseTime + 100; lastCronKeylet = cronKeylet; } else @@ -380,7 +415,7 @@ struct Cron_test : public beast::unit_test::suite for (auto const& account : accounts) { - env(cron::set(account), cron::delay(0), fee(XRP(1))); + env(cron::set(account), cron::startTime(0), fee(XRP(1))); } env.close(); diff --git a/src/test/app/SetHookTSH_test.cpp b/src/test/app/SetHookTSH_test.cpp index d3849c3bb..5df238a98 100644 --- a/src/test/app/SetHookTSH_test.cpp +++ b/src/test/app/SetHookTSH_test.cpp @@ -6297,6 +6297,7 @@ private: // cron set env(cron::set(account), + cron::startTime(0), cron::delay(100), cron::repeat(1), fee(XRP(1)), @@ -6332,8 +6333,11 @@ private: env.fund(XRP(1000), account); env.close(); + auto const baseTime = + env.current()->parentCloseTime().time_since_epoch().count(); // cron set env(cron::set(account), + cron::startTime(baseTime + 100), cron::delay(100), cron::repeat(1), fee(XRP(1)), diff --git a/src/test/jtx/cron.h b/src/test/jtx/cron.h index e8ccc9ad4..8c2113aa0 100644 --- a/src/test/jtx/cron.h +++ b/src/test/jtx/cron.h @@ -34,6 +34,21 @@ namespace cron { Json::Value set(jtx::Account const& account); +/** Sets the optional StartTime on a JTx. */ +class startTime +{ +private: + uint32_t startTime_; + +public: + explicit startTime(uint32_t startTime) : startTime_(startTime) + { + } + + void + operator()(Env&, JTx& jtx) const; +}; + /** Sets the optional DelaySeconds on a JTx. */ class delay { diff --git a/src/test/jtx/impl/cron.cpp b/src/test/jtx/impl/cron.cpp index 8b7fda57c..e78f08405 100644 --- a/src/test/jtx/impl/cron.cpp +++ b/src/test/jtx/impl/cron.cpp @@ -37,6 +37,12 @@ set(jtx::Account const& account) return jv; } +void +startTime::operator()(Env& env, JTx& jt) const +{ + jt.jv[sfStartTime.jsonName] = startTime_; +} + void delay::operator()(Env& env, JTx& jt) const {