This PR fixes a race for setLockedCoins when lockUnspents is true. For instance, it should not be possible to use the same unspent in concurrent fundrawtransaction calls.
Now the cs_main and cs_wallet locks are held during CreateTransaction and LockCoin(s). Also added some style nits around the change.
2606 | - for (const CTxIn& txin : tx.vin) 2607 | + for (const CTxIn& txin : tx.vin) { 2608 | coinControl.Select(txin.prevout); 2609 | + } 2610 | + 2611 | + LOCK2(cs_main, cs_wallet);
Would be good for a comment to explaining the lock. If I understand the PR description correctly, this is preventing concurrent fundraw transactions from spending the same outputs due to the delay between CreateTransaction and LockCoin below?
Yes, or concurrent fundrawtransaction and lockunspent. The problem may not be the delay, but when one releases the lock and the other acquires, for instance.
I'll add the comment.
Done.
Can you drop cs_main from it? I don't think cs_main is required, only cs_wallet.
Won't that change the lock order? CreateTransaction locks both cs_main and cs_wallet.
Oops, indeed, yes, needs both.
utACK a3c92f4f459a385fccf13eb28f18b48009324fa5, looks good to me.
Just to complain a little, though, the change seems pretty drowned out by reformatting. In the future I'd maybe suggest doing this in another commit.
@ryanofsky actually I was thinking removing the nits. But I can do that once I push again since atm needs squash.
@ryanofsky split in 2 commits, same patch.
LGTM, this is definitely better as two separate commits utACK 03a5dc9
2591 | @@ -2592,18 +2592,18 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nC 2592 | { 2593 | std::vector<CRecipient> vecSend; 2594 | 2595 | - // Turn the txout set into a CRecipient vector 2596 | - for (size_t idx = 0; idx < tx.vout.size(); idx++) 2597 | - { 2598 | + // Turn the txout set into a CRecipient vector. 2599 | + for (size_t idx = 0; idx < tx.vout.size(); idx++) {
Err, ++idx..
2622 | 2623 | - // Copy output sizes from new transaction; they may have had the fee subtracted from them 2624 | - for (unsigned int idx = 0; idx < tx.vout.size(); idx++) 2625 | + // Copy output sizes from new transaction; they may have had the fee 2626 | + // subtracted from them. 2627 | + for (unsigned int idx = 0; idx < tx.vout.size(); idx++) {
Err, ++idx..