feat: status endpoint#1099
Conversation
📝 WalkthroughWalkthroughAdds per-transaction metadata types and storage, a v1→v2 DB migration moving promoted_height into metadata, mempool metadata APIs and persistence, transaction-status computation plus API and client wiring, and extensive call-site/test migrations to metadata-wrapped types and new accessors. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Api as ApiServer
participant BlockTree as BlockTree
participant DB as Database
participant Mempool as Mempool
participant BlockIndex as BlockIndex
Client->>Api: GET /tx/{tx_id}/status
Api->>BlockTree: read current head height
Api->>DB: get_commitment_tx_metadata(tx_id)
Api->>DB: get_data_tx_metadata(tx_id)
Api->>Mempool: is_recent_valid_tx(tx_id)
Api->>Mempool: get_tx_metadata(tx_id)
Api->>Api: db_metadata_to_tx_metadata(commitment, data)
Api->>Api: compute_transaction_status(db_metadata, tx_id, block_index_guard, head_height, mempool_guard)
alt found in DB or mempool
Api->>BlockIndex: inspect included_height / confirmations
BlockIndex-->>Api: included/confirmed info
Api-->>Client: TransactionStatusResponse (Confirmed/Finalized)
else pending in mempool
Api-->>Client: TransactionStatusResponse (Pending)
else not found
Api-->>Client: ApiError::ErrNoId
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
b6d7994 to
7541c20
Compare
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
crates/p2p/src/gossip_client.rs (1)
953-974: Consider keepingArc<BlockBody>to avoid unnecessary cloning.The return type changed from
Arc<BlockBody>to ownedBlockBody, requiring a clone on line 966. SinceGossipDataV2::BlockBodyalready wraps the body in anArc, this clone may be wasteful if callers typically need to wrap the result inArcagain downstream.If callers often need shared ownership (e.g., for caching or passing to multiple consumers), consider keeping the
Arc<BlockBody>return type to avoid the clone:pub async fn pull_block_body_from_network( &self, header: Arc<IrysBlockHeader>, use_trusted_peers_only: bool, peer_list: &PeerList, - ) -> Result<(IrysAddress, BlockBody), PeerNetworkError> { + ) -> Result<(IrysAddress, Arc<BlockBody>), PeerNetworkError> { let data_request = GossipDataRequestV2::BlockBody(header.block_hash); self.pull_data_from_network( data_request, Some(&header), use_trusted_peers_only, peer_list, |gossip_data| match gossip_data { - GossipDataV2::BlockBody(body) => Ok((*body).clone()), + GossipDataV2::BlockBody(body) => Ok(body), _ => Err(PeerNetworkError::UnexpectedData(format!( "Expected BlockBody, got {:?}", gossip_data.data_type_and_id() ))), }, ) .await }However, if the design intent is to transfer ownership and callers don't need shared references, the current approach is acceptable.
crates/actors/src/block_tree_service.rs (2)
221-237: Minor: avoid repeatedblock.header()calls in the hot path.
You callblock.header()twice to extract hash/height; consider binding once for readability and to avoid accidental divergence if later refactors add more fields.
472-515: Fix potential u64 underflow in panic formatting (height - 1).
If a height-0 block ever reaches this path,block_header.height - 1will underflow and can panic before your intended message. Usesaturating_sub(1)in the formatting.Proposed diff
- block_header.height - 1 + block_header.height.saturating_sub(1)crates/actors/src/validation_service/block_validation_task.rs (1)
266-513: Optional: bindlet header = self.sealed_block.header();once invalidate_blockto reduce repetition.
It would simplify span fields and reduce repeated accessor calls across the task setup.crates/actors/src/mempool_service.rs (3)
2169-2230: Fix likely compile errors: functions still treatvalid_submit_ledger_txentries asDataTransactionHeader.
Withvalid_submit_ledger_tx: BTreeMap<H256, DataTxWithMetadata>, methods liketake_all_valid_txs,update_submit_transaction,clear_promoted_height, andset_promoted_heightmust unwrap/mutatewrapped.tx(and preservewrapped.metadata).Proposed fix (unwrap/mutate wrapped tx while preserving metadata)
pub async fn take_all_valid_txs( &self, ) -> ( BTreeMap<H256, DataTransactionHeader>, BTreeMap<IrysAddress, Vec<CommitmentTransaction>>, ) { let mut state = self.write().await; state.recent_valid_tx.clear(); ( - std::mem::take(&mut state.valid_submit_ledger_tx), + std::mem::take(&mut state.valid_submit_ledger_tx) + .into_iter() + .map(|(id, wrapped)| (id, wrapped.tx)) + .collect(), std::mem::take(&mut state.valid_commitment_tx), ) } pub async fn update_submit_transaction(&self, tx: DataTransactionHeader) { self.0 .write() .await .valid_submit_ledger_tx .entry(tx.id) - .and_modify(|t| *t = tx); + .and_modify(|wrapped| { + wrapped.tx = tx.clone(); + // If you cache fields like wrapped.data_size, ensure they stay in sync here. + }) + .or_insert_with(|| DataTxWithMetadata::new(tx)); } pub async fn clear_promoted_height(&self, txid: H256) -> bool { let mut cleared = false; let mut state = self.write().await; - if let Some(header) = state.valid_submit_ledger_tx.get_mut(&txid) { - header.promoted_height = None; + if let Some(wrapped) = state.valid_submit_ledger_tx.get_mut(&txid) { + wrapped.tx.promoted_height = None; state.recent_valid_tx.put(txid, ()); tracing::debug!(tx.id = %txid, "Cleared promoted_height in mempool"); cleared = true; } cleared } pub async fn set_promoted_height( &self, txid: H256, height: u64, ) -> Option<DataTransactionHeader> { let mut state = self.write().await; - let header = state.valid_submit_ledger_tx.get_mut(&txid)?; - if header.promoted_height.is_none() { - header.promoted_height = Some(height); + let wrapped = state.valid_submit_ledger_tx.get_mut(&txid)?; + if wrapped.tx.promoted_height.is_none() { + wrapped.tx.promoted_height = Some(height); } - let result = header.clone(); + let result = wrapped.tx.clone(); tracing::debug!(tx.id = %txid, promoted_height = height, "Set promoted_height in mempool"); state.recent_valid_tx.put(txid, ()); Some(result) }
2317-2433: Fix persistence helpers: they still treatCommitmentTxWithMetadataasCommitmentTransaction.
persist_mempool_to_disk()callsget_all_commitment_tx(), which currently iteratesvalid_commitment_txbut doesn’t unwrapwrapped.tx. Same issue inall_commitment_transactions(). This will either not compile or serialize the wrong type.Proposed fix (unwrap commitment wrappers)
pub async fn get_all_commitment_tx(&self) -> HashMap<IrysTransactionId, CommitmentTransaction> { let mut hash_map = HashMap::new(); let mempool_state_guard = self.read().await; // Get any CommitmentTransactions from the valid commitments mempool_state_guard .valid_commitment_tx .values() .flat_map(|txs| txs.iter()) .for_each(|tx| { - hash_map.insert(tx.id(), tx.clone()); + hash_map.insert(tx.id(), tx.tx.clone()); }); // Get any CommitmentTransactions from the pending commitments mempool_state_guard .pending_pledges .iter() .flat_map(|(_, inner)| inner.iter()) .for_each(|(tx_id, tx)| { hash_map.insert(*tx_id, tx.clone()); }); hash_map } async fn all_commitment_transactions(&self) -> HashMap<H256, CommitmentTransaction> { let mut hash_map = HashMap::new(); let mempool_state_guard = self.read().await; mempool_state_guard .valid_commitment_tx .values() .flat_map(|txs| txs.iter()) .for_each(|tx| { - hash_map.insert(tx.id(), tx.clone()); + hash_map.insert(tx.id(), tx.tx.clone()); }); mempool_state_guard .pending_pledges .iter() .flat_map(|(_, inner)| inner.iter()) .for_each(|(tx_id, tx)| { hash_map.insert(*tx_id, tx.clone()); }); hash_map }
2513-2694: Metadata preservation on “update existing tx” should be done field-wise (not wholesale overwrite).
bounded_insert_data_tx_wrappedcurrently preservesincluded_heightby swapping the entiremetadatastruct. Ifmetadatagrows beyondincluded_height, this risks dropping new fields from either side.crates/types/src/block.rs (1)
1068-1124:BlockBody::tx_ids_match_the_headershould also validateblock_hash.Right now it compares tx IDs/signatures but never checks
self.block_hash == header.block_hash. That allows a body to claim a differentblock_hashthan the header it’s paired with, which is an important integrity invariant for caching/routing.Proposed fix
pub fn tx_ids_match_the_header(&self, header: &IrysBlockHeader) -> eyre::Result<bool> { + if self.block_hash != header.block_hash { + return Ok(false); + } let res = self.verify_tx_signatures(); if !res { return Ok(false); }crates/chain/tests/validation/mod.rs (1)
55-67: Don’tunwrap()inside a helper that returnsResult.Either propagate the send error or change the signature to not return
Result.Proposed fix
fn send_block_to_block_validation( node_ctx: &IrysNodeCtx, block: Arc<SealedBlock>, ) -> Result<(), PreValidationError> { node_ctx .service_senders .validation_service .send(ValidationServiceMessage::ValidateBlock { block, skip_vdf_validation: false, - }) - .unwrap(); + })?; Ok(()) }
🤖 Fix all issues with AI agents
In `@crates/actors/src/block_producer.rs`:
- Around line 744-756: Extract a local reference to the block header to avoid
repeated calls to block.header(): create a variable like let header =
block.header(); then replace subsequent calls (block.header().data_ledgers,
block.header().block_hash, block.header().height, block.header().solution_hash,
block.header().vdf_limiter_info.global_step_number,
block.header().previous_block_hash) with header.data_ledgers, header.block_hash,
header.height, header.solution_hash, header.vdf_limiter_info.global_step_number,
and header.previous_block_hash respectively so the debug! invocation and the
tx_ids check use the single header reference.
In `@crates/api-server/src/routes/tx.rs`:
- Around line 305-346: In get_tx_status, the call to
state.block_index.read_blocking() is invalid; replace it with
state.block_index.read() and pass that read guard reference into
irys_actors::compute_transaction_status (e.g., use &state.block_index.read() in
the closure) so the types match the other handlers that use block_index.read().
Ensure the closure still returns the expected result and propagate errors as
before.
In `@crates/chain/tests/multi_node/fork_recovery.rs`:
- Around line 4-6: The BlockBody construction can end up with duplicate
DataTransaction entries because a_block2_txs.all_data_txs() may return the same
tx from multiple ledgers; before assigning to BlockBody.data_transactions (where
BlockBody is constructed), de-duplicate the collection by tx.id (e.g., iterate
a_block2_txs.all_data_txs(), track seen tx.id values in a HashSet and push only
the first occurrence) so the body contains unique transactions and avoids
ambiguous "extra tx" scenarios.
In `@crates/chain/tests/validation/blobs_rejected.rs`:
- Around line 83-92: Replace any uses of .unwrap() on IrysSealedBlock::new(...)
with the ? operator so errors propagate through the test's eyre::Result; e.g.,
change Arc::new(IrysSealedBlock::new(header, body).unwrap()) to create the block
from the Result and then wrap it (Arc::new(IrysSealedBlock::new(header,
body)?)); apply the same replacement for the other test occurrences that call
IrysSealedBlock::new (the other similar blocks in the file).
In `@crates/chain/tests/validation/mod.rs`:
- Around line 352-361: Tests call SealedBlock::new(...).unwrap() which loses
error context; replace those unwrap() calls with the ? operator on
SealedBlock::new (e.g., let block = Arc::new(SealedBlock::new(header, body)?);)
and update the test function signatures to return a Result (e.g., -> Result<(),
_> or -> anyhow::Result<()>) so the ? can propagate errors; apply the same
change for every other test that uses SealedBlock::new(...).unwrap().
In `@crates/database/src/db_index.rs`:
- Around line 73-139: The test module has a formatting violation flagged by
cargo fmt; run `cargo fmt` and reformat the file containing the tests (module
tests with functions test_tx_metadata_operations and test_batch_operations) to
fix spacing/indentation and brace placement inconsistencies around the end of
the tests (near the tests module closing brace). After running `cargo fmt`,
verify the module compiles and the functions set_tx_included_height,
clear_tx_included_height, batch_set_tx_included_height,
batch_clear_tx_included_height, and get_tx_metadata remain unchanged and the
imports (open_or_create_db, IrysTables, tempfile::tempdir, H256) are still
correct.
- Around line 29-48: Change set_tx_included_height and clear_tx_included_height
to detect when get_tx_metadata returns None instead of unconditionally creating
default metadata: call get_tx_metadata(tx, tx_id) and match on the Option, set
included_height on the existing metadata (or create it only if you intentionally
want upsert behavior) and then call put_tx_metadata(tx, tx_id, metadata); update
the function signatures to return Result<bool, reth_db::DatabaseError> where the
boolean indicates whether metadata existed prior to the call (true = updated,
false = created), or alternatively log a warning when metadata was missing
before calling put_tx_metadata — reference the functions set_tx_included_height,
clear_tx_included_height, get_tx_metadata, and put_tx_metadata when making the
change.
In `@crates/domain/src/snapshots/epoch_snapshot/epoch_snapshot.rs`:
- Around line 5-16: Remove the glob import use irys_database::data_ledger::*;
and replace it with explicit imports of only the concrete symbols this module
actually uses from irys_database::data_ledger (e.g., the specific
structs/functions referenced in epoch_snapshot.rs). Also audit and remove any
symbols that are now imported from irys_types (SystemLedger, DataLedger,
CommitmentStatus) to avoid duplicate/conflicting imports and ensure each type
comes from the correct crate.
In `@crates/types/src/block.rs`:
- Line 22: IrysBlockHeaderV1::get_commitment_ledger_tx_ids currently hardcodes
ledger_id == 0; update this to use the imported SystemLedger enum by replacing
the magic-number check with ledger_id == SystemLedger::Commitment as u32 and
remove or update the obsolete circular-dependency comment referencing the old
import limitation so the function relies on SystemLedger::Commitment instead of
the literal 0 (apply same change to the other occurrence around lines 460-472).
- Around line 1227-1296: The missing-IDs computation is quadratic because it
repeatedly scans the resolved vectors; instead build a HashSet of found IDs for
each ledger (e.g., for submit_txs, publish_txs and commitment_txs) — using the
tx.id or tx.id() accessor as appropriate — and compute missing_ids by set
subtraction from the expected IDs (submit_ids/publish_ids/commitment_ids
converted to a set); replace the current filter_map/any-based missing-id logic
in the block-to-transactions resolution (the code around submit_txs,
publish_txs, commitment_txs and the three header/body checks) with these HashSet
differences to make detection deterministic and O(n).
- Around line 1133-1150: SealedBlock::new currently ignores the boolean return
of body.tx_ids_match_the_header(&header)?; — call it, check the bool, and return
an error if false so mismatched tx ids cannot produce a SealedBlock. Concretely,
replace the discarded-call with a boolean binding (e.g., let ok =
body.tx_ids_match_the_header(&header)?;) and if !ok return an eyre error (use
eyre::bail! or eyre::ensure!) with a clear message; keep the subsequent ordering
logic (SealedBlock::order_transactions) only when the check passes.
📜 Review details
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (45)
crates/actors/src/block_discovery.rscrates/actors/src/block_producer.rscrates/actors/src/block_tree_service.rscrates/actors/src/block_validation.rscrates/actors/src/lib.rscrates/actors/src/mempool_guard.rscrates/actors/src/mempool_service.rscrates/actors/src/mempool_service/lifecycle.rscrates/actors/src/validation_service.rscrates/actors/src/validation_service/active_validations.rscrates/actors/src/validation_service/block_validation_task.rscrates/api-server/src/lib.rscrates/api-server/src/routes/tx.rscrates/chain/src/chain.rscrates/chain/tests/block_production/block_production.rscrates/chain/tests/block_production/block_rebuilding.rscrates/chain/tests/block_production/block_validation.rscrates/chain/tests/block_production/difficulty_adjustment.rscrates/chain/tests/external/block_production.rscrates/chain/tests/multi_node/fork_recovery.rscrates/chain/tests/multi_node/mempool_tests.rscrates/chain/tests/multi_node/validation.rscrates/chain/tests/utils.rscrates/chain/tests/validation/blobs_rejected.rscrates/chain/tests/validation/data_tx_pricing.rscrates/chain/tests/validation/invalid_perm_fee_refund.rscrates/chain/tests/validation/mod.rscrates/chain/tests/validation/unpledge_partition.rscrates/chain/tests/validation/unstake_edge_cases.rscrates/database/src/db_index.rscrates/database/src/system_ledger.rscrates/database/src/tables.rscrates/domain/src/models/block_tree.rscrates/domain/src/snapshots/epoch_snapshot/epoch_replay_data.rscrates/domain/src/snapshots/epoch_snapshot/epoch_snapshot.rscrates/p2p/src/block_pool.rscrates/p2p/src/chain_sync.rscrates/p2p/src/gossip_client.rscrates/p2p/src/gossip_data_handler.rscrates/p2p/src/tests/block_pool/mod.rscrates/p2p/src/tests/util.rscrates/types/src/block.rscrates/types/src/lib.rscrates/types/src/system_ledger.rscrates/utils/debug-utils/src/db.rs
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-01-05T16:21:34.820Z
Learnt from: antouhou
Repo: Irys-xyz/irys PR: 1057
File: crates/p2p/src/gossip_client.rs:143-170
Timestamp: 2026-01-05T16:21:34.820Z
Learning: In crates/p2p/src/gossip_client.rs, methods like pull_block_body_from_v1_peer intentionally convert all errors (Err(_)) to Ok(GossipResponse::Accepted(None)) because these methods must never return errors by design. This protocol will be replaced with direct socket communication later, so the current approach of treating network failures uniformly as "data not found" is acceptable.
Applied to files:
crates/p2p/src/chain_sync.rscrates/chain/tests/external/block_production.rscrates/actors/src/mempool_service/lifecycle.rscrates/p2p/src/tests/util.rscrates/actors/src/block_discovery.rscrates/p2p/src/tests/block_pool/mod.rscrates/chain/tests/multi_node/fork_recovery.rscrates/p2p/src/gossip_data_handler.rscrates/chain/tests/validation/mod.rscrates/p2p/src/block_pool.rscrates/chain/tests/multi_node/validation.rscrates/chain/tests/utils.rscrates/actors/src/block_producer.rs
📚 Learning: 2026-01-05T16:21:25.918Z
Learnt from: antouhou
Repo: Irys-xyz/irys PR: 1057
File: crates/p2p/src/gossip_client.rs:143-170
Timestamp: 2026-01-05T16:21:25.918Z
Learning: In crates/p2p/src/gossip_client.rs, ensure that methods like pull_block_body_from_v1_peer convert all errors (Err(_)) to Ok(GossipResponse::Accepted(None)) because these methods are designed to never return errors. This supports the current protocol (to be replaced later with direct socket communication). Reviewers should verify that no Err paths are surfaced in these methods and that error conditions are consistently mapped to an empty or 'not found' accepted response. This guidance applies specifically to this file due to its architectural role.
Applied to files:
crates/p2p/src/gossip_client.rs
🧬 Code graph analysis (29)
crates/p2p/src/chain_sync.rs (3)
crates/types/src/block.rs (2)
header(1152-1154)block_hash(267-269)crates/chain/src/chain.rs (1)
clone(291-293)crates/types/src/arbiter_handle.rs (1)
clone(205-209)
crates/chain/tests/external/block_production.rs (1)
crates/database/src/database.rs (14)
tx(76-76)tx(82-82)tx(93-94)tx(99-99)tx(111-111)tx(119-120)tx(129-129)tx(137-138)tx(152-152)tx(157-157)tx(174-174)tx(213-213)tx(223-223)block_header_by_hash(87-107)
crates/chain/tests/validation/unstake_edge_cases.rs (2)
crates/chain/tests/validation/mod.rs (1)
send_block_to_block_tree(35-53)crates/chain/tests/utils.rs (1)
read_block_from_state(3083-3138)
crates/api-server/src/lib.rs (1)
crates/api-server/src/routes/tx.rs (1)
get_tx_status(305-346)
crates/domain/src/snapshots/epoch_snapshot/epoch_replay_data.rs (1)
crates/database/src/database.rs (2)
block_header_by_hash(87-107)commitment_tx_by_txid(133-140)
crates/types/src/system_ledger.rs (1)
crates/types/src/block.rs (8)
iter(875-877)get_id(879-881)eq(859-861)eq(865-867)index(642-646)index(993-995)index_mut(650-654)index_mut(999-1001)
crates/p2p/src/gossip_client.rs (2)
crates/types/src/block.rs (2)
header(1152-1154)body(1156-1158)crates/p2p/src/peer_network_service.rs (2)
peer_list(237-239)peer_list(1179-1181)
crates/actors/src/mempool_service/lifecycle.rs (1)
crates/database/src/database.rs (2)
insert_commitment_tx(125-130)tx_header_by_txid(115-122)
crates/chain/tests/block_production/difficulty_adjustment.rs (2)
crates/types/src/block.rs (2)
height(272-274)body(1156-1158)crates/chain/tests/validation/mod.rs (1)
send_block_to_block_tree(35-53)
crates/chain/tests/validation/invalid_perm_fee_refund.rs (1)
crates/chain/tests/utils.rs (2)
read_block_from_state(3083-3138)assert_validation_error(3072-3081)
crates/chain/tests/block_production/block_rebuilding.rs (2)
crates/p2p/src/gossip_client.rs (1)
block(1146-1154)crates/types/src/block.rs (3)
header(1152-1154)block_hash(267-269)height(272-274)
crates/chain/tests/validation/unpledge_partition.rs (3)
crates/chain/tests/validation/mod.rs (3)
send_block_to_block_tree(35-53)err(625-625)err(1059-1059)crates/chain/tests/utils.rs (1)
read_block_from_state(3083-3138)crates/types/src/block.rs (2)
header(1152-1154)height(272-274)
crates/database/src/db_index.rs (1)
crates/actors/src/mempool_service.rs (3)
tx(1097-1098)set_tx_included_height(2065-2073)clear_tx_included_height(2077-2085)
crates/actors/src/validation_service/active_validations.rs (2)
crates/actors/src/validation_service/block_validation_task.rs (2)
hash(64-66)new(87-99)crates/types/src/block.rs (3)
header(1152-1154)block_hash(267-269)height(272-274)
crates/p2p/src/tests/util.rs (2)
crates/p2p/src/gossip_client.rs (1)
block(1146-1154)crates/types/src/block.rs (1)
header(1152-1154)
crates/actors/src/block_tree_service.rs (2)
crates/types/src/block.rs (3)
block_hash(267-269)header(1152-1154)height(272-274)crates/domain/src/models/block_tree.rs (2)
create_epoch_snapshot_for_block(1369-1390)create_commitment_snapshot_for_block(1343-1367)
crates/actors/src/block_discovery.rs (1)
crates/types/src/block.rs (7)
get_ingress_proofs(660-690)block_hash(267-269)height(272-274)header(1152-1154)transactions(1160-1162)new(88-110)new(1134-1150)
crates/actors/src/validation_service/block_validation_task.rs (1)
crates/types/src/block.rs (3)
header(1152-1154)block_hash(267-269)height(272-274)
crates/p2p/src/gossip_data_handler.rs (1)
crates/types/src/block.rs (1)
body(1156-1158)
crates/chain/tests/validation/mod.rs (1)
crates/types/src/block.rs (3)
header(1152-1154)height(272-274)body(1156-1158)
crates/chain/tests/block_production/block_production.rs (1)
crates/types/src/block.rs (3)
header(1152-1154)block_hash(267-269)height(272-274)
crates/chain/tests/block_production/block_validation.rs (2)
crates/actors/src/block_validation.rs (1)
prevalidate_block(361-687)crates/types/src/block.rs (4)
new(88-110)new(1134-1150)body(1156-1158)header(1152-1154)
crates/chain/tests/validation/data_tx_pricing.rs (3)
crates/chain/tests/utils.rs (1)
read_block_from_state(3083-3138)crates/chain/tests/validation/mod.rs (1)
send_block_to_block_tree(35-53)crates/types/src/block.rs (2)
header(1152-1154)body(1156-1158)
crates/p2p/src/block_pool.rs (1)
crates/types/src/block.rs (5)
block_hash(267-269)header(1152-1154)height(272-274)body(1156-1158)data_txs(624-630)
crates/actors/src/block_validation.rs (2)
crates/database/src/database.rs (3)
block_header_by_hash(87-107)cached_data_root_by_data_root(219-224)tx_header_by_txid(115-122)crates/types/src/block.rs (4)
header(1152-1154)block_hash(267-269)height(272-274)transactions(1160-1162)
crates/actors/src/validation_service.rs (2)
crates/actors/src/validation_service/block_validation_task.rs (1)
hash(64-66)crates/types/src/block.rs (2)
header(1152-1154)height(272-274)
crates/chain/tests/utils.rs (1)
crates/types/src/block.rs (5)
new(88-110)new(1134-1150)header(1152-1154)block_hash(267-269)height(272-274)
crates/chain/src/chain.rs (1)
crates/database/src/system_ledger.rs (2)
add_genesis_commitments(136-154)get_genesis_commitments(59-101)
crates/actors/src/block_producer.rs (1)
crates/types/src/block.rs (6)
header(1152-1154)block_hash(267-269)height(272-274)all_data_txs(1063-1065)new(88-110)new(1134-1150)
🪛 GitHub Actions: Rust Checks
crates/types/src/lib.rs
[error] 36-36: Rust compilation failed: file not found for module transaction_metadata referenced in crates/types/src/lib.rs:36:1 during cargo nextest run.
🪛 GitHub Check: cargo check
crates/types/src/lib.rs
[failure] 36-36:
file not found for module transaction_metadata
[failure] 68-68:
unused import: transaction_metadata::*
🪛 GitHub Check: cargo clippy
crates/types/src/lib.rs
[failure] 36-36:
file not found for module transaction_metadata
[failure] 68-68:
unused import: transaction_metadata::*
🪛 GitHub Check: cargo doc
crates/types/src/lib.rs
[failure] 36-36:
file not found for module transaction_metadata
[failure] 68-68:
unused import: transaction_metadata::*
🪛 GitHub Check: cargo fmt
crates/database/src/db_index.rs
[warning] 137-137:
Diff in /home/runner/actions-runner/_work/irys/irys/crates/database/src/db_index.rs
🪛 GitHub Check: cargo test
crates/types/src/lib.rs
[failure] 36-36:
file not found for module transaction_metadata
[failure] 68-68:
unused import: transaction_metadata::*
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| use irys_types::{ | ||
| BlockBody, DataLedger, DataTransaction, NodeConfig, SealedBlock, UnixTimestamp, H256, U256, | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Imports look fine; consider keeping BlockBody construction inputs unique-by-txid.
a_block2_txs.all_data_txs() can include the same tx multiple times if it exists in multiple ledgers; consider deduping by tx.id before building BlockBody.data_transactions to avoid bloating the body and creating ambiguous “extra tx” scenarios.
🤖 Prompt for AI Agents
In `@crates/chain/tests/multi_node/fork_recovery.rs` around lines 4 - 6, The
BlockBody construction can end up with duplicate DataTransaction entries because
a_block2_txs.all_data_txs() may return the same tx from multiple ledgers; before
assigning to BlockBody.data_transactions (where BlockBody is constructed),
de-duplicate the collection by tx.id (e.g., iterate a_block2_txs.all_data_txs(),
track seen tx.id values in a HashSet and push only the first occurrence) so the
body contains unique transactions and avoids ambiguous "extra tx" scenarios.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/actors/src/mempool_service.rs (3)
2176-2188: Type mismatch intake_all_valid_txsreturn type.The function signature returns
(BTreeMap<H256, DataTransactionHeader>, BTreeMap<IrysAddress, Vec<CommitmentTransaction>>)but the fields are nowDataTxWithMetadataandVec<CommitmentTxWithMetadata>. Thestd::mem::takewill return the wrapped types, not the unwrapped types.This should either:
- Update the return type to use wrapped types, or
- Map the results to unwrap the transactions
🐛 Option 1: Update return type to wrapped types
pub async fn take_all_valid_txs( &self, ) -> ( - BTreeMap<H256, DataTransactionHeader>, - BTreeMap<IrysAddress, Vec<CommitmentTransaction>>, + BTreeMap<H256, DataTxWithMetadata>, + BTreeMap<IrysAddress, Vec<CommitmentTxWithMetadata>>, ) {🐛 Option 2: Unwrap before returning
pub async fn take_all_valid_txs( &self, ) -> ( BTreeMap<H256, DataTransactionHeader>, BTreeMap<IrysAddress, Vec<CommitmentTransaction>>, ) { let mut state = self.write().await; state.recent_valid_tx.clear(); + let data_txs = std::mem::take(&mut state.valid_submit_ledger_tx) + .into_iter() + .map(|(k, wrapped)| (k, wrapped.tx)) + .collect(); + let commitment_txs = std::mem::take(&mut state.valid_commitment_tx) + .into_iter() + .map(|(k, wrapped_vec)| (k, wrapped_vec.into_iter().map(|w| w.tx).collect())) + .collect(); ( - std::mem::take(&mut state.valid_submit_ledger_tx), - std::mem::take(&mut state.valid_commitment_tx), + data_txs, + commitment_txs, ) }
2198-2205: Type mismatch inupdate_submit_transaction.The function takes
DataTransactionHeaderbut the map storesDataTxWithMetadata. The assignment*t = txwould fail to compile sincetis&mut DataTxWithMetadataandtxisDataTransactionHeader.🐛 Proposed fix
-pub async fn update_submit_transaction(&self, tx: DataTransactionHeader) { +pub async fn update_submit_transaction(&self, tx: DataTransactionHeader) { self.0 .write() .await .valid_submit_ledger_tx .entry(tx.id) - .and_modify(|t| *t = tx); + .and_modify(|wrapped| wrapped.tx = tx); }
2207-2236: MissingDerefMutimplementation for mutable field access.The
clear_promoted_heightandset_promoted_heightmethods attempt to mutateheader.promoted_heightthrough theDerefimplementation. However, onlyDerefis implemented forDataTxWithMetadata, notDerefMut, so mutable field access won't work.🐛 Option 1: Access through `.tx` field directly
pub async fn clear_promoted_height(&self, txid: H256) -> bool { let mut cleared = false; let mut state = self.write().await; if let Some(header) = state.valid_submit_ledger_tx.get_mut(&txid) { - header.promoted_height = None; + header.tx.promoted_height = None; state.recent_valid_tx.put(txid, ()); tracing::debug!(tx.id = %txid, "Cleared promoted_height in mempool"); cleared = true; } cleared } pub async fn set_promoted_height( &self, txid: H256, height: u64, ) -> Option<DataTransactionHeader> { let mut state = self.write().await; let header = state.valid_submit_ledger_tx.get_mut(&txid)?; - if header.promoted_height.is_none() { - header.promoted_height = Some(height); + if header.tx.promoted_height.is_none() { + header.tx.promoted_height = Some(height); } - let result = header.clone(); + let result = header.tx.clone(); tracing::debug!(tx.id = %txid, promoted_height = height, "Set promoted_height in mempool"); state.recent_valid_tx.put(txid, ()); Some(result) }🐛 Option 2: Add `DerefMut` to `DataTxWithMetadata` in transaction_metadata.rs
impl std::ops::DerefMut for DataTxWithMetadata { fn deref_mut(&mut self) -> &mut Self::Target { &mut self.tx } }
🤖 Fix all issues with AI agents
In `@crates/actors/src/mempool_guard.rs`:
- Around line 71-76: The call to the private AtomicMempoolState::read() inside
MempoolReadGuard::is_recent_valid_tx_blocking is failing; instead expose the
needed check via a public API on AtomicMempoolState (e.g., add a public
contains_recent_valid_tx(&self, tx_id: &IrysTransactionId) -> bool) and have
is_recent_valid_tx_blocking call that public method through the existing
mempool_state handle (invoke it using the runtime/blocking executor as you
currently do, but without calling read()); do not make read() public—add the new
contains_recent_valid_tx method to AtomicMempoolState and replace the read()
usage in is_recent_valid_tx_blocking with a call to that new public method.
In `@crates/actors/src/mempool_service.rs`:
- Line 2093: Run rustfmt to fix the formatting errors flagged by cargo fmt for
this file and package: execute `cargo fmt --package irys-actors` and stage the
resulting changes; ensure mempool_service.rs (and the other affected locations
around lines ~2093, ~2532, ~2540) are reformatted and committed so CI no longer
fails. If you have a rustfmt.toml, confirm it’s applied and re-run `cargo fmt`
before pushing.
In `@crates/actors/src/transaction_status.rs`:
- Around line 29-31: The code is swallowing database errors by calling
.ok().flatten() on get_tx_metadata(db_tx, tx_id); replace this with proper error
propagation (e.g., use the ? operator on get_tx_metadata(db_tx, tx_id) and then
flatten/transpose as needed) so actual DB errors are returned instead of treated
as "not found"; update the enclosing function's return type to
Result<Option<...>, E> (and adjust callers) so get_tx_metadata errors propagate
up rather than being discarded.
In `@crates/api-server/src/routes/tx.rs`:
- Around line 316-324: The current code maps a missing block tree head to
ApiError::Internal; instead return a transient/non-fatal error like
ApiError::ServiceUnavailable to indicate the node is not ready. Update the
expression that obtains current_head_height (the call chain
state.block_tree.read().await.head()...ok_or_else(...)? ) so the closure
constructs ApiError::ServiceUnavailable with a clear message (and optional
context) instead of ApiError::Internal; ensure the function's error type stays
compatible and adjust any callers if they rely on the Internal variant.
♻️ Duplicate comments (2)
crates/database/src/db_index.rs (1)
29-48: Consider the upsert semantics ofunwrap_or_default().The
set_tx_included_heightandclear_tx_included_heightfunctions useunwrap_or_default()which silently creates new metadata when none exists. While this provides upsert semantics, it may mask scenarios where the transaction should already exist (e.g., during re-orgs where you expect the metadata to be present).This was flagged in a previous review - consider whether callers need visibility into whether metadata was created vs updated.
crates/api-server/src/routes/tx.rs (1)
329-337:read_blocking()does not exist onBlockIndexReadGuard—useread()instead.As flagged in previous review,
BlockIndexReadGuarddoes not have aread_blocking()method. Other routes in this codebase usestate.block_index.read(). However, since this is inside a synchronousview_eyreclosure, you'll need to restructure to acquire the block index read guard before entering the closure.🛠️ Suggested fix
+ // Acquire block index guard before entering sync closure + let block_index_guard = state.block_index.read(); + // Compute status let status = state .db .view_eyre(|db_tx| { irys_actors::compute_transaction_status( db_tx, &tx_id, - &state.block_index.read_blocking(), + &block_index_guard, current_head_height, &state.mempool_guard, ) })
📜 Review details
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
crates/actors/src/lib.rscrates/actors/src/mempool_guard.rscrates/actors/src/mempool_service.rscrates/actors/src/transaction_status.rscrates/api-server/src/lib.rscrates/api-server/src/routes/tx.rscrates/database/src/db_index.rscrates/database/src/tables.rscrates/types/src/lib.rscrates/types/src/transaction_metadata.rs
🧰 Additional context used
🧬 Code graph analysis (8)
crates/api-server/src/lib.rs (1)
crates/api-server/src/routes/tx.rs (1)
get_tx_status(308-349)
crates/api-server/src/routes/tx.rs (1)
crates/actors/src/transaction_status.rs (1)
compute_transaction_status(12-61)
crates/actors/src/mempool_guard.rs (1)
crates/actors/src/mempool_service.rs (2)
state(2037-2041)state(2046-2050)
crates/actors/src/transaction_status.rs (2)
crates/database/src/db_index.rs (1)
get_tx_metadata(12-18)crates/types/src/transaction_metadata.rs (3)
confirmed(199-205)included(191-197)pending(183-189)
crates/actors/src/lib.rs (1)
crates/actors/src/transaction_status.rs (1)
compute_transaction_status(12-61)
crates/database/src/db_index.rs (2)
crates/actors/src/mempool_service.rs (3)
tx(1104-1105)set_tx_included_height(2072-2080)clear_tx_included_height(2084-2092)crates/database/src/database.rs (16)
tx(76-76)tx(82-82)tx(93-94)tx(99-99)tx(111-111)tx(119-120)tx(129-129)tx(137-138)tx(152-152)tx(157-157)tx(174-174)tx(213-213)tx(223-223)tx(234-234)tx(263-263)open_or_create_db(37-56)
crates/types/src/transaction_metadata.rs (1)
crates/types/src/block.rs (1)
height(272-274)
crates/actors/src/mempool_service.rs (4)
crates/database/src/db_index.rs (2)
set_tx_included_height(30-38)clear_tx_included_height(41-48)crates/types/src/commitment_v1.rs (1)
id(209-211)crates/types/src/commitment_v2.rs (1)
id(207-209)crates/types/src/irys.rs (1)
address(36-38)
🪛 GitHub Check: cargo check
crates/database/src/db_index.rs
[failure] 135-135:
no method named view found for struct reth_db::DatabaseEnv in the current scope
[failure] 129-129:
no method named update found for struct reth_db::DatabaseEnv in the current scope
[failure] 124-124:
no method named view found for struct reth_db::DatabaseEnv in the current scope
[failure] 118-118:
no method named update found for struct reth_db::DatabaseEnv in the current scope
[failure] 106-106:
no method named view found for struct reth_db::DatabaseEnv in the current scope
[failure] 101-101:
no method named update found for struct reth_db::DatabaseEnv in the current scope
[failure] 97-97:
no method named view found for struct reth_db::DatabaseEnv in the current scope
[failure] 92-92:
no method named update found for struct reth_db::DatabaseEnv in the current scope
[failure] 88-88:
no method named view found for struct reth_db::DatabaseEnv in the current scope
🪛 GitHub Check: cargo clippy
crates/database/src/db_index.rs
[failure] 135-135:
no method named view found for struct reth_db::DatabaseEnv in the current scope
[failure] 129-129:
no method named update found for struct reth_db::DatabaseEnv in the current scope
[failure] 124-124:
no method named view found for struct reth_db::DatabaseEnv in the current scope
[failure] 118-118:
no method named update found for struct reth_db::DatabaseEnv in the current scope
[failure] 106-106:
no method named view found for struct reth_db::DatabaseEnv in the current scope
[failure] 101-101:
no method named update found for struct reth_db::DatabaseEnv in the current scope
[failure] 97-97:
no method named view found for struct reth_db::DatabaseEnv in the current scope
[failure] 92-92:
no method named update found for struct reth_db::DatabaseEnv in the current scope
[failure] 88-88:
no method named view found for struct reth_db::DatabaseEnv in the current scope
🪛 GitHub Check: cargo doc
crates/actors/src/mempool_guard.rs
[failure] 73-73:
method read is private
🪛 GitHub Check: cargo fmt
crates/actors/src/mempool_guard.rs
[warning] 92-92:
Diff in /home/runner/actions-runner/_work/irys/irys/crates/actors/src/mempool_guard.rs
[warning] 87-87:
Diff in /home/runner/actions-runner/_work/irys/irys/crates/actors/src/mempool_guard.rs
[warning] 83-83:
Diff in /home/runner/actions-runner/_work/irys/irys/crates/actors/src/mempool_guard.rs
[warning] 76-76:
Diff in /home/runner/actions-runner/_work/irys/irys/crates/actors/src/mempool_guard.rs
crates/actors/src/transaction_status.rs
[warning] 18-18:
Diff in /home/runner/actions-runner/_work/irys/irys/crates/actors/src/transaction_status.rs
[warning] 6-6:
Diff in /home/runner/actions-runner/_work/irys/irys/crates/actors/src/transaction_status.rs
crates/actors/src/mempool_service.rs
[warning] 2093-2093:
Diff in /home/runner/actions-runner/_work/irys/irys/crates/actors/src/mempool_service.rs
[warning] 2540-2540:
Diff in /home/runner/actions-runner/_work/irys/irys/crates/actors/src/mempool_service.rs
[warning] 2532-2532:
Diff in /home/runner/actions-runner/_work/irys/irys/crates/actors/src/mempool_service.rs
🪛 GitHub Check: cargo test
crates/database/src/db_index.rs
[failure] 135-135:
no method named view found for struct reth_db::DatabaseEnv in the current scope
[failure] 129-129:
no method named update found for struct reth_db::DatabaseEnv in the current scope
[failure] 124-124:
no method named view found for struct reth_db::DatabaseEnv in the current scope
[failure] 118-118:
no method named update found for struct reth_db::DatabaseEnv in the current scope
[failure] 106-106:
no method named view found for struct reth_db::DatabaseEnv in the current scope
[failure] 101-101:
no method named update found for struct reth_db::DatabaseEnv in the current scope
[failure] 97-97:
no method named view found for struct reth_db::DatabaseEnv in the current scope
[failure] 92-92:
no method named update found for struct reth_db::DatabaseEnv in the current scope
[failure] 88-88:
no method named view found for struct reth_db::DatabaseEnv in the current scope
🔇 Additional comments (18)
crates/types/src/transaction_metadata.rs (5)
1-31: LGTM! Clean metadata type design.The
TransactionMetadatastruct with its lifecycle helpers is well-designed. The separation from transaction headers enables easier future migrations as documented. TheDefaultderive and explicitnew()constructor provide flexibility.
33-93: LGTM! Well-structured wrapper with ergonomic API.The
DataTxWithMetadatawrapper provides a clean interface for managing transaction metadata lifecycle. TheDerefimplementation enables transparent field access whilesplit()andas_parts()offer flexibility for storage operations.
95-154: LGTM! Consistent wrapper implementation.The
CommitmentTxWithMetadatawrapper mirrorsDataTxWithMetadataappropriately, adapting to the underlyingCommitmentTransactionAPI (method-basedid()vs field access).
156-206: LGTM! Clear status API types.The status enum and response struct are well-designed for the API. Using
saturating_subfor confirmations calculation is a safe choice that handles edge cases gracefully.
208-239: LGTM! Adequate test coverage for new types.Tests verify the core functionality of metadata creation and status response construction.
crates/types/src/lib.rs (1)
36-36: LGTM! Standard module export pattern.The new
transaction_metadatamodule is properly declared and re-exported, following the existing conventions in this file.Also applies to: 67-67
crates/database/src/tables.rs (3)
11-11: LGTM! Proper wrapper registration.The
TransactionMetadatawrapper follows the established pattern used for other types likeIrysBlockHeaderandDataTransactionHeader.Also applies to: 81-81
98-99: LGTM! Compression support added.
CompactTransactionMetadatais properly included in the compression macro.
132-137: LGTM! Clean table definition.The
IrysTransactionMetadatatable is well-documented and follows the existing table patterns. UsingH256as the key allows storing metadata for both data and commitment transactions in a unified table.crates/database/src/db_index.rs (2)
6-27: LGTM! Clean helper function implementations.The
get_tx_metadataandput_tx_metadatafunctions provide a clean abstraction over the database table operations.
73-139: The test code is correct.DatabaseEnvfrom thereth_dbcrate implements theDatabasetrait which providesview()andupdate()methods. No compilation errors will occur.crates/actors/src/mempool_service.rs (4)
50-52: LGTM! Proper imports for wrapped types.The new metadata wrapper types are imported alongside existing transaction types.
2070-2122: LGTM! Well-designed lifecycle methods.The included_height management methods provide a clean API for re-org handling. The commitment tx methods' O(n) iteration matches existing patterns in the codebase where commitments are stored by address.
2461-2468: LGTM! Clean migration to wrapped types.The state fields now use wrapped types with updated documentation explaining the metadata lifecycle benefits.
2538-2548: Good metadata preservation during updates.The logic to preserve existing metadata when re-inserting a transaction is important for maintaining state across updates. This prevents losing
included_heightinformation when a transaction is updated for other reasons (e.g.,promoted_heightflag).crates/actors/src/transaction_status.rs (1)
33-60: Edge case: metadata exists withoutincluded_heightand not in mempool returnsNone.If the database has transaction metadata without an
included_height(e.g., from a partial write or old state) and the transaction is no longer in mempool, the function returnsNone(not found). This might be unexpected since the transaction technically exists in the system.Consider whether this edge case should return
Pendingstatus or ifNoneis the correct behavior for this scenario.crates/actors/src/lib.rs (1)
21-21: LGTM!The new
transaction_statusmodule is correctly registered and itscompute_transaction_statusfunction is properly re-exported for use by downstream crates.Also applies to: 33-33
crates/api-server/src/lib.rs (1)
119-122: LGTM!The new
/tx/{tx_id}/statusroute is correctly wired and follows the existing routing patterns for transaction endpoints.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/actors/src/mempool_service/lifecycle.rs (1)
663-708: Reorg reconciliation updatespromoted_heightfor “published in both”, but notincluded_height.If a tx from the old fork is re-included at a different height in the new fork,
included_heightcan remain stale (since it’s not in the orphaned set, so it won’t get cleared either).🔧 Suggested fix: update included_height alongside promoted_height for canonical reconciliation
diff --git a/crates/actors/src/mempool_service/lifecycle.rs b/crates/actors/src/mempool_service/lifecycle.rs @@ - // Ensure the metadata table reflects the canonical promoted heights for txs published in both forks. + // Ensure the metadata table reflects canonical heights for txs published in both forks. if !promoted_height_updates.is_empty() { if let Err(e) = self.irys_db.update_eyre(|db_tx| { for (tx_id, height) in &promoted_height_updates { + irys_database::set_tx_included_height(db_tx, tx_id, *height) + .map_err(|e| eyre::eyre!("{:?}", e))?; irys_database::set_tx_promoted_height(db_tx, tx_id, *height) .map_err(|e| eyre::eyre!("{:?}", e))?; } Ok(()) }) {crates/database/src/db_index.rs (1)
1-28: Drop the_Xplaceholder constant (or replace with a module-level doc comment).
It reads like leftover scaffolding and doesn’t add signal.crates/types/src/transaction.rs (1)
256-268: Verify or enforce thatmetadatais never included in Compact serialization.The doc comment states this field is "NEVER serialized…not in…Compact", but the
#[rlp(skip)]and#[serde(skip)]attributes don't apply to Compact. TheCompactderive from reth-codecs does not support askipfield attribute, sometadatawill be included in the Compact encoding unless manually implemented. Either the implementation needs a customCompacttrait implementation to skip the field, or the comment is inaccurate.Add a regression test to confirm the contract (the suggested test in the original comment is valid).
🤖 Fix all issues with AI agents
In `@crates/actors/src/mempool_service.rs`:
- Around line 50-53: The clear_* helpers claim to return "found and updated" but
currently return true even when metadata was None; update the clear_*
implementations (the functions named clear_* referenced in this file) so they
only return true when an existing metadata value was Some and actually
cleared/changed (otherwise return false), and update their docstrings to match
semantics if you prefer the alternate behavior. Also extend get_tx_metadata() to
search pending_pledges in addition to valid_commitment_tx (use the same
lookup/borrow logic and return the metadata when present) so transactions that
are pending pledges are covered; ensure you reference and handle
TransactionMetadata, valid_commitment_tx, and pending_pledges consistently and
update unit tests or add coverage for both "found-and-updated" and
"found-but-none" cases.
In `@crates/actors/src/mempool_service/lifecycle.rs`:
- Around line 709-726: The code collects all_orphaned_tx_ids and unconditionally
calls self.irys_db.update_eyre to clear included_height; change the collection
to dedupe IDs (use a HashSet/into_iter().collect::<HashSet<_>>() then to Vec)
and only call self.irys_db.update_eyre when the deduped Vec is non-empty to
avoid unnecessary write transactions; apply the same
dedupe-and-skip-empty-update change to the other similar blocks that call
irys_database::batch_clear_tx_included_height / self.irys_db.update_eyre (the
same pattern around the other occurrences referenced: the nearby blocks handling
orphaned tx ids at the locations noted).
In `@crates/chain/tests/api/client.rs`:
- Around line 217-283: The test uses a hardcoded "55" loop count to reach
CONFIRMED; replace this magic number with the node's migration depth from the
test context (use the config already available via ctx, e.g.
ctx.node_ctx.config.node_config.migration_depth or
ctx.node_ctx.config.migration_depth) so the for-loop iterates based on that
config constant (and adjust +/-1 only if the confirmation semantics require an
offset).
In `@crates/database/src/db_index.rs`:
- Around line 71-184: Add tests mirroring the included_height ones that exercise
promoted_height APIs: write a unit test that uses set_tx_promoted_height and
clear_tx_promoted_height for a single tx_id and asserts
get_tx_metadata(...).promoted_height changes accordingly, and another test that
uses batch_set_tx_promoted_height and batch_clear_tx_promoted_height on several
tx_ids and verifies each metadata.promoted_height is set/cleared; reuse the test
pattern and helpers already in the file (db.view/db.update, H256::random) and
reference the functions set_tx_promoted_height, clear_tx_promoted_height,
batch_set_tx_promoted_height, batch_clear_tx_promoted_height, and
get_tx_metadata.
In `@crates/types/src/commitment_v1.rs`:
- Around line 65-72: The metadata field on CommitmentTransactionV1 is missing
the Compact derive skip attribute; add #[compact(skip)] above the pub metadata:
Option<crate::TransactionMetadata> declaration (in addition to the existing
#[rlp(skip)], #[rlp(default)], #[serde(skip)]) so the Compact encoder will
ignore this field as the docs require.
In `@crates/types/src/commitment_v2.rs`:
- Around line 63-70: The metadata field (pub metadata:
Option<crate::TransactionMetadata>) is being included in Compact encoding due to
Option<T> bitflag behavior; add the attribute #[codec(skip)] to the metadata
field declaration to prevent Compact serialization and ensure it matches the
comment, and apply the same fix to the other occurrences of the metadata field
(the same Option<crate::TransactionMetadata> declaration in the transaction and
commitment_v1 types) or, if you prefer the opposite approach, update the comment
to state that Compact encodes the Option as a bitflag instead of skipping it.
In `@crates/types/src/transaction_metadata.rs`:
- Around line 145-183: Add unit tests covering Compact encoding/decoding and the
saturating confirmations edge case: (1) a roundtrip test that constructs a
TransactionMetadata with both included_height and promoted_height, calls
TransactionMetadata::to_compact (or Compact::to_compact via its impl) into a
Vec, then calls TransactionMetadata::from_compact and asserts equality and no
remaining bytes; (2) a backward-compatibility test that writes only an
included_height using Compact::to_compact(&Some(100u64), &mut buf) and then
calls TransactionMetadata::from_compact to assert decoded.included_height ==
Some(100) and decoded.promoted_height == None; (3) an confirmations saturating
test that constructs TransactionStatusResponse::included(110, 100) and asserts
response.confirmations == Some(0). Use the existing test module and functions
TransactionMetadata::from_compact/to_compact, Compact::to_compact, and
TransactionStatusResponse::included to locate implementation.
- Around line 69-74: with_promoted_height currently allows promoted_height to be
set while included_height stays None, which contradicts the doc comment that a
transaction must be included before promoted; change
with_promoted_height(height: u64) so it also sets included_height: Some(height)
(i.e., set both included_height and promoted_height to Some(height)) and update
the doc comment to state that this constructor is for the common case where
inclusion and promotion occur at the same height; if you intentionally need a
promotion-only path instead, add a clear doc comment explaining that exceptional
migration use-case or implement an alternative API (e.g., a require-both
constructor like with_included_and_promoted(included: u64, promoted: u64) or a
set_promoted_height(&mut self, height: u64) method) and remove/mark the
inconsistent variant accordingly.
♻️ Duplicate comments (1)
crates/database/src/db_index.rs (1)
29-69: Avoid creating metadata rows onclear_*when none exists.
unwrap_or_default()makesclear_*an upsert, which can turn “no row” into “row with all None”. That changesget_tx_metadata()fromNonetoSome(default)and can complicate status logic.Proposed change (no-op clear when missing)
diff --git a/crates/database/src/db_index.rs b/crates/database/src/db_index.rs @@ pub fn clear_tx_included_height( tx: &(impl DbTxMut + DbTx), tx_id: &H256, ) -> Result<(), reth_db::DatabaseError> { - let mut metadata = get_tx_metadata(tx, tx_id)?.unwrap_or_default(); - metadata.included_height = None; - put_tx_metadata(tx, tx_id, metadata) + let Some(mut metadata) = get_tx_metadata(tx, tx_id)? else { + return Ok(()); + }; + metadata.included_height = None; + put_tx_metadata(tx, tx_id, metadata) } @@ pub fn clear_tx_promoted_height( tx: &(impl DbTxMut + DbTx), tx_id: &H256, ) -> Result<(), reth_db::DatabaseError> { - let mut metadata = get_tx_metadata(tx, tx_id)?.unwrap_or_default(); - metadata.promoted_height = None; - put_tx_metadata(tx, tx_id, metadata) + let Some(mut metadata) = get_tx_metadata(tx, tx_id)? else { + return Ok(()); + }; + metadata.promoted_height = None; + put_tx_metadata(tx, tx_id, metadata) }
📜 Review details
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (21)
crates/actors/src/mempool_guard.rscrates/actors/src/mempool_service.rscrates/actors/src/mempool_service/lifecycle.rscrates/actors/src/shadow_tx_generator.rscrates/actors/src/transaction_status.rscrates/api-client/src/lib.rscrates/api-server/src/lib.rscrates/api-server/src/routes/tx.rscrates/chain/tests/api/client.rscrates/chain/tests/validation/invalid_perm_fee_refund.rscrates/chain/tests/validation/mod.rscrates/database/src/db_index.rscrates/database/src/lib.rscrates/database/src/tables.rscrates/domain/src/snapshots/commitment_snapshot.rscrates/types/src/commitment_common.rscrates/types/src/commitment_v1.rscrates/types/src/commitment_v2.rscrates/types/src/signature.rscrates/types/src/transaction.rscrates/types/src/transaction_metadata.rs
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-01-13T12:16:14.104Z
Learnt from: JesseTheRobot
Repo: Irys-xyz/irys PR: 1095
File: crates/types/src/transaction.rs:920-921
Timestamp: 2026-01-13T12:16:14.104Z
Learning: CommitmentTransactionV1 has never been RLP encoded/decoded and is now deprecated with a frozen implementation, so RLP round-trip tests for V1 are not necessary.
Applied to files:
crates/types/src/commitment_v2.rscrates/types/src/commitment_v1.rscrates/chain/tests/api/client.rscrates/types/src/transaction.rs
📚 Learning: 2026-01-05T16:21:34.820Z
Learnt from: antouhou
Repo: Irys-xyz/irys PR: 1057
File: crates/p2p/src/gossip_client.rs:143-170
Timestamp: 2026-01-05T16:21:34.820Z
Learning: In crates/p2p/src/gossip_client.rs, methods like pull_block_body_from_v1_peer intentionally convert all errors (Err(_)) to Ok(GossipResponse::Accepted(None)) because these methods must never return errors by design. This protocol will be replaced with direct socket communication later, so the current approach of treating network failures uniformly as "data not found" is acceptable.
Applied to files:
crates/actors/src/transaction_status.rscrates/api-client/src/lib.rscrates/api-server/src/routes/tx.rs
🧬 Code graph analysis (11)
crates/database/src/lib.rs (1)
crates/database/src/db_index.rs (9)
batch_clear_tx_included_height(96-104)batch_clear_tx_promoted_height(107-115)batch_set_tx_included_height(72-81)batch_set_tx_promoted_height(84-93)clear_tx_included_height(52-59)clear_tx_promoted_height(62-69)get_tx_metadata(12-18)set_tx_included_height(30-38)set_tx_promoted_height(41-49)
crates/actors/src/mempool_service/lifecycle.rs (1)
crates/database/src/db_index.rs (5)
batch_set_tx_included_height(72-81)batch_set_tx_promoted_height(84-93)batch_clear_tx_promoted_height(107-115)set_tx_promoted_height(41-49)batch_clear_tx_included_height(96-104)
crates/actors/src/mempool_guard.rs (1)
crates/actors/src/mempool_service.rs (2)
is_recent_valid_tx(2197-2199)get_tx_metadata(2202-2218)
crates/actors/src/transaction_status.rs (3)
crates/types/src/commitment_common.rs (1)
metadata(266-271)crates/p2p/src/block_status_provider.rs (1)
block_index(197-199)crates/types/src/transaction_metadata.rs (3)
confirmed(136-142)included(128-134)pending(120-126)
crates/api-server/src/lib.rs (1)
crates/api-server/src/routes/tx.rs (1)
get_tx_status(318-368)
crates/api-client/src/lib.rs (1)
crates/p2p/src/tests/util.rs (1)
format(460-461)
crates/types/src/transaction_metadata.rs (5)
crates/types/src/commitment_v1.rs (2)
to_compact(453-480)from_compact(482-552)crates/types/src/commitment_v2.rs (2)
to_compact(485-512)from_compact(514-584)crates/types/src/signature.rs (2)
to_compact(113-125)from_compact(128-135)crates/types/src/transaction.rs (2)
to_compact(119-126)from_compact(127-136)crates/types/src/block.rs (1)
height(272-274)
crates/chain/tests/api/client.rs (2)
crates/api-client/src/lib.rs (1)
new(106-114)crates/types/src/commitment_v1.rs (3)
anchor(229-231)signer(221-223)new_pledge(145-163)
crates/api-server/src/routes/tx.rs (3)
crates/actors/src/mempool_service.rs (3)
state(2028-2032)state(2037-2041)get_tx_metadata(2202-2218)crates/database/src/db_index.rs (1)
get_tx_metadata(12-18)crates/actors/src/transaction_status.rs (1)
compute_transaction_status(7-52)
crates/database/src/db_index.rs (3)
crates/actors/src/mempool_service.rs (4)
get_tx_metadata(2202-2218)tx(1105-1106)set_tx_included_height(2063-2078)clear_tx_included_height(2082-2092)crates/actors/src/mempool_guard.rs (1)
get_tx_metadata(76-81)crates/types/src/commitment_common.rs (1)
metadata(266-271)
crates/actors/src/mempool_service.rs (2)
crates/types/src/commitment_common.rs (2)
metadata(266-271)metadata_mut(275-280)crates/types/src/transaction.rs (3)
new(172-174)new(348-365)new(724-728)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: cargo test
- GitHub Check: cargo doc
- GitHub Check: cargo clippy
- GitHub Check: cargo check
🔇 Additional comments (36)
crates/chain/tests/validation/mod.rs (1)
927-927: LGTM!The addition of
metadata: Nonecorrectly initializes the new optional metadata field for this test fixture, aligning with theTransactionMetadatafeature introduced in this PR.crates/types/src/signature.rs (2)
246-247: LGTM!The
metadata: Noneaddition correctly accommodates the new optional field for this signature serialization test. Since metadata is marked as non-serialized (rlp/serde skip per the AI summary), it won't affect the signature parity verification with the JS client.
332-333: LGTM!The
metadata: Noneaddition correctly initializes the new optional metadata field for this commitment transaction signature test.crates/api-server/src/lib.rs (1)
119-119: LGTM!The new
/tx/{tx_id}/statusroute is well-placed among related transaction endpoints and follows the existing routing pattern. The handler implementation (per the relevant code snippet) properly queries the block tree for current height, retrieves metadata from the database, and computes the transaction status with appropriate error handling.crates/domain/src/snapshots/commitment_snapshot.rs (1)
402-403: LGTM!The
metadata: Noneaddition in the test helper correctly initializes the new optional field forCommitmentTransactionV2. This change is propagated consistently to all tests using this helper.crates/chain/tests/validation/invalid_perm_fee_refund.rs (1)
94-95: LGTM!The
metadata: Noneaddition correctly initializes the new optional metadata field for this test'sDataTransactionHeaderV1fixture. The test's validation logic for perm fee refunds remains unaffected.crates/types/src/commitment_v2.rs (1)
91-105: LGTM!The
new()constructor correctly initializes the metadata field toNone, which is appropriate for this internal, non-serialized field.crates/types/src/commitment_v1.rs (1)
93-107: LGTM!The
new()constructor correctly initializes the metadata field toNone.crates/actors/src/shadow_tx_generator.rs (1)
764-782: LGTM!The test helper function correctly initializes the new
metadatafield toNone, aligning with the struct changes inCommitmentTransactionV2.crates/database/src/lib.rs (1)
28-32: LGTM!The re-exports provide a clean, symmetric API surface for transaction metadata management with appropriate single-item and batch variants for both
included_heightandpromoted_heightoperations.crates/chain/tests/api/client.rs (1)
285-360: LGTM!Well-designed test for commitment transaction status. Good choices:
- Using pledge instead of stake to avoid flakiness (well-documented in comments)
- Polling loop to handle timing variability in commitment tx inclusion
- Accepting either INCLUDED or CONFIRMED as valid final states
crates/actors/src/mempool_guard.rs (1)
69-81: LGTM! Clean delegation to public mempool APIs.The new accessor methods correctly delegate to the public async methods on
AtomicMempoolState(is_recent_valid_txandget_tx_metadata), avoiding the privateread()method that was flagged in earlier reviews. This follows the established pattern in the file for exposing mempool state through the guard.crates/api-server/src/routes/tx.rs (2)
299-313: LGTM! Improved data source priority for promotion status.The change correctly prefers DB metadata as the source of truth while falling back to the mempool header for older state. The chaining with
and_thenandoris idiomatic and handles the Option types correctly.
316-368: LGTM! Well-structured status endpoint implementation.The handler correctly:
- Obtains the current head height from the block tree
- Loads DB metadata for the transaction
- Delegates status computation to the dedicated function
- Returns appropriate error responses
The previous review's concern about
read_blocking()has been addressed—this code correctly usesblock_tree.read().One consideration: returning
ApiError::Internalwhen the block tree tip is unavailable (lines 331-333) treats this as a permanent error. During node startup, this could be a transient condition. Consider usingServiceUnavailablefor this specific case to signal clients they can retry.crates/database/src/tables.rs (4)
11-13: LGTM!Import addition is correct and follows the existing import grouping pattern.
83-83: LGTM!The wrapper struct follows the established pattern for other types like
IrysBlockHeader,DataTransactionHeader, andCommitmentTransaction.
100-102: LGTM!Adding compression support for the new wrapper type follows the existing pattern.
134-139: LGTM! Well-documented table definition.The new
IrysTransactionMetadatatable appropriately:
- Uses
H256(transaction ID) as the key- Stores compacted metadata as the value
- Includes clear documentation about its purpose
This follows the same pattern as other transaction-related tables in this file.
crates/actors/src/transaction_status.rs (1)
1-52: Well-designed status computation with clear state machine logic.The function correctly implements the transaction lifecycle states:
- Confirmed: Has
included_heightAND block exists in block index- Included: Has
included_heightbut block not yet in index- Pending: In mempool but no
included_height- None: Not found anywhere
The past review concern about swallowing DB errors has been addressed by having the caller handle DB errors before passing
db_metadataas anOption.One observation: the function returns
eyre::Result<Option<...>>but never actually returnsErr. If this is intentional (errors are handled elsewhere), consider simplifying to returnOption<TransactionStatusResponse>directly, or document that theResultwrapper is for future extensibility.crates/api-client/src/lib.rs (3)
82-92: LGTM! Clean API trait extension.The new
get_transaction_statusmethod follows the established pattern of other trait methods, and the re-exports make the response types conveniently available to consumers of this crate.
306-322: LGTM! Consistent implementation pattern.The implementation correctly:
- Constructs the path as
/tx/{tx_id}/status- Uses GET method
- Requires a response body (unlike endpoints that can return
Nonefor not-found)This matches the server-side behavior where
get_tx_statusreturnsApiError::ErrNoId(which maps to a non-200 status) rather than an empty body when the transaction isn't found.
404-411: LGTM! Mock implementations are appropriate.Both mock clients return errors for the status endpoint, which is reasonable default behavior. Tests that need specific status responses can extend these mocks or use the real client against a test server.
Also applies to: 507-514
crates/types/src/transaction.rs (4)
347-364: Good defaulting ofmetadatatoNoneinDataTransactionHeaderV1::new.
Keeps constructors deterministic and avoids “accidental metadata” leaking into runtime objects.
815-831: Test scaffolding updated consistently for the new field.
1107-1124: Mock header updated to includemetadata: None— consistent with the “internal-only” intent.
1304-1320: Commitment test helper now initializesmetadata: None— good to keep fixtures compiling across versions.crates/types/src/commitment_common.rs (1)
1-13: Nice ergonomic API: metadata access is version-agnostic via the wrapper.
Themetadata()/metadata_mut()/set_metadata()trio makes downstream code much cleaner across V1/V2.Also applies to: 264-289
crates/actors/src/mempool_service/lifecycle.rs (2)
618-642: Good: reorg now clearspromoted_heightin both mempool and DB metadata table.
This keeps the status surface aligned with canonical chain after orphaning.
19-55: Remove this review comment. The status endpoint (/v1/tx/{txId}/status) only usesincluded_heightto compute transaction status;promoted_heightis not consulted. The different error handling semantics (fatal forincluded_height, best-effort forpromoted_height) are appropriate:included_heightis required for the main status endpoint, whilepromoted_heightis supplementary info for the separate/promotion-statusendpoint.Likely an incorrect or invalid review comment.
crates/actors/src/mempool_service.rs (3)
2554-2565: Good: preserve existingmetadatawhen updating an existing data tx.
This prevents accidental loss of internal state during header refresh/update flows.
2621-2629: Nice cleanup: use a localtx_idfor consistent duplicate checks/logging.Also applies to: 2677-2683
3181-3214: Tests updated for the newmetadatafield; fixtures remain explicit and deterministic.crates/types/src/transaction_metadata.rs (4)
1-19: LGTM!The struct design is clean with appropriate derives. The documentation clearly explains the storage semantics and ledger-specific meaning of each height field.
21-52: LGTM - backward-compatibility approach is sound.The manual
Compactimplementation correctly handles older DB encodings that only containedincluded_height. The empty-buffer check at line 38 gracefully defaultspromoted_heighttoNonefor legacy data.
85-95: LGTM!The enum cleanly models the transaction lifecycle. Documentation clearly explains each state, and
SCREAMING_SNAKE_CASEis appropriate for API serialization.
97-143: LGTM - good defensive design.The response structure is well-designed for API consumption:
camelCaseserialization is appropriate for JSON APIsoptional_string_u64serialization prevents JavaScript precision issues with large u64 valuessaturating_subon lines 132 and 140 safely handles the edge case whereblock_height > current_head_height
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@crates/chain/tests/api/client.rs`:
- Around line 217-292: Move the local `use irys_api_client::TransactionStatus;`
inside `api_tx_status_lifecycle` up to the file-level imports alongside the
other `use` statements so the import is consistent across the test file; update
the top of `crates/chain/tests/api/client.rs` to include `use
irys_api_client::TransactionStatus;` and remove the in-function `use` line in
`api_tx_status_lifecycle`.
- Line 313: Replace the inconsistent signer access in the test: change the call
using ctx.cfg.signer() to use the same pattern as other tests,
ctx.node_ctx.config.irys_signer(), so the test uses the standardized signer
accessor; locate the usage of signer in the test (the variable named signer on
the line with let signer = ctx.cfg.signer()) and update it to call
ctx.node_ctx.config.irys_signer() instead.
📜 Review details
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
crates/chain/tests/api/client.rs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-13T12:16:14.104Z
Learnt from: JesseTheRobot
Repo: Irys-xyz/irys PR: 1095
File: crates/types/src/transaction.rs:920-921
Timestamp: 2026-01-13T12:16:14.104Z
Learning: CommitmentTransactionV1 has never been RLP encoded/decoded and is now deprecated with a frozen implementation, so RLP round-trip tests for V1 are not necessary.
Applied to files:
crates/chain/tests/api/client.rs
🔇 Additional comments (1)
crates/chain/tests/api/client.rs (1)
340-366: LGTM on polling logic and assertions.The polling approach correctly handles the non-deterministic timing of commitment transaction inclusion. Accepting either
INCLUDEDorCONFIRMEDas valid final states is appropriate since confirmations may accumulate during the polling loop. The debug output on assertion failure (line 363) will aid troubleshooting.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/actors/src/mempool_service.rs (1)
2260-2289: Dualpromoted_heighttracking may cause inconsistencies.There are now two
promoted_heightfields:
DataTransactionHeader.promoted_height(accessed viaDerefMutat line 2264, 2283)TransactionMetadata.promoted_height(viametadata.promoted_height)The
clear_promoted_heightandset_promoted_heightmethods onAtomicMempoolStateoperate on the transaction header's field, whileDataTxWithMetadata::set_promoted_heightoperates on the metadata's field. This could lead to the two values getting out of sync.Consider whether the metadata's
promoted_heightshould be the single source of truth, or if there's a specific reason to maintain both locations.
🤖 Fix all issues with AI agents
In `@crates/actors/src/mempool_service.rs`:
- Around line 2066-2076: The two methods set_tx_included_height and
set_data_tx_included_height have inconsistent semantics; consolidate them into a
single API (e.g., set_data_tx_included_height) that accepts an overwrite flag
(or a mode) and performs both setting and cache maintenance in one place:
acquire the same write lock on state, look up tx in
state.valid_submit_ledger_tx, set included height using
wrapped_tx.set_included_height() only-if-None or unconditionally according to
the flag, and update state.recent_valid_tx cache exactly as
set_data_tx_included_height does; then replace callers of set_tx_included_height
to use the consolidated method (or clearly document the two behaviors if you
choose to keep both), ensuring consistent semantics for valid_submit_ledger_tx
and recent_valid_tx updates.
♻️ Duplicate comments (2)
crates/actors/src/mempool_service/lifecycle.rs (1)
884-909: Optional: skip empty batch metadata writes to avoid write-tx churn.
This is the same “skip-empty updates” optimization discussed earlier, applied to the new migration fallback block.♻️ Proposed guard to skip empty updates
- if let Err(e) = self.irys_db.update_eyre(|tx| { - irys_database::batch_set_tx_included_height(tx, &all_tx_ids, block_height) - .map_err(|e| eyre::eyre!("{:?}", e)) - }) { - error!("Failed to batch set included_height in database: {}", e); - } + if !all_tx_ids.is_empty() { + if let Err(e) = self.irys_db.update_eyre(|tx| { + irys_database::batch_set_tx_included_height(tx, &all_tx_ids, block_height) + .map_err(|e| eyre::eyre!("{:?}", e)) + }) { + error!("Failed to batch set included_height in database: {}", e); + } + } - if let Err(e) = self.irys_db.update_eyre(|tx| { - irys_database::batch_set_tx_promoted_height(tx, &publish_tx_ids, block_height) - .map_err(|e| eyre::eyre!("{:?}", e)) - }) { - error!("Failed to batch set promoted_height in database: {}", e); - } + if !publish_tx_ids.is_empty() { + if let Err(e) = self.irys_db.update_eyre(|tx| { + irys_database::batch_set_tx_promoted_height(tx, &publish_tx_ids, block_height) + .map_err(|e| eyre::eyre!("{:?}", e)) + }) { + error!("Failed to batch set promoted_height in database: {}", e); + } + }crates/actors/src/mempool_service.rs (1)
2078-2088: Doc comment doesn't match return semantics.The doc says "Returns true if the transaction was found and the height was cleared" but it returns
trueeven whenincluded_heightwas alreadyNone(found but not actually changed).
📜 Review details
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (16)
crates/actors/src/block_validation.rscrates/actors/src/mempool_service.rscrates/actors/src/mempool_service/lifecycle.rscrates/actors/src/shadow_tx_generator.rscrates/chain/tests/api/client.rscrates/chain/tests/api/hardfork_tests.rscrates/chain/tests/api/tx_commitments.rscrates/chain/tests/synchronization/mod.rscrates/database/src/database.rscrates/database/src/db_index.rscrates/domain/src/snapshots/commitment_snapshot.rscrates/p2p/src/block_pool.rscrates/types/src/commitment_common.rscrates/types/src/signature.rscrates/types/src/transaction_metadata.rscrates/types/tests/dynamic_version_dispatch_tests.rs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-13T12:16:14.104Z
Learnt from: JesseTheRobot
Repo: Irys-xyz/irys PR: 1095
File: crates/types/src/transaction.rs:920-921
Timestamp: 2026-01-13T12:16:14.104Z
Learning: CommitmentTransactionV1 has never been RLP encoded/decoded and is now deprecated with a frozen implementation, so RLP round-trip tests for V1 are not necessary.
Applied to files:
crates/types/tests/dynamic_version_dispatch_tests.rscrates/domain/src/snapshots/commitment_snapshot.rscrates/chain/tests/api/tx_commitments.rscrates/types/src/signature.rscrates/p2p/src/block_pool.rscrates/actors/src/block_validation.rscrates/chain/tests/api/hardfork_tests.rscrates/chain/tests/synchronization/mod.rscrates/database/src/database.rscrates/types/src/commitment_common.rs
🧬 Code graph analysis (9)
crates/types/tests/dynamic_version_dispatch_tests.rs (1)
crates/types/src/commitment_common.rs (1)
default(137-142)
crates/chain/tests/api/tx_commitments.rs (2)
crates/types/src/commitment_common.rs (2)
anchor(238-243)default(137-142)crates/types/src/commitment_v2.rs (1)
anchor(219-221)
crates/database/src/db_index.rs (4)
crates/actors/src/mempool_service.rs (2)
get_tx_metadata(2216-2239)tx(1105-1106)crates/actors/src/mempool_guard.rs (1)
get_tx_metadata(76-81)crates/database/src/database.rs (16)
tx(76-76)tx(82-82)tx(93-94)tx(99-99)tx(111-111)tx(119-120)tx(129-129)tx(137-138)tx(152-152)tx(157-157)tx(174-174)tx(213-213)tx(223-223)tx(234-234)tx(263-263)open_or_create_db(37-56)crates/types/src/commitment_common.rs (1)
metadata(329-334)
crates/actors/src/shadow_tx_generator.rs (1)
crates/types/src/commitment_common.rs (5)
commitment_type(184-189)signer(229-234)value(220-225)fee(211-216)default(137-142)
crates/types/src/signature.rs (1)
crates/types/src/commitment_common.rs (1)
default(137-142)
crates/chain/tests/api/hardfork_tests.rs (4)
crates/types/src/commitment_common.rs (4)
anchor(238-243)fee(211-216)value(220-225)new(441-446)crates/types/src/commitment_v1.rs (2)
anchor(221-223)new(88-99)crates/types/src/commitment_v2.rs (2)
anchor(219-221)new(86-97)crates/types/src/transaction_metadata.rs (2)
new(31-36)new(244-249)
crates/chain/tests/synchronization/mod.rs (2)
crates/types/src/commitment_common.rs (2)
anchor(238-243)default(137-142)crates/types/src/commitment_v2.rs (1)
anchor(219-221)
crates/actors/src/mempool_service.rs (1)
crates/types/src/transaction_metadata.rs (3)
with_metadata(251-253)new(31-36)new(244-249)
crates/types/src/commitment_common.rs (4)
crates/types/src/transaction_metadata.rs (3)
new(31-36)new(244-249)id(263-265)crates/types/src/commitment_v1.rs (5)
new(88-99)id(209-211)signer(213-215)anchor(221-223)signature(217-219)crates/types/src/commitment_v2.rs (5)
new(86-97)id(207-209)signer(211-213)anchor(219-221)signature(215-217)crates/types/src/versioning.rs (1)
compact_with_discriminant(65-72)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: cargo fmt
- GitHub Check: cargo test
- GitHub Check: cargo check
- GitHub Check: cargo doc
- GitHub Check: cargo clippy
- GitHub Check: cargo test
- GitHub Check: cargo check
- GitHub Check: cargo clippy
- GitHub Check: cargo doc
🔇 Additional comments (40)
crates/types/src/signature.rs (1)
316-336: LGTM!The test correctly updates to use the new
CommitmentV2WithMetadatawrapper pattern. The inner transaction fields remain unchanged, and the metadata is properly initialized withDefault::default(). The test continues to verify signature signing, compact encoding, RLP encoding, and serde round-trips correctly.crates/types/tests/dynamic_version_dispatch_tests.rs (1)
42-45: LGTM!The test correctly updates to use
CommitmentV1WithMetadatawrapper, maintaining consistency with the metadata wrapper pattern applied across V1 and V2 variants. The test purpose remains intact: verifying that a versioned type can be constructed from an inner V1 type.crates/database/src/database.rs (1)
529-536: LGTM!The test correctly updates the commitment transaction construction to use the
CommitmentV2WithMetadatawrapper. The test continues to verify database insert/get operations for commitment transactions, with the ID override ensuring deserialization validation works as intended.crates/domain/src/snapshots/commitment_snapshot.rs (1)
393-405: LGTM!The test helper correctly adopts the
CommitmentV2WithMetadatawrapper pattern. All tests in this file that usecreate_test_commitmentwill now correctly construct metadata-wrapped commitment transactions. The subsequentset_id(H256::random())call properly assigns a unique ID after construction.crates/actors/src/shadow_tx_generator.rs (1)
771-783: LGTM!The test helper correctly updates to use the
CommitmentV2WithMetadatawrapper pattern, consistent with the other files in this PR. The helper properly constructs commitment transactions for shadow transaction generation tests, with all fields appropriately initialized for test purposes.crates/chain/tests/api/client.rs (3)
5-5: Good import alignment for status assertions.This keeps TransactionStatus available for the new status lifecycle tests.
217-290: Solid lifecycle test for PENDING → INCLUDED → CONFIRMED.The flow validates status transitions, block height/confirmations, and uses migration depth for confirmation, which is the right signal.
292-367: Commitment status progression test looks robust.The poll/mine loop acknowledges commitment inclusion variability while still asserting Included/Confirmed plus height/confirmations presence.
crates/chain/tests/synchronization/mod.rs (1)
455-464: Wrapper update matches new metadata surface.Using CommitmentV2WithMetadata keeps tests consistent with the updated commitment transaction shape.
crates/chain/tests/api/hardfork_tests.rs (1)
68-87: Metadata wrappers correctly applied for V1/V2 stake constructors.This keeps the test constructors in sync with the new commitment type signatures.
crates/p2p/src/block_pool.rs (2)
1689-1694: Test data updated to metadata-wrapped commitment V2.Keeps order_transactions_for_block tests aligned with the new commitment representation.
1890-1895: Consistent metadata wrapping in commitment mismatch test.This prevents stale construction patterns in the negative-path coverage.
crates/chain/tests/api/tx_commitments.rs (2)
508-517: Stake commitment helper updated to metadata wrapper.This matches the new CommitmentV2WithMetadata structure and keeps signing/posting flow intact.
542-553: Pledge commitment helper correctly wrapped with metadata.Consistent with the new commitment API and preserves existing test intent.
crates/actors/src/block_validation.rs (1)
3854-3870: LGTM: test helpers now build metadata-wrapped commitments.
Matches the new Commitment*WithMetadata shape without altering test semantics.crates/actors/src/mempool_service/lifecycle.rs (4)
23-69: Good: included/promoted metadata persisted on block confirmation.
Nice alignment with status endpoint behavior and mempool metadata updates.
479-488: Good reorg hygiene: metadata cleared for orphaned txs.
Clearing included_height/promoted_height in mempool/DB keeps status consistent across forks.Also applies to: 612-621, 653-662, 677-688
709-754: Reorg reconciliation looks solid.
Promoted-height reconciliation plus deduped orphan clearing is consistent and safe.Also applies to: 756-775
801-802: LGTM: explicit clone avoids ownership friction during migration.crates/database/src/db_index.rs (2)
11-115: LGTM: metadata helpers and batch APIs are straightforward.
Clear and consistent surface for included/promoted height updates.
117-244: Tests cover included/promoted metadata paths well.
Good coverage for single and batch operations.crates/types/src/commitment_common.rs (2)
65-353: LGTM: metadata wrappers + accessors are clean and consistent.
Serde behavior is explicit and keeps metadata out of network serialization.
355-516: Good: compact/RLP encode-decode and constructors honor wrapper shape.
Metadata initialization on decode/new is consistent with the new model.crates/actors/src/mempool_service.rs (10)
51-52: LGTM!The imports for
DataTxWithMetadataandTransactionMetadataare correctly added to support the new metadata tracking functionality.
1826-1827: LGTM!The accessor correctly unwraps
DataTxWithMetadatato returnDataTransactionHeader, maintaining the existing API contract.
2090-2136: LGTM!The commitment transaction metadata methods correctly search both
valid_commitment_txandpending_pledges, ensuring metadata can be updated regardless of where the transaction currently resides.
2210-2239: LGTM!The
get_tx_metadatamethod provides comprehensive lookup across all transaction storage locations: data transactions, commitment transactions, and pending pledges. This ensures metadata can be retrieved regardless of the transaction's current state.
2249-2258: LGTM!The update logic correctly preserves existing metadata by only modifying the inner
txfield within the wrapper.
2601-2655: LGTM!The metadata preservation logic correctly:
- Preserves existing metadata when updating an entry (lines 2610-2614)
- Creates default metadata for new insertions (lines 2650-2653)
This ensures metadata isn't lost during transaction updates.
2659-2664: LGTM!The fee comparison correctly accesses the wrapped transaction's fee via
Deref.
2675-2679: LGTM!Extracting
tx_idto a local variable improves readability and avoids repeated method calls.
3255-3268: LGTM!The test helper correctly constructs
CommitmentV2WithMetadatawith default metadata, aligning with the new type system.
2543-2546: LGTM!The storage type change to
DataTxWithMetadatais the foundation for metadata tracking. All accessor methods have been consistently updated to handle the wrapped type.crates/types/src/transaction_metadata.rs (7)
1-28: LGTM!The
TransactionMetadatastruct is well-designed:
- Clear documentation explaining the purpose of each field
- Appropriate derives for serialization, debugging, and testing
Compactderive for efficient database encoding
45-52: Doc comment addresses the concern.The added documentation explains that
with_promoted_heightis for testing or independent promotion tracking, addressing the earlier review feedback about domain model consistency.
63-73: LGTM!The
TransactionStatusenum provides clear states for API responses with appropriate SCREAMING_SNAKE_CASE serialization.
75-121: LGTM!The
TransactionStatusResponseis well-implemented:
- Uses
optional_string_u64for JavaScript compatibility- Correctly uses
saturating_subfor confirmation calculation- Skips serialization of
Nonevalues for cleaner API responses
123-233: Excellent test coverage!The tests comprehensively cover:
- Metadata creation and predicates
- Status response construction for all states
- Compact encoding roundtrip for all field combinations
- Edge cases for confirmation calculation saturation
This addresses the previous review feedback about missing Compact encoding tests.
235-296: LGTM!The
DataTxWithMetadatawrapper provides a clean API for managing transaction metadata:
- Multiple constructors for different use cases
- Mutators for height management
- Decomposition methods for database storage
298-312: LGTM!The
Deref/DerefMutimplementations enable ergonomic access to the underlying transaction's fields and methods.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/actors/src/mempool_service.rs`:
- Around line 2313-2333: The clear_data_tx_included_height implementation
duplicates logic; change it to call the shared clear_tx_included_height(txid)
method instead of reimplementing the mutation to keep cache update and logging
consistent. Locate clear_data_tx_included_height and replace its body to await
and return clear_tx_included_height(txid). Ensure you use the existing async
method clear_tx_included_height and retain the same signature and boolean return
value so callers behave unchanged.
♻️ Duplicate comments (1)
crates/actors/src/mempool_service.rs (1)
2066-2108: Return value doesn’t reflect whether included_height changed.
Both helpers returntrueeven when nothing was updated/cleared, which contradicts the docstring and can mislead callers that use the boolean to decide persistence or follow‑up actions.🛠 Proposed fix (track actual change)
pub async fn set_tx_included_height( &self, tx_id: H256, height: u64, overwrite: bool, ) -> bool { let mut state = self.write().await; if let Some(wrapped_tx) = state.valid_submit_ledger_tx.get_mut(&tx_id) { - if overwrite || wrapped_tx.metadata.included_height.is_none() { + let mut updated = false; + if overwrite || wrapped_tx.metadata.included_height.is_none() { wrapped_tx.metadata.included_height = Some(height); + updated = true; tracing::debug!( tx.id = %tx_id, included_height = height, overwrite = overwrite, "Set included_height in mempool" ); } // Always update recent_valid_tx cache when tx is found state.recent_valid_tx.put(tx_id, ()); - true + updated } else { false } } pub async fn clear_tx_included_height(&self, tx_id: H256) -> bool { let mut state = self.write().await; if let Some(wrapped_tx) = state.valid_submit_ledger_tx.get_mut(&tx_id) { - wrapped_tx.clear_included_height(); - true + let was_set = wrapped_tx.metadata.included_height.is_some(); + if was_set { + wrapped_tx.clear_included_height(); + } + was_set } else { false } }
📜 Review details
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
crates/actors/src/mempool_service.rs
🧰 Additional context used
🧬 Code graph analysis (1)
crates/actors/src/mempool_service.rs (4)
crates/types/src/transaction_metadata.rs (5)
id(263-265)clear_promoted_height(283-285)with_metadata(251-253)new(31-36)new(244-249)crates/types/src/commitment_common.rs (3)
id(202-207)signer(229-234)value(220-225)crates/types/src/transaction.rs (19)
id(360-362)id(393-393)id(428-430)id(506-508)id(597-602)new(172-174)new(341-357)new(716-720)signer(364-366)signer(395-395)signer(436-438)signer(514-516)signer(604-609)default(79-81)default(761-763)from(658-660)from(664-666)from(681-683)from(687-689)crates/database/src/db_index.rs (2)
set_tx_included_height(30-38)clear_tx_included_height(52-59)
🪛 GitHub Check: cargo fmt
crates/actors/src/mempool_service.rs
[warning] 2071-2071:
Diff in /home/runner/actions-runner/_work/irys/irys/crates/actors/src/mempool_service.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: cargo test
- GitHub Check: cargo clippy
- GitHub Check: cargo doc
- GitHub Check: cargo check
🔇 Additional comments (7)
crates/actors/src/mempool_service.rs (7)
51-52: DataTxWithMetadata integration is consistent across read/update paths.
Unwrapping for public reads and preserving metadata on updates keeps the external API stable while retaining metadata internally.Also applies to: 1826-1828, 1936-1949, 2560-2560, 2621-2628, 2664-2668, 2675-2678
2112-2158: Commitment included-height helpers cover both valid and pending pledge caches.
Good parity between valid_commitment_tx and pending_pledges.
2219-2229: Unwrapping wrapped txs before returning is correct.
Maintains the public API while still storing metadata internally.
2232-2261: Recent-valid and metadata accessors are well scoped.
Nice to see metadata retrieval covering data, commitments, and pending pledges.
2277-2286: Updates preserve metadata while mutating headers.
Switching to in‑place header updates avoids dropping wrapper metadata.Also applies to: 2303-2307
2689-2749: Use of tx_id variable improves logging consistency.
Clearer logs with less repeated computation.
3269-3280: Tests updated correctly for CommitmentV2WithMetadata.
Good alignment with the new metadata wrapper.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@crates/chain/tests/api/client.rs`:
- Around line 255-267: The test currently assumes a single ctx.mine_block()
makes the transaction INCLUDED/Mined; instead implement a bounded polling loop
that repeatedly calls api_client.get_transaction_status(api_address,
tx_id).await and checks status.status == TransactionStatus::Mined (and that
block_height/confirmations are Some), retrying with a short delay and overall
timeout/attempt limit similar to the commitment test to avoid flakiness; update
the assertions to occur after the poll succeeds and fail the test if the timeout
is reached.
In `@crates/database/src/migration.rs`:
- Around line 638-774: Add a new test that exercises Step 2 of v1→v2 migration:
after creating v1 headers with promoted_height (like in
should_migrate_from_v1_to_v2) also insert appropriate IrysBlockHeaders entries
that include those transaction IDs in Submit/Publish/Commitment ledgers, then
run super::v1_to_v2::migrate(tx) and assert that IrysDataTxMetadata entries were
created/updated with included_height populated from the matching block headers
(and promoted_height preserved), referencing the same helpers used in the
current test (temporary_directory, open_or_create_db,
crate::set_database_schema_version) and the table types IrysBlockHeaders and
IrysDataTxMetadata so the test explicitly verifies block-header-driven metadata
filling.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/chain/tests/api/client.rs`:
- Around line 258-279: The polling loop in the test assumes the first
non-Pending transaction status is TransactionStatus::Mined which can be flaky if
the tx becomes TransactionStatus::Finalized before the test observes it; update
the loop that calls api_client.get_transaction_status(api_address, tx_id) to
treat both TransactionStatus::Mined and TransactionStatus::Finalized as
acceptable terminal states (i.e., break and set status when either is seen), and
avoid calling ctx.mine_block().await after observing Finalized (skip extra
mining if status is Finalized) so assertions check for Mined-or-Finalized and
presence of block_height/confirmations.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/chain/tests/api/client.rs (1)
70-85:get_transactionsequality is still metadata-sensitive.
You switchedget_transactiontoeq_tx, buttxs.contains(...)still relies on full equality and can fail if the API enriches metadata. Compare viaeq_txper element for consistency.🧪 Suggested fix
- assert!(txs.contains(&IrysTransactionResponse::Storage(tx.header))); - assert!(txs.contains(&IrysTransactionResponse::Storage(tx_2.header))); + assert!(txs.iter().any(|resp| { + matches!(resp, IrysTransactionResponse::Storage(h) if h.eq_tx(&tx.header)) + })); + assert!(txs.iter().any(|resp| { + matches!(resp, IrysTransactionResponse::Storage(h) if h.eq_tx(&tx_2.header)) + }));
🤖 Fix all issues with AI agents
In `@crates/chain/tests/api/client.rs`:
- Around line 290-315: The test currently waits only for included_height via
ctx.wait_until_block_index_height(included_height, 15) which can race with the
block_migration_depth and leave the tx in TransactionStatus::Mined; update the
wait to ensure the indexer has processed migration_depth by waiting for
included_height + migration_depth (i.e., compute let target_height =
included_height + migration_depth and call
ctx.wait_until_block_index_height(target_height, 15).await) or replace that wait
with a polling loop that calls api_client.get_transaction_status(api_address,
tx_id).await and retries until status.status == TransactionStatus::Finalized,
ensuring the assertion that status is Finalized no longer races with indexing.
| #[derive(Clone, Debug, IntegerTagged, Arbitrary)] | ||
| #[repr(u8)] | ||
| #[integer_tagged(tag = "version")] | ||
| pub enum DataTransactionHeader { |
There was a problem hiding this comment.
maybe add a comment here (& on the CommitmentTransactionHeader) that PartialEq and Eq aren't derived on purpose
There was a problem hiding this comment.
I can easily see someone try to == two txs and go "huh" and then add it as a derive, when that's not what you'd want most of the time
There was a problem hiding this comment.
Sure, makes total sense
There was a problem hiding this comment.
Sure, makes total sense
| @@ -3,3 +3,382 @@ | |||
| /// and into a `db_index` that is able to store more properties (to support | |||
| /// mining) now that the data is confirmed. | |||
| const _X: u64 = 1; | |||
There was a problem hiding this comment.
any ideas why this is here?
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@crates/chain/tests/api/client.rs`:
- Around line 303-307: The CI flakes show
ctx.wait_until_block_index_height(target_height, 15) times out; increase the
retry/timeout budget used by wait_until_block_index_height (or replace the fixed
15 with a larger value or configurable constant) so the indexer has more time
under load—update the call that computes target_height from included_height +
migration_depth and replace the 15 with a higher retry count (e.g., 60) or wire
it to a test timeout constant to avoid flakes.
In `@crates/chain/tests/api/tx.rs`:
- Line 85: The current test uses storage_tx.eq_tx(&storage) which ignores
metadata; if the API should round‑trip metadata, add an explicit assertion
comparing the returned transaction's metadata to the original (e.g.,
assert_eq!(storage_tx.metadata, storage.metadata) or compare specific metadata
fields), keeping the existing eq_tx check for other fields and using the
storage_tx and storage identifiers to locate the variables to compare.
In `@crates/database/src/migration.rs`:
- Around line 262-264: Preallocate the migration batch buffer to avoid repeated
reallocations: replace the current let mut entries_to_migrate = Vec::new() with
let mut entries_to_migrate = Vec::with_capacity(BATCH_SIZE) (using the existing
const BATCH_SIZE). Also ensure the buffer is reused between batches (call
entries_to_migrate.clear() after processing each batch instead of reassigning)
and keep total_migrated handling unchanged.
In `@crates/types/src/block.rs`:
- Around line 715-731: The panic message in the Index and IndexMut impls for
Vec<SystemTransactionLedger> does not include the missing ledger value; update
the index(&self, ledger: SystemLedger) and index_mut(&mut self, ledger:
SystemLedger) code paths that currently call .expect(...) on the iterator result
to instead surface the ledger value in the panic (e.g. replace the .expect(...)
with an unwrap_or_else(|| panic!(...)) or equivalent so the message includes
ledger or ledger as u32), making failures show the missing ledger id when a
SystemTransactionLedger isn't found.
♻️ Duplicate comments (3)
crates/actors/src/block_producer/ledger_expiry.rs (1)
216-218: Redundant sort still present (already discussed).
You’re sorting here and again insideaggregate_balance_deltas; one of the sorts can be dropped.♻️ Suggested change
- transactions.sort_by(irys_types::DataTransactionHeader::compare_tx);Also applies to: 639-641
crates/actors/src/transaction_status.rs (1)
20-49: Prefer DB metadata when mempool metadata lacksincluded_height.
mempool_metadata.or(db_metadata)can returnPendingeven when the DB already has an included/confirmed height.🛠️ Suggested fix
- let metadata = mempool_metadata.or(db_metadata); + let metadata = match (mempool_metadata, db_metadata) { + (Some(m), Some(db)) if m.included_height().is_none() && db.included_height().is_some() => { + Some(db) + } + (Some(m), _) => Some(m), + (None, db) => db, + };crates/actors/src/mempool_service.rs (1)
2100-2109: Keep cache/log behavior consistent when clearing included_height.
clear_data_tx_included_height_innerdoesn’t updaterecent_valid_txor log like other metadata mutations, which can make reorg-driven updates harder to observe.🔧 Suggested adjustment
async fn clear_data_tx_included_height_inner(&self, tx_id: H256) -> bool { let mut state = self.write().await; if let Some(wrapped_tx) = state.valid_submit_ledger_tx.get_mut(&tx_id) { wrapped_tx.metadata_mut().included_height = None; + tracing::debug!(tx.id = %tx_id, "Cleared included_height in mempool"); + state.recent_valid_tx.put(tx_id, ()); true } else { false } }Also applies to: 2342-2346
| // Wait for indexer to process migration depth to ensure Finalized status | ||
| let target_height = included_height + migration_depth; | ||
| ctx.wait_until_block_index_height(target_height, 15) | ||
| .await | ||
| .expect("block index should reach target height for finalization"); |
There was a problem hiding this comment.
CI failure: block-index wait times out; increase retry budget.
The pipeline log shows Failed to reach target height 9 after 30 retries, so wait_until_block_index_height(..., 15) appears too short under CI load. Consider increasing the retry/timeout budget (or equivalent backoff) to avoid flakes.
🛠️ Suggested fix (increase wait budget)
- ctx.wait_until_block_index_height(target_height, 15)
+ ctx.wait_until_block_index_height(target_height, 60)
.await
.expect("block index should reach target height for finalization");🧰 Tools
🪛 GitHub Actions: Rust Checks
[error] 307-307: Failed to reach target height 9 after 30 retries
🤖 Prompt for AI Agents
In `@crates/chain/tests/api/client.rs` around lines 303 - 307, The CI flakes show
ctx.wait_until_block_index_height(target_height, 15) times out; increase the
retry/timeout budget used by wait_until_block_index_height (or replace the fixed
15 with a larger value or configurable constant) so the indexer has more time
under load—update the call that computes target_height from included_height +
migration_depth and replace the 15 with a higher retry count (e.g., 60) or wire
it to a test timeout constant to avoid flakes.
There was a problem hiding this comment.
That actually wasn't an issue. The issue was that we need to mine more block - migration depth is the amount of blocks needed to be added on top of the block before it gets migrated
There was a problem hiding this comment.
@antouhou, thank you for the clarification! You're absolutely right—I misdiagnosed the issue. The problem is indeed about mining enough blocks to reach the migration depth, not the wait timeout.
If the transaction is included at a certain height and we need migration_depth additional blocks on top of that block for it to be migrated/finalized, then the current logic of mining migration_depth + 2 blocks should be sufficient. However, if the CI is still timing out at the indexer wait, it might be that:
- The transaction isn't being included in the immediately next block after posting, so the
included_heightis higher than expected - The indexer is lagging behind the actual chain height
Would it help to log the actual included_height and target_height in the test to verify the math is working as expected during CI runs?
| } | ||
| }; | ||
| assert_eq!(storage_tx, storage); | ||
| assert!(storage_tx.eq_tx(&storage)); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider explicitly asserting metadata if the endpoint is expected to return it.
eq_tx ignores metadata; if metadata round‑trip is part of the contract, add a direct metadata assertion.
🤖 Prompt for AI Agents
In `@crates/chain/tests/api/tx.rs` at line 85, The current test uses
storage_tx.eq_tx(&storage) which ignores metadata; if the API should round‑trip
metadata, add an explicit assertion comparing the returned transaction's
metadata to the original (e.g., assert_eq!(storage_tx.metadata,
storage.metadata) or compare specific metadata fields), keeping the existing
eq_tx check for other fields and using the storage_tx and storage identifiers to
locate the variables to compare.
| let mut entries_to_migrate = Vec::new(); | ||
| const BATCH_SIZE: usize = 10000; | ||
| let mut total_migrated = 0; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Preallocate the migration batch buffer.
This avoids repeated reallocations during large migrations.
♻️ Proposed tweak
- let mut entries_to_migrate = Vec::new();
+ let mut entries_to_migrate = Vec::with_capacity(BATCH_SIZE);🤖 Prompt for AI Agents
In `@crates/database/src/migration.rs` around lines 262 - 264, Preallocate the
migration batch buffer to avoid repeated reallocations: replace the current let
mut entries_to_migrate = Vec::new() with let mut entries_to_migrate =
Vec::with_capacity(BATCH_SIZE) (using the existing const BATCH_SIZE). Also
ensure the buffer is reused between batches (call entries_to_migrate.clear()
after processing each batch instead of reassigning) and keep total_migrated
handling unchanged.
| impl Index<SystemLedger> for Vec<SystemTransactionLedger> { | ||
| type Output = SystemTransactionLedger; | ||
|
|
||
| fn index(&self, ledger: SystemLedger) -> &Self::Output { | ||
| self.iter() | ||
| .find(|tx_ledger| tx_ledger.ledger_id == ledger as u32) | ||
| .expect("No system transaction ledger found for given ledger type") | ||
| } | ||
| } | ||
|
|
||
| impl IndexMut<SystemLedger> for Vec<SystemTransactionLedger> { | ||
| fn index_mut(&mut self, ledger: SystemLedger) -> &mut Self::Output { | ||
| self.iter_mut() | ||
| .find(|tx_ledger| tx_ledger.ledger_id == ledger as u32) | ||
| .expect("No system transaction ledger found for given ledger type") | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Improve panic diagnostics for missing system ledger.
Including the missing ledger value makes failures easier to debug.
♻️ Proposed tweak
- self.iter()
- .find(|tx_ledger| tx_ledger.ledger_id == ledger as u32)
- .expect("No system transaction ledger found for given ledger type")
+ self.iter()
+ .find(|tx_ledger| tx_ledger.ledger_id == ledger as u32)
+ .unwrap_or_else(|| panic!("No system transaction ledger found for {:?}", ledger))
}
}
@@
- self.iter_mut()
- .find(|tx_ledger| tx_ledger.ledger_id == ledger as u32)
- .expect("No system transaction ledger found for given ledger type")
+ self.iter_mut()
+ .find(|tx_ledger| tx_ledger.ledger_id == ledger as u32)
+ .unwrap_or_else(|| panic!("No system transaction ledger found for {:?}", ledger))
}
}🤖 Prompt for AI Agents
In `@crates/types/src/block.rs` around lines 715 - 731, The panic message in the
Index and IndexMut impls for Vec<SystemTransactionLedger> does not include the
missing ledger value; update the index(&self, ledger: SystemLedger) and
index_mut(&mut self, ledger: SystemLedger) code paths that currently call
.expect(...) on the iterator result to instead surface the ledger value in the
panic (e.g. replace the .expect(...) with an unwrap_or_else(|| panic!(...)) or
equivalent so the message includes ledger or ledger as u32), making failures
show the missing ledger id when a SystemTransactionLedger isn't found.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@crates/chain/tests/api/client.rs`:
- Line 219: Update the doc comment that describes the transaction status
lifecycle to use the current enum variant names: replace "MINED" with
"Confirmed" so it reads "PENDING -> Confirmed -> FINALIZED"; locate the test
comment associated with the transaction status API lifecycle (the test that
references TransactionStatus) and edit the doc string accordingly to match the
TransactionStatus enum naming.
In `@crates/types/src/transaction_metadata.rs`:
- Around line 136-143: Update the stale doc comments that reference the old
status name "MINED" to the current enum variant "Confirmed" in
transaction_metadata.rs: change the comment above the block_height field (and
the matching trailing comment) from "Only present if status is MINED or
FINALIZED" to "Only present if status is Confirmed or Finalized" (matching the
TransactionStatus enum naming). Ensure the wording matches the enum variant
casing "Confirmed" and "Finalized" used elsewhere.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/chain/tests/api/client.rs`:
- Around line 295-316: Update the stale comments so they match the asserted
Finalized status: change the two comments that mention “CONFIRMED” to refer to
reaching Finalized (or finalization) instead. Locate the block around
ctx.mine_block(), ctx.wait_until_block_index_height(), and the
get_transaction_status call (symbols: ctx.mine_block, migration_depth,
ctx.wait_until_block_index_height, api_client.get_transaction_status,
TransactionStatus::Finalized) and edit the comments to state that we are
advancing blocks and waiting for the indexer so the transaction becomes
Finalized.
| // Mine more blocks to reach migration depth and make it CONFIRMED | ||
| // Skip if already Finalized | ||
| if matches!(status.status, TransactionStatus::Confirmed) { | ||
| for _ in 0..(migration_depth * 2) { | ||
| ctx.mine_block().await.expect("expected mined block"); | ||
| } | ||
| } | ||
|
|
||
| // Wait for indexer to process migration depth to ensure Finalized status | ||
| let target_height = included_height + migration_depth; | ||
| ctx.wait_until_block_index_height(target_height, 15) | ||
| .await | ||
| .expect("block index should reach target height for finalization"); | ||
|
|
||
| // Check status - should be CONFIRMED | ||
| let status = api_client | ||
| .get_transaction_status(api_address, tx_id) | ||
| .await | ||
| .expect("get_transaction_status should succeed") | ||
| .expect("status should exist"); | ||
|
|
||
| assert!(matches!(status.status, TransactionStatus::Finalized)); |
There was a problem hiding this comment.
Fix stale comments: they say “CONFIRMED” but the logic asserts Finalized.
The two comments in this block contradict the actual assertions, which can mislead future readers.
💡 Suggested fix
- // Mine more blocks to reach migration depth and make it CONFIRMED
+ // Mine more blocks to reach migration depth and make it FINALIZED
@@
- // Check status - should be CONFIRMED
+ // Check status - should be FINALIZED🤖 Prompt for AI Agents
In `@crates/chain/tests/api/client.rs` around lines 295 - 316, Update the stale
comments so they match the asserted Finalized status: change the two comments
that mention “CONFIRMED” to refer to reaching Finalized (or finalization)
instead. Locate the block around ctx.mine_block(),
ctx.wait_until_block_index_height(), and the get_transaction_status call
(symbols: ctx.mine_block, migration_depth, ctx.wait_until_block_index_height,
api_client.get_transaction_status, TransactionStatus::Finalized) and edit the
comments to state that we are advancing blocks and waiting for the indexer so
the transaction becomes Finalized.
Describe the changes
This PR introduces status endpoint for transactions
Related Issue(s)
Checklist
Additional Context
Add any other context about the pull request here.
Summary by CodeRabbit
New Features
Improvements
Migrations
Tests
✏️ Tip: You can customize this high-level summary in your review settings.