Skip to content

Hotfix: duplicate accounts in intents #744

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.

Sign up for GitHub

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

Open
taco-paco wants to merge 5 commits into
base: master
Choose a base branch
from
+5 −0
Open

Hotfix: duplicate accounts in intents #744

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
63d466d
hotfix: duplicate accounts in Intents
taco-paco Dec 12, 2025
6b1116c
fix: .lock
taco-paco Dec 12, 2025
3cb97d9
fix: fmt
taco-paco Dec 12, 2025
74aea1b
Merge branch 'master' into hotfix/base-layer-ix/duplicate-accounts-in…
taco-paco Dec 15, 2025
d7c9b69
feat: .lock
taco-paco Dec 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions magicblock-accounts/src/scheduled_commits_processor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,10 @@ impl ScheduledCommitsProcessorImpl {
return (intent, vec![]);
};

// Filter duplicate accounts
Copy link
Collaborator

@thlorenz thlorenz Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we not dedupe when the commit is scheduled instead?

Copy link
Contributor Author

@taco-paco taco-paco Dec 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@bmuddha bmuddha Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We really need to come up with more elegant/efficient way to dedup stuff... :(

let mut seen = HashSet::with_capacity(committed_accounts.len());
committed_accounts.retain(|account| seen.insert(account.pubkey));

// dump undelegated pubkeys
let pubkeys_being_undelegated: Vec<_> = committed_accounts
.iter()
Expand Down
1 change: 1 addition & 0 deletions magicblock-committor-service/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ tokio = { workspace = true }
tokio-util = { workspace = true }

[dev-dependencies]
solana-signature = { workspace = true, features = ["rand"] }
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 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).

Copy link
Collaborator

@thlorenz thlorenz Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, I don't understand why this was added when no added test is using it?

Copy link
Contributor

@coderabbitai coderabbitai bot Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 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:

  1. 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")
  2. 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.

Copy link
Contributor Author

@taco-paco taco-paco Dec 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests need it

Copy link
Contributor

@coderabbitai coderabbitai bot Dec 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

lazy_static = { workspace = true }
rand = { workspace = true }
tempfile = { workspace = true }
Expand Down