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

Evict orphans which are included or precluded by accepted blocks. #8179

Merged
merged 5 commits into from Jun 20, 2016

Conversation

Projects
None yet
4 participants
@gmaxwell
Member

gmaxwell commented Jun 9, 2016

This eliminates the primary leak that causes the orphan map to
always grow to its maximum size.

This does not go so far as to attempt to connect orphans made
connectable by a new block.

Keeping the orphan map less full helps improve the reliability
of relaying chains of transactions.

@laanwj laanwj added the P2P label Jun 9, 2016

@pstratem

View changes

Show outdated Hide outdated src/main.cpp
@pstratem

This comment has been minimized.

Show comment
Hide comment
@pstratem

pstratem Jun 10, 2016

Contributor

ACK 70b5f60c761f56e0e84583e77d20a81db3fe1424

modulo whitespace nit

Contributor

pstratem commented Jun 10, 2016

ACK 70b5f60c761f56e0e84583e77d20a81db3fe1424

modulo whitespace nit

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Jun 10, 2016

Member

So, I'm not super happy with the behavior-- the issue is that it removes included or conflicted orphans, but if those orphans themselves have orphaned children, those won't get removed. The behavior I'm seeing in testing is that it initially removes many transactions but over time seems to remove fewer or fewer, and I think it's because the orphanmap gets full of double-orphans that were conflicted.

I'm not super keen on the performance implications of recursively eliminating there.

Member

gmaxwell commented Jun 10, 2016

So, I'm not super happy with the behavior-- the issue is that it removes included or conflicted orphans, but if those orphans themselves have orphaned children, those won't get removed. The behavior I'm seeing in testing is that it initially removes many transactions but over time seems to remove fewer or fewer, and I think it's because the orphanmap gets full of double-orphans that were conflicted.

I'm not super keen on the performance implications of recursively eliminating there.

@pstratem

This comment has been minimized.

Show comment
Hide comment
@pstratem

pstratem Jun 10, 2016

Contributor

@gmaxwell Agreed this behavior is not ideal, but this is an improvement.

Contributor

pstratem commented Jun 10, 2016

@gmaxwell Agreed this behavior is not ideal, but this is an improvement.

@sipa

View changes

Show outdated Hide outdated src/main.cpp
@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jun 10, 2016

Member

@gmaxwell Here is a commit that switches the mapOrphansByPrev entirely to be by-COutPoint: https://github.com/sipa/bitcoin/commits/reworkorphans

Member

sipa commented Jun 10, 2016

@gmaxwell Here is a commit that switches the mapOrphansByPrev entirely to be by-COutPoint: https://github.com/sipa/bitcoin/commits/reworkorphans

sipa and others added some commits Jun 10, 2016

This eliminates the primary leak that causes the orphan map to
 always grow to its maximum size.

This does not go so far as to attempt to connect orphans made
 connectable by a new block.

Keeping the orphan map less full helps improve the reliability
 of relaying chains of transactions.
@sipa

View changes

Show outdated Hide outdated src/main.cpp
@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jun 11, 2016

Member

utACK with nit

Member

sipa commented Jun 11, 2016

utACK with nit

gmaxwell added some commits Jun 11, 2016

Adds an expiration time for orphan tx.
This prevents higher order orphans and other junk from
 holding positions in the orphan map.  Parents delayed
 twenty minutes are more are unlikely to ever arrive.

The freed space will improve the orphan matching success rate for
 other transactions.
Treat orphans as implicit inv for parents, discard when parents rejec…
…ted.

An orphan whos parents were rejected is never going to connect, so there
 is little utility in keeping it.

Orphans also helpfully tell us what we're missing, so go ahead and treat
 it as INVed.
Increase maximum orphan size to 100,000 bytes.
Although this increases node memory usage in the worst case by perhaps
 30MB, the current behavior causes severe issues with dependent tx relay.
@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Jun 15, 2016

Member

@sipa Nit picked.

Member

gmaxwell commented Jun 15, 2016

@sipa Nit picked.

@laanwj laanwj merged commit 54326a6 into bitcoin:master Jun 20, 2016

1 check passed

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

laanwj added a commit that referenced this pull request Jun 20, 2016

Merge #8179: Evict orphans which are included or precluded by accepte…
…d blocks.

54326a6 Increase maximum orphan size to 100,000 bytes. (Gregory Maxwell)
8c99d1b Treat orphans as implicit inv for parents, discard when parents rejected. (Gregory Maxwell)
11cc143 Adds an expiration time for orphan tx. (Gregory Maxwell)
db0ffe8 This eliminates the primary leak that causes the orphan map to  always grow to its maximum size. (Gregory Maxwell)
1b0bcc5 Track orphan by prev COutPoint rather than prev hash (Pieter Wuille)
@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 20, 2016

Member

utACK 54326a6

Member

laanwj commented Jun 20, 2016

utACK 54326a6

This was referenced Jun 22, 2016

codablock added a commit to codablock/dash that referenced this pull request Sep 16, 2017

Merge #8179: Evict orphans which are included or precluded by accepte…
…d blocks.

54326a6 Increase maximum orphan size to 100,000 bytes. (Gregory Maxwell)
8c99d1b Treat orphans as implicit inv for parents, discard when parents rejected. (Gregory Maxwell)
11cc143 Adds an expiration time for orphan tx. (Gregory Maxwell)
db0ffe8 This eliminates the primary leak that causes the orphan map to  always grow to its maximum size. (Gregory Maxwell)
1b0bcc5 Track orphan by prev COutPoint rather than prev hash (Pieter Wuille)

codablock added a commit to codablock/dash that referenced this pull request Sep 19, 2017

Merge #8179: Evict orphans which are included or precluded by accepte…
…d blocks.

54326a6 Increase maximum orphan size to 100,000 bytes. (Gregory Maxwell)
8c99d1b Treat orphans as implicit inv for parents, discard when parents rejected. (Gregory Maxwell)
11cc143 Adds an expiration time for orphan tx. (Gregory Maxwell)
db0ffe8 This eliminates the primary leak that causes the orphan map to  always grow to its maximum size. (Gregory Maxwell)
1b0bcc5 Track orphan by prev COutPoint rather than prev hash (Pieter Wuille)

codablock added a commit to codablock/dash that referenced this pull request Dec 27, 2017

Merge #8179: Evict orphans which are included or precluded by accepte…
…d blocks.

54326a6 Increase maximum orphan size to 100,000 bytes. (Gregory Maxwell)
8c99d1b Treat orphans as implicit inv for parents, discard when parents rejected. (Gregory Maxwell)
11cc143 Adds an expiration time for orphan tx. (Gregory Maxwell)
db0ffe8 This eliminates the primary leak that causes the orphan map to  always grow to its maximum size. (Gregory Maxwell)
1b0bcc5 Track orphan by prev COutPoint rather than prev hash (Pieter Wuille)

codablock added a commit to codablock/dash that referenced this pull request Dec 28, 2017

Merge #8179: Evict orphans which are included or precluded by accepte…
…d blocks.

54326a6 Increase maximum orphan size to 100,000 bytes. (Gregory Maxwell)
8c99d1b Treat orphans as implicit inv for parents, discard when parents rejected. (Gregory Maxwell)
11cc143 Adds an expiration time for orphan tx. (Gregory Maxwell)
db0ffe8 This eliminates the primary leak that causes the orphan map to  always grow to its maximum size. (Gregory Maxwell)
1b0bcc5 Track orphan by prev COutPoint rather than prev hash (Pieter Wuille)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment