From e95fef35042e0af13a382eaf463c01a1fc98ee9b Mon Sep 17 00:00:00 2001 From: Mike Hearn Date: Wed, 28 May 2014 18:44:43 +0200 Subject: [PATCH] Bloom bugfix: track false positives in blocks including when the tx was broadcast within the session, and don't print an error from the wallet in this case. Should have no impact beyond more accurate FP rate calculations. --- .../bitcoin/core/AbstractBlockChain.java | 21 +++++++++++-------- .../core/AbstractBlockChainListener.java | 5 +++-- .../bitcoin/core/BlockChainListener.java | 12 ++++++++--- .../java/com/google/bitcoin/core/Wallet.java | 11 +++++----- .../bitcoin/jni/NativeBlockChainListener.java | 4 ++-- 5 files changed, 32 insertions(+), 21 deletions(-) diff --git a/core/src/main/java/com/google/bitcoin/core/AbstractBlockChain.java b/core/src/main/java/com/google/bitcoin/core/AbstractBlockChain.java index 9e13de3b..3202ae00 100644 --- a/core/src/main/java/com/google/bitcoin/core/AbstractBlockChain.java +++ b/core/src/main/java/com/google/bitcoin/core/AbstractBlockChain.java @@ -498,8 +498,8 @@ public abstract class AbstractBlockChain { // (in the case of the listener being a wallet). Wallets need to know how deep each transaction is so // coinbases aren't used before maturity. boolean first = true; - Set falsePositives = Sets.newHashSet(); - if (filteredTxn != null) falsePositives.addAll(filteredTxn.values()); + Set falsePositives = Sets.newHashSet(); + if (filteredTxHashList != null) falsePositives.addAll(filteredTxHashList); for (final ListenerRegistration registration : listeners) { if (registration.executor == Threading.SAME_THREAD) { informListenerForNewTransactions(block, newBlockType, filteredTxHashList, filteredTxn, @@ -514,7 +514,7 @@ public abstract class AbstractBlockChain { public void run() { try { // We can't do false-positive handling when executing on another thread - Set ignoredFalsePositives = Sets.newHashSet(); + Set ignoredFalsePositives = Sets.newHashSet(); informListenerForNewTransactions(block, newBlockType, filteredTxHashList, filteredTxn, newStoredBlock, notFirst, registration.listener, ignoredFalsePositives); if (newBlockType == NewBlockType.BEST_CHAIN) @@ -539,7 +539,7 @@ public abstract class AbstractBlockChain { @Nullable Map filteredTxn, StoredBlock newStoredBlock, boolean first, BlockChainListener listener, - Set falsePositives) throws VerificationException { + Set falsePositives) throws VerificationException { if (block.transactions != null) { // If this is not the first wallet, ask for the transactions to be duplicated before being given // to the wallet when relevant. This ensures that if we have two connected wallets and a tx that @@ -556,11 +556,14 @@ public abstract class AbstractBlockChain { int relativityOffset = 0; for (Sha256Hash hash : filteredTxHashList) { Transaction tx = filteredTxn.get(hash); - if (tx != null) + if (tx != null) { sendTransactionsToListener(newStoredBlock, newBlockType, listener, relativityOffset, Arrays.asList(tx), !first, falsePositives); - else - listener.notifyTransactionIsInBlock(hash, newStoredBlock, newBlockType, relativityOffset); + } else { + if (listener.notifyTransactionIsInBlock(hash, newStoredBlock, newBlockType, relativityOffset)) { + falsePositives.remove(hash); + } + } relativityOffset++; } } @@ -726,11 +729,11 @@ public abstract class AbstractBlockChain { int relativityOffset, List transactions, boolean clone, - Set falsePositives) throws VerificationException { + Set falsePositives) throws VerificationException { for (Transaction tx : transactions) { try { if (listener.isTransactionRelevant(tx)) { - falsePositives.remove(tx); + falsePositives.remove(tx.getHash()); if (clone) tx = new Transaction(tx.params, tx.bitcoinSerialize()); listener.receiveFromBlock(tx, block, blockType, relativityOffset++); diff --git a/core/src/main/java/com/google/bitcoin/core/AbstractBlockChainListener.java b/core/src/main/java/com/google/bitcoin/core/AbstractBlockChainListener.java index 73d805a7..adabca0a 100644 --- a/core/src/main/java/com/google/bitcoin/core/AbstractBlockChainListener.java +++ b/core/src/main/java/com/google/bitcoin/core/AbstractBlockChainListener.java @@ -41,7 +41,8 @@ public class AbstractBlockChainListener implements BlockChainListener { } @Override - public void notifyTransactionIsInBlock(Sha256Hash txHash, StoredBlock block, BlockChain.NewBlockType blockType, - int relativityOffset) throws VerificationException { + public boolean notifyTransactionIsInBlock(Sha256Hash txHash, StoredBlock block, BlockChain.NewBlockType blockType, + int relativityOffset) throws VerificationException { + return false; } } diff --git a/core/src/main/java/com/google/bitcoin/core/BlockChainListener.java b/core/src/main/java/com/google/bitcoin/core/BlockChainListener.java index 44071fd0..bbba0ca4 100644 --- a/core/src/main/java/com/google/bitcoin/core/BlockChainListener.java +++ b/core/src/main/java/com/google/bitcoin/core/BlockChainListener.java @@ -81,8 +81,14 @@ public interface BlockChainListener { *

The relativityOffset parameter in this case is an arbitrary (meaningless) number, that is useful only when * compared to the relativity count of another transaction received inside the same block. It is used to establish * an ordering of transactions relative to one another.

+ * + *

This method should return false if the given tx hash isn't known about, e.g. because the the transaction was + * a Bloom false positive. If it was known about and stored, it should return true. The caller may need to know + * this to calculate the effective FP rate.

+ * + * @return whether the transaction is known about i.e. was considered relevant previously. */ - void notifyTransactionIsInBlock(Sha256Hash txHash, StoredBlock block, - BlockChain.NewBlockType blockType, - int relativityOffset) throws VerificationException; + boolean notifyTransactionIsInBlock(Sha256Hash txHash, StoredBlock block, + BlockChain.NewBlockType blockType, + int relativityOffset) throws VerificationException; } diff --git a/core/src/main/java/com/google/bitcoin/core/Wallet.java b/core/src/main/java/com/google/bitcoin/core/Wallet.java index 0cba7518..7a14ece6 100644 --- a/core/src/main/java/com/google/bitcoin/core/Wallet.java +++ b/core/src/main/java/com/google/bitcoin/core/Wallet.java @@ -617,9 +617,9 @@ public class Wallet extends BaseTaggableObject implements Serializable, BlockCha * inactive side chain. We must still record these transactions and the blocks they appear in because a future * block might change which chain is best causing a reorganize. A re-org can totally change our balance! */ - public void notifyTransactionIsInBlock(Sha256Hash txHash, StoredBlock block, - BlockChain.NewBlockType blockType, - int relativityOffset) throws VerificationException { + public boolean notifyTransactionIsInBlock(Sha256Hash txHash, StoredBlock block, + BlockChain.NewBlockType blockType, + int relativityOffset) throws VerificationException { lock.lock(); try { Transaction tx = transactions.get(txHash); @@ -629,8 +629,8 @@ public class Wallet extends BaseTaggableObject implements Serializable, BlockCha // If this happens our risk analysis is probably wrong and should be improved. log.info("Risk analysis dropped tx {} but was included in block anyway", tx.getHash()); } else { - log.error("TX {} not found despite being sent to wallet", txHash); - return; + // False positive that was broadcast to us and ignored by us because it was irrelevant to our keys. + return false; } } receive(tx, block, blockType, relativityOffset); @@ -642,6 +642,7 @@ public class Wallet extends BaseTaggableObject implements Serializable, BlockCha // This has to run outside the wallet lock as it may trigger broadcasting of new transactions. maybeRotateKeys(); } + return true; } /** diff --git a/core/src/main/java/com/google/bitcoin/jni/NativeBlockChainListener.java b/core/src/main/java/com/google/bitcoin/jni/NativeBlockChainListener.java index 2175b154..0291b4d7 100644 --- a/core/src/main/java/com/google/bitcoin/jni/NativeBlockChainListener.java +++ b/core/src/main/java/com/google/bitcoin/jni/NativeBlockChainListener.java @@ -42,6 +42,6 @@ public class NativeBlockChainListener implements BlockChainListener { int relativityOffset) throws VerificationException; @Override - public native void notifyTransactionIsInBlock(Sha256Hash txHash, StoredBlock block, BlockChain.NewBlockType blockType, - int relativityOffset) throws VerificationException; + public native boolean notifyTransactionIsInBlock(Sha256Hash txHash, StoredBlock block, BlockChain.NewBlockType blockType, + int relativityOffset) throws VerificationException; }