I think you’re right that we’re going to disagree, but for a topic like this – of where in our code base it is permitted to access CBlockIndex* objects and their member functions, and where it’s permitted to use and store these objects while not holding a lock – the project needs to make a decision about what our architecture looks like, and arrive at a common understanding (even if some might disagree on what the optimal architecture would be). So towards that end, I will try to explain further why I think it’s illogical to use general arguments around “holding raw pointers to another component’s data” as a reason to have this code live in validation specifically for the functions introduced in this PR.
These functions – UpdateChainTipSet(), IsBlockInMainOrBestChain(), and IsBlockInChainTipSet() – seem to do the following:
- look up block index entries in our blockmanager
- look up our current chain tip (ActiveChain.Tip())
- look up m_best_header (the most-work header we know of)
- invoke public functions on block index entries (namely,
GetAncestor())
Other than the block index and ActiveChain.Tip(), no chainstate-specific state is accessed and (importantly!) no consensus validation of any sort is performed in these functions. These functions do not change any state inside of validation either.
So to argue that these functions do not belong in net_processing, I think you’d have to argue that some of those operations should be private to the validation code. It is difficult for me to see for which of those that should be the case, because I don’t think net_processing would work at all without being able to look at the current tip (and see how much work it has, compared to the work of other headers a peer might be announcing), or look up block index entries, or know what the most work headers chain is (really this is a value that only exists for net_processing in the first place, so that we can save time when performing headers sync on startup). I’d add that the public functions of CBlockIndex would seem to exist precisely so that they could be invoked by other modules (otherwise they would not be public).
So the only remaining point that I think you are making which we could consider is whether caching CBlockIndex pointers after lookup and using them without holding cs_main is harmful. If that were possibly the case, then our validation interface callbacks – which pass const CBlockIndex* arguments without holding cs_main – would be broken. All of the places in our code where we look up a block index entry and then release cs_main and continue to operate on it – including in code touched by this PR (see ProcessHeadersMessage) – would also be broken. Never mind the countless other places in at least net_processing where we cache pointers to block index entries, such as in CNodeState.
Coming back to the particular functions introduced here: the data structures passed around to these functions (the sets of chain tips that are being processed) are not even a validation data structure. They are created in this PR and are owned by net_processing. The semantics of what goes in them is specified by net_processing. Having checks on them happen in validation is moving net_processing logic into validation – a layer violation.
In summary, this general philosophical claim that using CBlockIndex* pointers in net_processing is dangerous strikes me as unhelpful and out of sync with how our code is designed. And it’s hard for me to see what operations these particular functions do that we don’t want net_processing to be able to do generally, while it also seems like adding a net_processing specific function to validation is leaking implementation specifics outside their natural scope.
With the “one month limit” approach, if block x is older than one month we reject any headers that build on top of x and re-request the 2000 headers that connect to c, so we can’t reorg to the chain of block x if needed.
With the tip set approach we would also reject and re-request but when the re-requested headers arrive we add the tip of those to the tip set for that network, so further headers that connect to that tip would be not be rejected.
