diff --git a/core/src/main/java/com/google/bitcoin/core/AbstractWalletEventListener.java b/core/src/main/java/com/google/bitcoin/core/AbstractWalletEventListener.java index 4d48b337..ee777d89 100644 --- a/core/src/main/java/com/google/bitcoin/core/AbstractWalletEventListener.java +++ b/core/src/main/java/com/google/bitcoin/core/AbstractWalletEventListener.java @@ -95,10 +95,13 @@ public abstract class AbstractWalletEventListener implements WalletEventListener onChange(); } + public void onWalletChanged(Wallet wallet) { + onChange(); + } + /** * Called by the other default method implementations when something (anything) changes in the wallet. */ public void onChange() { } - } 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 6a2a9ab9..99ac6244 100644 --- a/core/src/main/java/com/google/bitcoin/core/Wallet.java +++ b/core/src/main/java/com/google/bitcoin/core/Wallet.java @@ -202,6 +202,16 @@ public class Wallet implements Serializable { txConfidenceListener = new TransactionConfidence.Listener() { public void onConfidenceChanged(Transaction tx) { invokeOnTransactionConfidenceChanged(tx); + // Many onWalletChanged events will not occur because they are suppressed, eg, because: + // - we are inside a re-org + // - we are in the middle of processing a block + // - the confidence is changing because a new best block was accepted + // It will run in cases like: + // - the tx is pending and another peer announced it + // - the tx is pending and was killed by a detected double spend that was not in a block + // The latter case cannot happen today because we won't hear about it, but in future this may + // become more common if conflict notices are implemented. + invokeOnWalletChanged(); } }; } @@ -714,6 +724,8 @@ public class Wallet implements Serializable { bitcoinValueToFriendlyString(valueDifference), tx.getHashAsString()}); } + onWalletChangedSuppressions++; + // If this transaction is already in the wallet we may need to move it into a different pool. At the very // least we need to ensure we're manipulating the canonical object rather than a duplicate. Transaction wtx; @@ -814,6 +826,9 @@ public class Wallet implements Serializable { } } + // Wallet change notification will be sent shortly after the block is finished processing, in notifyNewBestBlock + onWalletChangedSuppressions--; + checkState(isConsistent()); queueAutoSave(); } @@ -824,28 +839,33 @@ public class Wallet implements Serializable { * not be called (the {@link Wallet#reorganize(StoredBlock, java.util.List, java.util.List)} method will * call this one in that case).

* - *

Used to update confidence data in each transaction and last seen block hash. Triggers auto saving.

+ *

Used to update confidence data in each transaction and last seen block hash. Triggers auto saving. + * Invokes the onWalletChanged event listener if there were any affected transactions.

*/ public synchronized void notifyNewBestBlock(Block block) throws VerificationException { // Check to see if this block has been seen before. Sha256Hash newBlockHash = block.getHash(); - if (!newBlockHash.equals(getLastBlockSeenHash())) { - // Store the new block hash. - setLastBlockSeenHash(newBlockHash); - // Notify all the BUILDING transactions of the new block. - // This is so that they can update their work done and depth. - Set transactions = getTransactions(true, false); - for (Transaction tx : transactions) { - if (ignoreNextNewBlock.contains(tx.getHash())) { - // tx was already processed in receive() due to it appearing in this block, so we don't want to - // notify the tx confidence of work done twice, it'd result in miscounting. - ignoreNextNewBlock.remove(tx.getHash()); - } else { - tx.getConfidence().notifyWorkDone(block); - } + if (newBlockHash.equals(getLastBlockSeenHash())) + return; + // Store the new block hash. + setLastBlockSeenHash(newBlockHash); + // TODO: Clarify the code below. + // Notify all the BUILDING transactions of the new block. + // This is so that they can update their work done and depth. + onWalletChangedSuppressions++; + Set transactions = getTransactions(true, false); + for (Transaction tx : transactions) { + if (ignoreNextNewBlock.contains(tx.getHash())) { + // tx was already processed in receive() due to it appearing in this block, so we don't want to + // notify the tx confidence of work done twice, it'd result in miscounting. + ignoreNextNewBlock.remove(tx.getHash()); + } else { + tx.getConfidence().notifyWorkDone(block); } - queueAutoSave(); } + queueAutoSave(); + onWalletChangedSuppressions--; + invokeOnWalletChanged(); } /** @@ -1047,6 +1067,8 @@ public class Wallet implements Serializable { invokeOnCoinsReceived(tx, balance, newBalance); if (valueSentFromMe.compareTo(BigInteger.ZERO) > 0) invokeOnCoinsSent(tx, balance, newBalance); + + invokeOnWalletChanged(); } catch (ScriptException e) { // Cannot happen as we just created this transaction ourselves. throw new RuntimeException(e); @@ -1809,6 +1831,10 @@ public class Wallet implements Serializable { log.info(affectedUs ? "Re-org affected our transactions" : "Re-org had no effect on our transactions"); if (!affectedUs) return; + // Avoid spuriously informing the user of wallet changes whilst we're re-organizing. This also prevents the + // user from modifying wallet contents (eg, trying to spend) whilst we're in the middle of the process. + onWalletChangedSuppressions++; + // For simplicity we will reprocess every transaction to ensure it's in the right bucket and has the right // connections. Attempting to update each one with minimal work is possible but complex and was leading to // edge cases that were hard to fix. As re-orgs are rare the amount of work this implies should be manageable @@ -1966,6 +1992,8 @@ public class Wallet implements Serializable { listener.onReorganize(Wallet.this); } }); + onWalletChangedSuppressions--; + invokeOnWalletChanged(); checkState(isConsistent()); } @@ -2050,6 +2078,21 @@ public class Wallet implements Serializable { }); } + private int onWalletChangedSuppressions; + private synchronized void invokeOnWalletChanged() { + // Don't invoke the callback in some circumstances, eg, whilst we are re-organizing or fiddling with + // transactions due to a new block arriving. It will be called later instead. + Preconditions.checkState(onWalletChangedSuppressions >= 0); + if (onWalletChangedSuppressions > 0) return; + // Call with the wallet locked. + EventListenerInvoker.invoke(eventListeners, new EventListenerInvoker() { + @Override + public void invoke(WalletEventListener listener) { + listener.onWalletChanged(Wallet.this); + } + }); + } + /** * Returns an immutable view of the transactions currently waiting for network confirmations. */ diff --git a/core/src/main/java/com/google/bitcoin/core/WalletEventListener.java b/core/src/main/java/com/google/bitcoin/core/WalletEventListener.java index 10a3baf1..98e5ff32 100644 --- a/core/src/main/java/com/google/bitcoin/core/WalletEventListener.java +++ b/core/src/main/java/com/google/bitcoin/core/WalletEventListener.java @@ -61,28 +61,27 @@ public interface WalletEventListener { */ void onCoinsSent(Wallet wallet, Transaction tx, BigInteger prevBalance, BigInteger newBalance); + // TODO: Finish onReorganize to be more useful. /** - * This is called on a Peer thread when a block is received that triggers a block chain re-organization.

- *

- * A re-organize means that the consensus (chain) of the network has diverged and now changed from what we + *

This is called on a Peer thread when a block is received that triggers a block chain re-organization. + *

+ *

A re-organize means that the consensus (chain) of the network has diverged and now changed from what we * believed it was previously. Usually this won't matter because the new consensus will include all our old * transactions assuming we are playing by the rules. However it's theoretically possible for our balance to - * change in arbitrary ways, most likely, we could lose some money we thought we had.

- *

- * It is safe to use methods of wallet whilst inside this callback. - *

- * TODO: Finish this interface. + * change in arbitrary ways, most likely, we could lose some money we thought we had.

+ * + *

It is safe to use methods of wallet whilst inside this callback.

*/ void onReorganize(Wallet wallet); // TODO: Flesh out the docs below some more to clarify what happens during re-orgs and other edge cases. /** - * Called on a Peer thread when a transaction changes its confidence level. You can also attach event listeners to + *

Called on a Peer thread when a transaction changes its confidence level. You can also attach event listeners to * the individual transactions, if you don't care about all of them. Usually you would save the wallet to disk after - * receiving this callback.

+ * receiving this callback unless you already set up autosaving.

* - * You should pay attention to this callback in case a transaction becomes dead, that is, a transaction you - * believed to be active (send or receive) becomes overridden by the network. This can happen if

+ *

You should pay attention to this callback in case a transaction becomes dead, that is, a transaction + * you believed to be active (send or receive) becomes overridden by the network. This can happen if

* *
    *
  1. You are sharing keys between wallets and accidentally create/broadcast a double spend.
    2. @@ -91,12 +90,37 @@ public interface WalletEventListener { * will then re-use the same outputs when creating the next spend. *

* - * To find if the transaction is dead, you can use tx.getConfidence().getConfidenceType() == + *

To find if the transaction is dead, you can use tx.getConfidence().getConfidenceType() == * TransactionConfidence.ConfidenceType.DEAD. If it is, you should notify the user - * in some way so they know the thing they bought may not arrive/the thing they sold should not be dispatched. + * in some way so they know the thing they bought may not arrive/the thing they sold should not be dispatched.

+ * + *

Note that this callback will be invoked for every transaction in the wallet, for every new block that is + * received (because the depth has changed). If you want to update a UI view from the contents of the wallet + * it is more efficient to use onWalletChanged instead.

*/ void onTransactionConfidenceChanged(Wallet wallet, Transaction tx); + /** + *

Designed for GUI applications to refresh their transaction lists. This callback is invoked in the following + * situations:

+ * + *
    + *
  1. A new block is received (and thus building transactions got more confidence)
    2. + *
  2. A pending transaction is received
    3. + *
  3. A pending transaction changes confidence due to some non-new-block related event, such as being + * announced by more peers or by a double-spend conflict being observed.
    4. + *
  4. A re-organize occurs. Call occurs only if the re-org modified any of our transactions.
    5. + *
  5. A new spend is committed to the wallet
    6. + *
+ * + *

When this is called you can refresh the UI contents from the wallet contents. It's more efficient to use + * this rather than onTransactionConfidenceChanged() + onReorganize() because you only get one callback per block + * rather than one per transaction per block. Note that this is not called when a key is added. The wallet + * is locked whilst this handler is invoked, but if you relay the callback into another thread (eg the + * main UI thread) you should ensure to lock the wallet in the new thread as well.

+ */ + void onWalletChanged(Wallet wallet); + /** * Called by the {@link Wallet#addKey(ECKey)} method on whatever the calling thread was. */ diff --git a/core/src/test/java/com/google/bitcoin/core/ChainSplitTest.java b/core/src/test/java/com/google/bitcoin/core/ChainSplitTest.java index bf194cda..18795bbd 100644 --- a/core/src/test/java/com/google/bitcoin/core/ChainSplitTest.java +++ b/core/src/test/java/com/google/bitcoin/core/ChainSplitTest.java @@ -57,12 +57,17 @@ public class ChainSplitTest { // Check that if the block chain forks, we end up using the right chain. Only tests inbound transactions // (receiving coins). Checking that we understand reversed spends is in testForking2. final boolean[] reorgHappened = new boolean[1]; - reorgHappened[0] = false; + final int[] walletChanged = new int[1]; wallet.addEventListener(new AbstractWalletEventListener() { @Override public void onReorganize(Wallet wallet) { reorgHappened[0] = true; } + + @Override + public void onWalletChanged(Wallet wallet) { + walletChanged[0]++; + } }); // Start by building a couple of blocks on top of the genesis block. @@ -71,6 +76,7 @@ public class ChainSplitTest { assertTrue(chain.add(b1)); assertTrue(chain.add(b2)); assertFalse(reorgHappened[0]); + assertEquals(2, walletChanged[0]); // We got two blocks which generated 50 coins each, to us. assertEquals("100.00", Utils.bitcoinValueToFriendlyString(wallet.getBalance())); // We now have the following chain: @@ -85,10 +91,12 @@ public class ChainSplitTest { Block b3 = b1.createNextBlock(someOtherGuy); assertTrue(chain.add(b3)); assertFalse(reorgHappened[0]); // No re-org took place. + assertEquals(2, walletChanged[0]); assertEquals("100.00", Utils.bitcoinValueToFriendlyString(wallet.getBalance())); // Now we add another block to make the alternative chain longer. assertTrue(chain.add(b3.createNextBlock(someOtherGuy))); assertTrue(reorgHappened[0]); // Re-org took place. + assertEquals(3, walletChanged[0]); reorgHappened[0] = false; // // genesis -> b1 -> b2 @@ -106,6 +114,7 @@ public class ChainSplitTest { // \-> b3 -> b4 // assertTrue(reorgHappened[0]); + assertEquals(4, walletChanged[0]); assertEquals("200.00", Utils.bitcoinValueToFriendlyString(wallet.getBalance())); } diff --git a/core/src/test/java/com/google/bitcoin/core/WalletTest.java b/core/src/test/java/com/google/bitcoin/core/WalletTest.java index 99a908d9..3c4b9249 100644 --- a/core/src/test/java/com/google/bitcoin/core/WalletTest.java +++ b/core/src/test/java/com/google/bitcoin/core/WalletTest.java @@ -21,6 +21,7 @@ import com.google.bitcoin.core.WalletTransaction.Pool; import com.google.bitcoin.store.BlockStore; import com.google.bitcoin.store.MemoryBlockStore; import com.google.bitcoin.utils.BriefLogFormatter; +import com.google.common.collect.Lists; import org.junit.Before; import org.junit.Test; @@ -91,19 +92,19 @@ public class WalletTest { // We have NOT proven that the signature is correct! - final Transaction[] txns = new Transaction[1]; + final LinkedList txns = Lists.newLinkedList(); wallet.addEventListener(new AbstractWalletEventListener() { @Override public void onCoinsSent(Wallet wallet, Transaction tx, BigInteger prevBalance, BigInteger newBalance) { - assertNull(txns[0]); - txns[0] = tx; + txns.add(tx); } }); wallet.commitTx(t2); assertEquals(1, wallet.getPoolSize(WalletTransaction.Pool.PENDING)); assertEquals(1, wallet.getPoolSize(WalletTransaction.Pool.SPENT)); assertEquals(2, wallet.getPoolSize(WalletTransaction.Pool.ALL)); - assertEquals(t2, txns[0]); + assertEquals(t2, txns.getFirst()); + assertEquals(1, txns.size()); } @Test @@ -400,6 +401,7 @@ public class WalletTest { // This needs to work both for transactions we create, and that we receive from others. final Transaction[] eventDead = new Transaction[1]; final Transaction[] eventReplacement = new Transaction[1]; + final int[] eventWalletChanged = new int[1]; wallet.addEventListener(new AbstractWalletEventListener() { @Override public void onTransactionConfidenceChanged(Wallet wallet, Transaction tx) { @@ -410,12 +412,19 @@ public class WalletTest { eventReplacement[0] = tx.getConfidence().getOverridingTransaction(); } } + + @Override + public void onWalletChanged(Wallet wallet) { + eventWalletChanged[0]++; + } }); // Receive 1 BTC. BigInteger nanos = Utils.toNanoCoins(1, 0); Transaction t1 = createFakeTx(params, nanos, myAddress); - wallet.receiveFromBlock(t1, null, BlockChain.NewBlockType.BEST_CHAIN); + BlockPair bp1 = createFakeBlock(params, blockStore, t1); + wallet.receiveFromBlock(t1, bp1.storedBlock, BlockChain.NewBlockType.BEST_CHAIN); + wallet.notifyNewBestBlock(bp1.block); // Create a send to a merchant. Transaction send1 = wallet.createSend(new ECKey().toAddress(params), toNanoCoins(0, 50)); // Create a double spend. @@ -424,7 +433,9 @@ public class WalletTest { // Broadcast send1. wallet.commitTx(send1); // Receive a block that overrides it. - wallet.receiveFromBlock(send2, null, BlockChain.NewBlockType.BEST_CHAIN); + BlockPair bp2 = createFakeBlock(params, blockStore, send2); + wallet.receiveFromBlock(send2, bp2.storedBlock, BlockChain.NewBlockType.BEST_CHAIN); + wallet.notifyNewBestBlock(bp2.block); assertEquals(send1, eventDead[0]); assertEquals(send2, eventReplacement[0]); assertEquals(TransactionConfidence.ConfidenceType.DEAD, @@ -435,10 +446,13 @@ public class WalletTest { wallet.receivePending(doubleSpends.t1); assertEquals(TransactionConfidence.ConfidenceType.NOT_SEEN_IN_CHAIN, doubleSpends.t1.getConfidence().getConfidenceType()); - wallet.receiveFromBlock(doubleSpends.t2, null, BlockChain.NewBlockType.BEST_CHAIN); + BlockPair bp3 = createFakeBlock(params, blockStore, doubleSpends.t2); + wallet.receiveFromBlock(doubleSpends.t2, bp3.storedBlock, BlockChain.NewBlockType.BEST_CHAIN); + wallet.notifyNewBestBlock(bp3.block); assertEquals(TransactionConfidence.ConfidenceType.DEAD, doubleSpends.t1.getConfidence().getConfidenceType()); assertEquals(doubleSpends.t2, doubleSpends.t1.getConfidence().getOverridingTransaction()); + assertEquals(5, eventWalletChanged[0]); } @Test @@ -450,6 +464,7 @@ public class WalletTest { // First one is "called" second is "pending". final boolean[] flags = new boolean[2]; final Transaction[] notifiedTx = new Transaction[1]; + final int[] walletChanged = new int[1]; wallet.addEventListener(new AbstractWalletEventListener() { @Override public void onCoinsReceived(Wallet wallet, Transaction tx, BigInteger prevBalance, BigInteger newBalance) { @@ -462,6 +477,11 @@ public class WalletTest { flags[1] = tx.isPending(); notifiedTx[0] = tx; } + + @Override + public void onWalletChanged(Wallet wallet) { + walletChanged[0]++; + } }); wallet.receivePending(t1); @@ -483,8 +503,9 @@ public class WalletTest { assertEquals(TransactionConfidence.ConfidenceType.NOT_SEEN_IN_CHAIN, notifiedTx[0].getConfidence().getConfidenceType()); final Transaction t1Copy = new Transaction(params, t1.bitcoinSerialize()); - wallet.receiveFromBlock(t1Copy, createFakeBlock(params, blockStore, t1Copy).storedBlock, - BlockChain.NewBlockType.BEST_CHAIN); + BlockPair fakeBlock = createFakeBlock(params, blockStore, t1Copy); + wallet.receiveFromBlock(t1Copy, fakeBlock.storedBlock, BlockChain.NewBlockType.BEST_CHAIN); + wallet.notifyNewBestBlock(fakeBlock.block); assertFalse(flags[0]); assertTrue(flags[1]); assertEquals(TransactionConfidence.ConfidenceType.BUILDING, notifiedTx[0].getConfidence().getConfidenceType()); @@ -494,6 +515,7 @@ public class WalletTest { Transaction irrelevant = createFakeTx(params, nanos, new ECKey().toAddress(params)); wallet.receivePending(irrelevant); assertFalse(flags[0]); + assertEquals(2, walletChanged[0]); } @Test