-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Added documention for core invariants and fixed example config #5046
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
base: master
Are you sure you want to change the base?
Conversation
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.
Pull request overview
This PR adds comprehensive documentation for core invariants in stellar-core and updates the example configuration file to reflect currently available invariants.
- Adds a new
docs/invariants.mdfile documenting all invariant types, their purposes, implementation details, and configuration options - Updates
docs/stellar-core_example.cfgto remove outdatedCacheIsConsistentWithDatabaseinvariant and reorder existing invariants with improved descriptions - Documents three invariants with updated descriptions:
ConservationOfLumens,ConstantProductInvariant, and adds newSponsorshipCountIsValid
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| docs/stellar-core_example.cfg | Removes obsolete CacheIsConsistentWithDatabase entry, reorders invariants alphabetically, and adds documentation for ConstantProductInvariant and SponsorshipCountIsValid with improved descriptions |
| docs/invariants.md | Adds comprehensive documentation covering all 10 invariants with architectural overview, check methods, configuration instructions, and best practices for adding new invariants |
SirTyson
left a comment
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.
Overall LGTM. I would add a big warning at the top of this though that invariants are very expensive, may reduce performance, and will crash your node if a fault is detected. Basically just a section to dissuade tier 1 from actually turning these on for production validators.
docs/invariants.md
Outdated
| |--------|-------------| | ||
| | `checkOnOperationApply` | After each operation is applied | | ||
| | `checkOnBucketApply` | After bucket apply during catchup | | ||
| | `checkAfterAssumeState` | After assuming state from history | |
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.
assume state runs on every restarts, not just when catching up from history
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.
docs/invariants.md
Outdated
| - Only applies to Soroban entry types (CONTRACT_CODE, CONTRACT_DATA) | ||
| - Verifies that no entry exists in both the live state and the archived (Hot Archive) state simultaneously | ||
| - Checks eviction invariants: entries evicted from live state should not already exist in the hot archive | ||
| - Checks restoration invariants: entries restored from hot archive should match expected states |
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: restoration invariant checks that restored entries were not already in live state.
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.
docs/invariants.md
Outdated
|
|
||
| **File:** `BucketListIsConsistentWithDatabase.cpp` | ||
|
|
||
| **Purpose:** Validates that the BucketList and Database are in a consistent state after bucket operations. |
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 should add a note that this only applies to OFFER types, as all non-offer LIVEENTRY are not reflected in the database.
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.
docs/invariants.md
Outdated
| **Details:** | ||
| - Iterates over bucket entries and compares them to the database | ||
| - Fails if: | ||
| - A LIVEENTRY in the bucket doesn't match the database (not present or different) |
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 would remove the references to LIVEENTRY and DEADENTRY. It doesn't make sense to talk about individual Bucket types here, since an entry can be shadowed, so the existence or nonexistence of a LIVEENTRY/DEADENTRY isn't particularly relevant. I would just say "check that there is a one-to-one mapping of OFFER entry types between SQL DB and BucketList state".
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.
docs/invariants.md
Outdated
| - For AMM (Automated Market Maker) pools using constant product formula | ||
| - Validates: `currentReserveA * currentReserveB >= previousReserveA * previousReserveB` | ||
| - Excludes `LIQUIDITY_POOL_WITHDRAW`, `SET_TRUST_LINE_FLAGS`, and `ALLOW_TRUST` operations (which intentionally may decrease the product) | ||
| - Prevents manipulation that would drain liquidity unfairly |
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.
Instead of "prevent" I would say "detects"
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.
docs/invariants.md
Outdated
|
|
||
| ## Configuration | ||
|
|
||
| Invariants can be enabled through stellar-core configuration. The `InvariantManager` supports: |
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.
This section talks about what the interface of the InvariantManager class does and doesn't really have anything to do with configuration. I would just remove it and have the section below become "configuration"
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.
Edited.
docs/invariants.md
Outdated
|
|
||
| ### State Snapshot Invariants | ||
|
|
||
| Some expensive invariants (like `ArchivedStateConsistency.checkSnapshot`) run periodically on a background thread against ledger state snapshots, as they require scanning the entire BucketList and can't run in a blocking fashion during normal operation. |
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 should note that enabling snapshot invariants is very expensive and requires lot's of memory. For this reason, it is not allowed on validator nodes.
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.
Added a note.
docs/invariants.md
Outdated
|
|
||
| ## Best Practices for Adding New Invariants | ||
|
|
||
| 1. **Choose the right check method**: Select based on when the validation should occur |
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.
Item 1 and 3 aren't particularly helpful and are to niche.
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.
Cleaned up.
046ecfb to
1fa10b0
Compare
what
Added documention for core invariants and fixed example config
why
Improve documentation