The node-wallet interface was introduced in PR
14437. That PR added several
interfaces,
including the Chain interface, which is used by the wallet to access
blockchain and mempool state.
PR 14437 and the following PRs
(14711,
15288 and
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
time.”
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
findFirstBlockWithTimeAndHeight
PR 15842 replaced two
interface methods isPotentialtip and waitForNotifications with a
higher-level method.
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.
Questions
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
triggered?
What are some reasons that submitting a transaction to the mempool might
fail?
What do we mean by locking order? Why must locks always be acquired in the
same order?
<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
<jnewbery> we're trying to separate out the wallet from the node, and the fact that the wallet can tell the node to relay transactions to peers is really a hangover from when it was more mixed up
<bpo> All wallets rebroadcast all transactions if the wallet sees that a new chain tip doesn't include the wallet's transactions? Isn't that a large overhead?
<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. :-)
<hugohn> is it based on the arrival of a new block or a periodic schedule? I looked up who calls MabeResendWalletTxs() & it seems it's only called by the scheduler?
<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)
<michaelfolkson> When the node and wallet are eventually entirely separated your node will rebroadcast other people's transactions as well as your own?
<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
<michaelfolkson> Just for the privacy benefit? Or because that helps other people's transactions be propagated (in the case that they have bad connectivity)?
<jnewbery> michaelfolkson: Yeah, MAX_FEE_EXCEEDED is pretty weird. When we submit a transaction to the mempool locally, we set a max feerate to make sure we don't accidentally set a huge feerate.
<jnewbery> (I'd like to remove that argument from the AcceptToMemoryPool() interface and have the client check before submitting it. WIP PR here: https://github.com/bitcoin/bitcoin/pull/15810)
<jnewbery> harding: that's a good one. I think it's pretty rare that we make policy looser (ie more permissive), but one example is making segwit v1+ segwit txs standard recently
<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?
<jnewbery> lighlike: maybe. I think that would go away once amiti's proposal is implemented. It'd be the node/mempool's responsibility to rebroadcast once it has peers
<harding> hugohn: ATMP is really meant to run on transactions you receive from other nodes, and you don't want other nodes sending you loose coinbase transactions.
<hugohn> harding: you mean we shouldn't rely on ATMP checks for our own txs? they both route to the same path though yeah? I suppose defensive programming doesn't hurt
<jonatack> Question: Should there be tests for the changed code? SubmitMemoryPoolAndRelay? As someone who is in the habit of TDD, this seems somewhat... precarious :D
<jnewbery> some of the functionality is moving across the interface border from the wallet to the node (eg the IsCoinBase() and GetDepthInMainChain() checks that used to be in RelayWalletTransction())
<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!