Tiny optimization.
CheckFinalTx() gets always called in CWalletTx::IsTrusted().
Calling it together with IsTrusted() result in calling it twice.
CheckFinalTx() get's executed under cs_main (where it calls stuff like tips GetMedianTimePast(), `GetAdjustedTime().
utACK 1484993f691b0913d03154e2fdd0687736c65052
1938 | @@ -1939,9 +1939,6 @@ void CWallet::AvailableCoins(vector<COutput>& vCoins, bool fOnlyConfirmed, const 1939 | const uint256& wtxid = it->first; 1940 | const CWalletTx* pcoin = &(*it).second; 1941 | 1942 | - if (!CheckFinalTx(*pcoin)) 1943 | - continue; 1944 | - 1945 | if (fOnlyConfirmed && !pcoin->IsTrusted())
If fOnlyConfirmed is false, IsTrusted() is not executed at all. Is this OK?
Yes. Your right, was deluded by the OR further down. I just removed that part.
When I was adding the logic for sequence locks we decided it was not worth calculating those for wallet txs, primarily because it was near impossible to have a non-final transaction in your wallet (since you would never have accepted it to your mempool). Perhaps out of scope for this PR, but I think we should consider whether we want to support any notion of finality in the wallet at all. These existing CheckFinalTx checks are cheaper but still contribute to complication. Maybe we can just remove them all?
Or if we do want to support them, then maybe there is an efficient and general way we can support finality checks for both types of locks. Such as caching the time/height that the tx will be final and in the case of sequence locks, the hash that needs to remain on the main chain for that calculation to still be valid.
Code review ACK 4512550
we decided it was not worth calculating those for wallet txs, primarily because it was near impossible to have a non-final transaction in your wallet
This is true at the moment. Although non-final handling may make sense when the GUI, some day, allows producing transactions that are not final yet. But maybe they could just be flagged some way to keep them from being considered for balance computation until they're actually on the network. So I tend to agree.