Hotfix: duplicate accounts in intents #744
Conversation
Manual Deploy AvailableYou can trigger a manual deploy of this PR branch to testnet: Alternative: Comment
Comment updated automatically when the PR is synchronized. |
📝 WalkthroughWalkthroughThe change adds in-place deduplication of Suggested reviewers
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (4)📓 Common learnings📚 Learning: 2025-11-07T13:20:13.793ZApplied to files:
📚 Learning: 2025-12-03T09:36:01.527ZApplied to files:
📚 Learning: 2025-12-03T09:33:48.707ZApplied to files:
🧬 Code graph analysis (1)magicblock-accounts/src/scheduled_commits_processor.rs (1)
🔇 Additional comments (1)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
test-integration/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
magicblock-accounts/src/scheduled_commits_processor.rs(1 hunks)magicblock-committor-service/Cargo.toml(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-11-07T13:20:13.793Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/coordinator.rs:227-238
Timestamp: 2025-11-07T13:20:13.793Z
Learning: In magicblock-processor's ExecutionCoordinator (scheduler/coordinator.rs), the `account_contention` HashMap intentionally does not call `shrink_to_fit()`. Maintaining slack capacity is beneficial for performance by avoiding frequent reallocations during high transaction throughput. As long as empty entries are removed from the map (which `clear_account_contention` does), the capacity overhead is acceptable.
Applied to files:
magicblock-accounts/src/scheduled_commits_processor.rs
📚 Learning: 2025-12-03T09:36:01.527Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/src/remote_account_provider/mod.rs:1350-1353
Timestamp: 2025-12-03T09:36:01.527Z
Learning: Repo: magicblock-labs/magicblock-validator
File: magicblock-chainlink/src/remote_account_provider/mod.rs
Context: consolidate_fetched_remote_accounts
Learning: For unexpected result counts (>2), the project prefers logging an error and returning an empty Vec over panicking; acceptable during development per maintainer (Dodecahedr0x).
Applied to files:
magicblock-accounts/src/scheduled_commits_processor.rs
📚 Learning: 2025-12-03T09:33:48.707Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-committor-service/tests/test_ix_commit_local.rs:867-881
Timestamp: 2025-12-03T09:33:48.707Z
Learning: Repo: magicblock-labs/magicblock-validator PR: 639
Context: test-integration/test-committor-service/tests/test_ix_commit_local.rs (ix_commit_local)
Learning: The PhotonIndexer used for compressed account fetches (get_compressed_account) has built‑in retry logic (defaults to ~10 attempts), so tests should not add separate retry loops around compressed fetches unless there’s a specific need.
Applied to files:
magicblock-accounts/src/scheduled_commits_processor.rs
📚 Learning: 2025-10-21T14:00:54.642Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/requests/websocket/account_subscribe.rs:18-27
Timestamp: 2025-10-21T14:00:54.642Z
Learning: In magicblock-aperture account_subscribe handler (src/requests/websocket/account_subscribe.rs), the RpcAccountInfoConfig fields data_slice, commitment, and min_context_slot are currently ignored—only encoding is applied. This is tracked as technical debt in issue #579: https://github.com/magicblock-labs/magicblock-validator/issues/579
Applied to files:
magicblock-accounts/src/scheduled_commits_processor.rs
📚 Learning: 2025-11-24T14:21:00.996Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: Cargo.toml:58-58
Timestamp: 2025-11-24T14:21:00.996Z
Learning: In the magicblock-validator codebase, magicblock-api/Cargo.toml intentionally uses borsh = "1.5.3" (instead of the workspace version 0.10.4) because it needs to deserialize types from the magic-domain-program external dependency, which requires borsh 1.5.x compatibility. This is an intentional exception for interoperability with the magic domain program.
Applied to files:
magicblock-committor-service/Cargo.toml
🧬 Code graph analysis (1)
magicblock-accounts/src/scheduled_commits_processor.rs (1)
magicblock-committor-service/src/tasks/task_builder.rs (1)
committed_accounts(142-145)
🔇 Additional comments (2)
magicblock-accounts/src/scheduled_commits_processor.rs (2)
2-2: LGTM!The HashSet import is correctly added to support the deduplication logic introduced in this PR.
104-108: Effective deduplication logic that addresses the PR objective.The HashSet-based deduplication correctly keeps the first occurrence of each account by pubkey, removing duplicates before downstream processing. The implementation is efficient (O(n) with pre-allocated capacity) and prevents duplicate accounts from being logged, collected for undelegation, or processed downstream.
| tokio-util = { workspace = true } | ||
|
|
||
| [dev-dependencies] | ||
| solana-signature = { workspace = true, features = ["rand"] } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for Signature::new_unique or other rand-based signature methods in test files
echo "=== Searching for Signature::new_unique or related methods in tests ==="
rg -nP --type=rust -g '**/tests/**' -g '**/*test*.rs' 'Signature::(new_unique|from|default)' magicblock-committor-service/ || echo "No matches found"
echo ""
echo "=== Searching for imports of solana_signature in test files ==="
rg -nP --type=rust -g '**/tests/**' -g '**/*test*.rs' 'use.*solana_signature' magicblock-committor-service/ || echo "No matches found"
echo ""
echo "=== Checking for test files that might use rand/random ==="
fd -e rs -p 'test' magicblock-committor-service/ --exec rg -l 'rand|random' {} || echo "No matches found"
echo ""
echo "=== Listing all test files in magicblock-committor-service ==="
fd -e rs -p 'test' magicblock-committor-service/ || echo "No test files found"
echo ""
echo "=== Checking Cargo.toml to see dev-dependencies section ==="
cat magicblock-committor-service/Cargo.toml | grep -A 20 '\[dev-dependencies\]' || echo "No dev-dependencies section"Repository: magicblock-labs/magicblock-validator
Length of output: 663
Remove the solana-signature dev-dependency or add corresponding test files.
The solana-signature crate with the "rand" feature was added to dev-dependencies, but there are no test files in magicblock-committor-service that use it. Either remove this unused dev-dependency or add the test code that requires it.
🤖 Prompt for AI Agents
In magicblock-committor-service/Cargo.toml at line 57, the dev-dependency
"solana-signature = { workspace = true, features = [\"rand\"] }" is unused;
either remove this dev-dependency entry from Cargo.toml or add test files that
actually depend on solana-signature with the "rand" feature (e.g., create tests
under tests/ or #[cfg(test)] modules that import and exercise solana-signature
functionality so the feature is required). Ensure Cargo.toml is updated
consistently (remove the line if deleting the dev-dep, or add tests referencing
the crate if keeping it).
There was a problem hiding this comment.
I agree, I don't understand why this was added when no added test is using it?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the full diff of the PR to see all changed files
echo "=== Getting list of all files changed in this PR ==="
git diff --name-only origin/master...HEAD
echo ""
echo "=== Checking if scheduled_commits_processor.rs has any test modules ==="
rg -nP '#\[cfg\(test\)\]|#\[test\]' magicblock-accounts/src/scheduled_commits_processor.rs || echo "No test modules found"
echo ""
echo "=== Searching for any usage of Signature in scheduled_commits_processor.rs ==="
rg -nP 'Signature|solana_signature' magicblock-accounts/src/scheduled_commits_processor.rs || echo "No Signature usage found"
echo ""
echo "=== Searching more broadly for Signature::new_unique in the entire codebase ==="
rg -nP 'Signature::new_unique' --type=rust || echo "No matches found"
echo ""
echo "=== Checking if there are any test files in magicblock-committor-service ==="
fd -e rs . magicblock-committor-service/src/ magicblock-committor-service/tests/ 2>/dev/null || echo "Directories not found or no files"
Length of output: 7256
🏁 Script executed:
#!/bin/bash
# Check if the files using Signature::new_unique have test modules
echo "=== Checking for test modules in files that use Signature::new_unique ==="
echo ""
echo "--- changeset_committor_stub.rs ---"
rg -nP '#\[cfg\(test\)\]' magicblock-committor-service/src/stubs/changeset_committor_stub.rs || echo "No #[cfg(test)] found"
echo ""
echo "--- commit_persister.rs ---"
rg -nP '#\[cfg\(test\)\]' magicblock-committor-service/src/persist/commit_persister.rs || echo "No #[cfg(test)] found"
echo ""
echo "--- db.rs ---"
rg -nP '#\[cfg\(test\)\]' magicblock-committor-service/src/persist/db.rs || echo "No #[cfg(test)] found"
echo ""
echo "=== Checking the context around Signature::new_unique usage in these files ==="
echo ""
echo "--- In commit_persister.rs (showing context) ---"
rg -nC5 'Signature::new_unique' magicblock-committor-service/src/persist/commit_persister.rs | head -30
echo ""
echo "--- In db.rs (showing first few occurrences with context) ---"
rg -nC5 'Signature::new_unique' magicblock-committor-service/src/persist/db.rs | head -40
Length of output: 2922
🏁 Script executed:
#!/bin/bash
# Check if solana-signature already has rand feature in dependencies
echo "=== Checking current dependencies for solana-signature ==="
rg -nP 'solana-signature.*=' magicblock-committor-service/Cargo.toml
echo ""
echo "=== Checking workspace definition for solana-signature ==="
rg -nP 'solana-signature.*=' Cargo.toml
echo ""
echo "=== Checking git log to see when these test functions were added ==="
echo "--- commit_persister.rs test using Signature::new_unique ---"
git log -1 --format="%H %an %ad %s" -- magicblock-committor-service/src/persist/commit_persister.rs
echo ""
echo "--- db.rs test using Signature::new_unique ---"
git log -1 --format="%H %an %ad %s" -- magicblock-committor-service/src/persist/db.rs
echo ""
echo "=== Check if there are other crates in workspace with rand feature for solana-signature ==="
rg -nP 'solana-signature.*rand' --type=toml
Length of output: 912
🏁 Script executed:
#!/bin/bash
# Check what PR #733 changed in magicblock-committor-service/Cargo.toml
echo "=== Checking changes in PR #733 for this Cargo.toml ==="
git log --oneline --grep="733" | head -5
echo ""
echo "=== Checking the commit from PR #733 ==="
git show 33f33b70535feb5864b6bb337071c9df11095ec0:magicblock-committor-service/Cargo.toml | grep -A2 -B2 'solana-signature'
echo ""
echo "=== Checking if tests would compile without rand feature ==="
echo "Signature::new_unique() requires the 'rand' feature to be enabled in solana-signature"
echo ""
echo "=== Looking at current master branch for comparison ==="
git show origin/master:magicblock-committor-service/Cargo.toml | grep -A5 '\[dev-dependencies\]' || echo "Cannot access origin/master"
Length of output: 749
@thlorenz I found the explanation!
Looking at the git history, PR #733 (merged Dec 8) added test functions in commit_persister.rs and db.rs that use Signature::new_unique(). However, that PR didn't add the required rand feature to solana-signature, which means those tests likely don't compile on master right now.
Signature::new_unique() requires the rand feature to be enabled. So this dev-dependency addition is actually fixing broken tests from PR #733, but it's bundled into this PR about duplicate accounts, which is why it seems out of place.
Recommendation: This change is needed, but it should either:
- Be separated into its own small fix PR (e.g., "Fix: Enable rand feature for solana-signature to fix tests from chore: remove solana-sdk from workspace #733")
- Be documented in this PR's description explaining it's fixing a compilation issue from PR chore: remove solana-sdk from workspace #733
The mixing of unrelated fixes makes the PR harder to review and understand. Would you prefer to:
- Keep it here and update the PR description to mention this fix?
- Move it to a separate PR?
- Or merge as-is since it's a small fix?
🧠 Learnings used
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: Cargo.toml:58-58
Timestamp: 2025-11-24T14:21:00.996Z
Learning: In the magicblock-validator codebase, magicblock-api/Cargo.toml intentionally uses borsh = "1.5.3" (instead of the workspace version 0.10.4) because it needs to deserialize types from the magic-domain-program external dependency, which requires borsh 1.5.x compatibility. This is an intentional exception for interoperability with the magic domain program.
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 614
File: test-integration/programs/schedulecommit/Cargo.toml:19-20
Timestamp: 2025-11-07T18:19:48.996Z
Learning: In Solana programs (crates depending on solana-program), the empty feature flags `custom-heap = []` and `custom-panic = []` are standard declarations required to avoid compiler warnings. These should not be flagged as unused features in Cargo.toml files for Solana program crates.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| return (intent, vec![]); | ||
| }; | ||
|
|
||
| // Filter duplicate accounts |
There was a problem hiding this comment.
Why do we not dedupe when the commit is scheduled instead?
There was a problem hiding this comment.
To save CUs, but I'm actually not sure where it is more correct to do it. This isn't part of validation since the Intent would still be valid.
There was a problem hiding this comment.
We really need to come up with more elegant/efficient way to dedup stuff... :(
| tokio-util = { workspace = true } | ||
|
|
||
| [dev-dependencies] | ||
| solana-signature = { workspace = true, features = ["rand"] } |
There was a problem hiding this comment.
I agree, I don't understand why this was added when no added test is using it?
If user specifies same account twice in smart contract, it will result in creating Intent with duplicates in it.
Summary by CodeRabbit
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.