Make CWallet::FundTransaction atomic #11864

promag · 2017-12-11T15:19:14Z

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.

ryanofsky · 2017-12-11T15:44:30Z

src/wallet/wallet.cpp

        coinControl.Select(txin.prevout);
+    }
+
+    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.

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.

ryanofsky

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.

promag · 2017-12-13T20:01:46Z

@ryanofsky actually I was thinking removing the nits. But I can do that once I push again since atm needs squash.

promag · 2017-12-14T03:22:09Z

@ryanofsky split in 2 commits, same patch.

laanwj · 2017-12-14T09:38:15Z

LGTM, this is definitely better as two separate commits
utACK 03a5dc9

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

promag · 2017-12-14T09:40:53Z

src/wallet/wallet.cpp

-    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++) {


Err, ++idx..

promag · 2017-12-14T09:41:08Z

src/wallet/wallet.cpp

-    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++) {


Err, ++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)

promag force-pushed the 2017-12-atomic-fundtransaction branch from 52aca53 to 051aa18 Compare December 11, 2017 15:42

promag changed the title ~~wallet: Make fund transaction atomic~~ wallet: Make FundTransaction atomic Dec 11, 2017

ryanofsky reviewed Dec 11, 2017

View reviewed changes

laanwj added the Wallet label Dec 11, 2017

bitcoin deleted a comment Dec 13, 2017

bitcoin deleted a comment from promag Dec 13, 2017

ryanofsky reviewed Dec 13, 2017

View reviewed changes

promag changed the title ~~wallet: Make FundTransaction atomic~~ Make CWallet::FundTransaction atomic Dec 14, 2017

promag added 2 commits December 14, 2017 03:17

[wallet] Tidy up CWallet::FundTransaction

95d4450

[wallet] Make CWallet::FundTransaction atomic

03a5dc9

promag force-pushed the 2017-12-atomic-fundtransaction branch from a3c92f4 to 03a5dc9 Compare December 14, 2017 03:19

laanwj merged commit 03a5dc9 into bitcoin:master Dec 14, 2017

promag commented Dec 14, 2017

View reviewed changes

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make CWallet::FundTransaction atomic #11864

Make CWallet::FundTransaction atomic #11864

promag commented Dec 11, 2017

ryanofsky Dec 11, 2017

promag Dec 11, 2017

promag Dec 12, 2017

TheBlueMatt Dec 12, 2017

promag Dec 13, 2017

TheBlueMatt Dec 13, 2017

ryanofsky left a comment

promag commented Dec 13, 2017 •

edited

Loading

promag commented Dec 14, 2017

laanwj commented Dec 14, 2017

promag Dec 14, 2017

promag Dec 14, 2017

Make CWallet::FundTransaction atomic #11864

Make CWallet::FundTransaction atomic #11864

Conversation

promag commented Dec 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanofsky left a comment

Choose a reason for hiding this comment

promag commented Dec 13, 2017 • edited Loading

promag commented Dec 14, 2017

laanwj commented Dec 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

promag commented Dec 13, 2017 •

edited

Loading