search

ethereum / ethereumj

DEPRECATED! Java implementation of the Ethereum yellowpaper. For JSON-RPC and other client features check Ethereum Harmony
GNU Lesser General Public License v3.0
2.18k stars 1.09k forks source link

Null hash for transaction, mainly with Standalone blockchain #1267

Closed ESchouten closed 5 years ago

ESchouten commented 5 years ago

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

coveralls commented 5 years ago

Coverage Status

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

zilm13 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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

ESchouten commented 5 years ago

@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 commented 5 years ago

@ESchouten why it's not maintainable?

InternalTransaction never goes out of VM

ESchouten commented 5 years ago

@zilm13 Ah that explains.

ESchouten commented 5 years ago

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