fix and simplify hook-state

This commit is contained in:
Richard Holland
2022-02-08 10:00:04 +00:00
parent 249c127890
commit 7596d8bbd0
4 changed files with 98 additions and 53 deletions

View File

@@ -210,8 +210,8 @@
} }
// ptr = pointer inside the wasm memory space // ptr = pointer inside the wasm memory space
#define NOT_IN_BOUNDS(ptr, len, memory_length)\ #define NOT_IN_BOUNDS(ptr, len, memory_length)\
(ptr > memory_length || \ ((static_cast<uint64_t>(ptr) > static_cast<uint64_t>(memory_length)) || \
static_cast<uint64_t>(ptr) + static_cast<uint64_t>(len) > static_cast<uint64_t>(memory_length)) ((static_cast<uint64_t>(ptr) + static_cast<uint64_t>(len)) > static_cast<uint64_t>(memory_length)))
#define HOOK_EXIT(read_ptr, read_len, error_code, exit_type)\ #define HOOK_EXIT(read_ptr, read_len, error_code, exit_type)\
{\ {\

View File

@@ -1505,7 +1505,8 @@ updateHookParameters(
newParameters.push_back(std::move(param)); newParameters.push_back(std::move(param));
} }
newHook.setFieldArray(sfHookParameters, std::move(newParameters)); if (newParameters.size() > 0)
newHook.setFieldArray(sfHookParameters, std::move(newParameters));
return tesSUCCESS; return tesSUCCESS;
} }
@@ -1604,6 +1605,9 @@ SetHook::setHook()
uint32_t flags = hookSetObj->isFieldPresent(sfFlags) ? hookSetObj->getFieldU32(sfFlags) : 0; uint32_t flags = hookSetObj->isFieldPresent(sfFlags) ? hookSetObj->getFieldU32(sfFlags) : 0;
HookSetOperation op = inferOperation(*hookSetObj);
// if an existing hook exists at this position in the chain then extract the relevant fields // if an existing hook exists at this position in the chain then extract the relevant fields
if (oldHook && oldHook->get().isFieldPresent(sfHookHash)) if (oldHook && oldHook->get().isFieldPresent(sfHookHash))
{ {
@@ -1628,7 +1632,6 @@ SetHook::setHook()
oldHookOn = *defHookOn; oldHookOn = *defHookOn;
} }
// in preparation for three way merge populate fields if they are present on the HookSetObj // in preparation for three way merge populate fields if they are present on the HookSetObj
if (hookSetObj->isFieldPresent(sfHookHash)) if (hookSetObj->isFieldPresent(sfHookHash))
{ {
@@ -1645,7 +1648,6 @@ SetHook::setHook()
newDirKeylet = keylet::hookStateDir(account_, *newNamespace); newDirKeylet = keylet::hookStateDir(account_, *newNamespace);
} }
HookSetOperation op = inferOperation(*hookSetObj);
// users may destroy a namespace in any operation except NOOP and INVALID // users may destroy a namespace in any operation except NOOP and INVALID
if (op != hsoNOOP && op != hsoINVALID && (flags & hsoNSDELETE) && oldDirSLE) if (op != hsoNOOP && op != hsoINVALID && (flags & hsoNSDELETE) && oldDirSLE)
@@ -1702,8 +1704,13 @@ SetHook::setHook()
} }
// decrement the hook definition and mark it for deletion if appropriate // decrement the hook definition and mark it for deletion if appropriate
if (oldDefSLE && reduceReferenceCount(oldDefSLE)) if (oldDefSLE)
defsToDestroy.emplace(*oldDefKeylet); {
if (reduceReferenceCount(oldDefSLE))
defsToDestroy.emplace(*oldDefKeylet);
view().update(oldDefSLE);
}
continue; continue;
} }
@@ -1743,6 +1750,7 @@ SetHook::setHook()
<< "]: SetHook create operation would override but hsfOVERRIDE flag wasn't specified"; << "]: SetHook create operation would override but hsfOVERRIDE flag wasn't specified";
return tecREQUIRES_FLAG; return tecREQUIRES_FLAG;
} }
ripple::Blob wasmBytes = hookSetObj->getFieldVL(sfCreateCode); ripple::Blob wasmBytes = hookSetObj->getFieldVL(sfCreateCode);
@@ -1766,6 +1774,7 @@ SetHook::setHook()
// update reference count // update reference count
newDefSLE = view().peek(keylet); newDefSLE = view().peek(keylet);
newDefKeylet = keylet; newDefKeylet = keylet;
// this falls through to hsoINSTALL // this falls through to hsoINSTALL
} }
else else
@@ -1795,6 +1804,15 @@ SetHook::setHook()
<< "]: Malformed transaction: SetHook operation would create invalid hook wasm"; << "]: Malformed transaction: SetHook operation would create invalid hook wasm";
return tecINTERNAL; return tecINTERNAL;
} }
// decrement the hook definition and mark it for deletion if appropriate
if (oldDefSLE)
{
if (reduceReferenceCount(oldDefSLE))
defsToDestroy.emplace(*oldDefKeylet);
view().update(oldDefSLE);
}
auto newHookDef = std::make_shared<SLE>( keylet ); auto newHookDef = std::make_shared<SLE>( keylet );
newHookDef->setFieldH256(sfHookHash, *createHookHash); newHookDef->setFieldH256(sfHookHash, *createHookHash);
@@ -1838,10 +1856,15 @@ SetHook::setHook()
return tecNO_ENTRY; return tecNO_ENTRY;
} }
// decrement references of old hook definition // decrement the hook definition and mark it for deletion if appropriate
if (oldHook && reduceReferenceCount(oldDefSLE)) if (oldDefSLE)
{
if (reduceReferenceCount(oldDefSLE))
defsToDestroy.emplace(*oldDefKeylet); defsToDestroy.emplace(*oldDefKeylet);
view().update(oldDefSLE);
}
// set the hookhash on the new hook, and allow for a fall through event from hsoCREATE // set the hookhash on the new hook, and allow for a fall through event from hsoCREATE
if (!createHookHash) if (!createHookHash)
createHookHash = hookSetObj->getFieldH256(sfHookHash); createHookHash = hookSetObj->getFieldH256(sfHookHash);
@@ -1851,6 +1874,10 @@ SetHook::setHook()
// increment reference count of target HookDefintion // increment reference count of target HookDefintion
incrementReferenceCount(newDefSLE); incrementReferenceCount(newDefSLE);
// change which definition we're using to the new target
defNamespace = newDefSLE->getFieldH256(sfHookNamespace);
defHookOn = newDefSLE->getFieldU64(sfHookOn);
// set the namespace if it differs from the definition namespace // set the namespace if it differs from the definition namespace
if (newNamespace && *defNamespace != *newNamespace) if (newNamespace && *defNamespace != *newNamespace)
newHook.setFieldH256(sfHookNamespace, *newNamespace); newHook.setFieldH256(sfHookNamespace, *newNamespace);
@@ -1861,7 +1888,7 @@ SetHook::setHook()
// parameters // parameters
TER result = TER result =
updateHookParameters(ctx, *hookSetObj, oldDefSLE, newHook); updateHookParameters(ctx, *hookSetObj, newDefSLE, newHook);
if (result != tesSUCCESS) if (result != tesSUCCESS)
return result; return result;
@@ -1871,6 +1898,9 @@ SetHook::setHook()
newHook.setFieldArray(sfHookGrants, hookSetObj->getFieldArray(sfHookGrants)); newHook.setFieldArray(sfHookGrants, hookSetObj->getFieldArray(sfHookGrants));
newHooks.push_back(std::move(newHook)); newHooks.push_back(std::move(newHook));
view().update(newDefSLE);
continue; continue;
} }

View File

@@ -327,12 +327,14 @@ hook::setHookState(
auto& view = applyCtx.view(); auto& view = applyCtx.view();
auto j = applyCtx.app.journal("View"); auto j = applyCtx.app.journal("View");
auto const sle = auto const sleAccount =
( acc == hookResult.account ? ( acc == hookResult.account ?
view.peek(hookResult.accountKeylet) : view.peek(hookResult.accountKeylet) :
view.peek(ripple::keylet::account(acc))); view.peek(ripple::keylet::account(acc)));
if (!sle) JLOG(j.trace()) << "HookState[" << acc << "]: hookResult.account = " << hookResult.account;
if (!sleAccount)
return tefINTERNAL; return tefINTERNAL;
// if the blob is too large don't set it // if the blob is too large don't set it
@@ -342,10 +344,12 @@ hook::setHookState(
auto hookStateKeylet = ripple::keylet::hookState(acc, key, ns); auto hookStateKeylet = ripple::keylet::hookState(acc, key, ns);
auto hookStateDirKeylet = ripple::keylet::hookStateDir(acc, ns); auto hookStateDirKeylet = ripple::keylet::hookStateDir(acc, ns);
uint32_t stateCount = sle->getFieldU32(sfHookStateCount); uint32_t stateCount = sleAccount->getFieldU32(sfHookStateCount);
uint32_t oldStateReserve = COMPUTE_HOOK_DATA_OWNER_COUNT(stateCount); uint32_t oldStateReserve = COMPUTE_HOOK_DATA_OWNER_COUNT(stateCount);
auto const oldHookState = view.peek(hookStateKeylet); auto hookState = view.peek(hookStateKeylet);
bool createNew = !hookState;
// if the blob is nil then delete the entry if it exists // if the blob is nil then delete the entry if it exists
if (data.size() == 0) if (data.size() == 0)
@@ -354,14 +358,17 @@ hook::setHookState(
if (!view.peek(hookStateKeylet)) if (!view.peek(hookStateKeylet))
return tesSUCCESS; // a request to remove a non-existent entry is defined as success return tesSUCCESS; // a request to remove a non-existent entry is defined as success
auto const hint = (*oldHookState)[sfOwnerNode]; if (!view.peek(hookStateDirKeylet))
return tefBAD_LEDGER;
auto const hint = (*hookState)[sfOwnerNode];
// Remove the node from the namespace directory // Remove the node from the namespace directory
if (!view.dirRemove(hookStateDirKeylet, hint, hookStateKeylet.key, false)) if (!view.dirRemove(hookStateDirKeylet, hint, hookStateKeylet.key, false))
return tefBAD_LEDGER; return tefBAD_LEDGER;
// remove the actual hook state obj // remove the actual hook state obj
view.erase(oldHookState); view.erase(hookState);
// adjust state object count // adjust state object count
if (stateCount > 0) if (stateCount > 0)
@@ -369,23 +376,23 @@ hook::setHookState(
// if removing this state entry would destroy the allotment then reduce the owner count // if removing this state entry would destroy the allotment then reduce the owner count
if (COMPUTE_HOOK_DATA_OWNER_COUNT(stateCount) < oldStateReserve) if (COMPUTE_HOOK_DATA_OWNER_COUNT(stateCount) < oldStateReserve)
adjustOwnerCount(view, sle, -1, j); adjustOwnerCount(view, sleAccount, -1, j);
sle->setFieldU32(sfHookStateCount, stateCount); sleAccount->setFieldU32(sfHookStateCount, stateCount);
view.update(sle); view.update(sleAccount);
return tesSUCCESS; return tesSUCCESS;
} }
std::uint32_t ownerCount{(*sleAccount)[sfOwnerCount]};
std::uint32_t ownerCount{(*sle)[sfOwnerCount]}; if (createNew)
{
if (oldHookState) {
view.erase(oldHookState);
} else {
++stateCount; ++stateCount;
if (COMPUTE_HOOK_DATA_OWNER_COUNT(stateCount) > oldStateReserve) { if (COMPUTE_HOOK_DATA_OWNER_COUNT(stateCount) > oldStateReserve)
{
// the hook used its allocated allotment of state entries for its previous ownercount // the hook used its allocated allotment of state entries for its previous ownercount
// increment ownercount and give it another allotment // increment ownercount and give it another allotment
@@ -393,43 +400,45 @@ hook::setHookState(
XRPAmount const newReserve{ XRPAmount const newReserve{
view.fees().accountReserve(ownerCount)}; view.fees().accountReserve(ownerCount)};
if (STAmount((*sle)[sfBalance]).xrp() < newReserve) if (STAmount((*sleAccount)[sfBalance]).xrp() < newReserve)
return tecINSUFFICIENT_RESERVE; return tecINSUFFICIENT_RESERVE;
adjustOwnerCount(view, sle, 1, j); adjustOwnerCount(view, sleAccount, 1, j);
} }
// update state count // update state count
sle->setFieldU32(sfHookStateCount, stateCount); sleAccount->setFieldU32(sfHookStateCount, stateCount);
view.update(sle); view.update(sleAccount);
// create an entry
hookState = std::make_shared<SLE>(hookStateKeylet);
} }
// add new data to ledger hookState->setFieldVL(sfHookStateData, data);
auto newHookState = std::make_shared<SLE>(hookStateKeylet); hookState->setFieldH256(sfHookStateKey, key);
view.insert(newHookState);
newHookState->setFieldVL(sfHookStateData, data);
newHookState->setFieldH256(sfHookStateKey, key);
newHookState->setAccountID(sfAccount, acc); // RH TODO remove these
newHookState->setFieldH256(sfHookNamespace, ns); // in prod
if (!oldHookState) { if (createNew)
{
// Add the hook to the account's directory if it wasn't there already // Add the hook to the account's directory if it wasn't there already
auto const page = view.dirInsert( auto const page = view.dirInsert(
hookStateDirKeylet, hookStateDirKeylet,
hookStateKeylet.key, hookStateKeylet.key,
describeOwnerDir(acc)); describeOwnerDir(acc));
JLOG(j.trace()) << "HookInfo[" << HR_ACC() << "]: "
<< "Create/update hook state: "
<< (page ? "success" : "failure");
if (!page) if (!page)
return tecDIR_FULL; return tecDIR_FULL;
newHookState->setFieldU64(sfOwnerNode, *page); hookState->setFieldU64(sfOwnerNode, *page);
// add new data to ledger
view.insert(hookState);
} }
else
view.update(hookState);
return tesSUCCESS; return tesSUCCESS;
} }
@@ -751,9 +760,9 @@ DEFINE_HOOK_FUNCTION(
if (nread_ptr == 0 && nread_len == 0 && !(aread_ptr == 0 && aread_len == 0)) if (nread_ptr == 0 && nread_len == 0 && !(aread_ptr == 0 && aread_len == 0))
return INVALID_ARGUMENT; return INVALID_ARGUMENT;
if ((nread_len && !NOT_IN_BOUNDS(nread_ptr, nread_len, memory_length)) || if ((nread_len && NOT_IN_BOUNDS(nread_ptr, nread_len, memory_length)) ||
(kread_len && !NOT_IN_BOUNDS(kread_ptr, kread_len, memory_length)) || (kread_len && NOT_IN_BOUNDS(kread_ptr, kread_len, memory_length)) ||
(aread_len && !NOT_IN_BOUNDS(aread_ptr, aread_len, memory_length))) (aread_len && NOT_IN_BOUNDS(aread_ptr, aread_len, memory_length)))
return OUT_OF_BOUNDS; return OUT_OF_BOUNDS;
uint32_t maxSize = hook::maxHookStateDataSize(); uint32_t maxSize = hook::maxHookStateDataSize();
@@ -766,7 +775,7 @@ DEFINE_HOOK_FUNCTION(
: ripple::base_uint<256>::fromVoid(memory + nread_ptr); : ripple::base_uint<256>::fromVoid(memory + nread_ptr);
ripple::AccountID acc = ripple::AccountID acc =
aread_len == 0 aread_len == 20
? AccountID::fromVoid(memory + aread_ptr) ? AccountID::fromVoid(memory + aread_ptr)
: hookCtx.result.account; : hookCtx.result.account;
@@ -905,7 +914,17 @@ void hook::commitChangesToLedger(
// this entry isn't just cached, it was actually modified // this entry isn't just cached, it was actually modified
auto slice = Slice(blob.data(), blob.size()); auto slice = Slice(blob.data(), blob.size());
setHookState(hookResult, applyCtx, acc, ns, key, slice); TER result =
setHookState(hookResult, applyCtx, acc, ns, key, slice);
if (result != tesSUCCESS)
{
JLOG(j.warn())
<< "HookError[" << HR_ACC() << "]: SetHookState failed: " << result
<< " Key: " << key
<< " Value: " << slice;
}
// ^ should not fail... checks were done before map insert // ^ should not fail... checks were done before map insert
} }
} }
@@ -3239,7 +3258,7 @@ DEFINE_HOOK_FUNCTION(
{ {
JLOG(j.trace()) JLOG(j.trace())
<< "HookInfo[" << HC_ACC() << "]: Guard violation. " << "HookInfo[" << HC_ACC() << "]: Guard violation. "
<< "Src line: " << id << "Src line: " << id << " "
<< "Iterations: " << hookCtx.guard_map[id]; << "Iterations: " << hookCtx.guard_map[id];
} }
hookCtx.result.exitType = hook_api::ExitType::ROLLBACK; hookCtx.result.exitType = hook_api::ExitType::ROLLBACK;

View File

@@ -67,8 +67,6 @@ LedgerFormats::LedgerFormats()
{sfTakerGetsCurrency, soeOPTIONAL}, // for order book directories {sfTakerGetsCurrency, soeOPTIONAL}, // for order book directories
{sfTakerGetsIssuer, soeOPTIONAL}, // for order book directories {sfTakerGetsIssuer, soeOPTIONAL}, // for order book directories
{sfExchangeRate, soeOPTIONAL}, // for order book directories {sfExchangeRate, soeOPTIONAL}, // for order book directories
{sfHookNamespace, soeOPTIONAL}, // for hook state directories
{sfOwnerNode, soeOPTIONAL}, // for hook state directories
{sfReferenceCount, soeOPTIONAL}, // for hook state directories {sfReferenceCount, soeOPTIONAL}, // for hook state directories
{sfIndexes, soeREQUIRED}, {sfIndexes, soeREQUIRED},
{sfRootIndex, soeREQUIRED}, {sfRootIndex, soeREQUIRED},
@@ -210,11 +208,9 @@ LedgerFormats::LedgerFormats()
add(jss::HookState, add(jss::HookState,
ltHOOK_STATE, ltHOOK_STATE,
{ {
{sfAccount, soeOPTIONAL}, //RH TODO: this can be removed for production
{sfOwnerNode, soeREQUIRED}, {sfOwnerNode, soeREQUIRED},
{sfHookStateKey, soeREQUIRED}, {sfHookStateKey, soeREQUIRED},
{sfHookStateData, soeREQUIRED}, {sfHookStateData, soeREQUIRED},
{sfHookNamespace, soeOPTIONAL} // as can this
}, },
commonFields); commonFields);