Invariant: prevent a deleted account from leaving (most) artifacts on the ledger. #4663
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ximinez
force-pushed
the
invariant-acctdelete
branch
2 times, most recently
from
62db8b9 to
da3692c
Compare
ximinez
force-pushed
the
invariant-acctdelete
branch
8 times, most recently
from
6699dc2 to
cdda729
Compare
ximinez
force-pushed
the
invariant-acctdelete
branch
3 times, most recently
from
5249527 to
9136e22
Compare
ximinez
force-pushed
the
invariant-acctdelete
branch
4 times, most recently
from
53e0482 to
ca69b62
Compare
ximinez
force-pushed
the
invariant-acctdelete
branch
2 times, most recently
from
3f43634 to
06560e6
Compare
ximinez
force-pushed
the
invariant-acctdelete
branch
4 times, most recently
from
a679e84 to
d9d403e
Compare
* Checks that a deleted account doesn't leave any directly accessible artifacts behind. * Always tests, but only changes the transaction result if featureInvariantsV1_1 is enabled. * Unit tests. * Resolves XRPLF#4638
ximinez
force-pushed
the
invariant-acctdelete
branch
from
d9d403e to
bb4ce71
Compare
…lete * upstream/develop: fix: resolve database deadlock: (4989) test: verify the rounding behavior of equal-asset AMM deposits (4982) test: Add tests to raise coverage of AMM (4971) chore: Improve codecov coverage reporting (4977) test: Unit test for AMM offer overflow (4986) fix amendment to add `PreviousTxnID`/`PreviousTxnLgrSequence` (4751)
ximinez
force-pushed
the
invariant-acctdelete
branch
from
a74c89f to
853789e
Compare
…lete * upstream/develop: fix: Remove redundant STAmount conversion in test (4996)
…lete * upstream/develop: Set version to 2.2.0-b3
…lete * upstream/develop: Ignore more commits Address compiler warnings Add markers around source lists Fix source lists Rewrite includes Format formerly .hpp files Rename .hpp to .h Simplify protobuf generation Consolidate external libraries Remove packaging scripts Remove unused files
…lete * upstream/develop: Set version to 2.2.0-rc1 fix amendment: AMM swap should honor invariants: (5002) Add global access to the current ledger rules: chore: fix typos (4958) test: Add RPC error checking support to unit tests (4987)
ximinez
force-pushed
the
invariant-acctdelete
branch
from
141a785 to
1cdf099
Compare
…lete * upstream/develop: Remove flow assert: (5009) Update list of maintainers: (4984)
…lete * upstream/develop: Add external directory to Conan recipe's exports (5006) Add missing includes (5011)
|
Perf signed off by Antonio Sun on May 7, 2024 |
intelliot
added
Perf SignedOff
* commit 'c706926': (23 commits) Change order of checks in amm_info: (4924) Add the fixEnforceNFTokenTrustline amendment: (4946) Replaces the usage of boost::string_view with std::string_view (4509) docs: explain how to find a clang-format patch generated by CI (4521) XLS-52d: NFTokenMintOffer (4845) chore: remove repeat words (5041) Expose all amendments known by libxrpl (5026) fixReducedOffersV2: prevent offers from blocking order books: (5032) Additional unit tests for testing deletion of trust lines (4886) Fix conan typo: (5044) Add new command line option to make replaying transactions easier: (5027) Fix compatibility with Conan 2.x: (5001) Set version to 2.2.0 Set version to 2.2.0-rc3 Add xrpl.libpp as an exported lib in conan (5022) Fix Oracle's token pair deterministic order: (5021) Set version to 2.2.0-rc2 Fix last Liquidity Provider withdrawal: Fix offer crossing via single path AMM with transfer fee: Fix adjustAmountsByLPTokens(): ...
* commit 'f6879da': Add bin/physical.sh (4997) Prepare to rearrange sources: (4997)
…lete * upstream/develop: fixInnerObjTemplate2 amendment (5047) Set version to 2.3.0-b1 Ignore restructuring commits (4997) Recompute loops (4997) Rewrite includes (4997) Rearrange sources (4997) Move CMake directory (4997)
ximinez
force-pushed
the
invariant-acctdelete
branch
from
b23eea4 to
36389a0
Compare
ximinez
added
the
Passed
|
Perf testing has passed while I was out. I've merged in |
vlntb
pushed a commit
to vlntb/rippled
that referenced
this pull request
Aug 23, 2024
… the ledger. (XRPLF#4663) * Add feature / amendment "InvariantsV1_1" * Adds invariant AccountRootsDeletedClean: * Checks that a deleted account doesn't leave any directly accessible artifacts behind. * Always tests, but only changes the transaction result if featureInvariantsV1_1 is enabled. * Unit tests. * Resolves XRPLF#4638 * [FOLD] Review feedback from @gregtatcam: * Fix unused variable warning * Improve Invariant test const correctness * [FOLD] Review feedback from @mvadari: * Centralize the account keylet function list, and some optimization * [FOLD] Some structured binding doesn't work in clang * [FOLD] Review feedback 2 from @mvadari: * Clean up and clarify some comments. * [FOLD] Change InvariantsV1_1 to unsupported * Will allow multiple PRs to be merged over time using the same amendment. * fixup! [FOLD] Change InvariantsV1_1 to unsupported * [FOLD] Update and clarify some comments. No code changes. * Move CMake directory * Rearrange sources * Rewrite includes * Recompute loops * Fix merge issue and formatting --------- Co-authored-by: Pretty Printer <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
High Level Overview of Change
Adds invariant
AccountRootsDeletedClean, which checks that a deleted account doesn't leave any directly accessible artifacts behind. Directly accessible artifacts are those for which aKeyletcan be generated solely from the account ID, or are linked directly in theAccountRoot. These are currently:AccountRoot- redundant, but doesn't hurt to checkDirectoryNode- root node of the account's directorySignerListNFTokenPage- min and max, also using thesuccmethod used in NFT page handling.AMM- Uses theAMMIDin theAccountRootIf any of those items are found, the invariant fails.
Note that this search is not exhaustive. There are plenty of object types that can't be accessed directly, such as
RippleStateandEscrow. However, these are all indexed in the directory, so the current assumption is that if the directory is gone, those objects are gone, too. (It is possible to delete the directory without deleting the items, but hopefully something like that would be so obviously wrong, that it would get caught in review. Or it could be checked in a different invariant.)Amendment:
InvariantsV1_1Also introduces a new amendment named
InvariantsV1_1. My intention is to write several separate invariants all gated by this amendment, and have them all included in a single release, sort of like what was done forfixNonFungibleTokensV1_2.The cool thing about this amendment is the checking and logging is done regardless of whether the amendment is enabled. Only if the amendment is enabled will the transaction result be
tecINVARIANT_FAILED.This allows most of the benefits of this change to become immediately available, but avoid a fork in the unlikely event that it fails.
Fortunately, the original
EnforceInvariantsamendment has been retired, so there is no need to worry about ordering.Context of Change
Explained in #4638. tl;dr: The first version of AMM support did not delete the AMM account object correctly. A fix was merged in #4682. If an invariant like this had existed, the flaw would have been detected during development. To confirm that, run unit tests against https://github.com/ximinez/rippled/tree/invariant-acctdelete-base, which contains the invariant on top of the original AMM code, without the amendment,
AMMID, or extra unit tests. Many of the AMM tests fail because of this invariant. (Remember, these issues have since been fixed, as indicated by the tests passing on this branch.)Type of Change
Before / After
Before: If an account is not deleted completely, there will be no way to know it.
After, before the amendment is enabled: If an account is not deleted completely, a
FTLmessage will be logged.After the amendment: If an account is not deleted completely, a
FTLmessage will be logged, and the transaction changes will be reverted, and recorded with atecINVARIANT_FAILEDresult.Test Plan
Existing test suites should cover this scenario adequately. If any testing gets an anomalous result, that may indicate an existing, previously undetected problem, or a problem with this change.
Future Tasks
As mentioned above, I'd like to roll out a handful of other invariants in the same release as this one.