Skip to content
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

wtxmgr: only remove entry for specified spending transaction #609

Merged
merged 1 commit into from
Mar 27, 2019
Merged

wtxmgr: only remove entry for specified spending transaction #609

merged 1 commit into from
Mar 27, 2019

Conversation

wpaulino
Copy link
Contributor

In this commit, we address an issue with the wallet store where it'd be
possible for us to keep lingering unconfirmed transaction entries for an
output that has been spent by a different, confirmed transaction. This
was caused due to us removing all spending transaction entries for a
given output when removing conflicts. Since all of the entries would be
removed, we weren't able to retrieve the hashes of the remaining
spending transactions to remove their records as well. Instead, we
propose to only remove the entry for the specified spending transaction.

In this commit, we address an issue with the wallet store where it'd be
possible for us to keep lingering unconfirmed transaction entries for an
output that has been spent by a different, confirmed transaction. This
was caused due to us removing all spending transaction entries for a
given output when removing conflicts. Since all of the entries would be
removed, we weren't able to retrieve the hashes of the remaining
spending transactions to remove their records as well. Instead, we
propose to only remove the entry for the specified spending transaction.
@@ -69,13 +69,19 @@ func (s *Store) removeDoubleSpends(ns walletdb.ReadWriteBucket, rec *TxRecord) e

doubleSpendHashes := fetchUnminedInputSpendTxHashes(ns, prevOutKey)
for _, doubleSpendHash := range doubleSpendHashes {
doubleSpendVal := existsRawUnmined(ns, doubleSpendHash[:])
// We'll make sure not to remove ourselves.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This likely could have been included as a distinct commit. As is now, it's easy to miss this amongst everything else going on in this diff.

commitDBTx(t, store, db, func(ns walletdb.ReadWriteBucket) {
t.Helper()

outputs, err := store.UnspentOutputs(ns)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels as though we shouldn't even keep around this series of UTXOs where the creation transaction for each output all spend the same input. This just confuses zero conf coin selection (which maybe itself shouldn't really be a thing).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, but zero conf coin selection does have it uses within lnd and higher level applications. Another reason for wanting to keep track of all transactions that spend the same input would be for RBF, although not completely sure how useful that'd be.

Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🎖

@Roasbeef Roasbeef merged commit 8b90263 into btcsuite:master Mar 27, 2019
@wpaulino wpaulino deleted the utxos-after-remove branch March 28, 2019 01:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants