Repository navigation
feat(platform-wallet): snapshot readers + unified V002 schema migration #3986
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 17 commits
50cab4c
61c292c
c9603d6
e663a35
503b251
f06f9ff
00499e0
d514d01
c44d684
1d2612e
a7c1aa6
6dd08d4
ab12991
4f95f46
e3eb645
a511bde
0614fc4
d22d5c5
aa11d17
c0f17da
c159f34
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,75 @@ | ||
| //! Unified additive migration for `platform-wallet-storage` (#3968). | ||
| //! | ||
| //! Additive-only: V001 stays byte-identical so refinery's applied-migration | ||
| //! checksum for version 1 never diverges on an existing store. V002 lifts | ||
| //! `max_supported_version()` from 1 to 2 automatically (the value is derived | ||
| //! from the embedded list) and lands three concerns in one migration event: | ||
| //! | ||
| //! - `core_address_pool` — per-index address-pool rows with a `used` flag, | ||
| //! the first-class row store that replaces `core_utxos` script-derivation | ||
| //! for the address-reuse guard. `pool_type` is in the primary key so an | ||
| //! External (receive) and Internal (change) pool never collide at the same | ||
| //! `address_index`. `script` (the address' `script_pubkey`) is stored so | ||
| //! the reader returns used addresses verbatim and the UTXO writer can | ||
| //! attribute an outpoint to its owning account, both without re-deriving. | ||
| //! - `meta_data_versions` — per-`(wallet_id, domain)` monotonic `seq` | ||
| //! bumped inside the flush transaction, the cache-invalidation keystone. | ||
| //! No FK (a domain row may be written before its typed parent syncs, | ||
| //! mirroring the `meta_*` tables); a soft-cascade trigger reaps rows on | ||
| //! wallet delete. | ||
| //! - `meta_store_generation` — a single-row store-generation token, | ||
| //! initialized with `randomblob(16)` so the rendered SQL stays deterministic (the | ||
| //! content fingerprint pins the text, the runtime value is unique per | ||
| //! store). Regenerated on restore. | ||
| //! | ||
| //! No MAC column ships here — manifest authentication is deferred out of | ||
| //! this workstream (dev-plan §7). | ||
|
|
||
| pub fn migration() -> String { | ||
| "\ | ||
| CREATE TABLE core_address_pool ( | ||
| wallet_id BLOB NOT NULL, | ||
| account_index INTEGER NOT NULL, | ||
| key_class INTEGER NOT NULL DEFAULT 0, | ||
| pool_type INTEGER NOT NULL CHECK (pool_type IN (0, 1, 2, 3)), | ||
| address_index INTEGER NOT NULL, | ||
| script BLOB NOT NULL, | ||
| used INTEGER NOT NULL DEFAULT 0 CHECK (used IN (0, 1)), | ||
| PRIMARY KEY (wallet_id, account_index, key_class, pool_type, address_index), | ||
| FOREIGN KEY (wallet_id) REFERENCES wallets(wallet_id) ON DELETE CASCADE | ||
| ); | ||
|
Comment on lines
+34
to
+45
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Suggestion: core_address_pool PK omits DashPay (user_identity_id, friend_identity_id) — schema forethought while the migration is still landing The widened PK (wallet_id, account_type, account_index, key_class, pool_type, address_index) correctly closes the account_type-variant collisions the prior review flagged, verified by distinct_account_types_sharing_index_zero_do_not_collide and standard_and_coinjoin_index_zero_do_not_collide in sqlite_core_pool_writer.rs. However, account_type_db_label collapses every DashpayReceivingFunds{index, user_identity_id, friend_identity_id} to the fixed string "dashpay_receiving" (accounts.rs:265) and account_index() returns just the inner index field (accounts.rs:288). send_contact_request (contact_requests.rs:162) hardcodes account_index = 0 for every DashPay contact, so two contacts on the same wallet would collapse onto the identical PK tuple (wallet_id, 'dashpay_receiving', 0, 0, pool_type, address_index) and silently overwrite each other via ON CONFLICT ... DO UPDATE SET script = excluded.script (core_pool.rs:36-38). Mitigation for THIS PR: verified in-tree that register_contact_account (contacts.rs:100-156) only inserts the managed account into in-memory info.core_wallet.accounts and never emits an AccountAddressPoolEntry to the persister — DashPay pool rows are not currently written to core_address_pool in production, so no live collision. The sibling account_registrations table (V001) does carry (user_identity_id, friend_identity_id) in its PK for exactly this reason, with a passing distinct_dashpay_friends_do_not_collide test, and account_dashpay_ids() (accounts.rs:309) already exposes the pair. Since V002 is additive-only, widening the PK later requires another migration. If DashPay pool persistence is on the near-term roadmap for this workstream, mirror account_registrations' PK now (add user_identity_id/friend_identity_id columns, thread account_dashpay_ids through apply_pools/UPSERT_POOL_SQL/account_index_for_script, add a regression test with two DashpayReceivingFunds entries sharing (account_type, account_index, key_class) but differing only by contact identity). Otherwise, add a brief note to the V002 migration doc-comment explicitly deferring the DashPay identity axis (mirroring the manifest-MAC deferral note) and track it as a follow-up. source: ['claude'] |
||
|
|
||
| CREATE INDEX idx_core_address_pool_used | ||
| ON core_address_pool(wallet_id, used); | ||
|
|
||
| -- The UTXO writer attributes an outpoint to its owning account by matching | ||
| -- the outpoint's script against a pool row. | ||
| CREATE INDEX idx_core_address_pool_script | ||
| ON core_address_pool(wallet_id, script); | ||
|
|
||
| CREATE TABLE meta_data_versions ( | ||
| wallet_id BLOB NOT NULL, | ||
| domain TEXT NOT NULL, | ||
| seq INTEGER NOT NULL DEFAULT 0, | ||
| PRIMARY KEY (wallet_id, domain) | ||
| ); | ||
|
|
||
| -- Soft-cascade reap, matching the meta_* tables: no FK (a domain may be | ||
| -- bumped before its typed parent exists), so a trigger clears rows when | ||
| -- the owning wallet is deleted. | ||
| CREATE TRIGGER cascade_meta_data_versions_on_wallet_delete | ||
| AFTER DELETE ON wallets | ||
| FOR EACH ROW | ||
| BEGIN | ||
| DELETE FROM meta_data_versions WHERE wallet_id = OLD.wallet_id; | ||
| END; | ||
|
|
||
| CREATE TABLE meta_store_generation ( | ||
| id INTEGER NOT NULL PRIMARY KEY CHECK (id = 0), | ||
| generation BLOB NOT NULL | ||
| ); | ||
|
|
||
| INSERT INTO meta_store_generation (id, generation) VALUES (0, randomblob(16)); | ||
| " | ||
| .to_string() | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -274,6 +274,17 @@ pub fn restore_from(dest_db_path: &Path, src_backup: &Path) -> Result<(), Wallet | |
|
|
||
| // 10. Re-tighten perms (idempotent; SQLite may re-materialise -wal/-shm). | ||
| apply_secure_permissions(dest_db_path)?; | ||
|
|
||
| // 11. Regenerate the store-generation token so the restored copy is | ||
| // distinguishable from its source — a client cache keyed on the old | ||
| // generation misses rather than serving stale entries. A pre-V002 | ||
| // backup has no generation table; it is (re)seeded on its later | ||
| // migration to V002. | ||
| { | ||
| let conn = | ||
| crate::sqlite::conn::open_conn(dest_db_path, crate::sqlite::conn::Access::ReadWrite)?; | ||
| crate::sqlite::schema::versions::regenerate_generation(&conn)?; | ||
| } | ||
| Ok(()) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Suggestion: restore_from rotates the store-generation token AFTER the atomic swap — two coupled hazards Step 7 (
Both hazards close by folding the regeneration into the staged temp file before step 7, so the atomic rename is the single commit point that swaps in both restored bytes and a rotated token together. Alternatively, perform the rotation on the still-held EXCLUSIVE-lock connection before dropping it, and follow with source: ['claude']
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed — steps 8-11 (WAL cleanup, fsync, perms, generation regen) run after the step-7 atomic commit, outside the lock, so an 🤖 Co-authored by Claudius the Magnificent AI Agent
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correction: my automated reconciler posted an erroneous "resolved" reply here while processing the 0614fc4 review. This finding is still valid at 0614fc4 and was carried forward in the current review. The misleading resolved text came from using the live PR diff after the branch had already advanced to a newer commit. Please ignore the earlier resolved wording in this reply thread. |
||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,140 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| //! Writer + account-attribution helper for the `core_address_pool` table. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| //! | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| //! Per-index address-pool rows carrying a `used` flag, scoped by | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| //! `(wallet_id, account_index, key_class, pool_type, address_index)`. The | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| //! first-class row store the reader consumes verbatim — no `core_utxos` | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| //! script-derivation, no horizon-walk re-derivation. Populated from the | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| //! `account_address_pools` changeset snapshots; the UTXO writer reads it | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| //! back to attribute an outpoint to its owning account. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use rusqlite::{params, OptionalExtension, Transaction}; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use platform_wallet::changeset::AccountAddressPoolEntry; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use platform_wallet::wallet::platform_wallet::WalletId; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use key_wallet::managed_account::address_pool::AddressPoolType; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use crate::sqlite::error::WalletStorageError; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use crate::sqlite::schema::accounts; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// Stored `pool_type` discriminant. Kept in the primary key so an External | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// and an Internal pool never collide at the same `address_index`. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pub(crate) fn pool_type_to_i64(pool_type: AddressPoolType) -> i64 { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| match pool_type { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| AddressPoolType::External => 0, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| AddressPoolType::Internal => 1, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| AddressPoolType::Absent => 2, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| AddressPoolType::AbsentHardened => 3, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const UPSERT_POOL_SQL: &str = "INSERT INTO core_address_pool \ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| (wallet_id, account_index, key_class, pool_type, address_index, script, used) \ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7) \ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ON CONFLICT(wallet_id, account_index, key_class, pool_type, address_index) DO UPDATE SET \ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| script = excluded.script, \ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| used = MAX(used, excluded.used)"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// Expand `account_address_pools` snapshots into per-index | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// `core_address_pool` rows. Idempotent: `script` is derivation-stable and | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// `used` is monotonic (`MAX`), so re-applying the same snapshot is a no-op | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// and a used address can never revert to unused (the reuse-guard invariant). | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pub fn apply_pools( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| tx: &Transaction<'_>, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| wallet_id: &WalletId, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pools: &[AccountAddressPoolEntry], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) -> Result<(), WalletStorageError> { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if pools.is_empty() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return Ok(()); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let mut stmt = tx.prepare_cached(UPSERT_POOL_SQL)?; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for entry in pools { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let account_index = i64::from(accounts::account_index(&entry.account_type)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // TODO(key_class): PlatformPayment carries a real key_class; every | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // other account maps to the 0 sentinel until the pool snapshot | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // threads a per-pool key class. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let key_class = i64::from(accounts::account_key_class(&entry.account_type)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let pool_type = pool_type_to_i64(entry.pool_type); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for info in &entry.addresses { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| stmt.execute(params![ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| wallet_id.as_slice(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| account_index, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| key_class, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pool_type, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| i64::from(info.index), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| info.script_pubkey.as_bytes(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| info.used, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ])?; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Ok(()) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// Owning account index for a UTXO, matched by its `script_pubkey` against a | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// pool row. `None` when no pool row covers the script — the UTXO writer | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// then falls back to account 0 (the one-way historical-attribution default, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// R7): funds are never dropped, only conservatively bucketed. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pub fn account_index_for_script( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| tx: &Transaction<'_>, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| wallet_id: &WalletId, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| script: &[u8], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) -> Result<Option<u32>, WalletStorageError> { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let idx: Option<i64> = tx | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .prepare_cached( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "SELECT account_index FROM core_address_pool \ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| WHERE wallet_id = ?1 AND script = ?2 LIMIT 1", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| )? | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .query_row(params![wallet_id.as_slice(), script], |row| row.get(0)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .optional()?; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+94
to
+102
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Suggestion: account_index_for_script uses LIMIT 1 with no ORDER BY — non-deterministic on any script collision
Make the tie-break explicit — e.g.
Suggested change
source: ['claude']
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed real, and it gets more likely once finding #1 (PK missing account_type) is fixed — widening the PK increases the chance of two rows legitimately sharing a script across accounts (e.g. address reuse across account families). Fix in the same pass as #1. Not yet fixed. 🤖 Co-authored by Claudius the Magnificent AI Agent
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correction: my automated reconciler posted an erroneous "resolved" reply here while processing the 0614fc4 review. This finding is still valid at 0614fc4 and was carried forward in the current review. The misleading resolved text came from using the live PR diff after the branch had already advanced to a newer commit. Please ignore the earlier resolved wording in this reply thread. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| idx.map(|v| crate::sqlite::util::safe_cast::i64_to_u32("core_address_pool.account_index", v)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .transpose() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// Used addresses for a wallet, read verbatim from `core_address_pool` | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// (`used = 1`) with no re-derivation. Possibly empty. The caller **unions** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// this with the `core_utxos`-derived set — the reuse guard is monotonic, so | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// a mixed store (historical UTXOs a later partial pool snapshot never | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// enumerates) must surface both sources, never drop the historical ones. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// `network` turns each stored `script` back into an [`Address`]; a script | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// that isn't a valid address is a hard error — corruption is never silently | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// dropped, matching [`crate::sqlite::schema::core_state::load_used_addresses`]. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pub fn load_used_addresses( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| conn: &rusqlite::Connection, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| wallet_id: &WalletId, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| network: dashcore::Network, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) -> Result<Vec<dashcore::Address>, WalletStorageError> { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let mut stmt = conn.prepare( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "SELECT DISTINCT script FROM core_address_pool \ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| WHERE wallet_id = ?1 AND used = 1 ORDER BY script", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| )?; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let rows = stmt.query_map(params![wallet_id.as_slice()], |row| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| row.get::<_, Vec<u8>>(0) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| })?; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let mut out = Vec::new(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for r in rows { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let script = dashcore::ScriptBuf::from_bytes(r?); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let address = dashcore::Address::from_script(&script, network).map_err(|_| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| WalletStorageError::blob_decode("core_address_pool.script not an address") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| })?; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| out.push(address); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Ok(out) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+134
to
+150
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Suggestion: load_used_addresses skips the pre-read length() gate used elsewhere in this PR Every other reader touching a variable-width BLOB in this crate ( The connection-wide
Suggested change
source: ['claude']
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed real — and one correction to the comment's own citation: 🤖 Co-authored by Claudius the Magnificent AI Agent
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correction: my automated reconciler posted an erroneous "resolved" reply here while processing the 0614fc4 review. This finding is still valid at 0614fc4 and was carried forward in the current review. The misleading resolved text came from using the live PR diff after the branch had already advanced to a newer commit. Please ignore the earlier resolved wording in this reply thread. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #[cfg(test)] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| mod tests { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use super::*; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #[test] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fn pool_type_discriminants_are_stable_and_distinct() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let all = [ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| AddressPoolType::External, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| AddressPoolType::Internal, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| AddressPoolType::Absent, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| AddressPoolType::AbsentHardened, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ]; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let mapped: Vec<i64> = all.iter().copied().map(pool_type_to_i64).collect(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| assert_eq!(mapped, vec![0, 1, 2, 3]); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 Blocking: core_address_pool PK omits account_type — silent cross-account row collisions in the common case
The
core_address_poolPK is(wallet_id, account_index, key_class, pool_type, address_index). Verified againstaccounts.rs:273-303:account_key_class()returns the real key class only forPlatformPayment— every other variant maps to the sentinel0.account_index()collapses at least nine distinct variants (IdentityRegistration,IdentityTopUpNotBoundToIdentity,IdentityInvitation,AssetLockAddressTopUp,AssetLockShieldedAddressTopUp, all fourProvider*Keys) to0.So the following distinct accounts, all created concurrently in a normal wallet, upsert onto the same PK tuple in
core_address_pool:Standard{index:0, BIP44}andCoinJoin{index:0}collide on(wallet_id, 0, 0, {0,1}, address_index)(both derive External/Internal pools).IdentityRegistration,IdentityInvitation,ProviderVotingKeys,ProviderOwnerKeys,ProviderOperatorKeys, etc. — allAddressPoolType::Absentataddress_index=0— collide on(wallet_id, 0, 0, 2, 0).UPSERT_POOL_SQLatcore_pool.rs:31-36(ON CONFLICT ... DO UPDATE SET script = excluded.script, used = MAX(used, excluded.used)) silently overwrites the earlier account'sscriptwith the last one applied and mergesusedflags from unrelated addresses. This breaks both the "verbatim snapshot" guarantee this PR advertises and the address-reuse guard (ausedbit can bleed onto an unrelated address once a colliding pool row's script wins the last-writer race).This is a direct regression relative to
account_registrations, whose PK explicitly widens withaccount_typeTEXT specifically to disambiguate these variants (see theapply_registrationsinsert ataccounts.rs:122-127and the comment at 117-120: "distinct accounts that share(account_type, account_index)don't overwrite each other"). The pattern for correct disambiguation was already in the codebase but wasn't applied to the new table.Test gap: none of the new tests (
sqlite_core_pool_writer.rs,sqlite_pool_reader.rs,sqlite_v002_isolation.rs,sqlite_version_bump.rs) exercise more than one account type sharing(account_index, key_class)in the same wallet — onlyStandardandPlatformPaymentare used, and never together — so the collision is invisible to CI.Fix: widen the PK with an
account_typediscriminator (mirroringaccount_registrations), and add a regression test that flushes two different AccountType variants sharing(0, 0)(e.g.IdentityRegistration+ProviderVotingKeys, andStandard{0}+CoinJoin{0}) into the same wallet and asserts both survive with correct scripts andusedflags.source: ['claude']
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed and agreed — blocking. Verified
account_index/account_key_classcollapse ≥9AccountTypevariants to(0, 0), andUPSERT_POOL_SQL'sON CONFLICTgenuinely overwritesscript/mergesusedacross unrelated accounts. Also checked: this does NOT recur elsewhere in the schema —account_registrationsalready disambiguates correctly viaaccount_typein its PK;core_utxos/asset_locks/platform_addresseskey on real UTXO/address identity withaccount_indexas attribute-only. Isolated to this one new table. Needs a fix before merge — not yet fixed.🤖 Co-authored by Claudius the Magnificent AI Agent
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correction: my automated reconciler posted an erroneous "resolved" reply here while processing the 0614fc4 review. This finding is still valid at 0614fc4 and was carried forward in the current review.
The misleading resolved text came from using the live PR diff after the branch had already advanced to a newer commit. Please ignore the earlier resolved wording in this reply thread.