feat: adding onValidated observer

[gabrielmer]

Description

Adding an onValidated observer which will run every time a message is received and validated. This comes from the necessity of precisely track message deliveries and network activity.

onRecv observers run before any check is performed on the received message, which means that it runs every time a duplicate or invalid message arrives, which is inefficient and inaccurate for our purpose of tracking only received, unique and valid messages. Therefore, adding this extra option of running an observer for every message after all validation checks pass.

[diegomrsantos]

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

[gabrielmer]

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 dc83a1e 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]

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]

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]

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]

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]

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]

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 waku-org/nwaku#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]

Thanks for the update.

[gabrielmer]

We started having the original issue and were in need of info only available at the nim-libp2p layer, so pushed again changes to this PR

Currently having it over an old nim-libp2p commit so it matches the one used in our latest release, but soon will rebase it.

I think this new approach is simpler and pretty neat, please let me know if a solution like this is something that could be eventually merged into master :)

[gabrielmer]

@diegomrsantos @kaiserd WDYT about this new approach? is it something that could be merged?

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.53%. Comparing base (02c96fc) to head (5c307da).
Report is 40 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff            @@
##           master    #1128    +/-   ##
========================================
  Coverage   84.53%   84.53%            
========================================
  Files          91       92     +1     
  Lines       15517    16423   +906     
========================================
+ Hits        13118    13884   +766     
- Misses       2399     2539   +140     

Files	Coverage Δ
libp2p/protocols/pubsub/gossipsub.nim	85.02% <100.00%> (-1.45%)	⬇️
libp2p/protocols/pubsub/pubsubpeer.nim	82.91% <100.00%> (-3.23%)	⬇️

... and 79 files with indirect coverage changes

[diegomrsantos]

Please update the PR's title and description to reflect its current state.

[gabrielmer]

Please update the PR's title and description to reflect its current state.

Yup, ready 🫡

[diegomrsantos]

Amazing job, thanks a lot!

[gabrielmer]

Amazing job, thanks a lot!

Thanks to you for the time and all the help! 🫶

[kaiserd]

LGTM. Thank you very much.
Feel free to merge. The interop test is inactive for now.

[diegomrsantos]

It seems the test failed here https://github.com/vacp2p/nim-libp2p/actions/runs/10679844554/job/29599995590