search

vacp2p / nim-libp2p

libp2p implementation in Nim
https://vacp2p.github.io/nim-libp2p/docs/
MIT License
240 stars 52 forks source link

chore: creating branch to test Waku's received messages #1128

Open gabrielmer opened 1 week ago

gabrielmer commented 1 week ago

This PR is not intended to be merged

This new branch, enhance-logs-for-nwaku was created and will be used by nwaku to use as a test nim-libp2p version in simulations to help analyze message reliability.

The necessity comes from needing to dig deeper and add extra logging in nim-libp2p in order to precisely track message deliveries and network activity.

At the same time, we can't add the logs directly to nim-libp2p as they would be too many, we can't have them in TRACE as running at that level would pollute too much our simulations' logs, and also because we track Waku-specific constructs such as the message hash.

Therefore, this branch exists :)

Please let me know if there's any issue or suggestion!

diegomrsantos commented 1 week ago

Are you using the latest master? It seems there are some formatting changes that should be addressed in the latest master.

gabrielmer commented 1 week ago

Are you using the latest master? It seems there are some formatting changes that should be addressed in the latest master.

I branched out from https://github.com/vacp2p/nim-libp2p/commit/dc83a1e9b68f00b3be7e09febdb1a3f877321b9a so we do have the formatting changes. We actually use currently v1.3.0 but branched out one commit after the release to have the formatting in.

We also use nph v0.5.1 so we're aligned with that :))

There may be many compilation issues now, I wanted to run the CI first and see what pops up, but will fix them promptly

diegomrsantos commented 1 week ago

I see. It's cause a new param was added exactly when the param list stopped being in one line and it was difficult to see the change https://github.com/vacp2p/nim-libp2p/pull/1128/files#diff-abbf987f19a24c9940cd01bde79a1b1e0819f1c3b6d7ba2efa04ad680d022c78R385

kaiserd commented 1 week ago

Thinking about how we could potentially integrate this in master:

1) Would it be possible to express what you need in a app layer validator call back that gets handed tovalidateAndRelay ? That seems to be a generally useful approach. It could be called after the standard nim-libp2p validation, and add app specific validation. This might allow moving the additional logging to Waku / applications.

2) In case 1) is not enough: logging specific parts necessary in nim-libp2p could be enabled at compile time using enabled_topcis This is fine as long as this additional logging does not cause any runtime overhead.

gabrielmer commented 1 week ago
  1. Would it be possible to express what you need in a app layer validator call back that gets handed tovalidateAndRelay ? That seems to be a generally useful approach. It could be called after the standard nim-libp2p validation, and add app specific validation. This might allow moving the additional logging to Waku / applications.

Thanks so much! I think I can do exactly the same I'm doing in this PR but adding a generic construct to add app specific logic.

I think it can be hard to accurately understand what we exactly need without actually coding it and finding constraints we might hadn't think about.

If it sounds good to you, I can implement the same thing by doing this generic extension, get feedback, iterate and go from there.

lmk what you think :)) @kaiserd @Ivansete-status

diegomrsantos commented 1 week ago

I believe you can experiment here until you find what you need, but eventually it's better to find a way to add the changes to master for two reasons: 1) it can benefit other projects if they need the same. 2) it's going to be bothersome to keep this branch up-to-date with master.

gabrielmer commented 1 week ago

I believe you can experiment here until you find what you need, but eventually it's better to find a way to add the changes to master for two reasons:

  1. it can benefit other projects if they need the same.
  2. it's going to be bothersome to keep this branch up-to-date with master.

I agree 100%, the idea is to now change this PR with a more generic approach with the target to actually get merged to master

gabrielmer commented 4 days ago

It does seem that we can actually get the information that we want without changing anything in nim-libp2p by using onSend observers instead of onRecv which was our first approach.

We will merge https://github.com/waku-org/nwaku/pull/2851 for it.

If after using it for simulations we conclude that this approach logs all the necessary information, then I will close this PR. Otherwise, will implement over here a solution for it that could be merged to nim-libp2p

diegomrsantos commented 4 days ago

Thanks for the update.