Skip to content

Conversation

sipa
Copy link
Member

@sipa sipa commented Mar 22, 2019

As individual orphan transactions can be relatively expensive to handle, it's undesirable to process all of them (max 100) as soon as the parent becomes available, as it pegs the net processing the whole time.

Change this by interrupting orphan handling after every transactions, and continue in the next processing slot of the peer that gave us the parent - similar to how getdata processing works now. Messages from other peers arriving in the mean time are processed normally, but other messages from the peer that gave us the parent have to wait until all orphan processing is done.

@fanquake fanquake added the P2P label Mar 22, 2019
@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 22, 2019

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #15606 ([experimental] UTXO snapshots by jamesob)
  • #15437 (p2p: Remove BIP61 reject messages by MarcoFalke)
  • #15253 (Net: Consistently log outgoing INV messages by Empact)
  • #15141 (Rewrite DoS interface between validation and net_processing by sdaftuar)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@sipa sipa force-pushed the 201903_interruptibleorphans branch from ebf3331 to 358a4f8 Compare March 22, 2019 18:09
sipa added 3 commits March 22, 2019 19:10
This makes orphan processing work like handling getdata messages:
After every actual transaction validation attempt, interrupt
processing to deal with messages arriving from other peers.
@sipa sipa force-pushed the 201903_interruptibleorphans branch from 358a4f8 to 866c805 Compare March 23, 2019 02:26
@gmaxwell
Copy link
Contributor

ACK

@TheBlueMatt
Copy link
Contributor

utACK

@sdaftuar
Copy link
Member

ACK 866c805

@maflcko
Copy link
Member

maflcko commented Mar 28, 2019

utACK 866c805

@maflcko maflcko added this to the 0.18.0 milestone Mar 28, 2019
maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Mar 28, 2019
maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Mar 28, 2019
maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Mar 28, 2019
This makes orphan processing work like handling getdata messages:
After every actual transaction validation attempt, interrupt
processing to deal with messages arriving from other peers.

Github-Pull: bitcoin#15644
Rebased-From: 866c805
Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

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

Refactor LGTM, some comments though.

@promag
Copy link
Contributor

promag commented Mar 29, 2019

utACK 866c805. Verified refactor in 9453018 and moved code in 6e051f3. Not so sure about change in 866c805 just because I'm not familiar with net processing.

maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Apr 1, 2019
866c805 Interrupt orphan processing after every transaction (Pieter Wuille)
6e051f3 [MOVEONLY] Move processing of orphan queue to ProcessOrphanTx (Pieter Wuille)
9453018 Simplify orphan processing in preparation for interruptibility (Pieter Wuille)

Pull request description:

  As individual orphan transactions can be relatively expensive to handle, it's undesirable to process all of them (max 100) as soon as the parent becomes available, as it pegs the net processing the whole time.

  Change this by interrupting orphan handling after every transactions, and continue in the next processing slot of the peer that gave us the parent - similar to how getdata processing works now. Messages from other peers arriving in the mean time are processed normally, but other messages from the peer that gave us the parent have to wait until all orphan processing is done.

ACKs for commit 866c80:
  sdaftuar:
    ACK 866c805
  MarcoFalke:
    utACK 866c805
  promag:
    utACK 866c805. Verified refactor in 9453018 and moved code in 6e051f3. Not so sure about change in 866c805 just because I'm not familiar with net processing.

Tree-SHA512: d8e8a1ee5f2999446cdeb8fc9756ed9c24f3d5cd769a7774ec4c317fc8d463fdfceec88de97266f389b715a5dfcc2b0a3abaa573955ea451786cc43b870e8cde
@maflcko maflcko merged commit 866c805 into bitcoin:master Apr 1, 2019
@fanquake
Copy link
Member

fanquake commented Apr 2, 2019

Backported in #15691.

HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this pull request Apr 19, 2019
HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this pull request Apr 19, 2019
HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this pull request Apr 19, 2019
This makes orphan processing work like handling getdata messages:
After every actual transaction validation attempt, interrupt
processing to deal with messages arriving from other peers.

Github-Pull: bitcoin#15644
Rebased-From: 866c805
HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this pull request Aug 23, 2019
HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this pull request Aug 23, 2019
HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this pull request Aug 23, 2019
This makes orphan processing work like handling getdata messages:
After every actual transaction validation attempt, interrupt
processing to deal with messages arriving from other peers.

Github-Pull: bitcoin#15644
Rebased-From: 866c805
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request May 22, 2020
…uptibility

Summary:
bitcoin/bitcoin@9453018

---

Partial backport of Core [[bitcoin/bitcoin#15644 | PR15644]]

Test Plan:
  ninja check
  ./test/functional/test_runner.py --extended

Reviewers: #bitcoin_abc, deadalnix, nakihito

Reviewed By: #bitcoin_abc, deadalnix, nakihito

Subscribers: nakihito, deadalnix

Differential Revision: https://reviews.bitcoinabc.org/D6216
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request May 23, 2020
…sOrphanTx

Summary:
bitcoin/bitcoin@6e051f3

---

Depends on D6216

Partial backport of Core [[bitcoin/bitcoin#15644 | PR15644]]

Test Plan:
  ninja check

Reviewers: #bitcoin_abc, nakihito, deadalnix

Reviewed By: #bitcoin_abc, nakihito, deadalnix

Subscribers: deadalnix

Differential Revision: https://reviews.bitcoinabc.org/D6217
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request May 23, 2020
Summary:
This makes orphan processing work like handling getdata messages:
After every actual transaction validation attempt, interrupt
processing to deal with messages arriving from other peers.

---

Depends on D6217

Concludes backport of Core [[bitcoin/bitcoin#15644 | PR15644]]

Test Plan:
  ninja check-all

Reviewers: #bitcoin_abc, deadalnix

Reviewed By: #bitcoin_abc, deadalnix

Subscribers: deadalnix

Differential Revision: https://reviews.bitcoinabc.org/D6235
Munkybooty pushed a commit to Munkybooty/dash that referenced this pull request Sep 27, 2021
866c805 Interrupt orphan processing after every transaction (Pieter Wuille)
6e051f3 [MOVEONLY] Move processing of orphan queue to ProcessOrphanTx (Pieter Wuille)
9453018 Simplify orphan processing in preparation for interruptibility (Pieter Wuille)

Pull request description:

  As individual orphan transactions can be relatively expensive to handle, it's undesirable to process all of them (max 100) as soon as the parent becomes available, as it pegs the net processing the whole time.

  Change this by interrupting orphan handling after every transactions, and continue in the next processing slot of the peer that gave us the parent - similar to how getdata processing works now. Messages from other peers arriving in the mean time are processed normally, but other messages from the peer that gave us the parent have to wait until all orphan processing is done.

ACKs for commit 866c80:
  sdaftuar:
    ACK 866c805
  MarcoFalke:
    utACK 866c805
  promag:
    utACK 866c805. Verified refactor in 9453018 and moved code in 6e051f3. Not so sure about change in 866c805 just because I'm not familiar with net processing.

Tree-SHA512: d8e8a1ee5f2999446cdeb8fc9756ed9c24f3d5cd769a7774ec4c317fc8d463fdfceec88de97266f389b715a5dfcc2b0a3abaa573955ea451786cc43b870e8cde
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants