feat: Cascade hardfork — term ledgers (OneYear/ThirtyDay) and height-aware pricing#1166
Open
DanMacDonald wants to merge 24 commits intomasterfrom
Open
feat: Cascade hardfork — term ledgers (OneYear/ThirtyDay) and height-aware pricing#1166DanMacDonald wants to merge 24 commits intomasterfrom
DanMacDonald wants to merge 24 commits intomasterfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Cascade hardfork and two term ledgers (OneYear, ThirtyDay); generalizes ledger handling from static Publish/Submit to dynamic header.data_ledgers across mempool, producer, validation, migration, DB, pricing, epoch snapshots, and tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant API as "Price API"
participant Mempool
participant Producer as "BlockProducer"
participant Validator
participant DB as "Database"
Client->>API: Request price (OneYear/ThirtyDay, timestamp)
API->>Validator: is_cascade_active_for_epoch(epoch_snapshot)
Validator-->>API: cascade_active
API-->>Client: PriceInfo (term_fee, perm_fee)
Client->>Mempool: Submit term tx (OneYear/ThirtyDay)
Mempool->>Validator: Validate ledger, fees (no perm_fee for term, term_fee via EMA/timestamp)
Validator-->>Mempool: Accept/Reject
Producer->>Mempool: GetBestMempoolTxs (include term txs)
Mempool-->>Producer: Publish/Submit/OneYear/ThirtyDay tx batches
Producer->>DB: persist_block (iterate header.data_ledgers, insert headers, set included/promoted heights)
DB-->>Producer: Ack
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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🧪 Generate unit tests (beta)
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 Tip CodeRabbit can use Trivy to scan for security misconfigurations and secrets in Infrastructure as Code files.Add a .trivyignore file to your project to customize which findings Trivy reports. |
DanMacDonald
changed the title
DanMacDonald
force-pushed
the
dmac/cascade-annual-cost
branch
from
5151944 to
4efa89a
Compare
Thread block height through pricing functions so term/perm fees use the Cascade-overridden annual cost ($0.028/GB/year) when active, instead of the base rate ($0.01/GB/year). Includes unit and integration tests.
DanMacDonald
force-pushed
the
dmac/cascade-annual-cost
branch
from
4efa89a to
80ffc63
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/chain/tests/validation/data_tx_pricing.rs (1)
385-400:⚠️ Potential issue | 🟠 MajorAvoid hardcoding pricing height to
0in height-aware fee calculations.These expected-fee calculations should use the scenario height (e.g., the captured pricing block height), otherwise the test can drift from validator behavior under Cascade activation.
💡 Suggested fix
- let expected_term_fee = calculate_term_fee_from_config( + let pricing_height = block.height; + let expected_term_fee = calculate_term_fee_from_config( data_size, &genesis_node.node_ctx.config.consensus, number_of_ingress_proofs_total, price_before_the_interval.ema_for_public_pricing(), - 0, + pricing_height, )?; @@ let expected_perm_fee = calculate_perm_fee_from_config( data_size, &genesis_node.node_ctx.config.consensus, number_of_ingress_proofs_total, price_before_the_interval.ema_for_public_pricing(), expected_term_fee, - 0, + pricing_height, )?;Apply the same pattern in the second test block (
slow_heavy_same_block_promoted_tx_with_ema_price_change_gets_accepted).Also applies to: 513-530
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/chain/tests/validation/data_tx_pricing.rs` around lines 385 - 400, The test is passing a hardcoded height of 0 into calculate_term_fee_from_config and calculate_perm_fee_from_config, which makes fee expectations insensitive to Cascade height; change those calls to use the scenario's captured pricing height (the same height used elsewhere in the test, e.g., pricing_block_height or the variable that records the pricing block height) instead of 0 so the expected_term_fee and expected_perm_fee reflect height-aware pricing; apply the same replacement in the other test block slow_heavy_same_block_promoted_tx_with_ema_price_change_gets_accepted as well.crates/actors/src/block_validation.rs (1)
1618-1665: 🧹 Nitpick | 🔵 TrivialRedundant
cascade_activecomputation.
cascade_activeis computed at line 1618 and again at line 1645 for the sameblock.height. The inner computation can reuse the outer variable.Proposed fix
true, // expect txs to be promoted — return perm fee refund if not ) .in_current_span() .await?; // When Cascade is active, also process OneYear and ThirtyDay term ledgers. - let cascade_active = config.consensus.hardforks.is_cascade_active(block.height); if cascade_active { for ledger in [DataLedger::OneYear, DataLedger::ThirtyDay] {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/actors/src/block_validation.rs` around lines 1618 - 1665, The variable cascade_active is computed twice for the same block height; remove the inner recomputation inside the expired_ledger_fees block and reuse the outer cascade_active (declared with let cascade_active = config.consensus.hardforks.is_cascade_active(block.height);) in the subsequent if check that iterates over DataLedger::OneYear and DataLedger::ThirtyDay, so simply replace the inner let cascade_active = ... with using the existing cascade_active variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/actors/src/block_migration_service.rs`:
- Around line 316-320: persist_block validates DataLedger conversion with
DataLedger::try_from and returns an error on failure, but send_chunk_migration
currently swallows conversion failures; update the loop in send_chunk_migration
that iterates header.data_ledgers to handle DataLedger::try_from failures
consistently by propagating or returning an error instead of ignoring them —
locate the loop using symbols header.data_ledgers, DataLedger::try_from,
all_txs_map and transactions.get_ledger_txs and change the if let Ok(...) to a
fallible conversion that returns or propagates a descriptive error (or
logs+returns) when try_from fails so invalid ledger_id cases are not silently
ignored.
In `@crates/actors/src/block_producer/ledger_expiry.rs`:
- Around line 641-643: In the merge() method, appending
other.user_perm_fee_refunds with extend() breaks the struct's invariant that
user_perm_fee_refunds is sorted by tx ID; replace the extend() call with a merge
of the two sorted sequences (self.user_perm_fee_refunds and
other.user_perm_fee_refunds) into a new Vec, writing back the merged, sorted Vec
to self.user_perm_fee_refunds (or perform an in-place two-pointer merge) so the
resulting list preserves the tx-ID ordering declared by the struct contract.
- Around line 933-961: The test currently only checks fee sums but not the XOR
merge of rolling hashes; after calling LedgerExpiryBalanceDelta::merge on
delta1, assert that the rolling hash for addr1 equals RollingHash(U256::from(1)
^ U256::from(2)) and that addr2's rolling hash equals
RollingHash(U256::from(3)); locate the tuples in delta1.reward_balance_increment
for addr1 and addr2 (the stored values are (U256, RollingHash)) and compare the
second element to the expected RollingHash to cover the merge logic in
LedgerExpiryBalanceDelta::merge.
In `@crates/actors/src/mempool_service.rs`:
- Around line 1053-1068: The metrics and summary/rejection reporting still use
only submit_tx.len(), causing undercounting when DataLedger pushes transactions
into one_year_tx or thirty_day_tx; update all places that compute
selected/accepted data transaction counts and rejection metrics (including where
total_selected or metrics are emitted) to sum submit_tx.len() +
one_year_tx.len() + thirty_day_tx.len() (or reuse the existing total_selected
variable) instead of using submit_tx.len() alone so telemetry reflects the
multi-ledger selection logic (look for uses of submit_tx.len(), total_selected,
max_data_txs, and any metrics/logging or rejection-rate calculations in
mempool_service.rs).
- Around line 1022-1023: The comment identifies that term-ledger transactions
are not being validated against the protocol minimum fee before selection;
modify the mempool selection path (where candidates are assembled—look for the
function/select_candidates logic and the existing check_funding function) to
enforce protocol-fee validation for term transactions (Term::OneYear,
Term::ThirtyDay) prior to accepting them as candidates. Add a pre-selection
check (or extend check_funding) that inspects the transaction's Term on the
Tx/Transaction object and compares its fee to the protocol minimum for that term
(use the Term enum and the protocol fee getter/config used elsewhere), and
reject transactions whose fee is below the required term-specific protocol fee
so underpriced term-ledger txs cannot be admitted into candidates. Ensure the
new check runs before any downstream deferral logic.
- Around line 1008-1014: The Cascade activation check in the mempool gating uses
current_height but should use the height of the block being built; update the
condition that calls
.config.node_config.consensus_config().hardforks.is_cascade_active(current_height)
to use next_block_height instead so tx inclusion is evaluated against the
block's height (e.g., replace current_height with next_block_height in that
is_cascade_active(...) call inside the mempool_service logic that builds the
next block).
In `@crates/actors/src/mempool_service/data_txs.rs`:
- Around line 270-276: The current check only rejects perm_fee when it's Some
and greater than zero, allowing Some(0); change the validation on tx.perm_fee
used for term-only ledger transactions to reject any presence of perm_fee (i.e.,
treat any Some(_) as an error) and return TxIngressError::FundMisalignment with
the same message; update the condition that references tx.perm_fee (and the
error path that constructs TxIngressError::FundMisalignment) so term-only ledger
transactions fail if perm_fee.is_some().
- Around line 41-53: The cascade activation check in the cascade_active
computation uses the current tip height (entry.height()) but ingress should
validate against the next block height; update the check inside the block where
cascade_active is computed (the block reading self.block_tree_read_guard,
calling tree.get_canonical_chain and mapping over canonical.last()) to call
is_cascade_active with the next height (e.g., entry.height() + 1 or otherwise
compute next_block_height) instead of entry.height(); ensure you only change the
argument passed to is_cascade_active and keep the surrounding logic (canonical
chain retrieval and unwrap_or(false)) intact.
In `@crates/actors/tests/epoch_snapshot_tests.rs`:
- Around line 373-374: The call to
EpochSnapshot::get_num_capacity_partitions(..., 4) uses a magic literal for
num_active_ledgers; replace the hardcoded 4 with a well-named constant (e.g.,
NUM_ACTIVE_LEDGERS or DEFAULT_NUM_ACTIVE_LEDGERS) or add an inline comment
explaining that 4 corresponds to Publish, Submit, OneYear, ThirtyDay so the
intent is clear; update the invocation of get_num_capacity_partitions and any
related tests to use the constant or include the explanatory comment near the
capacity_count calculation (referencing
EpochSnapshot::get_num_capacity_partitions, capacity_count,
data_partition_count, and config).
In `@crates/api-server/src/routes/price.rs`:
- Around line 156-169: The cascade availability check currently calls
is_cascade_active using the current tip height (computed from
state.block_tree.read() -> get_canonical_chain -> tip_height); change it to
check the next block height instead (e.g., compute next_block_height =
tip_height + 1 or use any existing next_block_height variable) and call
state.config.consensus.hardforks.is_cascade_active(next_block_height) so
OneYear/ThirtyDay pricing is gated based on the upcoming block rather than the
current tip.
In `@crates/chain/tests/api/pricing_endpoint.rs`:
- Line 31: Replace hardcoded 0 passed as the quote height in expected-fee
calculations with the actual dynamic height from the test's quote/block used in
the test; update calls to calculate_perm_fee_from_config (and any expected_fee
construction) to use the quote's current height value (e.g., quote.height() or
the test variable that holds the quote/block height) so the assertions are
height-aware across cases mentioned (lines where 0 is used and the related
calculate_perm_fee_from_config calls).
In `@crates/database/src/data_ledger.rs`:
- Around line 299-318: activate_cascade currently returns early if any
TermLedger with ledger_id == DataLedger::OneYear exists, which leaves the pair
inconsistent if DataLedger::ThirtyDay is missing; change the logic to consult
config.hardforks.cascade.as_ref() and then individually check for the presence
of DataLedger::OneYear and DataLedger::ThirtyDay in self.term (e.g., using
term.iter().any(...) against each DataLedger variant) and push a new TermLedger
for only the missing variants via TermLedger::new(..., config,
cascade.one_year_epoch_length) and TermLedger::new(..., config,
cascade.thirty_day_epoch_length) so you never duplicate existing entries but
ensure both ledgers are present.
In `@crates/types/src/block.rs`:
- Around line 1928-1992: The tests
test_block_index_item_roundtrip_with_four_ledgers and
test_block_index_item_roundtrip_with_two_ledgers currently only assert
counts/totals/tx_roots but not the LedgerIndexItem.ledger discriminant; add
assertions that restored.ledgers[i].ledger == item.ledgers[i].ledger for each
ledger index (use the LedgerIndexItem.ledger field) to ensure ledger identity is
correctly decoded after to_bytes()/from_bytes().
In `@crates/types/src/config/consensus.rs`:
- Around line 499-512: In compute_cost_per_chunk_per_epoch, guard against
division-by-zero: check chunk_size != 0 before computing chunks_per_gb
(BYTES_PER_GB / chunk_size) and check epochs_per_year != 0 before dividing
cost_per_chunk_per_year by Decimal::from(epochs_per_year); if either is zero,
return an Err result with a descriptive error (e.g., InvalidInput or a new error
variant) instead of letting the raw division panic so the function's
Result/Error semantics are preserved.
In `@crates/types/src/config/mod.rs`:
- Around line 149-153: The validation for cascade.activation_height can panic
due to a modulo-by-zero when self.consensus.epoch.num_blocks_in_epoch == 0;
update Config::validate (the block that checks cascade.activation_height in
mod.rs) to first ensure num_blocks_in_epoch != 0 (using ensure! like the earlier
epoch checks) before performing the modulo operation, and return a clear error
if it is zero so the function returns an error instead of panicking.
---
Outside diff comments:
In `@crates/actors/src/block_validation.rs`:
- Around line 1618-1665: The variable cascade_active is computed twice for the
same block height; remove the inner recomputation inside the expired_ledger_fees
block and reuse the outer cascade_active (declared with let cascade_active =
config.consensus.hardforks.is_cascade_active(block.height);) in the subsequent
if check that iterates over DataLedger::OneYear and DataLedger::ThirtyDay, so
simply replace the inner let cascade_active = ... with using the existing
cascade_active variable.
In `@crates/chain/tests/validation/data_tx_pricing.rs`:
- Around line 385-400: The test is passing a hardcoded height of 0 into
calculate_term_fee_from_config and calculate_perm_fee_from_config, which makes
fee expectations insensitive to Cascade height; change those calls to use the
scenario's captured pricing height (the same height used elsewhere in the test,
e.g., pricing_block_height or the variable that records the pricing block
height) instead of 0 so the expected_term_fee and expected_perm_fee reflect
height-aware pricing; apply the same replacement in the other test block
slow_heavy_same_block_promoted_tx_with_ema_price_change_gets_accepted as well.
ℹ️ Review info
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (37)
crates/actors/src/block_migration_service.rscrates/actors/src/block_producer.rscrates/actors/src/block_producer/ledger_expiry.rscrates/actors/src/block_validation.rscrates/actors/src/mempool_service.rscrates/actors/src/mempool_service/data_txs.rscrates/actors/src/mempool_service/lifecycle.rscrates/actors/tests/epoch_snapshot_tests.rscrates/api-server/src/routes/price.rscrates/chain/src/chain.rscrates/chain/tests/api/hardfork_tests.rscrates/chain/tests/api/pricing_endpoint.rscrates/chain/tests/block_production/block_production.rscrates/chain/tests/data_sync/sync_partition_data_tests.rscrates/chain/tests/multi_node/api_ingress_validation.rscrates/chain/tests/multi_node/cascade_reorg.rscrates/chain/tests/multi_node/mod.rscrates/chain/tests/multi_node/validation.rscrates/chain/tests/utils.rscrates/chain/tests/validation/cascade_block_rejection.rscrates/chain/tests/validation/cascade_ledger_shape.rscrates/chain/tests/validation/cascade_term_expiry.rscrates/chain/tests/validation/data_tx_pricing.rscrates/chain/tests/validation/ingress_proof_validation.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/config/src/chain/chainspec.rscrates/database/src/data_ledger.rscrates/domain/src/models/block_index.rscrates/domain/src/snapshots/epoch_snapshot/epoch_snapshot.rscrates/types/src/block.rscrates/types/src/config/consensus.rscrates/types/src/config/mod.rscrates/types/src/hardfork_config.rscrates/types/src/storage_pricing.rs
Comment on lines
+641
to
+643
| self.user_perm_fee_refunds | ||
| .extend(other.user_perm_fee_refunds); | ||
| } |
Contributor
There was a problem hiding this comment.
Preserve user_perm_fee_refunds sort invariant in merge().
Line 641-Line 643 appends refunds without reordering, but the struct contract says this list is sorted by tx ID. Merging two deltas can break that invariant.
Proposed fix
self.user_perm_fee_refunds
.extend(other.user_perm_fee_refunds);
+ self.user_perm_fee_refunds
+ .sort_unstable_by_key(|(tx_id, _, _)| *tx_id);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| self.user_perm_fee_refunds | |
| .extend(other.user_perm_fee_refunds); | |
| } | |
| self.user_perm_fee_refunds | |
| .extend(other.user_perm_fee_refunds); | |
| self.user_perm_fee_refunds | |
| .sort_unstable_by_key(|(tx_id, _, _)| *tx_id); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/actors/src/block_producer/ledger_expiry.rs` around lines 641 - 643, In
the merge() method, appending other.user_perm_fee_refunds with extend() breaks
the struct's invariant that user_perm_fee_refunds is sorted by tx ID; replace
the extend() call with a merge of the two sorted sequences
(self.user_perm_fee_refunds and other.user_perm_fee_refunds) into a new Vec,
writing back the merged, sorted Vec to self.user_perm_fee_refunds (or perform an
in-place two-pointer merge) so the resulting list preserves the tx-ID ordering
declared by the struct contract.
Comment on lines
+933
to
+961
| fn test_ledger_expiry_balance_delta_merge_combines_rewards() { | ||
| use crate::shadow_tx_generator::RollingHash; | ||
|
|
||
| let addr1 = IrysAddress::random(); | ||
| let addr2 = IrysAddress::random(); | ||
|
|
||
| let mut delta1 = LedgerExpiryBalanceDelta::default(); | ||
| delta1 | ||
| .reward_balance_increment | ||
| .insert(addr1, (U256::from(100), RollingHash(U256::from(1)))); | ||
|
|
||
| let mut delta2 = LedgerExpiryBalanceDelta::default(); | ||
| delta2 | ||
| .reward_balance_increment | ||
| .insert(addr1, (U256::from(50), RollingHash(U256::from(2)))); | ||
| delta2 | ||
| .reward_balance_increment | ||
| .insert(addr2, (U256::from(200), RollingHash(U256::from(3)))); | ||
|
|
||
| delta1.merge(delta2); | ||
|
|
||
| // addr1: fees are summed | ||
| let (fee, _) = delta1.reward_balance_increment.get(&addr1).unwrap(); | ||
| assert_eq!(*fee, U256::from(150)); | ||
|
|
||
| // addr2: added from delta2 | ||
| let (fee, _) = delta1.reward_balance_increment.get(&addr2).unwrap(); | ||
| assert_eq!(*fee, U256::from(200)); | ||
| } |
Contributor
There was a problem hiding this comment.
Add a rolling-hash assertion to cover the new merge logic.
This test validates fee aggregation but not the XOR hash merge done on Line 637. A regression there would currently pass tests.
Proposed test addition
// addr1: fees are summed
- let (fee, _) = delta1.reward_balance_increment.get(&addr1).unwrap();
+ let (fee, hash) = delta1.reward_balance_increment.get(&addr1).unwrap();
assert_eq!(*fee, U256::from(150));
+ assert_eq!(hash.0, U256::from(1) ^ U256::from(2));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/actors/src/block_producer/ledger_expiry.rs` around lines 933 - 961,
The test currently only checks fee sums but not the XOR merge of rolling hashes;
after calling LedgerExpiryBalanceDelta::merge on delta1, assert that the rolling
hash for addr1 equals RollingHash(U256::from(1) ^ U256::from(2)) and that
addr2's rolling hash equals RollingHash(U256::from(3)); locate the tuples in
delta1.reward_balance_increment for addr1 and addr2 (the stored values are
(U256, RollingHash)) and compare the second element to the expected RollingHash
to cover the merge logic in LedgerExpiryBalanceDelta::merge.
| &ctx.node_ctx.config.consensus, | ||
| number_of_ingress_proofs_total, | ||
| ctx.node_ctx.config.consensus.genesis.genesis_price, | ||
| 0, |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use dynamic quote height instead of hardcoded 0 in expected-fee calculations.
These tests now target height-aware pricing; using a fixed height reduces assertion fidelity and can mask boundary regressions.
Suggested pattern
- let expected_term_fee = calculate_term_fee_from_config(
+ let quote_height = ctx.get_canonical_chain_height().await + 1;
+ let expected_term_fee = calculate_term_fee_from_config(
data_size_bytes,
&ctx.node_ctx.config.consensus,
number_of_ingress_proofs_total,
ctx.node_ctx.config.consensus.genesis.genesis_price,
- 0,
+ quote_height,
)?;Apply the same pattern to corresponding calculate_perm_fee_from_config calls.
Also applies to: 41-41, 125-125, 249-249, 465-465, 473-473, 521-521, 529-529, 664-664, 673-673
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/chain/tests/api/pricing_endpoint.rs` at line 31, Replace hardcoded 0
passed as the quote height in expected-fee calculations with the actual dynamic
height from the test's quote/block used in the test; update calls to
calculate_perm_fee_from_config (and any expected_fee construction) to use the
quote's current height value (e.g., quote.height() or the test variable that
holds the quote/block height) so the assertions are height-aware across cases
mentioned (lines where 0 is used and the related calculate_perm_fee_from_config
calls).
Comment on lines
+299
to
318
| pub fn activate_cascade(&mut self, config: &ConsensusConfig) { | ||
| if self | ||
| .term | ||
| .iter() | ||
| .any(|t| t.ledger_id == DataLedger::OneYear as u32) | ||
| { | ||
| return; // already activated | ||
| } | ||
| if let Some(cascade) = config.hardforks.cascade.as_ref() { | ||
| self.term.push(TermLedger::new( | ||
| DataLedger::OneYear, | ||
| config, | ||
| cascade.one_year_epoch_length, | ||
| )); | ||
| self.term.push(TermLedger::new( | ||
| DataLedger::ThirtyDay, | ||
| config, | ||
| cascade.thirty_day_epoch_length, | ||
| )); | ||
| } |
Contributor
There was a problem hiding this comment.
activate_cascade should reconcile partial state, not only guard on OneYear.
If OneYear exists but ThirtyDay is missing, current early return leaves an inconsistent active-ledger set.
Suggested fix
pub fn activate_cascade(&mut self, config: &ConsensusConfig) {
- if self
- .term
- .iter()
- .any(|t| t.ledger_id == DataLedger::OneYear as u32)
- {
- return; // already activated
- }
+ let has_one_year = self
+ .term
+ .iter()
+ .any(|t| t.ledger_id == DataLedger::OneYear as u32);
+ let has_thirty_day = self
+ .term
+ .iter()
+ .any(|t| t.ledger_id == DataLedger::ThirtyDay as u32);
+
+ if has_one_year && has_thirty_day {
+ return;
+ }
+
if let Some(cascade) = config.hardforks.cascade.as_ref() {
- self.term.push(TermLedger::new(
- DataLedger::OneYear,
- config,
- cascade.one_year_epoch_length,
- ));
- self.term.push(TermLedger::new(
- DataLedger::ThirtyDay,
- config,
- cascade.thirty_day_epoch_length,
- ));
+ if !has_one_year {
+ self.term.push(TermLedger::new(
+ DataLedger::OneYear,
+ config,
+ cascade.one_year_epoch_length,
+ ));
+ }
+ if !has_thirty_day {
+ self.term.push(TermLedger::new(
+ DataLedger::ThirtyDay,
+ config,
+ cascade.thirty_day_epoch_length,
+ ));
+ }
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| pub fn activate_cascade(&mut self, config: &ConsensusConfig) { | |
| if self | |
| .term | |
| .iter() | |
| .any(|t| t.ledger_id == DataLedger::OneYear as u32) | |
| { | |
| return; // already activated | |
| } | |
| if let Some(cascade) = config.hardforks.cascade.as_ref() { | |
| self.term.push(TermLedger::new( | |
| DataLedger::OneYear, | |
| config, | |
| cascade.one_year_epoch_length, | |
| )); | |
| self.term.push(TermLedger::new( | |
| DataLedger::ThirtyDay, | |
| config, | |
| cascade.thirty_day_epoch_length, | |
| )); | |
| } | |
| pub fn activate_cascade(&mut self, config: &ConsensusConfig) { | |
| let has_one_year = self | |
| .term | |
| .iter() | |
| .any(|t| t.ledger_id == DataLedger::OneYear as u32); | |
| let has_thirty_day = self | |
| .term | |
| .iter() | |
| .any(|t| t.ledger_id == DataLedger::ThirtyDay as u32); | |
| if has_one_year && has_thirty_day { | |
| return; | |
| } | |
| if let Some(cascade) = config.hardforks.cascade.as_ref() { | |
| if !has_one_year { | |
| self.term.push(TermLedger::new( | |
| DataLedger::OneYear, | |
| config, | |
| cascade.one_year_epoch_length, | |
| )); | |
| } | |
| if !has_thirty_day { | |
| self.term.push(TermLedger::new( | |
| DataLedger::ThirtyDay, | |
| config, | |
| cascade.thirty_day_epoch_length, | |
| )); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/database/src/data_ledger.rs` around lines 299 - 318, activate_cascade
currently returns early if any TermLedger with ledger_id == DataLedger::OneYear
exists, which leaves the pair inconsistent if DataLedger::ThirtyDay is missing;
change the logic to consult config.hardforks.cascade.as_ref() and then
individually check for the presence of DataLedger::OneYear and
DataLedger::ThirtyDay in self.term (e.g., using term.iter().any(...) against
each DataLedger variant) and push a new TermLedger for only the missing variants
via TermLedger::new(..., config, cascade.one_year_epoch_length) and
TermLedger::new(..., config, cascade.thirty_day_epoch_length) so you never
duplicate existing entries but ensure both ledgers are present.
Comment on lines
+1928
to
+1992
| fn test_block_index_item_roundtrip_with_four_ledgers() { | ||
| let item = BlockIndexItem { | ||
| block_hash: H256::random(), | ||
| num_ledgers: 4, | ||
| ledgers: vec![ | ||
| LedgerIndexItem { | ||
| total_chunks: 100, | ||
| tx_root: H256::random(), | ||
| ledger: DataLedger::Publish, | ||
| }, | ||
| LedgerIndexItem { | ||
| total_chunks: 200, | ||
| tx_root: H256::random(), | ||
| ledger: DataLedger::Submit, | ||
| }, | ||
| LedgerIndexItem { | ||
| total_chunks: 300, | ||
| tx_root: H256::random(), | ||
| ledger: DataLedger::OneYear, | ||
| }, | ||
| LedgerIndexItem { | ||
| total_chunks: 400, | ||
| tx_root: H256::random(), | ||
| ledger: DataLedger::ThirtyDay, | ||
| }, | ||
| ], | ||
| }; | ||
| let bytes = item.to_bytes(); | ||
| let restored = BlockIndexItem::from_bytes(&bytes); | ||
| assert_eq!(restored.num_ledgers, 4); | ||
| assert_eq!(restored.block_hash, item.block_hash); | ||
| assert_eq!(restored.ledgers.len(), 4); | ||
| assert_eq!(restored.ledgers[0].total_chunks, 100); | ||
| assert_eq!(restored.ledgers[0].tx_root, item.ledgers[0].tx_root); | ||
| assert_eq!(restored.ledgers[1].total_chunks, 200); | ||
| assert_eq!(restored.ledgers[2].total_chunks, 300); | ||
| assert_eq!(restored.ledgers[3].total_chunks, 400); | ||
| assert_eq!(restored.ledgers[3].tx_root, item.ledgers[3].tx_root); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_block_index_item_roundtrip_with_two_ledgers() { | ||
| let item = BlockIndexItem { | ||
| block_hash: H256::random(), | ||
| num_ledgers: 2, | ||
| ledgers: vec![ | ||
| LedgerIndexItem { | ||
| total_chunks: 100, | ||
| tx_root: H256::random(), | ||
| ledger: DataLedger::Publish, | ||
| }, | ||
| LedgerIndexItem { | ||
| total_chunks: 200, | ||
| tx_root: H256::random(), | ||
| ledger: DataLedger::Submit, | ||
| }, | ||
| ], | ||
| }; | ||
| let bytes = item.to_bytes(); | ||
| let restored = BlockIndexItem::from_bytes(&bytes); | ||
| assert_eq!(restored.num_ledgers, 2); | ||
| assert_eq!(restored.ledgers.len(), 2); | ||
| assert_eq!(restored.ledgers[0].total_chunks, 100); | ||
| assert_eq!(restored.ledgers[1].total_chunks, 200); | ||
| } |
Contributor
There was a problem hiding this comment.
Roundtrip tests should assert decoded ledger identities explicitly.
Line 1956 onward verifies totals/roots, but not ledger tags. With non-contiguous discriminants, position-only assertions can hide decode bugs.
✅ Suggested test hardening
fn test_block_index_item_roundtrip_with_four_ledgers() {
let item = BlockIndexItem {
block_hash: H256::random(),
num_ledgers: 4,
@@
let restored = BlockIndexItem::from_bytes(&bytes);
assert_eq!(restored.num_ledgers, 4);
assert_eq!(restored.block_hash, item.block_hash);
assert_eq!(restored.ledgers.len(), 4);
+ assert_eq!(restored.ledgers[0].ledger, DataLedger::Publish);
+ assert_eq!(restored.ledgers[1].ledger, DataLedger::Submit);
+ assert_eq!(restored.ledgers[2].ledger, DataLedger::OneYear);
+ assert_eq!(restored.ledgers[3].ledger, DataLedger::ThirtyDay);
assert_eq!(restored.ledgers[0].total_chunks, 100);
@@
fn test_block_index_item_roundtrip_with_two_ledgers() {
@@
let restored = BlockIndexItem::from_bytes(&bytes);
assert_eq!(restored.num_ledgers, 2);
assert_eq!(restored.ledgers.len(), 2);
+ assert_eq!(restored.ledgers[0].ledger, DataLedger::Publish);
+ assert_eq!(restored.ledgers[1].ledger, DataLedger::Submit);
assert_eq!(restored.ledgers[0].total_chunks, 100);
assert_eq!(restored.ledgers[1].total_chunks, 200);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/types/src/block.rs` around lines 1928 - 1992, The tests
test_block_index_item_roundtrip_with_four_ledgers and
test_block_index_item_roundtrip_with_two_ledgers currently only assert
counts/totals/tx_roots but not the LedgerIndexItem.ledger discriminant; add
assertions that restored.ledgers[i].ledger == item.ledgers[i].ledger for each
ledger index (use the LedgerIndexItem.ledger field) to ensure ledger identity is
correctly decoded after to_bytes()/from_bytes().
Comment on lines
+499
to
512
| fn compute_cost_per_chunk_per_epoch( | ||
| annual_cost_per_gb: Amount<(CostPerGb, Usd)>, | ||
| chunk_size: u64, | ||
| epochs_per_year: u64, | ||
| ) -> Result<Amount<(CostPerChunk, Usd)>> { | ||
| const BYTES_PER_GB: u64 = 1024 * 1024 * 1024; | ||
| let chunks_per_gb = BYTES_PER_GB / self.chunk_size; | ||
| let epochs_per_year = self.epochs_per_year(); | ||
| let chunks_per_gb = BYTES_PER_GB / chunk_size; | ||
|
|
||
| // Convert annual_cost_per_gb to cost_per_chunk_per_epoch | ||
| // annual_cost_per_gb / chunks_per_gb / epochs_per_year | ||
| let annual_decimal = self.annual_cost_per_gb.token_to_decimal()?; | ||
| let annual_decimal = annual_cost_per_gb.token_to_decimal()?; | ||
| let cost_per_chunk_per_year = annual_decimal / Decimal::from(chunks_per_gb); | ||
| let cost_per_chunk_per_epoch = cost_per_chunk_per_year / Decimal::from(epochs_per_year); | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the actual code at the specified lines
cat -n crates/types/src/config/consensus.rs | sed -n '495,520p'Repository: Irys-xyz/irys
Length of output: 1184
🏁 Script executed:
# Check the broader context of ConsensusConfig to see if parameters are validated at construction
grep -n "epochs_per_year\|chunk_size" crates/types/src/config/consensus.rs | head -30Repository: Irys-xyz/irys
Length of output: 930
🏁 Script executed:
# Look for where compute_cost_per_chunk_per_epoch is called
rg "compute_cost_per_chunk_per_epoch" --type rs -B 3 -A 3Repository: Irys-xyz/irys
Length of output: 83
🏁 Script executed:
# Check the test coverage for this function
sed -n '972,1065p' crates/types/src/config/consensus.rs | cat -nRepository: Irys-xyz/irys
Length of output: 3877
🏁 Script executed:
# Check if ConsensusConfig has validation in constructors/new methods
sed -n '50,150p' crates/types/src/config/consensus.rs | cat -nRepository: Irys-xyz/irys
Length of output: 4696
🏁 Script executed:
# Look for any validation or constraints on chunk_size and epochs_per_year
rg "chunk_size.*0|epochs_per_year.*0" --type rust crates/types/src/config/consensus.rsRepository: Irys-xyz/irys
Length of output: 102
🏁 Script executed:
# Check the testing() configuration specifically
rg -A 20 "fn testing\(\)" crates/types/src/config/consensus.rsRepository: Irys-xyz/irys
Length of output: 962
🏁 Script executed:
# Verify if Decimal division behavior - check what happens with division by zero
rg "Decimal.*division|divide.*zero" crates/types/src/config/consensus.rsRepository: Irys-xyz/irys
Length of output: 39
🏁 Script executed:
# Check if there's any validation or assertion in ConsensusConfig construction
rg "chunk_size|epochs_per_year" crates/types/src/config/consensus.rs -B 2 -A 2 | head -60Repository: Irys-xyz/irys
Length of output: 2410
🏁 Script executed:
# Look for any panic! or assert! statements in compute_cost_per_chunk_per_epoch or nearby
sed -n '480,520p' crates/types/src/config/consensus.rs | cat -nRepository: Irys-xyz/irys
Length of output: 2186
🏁 Script executed:
# Check if Amount::token() or similar methods have validation that could fail
rg "impl.*Amount|fn token" crates/types/src -A 10 | grep -A 10 "token\|Amount"Repository: Irys-xyz/irys
Length of output: 10831
🏁 Script executed:
# Check epochs_per_year implementation to see if it can return 0
rg -A 10 "fn epochs_per_year" crates/types/src/config/consensus.rsRepository: Irys-xyz/irys
Length of output: 485
🏁 Script executed:
# Check if there's validation in EpochConfig or difficulty_adjustment that ensures non-zero values
rg "num_blocks_in_epoch|block_time" crates/types/src/config/consensus.rs -B 2 -A 2Repository: Irys-xyz/irys
Length of output: 2108
🏁 Script executed:
# Look at the full context of where chunk_size and epochs_per_year can be set or modified
rg "pub struct ConsensusConfig" -A 50 crates/types/src/config/consensus.rs | head -70Repository: Irys-xyz/irys
Length of output: 1915
🏁 Script executed:
# Check if there's a builder or validation function for ConsensusConfig
rg "impl.*ConsensusConfig|fn new\|fn with_" crates/types/src/config/consensus.rsRepository: Irys-xyz/irys
Length of output: 79
🏁 Script executed:
# Check what Decimal division behavior is - does it panic on divide by zero?
rg "rust_decimal" crates/Cargo.toml -A 2 -B 2Repository: Irys-xyz/irys
Length of output: 114
🏁 Script executed:
# Find Cargo.toml files and check rust_decimal dependency
find . -name "Cargo.toml" -type f | head -5Repository: Irys-xyz/irys
Length of output: 180
🏁 Script executed:
# Check the root Cargo.toml or workspace Cargo.toml
fd Cargo.toml -x cat {} \; | grep -A 2 "rust_decimal"Repository: Irys-xyz/irys
Length of output: 1097
🏁 Script executed:
# Check Decimal division behavior - look at rust_decimal documentation or tests
rg "Decimal.*/" --type rust crates/types/src -A 2 -B 2 | head -40Repository: Irys-xyz/irys
Length of output: 465
🏁 Script executed:
# Check how rust_decimal handles division by zero
web_search "rust_decimal division by zero panic"Repository: Irys-xyz/irys
Length of output: 107
🏁 Script executed:
# Check if there are any panic tests or explicit handling of division by zero in the codebase
rg "divide.*zero|division.*zero|panic.*divide" --type rust crates/types/src -A 3 -B 3Repository: Irys-xyz/irys
Length of output: 713
🏁 Script executed:
# Look at the actual usage of Decimal division in the codebase to see if there's panic handling
rg "Decimal::from.*/" --type rust crates/types/src -B 2 -A 2 | head -50Repository: Irys-xyz/irys
Length of output: 39
🏁 Script executed:
# Verify the actual parameter values passed - check if chunk_size and epochs_per_year are ALWAYS non-zero
grep -r "chunk_size.*0\|epochs_per_year.*0" crates/types/src/config/consensus.rsRepository: Irys-xyz/irys
Length of output: 102
🏁 Script executed:
# Check if rust_decimal Divide operation panics on zero divisor
cat crates/Cargo.toml | grep -A 5 "rust_decimal"Repository: Irys-xyz/irys
Length of output: 106
🏁 Script executed:
# Search for any tests that might test zero division scenarios
rg "compute_cost_per_chunk_per_epoch|cost_per_chunk_per_epoch" crates/types/src/config/consensus.rs -B 5 -A 15 | tail -100Repository: Irys-xyz/irys
Length of output: 4189
🏁 Script executed:
# Check if there's any validation at ConsensusConfig construction or in accessors
sed -n '1,100p' crates/types/src/config/consensus.rs | cat -nRepository: Irys-xyz/irys
Length of output: 4376
🏁 Script executed:
# Check if ConsensusConfig fields are mutable after construction or if there's a builder pattern
rg "pub fn new\|Builder\|impl.*ConsensusConfig" crates/types/src/config/consensus.rs | head -20Repository: Irys-xyz/irys
Length of output: 39
🏁 Script executed:
# Verify rust_decimal Divide operator behavior with zero
cat Cargo.toml | grep -C 3 "workspace"Repository: Irys-xyz/irys
Length of output: 1408
🏁 Script executed:
# Test if Decimal division panics - check the actual implementation or behavior hints
rg "impl.*Div.*Decimal|fn div" crates -A 5 | head -40Repository: Irys-xyz/irys
Length of output: 39
🏁 Script executed:
# Check if rust_decimal Divide implementation panics on zero
python3 << 'EOF'
import subprocess
result = subprocess.run(['rg', 'rust_decimal.*=.*1', 'Cargo.lock'], capture_output=True, text=True)
print("Cargo.lock output:", result.stdout[:500])
EOFRepository: Irys-xyz/irys
Length of output: 76
🏁 Script executed:
# Check actual behavior: Does rust_decimal division panic or return error?
# Look for any documentation or tests that show the behavior
rg "Divide|division" crates/types/src -A 3 | grep -i "panic\|error\|zero" | head -20Repository: Irys-xyz/irys
Length of output: 276
🏁 Script executed:
# Verify if ConsensusConfig can ever have chunk_size or epochs_per_year as zero
# by checking all places where they're set
rg "chunk_size.*=" crates/types/src/config/consensus.rsRepository: Irys-xyz/irys
Length of output: 90
🏁 Script executed:
# Check the exact behavior of Decimal division - look at rust_decimal docs online
python3 << 'EOF'
# Since rust_decimal is a third-party crate, let's check if there are any examples
# of Decimal division in the codebase and how they handle edge cases
import subprocess
result = subprocess.run(['rg', 'Decimal::from.*/', '--type', 'rust', 'crates/types/src'],
capture_output=True, text=True)
lines = result.stdout.split('\n')
for line in lines[:30]:
print(line)
EOFRepository: Irys-xyz/irys
Length of output: 39
🏁 Script executed:
# Check if there are any Decimal division tests that show expected behavior
rg "Decimal.*test|#\[test\]" crates -A 30 | grep -B 5 -A 10 "Divide\|divide\|/" | head -50Repository: Irys-xyz/irys
Length of output: 3586
🏁 Script executed:
# Look for any rust_decimal documentation or comments in the codebase
rg "rust_decimal" crates -B 2 -A 2 --type rust | grep -i "divide\|zero\|panic" | head -20Repository: Irys-xyz/irys
Length of output: 39
🏁 Script executed:
# Final check: look at the actual lines in context to see if there's any error handling I missed
sed -n '499,514p' crates/types/src/config/consensus.rsRepository: Irys-xyz/irys
Length of output: 799
compute_cost_per_chunk_per_epoch can panic on division by zero despite returning Result.
Line 505 will panic if chunk_size == 0 (raw u64 division). Line 511 will panic if epochs_per_year == 0 (Decimal division). The function signature returns Result, implying these should be handled as errors instead.
💡 Proposed fix
fn compute_cost_per_chunk_per_epoch(
annual_cost_per_gb: Amount<(CostPerGb, Usd)>,
chunk_size: u64,
epochs_per_year: u64,
) -> Result<Amount<(CostPerChunk, Usd)>> {
const BYTES_PER_GB: u64 = 1024 * 1024 * 1024;
+ if chunk_size == 0 || chunk_size > BYTES_PER_GB {
+ return Err(eyre::eyre!(
+ "chunk_size ({chunk_size}) must be in 1..={BYTES_PER_GB}"
+ ));
+ }
+ if epochs_per_year == 0 {
+ return Err(eyre::eyre!("epochs_per_year must be > 0"));
+ }
let chunks_per_gb = BYTES_PER_GB / chunk_size;
// Convert annual_cost_per_gb to cost_per_chunk_per_epoch
// annual_cost_per_gb / chunks_per_gb / epochs_per_year
let annual_decimal = annual_cost_per_gb.token_to_decimal()?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/types/src/config/consensus.rs` around lines 499 - 512, In
compute_cost_per_chunk_per_epoch, guard against division-by-zero: check
chunk_size != 0 before computing chunks_per_gb (BYTES_PER_GB / chunk_size) and
check epochs_per_year != 0 before dividing cost_per_chunk_per_year by
Decimal::from(epochs_per_year); if either is zero, return an Err result with a
descriptive error (e.g., InvalidInput or a new error variant) instead of letting
the raw division panic so the function's Result/Error semantics are preserved.
Collaborator
Author
There was a problem hiding this comment.
yes, both of those should cause a panic as they represent irrecoverable errors.
Contributor
There was a problem hiding this comment.
@DanMacDonald, understood! I've noted that chunk_size and epochs_per_year are configuration invariants—if they're ever zero, the system is in an invalid state and should panic rather than propagate an error. This aligns with the principle that invariant violations should fail fast.
✏️ Learnings added
Learnt from: DanMacDonald
Repo: Irys-xyz/irys PR: 1166
File: crates/types/src/config/consensus.rs:499-512
Timestamp: 2026-02-26T22:00:11.666Z
Learning: In `crates/types/src/config/consensus.rs`, the `compute_cost_per_chunk_per_epoch` function should panic on division by zero (when `chunk_size == 0` or `epochs_per_year == 0`) rather than return an error, as these represent irrecoverable configuration invariant violations.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: glottologist
Repo: Irys-xyz/irys PR: 1090
File: crates/actors/src/supply_state_calculator.rs:121-130
Timestamp: 2026-01-20T13:27:23.880Z
Learning: In the supply state backfill logic (crates/actors/src/supply_state_calculator.rs), if the backfill process encounters failures such as batch count mismatches, the code should panic rather than return an error, as these represent critical invariant violations.
Learnt from: DanMacDonald
Repo: Irys-xyz/irys PR: 1104
File: crates/actors/src/mempool_service/data_txs.rs:31-37
Timestamp: 2026-01-24T02:18:04.459Z
Learning: In `crates/actors/src/mempool_service/data_txs.rs`, the precheck for DataLedger intentionally rejects OneYear and ThirtyDay ledgers during the staged rollout. Downstream match arms for these ledgers are placeholders for future implementation and should not be flagged as unreachable during this development phase.
roberts-pumpurs
requested changes
Feb 27, 2026
| drop(tree); | ||
|
|
||
| let next_block_height = tip_height + 1; | ||
| let epochs_for_storage = irys_types::ledger_expiry::calculate_submit_ledger_expiry( |
Contributor
There was a problem hiding this comment.
proposal: rename the function to calculate_term_ledger_expiry
- Switch Cascade activation from block height to timestamp (matching Borealis pattern) - Validate OneYear/ThirtyDay fees in block validation (add term_txs to data_txs_are_valid) - Remove silent unwrap_or defaults in block validation and pricing, return errors instead - Add term fee & EMA balance checks for OneYear/ThirtyDay in mempool selection - Add full validation in gossip path for term ledger txs - Add validate_term_ledger_expiry() for expires field validation - Reject Submit from pricing endpoint, consolidate shared pricing logic
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/chain/tests/validation/data_tx_pricing.rs (1)
381-390:⚠️ Potential issue | 🟡 MinorAvoid hardcoded
UnixTimestamp::from_secs(0)in pricing expectations.Using a fixed timestamp can drift from the block-time-dependent pricing path under hardfork-gated parameters. Derive the pricing timestamp from the block context used to build the transaction.
Also applies to: 393-400, 509-518, 523-530
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/chain/tests/validation/data_tx_pricing.rs` around lines 381 - 390, Replace hardcoded UnixTimestamp::from_secs(0) used when computing expected fees with the actual timestamp from the block context used to build the transaction; in places where you call calculate_term_fee_from_config (and compute number_of_ingress_proofs_total / expected_term_fee), pull the timestamp from the test's block/header context (e.g., the block or header timestamp used to construct the tx or block in this test) instead of using UnixTimestamp::from_secs(0) so pricing follows the block-time-dependent path under hardfork-gated config.crates/actors/src/block_producer.rs (1)
1428-1431:⚠️ Potential issue | 🟠 MajorSort OneYear/ThirtyDay mempool txs before block construction.
Line 1429 sorts only
submit_tx.one_year_txandthirty_day_txremain unsorted but feed tx roots/body ordering later, making block contents order-dependent on insertion order.💡 Proposed fix
mempool_txs .submit_tx .sort_by(irys_types::DataTransactionHeader::compare_tx); + mempool_txs + .one_year_tx + .sort_by(irys_types::DataTransactionHeader::compare_tx); + mempool_txs + .thirty_day_tx + .sort_by(irys_types::DataTransactionHeader::compare_tx); mempool_txs.commitment_tx.sort();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/actors/src/block_producer.rs` around lines 1428 - 1431, mempool_txs currently only sorts submit_tx but leaves one_year_tx and thirty_day_tx unsorted which makes block ordering depend on insertion order; before block construction, sort mempool_txs.one_year_tx and mempool_txs.thirty_day_tx with the same comparator used for submit_tx (irys_types::DataTransactionHeader::compare_tx) so that tx roots/body ordering is deterministic (similar to the existing submit_tx and commitment_tx sorts).
♻️ Duplicate comments (6)
crates/database/src/data_ledger.rs (1)
299-306:⚠️ Potential issue | 🟠 Major
activate_cascadecan leave ledgers partially activated.Early return keyed only on
OneYearmeans a state withOneYearpresent butThirtyDaymissing never self-heals. Reconcile each ledger independently.🩹 Suggested fix
pub fn activate_cascade(&mut self, config: &ConsensusConfig) { - if self - .term - .iter() - .any(|t| t.ledger_id == DataLedger::OneYear as u32) - { - return; // already activated - } + let has_one_year = self + .term + .iter() + .any(|t| t.ledger_id == DataLedger::OneYear as u32); + let has_thirty_day = self + .term + .iter() + .any(|t| t.ledger_id == DataLedger::ThirtyDay as u32); + + if has_one_year && has_thirty_day { + return; + } if let Some(cascade) = config.hardforks.cascade.as_ref() { - self.term.push(TermLedger::new( - DataLedger::OneYear, - config, - cascade.one_year_epoch_length, - )); - self.term.push(TermLedger::new( - DataLedger::ThirtyDay, - config, - cascade.thirty_day_epoch_length, - )); + if !has_one_year { + self.term.push(TermLedger::new( + DataLedger::OneYear, + config, + cascade.one_year_epoch_length, + )); + } + if !has_thirty_day { + self.term.push(TermLedger::new( + DataLedger::ThirtyDay, + config, + cascade.thirty_day_epoch_length, + )); + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/database/src/data_ledger.rs` around lines 299 - 306, activate_cascade currently returns early if the OneYear ledger exists which can leave other ledgers (e.g., ThirtyDay) uncreated; change activate_cascade (and use of self.term and ConsensusConfig) to reconcile each DataLedger independently instead of a single early return: for each required ledger variant (e.g., DataLedger::OneYear, DataLedger::ThirtyDay) check whether an entry with matching ledger_id exists in self.term and, if missing, perform the existing activation steps for that specific ledger using the same logic currently used for the OneYear path so all ledgers are created/healed individually.crates/api-server/src/routes/price.rs (1)
70-76:⚠️ Potential issue | 🟠 Major
/priceis using tip timestamp for next-block quotes (boundary lag).The endpoint quotes for the next includable block, but activation/pricing are computed from the current tip timestamp. Around activation boundaries, this can return stale 400s and stale fee quotes for OneYear/ThirtyDay and Publish pricing.
Suggested fix direction
- let latest_block_timestamp_secs = last_block_entry.header().timestamp_secs(); + let tip_timestamp_secs = last_block_entry.header().timestamp_secs(); + let quote_timestamp_secs = irys_types::UnixTimestamp::from_secs( + tip_timestamp_secs + .as_secs() + .saturating_add(state.config.consensus.difficulty_adjustment.block_time), + ); - let number_of_ingress_proofs_total = - state.config.number_of_ingress_proofs_total_at(latest_block_timestamp_secs); + let number_of_ingress_proofs_total = + state.config.number_of_ingress_proofs_total_at(quote_timestamp_secs); - if !state.config.consensus.hardforks.is_cascade_active_at(tip_timestamp) { ... } + if !state + .config + .consensus + .hardforks + .is_cascade_active_at(quote_timestamp_secs) + { ... } - calculate_term_fee(..., latest_block_timestamp_secs) + calculate_term_fee(..., quote_timestamp_secs) - calculate_perm_fee_from_config(..., latest_block_timestamp_secs) + calculate_perm_fee_from_config(..., quote_timestamp_secs)Also applies to: 104-117, 164-174, 204-241
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/api-server/src/routes/price.rs` around lines 70 - 76, The current logic uses latest_block_timestamp_secs (from last_block_entry) to compute activation/pricing for the next includable block (next_block_height), causing boundary lag; update the code paths that compute activation windows and fees (the blocks around latest_block_timestamp_secs / tip_height / next_block_height) to derive the timestamp for next_block_height instead of using tip timestamp — for example fetch or compute the header timestamp for next_block_height (or predict its activation timestamp consistently) before running OneYear/ThirtyDay/Publish pricing logic in the functions/blocks referenced by latest_block_timestamp_secs, tip_height, next_block_height (also apply the same change to the other ranges noted: lines 104-117, 164-174, 204-241) so pricing and 400 validations use the next-block timestamp rather than the current tip timestamp.crates/actors/src/block_validation.rs (1)
2163-2179:⚠️ Potential issue | 🟠 MajorTerm-ledger txs bypass historical inclusion checks.
txs_to_checkonly includes Publish/Submit (Line 2163), so the term-ledger branches in Step 4 are never exercised. Step 6 then validates term pricing only, which skips duplicate/past-inclusion enforcement for OneYear/ThirtyDay transactions.Suggested fix direction
let mut txs_to_check = publish_txs .iter() .map(|x| (x, DataLedger::Publish)) .chain(submit_txs.iter().map(|x| (x, DataLedger::Submit))) .map(|(tx, ledger_current)| { let state = if same_block_promotions.contains(&tx.id) { TxInclusionState::Found { ledger_current: (DataLedger::Publish, block.block_hash), ledger_historical: (DataLedger::Submit, block.block_hash), } } else { TxInclusionState::Searching { ledger_current } }; (tx.id, (tx, state)) }) .collect::<HashMap<_, _>>(); + for tx in term_txs { + let ledger_current = DataLedger::try_from(tx.ledger_id).map_err(|_| { + PreValidationError::LedgerIdInvalid { + ledger_id: tx.ledger_id, + } + })?; + txs_to_check.insert(tx.id, (tx, TxInclusionState::Searching { ledger_current })); + } - // Step 6: Validate term ledger transactions (OneYear/ThirtyDay) - for tx in term_txs { ... } + // Term-ledger txs are now validated by the shared Step 4 flow + // (historical inclusion + pricing).Also applies to: 2325-2427, 2470-2478
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/actors/src/block_validation.rs` around lines 2163 - 2179, txs_to_check currently only includes publish_txs and submit_txs so term-ledger transactions (e.g., OneYear/ThirtyDay) never hit the inclusion checks; update the construction of txs_to_check to also chain the term ledger iterators (the collections containing OneYear/ThirtyDay txs) and map them to the correct DataLedger variant(s) so they produce entries for term-ledger branches, and ensure the TxInclusionState logic (where same_block_promotions and ledger_current are set) handles term-ledger ids the same way as Publish/Submit (use the appropriate DataLedger enum values when initializing state for same_block_promotions and for Searching).crates/actors/src/mempool_service/data_txs.rs (2)
216-233:⚠️ Potential issue | 🟠 MajorReject any term-ledger
perm_feepresence (Some(_)) on both ingress paths.Lines 219-222 and 282-285 only reject
perm_fee > 0, soSome(0)is accepted even though term ledgers are documented as “must not carry a perm_fee”.💡 Proposed fix
- if tx - .perm_fee - .is_some_and(|f| f > irys_types::BoundedFee::zero()) - { + if tx.perm_fee.is_some() { return Err(TxIngressError::FundMisalignment( "Term-only ledger transactions must not have a perm_fee".to_string(), )); }- if tx - .perm_fee - .is_some_and(|f| f > irys_types::BoundedFee::zero()) - { + if tx.perm_fee.is_some() { return Err(TxIngressError::FundMisalignment( "Term-only ledger transactions must not have a perm_fee".to_string(), )); }Also applies to: 280-297
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/actors/src/mempool_service/data_txs.rs` around lines 216 - 233, The current checks in the DataLedger handling only reject perm_fee when it is Some(x) and x > 0, allowing Some(0); update both occurrences where perm_fee is inspected (the block matching DataLedger::OneYear | DataLedger::ThirtyDay and the analogous block later around the second ingress path) to treat any Some(_) as invalid by testing tx.perm_fee.is_some() (or equivalent) and returning TxIngressError::FundMisalignment with the same message, so term-ledger transactions reject any presence of perm_fee (including Some(0)).
40-57:⚠️ Potential issue | 🟠 MajorCascade ingress gating should be epoch-aware, not tip-timestamp-aware.
Line 52 checks Cascade activation from canonical tip timestamp. For epoch-aligned activation, this can admit OneYear/ThirtyDay txs before the boundary epoch is actually active.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/actors/src/mempool_service/data_txs.rs` around lines 40 - 57, The check gating OneYear/ThirtyDay ledgers uses the tip's timestamp via is_cascade_active_at(entry.header().timestamp_secs()), which can allow these ledgers before the new epoch actually activates; change the logic in the DataLedger branch that computes cascade_active (currently using block_tree_read_guard, get_canonical_chain and entry.header().timestamp_secs()) to determine cascade activation by epoch instead of tip timestamp — e.g. derive the tip's epoch (or the activation epoch) from the canonical tip header (use the header's epoch/epoch_index accessor) and call the hardforks API that checks activation by epoch (or compare against the configured cascade activation epoch) so that if cascade is not active at that epoch you return TxIngressError::InvalidLedger(tx.ledger_id) as before.crates/actors/src/mempool_service.rs (1)
1157-1194:⚠️ Potential issue | 🟡 MinorSelection summary still undercounts post-Cascade data txs.
Line 1170, Line 1180, and Line 1193 only use
submit_tx.len(), and Lines 1157-1166 sum fees only fromsubmit_tx.one_year_tx/thirty_day_txselections are omitted from telemetry and rejection-rate math.📌 Suggested patch
- let total_fee_collected: U256 = submit_tx + let selected_data_txs = submit_tx.len() + one_year_tx.len() + thirty_day_tx.len(); + let total_fee_collected: U256 = submit_tx .iter() + .chain(one_year_tx.iter()) + .chain(thirty_day_tx.iter()) .map(irys_types::IrysTransactionCommon::user_fee) .fold(U256::zero(), irys_types::U256::saturating_add) .saturating_add( commitment_tx .iter() @@ info!( mempool_selected.commitment_txs = commitment_tx.len(), - mempool_selected.data_txs = submit_tx.len(), + mempool_selected.data_txs = selected_data_txs, mempool_selected.publish_txs = publish_txs_and_proofs.txs.len(), mempool_selected.total_fee_collected = ?total_fee_collected, mempool_selected.unfunded_addresses = unfunded_address.len(), "Mempool transaction selection completed" ); @@ - let total_selected = commitment_tx.len() + submit_tx.len(); + let total_selected = commitment_tx.len() + selected_data_txs; @@ - mempool_selected.data_selected = submit_tx.len(), + mempool_selected.data_selected = selected_data_txs, mempool_selected.unfunded_addresses = unfunded_address.len(), "High transaction rejection rate detected" );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/actors/src/mempool_service.rs` around lines 1157 - 1194, The telemetry and rejection-rate math only account for submit_tx and omit downstream Cascade selections (one_year_tx and thirty_day_tx), so update the aggregation to include those vectors: compute a combined_data_txs count and sum fees into total_fee_collected by folding over submit_tx, one_year_tx, and thirty_day_tx (in addition to commitment_tx totals already present), then replace occurrences of submit_tx.len() in the info! and warn! calls (and in the total_selected calculation) with the combined_data_txs value; ensure mempool_selected.data_txs and mempool_selected.data_selected reflect the combined count and mempool_selected.total_fee_collected uses the summed total across all three data-tx collections.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/actors/src/block_producer.rs`:
- Around line 1074-1082: The Cascade enable check uses current_timestamp
(cascade activation can flip mid-epoch); change it to use the same epoch-aware
activation logic used elsewhere by replacing the current_timestamp comparison
against inner().config.consensus.hardforks.cascade.activation_timestamp with the
epoch-aware helper used in the file (the same activation check used at the other
Cascade usage), e.g., query the current epoch or call the existing
is_cascade_active_for_epoch/is_active_at_epoch helper and base the filter on
that result so ledger selection follows epoch-aligned Cascade activation (refer
to inner().config.consensus.hardforks.cascade and the epoch-aware activation
helper used elsewhere in this module).
In `@crates/actors/src/packing_service/guard.rs`:
- Around line 66-77: The test mutates the process-global panic hook
(take_hook/set_hook) unsafely; serialize access by introducing a test-only
global mutex (e.g., a once_cell::sync::Lazy<Mutex<()>> or lazy_static Mutex) and
acquire the lock around the entire sequence that calls std::panic::take_hook(),
std::panic::set_hook(Box::new(...)), the std::panic::catch_unwind block that
constructs ActiveWorkerGuard, and the final std::panic::set_hook(prev_hook)
restore so no two tests can interleave these calls; apply the same mutex-guard
pattern to the similar tests in commitment_v1.rs and commitment_v2.rs to ensure
correct restoration of the previous hook.
In `@crates/actors/src/validation_service/block_validation_task.rs`:
- Around line 494-510: The aggregation into term_txs (built from
sealed_block_for_data.transactions()) loses per-ledger context and can hide
invalid ledger_ids, leading to downstream unreachable!() in data_txs_are_valid;
before extending term_txs for irys_types::DataLedger::OneYear and
DataLedger::ThirtyDay, iterate each tx from txs.get_ledger_txs(...) and
assert/filter that tx.ledger_id matches the expected ledger (OneYear or
ThirtyDay) and fail fast (return an invalid-block error) if any tx has a
non-term ledger_id; alternatively preserve per-bucket context by passing the two
separate vectors (one_year_txs, thirty_day_txs) into data_txs_are_valid or
perform the ledger_id checks inside data_txs_are_valid, referencing the term_txs
variable, sealed_block_for_data.transactions(), the DataLedger enum, and
tx.ledger_id to locate the code to change.
In `@crates/chain/tests/api/pricing_endpoint.rs`:
- Around line 688-742: The test
heavy_cascade_pricing_endpoint_term_ledgers_pre_post_activation currently sets
Cascade.activation_timestamp to 0 so it only exercises post-activation behavior;
change the test to first use a non-zero/future activation timestamp (e.g.,
UnixTimestamp::from_secs(1000) or now()+N) to assert the pre-activation
responses for DataLedger::OneYear and DataLedger::ThirtyDay, then create a
second run/config with activation_timestamp = UnixTimestamp::from_secs(0) (or
otherwise advance the node time) to assert the post-activation responses and the
term_fee differences; update the Cascade.activation_timestamp in this function
(and the similar tests around the 826-899 range) and add the corresponding
pre-activation assertions before the post-activation checks so the
“pre_post”/transition behavior is actually validated.
In `@crates/chain/tests/multi_node/api_ingress_validation.rs`:
- Around line 454-459: The test
heavy_cascade_mempool_ingress_boundary_for_term_ledgers currently enables
Cascade at timestamp 0 but its docstring claims it also verifies pre-Cascade
rejection; update the test to make behavior consistent by either adding an
explicit pre-Cascade scenario or renaming to indicate post-Cascade-only. To add
the pre-Cascade case: before activating Cascade in
heavy_cascade_mempool_ingress_boundary_for_term_ledgers, run the same mempool
ingress assertions against OneYear/ThirtyDay term-ledger transactions expecting
InvalidLedger, then advance time/activate Cascade and re-run the post-Cascade
assertions; alternatively, if you prefer renaming, change the test name and
docstring to reflect it only validates post-Cascade behavior. Ensure references
to the Cascade activation call and the mempool ingress assertion helpers in the
test are updated accordingly.
In `@crates/chain/tests/multi_node/cascade_reorg.rs`:
- Around line 14-15: The variable activation_height is misleading because the
Cascade config uses timestamp-based activation (activation_timestamp:
UnixTimestamp::from_secs(0)), so rename activation_height to something like
pre_mine_blocks or blocks_to_mine_before_test and update any related comments
(e.g., the comment that currently reads "Mine past the Cascade activation
height") to reflect that you are simply mining a number of blocks to set up the
test rather than crossing a height-based activation; update occurrences in the
test (references to activation_height and the comment near the block-mining
logic) and ensure the new name makes it clear this controls how many blocks to
mine before running the scenario.
- Around line 165-170: Replace the panic-inducing .expect() on the canonical
chain with a clearer, non-panicking error path: capture the result of
genesis_node.get_canonical_chain() (referenced by get_canonical_chain and its
.last()), check that .last() is Some and return a contextual error (e.g., using
ok_or_else with eyre::eyre!("canonical chain empty after reorg")) or an assert
with a descriptive message before calling block_hash(), then pass that
canonical_tip into get_best_mempool_tx as before; this makes failures explicit
and diagnosable instead of panicking.
In `@crates/config/src/chain/chainspec.rs`:
- Around line 35-55: The check that only enables OneYear/ThirtyDay ledgers when
cascade.activation_timestamp == UnixTimestamp::from_secs(0) is too strict;
change the predicate to allow activation at-or-before genesis by using <=
instead of == (i.e. cascade.filter(|c| c.activation_timestamp <=
UnixTimestamp::from_secs(0))) so the two DataTransactionLedger pushes for
DataLedger::OneYear and DataLedger::ThirtyDay are emitted when the cascade
activates at or before genesis rather than only when activation equals zero.
In `@crates/types/src/block.rs`:
- Around line 1053-1064: The Vec<LedgerIndexItem> Index/IndexMut impls now rely
on LedgerIndexItem.ledger but deserialization doesn’t set that field, causing
panics; update BlockIndexItem::from_bytes (and/or LedgerIndexItem::from_bytes)
to restore the ledger field when reconstructing LedgerIndexItem entries from
bytes so non-Publish lookups succeed. Locate LedgerIndexItem::from_bytes and
BlockIndexItem::from_bytes and parse/assign the original ledger discriminant
into the LedgerIndexItem.ledger field during deserialization (ensure the same
enum/tag layout used in serialization is read back and assigned) so the
Index/IndexMut lookups no longer hit expect panics.
In `@crates/types/src/config/mod.rs`:
- Around line 849-873: The test test_cascade_activation_timestamp_valid
duplicates setup for multiple activation_timestamp values; refactor it into a
parameterized rstest by replacing the two hardcoded cases with a single
#[rstest] that takes activation_timestamp values (e.g., 0 and 5000) and runs the
same assertions: create NodeConfig::testing(), set
consensus.get_mut().hardforks.cascade = Some(Cascade { activation_timestamp:
UnixTimestamp::from_secs(<param>), ... }), build
Config::new_with_random_peer_id(node_config) and assert
config.validate().is_ok(); update the test function signature to accept the
timestamp parameter and add the rstest case table.
---
Outside diff comments:
In `@crates/actors/src/block_producer.rs`:
- Around line 1428-1431: mempool_txs currently only sorts submit_tx but leaves
one_year_tx and thirty_day_tx unsorted which makes block ordering depend on
insertion order; before block construction, sort mempool_txs.one_year_tx and
mempool_txs.thirty_day_tx with the same comparator used for submit_tx
(irys_types::DataTransactionHeader::compare_tx) so that tx roots/body ordering
is deterministic (similar to the existing submit_tx and commitment_tx sorts).
In `@crates/chain/tests/validation/data_tx_pricing.rs`:
- Around line 381-390: Replace hardcoded UnixTimestamp::from_secs(0) used when
computing expected fees with the actual timestamp from the block context used to
build the transaction; in places where you call calculate_term_fee_from_config
(and compute number_of_ingress_proofs_total / expected_term_fee), pull the
timestamp from the test's block/header context (e.g., the block or header
timestamp used to construct the tx or block in this test) instead of using
UnixTimestamp::from_secs(0) so pricing follows the block-time-dependent path
under hardfork-gated config.
---
Duplicate comments:
In `@crates/actors/src/block_validation.rs`:
- Around line 2163-2179: txs_to_check currently only includes publish_txs and
submit_txs so term-ledger transactions (e.g., OneYear/ThirtyDay) never hit the
inclusion checks; update the construction of txs_to_check to also chain the term
ledger iterators (the collections containing OneYear/ThirtyDay txs) and map them
to the correct DataLedger variant(s) so they produce entries for term-ledger
branches, and ensure the TxInclusionState logic (where same_block_promotions and
ledger_current are set) handles term-ledger ids the same way as Publish/Submit
(use the appropriate DataLedger enum values when initializing state for
same_block_promotions and for Searching).
In `@crates/actors/src/mempool_service.rs`:
- Around line 1157-1194: The telemetry and rejection-rate math only account for
submit_tx and omit downstream Cascade selections (one_year_tx and
thirty_day_tx), so update the aggregation to include those vectors: compute a
combined_data_txs count and sum fees into total_fee_collected by folding over
submit_tx, one_year_tx, and thirty_day_tx (in addition to commitment_tx totals
already present), then replace occurrences of submit_tx.len() in the info! and
warn! calls (and in the total_selected calculation) with the combined_data_txs
value; ensure mempool_selected.data_txs and mempool_selected.data_selected
reflect the combined count and mempool_selected.total_fee_collected uses the
summed total across all three data-tx collections.
In `@crates/actors/src/mempool_service/data_txs.rs`:
- Around line 216-233: The current checks in the DataLedger handling only reject
perm_fee when it is Some(x) and x > 0, allowing Some(0); update both occurrences
where perm_fee is inspected (the block matching DataLedger::OneYear |
DataLedger::ThirtyDay and the analogous block later around the second ingress
path) to treat any Some(_) as invalid by testing tx.perm_fee.is_some() (or
equivalent) and returning TxIngressError::FundMisalignment with the same
message, so term-ledger transactions reject any presence of perm_fee (including
Some(0)).
- Around line 40-57: The check gating OneYear/ThirtyDay ledgers uses the tip's
timestamp via is_cascade_active_at(entry.header().timestamp_secs()), which can
allow these ledgers before the new epoch actually activates; change the logic in
the DataLedger branch that computes cascade_active (currently using
block_tree_read_guard, get_canonical_chain and entry.header().timestamp_secs())
to determine cascade activation by epoch instead of tip timestamp — e.g. derive
the tip's epoch (or the activation epoch) from the canonical tip header (use the
header's epoch/epoch_index accessor) and call the hardforks API that checks
activation by epoch (or compare against the configured cascade activation epoch)
so that if cascade is not active at that epoch you return
TxIngressError::InvalidLedger(tx.ledger_id) as before.
In `@crates/api-server/src/routes/price.rs`:
- Around line 70-76: The current logic uses latest_block_timestamp_secs (from
last_block_entry) to compute activation/pricing for the next includable block
(next_block_height), causing boundary lag; update the code paths that compute
activation windows and fees (the blocks around latest_block_timestamp_secs /
tip_height / next_block_height) to derive the timestamp for next_block_height
instead of using tip timestamp — for example fetch or compute the header
timestamp for next_block_height (or predict its activation timestamp
consistently) before running OneYear/ThirtyDay/Publish pricing logic in the
functions/blocks referenced by latest_block_timestamp_secs, tip_height,
next_block_height (also apply the same change to the other ranges noted: lines
104-117, 164-174, 204-241) so pricing and 400 validations use the next-block
timestamp rather than the current tip timestamp.
In `@crates/database/src/data_ledger.rs`:
- Around line 299-306: activate_cascade currently returns early if the OneYear
ledger exists which can leave other ledgers (e.g., ThirtyDay) uncreated; change
activate_cascade (and use of self.term and ConsensusConfig) to reconcile each
DataLedger independently instead of a single early return: for each required
ledger variant (e.g., DataLedger::OneYear, DataLedger::ThirtyDay) check whether
an entry with matching ledger_id exists in self.term and, if missing, perform
the existing activation steps for that specific ledger using the same logic
currently used for the OneYear path so all ledgers are created/healed
individually.
ℹ️ Review info
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (23)
crates/actors/src/block_producer.rscrates/actors/src/block_validation.rscrates/actors/src/mempool_service.rscrates/actors/src/mempool_service/data_txs.rscrates/actors/src/packing_service/guard.rscrates/actors/src/validation_service/block_validation_task.rscrates/api-server/src/routes/price.rscrates/chain/tests/api/pricing_endpoint.rscrates/chain/tests/block_production/block_validation.rscrates/chain/tests/multi_node/api_ingress_validation.rscrates/chain/tests/multi_node/cascade_reorg.rscrates/chain/tests/validation/cascade_ledger_shape.rscrates/chain/tests/validation/cascade_term_expiry.rscrates/chain/tests/validation/data_tx_pricing.rscrates/config/src/chain/chainspec.rscrates/database/src/data_ledger.rscrates/domain/src/hardfork_ext.rscrates/domain/src/snapshots/epoch_snapshot/epoch_snapshot.rscrates/types/src/block.rscrates/types/src/config/consensus.rscrates/types/src/config/mod.rscrates/types/src/hardfork_config.rscrates/types/src/storage_pricing.rs
Comment on lines
+66
to
+77
| // Temporarily override any global panic hook that aborts the process. | ||
| let prev_hook = std::panic::take_hook(); | ||
| std::panic::set_hook(Box::new(|_| {})); | ||
|
|
||
| let result = std::panic::catch_unwind(|| { | ||
| let _guard = ActiveWorkerGuard::new(counter.clone(), notify.clone()); | ||
| assert_eq!(counter.load(Ordering::Relaxed), 1); | ||
| panic!("Simulated panic"); | ||
| }); | ||
|
|
||
| std::panic::set_hook(prev_hook); | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's locate and examine the guard.rs file
find . -name "guard.rs" -type f | head -20Repository: Irys-xyz/irys
Length of output: 101
🏁 Script executed:
#!/bin/bash
# Read the guard.rs file to see the full test implementation
cat -n crates/actors/src/packing_service/guard.rs | head -120Repository: Irys-xyz/irys
Length of output: 4593
🏁 Script executed:
#!/bin/bash
# Search for other panic hook mutations in the entire codebase
rg -n --type rust '\b(std::panic|panic)::(set_hook|take_hook)\s*\(' --context 3Repository: Irys-xyz/irys
Length of output: 6286
Serialize panic-hook mutation to avoid parallel-test interference.
The test at lines 66–77 mutates the process-global panic hook state without synchronization. Under cargo test with parallel test execution, concurrent tests can interfere: one test's take_hook() may overwrite another test's saved hook, and subsequent set_hook(prev_hook) calls can restore the wrong hook, causing flaky panic behavior.
This pattern also appears in crates/types/src/commitment_v1.rs and crates/types/src/commitment_v2.rs (tests at lines 590, 598, 606, 706, 714, 722), though those tests don't even attempt restoration, making the race condition worse.
Suggested fix
#[cfg(test)]
mod tests {
use super::*;
+ use std::sync::{Mutex, OnceLock};
+
+ static PANIC_HOOK_LOCK: OnceLock<Mutex<()>> = OnceLock::new();
#[test]
fn test_guard_handles_panic() {
let counter = Arc::new(AtomicUsize::new(0));
let notify = Arc::new(Notify::new());
+ let hook_lock = PANIC_HOOK_LOCK.get_or_init(|| Mutex::new(()));
+ let _hook_guard = hook_lock.lock().expect("panic hook lock poisoned");
+
// Temporarily override any global panic hook that aborts the process.
let prev_hook = std::panic::take_hook();
std::panic::set_hook(Box::new(|_| {}));
let result = std::panic::catch_unwind(|| {
let _guard = ActiveWorkerGuard::new(counter.clone(), notify.clone());
assert_eq!(counter.load(Ordering::Relaxed), 1);
panic!("Simulated panic");
});
std::panic::set_hook(prev_hook);
assert!(result.is_err());
assert_eq!(counter.load(Ordering::Relaxed), 0);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/actors/src/packing_service/guard.rs` around lines 66 - 77, The test
mutates the process-global panic hook (take_hook/set_hook) unsafely; serialize
access by introducing a test-only global mutex (e.g., a
once_cell::sync::Lazy<Mutex<()>> or lazy_static Mutex) and acquire the lock
around the entire sequence that calls std::panic::take_hook(),
std::panic::set_hook(Box::new(...)), the std::panic::catch_unwind block that
constructs ActiveWorkerGuard, and the final std::panic::set_hook(prev_hook)
restore so no two tests can interleave these calls; apply the same mutex-guard
pattern to the similar tests in commitment_v1.rs and commitment_v2.rs to ensure
correct restoration of the previous hook.
Contributor
There was a problem hiding this comment.
we use cargo nextest instead of normal cargo test. is this finding still relevant?
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
cargo nextest test execution model separate processes
💡 Result:
cargo nextest uses a process-per-test execution model: each individual test case is run in its own OS process, which is a key difference from cargo test’s shared-process model where many tests typically run within the same test-binary process. [1]
Practically, that means nextest re-invokes the test binary separately per test case (enabling per-test isolation), which makes features like per-test timeouts, retries, cancellation, and robust output capture much easier to implement using standard OS primitives (spawn/kill/wait, etc.). [1]
This model also explains why nextest can’t rely on “in-process” synchronization patterns some cargo test suites use (e.g., global mutexes inside the test binary); nextest instead provides test groups to rate-limit or serialize subsets of tests across processes. [2]
Sources:
[1] https://nexte.st/docs/design/why-process-per-test/
[2] https://nexte.st/docs/configuration/test-groups/
@roberts-pumpurs, great question! No, this finding is not relevant when using cargo nextest.
Cargo nextest uses a process-per-test execution model where each test runs in its own separate OS process, unlike cargo test which runs tests as threads within the same process. This means:
- Each test has its own process memory space and panic hook state
- Tests cannot interfere with each other's global state across process boundaries
- The race condition I flagged (where concurrent tests can overwrite each other's saved hooks) cannot occur
The mutex serialization I suggested is only necessary for cargo test's thread-based parallelism. With nextest's process isolation, the current code is already safe—each test can freely mutate its panic hook without affecting other tests.
So you can safely ignore this finding for your test suite. ✅
🧠 Learnings used
Learnt from: glottologist
Repo: Irys-xyz/irys PR: 1090
File: crates/actors/src/supply_state_calculator.rs:121-130
Timestamp: 2026-01-20T13:27:23.880Z
Learning: In the supply state backfill logic (crates/actors/src/supply_state_calculator.rs), if the backfill process encounters failures such as batch count mismatches, the code should panic rather than return an error, as these represent critical invariant violations.
Learnt from: antouhou
Repo: Irys-xyz/irys PR: 1097
File: crates/p2p/src/block_pool.rs:834-867
Timestamp: 2026-02-11T21:49:10.701Z
Learning: In crates/p2p/src/block_pool.rs, the spawned background task for mempool ingestion (handle_commitment_transaction_ingress_gossip and handle_data_transaction_ingress_gossip) intentionally uses a fire-and-forget pattern without panic handling because failures at this stage don't matter—block validation will re-ingest the transactions anyway.
Learnt from: CR
Repo: Irys-xyz/irys PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-26T16:43:55.948Z
Learning: Applies to **/*.rs : Use `#[tokio::test]` for async tests
Learnt from: CR
Repo: Irys-xyz/irys PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-26T16:43:55.948Z
Learning: Applies to **/*.rs : Use `#[test_log::test(tokio::test)]` for tests that require tracing output
Learnt from: DanMacDonald
Repo: Irys-xyz/irys PR: 1166
File: crates/actors/src/validation_service/block_validation_task.rs:396-400
Timestamp: 2026-03-01T03:16:36.969Z
Learning: In cascade hardfork activation (e.g., `crates/actors/src/validation_service/block_validation_task.rs`), the validator must use `parent_epoch_snapshot` to determine `cascade_active` (via `config.consensus.hardforks.is_cascade_active_for_epoch(&parent_epoch_snapshot)`) because the block producer uses the same parent snapshot to decide whether to include cascade ledgers (OneYear/ThirtyDay). This ensures producer and validator use identical logic. The new epoch snapshot reflecting cascade activation doesn't exist yet when validating epoch boundary blocks—it's computed after the block is accepted. This pattern is consistent with Borealis activation checks.
Collaborator
Author
There was a problem hiding this comment.
Yes, still relevant. The hook override isn't about the test runner — it's about catch_unwind. If a global panic hook calls std::process::abort() (which nextest's default hook does), then catch_unwind never gets a chance to catch the panic. The save/replace/restore pattern ensures the simulated panic is catchable so we can verify the guard decrements the counter on unwind.
Contributor
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Comment on lines
+494
to
+510
| let txs = sealed_block_for_data.transactions(); | ||
| let mut term_txs: Vec<irys_types::DataTransactionHeader> = Vec::new(); | ||
| for ledger in [ | ||
| irys_types::DataLedger::OneYear, | ||
| irys_types::DataLedger::ThirtyDay, | ||
| ] { | ||
| term_txs.extend_from_slice(txs.get_ledger_txs(ledger)); | ||
| } | ||
| data_txs_are_valid( | ||
| config, | ||
| service_senders, | ||
| block, | ||
| &self.service_inner.db, | ||
| &self.block_tree_guard, | ||
| sealed_block_for_data | ||
| .transactions() | ||
| .get_ledger_txs(irys_types::DataLedger::Submit), | ||
| sealed_block_for_data | ||
| .transactions() | ||
| .get_ledger_txs(irys_types::DataLedger::Publish), | ||
| txs.get_ledger_txs(irys_types::DataLedger::Submit), | ||
| txs.get_ledger_txs(irys_types::DataLedger::Publish), | ||
| &term_txs, |
Contributor
There was a problem hiding this comment.
Term-tx aggregation drops ledger context and can hit a panic path.
This merges both term buckets and relies on downstream tx.ledger_id inference. If a non-term ledger_id tx is placed in a term bucket, downstream validation can hit unreachable!() instead of returning an invalid-block error. Add explicit per-bucket ledger-id checks before aggregation (or pass bucket context downstream).
🛡️ Defensive check before aggregation
let data_txs_validation_task = async move {
let txs = sealed_block_for_data.transactions();
+ for tx in txs.get_ledger_txs(irys_types::DataLedger::OneYear) {
+ if tx.ledger_id != irys_types::DataLedger::OneYear as u32 {
+ return ValidationResult::Invalid(ValidationError::Other(format!(
+ "tx {} has ledger_id {} in OneYear bucket",
+ tx.id, tx.ledger_id
+ )));
+ }
+ }
+ for tx in txs.get_ledger_txs(irys_types::DataLedger::ThirtyDay) {
+ if tx.ledger_id != irys_types::DataLedger::ThirtyDay as u32 {
+ return ValidationResult::Invalid(ValidationError::Other(format!(
+ "tx {} has ledger_id {} in ThirtyDay bucket",
+ tx.id, tx.ledger_id
+ )));
+ }
+ }
let mut term_txs: Vec<irys_types::DataTransactionHeader> = Vec::new();
for ledger in [
irys_types::DataLedger::OneYear,
irys_types::DataLedger::ThirtyDay,
] {
term_txs.extend_from_slice(txs.get_ledger_txs(ledger));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let txs = sealed_block_for_data.transactions(); | |
| let mut term_txs: Vec<irys_types::DataTransactionHeader> = Vec::new(); | |
| for ledger in [ | |
| irys_types::DataLedger::OneYear, | |
| irys_types::DataLedger::ThirtyDay, | |
| ] { | |
| term_txs.extend_from_slice(txs.get_ledger_txs(ledger)); | |
| } | |
| data_txs_are_valid( | |
| config, | |
| service_senders, | |
| block, | |
| &self.service_inner.db, | |
| &self.block_tree_guard, | |
| sealed_block_for_data | |
| .transactions() | |
| .get_ledger_txs(irys_types::DataLedger::Submit), | |
| sealed_block_for_data | |
| .transactions() | |
| .get_ledger_txs(irys_types::DataLedger::Publish), | |
| txs.get_ledger_txs(irys_types::DataLedger::Submit), | |
| txs.get_ledger_txs(irys_types::DataLedger::Publish), | |
| &term_txs, | |
| let data_txs_validation_task = async move { | |
| let txs = sealed_block_for_data.transactions(); | |
| for tx in txs.get_ledger_txs(irys_types::DataLedger::OneYear) { | |
| if tx.ledger_id != irys_types::DataLedger::OneYear as u32 { | |
| return ValidationResult::Invalid(ValidationError::Other(format!( | |
| "tx {} has ledger_id {} in OneYear bucket", | |
| tx.id, tx.ledger_id | |
| ))); | |
| } | |
| } | |
| for tx in txs.get_ledger_txs(irys_types::DataLedger::ThirtyDay) { | |
| if tx.ledger_id != irys_types::DataLedger::ThirtyDay as u32 { | |
| return ValidationResult::Invalid(ValidationError::Other(format!( | |
| "tx {} has ledger_id {} in ThirtyDay bucket", | |
| tx.id, tx.ledger_id | |
| ))); | |
| } | |
| } | |
| let mut term_txs: Vec<irys_types::DataTransactionHeader> = Vec::new(); | |
| for ledger in [ | |
| irys_types::DataLedger::OneYear, | |
| irys_types::DataLedger::ThirtyDay, | |
| ] { | |
| term_txs.extend_from_slice(txs.get_ledger_txs(ledger)); | |
| } | |
| data_txs_are_valid( | |
| config, | |
| service_senders, | |
| block, | |
| &self.service_inner.db, | |
| &self.block_tree_guard, | |
| txs.get_ledger_txs(irys_types::DataLedger::Submit), | |
| txs.get_ledger_txs(irys_types::DataLedger::Publish), | |
| &term_txs, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/actors/src/validation_service/block_validation_task.rs` around lines
494 - 510, The aggregation into term_txs (built from
sealed_block_for_data.transactions()) loses per-ledger context and can hide
invalid ledger_ids, leading to downstream unreachable!() in data_txs_are_valid;
before extending term_txs for irys_types::DataLedger::OneYear and
DataLedger::ThirtyDay, iterate each tx from txs.get_ledger_txs(...) and
assert/filter that tx.ledger_id matches the expected ledger (OneYear or
ThirtyDay) and fail fast (return an invalid-block error) if any tx has a
non-term ledger_id; alternatively preserve per-bucket context by passing the two
separate vectors (one_year_txs, thirty_day_txs) into data_txs_are_valid or
perform the ledger_id checks inside data_txs_are_valid, referencing the term_txs
variable, sealed_block_for_data.transactions(), the DataLedger enum, and
tx.ledger_id to locate the code to change.
Comment on lines
+688
to
+742
| async fn heavy_cascade_pricing_endpoint_term_ledgers_pre_post_activation() -> eyre::Result<()> { | ||
| use irys_types::hardfork_config::Cascade; | ||
|
|
||
| let num_blocks_in_epoch = 4_u64; | ||
|
|
||
| let config = IrysNodeTest::<()>::default_async() | ||
| .cfg | ||
| .with_consensus(|consensus| { | ||
| consensus.epoch.num_blocks_in_epoch = num_blocks_in_epoch; | ||
| consensus.hardforks.cascade = Some(Cascade { | ||
| activation_timestamp: UnixTimestamp::from_secs(0), | ||
| one_year_epoch_length: 365, | ||
| thirty_day_epoch_length: 30, | ||
| annual_cost_per_gb: Cascade::default_annual_cost_per_gb(), | ||
| }); | ||
| // Disable minimum term fee so calculated fees reflect actual epoch-length differences | ||
| consensus.minimum_term_fee_usd = | ||
| irys_types::storage_pricing::Amount::token(rust_decimal_macros::dec!(0)).unwrap(); | ||
| }); | ||
|
|
||
| let ctx = IrysNodeTest::new_genesis(config).start().await; | ||
| let address = format!( | ||
| "http://127.0.0.1:{}", | ||
| ctx.node_ctx.config.node_config.http.bind_port | ||
| ); | ||
| let data_size_bytes = ctx.node_ctx.config.consensus.chunk_size; | ||
|
|
||
| // Cascade activates from genesis with timestamp 0, so OneYear and ThirtyDay | ||
| // should be available immediately. | ||
|
|
||
| // Post-Cascade: OneYear should return 200 with perm_fee=0, term_fee > 0 | ||
| let one_year_after = | ||
| price_endpoint_request(&address, DataLedger::OneYear, data_size_bytes).await; | ||
| assert_eq!(one_year_after.status(), reqwest::StatusCode::OK); | ||
| let one_year_price = one_year_after.json::<PriceInfo>().await?; | ||
| assert_eq!(one_year_price.perm_fee, U256::from(0)); | ||
| assert!(one_year_price.term_fee > U256::from(0)); | ||
|
|
||
| // Post-Cascade: ThirtyDay should return 200 with perm_fee=0, term_fee > 0 | ||
| let thirty_day_after = | ||
| price_endpoint_request(&address, DataLedger::ThirtyDay, data_size_bytes).await; | ||
| assert_eq!(thirty_day_after.status(), reqwest::StatusCode::OK); | ||
| let thirty_day_price = thirty_day_after.json::<PriceInfo>().await?; | ||
| assert_eq!(thirty_day_price.perm_fee, U256::from(0)); | ||
| assert!(thirty_day_price.term_fee > U256::from(0)); | ||
|
|
||
| // Longer storage = higher fee | ||
| assert!( | ||
| one_year_price.term_fee > thirty_day_price.term_fee, | ||
| "OneYear term_fee should be higher than ThirtyDay term_fee" | ||
| ); | ||
|
|
||
| ctx.stop().await; | ||
| Ok(()) | ||
| } |
Contributor
There was a problem hiding this comment.
These boundary tests currently validate only post-activation behavior.
Line 698 and Line 841 set activation_timestamp to 0, so the tests never assert a true pre-activation state despite “pre_post” / “exact transition” naming.
Also applies to: 826-899
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/chain/tests/api/pricing_endpoint.rs` around lines 688 - 742, The test
heavy_cascade_pricing_endpoint_term_ledgers_pre_post_activation currently sets
Cascade.activation_timestamp to 0 so it only exercises post-activation behavior;
change the test to first use a non-zero/future activation timestamp (e.g.,
UnixTimestamp::from_secs(1000) or now()+N) to assert the pre-activation
responses for DataLedger::OneYear and DataLedger::ThirtyDay, then create a
second run/config with activation_timestamp = UnixTimestamp::from_secs(0) (or
otherwise advance the node time) to assert the post-activation responses and the
term_fee differences; update the Cascade.activation_timestamp in this function
(and the similar tests around the 826-899 range) and add the corresponding
pre-activation assertions before the post-activation checks so the
“pre_post”/transition behavior is actually validated.
Comment on lines
+454
to
+459
| /// Test mempool ingress behavior at the Cascade hardfork boundary for OneYear/ThirtyDay term ledgers. | ||
| /// Pre-Cascade: term ledger transactions are rejected (InvalidLedger). | ||
| /// Post-Cascade: term ledger transactions with correct fees are accepted; | ||
| /// those with perm_fee or insufficient term_fee are rejected. | ||
| #[test_log::test(tokio::test)] | ||
| async fn heavy_cascade_mempool_ingress_boundary_for_term_ledgers() -> eyre::Result<()> { |
Contributor
There was a problem hiding this comment.
Boundary test description does not match exercised path.
The docstring says pre-Cascade term-ledger rejection is covered, but this test activates Cascade at timestamp 0 and only validates post-Cascade behavior. Add an explicit pre-Cascade case (or rename to post-Cascade-only).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/chain/tests/multi_node/api_ingress_validation.rs` around lines 454 -
459, The test heavy_cascade_mempool_ingress_boundary_for_term_ledgers currently
enables Cascade at timestamp 0 but its docstring claims it also verifies
pre-Cascade rejection; update the test to make behavior consistent by either
adding an explicit pre-Cascade scenario or renaming to indicate
post-Cascade-only. To add the pre-Cascade case: before activating Cascade in
heavy_cascade_mempool_ingress_boundary_for_term_ledgers, run the same mempool
ingress assertions against OneYear/ThirtyDay term-ledger transactions expecting
InvalidLedger, then advance time/activate Cascade and re-run the post-Cascade
assertions; alternatively, if you prefer renaming, change the test name and
docstring to reflect it only validates post-Cascade behavior. Ensure references
to the Cascade activation call and the mempool ingress assertion helpers in the
test are updated accordingly.
Comment on lines
+14
to
+15
| let num_blocks_in_epoch = 10_u64; | ||
| let activation_height = num_blocks_in_epoch; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Misleading variable name activation_height when using timestamp-based activation.
The variable activation_height and the comment on line 43 ("Mine past the Cascade activation height") suggest height-based activation, but the cascade config uses activation_timestamp: UnixTimestamp::from_secs(0), meaning Cascade is active from genesis. This variable is effectively just controlling how many blocks to mine before the test scenario, not activation.
Consider renaming for clarity:
♻️ Suggested rename for clarity
- let activation_height = num_blocks_in_epoch;
+ let min_blocks_before_fork = num_blocks_in_epoch;And update the comment on line 43:
- // Mine past the Cascade activation height so term ledgers are available
- while genesis_node.get_canonical_chain_height().await < activation_height {
+ // Mine some blocks to establish chain history before forking
+ while genesis_node.get_canonical_chain_height().await < min_blocks_before_fork {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/chain/tests/multi_node/cascade_reorg.rs` around lines 14 - 15, The
variable activation_height is misleading because the Cascade config uses
timestamp-based activation (activation_timestamp: UnixTimestamp::from_secs(0)),
so rename activation_height to something like pre_mine_blocks or
blocks_to_mine_before_test and update any related comments (e.g., the comment
that currently reads "Mine past the Cascade activation height") to reflect that
you are simply mining a number of blocks to set up the test rather than crossing
a height-based activation; update occurrences in the test (references to
activation_height and the comment near the block-mining logic) and ensure the
new name makes it clear this controls how many blocks to mine before running the
scenario.
Comment on lines
+165
to
+170
| let canonical_tip = genesis_node | ||
| .get_canonical_chain() | ||
| .last() | ||
| .expect("canonical chain should have a tip") | ||
| .block_hash(); | ||
| let mempool_txs = genesis_node.get_best_mempool_tx(canonical_tip).await?; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding a fallback or clearer error for empty canonical chain.
The .expect() on line 168 will panic if the canonical chain is empty. While this shouldn't happen in a properly running test, consider using a more descriptive error or adding an assertion before this point to make test failures easier to diagnose.
♻️ Suggested improvement
let canonical_tip = genesis_node
.get_canonical_chain()
.last()
- .expect("canonical chain should have a tip")
+ .expect("canonical chain is unexpectedly empty after reorg to height {base_height + 3}")
.block_hash();Or use ? with a contextual error:
let canonical_chain = genesis_node.get_canonical_chain();
let canonical_tip = canonical_chain
.last()
.ok_or_else(|| eyre::eyre!("canonical chain empty after reorg"))?
.block_hash();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/chain/tests/multi_node/cascade_reorg.rs` around lines 165 - 170,
Replace the panic-inducing .expect() on the canonical chain with a clearer,
non-panicking error path: capture the result of
genesis_node.get_canonical_chain() (referenced by get_canonical_chain and its
.last()), check that .last() is Some and return a contextual error (e.g., using
ok_or_else with eyre::eyre!("canonical chain empty after reorg")) or an assert
with a descriptive message before calling block_hash(), then pass that
canonical_tip into get_best_mempool_tx as before; this makes failures explicit
and diagnosable instead of panicking.
Comment on lines
+35
to
+55
| if let Some(cascade) = cascade.filter(|c| c.activation_timestamp == UnixTimestamp::from_secs(0)) | ||
| { | ||
| data_ledgers.push(DataTransactionLedger { | ||
| ledger_id: DataLedger::OneYear.into(), | ||
| tx_root: H256::zero(), | ||
| tx_ids: H256List::new(), | ||
| total_chunks: 0, | ||
| expires: Some(cascade.one_year_epoch_length), | ||
| proofs: None, | ||
| required_proof_count: None, | ||
| }); | ||
| data_ledgers.push(DataTransactionLedger { | ||
| ledger_id: DataLedger::ThirtyDay.into(), | ||
| tx_root: H256::zero(), | ||
| tx_ids: H256List::new(), | ||
| total_chunks: 0, | ||
| expires: Some(cascade.thirty_day_epoch_length), | ||
| proofs: None, | ||
| required_proof_count: None, | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
Cascade-at-genesis check is too strict (== 0) and can mis-shape genesis ledgers.
This only enables OneYear/ThirtyDay when activation is exactly zero. If genesis timestamp is non-zero and Cascade activates at or before genesis time, this still emits 2 ledgers instead of 4.
Suggested fix
- // Only include OneYear/ThirtyDay ledgers if Cascade activates at genesis (timestamp 0)
- if let Some(cascade) = cascade.filter(|c| c.activation_timestamp == UnixTimestamp::from_secs(0))
+ // Include OneYear/ThirtyDay ledgers if Cascade is active at genesis timestamp
+ let genesis_timestamp = UnixTimestamp::from_secs(config.timestamp_millis / 1000);
+ if let Some(cascade) = cascade.filter(|c| c.activation_timestamp <= genesis_timestamp)
{
data_ledgers.push(DataTransactionLedger {
ledger_id: DataLedger::OneYear.into(),
...
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/config/src/chain/chainspec.rs` around lines 35 - 55, The check that
only enables OneYear/ThirtyDay ledgers when cascade.activation_timestamp ==
UnixTimestamp::from_secs(0) is too strict; change the predicate to allow
activation at-or-before genesis by using <= instead of == (i.e.
cascade.filter(|c| c.activation_timestamp <= UnixTimestamp::from_secs(0))) so
the two DataTransactionLedger pushes for DataLedger::OneYear and
DataLedger::ThirtyDay are emitted when the cascade activates at or before
genesis rather than only when activation equals zero.
Comment on lines
+1053
to
1064
| self.iter() | ||
| .find(|item| item.ledger == ledger) | ||
| .expect("No ledger index item found for given ledger type") | ||
| } | ||
| } | ||
|
|
||
| impl IndexMut<DataLedger> for Vec<LedgerIndexItem> { | ||
| fn index_mut(&mut self, ledger: DataLedger) -> &mut Self::Output { | ||
| &mut self[ledger as usize] | ||
| self.iter_mut() | ||
| .find(|item| item.ledger == ledger) | ||
| .expect("No ledger index item found for given ledger type") | ||
| } |
Contributor
There was a problem hiding this comment.
Ledger-keyed indexing is now incompatible with current block-index deserialization.
These lookups now depend on LedgerIndexItem.ledger, but deserialization (BlockIndexItem::from_bytes → LedgerIndexItem::from_bytes) does not restore ledger. Restored entries default to Publish, so non-publish lookups can panic.
🐛 Suggested fix (restore ledger during `BlockIndexItem::from_bytes`)
impl BlockIndexItem {
pub fn from_bytes(bytes: &[u8]) -> Self {
let mut item = Self {
block_hash: H256::from_slice(&bytes[0..32]),
num_ledgers: bytes[32],
..Default::default()
};
let num_ledgers = item.num_ledgers as usize;
item.ledgers = Vec::with_capacity(num_ledgers);
for i in 0..num_ledgers {
let start = 33 + (i * 40);
let ledger_bytes = &bytes[start..start + 40];
- item.ledgers.push(LedgerIndexItem::from_bytes(ledger_bytes));
+ let mut ledger_item = LedgerIndexItem::from_bytes(ledger_bytes);
+ ledger_item.ledger = match item.num_ledgers {
+ 2 => match i {
+ 0 => DataLedger::Publish,
+ 1 => DataLedger::Submit,
+ _ => panic!("invalid 2-ledger index: {i}"),
+ },
+ 4 => match i {
+ 0 => DataLedger::Publish,
+ 1 => DataLedger::Submit,
+ 2 => DataLedger::OneYear,
+ 3 => DataLedger::ThirtyDay,
+ _ => panic!("invalid 4-ledger index: {i}"),
+ },
+ _ => panic!("unsupported num_ledgers: {}", item.num_ledgers),
+ };
+ item.ledgers.push(ledger_item);
}
item
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/types/src/block.rs` around lines 1053 - 1064, The Vec<LedgerIndexItem>
Index/IndexMut impls now rely on LedgerIndexItem.ledger but deserialization
doesn’t set that field, causing panics; update BlockIndexItem::from_bytes
(and/or LedgerIndexItem::from_bytes) to restore the ledger field when
reconstructing LedgerIndexItem entries from bytes so non-Publish lookups
succeed. Locate LedgerIndexItem::from_bytes and BlockIndexItem::from_bytes and
parse/assign the original ledger discriminant into the LedgerIndexItem.ledger
field during deserialization (ensure the same enum/tag layout used in
serialization is read back and assigned) so the Index/IndexMut lookups no longer
hit expect panics.
Comment on lines
+849
to
+873
| #[test] | ||
| fn test_cascade_activation_timestamp_valid() { | ||
| use crate::hardfork_config::Cascade; | ||
| use crate::UnixTimestamp; | ||
|
|
||
| let mut node_config = NodeConfig::testing(); | ||
| node_config.consensus.get_mut().hardforks.cascade = Some(Cascade { | ||
| activation_timestamp: UnixTimestamp::from_secs(0), | ||
| one_year_epoch_length: 365, | ||
| thirty_day_epoch_length: 30, | ||
| annual_cost_per_gb: Cascade::default_annual_cost_per_gb(), | ||
| }); | ||
| let config = Config::new_with_random_peer_id(node_config); | ||
| assert!(config.validate().is_ok()); | ||
|
|
||
| let mut node_config = NodeConfig::testing(); | ||
| node_config.consensus.get_mut().hardforks.cascade = Some(Cascade { | ||
| activation_timestamp: UnixTimestamp::from_secs(5000), | ||
| one_year_epoch_length: 365, | ||
| thirty_day_epoch_length: 30, | ||
| annual_cost_per_gb: Cascade::default_annual_cost_per_gb(), | ||
| }); | ||
| let config = Config::new_with_random_peer_id(node_config); | ||
| assert!(config.validate().is_ok()); | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use rstest for this timestamp matrix test.
This test encodes multiple activation-timestamp cases with duplicated setup. Convert it to a parameterized #[rstest] case table for maintainability and consistency.
♻️ Proposed refactor
+ use rstest::rstest;
+
- #[test]
- fn test_cascade_activation_timestamp_valid() {
+ #[rstest]
+ #[case(0)]
+ #[case(5000)]
+ fn test_cascade_activation_timestamp_valid(#[case] activation_secs: u64) {
use crate::hardfork_config::Cascade;
use crate::UnixTimestamp;
let mut node_config = NodeConfig::testing();
node_config.consensus.get_mut().hardforks.cascade = Some(Cascade {
- activation_timestamp: UnixTimestamp::from_secs(0),
+ activation_timestamp: UnixTimestamp::from_secs(activation_secs),
one_year_epoch_length: 365,
thirty_day_epoch_length: 30,
annual_cost_per_gb: Cascade::default_annual_cost_per_gb(),
});
let config = Config::new_with_random_peer_id(node_config);
assert!(config.validate().is_ok());
-
- let mut node_config = NodeConfig::testing();
- node_config.consensus.get_mut().hardforks.cascade = Some(Cascade {
- activation_timestamp: UnixTimestamp::from_secs(5000),
- one_year_epoch_length: 365,
- thirty_day_epoch_length: 30,
- annual_cost_per_gb: Cascade::default_annual_cost_per_gb(),
- });
- let config = Config::new_with_random_peer_id(node_config);
- assert!(config.validate().is_ok());
}As per coding guidelines: "Use rstest for parameterized tests".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/types/src/config/mod.rs` around lines 849 - 873, The test
test_cascade_activation_timestamp_valid duplicates setup for multiple
activation_timestamp values; refactor it into a parameterized rstest by
replacing the two hardcoded cases with a single #[rstest] that takes
activation_timestamp values (e.g., 0 and 5000) and runs the same assertions:
create NodeConfig::testing(), set consensus.get_mut().hardforks.cascade =
Some(Cascade { activation_timestamp: UnixTimestamp::from_secs(<param>), ... }),
build Config::new_with_random_peer_id(node_config) and assert
config.validate().is_ok(); update the test function signature to accept the
timestamp parameter and add the rstest case table.
Extract validate_price/validate_term_price closures into standalone functions, take &BlockTransactions instead of 3 slices, add structural pre-pass for submit/term txs, include term txs in the inclusion-checking pipeline with cross-ledger collision guards, and remove redundant Steps 5 & 6.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
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/validation/data_tx_pricing.rs (1)
390-400: 🧹 Nitpick | 🔵 TrivialAvoid hardcoded pricing timestamp in activation-aware fee checks.
Using
UnixTimestamp::from_secs(0)here can mask boundary regressions by forcing a fixed pricing context. Prefer deriving the timestamp/height from the block context used for the test scenario.Also applies to: 517-529
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/chain/tests/validation/data_tx_pricing.rs` around lines 390 - 400, The test is hardcoding UnixTimestamp::from_secs(0) when calling calculate_perm_fee_from_config (for expected_perm_fee and the other call at lines 517-529), which hides activation/interval boundary issues; replace the hardcoded timestamp with the timestamp/height derived from the block context used in the test (e.g., the block or header used to build the scenario—use that block's timestamp or corresponding UnixTimestamp/height variable) so the call to calculate_perm_fee_from_config uses the actual activation-aware pricing context (keep references to calculate_perm_fee_from_config, expected_perm_fee, number_of_ingress_proofs_total, and price_before_the_interval).
♻️ Duplicate comments (3)
crates/actors/src/block_validation.rs (1)
2282-2287:⚠️ Potential issue | 🟠 MajorPromoted height check should return a validation error, not just log.
The check at lines 2282-2287 logs an error when a Submit ledger transaction has
promoted_heightset, but doesn't return aPreValidationError. This allows invalid transactions to pass validation. Per past review feedback, this should be a proper validation failure.🔧 Suggested fix
if tx.promoted_height().is_some() { - tracing::error!( + return Err(PreValidationError::InvalidLedgerIdForTx { + tx_id: tx.id, + expected: DataLedger::Publish as u32, + actual: tx.ledger_id, + }); + // Or create a new error variant: + // return Err(PreValidationError::SubmitTxHasPromotedHeight { tx_id: tx.id }); - "Transaction {} in Submit ledger should not have a promoted_height", - tx.id - ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/actors/src/block_validation.rs` around lines 2282 - 2287, Replace the current tracing::error-only check for tx.promoted_height() in block_validation (the block where tx is validated) with a proper validation failure: when tx.promoted_height().is_some() return Err(PreValidationError::...) instead of only logging, including tx.id and a clear message in the error construction so callers see this as a validation rejection (update the check in the same function/method that contains the current snippet referencing tx and tx.id).crates/chain/tests/api/pricing_endpoint.rs (2)
687-742: 🧹 Nitpick | 🔵 TrivialTest name implies pre/post behavior but only exercises post-activation.
The test
heavy_cascade_pricing_endpoint_term_ledgers_pre_post_activationsetsactivation_timestamp: UnixTimestamp::from_secs(0), meaning Cascade is active from genesis. The test only verifies post-activation responses for OneYear and ThirtyDay ledgers.Consider either:
- Renaming to
heavy_cascade_pricing_endpoint_term_ledgers_post_activationto accurately reflect coverage, or- Adding a pre-activation scenario with a future activation timestamp that returns 400 for term ledgers before mining past the activation point.
,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/chain/tests/api/pricing_endpoint.rs` around lines 687 - 742, The test heavy_cascade_pricing_endpoint_term_ledgers_pre_post_activation misleadingly only exercises post-activation behavior; either rename it to heavy_cascade_pricing_endpoint_term_ledgers_post_activation, or add a pre-activation branch: set Cascade.activation_timestamp to a future UnixTimestamp, call price_endpoint_request(&address, DataLedger::OneYear/ThirtyDay, ...) and assert a 400 response (and/or appropriate error body), then advance the chain/time (or mine blocks) past activation and re-run price_endpoint_request to assert 200 with perm_fee == 0 and term_fee > 0; reference the test function name, the Cascade.activation_timestamp field, and price_endpoint_request helper when making changes.
823-899: 🧹 Nitpick | 🔵 TrivialTest exercises post-activation only despite "transition" naming.
Similar to the earlier test,
heavy_cascade_pricing_transition_at_exact_activation_heightsetsactivation_timestamp: UnixTimestamp::from_secs(0), so Cascade is always active. The test verifies fees at height 1 use the Cascade rate, but doesn't actually test the transition from pre-Cascade to post-Cascade pricing.For a true transition test, consider configuring Cascade to activate at a future timestamp/height, querying the API before and after mining past the activation point, and asserting the fee delta.
,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/chain/tests/api/pricing_endpoint.rs` around lines 823 - 899, The test heavy_cascade_pricing_transition_at_exact_activation_height currently sets Cascade.activation_timestamp to UnixTimestamp::from_secs(0) so Cascade is always active — change the setup to pick a future activation (e.g., set activation_timestamp to a nonzero timestamp corresponding to a specific activation height), compute fees for the pre-activation height and for the activation height using the existing compute_api_fee closure (which calls calculate_submit_ledger_expiry and calculate_term_fee), and then exercise price_endpoint_request both before and after advancing/mine the chain past the activation point (or start the node with a tip that simulates pre/post activation) asserting that fee at activation-1 equals the base rate and fee at activation equals the Cascade rate and that the API responses match those computed values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/actors/src/block_validation.rs`:
- Around line 2491-2498: Replace the generic
PreValidationError::SubmitTxAlreadyIncluded used in the DataLedger::OneYear and
DataLedger::ThirtyDay match arm with a new, specific error variant (e.g.
PreValidationError::TermLedgerTxAlreadyIncluded) to accurately reflect
term-ledger duplicate inclusion; add the new enum variant to PreValidationError
and ensure it carries the same context fields (tx_id, ledger, block_hash),
update the match arm here (the branch checking DataLedger::OneYear |
DataLedger::ThirtyDay that currently returns SubmitTxAlreadyIncluded with tx.id,
*ledger_historical, *historical_block_hash) to return the new
TermLedgerTxAlreadyIncluded, and update any error constructors/handlers or tests
that match on SubmitTxAlreadyIncluded where term-ledger semantics apply.
- Around line 1662-1666: Remove the redundant recomputation of cascade_active by
deleting the second call that reassigns cascade_active via
config.consensus.hardforks.is_cascade_active_at(block.timestamp_secs()); reuse
the existing cascade_active value computed earlier (the original variable from
the first is_cascade_active_at call) instead of recomputing it, ensuring any
conditional that follows uses that existing variable (referenced as
cascade_active and the method is_cascade_active_at on config.consensus.hardforks
and block.timestamp_secs()).
---
Outside diff comments:
In `@crates/chain/tests/validation/data_tx_pricing.rs`:
- Around line 390-400: The test is hardcoding UnixTimestamp::from_secs(0) when
calling calculate_perm_fee_from_config (for expected_perm_fee and the other call
at lines 517-529), which hides activation/interval boundary issues; replace the
hardcoded timestamp with the timestamp/height derived from the block context
used in the test (e.g., the block or header used to build the scenario—use that
block's timestamp or corresponding UnixTimestamp/height variable) so the call to
calculate_perm_fee_from_config uses the actual activation-aware pricing context
(keep references to calculate_perm_fee_from_config, expected_perm_fee,
number_of_ingress_proofs_total, and price_before_the_interval).
---
Duplicate comments:
In `@crates/actors/src/block_validation.rs`:
- Around line 2282-2287: Replace the current tracing::error-only check for
tx.promoted_height() in block_validation (the block where tx is validated) with
a proper validation failure: when tx.promoted_height().is_some() return
Err(PreValidationError::...) instead of only logging, including tx.id and a
clear message in the error construction so callers see this as a validation
rejection (update the check in the same function/method that contains the
current snippet referencing tx and tx.id).
In `@crates/chain/tests/api/pricing_endpoint.rs`:
- Around line 687-742: The test
heavy_cascade_pricing_endpoint_term_ledgers_pre_post_activation misleadingly
only exercises post-activation behavior; either rename it to
heavy_cascade_pricing_endpoint_term_ledgers_post_activation, or add a
pre-activation branch: set Cascade.activation_timestamp to a future
UnixTimestamp, call price_endpoint_request(&address,
DataLedger::OneYear/ThirtyDay, ...) and assert a 400 response (and/or
appropriate error body), then advance the chain/time (or mine blocks) past
activation and re-run price_endpoint_request to assert 200 with perm_fee == 0
and term_fee > 0; reference the test function name, the
Cascade.activation_timestamp field, and price_endpoint_request helper when
making changes.
- Around line 823-899: The test
heavy_cascade_pricing_transition_at_exact_activation_height currently sets
Cascade.activation_timestamp to UnixTimestamp::from_secs(0) so Cascade is always
active — change the setup to pick a future activation (e.g., set
activation_timestamp to a nonzero timestamp corresponding to a specific
activation height), compute fees for the pre-activation height and for the
activation height using the existing compute_api_fee closure (which calls
calculate_submit_ledger_expiry and calculate_term_fee), and then exercise
price_endpoint_request both before and after advancing/mine the chain past the
activation point (or start the node with a tip that simulates pre/post
activation) asserting that fee at activation-1 equals the base rate and fee at
activation equals the Cascade rate and that the API responses match those
computed values.
ℹ️ Review info
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (14)
crates/actors/src/block_validation.rscrates/actors/src/validation_service/block_validation_task.rscrates/chain/src/chain.rscrates/chain/tests/api/hardfork_tests.rscrates/chain/tests/api/pricing_endpoint.rscrates/chain/tests/block_production/block_production.rscrates/chain/tests/block_production/block_validation.rscrates/chain/tests/data_sync/sync_partition_data_tests.rscrates/chain/tests/multi_node/validation.rscrates/chain/tests/validation/data_tx_pricing.rscrates/chain/tests/validation/ingress_proof_validation.rscrates/chain/tests/validation/mod.rscrates/chain/tests/validation/unpledge_partition.rscrates/chain/tests/validation/unstake_edge_cases.rs
Comment on lines
+2491
to
2498
| DataLedger::OneYear | DataLedger::ThirtyDay => { | ||
| // Term-only ledger tx should not have any past inclusion | ||
| return Err(PreValidationError::SubmitTxAlreadyIncluded { | ||
| tx_id: tx.id, | ||
| ledger: *ledger_historical, | ||
| block_hash: *historical_block_hash, | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using a more specific error for term ledger duplicate inclusion.
Using SubmitTxAlreadyIncluded for OneYear/ThirtyDay ledger duplicates is semantically inaccurate since these transactions were never in the Submit ledger. Consider creating a more specific error variant like TermLedgerTxAlreadyIncluded for clarity in error messages and debugging.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/actors/src/block_validation.rs` around lines 2491 - 2498, Replace the
generic PreValidationError::SubmitTxAlreadyIncluded used in the
DataLedger::OneYear and DataLedger::ThirtyDay match arm with a new, specific
error variant (e.g. PreValidationError::TermLedgerTxAlreadyIncluded) to
accurately reflect term-ledger duplicate inclusion; add the new enum variant to
PreValidationError and ensure it carries the same context fields (tx_id, ledger,
block_hash), update the match arm here (the branch checking DataLedger::OneYear
| DataLedger::ThirtyDay that currently returns SubmitTxAlreadyIncluded with
tx.id, *ledger_historical, *historical_block_hash) to return the new
TermLedgerTxAlreadyIncluded, and update any error constructors/handlers or tests
that match on SubmitTxAlreadyIncluded where term-ledger semantics apply.
Replace all is_cascade_active_at(timestamp) calls with is_cascade_active_for_epoch(epoch_snapshot), consistent with how borealis activation is checked. Remove the now-unused method to prevent accidental timestamp-based activation checks.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
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/data_txs.rs (1)
362-381:⚠️ Potential issue | 🟠 MajorAvoid mixed canonical snapshots in EMA fee validation.
ema_snapshot/timestamp andlatest_heightare sourced via separate reads. A tip update between those reads can yield inconsistent expected fee calculations.💡 Proposed fix
- let (ema_snapshot, latest_block_timestamp_secs) = { + let (ema_snapshot, latest_block_timestamp_secs, latest_height) = { let tree = self.block_tree_read_guard.read(); let (canonical, _) = tree.get_canonical_chain(); let last_block_entry = canonical .last() .ok_or_else(|| TxIngressError::Other("Empty canonical chain".to_string()))?; let ema = tree .get_ema_snapshot(&last_block_entry.block_hash()) .ok_or_else(|| TxIngressError::Other("EMA snapshot not found".to_string()))?; // Convert block timestamp from millis to seconds let timestamp_secs = last_block_entry.header().timestamp_secs(); - (ema, timestamp_secs) + (ema, timestamp_secs, last_block_entry.height()) }; @@ - let latest_height = self.get_latest_block_height()?; let next_block_height = latest_height + 1;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/actors/src/mempool_service/data_txs.rs` around lines 362 - 381, The code reads the canonical chain/EMA snapshot under self.block_tree_read_guard.read() but calls self.get_latest_block_height() afterwards under a separate read, which can observe a tip update and produce inconsistent fee calculations; fix by obtaining latest_height (and next_block_height) from the same read guard used to build ema_snapshot/timestamp—e.g., inside the block started by let tree = self.block_tree_read_guard.read() use tree.get_canonical_chain() (or an equivalent tree method) to derive latest_height (instead of calling get_latest_block_height()), then compute next_block_height there so ema_snapshot, latest_block_timestamp_secs, pricing_ema, and latest_height are all from the same snapshot.
♻️ Duplicate comments (4)
crates/actors/src/mempool_service/data_txs.rs (1)
214-217:⚠️ Potential issue | 🟠 MajorReject any
perm_feepresence for term-ledger transactions.Current logic only rejects positive values, so
perm_fee = Some(0)is accepted even though term-only ledgers should not carryperm_feeat all.💡 Proposed fix
- if tx - .perm_fee - .is_some_and(|f| f > irys_types::BoundedFee::zero()) - { + if tx.perm_fee.is_some() { return Err(TxIngressError::FundMisalignment( "Term-only ledger transactions must not have a perm_fee".to_string(), )); } @@ - if tx - .perm_fee - .is_some_and(|f| f > irys_types::BoundedFee::zero()) - { + if tx.perm_fee.is_some() { return Err(TxIngressError::FundMisalignment( "Term-only ledger transactions must not have a perm_fee".to_string(), )); }Also applies to: 277-280
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/actors/src/mempool_service/data_txs.rs` around lines 214 - 217, The current check only rejects positive perm_fee values (perm_fee.is_some_and(|f| f > ...)) so perm_fee = Some(0) still passes; change the logic to reject any presence of perm_fee for term-ledger transactions by checking perm_fee.is_some() and returning the same error/path as used now. Update both occurrences that perform this check (the existing block using tx.perm_fee.is_some_and(...) and the similar block around the later occurrence) to use perm_fee.is_some(), preserving the same error handling/return flow and messages from the surrounding function so behavior is consistent.crates/actors/src/block_validation.rs (2)
1660-1664: 🧹 Nitpick | 🔵 TrivialRemove redundant
cascade_activecomputation.
cascade_activeis already computed at lines 1630-1633 with the same method call. This re-computation is unnecessary.♻️ Suggested fix
// When Cascade is active, also process OneYear and ThirtyDay term ledgers. - let cascade_active = config - .consensus - .hardforks - .is_cascade_active_for_epoch(&parent_epoch_snapshot); if cascade_active {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/actors/src/block_validation.rs` around lines 1660 - 1664, Remove the redundant recomputation of cascade_active: there's an earlier calculation of cascade_active using config.consensus.hardforks.is_cascade_active_for_epoch(&parent_epoch_snapshot) (lines around where cascade_active is first defined) so delete the second call and use the existing cascade_active variable instead where the if check currently re-computes it; ensure references to parent_epoch_snapshot remain unchanged and no other logic depends on a fresh call.
2486-2493: 🧹 Nitpick | 🔵 TrivialConsider using a more specific error for term ledger duplicate inclusion.
Using
SubmitTxAlreadyIncludedfor OneYear/ThirtyDay ledger duplicates is semantically inaccurate since these transactions were never in the Submit ledger. Consider creating a more specific error variant likeTermLedgerTxAlreadyIncludedfor clarity in error messages and debugging.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/actors/src/block_validation.rs` around lines 2486 - 2493, The code returns PreValidationError::SubmitTxAlreadyIncluded for DataLedger::OneYear and DataLedger::ThirtyDay duplicates, which is semantically wrong; add a new error variant PreValidationError::TermLedgerTxAlreadyIncluded (carrying the same fields: tx_id, ledger, block_hash) and replace the return in the DataLedger::OneYear | DataLedger::ThirtyDay arm to return that new variant; update any pattern matches, error construction sites, and error display/serialization logic to handle the new TermLedgerTxAlreadyIncluded variant so diagnostics and debugging reflect term-ledger-specific duplicates.crates/actors/src/mempool_service.rs (1)
1174-1204:⚠️ Potential issue | 🟡 MinorSelection telemetry undercounts term-ledger transactions.
The metrics and summary logs still use only
submit_tx.len()for data transaction counts:
- Line 1176:
mempool_selected.data_txs = submit_tx.len()- Line 1186:
total_selected = commitment_tx.len() + submit_tx.len()- Line 1199:
mempool_selected.data_selected = submit_tx.len()After Cascade activation, this undercounts selected data transactions and inflates the rejection rate calculation at line 1190.
🔧 Suggested fix
info!( mempool_selected.commitment_txs = commitment_tx.len(), - mempool_selected.data_txs = submit_tx.len(), + mempool_selected.data_txs = submit_tx.len() + one_year_tx.len() + thirty_day_tx.len(), mempool_selected.publish_txs = publish_txs_and_proofs.txs.len(), mempool_selected.total_fee_collected = ?total_fee_collected, mempool_selected.unfunded_addresses = unfunded_address.len(), "Mempool transaction selection completed" ); // Check for high rejection rate let total_commitments_available = sorted_commitments.len(); let total_available = total_commitments_available + total_data_available; - let total_selected = commitment_tx.len() + submit_tx.len(); + let total_selected = commitment_tx.len() + submit_tx.len() + one_year_tx.len() + thirty_day_tx.len(); if total_available > 0 { const REJECTION_RATE_THRESHOLD: usize = 70; let rejection_rate = ((total_available - total_selected) * 100) / total_available; if rejection_rate > REJECTION_RATE_THRESHOLD { warn!( mempool_selected.rejection_rate = rejection_rate, mempool_selected.total_available = total_available, mempool_selected.total_selected = total_selected, mempool_selected.commitments_available = total_commitments_available, mempool_selected.commitments_selected = commitment_tx.len(), mempool_selected.data_available = total_data_available, - mempool_selected.data_selected = submit_tx.len(), + mempool_selected.data_selected = submit_tx.len() + one_year_tx.len() + thirty_day_tx.len(), mempool_selected.unfunded_addresses = unfunded_address.len(), "High transaction rejection rate detected" ); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/actors/src/mempool_service.rs` around lines 1174 - 1204, The telemetry/logging currently counts only submit_tx for data transactions, undercounting term-ledger (publish) transactions; compute a combined data_selected_count = submit_tx.len() + publish_txs_and_proofs.txs.len() and use that in place of submit_tx.len() wherever data counts are reported or used in calculations (update mempool_selected.data_txs, total_selected = commitment_tx.len() + data_selected_count, and mempool_selected.data_selected), and use data_selected_count when computing the rejection rate; reference variables: submit_tx, publish_txs_and_proofs.txs, commitment_tx, sorted_commitments, total_data_available.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/actors/src/block_validation.rs`:
- Around line 2281-2286: The current tracing::error log in block_validation.rs
that checks tx.promoted_height() should be converted into a validation error:
replace the tracing::error block with code that returns an Err variant when
tx.promoted_height().is_some(); add a new error enum variant (e.g.,
SubmitTxHasPromotedHeight or similar) to your transaction/validation error type
and use it here (include tx.id in the error message/variant payload for
context), and update any callers to propagate this validation failure instead of
allowing the transaction to continue.
In `@crates/actors/src/validation_service/block_validation_task.rs`:
- Around line 396-400: The code currently derives `cascade_active` using
`parent_epoch_snapshot`, which incorrectly enforces pre-Cascade rules for a
block that belongs to the activation epoch; instead compute cascade activation
from the snapshot for the block's epoch (not the parent) — e.g. obtain the epoch
snapshot corresponding to `block.epoch` (or call the existing snapshot resolver
used elsewhere) and pass that to
`config.consensus.hardforks.is_cascade_active_for_epoch(...)` to set
`cascade_active`; update the `cascade_active` assignment in
block_validation_task.rs (replacing the use of `parent_epoch_snapshot`) and
ensure any callers that need the parent snapshot still use
`parent_epoch_snapshot` where appropriate.
In `@crates/api-server/src/routes/price.rs`:
- Around line 160-172: The code currently takes two separate block_tree read
locks which can observe different canonical tips; instead, acquire a single
snapshot from state.block_tree.read() once (via tree.canonical_epoch_snapshot()
or a full snapshot object) and reuse that snapshot for both the Cascade gating
check (is_cascade_active_for_epoch(&epoch_snapshot)) and for computing
fee/pricing inputs later in the function (the sections around startLine 160 and
194-204); replace the separate reads/drops with one read-scope that captures
epoch_snapshot and any fee-related values needed, then drop the lock after all
checks/inputs are derived so gating and pricing use the same snapshot.
---
Outside diff comments:
In `@crates/actors/src/mempool_service/data_txs.rs`:
- Around line 362-381: The code reads the canonical chain/EMA snapshot under
self.block_tree_read_guard.read() but calls self.get_latest_block_height()
afterwards under a separate read, which can observe a tip update and produce
inconsistent fee calculations; fix by obtaining latest_height (and
next_block_height) from the same read guard used to build
ema_snapshot/timestamp—e.g., inside the block started by let tree =
self.block_tree_read_guard.read() use tree.get_canonical_chain() (or an
equivalent tree method) to derive latest_height (instead of calling
get_latest_block_height()), then compute next_block_height there so
ema_snapshot, latest_block_timestamp_secs, pricing_ema, and latest_height are
all from the same snapshot.
---
Duplicate comments:
In `@crates/actors/src/block_validation.rs`:
- Around line 1660-1664: Remove the redundant recomputation of cascade_active:
there's an earlier calculation of cascade_active using
config.consensus.hardforks.is_cascade_active_for_epoch(&parent_epoch_snapshot)
(lines around where cascade_active is first defined) so delete the second call
and use the existing cascade_active variable instead where the if check
currently re-computes it; ensure references to parent_epoch_snapshot remain
unchanged and no other logic depends on a fresh call.
- Around line 2486-2493: The code returns
PreValidationError::SubmitTxAlreadyIncluded for DataLedger::OneYear and
DataLedger::ThirtyDay duplicates, which is semantically wrong; add a new error
variant PreValidationError::TermLedgerTxAlreadyIncluded (carrying the same
fields: tx_id, ledger, block_hash) and replace the return in the
DataLedger::OneYear | DataLedger::ThirtyDay arm to return that new variant;
update any pattern matches, error construction sites, and error
display/serialization logic to handle the new TermLedgerTxAlreadyIncluded
variant so diagnostics and debugging reflect term-ledger-specific duplicates.
In `@crates/actors/src/mempool_service.rs`:
- Around line 1174-1204: The telemetry/logging currently counts only submit_tx
for data transactions, undercounting term-ledger (publish) transactions; compute
a combined data_selected_count = submit_tx.len() +
publish_txs_and_proofs.txs.len() and use that in place of submit_tx.len()
wherever data counts are reported or used in calculations (update
mempool_selected.data_txs, total_selected = commitment_tx.len() +
data_selected_count, and mempool_selected.data_selected), and use
data_selected_count when computing the rejection rate; reference variables:
submit_tx, publish_txs_and_proofs.txs, commitment_tx, sorted_commitments,
total_data_available.
In `@crates/actors/src/mempool_service/data_txs.rs`:
- Around line 214-217: The current check only rejects positive perm_fee values
(perm_fee.is_some_and(|f| f > ...)) so perm_fee = Some(0) still passes; change
the logic to reject any presence of perm_fee for term-ledger transactions by
checking perm_fee.is_some() and returning the same error/path as used now.
Update both occurrences that perform this check (the existing block using
tx.perm_fee.is_some_and(...) and the similar block around the later occurrence)
to use perm_fee.is_some(), preserving the same error handling/return flow and
messages from the surrounding function so behavior is consistent.
ℹ️ Review info
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
crates/actors/src/block_validation.rscrates/actors/src/mempool_service.rscrates/actors/src/mempool_service/data_txs.rscrates/actors/src/validation_service/block_validation_task.rscrates/api-server/src/routes/price.rscrates/chain/tests/block_production/block_validation.rscrates/domain/src/snapshots/epoch_snapshot/epoch_snapshot.rscrates/types/src/hardfork_config.rs
Merge two separate block_tree.read() calls into one lock scope so cascade gating and fee/pricing inputs observe the same canonical tip.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/api-server/src/routes/price.rs`:
- Around line 209-223: Extract the EMA pricing selection logic into a shared
helper function (e.g. select_pricing_ema) that accepts the ema reference plus
next_block_height and price_adjustment_interval and returns the chosen
pricing_ema; inside the helper replicate the current logic that computes
position_in_interval, last_quarter_start, in_last_quarter and returns either
ema.ema_price_1_interval_ago or ema.ema_price_2_intervals_ago when
in_last_quarter, otherwise ema.ema_for_public_pricing(). Replace the duplicated
blocks in both the Publish path and the term ledger path with a call like
select_pricing_ema(&ema, next_block_height, price_adjustment_interval) to ensure
consistency.
Comment on lines
+209
to
+223
| let price_adjustment_interval = state.config.consensus.ema.price_adjustment_interval; | ||
| let position_in_interval = next_block_height % price_adjustment_interval; | ||
| let last_quarter_start = | ||
| price_adjustment_interval - price_adjustment_interval.div_ceil(4); | ||
| let in_last_quarter = position_in_interval >= last_quarter_start; | ||
|
|
||
| let pricing_ema = if in_last_quarter { | ||
| if ema.ema_price_1_interval_ago.amount < ema.ema_price_2_intervals_ago.amount { | ||
| ema.ema_price_1_interval_ago | ||
| } else { | ||
| ema.ema_price_2_intervals_ago | ||
| } | ||
| } else { | ||
| ema.ema_for_public_pricing() | ||
| }; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting the EMA pricing selection logic to reduce duplication.
This EMA pricing selection logic is nearly identical to lines 90–104 in the Publish path. Extracting it into a helper function would improve maintainability and ensure consistency if the logic changes.
♻️ Proposed refactor
+fn select_pricing_ema(
+ ema: &EmaSnapshot,
+ next_block_height: u64,
+ price_adjustment_interval: u64,
+) -> Amount<(IrysPrice, Usd)> {
+ let position_in_interval = next_block_height % price_adjustment_interval;
+ let last_quarter_start =
+ price_adjustment_interval - price_adjustment_interval.div_ceil(4);
+ let in_last_quarter = position_in_interval >= last_quarter_start;
+
+ if in_last_quarter {
+ if ema.ema_price_1_interval_ago.amount < ema.ema_price_2_intervals_ago.amount {
+ ema.ema_price_1_interval_ago
+ } else {
+ ema.ema_price_2_intervals_ago
+ }
+ } else {
+ ema.ema_for_public_pricing()
+ }
+}Then use let pricing_ema = select_pricing_ema(&ema, next_block_height, price_adjustment_interval); in both the Publish and term ledger paths.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/api-server/src/routes/price.rs` around lines 209 - 223, Extract the
EMA pricing selection logic into a shared helper function (e.g.
select_pricing_ema) that accepts the ema reference plus next_block_height and
price_adjustment_interval and returns the chosen pricing_ema; inside the helper
replicate the current logic that computes position_in_interval,
last_quarter_start, in_last_quarter and returns either
ema.ema_price_1_interval_ago or ema.ema_price_2_intervals_ago when
in_last_quarter, otherwise ema.ema_for_public_pricing(). Replace the duplicated
blocks in both the Publish path and the term ledger path with a call like
select_pricing_ema(&ema, next_block_height, price_adjustment_interval) to ensure
consistency.
Collaborator
Author
Changes since reviewAddressing all feedback from @roberts-pumpurs plus follow-up cleanup:
|
roberts-pumpurs
approved these changes
Mar 2, 2026
Contributor
roberts-pumpurs
left a comment
roberts-pumpurs
left a comment
There was a problem hiding this comment.
overall lgtm. posted some comments. make sure to do rounds with coderabbit on all critical / major / minor things it has highlighted
Comment on lines
+66
to
+77
| // Temporarily override any global panic hook that aborts the process. | ||
| let prev_hook = std::panic::take_hook(); | ||
| std::panic::set_hook(Box::new(|_| {})); | ||
|
|
||
| let result = std::panic::catch_unwind(|| { | ||
| let _guard = ActiveWorkerGuard::new(counter.clone(), notify.clone()); | ||
| assert_eq!(counter.load(Ordering::Relaxed), 1); | ||
| panic!("Simulated panic"); | ||
| }); | ||
|
|
||
| std::panic::set_hook(prev_hook); | ||
|
|
Contributor
There was a problem hiding this comment.
we use cargo nextest instead of normal cargo test. is this finding still relevant?
Co-authored-by: Roberts Pumpurs <33699735+roberts-pumpurs@users.noreply.github.com>
Replace raw timestamp comparison against cascade.activation_timestamp with is_cascade_active_for_epoch(epoch_snapshot) when building block data ledgers, consistent with all other activation checks.
The previous check `perm_fee.is_some_and(|f| f > zero())` allowed `Some(0)` through. Term-ledger txs must not carry a perm_fee at all.
…ress Submit is not user-targetable — a malicious peer could still attempt to gossip submit-level txs. Return TxIngressError::InvalidLedger instead of panicking.
Replace `continue` on invalid ledger ID with LedgerIdInvalid error, and replace `.expect()` on cascade config with proper error propagation.
Convert the tracing::error! for Submit ledger transactions that have a promoted_height tag into a proper PreValidationError, making it a block rejection rather than a silent log.
Clear promoted_height and included_height metadata from data transaction headers before including them in the block body. This metadata is mempool-internal state and must not leak into produced blocks, where it would cause false SubmitTxHasPromotedHeight validation failures on self-produced blocks (particularly after node restart when the mempool reconstructs metadata from DB).
After node restart, the mempool reconstructs promoted_height metadata from the DB. Previously these txs were re-selected as submit txs (just not re-promoted), wasting block space and causing validation failures from the SubmitTxHasPromotedHeight check. Now the mempool skips them entirely during block tx selection.
Term-only ledger transactions had their fees validated but no shadow transactions were generated to actually debit user balances and credit the treasury on the EVM layer. This adds a TermLedger phase to the shadow tx generator that processes one_year and thirty_day txs with the same 5% block-producer / 95% treasury split as submit txs. Also adds integration tests verifying wallet balance decrements after term ledger tx inclusion and treasury non-negativity through expiry.
M1: Replace HashSet with BTreeSet for miner deduplication in fee distribution so the remainder is assigned deterministically across nodes. M2: Sort one_year_tx and thirty_day_tx before block inclusion so all nodes compute identical merkle roots for the same tx set. M3: Add ledger_id validation for Publish, OneYear, and ThirtyDay tx sets in the structural pre-pass, matching the existing Submit check.
Resolves test file relocations from crates/chain/tests/ to crates/chain-tests/src/ and adds irys-config dependency.
Add independent partition count for term ledger slots (OneYear/ThirtyDay), replacing hardcoded replica count of 1. Defaults to same value as num_partitions_per_slot in all configs.
…-cost # Conflicts: # crates/actors/src/mempool_service.rs # crates/actors/src/mempool_service/data_txs.rs # crates/actors/src/mempool_service/lifecycle.rs
Add num_partitions_per_term_ledger_slot to TOML deserialization test and testnet config template. Update consensus hash regression test. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements the Cascade hardfork, a height-based protocol upgrade that:
annual_cost_per_gbfrom $0.01 to $0.028 when Cascade activates, making term fees economically viable for minersCascade hardfork config
New
Cascadestruct onIrysHardforkConfigwith height-based activation (must align with epoch boundary):activation_height— block height at which OneYear/ThirtyDay ledgers and new pricing become activeone_year_epoch_length/thirty_day_epoch_length— configurable term durationsannual_cost_per_gb— override for storage cost (default $0.028, serde-defaulted)Term ledger support
DataLedger::ALLexpanded from[Publish, Submit]to include[OneYear, ThirtyDay]LedgerExpiryBalanceDelta::merge()replica_count=1for OneYear/ThirtyDayactivate_cascade()adds term ledgers at epoch boundaryLedgers::new()takescascade_activeflag;activate_cascade()for mid-chain activationHeight-aware pricing
calculate_term_fee(),calculate_term_fee_from_config(),calculate_perm_fee_from_config()all takeheight: u64ConsensusConfig::effective_annual_cost_per_gb(height)returns Cascade override ($0.028) if active, else base ($0.01)ConsensusConfig::cost_per_chunk_per_epoch_at(height)for height-aware cost computationAPI changes
/priceendpoint supports OneYear/ThirtyDay (returnsperm_fee=0,term_fee>0)replica_count=1for term-only ledgersTest plan
New integration tests (4 new test files, 619 lines)
cascade_reorg— term ledger txs in orphaned blocks re-injected to mempoolcascade_block_rejection— blocks with term ledgers rejected when Cascade inactivecascade_ledger_shape— correct ledger count/structure at activation boundarycascade_term_expiry— OneYear/ThirtyDay expire at different rates per epoch_length configNew pricing integration tests
heavy_cascade_pricing_endpoint_uses_higher_annual_cost— API fees increase ~2.8x post-Cascadeheavy_cascade_pricing_transition_at_exact_activation_height— exact boundary: block N-1 uses base, block N uses Cascadeheavy_cascade_mempool_rejects_publish_tx_with_pre_cascade_pricing— pre-Cascade-priced tx rejected post-Cascadeheavy_cascade_pricing_endpoint_term_ledgers_pre_post_activation— OneYear/ThirtyDay unavailable→available at boundaryheavy_cascade_mempool_ingress_boundary_for_term_ledgers— full ingress validation for term ledgersNew unit tests (40+ tests across 8 files)
Verification
cargo fmt --allcargo clippy --workspace --tests --all-targets🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Tests