vacp2p / research

Thinking in code
MIT License
62 stars 4 forks source link

discv5-enr-lookup #25

Closed decanus closed 4 years ago

decanus commented 4 years ago

@kdeme so we seem to have a problem here, the field some is added to our ENR. This is successful, the node is started with that enr, however when we lookup and try to get some that field is always none which seems weird to me. Could this be another discv5 bug?

What I did to check:

kdeme commented 4 years ago

Not sure what goes wrong. I think this should work as the enr should be copied over as is received (it must, else validation can't work). However, I've never tested these custom fields, so I'll add a test case and see if anything goes wrong.

decanus commented 4 years ago

so @kdeme, I cannot tell if I am doing something wrong or discv5 is. From my investigation it seems like an issue with discv5?

kdeme commented 4 years ago

So the problem is that in the nim-eth enr code custom fields are only supported/working for seq[byte]. That is, any custom k:v pair that is read is added as that type. So currently, you have to read it as seq[byte] with tryGet, else it will fail.

But yeah, nim-eth should be improved here.

decanus commented 4 years ago

@kdeme that seems weird because I can call the tryGet with a uint right under initializing it it works.

kdeme commented 4 years ago

@decanus Yes, it is because currently when reading the raw received ENR, it only creates integers or string for the "known" keys. And stores unknown (custom) keys in seq[byte], as at that moment it has no information of what type this should be.

Then next, you do a tryGet with the type you gave (uint), but the code currently checks what type the value for that key has (seq[byte]) and sees it is not matching what you passed as type, hence the none.

When creating the ENR, it has the type information for the custom field, and stores it as such, and next converts it to raw bytes.

Yes, not exactly user friendly.