helium / miner

Miner for the helium blockchain
Apache License 2.0
607 stars 265 forks source link

Regression around the management of rssi #1083

Closed BenoitDuffez closed 3 years ago

BenoitDuffez commented 3 years ago

I suspect there is a regression in https://github.com/helium/miner/commit/317f1edf089ec5d17c673016a527be6f9d7f5dd1#diff-72653b0c74b5401df251b16d8460def13ffde2b484db58696c596c5ec4898bfaL642-R648 is where the rssis/rssic split has been modified a bit, and one invocation of miner_lora:packet_rssi uses a static true for UseRSSIS where I think it should use

UseRSSIS = case Chain /= undefined andalso blockchain:config(?poc_version, blockchain:ledger(Chain)) of
    {ok, X} when X > 10 -> true;
    _ -> false
end,

like it does in another place in the file.

BenoitDuffez commented 3 years ago

Also, I suspect we should not use rssis but rather rssic.

Vagabond commented 3 years ago

It's a static true in the other case because that's for normal packets, and those don't affect consensus.

RssiC is Channel RSSI, RSSIS is signal RSSI as I understand it.

BenoitDuffez commented 3 years ago

We don't send the RSSI in rssis but rather in rssic. We always have been, and packet forwarding worked a few weeks ago.

Now I see a crash:

2021-09-24 12:29:07.681 1528 [info] <0.1578.0>@miner_lora:handle_udp_packet:427 PUSH_DATA [{<<"rxpk">>,[[{<<"aesk">>,0},{<<"brd">>,261},{<<"codr">>,<<"4/5">>},{<<"data">>,<<"AF9ZUlZJXzQ5bmVnQaRPAwA5Oy2R4cw=">>},{<<"datr">>,<<"SF7BW125">>},{<<"freq">>,868.1},{<<"jver">>,2},{<<"modu">>,<<"LORA">>},{<<"rsig">>,[[{<<"ant">>,0},{<<"chan">>,5},{<<"lsnr">>,-7.0},{<<"rssic">>,-115}]]},{<<"size">>,23},{<<"stat">>,1},{<<"time">>,<<"2021-09-24T12:29:07.665049Z">>},{<<"tmst">>,421046188}]]}] from 8248060143997291342 on 43682
2021-09-24 12:29:07.683 1528 [notice] <0.1578.0>@miner_lora:handle_packets:600 Routing {eui,931991820658030,4122024327536335199}
2021-09-24 12:29:07.684 1528 [error] emulator@Undefined:Undefined:Undefined Error in process <0.18600.5> on node 'miner@127.0.0.1' with exit value:
{{badkey,<<"rssis">>},[{maps,get,[<<"rssis">>,#{<<"ant">> => 0,<<"chan">> => 5,<<"lsnr">> => -7.0,<<"rssic">> => -115}],[]},{miner_lora,packet_rssi,2,[{file,"/builds/helium/miner/src/miner_lora.erl"},{line,786}]},{miner_lora,send_to_router,4,[{file,"/builds/helium/miner/src/miner_lora.erl"},{line,656}]}]}

Were you using rssic before and switched over to rssis recently?

BenoitDuffez commented 3 years ago

Interestingly enough, witnessing works:

2021-09-24 08:20:43.779 1528 [info] <0.1578.0>@miner_lora:handle_udp_packet:427 PUSH_DATA [{<<"rxpk">>,[[{<<"aesk">>,0},{<<"brd">>,3},{<<"codr">>,<<"4/5">>},{<<"data">>,<<"QDDaAAHmWr9UAOI/AHJ1iXqW3NymkhMFqu2rS0miJI5L5012LSePdBrC5Wk5glXW1niHbQ==">>},{<<"datr">>,<<"SF12BW125">>},{<<"freq">>,867.7},{<<"jver">>,2},{<<"modu">>,<<"LORA">>},{<<"rsig">>,[[{<<"ant">>,0},{<<"chan">>,3},{<<"lsnr">>,-14.0},{<<"rssic">>,-114}]]},{<<"size">>,52},{<<"stat">>,1},{<<"time">>,<<"2021-09-24T08:20:43.653493Z">>},{<<"tmst">>,2696905852}]]}] from 8248060143997291342 on 43682
2021-09-24 08:20:44.095 1528 [info] <0.1577.0>@miner_onion_server:decrypt:360 sending witness at RSSI: -114, Frequency: 867.7, SNR: -14.0
2021-09-24 08:20:44.097 1528 [info] <0.1577.0>@miner_onion_server:decrypt:370 could not decrypt packet received via radio: treating as a witness
2021-09-24 08:20:44.103 1528 [info] <0.7346.5>@miner_onion_server:send_witness:188 sending witness at RSSI: -114, Frequency: 867.7, SNR: -14.0

No rssis in the json but the RSSI is properly extracted and the witness is properly handled (shows on explorer, earns HNT, etc).

This is only seen for regular LoRaWAN packets that people route on the network.

Vagabond commented 3 years ago

What packet forwarder are you using?

BenoitDuffez commented 3 years ago

Ours, called CPF.

snip commented 3 years ago

Ours == Kerlink

Vagabond commented 3 years ago

So, I would suggest adding a fallback there to use RSSIC if RSSIS is not available?

BenoitDuffez commented 3 years ago

What I don't understand is that it was working a couple of weeks ago. We didn't change our GWMP JSON so I'm assuming that there is a regression in the JSON parsing in the miner.

Vagabond commented 3 years ago

Yes, we assumed that that field was mandatory in GWMP v2

BenoitDuffez commented 3 years ago

GWMP v2 uses the rsig object (vs rssi directly for GWMP v1), but the RSSI field is rssic.
rssis is present only on Semtech Reference Design v2 gateways, but rssic is present on all gateways.

I think we should switch over to rssic and perhaps use rssis only if it is available.

Vagabond commented 3 years ago

Yes, that's what I'm proposing, we use rssis if available and fall back on rssic

Vagabond commented 3 years ago

Something like

Key = case UseRSSIS andalso maps:is_key(<<"rssis">>, Packet) of ...
BenoitDuffez commented 3 years ago

Exactly.

Vagabond commented 3 years ago

Can you make a PR?