From 9aed8ea0a57ae6091260017006a1fd66cb49992b Mon Sep 17 00:00:00 2001 From: Andreas Schildbach Date: Thu, 3 Mar 2016 16:53:38 +0100 Subject: [PATCH] For watching wallets, store the key creation time always in the watching key rather than the DeterministicKeyChain. Creation times in the DeterministicKeyChain can't be persisted to protobuf, as that structure has no full-blown protobuf equivalent. This means a couple of DeterministicKeyChain, KeyChainGroup and Wallet factory method variants that take creation dates have been removed. On the other hand, a convenient Wallet.fromWatchingKeyB58(params, xpub, creationDate) has been added. Also adds a test for protobuf-roundtripping watching wallets. Supposed to fix issue #1209. --- .../main/java/org/bitcoinj/core/Wallet.java | 14 ++++--- .../org/bitcoinj/wallet/BasicKeyChain.java | 2 +- .../wallet/DeterministicKeyChain.java | 37 ++++++------------- .../org/bitcoinj/wallet/KeyChainGroup.java | 9 ----- .../org/bitcoinj/wallet/MarriedKeyChain.java | 8 ++-- .../main/java/org/bitcoinj/wallet/Protos.java | 32 ++++++++-------- .../java/org/bitcoinj/core/WalletTest.java | 6 +-- .../store/WalletProtobufSerializerTest.java | 18 ++++++++- .../wallet/DeterministicKeyChainTest.java | 2 +- core/src/wallet.proto | 4 +- 10 files changed, 63 insertions(+), 69 deletions(-) diff --git a/core/src/main/java/org/bitcoinj/core/Wallet.java b/core/src/main/java/org/bitcoinj/core/Wallet.java index c64867247..d066033ba 100644 --- a/core/src/main/java/org/bitcoinj/core/Wallet.java +++ b/core/src/main/java/org/bitcoinj/core/Wallet.java @@ -239,16 +239,20 @@ public class Wallet extends BaseTaggableObject * Creates a wallet that tracks payments to and from the HD key hierarchy rooted by the given watching key. A * watching key corresponds to account zero in the recommended BIP32 key hierarchy. */ - public static Wallet fromWatchingKey(NetworkParameters params, DeterministicKey watchKey, long creationTimeSeconds) { - return new Wallet(params, new KeyChainGroup(params, watchKey, creationTimeSeconds)); + public static Wallet fromWatchingKey(NetworkParameters params, DeterministicKey watchKey) { + return new Wallet(params, new KeyChainGroup(params, watchKey)); } /** * Creates a wallet that tracks payments to and from the HD key hierarchy rooted by the given watching key. A - * watching key corresponds to account zero in the recommended BIP32 key hierarchy. + * watching key corresponds to account zero in the recommended BIP32 key hierarchy. The key is specified in base58 + * notation and the creation time of the key. If you don't know the creation time, you can pass + * {@link DeterministicHierarchy#BIP32_STANDARDISATION_TIME_SECS}. */ - public static Wallet fromWatchingKey(NetworkParameters params, DeterministicKey watchKey) { - return new Wallet(params, new KeyChainGroup(params, watchKey)); + public static Wallet fromWatchingKeyB58(NetworkParameters params, String watchKeyB58, long creationTimeSeconds) { + final DeterministicKey watchKey = DeterministicKey.deserializeB58(null, watchKeyB58, params); + watchKey.setCreationTimeSeconds(creationTimeSeconds); + return fromWatchingKey(params, watchKey); } /** diff --git a/core/src/main/java/org/bitcoinj/wallet/BasicKeyChain.java b/core/src/main/java/org/bitcoinj/wallet/BasicKeyChain.java index 1572078cd..7f54f42fb 100644 --- a/core/src/main/java/org/bitcoinj/wallet/BasicKeyChain.java +++ b/core/src/main/java/org/bitcoinj/wallet/BasicKeyChain.java @@ -384,7 +384,7 @@ public class BasicKeyChain implements EncryptableKeyChain { else ecKey = ECKey.fromPublicOnly(pub); } - ecKey.setCreationTimeSeconds((key.getCreationTimestamp() + 500) / 1000); + ecKey.setCreationTimeSeconds(key.getCreationTimestamp() / 1000); importKeyLocked(ecKey); } } finally { diff --git a/core/src/main/java/org/bitcoinj/wallet/DeterministicKeyChain.java b/core/src/main/java/org/bitcoinj/wallet/DeterministicKeyChain.java index 21f14d171..2a5d3a445 100644 --- a/core/src/main/java/org/bitcoinj/wallet/DeterministicKeyChain.java +++ b/core/src/main/java/org/bitcoinj/wallet/DeterministicKeyChain.java @@ -105,9 +105,6 @@ public class DeterministicKeyChain implements EncryptableKeyChain { @Nullable private DeterministicKey rootKey; @Nullable private DeterministicSeed seed; - // Ignored if seed != null. Useful for watching hierarchies. - private long creationTimeSeconds = MnemonicCode.BIP39_STANDARDISATION_TIME_SECS; - // Paths through the key tree. External keys are ones that are communicated to other parties. Internal keys are // keys created for change addresses, coinbases, mixing, etc - anything that isn't communicated. The distinction // is somewhat arbitrary but can be useful for audits. The first number is the "account number" but we don't use @@ -240,7 +237,6 @@ public class DeterministicKeyChain implements EncryptableKeyChain { return self(); } - public DeterministicKeyChain build() { checkState(random != null || entropy != null || seed != null || watchingKey!= null, "Must provide either entropy or random or seed or watchingKey"); checkState(passphrase == null || seed == null, "Passphrase must not be specified with seed"); @@ -252,9 +248,11 @@ public class DeterministicKeyChain implements EncryptableKeyChain { } else if (entropy != null) { chain = new DeterministicKeyChain(entropy, getPassphrase(), seedCreationTimeSecs); } else if (seed != null) { + seed.setCreationTimeSeconds(seedCreationTimeSecs); chain = new DeterministicKeyChain(seed); } else { - chain = new DeterministicKeyChain(watchingKey, seedCreationTimeSecs); + watchingKey.setCreationTimeSeconds(seedCreationTimeSecs); + chain = new DeterministicKeyChain(watchingKey); } return chain; @@ -316,11 +314,10 @@ public class DeterministicKeyChain implements EncryptableKeyChain { * balances and generally follow along, but spending is not possible with such a chain. Currently you can't use * this method to watch an arbitrary fragment of some other tree, this limitation may be removed in future. */ - public DeterministicKeyChain(DeterministicKey watchingKey, long creationTimeSeconds) { + public DeterministicKeyChain(DeterministicKey watchingKey) { checkArgument(watchingKey.isPubKeyOnly(), "Private subtrees not currently supported: if you got this key from DKC.getWatchingKey() then use .dropPrivate().dropParent() on it first."); checkArgument(watchingKey.getPath().size() == getAccountPath().size(), "You can only watch an account key currently"); basicKeyChain = new BasicKeyChain(); - this.creationTimeSeconds = creationTimeSeconds; this.seed = null; rootKey = null; addToBasicChain(watchingKey); @@ -328,17 +325,13 @@ public class DeterministicKeyChain implements EncryptableKeyChain { initializeHierarchyUnencrypted(watchingKey); } - public DeterministicKeyChain(DeterministicKey watchingKey) { - this(watchingKey, Utils.currentTimeSeconds()); - } - /** *

Creates a deterministic key chain with the given watch key. If isFollowing flag is set then this keychain follows * some other keychain. In a married wallet following keychain represents "spouse's" keychain.

*

Watch key has to be an account key.

*/ protected DeterministicKeyChain(DeterministicKey watchKey, boolean isFollowing) { - this(watchKey, Utils.currentTimeSeconds()); + this(watchKey); this.isFollowing = isFollowing; } @@ -352,20 +345,10 @@ public class DeterministicKeyChain implements EncryptableKeyChain { } /** - * Creates a key chain that watches the given account key. The creation time is taken to be the time that BIP 32 - * was standardised: most likely, you can optimise by selecting a more accurate creation time for your key and - * using the other watch method. + * Creates a key chain that watches the given account key. */ public static DeterministicKeyChain watch(DeterministicKey accountKey) { - return watch(accountKey, DeterministicHierarchy.BIP32_STANDARDISATION_TIME_SECS); - } - - /** - * Creates a key chain that watches the given account key, and assumes there are no transactions involving it until - * the given time (this is an optimisation for chain scanning purposes). - */ - public static DeterministicKeyChain watch(DeterministicKey accountKey, long seedCreationTimeSecs) { - return new DeterministicKeyChain(accountKey, seedCreationTimeSecs); + return new DeterministicKeyChain(accountKey); } /** @@ -682,7 +665,10 @@ public class DeterministicKeyChain implements EncryptableKeyChain { @Override public long getEarliestKeyCreationTime() { - return seed != null ? seed.getCreationTimeSeconds() : creationTimeSeconds; + if (seed != null) + return seed.getCreationTimeSeconds(); + else + return getWatchingKey().getCreationTimeSeconds(); } @Override @@ -868,6 +854,7 @@ public class DeterministicKeyChain implements EncryptableKeyChain { boolean isMarried = !isFollowingKey && !chains.isEmpty() && chains.get(chains.size() - 1).isFollowing(); if (seed == null) { DeterministicKey accountKey = new DeterministicKey(immutablePath, chainCode, pubkey, null, null); + accountKey.setCreationTimeSeconds(key.getCreationTimestamp() / 1000); chain = factory.makeWatchingKeyChain(key, iter.peek(), accountKey, isFollowingKey, isMarried); isWatchingAccountKey = true; } else { diff --git a/core/src/main/java/org/bitcoinj/wallet/KeyChainGroup.java b/core/src/main/java/org/bitcoinj/wallet/KeyChainGroup.java index 0c1be6ce1..ea7d0e97d 100644 --- a/core/src/main/java/org/bitcoinj/wallet/KeyChainGroup.java +++ b/core/src/main/java/org/bitcoinj/wallet/KeyChainGroup.java @@ -93,15 +93,6 @@ public class KeyChainGroup implements KeyBag { this(params, null, ImmutableList.of(DeterministicKeyChain.watch(watchKey)), null, null); } - /** - * Creates a keychain group with no basic chain, and an HD chain that is watching the given watching key which - * was assumed to be first used at the given UNIX time. - * This HAS to be an account key as returned by {@link DeterministicKeyChain#getWatchingKey()}. - */ - public KeyChainGroup(NetworkParameters params, DeterministicKey watchKey, long creationTimeSecondsSecs) { - this(params, null, ImmutableList.of(DeterministicKeyChain.watch(watchKey, creationTimeSecondsSecs)), null, null); - } - // Used for deserialization. private KeyChainGroup(NetworkParameters params, @Nullable BasicKeyChain basicKeyChain, List chains, @Nullable EnumMap currentKeys, @Nullable KeyCrypter crypter) { diff --git a/core/src/main/java/org/bitcoinj/wallet/MarriedKeyChain.java b/core/src/main/java/org/bitcoinj/wallet/MarriedKeyChain.java index df52f751f..91fa2c22c 100644 --- a/core/src/main/java/org/bitcoinj/wallet/MarriedKeyChain.java +++ b/core/src/main/java/org/bitcoinj/wallet/MarriedKeyChain.java @@ -103,9 +103,11 @@ public class MarriedKeyChain extends DeterministicKeyChain { } else if (entropy != null) { chain = new MarriedKeyChain(entropy, getPassphrase(), seedCreationTimeSecs); } else if (seed != null) { + seed.setCreationTimeSeconds(seedCreationTimeSecs); chain = new MarriedKeyChain(seed); } else { - chain = new MarriedKeyChain(watchingKey, seedCreationTimeSecs); + watchingKey.setCreationTimeSeconds(seedCreationTimeSecs); + chain = new MarriedKeyChain(watchingKey); } chain.addFollowingAccountKeys(followingKeys, threshold); return chain; @@ -121,10 +123,6 @@ public class MarriedKeyChain extends DeterministicKeyChain { super(accountKey, false); } - MarriedKeyChain(DeterministicKey accountKey, long seedCreationTimeSecs) { - super(accountKey, seedCreationTimeSecs); - } - MarriedKeyChain(DeterministicSeed seed, KeyCrypter crypter) { super(seed, crypter); } diff --git a/core/src/main/java/org/bitcoinj/wallet/Protos.java b/core/src/main/java/org/bitcoinj/wallet/Protos.java index 9f53f14a9..227183cb2 100644 --- a/core/src/main/java/org/bitcoinj/wallet/Protos.java +++ b/core/src/main/java/org/bitcoinj/wallet/Protos.java @@ -2405,8 +2405,8 @@ public final class Protos { * optional int64 creation_timestamp = 5; * * 
-     * Timestamp stored as millis since epoch. Useful for skipping block bodies before this point. Only reason it's
-     * optional is that some very old wallets don't have this data.
+     * Timestamp stored as millis since epoch. Useful for skipping block bodies before this point. The reason it's
+     * optional is that keys derived from a parent don't have this data.
      *
*/ boolean hasCreationTimestamp(); @@ -2414,8 +2414,8 @@ public final class Protos { * optional int64 creation_timestamp = 5; * * 
-     * Timestamp stored as millis since epoch. Useful for skipping block bodies before this point. Only reason it's
-     * optional is that some very old wallets don't have this data.
+     * Timestamp stored as millis since epoch. Useful for skipping block bodies before this point. The reason it's
+     * optional is that keys derived from a parent don't have this data.
      *
*/ long getCreationTimestamp(); @@ -2958,8 +2958,8 @@ public final class Protos { * optional int64 creation_timestamp = 5; * * 
-     * Timestamp stored as millis since epoch. Useful for skipping block bodies before this point. Only reason it's
-     * optional is that some very old wallets don't have this data.
+     * Timestamp stored as millis since epoch. Useful for skipping block bodies before this point. The reason it's
+     * optional is that keys derived from a parent don't have this data.
      *
*/ public boolean hasCreationTimestamp() { @@ -2969,8 +2969,8 @@ public final class Protos { * optional int64 creation_timestamp = 5; * * 
-     * Timestamp stored as millis since epoch. Useful for skipping block bodies before this point. Only reason it's
-     * optional is that some very old wallets don't have this data.
+     * Timestamp stored as millis since epoch. Useful for skipping block bodies before this point. The reason it's
+     * optional is that keys derived from a parent don't have this data.
      *
*/ public long getCreationTimestamp() { @@ -3905,8 +3905,8 @@ public final class Protos { * optional int64 creation_timestamp = 5; * * 
-       * Timestamp stored as millis since epoch. Useful for skipping block bodies before this point. Only reason it's
-       * optional is that some very old wallets don't have this data.
+       * Timestamp stored as millis since epoch. Useful for skipping block bodies before this point. The reason it's
+       * optional is that keys derived from a parent don't have this data.
        *
*/ public boolean hasCreationTimestamp() { @@ -3916,8 +3916,8 @@ public final class Protos { * optional int64 creation_timestamp = 5; * * 
-       * Timestamp stored as millis since epoch. Useful for skipping block bodies before this point. Only reason it's
-       * optional is that some very old wallets don't have this data.
+       * Timestamp stored as millis since epoch. Useful for skipping block bodies before this point. The reason it's
+       * optional is that keys derived from a parent don't have this data.
        *
*/ public long getCreationTimestamp() { @@ -3927,8 +3927,8 @@ public final class Protos { * optional int64 creation_timestamp = 5; * * 
-       * Timestamp stored as millis since epoch. Useful for skipping block bodies before this point. Only reason it's
-       * optional is that some very old wallets don't have this data.
+       * Timestamp stored as millis since epoch. Useful for skipping block bodies before this point. The reason it's
+       * optional is that keys derived from a parent don't have this data.
        *
*/ public Builder setCreationTimestamp(long value) { @@ -3941,8 +3941,8 @@ public final class Protos { * optional int64 creation_timestamp = 5; * * 
-       * Timestamp stored as millis since epoch. Useful for skipping block bodies before this point. Only reason it's
-       * optional is that some very old wallets don't have this data.
+       * Timestamp stored as millis since epoch. Useful for skipping block bodies before this point. The reason it's
+       * optional is that keys derived from a parent don't have this data.
        *
*/ public Builder clearCreationTimestamp() { diff --git a/core/src/test/java/org/bitcoinj/core/WalletTest.java b/core/src/test/java/org/bitcoinj/core/WalletTest.java index 12eac8b09..d78f7b1b3 100644 --- a/core/src/test/java/org/bitcoinj/core/WalletTest.java +++ b/core/src/test/java/org/bitcoinj/core/WalletTest.java @@ -1640,8 +1640,7 @@ public class WalletTest extends TestWithWallet { public void watchingWalletWithCreationTime() throws Exception { DeterministicKey watchKey = wallet.getWatchingKey(); String serialized = watchKey.serializePubB58(PARAMS); - watchKey = DeterministicKey.deserializeB58(null, serialized, PARAMS); - Wallet watchingWallet = Wallet.fromWatchingKey(PARAMS, watchKey, 1415282801); + Wallet watchingWallet = Wallet.fromWatchingKeyB58(PARAMS, serialized, 1415282801); DeterministicKey key2 = watchingWallet.freshReceiveKey(); assertEquals(myKey, key2); @@ -3499,8 +3498,7 @@ public class WalletTest extends TestWithWallet { public void watchingMarriedWallet() throws Exception { DeterministicKey watchKey = wallet.getWatchingKey(); String serialized = watchKey.serializePubB58(PARAMS); - watchKey = DeterministicKey.deserializeB58(null, serialized, PARAMS); - Wallet wallet = Wallet.fromWatchingKey(PARAMS, watchKey); + Wallet wallet = Wallet.fromWatchingKeyB58(PARAMS, serialized, 0); blockStore = new MemoryBlockStore(PARAMS); chain = new BlockChain(PARAMS, wallet, blockStore); diff --git a/core/src/test/java/org/bitcoinj/store/WalletProtobufSerializerTest.java b/core/src/test/java/org/bitcoinj/store/WalletProtobufSerializerTest.java index 870503d5d..6df927e63 100644 --- a/core/src/test/java/org/bitcoinj/store/WalletProtobufSerializerTest.java +++ b/core/src/test/java/org/bitcoinj/store/WalletProtobufSerializerTest.java @@ -289,7 +289,7 @@ public class WalletProtobufSerializerTest { @Test public void testRoundTripNormalWallet() throws Exception { - Wallet wallet1 = roundTrip(myWallet); + Wallet wallet1 = roundTrip(myWallet); assertEquals(0, wallet1.getTransactions(true).size()); assertEquals(Coin.ZERO, wallet1.getBalance()); assertArrayEquals(myKey.getPubKey(), @@ -300,6 +300,22 @@ public class WalletProtobufSerializerTest { wallet1.findKeyFromPubHash(myKey.getPubKeyHash()).getCreationTimeSeconds()); } + @Test + public void testRoundTripWatchingWallet() throws Exception { + final String xpub = "tpubD9LrDvFDrB6wYNhbR2XcRRaT4yCa37TjBR3YthBQvrtEwEq6CKeEXUs3TppQd38rfxmxD1qLkC99iP3vKcKwLESSSYdFAftbrpuhSnsw6XM"; + final long creationTimeSeconds = 1457019819; + Wallet wallet = Wallet.fromWatchingKeyB58(PARAMS, xpub, creationTimeSeconds); + Wallet wallet2 = roundTrip(wallet); + Wallet wallet3 = roundTrip(wallet2); + assertEquals(xpub, wallet.getWatchingKey().serializePubB58(PARAMS)); + assertEquals(creationTimeSeconds, wallet.getWatchingKey().getCreationTimeSeconds()); + assertEquals(creationTimeSeconds, wallet2.getWatchingKey().getCreationTimeSeconds()); + assertEquals(creationTimeSeconds, wallet3.getWatchingKey().getCreationTimeSeconds()); + assertEquals(creationTimeSeconds, wallet.getEarliestKeyCreationTime()); + assertEquals(creationTimeSeconds, wallet2.getEarliestKeyCreationTime()); + assertEquals(creationTimeSeconds, wallet3.getEarliestKeyCreationTime()); + } + @Test public void testRoundTripMarriedWallet() throws Exception { // create 2-of-2 married wallet diff --git a/core/src/test/java/org/bitcoinj/wallet/DeterministicKeyChainTest.java b/core/src/test/java/org/bitcoinj/wallet/DeterministicKeyChainTest.java index a97b9cd1b..5277dbc97 100644 --- a/core/src/test/java/org/bitcoinj/wallet/DeterministicKeyChainTest.java +++ b/core/src/test/java/org/bitcoinj/wallet/DeterministicKeyChainTest.java @@ -323,7 +323,7 @@ public class DeterministicKeyChainTest { watchingKey = DeterministicKey.deserializeB58(null, pub58, params); watchingKey.setCreationTimeSeconds(100000); chain = DeterministicKeyChain.watch(watchingKey); - assertEquals(DeterministicHierarchy.BIP32_STANDARDISATION_TIME_SECS, chain.getEarliestKeyCreationTime()); + assertEquals(100000, chain.getEarliestKeyCreationTime()); chain.setLookaheadSize(10); chain.maybeLookAhead(); diff --git a/core/src/wallet.proto b/core/src/wallet.proto index fb38a1212..190070273 100644 --- a/core/src/wallet.proto +++ b/core/src/wallet.proto @@ -120,8 +120,8 @@ message Key { // User-provided label associated with the key. optional string label = 4; - // Timestamp stored as millis since epoch. Useful for skipping block bodies before this point. Only reason it's - // optional is that some very old wallets don't have this data. + // Timestamp stored as millis since epoch. Useful for skipping block bodies before this point. The reason it's + // optional is that keys derived from a parent don't have this data. optional int64 creation_timestamp = 5; optional DeterministicKey deterministic_key = 7;