use CBlockIndex* insted of uint256 for UpdatedBlockTip signal #6680

pull jonasschnelli wants to merge 1 commits into bitcoin:master from jonasschnelli:2015/09/zmq_cblockindex changing 8 files +21 −17

jonasschnelli commented at 2:47 PM on September 16, 2015: contributor
- removes mapBlockIndex find operation
- theoretically allows removing the cs_main lock during zqm notification while introducing a new file position lock
jonasschnelli force-pushed on Sep 16, 2015

use CBlockIndex* insted of uint256 for UpdatedBlockTip signal

- removes mapBlockIndex find operation
- theoretically allows removing the cs_main lock during zqm notification while introducing a new file position lock

d76a8acb9b

jonasschnelli force-pushed on Sep 16, 2015
dcousens commented at 3:38 PM on September 16, 2015: contributor

utACK
promag commented at 10:34 PM on September 19, 2015: member

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

Otherwise LGTM.
sipa commented at 11:28 PM on September 19, 2015: member

They have infinite lifetime. Some fields are mutable (especially validation state and disk offsets), but even CBlockIndex::GetAncestor can be used without locking.
dcousens commented at 1:52 AM on September 20, 2015: contributor

@sipa didn't that invariant change with pruning?
sipa commented at 1:57 AM on September 20, 2015: member

Since pruning, the disk position information can go from existing to nonexisting, but otherwise, nothing chamged. CBlockIndex entries still live forever, and their pprev, pskip, chainwork, block hash, difficulty, timestamp, height, ... are still immutable.
dcousens commented at 2:00 AM on September 20, 2015: contributor

@sipa ah, so that was what could change in #6688?
laanwj commented at 12:03 PM on September 21, 2015: member

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 added the label Refactoring on Sep 21, 2015
sipa commented at 4:57 PM on September 22, 2015: member

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 commented at 10:19 PM on September 22, 2015: contributor

Agreed with @sipa, do the existing locks allow for recursive locking? Or would we have to re-write a lot of that code?
sipa commented at 10:21 PM on September 22, 2015: member

The sync.h locks support recursive locking.
jgarzik commented at 10:53 PM on September 22, 2015: contributor

+1 on not holding across disk I/O
jonasschnelli commented at 8:13 AM on September 26, 2015: contributor

@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 merged this on Sep 30, 2015
laanwj closed this on Sep 30, 2015
laanwj referenced this in commit 4f44530bc3 on Sep 30, 2015
zkbot referenced this in commit 36df5a92f8 on Feb 9, 2017
zkbot referenced this in commit dd8b38316f on Feb 9, 2017
zkbot referenced this in commit 253c610783 on Feb 9, 2017
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels