-
Notifications
You must be signed in to change notification settings - Fork 970
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Count read bytes for expired temp entries #4009
Conversation
{ | ||
entrySize += static_cast<uint32_t>(ttlBuf.data->size()); | ||
} | ||
entrySize += static_cast<uint32_t>(ttlBuf.data->size()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't noticed this code before - why do we increase the entry size here? TTL entries shouldn't be observable in non-refundable resources. Using it for one particular limit is rather misleading.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm that's a good point. TTL write fees come out of refundable fee, but it looks like all Soroban ops increase the read bytes when reading TTL entries. Should we be charging from refundable fee instead for the reads? That sounds more complex with the way things are set up now. Maybe we just don't charge for TTL reads? That isn't ideal though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, putting this into refundable fee won't be correct either because we perform reads no matter what. TBH I'm not sure if the confusion potential caused by this is worth the minor benefit this provides. TTL entries are constant size, so they can be thought of as a constant factor added to the entry read cost. Of course, not every entry has TTL entry, but it's seems ok to just charge the 'upper bound' cost, given that majority of entries do have TTL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I'm fine with the current approach. Ideally, we'd charge TTL writes via writeBytes. We can't do this though because we don't know if we will perform a TTL write until runtime. For reads this isn't the case, since we know we will perform all reads. I don't like overcharging for entries that don't have a TTL entry either. To the end user I think the interface makes sense. When you update the TTL, you get charged from the refundableFee. When you read a Soroban entry, there's a little more read bytes overhead than a Stellar classic entry. While there is an inconsistency in the implementation details, it's pretty deep in the stack so I doubt it would be a huge issue for developers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, we'd charge TTL writes via writeBytes.
No, this is not what we have discussed so far. TTL entries should be transparent to the users and we don't want to have them transparent only in some contexts (like footprint), but not the others (like RW bytes). We also do charge the writes as a part of refundable fee (part of the rent fee to be more specific).
While there is an inconsistency in the implementation details, it's pretty deep in the stack so I doubt it would be a huge issue for developers.
This is an issue for preflight/simulation. It brings this implementation detail up to the layers that shouldn't really worry about that.
I don't like overcharging for entries that don't have a TTL entry either.
It's not like we are really overcharging anything, it's just that reading any entry costs the same fee. If we have issues due to number of ledger entries read, then we can increase the entry read fees and/or reduce the tx/ledger limits on the number of entry reads. Adding a few stroops to the fee and a few bytes to the read bytes limit will not in any way prevent any issues. It will however create annoying issues for any downstream customer that tries to fill the soroban resources.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I agree that adding TTL bytes to the read counter is messy. We don't expose these entries anywhere, so expecting them to be accounted for as bytes read is confusing.
I updated the PR to not count TTL readBytes in InvokeHostFunctionOp, and only load soroban entries that aren't expired. If we're all in agreement with this approach, I'll update the other ops and increase the entry read cost (let me know if I misunderstood this discussion).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks - let's also update other ops in this PR then to have consistent semantics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated for all ops
@@ -420,25 +420,24 @@ InvokeHostFunctionOpFrame::doApply(Application& app, AbstractLedgerTxn& ltx, | |||
TTLEntry = ttlLtxe.current().data.ttl(); | |||
} | |||
|
|||
if (shouldAddEntry) | |||
auto leBuf = toCxxBuf(le); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should expose (and observe) the expired temp entries. I think we need to change the flow here: read the ttl entry first, then only proceed to reading the full entry in case if it's live.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm should we treat the absence of a ttlEntry == absence of Soroban entry without doing the explicit check? If this doesn't hold due to (a very bad) bug, this assumption would make things much worse.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's fine. If we have missing TTL entries, we're in a very bad state regardless. If we add an invariant check here, it's both computationally expensive because we have to do more reads and expensive to devs who have to pay fees for loading unusable, expired entries. If we just load the TTL entry and stop if the entry is expired, we only have to charge devs a small fee for the TTL read instead of charging for the whole entry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this precaution is worth the potential performance issues with this code path (or bad usability due to counting expired bytes), given that ledger state will be corrupted either way. We could also maybe add additional invariant checks at the write time, so the end result would be the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we only have to charge devs a small fee for the TTL read instead of charging for the whole entry.
FWIW we charge a flat fee for every entry in the footprint, whether it exists or not. TTL read or not is rather an implementation detail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated for all ops. This PR now checks the TTLEntry before deciding if it should load the actual entry for all Soroban ops.
@@ -420,25 +420,24 @@ InvokeHostFunctionOpFrame::doApply(Application& app, AbstractLedgerTxn& ltx, | |||
TTLEntry = ttlLtxe.current().data.ttl(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: TTLEntry
-> ttlEntry
for consistency with the naming style
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
…ng expired soroban entries
…check for TTLEntry before loading actual entry
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, one minor assert issue.
} | ||
else | ||
{ | ||
releaseAssertOrThrow(!sorobanEntryLive); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think releaseAssert(!ttlEntry)
would be better here. This doesn't catch the invalid case where there is an expired Temporary TTL Entry with no associated LedgerEntry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this check seems a bit confusing, I would also add a if (isSorobanEntry)
condition on the assertion to not mix soroban and non-soroban cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
r+ ba91936 |
Description
Expired temp entries are still read from the ledger, so they should be included in the cost.
Checklist
clang-format
v8.0.0 (viamake format
or the Visual Studio extension)