Skip to content

Commit d259367

Browse files
authored
fix: periodic committer attempts to commit undelegated accounts & sqlite INTEGER overflow (#556)
In `ExternalAccountsManager` ignore nondelegated accounts during periodic commits Additionally fixes sqlite INTEGER overflow <!-- greptile_comment --> <h2>Greptile Overview</h2> Updated On: 2025-09-25 12:32:37 UTC <h3>Summary</h3> This PR fixes a critical issue in the periodic commit process by preventing attempts to commit undelegated accounts, which was causing failures in the `ExternalAccountsManager`. **Key Changes:** - **Fixed MESSAGE_ID overflow**: Changed initialization from `u64::MAX - 1` to `(i64::MAX - 1) as u64` to prevent potential overflow issues - **Added delegation filter**: Introduced `.filter(|(_, _, _, acc)| acc.is_delegated())` in the periodic commit pipeline to only process accounts that are actually delegated to the validator - **Enhanced error logging**: Added intent ID to error messages in the intent execution engine for better debugging of failed executions - **Dependency updates**: Updated Cargo.lock with delegation program and async dependencies <h3>Confidence Score: 4/5</h3> - This PR is safe to merge with careful attention to the delegation filter logic - Score reflects a well-targeted fix for a specific delegation issue with good intent ID logging improvement. The MESSAGE_ID fix addresses a potential overflow, and the delegation filter prevents inappropriate commit attempts. Only minor risk is ensuring the is_delegated() method correctly identifies delegation status - Pay attention to magicblock-accounts/src/external_accounts_manager.rs to ensure the delegation filter works correctly in all scenarios <h3>Important Files Changed</h3> File Analysis | Filename | &nbsp;&nbsp;&nbsp;&nbsp; &nbsp; Score &nbsp;&nbsp;&nbsp;&nbsp; &nbsp; | Overview | |----------|-------|----------| | magicblock-accounts/src/external_accounts_manager.rs | 4/5 | Fixed MESSAGE_ID overflow issue and added delegation filter to prevent committing undelegated accounts during periodic commits | | magicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rs | 5/5 | Enhanced error logging by adding intent ID to error messages for better debugging of failed BaseIntent executions | </details> <h3>Sequence Diagram</h3> ```mermaid sequenceDiagram participant EAM as ExternalAccountsManager participant IAP as InternalAccountProvider participant Filter as DelegationFilter participant Committer as CommitService Note over EAM: Periodic commit cycle begins EAM->>EAM: create_scheduled_base_intents() EAM->>EAM: Initialize MESSAGE_ID with (i64::MAX - 1) as u64 loop For each account to be committed EAM->>IAP: get_account(pubkey) IAP-->>EAM: AccountSharedData Note over EAM,Filter: NEW: Check delegation status EAM->>Filter: acc.is_delegated() Filter-->>EAM: true/false alt Account is delegated EAM->>EAM: Check if account hash changed alt Hash changed EAM->>EAM: Create AccountCommittee EAM->>Committer: Schedule for commit end else Account not delegated Note over EAM: Skip account (prevents undelegated commit attempts) end end EAM-->>Committer: List of delegated accounts to commit ``` <!-- /greptile_comment -->
1 parent 882f208 commit d259367

File tree

3 files changed

+167
-11
lines changed

3 files changed

+167
-11
lines changed

magicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rs

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -246,7 +246,10 @@ where
246246
.execute(intent.inner.clone(), persister)
247247
.await
248248
.inspect_err(|err| {
249-
error!("Failed to execute BaseIntent: {:?}", err)
249+
error!(
250+
"Failed to execute BaseIntent. id: {}. {:?}",
251+
intent.id, err
252+
)
250253
})
251254
.map(|output| ExecutionOutputWrapper {
252255
id: intent.id,

magicblock-committor-service/src/persist/db.rs

Lines changed: 162 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -250,7 +250,7 @@ impl CommittsDb {
250250
.and_then(|s| s.finalize_stage_signature)
251251
.map(|s| s.to_string()),
252252
pubkey.to_string(),
253-
message_id
253+
u64_into_i64(message_id)
254254
],
255255
)?;
256256

@@ -283,7 +283,7 @@ impl CommittsDb {
283283
.and_then(|s| s.finalize_stage_signature)
284284
.map(|s| s.to_string()),
285285
pubkey.to_string(),
286-
commit_id
286+
u64_into_i64(commit_id)
287287
],
288288
)?;
289289

@@ -304,7 +304,11 @@ impl CommittsDb {
304304

305305
self.conn.execute(
306306
query,
307-
params![value.as_str(), pubkey.to_string(), commit_id],
307+
params![
308+
value.as_str(),
309+
pubkey.to_string(),
310+
u64_into_i64(commit_id)
311+
],
308312
)?;
309313

310314
Ok(())
@@ -324,7 +328,11 @@ impl CommittsDb {
324328

325329
self.conn.execute(
326330
query,
327-
params![commit_id, pubkey.to_string(), message_id],
331+
params![
332+
u64_into_i64(commit_id),
333+
pubkey.to_string(),
334+
u64_into_i64(message_id)
335+
],
328336
)?;
329337

330338
Ok(())
@@ -494,9 +502,9 @@ impl CommittsDb {
494502
(?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9, ?10, ?11, ?12, ?13, ?14, ?15, ?16, ?17)",
495503
),
496504
params![
497-
commit.message_id,
505+
u64_into_i64(commit.message_id),
498506
commit.pubkey.to_string(),
499-
commit.commit_id,
507+
u64_into_i64(commit.commit_id),
500508
commit.delegated_account_owner.to_string(),
501509
u64_into_i64(commit.slot),
502510
commit.ephemeral_blockhash.to_string(),
@@ -541,7 +549,7 @@ impl CommittsDb {
541549
let query =
542550
format!("{SELECT_ALL_COMMIT_STATUS_COLUMNS} WHERE message_id = ?1");
543551
let stmt = &mut self.conn.prepare(&query)?;
544-
let mut rows = stmt.query(params![message_id])?;
552+
let mut rows = stmt.query(params![u64_into_i64(message_id)])?;
545553

546554
extract_committor_rows(&mut rows)
547555
}
@@ -555,7 +563,8 @@ impl CommittsDb {
555563
"{SELECT_ALL_COMMIT_STATUS_COLUMNS} WHERE message_id = ?1 AND pubkey = ?2"
556564
);
557565
let stmt = &mut self.conn.prepare(&query)?;
558-
let mut rows = stmt.query(params![message_id, pubkey.to_string()])?;
566+
let mut rows =
567+
stmt.query(params![u64_into_i64(message_id), pubkey.to_string()])?;
559568

560569
extract_committor_rows(&mut rows).map(|mut rows| rows.pop())
561570
}
@@ -566,7 +575,7 @@ impl CommittsDb {
566575
) -> CommitPersistResult<()> {
567576
let query = "DELETE FROM commit_status WHERE message_id = ?1";
568577
let stmt = &mut self.conn.prepare(query)?;
569-
stmt.execute(params![message_id])?;
578+
stmt.execute(params![u64_into_i64(message_id)])?;
570579
Ok(())
571580
}
572581

@@ -583,7 +592,8 @@ impl CommittsDb {
583592
LIMIT 1";
584593

585594
let mut stmt = self.conn.prepare(query)?;
586-
let mut rows = stmt.query(params![commit_id, pubkey.to_string()])?;
595+
let mut rows =
596+
stmt.query(params![u64_into_i64(commit_id), pubkey.to_string()])?;
587597

588598
let result = rows
589599
.next()?
@@ -960,4 +970,146 @@ mod tests {
960970
let retrieved = db.get_commit_status(1, &row.pubkey).unwrap().unwrap();
961971
assert!(retrieved.undelegate);
962972
}
973+
974+
#[test]
975+
fn test_flow_message_and_commit_id_roundtrip_boundaries() {
976+
let (mut db, _file) = setup_test_db();
977+
978+
// Boundary IDs we care about for storage and retrieval.
979+
// Skip u64::MAX due to the known overflow behavior in u64_into_i64.
980+
let ids: [u64; 4] =
981+
[0, i64::MAX as u64, (i64::MAX as u64) + 1, u64::MAX - 1];
982+
983+
// Prepare rows with message_id == commit_id == each boundary value.
984+
let mut rows = Vec::new();
985+
for &id in &ids {
986+
let mut row = create_test_row(id, id);
987+
// give each row a distinct status/signatures pattern to make sure
988+
// we don't accidentally mix them up in queries/updates
989+
if id % 2 == 0 {
990+
row.commit_status = CommitStatus::Pending;
991+
} else {
992+
row.commit_status =
993+
CommitStatus::Succeeded(CommitStatusSignatures {
994+
commit_stage_signature: Signature::new_unique(),
995+
finalize_stage_signature: None,
996+
});
997+
}
998+
rows.push(row);
999+
}
1000+
1001+
// Insert all rows
1002+
db.insert_commit_status_rows(&rows).unwrap();
1003+
1004+
// 1) Retrieval by message_id should give back exactly the rows with that ID,
1005+
// and the internal IDs should be round-tripped correctly.
1006+
for row in &rows {
1007+
let fetched = db.get_commit_statuses_by_id(row.message_id).unwrap();
1008+
// We inserted a single row for each (message_id, pubkey, commit_id) triple,
1009+
// so count should be 1 for this message_id.
1010+
assert_eq!(
1011+
fetched.len(),
1012+
1,
1013+
"unexpected count for message_id={}",
1014+
row.message_id
1015+
);
1016+
let got = &fetched[0];
1017+
assert_eq!(got.message_id, row.message_id, "message_id mismatch");
1018+
assert_eq!(got.commit_id, row.commit_id, "commit_id mismatch");
1019+
assert_eq!(got.pubkey, row.pubkey, "pubkey mismatch");
1020+
assert_eq!(got.commit_status, row.commit_status, "status mismatch");
1021+
}
1022+
1023+
// 2) Retrieval by (message_id, pubkey) via get_commit_status should match exactly.
1024+
for row in &rows {
1025+
let got = db
1026+
.get_commit_status(row.message_id, &row.pubkey)
1027+
.unwrap()
1028+
.unwrap();
1029+
assert_eq!(got.message_id, row.message_id);
1030+
assert_eq!(got.commit_id, row.commit_id);
1031+
assert_eq!(got.pubkey, row.pubkey);
1032+
}
1033+
1034+
// 3) Verify update by message_id preserves IDs and writes the new status.
1035+
// Use the smallest and largest IDs to stress the conversion.
1036+
let smallest_id = ids[0];
1037+
let largest_id = ids[ids.len() - 1];
1038+
1039+
{
1040+
let target =
1041+
rows.iter().find(|r| r.message_id == smallest_id).unwrap();
1042+
let new_status = CommitStatus::Failed;
1043+
db.update_status_by_message(
1044+
target.message_id,
1045+
&target.pubkey,
1046+
&new_status,
1047+
)
1048+
.unwrap();
1049+
1050+
let got = db
1051+
.get_commit_status(target.message_id, &target.pubkey)
1052+
.unwrap()
1053+
.unwrap();
1054+
assert_eq!(got.message_id, target.message_id);
1055+
assert_eq!(got.commit_id, target.commit_id);
1056+
assert_eq!(got.commit_status, new_status);
1057+
}
1058+
1059+
// 4) Verify update by commit_id also works for large negative-mapped i64.
1060+
{
1061+
let target =
1062+
rows.iter().find(|r| r.commit_id == largest_id).unwrap();
1063+
let new_status = CommitStatus::Succeeded(CommitStatusSignatures {
1064+
commit_stage_signature: Signature::new_unique(),
1065+
finalize_stage_signature: Some(Signature::new_unique()),
1066+
});
1067+
db.update_status_by_commit(
1068+
target.commit_id,
1069+
&target.pubkey,
1070+
&new_status,
1071+
)
1072+
.unwrap();
1073+
1074+
let got = db
1075+
.get_commit_status(target.message_id, &target.pubkey)
1076+
.unwrap()
1077+
.unwrap();
1078+
assert_eq!(got.message_id, target.message_id);
1079+
assert_eq!(got.commit_id, target.commit_id);
1080+
assert_eq!(got.commit_status, new_status);
1081+
1082+
// also verify get_signatures_by_commit returns the same signatures
1083+
let sigs = db
1084+
.get_signatures_by_commit(target.commit_id, &target.pubkey)
1085+
.unwrap()
1086+
.unwrap();
1087+
if let CommitStatus::Succeeded(ss) = new_status {
1088+
assert_eq!(
1089+
sigs.commit_stage_signature,
1090+
ss.commit_stage_signature
1091+
);
1092+
assert_eq!(
1093+
sigs.finalize_stage_signature,
1094+
ss.finalize_stage_signature
1095+
);
1096+
} else {
1097+
panic!("unexpected status shape");
1098+
}
1099+
}
1100+
1101+
// 5) set_commit_id should update correctly across the boundary values
1102+
// (use XOR to create a different but valid u64).
1103+
for row in &rows {
1104+
let new_commit_id = row.commit_id ^ 0xDEAD_BEEF_DEAD_BEEF;
1105+
db.set_commit_id(row.message_id, &row.pubkey, new_commit_id)
1106+
.unwrap();
1107+
let got = db
1108+
.get_commit_status(row.message_id, &row.pubkey)
1109+
.unwrap()
1110+
.unwrap();
1111+
assert_eq!(got.message_id, row.message_id);
1112+
assert_eq!(got.commit_id, new_commit_id);
1113+
}
1114+
}
9631115
}

programs/magicblock/src/utils/account_actions.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ pub(crate) fn set_account_owner(
1414
acc.borrow_mut().set_owner(pubkey);
1515
}
1616

17+
/// Sets proper values on account during undelegation
1718
pub(crate) fn set_account_owner_to_delegation_program(
1819
acc: &RefCell<AccountSharedData>,
1920
) {

0 commit comments

Comments
 (0)