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

use CBlockIndex* insted of uint256 for UpdatedBlockTip signal #6680

Merged
merged 1 commit into from Sep 30, 2015

Conversation

@jonasschnelli
Copy link
Member

jonasschnelli commented Sep 16, 2015

  • removes mapBlockIndex find operation
  • theoretically allows removing the cs_main lock during zqm notification while introducing a new file position lock
@jonasschnelli jonasschnelli force-pushed the jonasschnelli:2015/09/zmq_cblockindex branch Sep 16, 2015
- removes mapBlockIndex find operation
- theoretically allows removing the cs_main lock during zqm notification while introducing a new file position lock
@jonasschnelli jonasschnelli force-pushed the jonasschnelli:2015/09/zmq_cblockindex branch to d76a8ac Sep 16, 2015
@dcousens
Copy link
Contributor

dcousens commented Sep 16, 2015

utACK

@promag
Copy link
Member

promag commented Sep 19, 2015

I choose uint256 because I don't know the ownership, thread safety and lifetime of CBlockIndex* objects.

Otherwise LGTM.

@sipa
Copy link
Member

sipa commented Sep 19, 2015

They have infinite lifetime. Some fields are mutable (especially validation state and disk offsets), but even CBlockIndex::GetAncestor can be used without locking.

@dcousens
Copy link
Contributor

dcousens commented Sep 20, 2015

@sipa didn't that invariant change with pruning?

@sipa
Copy link
Member

sipa commented Sep 20, 2015

@dcousens
Copy link
Contributor

dcousens commented Sep 20, 2015

@sipa ah, so that was what could change in #6688?

@laanwj
Copy link
Member

laanwj commented Sep 21, 2015

This makes sense. If there comes a time that CBlockIndex* are no longer infinitely kept in memory, we need to replace them with another handle type throughout the code, and this change will not be a specific blocker.

utACK.

@laanwj laanwj added the Refactoring label Sep 21, 2015
@sipa
Copy link
Member

sipa commented Sep 22, 2015

I think ReadBlockFromDisk should grab cs_main by itself, only when read the disk position information, and release before actual reading. That way, the publisher doesn't need access to cs_main anymore.

@dcousens
Copy link
Contributor

dcousens commented Sep 22, 2015

Agreed with @sipa, do the existing locks allow for recursive locking? Or would we have to re-write a lot of that code?

@sipa
Copy link
Member

sipa commented Sep 22, 2015

@jgarzik
Copy link
Contributor

jgarzik commented Sep 22, 2015

+1 on not holding across disk I/O

@jonasschnelli
Copy link
Member Author

jonasschnelli commented Sep 26, 2015

@sipa: do you prefer adding the cs_main lock to ReadBlockFromDisk() within this PR?
I personally would recommend to keep this PR as it is and address the ReadBlockFromDisk() cs_main lock in another PR because it might need some adaptation here and there.

@laanwj laanwj merged commit d76a8ac into bitcoin:master Sep 30, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
laanwj added a commit that referenced this pull request Sep 30, 2015
d76a8ac use CBlockIndex* insted of uint256 for UpdatedBlockTip signal (Jonas Schnelli)
@str4d str4d mentioned this pull request Jan 31, 2017
zkbot added a commit to zcash/zcash that referenced this pull request Feb 9, 2017
zkbot added a commit to zcash/zcash that referenced this pull request Feb 9, 2017
zkbot added a commit to zcash/zcash that referenced this pull request Feb 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.