Allow update JSHook Fee on Install/Update (#577)

* Allow update JSHook Fee on Install/Update, handle Instruction Limit Reach properly

* use `JS_FEE_OUT_OF_RANGE` as hook_log_code  instead `JS_FEE_TOO_HIGH`
This commit is contained in:
tequ
2025-10-09 18:59:51 +09:00
committed by GitHub
parent c44b50b98c
commit 21a5e82fad
6 changed files with 471 additions and 293 deletions

View File

@@ -227,7 +227,7 @@ enum hook_log_code : uint16_t {
INTERNAL_ERROR = 87, // an internal error described by the log text
JS_TEST_FAILURE = 88, // smoke test of js bytecode failed
JS_FEE_MISSING = 89,
JS_FEE_TOO_HIGH = 90,
JS_FEE_OUT_OF_RANGE = 90,
// RH NOTE: only HookSet msgs got log codes, possibly all Hook log lines
// should get a code?
};
@@ -347,6 +347,7 @@ enum ExitType : uint8_t {
JSVM_ERROR = 4,
LEDGER_ERROR =
5, // if the ledger contained for example a nonsense hookapi number
INSTRUCTION_LIMIT_REACHED = 6,
};
enum CodeType : uint8_t {

View File

@@ -1437,7 +1437,6 @@ public:
if (JS_IsException(val))
{
JS_FreeValue(ctx, val);
JS_FreeValue(ctx, obj);
JLOG(j.warn()) << "HookError[" << HC_ACC()
<< "]: Could not create QUICKJS instance (bytecode "
@@ -1490,6 +1489,12 @@ public:
{
normal_exit = 1;
}
else if (m == "Instruction limit reached")
{
normal_exit = 1;
hookCtx.result.exitType =
hook_api::ExitType::INSTRUCTION_LIMIT_REACHED;
}
else
{
JLOG(j.warn()) << "HookError[" << HC_ACC()

View File

@@ -217,12 +217,15 @@ validateJSHookFee(SetHookCtx& ctx, STObject const& hookSetObj)
return false;
}
uint64_t fee = amt.xrp().drops();
if (amt < beast::zero || fee < 1 || fee > 1000000)
// If the fee is 1, JS_EvalFunction will result in InstructionLimitReached,
// so it is necessary to require 2 or more.
if (amt < beast::zero || fee < 2 || fee > 1000000)
{
JLOG(ctx.j.trace())
<< "HookSet(" << hook::log::JS_FEE_TOO_HIGH << ")[" << HS_ACC()
<< "HookSet(" << hook::log::JS_FEE_OUT_OF_RANGE << ")[" << HS_ACC()
<< "]: Malformed transaction: When using a "
"JS Hook you must include a Fee <= 1000000.";
"JS Hook you must include a Fee >= 2 and <= 1000000.";
return false;
}
@@ -1639,7 +1642,7 @@ SetHook::setHook()
if (newFee)
{
if (oldFee.has_value() && *oldFee == *newFee)
if (defFee && *defFee == *newFee)
{
if (newHook.isFieldPresent(sfFee))
newHook.makeFieldAbsent(sfFee);

View File

@@ -1264,7 +1264,9 @@ Transactor::executeHookChain(
uint16_t hookApiVersion = hookDef->getFieldU16(sfHookApiVersion);
uint32_t fee = (uint32_t)(hookDef->getFieldAmount(sfFee).xrp().drops());
uint32_t fee = hookObj.isFieldPresent(sfFee)
? (uint32_t)(hookObj.getFieldAmount(sfFee).xrp().drops())
: (uint32_t)(hookDef->getFieldAmount(sfFee).xrp().drops());
try
{
@@ -1413,8 +1415,9 @@ Transactor::doHookCallback(
? hookObj.getFieldH256(sfHookNamespace)
: hookDef->getFieldH256(sfHookNamespace));
uint64_t instructionLimit =
hookDef->getFieldAmount(sfFee).xrp().drops();
uint32_t instructionLimit = hookObj.isFieldPresent(sfFee)
? (uint32_t)hookObj.getFieldAmount(sfFee).xrp().drops()
: (uint32_t)hookDef->getFieldAmount(sfFee).xrp().drops();
std::map<std::vector<uint8_t>, std::vector<uint8_t>> parameters;
if (hook::gatherHookParameters(hookDef, hookObj, parameters, j_))
@@ -1699,8 +1702,9 @@ Transactor::doAgainAsWeak(
return;
}
uint32_t instructionLimit =
(uint32_t)(hookDef->getFieldAmount(sfFee).xrp().drops());
uint32_t instructionLimit = hookObj.isFieldPresent(sfFee)
? (uint32_t)(hookObj.getFieldAmount(sfFee).xrp().drops())
: (uint32_t)(hookDef->getFieldAmount(sfFee).xrp().drops());
try
{

View File

@@ -299,17 +299,31 @@ public:
{
Json::Value iv;
iv[jss::HookHash] = accept_hash_str;
iv[jss::Fee] = "100";
jv[jss::Hooks][0U][jss::Hook] = iv;
env(jv,
M("Hook Install operation can set extant hook hash"),
HSFEE,
ter(tesSUCCESS));
env.close();
auto const hook = env.le(keylet::hook(Account("alice").id()));
BEAST_REQUIRE(hook);
BEAST_REQUIRE(hook->isFieldPresent(sfHooks));
auto const& hooks = hook->getFieldArray(sfHooks);
BEAST_EXPECT(hooks.size() == 1);
BEAST_EXPECT(hooks[0].isFieldPresent(sfHookHash));
BEAST_EXPECT(hooks[0].getFieldH256(sfHookHash) == accept_hash);
// check there Fee exist
BEAST_REQUIRE(hooks[0].isFieldPresent(sfFee));
BEAST_EXPECT(hooks[0].getFieldAmount(sfFee).xrp().drops() == 100);
}
// can't set with invalid fee
{
for (STAmount _fee : {drops(0), drops(1000001), bob["USD"](0)})
for (STAmount _fee :
{drops(0), drops(1), drops(1000001), bob["USD"](0)})
{
Json::Value iv;
iv[jss::HookHash] = accept_hash_str;
@@ -793,7 +807,8 @@ public:
// invalid fee
{
for (STAmount _fee : {drops(0), drops(1000001), bob["USD"](0)})
for (STAmount _fee :
{drops(0), drops(1), drops(1000001), bob["USD"](0)})
{
Json::Value iv;
iv[jss::CreateCode] = strHex(accept_wasm);
@@ -1040,7 +1055,8 @@ public:
// fee should be set with valid fee
{
for (STAmount _fee : {drops(0), drops(1000001), bob["USD"](0)})
for (STAmount _fee :
{drops(0), drops(1), drops(1000001), bob["USD"](0)})
{
Json::Value iv;
iv[jss::Fee] = _fee.getJson(JsonOptions::none);
@@ -1363,6 +1379,53 @@ public:
BEAST_REQUIRE(!hooks[0].isFieldPresent(sfHookParameters));
}
// update Fee
{
Json::Value iv;
iv[jss::Fee] = "101";
jv[jss::Hooks][0U] = Json::Value{};
jv[jss::Hooks][0U][jss::Hook] = iv;
env(jv, M("Update Fee"), HSFEE);
env.close();
// ensure hook still exists
auto const hook = env.le(keylet::hook(Account("alice").id()));
BEAST_REQUIRE(hook);
BEAST_REQUIRE(hook->isFieldPresent(sfHooks));
auto const& hooks = hook->getFieldArray(sfHooks);
BEAST_EXPECT(hooks.size() == 1);
BEAST_EXPECT(hooks[0].isFieldPresent(sfHookHash));
BEAST_EXPECT(hooks[0].getFieldH256(sfHookHash) == accept_hash);
// check there Fee exist
BEAST_REQUIRE(hooks[0].isFieldPresent(sfFee));
BEAST_EXPECT(hooks[0].getFieldAmount(sfFee).xrp().drops() == 101);
}
// reset Fee
{
Json::Value iv;
iv[jss::Fee] = "100"; // HookDefinition value
jv[jss::Hooks][0U] = Json::Value{};
jv[jss::Hooks][0U][jss::Hook] = iv;
env(jv, M("Update Fee"), HSFEE);
env.close();
// ensure hook still exists
auto const hook = env.le(keylet::hook(Account("alice").id()));
BEAST_REQUIRE(hook);
BEAST_REQUIRE(hook->isFieldPresent(sfHooks));
auto const& hooks = hook->getFieldArray(sfHooks);
BEAST_EXPECT(hooks.size() == 1);
BEAST_EXPECT(hooks[0].isFieldPresent(sfHookHash));
BEAST_EXPECT(hooks[0].getFieldH256(sfHookHash) == accept_hash);
// check there Fee does'nt exist
BEAST_REQUIRE(!hooks[0].isFieldPresent(sfFee));
}
// try to set each type of field on a non existent hook
{
Json::Value params{Json::arrayValue};
@@ -1577,6 +1640,75 @@ public:
// ter(temMALFORMED));
// }
void
testInstructionCount(FeatureBitset features)
{
testcase("Test InstructionCount");
using namespace jtx;
Env env{*this, features};
auto const alice = Account{"alice"};
auto const bob = Account{"bob"};
env.fund(XRP(10000), alice);
env.fund(XRP(10000), bob);
TestHook hook = jswasm[R"[test.hook](
const Hook = (reserved) => {
let num = 0;
for (let i = 0; i < 1000; i++){
num++;
}
return accept("", num);
}
)[test.hook]"];
env(ripple::test::jtx::hook(alice, {{hsov1(hook, 1, HSDROPS)}}, 0),
M("Install InstructionCount Hook"),
HSFEE);
env.close();
env(pay(bob, alice, XRP(1)),
M("Test InstructionCount Hook"),
fee(XRP(1)));
env.close();
auto const meta = env.meta();
BEAST_REQUIRE(meta->isFieldPresent(sfHookExecutions));
auto const hookExecutions = meta->getFieldArray(sfHookExecutions);
BEAST_REQUIRE(hookExecutions.size() == 1);
auto const instructionCount =
hookExecutions[0].getFieldU64(sfHookInstructionCount);
BEAST_EXPECT(instructionCount == 0x7d5);
// reset
env(ripple::test::jtx::hook(alice, {{hso_delete()}}, 0));
env.close();
env(ripple::test::jtx::hook(
alice, {{hsov1(hook, 1, drops(instructionCount - 1))}}, 0),
M("Install InstructionCount Hook 2"),
HSFEE);
env.close();
env(pay(bob, alice, XRP(1)),
M("Test InstructionCount Hook 2"),
fee(XRP(1)),
ter(tecHOOK_REJECTED));
env.close();
BEAST_REQUIRE(meta->isFieldPresent(sfHookExecutions));
auto const hookExecutions2 =
env.meta()->getFieldArray(sfHookExecutions);
BEAST_REQUIRE(hookExecutions2.size() == 1);
auto const instructionCount2 =
hookExecutions2[0].getFieldU64(sfHookInstructionCount);
BEAST_EXPECT(instructionCount2 == 0x7d4);
auto const hookResult = hookExecutions2[0].getFieldU8(sfHookResult);
BEAST_EXPECT(
hookResult == hook_api::ExitType::INSTRUCTION_LIMIT_REACHED);
}
void
test_accept(FeatureBitset features)
{
@@ -11111,6 +11243,8 @@ public:
TEST_TARGET_PATTERN(a, target, testUpdate, features);
TEST_TARGET_PATTERN(a, target, testWithTickets, features);
TEST_TARGET_PATTERN(a, target, testInstructionCount, features);
// // DA TODO: illegalfunc_wasm
// // TEST_TARGET_PATTERN(a, target, testWasm, features);
TEST_TARGET_PATTERN(a, target, test_accept, features);

File diff suppressed because it is too large Load Diff