diff --git a/include/xrpl/protocol/detail/features.macro b/include/xrpl/protocol/detail/features.macro index 43f3548e0c..83f8ffa5b3 100644 --- a/include/xrpl/protocol/detail/features.macro +++ b/include/xrpl/protocol/detail/features.macro @@ -34,6 +34,7 @@ // If you add an amendment here, then do not forget to increment `numFeatures` // in include/xrpl/protocol/Feature.h. +XRPL_FIX (HookOnV2InstallUpdate, Supported::yes, VoteBehavior::DefaultYes) XRPL_FIX (HookMap, Supported::yes, VoteBehavior::DefaultYes) XRPL_FIX (GuardDepth32, Supported::yes, VoteBehavior::DefaultNo) XRPL_FEATURE(NamedHooks, Supported::yes, VoteBehavior::DefaultNo) diff --git a/src/test/app/SetHook_test.cpp b/src/test/app/SetHook_test.cpp index 8c60f38624..0ce003c697 100644 --- a/src/test/app/SetHook_test.cpp +++ b/src/test/app/SetHook_test.cpp @@ -1534,12 +1534,10 @@ class SetHook0_test : public beast::unit_test::suite jv.removeMember(jss::HookOn); jv[jss::HookOnIncoming] = "0000000000000000000000000000000000000000000000000000000000" - "0000" - "00"; + "000000"; jv[jss::HookOnOutgoing] = "0000000000000000000000000000000000000000000000000000000000" - "0000" - "01"; + "000001"; env(ripple::test::jtx::hook(alice, {{jv}}, 0), M("Execution: Install"), HSFEE); @@ -1604,12 +1602,10 @@ class SetHook0_test : public beast::unit_test::suite jv.removeMember(jss::HookOn); jv[jss::HookOnIncoming] = "fffffffffffffffffffffffffffffffffffffff7ffffffffffffffffff" - "bfff" - "ff"; // Invoke high + "bfffff"; // Invoke high jv[jss::HookOnOutgoing] = "ffffffffffffffffffffffffffffffffffffffffffffffffffffffffff" - "bfff" - "fe"; // Payment high + "bffffe"; // Payment high env(ripple::test::jtx::hook(alice, {{jv}}, 0), M("Execution: Install"), HSFEE); @@ -1620,8 +1616,7 @@ class SetHook0_test : public beast::unit_test::suite jv[jss::Flags] = hsfOVERRIDE; jv[jss::HookOn] = "0000000000000000000000000000000000000000000000000000000000" - "0000" - "00"; + "000000"; env(ripple::test::jtx::hook(alice, {{jv}}, 0), M("Execution: Install"), HSFEE); @@ -1655,18 +1650,256 @@ class SetHook0_test : public beast::unit_test::suite deleteHook(alice); } + for (auto const& withFix : {false, true}) + { + // test fixHookOnV2InstallUpdate preflight + auto f = (features | featureHookOnV2) - fixHookOnV2InstallUpdate; + if (withFix) + f = f | fixHookOnV2InstallUpdate; + Env env{*this, f}; + + env.fund(XRP(10000), alice, bob); + env.close(); + + // 1. Create Hook with HookOnIncoming/Outgoing to alice + auto jv = hso(accept_wasm); + jv.removeMember(jss::HookOn); + jv[jss::HookOnIncoming] = + "fffffffffffffffffffffffffffffffffffffff7ffffffffffffffffff" + "bfffff"; // Invoke high + jv[jss::HookOnOutgoing] = + "ffffffffffffffffffffffffffffffffffffffffffffffffffffffffff" + "bffffe"; // Payment high + env(ripple::test::jtx::hook(alice, {{jv}}, 0), HSFEE); + env.close(); + + TER const expected = + env.current()->rules().enabled(fixHookOnV2InstallUpdate) + ? TER(temMALFORMED) + : TER(tesSUCCESS); + + // 2. Install Hook with HookOn, Incoming/Outgoing to bob + jv = Json::Value{}; + jv[jss::HookHash] = accept_hash_str; + jv[jss::Flags] = hsfOVERRIDE; + jv[jss::HookOn] = + "0000000000000000000000000000000000000000000000000000000000" + "000000"; + jv[jss::HookOnIncoming] = + "fffffffffffffffffffffffffffffffffffffff7ffffffffffffffffff" + "bfffff"; // Invoke high + jv[jss::HookOnOutgoing] = + "ffffffffffffffffffffffffffffffffffffffffffffffffffffffffff" + "bffffe"; // Payment high + env(ripple::test::jtx::hook(bob, {{jv}}, 0), HSFEE, ter(expected)); + env.close(); + + // 3. Update Hook with HookOn, Incoming/Outgoing to alice + jv = Json::Value{}; + jv[jss::HookOn] = + "0000000000000000000000000000000000000000000000000000000000" + "000000"; + jv[jss::HookOnIncoming] = + "fffffffffffffffffffffffffffffffffffffff7ffffffffffffffffff" + "bfffff"; // Invoke high + jv[jss::HookOnOutgoing] = + "ffffffffffffffffffffffffffffffffffffffffffffffffffffffffff" + "bffffe"; // Payment high + env(ripple::test::jtx::hook(alice, {{jv}}, 0), + HSFEE, + ter(expected)); + env.close(); + } + + for (auto const& withFix : {false, true}) + { + // test fixHookOnV2InstallUpdate install/update + auto f = (features | featureHookOnV2) - fixHookOnV2InstallUpdate; + if (withFix) + f = f | fixHookOnV2InstallUpdate; + + { + // install:: HookDefinition: HookOn -> Hook:Incoming/Outgoing + // update:: Hook: HookOn -> Hook:Incoming/Outgoing + Env env{*this, f}; + + env.fund(XRP(10000), alice, bob); + env.close(); + + // Create Hook with HookOn to alice + auto jv = hso(accept_wasm); + jv.removeMember(jss::HookOn); + jv[jss::HookOn] = + "0000000000000000000000000000000000000000000000000000000000" + "000000"; + env(ripple::test::jtx::hook(alice, {{jv}}, 0), HSFEE); + env.close(); + + // Install Hook with Incoming/Outgoing to bob + jv = Json::Value{}; + jv[jss::HookHash] = accept_hash_str; + jv[jss::Flags] = hsfOVERRIDE; + jv[jss::HookOnIncoming] = + "fffffffffffffffffffffffffffffffffffffff7ffffffffffffffffff" + "bfffff"; // Invoke high + jv[jss::HookOnOutgoing] = + "ffffffffffffffffffffffffffffffffffffffffffffffffffffffffff" + "bffffe"; // Payment high + env(ripple::test::jtx::hook(bob, {{jv}}, 0), HSFEE); + env.close(); + { + auto const hooksObj = env.le(keylet::hook(bob)); + BEAST_EXPECT(hooksObj && hooksObj->isFieldPresent(sfHooks)); + auto const& hooks = hooksObj->getFieldArray(sfHooks); + BEAST_EXPECT(hooks.size() == 1); + auto const& h = hooks[0]; + BEAST_EXPECT(!h.isFieldPresent(sfHookOn)); + BEAST_EXPECT(h.isFieldPresent(sfHookOnOutgoing)); + BEAST_EXPECT(h.isFieldPresent(sfHookOnIncoming)); + } + + // Update Hook with HookOn, Incoming/Outgoing to alice + jv = Json::Value{}; + jv[jss::HookOnIncoming] = + "fffffffffffffffffffffffffffffffffffffff7ffffffffffffffffff" + "bfffff"; // Invoke high + jv[jss::HookOnOutgoing] = + "ffffffffffffffffffffffffffffffffffffffffffffffffffffffffff" + "bffffe"; // Payment high + env(ripple::test::jtx::hook(alice, {{jv}}, 0), HSFEE); + env.close(); + { + auto const hooksObj = env.le(keylet::hook(alice)); + BEAST_EXPECT(hooksObj && hooksObj->isFieldPresent(sfHooks)); + auto const& hooks = hooksObj->getFieldArray(sfHooks); + BEAST_EXPECT(hooks.size() == 1); + auto const& h = hooks[0]; + BEAST_EXPECT(!h.isFieldPresent(sfHookOn)); + BEAST_EXPECT(h.isFieldPresent(sfHookOnOutgoing)); + BEAST_EXPECT(h.isFieldPresent(sfHookOnIncoming)); + } + + // Re-update Hook with HookOn, Incoming/Outgoing to alice + jv = Json::Value{}; + jv[jss::HookOn] = + "fffffffffffffffffffffffffffffffffffffff7ffffffffffffffffff" + "bfffff"; + env(ripple::test::jtx::hook(alice, {{jv}}, 0), HSFEE); + env.close(); + { + auto const hooksObj = env.le(keylet::hook(alice)); + BEAST_EXPECT(hooksObj && hooksObj->isFieldPresent(sfHooks)); + auto const& hooks = hooksObj->getFieldArray(sfHooks); + BEAST_EXPECT(hooks.size() == 1); + auto const& h = hooks[0]; + BEAST_EXPECT(h.isFieldPresent(sfHookOn)); + if (withFix) + { + BEAST_EXPECT(!h.isFieldPresent(sfHookOnOutgoing)); + BEAST_EXPECT(!h.isFieldPresent(sfHookOnIncoming)); + } + else + { + BEAST_EXPECT(h.isFieldPresent(sfHookOnOutgoing)); + BEAST_EXPECT(h.isFieldPresent(sfHookOnIncoming)); + } + } + } + + { + // install:: HookDefinition: Incoming/Outgoing -> Hook:HookOn + // update:: Hook: Incoming/Outgoing -> Hook:HookOn + Env env{*this, f}; + + env.fund(XRP(10000), alice, bob); + env.close(); + + // Create Hook with HookOn to alice + auto jv = hso(accept_wasm); + jv.removeMember(jss::HookOn); + jv[jss::HookOnIncoming] = + "fffffffffffffffffffffffffffffffffffffff7ffffffffffffffffff" + "bfffff"; // Invoke high + jv[jss::HookOnOutgoing] = + "ffffffffffffffffffffffffffffffffffffffffffffffffffffffffff" + "bffffe"; // Payment high + env(ripple::test::jtx::hook(alice, {{jv}}, 0), HSFEE); + env.close(); + + // Install Hook with Incoming/Outgoing to bob + jv = Json::Value{}; + jv[jss::HookHash] = accept_hash_str; + jv[jss::Flags] = hsfOVERRIDE; + jv[jss::HookOn] = + "0000000000000000000000000000000000000000000000000000000000" + "000000"; + env(ripple::test::jtx::hook(bob, {{jv}}, 0), HSFEE); + env.close(); + { + auto const hooksObj = env.le(keylet::hook(bob)); + BEAST_EXPECT(hooksObj && hooksObj->isFieldPresent(sfHooks)); + auto const& hooks = hooksObj->getFieldArray(sfHooks); + BEAST_EXPECT(hooks.size() == 1); + auto const& h = hooks[0]; + BEAST_EXPECT(h.isFieldPresent(sfHookOn)); + BEAST_EXPECT(!h.isFieldPresent(sfHookOnOutgoing)); + BEAST_EXPECT(!h.isFieldPresent(sfHookOnIncoming)); + } + + // Update Hook with HookOn, Incoming/Outgoing to alice + jv = Json::Value{}; + jv[jss::HookOn] = + "0000000000000000000000000000000000000000000000000000000000" + "000000"; + env(ripple::test::jtx::hook(alice, {{jv}}, 0), HSFEE); + env.close(); + { + auto const hooksObj = env.le(keylet::hook(alice)); + BEAST_EXPECT(hooksObj && hooksObj->isFieldPresent(sfHooks)); + auto const& hooks = hooksObj->getFieldArray(sfHooks); + BEAST_EXPECT(hooks.size() == 1); + auto const& h = hooks[0]; + BEAST_EXPECT(h.isFieldPresent(sfHookOn)); + BEAST_EXPECT(!h.isFieldPresent(sfHookOnOutgoing)); + BEAST_EXPECT(!h.isFieldPresent(sfHookOnIncoming)); + } + + // Re-update Hook with HookOn, Incoming/Outgoing to alice + jv = Json::Value{}; + jv[jss::HookOnIncoming] = + "fffffffffffffffffffffffffffffffffffffff7ffffffffffffffffff" + "bffff0"; // Invoke high + jv[jss::HookOnOutgoing] = + "ffffffffffffffffffffffffffffffffffffffffffffffffffffffffff" + "bffff1"; // Payment high + env(ripple::test::jtx::hook(alice, {{jv}}, 0), HSFEE); + env.close(); + { + auto const hooksObj = env.le(keylet::hook(alice)); + BEAST_EXPECT(hooksObj && hooksObj->isFieldPresent(sfHooks)); + auto const& hooks = hooksObj->getFieldArray(sfHooks); + BEAST_EXPECT(hooks.size() == 1); + auto const& h = hooks[0]; + if (withFix) + BEAST_EXPECT(!h.isFieldPresent(sfHookOn)); + else + BEAST_EXPECT(h.isFieldPresent(sfHookOn)); + BEAST_EXPECT(h.isFieldPresent(sfHookOnOutgoing)); + BEAST_EXPECT(h.isFieldPresent(sfHookOnIncoming)); + } + } + } + // Fee RPC { auto jv = hso(accept_wasm); jv.removeMember(jss::HookOn); jv[jss::HookOnIncoming] = "fffffffffffffffffffffffffffffffffffffff7ffffffffffffffffff" - "bfff" - "ff"; // Invoke high + "bfffff"; // Invoke high jv[jss::HookOnOutgoing] = "ffffffffffffffffffffffffffffffffffffffffffffffffffffffffff" - "bfff" - "fe"; // Payment high + "bffffe"; // Payment high env(ripple::test::jtx::hook(alice, {{jv}}, 0), HSFEE); env.close(); diff --git a/src/xrpld/app/tx/detail/SetHook.cpp b/src/xrpld/app/tx/detail/SetHook.cpp index 53b3b6b7e1..6636920838 100644 --- a/src/xrpld/app/tx/detail/SetHook.cpp +++ b/src/xrpld/app/tx/detail/SetHook.cpp @@ -244,6 +244,61 @@ SetHook::inferOperation(STObject const& hookSetObj) : hsoUPDATE; } +bool +validateHookOn(SetHookCtx& ctx, STObject const& hookSetObj) +{ + if (!hookSetObj.isFieldPresent(sfHookOn)) + { + if (!ctx.rules.enabled(featureHookOnV2)) + { + JLOG(ctx.j.trace()) + << "HookSet(" << hook::log::HOOKON_MISSING << ")[" << HS_ACC() + << "]: Malformed transaction: SetHook must include " + "sfHookOn before featureHookOnV2 is enabled."; + return false; + } + + if (!hookSetObj.isFieldPresent(sfHookOnOutgoing) || + !hookSetObj.isFieldPresent(sfHookOnIncoming)) + { + JLOG(ctx.j.trace()) + << "HookSet(" << hook::log::HOOKON_MISSING << ")[" << HS_ACC() + << "]: Malformed transaction: SetHook must include " + "sfHookOnOutgoing and sfHookOnIncoming " + "when creating a new hook without sfHookOn."; + return false; + } + + auto const outgoing = hookSetObj.getFieldH256(sfHookOnOutgoing); + auto const incoming = hookSetObj.getFieldH256(sfHookOnIncoming); + if (outgoing == incoming) + { + JLOG(ctx.j.trace()) + << "HookSet(" << hook::log::HOOKON_MISSING << ")[" << HS_ACC() + << "]: Malformed transaction: SetHook outgoing and " + "incoming hookon must be different."; + return false; + } + + return true; + } + else + { + if (hookSetObj.isFieldPresent(sfHookOnOutgoing) || + hookSetObj.isFieldPresent(sfHookOnIncoming)) + { + JLOG(ctx.j.trace()) + << "HookSet(" << hook::log::HOOKON_MISSING << ")[" << HS_ACC() + << "]: Malformed transaction: SetHook must no" + "include sfHookOnOutgoing and sfHookOnIncoming " + "when creating a new hook with sfHookOn."; + return false; + } + } + + return true; +} + // This is a context-free validation, it does not take into account the current // state of the ledger returns < valid, instruction count > may throw // overflow_error @@ -363,6 +418,13 @@ SetHook::validateHookSetEntry(SetHookCtx& ctx, STObject const& hookSetObj) // hookon may be present if the user so chooses // flags may be present if the user so chooses + if (ctx.rules.enabled(fixHookOnV2InstallUpdate) && + (hookSetObj.isFieldPresent(sfHookOn) || + hookSetObj.isFieldPresent(sfHookOnOutgoing) || + hookSetObj.isFieldPresent(sfHookOnIncoming)) && + !validateHookOn(ctx, hookSetObj)) + return false; + return true; } @@ -407,6 +469,13 @@ SetHook::validateHookSetEntry(SetHookCtx& ctx, STObject const& hookSetObj) // hookon may be present if the user so chooses // flags may be present if the user so chooses + if (ctx.rules.enabled(fixHookOnV2InstallUpdate) && + (hookSetObj.isFieldPresent(sfHookOn) || + hookSetObj.isFieldPresent(sfHookOnOutgoing) || + hookSetObj.isFieldPresent(sfHookOnIncoming)) && + !validateHookOn(ctx, hookSetObj)) + return false; + return true; } @@ -456,56 +525,8 @@ SetHook::validateHookSetEntry(SetHookCtx& ctx, STObject const& hookSetObj) } // validate sfHookOn - if (!hookSetObj.isFieldPresent(sfHookOn)) - { - if (!ctx.rules.enabled(featureHookOnV2)) - { - JLOG(ctx.j.trace()) - << "HookSet(" << hook::log::HOOKON_MISSING << ")[" - << HS_ACC() - << "]: Malformed transaction: SetHook must include " - "sfHookOn before featureHookOnV2 is enabled."; - return false; - } - - if (!hookSetObj.isFieldPresent(sfHookOnOutgoing) || - !hookSetObj.isFieldPresent(sfHookOnIncoming)) - { - JLOG(ctx.j.trace()) - << "HookSet(" << hook::log::HOOKON_MISSING << ")[" - << HS_ACC() - << "]: Malformed transaction: SetHook must include " - "sfHookOnOutgoing and sfHookOnIncoming " - "when creating a new hook without sfHookOn."; - return false; - } - - auto const outgoing = hookSetObj.getFieldH256(sfHookOnOutgoing); - auto const incoming = hookSetObj.getFieldH256(sfHookOnIncoming); - if (outgoing == incoming) - { - JLOG(ctx.j.trace()) - << "HookSet(" << hook::log::HOOKON_MISSING << ")[" - << HS_ACC() - << "]: Malformed transaction: SetHook outgoing and " - "incoming hookon must be different."; - return false; - } - } - else - { - if (hookSetObj.isFieldPresent(sfHookOnOutgoing) || - hookSetObj.isFieldPresent(sfHookOnIncoming)) - { - JLOG(ctx.j.trace()) - << "HookSet(" << hook::log::HOOKON_MISSING << ")[" - << HS_ACC() - << "]: Malformed transaction: SetHook must no" - "include sfHookOnOutgoing and sfHookOnIncoming " - "when creating a new hook with sfHookOn."; - return false; - } - } + if (!validateHookOn(ctx, hookSetObj)) + return false; // validate sfHookCanEmit // HookCanEmit field is an optional field for backward compatibility @@ -1627,6 +1648,17 @@ SetHook::setHook() newHook.setFieldH256(sfHookNamespace, *newNamespace); } + if (ctx.rules.enabled(fixHookOnV2InstallUpdate)) + { + // sanity check + if (newHookOn && (newHookOnOutgoing || newHookOnIncoming)) + return tecINTERNAL; // LCOV_EXCL_LINE + + if ((!newHookOnOutgoing && newHookOnIncoming) || + (newHookOnOutgoing && !newHookOnIncoming)) + return tecINTERNAL; // LCOV_EXCL_LINE + } + // set the hookon field if it differs from definition if (newHookOn) { @@ -1665,6 +1697,22 @@ SetHook::setHook() sfHookOnIncoming, *newHookOnIncoming); } + if (ctx.rules.enabled(fixHookOnV2InstallUpdate)) + { + if (newHookOn) + { + if (newHook.isFieldPresent(sfHookOnIncoming)) + newHook.makeFieldAbsent(sfHookOnIncoming); + if (newHook.isFieldPresent(sfHookOnOutgoing)) + newHook.makeFieldAbsent(sfHookOnOutgoing); + } + if (newHookOnOutgoing || newHookOnIncoming) + { + if (newHook.isFieldPresent(sfHookOn)) + newHook.makeFieldAbsent(sfHookOn); + } + } + // set the hookcanemit field if it differs from definition if (newHookCanEmit) {