wallet: double lock when funding

[pinheadmz]

Closes https://github.com/bcoin-org/bcoin/issues/906

Copied from the issue discussion...

---

So I think I see how a race condition could exist here, and actually it looks like it could happen with createTX accidentally using spent coins, the same way in this case createTX is accidentally using locked coins. The reason I think is because there is no shared lock in the wallet around the two functions that read and write coins in txdb.

When you create a TX (with smart: false) , wallet.fund() calls txdb.getCoins() and then filters them with txdb.filterUnlocked() -- with a "fund" lock:

https://github.com/bcoin-org/bcoin/blob/3623d04a9fc0ea1ad223597b2be941c07d16e0e9/lib/wallet/wallet.js#L1093-L1100

If at that same moment, a transaction is broadcast or received by the wallet, the walletDB will call txdb.insert() -- with a "write" lock:

https://github.com/bcoin-org/bcoin/blob/3623d04a9fc0ea1ad223597b2be941c07d16e0e9/lib/wallet/wallet.js#L1889-L1896

Inside txdb.insert() a bunch of stuff happens but especially these two things:

1. coins are marked as spent and removed from the credits bucket in the DB

2. coins are removed from the "locked coins" list

So...

Just looking at the above, I actually kinda wonder why we haven't seen a bug where wallet.fund() tries to use coins that, at that same moment, are being marked as spent by txdb.insert(). Maybe LevelDB is preventing that from happening concurrently. In which case it might make sense that we see this behavior around locked coins instead (which is just a BufferSet in memory, no special handling or wrappers).

I wonder if @nodar-chkuaselidze has any insight here... could be the solution is to add an extra lock to fund():

diff --git a/lib/wallet/wallet.js b/lib/wallet/wallet.js
index c4a4c197..bfbaeed9 100644
--- a/lib/wallet/wallet.js
+++ b/lib/wallet/wallet.js
@@ -1091,11 +1091,13 @@ class Wallet extends EventEmitter {
    */
 
   async fund(mtx, options, force) {
-    const unlock = await this.fundLock.lock(force);
+    const unlock1 = await this.fundLock.lock(force);
+    const unlock2 = await this.writeLock.lock();
     try {
       return await this._fund(mtx, options);
     } finally {
-      unlock();
+      unlock2();
+      unlock1();
     }
   }

[codecov-io]

Codecov Report

Merging #914 into master will increase coverage by 0.45%. The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #914      +/-   ##
==========================================
+ Coverage   61.93%   62.39%   +0.45%     
==========================================
  Files         156      156              
  Lines       26093    26087       -6     
==========================================
+ Hits        16161    16276     +115     
+ Misses       9932     9811     -121

Impacted Files	Coverage Δ
lib/wallet/wallet.js	68.67% <100%> (+0.07%)	:arrow_up:
lib/wallet/nodeclient.js	80.39% <0%> (-0.38%)	:arrow_down:
lib/primitives/tx.js	83.38% <0%> (+0.1%)	:arrow_up:
lib/wallet/walletdb.js	77.57% <0%> (+0.14%)	:arrow_up:
lib/script/script.js	80.12% <0%> (+0.15%)	:arrow_up:
lib/script/opcode.js	76.56% <0%> (+0.41%)	:arrow_up:
lib/workers/parser.js	75.28% <0%> (+1.12%)	:arrow_up:
lib/client/node.js	29.41% <0%> (+1.63%)	:arrow_up:
lib/primitives/keyring.js	77.99% <0%> (+2.91%)	:arrow_up:
lib/workers/jobs.js	57.69% <0%> (+5.76%)	:arrow_up:
... and 2 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 3623d04...026bc9a. Read the comment docs.

[christsim]

Thanks @pinheadmz . We've tested on testnet and all seems ok. We will run in the production for a while and report back.

[christsim]

FYI. Just some feedback. We've been running this PR since the 1st of December. So far this seems to have resolved our issue, we haven't seen any 'double spend' issues with our withdrawals. The issue used to occur at least twice per week before the deployment.