Uh oh!
There was an error while loading. Please reload this page.
Add --rescan-from-height CLI argument - #196
Conversation
👋 Hi! This PR is now in draft status. |
Use the latest LDK Node main revision now that wallet recovery support has landed upstream. Keep ldk-server compiling across the dependency's API changes so the remaining branch commits can focus on startup flags. Co-Authored-By: HAL 9000
Expose a startup option for bitcoind-backed wallet recovery without silently accepting unsupported chain sources. Pass the height through to LDK Node only when the bitcoind RPC chain source is configured. Co-Authored-By: HAL 9000
Expose a startup option for Electrum and Esplora wallet recovery without accepting it for bitcoind RPC. Pass the request through LDK Node sync config so a full scan is forced until one succeeds. Co-Authored-By: HAL 9000
bc347e8 to
879e589Compare
benthecarman
left a comment
There was a problem hiding this comment.
I see you excluded these from the toml config, makes sense because its not really something you set persistently, just for a single run. Should we exclude the env variations for these too then? Env variables feel similar to me but maybe that's just my systemd preference coming out
| if self.force_wallet_full_scan { | ||
| return Err(io::Error::new( | ||
| io::ErrorKind::InvalidInput, | ||
| "`--force-wallet-full-scan` requires the Electrum or Esplora chain source.", |
There was a problem hiding this comment.
we should reference rescan-from-height in this error message
There was a problem hiding this comment.
Hmm, could you expand what you're thinking of there? Seems they are slightly orthogonal, so not sure why we need the cross-referencing?
There was a problem hiding this comment.
If someone is setting --force-wallet-full-scan with a bitcoind backend I presume they are trying to rescan for missing funds and we should direct them to --rescan-from-height which is the config for their backend
There was a problem hiding this comment.
But they do slightly different things? Isn't it even more confusing to point to something that works differently than the thing you're trying to do? Note we already do a full scan on first startup anyways, so differntly to --rescan-from-height it's not required to import an old wallet from seed necessarily.
There was a problem hiding this comment.
I think the "works differently" here is mostly just an implementation detail to user that isn't really relevant to the outcome they are looking for.
Don't feel too strongly though, so if you dont want to, that's fine
There was a problem hiding this comment.
I think the "works differently" here is mostly just an implementation detail to user that isn't really relevant to the outcome they are looking for.
Hmm, but the usecase is different to begin with? --rescan-from-height should be used once when you import from pre-existing seed, while --force-full-scan is more a utility that you only need if for some reason you discover there are still funds missing after the fact?
There was a problem hiding this comment.
They are both "I'm missing funds, rescan chain" but yeah, I don't feel too strongly either way
| if self.rescan_from_height.is_some() { | ||
| return Err(io::Error::new( | ||
| io::ErrorKind::InvalidInput, | ||
| "`--rescan-from-height` requires the bitcoind RPC chain source.", |
There was a problem hiding this comment.
we should reference --force-wallet-full-scan in this error message
| if self.rescan_from_height.is_some() { | ||
| return Err(io::Error::new( | ||
| io::ErrorKind::InvalidInput, | ||
| "`--rescan-from-height` requires the bitcoind RPC chain source.", |
Uh oh!
There was an error while loading. Please reload this page.
Based on lightningdevkit/ldk-node#884
We expose
--rescan-from-heightand--force-wallet-full-scanCLI arguments. First we bump the LDK Node dependency though, to include the changes from #884.