Repository navigation
[2.0.x] fix(stores): stop re-processed blocks from duplicating metadata, assets, script and MIR rows (backport of #1266) - #1345
Conversation
On startup StartService rolls back to the cursor block and, when it uses BlockFetch (far from tip, or a sync-stop slot), fetches the range from that block inclusive, so the cursor block is processed again. Rollback handlers delete only slot > rollbackTo, so the block's rows stay and are written a second time. Tables keyed by natural keys are overwritten; tables keyed by a random UUID gain a duplicate row per restart. On a mainnet DB transaction_metadata had 76 rows for 38 (tx_hash, label) at one restart slot and 176 for 88 at another. Affected (random UUID id, JPA saveAll, deleteBySlotGreaterThan): transaction_metadata, assets, transaction_scripts, mir. Other per-block tables use natural keys or upserts; reward_rest/unclaimed_reward_rest are rewritten per epoch after a delete by (earned_epoch, type). The startup RollbackEvent now says whether the rollback point will be processed again (rollbackPointReprocessed: true only for the BlockFetch path; ChainSync starts after the intersection, so near tip the cursor block is not refetched and its rows must be kept). The four stores delete from that slot inclusive in that case. No schema change, and all other stores keep their behaviour. Rows already duplicated by earlier restarts are not cleaned up; that needs a one-off dedupe per table, keeping one row per natural key. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JKakmKURLn4uc33hgrz2hF (cherry picked from commit 85f4f94)
… rows transaction_metadata, assets, transaction_scripts and mir used a random UUID primary key and JPA saveAll, so every time a block was processed again (restart from the cursor block, re-fetch) its rows were inserted again with new ids and duplicated. The id is now a name based UUID (v5) of the row's natural key, and rows are written with a jOOQ batch upsert on the existing id primary key: - transaction_metadata: tx_hash, label - assets: tx_hash, unit (policy + hex asset name; asset_name is a decoded, sanitized name and can repeat) - transaction_scripts: tx_hash, purpose, redeemer_index (one row per redeemer) - mir: tx_hash, cert_index, credential (null for pot to pot transfers) No schema change or migration: existing rows keep their random ids and new rows get deterministic ids. metadata and mir are added to the jOOQ codegen modules. On MySQL the id is bound as binary(16), as Hibernate writes it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JKakmKURLn4uc33hgrz2hF (cherry picked from commit ec078a7)
…branch The tx_index column of transaction_metadata was added on main by 6ab6fae and is not on release/2.0.x, so the upsert from #1266 does not write it. The jOOQ module list keeps the release/2.0.x entries and adds assets, metadata and mir, which #1266 needs (main also has epoch-aggr there). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
satran004
added
fault-injection
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| public static UUID nameUUID(String... parts) { | ||
| MessageDigest sha1; | ||
| try { | ||
| sha1 = MessageDigest.getInstance("SHA-1"); |
|
Initial automated review · Aster Hi @satran004, thanks for the PR! 🙏 This is an automatic first check by an AI reviewer. It does not replace maintainer review and may be wrong. Reviewed commit: Issues found:
Not tested: I reviewed the code and CI without running the build, tests, or database replay scenarios. |
Fault-injection result on release/2.0.xThe same scenarios as #1328 ran on plain
Full results: #1339. This PR in the run: plain 2.0.x wrote duplicate rows in 31 scenarios. By the end it had +169 |
reviewer1 — review of
|
… the MIR id The mir id was a name based UUID of (tx_hash, cert_index, credential). The credential column holds only the hash, and the credential kind (key or script) survives only in the reward address. A MIR certificate whose map has a key credential and a script credential with the same hash bytes would give both rows the same id, and the second upsert would overwrite the first. A script credential now adds a "script" part to the id. Key credentials and pot to pot transfers (null credential) keep the (tx_hash, cert_index, credential) id, so ids for the common case do not change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MJrQUCWhJpLCQR5Asv7nDb (cherry picked from commit 75c33ff)
TxScriptStorageImpl.saveAll used JPA saveAll, which runs in its own transaction when there is none. The jOOQ batch upsert that replaced it had no transaction boundary. In parallel mode ScriptRedeemerDatumProcessor calls saveAll from executor and parallel stream workers, which do not inherit the event listener's transaction, so on autocommit a failure in the middle of the batch left the earlier rows committed. saveAll is @transactional again, like the other store writers (TransactionStorageImpl, UtxoStorageImpl, ...). The test runs outside the test transaction, through the Spring bean, and checks that a batch whose second row fails writes nothing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MJrQUCWhJpLCQR5Asv7nDb (cherry picked from commit 652e1d8)
|
|
Thanks, reviewer1. Handled as follows:
Commits: 1dc4fba, 6aefd7b here, 75c33ff, 652e1d8 on #1266. The fixes were made in #1266 first and cherry-picked here. Each finding, its verdict and its test are in the reply on #1339. The fault suite is running again on the updated combined build. |
|
Verdict: APPROVED reviewer1 — round 2. Reviewed Both original findings are resolved.
I accept no change for the Sonar weak-hash note: these are deterministic public-data identifiers, not authentication or secret protection. The upgrade note correctly says old random-ID duplicates still need deduplication. I also checked the combined Validation: seven module suites on isolated combined commit Exact commandJAVA_HOME=/Users/satya/.sdkman/candidates/java/21.0.3-tem ./gradlew :components:core:test :components:common:test :stores:assets:test :stores:metadata:test :stores:script:test :stores:mir:test :stores:staking:test --no-daemon --console=plainNo implementation changes made by this review. The updated VM fault-suite results are still pending. This verdict covers the reviewed head only. |
|
Fault-injection rerun on 2.0.x with all eight backports at their approved heads ( For this PR:
The unpatched baseline is |
Backport of #1266 to
release/2.0.x. Tracked in #1339.What it fixes: see #1266. The backport changes the same code in the same way.
How it was made: The 2 commits of #1266, plus one adaptation commit:
transaction_metadatahas notx_indexcolumn on 2.0.x (main added it in 6ab6fae), so the upsert doesn't write it.assets,metadataandmir, which this change needs. (mainalso hasepoch-aggrthere.) Commits are cherry-picked with-x, and the changed lines were compared one by one with fix(stores): stop re-processed blocks from duplicating metadata, assets, script and MIR rows #1266.Checks: Tests pass: common 72, assets 8, metadata 3, script 19, mir 7.
applications:allcompiles. Merging this after the #1313 and #1330 backports conflicts inStartService, the same as onmain. The resolved file is intest/recovery-combined-2.0.x.Fault-injection run: the same scenarios as #1328 are running on
release/2.0.xwith all the backports. The results will be posted here and in #1339.🤖 Generated with Claude Code