-
Notifications
You must be signed in to change notification settings - Fork 159
Derive shutdown scripts without blocking on wallet persistence #1011
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
base: main
Are you sure you want to change the base?
Changes from 5 commits
273e4d0
1b27621
338dc8f
6ed64e8
075a40c
97df7cf
77164dc
5fd08ef
a7c52af
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 |
|---|---|---|
|
|
@@ -27,8 +27,8 @@ use lightning_transaction_sync::ElectrumSyncClient; | |
|
|
||
| use super::WalletSyncStatus; | ||
| use crate::config::{ | ||
| clamp_full_scan_stop_gap, Config, ElectrumSyncConfig, MAX_FULL_SCAN_STOP_GAP, | ||
| MIN_FULL_SCAN_STOP_GAP, | ||
| clamp_full_scan_stop_gap, Config, ElectrumSyncConfig, ADDRESS_POOL_SIZE, | ||
| MAX_FULL_SCAN_STOP_GAP, MIN_FULL_SCAN_STOP_GAP, | ||
| }; | ||
| use crate::error::Error; | ||
| use crate::fee_estimator::{ | ||
|
|
@@ -598,7 +598,10 @@ impl ElectrumRuntimeClient { | |
| bounded | ||
| ); | ||
| } | ||
| bounded as usize | ||
| // Extend the gap by the address pool size: the pool keeps that many addresses standing | ||
| // revealed-but-unused, which a scan restoring the wallet from seed alone would otherwise | ||
| // count against the configured gap. | ||
| (bounded as usize).saturating_add(ADDRESS_POOL_SIZE as usize) | ||
|
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. I think with the FIFO design this (and below) is no longer needed. If we'd want to account for the address pool size it should just be considered the minimum in
Contributor
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. 🤖 Agreed, removed. Handouts consume the pool oldest-first, so pooled addresses sit past the handed-out ones, where they can't make a full scan stop short of funds. One qualifier: a handout that fails while a concurrent one proceeds can briefly leave a pooled address below a handed-out one. The inversion is bounded by the pool size and consumed by the next handouts, so the default gap of 20 covers it — but a configured gap below the pool size could stop short during that window. Happy to make the pool size the minimum in Chasing this down also turned up a bug, fixed in a separate fixup: the refill flushed its reveals even when the pool-record write failed, permanently skipping those indices and widening what a restore from seed must scan across. One narrow residual remains: reveals retained after a failed record write can still be flushed by a later unrelated persist, and after a restart they stay revealed-but-unused. Re-pooling them at load turns out unsound — a full scan (or an older version of the software) can also reveal indices past the record, and re-pooling those would reuse handed-out or even used addresses — so I left the residual in place: it's bounded by the pool size and needs a compound failure. Could revisit with a tx-graph check if you think it's worth closing. |
||
| } | ||
|
|
||
| async fn get_incremental_sync_wallet_update( | ||
|
|
||
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.
We need to parallelize the reads on startup.
IMO it would also preferable to keep up the pre-existing patterns of doing the
read_methods here in parallel, and then hand that in to initialize via anAddressPool::newor so rather than doing all/obfuscating in aninitialize_method.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.
🤖 Done. The record read now runs in the
tokio::join!with the other startup reads, its result is handed toWallet::new, which validates it inAddressPool::new, and the builder only blocks on the initial pool top-up. One side effect: a failed read now fails the build withReadFailedrather thanWalletSetupFailed.