fix: prevent intent tx signature aliasing on first commit after re-delegation#1408
Conversation
…legation The committor builds intent transactions from fully deterministic inputs. After an undelegate -> re-delegate cycle the delegation metadata nonce restarts at 0, so the next intent's first commit (nonce 1) can produce a transaction byte-identical to a prior instance's landed commit: same nonce, same committed state, same cached blockhash (5s TTL). Identical bytes yield an identical ed25519 signature, the skip-preflight send is deduped by the network, and the status-based confirmer instantly matches the old transaction - the intent is reported successful without anything being sent, leaving the account stuck delegated. Tag intents whose commit uses nonce <= 1 with the existing per-intent uniqueness noop (spl-noop instruction carrying the intent id) on all their stages, and reserve the noop's constant size in the strategist fit checks so strategy selection matches the assembled transaction. Retries of the same intent keep the same id, preserving intentional dedup.
|
Warning Review limit reached
Next review available in: 35 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe committor derives a per-intent uniqueness nonce for first commits and propagates it through single-stage and two-stage execution. Recovery paths preserve or restore the nonce. Strategy sizing, ALT fitting, and transaction preparation include the uniqueness noop, while tests and integration configurations are updated accordingly. 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 |
![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: fa0ac8fd6d
ℹ️ 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".
Reserve the noop's space only for strategies that will actually carry it, instead of unconditionally padding every fit estimate: non-first commits at the wire-size boundary would otherwise be forced into buffer/ALT paths or rejected with FailedToFitError for transactions that fit. The strategist now receives the nonce, accounts for it in both fit checks, and sets it on the strategies it returns.
Intent transactions for first commits now carry the spl-noop uniqueness instruction, so the program must exist on the base layer like it does on mainnet/devnet. Dump of noopb9bkMVfRPU8AsbpTUg8AQkHtKwMYZiFUjNRtMmV from mainnet, registered in every devnet config that loads dlp.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 708b229f7a
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
magicblock-committor-service/src/intent_executor/mod.rs (1)
620-666: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd test coverage for
CommitFinalizetasks inrequires_uniqueness_nonce.The function handles
BaseTaskImpl::CommitFinalize(task) => task.commit_id <= 1, but the test only coversCommitandFinalizevariants. ACommitFinalizetask withcommit_id <= 1is untested.🧪 Suggested test additions
fn test_requires_uniqueness_nonce_on_first_commit_only() { let finalize = BaseTaskImpl::Finalize(FinalizeTask { delegated_account: Pubkey::new_unique(), }); + let commit_finalize = BaseTaskImpl::CommitFinalize( + CommitFinalizeTask { + commit_id: 1, + committed_account: CommittedAccount { + pubkey: Pubkey::new_unique(), + account: Account::default(), + remote_slot: Default::default(), + }, + delivery_details: CommitDelivery::StateInArgs, + } + ); assert!(requires_uniqueness_nonce(&[commit_task(1)])); assert!(requires_uniqueness_nonce(&[commit_task(5), commit_task(1)])); assert!(!requires_uniqueness_nonce(&[commit_task(2)])); assert!(!requires_uniqueness_nonce(&[finalize])); + assert!(requires_uniqueness_nonce(&[commit_finalize])); + assert!(!requires_uniqueness_nonce(&[commit_task(2), finalize])); assert!(!requires_uniqueness_nonce(&[])); }Note: The exact
CommitFinalizeTaskstruct fields may differ; adjust the constructor to match the actual type definition.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@magicblock-committor-service/src/intent_executor/mod.rs` around lines 620 - 666, Add test coverage for the CommitFinalize branch in requires_uniqueness_nonce. Construct CommitFinalizeTask values using the actual required fields, then assert commit_id values 1 (or 0) return true and a value greater than 1 returns false; retain the existing Commit and Finalize assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@magicblock-committor-service/src/intent_executor/mod.rs`:
- Around line 620-666: Add test coverage for the CommitFinalize branch in
requires_uniqueness_nonce. Construct CommitFinalizeTask values using the actual
required fields, then assert commit_id values 1 (or 0) return true and a value
greater than 1 returns false; retain the existing Commit and Finalize
assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bc6f2923-f9aa-427a-ae65-577b0f27b0ee
⛔ Files ignored due to path filters (1)
test-integration/schedulecommit/elfs/noop.sois excluded by!**/*.so
📒 Files selected for processing (13)
magicblock-committor-service/src/intent_executor/mod.rsmagicblock-committor-service/src/tasks/task_strategist.rstest-integration/configs/aml.devnet.tomltest-integration/configs/chainlink-conf.devnet.tomltest-integration/configs/claim-fees-test.tomltest-integration/configs/cloning-conf.devnet.tomltest-integration/configs/committor-conf.devnet.tomltest-integration/configs/config-conf.devnet.tomltest-integration/configs/restore-ledger-conf.devnet.tomltest-integration/configs/schedule-task.devnet.tomltest-integration/configs/schedulecommit-conf.devnet.tomltest-integration/configs/validator-offline.devnet.tomltest-integration/test-committor-service/tests/test_intent_executor.rs
The first-commit uniqueness noop occupies 44 bytes of the 1232-byte packet, moving the diff-in-args boundary for these order-book commits from 680/681 to 636/637 (verified against a local devnet). Document the noop's rationale, scope, and fit accounting in the committor-service guide.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 63c7992c56
ℹ️ 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".
taco-paco
left a comment
taco-paco
left a comment
There was a problem hiding this comment.
Looks good! Some renamings are due + some places have divergence in logic now which has to be corrected.
attempt_lookup_tables( could be simplified and made less error prone by reusing existing logic and avoiding duplication
…ecovery - Rename standalone_action_nonce/noop naming to uniqueness_nonce/noop now that it covers first-commit intents, not only standalone actions. - attempt_lookup_tables reuses assemble_tasks_tx_with_uniqueness_nonce and collect_lookup_table_keys (which now folds the noop into involved-pubkey collection), so fit checks cannot diverge from assembled transactions. - handle_commit_id_error re-tags the strategy with the intent id when a stale-cache retry lands back on commit nonce 1 after a re-delegation: that retry is exactly the first-commit transaction the noop must disambiguate. Executors carry the intent id for this.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 859323163b
ℹ️ 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.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.agents/context/crates/magicblock-committor-service.md:
- Around line 243-246: Reword the finalize-stage sentence in the “Per-intent
uniqueness noop” section to state that finalize stages also receive the same
per-intent uniqueness noop protection, removing the implication that they omit
nonce bytes or alias without the noop.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4cd08678-9f9c-463d-8891-8a9cd54bd8f0
📒 Files selected for processing (12)
.agents/context/crates/magicblock-committor-service.mdmagicblock-committor-service/src/intent_executor/mod.rsmagicblock-committor-service/src/intent_executor/single_stage_executor.rsmagicblock-committor-service/src/intent_executor/two_stage_executor.rsmagicblock-committor-service/src/intent_executor/utils.rsmagicblock-committor-service/src/tasks/task_strategist.rsmagicblock-committor-service/src/tasks/utils.rsmagicblock-committor-service/src/transaction_preparator/mod.rstest-integration/test-committor-service/tests/test_delivery_preparator.rstest-integration/test-committor-service/tests/test_intent_executor.rstest-integration/test-committor-service/tests/test_ix_commit_local.rstest-integration/test-committor-service/tests/test_transaction_preparator.rs
When commit-id recovery re-tags the commit stage as a first commit, the already-built finalize strategy must carry the noop too: its bytes contain nothing per-instance and alias a prior delegation's finalize signature the same way. A boundary-sized stage that no longer fits with the noop fails loudly at the preparator's fit check rather than silently aliasing.
2 participants
Problem
The committor builds intent transactions from fully deterministic inputs. After an undelegate → re-delegate cycle the delegation metadata nonce restarts at 0, so the next intent's first commit (nonce 1) can be byte-identical to a prior instance's landed commit:
Fix
Intents whose commit uses nonce ≤ 1 are tagged with the existing per-intent uniqueness noop (spl-noop instruction carrying the intent id, already used for standalone-action intents), on all their stages — the two-stage finalize tx carries no nonce bytes, so it aliases across cycles exactly like the commit tx. Collisions are only possible at first-commit (the on-chain nonce never regresses otherwise), so other intents are left untouched. Retries of the same intent keep the same id, preserving intentional dedup.
Summary by CodeRabbit