Implement CAP-77 - ability to freeze ledger keys via network configuration#5162
Implement CAP-77 - ability to freeze ledger keys via network configuration#5162dmkozh wants to merge 5 commits intostellar:masterfrom
Conversation
39f36a6 to
005a309
Compare
There was a problem hiding this comment.
Pull request overview
This PR implements CAP-77 — the ability to freeze ledger keys via network configuration in stellar-core. It adds configuration settings and upgrade mechanisms for freezing/unfreezing keys and bypass transactions, rejects transactions that access frozen keys at validation and apply time, and removes DEX offers that reference frozen entries.
Changes:
- Adds frozen ledger key and freeze bypass TX config settings with delta-based upgrade mechanisms, validation, and caching in
SorobanNetworkConfig. - Extends every operation frame with
doesAccessFrozenKey()checks and adds newdoApplyoverloads that acceptsorobanConfigfor operations needing apply-time frozen key checks (DEX, liquidity pool, claimable balance). - Adds comprehensive tests in
FrozenLedgerKeysTests.cppand updates existing test infrastructure to handle the newtxFROZEN_KEY_ACCESSEDresult code.
Reviewed changes
Copilot reviewed 80 out of 80 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/ledger/NetworkConfig.h/.cpp | Adds frozen key/bypass TX storage, loading, creation, validation, and query methods |
| src/transactions/OperationFrame.h/.cpp | Adds accessesFrozenKey() and virtual doesAccessFrozenKey() to base operation class; adds doApply overload with sorobanConfig |
| src/transactions/TransactionFrame.h/.cpp | Adds accessesFrozenKey(), threads envelopeContentsHash through validation/apply, loads sorobanConfig for all txs at V20+ |
| src/transactions/FeeBumpTransactionFrame.cpp | Adds frozen fee bump source account check and passes contents hash to inner tx |
| src/transactions/TransactionUtils.h/.cpp | Adds offerAccessesFrozenKey() helper |
| src/transactions/OfferExchange.h/.cpp | Adds eSkipFrozen filter result with offer removal and liability release |
| src/transactions/ManageOfferOpFrameBase.h/.cpp | Adds frozen key checks for offer operations at validation and apply time |
| src/transactions/PathPaymentOpFrameBase.h/.cpp | Adds frozen key checks for path payments and passes sorobanConfig to convert |
| src/transactions/PaymentOpFrame.h/.cpp | Adds frozen key checks for payment destination/source trustlines |
| src/transactions/{various}OpFrame.h/.cpp | Adds doesAccessFrozenKey() implementations for all remaining operations |
| src/transactions/LiquidityPool{Deposit,Withdraw}OpFrame.h/.cpp | Adds apply-time frozen trustline checks for LP operations |
| src/transactions/ClaimClaimableBalanceOpFrame.h/.cpp | Adds apply-time frozen key check for claiming balances |
| src/transactions/test/FrozenLedgerKeysTests.cpp | Comprehensive tests for frozen key config, validation, apply-time checks, DEX behavior |
| src/transactions/test/InvokeHostFunctionTests.cpp | Skips delta config settings in upgrade tests |
| src/herder/Upgrades.cpp | Applies frozen key delta upgrades and creates v26 ledger entries |
| src/simulation/TxGenerator.h/.cpp, LoadGenerator.cpp | Supports frozen key deltas in upgrade config generation |
| src/test/TxTests.cpp, TestExceptions.h/.cpp | Adds txFROZEN_KEY_ACCESSED and new operation result codes |
| src/testdata/*.json | Updated test ledger close meta snapshots |
| Builds/VisualStudio/* | Build system updates for new test file and preprocessor defs |
Comments suppressed due to low confidence (5)
src/transactions/PaymentOpFrame.cpp:1
- When the asset is native,
doesAccessFrozenKeyonly checks the destination account and immediately returns, skipping the source account check. But the source account's frozen state also matters for native payments (the source account entry holds the native balance). The source account is checked at the transaction level inTransactionFrame::accessesFrozenKey, so this is not a full bug, but the early return means that for native payments the destination account is checked but nothing else at the operation level — which means if the destination account is not frozen, the function returnsfalsewithout checking whether the source account's entry is frozen for the native balance. Since the tx-level check covers the source account key, this is technically safe but inconsistent with other operations and withofferAccessesFrozenKeywhich explicitly checks the account key for native. Consider also checking the destination account key when the asset is non-native (for the destination's account entry receiving native in a different path), or documenting why the early return is correct.
src/transactions/SetOptionsOpFrame.cpp:1 - There is an extra closing brace at line 336. The original file ends the
stellarnamespace with a}that was already present. The added function body closes with}at line 335, but then line 336 has another}which appears to be a duplicate namespace-closing brace, resulting in a mismatched or extra brace.
src/transactions/InflationOpFrame.cpp:1 - Same issue as in
SetOptionsOpFrame.cpp— there is an extra closing brace at line 155. The function body closes at line 154, and then line 155 adds a second}that would result in a mismatched brace with the namespace.
src/transactions/test/FrozenLedgerKeysTests.cpp:1 - Minor grammar issue: 'adds and remove' should be 'adds and removes'.
src/transactions/test/FrozenLedgerKeysTests.cpp:1 - The variable names
buyerSellingPreandbuyerBuyingPreare confusing becausebuyerSellingPreloads the balance formakerBuying(which is what the buyer is selling/spending), andbuyerBuyingPreloads formakerSelling(which is what the buyer receives). The naming convention is reversed from the perspective of the buyer's assets. Consider renaming tobuyerSpendAssetPre/buyerReceiveAssetPreor adding a comment to clarify the naming convention.
src/herder/Upgrades.cpp
Outdated
| LedgerKey frozenKeysLk(LedgerEntryType::CONFIG_SETTING); | ||
| frozenKeysLk.configSetting().configSettingID = | ||
| CONFIG_SETTING_FROZEN_LEDGER_KEYS; | ||
| auto& frozenKeysVec = ltx.load(frozenKeysLk) |
There was a problem hiding this comment.
I think this technically works because the reference is pointing to the entry in LedgerTxn mEntry, but the LedgerTxnEntry returned by current() goes out of scope after this line. We shouldn't do this. This comment applies below as well. Relevant slack discussion.
| return false; | ||
| } | ||
| } | ||
| catch (...) |
There was a problem hiding this comment.
Should we catch xdr_runtime_error or std::exception and log the exception along with the key that caused the issue? We can keep the catch (...) as a fallback.
Description
Implement CAP-77 - ability to freeze ledger keys via network configuration.
This implements all of the CAP features:
This is a big change, but the bulk of it is tests (as we have a lot of operations to cover) and updates to the interface of every operation. The actual freeze logic is quite straightforward and short, so I'm not sure if it's worth to break this down.
Checklist
clang-formatv8.0.0 (viamake formator the Visual Studio extension)