-
Notifications
You must be signed in to change notification settings - Fork 14
feat: committor service #366
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
Merged
Merged
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
- at this point schedule commit tests pass with maximum concurrency
629d0a2
to
6449d59
Compare
GabrielePicco
approved these changes
Jul 9, 2025
thlorenz
added a commit
that referenced
this pull request
Jul 11, 2025
* master: feat: committor service (#366)
This was referenced Jul 11, 2025
thlorenz
added a commit
that referenced
this pull request
Jul 17, 2025
## 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 | +-------------------------------+--------------+--------+------+-------+------+-----------+--------+ ```
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.
Summary
Adding a full fledged committor service to support the following kind of commits:
easy to manually retry them or do this automatically (next committor version will support this)
Details
The following crates were developed
here and integrated
into the validator:
magicblock-rpc-client
signature status checking (including sane retries)
magicblock-table-mania
used in a transaction the needed table addresses are provided
all its pubkeys are released
magicblock-committor-program
(out of order is ok)
magicblock-committor-service
available
Sample Flow
service
lookup tables which we might need as fast as possible
is provided to the user via logs in a transaction (as we did before, just now we have to wait
for the commit to complete)
For those commits the committor service ensures that accounts with the same commit (bundle) id
are always processed atomically.
The committor service also picks the best possible strategy to commit each changeset,
preferring speed and then cost.
It also inserts configurable (via code) compute budget instructions to each transaction.
The outcome of each commit is persisted to a database which allows manual (and in the next
version) automated retries of failed commits.
Succeessful commits are also persisted to the database and can be used for diagnostics. In the
future they should be removed since no retry is necessary.
On chain signatures for process-commit/finaliz/undelegate transactions are also persisted to
the database in a separate table.
This table is queried by the validator to log those signatures as part of a transaction that
the user waits for.
Performance
I made sure that there was no performance degradation and
redline config using 1 account per transaction
master
new committor
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.
I improved this delay a bit by initiating the reserve transaction before running the
clone transaction, thus performing both steps in parallel. However the reserve transaction takes a
magnitude longer than the cloning into our validator and thus the overall time didn't improve
much.
Redline Configuration same as above with below change
master
Lots of committor transactions fail here since the created transactions are too large since we
commit up to 8 accounts in each transaction.
new committor
NOTE: that here lots of commit transactions fail since they are created with the same ephemeral
blockhash and thus are identical to transactions that ran already.
Others fail due to commits being requested without the finalize of the previous commit having
completed.
This is an issue that we won't see in real life since identical account bundles won't be
committeed in such quick succession in practice.