novitski / bitcoinj

Automatically exported from code.google.com/p/bitcoinj
Apache License 2.0
0 stars 0 forks source link

more robust serialization format for wallet #4

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
A recent added logic methos in TransactionOutPoint made me aware of the fact 
how vulnerable our current wallet format (based on Serialization) is. Without 
any change of data at all, the old format is not readble any more.

My proposal:

1) short-term: Go away from autogenerated serialVersionUID to manually assigned 
ones. Advance only if strictly necessary (if the meaning of data changes).

2) mid-term: Allow for automatic migration if something needs to change. This 
means old formats have to be supported for reading. I don't know if this is 
possible with serialVersionUID.

3) long-term: As the Java serialization mechanism is not meant for long-term 
persistence, move to an own solution. This is probably platform dependent. For 
example, on Android I'd prefer to use an sqlite DB to store keys. Perhaps some 
kind of plugin mechanism would be nice.

Original issue reported on code.google.com by andreas....@gmail.com on 13 Mar 2011 at 11:39

GoogleCodeExporter commented 9 years ago
Here is a patch to revert the TransactionOutPoint serialVersionUID change:

Index: TransactionOutPoint.java
===================================================================
--- TransactionOutPoint.java    (revision 22)
+++ TransactionOutPoint.java    (working copy)
@@ -26,6 +26,9 @@
  * This message is a reference or pointer to an output of a different transaction.
  */
 public class TransactionOutPoint extends Message implements Serializable {
+    
+    static final long serialVersionUID = -6320880638344662579L;
+
     /** Hash of the transaction to which we refer. */
     byte[] hash;
     /** Which output of that transaction we are talking about. */

Original comment by andreas....@gmail.com on 13 Mar 2011 at 11:49

GoogleCodeExporter commented 9 years ago
Thanks. I added UIDs to the classes that were missing them.

The wallet format will indeed be changing a fair bit in future (like we should 
zip it as a start). In this case though I have just forgotten to put 
serialVersionUIDs on every class that needs them. The rules for loading classes 
where the serializing class is different to the current class and the UIDs 
match isn't so far off most serialization frameworks.

http://download.oracle.com/javase/6/docs/platform/serialization/spec/version.htm
l

See "incompatible changes". So the answer is we can indeed read old wallets if 
we want to. I just didn't put any effort into that. Once the library is further 
along it'll become more important.

Other formats, plugins etc could be done but it's a lot of complexity. Let's 
see how well plain old serialization fares first. After all you'd have to take 
care around format upgrades/downgrades in future anyway.

Once we decide to draw a line and support old wallet formats, we can add some 
unit tests to ensure it never breaks.

Original comment by hearn@google.com on 13 Mar 2011 at 12:54

GoogleCodeExporter commented 9 years ago
Another patch to revert an unnecessary serialVersionUID change:

Index: Block.java
===================================================================
--- Block.java  (revision 24)
+++ Block.java  (working copy)
@@ -33,7 +33,7 @@
  * you grab it from a downloaded {@link BlockChain}.
  */
 public class Block extends Message {
-    private static final long serialVersionUID = -2834162413473103042L;
+    private static final long serialVersionUID = 2738848929966035281L;

     static final long ALLOWED_TIME_DRIFT = 2 * 60 * 60;  // Same value as official client.
     /** A value for difficultyTarget (nBits) that allows half of all possible hash solutions. Used in unit testing. */

Original comment by andreas....@gmail.com on 13 Mar 2011 at 11:28

GoogleCodeExporter commented 9 years ago
"3) long-term: As the Java serialization mechanism is not meant for long-term 
persistence, move to an own solution. This is probably platform dependent. For 
example, on Android I'd prefer to use an sqlite DB to store keys. Perhaps some 
kind of plugin mechanism would be nice."

Rather than defining a new long-term persistence format (for this project), it 
would probably be better to have a set of export/import functions, so that keys 
and other wallet data could be transferred between bitcoinj and other tools, in 
particular the wallet format used by the C version.

Also, if we are moving away from the simple serialization, would it make sense 
to store wallet data using the standard Java Keystore mechanism? 

Original comment by thilopl...@googlemail.com on 25 Mar 2011 at 12:30

GoogleCodeExporter commented 9 years ago
I don't know that Java serialization isn't meant for long term persistence. 
It's true for some object hierarchies like if you try and persist Swing GUIs. 
But for simpler hierarchies like BitCoinJ has it can certainly work.

Wallets contain not just keys but also transactions and other data. So if we 
don't use Java serialization then some custom format would be required, 
probably protocol buffers.

At some point BitCoinJ will settle down and most of the work will be on the 
periphery, supporting things like decoding bitcoin: URIs, supporting exotic 
scripts or looking up exchange rates. So the wallet format should stabilize 
naturally.

Until then I think it's fair to say the library comes with a health (wealth?) 
warning. Once we decide to lock down the wallet format some unit tests can be 
written to ensure we can always deserialize old wallets.

Original comment by hearn@google.com on 25 Mar 2011 at 12:34

GoogleCodeExporter commented 9 years ago
Hi everyone,

i am currently working on a (Android-) SQLite backend/blockstore for bitcoinj.

In order to get the 'lightweight' client it is necessary to seperate the block 
headers and the transactions. The current implementation requires a StoredBlock 
to be handled. IMHO the database shouldn't store anything that isn't important 
to the network -> resemble the c structs as close as possible.

On the 'way back' out of the database new blocks and transactions need to be 
constructed from the database fields. Currently this is not possible as the 
only way to construct blocks and transactions is via 'wire format'.

I propose that the classes Block, Transaction, TransactionInput, 
TransactionOutput and TransactionOutPoint should be stripped of everything that 
isn't strictly necessary.
This would enable the creation of different backends (why should a sql backend 
have to deal with the wire format?). Additionally this would be a first step to 
make bitcoinj conformant to the bitcoin system (p2p net <- p2p protocol -> peer 
<- p2p protocoll -> wallet <- wallet protocoll -> client or miner)

If I get a confirmation on this matter i can implement this right away.

Kind regards,
Florian

Original comment by fha...@gmail.com on 6 Apr 2011 at 7:13

GoogleCodeExporter commented 9 years ago
Hi Florian,

This seems to be unrelated to what the bug is about. Could you repost your mail 
to the mailing list where we can discuss it?

Original comment by hearn@google.com on 10 Apr 2011 at 7:44

GoogleCodeExporter commented 9 years ago
If you're considering other serialization formats, you might consider storing 
the wallet as in a protocol buffer type format.  You'd have to do versioning, 
of course, but it would be a nice open wallet standard to start from, perhaps.  
Actually... wallet interop would be really nice, right? so the wallet that my 
android app used could be the same one that my desktop uses (because my phone 
is playing USB storage device).  So the more standard and cross-platform the 
format, the better.

Original comment by pjimen...@gmail.com on 11 Jun 2011 at 3:19

GoogleCodeExporter commented 9 years ago
Interop is an important issue (though not necessarily tied to the bitcoinj 
internal storage format, there could be an export mechanism). A standard format 
would have to be defined in coordination (or even by) the bitcoin.org folks 
first, though, before bicoinj should start implementing it. There has been some 
discussion about a plain-text wallet export format. I suppose that would fit 
the bill.

I have also briefly looked at implementing an importer for the official 
client's wallet files. However, those are Berkeley DB files, and it is 
difficult to read them from Java. On the desktop, you can use JNI, but there 
seems to be no "pure" Java (and Android-compatible) solution. (If you have one, 
please mention it at 
http://stackoverflow.com/questions/6313445/can-i-read-berkeleydb-non-java-editio
n-files-from-java ).

Original comment by thilopl...@googlemail.com on 12 Jun 2011 at 1:18

GoogleCodeExporter commented 9 years ago
I think the way we'll go is defining some wallet persistence API (perhaps 
subclassing Wallet itself), so it's pluggable like BlockStores are. There 
doesn't need to be a single format.

Java serialization is convenient for now and works fine for small wallets. In 
future we may see BitCoinJ be used in more of an enterprise setting and at that 
point, the ability to store wallets in relational databases might prove useful. 
It needs some careful though around how to handle sharding, how to be 
efficient, etc.

Original comment by hearn@google.com on 13 Jun 2011 at 9:34

GoogleCodeExporter commented 9 years ago
I am going to write a subclass that encrypts the keys during de/serialization. 
It will require a password on startup and password changes will be possible.

Going to use bouncycastle for encryption.

Any suggestions?

Original comment by bobby.si...@gmail.com on 16 Jun 2011 at 5:35

GoogleCodeExporter commented 9 years ago
Also may when addKey is called an DontUseException should be thrown. Rather a 
new method createKey() should be called and this method will serialize the 
wallet before returning a new instance of ECKey. This will restrict the user to 
using keys that are already serialized on disk. 

??

Original comment by bobby.si...@gmail.com on 16 Jun 2011 at 6:27

GoogleCodeExporter commented 9 years ago
Interesting ideas. I'd certainly be willing to take a patch for that.

I think for an API, flexibility is important and we should rely on 
documentation to help people do the right thing. There are valid use cases for 
adding keys to a wallet without saving it to disk, but a createKey that does 
both simultaneously would be a nice addition.

I'd suggest reviewing how Bitcoin C++ does wallet encryption before 
implementing it yourself. It's important to choose the right password2key 
algorithm parameters, etc.

Original comment by hearn@google.com on 16 Jun 2011 at 8:32

GoogleCodeExporter commented 9 years ago
I'll keep addKey with it's current functionality, I'll specify the difference 
in the docs, but I will run the encrypt/serialize methods for each new key, 
regardless of if they are added by addKey(..) or createKey(...) . 

Thoughts?

Original comment by bobby.si...@gmail.com on 16 Jun 2011 at 8:57

GoogleCodeExporter commented 9 years ago
https://github.com/smellyBobby/bitcoinj/pull/3.diff

Let me know if anything needs fixing.

Original comment by bobby.si...@gmail.com on 20 Jun 2011 at 1:42

GoogleCodeExporter commented 9 years ago
Thanks, I'll do a more thorough review soon. For now here are some high level 
comments.

Do you need to rebase? BlockChain.java appears to have some of my changes in.

Please ensure the formatting/code style/indenting is consistent with the rest 
of the codebase. It makes it easier to review and understand the code.

Having seen the code, I wonder if having a special EncryptedWallet class is the 
right way to go, or whether encryption should be purely a property of the key. 
I've been thinking of making ECKey abstract for a while, as not everyone might 
want to use the included Bouncy Castle. 

An EncryptedKey class that extends ECKey might make more sense. The question 
then is, how does the wallet manipulate such keys? The ECKey interface could 
have a concept of unavailability that generalizes to more than just "password 
required" - eg, keys that are stored on smart cards or other wild ideas might 
also fit. The regular Wallet class would then understand that if a key claims 
to be unavailable, it should be treated as if it doesn't exist for the purposes 
of calculating your spendable balance, but is otherwise still useful for 
matching transactions. 

It's important that the concept of unavailability NOT restrict the public part, 
but only the private part. I think from reading your patch, key availability is 
an all or nothing affair, which means when the wallet is encrypted blocks 
sending coins to you will be ignored - is that right?

The password-to-key algorithm needs some more explanation, I think. Is SHA-256 
with 100,000 rounds an accepted way to generate such keys? Where did 100,000 
come from? It's probably better to use the Java standard crypto APIs for this 
rather than Bouncy Castle, the docs for the PKCS12ParametersGenerator aren't 
very good. We use Bouncy Castle for the elliptic curve crypto due to problems 
on Android. For non-EC crypto, the standard Java APIs are a better fit. PBKDF2 
is probably supported out of the box.

I like the focus on keeping the keys in memory for the shortest possible time. 
We should ensure that this doesn't overly influence the API or implementation 
complexity though, as if you have local memory access breakpointing the app at 
sensitive points is only a bit harder.

So generally, I think the patch will become much simpler if we see this as 
primarily about subclassing ECKey. Wallet users can then iterate over the 
keychain and unlock encrypted keys themselves, if they know they put such keys 
in. This also lets us support per-key passwords, which I think is a very 
valuable feature: for instance, it lets you put most of your value into a 
"savings account key" without the hassle of dealing with multiple wallets.

Original comment by hearn@google.com on 20 Jun 2011 at 11:59

GoogleCodeExporter commented 9 years ago
+1 for focusing on encrypting just the private keys first.

While it is true that the rest of the wallet is also sensitive information (if 
someone finds your public keys, your anonymity is gone, if someone sees the key 
names you gave to your sending addresses, the anonymity of your business 
partners is compromised), the private keys should be the first order of 
business. The rest of the wallet can be addressed later.

Does Android support the Java Keystore facility? If so, that would be a good 
choice for storing the private keys. Using Keystore, the wallet would not 
contain the private keys at all, but load them on-demand (i.e. only when trying 
to send money, and only one by one) from the keystore (which would be a 
separate file or even device). 

Original comment by thilopl...@googlemail.com on 21 Jun 2011 at 12:16

GoogleCodeExporter commented 9 years ago
Interesting thoughts. Sorry about the lack of re-basing and layout, my 
understanding of these tools and code standardisation is still lacking.

Generally I found looking on the web that one-thousand hash iterations for PBE 
to be the minimum. I choose 100,000 based on the encryption discussion in the 
bitcoin forums for the c++ version, gmaxwell: “CPUs are doing roughly 4 
million SHA-256 per core per second with optimized code” “at least encode 
the iteration count in the file and turn it up so that it takes, say, 100ms 
when the password is set” , to me this indicates that if the user has a low 
quality password then  1,000 iterations is pointless. Also mtgox is upgrading 
to SHA-512, maybe we should consider this? And SHA was chosen because it is a 
secure hash function, not an encryption algorithm, and I found that 
key-derivation was generally based on a hashing function.

Furthur Gavin points out that if user’s do not choose good passwords then the 
amount of password-hashing done is pointless. I think this could be solved by 
creating a password-cracker and allowing users to have a “password” based 
upon geographic coordinates of a familiar location. The coordinates could be 
obtained from google-maps. The user would enter coordinates and the cracker 
would do a spiral-search until found or some radius bound is broken. Further 
this could be made more complex by incorporating more locations. IMO this will 
be more user friendly than using certificates.

When using java based encryption, I found that I would need to install 
unlimited strength security jurisdiction policy files. Out of curiosity do you 
remember the android issues that forced the adoption of bouncy-castle?

I do think that it would be a good idea to de-couple cryptographic code if we 
want to give the user multiple options. And creating some interface/abstraction 
that allows different types of encryption.

And by the end of writing the patch, I was looking at it realising this had 
little to do with the wallet itself, but I didn’t understand the wallet well 
and thought it was best to get some feedback.

http://www.jasypt.org/howtoencryptuserpasswords.html
http://www.javamex.com/tutorials/cryptography/pbe_key_derivation.shtml
http://forum.bitcoin.org/index.php?topic=8728.40
https://support.mtgox.com/entries/20208066-huge-bitcoin-sell-off-due-to-a-compro
mised-account-rollback
http://keepass.info/help/base/security.html

Original comment by bobby.si...@gmail.com on 21 Jun 2011 at 3:00

GoogleCodeExporter commented 9 years ago
Great comments.

The concern about weak passwords is rainbow tables? I think we can solve that 
just by salting the password with a random nonce?

The Android issues were to do with EC crypto being stripped out of the library 
on the phone. If the standard Java APIs require screwing around with policy 
files I agree it's best to ignore them (that is a weird issue to have though).

Original comment by hearn@google.com on 21 Jun 2011 at 9:03

GoogleCodeExporter commented 9 years ago
In the thread, they were discussing that salting was not being done at all, btw 
salting is in the patch. I think their biggest concern is password choice, they 
say no reasonable amount of hashing and salting will defend accounts against 
the top 100,000 passwords. My understanding is that it would still be simple 
for a hacker to re-create a rainbow table despite salting and hashing. To 
really fix the risk you have to force users to choose better passwords, which 
is probably outside the scope of bitcoinj. 

Original comment by bobby.si...@gmail.com on 21 Jun 2011 at 9:42

GoogleCodeExporter commented 9 years ago
Salting is the standard procedure for breaking rainbow tables. Eg, concatenate 
the password with the public part of the key, hash that a whole bunch of times.

Original comment by hearn@google.com on 21 Jun 2011 at 9:52

GoogleCodeExporter commented 9 years ago
Please don't reinvent the wheel when it comes to iterations and salting. 
Instead consider recycling the work put into the "Unix crypt using SHA-256 and 
SHA-512" specification: http://www.akkadia.org/drepper/SHA-crypt.txt

Original comment by noa.res...@gmail.com on 21 Jun 2011 at 4:56

GoogleCodeExporter commented 9 years ago
Closing this one, with the assumption the Java serialization is robust enough 
for the short term.  Will open another bug for import/export format.

Original comment by mi...@google.com on 23 Sep 2011 at 8:18

GoogleCodeExporter commented 9 years ago
We also need a persistence format, not only import/export! I'd rather not close 
this issue.

Original comment by andreas....@gmail.com on 23 Sep 2011 at 8:47

GoogleCodeExporter commented 9 years ago
I agree, this is not fixed, changing some a class will break the wallet.

Original comment by heg...@gmail.com on 23 Sep 2011 at 9:16

GoogleCodeExporter commented 9 years ago
Re-opening.  Apparently the issue is that Java serialization is not very robust 
for small memory devices (Android) due to stack overflows.

Original comment by mi...@google.com on 23 Sep 2011 at 9:38

GoogleCodeExporter commented 9 years ago

Original comment by mi...@google.com on 14 Oct 2011 at 8:35

GoogleCodeExporter commented 9 years ago
I've come to appreciate that basing the on-disk format for Wallet on Java 
serialization makes it hard to refactor.

I have an early draft of protobuf serialization for Wallet here:

http://code.google.com/r/miron-bitcoinj/source/detail?r=0566199e78a232ea2e0adc9a
fee4d412bcc7fbb4&name=wallet

Let me know what you think.

Original comment by mi...@google.com on 6 Jan 2012 at 11:03

GoogleCodeExporter commented 9 years ago
[deleted comment]
GoogleCodeExporter commented 9 years ago
Argh, missed a file.

Another try:

http://code.google.com/r/miron-bitcoinj/source/detail?r=ae58994020b02bf7f635d7ed
06ea48a7a543be44&name=wallet

Original comment by mi...@google.com on 6 Jan 2012 at 11:12

GoogleCodeExporter commented 9 years ago
And now reading from a stream:

http://code.google.com/r/miron-bitcoinj/source/detail?r=25487ea1b26f93c1f813e221
2a3e4f77151d7373&name=wallet#

Note that BOBS is not a very good match for this, because arbitrary blocks may 
be retrieved from the block store to populate Transaction.appearsIn.

Original comment by mi...@google.com on 7 Jan 2012 at 12:40

GoogleCodeExporter commented 9 years ago
Thanks. I'll take a deeper look next week. For now:

- I think txns can be in both pending and inactive simultaneously.
- Why store keys in base58 encoding rather than bytes?
- Maybe make the messages all top level rather than submessages of Wallet?
- It might be better to store the Satoshi-format transaction rather than break 
out individual fields. Script bytes are not enough to properly reconstruct the 
transaction. Think about lock times and sequence numbers as well.

Original comment by hearn@google.com on 7 Jan 2012 at 12:12

GoogleCodeExporter commented 9 years ago
Is this the only overlap?  Do you think it's better to allow multiple pools
or to have an enum value for the overlap?

I felt that it was easier in case of forensics on a corrupted wallet, but
maybe that's not a good reason.

Yes, agreed.

That would put the burden of parsing the bitcoin format tx on other clients
of this format.  Seems better to break out the fields.  Also, locktime and
sequence are currently unused and we can add them later if they start being
used.  Or we can add them right now.  What do you think?

Original comment by mi...@google.com on 10 Jan 2012 at 7:02

GoogleCodeExporter commented 9 years ago
My last comment got munged by code.google.com.  Here it is in full:

- I think txns can be in both pending and inactive simultaneously.

Is this the only overlap?  Do you think it's better to allow multiple pools or 
to have an enum value for the overlap?

- Why store keys in base58 encoding rather than bytes?

I felt that it was easier in case of forensics on a corrupted wallet, but maybe 
that's not a good reason.

- Maybe make the messages all top level rather than submessages of Wallet?

Yes, agreed.

- It might be better to store the Satoshi-format transaction rather than break 
out individual fields. Script bytes are not enough to properly reconstruct the 
transaction. Think about lock times and sequence numbers as well.

That would put the burden of parsing the bitcoin format tx on other clients of 
this format.  Seems better to break out the fields.  Also, locktime and 
sequence are currently unused and we can add them later if they start being 
used.  Or we can add them right now.  What do you think?

Original comment by mi...@google.com on 10 Jan 2012 at 7:12

GoogleCodeExporter commented 9 years ago
I think it is. I'm not sure, would have to double check. The pools code is very 
complicated unfortunately. I'd like to simplify it down at some point. 
Conceptually it needs to remain the same but I'm sure the implementation can be 
simplified.

I don't think data gets randomly corrupted like that very often, but if it did, 
you'd want the raw key because it's less dense than base58 so bit flips cause 
"less" damage. If we really wanted some kind of corruption solution, putting an 
error correcting code on the end is what you really need (a checksum is kind of 
useless).

If we're going to reimplement the Satoshi format in protobufs (which is fine by 
me), then definitely we should add every field.

Original comment by hearn@google.com on 10 Jan 2012 at 7:14

GoogleCodeExporter commented 9 years ago
I rebased on master and implemented everything we discussed to date here:

http://code.google.com/r/miron-bitcoinj/source/detail?r=a8fd0d474d86358d6aef4317
b816241e719870cb

Original comment by mi...@google.com on 11 Jan 2012 at 12:10

GoogleCodeExporter commented 9 years ago
Added this project to the CI server under BitCoinJ-miron. It only performs a 
package build to avoid deployment into the Maven repos.

Original comment by g.rowe.f...@gmail.com on 11 Jan 2012 at 10:31

GoogleCodeExporter commented 9 years ago
bitcoin.proto:
- Spurious comment formatting change on first line
- Why store the ASN.1 form of the private key? It just adds bloat on top of the 
raw EC key value.
- We should probably specify endiannness examples in the hash fields, as 
endianness in Bitcoin is such a PITA
- On TransactionInput.sequence add a comment like
# Sequence number, used for contracts. Incrementing sequence numbers allows new 
versions of a transaction
# to be written without invalidating other signatures.
- How were the numbers for the pool enum chosen? Maybe comment that 
PENDING_INACTIVE is when a transaction has appeared on a non-best side chain 
and is still waiting to appear in the best chain.
- docs for locktime

I'm wondering if we should have some kind of "vendor extension" mechanism. 
Inside Google it's rarely needed because you just grab a new field number. 
Something less co-ordinated might be useful for this, if it'll end up being 
re-used by other projects. Perhaps like

repeated Extension extensions = 10;

message Extension {
  required string id = 1;   // like org.whatever.foo.bar
  required bytes data = 2;
}

Transaction.java:
- The convention elsewhere in the API is that NetworkParameters comes first
- What's the masking for? Taking out the sign bit? Comment that, it doesn't 
seem related.

Wallet.java
- Why is receivedFromBlock now public?

WalletProtobufSerializer.java:
- 
.setSpentByTransactionHash(ByteString.copyFrom(spentBy.getOutpoint().getHash().g
etBytes()))
  Why did you add getOutpoint here: is that right?

Tests:
- Should there be tests that check the actual contents of the protobufs too 
(maybe compare against text constants)?

Original comment by hearn@google.com on 11 Jan 2012 at 10:47

GoogleCodeExporter commented 9 years ago
> bitcoin.proto:
> - Spurious comment formatting change on first line

Done

> - Why store the ASN.1 form of the private key? It just adds bloat on top of 
the raw EC key value.

Done

> - We should probably specify endiannness examples in the hash fields, as 
endianness in Bitcoin is such a PITA

Added a comment that it's all Big Endian
.
> - On TransactionInput.sequence add a comment like

Added a short comment.  Since these are disabled in the Satoshi client probably 
not too interesting to clients yet.

> - How were the numbers for the pool enum chosen? Maybe comment that 
PENDING_INACTIVE is when a transaction has appeared on a non-best side chain 
and is still waiting to appear in the best chain.

I made PENDING = 16 so that it can be added to the INACTIVE value to get 
PENDING_INACTIVE.  Probably unimportant, but just in case we have other flag 
bits in the future.

> - docs for locktime

See comment about sequence.

> I'm wondering if we should have some kind of "vendor extension" mechanism. 
[...]

Done.  Added "mandatory" boolean to fail right away if we are losing data by 
not understanding an extension.

> Transaction.java:
> - The convention elsewhere in the API is that NetworkParameters comes first

Done.

> - What's the masking for? Taking out the sign bit? Comment that, it doesn't 
seem related.

Done.

> Wallet.java
> - Why is receivedFromBlock now public?

Need it in the new test, which is in a different package.

> WalletProtobufSerializer.java:
> - 
.setSpentByTransactionHash(ByteString.copyFrom(spentBy.getOutpoint().getHash().g
etBytes()))
>  Why did you add getOutpoint here: is that right?

No, I messed up.  Fixed.

> Tests:
> - Should there be tests that check the actual contents of the protobufs too 
(maybe compare against text constants)?

Did some of this, but many fields are already checked in the round-trip or are 
runtime dependent.

Original comment by mi...@google.com on 11 Jan 2012 at 6:11

GoogleCodeExporter commented 9 years ago
Merged into master, 5 commits ending with:

http://code.google.com/p/bitcoinj/source/detail?r=1a2ce7d982d24ccf9e22bf1df9ad9f
8f716498cc

Other serializers (DB?) can be created with this as a reference.

Original comment by mi...@google.com on 11 Jan 2012 at 6:51

GoogleCodeExporter commented 9 years ago
Reopening, needs a bit more work to work well with BoundedOverheadBlockStore, 
first draft here:

http://code.google.com/r/miron-bitcoinj/source/detail?r=af8b86c7c5e60c457411a445
1553bf7826eeb8dd&name=wallet
http://code.google.com/r/miron-bitcoinj/source/detail?r=81f7e593c4b95209c4461692
143f5a25ab51b133&name=wallet

Original comment by mi...@google.com on 17 Jan 2012 at 10:37

GoogleCodeExporter commented 9 years ago
Also need to consider what to do with tx confidence.

Original comment by mi...@google.com on 17 Jan 2012 at 10:38

GoogleCodeExporter commented 9 years ago
Fixed in commits:

http://code.google.com/p/bitcoinj/source/detail?r=1c28bd3972e93e7eeffbe4b777b13d
e925b45f47
http://code.google.com/p/bitcoinj/source/detail?r=69ee4c77292037707593fbd64cbd7d
364a09b561

Original comment by mi...@google.com on 2 Feb 2012 at 9:23

GoogleCodeExporter commented 9 years ago
+1 for focusing on encrypting just the private keys first. Its important and 
also asked in interviews.
http://javarevisited.blogspot.com/2011/04/top-10-java-serialization-interview.ht
ml

Original comment by savingfu...@gmail.com on 5 Apr 2012 at 11:16