fix: dao reog and init path - #7774
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR refines snapshot recovery and chain-height validation in the DAO state layer. ChangesDAO Snapshot Recovery and Validation Refinement
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/src/main/java/bisq/core/dao/state/DaoStateSnapshotService.java`:
- Around line 325-327: The unreachable persistedDaoState == null branch is due
to storage synthesizing new DaoState() when none exists; update
DaoStateSnapshotService to detect that synthetic "no snapshot" state before the
empty-block resync path by treating a DaoState with no blocks
(persistedDaoState.getBlocks().isEmpty()) and other default/absent identifying
fields (e.g., no last block/header or timestamp) as equivalent to null and
return true (benign retry) instead of falling through to revertToLastSnapshot();
make the same change in the other occurrence covering lines ~342-352; reference
persistedDaoState, DaoStateSnapshotService,
DaoStateStorageService#getPersistedBsqStateAtStartup(),
loadPersistedDaoData(...), and revertToLastSnapshot() when implementing the
check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b9ebecd4-049e-4f6e-ac68-b975ee74f478
📒 Files selected for processing (2)
core/src/main/java/bisq/core/dao/state/DaoStateSnapshotService.javacore/src/main/java/bisq/core/dao/state/storage/DaoStateStorageService.java
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/src/main/java/bisq/core/dao/state/storage/DaoStateStorageService.java`:
- Around line 225-239: The isChainHeightMatchingLastBlockHeight(...) helper
currently treats a non-zero chainHeight with empty persistedDaoState.getBlocks()
as a mismatch, which incorrectly forces resync for valid pre-genesis snapshots;
update DaoStateStorageService.isChainHeightMatchingLastBlockHeight(...) to
exempt pre-genesis states by checking the DAO genesis height (use the existing
genesis-height accessor used elsewhere) and return true when
chainHeightOfPersistedDaoState < genesisHeight (i.e., pre-genesis), or
alternatively adjust DaoStateSnapshotService.applySnapshot(...) to call
isHeightBelowGenesisHeight(...) before invoking
isChainHeightMatchingLastBlockHeight(...); implement the exemption inside
isChainHeightMatchingLastBlockHeight(...) for minimal churn by early-returning
true for pre-genesis chainHeight values so applySnapshot keeps the benign no-op
path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 524def24-8036-4b72-98c1-67acf6c36d63
📒 Files selected for processing (2)
core/src/main/java/bisq/core/dao/state/DaoStateSnapshotService.javacore/src/main/java/bisq/core/dao/state/storage/DaoStateStorageService.java
🚧 Files skipped from review as they are similar to previous changes (1)
- core/src/main/java/bisq/core/dao/state/DaoStateSnapshotService.java
| if (persistedDaoState.getBlocks().isEmpty()) { | ||
| log.warn("Cannot check chain height: DaoState has no blocks."); | ||
| return true; | ||
| // Empty blocks list is only consistent with the freshly initialized state (chainHeight==0). | ||
| // Any other value means the proto recorded a chainHeight but the side-channel block file | ||
| // (bsqBlocksStorageService) didn't provide matching blocks — likely an incomplete or | ||
| // mis-versioned data dir. Returning true here would make applySnapshot proceed with a | ||
| // DaoState that reports a synced height but has no parsed blocks, freezing reported | ||
| // chainHeight at the stale value and stalling LiteNode sync (its startParseBlocks | ||
| // short-circuits when chainHeight == bsqWallet best height). | ||
| if (chainHeightOfPersistedDaoState == 0) { | ||
| return true; | ||
| } | ||
| log.warn("DaoState reports chainHeight={} but has no blocks — block storage is incomplete. " + | ||
| "Treating as chain-height mismatch so applySnapshot can trigger resyncDaoStateFromResources.", | ||
| chainHeightOfPersistedDaoState); | ||
| return false; |
There was a problem hiding this comment.
Pre-genesis snapshots now fall into the resync path.
DaoStateSnapshotService.applySnapshot(...) calls isChainHeightMatchingLastBlockHeight(...) before isHeightBelowGenesisHeight(...) (core/src/main/java/bisq/core/dao/state/DaoStateSnapshotService.java, Lines 322-378). For a valid persisted state captured before BSQ genesis, chainHeight can be non-zero while blocks is still empty. This branch now returns false for that case, so the later benign no-op path is never reached and recovery unnecessarily resyncs from resources. Please keep the sub-genesis case out of this mismatch path, either by moving the genesis-height check ahead of this call or by teaching this helper to exempt pre-genesis heights.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core/src/main/java/bisq/core/dao/state/storage/DaoStateStorageService.java`
around lines 225 - 239, The isChainHeightMatchingLastBlockHeight(...) helper
currently treats a non-zero chainHeight with empty persistedDaoState.getBlocks()
as a mismatch, which incorrectly forces resync for valid pre-genesis snapshots;
update DaoStateStorageService.isChainHeightMatchingLastBlockHeight(...) to
exempt pre-genesis states by checking the DAO genesis height (use the existing
genesis-height accessor used elsewhere) and return true when
chainHeightOfPersistedDaoState < genesisHeight (i.e., pre-genesis), or
alternatively adjust DaoStateSnapshotService.applySnapshot(...) to call
isHeightBelowGenesisHeight(...) before invoking
isChainHeightMatchingLastBlockHeight(...); implement the exemption inside
isChainHeightMatchingLastBlockHeight(...) for minimal churn by early-returning
true for pre-genesis chainHeight values so applySnapshot keeps the benign no-op
path.
|
It will more time to address that area, thus I have converted it to draft. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
Still relevant |
fixes some edge cases and re-org issues
Summary by CodeRabbit