Move expiration enforcement to OpFrame level #3857
Conversation
SirTyson
force-pushed
the
move-expiration
branch
from
03cc0ff
to
d921240
Compare
SirTyson
force-pushed
the
move-expiration
branch
from
d921240
to
54c8382
Compare
SirTyson
force-pushed
the
move-expiration
branch
from
54c8382
to
fadfcbe
Compare
Yeah this is a good catch. This PR switches |
| @@ -1300,8 +1300,7 @@ ConfigUpgradeSetFrameConstPtr | |||
| ConfigUpgradeSetFrame::makeFromKey(AbstractLedgerTxn& ltx, | |||
| ConfigUpgradeSetKey const& key) | |||
| { | |||
| auto ltxe = ltx.loadWithoutRecord(ConfigUpgradeSetFrame::getLedgerKey(key), | |||
| /*loadExpiredEntry=*/false); | |||
| auto ltxe = ltx.loadWithoutRecord(ConfigUpgradeSetFrame::getLedgerKey(key)); | |||
There was a problem hiding this comment.
This is interesting... we don't check if the entry is live here, so we can technically upgrade using an expired entry. Should we keep this as is for more flexibility? Or should we make sure the entry is live? Checking that it's live sounds more consistent. cc @dmkozh.
There was a problem hiding this comment.
I don't have a great understanding of the upgrade system, but allowing anything to use an expired entry seems very sketchy (except for restoreOp of course). I think we should add a isLive check here.
There was a problem hiding this comment.
Yeah, I don't think we should need exceptions to the rules.
| @@ -720,7 +720,7 @@ CommandHandler::getLedgerEntry(std::string const& params, std::string& retStr) | |||
|
|
|||
| LedgerKey k; | |||
| fromOpaqueBase64(k, key); | |||
| auto le = ltx.loadWithoutRecord(k, /*loadExpiredEntry=*/false); | |||
There was a problem hiding this comment.
This command will now return expired entries, when it previously didn't . I'm guessing returning expired entries is what we want, but want to make sure we didn't have a good reason for ignoring them in the first place.
There was a problem hiding this comment.
This should return expired entries. This is used for services like soroban-rpc to track state, and it would be helpful for these systems to know about expired entries. This not returning expired entries before was a bug.
|
r+ 0ad2053 |
Description
Resolves #3854 and #3850.
This change moves state expiration enforcement to the
InvokeHostFunctionOpFramelevel. By doing so, the database and "ledger state" is agnostic to expiration semantics. This simplifies BucketList invariants and makes it easier for downstream systems to ingest state related meta. This resolves the issue that crashed preview 10.This PR also adds more state expiration unit tests.
Checklist
clang-formatv8.0.0 (viamake formator the Visual Studio extension)