In commit "Use CWallet::m_last_block_processed_height in GetDepthInMainChain" (233ae92f2d51333f158810d85d13cf63a1144e6b)

Can we use -1 instead of 0 for this to be consistent with nIndex? Also would be good to note here that when a transaction is conflicted, hashBlock and block_height refer to the earliest block with a conflicting transaction instead of the block containing the transaction.

The comment says An block_height == 0 in commit Add m_block_height field in CMerkleTx and then changes to A block_height == -1 in Use CWallet::m_last_block_processed_height in GetDepthInMainChain.

It would be better to not change this in the course of the PR and just set it to A block_height == -1 in the first PR.

In commit "Use CWallet::m_last_block_processed_height in GetDepthInMainChain" (233ae92f2d51333f158810d85d13cf63a1144e6b)

Isn't this going to break existing serialization of transactions in the wallet? I think it'd be best not to change the serialized representation and just make this a memory-only field that gets filled when the wallet is loaded.

Hmmm filled at startup using getBlockHeight, given we already have hashBlock ? Do you envision the wallet to always query chain state to succeed its startup ?

In commit "Add m_block_height field in CMerkleTx" (8a1975586d3cc7f98961581859fd146a632bbd34)

This logic is getting hard to follow with three repetitive if(hashBlock...) blocks. Now that this is unconditionally updating hashBlock, I think the new logic is equivalent to the following and can be simplified:

assert(!wtxIn.isAbandoned()); // Incoming transactions should never have special abandoned block hash
if (wtxIn.hashBlock != wtx.hashBlock) {
    wtx.hashBlock = wtxIn.hashBlock;
    wtx.m_block_height = wtxIn.m_block_height;
    fUpdated = true;
}

Updated AddToWallet, hope it's clearer

In commit "Add m_block_height field in CMerkleTx" (8a1975586d3cc7f98961581859fd146a632bbd34)

It looks like a remaining place where m_block_height is still unset is in importprunedfunds. It would probably be good to change that code to call wtx.SetMerkleBranch instead of setting members directly.

Oooops, I forgot this one. I've added a call to SetMerkleBranch in importprunedfunds but had to change its argument to hask for transaction height too. Seems to me similar to the serialization issue, do we want wallet to ask chain the state of its stored transactions every time, even it should be able to infer it from the blocks already connected ?

In commit "Use CWallet::m_last_block_processed_height in GetDepthInMainChain" (233ae92f2d51333f158810d85d13cf63a1144e6b)

Can we assert(m_block_height > 0) after this point? I'm afraid of another bug like the importprunedfunds one mentioned earlier leaving m_block_height unset and this function returning bogus values.

Hmmm, I introduced assert(m_block_height >= -1) as we may GetDepthInMainChain on fresh transactions, their hashBlock being unset, their depth should appear as zero.