This PR is the latest in a sequence of PRs to clean up the node-wallet
interface.
We previously reviewed PR
15713 in Bitcoin Core
review club. See the notes and logs for that meeting for more information
about the interface and the recent work to tidy it up.
One of the main goals of that work is to remove the wallet’s ability to lock
cs_main. PR 16426 is a
proof-of-concept PR which does that.
This PR is a big step towards removing the wallet’s ability (and requirement)
to lock cs_main. It removes the locked_chain dependency from the
CWalletTx::GetDepthInMainChain() function.
For a given wallet transaction, GetDepthInMainChain() returns how many
confirmations that transaction has in the block chain.
When a wallet transaction is included in a block, the block’s hash is stored
in the CWalletTx object (see hashBlock and SetMerkleBranch().
GetDepthInMainChain() previously worked by taking that hashBlock and
checking its depth in the block chain. That requires locking cs_main since
block chain state is being accessed.
After this PR, each wallet transaction stores the height of the block that
it was confirmed in, and the wallet stores the height of the best block
in the block chain. By storing these values internally, the wallet no
longer needs to query the block chain state to calculate the transaction’s
number of confirmation.
Part of this PR has been split off into a separate PR, wallet: encapsulate
transactions state to make
review easier. Reviewers should leave comments on that PR before reviewing
this PR.
Questions
An early version of this PR added an m_block_height field the wallet
transaction serialization
(comment).
Why wouldn’t this work?
How does the wallet learn about new transactions in the mempool or included
in blocks?
What are the wallet’s expectations about block notifications? Is it a problem
if the wallet is informed of the same block more than once? If blocks arrive
in the wrong order? If a block is skipped? If a block is re-orged out of the
main chain?
<dergigi> I don't have any specific questions unfortunately - just ran into the compilation error (it's the first PR I'm looking at, still trying to wrap my head around the code)
<jnewbery> right - the PR author pulled out a small part of this PR and made it into its own PR. I'd already chosen 15931 for today's discussion and didn't want to change it in case you'd all started reviewing it
<michaelfolkson> It is certainly one that is interlocking with a bunch of other PRs and requires some organizational roadmap so they are merged in the right order. Seems like a high quality PR by itself.
<nehan> jnewbery: no, but looking forward to discussing. the goal is great, but given i'm pretty unfamiliar with the wallet code i can't convince myself that these PRs don't change behavior (or that the behavior they change is 'safe')
<jnewbery> "I can't convince myself that these PRs don't change behavior (or that the behavior they change is 'safe')" - I agree this is very difficult!
<jnewbery> for PRs like 15931 and 16624, where there are changes in serialization (or at least in the way we deserialize and hold data at runtime), being able to do regression testing on old wallet files would be really useful
<jnewbery> lightlike: when you talk about 'callback mechanism', is that things like the BlockConnected/BlockDisconnected/TransactionAddedToMempool stuff?
<jnewbery> first question I had was: An early version of this PR added an m_block_height field the wallet transaction serialization (comment). Why wouldn’t this work?
<jnewbery> ok, next question: The PR author offers two ways for the wallet to populate the wallet transactions’ heights (save the transaction height to disk or calculate the height for each transactions at wallet load time). What are the trade-offs? Which approach do you prefer?
<dergigi> side note: i really like the tx status (suggested by ryanofsky) in 16624: unconfirmed/confirmed/conflicted/abandoned - the comments on that are very clear, good job on that ariard
<jnewbery> jonatack: quite difficult. We absolutely need to remain compatible with old wallet.dat files, and it's also important that new wallet.dat files are compatible with old versions, so we couldn't just change it completely
<lightlike> jnewbery: the serialization link https://github.com/bitcoin/bitcoin/pull/15931#discussion_r295434058 you gave pointed to CMerkleTx. Isn't that only for old wallet files, so why try to add block height there? Or did the change to CWalletTx happen only recently?
<jnewbery> if we were designing it from scratch, we might use a type-length-value scheme so wallet software could just ignore fields that it doesn't recognize, but we're constrained by all the existing wallet.dat files already in existence
<hugohn> brainstorming: perhaps one way to get around the lack of versioning in older Core versions is to write new stuff to a new .dat file? and start versioning in the new .dat file?
<ariard> let's say a new frontend serializer you may try to read first byte of files, if there is a magic number showing versioning support you use new serializer if not the old one
<jnewbery> lightlike: going back to your previous point: all of the wallet callbacks come from CValidationInterface functions. I think there are only 5 of those methods that are overridden by the wallet: TransactionAddedToMempool, TransactionRemovedFromMempool, BlockConnected, BlockDisconnected, UpdatedBlockTip, ChainStateFlushed
<jnewbery> I haven't seen any answers to: The PR author offers two ways for the wallet to populate the wallet transactions’ heights (save the transaction height to disk or calculate the height for each transactions at wallet load time). What are the trade-offs? Which approach do you prefer?
<hugohn> saving tx height to disk seems to make more sense in the long-term, doesn't make sense to recompute the height each time the process starts, which doesn't change.
<provoostenator> @michaelfolkson at the moment it tests back to 0.17.1, but more could be added. The main drawback is having too many switch statements in Python test framework to deal with ancient RPC stuff
<nehan> it seems to me it's not safe to rely on the serialized txn heights unless you're sure that on restart you'll get the appropriate notifications to update them (which you might. i'm not sure)
<bcribles> possibly because I haven't read enough of the PR but the malleability of using height caused when a reorg happens (like nehan was saying) is something I need to work through
<jnewbery> nehan: yes, the wallet knows its own best block hash and tries to find the point that it diverges from the main chain, then rescans from there
<jnewbery> to add some more detail: the wallet has a 'locator', which is a sparse list of blockhashes in its view of the block chain. It uses that to try to find a fork point with the nodes view of the block chain at startup
<hugohn> to me the decision to persist to disk or not probably shouldn't impact how the wallet reacts to reorgs - one is a memory management issue, one is a consensus issue. reorgs could happen _after_ we have recomputed the height at startup anyway, so the issues should be completely orthogonal. but I could be wrong.
<jnewbery> bcribles: not directly, but it would know that there had been a re-org since it went offline, and therefore rescan the block chain from a height where it knows it shares history with the node
<jnewbery> if a re-org happened when the wallet is online, we'd expect the node to inform the wallet of the blocks being rewound with BlockDisconnected calls, followed by BlockConnected calls for the new chain
<jnewbery> ok, final question: What are the wallet’s expectations about block notifications? Is it a problem if the wallet is informed of the same block more than once? If blocks arrive in the wrong order? If a block is skipped? If a block is re-orged out of the main chain?
<jkczyz> It seems odd that BlockDisconnected uses this with a null block hash / position. At very least it's hard to follow what the code is doing in this case given the reuse for adding blocks
<lightlike> A comment in ValidationInterface says that it is guaranteed that calls arrive in the same order as events are generated in validation - in that case, how is it possible that a block is skipped?
<jnewbery> because there's not really the concept of a 'from address' in a transaction, the wallet needs to know about all prior transactions to know if a new tx is spending from it
<jkczyz> nehan: Possibly informed of the same block more than once (BlockConnected) and re-org (vis BlockDisconnected), though I'm not sure if the former happens in practice