fix(flaky_tests): fix race condition on promotion#1087
Merged
Conversation
a30a7df to
c1c9d22
Compare
Member
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
📝 WalkthroughWalkthroughThis change introduces a new Changes
Sequence DiagramsequenceDiagram
actor Lifecycle
participant DB
participant Mempool as AtomicMempoolState
participant Cache as recent_valid_tx Cache
rect rgb(240, 248, 255)
note over Lifecycle,Cache: New DB-First Promotion Flow
end
Lifecycle->>DB: Fetch transaction header
alt DB has header
DB-->>Lifecycle: Return header
note over Lifecycle: DB hit - header found
else DB no header
DB-->>Lifecycle: None
note over Lifecycle: DB miss
end
rect rgb(220, 240, 220)
note over Lifecycle,Cache: Attempt Mempool Promotion
end
Lifecycle->>Mempool: set_promoted_height(txid, height)
alt Promotion successful
Mempool->>Mempool: Acquire write lock
Mempool->>Mempool: Find header in valid_submit_ledger_tx
Mempool->>Mempool: Set promoted_height (if None)
Mempool->>Cache: Mark as recently valid
Mempool-->>Lifecycle: Return updated header
note over Lifecycle: Mempool promotion success
rect rgb(255, 240, 220)
note over Lifecycle,DB: Update DB with mempool result
end
Lifecycle->>DB: Update header with promoted_height
else Promotion failed (txid not in mempool)
Mempool-->>Lifecycle: Return None
note over Lifecycle: Fallback to DB path
Lifecycle->>DB: Ensure promoted_height set, update
Lifecycle->>Mempool: Insert header into mempool
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)✅ Copyable Unit Test edits generated.
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🧰 Additional context used🧬 Code graph analysis (2)crates/chain/tests/promotion/data_promotion_basic.rs (1)
crates/actors/src/mempool_service/lifecycle.rs (1)
🔇 Additional comments (5)
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 |
Contributor
|
Note Unit test generation is an Early Access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
Contributor
|
Here are the copyable unit test edits: Copyable Editscoderabbit.markdownlint-cli2.jsoncThis is a new file. crates/types/tests/commitment_transaction_tests.rsThis is a new file. crates/types/tests/transaction_signing_versioned_tests.rs@@ -114,3 +114,186 @@
.signature
.validate_signature(sig_hash, signer.address()));
}
+
+#[test]
+fn commitment_tx_signature_validation_after_signing() {
+ use irys_types::irys::IrysSigner;
+ use irys_types::transaction::IrysTransactionCommon;
+
+ let config = ConsensusConfig::testing();
+ let signer = IrysSigner::random_signer(&config);
+ let mut tx = CommitmentTransaction::new_stake(&config, H256::zero());
+
+ // Sign the transaction
+ signer.sign_commitment(&mut tx).expect("signing should succeed");
+
+ // Verify the signature is valid
+ assert!(tx.is_signature_valid(), "Signature should be valid after signing");
+
+ // Verify ID is set correctly
+ assert_ne!(tx.id, H256::zero(), "ID should be set after signing");
+
+ // Verify signer is set correctly
+ assert_eq!(tx.signer, signer.address(), "Signer should match");
+}
+
+#[test]
+fn commitment_tx_signature_invalidated_by_field_changes() {
+ use irys_types::irys::IrysSigner;
+ use irys_types::transaction::IrysTransactionCommon;
+
+ let config = ConsensusConfig::testing();
+ let signer = IrysSigner::random_signer(&config);
+ let mut tx = CommitmentTransaction::new_stake(&config, H256::zero());
+
+ signer.sign_commitment(&mut tx).expect("signing should succeed");
+ assert!(tx.is_signature_valid());
+
+ // Changing the anchor should invalidate the signature
+ tx.anchor = H256::from([99_u8; 32]);
+ assert!(!tx.is_signature_valid(), "Signature should be invalid after modifying anchor");
+}
+
+#[test]
+fn commitment_tx_different_types_have_different_signatures() {
+ use irys_types::irys::IrysSigner;
+ use irys_types::transaction::IrysTransactionCommon;
+ use irys_types::CommitmentType;
+
+ let config = ConsensusConfig::testing();
+ let signer = IrysSigner::random_signer(&config);
+ let anchor = H256::from([1_u8; 32]);
+
+ let mut stake_tx = CommitmentTransaction::new_stake(&config, anchor);
+ signer.sign_commitment(&mut stake_tx).expect("signing should succeed");
+
+ let mut unstake_tx = CommitmentTransaction::new_unstake(&config, anchor);
+ signer.sign_commitment(&mut unstake_tx).expect("signing should succeed");
+
+ // Same signer, same anchor, but different commitment types should have different signatures
+ assert_ne!(stake_tx.signature, unstake_tx.signature);
+ assert_ne!(stake_tx.id, unstake_tx.id);
+}
+
+#[test]
+fn commitment_tx_pledge_signature_includes_count() {
+ use irys_types::irys::IrysSigner;
+ use irys_types::transaction::IrysTransactionCommon;
+ use irys_types::CommitmentType;
+
+ let config = ConsensusConfig::testing();
+ let signer = IrysSigner::random_signer(&config);
+ let anchor = H256::from([1_u8; 32]);
+
+ let mut pledge_tx1 = CommitmentTransaction::new_stake(&config, anchor);
+ pledge_tx1.commitment_type = CommitmentType::Pledge {
+ pledge_count_before_executing: 5,
+ };
+ signer.sign_commitment(&mut pledge_tx1).expect("signing should succeed");
+
+ let mut pledge_tx2 = CommitmentTransaction::new_stake(&config, anchor);
+ pledge_tx2.commitment_type = CommitmentType::Pledge {
+ pledge_count_before_executing: 10,
+ };
+ signer.sign_commitment(&mut pledge_tx2).expect("signing should succeed");
+
+ // Different pledge counts should produce different signatures
+ assert_ne!(pledge_tx1.signature, pledge_tx2.signature);
+ assert_ne!(pledge_tx1.id, pledge_tx2.id);
+}
+
+#[test]
+fn commitment_tx_unpledge_signature_includes_partition_hash() {
+ use irys_types::irys::IrysSigner;
+ use irys_types::transaction::IrysTransactionCommon;
+ use irys_types::CommitmentType;
+
+ let config = ConsensusConfig::testing();
+ let signer = IrysSigner::random_signer(&config);
+ let anchor = H256::from([1_u8; 32]);
+
+ let mut unpledge_tx1 = CommitmentTransaction::new_stake(&config, anchor);
+ unpledge_tx1.commitment_type = CommitmentType::Unpledge {
+ pledge_count_before_executing: 5,
+ partition_hash: H256::from([1_u8; 32]),
+ };
+ signer.sign_commitment(&mut unpledge_tx1).expect("signing should succeed");
+
+ let mut unpledge_tx2 = CommitmentTransaction::new_stake(&config, anchor);
+ unpledge_tx2.commitment_type = CommitmentType::Unpledge {
+ pledge_count_before_executing: 5,
+ partition_hash: H256::from([2_u8; 32]),
+ };
+ signer.sign_commitment(&mut unpledge_tx2).expect("signing should succeed");
+
+ // Different partition hashes should produce different signatures
+ assert_ne!(unpledge_tx1.signature, unpledge_tx2.signature);
+ assert_ne!(unpledge_tx1.id, unpledge_tx2.id);
+}
+
+#[test]
+fn commitment_tx_fee_affects_signature() {
+ use irys_types::irys::IrysSigner;
+ use irys_types::transaction::IrysTransactionCommon;
+
+ let config = ConsensusConfig::testing();
+ let signer = IrysSigner::random_signer(&config);
+ let anchor = H256::from([1_u8; 32]);
+
+ let mut tx1 = CommitmentTransaction::new_stake(&config, anchor);
+ tx1.fee = 100;
+ signer.sign_commitment(&mut tx1).expect("signing should succeed");
+
+ let mut tx2 = CommitmentTransaction::new_stake(&config, anchor);
+ tx2.fee = 200;
+ signer.sign_commitment(&mut tx2).expect("signing should succeed");
+
+ // Different fees should produce different signatures
+ assert_ne!(tx1.signature, tx2.signature);
+ assert_ne!(tx1.id, tx2.id);
+}
+
+#[test]
+fn commitment_tx_value_affects_signature() {
+ use irys_types::irys::IrysSigner;
+ use irys_types::transaction::IrysTransactionCommon;
+ use irys_types::U256;
+
+ let config = ConsensusConfig::testing();
+ let signer = IrysSigner::random_signer(&config);
+ let anchor = H256::from([1_u8; 32]);
+
+ let mut tx1 = CommitmentTransaction::new_stake(&config, anchor);
+ tx1.value = U256::from(1000u64);
+ signer.sign_commitment(&mut tx1).expect("signing should succeed");
+
+ let mut tx2 = CommitmentTransaction::new_stake(&config, anchor);
+ tx2.value = U256::from(2000u64);
+ signer.sign_commitment(&mut tx2).expect("signing should succeed");
+
+ // Different values should produce different signatures
+ assert_ne!(tx1.signature, tx2.signature);
+ assert_ne!(tx1.id, tx2.id);
+}
+
+#[test]
+fn commitment_tx_chain_id_affects_signature() {
+ use irys_types::irys::IrysSigner;
+ use irys_types::transaction::IrysTransactionCommon;
+
+ let config = ConsensusConfig::testing();
+ let signer = IrysSigner::random_signer(&config);
+ let anchor = H256::from([1_u8; 32]);
+
+ let mut tx1 = CommitmentTransaction::new_stake(&config, anchor);
+ tx1.chain_id = 1;
+ signer.sign_commitment(&mut tx1).expect("signing should succeed");
+
+ let mut tx2 = CommitmentTransaction::new_stake(&config, anchor);
+ tx2.chain_id = 2;
+ signer.sign_commitment(&mut tx2).expect("signing should succeed");
+
+ // Different chain IDs should produce different signatures (replay protection)
+ assert_ne!(tx1.signature, tx2.signature);
+ assert_ne!(tx1.id, tx2.id);
+}crates/types/tests/versioned_compact_roundtrip_tests.rs@@ -154,3 +154,97 @@
// This should panic with UnsupportedVersion error
let _ = CommitmentTransaction::from_compact(&buf, buf.len());
}
+
+#[test]
+fn test_commitment_transaction_compact_with_pledge() {
+ use irys_types::CommitmentType;
+
+ let config = ConsensusConfig::testing();
+ let mut versioned = CommitmentTransaction::new_stake(&config, H256::random());
+ versioned.commitment_type = CommitmentType::Pledge {
+ pledge_count_before_executing: 7,
+ };
+ versioned.id = H256::random();
+
+ let mut buf = Vec::new();
+ versioned.to_compact(&mut buf);
+
+ assert!(!buf.is_empty(), "buffer should contain encoded data");
+ assert_eq!(buf[0], 1, "first byte should be the discriminant");
+
+ let (decoded_versioned, rest) = CommitmentTransaction::from_compact(&buf, buf.len());
+
+ assert!(rest.is_empty(), "entire buffer should be consumed");
+ assert_eq!(decoded_versioned, versioned);
+ assert_eq!(decoded_versioned.version(), 1);
+
+ // Verify pledge-specific fields
+ match decoded_versioned.commitment_type {
+ CommitmentType::Pledge { pledge_count_before_executing } => {
+ assert_eq!(pledge_count_before_executing, 7);
+ }
+ _ => panic!("Expected Pledge commitment type"),
+ }
+}
+
+#[test]
+fn test_commitment_transaction_compact_with_unpledge() {
+ use irys_types::CommitmentType;
+
+ let config = ConsensusConfig::testing();
+ let partition_hash = H256::from([42_u8; 32]);
+ let mut versioned = CommitmentTransaction::new_stake(&config, H256::random());
+ versioned.commitment_type = CommitmentType::Unpledge {
+ pledge_count_before_executing: 10,
+ partition_hash,
+ };
+ versioned.id = H256::random();
+
+ let mut buf = Vec::new();
+ versioned.to_compact(&mut buf);
+
+ let (decoded_versioned, rest) = CommitmentTransaction::from_compact(&buf, buf.len());
+
+ assert!(rest.is_empty());
+ assert_eq!(decoded_versioned, versioned);
+
+ // Verify unpledge-specific fields
+ match decoded_versioned.commitment_type {
+ CommitmentType::Unpledge { pledge_count_before_executing, partition_hash: ph } => {
+ assert_eq!(pledge_count_before_executing, 10);
+ assert_eq!(ph, partition_hash);
+ }
+ _ => panic!("Expected Unpledge commitment type"),
+ }
+}
+
+#[test]
+fn test_commitment_transaction_compact_with_all_commitment_types() {
+ use irys_types::CommitmentType;
+
+ let config = ConsensusConfig::testing();
+ let test_cases = vec![
+ ("Stake", CommitmentType::Stake),
+ ("Unstake", CommitmentType::Unstake),
+ ("Pledge", CommitmentType::Pledge { pledge_count_before_executing: 5 }),
+ ("Unpledge", CommitmentType::Unpledge {
+ pledge_count_before_executing: 3,
+ partition_hash: H256::from([99_u8; 32]),
+ }),
+ ];
+
+ for (name, commitment_type) in test_cases {
+ let mut tx = CommitmentTransaction::new_stake(&config, H256::random());
+ tx.commitment_type = commitment_type.clone();
+ tx.id = H256::random();
+
+ let mut buf = Vec::new();
+ tx.to_compact(&mut buf);
+
+ let (decoded, rest) = CommitmentTransaction::from_compact(&buf, buf.len());
+
+ assert!(rest.is_empty(), "{}: entire buffer should be consumed", name);
+ assert_eq!(decoded, tx, "{}: decoded should match original", name);
+ assert_eq!(decoded.commitment_type, commitment_type, "{}: commitment type should match", name);
+ }
+}crates/types/tests/versioned_json_serde_tests.rs@@ -102,3 +102,89 @@
"Version should be 1"
);
}
+
+#[test]
+fn test_commitment_transaction_pledge_json_roundtrip() {
+ use irys_types::CommitmentType;
+
+ let config = ConsensusConfig::testing();
+ let mut commitment_tx = CommitmentTransaction::new_stake(&config, H256::zero());
+ commitment_tx.commitment_type = CommitmentType::Pledge {
+ pledge_count_before_executing: 5,
+ };
+ commitment_tx.id = H256::from([3_u8; 32]);
+
+ let json = serde_json::to_string(&commitment_tx).unwrap();
+ let deserialized: CommitmentTransaction = serde_json::from_str(&json).unwrap();
+
+ assert_eq!(commitment_tx.id, deserialized.id);
+ assert_eq!(commitment_tx.commitment_type, deserialized.commitment_type);
+ assert_eq!(commitment_tx.version(), deserialized.version());
+}
+
+#[test]
+fn test_commitment_transaction_unpledge_json_roundtrip() {
+ use irys_types::CommitmentType;
+
+ let config = ConsensusConfig::testing();
+ let mut commitment_tx = CommitmentTransaction::new_stake(&config, H256::zero());
+ commitment_tx.commitment_type = CommitmentType::Unpledge {
+ pledge_count_before_executing: 10,
+ partition_hash: H256::from([42_u8; 32]),
+ };
+ commitment_tx.id = H256::from([4_u8; 32]);
+
+ let json = serde_json::to_string(&commitment_tx).unwrap();
+ let deserialized: CommitmentTransaction = serde_json::from_str(&json).unwrap();
+
+ assert_eq!(commitment_tx.id, deserialized.id);
+ assert_eq!(commitment_tx.commitment_type, deserialized.commitment_type);
+
+ // Verify partition_hash is preserved
+ if let CommitmentType::Unpledge { partition_hash, .. } = deserialized.commitment_type {
+ assert_eq!(partition_hash, H256::from([42_u8; 32]));
+ } else {
+ panic!("Expected Unpledge commitment type");
+ }
+}
+
+#[test]
+fn test_commitment_transaction_unstake_json_roundtrip() {
+ use irys_types::CommitmentType;
+
+ let config = ConsensusConfig::testing();
+ let mut commitment_tx = CommitmentTransaction::new_unstake(&config, H256::from([5_u8; 32]));
+ commitment_tx.id = H256::from([6_u8; 32]);
+
+ let json = serde_json::to_string(&commitment_tx).unwrap();
+ let deserialized: CommitmentTransaction = serde_json::from_str(&json).unwrap();
+
+ assert_eq!(commitment_tx.id, deserialized.id);
+ assert_eq!(commitment_tx.commitment_type, CommitmentType::Unstake);
+ assert_eq!(deserialized.commitment_type, CommitmentType::Unstake);
+}
+
+#[test]
+fn test_commitment_transaction_preserves_all_fields_json() {
+ use irys_types::{CommitmentType, IrysAddress, IrysSignature, Signature, U256};
+
+ let config = ConsensusConfig::testing();
+ let mut commitment_tx = CommitmentTransaction::new_stake(&config, H256::from([7_u8; 32]));
+ commitment_tx.id = H256::from([8_u8; 32]);
+ commitment_tx.signer = IrysAddress::from_slice(&[9_u8; 20]);
+ commitment_tx.fee = 12345;
+ commitment_tx.value = U256::from(67890u64);
+ commitment_tx.chain_id = 999;
+ commitment_tx.signature = IrysSignature::new(Signature::test_signature());
+
+ let json = serde_json::to_string(&commitment_tx).unwrap();
+ let deserialized: CommitmentTransaction = serde_json::from_str(&json).unwrap();
+
+ assert_eq!(commitment_tx.id, deserialized.id);
+ assert_eq!(commitment_tx.anchor, deserialized.anchor);
+ assert_eq!(commitment_tx.signer, deserialized.signer);
+ assert_eq!(commitment_tx.fee, deserialized.fee);
+ assert_eq!(commitment_tx.value, deserialized.value);
+ assert_eq!(commitment_tx.chain_id, deserialized.chain_id);
+ assert_eq!(commitment_tx.commitment_type, deserialized.commitment_type);
+}docs/testing/commitment_transaction_tests.mdThis is a new file. markdownlint-cli2-results.jsonThis is a new file. |
JesseTheRobot
approved these changes
Dec 29, 2025
3 tasks
This was referenced Mar 30, 2026
Open
Open
5 tasks
This was referenced Apr 16, 2026
This was referenced May 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe the changes
Summary
Before:
Tests that check
is_promotedaftermine_block()exhibited race conditions. The test would check promotion status before the mempool service processed theBlockConfirmedmessage, causing intermittent failures.After:
Added
wait_until_height_confirmed()call after mining to ensureBlockConfirmedmessage is processed before checking promotion status. Also added atomicset_promoted_height()method to mempool for thread-safe promotion updates.Changes
Mempool Service (
crates/actors/src/mempool_service.rs)set_promoted_height()method that atomically sets promotion height while holding write lockMempool Lifecycle (
crates/actors/src/mempool_service/lifecycle.rs)BlockConfirmedhandler to use atomicset_promoted_height()firstTest Fix (
crates/chain/tests/promotion/data_promotion_basic.rs)heavy_promotion_validates_ingress_proof_anchor: Get height before mining, wait forheight + 1confirmation before checkingis_promotedRelated Issue(s)
Please link to the issue(s) that will be closed with this PR.
Checklist
Additional Context
Add any other context about the pull request here.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.