Merge pull request #399 from horizontalsystems/bugfixes

Bugfixes (#395)

BitcoinCore/BitcoinCore/Core/BitcoinCoreBuilder.swift

@@ -201,7 +201,7 @@ public class BitcoinCoreBuilder {
         let scriptBuilder = ScriptBuilderChain()
         let transactionBuilder = TransactionBuilder(unspentOutputSelector: unspentOutputSelector, unspentOutputProvider: unspentOutputProvider, addressManager: addressManager, addressConverter: addressConverter, inputSigner: inputSigner, scriptBuilder: scriptBuilder, factory: factory)
         let transactionSender = TransactionSender(transactionSyncer: transactionSyncer, peerManager: peerManager, initialBlockDownload: initialBlockDownload, syncedReadyPeerManager: syncedReadyPeerManager, logger: logger)
-        let transactionCreator = TransactionCreator(transactionBuilder: transactionBuilder, transactionProcessor: transactionProcessor, transactionSender: transactionSender)
+        let transactionCreator = TransactionCreator(transactionBuilder: transactionBuilder, transactionProcessor: transactionProcessor, transactionSender: transactionSender, bloomFilterManager: bloomFilterManager)
 
         let syncManager = SyncManager(reachabilityManager: reachabilityManager, initialSyncer: initialSyncer, peerGroup: peerGroup)
 

BitcoinCore/BitcoinCore/Managers/BloomFilterManager.swift

@@ -66,11 +66,8 @@ extension BloomFilterManager: IBloomFilterManager {
         }
 
         if !elements.isEmpty {
-            let bloomFilter = factory.bloomFilter(withElements: elements)
-            if self.bloomFilter?.filter != bloomFilter.filter {
-                self.bloomFilter = bloomFilter
-                delegate?.bloomFilterUpdated(bloomFilter: bloomFilter)
-            }
+            bloomFilter = factory.bloomFilter(withElements: elements)
+            delegate?.bloomFilterUpdated(bloomFilter: bloomFilter!)
         }
     }
 

BitcoinCore/BitcoinCore/Transactions/TransactionCreator.swift

@@ -6,11 +6,13 @@ class TransactionCreator {
     private let transactionBuilder: ITransactionBuilder
     private let transactionProcessor: ITransactionProcessor
     private let transactionSender: ITransactionSender
+    private let bloomFilterManager: IBloomFilterManager
 
-    init(transactionBuilder: ITransactionBuilder, transactionProcessor: ITransactionProcessor, transactionSender: ITransactionSender) {
+    init(transactionBuilder: ITransactionBuilder, transactionProcessor: ITransactionProcessor, transactionSender: ITransactionSender, bloomFilterManager: IBloomFilterManager) {
         self.transactionBuilder = transactionBuilder
         self.transactionProcessor = transactionProcessor
         self.transactionSender = transactionSender
+        self.bloomFilterManager = bloomFilterManager
     }
 
 }
@@ -21,7 +23,12 @@ extension TransactionCreator: ITransactionCreator {
         try transactionSender.verifyCanSend()
 
         let transaction = try transactionBuilder.buildTransaction(value: value, feeRate: feeRate, senderPay: senderPay, toAddress: address)
-        try transactionProcessor.processCreated(transaction: transaction)
+
+        do {
+            try transactionProcessor.processCreated(transaction: transaction)
+        } catch _ as BloomFilterManager.BloomFilterExpired {
+            bloomFilterManager.regenerateBloomFilter()
+        }
 
         try transactionSender.send(pendingTransaction: transaction)
     }

BitcoinCore/BitcoinCore/Transactions/TransactionProcessor.swift

@@ -27,8 +27,8 @@ class TransactionProcessor {
         self.queue = queue
     }
 
-    private func hasUnspentOutputs(transaction: FullTransaction) -> Bool {
-        for output in transaction.outputs {
+    private func expiresBloomFilter(outputs: [Output]) -> Bool {
+        for output in outputs {
             if output.publicKeyPath != nil, (output.scriptType == .p2wpkh || output.scriptType == .p2pk || output.scriptType == .p2wpkhSh)  {
                 return true
             }
@@ -53,9 +53,6 @@ class TransactionProcessor {
     }
 
     private func relay(transaction: Transaction, withOrder order: Int, inBlock block: Block?) {
-        guard block != nil || transaction.blockHash == nil else {
-            return
-        }
         transaction.blockHash = block?.headerHash
         transaction.status = .relayed
         transaction.timestamp = block?.timestamp ?? Int(dateGenerator().timeIntervalSince1970)
@@ -80,6 +77,9 @@ extension TransactionProcessor: ITransactionProcessor {
         try queue.sync {
             for (index, transaction) in transactions.inTopologicalOrder().enumerated() {
                 if let existingTransaction = self.storage.transaction(byHash: transaction.header.dataHash) {
+                    if existingTransaction.blockHash != nil && block == nil {
+                        continue
+                    }
                     self.relay(transaction: existingTransaction, withOrder: index, inBlock: block)
                     try self.storage.update(transaction: existingTransaction)
                     updated.append(existingTransaction)
@@ -95,7 +95,7 @@ extension TransactionProcessor: ITransactionProcessor {
                     inserted.append(transaction.header)
 
                     if !skipCheckBloomFilter {
-                        needToUpdateBloomFilter = needToUpdateBloomFilter || self.addressManager.gapShifts() || self.hasUnspentOutputs(transaction: transaction)
+                        needToUpdateBloomFilter = needToUpdateBloomFilter || self.addressManager.gapShifts() || self.expiresBloomFilter(outputs: transaction.outputs)
                     }
                 }
             }
@@ -118,6 +118,10 @@ extension TransactionProcessor: ITransactionProcessor {
         process(transaction: transaction)
         try storage.add(transaction: transaction)
         listener?.onUpdate(updated: [], inserted: [transaction.header], inBlock: nil)
+
+        if expiresBloomFilter(outputs: transaction.outputs) {
+            throw BloomFilterManager.BloomFilterExpired()
+        }
     }
 
 }

BitcoinCore/BitcoinCoreTests/Managers/BloomFilterManagerTests.swift

@@ -164,25 +164,6 @@ class BloomFilterManagerTests: QuickSpec {
                     verify(mockBloomFilterManagerDelegate, never()).bloomFilterUpdated(bloomFilter: any())
                 }
             }
-
-            context("when no new element") {
-                it("doesn't trigger events") {
-                    stub(mockStorage) { mock in
-                        when(mock.publicKeys()).thenReturn([self.getPublicKey(withIndex: 0, chain: .external)])
-                        when(mock.outputsWithPublicKeys()).thenReturn([])
-                    }
-
-                    manager.regenerateBloomFilter()
-                    verify(mockBloomFilterManagerDelegate).bloomFilterUpdated(bloomFilter: any())
-                    reset(mockBloomFilterManagerDelegate)
-                    stub(mockBloomFilterManagerDelegate) { mock in
-                        when(mock.bloomFilterUpdated(bloomFilter: any())).thenDoNothing()
-                    }
-
-                    manager.regenerateBloomFilter()
-                    verify(mockBloomFilterManagerDelegate, never()).bloomFilterUpdated(bloomFilter: any())
-                }
-            }
         }
     }
 

BitcoinCore/BitcoinCoreTests/Transactions/TransactionCreatorTests.swift

@@ -9,12 +9,13 @@ class TransactionCreatorTests: QuickSpec {
         let mockTransactionBuilder = MockITransactionBuilder()
         let mockTransactionProcessor = MockITransactionProcessor()
         let mockTransactionSender = MockITransactionSender()
+        let mockBloomFilterManager = MockIBloomFilterManager()
 
         var transactionCreator: TransactionCreator!
         let transaction = TestData.p2pkhTransaction
 
         afterEach {
-            reset(mockTransactionBuilder, mockTransactionProcessor, mockTransactionSender)
+            reset(mockTransactionBuilder, mockTransactionProcessor, mockTransactionSender, mockBloomFilterManager)
             transactionCreator = nil
         }
 
@@ -29,11 +30,40 @@ class TransactionCreatorTests: QuickSpec {
                 stub(mockTransactionSender) { mock in
                     when(mock.send(pendingTransaction: any())).thenDoNothing()
                 }
+                stub(mockBloomFilterManager) { mock in
+                    when(mock.regenerateBloomFilter()).thenDoNothing()
+                }
+
+                transactionCreator = TransactionCreator(transactionBuilder: mockTransactionBuilder, transactionProcessor: mockTransactionProcessor, transactionSender: mockTransactionSender, bloomFilterManager: mockBloomFilterManager)
+            }
+
+            context("when BloomFilterManager.BloomFilterExpired error") {
+                beforeEach {
+                    stub(mockTransactionProcessor) { mock in
+                        when(mock.processCreated(transaction: any())).thenThrow(BloomFilterManager.BloomFilterExpired())
+                    }
+                    stub(mockTransactionSender) { mock in
+                        when(mock.verifyCanSend()).thenDoNothing()
+                    }
+
+                    try? transactionCreator.create(to: "", value: 0, feeRate: 0, senderPay: false)
+                }
+
+                it("does create transaction") {
+                    verify(mockTransactionBuilder).buildTransaction(value: any(), feeRate: any(), senderPay: any(), toAddress: any())
+                    verify(mockTransactionProcessor).processCreated(transaction: any())
+                }
+
+                it("does send transaction") {
+                    verify(mockTransactionSender).send(pendingTransaction: equal(to: transaction))
+                }
 
-                transactionCreator = TransactionCreator(transactionBuilder: mockTransactionBuilder, transactionProcessor: mockTransactionProcessor, transactionSender: mockTransactionSender)
+                it("regenerates bloomfilter") {
+                    verify(mockBloomFilterManager).regenerateBloomFilter()
+                }
             }
 
-            context("when error") {
+            context("when other error") {
                 beforeEach {
                     stub(mockTransactionSender) { mock in
                         when(mock.verifyCanSend()).thenThrow(BitcoinCoreErrors.TransactionSendError.noConnectedPeers)
@@ -46,6 +76,10 @@ class TransactionCreatorTests: QuickSpec {
                     verify(mockTransactionBuilder, never()).buildTransaction(value: any(), feeRate: any(), senderPay: any(), toAddress: any())
                     verify(mockTransactionProcessor, never()).processCreated(transaction: any())
                 }
+
+                it("doesn't regenerate bloomfilter") {
+                    verify(mockBloomFilterManager, never()).regenerateBloomFilter()
+                }
             }
 
             context("when success") {

BitcoinCore/BitcoinCoreTests/Transactions/TransactionProcessorTests.swift

@@ -129,6 +129,20 @@ class TransactionProcessorTests: XCTestCase {
         verify(mockInputExtractor, never()).extract(transaction: any())
     }
 
+    func testProcessCreated_OutputsExpireBloomFiler() {
+        let transaction = TestData.p2wpkhTransaction
+        transaction.header.isMine = true
+        transaction.outputs[0].publicKeyPath = TestData.pubKey().path
+
+        do {
+            try transactionProcessor.processCreated(transaction: transaction)
+            XCTFail("Expecting error")
+        } catch _ as BloomFilterManager.BloomFilterExpired {
+        } catch {
+            XCTFail("Unexpected error")
+        }
+    }
+
     func testProcessReceived_TransactionExists() {
         let transaction = TestData.p2pkhTransaction
         transaction.header.status = .new
@@ -189,12 +203,24 @@ class TransactionProcessorTests: XCTestCase {
         }
 
         try! transactionProcessor.processReceived(transactions: [transaction], inBlock: block, skipCheckBloomFilter: false)
+        verify(mockStorage).update(transaction: equal(to: transaction.header))
+        verify(mockBlockchainDataListener).onUpdate(updated: equal(to: [transaction.header]), inserted: equal(to: []), inBlock: equal(to: block))
+
+        reset(mockStorage, mockBlockchainDataListener)
+        stub(mockStorage) { mock in
+            when(mock.transaction(byHash: equal(to: transaction.header.dataHash))).thenReturn(transaction.header)
+        }
+
         try! transactionProcessor.processReceived(transactions: [transaction], inBlock: nil, skipCheckBloomFilter: false)
 
+        verify(mockStorage, never()).update(transaction: equal(to: transaction.header))
+        verify(mockBlockchainDataListener, never()).onUpdate(updated: equal(to: [transaction.header]), inserted: equal(to: []), inBlock: equal(to: nil))
+
         XCTAssertEqual(transaction.header.status, TransactionStatus.relayed)
         XCTAssertEqual(transaction.header.blockHash, block.headerHash)
         XCTAssertEqual(transaction.header.timestamp, block.timestamp)
         XCTAssertEqual(transaction.header.order, 0)
+
     }
 
     func testProcessReceivedBlock_After_Block_TransactionExists() {
@@ -208,8 +234,24 @@ class TransactionProcessorTests: XCTestCase {
         }
 
         try! transactionProcessor.processReceived(transactions: [transaction], inBlock: block, skipCheckBloomFilter: false)
+        verify(mockStorage).update(transaction: equal(to: transaction.header))
+        verify(mockBlockchainDataListener).onUpdate(updated: equal(to: [transaction.header]), inserted: equal(to: []), inBlock: equal(to: block))
+
+        reset(mockStorage, mockBlockchainDataListener)
+        stub(mockStorage) { mock in
+            when(mock.update(transaction: any())).thenDoNothing()
+            when(mock.update(block: any())).thenDoNothing()
+            when(mock.transaction(byHash: equal(to: transaction.header.dataHash))).thenReturn(transaction.header)
+        }
+        stub(mockBlockchainDataListener) { mock in
+            when(mock.onUpdate(updated: any(), inserted: any(), inBlock: any())).thenDoNothing()
+        }
+
         try! transactionProcessor.processReceived(transactions: [transaction], inBlock: nextBlock, skipCheckBloomFilter: false)
 
+        verify(mockStorage).update(transaction: equal(to: transaction.header))
+        verify(mockBlockchainDataListener).onUpdate(updated: equal(to: [transaction.header]), inserted: equal(to: []), inBlock: equal(to: nextBlock))
+
         XCTAssertEqual(transaction.header.status, TransactionStatus.relayed)
         XCTAssertEqual(transaction.header.blockHash, nextBlock.headerHash)
         XCTAssertEqual(transaction.header.timestamp, nextBlock.timestamp)
@@ -308,7 +350,7 @@ class TransactionProcessorTests: XCTestCase {
         XCTAssertEqual(transaction.header.blockHash, nil)
     }
 
-    func testProcessReceived_TransactionNotExists_Mine_HasUnspentOutputs() {
+    func testProcessReceived_TransactionNotExists_Mine_OutputsExpireBloomFiler() {
         let transaction = TestData.p2wpkhTransaction
         transaction.header.isMine = true
         transaction.outputs[0].publicKeyPath = TestData.pubKey().path