Remove Broadcast/ResendWalletTransactions from validation interface #15619

1. 
   jnewbery commented at 5:58 pm on March 18, 2019: member

   The Broadcast/ResendWalletTransactions interface is strange:

   • it’s the node saying to the wallet “Try to submit your unconfirmed transactions to the mempool again”
   • it’s called in one place by the node, in SendMessages() here: https://github.com/bitcoin/bitcoin/blob/c033c4b5cef89a654e4d9d5c5f9bd823871b068b/src/net_processing.cpp#L3515. SendMessages() is called on the message handling thread on a loop.
   • the actual scheduling of when the wallet sends messages is handled in the ResendWalletTransactions() function itself, using the nNextResend and nLastResend wallet globals (which could as easily be static variables in ResendWalletTransactions()).

   Therefore the only use of Broadcast() is to periodically prod the wallet. The wallet itself decides whether it’s time to resend transactions. This could be done by scheduling regular calls to ResendWalletTransactions(), in the same way that we schedule regular wallet flushes here: https://github.com/bitcoin/bitcoin/blob/c033c4b5cef89a654e4d9d5c5f9bd823871b068b/src/wallet/init.cpp#L215.

   A couple of minor implementation details:

   • the call to Broadcast() in SendMessages() is protected by an if statement if (!fReindex && !fImporting && !IsInitialBlockDownload()) {. Those tests could be moved into ResendWalletTransactions() (through the Chain interface).
   • The Broadcast notification passes in the nTimeBestReceived time of the best block. That could be fetched by ResendWalletTransactions() through the Chain interface.
2. MarcoFalke added the label Brainstorming on Mar 18, 2019
3. MarcoFalke added the label Refactoring on Mar 18, 2019
4. MarcoFalke added the label Wallet on Mar 18, 2019
5. MarcoFalke added the label Validation on Mar 18, 2019
6. 
   jnewbery commented at 6:19 pm on March 18, 2019: member

   @ryanofsky attempted to remove this interface before in #10584. That PR was missing the wallet scheduling its own rebroadcasts.

   We shouldn’t do this before #10973, to avoid having to rebase that PR again.

7. 
   ryanofsky commented at 6:27 pm on March 18, 2019: member

   Overall I think this makes sense I don’t see a reason why the wallet couldn’t schedule resends itself. One minor point on

   the nNextResend and nLastResend wallet globals (which could as easily be static variables in ResendWalletTransactions())

   These appear to be CWallet members and not global symbols:

   https://github.com/bitcoin/bitcoin/blob/c033c4b5cef89a654e4d9d5c5f9bd823871b068b/src/wallet/wallet.h#L656-L657

   I didn’t look very closely but if they were static members or static variables inside the function instead of instance members, it could be a problem because they’d be shared across multiple CWallet instances and not work with multiple wallets loaded.

8. 
   jnewbery commented at 6:28 pm on March 18, 2019: member

   if they were static members or static variables inside the function instead of instance members, it could be a problem because they’d be shared across multiple CWallet instances and not work with multiple wallets loaded.

   Ah yes, you’re right. Thanks for pointing this out.

9. 
   jnewbery commented at 10:18 pm on March 20, 2019: member

   Fixed in #15632
10. 
    promag commented at 3:24 pm on March 27, 2019: member

    ACK, actually I was surprised to see this in SendMessages https://github.com/bitcoin/bitcoin/blob/656a15e5394d8d841619b5b186fa0f9c8be0322b/src/net_processing.cpp#L3538-L3544
11. 
    jnewbery commented at 3:38 pm on March 27, 2019: member

    I was surprised to see this in SendMessages

    Yes, it’s a violation of component separation. Net processing is using the validation interface to tell the wallet to rebroadcast.

12. meshcollider closed this on Apr 9, 2019

    ---

13. MarcoFalke locked this on Dec 16, 2021

Contributors
jnewbery ryanofsky promag

Labels
Brainstorming Refactoring Wallet Validation