fix(store): stop registering note tags for output notes#2323
Merged
Conversation
…ked ones Output notes are committed (with inclusion proofs) through account-matched transaction sync, so their tags were dead weight, and with tag cleanup only covering input notes they leaked one row per created note. A store migration prunes the leaked tags (keeping those still needed by pre-inclusion input notes), collapses duplicate rows, and adds a unique (tag, source) index. Sync-time cleanup now also removes tags of input notes consumed within the same sync, and migration schema versions and integrity hashes are committed atomically via migration hooks.
Expresses the "note sync can still advance this record" predicate used by tag cleanup and erased-note handling, mirroring the prune migration's guard. Also drops the custom-sync warning from sync_state: accounts cannot be removed from a client, so the StateSyncInput doc contract is sufficient.
(cherry picked from commit 7bbea6c)
…sync delta Extends MockRpcApi with per-block account states (set_account_state_at) so get_account can answer block-pinned queries with a state other than the chain's latest, and adds a regression test reproducing the 2026-07-13 testnet faucet outage scenario: the client's own transaction commits between the sync delta snapshot and the account fetch. With the fetch pinned to the sync target the update is correctly ignored; with an unpinned (chain tip) fetch the equal-nonce branch would falsely discard the committed transaction as Superseded (verified: the test fails if the pin is reverted).
igamigo
force-pushed
the
remove-output-note-tags
branch
from
July 16, 2026 14:56
dd24a83 to
e1d641d
Compare
igamigo
marked this pull request as ready for review
July 16, 2026 15:29
igamigo
commented
Jul 16, 2026
Comment on lines
-493
to
-497
| new_tags.extend(note_updates.updated_output_notes().map(|note| { | ||
| let note = note.inner(); | ||
| NoteTagRecord::with_note_source(note.metadata().tag(), note.details_commitment()) | ||
| })); | ||
|
|
Collaborator
Author
There was a problem hiding this comment.
This is the real fix for the original problem
Comment on lines
770
to
780
| let (proof_block_num, proof) = self | ||
| .rpc_api | ||
| .get_account( | ||
| account_id, | ||
| GetAccountRequest::new() | ||
| .at(AccountStateAt::Block(target_block_num)) | ||
| .with_storage(StorageMapFetch::All) | ||
| .with_vault(VaultFetch::Always), | ||
| ) | ||
| .await | ||
| .map_err(ClientError::RpcError)?; |
Collaborator
Author
There was a problem hiding this comment.
This is the backported fix from next
Comment on lines
+64
to
+69
| /// Historical account states served by `get_account`, keyed by (block number, account id). | ||
| /// The [`MockChain`] only keeps the latest committed account state, so tests that need | ||
| /// `get_account` to answer block-pinned queries (`AccountStateAt::Block`) with a state | ||
| /// other than the latest one register the per-block state here. Queries for a block with | ||
| /// no registered entry fall back to the chain's latest committed account. | ||
| account_states_at: Arc<RwLock<BTreeMap<(BlockNumber, AccountId), Account>>>, |
Collaborator
Author
There was a problem hiding this comment.
Added this mechanism so that the mockchain-backed client can serve requests for older account states
SantiagoPittella
approved these changes
Jul 16, 2026
| /// The prune migration deletes output-note tags (and duplicates) but keeps tags still | ||
| /// needed by pre-inclusion input notes, input-only tags, and non-`Note`-sourced tags. | ||
| #[test] | ||
| fn migration_prunes_output_note_tags() { |
TomasArrachea
approved these changes
Jul 16, 2026
Collaborator
Author
|
I ended up simplifying the test setup a bit more on 5252bca, removing the need for the |
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
4 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.
note_tagsrow per created note. A migration removes leaked tags, preserves those still needed by pending input notes, deduplicates rows, and adds a unique(tag, source)index.is_inclusion_pendinggetters toInputNoteRecordandOutputNoteRecord.next, pinning public-accountget_accountrequests to the sync target block. This fixes the race that caused newly committed transactions to be markedSuperseded, wedging sole-writer accounts and triggering the July 13 testnet faucet outage.MockRpcApiwith per-block account states and added an end-to-end regression test for the outage.