Refactor: separate wallet from node by ryanofsky · Pull Request #10973 · bitcoin/bitcoin

ryanofsky · 2017-08-01T22:17:26Z

This PR is the last in a chain of PRs (#14437, #14711, and #15288) that make the wallet code access node state through an abstract Chain class in src/interfaces/ instead of using global variables like cs_main, chainActive, and g_connman. After this PR, wallet code no longer accesses global variables declared outside the wallet directory, and no longer calls functions accessing those globals (as verified by the hide-globals script in #10244).

This PR and the previous PRs have been refactoring changes that do not affect behavior. Previous PRs have consisted of lots of mechanical changes like:

-    wtx.nTimeReceived = GetAdjustedTime();
+    wtx.nTimeReceived = m_chain->getAdjustedTime();

This PR is smaller, but less mechanical. It replaces last few bits of wallet code that access node state directly (through CValidationInterface, CRPCTable, and CCoinsViewMemPool interfaces) with code that uses the Chain interface.

These changes allow followup PR #10102 (multiprocess gui & wallet PR) to work without any significant updates to wallet code. Additionally they:

Provide a single place to describe the interface between wallet and node code.
Can make better wallet testing possible, because the Chain object consists of virtual methods that can be overloaded for mocking. (This could be used to test edge cases in the rescan code, for example).

This commit just moves a few function declarations and updates callers. Function bodies are moved in two followup MOVEONLY commits. This change is desirable because wallet.h/cpp are monolithic and hard to navigate, so pulling things out and grouping together pieces of related functionality should improve the organization. Another proximate motivation is the wallet process separation work in bitcoin#10973, where (at least initially) parameter parsing and fee estimation are still done in the main process rather than the wallet process, and having functions that run in different processes scrambled up throughout wallet.cpp is unnecessarily confusing.

jonasschnelli · 2017-08-03T09:41:03Z

Concept ACK

This commit just moves a few function declarations and updates callers. Function bodies are moved in two followup MOVEONLY commits. This change is desirable because wallet.h/cpp are monolithic and hard to navigate, so pulling things out and grouping together pieces of related functionality should improve the organization. Another proximate motivation is the wallet process separation work in bitcoin#10973, where (at least initially) parameter parsing and fee estimation are still done in the main process rather than the wallet process, and having functions that run in different processes scrambled up throughout wallet.cpp is unnecessarily confusing.

f01103c MOVEONLY: Init functions wallet/wallet.cpp -> wallet/init.cpp (Russell Yanofsky) e7fe320 MOVEONLY: Fee functions wallet/wallet.cpp -> wallet/fees.cpp (Russell Yanofsky) d97fe20 Move some static functions out of wallet.h/cpp (Russell Yanofsky) Pull request description: This just moves some static wallet fee and init functions out of `wallet/wallet.cpp` and into new `wallet/fees.cpp` and `wallet/init.cpp` source files. There is one commit updating declarations and callers, followed by two MOVEONLY commits actually moving the function bodies. This change is desirable because wallet.h/cpp are monolithic and hard to navigate, so pulling things out and grouping together pieces of related functionality should improve the organization. Another motivation is the wallet process separation work in #10973, where (at least initially) parameter parsing and fee estimation are still done in the main process rather than the wallet process, and having functions that run in different processes scrambled up throughout wallet.cpp is unnecessarily confusing. Tree-SHA512: 6e6982ff82b2ab4e681c043907e2b1801ceb9513394730070f16c46ad338278a863f5b3759aa13db76a259b268b1c919c81f4e339f0796a3cfb990161e8c316d

ryanofsky · 2017-09-01T17:05:20Z

@jnewbery, the change to stop deduping link arguments is in Makefile.am here

JeremyRubin

I did a superficial review of the code & the approach seems reasonable to me!

utACKs from me for the commits checked below...

src/ipc/local/bitcoind.cpp

src/ipc/interfaces.h

src/init.cpp

src/ipc/interfaces.h

Return unset optional instead of -1 from functions trying to return heights of blocks that might not exist. Change suggested by Jeremy Rubin <[email protected]> bitcoin#10973 (comment)

This commit does not change behavior.

DrahtBot · 2019-03-06T19:07:38Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#15606 ([experimental] UTXO snapshots by jamesob)
#15557 (Enhance bumpfee to include inputs when targeting a feerate by instagibbs)
#15329 (Fix InitError() and InitWarning() content by hebasto)
#15157 (rpc: Bumpfee units change, satoshis to BTC by benthecarman)
#14942 (wallet: Make scan / abort status private to CWallet by Empact)
#14912 ([WIP] External signer support (e.g. hardware wallet) by Sjors)
#14053 (Add address-based index (attempt 4?) by marcinja)
#13804 (WIP: Transaction Pool Layer by MarcoFalke)
#12911 (wallet: Show fee in results for signrawtransaction* for segwit inputs by kallewoof)
#12096 ([rpc] [wallet] Allow specifying the output index when using bumpfee by kallewoof)

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.

maflcko · 2019-03-06T19:16:50Z

utACK 6898f53

promag

Looks good, as @jnewbery points out, a bit hard to review, but I think it's worth the effort. Sorry for these nits, feel free to ignore them.

After this PR, wallet code no longer accesses global variables declared outside the wallet directory, and no longer calls functions accessing those globals (as verified by the hide-globals script in #10244).

Do you think this should be committed and verified in CI?

promag · 2019-03-11T10:40:50Z

src/rpc/rawtransaction.cpp

+        m_cache.SetBackend(m_backend);
+    }
+
+    private:


nit, align.

re: #10973 (comment)

nit, align.

Removed in simplification.

promag · 2019-03-11T10:40:59Z

src/rpc/rawtransaction.cpp

+    }
+
+    private:
+    bool GetCoin(const COutPoint &output, Coin &coin) const override {


nit, format.

re: #10973 (comment)

nit, format.

Removed in simplification.

promag · 2019-03-11T11:11:13Z

src/interfaces/chain.h

        int64_t* max_time = nullptr) = 0;

+    //! Get unspent outputs associated with a transaction.
+    virtual std::vector<Coin> findCoins(const std::vector<COutPoint>& outputs) = 0;


I wonder if this should return an std::vector<std::pair<COutPoint, Coin>>? Otherwise could note in the comment that result follows outputs order.

re: #10973 (comment)

I wonder if this should return an std::vector<std::pair<COutPoint, Coin>>? Otherwise could note in the comment that result follows outputs order.

Agree this was awkward. Replaced with in/out param in simplification.

promag · 2019-03-11T11:11:29Z

src/rpc/rawtransaction.cpp

+    }
+    CCoinsViewCache& m_cache;
+    const COutPoint& m_output;
+    Coin&& m_coin;


re: #10973 (comment)

In commit "Remove use of CCoinsViewMemPool::GetCoin in wallet code" (016b121)

Why?

Removed in simplification. & would have worked here too, but && seemed to make sense since this was a reference to a temporary that was moved from.

src/rpc/server.cpp

-            rpcfn_type pfn = pcmd->actor;
-            if (setDone.insert(pfn).second)
-                (*pfn)(jreq);
+            if (setDone.insert(pcmd->unique_id).second)


promag · 2019-03-11T11:23:27Z

src/rpc/rawtransaction.cpp

+        }
+        std::vector<Coin> coins = chain.findCoins(outputs);
+        for (size_t i = 0; i < coins.size(); ++i) {
+            CoinFill(view, outputs[i], std::move(coins[i]), viewDummy);


Can you explain why not do the following:

CoinFill view(...); for (...) { view.AccessCoin(...) }

re: #10973 (comment)

Can you explain why not do the following:

Prefer short lifetimes and less persistent state. In any case, this has been removed in a simplification.

promag · 2019-03-11T11:32:15Z

src/interfaces/chain.cpp

+{
+    auto inserted = m_rpc_forwarders.emplace(
+        std::piecewise_construct, std::forward_as_tuple(command.name), std::forward_as_tuple(command));
+    if (inserted.second && !inserted.first->second.registerForwarder()) {


When can inserted.second fail?

re: #10973 (comment)

When can inserted.second fail?

Added comment, but inserted.second being false isn't a failure. It's just checked to prevent work being repeated when there are multiple wallet processes.

promag · 2019-03-11T11:33:37Z

src/wallet/rpcwallet.cpp

 {
    for (unsigned int vcidx = 0; vcidx < ARRAYLEN(commands); vcidx++)
-        t.appendCommand(commands[vcidx].name, &commands[vcidx]);
+        handlers.emplace_back(chain.handleRpc(commands[vcidx]));


handleRpc can return an "empty" handler.

re: #10973 (comment)

handleRpc can return an "empty" handler.

Not sure if there is a suggested change, but handleRpc will return null if CRPCTable::appendCommand returns false. There should be no change in behavior as this is.

But why let handlers have a null entry?

But why let handlers have a null entry?

What is the suggestion? This condition won't happen and it wouldn't be a problem if it did happen. I'm trying not to change behavior and this is a straight translation of the previous code. Neither the old code nor the new code cares about this condition. If there are concerns about rpc registration they would probably be better to address in another PR.

promag · 2019-03-11T11:34:24Z

src/interfaces/chain.cpp

+        return MakeUnique<HandlerImpl>(notifications);
+    }
+    void waitForNotifications() override { SyncWithValidationInterfaceQueue(); }
+    std::unique_ptr<Handler> handleRpc(const CRPCCommand& command) override;


nit, maybe a bit late, but should be uppercase RPC?

re: #10973 (comment)

nit, maybe a bit late, but should be uppercase RPC?

IMO, strict camel case without exceptions for acronyms is easier to read. (JSONRPCRequest JsonRpcRequest)

ryanofsky

Thanks for reviews!

Updated 6898f53 -> 244edfe (pr/wipc-sep.97 -> pr/wipc-sep.98, compare).

Main change is adding a new commit "Remove remaining wallet accesses to node globals" (244edfe).
Commit "Remove use CValidationInterface in wallet code" (5c7f411, formerly fce0e3c) is simplified to remove workaround for UnregisterValidationInterface crash fixed by 2196c51 in #13743.
Commit "Remove use of CRPCTable::appendCommand in wallet code" (a15a7bf, formerly 53a730a) is unchanged except for a code comment.
Commit "Remove use of CCoinsViewMemPool::GetCoin in wallet code" (9863ddc formerly 6898f53) has been simplified by replacing a custom CCoinsView subclass with a plain std::map.

re: #10973 (review)

After this PR, wallet code no longer accesses global variables declared outside the wallet directory, and no longer calls functions accessing those globals (as verified by the hide-globals script in #10244).

Do you think this should be committed and verified in CI?

I'd like to verify this, but not with a script, since it would be awkward to set up and maintain. Instead, I'd like to just add a new libbitcoin_node.a library, analogous to the existing libbitcoin_wallet.a library, and move symbol definitions like cs_main pcoinsTip chainActive there that wallet and gui code shouldn't access. That way any bad references will just result in link errors while building bitcoin-wallet and bitcoin-gui executables.

Since we have src/node/ src/wallet/ and src/qt/ folders we could also set up rules preventing source files in one of these folders including headers in the other folders. This would provide the clearest separation, but would also require moving a bunch of code around, so would not be a short-term goal.

ryanofsky · 2019-03-11T18:08:30Z

src/interfaces/chain.cpp

+        return MakeUnique<HandlerImpl>(notifications);
+    }
+    void waitForNotifications() override { SyncWithValidationInterfaceQueue(); }
+    std::unique_ptr<Handler> handleRpc(const CRPCCommand& command) override;


re: #10973 (comment)

nit, maybe a bit late, but should be uppercase RPC?

IMO, strict camel case without exceptions for acronyms is easier to read. (JSONRPCRequest JsonRpcRequest)

ryanofsky · 2019-03-11T18:08:38Z

src/interfaces/chain.cpp

+{
+    auto inserted = m_rpc_forwarders.emplace(
+        std::piecewise_construct, std::forward_as_tuple(command.name), std::forward_as_tuple(command));
+    if (inserted.second && !inserted.first->second.registerForwarder()) {


re: #10973 (comment)

When can inserted.second fail?

Added comment, but inserted.second being false isn't a failure. It's just checked to prevent work being repeated when there are multiple wallet processes.

src/rpc/server.cpp

-            rpcfn_type pfn = pcmd->actor;
-            if (setDone.insert(pfn).second)
-                (*pfn)(jreq);
+            if (setDone.insert(pcmd->unique_id).second)


ryanofsky · 2019-03-11T18:09:27Z

src/wallet/rpcwallet.cpp

 {
    for (unsigned int vcidx = 0; vcidx < ARRAYLEN(commands); vcidx++)
-        t.appendCommand(commands[vcidx].name, &commands[vcidx]);
+        handlers.emplace_back(chain.handleRpc(commands[vcidx]));


re: #10973 (comment)

handleRpc can return an "empty" handler.

Not sure if there is a suggested change, but handleRpc will return null if CRPCTable::appendCommand returns false. There should be no change in behavior as this is.

ryanofsky · 2019-03-11T18:09:46Z

src/interfaces/chain.h

        int64_t* max_time = nullptr) = 0;

+    //! Get unspent outputs associated with a transaction.
+    virtual std::vector<Coin> findCoins(const std::vector<COutPoint>& outputs) = 0;


re: #10973 (comment)

I wonder if this should return an std::vector<std::pair<COutPoint, Coin>>? Otherwise could note in the comment that result follows outputs order.

Agree this was awkward. Replaced with in/out param in simplification.

ryanofsky · 2019-03-11T18:09:56Z

src/rpc/rawtransaction.cpp

+        m_cache.SetBackend(m_backend);
+    }
+
+    private:


re: #10973 (comment)

nit, align.

Removed in simplification.

ryanofsky · 2019-03-11T18:10:06Z

src/rpc/rawtransaction.cpp

+    }
+
+    private:
+    bool GetCoin(const COutPoint &output, Coin &coin) const override {


re: #10973 (comment)

nit, format.

Removed in simplification.

ryanofsky · 2019-03-11T18:10:27Z

src/rpc/rawtransaction.cpp

+    }
+    CCoinsViewCache& m_cache;
+    const COutPoint& m_output;
+    Coin&& m_coin;


re: #10973 (comment)

In commit "Remove use of CCoinsViewMemPool::GetCoin in wallet code" (016b121)

Why?

Removed in simplification. & would have worked here too, but && seemed to make sense since this was a reference to a temporary that was moved from.

ryanofsky · 2019-03-11T18:10:37Z

src/rpc/rawtransaction.cpp

+        }
+        std::vector<Coin> coins = chain.findCoins(outputs);
+        for (size_t i = 0; i < coins.size(); ++i) {
+            CoinFill(view, outputs[i], std::move(coins[i]), viewDummy);


re: #10973 (comment)

Can you explain why not do the following:

Prefer short lifetimes and less persistent state. In any case, this has been removed in a simplification.

ryanofsky · 2019-03-12T01:02:32Z

Updated 244edfe -> 591434b (pr/wipc-sep.98 -> pr/wipc-sep.99, compare) to fix travis error with old boost.

maflcko · 2019-03-12T16:15:33Z

nit: In commit 591434b

Could also update the TODO comment for GetAvailableCredit and remove the cs_main from it?

ryanofsky · 2019-03-12T18:01:08Z

Updated 591434b -> b7c7821 (pr/wipc-sep.99 -> pr/wipc-sep.100, compare) with Marco's GetAvailableCredit suggestion from #10973 (comment)

promag · 2019-03-17T22:58:28Z

Instead, I'd like to just add a new libbitcoin_node.a library, analogous to the existing libbitcoin_wallet.a library, and move symbol definitions like cs_main pcoinsTip chainActive there that wallet and gui code shouldn't access.

Nice!

jnewbery

I've reviewed 5c7f411 (Remove use CValidationInterface in wallet code) and it looks good so far! A few comments inline.

Incidentally, this github PR is pretty unwieldly - the notes are very long, and without reading through them all it's difficult to tell which comments are for the overall approach and which are for the commits under review now. I don't know if's worth it at this point, but it might be clearer to open one final PR for the last commits, and close this or leave it as the overarching PR.

jnewbery · 2019-03-18T15:02:26Z

src/interfaces/chain.h

+        virtual ~Notifications() {}
+        virtual void TransactionAddedToMempool(const CTransactionRef& tx) {}
+        virtual void TransactionRemovedFromMempool(const CTransactionRef& ptx) {}
+        virtual void BlockConnected(const CBlock& block,


Can you simplify this interface to (cont CBlock& block, const std::vector<CTransactionRef>& tx_conflicted)? The client can get the block hash using CBlock->GetHash().

re: #10973 (comment)

Can you simplify this interface to (cont CBlock& block, const std::vector<CTransactionRef>& tx_conflicted)?

Simplified as suggested

jnewbery · 2019-03-18T15:17:49Z

src/wallet/wallet.cpp

@@ -4386,7 +4384,7 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain,
    chain.loadWallet(interfaces::MakeWallet(walletInstance));

    // Register with the validation interface. It's ok to do this after rescan since we're still holding cs_main.


I think this comment could be changed slightly now, since 'we' (the wallet) don't ever hold cs_main now.

since we're still holding cs_main -> since we're still holding the Chain::Lock interface.

re: #10973 (comment)

I think this comment could be changed slightly now, since 'we' (the wallet) don't ever hold cs_main now.

Changed this to reference locked_chain instead of cs_main (it seemed to good to reference the actual variable name).

jnewbery · 2019-03-18T15:34:09Z

src/wallet/wallet.h

    std::set<COutPoint> setLockedCoins GUARDED_BY(cs_wallet);

+    /** Validation event callback handler. */
+    std::unique_ptr<interfaces::Handler> m_handler;


I don't really like this name m_handler, (or the class name HandlerImpl), since the name handler is generic for any interface handler. It's more verbose, but I think m_validation_interface_handler and ValidationInterfaceHandlerImpl are more explicit.

re: #10973 (comment)

I don't really like this name m_handler, (or the class name HandlerImpl)

Renamed to m_chain_notifications_handler to match Chain::Notifications, Chain::handleNotifications, Chain::waitForNotifications.

ryanofsky

Updated b7c7821 -> 1a1036a (pr/wipc-sep.100 -> pr/wipc-sep.101, compare) with suggestions from #10973 (review).

Incidentally, this github PR is pretty unwieldly

A downside of making a new PR is that comments that show up in the diffs will be gone. I think the diff comments are probably more useful than other historical comments which appear above, which I think no one will look at unless they are counting acks. (And if anyone is counting ACKs, there is only one so far from Marco in #10973 (comment) (pr/wipc-sep.97) before some recent cleanups.

ryanofsky · 2019-03-18T16:49:19Z

src/interfaces/chain.h

+        virtual ~Notifications() {}
+        virtual void TransactionAddedToMempool(const CTransactionRef& tx) {}
+        virtual void TransactionRemovedFromMempool(const CTransactionRef& ptx) {}
+        virtual void BlockConnected(const CBlock& block,


re: #10973 (comment)

Can you simplify this interface to (cont CBlock& block, const std::vector<CTransactionRef>& tx_conflicted)?

Simplified as suggested

ryanofsky · 2019-03-18T16:49:28Z

src/wallet/wallet.cpp

@@ -4386,7 +4384,7 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain,
    chain.loadWallet(interfaces::MakeWallet(walletInstance));

    // Register with the validation interface. It's ok to do this after rescan since we're still holding cs_main.


re: #10973 (comment)

I think this comment could be changed slightly now, since 'we' (the wallet) don't ever hold cs_main now.

Changed this to reference locked_chain instead of cs_main (it seemed to good to reference the actual variable name).

ryanofsky · 2019-03-18T16:49:35Z

src/wallet/wallet.h

    std::set<COutPoint> setLockedCoins GUARDED_BY(cs_wallet);

+    /** Validation event callback handler. */
+    std::unique_ptr<interfaces::Handler> m_handler;


re: #10973 (comment)

I don't really like this name m_handler, (or the class name HandlerImpl)

Renamed to m_chain_notifications_handler to match Chain::Notifications, Chain::handleNotifications, Chain::waitForNotifications.

jamesob

utACK 1a1036a

Changes look fine to me - feel free the ignore the nit. I'll do some manual testing of this tomorrow.

jamesob · 2019-03-18T19:09:26Z

src/wallet/wallet.h

    /** Add a KeyOriginInfo to the wallet */
    bool AddKeyOrigin(const CPubKey& pubkey, const KeyOriginInfo& info);
+
+    friend struct WalletTestingSetup;


Maybe outdated? Since m_chain_notifications_handler is public I don't see why we'd need this.

re: #10973 (comment)

Maybe outdated? Since m_chain_notifications_handler is public I don't see why we'd need this.

It's needed to avoid "error: cannot cast 'CWallet' to its private base class 'interfaces::Chain::Notifications'" in the WalletTestingSetup constructor:

bitcoin/src/wallet/test/wallet_test_fixture.cpp

Line 16 in 0ca5446

m_wallet.m_chain_notifications_handler = m_chain->handleNotifications(m_wallet);

because the relevant base class is private:

bitcoin/src/wallet/wallet.h

Line 641 in 0ca5446

class CWallet final : public CCryptoKeyStore, private interfaces::Chain::Notifications

jnewbery

I've reviewed:

0ca5446 (Remove use CValidationInterface in wallet code) utACK
f157e7a (Remove use of CRPCTable::appendCommand in wallet code)
0bafbfc (Remove use of CCoinsViewMemPool::GetCoin in wallet code) and utACK
1a1036a (Remove remaining wallet accesses to node globals)

I found the Remove use of CRPCTable::appendCommand in wallet code quite hard to review, probably due to my C++ ineptitude. I certainly wouldn't want to hold up this PR because I'm not able to fully work out what's going on in that commit. I think it could be made marginally simpler by removing support for multiple clients (#10973 (comment)), but Russ points out that most of the complexity is elsewhere. I also think some additional commenting in the RpcForwarder class could help.

Russ is going to look at moving more code into the RPC server to simplify that commit, but again, I don't want to hold this up because of my inability to review. If other reviewers are happy to ACK this, then it should stay as it is.

jnewbery · 2019-03-19T16:57:35Z

src/wallet/wallet.h

    void TransactionRemovedFromMempool(const CTransactionRef &ptx) override;
    void ReacceptWalletTransactions();
-    void ResendWalletTransactions(int64_t nBestBlockTime, CConnman* connman) override EXCLUSIVE_LOCKS_REQUIRED(cs_main);
+    void ResendWalletTransactions(int64_t nBestBlockTime) override EXCLUSIVE_LOCKS_REQUIRED(cs_main);


Is this EXCLUSIVE_LOCKS_REQUIRED(cs_main) still required? ResendWalletTransactions() grabs a locked_chain interface.

See also the LOCKS_EXCLUDED(cs_main annotation on BlockUntilSyncedToCurrentChain() and the comment above it. Can that also go?

The BlockUntilSyncedToCurrentChain() function can definitely use some clean-up after this PR to remove the locked_chain and move isPotentialTip out of the Chain::Lock interface, but that can wait for later.

re: #10973 (comment)

Is this EXCLUSIVE_LOCKS_REQUIRED(cs_main) still required? ResendWalletTransactions() grabs a locked_chain interface.

It's not required and should have been removed. I replaced EXCLUSIVE_LOCKS_REQUIRED(cs_main) with a locked_chain argument.

I also added todos in https://github.com/ryanofsky/bitcoin/blob/pr/wipc-sep.102/src/interfaces/chain.h#L46-L50 and https://github.com/ryanofsky/bitcoin/blob/pr/wipc-sep.102/src/interfaces/chain.cpp#L208-L212 for simplifying BlockUntilSyncedToCurrentChain and removing ResendWalletTransactions.

jnewbery · 2019-03-19T20:32:26Z

src/rpc/rawtransaction.cpp

@@ -793,20 +794,11 @@ static UniValue combinerawtransaction(const JSONRPCRequest& request)
 UniValue SignTransaction(interfaces::Chain& chain, CMutableTransaction& mtx, const UniValue& prevTxsUnival, CBasicKeyStore *keystore, bool is_temp_keystore, const UniValue& hashType)


I don't very much like that:

SignTransction() is taking a Chain interface as an argument

rpcwallet is calling into rpc/rawtransaction , which should eventually be part of libbitcoin_node (Refactor: separate wallet from node #10973 (review))

Those can both be done in a future PR.

re: #10973 (comment)

Those can both be done in a future PR.

Added todo (https://github.com/ryanofsky/bitcoin/blob/pr/wipc-sep.102/src/rpc/rawtransaction.cpp#L794-L799) and also moved findCoins code into a global method so it will be easier to get rid of the Chain argument later.

jnewbery · 2019-03-19T20:47:26Z

src/interfaces/chain.cpp

+        CCoinsViewMemPool mempool_view(&chain_view, ::mempool);
+        for (auto& coin : coins) {
+            if (!mempool_view.GetCoin(coin.first, coin.second)) {
+                coin.second.Clear();


Might be worth a comment here:

// Either the coin is not in the CCoinsViewCache or is spent. Clear it.

re: #10973 (comment)

// Either the coin is not in the CCoinsViewCache or is spent. Clear it.

Added comment

ryanofsky

Updated 1a1036a -> bffd676 (pr/wipc-sep.101 -> pr/wipc-sep.102, compare) with John's suggestions.
Updated bffd676 -> d358466 (pr/wipc-sep.102 -> pr/wipc-sep.103, compare) renaming new src/node/coin.h file to be consistent with existing src/node/transaction.h file.

I found the Remove use of CRPCTable::appendCommand in wallet code [f157e7a] quite hard to review

New version of this commit (4e4d9e9) should be a lot simpler. I was too reluctant before to change rpc server code. Should be much more straightforward now.

ryanofsky · 2019-03-20T02:21:46Z

src/interfaces/chain.cpp

+        CCoinsViewMemPool mempool_view(&chain_view, ::mempool);
+        for (auto& coin : coins) {
+            if (!mempool_view.GetCoin(coin.first, coin.second)) {
+                coin.second.Clear();


re: #10973 (comment)

// Either the coin is not in the CCoinsViewCache or is spent. Clear it.

Added comment

ryanofsky · 2019-03-20T02:23:45Z

src/interfaces/chain.cpp

+
+    UniValue forwardRequest(const JSONRPCRequest& request) const
+    {
+        // Simple forwarding of RPC requests. This just sends the request to the


re: #10973 (comment)

Supporting multiple clients makes this more complex than it needs to be.

This should be a lot simpler with commit f157e7a replaced by 4e4d9e9 (pr/wipc-sep.101 -> pr/wipc-sep.102).

Adding a new rpc server removeCommand method allowed dropping the m_rpc_forwarders map and merging the RpcHandler and RpcForwarder classes. Changing rpc server appendCommand to accept multiple commands was also not too hard and allowed getting rid of the RpcForwarder loop and m_commands vector.

ryanofsky · 2019-03-20T02:24:08Z

src/rpc/rawtransaction.cpp

@@ -793,20 +794,11 @@ static UniValue combinerawtransaction(const JSONRPCRequest& request)
 UniValue SignTransaction(interfaces::Chain& chain, CMutableTransaction& mtx, const UniValue& prevTxsUnival, CBasicKeyStore *keystore, bool is_temp_keystore, const UniValue& hashType)


re: #10973 (comment)

Those can both be done in a future PR.

Added todo (https://github.com/ryanofsky/bitcoin/blob/pr/wipc-sep.102/src/rpc/rawtransaction.cpp#L794-L799) and also moved findCoins code into a global method so it will be easier to get rid of the Chain argument later.

ryanofsky · 2019-03-20T02:24:20Z

src/wallet/wallet.h

    void TransactionRemovedFromMempool(const CTransactionRef &ptx) override;
    void ReacceptWalletTransactions();
-    void ResendWalletTransactions(int64_t nBestBlockTime, CConnman* connman) override EXCLUSIVE_LOCKS_REQUIRED(cs_main);
+    void ResendWalletTransactions(int64_t nBestBlockTime) override EXCLUSIVE_LOCKS_REQUIRED(cs_main);


re: #10973 (comment)

Is this EXCLUSIVE_LOCKS_REQUIRED(cs_main) still required? ResendWalletTransactions() grabs a locked_chain interface.

It's not required and should have been removed. I replaced EXCLUSIVE_LOCKS_REQUIRED(cs_main) with a locked_chain argument.

I also added todos in https://github.com/ryanofsky/bitcoin/blob/pr/wipc-sep.102/src/interfaces/chain.h#L46-L50 and https://github.com/ryanofsky/bitcoin/blob/pr/wipc-sep.102/src/interfaces/chain.cpp#L208-L212 for simplifying BlockUntilSyncedToCurrentChain and removing ResendWalletTransactions.

jnewbery · 2019-03-20T20:27:20Z

utACK d358466. This version is much clearer to me than the previous branch. Thank you!

I still think that handling multiple clients is an unnecessary complication for this PR. The RPC_WALLET_NOT_FOUND catch/throw in the RpcHandlerImpl actor and the last_handler bool passed to the actor seem particularly strange to me.

That said, I've reviewed the code and it all seems fine. I wouldn't want to hold this PR up because of my own aesthetic inclinations.

Very nice work Russ. Thanks for seeing this through!

meshcollider · 2019-03-21T07:57:09Z

I think this is ready 🎉

ryanofsky · 2019-03-22T17:07:31Z

re: #10973 (review)

Instead, I'd like to just add a new libbitcoin_node.a library, analogous to the existing libbitcoin_wallet.a library, and move symbol definitions like cs_main pcoinsTip chainActive there that wallet and gui code shouldn't access

To follow up, I did this in #15638 and #15639, only using the existing libbitcoin_server.a library for simplicity instead of adding a new libbitcoin_node.a library.

ryanofsky force-pushed the pr/wipc-sep branch from 92d3914 to d1f0aef Compare August 2, 2017 10:22

ryanofsky mentioned this pull request Aug 2, 2017

[MOVEONLY] Move some static functions out of wallet.h/cpp #10976

Merged

jonasschnelli added Wallet Refactoring labels Aug 3, 2017

jonasschnelli mentioned this pull request Aug 8, 2017

[Qt] import* rescans are not abortable and can confuse users #10992

Closed

ryanofsky force-pushed the pr/wipc-sep branch from d1f0aef to 26383cc Compare August 14, 2017 18:59

ryanofsky changed the title ~~WIP: Add IPC layer between node and wallet~~ Add IPC layer between node and wallet Aug 14, 2017

ryanofsky force-pushed the pr/wipc-sep branch from 26383cc to 8585b63 Compare August 16, 2017 18:00

ryanofsky force-pushed the pr/wipc-sep branch from 8585b63 to d6e932f Compare August 25, 2017 15:58

ryanofsky force-pushed the pr/wipc-sep branch 2 times, most recently from 8e7fdd5 to 8fb011e Compare August 29, 2017 09:54

ryanofsky mentioned this pull request Aug 29, 2017

Make feebumper class stateless #10600

Merged

JeremyRubin reviewed Sep 5, 2017

View reviewed changes

src/ipc/local/bitcoind.cpp Outdated Show resolved Hide resolved

src/ipc/interfaces.h Outdated Show resolved Hide resolved

src/init.cpp Outdated Show resolved Hide resolved

src/ipc/interfaces.h Outdated Show resolved Hide resolved

ryanofsky force-pushed the pr/wipc-sep branch 5 times, most recently from e0688ad to 2015123 Compare September 22, 2017 21:48

ryanofsky added 2 commits March 5, 2019 10:20

Remove use of CRPCTable::appendCommand in wallet code

4e4d9e9

This commit does not change behavior.

Remove use of CCoinsViewMemPool::GetCoin in wallet code

b1b2b23

This commit does not change behavior.

Remove remaining wallet accesses to node globals

d358466

promag reviewed Mar 11, 2019

View reviewed changes

ryanofsky commented Mar 12, 2019

View reviewed changes

jnewbery reviewed Mar 18, 2019

View reviewed changes

ryanofsky commented Mar 18, 2019

View reviewed changes

jnewbery mentioned this pull request Mar 18, 2019

Remove Broadcast/ResendWalletTransactions from validation interface #15619

Closed

jamesob approved these changes Mar 18, 2019

View reviewed changes

jnewbery reviewed Mar 19, 2019

View reviewed changes

ryanofsky commented Mar 20, 2019

View reviewed changes

jnewbery mentioned this pull request Mar 20, 2019

Remove ResendWalletTransactions from the Validation Interface #15632

Merged

ryanofsky mentioned this pull request Mar 21, 2019

Enhance bumpfee to include inputs when targeting a feerate #15557

Merged

jnewbery mentioned this pull request Mar 22, 2019

Move-only: Pull wallet code out of libbitcoin_server #15638

Merged

jnewbery mentioned this pull request Apr 10, 2019

[rpc] Remove the addresses field from the getaddressinfo return object #15750

Merged

ariard mentioned this pull request Jul 20, 2019

Reverse cs_main, cs_wallet lock order and reduce cs_main locking #16426

Merged

maflcko mentioned this pull request May 8, 2020

[enhancement] Separate Node and Wallet functions #7525

Closed

5tefan mentioned this pull request Jul 28, 2021

Merge bitcoin#14636: Avoid using numeric_limits for sequence numbers and lock times dashpay/dash#4296

Merged

ryanofsky mentioned this pull request Aug 25, 2021

refactor: Move mutable globals cleared in ::UnloadBlockIndex to BlockManager #22564

Merged

This was referenced Oct 31, 2021

merge bitcoin#10973: separate wallet from node dashpay/dash#4513

Closed

merge bitcoin#10973, #15039, #15288: separate wallet from node dashpay/dash#4558

Merged

		@@ -4386,7 +4384,7 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain,
		chain.loadWallet(interfaces::MakeWallet(walletInstance));

		// Register with the validation interface. It's ok to do this after rescan since we're still holding cs_main.

		@@ -793,20 +794,11 @@ static UniValue combinerawtransaction(const JSONRPCRequest& request)
		UniValue SignTransaction(interfaces::Chain& chain, CMutableTransaction& mtx, const UniValue& prevTxsUnival, CBasicKeyStore *keystore, bool is_temp_keystore, const UniValue& hashType)

Conversation

ryanofsky commented Aug 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonasschnelli commented Aug 3, 2017

Uh oh!

ryanofsky commented Sep 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JeremyRubin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DrahtBot commented Mar 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

maflcko commented Mar 6, 2019

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky Mar 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky Mar 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Aug 1, 2017 •

edited

Loading

ryanofsky commented Sep 1, 2017 •

edited

Loading

DrahtBot commented Mar 6, 2019 •

edited

Loading

ryanofsky Mar 11, 2019 •

edited

Loading

ryanofsky Mar 12, 2019 •

edited

Loading

ryanofsky Mar 11, 2019 •

edited

Loading