helium / miner

Miner for the helium blockchain
Apache License 2.0
608 stars 266 forks source link

relx version used uses non portable code #593

Closed BenoitDuffez closed 3 years ago

BenoitDuffez commented 3 years ago

relx contains a function to generate a random hash: relx_gen_id

The current version of relx contains a portable way of doing so:

dd count=1 bs=4 if=/dev/urandom 2> /dev/null | od -x  | head -n1 | awk '{print $2$3}'

However the version currently embedded with the miner does the following:

od -t x -N 4 /dev/urandom | head -n1 | awk '{print $2}'

The problem for this is that the busybox implementation of od doesn't support the arguments. Example on a Kerlink gateway:

root@klk-wiis-050029:~ # dd count=1 bs=4 if=/dev/urandom 2> /dev/null | od -x  | head -n1 | awk '{print $2$3}'
955ffdcc
root@klk-wiis-050029:~ # od -t x -N 4 /dev/urandom | head -n1 | awk '{print $2}'                              
od: invalid option -- 't'
BusyBox v1.24.1 (2020-08-03 15:13:50 CEST) multi-call binary.

Usage: od [-aBbcDdeFfHhIiLlOovXx] [FILE]

I'd have submitted a PR if I knew how relx is invoked.

madninja commented 3 years ago

Why are you invoking relx at all on the target system?

BenoitDuffez commented 3 years ago

I'm not, I'm assuming that the build system creates bin/miner, this one contains a method relx_gen_id which contains the non portable code. I can't remember which invocation of bin/miner <xxx> triggers the problem. I'll try to find out.

madninja commented 3 years ago

Did you get this resolved @BenoitDuffez ?

BenoitDuffez commented 3 years ago

I can't! The build system uses an (old?) version of relx that uses a non portable call to od. My current solution is to patch the generated bin/miner with:

diff --git a/opt/miner/bin/miner b/opt/miner/bin/miner
index 66731c8..8c8c84a 100755
--- a/opt/miner/bin/miner
+++ b/opt/miner/bin/miner
@@ -185,7 +185,7 @@ relx_rem_sh() {

 # Generate a random id
 relx_gen_id() {
-    od -t x -N 4 /dev/urandom | head -n1 | awk '{print $2}'
+    dd count=1 bs=4 if=/dev/urandom 2> /dev/null | od -x | head -n1 | awk '{print $2$3}'
 }

 # Control a node

I guess that a real solution is to change the relx dependency so that the generated code uses dd instead of od. If I knew how to do this without breaking everything, I'd have submitted a PR. I'm sorry that I am only able to report the problem.

It is not a big deal though, patching the generated script is easy to do with a CI script.

madninja commented 3 years ago

Unfortunately the startup scripts are generated by erlang itself, and work just fine on many systems. I'm going to close this since this is not something we can fix here

BenoitDuffez commented 3 years ago

I did not invent the fix, it comes from Erlang itself: https://github.com/erlware/relx/blob/master/priv/templates/extended_bin#L360 As you can see the master branch contains portable code which works with Busybox. The version used by the Helium miner seems to be prior to this patch: https://github.com/erlware/relx/commit/200e12815ef665338cea1280244e9c4f9dfa06b9

The goal of the issue I created here is basically "use a version of <whatever, I don't know how it's invoked> that uses a relx release that includes the patch above".

I am sorry I didn't put all of this information on my initial post.

madninja commented 3 years ago

I did not invent the fix, it comes from Erlang itself: https://github.com/erlware/relx/blob/master/priv/templates/extended_bin#L360 As you can see the master branch contains portable code which works with Busybox. The version used by the Helium miner seems to be prior to this patch: erlware/relx@200e128

The goal of the issue I created here is basically "use a version of <whatever, I don't know how it's invoked> that uses a relx release that includes the patch above".

I am sorry I didn't put all of this information on my initial post.

Thanks for putting in the detail. Yes I should have been clearer that this is not fixed by us but by a newer version of erlang. We have a number of PRs ready to upgrade all miner dependencies to erlang 23, which we hope to be able to complete soon.

BenoitDuffez commented 3 years ago

Thanks for taking the time to read all this. We shall see how Erlang 23 impacts this, but the impact is very limited anyway.