test: add backend E2E tests for masternode identity features (TC-084 to TC-090)#827
Draft
lklimek wants to merge 20 commits into
Draft
test: add backend E2E tests for masternode identity features (TC-084 to TC-090)#827lklimek wants to merge 20 commits into
lklimek wants to merge 20 commits into
Conversation
…to TC-090) Seven new tests covering masternode/evonode identity operations: - TC-084: Load Masternode identity by ProTx hash (all keys) - TC-085: Load Evonode identity (same creds, different type label) - TC-086: Load MN with voting key only (partial keys) - TC-087: Refresh MN identity (load + refresh round-trip) - TC-088: Load MN with invalid ProTx (error case, always runs) - TC-089: Load MN with wrong voting key (error case) - TC-090: Vote Lock on contested DPNS name with MN voter identity Tests are gated on E2E_MN_* env vars and skip gracefully when credentials are unavailable. Each test is fully independent. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- TC-086: reuse build_mn_identity_input with modified MnCredentials instead of manual IdentityInputToLoad construction (CODE-001/CODE-003) - TC-088/TC-089: log errors via Display not Debug to avoid potential credential leakage in copy-paste scenarios (SEC-001) - TC-090: use full u32 range for random contest name (CODE-002) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
…ntly The payout key hits the same PrivateKeyOnMainIdentity assertion as the owner key in TC-084, and no test requires it specifically. Dashmate does not persistently store the payout address private key, making it unavailable in practice. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add `#[tracing::instrument]` to all MN identity tests and helpers so every log line is tagged with the test function name - Use structured tracing fields (id=, identity_type=, error=) instead of manual TC-0XX prefixes in log messages - Enable `FmtSpan::ENTER` in test subscriber for span entry events - Drop `--test-threads=1` from main.rs doc comment — tests are independent and safe to run in parallel Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Document established conventions for writing backend E2E tests: - Test independence and parallel execution (no --test-threads=1) - Tracing spans via #[tracing::instrument] for parallel log output - Structured tracing fields instead of string-interpolated prefixes - run_task_with_nonce_retry for state transitions - Assertion quality rules (specific values, filter by ID, FeatureGate) - Error path testing conventions Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
`load_mn_credentials()` reads env vars before `ctx().await` initializes the harness, so `dotenvy::dotenv()` hadn't been called yet. This caused 6 MN tests to silently skip (env vars invisible) when credentials were only defined in the project root `.env` file. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When a single Core transaction sends equal amounts to multiple never-used addresses (belonging to different wallets), only the first output becomes spendable via InstantSend lock. The other wallets see total_balance but spendable_balance stays at 0. This test creates 3 fresh wallets, sends one multi-output tx, and asserts all 3 should have spendable funds. Also adds create_empty_test_wallet() helper to BackendTestContext for registering wallets without funding them. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Adds test_multi_output_single_wallet_spendable: sends one Core transaction with 3 outputs to 3 distinct BIP44 addresses of the SAME wallet. Uses GenerateReceiveAddress backend task to derive each address (advances the BIP44 index properly). Asserts all 3 outputs become spendable via IS lock. Expected to fail until the IS lock bug is fixed — only 1 of 3 outputs gets marked as spendable. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Adapts dash-evo-tool to rust-dashcore 309fac8 API changes shipped via platform rev 2cc4cdcf85ce62b437031671336bf622b3aafaa5. API surface changes handled: - `MasternodeListEngine::feed_qr_info` signature changed: now takes `&QRInfoBlockData` rather than an `Option<F>` height-lookup closure. Both call sites in `masternode_list_diff_screen.rs` now pre-populate the bundle via a new `build_qr_info_block_data` helper that resolves hash->height from the screen's cache/Core RPC and height->hash for cycle boundary blocks via `get_block_hash` (Core RPC). - `MasternodeListEngine::rotated_quorums_per_cycle` is now a `BTreeMap<BlockHash, BTreeMap<u16, QualifiedQuorumEntry>>` (was a `Vec`). `render_last_commitments` now destructures the inner map directly and the quorum index comes from the map key. - `AddressProvider::on_address_found` / `on_address_absent` are now `async fn` (via the trait's `#[async_trait]`). `WalletAddressProvider` is annotated with `#[async_trait]` and both callbacks made `async`. - `sml::quorum_validation_error::ClientDataRetrievalError` was removed upstream. The only DET uses were inside the feed_qr_info closure that is now replaced wholesale; the import is gone. No behavioural change - this is pure API adaptation. All existing SPV fixes on this branch (DB fallback for start_spv, atomic BIP44 pool extension, SPV restart on mid-session import) continue to behave identically and their tests still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
At rust-dashcore 309fac8, WalletManager gained an independent filter_committed_height field. FiltersManager::new() reads filter_committed_height() for its "already synced" guard; the field is no longer derived from synced_height. Previously clear_data_dir called update_synced_height(0), which pre-bump effectively reset both fields via the trait's default impl. Post-bump it only resets synced_height, leaving filter_committed_height at its stale previous value. FiltersManager then logs "Filters already synced to N" and skips the rescan, producing zero balance after a Clear SPV Data. Switch clear_data_dir to update_filter_committed_height(0), which unconditionally sets the field and only bumps synced_height upward if the incoming height is larger — exactly the semantics we need for a rescan-floor reset. Adds a regression test asserting filter_committed_height is lowered to 0 after clear_data_dir(). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The previous generator `format!("e2emn{:08x}", rand::random::<u32>())`
emitted names containing hex digits 2-9 and the `2` in its prefix, all
of which fall outside the DPNS contested-resource alphabet
`[a-zA-Z01-]`. Platform accepted the registration as an uncontested
domain document, then rejected the subsequent MasternodeVote with
`VotePollNotFoundError`, which surfaced as an opaque 72-second vote
timeout.
Changes:
- Replace the hex generator with `generate_contested_dpns_name()`,
which composes a fixed `e0emn` prefix (alphabet-safe, still
recognisable as test data) with an 8-char random suffix drawn from
`[a-z01]`. 13 chars total, always inside the 3..=19 bound.
- Insert a precheck between DPNS registration and the vote cast:
`wait_for_dpns_contest()` polls Platform via the
`vote_polls_by_document_type_query` SDK endpoint until the contested
resource for `(dash, normalizedLabel)` appears, up to 30s. On
timeout it panics with a diagnostic naming the regex, so future
regressions in the generator or registration path fail loudly and
obviously instead of hiding behind a vote timeout.
- Minor rustfmt hits in sibling test files picked up by
`cargo +nightly fmt --all` (no semantic changes).
QA-003: The per-vote entry in `BackendTaskSuccessResult::DPNSVoteResults`
stored the error as `String`, stringifying the `TaskError` chain at the
producer and throwing away all downstream ability to match on variants.
CLAUDE.md's typed-error rule is explicit:
Never store user-facing strings in error variants — error variants
must not contain `String` fields that hold messages for the user.
Replace `Result<(), String>` with `Result<(), Arc<TaskError>>`. `Arc`
wrapping is the minimal accommodation required because
`BackendTaskSuccessResult` derives `Clone` (the UI fan-out in
`dashpay_screen::display_task_result` relies on it) while `TaskError`
intentionally does not implement `Clone`. The `Arc` preserves the full
typed error chain for future structural matching while keeping
`Display` / `Debug` behaviour identical.
User-facing output is unchanged: the DPNS contested names screen still
extracts `e.to_string()` when rendering the bulk-vote banner, which
dereferences through `Arc<TaskError>` to `TaskError`'s `Display` impl.
The `TC-090` E2E assertion (`outcome.is_ok()` plus `{:?}` diagnostic
print) keeps working and now formats the typed error on failure instead
of an opaque string.
Out-of-scope note: a grep across `src/backend_task/` turned up several
other `Result<_, String>` uses (DashPay helpers, identity helpers,
`extract_contract_id_from_error`). They are symptoms of the same
broader pattern but not part of QA-003 and are left for follow-up.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…+ pre-flight existence check
`vote_on_dpns_name` was constructing `ContestedDocumentResourceVotePoll`
with the raw DPNS label passed in by the caller. Platform indexes the
poll under the **normalized** label produced by
`convert_to_homograph_safe_chars` (`i`→`1`, `l`→`1`, `o`→`0`,
`to_lowercase`). Any label containing those characters — i.e. the common
case — produced a `(dash, <raw>)` lookup against an index keyed on
`(dash, <normalized>)`, triggering `VotePollNotFoundError` at
`PrepareProposal`. Client-side this surfaced as a ~70 s opaque
"An unexpected error occurred" timeout from the vote retry chain.
Confirmed against drive-abci logs at heights 319048, 319073, and 318950.
Changes:
* Normalize the label via `convert_to_homograph_safe_chars` before
building `index_values`. Extracted as `dpns_vote_poll_index_values`,
a pure helper with unit coverage.
* Add a pre-flight existence check that queries Platform for the vote
poll before broadcasting any state transitions. Missing polls now
return immediately instead of burning ~70 s on retries.
* New `TaskError::VotePollNotFound { name }` variant with a
user-facing message (Everyday User persona, "what happened + what
to do"): "The contested name \"{name}\" is not currently open for
voting. It may have been resolved or may not exist. Refresh the
contested names list and try again."
* Unit tests pin the normalization contract (`alice` → `a11ce`,
`bar22` → `bar22`) so regressions in the helper fail in CI.
Other call-sites surveyed:
* `query_dpns_vote_contenders.rs` builds the same poll but is only
called with labels already fetched from Platform (normalized), so
is left unchanged per scope.
* `query_ending_times.rs` only **reads** `index_values` from polls
returned by Platform — no construction, no normalization needed.
In the DET UI, `VoteOnDPNSNames` is dispatched with
`contested_name.normalized_contested_name`, so the user-facing path
happens to already supply a normalized string — but the backend must
be defensive regardless of caller (CLI, scheduled vote replay,
tests), hence the fix at the boundary.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…n TC-090 Add a second `wait_for_dpns_contest` call (step 2c) between the initial "contest created" precheck and the actual `VoteOnDPNSNames` dispatch. Belt-and-suspenders against poll cleanup between DPNS registration and voting — e.g. on testnets with short voting windows, or if an earlier vote already resolved the contest while the test was still setting up. The first precheck (step 2b) stays on a 30 s budget; the new step 2c uses a 5 s budget since it's a single query — enough to confirm the poll is still live without adding meaningful runtime to the happy path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
5 tasks
…when/then Address review feedback on #835: the `dpns_vote_poll_index_values` doc comment and the pre-flight check inline comment were over-explanatory; trimmed each to 2-3 lines with concrete examples. The two unit tests now use inline Given/When/Then comments instead of a free-form doc comment — easier to scan and matches project test-style preference. No logic changes; assertions unchanged. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…re-flight comment The ~70 s retry chain text is platform-version-bound and will age badly once Platform's MasternodeVote picks up `validates_full_state_on_check_tx()` (b44ccff5, v3.1.0-dev.1+) — CheckTx will start rejecting missing polls and the client-visible cost drops. The pre-flight's durable value is fail-fast with an actionable error regardless of server-side timing. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…l_index_values Bot reviewers on #835 (copilot-pull-request-reviewer, coderabbitai) flagged that convert_to_homograph_safe_chars ran twice per vote: once by the caller to build normalized_label for the pre-flight query, and once inside the helper. Harmless today, but if either call site ever picks up a different normalization the pre-flight and the vote poll would key off different strings — silently reproducing the very bug this PR fixes. Helper now takes a pre-normalized &str and returns it verbatim. The caller's normalized_label is passed through once, used for both the pre-flight query and the vote poll's index_values. A dedicated test pins convert_to_homograph_safe_chars("alice") == "a11ce" so the normalization contract itself stays covered after the move. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
10 tasks
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
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.
Summary
framework/mn_helpers.rsmodule for credential loading from env vars.E2E_MN_*env vars and skip gracefully when credentials unavailable.d17cfae8, filed standalone as #835) surfaced while developing TC-090 — see the "Production fix" section below.Test Catalog
Environment Variables
E2E_MN_PROTX_HASHE2E_MN_VOTING_KEYE2E_MN_OWNER_KEYE2E_MN_PAYOUT_KEYProduction fix discovered during TC-090 development
TC-090 failed three consecutive times against testnet with an opaque ~70 s "unexpected error" on every masternode vote. Drive-abci server logs (correlated by block height across runs at 318950 / 319048 / 319073) revealed the real cause:
VotePollNotFoundErroratPrepareProposal.Root cause:
src/backend_task/contested_names/vote_on_dpns_name.rswas building the vote poll'sindex_valuesfrom the raw DPNS label. Platform indexes contested polls under the homograph-safe normalized label (convert_to_homograph_safe_chars:i→1,l→1,o→0, then lowercase). Any label containingi/l/osilently mismatched the index. The tx passedCheckTxbecausevalidates_full_state_on_check_tx()forMasternodeVotewas only added to Platform in December 2025 (v3.1.0-dev.1 onward); testnet v3.0.x predates it.The fix — commit
d17cfae8on this branch — normalizes the label at the backend boundary, adds a pre-flight existence check viaContestedResource::fetch_manythat short-circuits the retry chain, and introduces a typedTaskError::VotePollNotFound { name }variant with an Everyday-User-persona actionable message. After the fix, TC-090 passes in ~30 s end-to-end (previously: 3 × ~70 s failures).That same commit is filed as #835 against
v1.0-devfor cherry-pick review. On rebase of this branch after #835 merges, git will deduplicate.Also on this branch:
ca46282a— wraps per-voteTaskErrorinArcinsideBackendTaskSuccessResult::DPNSVoteResults(required becauseTaskErroris deliberately non-Clone).76339e55— test hardening: TC-090 re-confirms the contested vote poll exists immediately before the vote dispatch viawait_for_dpns_contest, catching poll-disappearance races.Full-suite verification
Ran the full backend-e2e suite (68 tests,
--release,--ignored) against this branch tip (76339e55) in 408 s (~7 min) on testnet.PR 827 canaries — all PASS:
Other suite failures — pre-existing, not regressions:
ConnectionRefused— SPV-only single-key wallet tasks still calldashdwithout SPV fallbackConfirmationTimeoutin SPV modeAddKeyToIdentity→InvalidIdentityRevisionError(stale revision)None of these failures touch code modified by this PR. They live in test modules introduced in #818 (the framework PR) and fail against
v1.0-devdirectly. Filing them as follow-ups is out of scope for #827.Test plan
cargo clippy --all-features --all-targets -- -D warningspassescargo +nightly fmt --all -- --checkpassesE2E_MN_PROTX_HASHis unset🤖 Co-authored by Claudius the Magnificent AI Agent