Allow retrieval of SpendableOutputDescriptors from relevant transactions #2609
Conversation
Codecov ReportAll modified lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2609 +/- ##
==========================================
+ Coverage 88.84% 88.97% +0.13%
==========================================
Files 113 112 -1
Lines 84729 86261 +1532
Branches 84729 86261 +1532
==========================================
+ Hits 75275 76749 +1474
- Misses 7240 7269 +29
- Partials 2214 2243 +29
☔ View full report in Codecov by Sentry. |
wpaulino
force-pushed
the
monitor-get-spendable-output
branch
from
7a19b6d to
67923b0
Compare
wpaulino
force-pushed
the
monitor-get-spendable-output
branch
from
67923b0 to
ff510c3
Compare
wpaulino
force-pushed
the
monitor-get-spendable-output
branch
from
ff510c3 to
29b714c
Compare
wpaulino
force-pushed
the
monitor-get-spendable-output
branch
from
29b714c to
130abf8
Compare
wpaulino
force-pushed
the
monitor-get-spendable-output
branch
2 times, most recently
from
d09d457 to
7e10b94
Compare
Currently, our API will only expose `SpendableOutputDescriptor`s once after they are no longer under reorg risk (see `ANTI_REORG_DELAY`). Users have often requested they'd like the ability to retrieve these in some other way, either for historical purposes, or to handle replaying any in the event of a failure.
Assuming our keys haven't been compromised, and that random transactions aren't learning of these scripts somehow and sending funds to them, it was only possible for one spendable output to exist within a transaction. - `shutdown_script` can only exist in co-op close transactions. - `counterparty_payment_script` can only exist in counterparty commitment transactions. - `broadcasted_holder_revokable_script` can only exist in holder commitment/HTLC transactions. - `destination_script` can exist in any other type of claim we support. Now that we're exposing this API to users such that they can rescan any relevant transactions, there's no harm in allowing them to claim more funds from spendable outputs than we expected.
wpaulino
force-pushed
the
monitor-get-spendable-output
branch
from
7e10b94 to
ffec24b
Compare
| /// descriptors at a later time, either for historical purposes, or to replay any | ||
| /// missed/unhandled descriptors. For the purpose of gathering historical records, if the | ||
| /// channel close has fully resolved (i.e., [`ChannelMonitor::get_claimable_balances`] returns | ||
| /// an empty set), you can retrieve all spendable outputs by providing all descendant spending |
There was a problem hiding this comment.
This seems to imply you only need to pass the one immediate descendant - the commitment tx, but actually we need to go two layers deep.
|
Also need to make sure we don't yield a |
| pub fn get_spendable_outputs(&self, tx: &Transaction, confirmation_height: u32) -> Vec<SpendableOutputDescriptor> { | ||
| let inner = self.inner.lock().unwrap(); | ||
| let current_height = inner.best_block.height; | ||
| if current_height.saturating_sub(ANTI_REORG_DELAY) + 1 >= confirmation_height { |
There was a problem hiding this comment.
One API advance could be to pass a user-selected value rather than to rely on the hardcoded ANTI_REORG_DELAY. If the spend is low-value or you wish to have the liquidity locked up in a let’s say StaticPaymentOutputDescriptor available to spend now 1-conf or 2-conf might be enough.
Here ANTI_REORG_DELAY doesn’t necessarily have the same level of severity than in other locations of channel monitor, where we wanna to be sure a HTLC-timeout cannot be reorged out by a HTLC-preimage on an outbound link before passing backward the timeout. I think a descriptor_confirmation_delay user setting could be introduced in the future.
| @@ -3441,7 +3468,7 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> { | |||
| } | |||
| self.is_resolving_htlc_output(&tx, height, &block_hash, &logger); | |||
|
|
|||
| self.is_paying_spendable_output(&tx, height, &block_hash, &logger); | |||
| self.check_tx_and_push_spendable_outputs(&tx, height, &block_hash, &logger); | |||
There was a problem hiding this comment.
If a counterparty is paying to one of the spendable descriptor (e.g our payment_point as a P2WSH or P2WPKH) from let’s say a second-stage malleable HTLC-transaction on our local / holder commitment transaction, we might receive a descriptor of a “tag” value that can be latter used for e.g deanonymization attack (at the image of sending dust value to on-chain wallet to fingerprint them).
I don’t think this is a new concern and this sounds already permissible by current is_paying_spendable_output. l still think our transaction pattern matching in transactions_confirmed could be stronger to check that non-commitment-or-closing-transaction-paying-to-us-is-spent-by-us.
Currently, our API will only expose
SpendableOutputDescriptors once after they are no longer under reorg risk (seeANTI_REORG_DELAY). Users have often requested they'd like the ability to retrieve these in some other way, either for historical purposes, or to handle replaying any in the event of a failure.Fixes #2493.