ExtendedHookState #406

tequdev · 2024-12-11T13:44:58Z

Extended the size of HookStateData, and cost one incremental reserve for every 256 bytes
The maximum HookStateData size is tentatively set to 2048 (256*8) / Discussion needed

High Level Overview of Change

Context of Change

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Refactor (non-breaking change that only restructures code)
Tests (you added tests for code that already exists, or your new feature included in this PR)
Documentation update
Chore (no impact to binary, e.g. .gitignore, formatting, dropping support for older tooling)
Release

API Impact

Public API: New feature (new methods and/or new fields)
Public API: Breaking change (in general, breaking changes should only impact the next api_version)
libxrpl change (any change that may affect libxrpl or dependents of libxrpl)
Peer protocol change (must be backward compatible or bump the peer protocol version)

src/ripple/app/hook/impl/applyHook.cpp

src/ripple/app/tx/impl/SetHook.cpp

RichardAH · 2025-06-18T06:22:25Z

I think we should not add the complexity of incremental reserve increase, rather just increase the allowable size for the same existing reserve cost. I think 1 reserve for 2048 bytes is probably ok.

An alternative simpler fee model would be to nominate your account for "large hook data" whereupon all your hook states cost more to store but you have access to the larger size. I really dislike the idea of a variable length field claiming different reserves depending on its current size.

tequdev · 2025-06-19T08:47:07Z

I think we should not add the complexity of incremental reserve increase, rather just increase the allowable size for the same existing reserve cost. I think 1 reserve for 2048 bytes is probably ok.

An alternative simpler fee model would be to nominate your account for "large hook data" whereupon all your hook states cost more to store but you have access to the larger size. I really dislike the idea of a variable length field claiming different reserves depending on its current size.

Can you explain “large hook data” in more detail?

RichardAH · 2025-06-20T01:56:05Z

Can you explain “large hook data” in more detail?

Sure, so you do an account set flag which is like the equivalent of specifying large pages in your OS. All your hook states cost more but they're allowed to be bigger. It makes the computation easier. So let's say in large mode you get 2048 bytes per hook state but in computing reserve requirements for your hook state we take your HookStateCount and multiply it by 8. Once the flag is enabled it can only be disabled if HookStateCount is 0.

tequdev · 2025-06-22T06:22:43Z

Can you explain “large hook data” in more detail?

Sure, so you do an account set flag which is like the equivalent of specifying large pages in your OS. All your hook states cost more but they're allowed to be bigger. It makes the computation easier. So let's say in large mode you get 2048 bytes per hook state but in computing reserve requirements for your hook state we take your HookStateCount and multiply it by 8. Once the flag is enabled it can only be disabled if HookStateCount is 0.

That's interesting.
Instead of using the flag, how about using a numeric field to represent the maximum size the account allows?

If we use the flag, whenever we need to increase the protocol's maximum allowed size, we end up having to add a new flag.

RichardAH · 2025-06-25T05:49:56Z

That's interesting. Instead of using the flag, how about using a numeric field to represent the maximum size the account allows?

If we use the flag, whenever we need to increase the protocol's maximum allowed size, we end up having to add a new flag.

I think that's reasonable. We could say HookStateScale or something 0,1 ...

RichardAH · 2025-06-29T23:52:58Z

src/ripple/app/hook/applyHook.h

@@ -32,6 +32,7 @@ class HookStateMap : public std::map<
                         std::tuple<
                             int64_t,  // remaining available ownercount
                             int64_t,  // total namespace count
+                             int16_t,  // hook state scale


I don't mind if you use int16_t or uint16_t or uint32_t but keep it consistent with the function calls? like maxHookStateDataSize(uint32_t hookStateScale) same for the sf type

RichardAH · 2025-06-29T23:53:57Z

src/ripple/app/tx/impl/SetHook.cpp

@@ -921,7 +927,7 @@ SetHook::destroyNamespace(
        view.erase(sleItem);
    }

-    uint32_t stateCount = oldStateCount - toDelete.size();
+    uint32_t stateCount = oldStateCount - toDeleteStateCount;


was there a reason for changing this? I assume it doesn't change the outcome

RichardAH · 2025-06-29T23:54:26Z

src/ripple/app/tx/impl/SetHook.cpp

@@ -937,7 +943,7 @@ SetHook::destroyNamespace(
        sleAccount->setFieldU32(sfHookStateCount, stateCount);

    if (ctx.rules.enabled(fixNSDelete))
-        adjustOwnerCount(view, sleAccount, -toDelete.size(), ctx.j);
+        adjustOwnerCount(view, sleAccount, -toDeleteStateCount * scale, ctx.j);


we should do a sanity check on this value before passing it to make sure it is reasonable and hasn't overflowed

RichardAH · 2025-06-29T23:58:42Z

src/ripple/app/hook/impl/applyHook.cpp

@@ -1517,19 +1528,22 @@ set_state_cache(
        stateMap.modified_entry_count++;

        stateMap[acc] = {
-            availableForReserves - 1,
+            availableForReserves - hookStateScale,


we need defensive sanity checks on math like this

RichardAH · 2025-06-29T23:59:07Z

src/ripple/app/hook/impl/applyHook.cpp

@@ -1547,7 +1561,7 @@ set_state_cache(
                namespaceCount++;
            }

-            availableForReserves--;
+            availableForReserves -= hookStateScale;


overflow sanity check needed

RichardAH · 2025-06-29T23:59:17Z

src/ripple/app/hook/impl/applyHook.cpp

        if (modified)
        {
            if (!canReserveNew)
                return RESERVE_INSUFFICIENT;
-            availableForReserves--;
+            availableForReserves -= hookStateScale;


RichardAH · 2025-06-30T00:00:30Z

src/ripple/protocol/impl/TER.cpp

@@ -94,6 +94,8 @@ transResults()
        MAKE_ERROR(tecINSUF_RESERVE_SELLER,          "The seller of an object has insufficient reserves, and thus cannot complete the sale."),
        MAKE_ERROR(tecIMMUTABLE,                     "The remark is marked immutable on the object, and therefore cannot be updated."),
        MAKE_ERROR(tecTOO_MANY_REMARKS,              "The number of remarks on the object would exceed the limit of 32."),
+        MAKE_ERROR(tecHAS_HOOK_STATE,                "The account has hook state."),


maybe expand the explanation to "The account has hook state. Delete all existing state before changing the scale."

add ExtendedHookState

798366c

RichardAH force-pushed the dev branch from 4221267 to da8df63 Compare February 4, 2025 06:02

Merge branch 'dev' into ExtendedHookState

53c3229

tequdev force-pushed the ExtendedHookState branch from d7fb314 to 53c3229 Compare February 12, 2025 08:49

tequdev commented Feb 12, 2025

View reviewed changes

src/ripple/app/hook/impl/applyHook.cpp Outdated Show resolved Hide resolved

tequdev added 5 commits February 14, 2025 17:27

Fix not to effect to HookStateCount.

a3902e2

fix sfHookStateCount == 0

ee79065

fix set_state_cache

52e39d2

add tests

2c6b1ba

fix

dbc187f

tequdev commented Feb 14, 2025

View reviewed changes

src/ripple/app/tx/impl/SetHook.cpp Outdated Show resolved Hide resolved

tequdev added 4 commits February 27, 2025 21:00

Merge branch 'dev' into ExtendedHookState

adea596

Merge branch 'dev' into ExtendedHookState

be0bdd3

tests for NSDelete

f588352

clang-format

8dbbdc0

This comment was marked as resolved.

Sign in to view

tequdev added 2 commits March 29, 2025 22:23

Merge branch 'dev' into ExtendedHookState

272129a

add tests for set_state_cache

3e0c8dd

tequdev changed the title ~~ExtendedHookState [DO NOT MERGE]~~ ExtendedHookState Mar 30, 2025

tequdev added Amendment New feature or fix amendment feature Feature Request labels Mar 30, 2025

tequdev and others added 4 commits April 2, 2025 19:32

Merge branch 'dev' into ExtendedHookState

b461e75

Merge branch 'dev' into ExtendedHookState

2805948

Merge branch 'dev' into ExtendedHookState

9ef6dee

Merge branch 'dev' into ExtendedHookState

e987dd7

tequdev added 3 commits June 26, 2025 12:56

fix to use HookStateScale

82154df

fix, update tests

523804d

fix

eb70e6f

RichardAH reviewed Jun 29, 2025

View reviewed changes

RichardAH reviewed Jun 30, 2025

View reviewed changes

tequdev added 6 commits June 30, 2025 11:03

fix hookStateScale type consistency

c1d6b83

fix error message for tecHAS_HOOK_STATE.

4f8ef58

fix toDeleteStateCount

a75daae

add sanity checks

91f683a

Merge branch 'dev' into ExtendedHookState

e3d9fe2

Merge branch 'dev' into ExtendedHookState

3f6f354

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

ExtendedHookState #406

ExtendedHookState #406

tequdev commented Dec 11, 2024 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

RichardAH commented Jun 18, 2025 •

edited

Loading

Uh oh!

tequdev commented Jun 19, 2025

Uh oh!

RichardAH commented Jun 20, 2025

Uh oh!

tequdev commented Jun 22, 2025

Uh oh!

RichardAH commented Jun 25, 2025

Uh oh!

RichardAH Jun 29, 2025

Uh oh!

RichardAH Jun 29, 2025

Uh oh!

RichardAH Jun 29, 2025

Uh oh!

RichardAH Jun 29, 2025

Uh oh!

RichardAH Jun 29, 2025

Uh oh!

RichardAH Jun 29, 2025

Uh oh!

RichardAH Jun 30, 2025

Uh oh!

Uh oh!

ExtendedHookState #406

Are you sure you want to change the base?

ExtendedHookState #406

Conversation

tequdev commented Dec 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

High Level Overview of Change

Context of Change

Type of Change

API Impact

Uh oh!

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

RichardAH commented Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tequdev commented Jun 19, 2025

Uh oh!

RichardAH commented Jun 20, 2025

Uh oh!

tequdev commented Jun 22, 2025

Uh oh!

RichardAH commented Jun 25, 2025

Uh oh!

RichardAH Jun 29, 2025

Choose a reason for hiding this comment

Uh oh!

RichardAH Jun 29, 2025

Choose a reason for hiding this comment

Uh oh!

RichardAH Jun 29, 2025

Choose a reason for hiding this comment

Uh oh!

RichardAH Jun 29, 2025

Choose a reason for hiding this comment

Uh oh!

RichardAH Jun 29, 2025

Choose a reason for hiding this comment

Uh oh!

RichardAH Jun 29, 2025

Choose a reason for hiding this comment

Uh oh!

RichardAH Jun 30, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tequdev commented Dec 11, 2024 •

edited

Loading

RichardAH commented Jun 18, 2025 •

edited

Loading