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

Fix mempool transactions disappearing if a parent tx is confirmed. #539

Merged
merged 1 commit into from
Nov 6, 2015

Conversation

Dirbaio
Copy link
Contributor

@Dirbaio Dirbaio commented Nov 5, 2015

There's a bug in the logic that processes new blocks that causes txs to be unnecessarily dropped from mempool.

Basically:

There are two txs A and B in mempool. Tx B uses an output from A.
A new block is received that contains A but not B.

RemoveTransaction(A) will be called on the mempool, which will recursively remove B. However, B should not be removed because it's not orphan, its parent is now on the blockchain.

This fixes the bug by not recursively removing depending transactions when processing blocks.

Review on Reviewable

@dajohi
Copy link
Member

dajohi commented Nov 5, 2015

I much rather prefer RemoveTransaction get the API change like removeTransaction, and get rid of RemoveConfirmedTransaction. The update all callers of RemoveTransaction() to specify whether they want to remove depending or not.

@davecgh
Copy link
Member

davecgh commented Nov 5, 2015

The concept is correct, but I agree with @dajohi regarding the implementation.

@davecgh
Copy link
Member

davecgh commented Nov 5, 2015

Reviewed 2 of 2 files at r1, 2 of 2 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from the review on Reviewable.io

@dajohi
Copy link
Member

dajohi commented Nov 5, 2015

OK

1 similar comment
@davecgh
Copy link
Member

davecgh commented Nov 5, 2015

OK

@jrick
Copy link
Member

jrick commented Nov 5, 2015

Not sure depending is the best name for those transactions, because depending could also refer to parent txs. Perhaps removeChildren?

@davecgh
Copy link
Member

davecgh commented Nov 5, 2015

I suppose the name could be confusing. Given the comment accurately describes it I didn't worry about it, but perhaps removeRedeemers or removeChildren might be less confusing.

@jrick
Copy link
Member

jrick commented Nov 5, 2015

+1 for removeRedeemers

@Dirbaio
Copy link
Contributor Author

Dirbaio commented Nov 5, 2015

Changed.

@davecgh
Copy link
Member

davecgh commented Nov 5, 2015

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from the review on Reviewable.io

@jrick
Copy link
Member

jrick commented Nov 5, 2015

ok

// pool also removes any transactions which depend on it,
// recursively.
// no longer an orphan. Transactions which depend on a confirmed
// transaction are NOT removed recursively because they are still valid.
Copy link
Member

Choose a reason for hiding this comment

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

While here, can we make the above comment limited to 80 chars?

@Dirbaio
Copy link
Contributor Author

Dirbaio commented Nov 6, 2015

Review status: 1 of 2 files reviewed at latest revision, 1 unresolved discussion.


blockmanager.go, line 1219 [r3] (raw file):
Done.


Comments from the review on Reviewable.io

@davecgh
Copy link
Member

davecgh commented Nov 6, 2015

Reviewed 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from the review on Reviewable.io

@dajohi
Copy link
Member

dajohi commented Nov 6, 2015

OK

@conformal-deploy conformal-deploy merged commit 912a8d8 into btcsuite:master Nov 6, 2015
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.

5 participants