fix(test-suite): stabilize platform e2e tests - #4214
Open
shumkov wants to merge 26 commits into
Open
Conversation
Wait for restored Core nodes to converge before mining, restore finite historical header streams, and run E2Es for direct inputs with cache-safe cold starts. Test would have caught these regressions in CI: ✖ before fix, ✔ after.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe pull request expands E2E reliability coverage across wallet initialization, SPV validation, proof verification, document handling, epoch queries, Dashmate startup, trusted-context refresh, Swift CI keychain management, and workflow orchestration. It also adds a detailed reliability specification and regression coverage. ChangesE2E reliability changes
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant Wallet
participant BlockHeadersProvider
participant Account
Caller->>Wallet: request account
Wallet->>BlockHeadersProvider: initialize chain
BlockHeadersProvider-->>Wallet: initialization complete
Wallet->>Account: create account
Account-->>Caller: return account
sequenceDiagram
participant Changes
participant E2EFilter as filter-e2e
participant Builds
participant Tests
Changes->>E2EFilter: evaluate changed paths
E2EFilter-->>Changes: publish e2e-tests-changed
Changes->>Builds: enable required builds
Builds-->>Tests: provide successful build results
Changes->>Tests: enable E2E jobs
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
shumkov
changed the title
Collaborator
|
⛔ Blockers found — Sonnet deferred (commit 259d3ba) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/dashmate/src/listr/tasks/startGroupNodesTaskFactory.js (1)
39-39: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider passing
coreRpcClientsvia Listrctxinstead of a closure variable.
coreRpcClientsis populated in one task step and consumed by the next relying on their being adjacent, sequential Listr steps. This file already usesctxfor cross-step state (e.g.ctx.skipBuildServices), which is the more idiomatic and reorder-safe pattern here.♻️ Suggested refactor
- let coreRpcClients = []; - const minerConfig = configGroup.find((config) => ( config.get('core.miner.enable') )); @@ { title: 'Wait for Core peers to be connected', enabled: isLocalMinerEnabled, - task: async () => { - coreRpcClients = await Promise.all(configGroup.map(async (config) => ( + task: async (ctx) => { + ctx.coreRpcClients = await Promise.all(configGroup.map(async (config) => ( createRpcClient({ port: config.get('core.rpc.port'), user: 'dashmate', pass: config.get('core.rpc.users.dashmate.password'), host: await getConnectionHost(config, 'core', 'core.rpc.host'), }) ))); const tasks = configGroup.map((config, index) => ({ title: `Checking ${config.getName()} peers`, - task: () => waitForCorePeersConnected(coreRpcClients[index]), + task: () => waitForCorePeersConnected(ctx.coreRpcClients[index]), })); return new Listr(tasks, { concurrent: true }); }, }, { title: 'Wait for Core nodes to have the same height', enabled: isLocalMinerEnabled, - task: () => waitForNodesToHaveTheSameHeight( - coreRpcClients, + task: (ctx) => waitForNodesToHaveTheSameHeight( + ctx.coreRpcClients, WAIT_FOR_NODES_TIMEOUT, ), },Also applies to: 72-98
🤖 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 `@packages/dashmate/src/listr/tasks/startGroupNodesTaskFactory.js` at line 39, Move coreRpcClients from the closure-scoped variable into the Listr context, initializing and updating the corresponding ctx property in the producer task and reading it in the consumer task. Follow the existing ctx.skipBuildServices pattern so the tasks no longer depend on closure state or adjacency.
🤖 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 @.github/actions/local-network/action.yaml:
- Line 37: The shared local-network cache key is incomplete because it omits
image/source inputs that determine the generated genesis volumes. Update the
restore key at .github/actions/local-network/action.yaml lines 37-37 to include
the relevant image/source build inputs or immutable image digests, and apply the
identical completed key contract at .github/workflows/tests-dashmate.yml lines
93-93; keep both restore sites consistent.
In @.github/workflows/tests.yml:
- Around line 462-464: Add an explicit least-privilege permissions block for the
affected workflow job, beginning with contents: read and including only
permissions verified as required by its Docker, AWS, secrets, and E2E steps.
Keep the existing trigger condition unchanged, and avoid relying on
repository-default GITHUB_TOKEN permissions.
In `@packages/dashmate/test/unit/listr/tasks/startGroupNodesTaskFactory.spec.js`:
- Around line 104-190: Update the Sinon-Chai assertions in the affected tests to
use property syntax: remove trailing parentheses from
`.to.not.have.been.called()` and `.to.have.been.calledOnce()`. Preserve existing
assertion targets and argument-bearing assertions such as
`.calledOnceWithExactly(...)`.
---
Nitpick comments:
In `@packages/dashmate/src/listr/tasks/startGroupNodesTaskFactory.js`:
- Line 39: Move coreRpcClients from the closure-scoped variable into the Listr
context, initializing and updating the corresponding ctx property in the
producer task and reading it in the consumer task. Follow the existing
ctx.skipBuildServices pattern so the tasks no longer depend on closure state or
adjacency.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d95fadbe-cb92-4532-8eb9-a0fdfbcc61a6
📒 Files selected for processing (9)
.github/actions/local-network/action.yaml.github/workflows/tests-dashmate.yml.github/workflows/tests.ymldocs/E2E_TESTS_GREEN_SPEC.mdpackages/dashmate/src/createDIContainer.jspackages/dashmate/src/listr/tasks/startGroupNodesTaskFactory.jspackages/dashmate/test/unit/listr/tasks/startGroupNodesTaskFactory.spec.jspackages/js-dapi-client/lib/BlockHeadersProvider/createBlockHeadersProviderFromOptions.jspackages/js-dapi-client/test/unit/BlockHeadersProvider/createBlockHeadersProviderFromOptions.spec.js
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4214 +/- ##
============================================
+ Coverage 87.51% 87.55% +0.03%
============================================
Files 2666 2671 +5
Lines 337942 338891 +949
============================================
+ Hits 295765 296710 +945
- Misses 42177 42181 +4
🚀 New features to boost your workflow:
|
Await one wallet-shared block-header provider initialization so empty stores authenticate the network genesis before height-1 historical sync. Preserve custom transports and serialize concurrent getAccount calls by account index. Remove the disproven default-provider callback change; the browser E2E uses the already-correct configured-provider path. Test would have caught this in CI: ✖ before fix, empty stores skipped initialization, concurrent getAccount returned duplicate index-0 accounts, and custom transports threw; ✔ after fix, focused tests, wallet unit/integration suites, and 412 Chrome tests pass.
Treat grpc-web's exact already-closed exception as idempotent cleanup while continuing to propagate other cancellation failures. Test would have caught this in CI: ✖ before fix, ✔ after.
Contributor
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
`@packages/js-dapi-client/test/unit/BlockHeadersProvider/BlockHeadersReader.spec.js`:
- Line 140: Update the test description in the rejected-headers stream case so
it starts with “should” and moves the “[data]” marker to the end, preserving the
existing test meaning.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4bd4bcd9-5a4f-46aa-9fdf-6db763a9e04a
📒 Files selected for processing (8)
docs/E2E_TESTS_GREEN_SPEC.mdpackages/js-dapi-client/lib/BlockHeadersProvider/BlockHeadersReader.jspackages/js-dapi-client/test/integration/BlockHeadersProvider/BlockHeadersProvider.spec.jspackages/js-dapi-client/test/unit/BlockHeadersProvider/BlockHeadersReader.spec.jspackages/wallet-lib/src/types/Wallet/Wallet.jspackages/wallet-lib/src/types/Wallet/methods/createAccount.jspackages/wallet-lib/src/types/Wallet/methods/createAccount.spec.jspackages/wallet-lib/src/types/Wallet/methods/getAccount.js
Dash Core regtest keeps the preceding target while dash-spv was applying Dark Gravity Wave after 24 headers. Test would have caught this in CI: ✖ before fix: a valid fast-mined header was rejected and a changed-target first header was accepted. ✔ after fix: the fast chain is accepted and target changes are rejected from genesis.
Restore cached wallet headers at their first authenticated height, use affected-state proof waits and explicit epoch selectors, and sanitize schema-declared binary document properties before transition construction. Test would have caught these in CI: ✖ before fix, ✔ after.
Contributor
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 `@docs/E2E_TESTS_GREEN_SPEC.md`:
- Around line 244-245: Qualify the wallet genesis-initialization requirement in
the documented goal to apply only to DAPI-backed online wallets, and explicitly
state that custom online transports without DAPI internals intentionally skip
provider initialization. Keep the existing historical-header behavior and align
the wording with the custom-transport exception documented elsewhere.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e1f9b614-4e57-4dad-b176-9cda091bfdea
📒 Files selected for processing (12)
docs/E2E_TESTS_GREEN_SPEC.mdpackages/bench-suite/lib/client/createPlatformProofVerifier.jspackages/dash-spv/lib/consensus.jspackages/dash-spv/test/index.jspackages/js-dapi-client/test/unit/BlockHeadersProvider/BlockHeadersReader.spec.jspackages/js-dash-sdk/src/SDK/Client/Platform/IPlatformProofVerifier.tspackages/platform-test-suite/lib/test/createPlatformProofVerifier.jspackages/platform-test-suite/test/unit/createPlatformProofVerifier.spec.jspackages/rs-sdk/src/platform/transition/put_document.rspackages/wallet-lib/src/types/Wallet/methods/createAccount.jspackages/wallet-lib/src/types/Wallet/methods/createAccount.spec.jspackages/wasm-sdk/tests/functional/epochs-blocks.spec.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/wallet-lib/src/types/Wallet/methods/createAccount.spec.js
- packages/wallet-lib/src/types/Wallet/methods/createAccount.js
- packages/js-dapi-client/test/unit/BlockHeadersProvider/BlockHeadersReader.spec.js
Proof-wait E2Es in workflow 30036234705 reproduced unknown-contract verification failures for custom document batches, while Swift resolver tests reproduced SecItemAdd -61 on the persistent runner keychain. Test evidence: ✖ the prior workflow failed both paths; ✔ focused contract extraction/cache regressions, shell syntax, formatting, and forced-failure keychain cleanup checks pass locally. Full Rust and network E2E confirmation follows in CI because the configured local sccache fails at the rustc -vV probe.
Limit the SPV genesis-initialization goal to DAPI-backed wallets and retain the documented custom-transport exception. Tests omitted: documentation-only clarification with no behavior change.
Repair a dangling user-domain default from validated per-user keychains before provisioning the ephemeral test keychain. Keep partial preference mutations contained, preserve unrelated Security errors, and run the regression from the reusable Swift workflow. Test would have caught this in CI: ✖ before fix: exit-44 fixture returned 44 before provisioning; ✔ after fix: Bash 3.2 state-machine regression passes recovery and all injected mutation boundaries.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/wasm-sdk/src/state_transitions/broadcast.rs (1)
21-42: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winParallelize the per-contract refresh calls.
prepare_state_transition_contextcallsrefresh_contract(contract_id)sequentially, while eachDataContract::fetch(...).ok_or_else(...)call bypasses the cache and must complete proof verification before the next contract is fetched. Collect the independent futures for the distinct batch contract IDs andtry_join_all/join_allthem instead of awaiting one by one.🤖 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 `@packages/wasm-sdk/src/state_transitions/broadcast.rs` around lines 21 - 42, The per-contract refreshes in WasmSdk::prepare_state_transition_context currently run sequentially. Collect futures for each distinct ID from referenced_contract_ids and await them concurrently with try_join_all (propagating refresh_contract errors), preserving the existing successful empty-set behavior.
🤖 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.
Nitpick comments:
In `@packages/wasm-sdk/src/state_transitions/broadcast.rs`:
- Around line 21-42: The per-contract refreshes in
WasmSdk::prepare_state_transition_context currently run sequentially. Collect
futures for each distinct ID from referenced_contract_ids and await them
concurrently with try_join_all (propagating refresh_contract errors), preserving
the existing successful empty-set behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: dbd35af5-8701-4beb-b1d9-bcda6826c380
📒 Files selected for processing (7)
.github/workflows/swift-sdk-build.yml.github/workflows/tests.ymldocs/E2E_TESTS_GREEN_SPEC.mdpackages/swift-sdk/run_tests.shpackages/swift-sdk/tests/run-tests-keychain-regression.shpackages/wasm-sdk/src/sdk.rspackages/wasm-sdk/src/state_transitions/broadcast.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/tests.yml
- docs/E2E_TESTS_GREEN_SPEC.md
The reusable Platform test-suite job could exhaust its 15-minute cap during a healthy cold helper-image pull, cancelling the main suite before readiness and a browser shard while tests were passing. Keep the timeout bounded but raise it to 30 minutes so bootstrap variance does not consume the execution budget. Test would have caught this in CI: ✖ before fix: the timeout policy assertion reported 15m below the cold-pull-safe 30m floor; ✔ after fix: the assertion and workflow parsers pass at 30m.
Refresh current and previous trusted quorum caches at the shared proof-preparation boundary. Browser requests bypass HTTP caches and have a bounded timeout; partial failures retain every successful exact-hash update while proof verification remains fail-closed. TDD proof: ✖ before, browser credit transfer failed after quorum rotation with an InvalidQuorum cache miss in run 30048343835; ✔ after, deterministic provider and WASM preparation regressions cover absent-to-present refresh, independent failures, cache preservation, semantic failures, and exact-hash rejection (CI is the execution authority because the local sccache rustc probe is blocked).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/rs-sdk-trusted-context-provider/src/provider.rs (1)
318-344: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider fetching current/previous quorums concurrently.
refresh_quorum_cachesawaits both fetches sequentially even though they're independent. Since this refresh runs on the proof-verification hot path (perprepare_state_transition_contextin broadcast.rs), running them concurrently halves the round-trip latency.Please confirm `tokio::join!` is available/appropriate for this crate's wasm32 target (it doesn't spawn tasks, only interleaves polling, but worth double-checking against this crate's tokio feature flags for wasm32 builds).♻️ Proposed fix
- let current_error = self.fetch_current_quorums().await.err(); - let previous_error = self.fetch_previous_quorums().await.err(); + let (current_result, previous_result) = + tokio::join!(self.fetch_current_quorums(), self.fetch_previous_quorums()); + let current_error = current_result.err(); + let previous_error = previous_result.err();🤖 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 `@packages/rs-sdk-trusted-context-provider/src/provider.rs` around lines 318 - 344, Update refresh_quorum_caches to initiate fetch_current_quorums and fetch_previous_quorums concurrently using an appropriate non-spawning combinator such as tokio::join!, preserving the existing independent-error handling and result messages. Verify the crate’s Tokio features and wasm32 compatibility before using it.
🤖 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 `@packages/rs-sdk-trusted-context-provider/src/provider.rs`:
- Around line 42-44: Generalize the WASM timeout helper used by quorum_request
into an http_request method that applies the per-request timeout consistently.
Update both existing quorum_request call sites and fetch_masternode_addresses to
use http_request instead of calling self.client.get directly, preserving the
existing request behavior while ensuring masternode discovery cannot hang
indefinitely on wasm32.
---
Nitpick comments:
In `@packages/rs-sdk-trusted-context-provider/src/provider.rs`:
- Around line 318-344: Update refresh_quorum_caches to initiate
fetch_current_quorums and fetch_previous_quorums concurrently using an
appropriate non-spawning combinator such as tokio::join!, preserving the
existing independent-error handling and result messages. Verify the crate’s
Tokio features and wasm32 compatibility before using it.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: bc4b7b5e-83cd-43b7-80fb-d767d35b5eb2
📒 Files selected for processing (5)
.github/workflows/tests-test-suite.ymldocs/E2E_TESTS_GREEN_SPEC.mdpackages/rs-sdk-trusted-context-provider/src/provider.rspackages/wasm-sdk/src/context_provider.rspackages/wasm-sdk/src/state_transitions/broadcast.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/wasm-sdk/src/state_transitions/broadcast.rs
- docs/E2E_TESTS_GREEN_SPEC.md
Red evidence: PR run 30052329512 observed a stale purchased-document owner and the restored-wallet test exhausted its 950-second timeout. Green evidence: focused syntax and ESLint checks pass; the required full functional and main E2E proof will come from this commit's PR run.
Test would have caught this in CI: ✖ unsafe remote checkpoint and stale header baseline before fix, ✔ genesis-rooted resume and durable header normalization after.
Treat missing user search-list preferences as recoverable only on the dedicated Swift runner, preserve validated baselines, and verify cleanup before removing the job keychain. Test would have caught this in CI: ✖ before fix: the exit-44 search-list fixture stopped before provisioning; ✔ after fix: the Bash 3.2 lifecycle suite passes recovery, cleanup containment, and next-run orphan isolation.
The Dashmate local-network E2E spent 11m13s pulling healthy helper images and was then cancelled by the reusable job's 15-minute cap. Match the sibling platform test-suite workflow's bounded 30-minute budget without changing any test-level deadline. Test would have caught this in CI: ✖ before fix: the timeout policy assertion reported the Dashmate 15m budget below the cold-pull-safe 30m floor; ✔ after fix: both reusable E2E workflows parse and satisfy the 30m policy.
Use the job keychain path for the CLI smoke read and delete, then scope the macOS resolver integration tests to the user preference domain with guaranteed restoration and serialized execution. Test would have caught this in CI: ✖ before fix: stale implicit search exited 44 before the Swift body (expected sentinel 23) ✔ after fix: explicit read/delete reached the Swift body and preserved sentinel 23
Warm the WASM functional process once against the real trusted-context readiness condition, preserve concrete retry diagnostics, and keep the cold-start workflow within its established outer budget. Poll wallet transaction indexes before asserting exact membership so balance propagation cannot race the state under test. Test would have caught this in CI: ✖ the old retry skipped the final partial sleep and the main/package-functional jobs failed; ✔ the focused regression, 398 Node tests, and 398 Karma tests pass before the full E2E rerun.
thepastaclaw
suggested changes
Jul 24, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Preliminary review — Codex only
The proof-verification integration generally authenticates GroveDB state and Tenderdash roots, but the affected-state fallback weakens the state-transition confirmation boundary. Because a verified snapshot does not prove execution and the caller relies on an unauthenticated DAPI error field, affected-state transition families can be reported as successful without having executed.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking
🤖 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/platform-test-suite/lib/test/createPlatformProofVerifier.js`:
- [BLOCKING] packages/platform-test-suite/lib/test/createPlatformProofVerifier.js:169-171: Affected-state snapshots cannot confirm transition execution
`waitForAffectedState` accepts `AffectedState` outcomes that authenticate only a height-pinned snapshot of keys affected by the transition; the Rust and WASM APIs explicitly state that these outcomes are not evidence that the specific transition executed. This verifier nevertheless implements `verifyStateTransitionResult`, whose caller in `broadcastStateTransition.ts` returns success after checking the original DAPI response's unauthenticated `error` field and merely requiring proof and metadata to be present. A malicious or faulty selected DAPI node can therefore suppress a consensus error or drop an identity top-up, transfer, withdrawal, address-funds movement, shield, or no-history token transition, then return a valid proof of unchanged affected state from a quorum-authenticated root. The application will report the transition as successful even though no execution was proved. Keep this confirmation API strict, or redesign the caller to expose affected-state results as non-confirming snapshots instead of successful transition confirmations. Apply the same correction to `packages/bench-suite/lib/client/createPlatformProofVerifier.js:165-167`.
The specification was a working artifact for this task, not documentation the project keeps. It stays out of the repository; the durable record of each fix is its own code comments, tests, and commit message. No test: documentation-only removal with no behavior delta.
…sons
names.register returns the domain document built locally, before broadcast.
The stored document gains $creatorId during the transition-to-document
conversion, which sets it to the owner id whenever the document type and
contract configuration call for a creator id. The local object never has it,
so comparing the two required stripping that field alongside the other
values only the network can supply.
All three comparisons get the same treatment, including the one currently
pending, so it stays correct when re-enabled.
Red to green from run 30330459238: 'should be able to search a domain' and
'should be able to resolve domain by it's record' both failed with
"expected { …(12) } to deeply equal { '$creatorId': null, …(11) }", the
only differing key being $creatorId (actual: the owner id, expected: null).
Stripping it leaves the objects equal. The e2e specs need a running local
network, so this was not reproduced locally; the CI run is the red evidence
and the next run is the green one.
The exit-44 failures this machinery was built for came from the smoke check itself: find-generic-password searches for an item and needs the keychain argument, which list-keychains and default-keychain do not. Passing the job keychain explicitly fixes them, so the recovery paths defended against failures no logged run ever produced. Removing them also removes their own hazards: repairing an incoherent user default was ungated by environment and could permanently rewrite a self-hosted runner's real keychain search list, the repaired state was promoted to the restore baseline so the trap put back the wrong one, cleanup hiccups turned green runs red, and orphaned keychains accumulated with no reaper. The regression harness ran under bash 3.2 while CI runs run_tests.sh under homebrew bash 5, so it exercised a configuration CI never uses. What stays is the ephemeral per-run keychain, the restore trap, and the smoke check with its explicit keychain argument. The macOS preference domain override and the serial swift test flag go too; neither had a failing run behind it. No test: this deletes a test harness and its subject code; the retained provisioning is unchanged from before the reverted commits.
Proof preparation runs after the transition has been broadcast, so the unconditional refetch of every referenced contract turned one transient fetch error into a discarded result for a transition the network had already accepted. Referenced contracts now resolve through the cache and only reach the network on a miss, so a cached contract cannot be lost to a failed request; a miss that cannot be fetched stays fatal because the proof needs the contract. System contracts are skipped entirely: the SDK compiles in their definitions, and fetching one lets a node-supplied copy shadow the built-in, which decides verification when proofs are off. The referenced-id test only ever covered deduplication and ordering, so it is renamed for what it checks. Two tests now cover preparation itself, which the existing integration test could not reach because its IdentityTopUp transition references no contracts: a referenced contract missing from the context is fetched and cached, and a referenced system contract is left alone. Not run locally: cargo cannot start its sccache-wrapped rustc on this machine (direct invocation of the same command succeeds), so CI is the execution authority for these tests.
The verifier accepted an affected-state snapshot for every transition, which relaxed verification for the families that do produce an execution proof: a document batch could be reported as confirmed on a snapshot of unchanged state. It now waits strictly first and only falls back when the SDK reports ExecutionNotProved, the typed signal that this family has no proof binding one specific transition. Any other failure propagates. The typed signal needs to survive the boundary, so waitForResponse stops flattening every SDK error into a generic one. The bench-suite copy was byte-identical before this branch and had drifted; it is identical again. The unit spec asserted that waitForResponse is never called, which cemented the weaker contract, and it only ran inside the browser Karma batch, where bail means one unit failure aborts the batch. The karma glob now covers e2e and functional only, and the spec runs under a test:unit script that the JS package job invokes for this package instead of skipping its tests. Red to green: against the previous verifier the strict-first and no-fallback-on-real-failure cases fail (waitForResponse never called; a rejected execution proof fulfils instead of rejecting); all four pass after. Not run locally: the Rust error-typing change, since cargo cannot start its sccache-wrapped rustc here.
Header validation now runs as a chain from an authenticated root, so a mid-chain start cannot be verified and skipSynchronization and skipSynchronizationBeforeHeight no longer affect header sync. They are still honoured by transaction synchronization, so one option now means two different things depending on the worker. Header sync says which of the two applies instead of ignoring the option in silence. Caches written before that change are rooted at an unauthenticated checkpoint, and configure() only wipes a wallet when the skip value changes, so an unchanged value carried a tainted chain forward. The storage version bump invalidates them. A failed provider initialization was also memoized, so one transient failure made every later account creation replay the same rejection for the lifetime of the wallet. The memo is cleared on rejection. Red to green: 'should retry provider initialization after a failed attempt' fails against the memoized promise, where the second createAccount replays 'SPV initialization failed' instead of retrying, and passes after. wallet-lib unit: 403 passing, 2 pending, 0 lint errors.
The continuous-header reader cancelled its stream directly to keep the listeners its retry needs, which meant it never gained the tolerance for an already-closed transport that historical cancellation has. The guarded cancel is now its own step, so both callers share it and only cancelStream also unsubscribes. waitForTransaction returned whatever it had when the deadline passed, so a transaction that never propagated surfaced as a confusing assertion on an empty set instead of the timeout it was. The DPNS comparison drops $creatorId on both sides because the local document never carries it; the search case now pins the value first, so the field is checked rather than only discarded. It is compared through Buffer because toObject renders identifiers as Uint8Array. The functional workflow gets the timeout rationale its two siblings already carry. No test for the workflow comment; the reader change is covered by the existing already-closed specs. dapi-client 318 passing, dash-spv 63 passing/2 pending, dashmate unit 160 passing, wallet-lib 403 passing; 0 lint errors in each. The DPNS assertion needs a live network and runs in CI.
The per-request timeout existed because wasm32 has no ClientBuilder::timeout, but only the two quorum endpoints used it. Masternode discovery still called the client directly, so on wasm32 it had no timeout and no cache bypass: a slow or hung /masternodes endpoint blocked trusted-context prefetch in the browser indefinitely, and a cached response could hide a changed masternode list. Readiness prefetch depends on that call, which is the failure this helper was introduced to prevent. The helper is named for what it does now and every request to base_url goes through it. No test: this is a wasm32-only request attribute with no observable effect on the native target the test suite builds, and the crate has no wasm harness. Not run locally either: cargo cannot start its sccache-wrapped rustc on this machine, so CI compiles it.
A verifier failure arrived at the reporter with nothing in it: WASM SDK
errors are wasm-bindgen class instances rather than Errors, carrying kind
and message on prototype getters, and the suite runs in parallel mocha
workers that serialize a failure by copying the error's own properties.
Run 30358345245 shows the result — 'should be able to withdraw credits'
failed with a completely empty message and no trace of the underlying
error anywhere in the job log.
Errors leaving the verifier are converted to an Error carrying the kind
and message, and the kind is read defensively because a released WASM
object throws on property access.
This is a diagnosis fix, not a fix for that failure: the cause is still
unidentified, and the next occurrence will now say what it was.
Red to green: the new case fails against the previous verifier with
'expected {} to be an instance of Error' — the same empty rendering CI
produced — and passes after. Unit suite 5 passing.
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
2 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.
Issue being fixed or feature implemented
Platform E2Es could fail for two independent startup/lifecycle reasons:
restored Core nodes could begin mining before sharing a common tip, and a new
browser wallet could process height-1 historical headers before its SPV chain
had an authenticated genesis root. When historical-header validation rejects a
batch, repeated cleanup of the same gRPC-web stream can then surface
Client already closed - cannot .close()instead of the primary SPV error.Once exposed, that primary error showed that SPV applied Dark Gravity Wave to
fast-mined regtest headers even though Dash Core disables regtest retargeting.
Relevant package and harness changes also did not trigger the E2E fleet unless
they changed the Platform version.
Related: #4210
What was done?
and block hash before the local miner starts. A mismatch or the established
five-minute deadline fails startup instead of mining onto a divergent tip.
headers or the configured network genesis before account synchronization.
Initialization is awaited once per wallet; concurrent
getAccountcallsshare account creation by index, while offline and custom transports without
DAPI client internals keep their prior behavior.
exception as idempotent. Other cancellation failures still propagate, while
the original SPV rejection now reaches the caller and CI output.
Dash Core's no-retarget rule. Canonical-target, proof-of-work, timestamp, and
changed-target checks remain enforced; other networks keep the DGW path.
consumed environment templates trigger the full fleet. Fixture cache keys
include their deterministic construction inputs, and cold cache misses always
execute the uncached Dashmate test.
The initial callback-swap hypothesis was removed after the first PR run showed
that browser E2Es use the already-correct configured-provider path. The trigger
deliberately follows direct E2E consumers rather than the entire transitive
monorepo dependency graph; broad Platform source changes continue to use the
existing version-change gate.
How Has This Been Tested?
initializeChainWith([], -1)and concurrent account creation did not shareprovider initialization; both regressions pass after the awaited,
wallet-shared initialization.
getAccount({ index: 0 })returned two distinct accounts and a conforming custom transport threw on
transport.client; the focused suite went from 5 passing / 2 failing to9 passing.
cancellation succeeds and the second throws grpc-web's exact already-closed
exception. Without the fix the cleanup exception escapes; with it the reader
performs both cleanup attempts, drains the tracked stream, and emits the
original validation error.
30029414181: before the consensus fix, a validfast-mined regtest header was rejected after the 24-header DGW window and a
changed-target first header was accepted. After the fix, the same regressions
accept Core's no-retarget chain and reject the target change from genesis.
412 completed, 4 skipped.
4 skipped. The genesis-to-height-1 provider integration regression passes.
ordering, timeout forwarding, failure propagation, and miner blocking.
fixture attempted to write the sandbox-blocked
~/.dashmatepath; the changedfocused suite completed independently.
warnings remain. Edited YAML parses, deterministic trigger/cache assertions
pass,
yarn constraintspasses, andgit diff --checkis clean.PR run
30025175452proved the Core convergence gate: all three Dashmate E2Espassed, including the cached local-network job. Both browser shards then
reproduced the repeated-cleanup exception, while the Node platform and
functional suites timed out during wallet startup without exposing the
underlying validation error. Diagnostic run
30029414181on561da403d8preserved that error as
SPV: Header 2e53e5ed...d8afbd55 is invalidduring theheight-1 request for 1393 regtest headers. Signed head
3bfa5d7dfbpins andfixes the confirmed no-retarget mismatch; its full E2E run is in progress.
Breaking Changes
No API changes, but two wallet-lib behaviour changes are worth calling out for
anyone relying on the previous behaviour.
skipSynchronizationandskipSynchronizationBeforeHeightno longer affectheader synchronization. Headers are validated as a chain from an
authenticated root, so a mid-chain start cannot be verified. Both options are
still declared in
Wallet.d.tsand are still honoured byTransactionsSyncWorker, so one option now means two different thingsdepending on the worker; header sync logs a warning naming which of the two
applies rather than ignoring it silently. Reconciling the two workers is
follow-up work.
Every new wallet now syncs headers from genesis, which on mainnet is
roughly 2.3M headers rather than the trailing window a new-mnemonic wallet
used to fetch. This is the cost of authenticating the chain.
CONSTANTS.STORAGE.versionis bumped to 4 so caches written before this changeare discarded. They are rooted at an unauthenticated checkpoint, and
Storage#configureonly wipes a wallet when the skip value changes, so anunchanged value would otherwise carry a tainted chain forward.
Checklist:
For repository code-owners and collaborators only
Post-Deploy Monitoring & Validation
an infrastructure flake requires one.
every Dashmate, platform-suite, browser, and functional E2E job; no dependent
job is unexpectedly skipped; all launched jobs finish green.
Client already closed,SPV:,Empty SPV chain,unauthenticated remote checkpoint,mismatched block hashes,Core tips do not match, andcache-hit.first historical batch is rejected, convergence reaches the five-minute
deadline, a cold-cache Dashmate job runs no Mocha command, or an expected E2E
job is skipped/cancelled.
gh run view; revert theresponsible startup/workflow change if it broadly blocks valid local-network
startup, otherwise correct the pinned root cause and rerun the full fleet
before merge.
Summary by CodeRabbit