feat(dashmate): state sync configuration for tenderdash and drive snapshots - #4521
feat(dashmate): state sync configuration for tenderdash and drive snapshots#4521PastaPastaPasta wants to merge 12 commits into
Conversation
Adds platform.drive.tenderdash.stateSync (enabled, retries, chunkRequestTimeout, fetchersCount) and platform.drive.abci.stateSync.snapshots (enabled, frequencySeconds, maxCount). Tenderdash 1.7 minimums are encoded in the schema: chunk request timeout of at least 5s, 1-64 fetchers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Base config enables consuming (tenderdash stateSync) and serving (drive snapshots every 600s, keeping 6). Serving is always on in Tenderdash and a node with local state ignores the consume flag, so the default is safe for existing nodes. The local preset disables both: a local network genesis starts every node from scratch, so there is no populated peer to sync from. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Templates the statesync section from config: enable, retries, chunk-request-timeout and fetchers. use-p2p is hardcoded to true because the RPC state provider needs two reachable RPC servers while dashmate publishes the Tenderdash RPC on loopback only, unproxied and without TLS. Drops the trust-height/trust-hash/trust-period keys removed in Tenderdash 1.7. Routes ListSnapshots and LoadSnapshotChunk to the drive gRPC app alongside CheckTx and bounds their concurrency; OfferSnapshot and ApplySnapshotChunk stay on the consensus socket. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Maps the state sync snapshot config to the SNAPSHOTS_ENABLED, SNAPSHOTS_FREQUENCY_SECONDS and MAX_NUM_SNAPSHOTS envs drive-abci consumes. Checkpoints are written to the default CHECKPOINTS_PATH under DB_PATH, which is already inside the drive_abci_data volume, so no new volume is needed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Keyed at 4.2.0-dev.6, above the 4.2.0-dev.5 the package is at, so the runner picks it up and dev-build stamped configs cross it. Options are pulled from the default config matching each config's name or group, which gives the local preset its disables and everything else the base defaults. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
When drive snapshots are enabled, the doctor adds a conservative 10GB to the required free disk space and says so in the problem message. Checkpoints hard-link unchanged data, so a small fixed headroom is enough. Configs collected by an older dashmate have no state sync options and skip the headroom. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adds a State Sync section to the tenderdash config doc (consume side, P2P-only rationale, self-disable semantics) and a State Sync Snapshots section to the drive-abci doc (serve side, checkpoint location and cost). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Replaces for-of loops over test fixtures with forEach to satisfy no-loop-func, and drops an unused catch binding. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…on pattern Judges disk problem severity against the base minimum so enabling snapshots widens when a problem is raised but never downgrades a HIGH shortage to MEDIUM. Accepts fractional minute and hour chunk request timeouts down to 0.1 (all at least 6s, above the 5s Tenderdash floor). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks 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 |
|
⛔ Final review complete — 1 blocking finding(s) (commit 11a3582) |
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Sol-only technical fallback
The state-sync configuration and template wiring are coherent, but three confirmed issues remain. The documented zero-retry value prevents block-sync fallback, routed transport discards the new gRPC concurrency limits, and enabling snapshot serving by default exposes unbounded checkpoint retention; the first and third issues are blocking.
Source: reviewer backends grok-4.5 (Claude general and security lanes) and Codex (exact model ID unavailable in the supplied evidence); final verifier backend grok-4.5.
One or more required Phase-1 GLM Flash lanes remained technically unusable after the bounded exact-model retry. Their evidence was discarded as authoritative, and the complete selected role cohort was rerun fresh on exact gpt-5.6-sol before this fresh Sol verifier produced the final decision. No additional Phase-2 reviewer pass ran.
Review provenance
- Phase 1 GLM evidence: technically unusable after bounded retry; discarded from the decision
- GLM failure attempts:
codex-security-auditor-0297efd1daab41ea953130ce483ce052(completed),codex-general-e111cb8d34114cb79faa3ee8101fb1c7(failed),codex-general-e4d7bd95e71e48a9ade37d92d316eec9(failed) - Sol-only fallback reasons:
launch_transport_or_nonzero_exit - Sol-only fallback reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed) - Fresh verifier (Sol):
gpt-5.6-sol— final-verifier - Additional Phase 2 pass: not run; the Sol-only fallback is final
🔴 2 blocking | 🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/dashmate/src/config/configJsonSchema.js`:
- [BLOCKING] packages/dashmate/src/config/configJsonSchema.js:1376-1380: A retry count of zero prevents fallback instead of disabling retries
The schema accepts `retries: 0` and documents it as disabling retries, but Tenderdash interprets zero as an unlimited retry count. With the rendered nonzero `discovery-time`, Tenderdash's `SyncAny` returns `errNoSnapshots` only when `retries > 0 && iters > retries`; zero therefore repeats snapshot discovery indefinitely. Because the state-sync reactor switches to block sync only after receiving `errNoSnapshots`, a fresh node configured with this explicitly supported value never falls back when no snapshot is available. Require at least one retry, or document zero as unlimited retries and update the template, documentation, and tests consistently.
In `packages/dashmate/templates/platform/drive/tenderdash/config.toml.dot`:
- [SUGGESTION] packages/dashmate/templates/platform/drive/tenderdash/config.toml.dot:98-104: Routed transport drops the new gRPC concurrency limits
These snapshot limits are not applied when `transport = "routed"`. Tenderdash passes `GrpcConcurrency` to `NewGRPCClient` only for a direct `grpc` transport. The routed branch calls `NewRoutedClientWithAddr`, which constructs each nested `AbciConfig` with only `Address` and `Transport`; the resulting gRPC client receives an empty concurrency map, and its rate limiter consequently enforces no limit. Use a Tenderdash release that propagates the concurrency map into routed clients, add another effective limiter, or remove these misleading entries and comments.
In `packages/dashmate/configs/defaults/getBaseConfigFactory.js`:
- [BLOCKING] packages/dashmate/configs/defaults/getBaseConfigFactory.js:361: Default snapshot serving exposes unbounded remote checkpoint retention
This default is migrated into existing non-local configurations and exported as `SNAPSHOTS_ENABLED=true`, exposing the companion Drive snapshot handler to unauthenticated P2P chunk requests. Tenderdash forwards arbitrary peer-supplied snapshot heights, versions, and chunk IDs directly to `LoadSnapshotChunk`. Drive resolves checkpoints from either the normal registry or its serving-pin map, refreshes the pin before fetching the requested chunk, and retains pins using only a refreshable 600-second inactivity timeout with no absolute lifetime or count bound. A peer can pin each advertised checkpoint before normal pruning and periodically submit malformed chunk requests for every retained height; the request refreshes the pin before fetching fails, and Tenderdash logs the ABCI error without penalizing the peer. The configured `maxCount: 6` therefore does not bound checkpoint disk usage, allowing stale RocksDB files to accumulate until disk exhaustion. Keep snapshot serving disabled by default until Drive enforces a hard lifetime, count, or disk bound and cannot refresh retired snapshots through arbitrary requests.
| retries: { | ||
| type: 'integer', | ||
| minimum: 0, | ||
| description: 'How many times to retry state sync before falling back to block sync.' | ||
| + ' 0 disables retries', |
There was a problem hiding this comment.
🔴 Blocking: A retry count of zero prevents fallback instead of disabling retries
The schema accepts retries: 0 and documents it as disabling retries, but Tenderdash interprets zero as an unlimited retry count. With the rendered nonzero discovery-time, Tenderdash's SyncAny returns errNoSnapshots only when retries > 0 && iters > retries; zero therefore repeats snapshot discovery indefinitely. Because the state-sync reactor switches to block sync only after receiving errNoSnapshots, a fresh node configured with this explicitly supported value never falls back when no snapshot is available. Require at least one retry, or document zero as unlimited retries and update the template, documentation, and tests consistently.
source: ['claude']
There was a problem hiding this comment.
Resolved in 11a3582 — A retry count of zero prevents fallback instead of disabling retries no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| grpc-concurrency = [ | ||
| { "check_tx" = {{= it.platform.drive.tenderdash.mempool.maxConcurrentCheckTx }} }, | ||
| # Snapshot serving: discovery is one request per peer, chunk downloads run | ||
| # several concurrent fetchers per syncing peer. | ||
| { "list_snapshots" = 10 }, | ||
| { "load_snapshot_chunk" = 100 }, | ||
| ] |
There was a problem hiding this comment.
🟡 Suggestion: Routed transport drops the new gRPC concurrency limits
These snapshot limits are not applied when transport = "routed". Tenderdash passes GrpcConcurrency to NewGRPCClient only for a direct grpc transport. The routed branch calls NewRoutedClientWithAddr, which constructs each nested AbciConfig with only Address and Transport; the resulting gRPC client receives an empty concurrency map, and its rate limiter consequently enforces no limit. Use a Tenderdash release that propagates the concurrency map into routed clients, add another effective limiter, or remove these misleading entries and comments.
source: ['claude']
There was a problem hiding this comment.
Resolved in 11a3582 — Routed transport drops the new gRPC concurrency limits no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| epochTime: 788400, | ||
| stateSync: { | ||
| snapshots: { | ||
| enabled: true, |
There was a problem hiding this comment.
🔴 Blocking: Default snapshot serving exposes unbounded remote checkpoint retention
This default is migrated into existing non-local configurations and exported as SNAPSHOTS_ENABLED=true, exposing the companion Drive snapshot handler to unauthenticated P2P chunk requests. Tenderdash forwards arbitrary peer-supplied snapshot heights, versions, and chunk IDs directly to LoadSnapshotChunk. Drive resolves checkpoints from either the normal registry or its serving-pin map, refreshes the pin before fetching the requested chunk, and retains pins using only a refreshable 600-second inactivity timeout with no absolute lifetime or count bound. A peer can pin each advertised checkpoint before normal pruning and periodically submit malformed chunk requests for every retained height; the request refreshes the pin before fetching fails, and Tenderdash logs the ABCI error without penalizing the peer. The configured maxCount: 6 therefore does not bound checkpoint disk usage, allowing stale RocksDB files to accumulate until disk exhaustion. Keep snapshot serving disabled by default until Drive enforces a hard lifetime, count, or disk bound and cannot refresh retired snapshots through arbitrary requests.
source: ['claude']
Tenderdash's SyncAny only returns errNoSnapshots (the block sync fallback trigger) when retries > 0, so retries=0 repeats snapshot discovery indefinitely rather than disabling retries - Tenderdash's own sample config comment is wrong about this. Describe 0 as retry-indefinitely in the schema, rendered template, docs, and test. Also annotate the grpc-concurrency block: Tenderdash drops the concurrency map for the routed transport (NewRoutedClientWithAddr passes address and transport only), so the entries, including the pre-existing check_tx one, are declarative until that propagates. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Findings triaged and addressed (pushed in
Dashmate unit suite 829 passing, lint clean. 🤖 Posted autonomously by Claude on behalf of pasta. |
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Sol-only technical fallback
The retries=0 documentation is now consistent with Tenderdash semantics, and the routed-transport concurrency limitation is clearly documented as deferred. One blocking security issue remains because snapshot serving is enabled by default while the companion handler allows remote peers to retain an unbounded number of retired checkpoints; Doctor's disabled-Platform accounting and the timeout schema also have non-blocking correctness issues.
Source: reviewer 1: gpt-5.6-sol (agent: sol-fallback-reviewer, role: general); reviewer 2: gpt-5.6-sol (agent: sol-fallback-reviewer, role: security-auditor); final verifier: gpt-5.6-sol (agent: sol-verifier, role: final-verifier)
One or more required Phase-1 GLM Flash lanes remained technically unusable after the bounded exact-model retry. Their evidence was discarded as authoritative, and the complete selected role cohort was rerun fresh on exact gpt-5.6-sol before this fresh Sol verifier produced the final decision. No additional Phase-2 reviewer pass ran.
Review provenance
- Phase 1 GLM evidence: technically unusable after bounded retry; discarded from the decision
- GLM failure attempts:
codex-general-26925ed5e29d411f93f18aa0b98bbb4a(failed),codex-general-550590841b7147439482fe1feef9510b(failed),codex-security-auditor-95af32ff40e043f393371a718884c7df(failed),codex-security-auditor-e41fca826ead4c7f8e54df875501138d(failed) - Sol-only fallback reasons:
launch_transport_or_nonzero_exit,launch_transport_or_nonzero_exit - Sol-only fallback reviewers:
gpt-5.6-sol— general (completed); agentsol-fallback-reviewer,gpt-5.6-sol— security-auditor (completed); agentsol-fallback-reviewer - Fresh verifier (Sol):
gpt-5.6-sol— final-verifier; agentsol-verifier - Additional Phase 2 pass: not run; the Sol-only fallback is final
🟡 2 suggestion(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/dashmate/src/doctor/analyse/analyseSystemResourcesFactory.js`:
- [SUGGESTION] packages/dashmate/src/doctor/analyse/analyseSystemResourcesFactory.js:27-28: Snapshot headroom is applied when Platform is disabled
The snapshot flag is derived without checking `platform.enable`. Regular fullnode and masternode setup sets `platform.enable` to false but retains the base snapshot default of true; the Platform Docker profile is therefore not started and Drive creates no checkpoints, yet Doctor still adds 10 GB to the disk requirement. For example, a Core-only node with 12 GB available is incorrectly reported as requiring 15 GB. Gate snapshot headroom on Platform actually being enabled.
In `packages/dashmate/src/config/configJsonSchema.js`:
- [SUGGESTION] packages/dashmate/src/config/configJsonSchema.js:1390-1391: Timeout schema rejects valid durations above five seconds
The minutes/hours branch permits fractional values only from `0.1`, which is stricter than the documented Tenderdash minimum of five seconds. Go durations such as `0.09m` (5.4 seconds) and `0.01h` (36 seconds) exceed that minimum but fail this pattern. Parse the duration into a common unit and validate it against five seconds instead of approximating a separate threshold for each suffix; add fractional-minute and fractional-hour boundary tests.
In `packages/dashmate/configs/defaults/getBaseConfigFactory.js`:
- [BLOCKING] packages/dashmate/configs/defaults/getBaseConfigFactory.js:361: Default snapshot serving exposes unbounded remote checkpoint retention
(existing thread: https://github.com/dashpay/platform/pull/4521#discussion_r3890016790)
This PR migrates every existing non-local configuration to `enabled: true`, exports that value as `SNAPSHOTS_ENABLED=true`, and routes Tenderdash P2P snapshot requests to the companion Drive handler. In companion commit `33296859f4`, `LoadSnapshotChunk` resolves checkpoints from either the normal registry or the serving-pin map and calls `pin_for_serving` before `fetch_chunk`; the pin map has only a refreshable 600-second inactivity timeout, with no absolute lifetime, count, or disk bound. A connected peer can request each advertised generation, then periodically send an invalid chunk request for every retired height. Each request refreshes the pin before retrieval fails, so `maxCount: 6` bounds only the normal registry while hard-linked RocksDB files from retired generations continue accumulating. Keep serving disabled by default until the companion implementation bounds retained pins and refreshes them only after a successful chunk fetch.
| stateSyncSnapshotsEnabled = samples.getDashmateConfig() | ||
| .get('platform.drive.abci.stateSync.snapshots.enabled') === true; |
There was a problem hiding this comment.
🟡 Suggestion: Snapshot headroom is applied when Platform is disabled
The snapshot flag is derived without checking platform.enable. Regular fullnode and masternode setup sets platform.enable to false but retains the base snapshot default of true; the Platform Docker profile is therefore not started and Drive creates no checkpoints, yet Doctor still adds 10 GB to the disk requirement. For example, a Core-only node with 12 GB available is incorrectly reported as requiring 15 GB. Gate snapshot headroom on Platform actually being enabled.
| stateSyncSnapshotsEnabled = samples.getDashmateConfig() | |
| .get('platform.drive.abci.stateSync.snapshots.enabled') === true; | |
| const config = samples.getDashmateConfig(); | |
| stateSyncSnapshotsEnabled = config.get('platform.enable') === true | |
| && config.get('platform.drive.abci.stateSync.snapshots.enabled') === true; |
source: ['claude']
| // At least 5 seconds: 5s+, 5000ms+, or minutes/hours down to 0.1 | ||
| pattern: '^(([5-9]|[1-9][0-9]+)(\\.[0-9]+)?s|([5-9][0-9]{3}|[1-9][0-9]{4,})(\\.[0-9]+)?ms|([1-9][0-9]*(\\.[0-9]+)?|0\\.[1-9][0-9]*)[mh])$', |
There was a problem hiding this comment.
🟡 Suggestion: Timeout schema rejects valid durations above five seconds
The minutes/hours branch permits fractional values only from 0.1, which is stricter than the documented Tenderdash minimum of five seconds. Go durations such as 0.09m (5.4 seconds) and 0.01h (36 seconds) exceed that minimum but fail this pattern. Parse the duration into a common unit and validate it against five seconds instead of approximating a separate threshold for each suffix; add fractional-minute and fractional-hour boundary tests.
source: ['claude']
Fullnode and masternode setup disables Platform but keeps the base snapshot default of true, so doctor added 10GB of disk headroom for checkpoints that Drive never creates. Gate the headroom on platform.enable. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…timeout The 0.1 m/h floor rejects a sliver of valid durations (0.09m = 5.4s) because 5s is a non-terminating decimal in minutes and no regex can hit the boundary exactly. Document the intent and pin it with boundary tests; s and ms spellings express any duration exactly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Both follow-up suggestions triaged; one fixed, one deferred with the requested tests added (pushed in
Dashmate unit suite 833 passing (baseline 829 + 4 new), lint clean. 🤖 Posted autonomously by Claude on behalf of pasta. |
Issue being fixed or feature implemented
Dashmate plumbing for Tenderdash state sync + Drive snapshot serving. Companion to the drive-abci state sync PR #4520 (config surface and env names are its contract) and finishes what #2558 started, retargeted to tenderdash 1.7.
What was done?
platform.drive.tenderdash.stateSync={ enabled (base default true — safe: tenderdash self-disables when local state exists, so only fresh nodes consume; local preset false), retries (3), chunkRequestTimeout ('15s', min 5s), fetchersCount (4) }platform.drive.abci.stateSync.snapshots={ enabled (base true, local false), frequencySeconds (600, min 60), maxCount (6, min 2) }[statesync]templated;use-p2p = truehardcoded — tenderdash RPC is loopback-bound, not gateway-proxied, and has no TLS/auth, so the RPC state-provider mode (≥2 rpc-servers) is not viable between evonodes; deleted the staletrust-height/trust-hash/trust-periodkeys (removed in tenderdash 1.7); addedretries.ListSnapshots/LoadSnapshotChunkroute to drive-abci's gRPC endpoint (26670) so snapshot serving never blocks the consensus socket;grpc-concurrencyentries added.SNAPSHOTS_ENABLED/SNAPSHOTS_FREQUENCY_SECONDS/MAX_NUM_SNAPSHOTSto drive-abci. Checkpoints stay inside the existingdrive_abci_datavolume (DB_PATH/checkpoints) — no new volume.4.2.0-dev.6; doctor accounts for snapshot disk headroom (+10 GB when enabled, without downgrading existing severities); docs for both new sections.Dropped from the old reference branch:
maxConcurrentListSnapshots/maxConcurrentSnapshotChunk(don't exist in tenderdash 1.7), thefeat-statesync-improvementsimage pin (1.7 ships everything needed),GROVEDB_LATEST_FILE, and the separate checkpoints volume.How Has This Been Tested?
Full dashmate unit suite: 829 passing, 0 failing, including new schema-bounds specs, the 4.2.0-dev.5→dev.6 migration test (deep-equal against new defaults), doctor severity tests, and template render-sanity (base and local configs rendered and parsed with a real TOML parser — valid, no
undefined, correct statesync/routing values).docker-compose.ymlYAML-parsed; eslint clean.Breaking Changes
None. New config keys are added by migration; tenderdash consuming is enabled by default but self-disables on nodes with existing state.
Checklist:
🤖 Generated with Claude Code