Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Address: Fix Java serialization and add serialization test case. #1036

Merged
merged 1 commit into from Jul 16, 2015

Conversation

schildbach
Copy link
Member

The issue became uncovered because the newly-for-0.13 introduced HttpDiscovery.Details member into MainNetParams broke the serialization chain for all mainnet addresses.

@mikehearn
Copy link
Member

Would this test actually have caught the issue? Surely the problem is a change in the schema, in which case you'd want to save a file to disk and load it in the test case to make sure it can still be deserialized?

@schildbach
Copy link
Member Author

Emberassingly no, it's doesn't show the problem. It should – I'm not sure why it doesn't – need to investigate.

What's gained by saving the byte array to disk and loading it back? That should not touch (de)serialization.

@schildbach
Copy link
Member Author

What clearly shows the problem if I run the same test for MainNetParams. But then I thought rather than fixing serialization for NetworkParams we should make NetworkParams not support Java serialzation any more. It's quite trivial to "serialize" params by just remembering the ID, since NetworkParams are meant to be immutable and singleton.

@schildbach
Copy link
Member Author

Sorry, Eclipse played a little joke on me. The issue is reproduable using the test:

java.io.NotSerializableException: org.bitcoinj.net.discovery.HttpDiscovery$Details
    at java.io.ObjectOutputStream.writeObject0(ObjectOutputStream.java:1184)
    at java.io.ObjectOutputStream.writeArray(ObjectOutputStream.java:1378)
    at java.io.ObjectOutputStream.writeObject0(ObjectOutputStream.java:1174)
    at java.io.ObjectOutputStream.defaultWriteFields(ObjectOutputStream.java:1548)
    at java.io.ObjectOutputStream.writeSerialData(ObjectOutputStream.java:1509)
    at java.io.ObjectOutputStream.writeOrdinaryObject(ObjectOutputStream.java:1432)
    at java.io.ObjectOutputStream.writeObject0(ObjectOutputStream.java:1178)
    at java.io.ObjectOutputStream.defaultWriteFields(ObjectOutputStream.java:1548)
    at java.io.ObjectOutputStream.writeSerialData(ObjectOutputStream.java:1509)
    at java.io.ObjectOutputStream.writeOrdinaryObject(ObjectOutputStream.java:1432)
    at java.io.ObjectOutputStream.writeObject0(ObjectOutputStream.java:1178)
    at java.io.ObjectOutputStream.writeObject(ObjectOutputStream.java:348)
    at org.bitcoinj.core.AddressTest.testJavaSerialization(AddressTest.java:52)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:497)
    at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:47)
    at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
    at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:44)
    at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
    at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:271)
    at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:70)
    at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:50)
    at org.junit.runners.ParentRunner$3.run(ParentRunner.java:238)
    at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:63)
    at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:236)
    at org.junit.runners.ParentRunner.access$000(ParentRunner.java:53)
    at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:229)
    at org.junit.runners.ParentRunner.run(ParentRunner.java:309)
    at org.eclipse.jdt.internal.junit4.runner.JUnit4TestReference.run(JUnit4TestReference.java:50)
    at org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:38)
    at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:459)
    at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:675)
    at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:382)
    at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main(RemoteTestRunner.java:192)

@mikehearn
Copy link
Member

Ah I see. You're just testing it can be serialized at all, not that it's compatible. OK that makes sense. Maybe you could add a test that verifies we can deserialize old bytestreams as well?

Anyway the fix looks good.

@schildbach
Copy link
Member Author

Afaicr we agreed that we don't support Java serialization as a persistence mechanism. Only for quick data exchange, e.g. between app components.

@mikehearn
Copy link
Member

Right, that's true. So this LGTM.

@schildbach schildbach merged commit b1402af into bitcoinj:master Jul 16, 2015
@schildbach schildbach deleted the fix-address-serialization branch July 16, 2015 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants