Interaction cleanups between main and wallet #3115

1. 
   sipa commented at 4:47 pm on October 19, 2013: member

   This pull request converts the custom callback handling (from main to wallet) to using boost::signals2, which allow concurrent modification/execution, and are thread-safe. To implement callbacks, a new CWalletInterface is defined in main, and implemented by wallet. As a result, together with a few more changes, we can break the dependency from init (and indirectly, from main) on wallet.

   Two functional cleanups were included as well, namely removing the behaviour where transactions “fFromMe” were always trickled (we shouldn’t treat our own transactions differently, privacy leak) and removing the (apparently since-long broken) PrintWallets functionality.

   Closes #2965.

2. 
   gavinandresen commented at 5:54 am on October 20, 2013: contributor

   ACK concept, and code looks good to me.

   Needs at least a sketch of a test plan that exercises all of the signals.

3. in src/wallet.cpp: in 334e15a06d outdated

   550+}
   551+
   552+void CWallet::EraseFromWallet(const uint256 &hash)
   553 {
   554     if (!fFileBacked)
   555-        return false;
   

   ---

   ---

   

   laanwj commented at 10:07 am on October 20, 2013:

   By declaring the signals as void, we do lose error reporting from the wallets. Not sure this is a big loss, but in case we’d like to keep this, boost::signals2 has various ways of combining the results from the handlers.
4. 
   laanwj commented at 10:07 am on October 20, 2013: member

   Haven’t tested yet, but code lookg good and I like the approach.
5. 
   sipa commented at 11:35 am on October 20, 2013: member

   @laanwj The reasoning is that there are callbacks from the node/validation to the wallet; they’re really just notifications in general, as the node code shouldn’t care about what the wallet does with it (in fact, the signal names shouldn’t contain “wallet” even, though I chose not to change them here). As EraseFromWallet was only called as a callback, I just simplified the method here. In the case of AddToWalletIfInvolvingMe (which is called internally inside the wallet as well, with a relevant return code), I added a wrapper to use as signal handler.
6. 
   laanwj commented at 12:17 pm on October 20, 2013: member

   Ok, thanks, that makes it clear, I was just not sure if it was overlooked or on purpose.

   I like the idea of renaming the signals to be more general (so that theoretically non-wallets could also use the API to keep track)

   0SyncWithWallets -> SyncTransactions
   1EraseFromWallets -> EraseTransaction
   2SetBestChain -> stays the same
   3UpdatedTransaction -> stays the same
   4Inventory -> stays the same
   5ResendWalletTransactions -> ResendTransactions
   

7. 
   sipa commented at 3:09 pm on October 20, 2013: member

   @laanwj Modified the signal names a bit, and added comments.
8. Do not treat fFromMe transaction differently when broadcasting fe52346450
9. Remove broken PrintWallet functionality e010af7089
10. Use boost signals for callbacks from main to wallet 00588c3fac
11. Break dependency of init on wallet.

    This required some code movement (what was CWalletTx::AcceptToMemoryPool
    doing in main?), and adding a few explicit includes that used to be
    implicit through init.h.

    722fa283d0
12. 
    sipa commented at 12:56 pm on October 26, 2013: member

    Rebased.
13. 
    BitcoinPullTester commented at 1:34 pm on October 26, 2013: none

    Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/722fa283d04dfe9c70418e69535a08eea06b4377 for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.
14. 
    sipa commented at 5:49 pm on October 26, 2013: member

    @gavinandresen Test plan:

    • Create a transaction in the GUI, and check whether the number of peers that have seen it goes up. This exercises the Inventory signal.
      • Leave the client open until it confirms. This exercises the SyncTransaction signal.
    • Every at most 30 minutes, ResendWalletTransactions should be called (LogPrintf’ed). This exercises the Broadcast signal.
    • Mine a block yourself, and wait until another on top is mined, while leaving the GUI open. The block’s payout should appear only when the second block is mined. This exercises the UpdatedTransaction signal.
    • After being fully synchronized, wait for a block whose height is a multiple of 144, and forcibly kill the client. At startup, the automatic rescan shouldn’t go back beyond that 144-multiple block. This exercises the SetBestChain signal.
      • Alternatively, do a -reindex, and wait until a block whose height is a multiple of 20160 is crossed, and do the same kill/restart cycle.
15. 
    gavinandresen commented at 1:01 am on October 30, 2013: contributor

    ACK. Ran through test plan with a Bitcoin-Qt in regtest mode and two -regtest bitcoind’s, all tests passed.
16. gavinandresen referenced this in commit e13934c94e on Oct 30, 2013
17. gavinandresen merged this on Oct 30, 2013
18. gavinandresen closed this on Oct 30, 2013

    ---

19. Bushstar referenced this in commit 3ac583cceb on Apr 8, 2020
20. DrahtBot locked this on Sep 8, 2021

Contributors
sipa gavinandresen laanwj BitcoinPullTester