Uh oh!
There was an error while loading. Please reload this page.
fix(drive-abci): mimic block time truncated to whole seconds - #4477
Merged
Conversation
The mimic execution converted milliseconds to protobuf Timestamp.nanos with a microsecond scale ((time_ms % 1000) * 1000). BlockProposal converts back via to_millis() (nanos / 1_000_000), so the sub-second part of every mimic-executed block time was silently dropped. Same fix as applied to process_proposal_collision_tests.rs in #4462. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change corrects millisecond-to-nanosecond conversion in ChangesProposal timestamp fix
Estimated code review effort: 1 (Trivial) | ~3 minutes Merge Risk:⚪ Minimal · up to This localized test-harness fix corrects sub-second block timestamps, with the affected strategy tests passing; no actionable merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Collaborator
ℹ️ Review superseded (commit 1e32dd9) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@## v4.2-dev #4477 +/- ##
============================================
+ Coverage 86.69% 86.78% +0.08%
============================================
Files 2735 2735 Lines 350211 350211 ============================================
+ Hits 303610 303921 +311 + Misses 46601 46290 -311
🚀 New features to boost your workflow:
|
Uh oh!
There was an error while loading. Please reload this page.
bfoss765 added a commit
that referenced
this pull request
Aug 25, 2026
Brings the branch up to date with upstream after #4457, #4465, #4399, #4467, #4257, #4382, #4423, #4463, #4377, #4440, #4472, #4477, #4470, and #4469 landed on v4.2-dev (base tip 1e26927). One conflict, in packages/kotlin-sdk/.../dashsdk/wallet/ManagedCoreWallet.kt: upstream #4377 inserts a new setGapLimit() immediately above broadcastTransaction(), while this branch rewrites that same broadcastTransaction() — expanding its KDoc to document the age-guard refusal and wrapping the body in mapNativeErrors { } so the native stale-broadcast error (code 34) surfaces typed. The two edits are additive and independent, so resolved as the union: setGapLimit() kept verbatim from upstream, broadcastTransaction() kept verbatim from this branch. Three more files overlapped but auto-merged, and were verified rather than assumed: - changeset/core_bridge.rs: this branch factors the input walk into spent_outpoint()/spent_outpoints() so the in-broadcast fence and the persister's spent-set cannot disagree about which inputs count; upstream #4257 replaces the synthetic ScriptBuf::default() with the input's real locking script. Orthogonal — #4257 changes the Utxo payload, the fence's filter predicate is unchanged. Both sides' tests pass, including #4257's two new script-reconstruction tests running through this branch's refactored walk. - manager/mod.rs: upstream adds the tracked_masternodes field and its initializer; this branch's SpendObservationHandler registration and its cfg(any(test, feature = "shielded")) widening are untouched. - rs-platform-wallet-ffi/src/error.rs: upstream adds ErrorMasternodeListUnavailable = 46; this branch maps PlatformWalletError::StaleReservation onto the existing shared code 34. No discriminant or name collides. Upstream's three new PlatformWalletPersistence methods all carry default bodies, so this branch's NoopTestPersister needs no change. Verified: the merged tree is identical to origin/v4.2-dev except in exactly the 18 files this branch owns, and this branch's net delta against the new base is unchanged at +3457/-103. cargo test -p platform-wallet --lib: 784 passed, 0 failed. cargo test -p platform-wallet-ffi --lib: 278 passed, 0 failed. cargo fmt --check and cargo clippy --all-targets -D warnings: clean on both crates.
bfoss765 added a commit
that referenced
this pull request
Aug 25, 2026
Brings the shielded-invite branch up to date with upstream v4.2-dev (#4470 active-protocol-version shielded fees, #4472 shield credits to an external Orchard recipient, #4477, #4469 swift async shutdown). One conflict, in rs-platform-wallet/src/wallet/shielded/operations.rs: both sides appended a #[cfg(test)] module at the same insertion point — this branch's foreign_claim_guard_tests (single-flight claim lifecycle guard, #4313 review finding 979bbc2fcb3c) and upstream #4472's shield_recipient_tests (resolve_shield_recipient classification). Resolved by keeping BOTH modules in full, this branch's first, each under its own #[cfg(test)]. No code from either side dropped or altered. The FFI error-code seam needed no hand-merge: upstream #4469's ErrorMasternodeListUnavailable = 46 was allocated explicitly around this branch's 43/44/45 shielded-invite trio. Verified: cargo check -p platform-wallet --features shielded and platform-wallet-ffi --all-features clean; cargo test platform-wallet --features shielded --lib = 984 passed / 1 failed — shield_input_selection_tests::regression_reports_max_from_usable_suffix _not_total_account_balance, proven PRE-EXISTING on unmerged origin/v4.2-dev (1e26927): upstream's versioned-fee change dropped shield_fee_reserve_credits(LATEST) below the test's seeded 297_264_780 leading balance; the unmerged PR head passes it. platform-wallet-ffi = 330 passed / 0 failed; rs-unified-sdk-jni = 37 passed / 0 failed; kotlin-sdk :sdk:test = 353 tests x debug+release, 0 failures. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
bfoss765 added a commit
that referenced
this pull request
Aug 25, 2026
…4470 active-version fees + #4472 shield-to-recipient) Reconciles the branch with the active-protocol-version fee estimation (#4470), the shield-to-external-Orchard-recipient operation (#4472), the block-time truncation fix (#4477) and the async wallet-manager shutdown (#4469). One textual conflict, packages/rs-platform-wallet-ffi/src/shielded_send.rs, plus one silent auto-merge hazard in the same file: - Duplicate guard helpers (auto-merged, NOT flagged by git). #4472 adopted this branch's panic-guard split verbatim, so the merge kept BOTH copies of panic_payload_message / catch_panic_to_code / SPEND_PANIC_GUIDANCE / catch_spend_panic. The bodies are byte identical; this branch's copy is a strict superset (it also carries IDENTITY_CREATE_PANIC_GUIDANCE, ASSET_LOCK_FUNDING_PANIC_GUIDANCE and SEED_POOL_PANIC_GUIDANCE for its non-spend guard call sites). Kept this branch's block, deleted upstream's duplicate, and widened the SPEND_PANIC_GUIDANCE doc to name shield-to-recipient among the operations it covers. - The shared guard tests. Upstream re-labelled the operation string in catch_spend_panic_maps_a_panic_to_the_unconfirmed_contract from "shielded multi-output transfer" to "shielded shield to recipient" and dropped the #4312 review-finding citation. Kept this branch's labels and citation (one test name, one definition); the guard the new export uses is exercised either way, and this branch's catch_panic_to_code_carries_the_per_operation_contract and max_recipients_matches_the_effective_action_ceiling tests survive. Everything else interleaved cleanly and was verified rather than assumed: this branch's four catch_pre_broadcast_panic sites (unshield / transfer / transfer_multi / withdraw) and catch_pre_broadcast_panic_async sit outside shield(), which is the only function #4472 rewrote in operations.rs (into shield + shield_to + resolve_shield_recipient), so both survive whole. No FFI export and no test was lost from either side: the merged shielded_send.rs gains exactly platform_wallet_manager_shielded_shield_to_recipient, and the only retired test is upstream's own rename of estimate_fee_matches_observed_onchain_values_for_2_actions into its protocol-13 / protocol-14 / manager-handle triple. No fee numbers needed recalibrating, and the output-aware predictor needed no change to adopt #4470's active-version sourcing: it is already version-parameterized end to end and every production call site feeds it sdk.version() -- the same network-tracked accessor #4470 switched the FFI estimator to. ShieldedFeeKind::compute takes &PlatformVersion (note_selection.rs:56); select_notes_with_fee and select_notes_for_denomination thread it through (:197-207, :288-311); reserve_unspent_notes and its denomination sibling pass sdk.version() (operations.rs:2329, :2362), as do shield's fee carve (:593) and every builder call (:647, :887, :1081, :1285, :1495, :1708, :1907). PlatformVersion::latest() survives only in #[cfg(test)] fixtures and in MAX_SHIELDED_TRANSFER_RECIPIENTS's ceiling assertion -- a structural action bound, not a fee, and version-invariant in any case (max_shielded_transition_actions = 16 and max_state_transition_size = 20480 in every system_limits version, so protocol 13 and 14 yield the same ceiling of 6). The shield_input_selection fixture survives because it derives: reserve() calls shield_fee_reserve_credits(LATEST_PLATFORM_VERSION) (platform_wallet.rs:2203) rather than pinning a literal, so it tracks any fee-constant movement automatically. #4470 did not touch reserves at all. Verified: cargo fmt --check and cargo clippy clean on platform-wallet, platform-wallet-ffi, dpp and rs-unified-sdk-jni. Tests: 942 passed platform-wallet (--features shielded --lib), including all four *_prover_panic_releases_the_note_reservation tests, all twelve shield_input_selection_tests and #4472's four shield_recipient_tests; 294 passed platform-wallet-ffi (--features shielded --lib), including #4470's estimate_fee protocol-13 / protocol-14 / manager-handle / unknown-handle tests; 241 passed dpp shielded (--all-features --lib shielded), including all three wire_cost_measured_tests. 0 failures. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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 freeto 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
mimic_execute_blockconverts a block'stime_msinto a protobufTimestampwith the wrong scale:nanos: ((time_ms % 1000) * 1000)produces microseconds, not nanoseconds.BlockProposal::try_fromconverts back viato_millis()(seconds * 1000 + nanos / 1_000_000), so the sub-second part of every mimic-executed test block time was silently truncated to whole seconds — e.g. strategy tests with 300ms block spacing executed several consecutive blocks with an identical timestamp.Same bug as the one fixed in
process_proposal_collision_tests.rson #4462 (review by @thepastaclaw), but in the shared mimic harness.What was done?
Fixed both occurrences in
packages/rs-drive-abci/src/mimic/mod.rs(prepare-proposal and process-proposal requests) tonanos: ((time_ms % 1000) * 1_000_000). Repo-wide grep confirms no other occurrence of the bad pattern remains.How Has This Been Tested?
cargo test -p drive-abci --test strategy_tests: 95 passed, 0 failed, 4 ignored (the ignored ones are the long-running upgrade-fork tests, permanently#[ignore]d).block_spacing_msin the suite: genesis time and all spacings except 300ms are whole-second multiples, so for those tests the produced timestamps are bit-identical before/after. The only behavior-affected tests are the fourrun_chain_core_height_randomly_increasing_with_quorum_updates*tests (300ms spacing); all four pass — no test relied on the truncation.Breaking Changes
None.
mimicis test-only tooling (behind themocksfeature); its sole consumer is the drive-abci strategy-test harness.Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit