Always clean up manual transaction prioritization #6464

Merged
merged 1 commit into from Oct 27, 2015

Conversation

Projects
None yet
6 participants
@casey
Contributor

casey commented Jul 22, 2015

This fixes #4818

An alternative fix is to add the transaction prioritization information to CTxMemPoolEntry itself, instead of storing it separately. This would add 128 bits to every transaction in the mempool, which seems undesirable, given that the majority of transactions won't have a manual transaction prioritization delta.

@morcos

@jgarzik

This comment has been minimized.

Show comment
Hide comment
@jgarzik

jgarzik Jul 23, 2015

Contributor

concept ACK - will review & test in depth

Contributor

jgarzik commented Jul 23, 2015

concept ACK - will review & test in depth

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jul 23, 2015

Member

Prioritisations should never be removed until a transaction is mined. It's intentionally not on CTxMemPoolEntry because we may not have (or be willing to accept) the transaction until after prioritising it. Although it does make sense to remove the entry if a conflicting transaction is confirmed.

Member

luke-jr commented Jul 23, 2015

Prioritisations should never be removed until a transaction is mined. It's intentionally not on CTxMemPoolEntry because we may not have (or be willing to accept) the transaction until after prioritising it. Although it does make sense to remove the entry if a conflicting transaction is confirmed.

@laanwj laanwj added the Mining label Jul 24, 2015

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jul 24, 2015

Member

Agree with @luke-jr. @casey can you move the remove call to CTxMemPool::removeForBlock ?

Member

sipa commented Jul 24, 2015

Agree with @luke-jr. @casey can you move the remove call to CTxMemPool::removeForBlock ?

@casey

This comment has been minimized.

Show comment
Hide comment
@casey

casey Jul 24, 2015

Contributor

@luke-jr @sipa In that case, shouldn't the existing call to ClearPrioritisation in CTxMemPool::removeForBlock be left where it is, and an additional one be added to CTxMemPool::removeConflicts, to clear the priority of conflicting transactions?

Contributor

casey commented Jul 24, 2015

@luke-jr @sipa In that case, shouldn't the existing call to ClearPrioritisation in CTxMemPool::removeForBlock be left where it is, and an additional one be added to CTxMemPool::removeConflicts, to clear the priority of conflicting transactions?

@casey

This comment has been minimized.

Show comment
Hide comment
@casey

casey Aug 3, 2015

Contributor

So I think in that case it's just a simple one line change to clear prioritizations when a conflicting transaction is mined.

Contributor

casey commented Aug 3, 2015

So I think in that case it's just a simple one line change to clear prioritizations when a conflicting transaction is mined.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Aug 4, 2015

Member

Seems reasonable to me.

Member

sipa commented Aug 4, 2015

Seems reasonable to me.

@casey

This comment has been minimized.

Show comment
Hide comment
@casey

casey Sep 3, 2015

Contributor

@luke-jr, could you take a look and make sure that it's now doing the right thing?

Contributor

casey commented Sep 3, 2015

@luke-jr, could you take a look and make sure that it's now doing the right thing?

@gavinandresen

This comment has been minimized.

Show comment
Hide comment
@gavinandresen

gavinandresen Sep 3, 2015

Contributor

Untested ACK.

Contributor

gavinandresen commented Sep 3, 2015

Untested ACK.

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Oct 23, 2015

Member

Looks reasonable, utACK

Member

luke-jr commented Oct 23, 2015

Looks reasonable, utACK

@laanwj laanwj merged commit 2d8c49d into bitcoin:master Oct 27, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Oct 27, 2015

Merge pull request #6464
2d8c49d Clean up tx prioritization when conflict mined (Casey Rodarmor)

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Nov 18, 2015

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Dec 8, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment