Skip to content

Conversation

gerceboss
Copy link

  • Clean structure
  • Better config for discovery and ability to handle both v4 and v5

bindIp = bindIp,
enrAutoUpdate = enrAutoUpdate,
rng = rng
)

proc receiveV5*(d: Protocol, a: Address, packet: openArray[byte]): Result[void, cstring] =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to port this function receiveV5 to nim-eth

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to directly use the nim-eth inside receiveV5 function or rewrite it according to nim-eth's receive ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nim-eth's receive return no error, only print logs. So we need to make it return error.

@gerceboss gerceboss requested a review from jangko July 29, 2025 14:03
discv5_network_bytes.inc(packet.len.int64, labelValues = [$Direction.In])

let packet = ?d.codec.decodePacket(a, packet)
let decoded = d.codec.decodePacket(a, packet)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jangko Something like this , correct? We can reuse DiscResult from discovery v4 but will need to have some common file for them, then the ones specific to their functionalities and variables.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally we want to share from discovery V5 to V4, not the other way around.

What I mean by porting receiveV5, the one in nim-eth should be fixed, so we don't have two copies of receive proc.
And discovery v5 designed to accept external shared stream transport, and don't have to open it itself.

Same with discovery V4.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jangko ,So how should I proceed in this repository for now? update implementations in nim-eth?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having that code duplication in the current solution of discv5 integration (with discv4) is definitely a bit ugly and not great for maintainability.

I've been wanting to add some other discv5 init calls to the API to be able to pass in an already created transport. This being mostly needed for Portal, but would be useful also for this double discv4/discv5 on same UDP port use case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants