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.

Sign up for GitHub

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

Jump to bottom

refactoring: [Net Processing] Follow-ups to #21160 #24692

Merged
merged 3 commits into from
Mar 30, 2022
Merged

refactoring: [Net Processing] Follow-ups to #21160 #24692

merged 3 commits into from
Mar 30, 2022

Conversation

jnewbery
Copy link
Contributor

#21160 ([net/net processing]: Move tx inventory into net_processing) had some unaddressed review comments when it was merged. This branch addresses those comments.

@jnewbery
net_processing: move CNode data access out of lock
21154ff 
CNode::m_relays_tx and CNode::m_bloom_filter_loaded access don't require
the Peer::TxRelay::m_bloom_filter_mutex lock, so move them out of the
lock scope.

See bitcoin#21160 (comment)
and bitcoin#21160 (comment).
@jnewbery jnewbery mentioned this pull request Mar 28, 2022
Merged
@jnewbery
Copy link
Contributor Author

jnewbery commented Mar 28, 2022 •
edited
Loading

@theuni I believe this addresses all your remaining review comments from #21160

@fanquake fanquake added the P2P label Mar 28, 2022
@fanquake
Copy link
Member

cc @dergoegge

dergoegge
dergoegge suggested changes Mar 28, 2022
View reviewed changes
Copy link
Member

@dergoegge dergoegge left a comment

Choose a reason for hiding this comment

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

The tests are failing because the wrong value is being passed to PushNodeVersion for the block_relay_only param.

src/net_processing.cpp Outdated Show resolved Hide resolved
src/net_processing.cpp Outdated Show resolved Hide resolved
@jnewbery jnewbery force-pushed the 202202_21160_followups branch from 2432ec4 to 0807116 Compare March 28, 2022 20:27
@jnewbery
Copy link
Contributor Author

The tests are failing because the wrong value is being passed to PushNodeVersion for the block_relay_only param.

Oops. Now fixed. Thank you!

ajtowns
ajtowns reviewed Mar 28, 2022
View reviewed changes
src/net_processing.cpp Outdated Show resolved Hide resolved
src/net_processing.cpp Outdated Show resolved Hide resolved
@maflcko
Copy link
Member

maflcko commented Mar 29, 2022

See also suggestion in #21160 (comment)

@DrahtBot
Copy link
Contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #24474 (Additional thread safety annotations for CNode/Peer by ajtowns)
  • #22778 (net processing: Reduce resource usage for inbound block-relay-only connections by jnewbery)
  • #21527 (net_processing: lock clean up by ajtowns)

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.

@jnewbery
Copy link
Contributor Author

See also suggestion in #21160 (comment)

Added.

I've removed the net_processing: MaybeSendFeeFilter() takes a Peer::TxFilter* and net_processing: PushNodeVersion() takes a bool instead of a Peer& commits. There doesn't seem to be agreement on what should be passed down to these functions, and in the absence of agreement, continuing to pass just a reference to the Peer object seems fine. I have however changed the function signature of PushNodeVersion() to take a const reference to Peer. That seems like an uncontroversial improvement.

@maflcko
Copy link
Member

maflcko commented Mar 29, 2022

I think you forgot to force push your local branch?

@jnewbery jnewbery force-pushed the 202202_21160_followups branch from 0807116 to a40978d Compare March 29, 2022 15:19
@maflcko maflcko changed the title [Net Processing] Follow-ups to #21160 refactoring: [Net Processing] Follow-ups to #21160 Mar 29, 2022
@maflcko
Copy link
Member

maflcko commented Mar 29, 2022

review ACK a40978d

@dergoegge
Copy link
Member

ACK a40978d

@DrahtBot DrahtBot mentioned this pull request Mar 29, 2022
Closed
@ajtowns
Copy link
Contributor

ajtowns commented Mar 30, 2022

ACK a40978d

@fanquake fanquake merged commit f089a08 into bitcoin:master Mar 30, 2022
@jnewbery jnewbery deleted the 202202_21160_followups branch March 30, 2022 07:27
This was referenced Mar 30, 2022
Merged
Closed
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 3, 2022
@fanquake
Merge bitcoin#24692: refactoring: [Net Processing] Follow-ups to bitc…
7a50d84 
…oin#21160

a40978d [fuzz] Assert that Peer.m_tx_relay.m_relay_txs has been set correctly (John Newbery)
0bca5f2 [net processing] PushNodeVersion() takes a const Peer& (John Newbery)
21154ff net_processing: move CNode data access out of lock (John Newbery)

Pull request description:

  bitcoin#21160 ([net/net processing]: Move tx inventory into net_processing) had some unaddressed review comments when it was merged. This branch addresses those comments.

ACKs for top commit:
  MarcoFalke:
    review ACK a40978d
  dergoegge:
    ACK a40978d
  ajtowns:
    ACK a40978d

Tree-SHA512: 46624e275f918c5f32d0adab0766e9b3ef8ebdbc74a3c8886d8a2e2ff1079029dcc371b40ef0d787609e9c05219b7456f3e2dfe4fb0cb7bf23ef966769aef1a1
@bitcoin bitcoin locked and limited conversation to collaborators Mar 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@maflcko maflcko maflcko left review comments

@ajtowns ajtowns ajtowns left review comments

@dergoegge dergoegge dergoegge requested changes

Assignees
No one assigned
Labels
P2P Refactoring
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jnewbery @fanquake @maflcko @DrahtBot @dergoegge @ajtowns