brainstorm: abstract ChainstateManager away in net_processing #35453

Seeking feedback on whether abstracting away ChainstateManager (and Chainstate) in net_processing is worthwhile. This was previously discussed in #27502. Using a wrapper class, here's where I am currently:

• net_processing.h defines an abstract class NetValidationInterface (name TODO)
• net_processing.cpp defines a derived class ChainManAdapter that takes ChainstateManager& and wraps calls to it. This hides members like m_blockman, m_best_header, etc.
• PeerManagerImpl stores a std::unique_ptr<NetValidationInterface> that is never nullptr and replaces all previous calls to m_chainman.

With this, it becomes possible to reach most (all?) branches gated on m_chainman in net_processing when fuzzing without a TestingSetup. For example, this lets us toggle IsPruneMode, mock reorgs easier, and report the full set of BlockValidationState / MempoolAcceptResult. This should give speed and coverage wins (determinism too?). The same approach could be applied to m_mempool, though I want to see if the general approach is acceptable.

Questions:

• My rough draft diff to net_processing.h is ~+81, to net_processing.cpp is ~+268,-130. Is the benefit worth the cost?
• Is there a clearer design pattern that works?
• Could the interface be somewhere else and more general so other callers may use it?
• Is there ongoing work in kernel that can address the same problem?
• Is there a way to also reduce cs_main locking at the same time?
• Abstracting away m_chainman but not m_mempool is a bit weird for a fuzz test since the mempool will always be empty yet net_processing may think that a transaction should exist in the mempool based on ProcessTransaction. Should we abstract both?

Rough branch that does this (helped by Claude, though I re-wrote it). The first commit introduces + uses the interface in net_processing, the second is a copy of the process_messages harness that uses FuzzedNetValidation instead of TestingSetup. The interface is a bit weird currently: it has a GetMutex function and it hacks around ProcessNewPackage being a free function accepting a Chainstate. The fuzz test is sparse because I don't know all invariants that need to hold while mocking to avoid a crash.

Inspired by #27502 (comment).

I'm very much in favour of having some form of interface between net processing and validation. I briefly checked what the overlap is between the ChainstateMaanger's public member functions and the member functions you define in the NetValidationInterface. It seems like most functions not called by the interface fall in roughly two categories: Initializers for the chainstate, and functions that allow a Chainstate to reach back to the ChainstateManager. For your goal of mocking validation the former can just be ignored, but the latter may be a bit annoying to deal with. I've been working on eliminating this circular dependency between the Chainstate and ChainstateManager here (didn't quite finish it, and needs a bit of rebasing again). I think this would already get us a good ways closer to having this interface be more or less the public member functions of the ChainstateManager.

Instead of having an adapter/proxy class, could the chainman just inherit from this interface class and override its methods? I could see that making it a bit easier down the line to also intercept and schedule calls to the chainman, i.e. to prioritize data processing over data queries. Thinking even further this could also make splitting up the net processing and validation responsibilities of cs_main much easier by just having to deal with and take into account a subset of validation functions.

Thanks for the feedback @sedited

For your goal of mocking validation the former can just be ignored, but the latter may be a bit annoying to deal with.

The circular dependency definitely threw me off when looking at the code, it's not immediately clear to me how it affects mocking? Can you explain why -- is it because the goal should be to have the public member functions be the interface? If so, that makes sense and then this work would be a prerequisite for an interface.

Instead of having an adapter/proxy class, could the chainman just inherit from this interface class and override its methods?

This also works and I think makes more sense to me than having net_processing coerce chainman into a specific interface. My questions here are:

• where would the interface live? In validation.h or in node/interfaces?
• if something besides net_processing wants to mock calls to chainman, what if it only needs a subset of the interface? is it wasteful for the mock to have to override unused functions? the alternative would be defining multiple sub-interfaces which seems complex and not desirable?

I could see that making it a bit easier down the line to also intercept and schedule calls to the chainman, i.e. to prioritize data processing over data queries.

Just curious, do you think this would be done by modifying one of the chainman's overridden methods or something else?

Thinking even further this could also make splitting up the net processing and validation responsibilities of cs_main much easier by just having to deal with and take into account a subset of validation functions.

This seems like a nice win independent of mocking.

t because the goal should be to have the public member functions be the interface? If so, that makes sense and then this work would be a prerequisite for an interface.

Yes, that was what I was getting at. I'm not sure if this really makes sense as a true prerequisite, but I think that is what we should strive for architecturally.

This also works and I think makes more sense to me than having net_processing coerce chainman into a specific interface. My questions here are: where would the interface live? In validation.h or in node/interfaces?

I think if this is baked into the chainman, it could make sense to put it in the kernel/ subdirectory for now.

if something besides net_processing wants to mock calls to chainman, what if it only needs a subset of the interface? is it wasteful for the mock to have to override unused functions? the alternative would be defining multiple sub-interfaces which seems complex and not desirable?

I think besides just stubbing out all the unwanted fnuctions, we don't have a clean solution for that anyway. An example of thi is the CCoinsViewEmpty class that just contains stubs. My impression is that while this might be a bit clumsy to deal with sometimes, it is also not a particularly evil or dangerous pattern.

Just curious, do you think this would be done by modifying one of the chainman's overridden methods or something else?

I was more thinking of implementing a dispatcher proxy that also inherits from the same interface and then gets wired to the chainman. In this case, net processing would call this dispatcher that then calls to the actual chainman.

I'm not sure if this really makes sense as a true prerequisite, but I think that is what we should strive for architecturally.

I might have been unclear, by this I meant that removing the circular dependency seemed to me to be a prerequisite, and not that the public member functions needed to match the interface exactly. The interface could be written right now, but I think it's probably premature -- I think once the circular dependency is removed, then hopefully we'd have a better idea of some of the other public member functions that could be pruned away. There is some weirdness in my code where I was forced to call chainstate & blockman functions through chainman, but it's not clear to me how to fix that.

My impression is that while this might be a bit clumsy to deal with sometimes, it is also not a particularly evil or dangerous pattern.

This was the only downside I can think of, which doesn't seem like a big deal.

I was more thinking of implementing a dispatcher proxy that also inherits from the same interface and then gets wired to the chainman. In this case, net processing would call this dispatcher that then calls to the actual chainman.

Gotcha, makes sense. @ajtowns mentioned to me out-of-band his comment that reminds me of this. I haven't thought too much about it. It would be cool if the interface allowed more customization for net_processing.

More generally, I like having the chainman inherit from this interface, pruning away the member functions if possible, and I like removing the circular dependency between chainman and chainstate. I'd like to get broader buy-in from other folks, but this seems like a good approach and I can't think of any downsides (other than the review burden perhaps)?