refactor: make all NodeImpl/ChainImpl/ExternalSignerImpl members public, rm temporaries, simplify #25651

• Make all NodeImpl, ChainImpl and ExternalSignerImpl class members public (and document why), to be consistent in all the *Impl classes in src/node/interfaces.cpp and src/wallet/interfaces.cpp and to help future reviewers and contributors.

• Remove unneeded temporaries in NodeImpl and ChainImpl methods in src/node/interfaces.cpp and simplify, to make the code easier to read and understand and to improve performance by avoiding unnecessary move operations.

Some notes:

• The whole file is in an unnamed namespace, so everything in it is already "private" for all other files.
• I can't imagine code that instantiates this class in this file and calls methods on it, so I don't think private/public makes any difference in practise.
• As no methods are called (and will ever be called directly), except the constructor, everything else could be private as well. Though, as this makes no difference in practise, I am not sure if this even makes sense conceptually.

Thanks for having a look, Marco. It seems some of those things could change in the future, so this change looks preferable to me.

In addition to the motivations in the OP, having m_node next to the already private ChainstateManager& chainman() { return *Assert(m_node.chainman); } method seems easier to reason about. This is what made me look for m_node in the first place and wonder why it was placed there.

It seems some of those things could change in the future

I don't think any of the things could change in the future, unless I am missing something or you have examples where it would change.

So to me this mostly seems like a stylistic change, as I can't imagine a real-world example where this could make any difference.

I guess I have a mild preference for making everything public, but this change seems fine.

By design everything in ChainImpl NodeImpl WalletImpl has been public since the classes were written because the classes themselves are private. IMO it is only meaningful to distinguish between overridden and non-overridden members and private/public seems like noise in this context.

Whatever style you land on though I do think would be good to consistently apply it to all the *Impl classes in src/node/interfaces.cpp and src/wallet/interfaces.cpp

Thanks, I'm convinced. Nothing in src/wallet/interfaces.cpp is private. Making everything in src/node/interfaces.cpp public (and maybe documenting why) makes sense.

Updated per review feedback (thank you).

Added a second commit removing unnecessary and verbose temporaries in NodeImpl and ChainImpl methods in src/node/interfaces.cpp to make the code easier to read and understand and avoid unnecessary move operations. Updated the pull title and description. Refactoring only, no change in behavior; should be easy to review.

<!--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:

• #25673 (refactor: make member functions const when applicable by aureleoules)

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.

Code review ACK d742ebbc91e5ca40389a3923bffba3e24911cc6f. Suggested a change but is already an improvement as-is and the second commit does a nice job of modernizing some of this code

Updated the first commit with @ryanofsky's suggestion (thanks!) and the second commit with a simplification.

<details><summary><code>git diff d742ebb 4bedfd7</code></summary><p>

diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp
index bd819cf53e..699dca0a73 100644
--- a/src/node/interfaces.cpp
+++ b/src/node/interfaces.cpp
@@ -73,9 +73,9 @@ namespace {
 class ExternalSignerImpl : public interfaces::ExternalSigner
 {
 public:
-    ::ExternalSigner m_signer;
     ExternalSignerImpl(::ExternalSigner signer) : m_signer(std::move(signer)) {}
     std::string getName() override { return m_signer.m_name; }
+    ::ExternalSigner m_signer;
 };
 #endif
 
@@ -378,13 +378,13 @@ public:
                     /* verification progress is unused when a header was received */ 0);
             }));
     }
-    NodeContext* m_context{nullptr};
     NodeContext* context() override { return m_context; }
     void setContext(NodeContext* context) override
     {
         m_context = context;
     }
     ChainstateManager& chainman() { return *Assert(m_context->chainman); }
+    NodeContext* m_context{nullptr};
 };
 
 bool FillBlock(const CBlockIndex* index, const FoundBlock& block, UniqueLock<RecursiveMutex>& lock, const CChain& active)
@@ -702,10 +702,7 @@ public:
     }
     void waitForNotificationsIfTipChanged(const uint256& old_tip) override
     {
-        if (!old_tip.IsNull()) {
-            LOCK(::cs_main);
-            if (old_tip == chainman().ActiveChain().Tip()->GetBlockHash()) return;
-        }
+        if (!old_tip.IsNull() && old_tip == WITH_LOCK(::cs_main, return chainman().ActiveChain().Tip()->GetBlockHash())) return;
         SyncWithValidationInterfaceQueue();
     }
     std::unique_ptr<Handler> handleRpc(const CRPCCommand& command) override
@@ -760,9 +757,9 @@ public:
         return chainman().IsSnapshotActive();
     }
 
-    NodeContext& m_node;
     NodeContext* context() override { return &m_node; }
     ChainstateManager& chainman() { return *Assert(m_node.chainman); }
+    NodeContext& m_node;
 };

</p></details>

Code review ACK 4bedfd702ad878645c51bea6ee8ce40d8c0bd3da. Changes since last review, applying suggested style & simplifiying first commit. Also avoiding another lock in second commit.