Closed GoogleCodeExporter closed 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
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
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
"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
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
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
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
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
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
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
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
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
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
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
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
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
+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
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
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
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
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
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
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
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
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
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
Original comment by mi...@google.com
on 14 Oct 2011 at 8:35
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
[deleted comment]
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
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
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
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
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
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
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
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
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
> 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
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
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
Also need to consider what to do with tx confidence.
Original comment by mi...@google.com
on 17 Jan 2012 at 10:38
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
+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
Original issue reported on code.google.com by
andreas....@gmail.com
on 13 Mar 2011 at 11:39