Repository navigation
Conversation
#5456) Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
…ion 14 (#5463) Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
… same operator key (#5461)
…e re-encodes its operator key (#5462)
…o a relaunch resumes instead of rewinding again (#5026) Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
Brings in v5.0.0-rc.4 (#5456, #5458, #5463, #5461, #5462, #5480) and #5026 from v5.0-dev at f282f29. Conflicts: - CHANGELOG.md: [Unreleased] stays on top, followed by the rc.4 entry. - rs-platform-wallet/src/changeset/core_bridge.rs: keep v5.1-dev's WalletEvent::ChainTruncated arm and take #5026's SyncHeightAdvanced arm, which drops a watermark queued above the in-memory cursor. Fixes for v5.0-dev code written against APIs v5.1-dev changed: - update_state_masternode_list (#5461): the legacy-to-basic operator key check used blsful's legacy decoder, whose dpp::bls_signatures re-export v5.1-dev removed (#5320). It now moves the legacy flags to their basic positions as blsful did (refusing bits 5 and 6) and decodes the result with dpp::bls::PublicKey, which checks the field bound, the curve and the subgroup as blsful did. dash-pkc's own legacy decoder is not used: it clears stray flag bits, skips the subgroup check and refuses the point at infinity. A throwaway differential run against blsful over 81,773 inputs (valid keys in both schemes, flag variants, bit flips, 500 on-curve points outside the subgroup in both flag layouts, random and edge bytes) gave identical results. The test helper builds a key's negation from the negated secret. - masternode/locator.rs (#5462): the basic-to-legacy id uses dpp::bls::PublicKey::to_legacy_bytes; same differential run. - manager/load.rs and platform-wallet-ffi persistence.rs (#5026): ExtendedPubKey::from_priv no longer takes a secp256k1 context. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
|
Waiting for bot review — coderabbitai not yet · thepastaclaw requested changes — dismiss the review or push a fix, 5 threads unresolved — resolve them. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
the
waiting-bots
|
⛔ Final review complete — 5 blocking finding(s) (commit c89ff0b) · triage: critical |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
Static verification of c89ff0b confirms five blocking issues: two changes to shipped consensus generations without PlatformVersion activation, an unsafe truncation/backfill restart sequence, and two binding-layer ownership violations. The PV14 hash-key versioning finding is not retained because that generation belongs only to unreleased protocols. Current CI shows Rust workspace and native SDK checks passing, with one browser suite still pending; no local builds or tests were run.
🔴 5 blocking | 🟡 5 suggestion(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: gemini-3.8-flash-high (agent: phase1-reviewer, role: general); reviewer 6: gemini-3.8-flash-high (agent: phase1-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: tenderdash-consensus-security); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The large, intricate diff changes consensus-critical operator-key decoding in update_state_masternode_list/mod.rs, protocol-14 identity-key validation, and persisted wallet state with database migrations. - Phase 1 reviewers:
gemini-3.8-flash-high— general (completed, effort high); agentphase1-reviewer,gemini-3.8-flash-high— rust-quality (completed, effort high); agentphase1-reviewer - Phase 1 model:
gemini-3.8-flash-high— antigravity quota: weekly 71% left, 5h 100% left - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— tenderdash-consensus-security (completed, effort xhigh); agentphase2-reviewer
🤖 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/rs-platform-wallet/src/wallet/identity/network/payments.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/payments.rs:380-389: Account for chain-truncation rewinds before persisting coverage
The merge makes the missing rewind bookkeeping in PlatformWalletInfo::truncate_above observable through durable coverage today, independently of #5380. For example, a permitted truncation from 1000 to the last chain lock at 999 lowers the live cursor but leaves the host cursor at 1000. Registering a receival contact with checkpoint 999 then takes the forward-covered path: floor and owed are both None, so this round persists coverage without lowering the host cursor. After restart, the restored coverage suppresses reconciliation's rewind, while the pinned dash-spv scanner skips the range through the restored cursor, although the new account was never scanned through block 1000. Previously the process-local coverage guard disappeared on restart and the contact triggered a backfill. Route successful cursor lowering in truncate_above through the same barrier and cursor-debt bookkeeping as account additions and manual resets. Add a truncation → contact registration → record-only reconciliation → restart regression, including advances queued before truncation; dropping queued heights above the current cursor does not protect this record-only path.
In `packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_list/update_state_masternode_list/v0/mod.rs`:
- [BLOCKING] packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_list/update_state_masternode_list/v0/mod.rs:130-138: Keep the Core-V24 consensus change out of shipped list generation v0
Protocols 1–13 still select update_masternode_list generation 0. Once Core V24 is active, this new call removes an operator-key update that the previous binary applied, preserving different stored bytes and a different operator identity under the same Platform protocol. The Core-height gate makes upgraded nodes deterministic and preserves earlier history, but it does not make the edit inert: the new tests exercise the changed result at PV13, and Core activation does not advance PlatformVersion. The book explicitly requires a new generation when a runtime-gated edit can change shipped behavior. Preserve the old v0 path and activate normalization through a new list-update generation selected only by unreleased protocol tables. Coordinate that correction with v5.0-dev so the branches do not acquire different rules for the same protocol; a deployment instruction to upgrade every node before Core activation is not the required version boundary.
In `packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_identities/update_operator_identity/v0/mod.rs`:
- [BLOCKING] packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_identities/update_operator_identity/v0/mod.rs:214-218: Version activation of the new masternode key operation
The replacement is not consensus-equivalent in this shipped generation. When a new payout key's hash already exists in the unique-key index, the previous AddNewKeysToIdentity path returns UniqueKeyAlreadyExists; AddNewKeysToMasternodeIdentity instead omits the non-unique reference and successfully adds the key. A masternode can reach this branch by changing its payout address to the HASH160 of an already registered user-identity public key, including at PV13 without Core V24 activation. The new Drive dispatcher reuses add_new_keys_to_identity: 0, so it supplies no activation boundary. Preserve the old operation selection in shipped operator and owner-withdrawal generations, introduce new generations selected only by unreleased tables, and independently version the new Drive method. Matching historical blocks or comparing operations only for non-colliding hashes does not establish agreement on this reachable collision branch. Carry the same correction to the source branch to preserve cross-branch protocol consistency.
In `packages/rs-unified-sdk-jni/src/persistence.rs`:
- [BLOCKING] packages/rs-unified-sdk-jni/src/persistence.rs:2921-2929: Use the wallet-owned backfill decoder instead of duplicating it in JNI
JNI introduces a second implementation of the persisted coverage format: it defines the 72-byte entry width, validates blob alignment, chooses to discard malformed records, and decodes each field. The wallet already owns this exact codec and malformed-blob contract in DashPayBackfillRecord::from_parts, with covered_bytes as its inverse; the JNI write trampoline independently reproduces that packing too. This violates the project's single-owner validation boundary and makes a future format change require synchronized native and Android implementations. Keep JVM field access and numeric representation checks at the boundary, but expose an owner-backed conversion through platform-wallet-ffi and delegate both blob decoding and encoding to it. The existing FFI restore path can still perform canonicalization; the problem here is the additional format validation and admission implementation, not a claim that Android currently skips canonicalization.
In `packages/rs-platform-wallet-ffi/src/persistence.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/persistence.rs:2307-2312: Keep backfill coverage policy in platform-wallet rather than the FFI persister
This filter distinguishes an empty invalidation from a covering record and decides whether Rust-emitted state is delivered based on atomicity, cursor presence, and cursor callback delivery. A withheld record does not itself fail the store. Those are coverage eligibility and invalidation decisions, not C representation conversion, and platform-wallet already owns them through persists_backfill_coverage and settle_published_backfill. The book permits bindings to persist and marshal Rust-owned state, not to implement a second version of its safety policy. Keep callback availability and delivery results in the adapter, but centralize the record-delivery decision in the wallet owner using those facts. The adapter should then marshal the selected record or report that a required write could not be completed, rather than silently choosing a binding-specific subset.
In `packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_list/update_state_masternode_list/mod.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_list/update_state_masternode_list/mod.rs:170-172: Test subgroup rejection against the matching basic re-encoding
The two x=0 inputs do not currently demonstrate subgroup rejection. The loop compares each against an unrelated valid key and its unchanged legacy bytes, so a decoder without subgroup validation would still return false because neither comparison matches its translated basic encoding. Add comparisons against 0x80 followed by zeros for the null legacy encoding and 0xa0 followed by zeros for its sign-set counterpart. These assertions specifically detect removal of the subgroup check on which this consensus decoder port relies.
In `packages/rs-platform-wallet-ffi/src/wallet_restore_types.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/wallet_restore_types.rs:727-730: Carry the wallet API migration into Unreleased release notes
The new fields enlarge the host-allocated WalletRestoreEntryFFI array element, and ContactRequestFFI explicitly grows from 200 to 208 bytes. Size negotiation on PersistenceCallbacksExtension protects the callback extension, not the allocation size or stride of these other structs, so C consumers must rebuild against matching headers. CHANGELOG.md retains the rc.4 entry but has no Unreleased entry for #5026, which landed afterward. Record the rebuild requirement there alongside the Room schema 15 migration and affected public Rust enum matches, so consumers do not mistake callback compatibility for compatibility of all persistence structures.
In `packages/rs-platform-wallet/src/changeset/core_bridge.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/changeset/core_bridge.rs:396-405: Track a producer-owned scan epoch upstream
At both the pinned rust-dashcore revision and its supplied default tip, SyncHeightAdvanced contains only wallet_id and height. Platform therefore reconstructs scan identity by counting update_synced_height hooks, consuming events in order, and carrying counters across same-id wallet replacements. The pinned process_block implementation supports the current hook/emission pairing, but this is a workaround for missing producer-owned event identity in dashpay/rust-dashcore, not a responsibility every persistence consumer should reproduce. Track an upstream change that attaches a scan/lifecycle epoch to cursor events and advances it through the authoritative rewind/reset APIs. Link that issue or PR here; #649's scan-certification safeguards and the existing resync/recovery work do not provide cursor-event identity.
In `packages/rs-platform-wallet/src/manager/load.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/manager/load.rs:270-272: Skip already-loaded wallets before validating every external address
The new external-pool validation runs before the existing-wallet check at the publication step. The loader is deliberately called again on SDK reactivation and StateFlow re-emission, so a call that ultimately skips an already-loaded wallet still re-derives every address in its marked external pools. pool_derives_from performs synchronous derive_address_at work for each restored row on the async worker. Add an early existing-wallet check before this validation while retaining the authoritative publication-time check for concurrent loaders. The existing pool-validation counter can assert that an idempotent repeated load performs no derivation.
In `packages/rs-platform-wallet/src/masternode/locator.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/masternode/locator.rs:395-402: Avoid pushing a duplicate operator identifier for identical encodings
The DPP PublicKey facade accepts canonical infinity and returns the same bytes from to_legacy_bytes. For that input, this unconditional push adds the literal operator identifier twice. first_operator_identity iterates candidates without deduplication, so a missing identity causes the same proof-backed fetch to run twice. Append the legacy candidate only when its bytes differ from the input.
| let needed_cursor = match (floor, owed) { | ||
| (Some(floor), Some(owed)) => Some(floor.min(owed)), | ||
| (Some(floor), None) => Some(floor), | ||
| (None, owed) => owed, | ||
| }; | ||
| // Never write a cursor at or above the one the host already holds: | ||
| // coverage is safe beside any lower durable cursor, and writing a | ||
| // higher one would claim rows the adapter has not persisted yet. | ||
| let cursor_to_write = | ||
| needed_cursor.filter(|cursor| durable.is_none_or(|durable| *cursor < durable.height)); |
There was a problem hiding this comment.
🔴 Blocking: Account for chain-truncation rewinds before persisting coverage
The merge makes the missing rewind bookkeeping in PlatformWalletInfo::truncate_above observable through durable coverage today, independently of #5380. For example, a permitted truncation from 1000 to the last chain lock at 999 lowers the live cursor but leaves the host cursor at 1000. Registering a receival contact with checkpoint 999 then takes the forward-covered path: floor and owed are both None, so this round persists coverage without lowering the host cursor. After restart, the restored coverage suppresses reconciliation's rewind, while the pinned dash-spv scanner skips the range through the restored cursor, although the new account was never scanned through block 1000. Previously the process-local coverage guard disappeared on restart and the contact triggered a backfill. Route successful cursor lowering in truncate_above through the same barrier and cursor-debt bookkeeping as account additions and manual resets. Add a truncation → contact registration → record-only reconciliation → restart regression, including advances queued before truncation; dropping queued heights above the current cursor does not protect this record-only path.
source: gemini-3.8-flash-high (phase1-reviewer: general, rust-quality); gpt-6.1-sol (phase2-reviewer: general)
| if !start_from_scratch { | ||
| // Protocol versions 1 to 13 select this generation. The call changes no entry unless | ||
| // DEPLOYMENT_V24 is active at `core_block_height`, so blocks at earlier Core heights | ||
| // execute unchanged. | ||
| self.drop_unchanged_operator_keys( | ||
| state.full_masternode_list(), | ||
| core_block_height, | ||
| &mut masternode_diff.updated_mns, | ||
| )?; |
There was a problem hiding this comment.
🔴 Blocking: Keep the Core-V24 consensus change out of shipped list generation v0
Protocols 1–13 still select update_masternode_list generation 0. Once Core V24 is active, this new call removes an operator-key update that the previous binary applied, preserving different stored bytes and a different operator identity under the same Platform protocol. The Core-height gate makes upgraded nodes deterministic and preserves earlier history, but it does not make the edit inert: the new tests exercise the changed result at PV13, and Core activation does not advance PlatformVersion. The book explicitly requires a new generation when a runtime-gated edit can change shipped behavior. Preserve the old v0 path and activate normalization through a new list-update generation selected only by unreleased protocol tables. Coordinate that correction with v5.0-dev so the branches do not acquire different rules for the same protocol; a deployment instruction to upgrade every node before Core activation is not the required version boundary.
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, platform-versioning, rust-quality, security-auditor, tenderdash-consensus-security)
| if !non_unique_keys_to_add.is_empty() { | ||
| drive_operations.push(IdentityOperation(AddNewKeysToIdentity { | ||
| drive_operations.push(IdentityOperation(AddNewKeysToMasternodeIdentity { | ||
| identity_id: operator_identifier.to_buffer(), | ||
| unique_keys_to_add: vec![], | ||
| non_unique_keys_to_add, | ||
| })); |
There was a problem hiding this comment.
🔴 Blocking: Version activation of the new masternode key operation
The replacement is not consensus-equivalent in this shipped generation. When a new payout key's hash already exists in the unique-key index, the previous AddNewKeysToIdentity path returns UniqueKeyAlreadyExists; AddNewKeysToMasternodeIdentity instead omits the non-unique reference and successfully adds the key. A masternode can reach this branch by changing its payout address to the HASH160 of an already registered user-identity public key, including at PV13 without Core V24 activation. The new Drive dispatcher reuses add_new_keys_to_identity: 0, so it supplies no activation boundary. Preserve the old operation selection in shipped operator and owner-withdrawal generations, introduce new generations selected only by unreleased tables, and independently version the new Drive method. Matching historical blocks or comparing operations only for non-colliding hashes does not establish agreement on this reachable collision branch. Carry the same correction to the source branch to preserve cross-branch protocol consistency.
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, platform-versioning, rust-quality, security-auditor, tenderdash-consensus-security)
| if floor < 0 | ||
| || rewound_from < 0 | ||
| || !blob | ||
| .len() | ||
| .is_multiple_of(DASHPAY_BACKFILL_COVERED_ENTRY_LEN) | ||
| { | ||
| log::warn!( | ||
| "load: malformed DashPay backfill record on the wallet row (floor={floor}, \ | ||
| rewound_from={rewound_from}, blob_len={}); reading as no record", |
There was a problem hiding this comment.
🔴 Blocking: Use the wallet-owned backfill decoder instead of duplicating it in JNI
JNI introduces a second implementation of the persisted coverage format: it defines the 72-byte entry width, validates blob alignment, chooses to discard malformed records, and decodes each field. The wallet already owns this exact codec and malformed-blob contract in DashPayBackfillRecord::from_parts, with covered_bytes as its inverse; the JNI write trampoline independently reproduces that packing too. This violates the project's single-owner validation boundary and makes a future format change require synchronized native and Android implementations. Keep JVM field access and numeric representation checks at the boundary, but expose an owner-backed conversion through platform-wallet-ffi and delegate both blob decoding and encoding to it. The existing FFI restore path can still perform canonicalization; the problem here is the additional format validation and admission implementation, not a claim that Android currently skips canonicalization.
source: gpt-6.1-sol (phase2-reviewer: architecture-layering)
| .as_ref() | ||
| .is_some_and(|core| core.synced_height.is_some()); | ||
| if let Some(record) = changeset.dashpay_backfill.as_ref().filter(|record| { | ||
| record.is_empty() | ||
| || (host_stores_cursors && (!backfill_cursor_owed || cursor_delivered)) | ||
| }) { |
There was a problem hiding this comment.
🔴 Blocking: Keep backfill coverage policy in platform-wallet rather than the FFI persister
This filter distinguishes an empty invalidation from a covering record and decides whether Rust-emitted state is delivered based on atomicity, cursor presence, and cursor callback delivery. A withheld record does not itself fail the store. Those are coverage eligibility and invalidation decisions, not C representation conversion, and platform-wallet already owns them through persists_backfill_coverage and settle_published_backfill. The book permits bindings to persist and marshal Rust-owned state, not to implement a second version of its safety policy. Keep callback availability and delivery results in the adapter, but centralize the record-delivery decision in the wallet owner using those facts. The adapter should then marshal the selected record or report that a required write could not be completed, rather than silently choosing a binding-specific subset.
source: gpt-6.1-sol (phase2-reviewer: ffi-engineer)
| let null_key = vec![0u8; 48]; | ||
| let mut sign_set_x_zero = vec![0u8; 48]; | ||
| sign_set_x_zero[0] = 0x80; |
There was a problem hiding this comment.
🟡 Suggestion: Test subgroup rejection against the matching basic re-encoding
The two x=0 inputs do not currently demonstrate subgroup rejection. The loop compares each against an unrelated valid key and its unchanged legacy bytes, so a decoder without subgroup validation would still return false because neither comparison matches its translated basic encoding. Add comparisons against 0x80 followed by zeros for the null legacy encoding and 0xa0 followed by zeros for its sign-set counterpart. These assertions specifically detect removal of the subgroup check on which this consensus decoder port relies.
| let null_key = vec![0u8; 48]; | |
| let mut sign_set_x_zero = vec![0u8; 48]; | |
| sign_set_x_zero[0] = 0x80; | |
| let null_key = vec![0u8; 48]; | |
| let mut sign_set_x_zero = vec![0u8; 48]; | |
| sign_set_x_zero[0] = 0x80; | |
| assert!(!is_basic_re_encoding_of_legacy_key( | |
| &null_key, | |
| &sign_set_x_zero | |
| )); | |
| let mut basic_negative_x_zero = vec![0u8; 48]; | |
| basic_negative_x_zero[0] = 0xa0; | |
| assert!(!is_basic_re_encoding_of_legacy_key( | |
| &sign_set_x_zero, | |
| &basic_negative_x_zero | |
| )); |
source: gpt-6.1-sol (phase2-reviewer: rust-quality)
| pub has_dashpay_backfill: bool, | ||
| /// Lowest height the backfill rewound the cursor to. | ||
| pub dashpay_backfill_floor: u32, | ||
| /// Highest cursor the backfill rewound from — the height the scan climbs |
There was a problem hiding this comment.
🟡 Suggestion: Carry the wallet API migration into Unreleased release notes
The new fields enlarge the host-allocated WalletRestoreEntryFFI array element, and ContactRequestFFI explicitly grows from 200 to 208 bytes. Size negotiation on PersistenceCallbacksExtension protects the callback extension, not the allocation size or stride of these other structs, so C consumers must rebuild against matching headers. CHANGELOG.md retains the rc.4 entry but has no Unreleased entry for #5026, which landed afterward. Record the rebuild requirement there alongside the Room schema 15 migration and affected public Rust enum matches, so consumers do not mistake callback compatibility for compatibility of all persistence structures.
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, platform-versioning, rust-quality, security-auditor, tenderdash-consensus-security)
| /// The barrier counts instead. The engine emits a `SyncHeightAdvanced` only | ||
| /// right after calling this wallet's `update_synced_height` hook, under the | ||
| /// same manager write lock, so the hook counts emissions exactly | ||
| /// ([`note_emitted`](Self::note_emitted)), and the adapter counts them back | ||
| /// down as it projects them ([`take_next`](Self::take_next)). A rewind — | ||
| /// made under that same write lock — snapshots how many are still in flight | ||
| /// ([`arm`](Self::arm)): exactly those were emitted before it, and the | ||
| /// adapter drops them as they arrive. Every later advance is tagged with the | ||
| /// rewind epoch it was projected in, and a commit strips one whose epoch a | ||
| /// rewind has since superseded, which covers an advance the adapter had |
There was a problem hiding this comment.
🟡 Suggestion: Track a producer-owned scan epoch upstream
At both the pinned rust-dashcore revision and its supplied default tip, SyncHeightAdvanced contains only wallet_id and height. Platform therefore reconstructs scan identity by counting update_synced_height hooks, consuming events in order, and carrying counters across same-id wallet replacements. The pinned process_block implementation supports the current hook/emission pairing, but this is a workaround for missing producer-owned event identity in dashpay/rust-dashcore, not a responsibility every persistence consumer should reproduce. Track an upstream change that attaches a scan/lifecycle epoch to cursor events and advances it through the authoritative rewind/reset APIs. Link that issue or PR here; #649's scan-certification safeguards and the existing resync/recovery work do not provide cursor-event identity.
source: gpt-6.1-sol (phase2-reviewer: architecture-layering)
| if crate::changeset::persists_outbound_account_markers(&*self.persister) { | ||
| let distrusted = | ||
| distrust_markers_of_foreign_external_pools(&wallet, &mut platform_info); |
There was a problem hiding this comment.
🟡 Suggestion: Skip already-loaded wallets before validating every external address
The new external-pool validation runs before the existing-wallet check at the publication step. The loader is deliberately called again on SDK reactivation and StateFlow re-emission, so a call that ultimately skips an already-loaded wallet still re-derives every address in its marked external pools. pool_derives_from performs synchronous derive_address_at work for each restored row on the async worker. Add an early existing-wallet check before this validation while retaining the authoritative publication-time check for concurrent loaders. The existing pool-validation counter can assert that an idempotent repeated load performs no derivation.
source: gpt-6.1-sol (phase2-reviewer: rust-quality)
| if let Ok(legacy) = | ||
| BlsPublicKey::try_from(operator_public_key.as_slice()).and_then(|key| key.to_legacy_bytes()) | ||
| { | ||
| ids.push(Identifier::create_operator_identifier( | ||
| pro_tx_hash_display, | ||
| &legacy, | ||
| )); | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Avoid pushing a duplicate operator identifier for identical encodings
The DPP PublicKey facade accepts canonical infinity and returns the same bytes from to_legacy_bytes. For that input, this unconditional push adds the literal operator identifier twice. first_operator_identity iterates candidates without deduplication, so a missing identity causes the same proof-backed fetch to run twice. Append the legacy candidate only when its bytes differ from the input.
| if let Ok(legacy) = | |
| BlsPublicKey::try_from(operator_public_key.as_slice()).and_then(|key| key.to_legacy_bytes()) | |
| { | |
| ids.push(Identifier::create_operator_identifier( | |
| pro_tx_hash_display, | |
| &legacy, | |
| )); | |
| } | |
| if let Ok(legacy) = | |
| BlsPublicKey::try_from(operator_public_key.as_slice()).and_then(|key| key.to_legacy_bytes()) | |
| { | |
| if &legacy != operator_public_key { | |
| ids.push(Identifier::create_operator_identifier( | |
| pro_tx_hash_display, | |
| &legacy, | |
| )); | |
| } | |
| } |
source: gemini-3.8-flash-high (phase1-reviewer: general, rust-quality)
This branch has not been deployed
3 participants
Basic explanation
What this does: Brings
v5.1-devup to date withv5.0-devatf282f29999: the 5.0.0-rc.4 release (six PRs) and #5026, which landed onv5.0-devafter rc.4. 5.0.0-rc.3 and #5431 already reachedv5.1-devthrough #5432, whose merge base (51ee8a7261) comes after rc.3.Value:
v5.1-devgets the rc.4 fixes:It also gets #5026's persisted DashPay coreHeight backfill coverage.
Risks: Low to moderate.
devat626c91d1, and both branches already use GroveDBfc7028d8.v5.1-devno longer ships (refactor: share rust-dashcore BLS backend through dpp::bls #5320). It was ported todpp::bls. The ported check accepts exactly what the blsful version accepts; see the merge fixes and how it was checked below.v5.1-dev's reorg handling (ChainTruncated). That is fine today; see Known limitations.Issue being fixed or feature implemented
v5.1-devwas seven PRs behindv5.0-dev.What was done?
Merged
origin/v5.0-dev(f282f29999) intoorigin/v5.1-dev(a9336f9486) as a merge commit. Incoming PRs:Conflicts resolved by hand:
CHANGELOG.md: kept the[Unreleased]section on top, followed by the rc.4 entry.rs-platform-wallet/src/changeset/core_bridge.rs(build_core_changeset): keptv5.1-dev'sWalletEvent::ChainTruncatedarm, which persists nothing yet (platform-wallet: persist chain truncation (reorg) reported by key-wallet #5380). Took fix(platform-wallet)!: persist DashPay coreHeight backfill coverage so a relaunch resumes instead of rewinding again #5026'sSyncHeightAdvancedarm, which drops a watermark queued above the wallet's in-memory cursor.Merge fixes for
v5.0-devcode written against APIs thatv5.1-devchanged (part of the merge commit):rs-drive-abci/.../update_state_masternode_list/mod.rs(fix(drive-abci)!: keep the operator identity when Core re-encodes the same operator key #5461), consensus code.is_basic_re_encoding_of_legacy_keydecoded the stored operator key with blsful'sfrom_bytes_with_mode(.., Legacy).v5.1-devremoved thedpp::bls_signaturesre-export (refactor: share rust-dashcore BLS backend through dpp::bls #5320). The port does what blsful did:dpp::bls::PublicKey::try_from. Like blsful, that decode checks that x is below the field modulus, that the point is on the curve and that it is in the subgroup, and it accepts the canonical point at infinity.dash-pkc's own legacy (Chia) decoder is deliberately not used. It clears stray flag bits instead of refusing them, skips the subgroup check and refuses the point at infinity, so
v5.1-devwould have replayed protocol 14 differently fromv5.0-dev.How the port was checked:
0c34a7a4with the port on 81,773 inputs: valid keys in both schemes, every first-byte flag variant, random bit flips, 500 on-curve points outside the subgroup in both flag layouts, x = 0, x = p, random bytes and wrong lengths. It found no differences: the same 16,501 legacy encodings were accepted, with byte-identical output. (blsful pinsblst =0.3.12and dash-pkc needs 0.3.17, so the comparison ran as two binaries that exchanged results through a file.)e1.c,map_to_g1.candsqrt.care byte-identical between 0.3.12 and 0.3.17.operator_key_encoding_tests.rs:key_and_negationbuilt the negation with blsful's point negation. It now derives it as the public key of the negated secret (group order minus the secret), which keeps it independent of the encoding logic under test.rs-platform-wallet/src/masternode/locator.rs(fix(platform-wallet): find a masternode's operator identity after Core re-encodes its operator key #5462): the legacy-derived operator identity id usesdpp::bls::PublicKey::try_from(..)andto_legacy_bytes()instead of blsful's Modern decode and Legacy encode. The same differential run covered it: the same 16,530 inputs were accepted, with identical output.rs-platform-wallet/src/manager/load.rsandrs-platform-wallet-ffi/src/persistence.rs(fix(platform-wallet)!: persist DashPay coreHeight backfill coverage so a relaunch resumes instead of rewinding again #5026 tests):ExtendedPubKey::from_privno longer takes a secp256k1 context (secp256k1 0.33, chore(platform)!: update rust-dashcore (incl secp256k1 0.33) #5307).Pins and
Cargo.lockCargo.toml: only the workspace version moves (5.0.0-rc.3→5.0.0-rc.4).v5.1-devkeeps itsdevpin,626c91d1.v5.0-dev's pin (edc4d430) hasn't moved since chore: merge v5.0-dev into v5.1-dev #5432, and the incoming code builds against626c91d1after the fixes above, so nothing needs the integration branch.fc7028d8.Cargo.lock: keptv5.1-dev's lockfile. The merge changes only the 48 workspace package versions (5.0.0-rc.3→5.0.0-rc.4). No crates.io or git package moved, and every build and test ran with--locked.Protocol version 15
PLATFORM_V15withPLATFORM_V14field by field, with comments stripped. They are identical apart fromprotocol_version.v5.1-devhas no PV15-only change yet: fix(drive-abci)!: leave chain locks signed by quorums Platform does not hold to Core (PV15) #5454'sDRIVE_ABCI_METHOD_VERSIONS_V11is not part of this merge, and whichever of the two lands second rebases.validate_identity_public_keys_structure1 inSTATE_TRANSITION_METHOD_VERSIONS_V2.drop_unchanged_operator_keys, shared byupdate_state_masternode_listv0 and v1 (DRIVE_ABCI_METHOD_VERSIONS_V10,update_masternode_list: 1). It is gated on Core's V24 deployment, not on the protocol version.add_new_keys_to_identityinDRIVE_VERSION_V9.identity_create_state_transition.advanced_structure: Some(1)inDRIVE_ABCI_VALIDATION_VERSIONS_V10. Still in place.protocol_version >= 14checks include 15.Generated artefacts
None needed regeneration. The incoming PRs change no GroveDB structure:
should_match_committed_grovedb_structure_jsonpasses at protocol 15. They also change no wasm-sdk signed-response fixtures, and the wasm-sdk tests pass with the committed ones.Files changed on both branches and merged automatically (worth a look in review)
rs-drive-abci/.../update_masternode_identities/update_operator_identity/v0/mod.rs,.../update_masternode_list/mod.rsand.../update_state_masternode_list/v1/mod.rs:v5.1-devonly moved their tests todpp::bls(refactor: share rust-dashcore BLS backend through dpp::bls #5320).rs-drive-abci/.../identity_create_from_addresses/tests.rsand.../identity_update/mod.rsrs-platform-wallet:error.rs,manager/accessors.rs,manager/mod.rs,masternode/locator.rs,test_support.rs,wallet/persister.rs,wallet/platform_wallet_traits.rs,wallet/provider_ecdsa_key_tests.rs,wallet/identity/network/{contact_requests,dpns_marketplace,identity_handle,payments}.rsrs-platform-wallet-ffi:manager.rs,persistence.rsyarn.lock,.pnp.cjsand thepackage.jsonversion bumpsOther effects on
v5.1-dev:5.0.0-rc.3to5.0.0-rc.4, and the changelog gains the rc.4 entry.v5.1-devhad not changed the Room schema, so there is no version collision.Please merge with a merge commit (not squash) so the
v5.0-devhistory stays shared withv5.1-dev.How Has This Been Tested?
Run locally on the merged tree:
cargo check --workspace --all-targets --locked: passes.cargo clippy --workspace --all-targets --all-features --locked -- --no-deps -D warnings(CI's invocation): passes.cargo fmt --all -- --check: passes.cargo test --all-features --locked --lib --testswith CI'sRUST_MIN_STACK=4194304:dpp: 5,773 passed, 0 failed (6 ignored).drive: 4,445 passed, 0 failed (6 ignored). This includesshould_match_committed_grovedb_structure_json.drive-abcilib: 4,477 passed, 0 failed (17 ignored). This includes fix(drive-abci)!: keep the operator identity when Core re-encodes the same operator key #5461's operator key encoding tests and theis_basic_re_encoding_of_legacy_keypins.dash-sdk: 575 passed, 0 failed (22 ignored).platform-wallet,platform-wallet-storageandplatform-wallet-ffi: 3,056 passed, 0 failed (10 ignored).drive-abcistrategy_tests(--all-features,RUST_MIN_STACK=4194304): 115 passed, 0 failed (4 ignored).cargo test -p wasm-sdk --lib --locked: 189 passed, with the committed signed-response fixtures.cargo check -p wasm-sdk -p wasm-dpp2 -p wasm-drive-verify --target wasm32-unknown-unknown --locked: passes.yarn constraintsandyarn install --immutable --immutable-cache: both pass, and the tree stays clean.origin/v5.1-dev: no violations.Known limitations
v5.1-dev's reorg path (PlatformWalletInfo::truncate_above,wallet/platform_wallet_traits.rs) lowers the in-memory synced height without arming fix(platform-wallet)!: persist DashPay coreHeight backfill coverage so a relaunch resumes instead of rewinding again #5026'sRewindBarrieror owing the backfill cursor, asnote_account_add_rewinddoes. This has no effect today. TheChainTruncatedarm persists nothing (platform-wallet: persist chain truncation (reorg) reported by key-wallet #5380), so the stored cursor and rows keep their pre-fork state either way. Whoever makes the persister mirror truncations (platform-wallet: persist chain truncation (reorg) reported by key-wallet #5380) needs to arm the barrier there too.v5.1-dev'sreserve_payment_address(payments.rs) still wraps a flush failure asPlatformWalletError::Persistence(String). fix(platform-wallet)!: persist DashPay coreHeight backfill coverage so a relaunch resumes instead of rewinding again #5026'sPlatformWalletError::from_flush_failurewould keep its transient/constraint/fatal kind. Both map to the same FFI error, so only Rust callers would notice.Breaking Changes
The merge adds none of its own. It carries the already-reviewed breaking changes #5461, #5463 and #5026 into
v5.1-dev.Checklist:
structure.rs, regeneratedgrovedb-structure.json, and checked the structure viewer link posted on this pull requestFor repository code-owners and collaborators only
🤖 Generated with Claude Code
PR Hygiene ·
c89ff0b/skip-botsproceeds without the ones not yet reported/self-revieweddashmate— you own itsystem-contracts— you own itjs-wasm-sdk— you own itkotlin-sdk(packages/kotlin-sdk/sdk/schemas/org.dashfoundation.dashsdk.persistence.DashDatabase/15.json,packages/kotlin-sdk/sdk/src/androidTest/kotlin/org/dashfoundation/dashsdk/persistence/DashDatabaseMigrationTest.kt,packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/NativePersistenceBridge.ktand 6 more) — HashEngineeringdpp— you own itrs-drive-abci— you own itrs-drive— you own itrs-platform-wallet-ffi(packages/rs-platform-wallet-ffi/src/contact_persistence.rs,packages/rs-platform-wallet-ffi/src/core_wallet_types.rs,packages/rs-platform-wallet-ffi/src/manager.rsand 2 more) — HashEngineering or ZocoLini or llbartekll or romchornyiwallet-storage(packages/rs-platform-wallet-storage/src/sqlite/persister.rs,packages/rs-platform-wallet-storage/src/sqlite/schema/versions.rs) — lklimekrs-platform-wallet(packages/rs-platform-wallet/src/changeset/changeset.rs,packages/rs-platform-wallet/src/changeset/client_wallet_start_state.rs,packages/rs-platform-wallet/src/changeset/core_bridge.rsand 29 more) — HashEngineering or ZocoLini or llbartekll or romchornyiWhen every merge requirement is met, the
PR Hygienecheck passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.