Add V1 consensus upgrade for double spends (#2201)

* Add additional nullifier check for new blocks

* fix lint

* Make verification consensus based

I moved verification of V1 upgrtade to a specific phase that's related
to verifying on connect instead of being specifically about
nullifiers.

* Added another test for re-org case

Co-authored-by: Jason Spafford <nullprogrammer@gmail.com>

ironfish/src/blockchain/blockchain.test.ts

@@ -10,6 +10,7 @@ import {
   useBlockWithTx,
   useMinerBlockFixture,
   useMinersTxFixture,
+  useTxFixture,
   useTxSpendsFixture,
 } from '../testUtilities'
 import { makeBlockAfter } from '../testUtilities/helpers/blockchain'
@@ -770,4 +771,155 @@ describe('Blockchain', () => {
       reason: VerificationResultReason.MINERS_FEE_MISMATCH,
     })
   })
+
+  it('rejects double spend transactions', async () => {
+    /**
+     * This test tests that our double spend code is working properly. We had a
+     * bug that allowed double spends, but fixed the bug at roughly 200k blocks
+     * in. So we need to test that blocks are allowed in before the block
+     * activation of the fix.
+     */
+    const { node, chain } = await nodeTest.createSetup()
+
+    // Set this up so we can reject block with a double spend starting at sequence 5
+    node.chain.consensus.V1_DOUBLE_SPEND = 5
+
+    const accountA = await useAccountFixture(node.accounts, 'accountA')
+    const accountB = await useAccountFixture(node.accounts, 'accountB')
+
+    const block2 = await useMinerBlockFixture(chain, 2, accountA)
+    await expect(chain).toAddBlock(block2)
+
+    // Now create the double spend
+    await node.accounts.updateHead()
+    const tx = await useTxFixture(node.accounts, accountA, accountB)
+    const doubleSpend = tx.getSpend(0)
+
+    const treeSize = await node.chain.nullifiers.size()
+
+    // The nullifier is not found in the tree
+    await expect(node.chain.nullifiers.contains(doubleSpend.nullifier)).resolves.toBe(false)
+    await expect(
+      node.chain.nullifiers.contained(doubleSpend.nullifier, treeSize),
+    ).resolves.toBe(false)
+
+    // Let's spend the transaaction for the first time
+    const block3 = await useMinerBlockFixture(node.chain, 3, undefined, undefined, [tx])
+    await expect(node.chain).toAddBlock(block3)
+
+    // The nullifier is not found at the old tree size, but is found at the new tree size
+    await expect(node.chain.nullifiers.contains(doubleSpend.nullifier)).resolves.toBe(true)
+    await expect(
+      node.chain.nullifiers.contained(doubleSpend.nullifier, treeSize),
+    ).resolves.toBe(false)
+    await expect(
+      node.chain.nullifiers.contained(doubleSpend.nullifier, treeSize + tx.spendsLength()),
+    ).resolves.toBe(true)
+
+    // Let's spend the transaction a second time
+    const block4 = await useMinerBlockFixture(node.chain, 4, undefined, undefined, [tx])
+    await expect(node.chain).toAddBlock(block4)
+
+    // We've now added a double spend, we can see that because of a bug in the
+    // MerkleTree implementation the nullifier is no longer found at the tree
+    // size before this block, but moved to the tree size at block3
+    await expect(node.chain.nullifiers.contains(doubleSpend.nullifier)).resolves.toBe(true)
+    await expect(
+      node.chain.nullifiers.contained(doubleSpend.nullifier, treeSize + tx.spendsLength()),
+    ).resolves.toBe(false)
+    await expect(
+      node.chain.nullifiers.contained(
+        doubleSpend.nullifier,
+        treeSize + tx.spendsLength() + tx.spendsLength(),
+      ),
+    ).resolves.toBe(true)
+
+    // Now we set our fix to activate at sequence 5 so this block will not let
+    // us add the transaction a third time
+    const block5 = await useMinerBlockFixture(node.chain, 5, undefined, undefined, [tx])
+    await expect(node.chain.addBlock(block5)).resolves.toMatchObject({
+      isAdded: false,
+      reason: VerificationResultReason.DOUBLE_SPEND,
+    })
+  })
+
+  it('rejects double spend during reorg', async () => {
+    // G -> A2 -> A3
+    //   -> B2 -> B3* -> A4
+    const { node: nodeA } = await nodeTest.createSetup()
+    const { node: nodeB } = await nodeTest.createSetup()
+
+    nodeA.chain.consensus.V1_DOUBLE_SPEND = 0
+    nodeB.chain.consensus.V1_DOUBLE_SPEND = 5
+
+    const blockA2 = await useMinerBlockFixture(nodeA.chain, 2)
+    await expect(nodeA.chain).toAddBlock(blockA2)
+    const blockA3 = await useMinerBlockFixture(nodeA.chain, 3)
+    await expect(nodeA.chain).toAddBlock(blockA3)
+    const blockA4 = await useMinerBlockFixture(nodeA.chain, 4)
+    await expect(nodeA.chain).toAddBlock(blockA4)
+    const blockA5 = await useMinerBlockFixture(nodeA.chain, 4)
+    await expect(nodeA.chain).toAddBlock(blockA5)
+
+    const accountA = await useAccountFixture(nodeB.accounts, 'accountA')
+    const accountB = await useAccountFixture(nodeB.accounts, 'accountB')
+
+    // Now create the double spend chain
+    const blockB2 = await useMinerBlockFixture(nodeB.chain, 2, accountA)
+    await expect(nodeB.chain).toAddBlock(blockB2)
+
+    // Now create the double spend tx
+    await nodeB.accounts.updateHead()
+    const tx = await useTxFixture(nodeB.accounts, accountA, accountB)
+
+    const blockB3 = await useMinerBlockFixture(nodeB.chain, 3, undefined, undefined, [tx])
+    await expect(nodeB.chain).toAddBlock(blockB3)
+
+    const blockB4 = await useMinerBlockFixture(nodeB.chain, 4, undefined, undefined, [tx])
+    await expect(nodeB.chain).toAddBlock(blockB4)
+
+    const blockB5 = await useMinerBlockFixture(nodeB.chain, 5)
+    await expect(nodeB.chain).toAddBlock(blockB5)
+
+    const blockB6 = await useMinerBlockFixture(nodeB.chain, 6)
+    await expect(nodeB.chain).toAddBlock(blockB6)
+
+    // Now start adding the double spend chain until we reorg to it
+    await expect(nodeA.chain.addBlock(blockB2)).resolves.toMatchObject({
+      isAdded: true,
+      isFork: true,
+    })
+
+    await expect(nodeA.chain.addBlock(blockB3)).resolves.toMatchObject({
+      isAdded: true,
+      isFork: true,
+    })
+
+    await expect(nodeA.chain.addBlock(blockB4)).resolves.toMatchObject({
+      isAdded: true,
+      isFork: true,
+    })
+
+    const addedB5 = await nodeA.chain.addBlock(blockB5)
+    const addedB6 = await nodeA.chain.addBlock(blockB6)
+
+    if (!addedB5.isAdded) {
+      expect(addedB5).toMatchObject({
+        isAdded: false,
+        isFork: null,
+        reason: VerificationResultReason.DOUBLE_SPEND,
+      })
+    } else {
+      expect(addedB5).toMatchObject({
+        isAdded: true,
+        isFork: true,
+      })
+
+      expect(addedB6).toMatchObject({
+        isAdded: false,
+        isFork: null,
+        reason: VerificationResultReason.DOUBLE_SPEND,
+      })
+    }
+  })
 })

ironfish/src/blockchain/blockchain.ts

@@ -6,10 +6,12 @@ import LRU from 'blru'
 import { BufferMap } from 'buffer-map'
 import { Assert } from '../assert'
 import {
+  ConsensusParameters,
   GENESIS_BLOCK_PREVIOUS,
   GENESIS_BLOCK_SEQUENCE,
   MAX_SYNCED_AGE_MS,
   TARGET_BLOCK_TIME_IN_SECONDS,
+  TestnetParameters,
 } from '../consensus'
 import { VerificationResultReason, Verifier } from '../consensus/verifier'
 import { Event } from '../event'
@@ -68,6 +70,7 @@ export class Blockchain {
   metrics: MetricsMonitor
   location: string
   files: FileSystem
+  consensus: ConsensusParameters
 
   synced = false
   opened = false
@@ -169,6 +172,7 @@ export class Blockchain {
     this.orphans = new LRU(100, null, BufferMap)
     this.logAllBlockAdd = options.logAllBlockAdd || false
     this.autoSeed = options.autoSeed ?? true
+    this.consensus = new TestnetParameters()
 
     // Flat Fields
     this.meta = this.db.addStore({
@@ -1234,6 +1238,15 @@ export class Blockchain {
     tx: IDatabaseTransaction,
   ): Promise<void> {
     // TODO: transaction goes here
+
+    const { valid, reason } = await this.verifier.verifyBlockConnect(block, tx)
+
+    if (!valid) {
+      Assert.isNotUndefined(reason)
+      this.addInvalid(block.header.hash, reason)
+      throw new VerifyError(reason, BAN_SCORE.MAX)
+    }
+
     if (prev) {
       await this.hashToNextHash.put(prev.hash, block.header.hash, tx)
     }

ironfish/src/consensus/consensus.ts

@@ -64,3 +64,24 @@ export const GRAFFITI_SIZE = 32
  * It's used in calculating how much a miner should get in rewards.
  */
 export const IRON_FISH_YEAR_IN_BLOCKS = (365 * 24 * 60 * 60) / TARGET_BLOCK_TIME_IN_SECONDS
+
+export class ConsensusParameters {
+  /**
+   * Before upgrade V1 we had double spends. At this block we do a double spend
+   * check to disallow it.
+   *
+   * TODO: remove this sequence check before mainnet
+   */
+  V1_DOUBLE_SPEND = 0
+
+  isActive(upgrade: number, sequence: number): boolean {
+    return sequence >= upgrade
+  }
+}
+
+export class TestnetParameters extends ConsensusParameters {
+  constructor() {
+    super()
+    this.V1_DOUBLE_SPEND = 204000
+  }
+}

ironfish/src/consensus/verifier.ts

@@ -348,6 +348,36 @@ export class Verifier {
     })
   }
 
+  /**
+   * Verify the block before connecting it to the main chain
+   */
+  async verifyBlockConnect(
+    block: Block,
+    tx?: IDatabaseTransaction,
+  ): Promise<VerificationResult> {
+    if (
+      this.chain.consensus.isActive(this.chain.consensus.V1_DOUBLE_SPEND, block.header.sequence)
+    ) {
+      // Loop over all spends in the block and check that the nullifier has not previously been spent
+      const seen = new BufferSet()
+      const size = await this.chain.nullifiers.size(tx)
+
+      for (const spend of block.spends()) {
+        if (seen.has(spend.nullifier)) {
+          return { valid: false, reason: VerificationResultReason.DOUBLE_SPEND }
+        }
+
+        if (await this.chain.nullifiers.contained(spend.nullifier, size, tx)) {
+          return { valid: false, reason: VerificationResultReason.DOUBLE_SPEND }
+        }
+
+        seen.add(spend.nullifier)
+      }
+    }
+
+    return { valid: true }
+  }
+
   /**
    * Verify that the given spend was not in the nullifiers tree when it was the given size,
    * and that the root of the notes tree is the one that is actually associated with the

ironfish/src/testUtilities/fixtures.ts

@@ -189,16 +189,18 @@ export async function useMinerBlockFixture(
   sequence?: number,
   account?: Account,
   addTransactionsTo?: Accounts,
+  transactions: Transaction[] = [],
 ): Promise<Block> {
   const spendingKey = account ? account.spendingKey : generateKey().spending_key
+  const transactionFees = transactions.reduce((a, t) => a + t.fee(), BigInt(0))
 
   return await useBlockFixture(
     chain,
     async () =>
       chain.newBlock(
-        [],
+        transactions,
         await chain.strategy.createMinersFee(
-          BigInt(0),
+          transactionFees,
           sequence || chain.head.sequence + 1,
           spendingKey,
         ),