Make CWallet::FundTransaction atomic #11864
Conversation
promag
force-pushed
the
2017-12-atomic-fundtransaction
branch
from
52aca53 to
051aa18
Compare
promag
changed the title
src/wallet/wallet.cpp
Outdated
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Can you drop cs_main from it? I don't think cs_main is required, only cs_wallet.
There was a problem hiding this comment.
Won't that change the lock order? CreateTransaction locks both cs_main and cs_wallet.
There was a problem hiding this comment.
Oops, indeed, yes, needs both.
ryanofsky
left a comment
ryanofsky
left a comment
There was a problem hiding this comment.
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. |
promag
changed the title
promag
force-pushed
the
2017-12-atomic-fundtransaction
branch
from
a3c92f4 to
03a5dc9
Compare
|
@ryanofsky split in 2 commits, same patch. |
|
LGTM, this is definitely better as two separate commits |
03a5dc9 [wallet] Make CWallet::FundTransaction atomic (João Barbosa) 95d4450 [wallet] Tidy up CWallet::FundTransaction (João Barbosa) Pull request description: 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. Tree-SHA512: ccf383c0c5f6db775655a3e9ccd200c3bd831a83afae2b7c389564c74f7227f5bea86a4775727de2c3603b188f383f8a12d3f9d6d94f7887865c31c94ce95ef6
| for (size_t idx = 0; idx < tx.vout.size(); idx++) | ||
| { | ||
| // Turn the txout set into a CRecipient vector. | ||
| for (size_t idx = 0; idx < tx.vout.size(); idx++) { |
| for (unsigned int idx = 0; idx < tx.vout.size(); idx++) | ||
| // Copy output sizes from new transaction; they may have had the fee | ||
| // subtracted from them. | ||
| for (unsigned int idx = 0; idx < tx.vout.size(); idx++) { |
03a5dc9 [wallet] Make CWallet::FundTransaction atomic (João Barbosa) 95d4450 [wallet] Tidy up CWallet::FundTransaction (João Barbosa) Pull request description: 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. Tree-SHA512: ccf383c0c5f6db775655a3e9ccd200c3bd831a83afae2b7c389564c74f7227f5bea86a4775727de2c3603b188f383f8a12d3f9d6d94f7887865c31c94ce95ef6
>>> Inspired by bitcoin#11864 (+bitcoin#10784) Since CreateTransaction still needs cs_main we need to obtain both locks in FundTransaction, to preserve locking order. After introducing the chain interface (bitcoin#14437) we can change the LOCK2(cs_main, cs_wallet) to just LOCK(cs_wallet)
>>> Inspired by bitcoin#11864 (+bitcoin#10784) Since CreateTransaction still needs cs_main we need to obtain both locks in FundTransaction, to preserve locking order. After introducing the chain interface (bitcoin#14437) we can change the LOCK2(cs_main, cs_wallet) to just LOCK(cs_wallet)
>>> Inspired by bitcoin#11864 (+bitcoin#10784) Since CreateTransaction still needs cs_main we need to obtain both locks in FundTransaction, to preserve locking order. After introducing the chain interface (bitcoin#14437) we can change the LOCK2(cs_main, cs_wallet) to just LOCK(cs_wallet)
4 participants
This PR fixes a race for
setLockedCoinswhenlockUnspentsis true. For instance, it should not be possible to use the same unspent in concurrentfundrawtransactioncalls.Now the
cs_mainandcs_walletlocks are held duringCreateTransactionandLockCoin(s). Also added some style nits around the change.