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

Backport of Bitcoin PR #21750 #608

Merged
merged 1 commit into from
Sep 26, 2023
Merged

Conversation

JackPiri
Copy link
Contributor

@JackPiri JackPiri commented Sep 19, 2023

Backport of Bitcoin PR #21750 (bitcoin/bitcoin#21750).

Referencing Bitcoin PR description, "[...] Indeed all locks of CNode::cs_vSend are done either when the reference
count is >0 or under the protection of CConnman::cs_vNodes and the
node being in CConnman::vNodes. [...]"
still applies to Zend current codebase:

  • TLSManager::threadSocketHandler: node in CConnman::vNodes and CConnman::cs_vNodes protected
  • ThreadSocketHandler: node in CConnman::vNodes and CConnman::cs_vNodes protected
  • ThreadMessageHandler: reference count >0
  • CNode::BeginMessage/CNode::AbortMessage/CNode::EndMessage: called in PushMessage, in turn called (directly/indirectly) in ProcessMessages/SendMessages which fall in the case of reference count >0

Regarding the other two locks:

  • TRY_LOCK(pnode->cs_vRecvMsg, lockRecv);: will be addressed by backport of Bitcoin PR #9441 (Net: Massive speedup. Net locks overhaul bitcoin/bitcoin#9441)

  • TRY_LOCK(pnode->cs_inventory, lockInv);: can be linked to Bitcoin PR #19347 ([net] Make cs_inventory nonrecursive bitcoin/bitcoin#19347), considering the commit message "[...] The only other places that cs_inventory can be taken are:

    • In ProcessMessages() or SendMessages(), when the CNode's nRefCount
      must be >0 (see ThreadMessageHandler(), where the refcount is
      incremented before calling ProcessMessages() and SendMessages()).
    • In a ForEachNode() lambda in PeerLogicValidation::UpdatedBlockTip().
      ForEachNode() locks cs_vNodes and calls the function on the CNode
      objects in vNodes. [...]"

    in Zend codebase there are some lockings on cs_inventory in ActivateBestChain, RelayAlternativeChain, Relay but they are protected through connman->cs_vNodes, hence the lock can be removed.

Copy link
Contributor

@a-petrini a-petrini left a comment

Choose a reason for hiding this comment

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

Thanks, looks good to me!

Copy link
Contributor

@drgora drgora left a comment

Choose a reason for hiding this comment

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

Looks good to me, thank you!

@JackPiri JackPiri force-pushed the gp/remove_unnecessary_check_csvSend branch from e1b1ebf to 1ed962f Compare September 20, 2023 08:21
Partial backport of Bitcoin PR #19347
@ptagl ptagl force-pushed the gp/remove_unnecessary_check_csvSend branch from 1ed962f to c82860f Compare September 26, 2023 06:02
@ptagl ptagl merged commit 2ec528f into main Sep 26, 2023
@ptagl ptagl deleted the gp/remove_unnecessary_check_csvSend branch September 26, 2023 06:03
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.

4 participants