Preflight oversized committor transactions before delivery prep#1286
Preflight oversized committor transactions before delivery prep#1286GabrielePicco wants to merge 2 commits into
Conversation
|
Warning Review limit reached
More reviews will be available in 23 minutes and 49 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR adds comprehensive transaction size validation to prevent oversized transaction assembly and implements proper error recovery. The core addition is a post-signing size check in Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@CodeRabbit review |
✅ Action performedReview finished.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 84dc4847a2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ba21952ddd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Thanks for the fix!
Overall seems like the main point was
if serialize_and_encode_base64(&tx).len() > MAX_ENCODED_TRANSACTION_SIZE
{
return Err(TaskStrategistError::FailedToFitError);
}But PR included other changes on delivery preparation. Hope in the future we could make PRs more atomic.
Added comments on both matters.
Additionally as new patching logic appeared it would be good add tests in corresponding file that checks exactly patching correctness and recovery of intents. See
test-integration/test-committor-service/tests/test_intent_executor.rs
| } | ||
| Err(IntentExecutorError::FailedFinalizePreparationError( | ||
| TransactionPreparatorError::FailedToFitError, | ||
| )) if !committed_pubkeys.is_empty() => (None, true), |
There was a problem hiding this comment.
If this error is handled by splitting into two stage flow, I'd assume we want to add it into patched report. Anything that is recoverd would be there, why do we return None here?
| handle_cpi_limit_error( | ||
| &self.authority.pubkey(), | ||
| strategy, | ||
| cleanup_lookup_tables, |
There was a problem hiding this comment.
Additionally the tests are missing for new logic. To validate this case please add test into test-integration/test-committor-service/tests/test_intent_executor.rs
| pub async fn prepare_lookup_tables_for_delivery( | ||
| &self, | ||
| authority: &Keypair, | ||
| strategy: &TransactionStrategy, | ||
| ) -> DeliveryPreparatorResult<Vec<AddressLookupTableAccount>> { | ||
| let _timer = metrics::observe_committor_intent_alt_preparation_time(); | ||
| self.prepare_lookup_tables(authority, &strategy.lookup_tables_keys) | ||
| .await | ||
| .map_err(DeliveryPreparatorError::FailedToCreateALTError) | ||
| } | ||
|
|
||
| /// Prepares buffers and necessary pieces for optimized TX | ||
| pub async fn prepare_for_delivery<P: IntentPersister>( | ||
| &self, | ||
| authority: &Keypair, | ||
| strategy: &mut TransactionStrategy, | ||
| persister: &Option<P>, | ||
| ) -> DeliveryPreparatorResult<Vec<AddressLookupTableAccount>> { | ||
| let lookup_tables = self | ||
| .prepare_lookup_tables_for_delivery(authority, strategy) | ||
| .await?; | ||
| self.prepare_tasks_for_delivery(authority, strategy, persister) | ||
| .await?; | ||
| Ok(lookup_tables) |
There was a problem hiding this comment.
What is the reason for split up here?
Now instead of preparing in parallel we prepare ALTs first and then buffers.
Both of them need to be prepared and one doesn't depend on another, those should be prepared in parallel as before
| // Prepare lookup tables before buffers so real ALT layout is checked | ||
| // before creating buffer accounts. |
There was a problem hiding this comment.
Could you elaborate on what is ALT layout check? Why buffer preparation should be concerned with it?
Buffer preparation is agnostic to ALTs and don't use it, additionally buffer accounts are deterministically defined from commit id and will be the same anyway. If they're in ALTs they will be there in next run.
Buffer preparation also handles already prepared state. For now I don't see reason for this split as it will just make preparation slower
| ); | ||
| execution_report.dispose(cleanup); | ||
| execution_report.add_patched_error(execution_err); | ||
| if let Some(execution_err) = execution_err { |
There was a problem hiding this comment.
If we got here it means we handled some error by splitting into two stage, that error should be added in report.
There was a problem hiding this comment.
In case we don't want some errors in report for whatever reason, we could add fn reportable(&self) -> bool on Error which would be a cleaner solution
| )?; | ||
| ) | ||
| .map_err(|err| match err { | ||
| TaskStrategistError::FailedToFitError => { |
There was a problem hiding this comment.
This highlights my doubts on need of new PreflightFailedToFitError variant.
FailedToFitError is error returned "preflight". TaskStrategist returns it preflight, new assemble_tasks_tx_with_standalone_action_nonce returns it preflight.
What is a reason for another enum variant?
Open to renaming it if it is confusing.
| )) if !committed_pubkeys.is_empty() => (None, true), | ||
| Err(IntentExecutorError::FailedFinalizePreparationError( | ||
| TransactionPreparatorError::PreflightFailedToFitError, | ||
| )) if !committed_pubkeys.is_empty() => (None, false), |
There was a problem hiding this comment.
Why we return false here while true for FailedToFitError?
| let strategy = single_stage_executor.consume_strategy(); | ||
| let (commit_strategy, finalize_strategy, cleanup) = | ||
| handle_cpi_limit_error(&self.authority.pubkey(), strategy); | ||
| handle_cpi_limit_error( |
There was a problem hiding this comment.
Let's rename function for clarity as it not only handle cpi limit error now
|
Closing as this PR seems the proper fix: #1285 rather than the defensive logic applied here |
2 participants
Summary
FailedToFitErrorbefore they are sent.FailedToFitErrorthrough the existing single-stage split fallback and recognize the shorter RPCbase64 encoded too largeerror message.Root cause
Oversized single-stage transactions could still reach delivery preparation, or be classified only after
sendTransaction, because the final assembled transaction size was not checked at the same boundary used for actual submission.Summary by CodeRabbit
Release Notes
Bug Fixes
Refactor