fix: add non-zero balance checks and moved tx ingestion to post-val#1083
fix: add non-zero balance checks and moved tx ingestion to post-val#1083glottologist wants to merge 2 commits intomasterfrom
Conversation
glottologist
force-pushed
the
jason/tx_ingestion
branch
2 times, most recently
from
475f3ec to
5b711a8
Compare
glottologist
force-pushed
the
jason/tx_ingestion
branch
from
5b711a8 to
3c45a6d
Compare
| ); | ||
| } | ||
| } | ||
| MempoolServiceMessage::BlockTransactionsValidated { |
There was a problem hiding this comment.
Looks good to me, the only thing is I don't entirely get why would be re-ingest those after the successful validation here - is it to account the transactions that made it to the mempool not from the gossip, but from the block body?
There was a problem hiding this comment.
This is so that they can be propogated to other nodes
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughAdds a new Changes
Sequence Diagram(s)sequenceDiagram
participant BP as Block Pool
participant VS as Validation Service
participant VC as Validation Coordinator
participant MS as Mempool Service
rect rgb(235,245,255)
note over BP: Block discovery stage
BP->>BP: Cache data roots for Submit txs
BP->>VS: Submit block for validation
end
rect rgb(245,255,235)
note over VS,VC: Validation flow (concurrent)
VS->>VC: process_vdf(block)
VC->>VC: Spawn concurrent validation (includes block, transactions)
VC-->>VS: ConcurrentValidationResult (Valid + block + transactions)
end
rect rgb(255,250,235)
note over VS,MS: Post-validation ingestion
VS->>VS: Detect Valid result
VS->>MS: Send BlockTransactionsValidated(block, transactions)
MS->>MS: handle_block_transactions_validated()
MS->>MS: ingest_validated_commitment_tx / ingest_validated_data_tx
MS-->>MS: Transactions inserted into mempool (skip gossip balance checks)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (3)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2026-01-05T16:21:25.918ZApplied to files:
🧬 Code graph analysis (1)crates/actors/src/mempool_service.rs (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
🔇 Additional comments (3)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
crates/actors/src/mempool_service.rscrates/actors/src/mempool_service/commitment_txs.rscrates/actors/src/mempool_service/data_txs.rscrates/actors/src/mempool_service/lifecycle.rscrates/actors/src/validation_service.rscrates/actors/src/validation_service/active_validations.rscrates/chain/tests/multi_node/mempool_tests.rscrates/p2p/src/block_pool.rscrates/p2p/src/tests/integration/mod.rs
🧰 Additional context used
🧬 Code graph analysis (5)
crates/actors/src/mempool_service/lifecycle.rs (4)
crates/types/src/block.rs (2)
block_hash(266-268)height(271-273)crates/actors/src/block_discovery.rs (1)
all_data_txs(96-98)crates/types/src/transaction.rs (5)
id(362-364)id(395-395)id(430-432)id(508-510)id(599-604)crates/types/src/commitment_transaction.rs (3)
id(215-217)id(502-504)id(655-660)
crates/p2p/src/block_pool.rs (2)
crates/actors/src/mempool_service/data_txs.rs (1)
db_tx(448-448)crates/database/src/database.rs (1)
cache_data_root(166-216)
crates/actors/src/mempool_service/commitment_txs.rs (2)
crates/types/src/commitment_transaction.rs (6)
id(215-217)id(502-504)id(655-660)signer(219-221)signer(506-508)signer(682-687)crates/reth-node-bridge/src/ext.rs (2)
balance(17-21)balance(29-40)
crates/actors/src/mempool_service/data_txs.rs (1)
crates/reth-node-bridge/src/ext.rs (2)
balance(17-21)balance(29-40)
crates/p2p/src/tests/integration/mod.rs (2)
crates/types/src/serialization.rs (2)
len(632-634)len(705-707)crates/database/src/data_ledger.rs (1)
len(280-282)
🔇 Additional comments (18)
crates/chain/tests/multi_node/mempool_tests.rs (1)
2513-2513: Test adjustment aligns with non-zero balance DoS protection.The change from
U256::from(0)toU256::from(1)correctly adapts the test to the new gossip-path validation that rejects zero-balance accounts. The test still validates the insufficient-balance scenario while ensuring the account passes the initial non-zero balance check introduced for DoS protection.crates/actors/src/mempool_service/commitment_txs.rs (2)
160-163: Non-zero balance DoS protection correctly implemented for gossip path.The implementation properly:
- Fetches balance accounting for both canonical and pending state
- Rejects gossip transactions from zero-balance accounts to prevent DoS
- Maintains separate validation logic for gossip vs. API paths (full balance validation only on API path)
- Provides clear error messages and logging
The comment updates clarify the rationale: full balance validation is skipped for gossip since balances may differ across forks, but the non-zero check prevents resource exhaustion attacks.
Also applies to: 166-167, 255-281
4-4: Verify the production use of test-context extension trait.The import
IrysRethRpcTestContextExtstrongly suggests test-only use—it's implemented exclusively forRpcTestContextfromreth_e2e_test_utils, yet it's being called in production code at line 264 viaself.reth_node_adapter.rpc.get_balance_irys_canonical_and_pending(). Either:
- Rename the trait to avoid confusion if this is intentional production use
- Implement this method on a production-appropriate type instead
crates/p2p/src/tests/integration/mod.rs (1)
282-293: Test correctly updated to reflect post-validation ingestion flow.The test now validates that the block is received and queued for validation (in
discovery_blocks) rather than checking mempool contents directly. The comments clearly explain that transactions are now ingested into mempool after validation succeeds viaBlockTransactionsValidated, not during block pool processing. This aligns with the PR's architectural change to defer mempool ingestion until post-validation.crates/actors/src/validation_service.rs (1)
267-281: Validation-to-mempool propagation correctly implemented.The code properly:
- Gates the message send on successful validation (
ValidationResult::Valid)- Forwards both the block header and transactions to the mempool service
- Handles send failures gracefully with error logging (non-fatal, as the validation result still propagates to the block tree)
- Uses Arc clones for efficient data sharing
This completes the new validation flow where mempool ingestion occurs after validation rather than during block discovery.
crates/p2p/src/block_pool.rs (1)
12-12: Import addition required for database operations.The
Databasetrait import is necessary for thedb.update()call at line 827.crates/actors/src/mempool_service.rs (3)
204-208: LGTM - Well-structured message variant.The new
BlockTransactionsValidatedvariant follows existing patterns and uses appropriate types (Arc<IrysBlockHeader>,Arc<BlockTransactions>).
278-278: LGTM - Variant name correctly mapped.
324-341: LGTM - Message handling follows established patterns.The error handling mirrors the
BlockConfirmedcase (lines 312-322), appropriately logging errors without failing the service. As noted in past review comments, this enables transaction propagation to other nodes.crates/actors/src/mempool_service/data_txs.rs (4)
8-8: LGTM - Necessary import for balance validation.
163-164: LGTM - DoS protection correctly positioned.The non-zero balance check is appropriately placed after basic precheck validation and before ledger-specific processing, providing effective DoS protection.
166-184: LGTM - Clear rationale for gossip validation strategy.The updated comments effectively explain why gossip paths skip full EMA/balance validation while still checking for non-zero balance as a DoS prevention measure.
287-313: LGTM - Non-zero balance validation implemented correctly.The method appropriately:
- Fetches canonical + pending balance for current chain state
- Returns
Unfundederror for zero-balance accounts- Uses debug-level logging for expected rejections
- Handles RPC errors with descriptive error messages
crates/actors/src/mempool_service/lifecycle.rs (3)
128-166: LGTM - Block transaction ingestion correctly implemented.The method appropriately:
- Logs ingestion counts for observability
- Processes commitment and data transactions separately
- Continues processing on individual failures (except for already-known
Skippederrors)- Returns success even if some transactions fail to ingest (appropriate for batch processing)
168-188: LGTM - Validated commitment ingestion correctly bypasses gossip checks.The method appropriately:
- Skips already-known transactions
- Uses
insert_commitment_and_mark_validfor bounded insertion- Bypasses balance validation (as documented) since validation already occurred
- Logs successful ingestion for observability
190-209: No action needed. Thebounded_insert_data_txmethod called at line 205 ofingest_validated_data_tx(the async version at line 1950 ofmempool_service.rs) already updatesrecent_valid_txon line 1960, just likeinsert_commitment_and_mark_validdoes. There is no inconsistency in how validated data and commitment transactions are tracked.Likely an incorrect or invalid review comment.
crates/actors/src/validation_service/active_validations.rs (2)
82-87: LGTM - Result structure appropriately extended.The new
blockandtransactionsfields enable downstream consumers (like mempool service) to access validated block data. Arc usage is appropriate for async task boundaries.
344-365: LGTM - Block and transactions correctly propagated through validation pipeline.The implementation appropriately:
- Clones Arc references (efficient, no deep copy)
- Passes validated data to concurrent validation task
- Populates result with block and transactions for downstream use
| // Cache data roots before validation (needed for publish tx ingress proof validation). | ||
| // Full tx ingestion happens after validation via BlockTransactionsValidated. | ||
| for data_tx in block_transactions.get_ledger_txs(DataLedger::Submit) { | ||
| if let Err(e) = self.db.update(|db_tx| { | ||
| irys_database::cache_data_root(db_tx, data_tx, Some(&block_header))?; | ||
| Ok::<_, eyre::Report>(()) | ||
| }) { | ||
| warn!("Failed to cache data_root for tx {}: {:?}", data_tx.id, e); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the cache_data_root function definition and its signature
ast-grep --pattern 'fn cache_data_root($$$) { $$$ }'Repository: Irys-xyz/irys
Length of output: 39
🏁 Script executed:
# Search for how publish transaction validation depends on cached data roots
rg "cache_data_root|publish.*ingress.*proof" -A 5 -B 5Repository: Irys-xyz/irys
Length of output: 28417
🏁 Script executed:
# Check the irys_database module to understand cache_data_root
fd "irys_database" -t f | head -20Repository: Irys-xyz/irys
Length of output: 39
🏁 Script executed:
# Read the cache_data_root function implementation
sed -n '1,100p' crates/database/src/database.rs | grep -A 50 "fn cache_data_root"Repository: Irys-xyz/irys
Length of output: 39
🏁 Script executed:
# Get more context around cache_data_root implementation
sed -n '1,500p' crates/database/src/database.rs | tail -100Repository: Irys-xyz/irys
Length of output: 2802
🏁 Script executed:
# Search for how publish tx validation uses cached data roots
rg "cache_data_root|CachedDataRoots" -A 3 -B 3 crates/actors/src/block_validation.rsRepository: Irys-xyz/irys
Length of output: 39
🏁 Script executed:
# Look for publish transaction ingress proof validation logic
rg "ingress.*proof.*validat|publish.*ledger" -i crates/actors/src/block_validation.rs | head -20Repository: Irys-xyz/irys
Length of output: 1471
🏁 Script executed:
# Find where store_ingress_proof_checked and store_external_ingress_proof_checked are called
rg "store_ingress_proof_checked|store_external_ingress_proof_checked" -B 3 -A 3Repository: Irys-xyz/irys
Length of output: 3910
🏁 Script executed:
# Get the full cache_data_root implementation from database.rs
rg -A 30 "pub fn cache_data_root" crates/database/src/database.rsRepository: Irys-xyz/irys
Length of output: 1299
🏁 Script executed:
# Check what errors cache_data_root can return and under what conditions
rg "cache_data_root" -B 5 -A 10 crates/database/src/database.rs | head -80Repository: Irys-xyz/irys
Length of output: 618
🏁 Script executed:
# Check the sequence of operations in block_pool.rs around the cache_data_root call
sed -n '800,850p' crates/p2p/src/block_pool.rsRepository: Irys-xyz/irys
Length of output: 2112
🏁 Script executed:
# Check where store_ingress_proof_checked gets called in block validation/discovery flow
rg "store_ingress_proof|store_external_ingress_proof" -B 5 crates/actors/src/block_discovery.rs crates/actors/src/block_validation.rs 2>/dev/null | head -40Repository: Irys-xyz/irys
Length of output: 39
🏁 Script executed:
# Check what happens during publish transaction processing in block discovery
sed -n '1,100p' crates/actors/src/block_discovery.rsRepository: Irys-xyz/irys
Length of output: 3636
🏁 Script executed:
# Search for the full cache_data_root implementation to see all possible error cases
rg -A 50 "pub fn cache_data_root" crates/database/src/database.rs | head -80Repository: Irys-xyz/irys
Length of output: 2009
🏁 Script executed:
# Find where ingress proofs are stored during block validation/processing
rg "store_ingress_proof|store_external_ingress_proof" -B 10 crates/actors/src/block_validation.rs crates/actors/src/block_discovery.rs | head -100Repository: Irys-xyz/irys
Length of output: 39
🏁 Script executed:
# Check how publish ledger validation processes ingress proofs
rg -A 20 "validate_publish_ledger|PublishLedger" crates/actors/src/block_validation.rs | head -80Repository: Irys-xyz/irys
Length of output: 3578
🏁 Script executed:
# Search for where ingress proofs are being written to the database during block processing
rg "IngressProofs|store.*proof" crates/actors/src/block_validation.rs crates/actors/src/block_discovery.rsRepository: Irys-xyz/irys
Length of output: 471
🏁 Script executed:
# Look at the full block validation flow - where ingress proofs are stored
rg "BlockTransactionsValidated|BlockValidated" -A 10 crates/actors/src/mempool_service/lifecycle.rs | head -60Repository: Irys-xyz/irys
Length of output: 39
🏁 Script executed:
# Search for where publish transaction proofs are processed in block discovery
rg -A 30 "process.*publish|handle_block" crates/actors/src/block_discovery.rs | head -80Repository: Irys-xyz/irys
Length of output: 1622
🏁 Script executed:
# Check the block handler flow that uses cached data roots
rg "cached_data_root_by_data_root" -B 5 -A 10Repository: Irys-xyz/irys
Length of output: 17469
🏁 Script executed:
# Look at what happens after BlockTransactionsValidated - where proofs are stored
rg "BlockTransactionsValidated" -A 20 crates/actors/src/mempool_service/lifecycle.rsRepository: Irys-xyz/irys
Length of output: 39
🏁 Script executed:
# Find the exact panic location in block_validation.rs where cached_data_root is accessed
rg -B 5 -A 15 'panic.*CachedDataRoot should be found' crates/actors/src/block_validation.rsRepository: Irys-xyz/irys
Length of output: 39
🏁 Script executed:
# Check if there are other places in block validation that assume cached data roots exist
rg 'cached_data_root_by_data_root' -A 5 crates/actors/src/block_validation.rs | head -60Repository: Irys-xyz/irys
Length of output: 654
🏁 Script executed:
# Search for all callers of store_ingress_proof_checked to see how failures are handled
rg 'store_ingress_proof_checked|store_external_ingress_proof_checked' -B 2 -A 5 | grep -A 5 "block.*validat"Repository: Irys-xyz/irys
Length of output: 39
🏁 Script executed:
# Confirm the full context of the panic and when it's triggered
sed -n '550,620p' crates/actors/src/block_validation.rsRepository: Irys-xyz/irys
Length of output: 2628
🏁 Script executed:
# Check how often this validation path is executed (is it critical path?)
rg "validate_ingress_proof|validate_publish_ledger" -B 3 -A 10 crates/actors/src/block_validation.rs | head -80Repository: Irys-xyz/irys
Length of output: 39
🏁 Script executed:
# Find the exact validation function that uses cached_data_root and panics if missing
rg -B 10 -A 30 "CachedDataRoot should be found" crates/actors/src/block_validation.rsRepository: Irys-xyz/irys
Length of output: 1817
🏁 Script executed:
# Check where this validation is called in the block validation flow
rg "validate_ingress_proof_signer_uniqueness|validate.*publish" -A 20 crates/actors/src/block_validation.rs | head -100Repository: Irys-xyz/irys
Length of output: 1843
Cache failures in block processing can cause validation panics downstream.
The code silently continues after cache_data_root failures in the Submit ledger, but publish transaction validation in block_validation.rs has an explicit panic requirement for these cached entries when validating ingress proofs. If caching fails, the block will reach validation only to panic with "CachedDataRoot should be found for data_root..." during ingress proof validation—a critical validation path.
Rather than silent degradation followed by validation crashes, consider treating cache failures as fatal for the block processing step, or add explicit validation to catch missing cached data roots before they cause panics.
2 participants
Describe the changes
Before:
Gossip transactions were ingested into the mempool in
block_pool.rsbefore block validation, allowing inclusion of unfunded txs.After:
Transaction ingestion moved to post-validation via
BlockTransactionsValidatedmessage. Gossip path now rejects transactions from zero-balance accounts.Changes
BlockTransactionsValidatedmessage type for post-validation tx ingestionhandle_block_transactions_validated()handler inlifecycle.rsingest_validated_commitment_tx()andingest_validated_data_tx()functions that bypass gossip-path balance checksvalidate_gossip_nonzero_balance()indata_txs.rs- rejects data txs from zero-balance accountsvalidate_commitment_gossip_nonzero_balance()incommitment_txs.rs- rejects commitment txs from zero-balance accountshandle_data_transaction_ingress_gossip,handle_commitment_transaction_ingress_gossip)irys_database::cache_data_root()before validation (required for publish tx ingress proof validation)validation_service.rsto sendBlockTransactionsValidatedto the mempool on successful validationConcurrentValidationResultinactive_validations.rsto includeblockandtransactionsfieldsRelated Issue(s)
Please link to the issue(s) that will be closed with this PR.
Checklist
Additional Context
Add any other context about the pull request here.
Summary by CodeRabbit
Security
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.