Null hash for transaction, mainly with Standalone blockchain

[ESchouten]

Fix for empty array returned when (hash == null && parsed == true && rlpEncoded != null). See pull request #1057

[coveralls]

Coverage increased (+0.4%) to 57.264% when pulling be80cf8 on ESchouten:null-hash-value-fix into 8731060 on ethereum:develop.

[zilm13]

@ESchouten could you, please, add a test which is failing in current develop, but will pass with your fix. I couldn't reproduce the same bug and have not find the place in code where it could happen. Looks like I'm missing something.
Also such test could be helpful not to open this bug again in future.

[ESchouten]

@zilm13 I agree writing a unit test which demonstrates the race condition would be ideal. Problem is, I have not yet identified the source of this issue.
The error occurs while deploying a smart contract on the standalone blockchain on faster pc's and servers, while on slower servers and often in the IDE's debug mode it does not, making finding the source harder.
If it helps, we're using https://github.com/adridadou/eth-propeller-ethj

[ESchouten]

I am however able to repoduce the state of where rlpEncoded is initialized and the hash byte array is empty, though when trying to debug the origin of this, the race condition does not occur, probably due to the IDE pausing threads etc.

The parsed boolean is always true, due to the Transaction class constructor used, so rlpParse() is never executed.

The only code I could find initializing the rlpEncoded array and not setting the hash value is located in InternalTransaction.java, though the class' method does not seem to be called.

[zilm13]

@ESchouten so, it's race condition, in this case setting getEncoded to be synchronized makes more sense for me. What do you think?

[ESchouten]

@zilm13 Agree, seems to work aswell! Still, in that case, we're still assuming the hash string is set when rlpEncoded is set and parsed is true, which works right now, but might not be very maintainable.

E.g. The class InternalTransaction extends Transaction and overrides getEncoded but doesn't set the hash value.

What do you think?

[zilm13]

@ESchouten why it's not maintainable?

InternalTransaction never goes out of VM

[ESchouten]

@zilm13 Ah that explains.

[ESchouten]

@zilm13 Reverted my change and made getEncoded synchronized to prevent the race condition, as you suggested.