merge: dev #474
Merged
merge: dev #474
Conversation
This file contains hidden or 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
Main changes: 1. Use a single environment for all the accountsdb indexes 2. Introduce table abstraction to make the code more clean 3. Disable read ahead in the lmdb env configuration, which was unnecessary due to the mostly random access nature of operations. Due to the use of a single environment instead of previously used 3, this is a breaking change, which requires running a migration script to fix up the index directory. <!-- greptile_comment --> ## Greptile Summary Major refactoring of the AccountsDB index system to improve performance and code organization by consolidating database environments and optimizing LMDB operations. - Consolidated three separate LMDB environments into a single environment for all indices in `magicblock-accounts-db/src/index.rs` - Added new `Table` abstraction in `magicblock-accounts-db/src/index/table.rs` to encapsulate database operations cleanly - Disabled LMDB read-ahead for better performance with random access patterns - Changed program account retrieval to return empty vector instead of NotFound error when no accounts exist - **Breaking Change**: Requires migration script to fix index directory structure due to environment consolidation <!-- /greptile_comment -->
* master: chore: avoid CU budget exceeded for lookup table management (#455)
this fix introduces a change to the subscription handling logic in such a way that the subscription processing stops briefly, if all of the existing connections are busy by inflight subscription request, thus avoiding an attempt to send a subscription when no connection is available to handle it --------- Co-authored-by: Thorsten Lorenz <[email protected]>
Modify account layout in accounts database: 1. reuse slack space which is available in block allocation if possible, this avoids reallocation of accounts if the data field grows insignificantly. 2. track account owner change to avoid index checks in happy path depends on #419 closes #443 closes #441 <!-- greptile_comment --> ## Greptile Summary Major refactoring of AccountsDB storage and indexing system to optimize account data storage and improve performance through block space reuse and owner tracking. - Consolidated multiple LMDB environments into a single environment and introduced new `Table` abstraction in `magicblock-accounts-db/src/index/table.rs` for better database operation encapsulation - Modified account serialization in `magicblock-accounts-db/src/lib.rs` to reuse slack space in block allocations, reducing unnecessary reallocations for small data growth - Added owner change tracking to optimize index checks by skipping them in the happy path when owner hasn't changed - Improved LMDB iteration safety and efficiency in `magicblock-accounts-db/src/index/iterator.rs` by removing manual cursor management - Changed program account iteration behavior to return empty iterators instead of NotFound errors for better API consistency <!-- /greptile_comment -->
Add persistence for the delegation status of accounts in accountsdb. This is achieved via setting delegation flag explicitly after account has been "dumped" to the database. closes #442 <!-- greptile_comment --> ## Greptile Summary Implements account delegation status persistence in AccountsDB with comprehensive database architecture changes. - Refactored LMDB database implementation in `magicblock-accounts-db/src/index.rs` to use new Table abstraction for better encapsulation and performance - Added explicit delegation status tracking in `magicblock-account-dumper/src/account_dumper_bank.rs` for both delegated and undelegated accounts - Removed standalone indices (`standalone.rs`) in favor of consolidated main environment structure - Updated account index handling in `magicblock-accounts-db/src/index/tests.rs` to return empty iterators instead of NotFound errors - Added 'dev' branch to CI workflows for enhanced testing coverage <!-- /greptile_comment -->
This PR is another attempt to allow layered configuration (default -> file -> args -> env) while providing users with information on what parameters they can adjust. It overrides #346 with the following advantages: - Automatically derives CLI and env var names to avoid repetitions and reduce risk of typos - Single definition of the configuration The weakness of this approach is that it utilizes small procedural macros to avoid repetition, which can compromise readability and make debugging more challenging. It introduces a breaking change in the format of the remote because Clap does not accept an enum variant with complex values. <!-- greptile_comment --> ## Greptile Summary Major refactoring of configuration management using Clap for CLI support, introducing layered configuration (default -> file -> args -> env) with automatic CLI and environment variable derivation. - Added `magicblock-config-macro` crate with procedural macros `clap_prefix` and `clap_from_serde` for automatic CLI argument generation - Changed remote configuration format from `remote = "devnet"` to `remote.cluster = "devnet"` across all TOML files (breaking change) - Consolidated configuration types from individual crates into centralized `magicblock-config` crate - Fixed FQDN typo in validator config (renamed from `fdqn` to `fqdn`) - Added merge functionality to all config structs for layered configuration support <!-- /greptile_comment -->
## Summary This PR fixes the performance regression introduced in the previous [this PR](#366), by not waiting for the lookup table transaction to succeed. Instead it just logs the result and ensures right before committing an account that the necessary pubkeys are in place. ## Details ### Table Mania Enhancements - added ensure_pubkeys_table() method to guarantee pubkeys exist in lookup tables without increasing reference counts - implemented get_pubkey_refcount() method for querying refcount of pubkeys across tables - ix test for the new ensure functionality ### Committor Service Optimizations - Modified commit process to ensure all pubkeys have tables before proceeding with transactions - Improved parallel processing by moving table reservation to async spawned tasks ### General Improvements #### Integration Testing Improvements - Added individual make targets for all integration tests (test-schedulecommit, test-cloning, test-committor, etc.) - Renamed list-tasks to list in the Makefile - Enhanced test runner output with better test name reporting for clearer failure diagnostics ### Performance Performance is back to what it was on master, i.e. the first clone no longer takes much longer than subsequent clones. For comparison here are the performance results, for more details see [this PR](#366) #### master ``` +-------------------------------+--------------+--------+------+-------+------+-----------+--------+ | Metric | Observations | Median | Min | Max | Avg | 95th Perc | Stddev | +-------------------------------+--------------+--------+------+-------+------+-----------+--------+ | sendTransaction Response (μs) | 2000 | 2690 | 1116 | 12516 | 3340 | 7159 | 1801 | +-------------------------------+--------------+--------+------+-------+------+-----------+--------+ | Account Update (μs) | 0 | 0 | 0 | 0 | 0 | 0 | 0 | +-------------------------------+--------------+--------+------+-------+------+-----------+--------+ | Signature Confirmation (μs) | 2000 | 2756 | 1125 | 12559 | 3433 | 7494 | 1901 | +-------------------------------+--------------+--------+------+-------+------+-----------+--------+ | Transactions Per Second (TPS) | 51 | 40 | 40 | 40 | 40 | 40 | 0 | +-------------------------------+--------------+--------+------+-------+------+-----------+--------+ ``` #### new committor ``` +-------------------------------+--------------+--------+------+---------+-------+-----------+--------+ | Metric | Observations | Median | Min | Max | Avg | 95th Perc | Stddev | +-------------------------------+--------------+--------+------+---------+-------+-----------+--------+ | sendTransaction Response (μs) | 2000 | 2219 | 1104 | 5525915 | 39628 | 3647 | 329493 | +-------------------------------+--------------+--------+------+---------+-------+-----------+--------+ | Account Update (μs) | 0 | 0 | 0 | 0 | 0 | 0 | 0 | +-------------------------------+--------------+--------+------+---------+-------+-----------+--------+ | Signature Confirmation (μs) | 2000 | 2247 | 1116 | 5525967 | 39668 | 3730 | 329498 | +-------------------------------+--------------+--------+------+---------+-------+-----------+--------+ | Transactions Per Second (TPS) | 52 | 40 | 13 | 40 | 39 | 17 | 4 | +-------------------------------+--------------+--------+------+---------+-------+-----------+--------+ ``` We can see that there is a huge deviation in this branch due to the first clone taking a lot longer as it reserves the pubkeys needed to commit the cloned account in a lookup table. #### new committor on this branch ``` +-------------------------------+--------------+--------+------+-------+------+-----------+--------+ | Metric | Observations | Median | Min | Max | Avg | 95th Perc | Stddev | +-------------------------------+--------------+--------+------+-------+------+-----------+--------+ | sendTransaction Response (μs) | 2000 | 1827 | 1250 | 19942 | 1929 | 2537 | 705 | +-------------------------------+--------------+--------+------+-------+------+-----------+--------+ | Account Update (μs) | 0 | 0 | 0 | 0 | 0 | 0 | 0 | +-------------------------------+--------------+--------+------+-------+------+-----------+--------+ | Signature Confirmation (μs) | 2000 | 1867 | 1272 | 21436 | 2003 | 2768 | 848 | +-------------------------------+--------------+--------+------+-------+------+-----------+--------+ | Transactions Per Second (TPS) | 51 | 40 | 40 | 40 | 40 | 40 | 0 | +-------------------------------+--------------+--------+------+-------+------+-----------+--------+ ``` We can see that the deviation is back to a _sane_ amount. In this case the max is still higher than on master, but that could be an outlier. I ran the perf test another time and confirmed this: ``` +-------------------------------+--------------+--------+------+-------+------+-----------+--------+ | Metric | Observations | Median | Min | Max | Avg | 95th Perc | Stddev | +-------------------------------+--------------+--------+------+-------+------+-----------+--------+ | sendTransaction Response (μs) | 2000 | 1889 | 1221 | 11291 | 1977 | 2656 | 471 | +-------------------------------+--------------+--------+------+-------+------+-----------+--------+ | Account Update (μs) | 0 | 0 | 0 | 0 | 0 | 0 | 0 | +-------------------------------+--------------+--------+------+-------+------+-----------+--------+ | Signature Confirmation (μs) | 2000 | 1922 | 1354 | 40313 | 2064 | 2847 | 1047 | +-------------------------------+--------------+--------+------+-------+------+-----------+--------+ | Transactions Per Second (TPS) | 51 | 40 | 40 | 40 | 40 | 40 | 0 | +-------------------------------+--------------+--------+------+-------+------+-----------+--------+ ```
## Summary This PR adds a configurable option to control if lookup tables are prepared during account cloning operations. The new `AccountsCloneConfig` allows users to set lookup table preparation to "always" or "never", addressing performance + cost concerns when eager preparation is not needed. ```toml [accounts.clone] prepare_lookup_tables = "always" ``` CLOSES: #448 ## Details ### Configuration System Enhancements - Added `AccountsCloneConfig` with `PrepareLookupTables` enum supporting "always" and "never" modes - Integrated clone configuration into the main `AccountsConfig` structure with proper merging and serialization support - Updated account cloner constructor to accept the new configuration parameter ### Validator Behavior Changes - Modified `MagicValidator` to only reserve common pubkeys when `prepare_lookup_tables` is set to "always" - Working around our current issues by defaulting to "never" mode - Updated all test setups to explicitly pass the new configuration parameter ### Integration Tests - Created comprehensive `test-config` integration test suite to verify lookup table behavior - Added tests for both "always" and "never" modes that count actual lookup table transactions on chain - Moved common validator utilities from `test-ledger-restore` to `test-tools` for reusability - Enhanced `IntegrationTestContext` with convenience methods for instruction handling ### Code Quality Improvements - Silenced clippy warnings about zombie processes in appropriate test contexts - Enhanced error handling with better context messages
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.
No description provided.