Add error handling in newHashKey

[Mdaiki0730]

Description

When calling the function newHashKey() if an error occurs while filling the new key with random data from the defined random source, the created hash key will be zeroed, which means, the content of the slice will be all 0's.

I think it's better to handle errors properly.

[codecov]

Codecov Report

Merging #499 (8748158) into main (0681636) will increase coverage by 0.16%.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #499      +/-   ##
==========================================
+ Coverage   65.44%   65.60%   +0.16%     
==========================================
  Files         278      278              
  Lines       37936    37995      +59     
==========================================
+ Hits        24828    24928     +100     
+ Misses      11305    11261      -44     
- Partials     1803     1806       +3     

Impacted Files	Coverage Δ
p2p/pex/addrbook.go	71.66% <100.00%> (ø)
privval/socket_listeners.go	79.72% <0.00%> (-4.06%)	⬇️
types/tx.go	90.24% <0.00%> (-3.66%)	⬇️
privval/signer_listener_endpoint.go	88.88% <0.00%> (-2.39%)	⬇️
crypto/ed25519/ed25519.go	45.12% <0.00%> (-1.22%)	⬇️
mempool/reactor.go	78.57% <0.00%> (-1.10%)	⬇️
p2p/transport.go	82.77% <0.00%> (-0.71%)	⬇️
p2p/node_info.go	95.12% <0.00%> (ø)
proxy/multi_app_conn.go	47.66% <0.00%> (ø)
crypto/vrf/vrf_r2ishiguro.go
... and 10 more

[torao]

This title and description don't appear to represent the modifications contained in this PR correctly. Is newHashKey() not fixed to return an error?

[torao]

Separating certain functions into a package for the purpose of passing coverage seems to me to be out of line with module design and doesn't seem to be a good idea. And I'm also concerned about the project-local implicit rules that arise.

• What was the key point of the problems
• What options were available
• What were the considerations
• Then, why did you decide this was the best approach?

[torao]

It seems too assertive compared to other sources and golang's customary package alias. I think crand, scrand, or stdcrand is enough.

[torao]

No one looking at the source would know why this function is separated in this package. Any purpose or intent that can only be recognized outside of the source code should be noted in the comment.

[Mdaiki0730]

@torao
After thinking about the following, I am implementing it now.

• Problem: Error handling of crand.Read in newHashKey provides insufficient coverage, but it is difficult to write tests for it.
• Options:
  • Ignore coverage → The convention of merging after all green is broken
  • Wrap crand.Read as a private function → A wrapper just for testing the standard package is born
  • Wrap crand.Read as public function in libs → Allows aggregation of packages that wrap functions for which test is essentially unnecessary

The current option is selected because the layer where the error occurs is not at the application level but is connected to the test ignore.
I would like to hear your opinion if there are better options for this.

[torao]

@Mdaiki0730
Looking for other code that uses crand.Read(), there is a function CRandBytes() in crypto/random.go that could be used for this purpose. It may work the same as the current newHashKey(), so how about using this instead of creating a specific package?

https://github.com/line/ostracon/blob/main/crypto/random.go#L9-L22

[Mdaiki0730]

I think very good@@
Thank you for finding this function!

[Mdaiki0730]

I created another PR because we found a completely different solution than the this PR.
New One: #509