Skip to content

[2.0.x] fix(staking): replaying a block keeps a pool registration a registration (backport of #1335) - #1347

Open
satran004 wants to merge 4 commits into
release/2.0.xfrom
backport/2.0.x/1335-pool-registration-replay
Open

satran004 wants to merge 4 commits into
release/2.0.xfrom
backport/2.0.x/1335-pool-registration-replay

Conversation

@satran004

Copy link
Copy Markdown
Member

Backport of #1335 to release/2.0.x. Tracked in #1339.

What it fixes: see #1335. The backport changes the same code in the same way.

How it was made: The commit of #1335, no changes. Commits are cherry-picked with -x, and the changed lines were compared one by one with #1335.

Checks: 32 stores/staking tests pass, including the 5 replay tests.

Fault-injection run: the same scenarios as #1328 are running on release/2.0.x with all the backports. The results will be posted here and in #1339.

🤖 Generated with Claude Code

When a block is processed again after a restart, the row it wrote the first
time is still in the pool table. PoolStatusProcessor looked up the latest
REGISTRATION of the pool up to the current epoch, found that row, and saved
the registration again as an UPDATE (amount 0, active epoch +3). The pool's
next certificate then became the REGISTRATION, so adapot counted the 500 ADA
deposit in the wrong epoch.

Look only at registrations that come before the current certificate
(slot, tx index, cert index), so a replay gets the same answer as the first
pass. Found by the fault-injection re-test (#1328).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
(cherry picked from commit 4da900f)
@satran004 satran004 added fault-injection Found or verified by the auto-recovery fault-injection test 2.0.x Fix or backport for the release/2.0.x stable line labels Nov 7, 2026

Copy link
Copy Markdown

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: 03274d87164b9cd9a99075ecebada20650e7f03d

Issues found:

  • stores/staking/src/main/java/com/bloxbean/cardano/yaci/store/staking/storage/PoolStorage.java:24 — An existing custom PoolStorage bean can throw AbstractMethodError on the first pool registration after a 2.0.x upgrade because this new method is required.
  • stores/staking/src/main/java/com/bloxbean/cardano/yaci/store/staking/processor/PoolStatusProcessor.java:116 — If a failed block leaves its registration in the cache, retrying the block can count the same certificate twice in StakingDepositEvent even though only one row is saved.

Not tested: I reviewed the code and CI without running the build, tests, or a failed-block replay.

@satran004

Copy link
Copy Markdown
Member Author

Fault-injection result on release/2.0.x

The same scenarios as #1328 ran on plain release/2.0.x and on release/2.0.x with all 8 backports (#1340–#1347). With the backports:

  • all 104 scenarios recovered on their own, with no manual restart (plain 2.0.x: 6 stuck);
  • no duplicate rows (plain 2.0.x: 31 scenarios);
  • only the 2 blocks that B04 skips on purpose are missing (plain 2.0.x: 9 blocks).

Full results: #1339.

This PR in the run: no replayed block held a pool registration in this run, and the pool table matched the reference on both builds. The fix is covered by its 5 tests.

Copy link
Copy Markdown

reviewer1 — review of 03274d87164b9cd9a99075ecebada20650e7f03d

Changes requested on two points. The new strict (slot, txIndex, certIndex) predicate correctly excludes the certificate being replayed and later registrations. The five tests cover successful row replay and ordering, including re-registration after retirement.

  1. [P2] Make the cached event path replay-safe too. PoolStatusProcessor.java:114–117 now correctly classifies each copy of the same first certificate as a REGISTRATION. But poolRegistrationsCache survives a failed block and handleRollback() deletes database rows without clearing that cache. If a listener fails after processPoolRegistrationEvent() and before successful pre-commit completion, replay queues the certificate again. The next pre-commit iterates both copies and publishes registration count 2 while the natural-key database row remains 1. I compiled the actual processor source and reproduced distinct_saved_rows=1, deposit_registration_count=2 with a stub storage implementation modeling the new before-certificate query. This affects StakingDepositEvent consumers/plugins; I am not claiming a measured double charge in the built-in adapot calculation. Clear/discard failed-session cache entries and deduplicate by certificate identity. Test through processPoolRegistrationEvent → failure/rollback → replay → PreCommitEvent, including the emitted event, rather than only calling handlePoolRegistration() directly.

  2. [P2] Address custom-storage compatibility for this stable-line backport. PoolStorage.java:24 adds a required abstract method to the replaceable storage interface. An existing separately compiled PoolStorage bean, supported by @ConditionalOnMissingBean, can still load but fail with AbstractMethodError on the first registration. Provide an explicit compatibility/capability strategy or clearly require provider upgrades as part of the release contract; do not silently fall back to the old query, which would restore the replay bug. Add a compatibility check for a provider built against the previous API.

The VM report explicitly says no replayed block contained a pool registration. Its matching pool table therefore does not validate this fix's trigger. Keep #1337's retirement-ordering issue tracked separately, and distinguish prevention of future replay damage from repair of already misclassified historical rows.

Validation: exact-head source/query/test inspection and the focused compiled processor harness above. No full staking suite, Spring transaction integration, or VM rerun.

satran004 and others added 3 commits November 8, 2026 19:20
…itEvent

PoolStatusProcessor caches pool registration events until pre-commit and
publishes the number of REGISTRATION rows in StakingDepositEvent. When a
batch failed before pre-commit, the cache kept its events. A block processed
again then sat in the cache twice. Now that a replayed registration stays a
registration, both copies were counted: 2 in the event, 1 row in the database.

- A block processed again replaces its earlier cached event (same slot and
  block hash), so each certificate is counted once.
- A rollback clears all four caches. A restart publishes a rollback to the
  cursor before it processes blocks again, so events of the failed batch,
  including blocks the node no longer serves, are not counted or saved at the
  next pre-commit. This also resets the stake key registration counts.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MJrQUCWhJpLCQR5Asv7nDb
(cherry picked from commit 15f7a3d)
…PoolRegistrationBefore

PoolStorage can be replaced by a custom bean. A provider compiled against the
previous interface does not have the new method and failed with
AbstractMethodError at the first pool registration. The method now has a
default that throws UnsupportedOperationException, naming the class and the
method to implement. It does not fall back to findRecentPoolRegistration,
because that query sees the certificate's own row and would turn a replayed
registration into an update again.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MJrQUCWhJpLCQR5Asv7nDb
(cherry picked from commit 132f4fb)
…er block

PoolRegistrationProcessor publishes one PoolRegistrationEvent per transaction. Replacing a cached event by
block dropped the other transactions of the same block, so their pool status rows were not written and the
registration count was too low. Replace only the copy of the same transaction.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MJrQUCWhJpLCQR5Asv7nDb
(cherry picked from commit 1e85144)
@sonarqubecloud

sonarqubecloud Bot commented Nov 8, 2026

Copy link
Copy Markdown

@satran004

Copy link
Copy Markdown
Member Author

Thanks, reviewer1. Handled as follows:

  1. Cache: it replaces an earlier copy of the same transaction (one event per transaction, so two pools registered in one block are both kept). All caches are cleared on rollback. The tests go through processPoolRegistrationEvent, rollback, replay and PreCommitEvent.
  2. Custom PoolStorage: the new method is a default that throws a clear error naming it, with no silent fallback. There is also a note in the upgrade section of release/2.0.x: backport the auto-recovery and data-correctness fixes from the fault-injection test #1339.

Commits: 0fb6071, 72b8c9f, 776f9eb here, 15f7a3d, 132f4fb, 1e85144 on #1335.

The fixes were made in #1335 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.

@satran004

Copy link
Copy Markdown
Member Author

Verdict: APPROVED

reviewer1 — round 2. Reviewed 776f9eb5428e1609f19b721e8d378added6796cf, compared with 03274d87164b9cd9a99075ecebada20650e7f03d.

Both original findings are resolved.

Finding Status Reason
F1 [P2] Resolved Cache replacement is per transaction, preserving separate registrations in one block; rollback clears all four pending caches. Replay/rollback/multiple-transaction tests cover the reported failure.
F2 [P2] Resolved The default method gives old providers an explicit failure instead of a linkage error or silently incorrect fallback. The upgrade notes require providers to implement the ordering-aware query.

This is an explicit provider upgrade requirement, not transparent behavioural compatibility. I accept that documented choice.

Validation: seven module suites on isolated combined commit 121ee4599fcdc41226239b9195ce45daa47cfc9d: 366 passed, 1 skipped, 0 failures/errors (core: 219 passed, 1 skipped). This is combined-build validation, not a separate run on every backport.

Exact command
JAVA_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=plain

No implementation changes made by this review. The updated VM fault-suite results are still pending. This verdict covers the reviewed head only.

@satran004
satran004 requested a review from huylevt November 9, 2026 00:00
@satran004

Copy link
Copy Markdown
Member Author

Fault-injection rerun on 2.0.x with all eight backports at their approved heads (test/recovery-combined-2.0.x bbac1a25a): 104 of 104 scenarios recovered, with no duplicate rows and no unexpected missing blocks. The full report is on #1339: #1339 (comment)

For this PR:

  • pool, pool_registration, pool_retirement, delegation and stake_registration match the reference exactly, in row counts and content, after 104 scenarios of restarts, rollbacks and replays.

The unpatched baseline is release/2.0.x (b2x), run on the same matrix.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2.0.x Fix or backport for the release/2.0.x stable line fault-injection Found or verified by the auto-recovery fault-injection test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants