-
Notifications
You must be signed in to change notification settings - Fork 20
feat (chain): implement genesis UTXO extraction workaround for missing events #100
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
764c696
fix: add fallback hash for UnshieldedTokens events without transactio…
cosmir17 af55ab3
feat: implement genesis UTXO extraction workaround
cosmir17 c00de3f
feat: implement genesis UTXO extraction workaround
cosmir17 4168285
fmt
cosmir17 a09cd39
fix(indexer): reset transaction hash context after processing Unshiel…
cosmir17 4fd806b
addressed pr comments (phase 2)
cosmir17 e6552fb
addressed pr comments
cosmir17 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,67 @@ | ||
| use anyhow::Context; | ||
| use chain_indexer::{ | ||
| domain::Node, | ||
| infra::node::{Config, SubxtNode}, | ||
| }; | ||
| use futures::{StreamExt, TryStreamExt}; | ||
| use indexer_common::domain::{NetworkId, PROTOCOL_VERSION_000_013_000}; | ||
| use std::{pin::pin, time::Duration}; | ||
|
|
||
| /// Simple test to verify connection to midnight-node and basic block retrieval. | ||
| /// Note: This test bypasses the full indexing pipeline and calls the node interface | ||
| /// directly via `node.finalized_blocks()`. As a result, it doesn't trigger the | ||
| /// genesis UTXO extraction that happens in the zswap transaction processing layer. | ||
| /// | ||
| /// For proper genesis UTXO extraction testing, use the e2e tests which go through | ||
| /// the complete indexing pipeline. | ||
| /// | ||
| /// Background: | ||
| /// - Genesis blocks don't emit UnshieldedTokens events due to Substrate PR #5463. | ||
| /// - Genesis UTXO extraction is integrated into zswap transaction processing. | ||
| /// - Full extraction only occurs when blocks are processed through the indexing pipeline. | ||
| #[tokio::main] | ||
| async fn main() -> anyhow::Result<()> { | ||
| // Note: logging is disabled for this simple test | ||
|
|
||
| let config = Config { | ||
| url: "ws://localhost:9944".to_string(), | ||
| genesis_protocol_version: PROTOCOL_VERSION_000_013_000, | ||
| reconnect_max_delay: Duration::from_secs(1), | ||
| reconnect_max_attempts: 3, | ||
| }; | ||
| let mut node = SubxtNode::new(config).await.context("create SubxtNode")?; | ||
|
|
||
| let blocks = node.finalized_blocks(None, NetworkId::Undeployed).take(3); | ||
| let mut blocks = pin!(blocks); | ||
|
|
||
| while let Some(block) = blocks.try_next().await.context("get next block")? { | ||
| println!("## BLOCK: height={}, \thash={}", block.height, block.hash); | ||
|
|
||
| // For genesis block, note that UTXO extraction doesn't happen in this test | ||
| if block.height == 0 { | ||
| println!("*** GENESIS BLOCK DETECTED ***"); | ||
|
|
||
| let utxo_count = block | ||
| .transactions | ||
| .get(0) | ||
| .map(|t| t.created_unshielded_utxos.len()) | ||
| .unwrap_or(0); | ||
|
|
||
| println!( | ||
| "*** UTXOs: {} (extraction requires full indexing pipeline) ***", | ||
| utxo_count | ||
| ); | ||
| } | ||
|
|
||
| for transaction in &block.transactions { | ||
| println!( | ||
| " ## TRANSACTION: hash={}, created_utxos={}, spent_utxos={}", | ||
| transaction.hash, | ||
| transaction.created_unshielded_utxos.len(), | ||
| transaction.spent_unshielded_utxos.len() | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| Ok(()) | ||
| } |
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
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
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not understand this change. Please add a better/more descriptive comment.
I thought that for the genesis block there are not events, so how is this change related to the topic of this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enhanced comment with context.
You're absolutely correct that genesis blocks don't emit events - that's exactly why we need the workaround in application.rs.
This runtimes.rs change is not related to genesis events. It's a separate fix for failed transactions in general (any block height) that don't have transaction context.
The issue: When transactions fail and no TxStart/TxSuccess event provides context, current_tx_hash becomes None. But UnshieldedTokens events can still be emitted for these failed transactions. Without a transaction hash, these events would be dropped.
The fix: Use a fallback hash [0u8; 32] to group events from failed transactions, ensuring they're still processed even without transaction context.
This change improves event processing reliability for failed transactions across all blocks, not just genesis. The genesis-specific logic is entirely in application.rs where we extract UTXOs directly from ledger state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realise my explanation was incorrect.
the [0u8; 32] fallback isn't about 'completely failed transactions' having valid UTXOs.
What's actually happening:
- Empty transactions (no UTXO changes)
- Partial transaction failures where successful segments still emit events
- Event ordering issues
Given that unshielded functionality appears to be incomplete/unstable in midnight-node, this change might be addressing event processing gaps rather than legitimate UTXO creation from failed transactions.
Should I investigate the specific scenarios where current_tx_hash becomes None to verify these events are actually valid? The midnight-node codebase suggests there may be broader unshielded functionality issues we need to account for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest you ask Andy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, while not part of this PR, I think that there is no code setting the
current_tx_hashback toNoneat the end of processing theUnshieldedTokensevent. So if the "in the next round" there is noTxAppliedorTxPartialSuccess, then the oldcurrent_tx_hashwill be used.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am doing an investigation now.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're absolutely right to question this, and I've now done a thorough investigation in the midnight-node codebase to understand exactly when current_tx_hash is None.
Key Finding: The [0u8; 32] fallback hash addresses legitimate system transactions, not failed user transactions.
Primary scenario: System transactions like block rewards that create UTXOs via mint_coins without going through normal transaction processing:
// In on_finalize - Block reward minting (pallets/midnight/src/lib.rs)
match LedgerApi::mint_coins(&network_id, &state_key, reward, &beneficiary[..]) {
Ok(new_state_key) => {
Self::deposit_event(Event::PayoutMinted(...));
}
}
These system operations:
Your concern about completely failed transactions having valid UTXOs doesn't apply here because:
The fallback hash is legitimate and correctly handles system transactions that create UTXOs outside the normal transaction flow. These represent actual ledger state changes that the indexer should capture.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a (short) respective comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're absolutely right - that's a real bug I missed! The current_tx_hash isn't reset to None after processing, so subsequent UnshieldedTokens events without corresponding TxApplied/TxPartialSuccess events would incorrectly use stale transaction hashes from previous transactions.
This means the fallback hash mechanism isn't working as intended. We need to fix this by resetting the transaction hash context appropriately.
The fix should be:
Event::Midnight(midnight::Event::TxApplied(tx_applied)) => {
current_tx_hash = Some(tx_applied.tx_hash);
}
Event::Midnight(midnight::Event::TxPartialSuccess(tx_partial)) => {
current_tx_hash = Some(tx_partial.tx_hash);
}
Event::Midnight(midnight::Event::UnshieldedTokens(event_data)) => {
let tx_hash = current_tx_hash.unwrap_or_else(|| [0u8; 32].into());
// ... process event
current_tx_hash = None; // Reset after processing
}
Good catch - this explains why the fallback might not be working as expected in practice!
made a commit