refactor(trie): remove proof task manager #18934
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
- Added configuration for maximum and minimum storage proof workers. - Implemented a worker pool for processing storage proof tasks, improving efficiency by reusing transactions. - Updated `ProofTaskManager` to handle storage proof tasks via a dedicated channel. - Enhanced metrics to track storage proof requests and fallback scenarios. - Adjusted existing tests to accommodate the new storage worker functionality.
- Enhanced documentation for `StorageProofJob` to clarify its current unused status and potential for future type-safe design. - Updated comments in `ProofTaskManager` regarding the handling of on-demand tasks and the possibility of refactoring to a more type-safe enum. - Improved logging for worker pool disconnection scenarios, emphasizing fallback to on-demand execution.
…Metrics and ProofTaskTrieMetrics
…clarity - Updated comments in `ProofTaskManager` to enhance clarity regarding on-demand transaction handling and queue management. - Renamed `pending_on_demand` to `on_demand_queue` for better understanding of its purpose. - Adjusted the `new` function documentation to reflect the correct allocation of concurrency budget between storage workers and on-demand transactions. - Improved the `queue_proof_task` method to use the new queue name.
…ement - Removed the unused `OnDemandTask` enum and updated comments in `ProofTaskManager` to clarify the distinction between storage worker pool and on-demand execution. - Enhanced documentation to better describe the public interface and task submission process. - Improved clarity regarding transaction handling and execution paths for proof requests.
- Eliminated the `storage_proof_workers` field and related constants from `TreeConfig`. - Updated the default implementation and related methods to reflect the removal, streamlining the configuration structure.
- Improved comments in `ProofTaskManager` and related functions for better clarity on task management and processing. - Updated queue capacity calculation to use 4x buffering, reducing fallback to slower on-demand execution during burst loads. - Removed redundant variable assignments to streamline the code.
Co-authored-by: Brian Picciano <[email protected]>
Co-authored-by: Brian Picciano <[email protected]>
…ursor factories - Introduced pre-created cursor factories in `storage_worker_loop` to reduce overhead during proof computation. - Updated `compute_storage_proof` to accept cursor factories as parameters, enhancing efficiency and clarity. - Improved logging to provide better insights into proof task calculations.
- not change the logic for pending_tasks and proof_tasks_txs (on-demand proofs) and just continue using it for the BlindedAccountNode requests, but start using dedicated storage workers for StorageProof and BlindedStorageNode requests
Co-authored-by: Copilot <[email protected]>
- Added a function to determine the default number of storage worker threads based on available parallelism. - Updated TreeConfig to include a storage_worker_count field, initialized with the default value. - Modified payload processor to utilize the new storage_worker_count instead of a hardcoded value.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
mediocregopher
approved these changes
Oct 13, 2025
mattsse
approved these changes
Oct 14, 2025
| let _ = self.proof_task_handle.queue_task(ProofTaskKind::StorageProof(input, sender)); | ||
| receiver | ||
| self.proof_task_handle | ||
| .queue_storage_proof(input) |
There was a problem hiding this comment.
can we rename these after we merge, because I find queue very confusing here because this only sends
There was a problem hiding this comment.
thats true -> should rename it as send_task
Comment on lines
+882
to
+883
| for worker_id in 0..storage_worker_count { | ||
| let provider_ro = view.provider_ro()?; |
There was a problem hiding this comment.
I believe this is still something we pay for upfront, meaning this isn't done in the background
this does mean it currently takes more time to set this up if we bump the worker count?
I think ideally we return the channels right away and do this setup in the background so that we don't block here:
There was a problem hiding this comment.
this makes sense, good idea for us to do in bg
Comment on lines
+909
to
+910
| for worker_id in 0..account_worker_count { | ||
| let provider_ro = view.provider_ro()?; |
yongkangc
commented
Oct 14, 2025
yongkangc
commented
Oct 14, 2025
Rjected
approved these changes
Oct 14, 2025
shekhirin
approved these changes
Oct 15, 2025
| /// eliminating the need for a routing thread. All handles share reference-counted | ||
| /// channels, and workers shut down gracefully when all handles are dropped. | ||
| #[derive(Debug, Clone)] | ||
| pub struct ProofTaskManagerHandle { |
There was a problem hiding this comment.
I find this a tiny bit confusing that we have a ProofTaskManagerHandle, but no ProofTaskManager, as there's usually a pattern of having a fn handle(&self) -> Handle method. Not necessary to address in this PR, can be a followup.
| self.account_work_tx | ||
| .send(AccountWorkerJob::AccountMultiproof { input, result_sender: tx }) | ||
| .map_err(|_| { | ||
| ProviderError::other(std::io::Error::other("account workers unavailable")) |
There was a problem hiding this comment.
would also like to address this in a followup cleanup, this feels like misuse of std::io::Error
yongkangc
commented
Oct 15, 2025
| let (storage_work_tx, storage_work_rx) = unbounded::<StorageWorkerJob>(); | ||
| let (account_work_tx, account_work_rx) = unbounded::<AccountWorkerJob>(); | ||
|
|
||
| tracing::info!( |
There was a problem hiding this comment.
Suggested change
| tracing::info!( | |
| tracing::debug!( |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
Context:
closes: #18801
impact:
run()loop threadreference PRs: