chore: bump rust-dashcore to dev head (#1000 merged: a known transaction is not announced new twice) - #4627
chore: bump rust-dashcore to dev head (#1000 merged: a known transaction is not announced new twice)#4627romchornyi wants to merge 1 commit into
Conversation
…ion is not announced new twice) Moves the pin from 93260bf3 (#998) to 7e315af5, one commit ahead and the current head of `dev`. The delta is dashpay/rust-dashcore#1000 alone, touching one file, `key-wallet/src/transaction_checking/wallet_checker.rs`. Upstream's `is_new` polled only `affected_accounts`, a set that is not stable across deliveries: an account that matched solely because the transaction spent its coin drops out on every later delivery, the coin having left `utxos` when the spend was recorded. Once the account holding the record stops matching, a transaction the wallet has held all along is announced as new a second time. It now asks every account, behind the existing relevance early-return. Upstream measured 13-14 double counts per full mainnet restore, a reported total of 6800 or 6801 over the same 6787 transactions; with the fix the total is 6787 on every run. Nothing in platform reads `is_new_transaction` outside two assertions in `test_support.rs`, so the surface here is the pin itself.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe workspace updates the pinned git revision for eight rust-dashcore dependencies. Dependency names, repository URLs, and other configuration values remain unchanged. ChangesRust Dashcore Revision Update
Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: ⚪ Minimal · up to This updates rust-dashcore to include the duplicate transaction-announcement fix while retaining consistent workspace and lockfile resolution. The change is ready to merge. 🚥 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 |
|
✅ Final review complete — no blockers (commit c1d5218) · triage: critical · Phase 2 only (queue backlog) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 2 only (queue backlog)
At the exact head, all eight workspace dependency pins and twelve lockfile entries consistently reference the new rust-dashcore revision, with no platform source changes. The cached upstream source confirms wallet-wide transaction detection behind the relevance guard and includes the stated regression test; no actionable in-scope findings were identified. Independent validation passed the diff check and built the targeted test suites, but test execution exceeded the timeout, so the reported 1,744 passing tests were not independently reconfirmed.
Source: reviewer 1: gpt-6-astra (agent: phase2-reviewer, role: general); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
Review provenance
- Triage:
criticalbygpt-6-astra(effort low) — Although limited to dependency pins, this changes wallet transaction classification in shared Core/SPV dependencies, where regressions could affect transaction notifications and sync accounting across funds-handling consumers. - Phase 1 reviewers: not run (skipped for throughput: 32 PRs queued, above the 10 limit)
- Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer
4 participants
Issue being fixed or feature implemented
The pin has been one commit behind
rust-dashcore'sdevsince #4560 landed, and thecommit it is missing is a sync-accounting fix worth having: dashpay/rust-dashcore#1000.
Upstream's
is_newpolled onlyaffected_accounts. That set is not stable acrossdeliveries — an account that matched solely because the transaction spent its coin drops
out of it on every later delivery, the coin having left
utxoswhen the spend wasrecorded. Once the account holding the record stops matching, the remaining candidates
have no record, and a transaction the wallet has held all along is announced as new a
second time. The counter is fed
new_txids, so the transaction total a sync reportsdisagreed between runs whose wallet state was identical.
Upstream measured 13-14 double counts per full mainnet restore — a reported total of 6800
or 6801 over the same 6787 transactions. With the fix the total is 6787 on every run.
What was done?
rust-dashcorepin from93260bf3(fix(key-wallet): promote confirmed spend-only transactions rust-dashcore#998) to7e315af5,the current head of
dev, across all eight crates in the workspaceCargo.toml:dashcore,dash-network-seeds,dash-spv,key-wallet,key-wallet-ffi,key-wallet-manager,dash-network,dashcore-rpc.Cargo.lock.The delta is dashpay/rust-dashcore#1000 alone, touching one file
(
key-wallet/src/transaction_checking/wallet_checker.rs):is_newnow asks every accountrather than only the matched ones, behind the existing relevance early-return, so it runs
only for transactions that already matched.
No platform code changes. Nothing here reads
is_new_transactionoutside two assertionsin
packages/rs-platform-wallet/src/test_support.rs, both of which cover a first deliveryand are unaffected by the change.
How Has This Been Tested?
cargo check -p platform-wallet -p platform-wallet-ffi -p platform-wallet-storage --tests— clean.cargo test -p platform-wallet -p platform-wallet-ffi -p platform-wallet-storage— 1744 passed, 0 failed.The upstream change ships with its own regression test
(
known_transaction_is_not_new_again_when_its_holder_stops_matching), which runs inrust-dashcore's suite rather than here.
Breaking Changes
None. The delta carries no API change — the modified function is internal to
wallet_checker, and its signature and return type are unchanged.Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit