wip: Only support shared validation interfaces #18791

1. 
   promag commented at 11:41 pm on April 27, 2020: member
2. wip: Only support shared validation interfaces df4c642714
3. fanquake added the label Validation on Apr 28, 2020
4. 
   ryanofsky commented at 0:34 am on April 28, 2020: member

   I’m not sure this change is a good idea. I think it made sense to add shared_ptr support to the validation interface in #18338 because it only required 4 new lines of code in validationinterface.cpp, and because without that support it would have been more complicated to unload wallets safely, given the wallet was already using shared_ptr to share CWallet references between vpwallet vector, active RPC calls, and the GUI.

   But I don’t think the non-shared validation interface should be removed just because a shared_ptr interface is available. Or if this change is a good idea, we should figure out exactly what the goal is. I don’t think when we first switched to shared_ptr in the wallet we intended it to spread virally to unrelated classes like BaseIndex and PeerLogicValidation

5. 
   promag commented at 0:45 am on April 28, 2020: member

   @ryanofsky I’m on the same page. I’ve submitted this for 2 reasons:

   • address same issue as #18742
   • show the extent of the change

   I’ll leave this open so others can weight in too.

6. 
   ryanofsky commented at 0:51 am on April 28, 2020: member

   Makes sense. This is good as an experiment to uncover other possible use-after-free bugs, and see what impact of removing the non-shared functions would be
7. 
   DrahtBot commented at 4:20 am on April 28, 2020: member

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

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #18742 (miner: Avoid stack-use-after-return in validationinterface by MarcoFalke)
   • #18531 (rpc: Assert that RPCArg names are equal to CRPCCommand ones by MarcoFalke)
   • #13686 (ZMQ: Small cleanups in the ZMQ code by domob1812)

   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.

8. 
   MarcoFalke commented at 11:41 am on April 28, 2020: member

   The normal shutdown sequence stops and unregisters the interfaces before stopping them, so this shouldn’t be an issue.
9. promag closed this on May 13, 2020

   ---

10. promag deleted the branch on May 13, 2020
11. DrahtBot locked this on Feb 15, 2022

Contributors
promag ryanofsky DrahtBot MarcoFalke

Labels
Validation