ExtendedHookState

[tequdev]

• 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)

[RichardAH]

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]

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]

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]

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]

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]

sanity check newOwnerCount > oldOwnerCount

[RichardAH]

Change comment to something like: Delete all hook state before reducing scale.

[tequdev]

Since the number of TER codes is limited, we used a generic error message. However, we will change it as you pointed out, as it can be modified later when this code is used for other purposes.

[sublimator]

extract a constant for this ?

[sublimator]

we could add an extra defensive check here with a constant, I guess

[tequdev]

like this?

inline uint32_t
maxHookStateDataSize(uint16_t hookStateScale)
{
    if (hookStateScale == 0)
    {
        // should not happen, but just in case
        return 256U;
    }
    if (hookStateScale > maxHookStateScale())
    {
        // should not happen, but just in case
        return 256 * maxHookStateScale();
    }
    return 256U * hookStateScale;
}

[sublimator]

I think assert/throw "fail fast" because it's a violated invariant indicating something went awry ?

[tequdev]

I think it's too much to add that kind of processing here.

[sublimator]

Well, we can do "return 256U * hookStateScale;" if it's just a simple calculation, but if you want to handle strange cases (0 || > max_scale) this way, it means you're explicitly washing over bad state. If it should not happen, assert that it doesn't happen. This is some weird middle ground, no?

[sublimator]

1. Decrease WITHOUT state (the escape hatch)

applyCount(5, 0, 50); // ← stateCount=0 (all state deleted)
jt[sfHookStateScale.fieldName] = 2;
env(jt); // ← Should succeed! This is how you escape high scale
BEAST_EXPECT(env.le(alice)->getFieldU16(sfHookStateScale) == 2);

This is important because it tests the only way to reduce scale - by deleting all state first. The spec calls this the "Scale Commitment Trap" escape route.

2. Insufficient reserves on increase

// Alice has exactly enough for current reserves, but not for 16x increase
applyCount(1, 100, 200); // 100 entries at scale=1
// Manually set balance to just cover current reserves
jt[sfHookStateScale.fieldName] = 16; // Needs 1600 reserves (100×16)
env(jt, ter(tecINSUFFICIENT_RESERVE)); // Should fail - can't afford it