The node-wallet interface was introduced in PR
14437. That PR added several
including the Chain interface, which is used by the wallet to access
blockchain and mempool state.
PR 14437 and the following PRs
10973) did not make
significant changes to which node functions the wallet could call. From the
code comments “TODO: Current chain methods are too low level, exposing too
much of the internal workings of the bitcoin node, and not being very
convenient to use. Chain methods should be cleaned up and simplified over
Since that PR was merged, several PRs have been opened and merged to
clean up and simplify the Chain interface:
PR 15670 combined two
interface methods findFirstBlockWithTime and
PR 15842 replaced two
interface methods isPotentialtip and waitForNotifications with a
this is the next PR in that sequence, and replaces two interface methods
relayTransactions and submitMemoryPool with a higher-level method.
the eventual end-point of this sequence of PRs is PR
16426 which removes the
wallet’s ability to directly lock cs_main. That PR also reverses the locking
order from cs_main before cs_wallet to cs_wallet before cs_main.
This PR also impacts work to cleanly separate the node and wallet
responsibilities for rebroadcasting transactions. See issue
15734 for details.
Why is the wallet able to directly call functionality to broadcast
transactions to peers?
What is a transaction rebroadcast? How (and how often) are they currently
What are some reasons that submitting a transaction to the mempool might
What do we mean by locking order? Why must locks always be acquired in the
<jnewbery> Before we begin, I'd like to encourage everyone to leave review comments on github. Review club should be here to help answer your questions, but the point is to give you the knowledge and tools to take part in the review process
<harding> As an aside, transaction rebroadcast is really useful if your node didn't have any peers at the time you created your transaction, perhaps because you were having issues with Internet at the office. :-)
<jnewbery> bpo: lightlike: the rebroadcast logic right now isn't great. The main purpose it's serving is to make sure that your transaction doesn't get dropped. Like harding says, that's mostly useful if you tried to send the tx when you didn't have connectivity
<bpo> jnewbery so I potential improvement would be to have logic that would estimate how likely that your txn was received by peers (If transaction was relayed when node had no peers, try again when I have peers)
<jnewbery> hugohn: it's based on both. I can't remember the exact logic, but the idea is that we won't rebroadcast too frequently (that's the part that's handled by the scheduler), and we only rebroadcast after a block is received
<harding> So if the wallet created a consensus-invalid transaction, we don't worry about it here (doesn't matter---it'll never confirm anyway). We only care about mempool errors that might be temporary and that we can get around by rebroadcasting?
<jonatack> For example, when I find myself writing code comments like " // Note: this will need to be updated if BroadcastTransactions() is updated to return other non-mempool failures // that Chain clients do not need to know about."
<amiti> jkczyz: the relay param is needed mainly for `ResendWalletTransactions` to force the node relaying txns that would otherwise not be sent out due to the checks in `BroadcastTransaction`. my proposed changes to rebroadcast logic should make a cleaner separation between wallet & node and allow us to remove that relay param
<jnewbery> ok, let's wrap it up there. Reminder that next week we'll look at https://github.com/bitcoin/bitcoin/pull/15505. It's been closed, but it's still an interesting PR, and if enough of us review it, maybe we can get it opened again and merged!