validationinterface: add unused CChainState parameter #16487

This is part of the assumeutxo project:

Parent PR: #15606 Issue: #15605 Specification: https://github.com/jamesob/assumeutxo-docs/tree/master/proposal

Once we have multiple chainstates doing their thing simultaneously, we'll need to allow CValidationInterface clients to determine which chainstate a given event pertains to so that they can respond accordingly; for example, a wallet client should respond differently to BlockConnected events for the background validation chain vs. the active chain.

This commit adds a (for now unused) parameter to allow VI clients to determine which chainstate they're being notified about.

Could add a test to make it technically in use? Perhaps with a comment describing that the parameter is intentionally (and temporarily) unused outside of tests.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #16324 (Get cs_main out of the critical path in ProcessMessages by TheBlueMatt)
• #16323 (Call ProcessNewBlock() asynchronously by TheBlueMatt)
• #16279 (Return the AcceptBlock CValidationState directly in ProcessNewBlock by TheBlueMatt)
• #15931 (Remove GetDepthInMainChain dependency on locked chain interface by ariard)
• #14384 (Fire TransactionRemovedFromMempool callbacks from mempool by l2a5b1)
• #13686 (ZMQ: Small cleanups in the ZMQ code by domob1812)
• #9849 (Qt: Network Watch tool by luke-jr)

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.

The current appveyor errors are really brutal, they don't even tell you what source file they come from

c:\program files (x86)\microsoft visual studio\2017\community\vc\tools\msvc\14.16.27023\include\type_traits(912): error C2139: 'CChainState': an undefined class is not allowed as an argument to compiler intrinsic type trait '__is_assignable' [C:\projects\bitcoin\build_msvc\libbitcoin_server\libbitcoin_server.vcxproj]
c:\program files (x86)\microsoft visual studio\2017\community\vc\tools\msvc\14.16.27023\include\type_traits(901): error C2139: 'CChainState': an undefined class is not allowed as an argument to compiler intrinsic type trait '__is_assignable' [C:\projects\bitcoin\build_msvc\libbitcoin_server\libbitcoin_server.vcxproj]

@MarcoFalke, maybe we should consider removing the /v:q option from the appveyor config? (I'm guessing it controls verbosity)?

https://github.com/bitcoin/bitcoin/blob/74f1a27f2f45af7dafcc34df766cf76d29c7c6ed/.appveyor.yml#L36

We could experiment with removing /v:q in this PR to see if that helps.

cc @sipsorcery

utACK fa47a73330199758d8c7b7e1ad6ab209276d54c3 ignoring the msvc error. I will say I like the general direction of this change if only because it is sharing state though explicitly passed parameters, which should make dependencies between parts of bitcoin code obvious instead of masked though use of global variables.

utACK fa47a73 on codes changes. On the more general way to let chain clients select which chainstate they want to follow I don't know. For a wallet dealing correctly with its transactions for one chainstate is hard enough, now if it has to filter them, that's need more thought... I find nice that CValidationInterface gives to its clients the "authoritarian-but-not-definitive" view of the blockchain with still reorgs to manage but already less overhead. We can't exclude monitor/analysis tools which may be interested to keep an eye on tip while collecting data on IBD. Wandering if clients shouldn't choose at RegisterValidationInterface which chainstate they subscribe. And call it multiple times if they want both background and foreground.

I'm really not a fan of this. Specifically, it seems to be muddling chain sync state outside of validation. assumeutxo is very, very tied to validation process and logic, and I think should be (as much as possible, though admittedly that may not be much) unobservable outside of validation.cpp. Long-long-long term, I want validation.h to be something that ends up looking like libconsensus, but this is a step towards the "driver" thereof deciding how to connect what block to where.

For a wallet dealing correctly with its transactions for one chainstate is hard enough, now if it has to filter them, that's need more thought...

I think for now assumeutxo is just supposed to be useful for new wallets (or wallets with birthdays after the utxo snapshot time), so these wallets should be ok with not seeing notifications from the background sync chain.

assumeutxo is very, very tied to validation process and logic, and I think should be (as much as possible, though admittedly that may not be much) unobservable outside of validation.cpp

I don't think I understand what this is envisioning. If the point is to download the more useful blocks first, then go get the less useful blocks, instead of downloading all the blocks in order, it kind of defeats the purpose if you don't tell anyone about the more useful blocks.

When you were talking about this earlier I thought you were just objecting to a full CChainState reference being passed around rather than an enum or something. But this now sounds like even an enum would be exposing too much information.

It'd probably be good to say how you think assumeutxo could be implemented in a more acceptable way, or be more specific about the problems the current design causes or could cause.

I've submitted #16505 to increase the msbuild verbosity in the appveyor script.

In the meantime additional log messages for the 2 msbuild errors in this PR:

1>------ Build started: Project: libbitcoin_server, Configuration: Debug x64 ------
1>validationinterface.cpp
1>c:\program files (x86)\microsoft visual studio\2017\community\vc\tools\msvc\14.16.27023\include\type_traits(912): error C2139: 'CChainState': an undefined class is not allowed as an argument to compiler intrinsic type trait '__is_assignable'
1>c:\dev\github\bitcoin\src\validationinterface.h(16): note: see declaration of 'CChainState'
1>c:\dev\github\vcpkg\installed\x64-windows-static\include\boost\signals2\detail\variadic_slot_invoker.hpp(138): note: see reference to class template instantiation 'std::is_move_assignable<_This>' being compiled
1>        with
1>        [
1>            _This=const CChainState &
1>        ]
1>c:\program files (x86)\microsoft visual studio\2017\community\vc\tools\msvc\14.16.27023\include\type_traits(912): note: see reference to variable template 'const bool conjunction_v<std::is_move_assignable<CChainState const &>,std::is_move_assignable<CBlockIndex const * &>,std::is_move_assignable<std::shared_ptr<CBlock const > const &> >' being compiled
1>c:\dev\github\vcpkg\installed\x64-windows-static\include\boost\signals2\detail\signal_template.hpp(239): note: see reference to class template instantiation 'boost::signals2::detail::variadic_slot_invoker<boost::signals2::detail::void_type,const CChainState &,const CBlockIndex *,const std::shared_ptr<const CBlock> &>' being compiled
1>c:\dev\github\vcpkg\installed\x64-windows-static\include\boost\signals2\detail\signal_template.hpp(226): note: while compiling class template member function 'void boost::signals2::detail::signal_impl<R (const CChainState &,const CBlockIndex *,const std::shared_ptr<const CBlock> &),Combiner,Group,GroupCompare,SlotFunction,ExtendedSlotFunction,Mutex>::operator ()(const CChainState &,const CBlockIndex *,const std::shared_ptr<const CBlock> &)'
1>        with
1>        [
1>            R=void,
1>            Combiner=boost::signals2::optional_last_value<void>,
1>            Group=int,
1>            GroupCompare=std::less<int>,
1>            SlotFunction=boost::function<void (const CChainState &,const CBlockIndex *,const std::shared_ptr<const CBlock> &)>,
1>            ExtendedSlotFunction=boost::function<void (const boost::signals2::connection &,const CChainState &,const CBlockIndex *,const std::shared_ptr<const CBlock> &)>,
1>            Mutex=boost::signals2::mutex
1>        ]
1>c:\dev\github\vcpkg\installed\x64-windows-static\include\boost\signals2\detail\signal_template.hpp(722): note: see reference to function template instantiation 'void boost::signals2::detail::signal_impl<R (const CChainState &,const CBlockIndex *,const std::shared_ptr<const CBlock> &),Combiner,Group,GroupCompare,SlotFunction,ExtendedSlotFunction,Mutex>::operator ()(const CChainState &,const CBlockIndex *,const std::shared_ptr<const CBlock> &)' being compiled
1>        with
1>        [
1>            R=void,
1>            Combiner=boost::signals2::optional_last_value<void>,
1>            Group=int,
1>            GroupCompare=std::less<int>,
1>            SlotFunction=boost::function<void (const CChainState &,const CBlockIndex *,const std::shared_ptr<const CBlock> &)>,
1>            ExtendedSlotFunction=boost::function<void (const boost::signals2::connection &,const CChainState &,const CBlockIndex *,const std::shared_ptr<const CBlock> &)>,
1>            Mutex=boost::signals2::mutex
1>        ]
1>c:\dev\github\vcpkg\installed\x64-windows-static\include\boost\signals2\detail\signal_template.hpp(613): note: see reference to class template instantiation 'boost::signals2::detail::signal_impl<R (const CChainState &,const CBlockIndex *,const std::shared_ptr<const CBlock> &),Combiner,Group,GroupCompare,SlotFunction,ExtendedSlotFunction,Mutex>' being compiled
1>        with
1>        [
1>            R=void,
1>            Combiner=boost::signals2::optional_last_value<void>,
1>            Group=int,
1>            GroupCompare=std::less<int>,
1>            SlotFunction=boost::function<void (const CChainState &,const CBlockIndex *,const std::shared_ptr<const CBlock> &)>,
1>            ExtendedSlotFunction=boost::function<void (const boost::signals2::connection &,const CChainState &,const CBlockIndex *,const std::shared_ptr<const CBlock> &)>,
1>            Mutex=boost::signals2::mutex
1>        ]
1>c:\dev\github\bitcoin\src\validationinterface.cpp(38): note: see reference to class template instantiation 'boost::signals2::signal<void (const CChainState &,const CBlockIndex *,const std::shared_ptr<const CBlock> &),boost::signals2::optional_last_value<void>,int,std::less<int>,boost::function<Signature>,boost::function<R (const boost::signals2::connection &,const CChainState &,const CBlockIndex *,const std::shared_ptr<const CBlock> &)>,boost::signals2::mutex>' being compiled
1>        with
1>        [
1>            Signature=void (const CChainState &,const CBlockIndex *,const std::shared_ptr<const CBlock> &),
1>            R=void
1>        ]
1>c:\dev\github\vcpkg\installed\x64-windows-static\include\boost\bind\placeholders.hpp(54): note: see reference to class template instantiation 'boost::arg<9>' being compiled
1>c:\dev\github\vcpkg\installed\x64-windows-static\include\boost\bind\placeholders.hpp(53): note: see reference to class template instantiation 'boost::arg<8>' being compiled
1>c:\dev\github\vcpkg\installed\x64-windows-static\include\boost\bind\placeholders.hpp(52): note: see reference to class template instantiation 'boost::arg<7>' being compiled
1>c:\dev\github\vcpkg\installed\x64-windows-static\include\boost\bind\placeholders.hpp(51): note: see reference to class template instantiation 'boost::arg<6>' being compiled
1>c:\dev\github\vcpkg\installed\x64-windows-static\include\boost\bind\placeholders.hpp(50): note: see reference to class template instantiation 'boost::arg<5>' being compiled
1>c:\dev\github\vcpkg\installed\x64-windows-static\include\boost\bind\placeholders.hpp(49): note: see reference to class template instantiation 'boost::arg<4>' being compiled
1>c:\dev\github\vcpkg\installed\x64-windows-static\include\boost\bind\placeholders.hpp(48): note: see reference to class template instantiation 'boost::arg<3>' being compiled
1>c:\dev\github\vcpkg\installed\x64-windows-static\include\boost\bind\placeholders.hpp(47): note: see reference to class template instantiation 'boost::arg<2>' being compiled
1>c:\dev\github\vcpkg\installed\x64-windows-static\include\boost\bind\placeholders.hpp(46): note: see reference to class template instantiation 'boost::arg<1>' being compiled
1>c:\program files (x86)\microsoft visual studio\2017\community\vc\tools\msvc\14.16.27023\include\type_traits(901): error C2139: 'CChainState': an undefined class is not allowed as an argument to compiler intrinsic type trait '__is_assignable'
1>c:\dev\github\bitcoin\src\validationinterface.h(16): note: see declaration of 'CChainState'
1>c:\dev\github\vcpkg\installed\x64-windows-static\include\boost\signals2\detail\variadic_slot_invoker.hpp(138): note: see reference to class template instantiation 'std::is_copy_assignable<_This>' being compiled
1>        with
1>        [
1>            _This=const CChainState &
1>        ]
1>c:\program files (x86)\microsoft visual studio\2017\community\vc\tools\msvc\14.16.27023\include\type_traits(901): note: see reference to variable template 'const bool conjunction_v<std::is_copy_assignable<CChainState const &>,std::is_copy_assignable<CBlockIndex const * &>,std::is_copy_assignable<std::shared_ptr<CBlock const > const &> >' being compiled
1>Done building project "libbitcoin_server.vcxproj" -- FAILED.
========== Build: 0 succeeded, 1 failed, 18 up-to-date, 0 skipped ==========

Including validation.h in validationinterface.cpp fixes the msbuild errors. That makes the concrete CChainState implementation available rather than only a forward declaration. I'm not familiar enough with this part of the code to know whether that will cause other side effects.

Once we have multiple chainstates doing their thing simultaneously, we'll need to allow CValidationInterface clients to determine which chainstate a given event pertains to so that they can respond accordingly; for example, a wallet client should respond differently to BlockConnected events for the background validation chain vs. the active chain.

I don't understand why this is needed. The wallet should never even receive the notifications for the background validation.

Can you explain a bit more when a chainstate different from the active one is passed through here? And which listeners are supposed to receive it?

I'd presume the answers are:

• When a block is connected on the background chain
• Only net_processing needs to know about the event

So to me it seems simpler, less code, and less computational complexity at listeners, to just add a single new connection for BackroundBlockConnected (or none at all if it is not needed), no?

re: #16487#pullrequestreview-268744787

Wandering if clients shouldn't choose at RegisterValidationInterface which chainstate they subscribe. And call it multiple times if they want both background and foreground.

I like this suggestion, and suspect it'd slightly simplify the overall assumeutxo implementation by avoiding the need to add new parameters to various validationinterface notifications. It's actually pretty similar to Marco's suggestion of adding a separate background BlockConnected notification (though I think it's a little more future proof in case clients start also being interested in background ChainStateFlushed events or whatever).

re: #16487 (comment)

assumeutxo is very, very tied to validation process and logic, and I think should be (as much as possible, though admittedly that may not be much) unobservable outside of validation.cpp

Turns out this is overstated and Matt is ok with exposing existence of multiple chains as long as they are referenced by enums instead of full CChainState objects.

re: #16487 (comment)

Thanks for the debug help sipsorcery! And for confirming a simple #include fixes this.

re: #16487 (comment)

The wallet should never even receive the notifications for the background validation.

I don't think this is necessarily true, though maybe it's true in the near term. I could imagine the wallet being updated to process the utxo snapshot and show balances right away, and then backfilling actual transaction history from background notifications. I believe Jonas Schnelli implemented something like this a while ago with his "no validation" wallet transactions in #9076.

My previous ACK is still valid, by the way. The decision to use enums or references to indicate chain state, and the decision whether to add a parameter to existing callbacks, or add a new callback for background blocks, or to not change any callbacks but add a chain argument to the register function all seem skin-deep. I do not see how these decisions would have any significant impact on future code added to bitcoin or hypothetical code written to use libconsensus.

Thanks to everyone for the thoughts. I think I'm going to try the approach suggested by @MarcoFalke, since I think it'll reduce the diff by at least 40 lines or so. After reviewing the parent PR (#15606) I've realized that the majority of validationinterface clients just ignore the background chainstate, and so introducing a BackgroundBlockConnected() event should be a less intrusive way of doing what this PR was intended to prepare for.

Closing for now since the alternate approaches suggested will mostly exclude these changes.

Don't forget to remove this from the "In progress" column: https://github.com/bitcoin/bitcoin/projects/11