test: prove platform PR #3376 breaks Send inference - #777
Conversation
Bump dash-sdk to platform rev 3286fcfc (which includes PR dashpay/platform#3376 "extract retry logic from trait default methods to fix HRTB Send issue") and rename Network::Dash → Network::Mainnet per upstream rust-dashcore. This does NOT compile — 4 Send bound errors in backend_task dispatch: error: implementation of `Send` is not general enough = note: `Send` would have to be implemented for the type `&DataContract`, for some specific lifetime `'0` The old platform rev (aa86b74f, currently on v1.0-dev) compiles clean without any workaround. PR #3376 paradoxically introduces the regression by extracting inline closure logic into standalone `async fn` helpers, adding opaque future boundaries that defeat HRTB Send inference (Rust issue #100013). See: dashpay/platform#3376 See: rust-lang/rust#100013 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReplaces usages of the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can enable review details to help with troubleshooting, context usage and more.Enable the |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/database/utxo.rs (1)
254-259:⚠️ Potential issue | 🔴 CriticalFix the inconsistent network string in the mainnet UTXO query.
The test inserts UTXOs with
Network::Mainnet(line 243), which stores network as"mainnet"via theinsert_utxoimplementation. However, line 256 queries with the hardcoded string"dash", causing a mismatch that makes the query return zero results. The assertion at line 258 will fail becausemainnet_utxoswill be empty.🔧 Suggested fix
// Query mainnet UTXOs let mainnet_utxos = db - .get_utxos_by_address(&mainnet_address.to_string(), "dash") + .get_utxos_by_address(&mainnet_address.to_string(), &Network::Mainnet.to_string()) .expect("Failed to get mainnet UTXOs");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/database/utxo.rs` around lines 254 - 259, The test queries mainnet UTXOs with the wrong network string; update the call to get_utxos_by_address so the network argument matches how insert_utxo stores Network::Mainnet (replace the hardcoded "dash" with "mainnet")—modify the invocation that produces mainnet_utxos (the get_utxos_by_address call using mainnet_address) so assertions on mainnet_utxos succeed.src/app.rs (1)
325-354:⚠️ Potential issue | 🔴 CriticalApp startup will crash for existing users with "Dash" stored in database—missing backward compatibility.
Old databases contain the string "Dash" (stored via
Network::Dash.to_string()), but the recent Network enum rename toNetwork::Mainnetremoved that variant. WhenNetwork::from_str("Dash")is called at line 586 insrc/database/settings.rs, it will fail and returnInvalidQuery, causing the error to propagate at line 221 insrc/app.rs. The app crashes before reaching the match statement at lines 325–354.No migration code exists to handle the string-to-enum conversion for old database values. Without either (1) updating
Network::from_str()to map "Dash" →Mainnet, or (2) adding migration logic to rewrite "Dash" entries to "Mainnet" in existing databases, existing users cannot launch the app.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app.rs` around lines 325 - 354, Existing databases still store the string "Dash" which causes Network::from_str(...) to fail; update Network::from_str to accept "Dash" (case-insensitive) and return Network::Mainnet when encountered, and additionally add a small migration step where settings loading (the code path that calls Network::from_str) detects the "Dash" raw string and rewrites/saves it as "Mainnet" before constructing the Network enum so old DBs are transparently upgraded; modify the parsing function Network::from_str and the settings load/save logic that calls it to implement these changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/database/settings.rs`:
- Around line 649-650: The inline comment above the assertion for network is
stale ("dash") and should reflect the renamed enum; update the comment near the
assertion that checks network and Network::Mainnet so it reads that the default
network is "mainnet" (or otherwise matches Network::Mainnet) to avoid
confusion—look for the comment immediately preceding the assert_eq!(network,
Network::Mainnet) and change its text accordingly.
In `@src/ui/tools/masternode_list_diff_screen.rs`:
- Line 148: The Testnet fallback currently constructs a Mainnet engine via
MasternodeListEngine::default_for_network(Network::Mainnet); change the fallback
to instantiate the correct Testnet engine by calling
MasternodeListEngine::default_for_network(Network::Testnet) when handling the
Testnet artifact-missing path so the masternode list/quorum logic uses Testnet
parameters instead of Mainnet.
---
Outside diff comments:
In `@src/app.rs`:
- Around line 325-354: Existing databases still store the string "Dash" which
causes Network::from_str(...) to fail; update Network::from_str to accept "Dash"
(case-insensitive) and return Network::Mainnet when encountered, and
additionally add a small migration step where settings loading (the code path
that calls Network::from_str) detects the "Dash" raw string and rewrites/saves
it as "Mainnet" before constructing the Network enum so old DBs are
transparently upgraded; modify the parsing function Network::from_str and the
settings load/save logic that calls it to implement these changes.
In `@src/database/utxo.rs`:
- Around line 254-259: The test queries mainnet UTXOs with the wrong network
string; update the call to get_utxos_by_address so the network argument matches
how insert_utxo stores Network::Mainnet (replace the hardcoded "dash" with
"mainnet")—modify the invocation that produces mainnet_utxos (the
get_utxos_by_address call using mainnet_address) so assertions on mainnet_utxos
succeed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 47f7b08f-62be-482f-812c-31d41cf1372e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (29)
Cargo.tomlsrc/app.rssrc/app_dir.rssrc/backend_task/core/mod.rssrc/backend_task/dashpay/hd_derivation.rssrc/backend_task/mnlist.rssrc/backend_task/platform_info.rssrc/components/core_p2p_handler.rssrc/config.rssrc/context/connection_status.rssrc/context/mod.rssrc/context/wallet_lifecycle.rssrc/database/contested_names.rssrc/database/settings.rssrc/database/utxo.rssrc/model/qualified_identity/mod.rssrc/model/qualified_identity/qualified_identity_public_key.rssrc/model/settings.rssrc/model/wallet/mod.rssrc/spv/manager.rssrc/ui/components/left_panel.rssrc/ui/contracts_documents/contracts_documents_screen.rssrc/ui/identities/transfer_screen.rssrc/ui/identities/withdraw_screen.rssrc/ui/network_chooser_screen.rssrc/ui/theme.rssrc/ui/tokens/tokens_screen/mod.rssrc/ui/tools/masternode_list_diff_screen.rssrc/ui/wallets/wallets_screen/dialogs.rs
| // Default network is "dash" (mainnet) | ||
| assert_eq!(network, Network::Dash); | ||
| assert_eq!(network, Network::Mainnet); |
There was a problem hiding this comment.
Update the stale inline comment to match the renamed enum.
Line 649 still says "dash" while Line 650 asserts Network::Mainnet. Please align the comment text to avoid confusion.
Suggested patch
- // Default network is "dash" (mainnet)
+ // Default network is Mainnet
assert_eq!(network, Network::Mainnet);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Default network is "dash" (mainnet) | |
| assert_eq!(network, Network::Dash); | |
| assert_eq!(network, Network::Mainnet); | |
| // Default network is Mainnet | |
| assert_eq!(network, Network::Mainnet); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/database/settings.rs` around lines 649 - 650, The inline comment above
the assertion for network is stale ("dash") and should reflect the renamed enum;
update the comment near the assertion that checks network and Network::Mainnet
so it reads that the default network is "mainnet" (or otherwise matches
Network::Mainnet) to avoid confusion—look for the comment immediately preceding
the assert_eq!(network, Network::Mainnet) and change its text accordingly.
| } else { | ||
| tracing::warn!("MNListDiff file not found: {}", file_path); | ||
| MasternodeListEngine::default_for_network(Network::Dash) | ||
| MasternodeListEngine::default_for_network(Network::Mainnet) |
There was a problem hiding this comment.
Use Testnet engine in the Testnet fallback path.
When the Testnet artifact file is missing, this currently initializes a Mainnet engine, which can produce incorrect MN list/quorum behavior on Testnet.
Suggested fix
- MasternodeListEngine::default_for_network(Network::Mainnet)
+ MasternodeListEngine::default_for_network(Network::Testnet)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| MasternodeListEngine::default_for_network(Network::Mainnet) | |
| MasternodeListEngine::default_for_network(Network::Testnet) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ui/tools/masternode_list_diff_screen.rs` at line 148, The Testnet
fallback currently constructs a Mainnet engine via
MasternodeListEngine::default_for_network(Network::Mainnet); change the fallback
to instantiate the correct Testnet engine by calling
MasternodeListEngine::default_for_network(Network::Testnet) when handling the
Testnet artifact-missing path so the masternode list/quorum logic uses Testnet
parameters instead of Mainnet.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The mechanical Network::Dash → Network::Mainnet rename is thorough across .rs files and the PR achieves its stated goal of proving platform PR #3376 introduces Send bound regressions. One doc file was missed in the rename, and one pre-existing bug (Testnet fallback using Mainnet default) was carried forward unchanged. The database serialization gap (DEFAULT_NETWORK still 'dash') and hardcoded test strings are valid observations for the companion migration PR, not blocking for this intentionally non-compiling proof.
Reviewed commit: e55f89f
🟡 3 suggestion(s) | 💬 2 nitpick(s)
4 additional findings
🟡 suggestion: Hardcoded 'dash' string in test will break after rename
src/database/utxo.rs (line 256)
The test inserts a UTXO with Network::Mainnet (line 243, which now serializes to "mainnet") but queries with the hardcoded string "dash" (line 256). On v1.0-dev this worked because Network::Dash.to_string() returned "dash". Observation for the companion PR — this test will fail once the rename is real.
💡 Suggested change
.get_utxos_by_address(&mainnet_address.to_string(), "mainnet")
🟡 suggestion: DEFAULT_NETWORK still 'dash' — needs migration in companion PR
src/database/initialization.rs (lines 7-9)
DEFAULT_NETWORK is "dash" and DEFAULT_DB_VERSION is still 28. If Network::Mainnet.to_string() returns "mainnet" in the new SDK, all 17 tables storing network as text will have stale "dash" values, and Network::from_str("dash") may fail. This is not blocking for a proof-of-concept that intentionally doesn't compile, but the real migration PR will need a version 29 migration to UPDATE all network columns from 'dash' to 'mainnet'.
💬 nitpick: Stale Network::Dash reference in documentation
docs/local-network.md (line 100)
The comparison table still reads Network::Dash in the Mainnet column. This was missed during the mechanical rename.
💡 Suggested change
| Network enum | `Network::Regtest` | `Network::Testnet` / `Network::Devnet` | `Network::Mainnet` |
💬 nitpick: Removed NetworkExt import — verify upstream removed the trait
src/ui/tools/masternode_list_diff_screen.rs (line 17)
The v1.0-dev version had use dash_sdk::dpp::dashcore::network::constants::NetworkExt; at line 20 (old numbering). This import was removed with no replacement. Worth confirming the upstream dashcore rev at 3286fcfc actually removed or renamed this trait, so it doesn't silently drop functionality.
🤖 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 `src/ui/tools/masternode_list_diff_screen.rs`:
- [SUGGESTION] line 148: Pre-existing bug: Testnet fallback creates Mainnet engine
In the `Network::Testnet` match arm (line 125), when the MNListDiff file is not found (else branch at line 146), the fallback creates `MasternodeListEngine::default_for_network(Network::Mainnet)`. This should be `Network::Testnet`. On v1.0-dev this was `Network::Dash` (equally wrong). The mechanical rename carried the bug forward. Not introduced by this PR, but now more visible thanks to the rename making Mainnet/Testnet semantically distinct.
In `src/database/utxo.rs`:
- [SUGGESTION] line 256: Hardcoded 'dash' string in test will break after rename
The test inserts a UTXO with `Network::Mainnet` (line 243, which now serializes to `"mainnet"`) but queries with the hardcoded string `"dash"` (line 256). On v1.0-dev this worked because `Network::Dash.to_string()` returned `"dash"`. Observation for the companion PR — this test will fail once the rename is real.
In `src/database/initialization.rs`:
- [SUGGESTION] lines 7-9: DEFAULT_NETWORK still 'dash' — needs migration in companion PR
DEFAULT_NETWORK is `"dash"` and DEFAULT_DB_VERSION is still 28. If `Network::Mainnet.to_string()` returns `"mainnet"` in the new SDK, all 17 tables storing network as text will have stale `"dash"` values, and `Network::from_str("dash")` may fail. This is not blocking for a proof-of-concept that intentionally doesn't compile, but the real migration PR will need a version 29 migration to UPDATE all network columns from `'dash'` to `'mainnet'`.
| } else { | ||
| tracing::warn!("MNListDiff file not found: {}", file_path); | ||
| MasternodeListEngine::default_for_network(Network::Dash) | ||
| MasternodeListEngine::default_for_network(Network::Mainnet) |
There was a problem hiding this comment.
🟡 Suggestion: Pre-existing bug: Testnet fallback creates Mainnet engine
In the Network::Testnet match arm (line 125), when the MNListDiff file is not found (else branch at line 146), the fallback creates MasternodeListEngine::default_for_network(Network::Mainnet). This should be Network::Testnet. On v1.0-dev this was Network::Dash (equally wrong). The mechanical rename carried the bug forward. Not introduced by this PR, but now more visible thanks to the rename making Mainnet/Testnet semantically distinct.
💡 Suggested change
| MasternodeListEngine::default_for_network(Network::Mainnet) | |
| MasternodeListEngine::default_for_network(Network::Testnet) |
source: ['claude']
🤖 Fix this 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 `src/ui/tools/masternode_list_diff_screen.rs`:
- [SUGGESTION] line 148: Pre-existing bug: Testnet fallback creates Mainnet engine
In the `Network::Testnet` match arm (line 125), when the MNListDiff file is not found (else branch at line 146), the fallback creates `MasternodeListEngine::default_for_network(Network::Mainnet)`. This should be `Network::Testnet`. On v1.0-dev this was `Network::Dash` (equally wrong). The mechanical rename carried the bug forward. Not introduced by this PR, but now more visible thanks to the rename making Mainnet/Testnet semantically distinct.
Review GateCommit:
|
3 participants
Summary
dash-sdkto platform rev3286fcfc(contains dashpay/platform#3376)Network::Dash→Network::Mainnetper upstream rust-dashcoreThis PR intentionally does NOT compile. It exists as proof that PR #3376 ("extract retry logic from trait default methods to fix HRTB Send issue") paradoxically introduces 4 Send bound errors in evo-tool's backend task dispatch:
What changed upstream
PR #3376 extracted inline retry+closure logic from
#[async_trait]trait default methods (fetch_with_metadata_and_proof,fetch_many_with_metadata_and_proof) into standaloneasync fnhelpers. This adds an opaque future boundary that the compiler cannot see through, defeating HRTB Send inference (rust-lang/rust#100013).Proof
aa86b74f, currentv1.0-dev): compiles clean, no workaround needed3286fcfc, this PR): 4 Send errors, does not compileWorkaround
unsafe { transmute }to assert Send on the pinned boxed future — implemented separately in thefeat/mempool-supportbranch.See: dashpay/platform#3376
See: rust-lang/rust#100013
🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit