kernel: Remove util/system from kernel library, interface

TheCharlatan commented at 8:03 am on May 12, 2023: contributor

This pull request is part of the libbitcoinkernel project #27587 https://github.com/bitcoin/bitcoin/projects/18 and more specifically its “Step 2: Decouple most non-consensus code from libbitcoinkernel”.

It removes the kernel library’s dependency on util/system and interface_ui. util/system contains networking and shell-related code that should not be part of the kernel library. The following pull requests prepared util/system for this final step: #27419 https://github.com/bitcoin/bitcoin/pull/27254 #27238.

interface_ui defines functions for a more general node interface and has a dependency on boost/signals2. After applying the patches from this pull request, the kernel’s reliance on boost is down to boost::multiindex.

The approach implemented here introduces some indirection, which makes the code a bit harder to read. Any suggestions for improving or reworking this pull request to make it more concise, or even reworking it into a more proper interface, are appreciated.

DrahtBot commented at 8:03 am on May 12, 2023: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	stickies-v, MarcoFalke, hebasto
Stale ACK	ryanofsky

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#27607 (index: improve initialization and pruning violation check by furszy)
#27596 (assumeutxo (2) by jamesob)
#27460 (rpc: Add importmempool RPC by MarcoFalke)
#27331 (refactor: extract CCheckQueue’s data handling into a separate container “Bag” by martinus)
#26762 (refactor: Make CCheckQueue RAII-styled by hebasto)
#19461 (multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky)
#19460 (multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky)

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.

DrahtBot added the label Validation on May 12, 2023

fanquake requested review from ryanofsky on May 12, 2023

fanquake requested review from theuni on May 12, 2023

in src/kernel/chainstatemanager_opts.h:39 in 0b159b9256 outdated

34+    const std::function<void(SynchronizationState state, CBlockIndex* index)> notify_block_tip;
35+    const std::function<void(SynchronizationState state, int64_t height, int64_t timestamp, bool presync)> notify_header_tip;
36+    const std::function<void(const std::string& title, int nProgress, bool resume_possible)> show_progress;
37+    const std::function<void(const bilingual_str& warning)> do_warning;
38+    const std::function<void(const bilingual_str& user_message)> init_error;
39+};

ryanofsky commented at 11:49 am on May 12, 2023:

It seems like it would be more natural to write this as:

0class ChainstateNotifications
1{
2    virtual void NotifyBlockTip(SynchronizationState state, CBlockIndex* index) {}
3    virtual void NotifyHeaderTip(SynchronizationState state, int64_t height, int64_t timestamp, bool presync) {}
4    virtual void ShowProgress(const std::string& title, int nProgress, bool resume_possible) {}
5    virtual void DoWarning(const bilingual_str& warning) {}
6    virtual void InitError(const bilingual_str& user_message) {}
7};

in src/node/blockstorage.cpp:530 in 0b159b9256 outdated

526@@ -527,7 +527,7 @@ void BlockManager::FlushUndoFile(int block_file, bool finalize)
527 {
528     FlatFilePos undo_pos_old(block_file, m_blockfile_info[block_file].nUndoSize);
529     if (!UndoFileSeq().Flush(undo_pos_old, finalize)) {
530-        AbortNode("Flushing undo file to disk failed. This is likely the result of an I/O error.");
531+        AbortNode("Flushing undo file to disk failed. This is likely the result of an I/O error.", m_opts.init_error_callback);

ryanofsky commented at 12:18 pm on May 12, 2023:

These new AbortNode calls seem more awkward. And they aren’t really future-proof since AbortNode calls the global StartShutdown function which is specific to the node and will have to be changed for the kernel. Also the name “abort node” doesn’t make sense in the kernel context if the kernel application isn’t a bitcoin node. Would suggest adding a FatalError notification to complement InitError in ChainstateNotifications class suggested previously:

0class ChainstateNotifications
1{
2    ...
3    virtual void FatalError(const bilingual_str& error_message, const bilingual_str abort_message={}) {}
4};

Other kernel clients could customize FatalError but the node implementation would call SetMiscWarning(error_message) and InitError(abort_message or "A fatal internal error occurred, see debug.log for details") and StartShutdown() like AbortNode function does currently, to keep behavior unchanged.

in src/node/chainstatemanager_notifications.h:11 in 0b159b9256 outdated

 6+#define BITCOIN_NODE_CHAINSTATEMANAGER_NOTIFICATIONS_H
 7+
 8+#include <kernel/chainstatemanager_opts.h>
 9+
10+namespace node {
11+kernel::ChainstateManagerNotificationCallbacks DefaultChainstateManagerNotifications();

ryanofsky commented at 12:23 pm on May 12, 2023:

I don’t think I would tie these notifications to the ChainStateManager class specifically as they really make sense anywhere in validation code. Would suggest changing chainstatemanager_notifications to chainstate_notifications, or validation_notifications and dropping the “manager” in these places

TheCharlatan commented at 7:08 pm on May 12, 2023:

Would you also prefer passing this validation_notifications class to the other users of these notification functions, even if they only use one of the methods?

ryanofsky commented at 8:46 pm on May 12, 2023:

Yes, I think that’s preferable. Just pass const ChainstateNotifications& everywhere and don’t have a lot of different awkward notification types

ryanofsky approved

ryanofsky commented at 12:28 pm on May 12, 2023: contributor

Approach ACK. I think adding the level of indirection here is right approach, but I left some suggestions below which I think would make the implementation less awkward and more future-proof

Aminkavoos approved

TheCharlatan force-pushed on May 13, 2023

TheCharlatan commented at 5:24 pm on May 13, 2023: contributor

Thank you for the suggestions @ryanofsky!

Updated 0b159b9256c214a289135ca7bfaf70e48ab28e20 -> 63e915af5a437d36abdd79a1b90fed2395086589 (chainstateRmKernelUiInterface_0 -> chainstateRmKernelUiInterface_1, compare).

Re-worked the commits as suggested by @ryanofsky’s comment by changing the approach from passing around lambdas to using polymorphisms of a virtual class.
Addressed @ryanofsky’s comment, renaming chainstatemanager_notifications to validation_notifications.
Re-worked the last commit as suggested by @ryanofsky’s comment, by moving away from wrapping InitError to implementing a FatalError method, replacing the AbortNode function in shutdown. This is also a similar approach to the existing FatalErrorf method in index/base.cpp
Added a commit renaming FatalError to FatalErrorf in index/base.cpp to better distinguish it from the new FatalError function and not let the format string linter trip up on it.
Made the ChainstateManagerOpts notification member a shared_ptr in order to allow the option struct to be safely re-used for multiple ChainstateManagers

hebasto commented at 10:53 am on May 14, 2023: member

Concept ACK.

hebasto commented at 11:42 am on May 14, 2023: member

Approach ACK 63e915af5a437d36abdd79a1b90fed2395086589.

The approach implemented here introduces some indirection, which makes the code a bit harder to read.

As for me, it does not look like a problem at all. Good function naming works well :D

In commit 73556797ab9c44038994954b945db996642656d2:

 0--- a/src/validation.cpp
 1+++ b/src/validation.cpp
 2@@ -4659,7 +4659,7 @@ void Chainstate::LoadExternalBlockFile(
 3                     }
 4                 }
 5 
 6-                NotifyHeaderTip(*this);
 7+                ::NotifyHeaderTip(*this);
 8 
 9                 if (!blocks_with_unknown_parent) continue;
10

Also mind considering the IWYU output regarding src/node/validation_notifications.{h,cpp}?

in src/init.cpp:1039 in 63e915af5a outdated

1035@@ -1034,13 +1036,15 @@ bool AppInitParameterInteraction(const ArgsManager& args, bool use_syscall_sandb
1036         ChainstateManager::Options chainman_opts_dummy{
1037             .chainparams = chainparams,
1038             .datadir = args.GetDataDirNet(),
1039+            .notifications = std::make_shared<ValidationNotificationsImpl>(),

hebasto commented at 11:58 am on May 14, 2023:

Shouldn’t this change be moved to the first commit?

TheCharlatan commented at 12:04 pm on May 14, 2023:

We also don’t set other params, like adjusted_time_callback in the dummy opts, since they are not required to read the args into the struct. The commit where this is changed, adds it because the BlockManager::Options requires a reference to the notifications starting at that commit.

hebasto commented at 12:53 pm on May 14, 2023:

FWIW, gcc emits a warning here.

TheCharlatan commented at 1:23 pm on May 14, 2023:

Thank you for following up, will patch.

TheCharlatan commented at 1:40 pm on May 14, 2023: contributor

Re #27636#pullrequestreview-1425535676

In commit 7355679:

 0--- a/src/validation.cpp
 1+++ b/src/validation.cpp
 2@@ -4659,7 +4659,7 @@ void Chainstate::LoadExternalBlockFile(
 3                     }
 4                 }
 5 
 6-                NotifyHeaderTip(*this);
 7+                ::NotifyHeaderTip(*this);
 8 
 9                 if (!blocks_with_unknown_parent) continue;
10

I added the global scope resolution where it was required, because of the naming conflict with the ChainstateManager method. These here are methods of Chainstate and don’t have a naming conflict. Is adding them good practice for consistency’s sake?

in src/kernel/validation_notifications_interface.h:31 in 63e915af5a outdated

26+
27+    virtual void notifyBlockTip(SynchronizationState state, CBlockIndex* index) = 0;
28+    virtual void notifyHeaderTip(SynchronizationState state, int64_t height, int64_t timestamp, bool presync) = 0;
29+    virtual void showProgress(const std::string& title, int nProgress, bool resume_possible) = 0;
30+    virtual void doWarning(const bilingual_str& warning) = 0;
31+    virtual bool fatalError(const std::string& strMessage, bilingual_str user_message = {}) = 0;

hebasto commented at 2:52 pm on May 14, 2023:

Should these methods be non-const?

TheCharlatan commented at 3:23 pm on May 14, 2023:

Yes, as far as I am aware, const virtual methods are bad practice. See: #25337 (review).

ryanofsky commented at 1:07 pm on May 15, 2023:

In PR version 2f9c2d245360b3fad6d469a76c2916d75b027b57:

This is a notifications interface, so I don’t think these methods should be = 0 pure virtual. If you defined them to do nothing by default {} that should make the notification interface easier to use because you don’t have to implement handlers for notifications you do not care about. Also it can make the interface more future proof because adding new notification types will not break existing code.

in src/kernel/validation_notifications_interface.h:35 in 63e915af5a outdated

30+    virtual void doWarning(const bilingual_str& warning) = 0;
31+    virtual bool fatalError(const std::string& strMessage, bilingual_str user_message = {}) = 0;
32+};
33+} // namespace kernel
34+
35+#endif // BITCOIN_KERNEL_VALIDATION_NOTIFICATIONS_INTERFACE_H

hebasto commented at 2:52 pm on May 14, 2023:

nit: EOL?

TheCharlatan force-pushed on May 14, 2023

TheCharlatan commented at 3:35 pm on May 14, 2023: contributor

Thank you for the review @hebasto,

Updated 63e915af5a437d36abdd79a1b90fed2395086589 -> 97b0e28d6f5e86e8c7ee0775d22cd923dabca09d (chainstateRmKernelUiInterface_1 -> chainstateRmKernelUiInterface_2, compare).

Addressed @hebasto comment, adding EOL.
Addressed @hebasto comment, correcting initialization warning in the first commit.
Addressed @hebasto comment, correcting IWYU in last commit.
Slightly improved commit messages (s/option/options/).

hebasto commented at 4:42 pm on May 14, 2023: member

Approach ACK 97b0e28d6f5e86e8c7ee0775d22cd923dabca09d.

In d84952cd64bd03ce444bbf6077d060c92526d040 commit message: s/“to FatalError”/“to FatalErrorf”/

Maybe make it a scripted-diff?

hebasto commented at 4:53 pm on May 14, 2023: member

nit: https://api.cirrus-ci.com/v1/task/4542005133443072/logs/ci.log:

0node/validation_notifications.h should add these lines:
1enum class SynchronizationState;

0node/validation_notifications.cpp should add these lines:
1enum class SynchronizationState;
2
3node/validation_notifications.cpp should remove these lines:
4- #include <kernel/validation_notifications_interface.h>

TheCharlatan force-pushed on May 14, 2023

TheCharlatan commented at 5:59 pm on May 14, 2023: contributor

Thank you for the review @hebasto,

Updated 97b0e28d6f5e86e8c7ee0775d22cd923dabca09d -> 2f9c2d245360b3fad6d469a76c2916d75b027b57 (chainstateRmKernelUiInterface_2 -> chainstateRmKernelUiInterface_3, compare).

Addressed @hebasto comment, changed to scripted diff and fixed commit message.
Addressed @hebasto comment, fixed IWYU (sorry, I had an outdated version installed locally).

in src/kernel/chainstatemanager_opts.h:47 in 620faa21ce outdated

43@@ -42,6 +44,7 @@ struct ChainstateManagerOpts {
44     DBOptions block_tree_db{};
45     DBOptions coins_db{};
46     CoinsViewOptions coins_view{};
47+    const std::shared_ptr<ValidationNotifications> notifications;

ryanofsky commented at 1:39 pm on May 15, 2023:

In commit “refactor: Pass BlockNotify in options to ChainstateManager” (620faa21ce0210dddca243511b7674c3b8f4e901)

Would suggest ValidationNotifications& notifications or ValdiataionNotifications* notifcations{nullptr} here, and adding a std::unique_ptr<node::ValidationNotificationsImpl> member to NodeContext. I don’t think this should be a shared pointer and also don’t think it should be const.

On const: This is an options struct that is supposed to be filled with options set by external applications. We don’t know where those options are coming from. The might be hardcoded or coming from the environment or user configuration. Make struct members const just makes the struct more awkward to fill out and harder for application code to be written in a modular way since it requires the option values to be known in advance of creating the struct. Generally it would be good to avoid const members in options structs where where possible. Having const struct members can also be bad for efficiency because it can make the struct impossible to move from (though it doesn’t matter in this case because moving is not much more efficient than copying for this type).

On shared_ptr: I think shared_ptr semantics are more complicated than what we want here. The other places that use shared_ptr for notifications (in RegisterSharedValidationInterface and interfaces::Chain::handleNotifications) use it because wallets can be loaded and unloaded at any time, and using shared_ptr lets wallets register for notifications and get unloaded and deleted without having to wait for unregister call (i.e. it lets the unregister function be non-blocking). But there should be no need for that complexity here. Kernel applications should be able to provide a notifications callback that is as long lived as the validation code is and not have complicated lifetimes.

Adding a std::unique_ptr<node::ValidationNotificationsImpl> member to NodeContext would also be good for our own code to let us stop relying on the uiInterface global to receive notifications in the future.

TheCharlatan commented at 5:37 pm on May 15, 2023:

Thank you for the time you took for these detailed comments. Fleshing this out to a more proper kernel interface sounds good to me. How about moving ValidationNotifications to the KernelContext instead? Or does that conflict with the vision you have for getting rid of uiInterface global?

ryanofsky commented at 5:52 pm on May 15, 2023:

Thank you for the time you took for these detailed comments. Fleshing this out to a more proper kernel interface sounds good to me. How about moving ValidationNotifications to the KernelContext instead? Or does that conflict with the vision you have for getting rid of uiInterface global?

Hmm, It doesn’t make sense to me that an object which is supposed to receive notifications from the kernel would be owned by the kernel. Outside code that wants to receive notifications should be able to receive them and manage its own memory without transferring ownership.

Also yes the NotificationImpl can basically be a replacement for the uiInterface global, so it makes sense to put it in NodeContext or some place similar. It would be weird to put in the kernel and have to static_cast it when we can just store it directly in NodeContext.

hebasto commented at 3:48 pm on May 19, 2023:

Would suggest ValidationNotifications& notifications or ValdiataionNotifications* notifcations{nullptr} here,

I’d prefer the pointer type to do not bother about lifetime of variables that were used for assigning.

ryanofsky commented at 4:34 pm on May 19, 2023:

I’d prefer the pointer type to do not bother about lifetime of variables that were used for assigning.

I’d agree with you if we passing a string or some other passive data structure as an option to the kernel. Better to just use copy/move/reference counts to get the options struct to manage the memory rather than worry about lifetime of external variables.

But kernel::Notifications option is not just a passive set-and-forget option because it actively receives callbacks from the kernel. It will very likely own resources and contain pointers to other state. (Our KernelNotifications implementation will need to hold more state to get rid of uiInterface and fWarned globals.) I think it’s important for lifetime of the object to be explicit and not use copies/movies/reference counts so related object lifetimes can also be known and fixed, and we don’t make memory management harder to debug in the long term just to avoid thinking about it in the short term.

in src/validation.cpp:3547 in 620faa21ce outdated

3567@@ -3568,6 +3568,11 @@ bool CheckBlock(const CBlock& block, BlockValidationState& state, const Consensu
3568     return true;
3569 }
3570 
3571+void ChainstateManager::NotifyBlockTip(SynchronizationState state, CBlockIndex* index) const
3572+{
3573+    return m_options.notifications->notifyBlockTip(state, index);
3574+}

ryanofsky commented at 1:57 pm on May 15, 2023:

In commit “refactor: Pass BlockNotify in options to ChainstateManager” (620faa21ce0210dddca243511b7674c3b8f4e901)

I don’t see the point of having one line ChainstateManager wrapper methods for all the notifications. It seems like they just make the call sequence harder to follow, make code hard coder to grep, and create name conflicts with other functions that have to be overcome with :: specifiers and the FatalErrorf renaming.

The one benefit of these wrapper methods is that they reduce some verbosity at call sites and avoid the need to type m_options.notifications everywhere. But I think think adding a simple accessor kernel::ValidationNotifications& notifications() const { return *Assert(m_options.notifications); } would be a just as good for that purpose.

EDIT: I see an upcoming commit adds a GetNotifications() method, so I think calls to these wrapper methods could just be replaced by calls to that method, and these wrappers could go away.

in src/node/validation_notifications.cpp:13 in 620faa21ce outdated

 8+
 9+namespace node {
10+
11+void ValidationNotificationsImpl::notifyBlockTip(SynchronizationState state, CBlockIndex* index)
12+{
13+    uiInterface.NotifyBlockTip(state, index);

ryanofsky commented at 2:12 pm on May 15, 2023:

In commit “refactor: Pass BlockNotify in options to ChainstateManager” (620faa21ce0210dddca243511b7674c3b8f4e901)

Note: After this commit, there is only one line of code sending the uiInterface.NotifyBlockTip global boost signal, so it should be easier to drop the signal, probably replacing it with a std::list<NotifyHeaderTipFn> or similar member. Other boost signals could be eliminated after this in a similar way.

in src/bitcoin-chainstate.cpp:97 in 8fe776b303 outdated

93@@ -93,6 +94,10 @@ int main(int argc, char* argv[])
94         {
95             std::cout << "Header tip changed: " << height << ", " << timestamp << ", " << presync << std::endl;
96         }
97+        void showProgress(const std::string& title, int nProgress, bool resume_possible) override

ryanofsky commented at 2:40 pm on May 15, 2023:

In commit “refactor: Pass ShowProgress in options to ChainstateManager” (8fe776b303dddf9b3f1381d263bba6c913591dde)

Would encourage using modern variable names in new code, especially if adding an API that is supposed to be part of the kernel interface. Specifically would suggest renaming nProgress to progress_percent or something like that here.

Also since this is a kernel API and we don’t know what the application UI might be, maybe calling it “notify progress” might be better than “show progress” and more consistent with other names.

in src/bitcoin-chainstate.cpp:101 in bcd417ad18 outdated

 97@@ -98,6 +98,10 @@ int main(int argc, char* argv[])
 98         {
 99             std::cout << "Progress: " << title << ", " << nProgress << ", " << resume_possible << std::endl;
100         }
101+        void doWarning(const bilingual_str& warning) override

ryanofsky commented at 2:46 pm on May 15, 2023:

In commit “refactor: Pass DoWarning in options to ChainstateManager” (bcd417ad1847cd4c93bffbc9d8a32255e7f2b156)

I think “notify warning” would be more descriptive than “do warning” and more consistent with other methods.

Also commit title “Pass DoWarning in options to ChainstateManager” maybe should be updated to something like “Add NotifyWarning kernel notification”

in src/kernel/validation_notifications_interface.h:17 in 620faa21ce outdated

12+
13+/**
14+ * A base class defining functions for notifying about certain validation
15+ * events.
16+ */
17+class ValidationNotifications

ryanofsky commented at 2:50 pm on May 15, 2023:

In commit “refactor: Pass BlockNotify in options to ChainstateManager” (620faa21ce0210dddca243511b7674c3b8f4e901)

I’d consider calling this class just kernel::Notifications not kernel::ValidationNotifications. I think it’s not clear how much functionality is going to be available in the kernel, and whether every notification it will ever need to send will be related to validation. If kernel needs to send other notifications that aren’t related to validation, I think it would be good if it were possible to send them through the same interface.

in src/node/validation_notifications.cpp:48 in bcd417ad18 outdated

43+static void DoWarning(const bilingual_str& warning)
44+{
45+    static bool fWarned = false;
46+    SetMiscWarning(warning);
47+    if (!fWarned) {
48+        AlertNotify(warning.original);

ryanofsky commented at 2:59 pm on May 15, 2023:

In commit “refactor: Pass DoWarning in options to ChainstateManager” (bcd417ad1847cd4c93bffbc9d8a32255e7f2b156)

Behavior isn’t changing here, but it seems strange that fWarned check is only letting the warning notification to be sent and the GUI progress bar to be updated once over the lifetime of the application. May be something to look into in a followup.

in src/bitcoin-chainstate.cpp:117 in 2f9c2d2453 outdated

112+            if (user_message.empty()) {
113+                user_message = _("A fatal internal error occurred, see debug.log for details");
114+            }
115+            std::cout << "Warning: " << user_message.original << std::endl;
116+            StartShutdown();
117+            return false;

ryanofsky commented at 3:05 pm on May 15, 2023:

In commit “refactor: Pass FatalError in options to ChainstateManager” (2f9c2d245360b3fad6d469a76c2916d75b027b57)

I think this method should just be printing the messages to cout, letting error handling code below clean up after the failure. The chainstate example program shouldn’t need to be logging things and calling StartShutdown.

in src/kernel/validation_notifications_interface.h:31 in 2f9c2d2453 outdated

27@@ -27,6 +28,7 @@ class ValidationNotifications
28     virtual void notifyHeaderTip(SynchronizationState state, int64_t height, int64_t timestamp, bool presync) = 0;
29     virtual void showProgress(const std::string& title, int nProgress, bool resume_possible) = 0;
30     virtual void doWarning(const bilingual_str& warning) = 0;
31+    virtual bool fatalError(const std::string& strMessage, bilingual_str user_message = {}) = 0;

ryanofsky commented at 3:25 pm on May 15, 2023:

In commit “refactor: Pass FatalError in options to ChainstateManager” (2f9c2d245360b3fad6d469a76c2916d75b027b57)

I think this signature should be cleaned up and not be added to kernel like this. Would suggest:

Changing bool return type to void. The return value is never used and the only value that is ever returned is false. Having the return value makes the interface confusing because it’s unclear how the value even could be used.
Renaming strMessage parameter to debug_message to modernize the name and distinguish the technical error message which is meant to go to the debug log from the translated user message.

ryanofsky commented at 3:43 pm on May 15, 2023: contributor

Light code review ACK 2f9c2d245360b3fad6d469a76c2916d75b027b57. Overall looks good, but I mostly left high level feedback and did not look closely at all the code yet. I think the suggestions I left would make this cleaner, but it already looks pretty good in it’s current form.

TheCharlatan force-pushed on May 16, 2023

TheCharlatan commented at 12:53 pm on May 16, 2023: contributor

Thank you for the detailed comments on your review @ryanofsky! Very happy to see this being fleshed out into a more general and proper kernel notification interface.

Updated 2f9c2d245360b3fad6d469a76c2916d75b027b57 -> 2c58fbf816d73395167a3046c4ce957829bf45f9 (chainstateRmKernelUiInterface_3 -> chainstateRmKernelUiInterface_4, compare).

Addressed @ryanofsky’s comment, removing pure virtual (=0) declaration.
Addressed @ryanofsky’s comment, moving notifications to the node context and declaring them as a unique_ptr. The ChainstateManagerOpts now takes a reference of kernel::Notifications.
Addressed @ryanofsky’s comment, removing wrapper methods from ChainstateManager and instead only using a simple getter. This allowed for removal of the extra :: qualifiers.
Addressed @ryanofsky’s comment, renaming showProgress to notifyProgress and progress_percent instead of nProgress.
Addressed @ryanofsky’s comment, renaming doWarning to notifyWarning.
Addressed @ryanofsky’s comment, improving / correcting the commit messages.
Addressed @ryanofsky’s comment, renaming ValidationNotifications to KernelNotifications, including file names, variable names and virtual class name.
Addressed @ryanofsky’s comment, removing unneeded fatalError handling code in bitcoin-chainstate.cpp.
Addressed @ryanofsky’s cmment, changing fatalError return type to from bool to void, renamed strMessage to debug_message.
Added a commit removing the goto epilogue statement in bitcoin-chainstate and replacing it with a lambda function that takes care of program teardown. This function is now called in case of a fatalError notification. Before the patches in this pull request, if a call to AbortNode was encountered in bitcoin-chainstate, it could lead to it it either being ignored, or the process crashing (core dumped).

DrahtBot added the label CI failed on May 16, 2023

in src/node/kernel_notifications.h:29 in 155f8de067 outdated

24@@ -25,6 +25,8 @@ class KernelNotifications : public kernel::Notifications
25     void notifyProgress(const std::string& title, int progress_percent, bool resume_possible) override;
26 
27     void notifyWarning(const bilingual_str& warning) override;
28+
29+    void fatalError(const std::string& debug_message, bilingual_str user_message = {}) override;

ryanofsky commented at 3:02 pm on May 16, 2023:

In commit “kernel: Add fatalError to notifications” (155f8de067db5d7b9ebf6de0b30729143cc9c87f)

It seems like this message should be called “notify fatal” to be consistent with other notification methods.

Alternately, it seems like word “notify” in all the method names is pretty name is pretty redundant and could be dropped. The methods are always called with notifications. or GetNotifications() prefixes so there is not really a context where repeating the word “notify” clarifies anything

ryanofsky approved

ryanofsky commented at 5:08 pm on May 16, 2023: contributor

Partial code review ACK 155f8de067db5d7b9ebf6de0b30729143cc9c87f for everything except the last commit (82ef31a5eb6094a94e05e44a658072f7afa08a47).

2830b75b91b2b77814f5099f392bf0117b652d37 kernel: Add notification interface (1/9)
234795b02db05eb4deaa20e514fe9ad7f8e9d7eb kernel: Add notifyHeaderTip to notifications (2/9)
dba41283095175620e78a5d72282bd6770b12040 kernel: Add notifyProgress to notifications (3/9)
4b810032b09af76dd71aa605863da9ca2e6bb1c6 kernel: Add notifyWarning to notifications (4/9)
c0eb82cd6c8e4af307e147958acc220ce3ff3c04 refactor: Move ScheduleBatchPriority to its own file (5/9)
fb5355d2103279186b084b6c5c4b7a56d6b2697d refactor: Move system from util to common library (6/9)
4940c4e696d3ea4ca896606e648a4e51118db0d5 scripted-diff: Rename FatalError to FatalErrorf (7/9)
155f8de067db5d7b9ebf6de0b30729143cc9c87f kernel: Add fatalError to notifications (8/9)
82ef31a5eb6094a94e05e44a658072f7afa08a47 refactor: Introduce tear down lambda to bitcoin-chainstate (9/9)

I think the last commit could just be dropped, or I don’t understand what problem it is solving. In general it seems fine to use kernel::Notifications for error reporting, but I don’t think it would be good to rely on it for error-handling. Trying to handle errors with callbacks and calls to exit() is confusing, and also very inflexible because it skips calling destructors. In the bitcoin-chainstate case if LoadChainstate or ActivateBestChain or ProcessNewBlock functions fail they should just return errors or raise exceptions to make it stop reading blocks and quit gracefully. In the bitcoind and bitcoin-qt cases, errors are already handled and the fatal error callback just triggers a clean shutdown after the fact. So I think no other changes should be needed.

TheCharlatan commented at 5:46 pm on May 16, 2023: contributor

Re #27636#pullrequestreview-1428804291

I think the last commit could just be dropped, or I don’t understand what problem it is solving.

The rationale for the commit was to preserve bitcoin-chainstate shutting down when a fatal error is encountered. I thought this through again, and I agree that this does not make a lot of sense. The function where this fatal error was encountered should always be bubbling it up anyway, so this is not needed. Dropping the commit.

TheCharlatan force-pushed on May 16, 2023

TheCharlatan commented at 7:43 pm on May 16, 2023: contributor

Thank you for the review,

Updated 2c58fbf816d73395167a3046c4ce957829bf45f9 -> b29eab3fc68de28fe2aa67700fd99c6744e37f61 (chainstateRmKernelUiInterface_4 -> chainstateRmKernelUiInterface_5, compare).

Addressed @ryanofsky’s comment, dropping notify prefix from kernel::Notifications methods
Dropped the last commit

DrahtBot removed the label CI failed on May 16, 2023

DrahtBot added the label Needs rebase on May 17, 2023

TheCharlatan force-pushed on May 17, 2023

TheCharlatan commented at 1:08 pm on May 17, 2023: contributor

Rebased b29eab3fc68de28fe2aa67700fd99c6744e37f61 -> 0b88c307a8ed81705cf8e6fb6332fdf969eb0e2e (chainstateRmKernelUiInterface_5 -> chainstateRmKernelUiInterface_6, compare).

Fixed conflict with https://github.com/bitcoin/bitcoin/pull/27571

in src/validation.h:959 in c6f2fbef2c outdated

955@@ -956,6 +956,8 @@ class ChainstateManager
956     const arith_uint256& MinimumChainWork() const { return *Assert(m_options.minimum_chain_work); }
957     const uint256& AssumedValidBlock() const { return *Assert(m_options.assumed_valid_block); }
958 
959+    kernel::Notifications& GetNotifications() const;

ryanofsky commented at 1:58 pm on May 17, 2023:

In commit “kernel: Add notification interface” (c6f2fbef2c8ecc0ded3abb9ab2fdd5369e954d52)

It would be good to write this accessor as an inline method kernel::Notifications& GetNotifications() const { return m_options.notifications; } for discoverability and consistency with other accessors above. Could also drop “Get” prefix since the other accessors don’t seem to have it, but whatever you prefer is fine.

in src/common/system.h:7 in 0a7b3e5058 outdated

2@@ -3,8 +3,8 @@
3 // Distributed under the MIT software license, see the accompanying
4 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
5 
6-#ifndef BITCOIN_UTIL_SYSTEM_H
7-#define BITCOIN_UTIL_SYSTEM_H
8+#ifndef BITCOIN_COMMON_SYSTEM_H
9+#define BITCOIN_COMMON_SYSTEM_H

ryanofsky commented at 2:35 pm on May 17, 2023:

In commit “refactor: Move system from util to common library” (0a7b3e5058b58c448bbc473ffb347910d781c3bd)

I think it makes sense to keep util::insert and util::AnyPtr functions in util/, not common/ as these are header-only template functions with no dependencies, and there’s no particular reason they shouldn’t be used by kernel code even if they aren’t used right now. In terms of functionality they are also more related to other helper functions like util/overloaded.h util/strings.h util/time.h util/types.h and util/vector.h than to anything in common/

I think ideally the util::insert function might move to util/insert.h and the util::AnyPtr function might move to util/any.h, but you could also just leave them in util/system.h if that is easier.

TheCharlatan commented at 3:32 pm on May 17, 2023:

Mmh, I was originally thinking of moving util::insert to wallet/walletutil.h and util::AnyPtr to rpc/util.h in a follow up after this pull request. I think your suggestion is better though. Moving around these kind of headers depending on which part of the code needs them seems silly. I’ll patch this then and will apply your other suggestions.

in src/kernel/notifications_interface.h:31 in 0b88c307a8 outdated

27@@ -27,6 +28,7 @@ class Notifications
28     virtual void headerTip(SynchronizationState state, int64_t height, int64_t timestamp, bool presync) {}
29     virtual void progress(const std::string& title, int progress_percent, bool resume_possible) {}
30     virtual void warning(const bilingual_str& warning) {}
31+    virtual void fatalError(const std::string& debug_message, bilingual_str user_message = {}) {}

ryanofsky commented at 2:38 pm on May 17, 2023:

In commit “kernel: Add fatalError method to notifications” (0b88c307a8ed81705cf8e6fb6332fdf969eb0e2e)

Would be good to change bilingual_str to const bilingual_str&. That should also avoid the need to remove the forward declaration and include #include <util/translation.h> above.

TheCharlatan commented at 4:23 pm on May 17, 2023:

Not sure about this one. The include is required for setting the default argument and it is non-const, because we mutate the message.

ryanofsky commented at 5:15 pm on May 17, 2023:

Not sure about this one. The include is required for setting the default argument and it is non-const, because we mutate the message.

That’s a good point about the include. But changing to a const bilingual_str& reference and avoiding unnecessary copies would provide a more consistent interface.

For one thing, getting of the mutations would actually make current implementations of the interface more legible:

0InitError(user_message.empty() ? _("A fatal internal error occurred, see debug.log for details") : user_message);
1
2std::cerr << (user_message.empty() ? "A fatal internal error occurred." : user_message.original) << std::endl;

But more importantly kernel code should not care about quirks of particular implementations of the interface. Only 2 out of the 3 of the fatalError implementation currently mutate their arguments, and mutating the argument is always unnecessary. I don’t think you would argue it would make sense to change other const string& and const bilingual_str& parameters in the interface if some future implementation of those interfaces decides it wants to be quirky and mutate arguments. The interface should pass strings by reference or by value in some consistent way.

TheCharlatan commented at 5:19 pm on May 17, 2023:

Thanks for the follow-up!

in src/validation.h:106 in 0b88c307a8 outdated

102@@ -103,7 +103,7 @@ void StopScriptCheckWorkerThreads();
103 
104 CAmount GetBlockSubsidy(int nHeight, const Consensus::Params& consensusParams);
105 
106-bool AbortNode(BlockValidationState& state, const std::string& strMessage, const bilingual_str& userMessage = bilingual_str{});
107+bool FatalError(BlockValidationState& state, const std::string& strMessage, kernel::Notifications& notifications, const bilingual_str& userMessage = {});

ryanofsky commented at 2:41 pm on May 17, 2023:

In commit “kernel: Add fatalError method to notifications” (0b88c307a8ed81705cf8e6fb6332fdf969eb0e2e)

Would maybe make notifications the first parameter. It seems awkward for it to placed between the debug message and the user message.

ryanofsky approved

ryanofsky commented at 2:48 pm on May 17, 2023: contributor

Code review ACK 0b88c307a8ed81705cf8e6fb6332fdf969eb0e2e. These changes all seem very clean and straightforward now. I left some more suggestions, but you can feel free to ignore them them

DrahtBot removed the label Needs rebase on May 17, 2023

TheCharlatan force-pushed on May 17, 2023

TheCharlatan commented at 7:01 pm on May 17, 2023: contributor

Updated 0b88c307a8ed81705cf8e6fb6332fdf969eb0e2e -> a6612baf6aed92f74f96fa4bc04d0ee359f5cc3f (chainstateRmKernelUiInterface_6 -> chainstateRmKernelUiInterface_7, compare).

Addressed @ryanofsky’s comment inlining GetNotifications method (but decided to keep the name).
Added two comits for addressing @ryanofsky’s comment, splitting out util::insert and util::AnyPtr into their own files.
Addressed @ryanofsky’s comment, changed parameter in fatalError to const reference.
Addressed @ryanofsky’s comment, inserting new FatalError argument in first position.

Rebased a6612baf6aed92f74f96fa4bc04d0ee359f5cc3f -> 861582b4087f1602e7115ee4a70ef7a7263eb36b (chainstateRmKernelUiInterface_7 -> chainstateRmKernelUiInterface_8, compare).

Fixed conflicts with https://github.com/bitcoin/bitcoin/pull/25193

in src/util/insert.h:18 in d492a2e5c8 outdated

11+template <typename Tdst, typename Tsrc>
12+inline void insert(Tdst& dst, const Tsrc& src) {
13+    dst.insert(dst.begin(), src.begin(), src.end());
14+}
15+template <typename TsetT, typename Tsrc>
16+inline void insert(std::set<TsetT>& dst, const Tsrc& src) {

ryanofsky commented at 3:02 pm on May 18, 2023:

In commit “refactor: Split util::insert into its own file” (d492a2e5c88257cb14c417615dba525ce2cd9cfc)

Does this file need to #include <set>. Just wondering if IWYU would complain.

TheCharlatan commented at 4:22 pm on May 18, 2023:

Seems wrong not to include it, will do so.

in src/node/chainstate.cpp:206 in 861582b408 outdated

199@@ -200,14 +200,17 @@ ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const CacheSize
200     // snapshot is actually validated? Because this entails unusual
201     // filesystem operations to move leveldb data directories around, and that seems
202     // too risky to do in the middle of normal runtime.
203-    auto snapshot_completion = chainman.MaybeCompleteSnapshotValidation();
204+    auto snapshot_completion = chainman.MaybeCompleteSnapshotValidation(
205+        [&chainman](bilingual_str msg) {
206+            chainman.GetNotifications().fatalError(msg.original, msg);
207+        });

ryanofsky commented at 3:45 pm on May 18, 2023:

In commit “kernel: Add fatalError method to notifications” (861582b4087f1602e7115ee4a70ef7a7263eb36b)

I think it would make sense to get rid of this callback now that there is a notifications object. Following seems to work:

  0diff --git a/src/node/chainstate.cpp b/src/node/chainstate.cpp
  1index 41056272f549..3a055c8e0bc7 100644
  2--- a/src/node/chainstate.cpp
  3+++ b/src/node/chainstate.cpp
  4@@ -200,10 +200,7 @@ ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const CacheSize
  5     // snapshot is actually validated? Because this entails unusual
  6     // filesystem operations to move leveldb data directories around, and that seems
  7     // too risky to do in the middle of normal runtime.
  8-    auto snapshot_completion = chainman.MaybeCompleteSnapshotValidation(
  9-        [&chainman](bilingual_str msg) {
 10-            chainman.GetNotifications().fatalError(msg.original, msg);
 11-        });
 12+    auto snapshot_completion = chainman.MaybeCompleteSnapshotValidation();
 13 
 14     if (snapshot_completion == SnapshotCompletionResult::SKIPPED) {
 15         // do nothing; expected case
 16diff --git a/src/node/kernel_notifications.cpp b/src/node/kernel_notifications.cpp
 17index 5fc116d87097..7f2e55ec6738 100644
 18--- a/src/node/kernel_notifications.cpp
 19+++ b/src/node/kernel_notifications.cpp
 20@@ -79,7 +79,7 @@ void KernelNotifications::fatalError(const std::string& debug_message, const bil
 21     SetMiscWarning(Untranslated(debug_message));
 22     LogPrintf("*** %s\n", debug_message);
 23     InitError(user_message.empty() ? _("A fatal internal error occurred, see debug.log for details") : user_message);
 24-    StartShutdown();
 25+    if (m_do_shutdown) StartShutdown();
 26 }
 27 
 28 } // namespace node
 29diff --git a/src/node/kernel_notifications.h b/src/node/kernel_notifications.h
 30index 3db4de0a8c14..99cb04325ac9 100644
 31--- a/src/node/kernel_notifications.h
 32+++ b/src/node/kernel_notifications.h
 33@@ -27,6 +27,9 @@ public:
 34     void warning(const bilingual_str& warning) override;
 35 
 36     void fatalError(const std::string& debug_message, const bilingual_str& user_message = {}) override;
 37+
 38+    //! Useful for tests, can be set to false to avoid shutdown on fatal error.
 39+    bool m_do_shutdown{true};
 40 };
 41 } // namespace node
 42 
 43diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp
 44index 81a015513a5a..3c52d76a6ba5 100644
 45--- a/src/test/validation_chainstatemanager_tests.cpp
 46+++ b/src/test/validation_chainstatemanager_tests.cpp
 47@@ -564,7 +564,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_completion, SnapshotTestSetup
 48     auto db_cache_before_complete = active_cs.m_coinsdb_cache_size_bytes;
 49 
 50     SnapshotCompletionResult res;
 51-    auto mock_shutdown = [](bilingual_str msg) {};
 52+    m_node.notifications->m_do_shutdown = false;
 53 
 54     fs::path snapshot_chainstate_dir = *node::FindSnapshotChainstateDir();
 55     BOOST_CHECK(fs::exists(snapshot_chainstate_dir));
 56@@ -574,8 +574,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_completion, SnapshotTestSetup
 57     const uint256 snapshot_tip_hash = WITH_LOCK(chainman.GetMutex(),
 58         return chainman.ActiveTip()->GetBlockHash());
 59 
 60-    res = WITH_LOCK(::cs_main,
 61-        return chainman.MaybeCompleteSnapshotValidation(mock_shutdown));
 62+    res = WITH_LOCK(::cs_main, return chainman.MaybeCompleteSnapshotValidation());
 63     BOOST_CHECK_EQUAL(res, SnapshotCompletionResult::SUCCESS);
 64 
 65     WITH_LOCK(::cs_main, BOOST_CHECK(chainman.IsSnapshotValidated()));
 66@@ -591,8 +590,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_completion, SnapshotTestSetup
 67     BOOST_CHECK_EQUAL(all_chainstates[0], &active_cs);
 68 
 69     // Trying completion again should return false.
 70-    res = WITH_LOCK(::cs_main,
 71-        return chainman.MaybeCompleteSnapshotValidation(mock_shutdown));
 72+    res = WITH_LOCK(::cs_main, return chainman.MaybeCompleteSnapshotValidation());
 73     BOOST_CHECK_EQUAL(res, SnapshotCompletionResult::SKIPPED);
 74 
 75     // The invalid snapshot path should not have been used.
 76@@ -645,7 +643,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_completion_hash_mismatch, Sna
 77     Chainstate& validation_chainstate = *std::get<0>(chainstates);
 78     ChainstateManager& chainman = *Assert(m_node.chainman);
 79     SnapshotCompletionResult res;
 80-    auto mock_shutdown = [](bilingual_str msg) {};
 81+    m_node.notifications->m_do_shutdown = false;
 82 
 83     // Test tampering with the IBD UTXO set with an extra coin to ensure it causes
 84     // snapshot completion to fail.
 85@@ -661,8 +659,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_completion_hash_mismatch, Sna
 86     fs::path snapshot_chainstate_dir = gArgs.GetDataDirNet() / "chainstate_snapshot";
 87     BOOST_CHECK(fs::exists(snapshot_chainstate_dir));
 88 
 89-    res = WITH_LOCK(::cs_main,
 90-        return chainman.MaybeCompleteSnapshotValidation(mock_shutdown));
 91+    res = WITH_LOCK(::cs_main, return chainman.MaybeCompleteSnapshotValidation());
 92     BOOST_CHECK_EQUAL(res, SnapshotCompletionResult::HASH_MISMATCH);
 93 
 94     auto all_chainstates = chainman.GetAll();
 95diff --git a/src/validation.cpp b/src/validation.cpp
 96index e753b75c88ae..491376f0e4f0 100644
 97--- a/src/validation.cpp
 98+++ b/src/validation.cpp
 99@@ -2871,10 +2871,7 @@ bool Chainstate::ConnectTip(BlockValidationState& state, CBlockIndex* pindexNew,
100     if (this != &m_chainman.ActiveChainstate()) {
101         // This call may set `m_disabled`, which is referenced immediately afterwards in
102         // ActivateBestChain, so that we stop connecting blocks past the snapshot base.
103-        m_chainman.MaybeCompleteSnapshotValidation(
104-            [&notifications = m_chainman.GetNotifications()](bilingual_str msg) {
105-                notifications.fatalError(msg.original, msg);
106-            });
107+        m_chainman.MaybeCompleteSnapshotValidation();
108     }
109 
110     connectTrace.BlockConnected(pindexNew, std::move(pthisBlock));
111@@ -5385,8 +5382,7 @@ bool ChainstateManager::PopulateAndValidateSnapshot(
112 //      through IsUsable() checks, or
113 //
114 //  (ii) giving each chainstate its own lock instead of using cs_main for everything.
115-SnapshotCompletionResult ChainstateManager::MaybeCompleteSnapshotValidation(
116-      std::function<void(bilingual_str)> shutdown_fnc)
117+SnapshotCompletionResult ChainstateManager::MaybeCompleteSnapshotValidation()
118 {
119     AssertLockHeld(cs_main);
120     if (m_ibd_chainstate.get() == &this->ActiveChainstate() ||
121@@ -5435,7 +5431,7 @@ SnapshotCompletionResult ChainstateManager::MaybeCompleteSnapshotValidation(
122 
123         m_snapshot_chainstate->InvalidateCoinsDBOnDisk();
124 
125-        shutdown_fnc(user_error);
126+        GetNotifications().fatalError(user_error.original, user_error);
127     };
128 
129     if (index_new.GetBlockHash() != snapshot_blockhash) {
130diff --git a/src/validation.h b/src/validation.h
131index 136992a9ac15..b03e3d4a5fe4 100644
132--- a/src/validation.h
133+++ b/src/validation.h
134@@ -1044,8 +1044,7 @@ public:
135     //! If the coins match (expected), then mark the validation chainstate for
136     //! deletion and continue using the snapshot chainstate as active.
137     //! Otherwise, revert to using the ibd chainstate and shutdown.
138-    SnapshotCompletionResult MaybeCompleteSnapshotValidation(
139-        std::function<void(bilingual_str)> shutdown_fnc) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
140+    SnapshotCompletionResult MaybeCompleteSnapshotValidation() EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
141 
142     //! The most-work chain.
143     Chainstate& ActiveChainstate() const;

ryanofsky approved

ryanofsky commented at 3:49 pm on May 18, 2023: contributor

Code review ACK 861582b4087f1602e7115ee4a70ef7a7263eb36b. Just suggested changes since last review. (Thanks!)

TheCharlatan force-pushed on May 18, 2023

TheCharlatan commented at 5:34 pm on May 18, 2023: contributor

Updated 861582b4087f1602e7115ee4a70ef7a7263eb36b -> 5a01a63ac8267debf152c8757e24f300e18ae379 (chainstateRmKernelUiInterface_8 -> chainstateRmKernelUiInterface_9, compare).

Addressed @ryanofsky’s comment, including std::set
Applied @ryanofsky’s patch

Re. the last patch, I thought this through a bit, and played with the idea of instead adding a custom notification class that leaves out the StartShutdown call in fatalError for this test case. However, that seems to mess with the initial assumptions of the test case, since we’d have to re-instantiate a ChainstateManager with the new notifications in its options. Hiding the shutdown switch as an implementation detail of the child class seems preferable to me.

Also thought about whether it is safe to override the shutdown behavior for other parts of the validation code in this test case and if this might this mask potential future bugs. I believe this should be safe, since any other occurrences of a fatalError should return something unexpected that will make the tests fail.

Also took the liberty to rename m_do_shutdown to m_shutdown_on_fatal_error.

ryanofsky approved

ryanofsky commented at 5:45 pm on May 18, 2023: contributor

Code review ACK 5a01a63ac8267debf152c8757e24f300e18ae379

I’d also squash last commit 5a01a63ac8267debf152c8757e24f300e18ae379 into earlier commit acc42d15be26434a3d52fbbe1d35a77c72254115 so reviewers only need to look at one version of MaybeCompleteSnapshotValidation calls and the fatal error function instead of 2 versions. It would also be good to rebase the PR if it would fix the CI errors

TheCharlatan force-pushed on May 18, 2023

TheCharlatan commented at 6:04 pm on May 18, 2023: contributor

Rebased 5a01a63ac8267debf152c8757e24f300e18ae379 -> d96c82a76775b1a41c098e6af60130fbdbba9975 (chainstateRmKernelUiInterface_9 -> chainstateRmKernelUiInterface_10, compare).

Squashed the last commit

DrahtBot added the label CI failed on May 18, 2023

DrahtBot removed the label CI failed on May 18, 2023

in src/kernel/notifications_interface.h:22 in 974b4293a5 outdated

17+class Notifications
18+{
19+public:
20+    virtual ~Notifications(){};
21+
22+    virtual void blockTip(SynchronizationState state, CBlockIndex* index) {}

maflcko commented at 8:16 am on May 19, 2023:

974b4293a5f2fe01302c608bf0063e3b52721d0b: Any reason to accept a nullptr for something that can never be null and would lead to UB if it was null? Might be better to use a reference.

TheCharlatan commented at 10:11 am on May 19, 2023:

Just seemed weird to go from CBlockIndex* to CBlockIndex& back to CBlockIndex*

maflcko commented at 10:14 am on May 19, 2023:

Yeah, it may be odd in the meantime, but if it is done for all the lines that are touched anyway and if longer term the ui_interface lines are touched, they can also be switched over?

in src/validation.cpp:4153 in 1be1c16b70 outdated

4146@@ -4145,14 +4147,15 @@ bool Chainstate::LoadChainTip()
4147     return true;
4148 }
4149 
4150-CVerifyDB::CVerifyDB()
4151+CVerifyDB::CVerifyDB(Notifications& notifications)
4152+    : m_notifications{notifications}
4153 {
4154-    uiInterface.ShowProgress(_("Verifying blocks…").translated, 0, false);
4155+    m_notifications.progress(_("Verifying blocks…").translated, 0, false);

maflcko commented at 8:50 am on May 19, 2023:

1be1c16b70edc5def98eb1903cee08b5d546ba88: Not sure about passing only the translated string here. I think it should be up to the receiving code to decide whether to use .original (bitcoin-chainstate.cpp) or .translated (node/kernel_notif.cpp)

in src/wallet/coinselection.h:15 in c05c15cc7b outdated

10@@ -11,8 +11,8 @@
11 #include <policy/feerate.h>
12 #include <primitives/transaction.h>
13 #include <random.h>
14-#include <util/system.h>
15 #include <util/check.h>
16+#include <util/insert.h>

maflcko commented at 9:32 am on May 19, 2023:

c05c15cc7b4fb9052b87bc2f8ea0500cd5218f4e: question for clarity: This commit is not needed and unrelated to kernel? Seems fine to do, just asking.

maflcko commented at 9:33 am on May 19, 2023:

Same for AnyPtr?

TheCharlatan commented at 10:12 am on May 19, 2023:

Not strictly required, but I decided this makes sense, since these are currently in namespace util and are just a small amount of header-only code. See the discussion we had here: #27636 (review).

maflcko commented at 10:16 am on May 19, 2023:

Ah sure. You didn’t want to move them to common, so you moved them to util ahead of the other change.

in src/qt/optionsdialog.cpp:22 in e850319d53 outdated

20-#include <validation.h> // for DEFAULT_SCRIPTCHECK_THREADS and MAX_SCRIPTCHECK_THREADS
21 #include <netbase.h>
22-#include <txdb.h> // for -dbcache defaults
23-#include <util/system.h>
24+#include <txdb.h>       // for -dbcache defaults
25+#include <validation.h> // for DEFAULT_SCRIPTCHECK_THREADS and MAX_SCRIPTCHECK_THREADS

maflcko commented at 9:36 am on May 19, 2023:

nit in e850319d5336335a9efbf04307261fb565b7bc68: Would be good to either not touch it or remove the comment?

fanquake commented at 10:13 am on May 19, 2023:

Yea. In general, it’s fine for those comments to just be dropped if touching the line.

TheCharlatan commented at 10:27 am on May 19, 2023:

The changes here and here are done by git diff -U0 HEAD~1.. | ./contrib/devtools/clang-format-diff.py -p1 -i -v. Should I always drop these format changes that just re-order includes? I already drop a bunch of other changes that re-format moved code, or squash code onto a single line, that would be easier to read with its current hand-written multi line formatting.

maflcko commented at 10:31 am on May 19, 2023:

Should I always drop these format changes that just re-order includes?

No, it is just a style nit. Feel free to ignore. I just think that instead of moving one include in the wrong section to another place in the wrong section is worse than just moving it to the right place in the right section in one go.

in src/qt/bitcoingui.cpp:36 in e850319d53 outdated

29@@ -30,13 +30,13 @@
30 #include <qt/macdockiconhandler.h>
31 #endif
32 
33-#include <functional>
34 #include <chain.h>
35 #include <chainparams.h>
36+#include <common/system.h>
37+#include <functional>

maflcko commented at 9:37 am on May 19, 2023:

nit in https://github.com/bitcoin/bitcoin/commit/e850319d5336335a9efbf04307261fb565b7bc68: Wrong section

in src/bitcoin-chainstate.cpp:109 in d96c82a767 outdated

102@@ -103,6 +103,11 @@ int main(int argc, char* argv[])
103         {
104             std::cout << "Warning: " << warning.original << std::endl;
105         }
106+        void fatalError(const std::string& debug_message, const bilingual_str& user_message) override
107+        {
108+            std::cerr << "Error: " << debug_message << std::endl;
109+            std::cerr << (user_message.empty() ? "A fatal internal error occurred." : user_message.original) << std::endl;

maflcko commented at 9:47 am on May 19, 2023:

d96c82a76775b1a41c098e6af60130fbdbba9975: I think you forgot the call to std::abort, otherwise it seems that the error isn’t treated fatal?

TheCharlatan commented at 10:33 am on May 19, 2023:

See the discussion in this and this comment. To summarize: Calling std::abort or exit(0) would skip over destructors and lead to an unclean exit. Functions calling fatalError should already be returning values that would lead to bitcoin-chainstate shutting down normally.

ryanofsky commented at 10:55 am on May 19, 2023:

Would suggest documenting this in src/kernel/notifications_interface.h. Maybe:

0//! The fatal error notification is sent to notify the user and start
1//! shutting down if an error happens in kernel code that can't be recovered
2//! from. After this notification is sent, whatever function triggered the
3//! error should also return an error code or raise an exception.

in src/node/kernel_notifications.h:32 in d96c82a767 outdated

24@@ -25,6 +25,11 @@ class KernelNotifications : public kernel::Notifications
25     void progress(const std::string& title, int progress_percent, bool resume_possible) override;
26 
27     void warning(const bilingual_str& warning) override;
28+
29+    void fatalError(const std::string& debug_message, const bilingual_str& user_message = {}) override;
30+
31+    //! Useful for tests, can be set to false to avoid shutdown on fatal error.
32+    bool m_shutdown_on_fatal_error{true};

maflcko commented at 10:06 am on May 19, 2023:

brainstorming nit in d96c82a76775b1a41c098e6af60130fbdbba9975: Not sure about adding test-only code to real code. Would it be a lot more effort to add a test-only derived MockNotif : node::KernelNotifications struct that no-ops the fatalError method and use it in the one test?

TheCharlatan commented at 10:36 am on May 19, 2023:

I ran through the scenario in this comment. We also already have a bunch of other test-only or debug-only options, so I don’t think we are setting a precedent here.

ryanofsky commented at 11:06 am on May 19, 2023:

I suggested adding the bool as way to clean up the MaybeCompleteSnapshotValidation interface which seemed unnecessarily complicated. Could have gone with a mock notifications object instead but adding the bool was simpler.

Maybe more ideally we could drop the bool by just having the test call BOOST_CHECK(ShutdownRequested()) and AbortShutdown() after it calls MaybeCompleteSnapshotValidation. This would make the test more realistic and add better coverage, but I didn’t check if that worked.

TheCharlatan commented at 8:16 pm on May 19, 2023:

Maybe more ideally we could drop the bool by just having the test call BOOST_CHECK(ShutdownRequested()) and AbortShutdown() after it calls MaybeCompleteSnapshotValidation. This would make the test more realistic and add better coverage, but I didn’t check if that worked.

This doesn’t work, because calling StartShutdown without calling InitShutdownState beforehand triggers an assert(0).

ryanofsky commented at 8:42 pm on May 19, 2023:

This doesn’t work, because calling StartShutdown without calling InitShutdownState beforehand triggers an assert(0).

I guess then question would be why can’t the test framework call InitShutdownState? But this suggests that if we remove globals in shutdown code later (I don’t know if that’s required for the kernel), we should be able to get rid of the m_shutdown_on_fatal_error variable pretty easily later too.

maflcko approved

maflcko commented at 10:06 am on May 19, 2023: member

nice, lgtm ACK d96c82a76775b1a41c098e6af60130fbdbba9975 🏬

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: nice, lgtm ACK d96c82a76775b1a41c098e6af60130fbdbba9975 🏬
3iJs751g2LIsR+MmICyytU8Mz93hP9WS3Wvm5wtnY0ZwDP2JaE57c9hxisUjtX90b3vyK6GjKUXWWPpb3w+emBQ==

DrahtBot requested review from ryanofsky on May 19, 2023

in src/kernel/chainstatemanager_opts.h:47 in d96c82a767 outdated

43@@ -42,6 +44,7 @@ struct ChainstateManagerOpts {
44     DBOptions block_tree_db{};
45     DBOptions coins_db{};
46     CoinsViewOptions coins_view{};
47+    Notifications& notifications;

hebasto commented at 2:35 pm on May 19, 2023:

Why is it safe to use a reference type here?

ryanofsky commented at 4:16 pm on May 19, 2023:

re: #27636 (review)

Why is it safe to use a reference type here?

I wouldn’t call it “safe”, since in general pointers and references in C++ are pretty dangerous, but as long as the notifications object is destroyed after the last notification is sent, there is not a bug. Could add a comment to the ChainstateManagerOpts struct to make this explicit, saying the notifications option is mandatory and the object lifetime needs to be at least as long as the ChainstateManager object.

I think other alternatives would not be good. Changing it to a unique_ptr would make it not possible for the block manager and chainstate manager to share the same notifications object, or for multiple chainstate managers to share the same notifications object. Changing it to a shared_ptr could work (it actually was a shared_ptr in the initial version of this PR), but it would make the object lifetime more complicated and cause harder to debug resource leaks if the notifications object owns other resources, or use-after-free bugs if the notification objects contains other pointers. In general I think it’s best to avoid shared_ptrs where possible because it’s hard to identify all the places reference counts could go to 0 and do unexpected things. It would also just seem asymmetric if application code is allocating the object and kernel code is deleting it. Better for kernel not to get involved in application memory management if possible.

hebasto commented at 4:31 pm on May 19, 2023:

Could add a comment to the ChainstateManagerOpts struct to make this explicit, saying the notifications option is mandatory and the object lifetime needs to be at least as long as the ChainstateManager object.

Right. But that is not the case for the notifications variable in: https://github.com/bitcoin/bitcoin/blob/d96c82a76775b1a41c098e6af60130fbdbba9975/src/init.cpp#L1022-L1029

ryanofsky commented at 4:40 pm on May 19, 2023:

Right. But that is not the case for the notifications variable in:

That code looks safe to me, but is there a bug?

It definitely is verbose and duplicative, and probably the repeated parsing could be removed later with a refactoring.

hebasto commented at 7:13 am on May 20, 2023:

but is there a bug?

No, there is not. I’ve double-checked the code, it looks OK for me too.

hebasto commented at 7:26 am on May 20, 2023: member

Reviewing the code:

974b4293a5f2fe01302c608bf0063e3b52721d0b “kernel: Add notification interface”
ec5043d8c120332c0bf916b5b576aff8374af1ed “kernel: Add headerTip method to notifications”
1be1c16b70edc5def98eb1903cee08b5d546ba88 “kernel: Add progress method to notifications” +1 for changing the type of the parameter title from std::string& to bilingual_str&
483fe64956179872e5821483474c52d955949e40 “kernel: Add warning method to notifications”
3785d38417618e921c5233cf14aa3c802f37387b “refactor: Move ScheduleBatchPriority to its own file”
c05c15cc7b4fb9052b87bc2f8ea0500cd5218f4e “refactor: Split util::insert into its own file”
465a3d2b90d3a7ad523201214e6bbad9de9ea79e “refactor: Split util::AnyPtr into its own file”
e850319d5336335a9efbf04307261fb565b7bc68 “refactor: Move system from util to common library”
ce4b11d717ca1c6b0950aac82ede448d6689ff74 “scripted-diff: Rename FatalError to FatalErrorf”
d96c82a76775b1a41c098e6af60130fbdbba9975 “kernel: Add fatalError method to notifications” nit: in ChainstateManager function member definitions, to call ChainstateManager::GetNotifications, GetNotifications() is used 2 times, this->GetNotifications() is used 2 times as well. Could it be consistent?

hebasto commented at 9:29 am on May 20, 2023: member

ACK d96c82a76775b1a41c098e6af60130fbdbba9975, I have reviewed the code and it looks OK.

hebasto approved

TheCharlatan force-pushed on May 20, 2023

TheCharlatan commented at 9:55 am on May 20, 2023: contributor

Updated d96c82a76775b1a41c098e6af60130fbdbba9975 -> a30cd548f1bc0ed73a4e157c6e81dbcf2542a916 (chainstateRmKernelUiInterface_10 -> chainstateRmKernelUiInterface_11, compare).

Addressed @hebasto’s comment, improving consistency when calling GetNotifications.
Addressed @MarcoFalke’s comment, changed CBlockIndex* to CBlockIndex&.
Addressed @MarcoFalke’s comment, change progress function signature to take a bilingual_str.
Addressed @MarcoFalke’s comment_0 and comment_1, cleaning up the includes.
Added @ryanofsky’s suggestion, a docstring for fatalError.

kernel: Add notification interface

This commit is part of the libbitcoinkernel project and seeks to remove
the ChainstateManager's and, more generally, the kernel library's
dependency on interface_ui with options methods in this and the following
few commits. By removing interface_ui from the kernel library, its
dependency on boost is reduced to just boost::multi_index.

Define a new kernel notification class with virtual methods for
notifying about internal kernel events. Create a new file in the node
library for defining a function creating the default set of notification
methods such that these do not need to be re-defined all over the
codebase. As a first step, add a `blockTip` method, wrapping
`uiInterface.NotifyBlockTip`.

447761c822

kernel: Add headerTip method to notifications

This commit is part of the libbitcoinkernel project and seeks to remove
the ChainstateManager's and, more generally, the kernel library's
dependency on interface_ui with options methods in this and the following
few commits. By removing interface_ui from the kernel library, its
dependency on boost is reduced to just boost::multi_index.

84d71457e7

kernel: Add progress method to notifications

This commit is part of the libbitcoinkernel project and seeks to remove
the ChainstateManager's and, more generally, the kernel library's
dependency on interface_ui with options methods in this and the
following few commits. By removing interface_ui from the kernel library,
its dependency on boost is reduced to just boost::multi_index.

4452707ede

kernel: Add warning method to notifications

This commit is part of the libbitcoinkernel project and seeks to remove
the ChainstateManager's and, more generally, the kernel library's
dependency on interface_ui with options methods in this and the following
few commits. By removing interface_ui from the kernel library, its
dependency on boost is reduced to just boost::multi_index.

The DoWarning and AlertNotify functions are moved out of the
validation.cpp file, which removes its dependency on interface_ui as
well as util/system.

f871c69191

refactor: Move ScheduleBatchPriority to its own file

With the previous move of AlertNotify out of the validation file, and
thus out of the kernel library, ScheduleBatchPriority is the last
remaining function used by the kernel library from util/system. Move it
to its own file, such that util/system can be moved out of the util
library in the following few commits.

Moving util/system out of the kernel library removes further networking
as well as shell related code from it.

9ec5da36b6

refactor: Split util::insert into its own file 44de325d95

refactor: Split util::AnyPtr into its own file 7eee356c0a

refactor: Move system from util to common library

Since the kernel library no longer depends on the system file, move it
to the common library instead in accordance to the diagram in
doc/design/libraries.md.

7d3b35004b

TheCharlatan force-pushed on May 20, 2023

DrahtBot added the label CI failed on May 20, 2023

TheCharlatan commented at 10:11 am on May 20, 2023: contributor

Rebased a30cd548f1bc0ed73a4e157c6e81dbcf2542a916 -> bbfbcd0360e486d47025a412f2c0f4e8535ab255 (chainstateRmKernelUiInterface_11 -> chainstateRmKernelUiInterface_12, compare).

Fixed broken include introduced by #27021

hebasto approved

hebasto commented at 11:01 am on May 20, 2023: member

re-ACK bbfbcd0360e486d47025a412f2c0f4e8535ab255

DrahtBot requested review from maflcko on May 20, 2023

DrahtBot removed the label CI failed on May 20, 2023

in src/validation.cpp:4158 in 4452707ede outdated

4156 }
4157 
4158 CVerifyDB::~CVerifyDB()
4159 {
4160-    uiInterface.ShowProgress("", 100, false);
4161+    m_notifications.progress(bilingual_str{}, 100, false);

maflcko commented at 7:32 am on May 22, 2023:

nit in 4452707edec91c7d7991f486dd41ef3edb4f7fbf: not sure what the convention is, but most other places seem to use Untranslated("") over this?

hebasto commented at 9:19 am on May 22, 2023:

not sure what the convention is

Neither do I.

but most other places seem to use Untranslated("") over this?

0$ git grep 'Untranslated("")'
1src/net_permissions.cpp:    error = Untranslated("");
2src/net_permissions.cpp:    error = Untranslated("");
3src/net_permissions.cpp:    error = Untranslated("");

I think that additional semantics–being untranslated–is not required for an empty string. An empty bilingual_str instance looks OK here in my opinion.

Anyway, I’m fine with either variant.

maflcko commented at 9:23 am on May 22, 2023:

Ok, seems best to keep as-is.

in src/validation.cpp:4386 in 4452707ede outdated

4383     }
4384 
4385     cache.SetBestBlock(pindexNew->GetBlockHash());
4386     cache.Flush();
4387-    uiInterface.ShowProgress("", 100, false);
4388+    m_chainman.GetNotifications().progress(bilingual_str{}, 100, false);

maflcko commented at 7:32 am on May 22, 2023:

same?

maflcko commented at 7:35 am on May 22, 2023: member

nice ACK bbfbcd0360e486d47025a412f2c0f4e8535ab255 😇

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: nice ACK bbfbcd0360e486d47025a412f2c0f4e8535ab255 😇
3BbApFuKhj/XG7Duq8EeZ7blM3oY0cd/cJkgn9seZt34G5Kub2xfYavP2fdqQMwFHb+s5quPdTNaHumnzFKGaAg==

maflcko assigned ryanofsky on May 22, 2023

in src/kernel/notifications_interface.h:35 in bbfbcd0360 outdated

27@@ -27,6 +28,12 @@ class Notifications
28     virtual void headerTip(SynchronizationState state, int64_t height, int64_t timestamp, bool presync) {}
29     virtual void progress(const bilingual_str& title, int progress_percent, bool resume_possible) {}
30     virtual void warning(const bilingual_str& warning) {}
31+
32+    //! The fatal error notification is sent to notify the user and start
33+    //! shutting down if an error happens in kernel code that can't be recovered
34+    //! from. After this notification is sent, whatever function triggered the
35+    //! error should also return an error code or raise an exception.

theuni commented at 1:27 pm on May 22, 2023:

Sorry for the late conceptual review, but I’m worried about how true this is and how well it’s enforced? IIRC last time I played with my clang-tidy plugin, there were at least a few places where StartShutdown() was called (maybe in a nested function) but then the function didn’t return immediately.

maflcko commented at 1:40 pm on May 22, 2023:

This seems like something that should be improved. Though, as it would be a behavior change, it may be better to do it separate from this refactoring change?

theuni commented at 1:46 pm on May 22, 2023:

I’m more worried about the enforcement question. I’m nearly positive that this assumption is not true now, and even if so there’s nothing other than review keeping it true in the future.

I’ll work on generating a more current shutdown-bubble-up demo to demonstrate the problem.

ryanofsky commented at 1:50 pm on May 22, 2023:

re: #27636 (review)

I’m more worried about the enforcement question. I’m nearly positive that this assumption is not true now, and even if so there’s nothing other than review keeping it true in the future.

Hmm, I wasn’t aware of these cases, but if we want to add an enforcement mechanism that could be interesting. I guess one way to do it would be to make the method private and add FATAL_THROW FATAL_RETURN macros that are friends of the interface and enforce throwing or returning.

I would have to look at the cases, but I think if code is detecting fatal errors and not actually returning an error that seems more like a bug than something that just needs to be improved. It seems like it could lead to really messy shutdowns and possibly corrupt state, if code is relying on asynchronous shutdowns as a form of error handling.

maflcko commented at 2:03 pm on May 22, 2023:

Yes, it is certainly possible that this is not true now. However, then again, if it is a problem on current master and this pull request isn’t changing that, nor making it worse, then it seems better to do in a separate pull/issue? (I agree it should be addressed, btw)

theuni commented at 1:36 pm on May 22, 2023: member

A few high-level conceptual questions that weren’t obvious from a quick skim:

What’s the threading model of the notification interface? For example, might it be possible that I:

Receive a fatal error callback before the parent function has completed
Not yet received the fatal callback before calling another function

I guess your answer to the latter would be “the caller should’ve received an error as well”. See my other comment about my concern there.

ryanofsky commented at 2:09 pm on May 22, 2023: contributor

re: #27636#pullrequestreview-1436636736

A few high-level conceptual questions that weren’t obvious from a quick skim:

What’s the threading model of the notification interface? For example, might it be possible that I:

Receive a fatal error callback before the parent function has completed

Not yet received the fatal callback before calling another function

The first thing should always happen and the second thing should never happen. Could maybe clarify this by changing “After this notification is sent” to “After this notification returns”. It would also be good to add an overall comment to the interface class saying that notifications block kernel code, so notification methods should return quickly to avoid affecting performance.

DrahtBot requested review from ryanofsky on May 22, 2023

theuni commented at 3:05 pm on May 22, 2023: member

What’s the threading model of the notification interface? For example, might it be possible that I:

Receive a fatal error callback before the parent function has completed

Not yet received the fatal callback before calling another function

The first thing should always happen

I know I sound like a broken record here but… is that a guarantee? If so I think it should be documented. And again it feels brittle without some type of enforcement that the callback is always called from the user’s thread and not (for ex) the scheduler. Worth adding some ASSERT_CALLING_THREAD or something like that?

Could maybe clarify this by changing “After this notification is sent” to “After this notification returns”. It would also be good to add an overall comment to the interface class saying that notifications block kernel code, so notification methods should return quickly to avoid affecting performance.

Yes please. I do wonder if that’s a substantial footgun to provide, but I suppose this interface can always be improved in the future.

ryanofsky commented at 4:01 pm on May 22, 2023: contributor

Receive a fatal error callback before the parent function has completed

The first thing should always happen

I know I sound like a broken record here but… is that a guarantee? If so I think it should be documented.

I think possibly I’m not understanding the question. It seems self-evident that if some kernel function calls the kernel::Notifications::fatalError method, that function won’t finish executing until the kernel::Notifications::fatalError method implementation completes and returns. Function calls in c++ are blocking, so functions don’t return until the functions they call return, and this applies recursively all the way up to the “parent function”, whatever that may be.

The only thing the documentation doesn’t say right now is that kernel functions make normal function calls to kernel::Notifications::fatalError and don’t call them on some asynchronous thread. But I think not specifying that is probably a good thing. I think it would be best if kernel applications handle errors properly as they occur rather than relying on the timing of notifications. But we could definitely change “After this notification is sent” to “After this notification returns” to give a timing guarantee.

And again it feels brittle without some type of enforcement that the callback is always called from the user’s thread and not (for ex) the scheduler. Worth adding some ASSERT_CALLING_THREAD or something like that?

I don’t think notification callbacks will always be called from the users thread. Kernel could definitely spawn it’s own threads and operate asyncronously depending on the application. I think applications just should try to handle notifications as quickly as possible and generally not be sensitive to when they arrive.

theuni commented at 4:45 pm on May 22, 2023: member

I know I sound like a broken record here but… is that a guarantee? If so I think it should be documented.

I think possibly I’m not understanding the question. It seems self-evident that if some kernel function calls the kernel::Notifications::fatalError method, that function won’t finish executing until the kernel::Notifications::fatalError method implementation completes and returns. Function calls in c++ are blocking, so functions don’t return until the functions they call return, and this applies recursively all the way up to the “parent function”, whatever that may be.

As a caller this would be non-obvious to me. It would be perfectly reasonable to have a dedicated dispatcher thread for these messages to avoid blocking kernel execution and guaranteeing the threading properties for the caller. In that case, due to the dispatching overhead, a notification could come in after the return. That’s why I think it’s worth a mention.

Ignore my questions/comments about the calling thread, I definitely misunderstood you :)

in src/bitcoin-chainstate.cpp:88 in 447761c822 outdated

81@@ -80,12 +82,22 @@ int main(int argc, char* argv[])
82 
83     GetMainSignals().RegisterBackgroundSignalScheduler(scheduler);
84 
85+    class KernelNotifications : public kernel::Notifications
86+    {
87+    public:
88+        void blockTip(SynchronizationState, CBlockIndex&) override

theuni commented at 8:06 pm on May 23, 2023:

Would it be possible to keep pointers/references to internal data out of this interface from the beginning? I’m not sure if that’s a goal at this layer.

For blocktip for example, taking a quick look around, for the ui it looks like this eventually gets converted to: void(SynchronizationState, interfaces::BlockTip tip, double verification_progress)

I wonder if we can do the same here?

TheCharlatan commented at 10:10 pm on May 23, 2023:

Primarily, I did not want to mess with existing interfaces, but I checked through everything and it would be feasibly to entirely drop usage of CBlockIndex for this notification. The change is a bit sprawling though and also ends up touching rpc code.

I’m not sure if that’s a goal at this layer.

The way I see this play out during the next phase of exploring a more suitable kernel api, these notifications might be used to feed back into the ChainstateManager. As long as the chainman interface primarily takes a CBlockIndex such a change feels like putting the cart before the horse to me.

theuni commented at 1:26 am on May 25, 2023:

Sure. I only asked because it looked like it was possible in this case. But this can be done after-the-fact if we decide that’s what we want.

Longer-term, we’d want to detach from our internal structures to avoid changing the abi every time we change something internally. So I don’t imagine CBlockIndex ever being part of a public api. Maybe a trivial wrapper or something, but not CBlockIndex itself. (It’s also worth keeping that in mind at this stage because that external-to-internal-component translation becomes part of the overhead of using the api)

TheCharlatan commented at 9:44 pm on May 25, 2023:

Will add this to the TODO section in the tracking issue.

theuni commented at 8:11 pm on May 23, 2023: member

From IRC:

<TheCharlatan> Hey, how about moving the last two commits of #27636 (the only ones to touch the shutdown logic) to #27711? Then we can get #27636 merged and move ahead with #27576.

This is a good idea imo. I really don’t want to slow this down, but I would benefit from a few extra days of catching up and testing the shutdown logic before reviewing/critiquing it in detail.

TheCharlatan force-pushed on May 23, 2023

TheCharlatan commented at 10:16 pm on May 23, 2023: contributor

Dropped bbfbcd0360e486d47025a412f2c0f4e8535ab255 -> 7d3b35004b039f2bd606bb46a540de7babdbc41e (chainstateRmKernelUiInterface_12 -> chainstateRmKernelUiInterface_13, compare.

This reverts the last two commits touching the shutdown logic. I feel like these two changes produced the most controversy and could benefit from further review in another PR. The two commits will be picked up in #27711 where a further patch-set is presented to complete the removal of shutdown code from the kernel library. Hopefully this allows things to move forward and a more focused review of just the shutdown functionality.

TheCharlatan renamed this:
~~kernel: Remove interface_ui, util/system from kernel library~~
kernel: Remove util/system from kernel library, interface_ui from validation.
on May 24, 2023

in src/bitcoin-chainstate.cpp:40 in 4452707ede outdated

36@@ -37,6 +37,7 @@
37 #include <functional>
38 #include <iosfwd>
39 #include <memory>
40+#include <string>

stickies-v commented at 12:43 pm on May 24, 2023:

nit: I think you also need to #include <util/translation.h> for bilingual_str

in src/kernel/notifications_interface.h:9 in 4452707ede outdated

5@@ -6,9 +6,11 @@
6 #define BITCOIN_KERNEL_NOTIFICATIONS_INTERFACE_H
7 
8 #include <cstdint>
9+#include <string>

stickies-v commented at 12:45 pm on May 24, 2023:

nit: Do we need this?

in src/node/kernel_notifications.h:27 in f871c69191 outdated

22@@ -23,6 +23,8 @@ class KernelNotifications : public kernel::Notifications
23     void headerTip(SynchronizationState state, int64_t height, int64_t timestamp, bool presync) override;
24 
25     void progress(const bilingual_str& title, int progress_percent, bool resume_possible) override;
26+
27+    void warning(const bilingual_str& warning) override;

stickies-v commented at 12:55 pm on May 24, 2023:

nit: could use warn() to avoid having an identical fn and param name (also more intuitive for a fn name imo, but that’s personal)

in src/node/blockstorage.cpp:23 in 9ec5da36b6 outdated

16@@ -17,6 +17,7 @@
17 #include <signet.h>
18 #include <streams.h>
19 #include <undo.h>
20+#include <util/batchpriority.h>
21 #include <util/fs.h>
22 #include <util/syscall_sandbox.h>
23 #include <util/system.h>

stickies-v commented at 1:33 pm on May 24, 2023:

nit: I think this include is no longer necessary

maflcko commented at 6:23 am on May 25, 2023:

It’s removed, no?

stickies-v commented at 9:54 am on May 25, 2023:

Yeah you’re right, I was leaving comments on a per-commit review basis, but it’s removed later on in 7d3b35004b039f2bd606bb46a540de7babdbc41e . Can (should) be removed in this commit already, though. But no biggie either way.

in src/test/fuzz/system.cpp:58 in 7d3b35004b

54@@ -55,7 +55,7 @@ FUZZ_TARGET_INIT(system, initialize_system)
55             [&] {
56                 const OptionsCategory options_category = fuzzed_data_provider.PickValueInArray<OptionsCategory>({OptionsCategory::OPTIONS, OptionsCategory::CONNECTION, OptionsCategory::WALLET, OptionsCategory::WALLET_DEBUG_TEST, OptionsCategory::ZMQ, OptionsCategory::DEBUG_TEST, OptionsCategory::CHAINPARAMS, OptionsCategory::NODE_RELAY, OptionsCategory::BLOCK_CREATION, OptionsCategory::RPC, OptionsCategory::GUI, OptionsCategory::COMMANDS, OptionsCategory::REGISTER_COMMANDS, OptionsCategory::HIDDEN});
57                 // Avoid hitting:
58-                // util/system.cpp:425: void ArgsManager::AddArg(const std::string &, const std::string &, unsigned int, const OptionsCategory &): Assertion `ret.second' failed.
59+                // common/args.cpp:563: void ArgsManager::AddArg(const std::string &, const std::string &, unsigned int, const OptionsCategory &): Assertion `ret.second' failed.

stickies-v commented at 2:36 pm on May 24, 2023:

nit: I think adding line number is unmaintainable, would suggest leaving it out (+ a few lines down). Even the filename is probably not even necessary, but less of a problem.

stickies-v commented at 3:56 pm on May 24, 2023: contributor

Code Review ACK 7d3b35004b039f2bd606bb46a540de7babdbc41e

I’m still familiarising myself with the libbitcoinkernel project so for now I’ve mostly focused on the code being correct, readable, … and not so much on architecture (which is arguably the most important thing here) etc. All the changes made sense, and are very well structured and readable.

I’ve left a few nits (+here) mostly regarding includes, all of them can be safely ignored and definitely shouldn’t stand in the way of this PR making progress.

~general nit: could we stick to camelcase naming for new functions, e.g. Notifications::headerTip() -> Notification::HeaderTip()?~ (edit: TheCharlatan pointed me to this section in the developer notes which I wasn’t aware of: https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#internal-interface-naming-style)

DrahtBot requested review from hebasto on May 24, 2023

DrahtBot requested review from maflcko on May 24, 2023

TheCharlatan commented at 2:18 pm on May 25, 2023: contributor

Re #27636#pullrequestreview-1441753577

Thank you for the review!

all of them can be safely ignored and definitely shouldn’t stand in the way of this PR making progress.

I’ll fix these if I have to push again, otherwise will address them in follow ups.

maflcko requested review from theuni on May 25, 2023

maflcko commented at 2:22 pm on May 25, 2023: member

re-ACK 7d3b35004b039f2bd606bb46a540de7babdbc41e (no change) 🎋

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: re-ACK 7d3b35004b039f2bd606bb46a540de7babdbc41e (no change) 🎋
3KyefeMll325W9RWk14PPm6bu59qHDnXgwGbMBKjFGB9wmlCtLMlA81b/7AQmrnMQk3tKDPrwtNU3/dBtq+dvDg==

DrahtBot removed review request from maflcko on May 25, 2023

hebasto approved

hebasto commented at 8:25 am on May 29, 2023: member

re-ACK 7d3b35004b039f2bd606bb46a540de7babdbc41e, only last two commits dropped since my recent review.

fanquake merged this on May 30, 2023

fanquake closed this on May 30, 2023

sidhujag referenced this in commit 46edd31b0f on May 30, 2023

ryanofsky referenced this in commit 75135c673e on Jul 6, 2023

hebasto referenced this in commit 592da16150 on Aug 29, 2023

hebasto referenced this in commit c77e8b5c38 on Aug 29, 2023

hebasto referenced this in commit 8b025633e4 on Aug 29, 2023

hebasto referenced this in commit 14ddf61693 on Aug 31, 2023

PastaPastaPasta referenced this in commit ba97f49f2f on Nov 17, 2023

Fabcien referenced this in commit 61686971b5 on May 23, 2024

Fabcien referenced this in commit e39c999e4f on May 23, 2024

Fabcien referenced this in commit c2307765fd on May 23, 2024

Fabcien referenced this in commit fa9a010420 on May 23, 2024

Fabcien referenced this in commit 175e03b01a on May 23, 2024

Fabcien referenced this in commit bc5ce1e1e4 on May 24, 2024

Fabcien referenced this in commit 63ec99bf40 on May 24, 2024

Fabcien referenced this in commit 204370d2ee on May 24, 2024

bitcoin locked this on May 29, 2024

kernel: Remove util/system from kernel library, interface_ui from validation. #27636

Reviews

Conflicts