Patch to add missing getters to allow external Block serialization

[GoogleCodeExporter]

Block and related classes are missing a few getters in order to allow external 
code to query all relevant fields. With these additions, I could write code to 
serialize a block as JSON in the format used by Blockexplorer (not included in 
this patch).

While doing this, I also noticed that the existing getter 'getMerkleRoot' 
returns a byte array that is also stored and used internally (it is transient, 
though). This would potentially allow external code to change the contents of 
the array, breaking the Block instance (but probably only impacting itself). 
The method should maybe return a copy. The other getters seem to enforce 
immutability.

https://github.com/thiloplanz/bitcoinj/commit/8181a1ee03c61145cc08e12270a74654f7
328a6d

Original issue reported on code.google.com by thilopl...@googlemail.com on 7 Apr 2011 at 11:12

[GoogleCodeExporter]

Thanks for the patch and sorry for being so slow in getting back to you. 
Vacations, work spikes etc.

Would it help if I gave you commit access, then you could work on a Subversion 
branch rather than git?

A few comments:
- Why do you return unmodifiable lists? That seems unnecessarily restrictive. 
We could just clone the lists instead.
- buildMerkleTree puts the root at the end. Your methods put the root at the 
start. I realize this is how the block explorer does it, but the choice is 
largely arbitrary. It seems better to be internally consistent.

Original comment by hearn@google.com on 20 Apr 2011 at 4:00

[GoogleCodeExporter]

>Would it help if I gave you commit access, then you could work on a Subversion 
branch rather than git?

Now that I have set up my branch on github, I rather like that system. The 
workflow would be roughly the same anyway (you'd still have to merge to trunk), 
and this way, my intermediate commits that need additional fixes (like this 
one) stay out of the official repo.

> Why do you return unmodifiable lists? That seems unnecessarily restrictive. 
We could just clone the lists instead.

That would also work. Either way, any modifications the caller makes to the 
list (or its elements) should not cascade back into the cached block chain. 
Your call.

Should I also make 'getMerkleRoot' return a copy?

> buildMerkleTree puts the root at the end. Your methods put the root at the 
start. I realize this is how the block explorer does it, but the choice is 
largely arbitrary. It seems better to be internally consistent.

I am a huge fan of internal consistency. Will fix the patch. 

Original comment by thilopl...@googlemail.com on 21 Apr 2011 at 1:11

[GoogleCodeExporter]

Sure, let's make things return copies.

Original comment by hearn@google.com on 21 Apr 2011 at 5:42

[GoogleCodeExporter]

> Why do you return unmodifiable lists? That seems unnecessarily restrictive. 
We could just clone the lists instead.

In the process of changing it to clones I found "prior art" in 
Transaction#getInputs() which already "returns a read-only list of the inputs 
of this transaction".

And thinking this over again, why would someone want to modify the returned 
lists, since either way it would not have an impact on the object that produced 
them? This goes well with the expectation that when a container class in Java 
returns a modifiable collection of its elements, it should be a view that you 
can modify the container through (for example HashMap.entrySet). So maybe 
read-only immutable lists are better after all.

Original comment by thilopl...@googlemail.com on 21 Apr 2011 at 12:13

[GoogleCodeExporter]

I don't really care unless one way is a lot more efficient than the other. We 
just have to be aware that on mobile phones object allocation is never free.

Original comment by hearn@google.com on 21 Apr 2011 at 12:39

[GoogleCodeExporter]

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

• Added labels: Component-Persistence

[GoogleCodeExporter]

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

• Added labels: Priority-Low
• Removed labels: Priority-Medium

[GoogleCodeExporter]

Unfortunately this issue is old enough that Thilos patch has vanished. As far 
as I know we now expose all interesting fields through getters though.

Original comment by hearn@google.com on 8 Jan 2013 at 4:30

• Changed state: Fixed