fix false positive in conflict detection (#677)

* fix false positive in conflict detection

* dont clear bitfield, just dont reply

* no error

* add tmp log for awareness

* test still flaky

Author: mafintosh

lib/core.js

@@ -171,6 +171,7 @@ module.exports = class Core {
         key: opts.key || (compat ? manifest.signers[0].publicKey : Verifier.manifestHash(manifest)),
         manifest,
         keyPair: keyPair ? { publicKey: keyPair.publicKey, secretKey: keyPair.secretKey || null } : null,
+        frozen: false,
         userData: [],
         tree: {
           fork: 0,
@@ -190,6 +191,7 @@ module.exports = class Core {
         key: header.key,
         manifest,
         keyPair,
+        frozen: false,
         discoveryKey,
         userData: opts.userData || [],
         alias: opts.alias || null
@@ -494,6 +496,11 @@ module.exports = class Core {
       return false
     }
 
+    // sanity check -> no manifest, no way to verify
+    if (!this.header.manifest) {
+      return false
+    }
+
     const batch = MerkleTree.verifyFullyRemote(this.state, proof)
 
     try {
@@ -502,24 +509,57 @@ module.exports = class Core {
       return true
     }
 
+    const roots = await MerkleTree.getRootsFromStorage(this.storage, proof.upgrade.length)
+    const remoteTreeHash = crypto.tree(proof.upgrade.nodes)
+    const localTreeHash = crypto.tree(roots)
+
+    try {
+      const rx = this.state.storage.read()
+      const treeProofPromise = MerkleTree.proof(this.state, rx, {
+        block: null,
+        hash: null,
+        seek: null,
+        upgrade: {
+          start: 0,
+          length: proof.upgrade.length
+        }
+      })
+
+      rx.tryFlush()
+
+      const treeProof = await treeProofPromise
+
+      const verifyBatch = MerkleTree.verifyFullyRemote(this.state, await treeProof.settle())
+      this._verifyBatchUpgrade(verifyBatch, this.header.manifest)
+    } catch {
+      return true
+    }
+
+    // both proofs are valid, now check if they forked
+    if (b4a.equals(localTreeHash, remoteTreeHash)) return false
+
     await this.state.mutex.lock()
 
     try {
       const tx = this.state.createWriteBatch()
-      if (this.header.manifest === null && proof.manifest) {
-        this._setManifest(tx, proof.manifest, null)
-      }
+
+      this.header.frozen = true
+
+      tx.setAuth({
+        key: this.header.key,
+        discoveryKey: this.discoveryKey,
+        manifest: this.header.manifest,
+        keyPair: this.header.keyPair,
+        frozen: true
+      })
 
       await this.state.flush()
     } finally {
       this.state.mutex.unlock()
     }
 
-    const remoteTreeHash = crypto.tree(proof.upgrade.nodes)
-    const localTreeHash = crypto.tree(await MerkleTree.getRootsFromStorage(this.storage, proof.upgrade.length))
-
-    if (b4a.equals(localTreeHash, remoteTreeHash)) return false
-
+    // tmp log so we can see these
+    console.log('[hypercore] conflict detected in ' + this.id + ' (writable=' + !!this.header.keyPair + ',quorum=' + this.header.manifest.quorum + ')')
     await this._onconflict(proof)
     return true
   }

lib/replicator.js

@@ -475,6 +475,8 @@ class Peer {
   }
 
   broadcastRange (start, length, drop) {
+    if (!this.isActive()) return
+
     if (drop) this._unclearLocalRange(start, length)
     else this._clearLocalRange(start, length)
 
@@ -779,6 +781,10 @@ class Peer {
     // sync from now on, so safe to delete from the map
     this.remoteRequests.delete(req.msg.id)
 
+    if (!this.isActive() && proof.block !== null) {
+      return
+    }
+
     if (proof === null) {
       if (req.msg.manifest && this.core.header.manifest) {
         const manifest = this.core.header.manifest
@@ -1389,7 +1395,7 @@ class Peer {
   }
 
   isActive () {
-    if (this.paused || this.removed) return false
+    if (this.paused || this.removed || this.core.header.frozen) return false
     return true
   }
 
@@ -2120,6 +2126,8 @@ module.exports = class Replicator {
   }
 
   _onwant (peer, start, length) {
+    if (!peer.isActive()) return
+
     const contig = Math.min(this.core.state.length, this.core.header.hints.contiguousLength)
 
     if (start + length < contig || (this.core.state.length === contig)) {

test/conflicts.js

@@ -11,10 +11,14 @@ test.skip('one forks', async function (t) {
   const a = await create(t)
   await a.append(['a', 'b', 'c', 'd', 'e'])
 
+  a.core.name = 'a'
+
   const b = await create(t, a.key)
+  b.core.name = 'b'
 
   const c = await create(t, { keyPair: a.core.header.keyPair })
   await c.append(['a', 'b', 'c', 'd', 'f', 'e'])
+  c.core.name = 'c'
 
   const streams = replicate(a, b, t)