elixir-node icon indicating copy to clipboard operation
elixir-node copied to clipboard

Fix expected data type when decoding RLP in PeersConnection

Open gspasov opened this issue 7 years ago • 10 comments

Fill free to look at the discussion that led to this issue. When decoding RLP data, we should be expecting a particular type of data, so the type should be matched as well, instead of using _vsn.

gspasov avatar Sep 03 '18 07:09 gspasov

We do handle types, I don't think we should care about versions really, just have the latest version for the different messages

d-velev avatar Sep 03 '18 08:09 d-velev

When you use TYPE.rlp_decode or Serialization.rlp_decode_only so the type is checked.

cytadela8 avatar Sep 03 '18 08:09 cytadela8

I think what currently is missing is some more checks if the data decoded from RLP matches what we are expecting. For sure in some places we will accept a list, where binary is expected.

cytadela8 avatar Sep 03 '18 08:09 cytadela8

We do handle types, I don't think we should care about versions really, just have the latest version for the different messages.

I partially agree that we don't really need care about versions now, because whenever we make a change we remove support for old version. But I did small preparations in my code to handle different versions when that will be necessary. (You can add deserialization for different version numbers in all structures handled by Aecore.Utils.Serializable) That will come useful after minenet launch, but is not important now.

cytadela8 avatar Sep 03 '18 08:09 cytadela8

I guess there was a misunderstanding on my part here. I was talking for the versions, but maybe as you say it's not that vital to pattern-match them ATM. @cytadela8 Maybe you could decide to either have this as low_priority or closing the issue.

gspasov avatar Sep 03 '18 09:09 gspasov

I'm confused. Is this issue about adding checks that version is correct when we decode? We already do that for all Aecore.Utils.Serializable structures. Where is it missing?

cytadela8 avatar Sep 03 '18 14:09 cytadela8

I was referring to places in PeerConnection like this one. Maybe it's not needed to match it.

gspasov avatar Sep 03 '18 15:09 gspasov

@gspasov @cytadela8 so we need to check rlp versions when decoding for PeerConnection?

thepiwo avatar Sep 11 '18 10:09 thepiwo

World won't explode (nothing horrible will happen) if we don't, but it's just a simple check that should be done. If the version mismatches then we are trying to talk with a node that's incompatible with us. (that's what the version is for)

cytadela8 avatar Sep 11 '18 11:09 cytadela8

okay thanks, was unsure if this is still todo

thepiwo avatar Sep 11 '18 11:09 thepiwo