Skip to content

Conversation

@andrewtoth
Copy link
Contributor

When CZMQPublishRawBlockNotifier::NotifyBlock is called it calls ReadBlockFromDisk with the passed in block index. However, we will always have already read this block into memory during ActivateBestChain and use it to call BlockConnected. We can check for the tip block there and pass that into UpdatedBlockTip to avoid having the zmq notifier read the block. This also has the benefit of removing the node/blockstorage.h dependency entirely from the zmq code, so we can continue working to remove ReadBlockFromDisk out of global scope (#26316 (comment)).

@DrahtBot
Copy link
Contributor

DrahtBot commented Oct 24, 2022

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #26308 (rpc/rest/zmq: reduce LOCK(cs_main) scope: ~6 times as many requests per second by andrewtoth)
  • #15606 (assumeutxo by jamesob)

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.

Copy link
Contributor

@aureleoules aureleoules left a comment

Choose a reason for hiding this comment

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

ACK 7b631dc - LGTM

Copy link
Contributor

@w0xlt w0xlt left a comment

Choose a reason for hiding this comment

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

crACK 7b631dc

@maflcko
Copy link
Member

maflcko commented Nov 14, 2022

This will increase the memory use even if not using zmq. I guess it is fine because the queue is limited?

@andrewtoth
Copy link
Contributor Author

This will increase the memory use even if not using zmq. I guess it is fine because the queue is limited?

That sounds accurate. It seems LimitValidationInterfaceQueue in the loop prevents this from blowing up.

@andrewtoth
Copy link
Contributor Author

I don't think this is worth the effort to change anymore. It was a step towards #26316, but I think #26533 is a better way to solve that issue.

@andrewtoth andrewtoth closed this Nov 18, 2022
luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Jun 28, 2023
luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Jun 28, 2023
luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Aug 16, 2023
luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Aug 16, 2023
@bitcoin bitcoin locked and limited conversation to collaborators Nov 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants