Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Expose onchain transactions in store #432
Expose onchain transactions in store #432
Changes from all commits
cd8958f
d92a568
c9ddfae
62ff60c
e46e407
0710434
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Say the transaction was re-orged out and was then RBF'ed. Is that possible? Would we have payment stuck in pending?
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.
Hmm, that's a very valid concern. Tbh. I'm not entirely sure how to handle that: for one, the RBF'd transaction is technically pending as there could be another reorg that makes it part of the best chain again. So essentially any signed & broadcasted transaction remains 'pending' forever.
I think the only way I can come up with to handle this would be to never add
Pending
transactions but jump toSucceeded
directly once they reachANTI_REORG_DELAY
. Or we just keep the current approach and add docs noting that transactions in pending might never succeed for one reason or another?I think at least for BDK's own RBF's we could see if we find a better solution when we do #367.
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.
Could we possibly detect that the UTXO has been spent and remove the pending payment?
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.
I guess, but that would require us to keep track and persist a list of all the UTXOs, write an adapter around
lightning-transaction-sync
'sFilter
implementation to also register spends for them, and then implementConfirm
onWallet
, just to be able to remove anyPending
entries that have been replaced.For now that seems like a lot of overhead to just account for this rare edgecase (reorg + RBF)? But I wouldn't completely rule it out to do something like it in if we find we need to do additional RBF tracking anyways as part of #367.
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.
I wonder if this gets more complicated with splicing in/out or more exotic transactions paying us and opening a channel, for instance.
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.
Hmm, good point, now opened the more lightningdevkit/rust-lightning#3566 to supersede lightningdevkit/rust-lightning#3548
Would be great if an API allowing to query the type of a given transaction (
Txid
) could be added as part of the splicing work.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.
Is change abstracted away from this API?
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.
I don't think so, the docs state:
so IIUC
received
would include the change.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.
Couldn't the change be more than the amount sent, making it look like an inbound payment when it was really an outbound? Or am I misunderstanding?
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.
IIUC, any change amount would be accounted for both in
sent
andreceived
, so you can essentially ignore it in terms of comparisons (think: subtract on both sides).Say you send a payment of 10 sats, and add a txin spending an wallet output with 100 sats. This would result in two txouts, one to the recipient with 10 sats, one change back to a wallet address with 85sats or so, leaving 5 for mining fees. IIUC, this would result in
sent = 100
,received = 85
, resulting insent > received => PaymentDirection::Outbound
.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.
Should we consider these two separate payments? I guess we could't use the txid as the payment id, though.
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.
Mhh, given that (IIUC)
received
includes the change, I'd rather not add each change output as an inbound payment?