OrderedTxItems returns a multimap of pointers, but needs a place to store the actual CAccountingEntries it points to. It had been using a stack item, which was clobbered as soon as it returned, resulting in undefined behaviour.
This fixes at least bug #1768.
ACK. makes valgrind happy again.
304 | @@ -306,7 +305,7 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase) 305 | CWalletTx* wtx = &((*it).second); 306 | txOrdered.insert(make_pair(wtx->nOrderPos, TxPair(wtx, (CAccountingEntry*)0))); 307 | } 308 | - list<CAccountingEntry> acentries; 309 | + acentries.empty();
Don't you mean .clear()?
Yes, oops.
I agree that this would solve the immediate problem. std::list (unlike std::vector) does not invalidate iterators when items are added, so it is safe to keep pointers to items in it.
I do think the solution is a bit strange, but maybe it's just me. Why not return a type from OrderedTxItems that has the actual CAccountingEntry instead of one that has pointers to another (temporary) structure, which is being built at the same time?
@laanwj It does feel strange to me, but I failed to think up a better way short of trying to rewrite it in some other paradigm. Maybe boost::variant would work better (most of the CAccountingEntry pointers are NULL), but I'm not comfortable with adding that into the mix myself this late in the release cycle.
Yes, this is the simplest solution for now, that's for sure. Maybe add a (doxygen) comment to CWallet::OrderedTxItems that warns of the dangers, instead of only at the callsite in wallet.h.
OrderedTxItems returns a multimap of pointers, but needs a place to store the actual CAccountingEntries it points to.
It had been using a stack item, which was clobbered as soon as it returned, resulting in undefined behaviour.
This fixes at least bug #1768.
Like that?
Yes. ACK
ACK. (oops. clicked the wrong button)