Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support querying peer reputation #2392

Merged

Conversation

nazar-pc
Copy link
Contributor

@nazar-pc nazar-pc commented Nov 18, 2023

Description

Trivial change that resolves #2185.

Since there was a mix of who and peer_id argument names nearby I changed them all to peer_id.

Checklist

  • My PR includes a detailed description as outlined in the "Description" section above
  • My PR follows the labeling requirements of this project (at minimum one label for T
    required)
  • I have made corresponding changes to the documentation (if applicable)
  • I have added tests that prove my fix is effective or that my feature works (if applicable)

@altonen
Copy link
Contributor

altonen commented Nov 18, 2023

Please see my comment in the issue you've opened. I don't think this PR can be accepted as-is, not only because I think the locking mechanism in PeerStore is not optimal but also because the whole reputation system is dubious (#2257)

@nazar-pc
Copy link
Contributor Author

nazar-pc commented Nov 18, 2023

BTW this PR removes channel usage for reputation changes, it now interacts with PeerStoreHandle directly.

@nazar-pc
Copy link
Contributor Author

Is there anything else I can do to help to move this forward?

@altonen
Copy link
Contributor

altonen commented Nov 21, 2023

Sorry for delayed reply, I've been busy with something else but I'll look into this issue soon.

I need to double-check that updating the reputation in NetworkService is OK. With high peer counts and with lots of message it may become an issue because the GRANDPA/transactions are doing a lot of updates.

@nazar-pc
Copy link
Contributor Author

nazar-pc commented Dec 1, 2023

Friendly ping here

@bkchr
Copy link
Member

bkchr commented Dec 2, 2023

@dmitry-markin could you take a look at this pr please?

@dmitry-markin dmitry-markin added R0-silent Changes should not be mentioned in any release notes T0-node This PR/Issue is related to the topic “node”. labels Dec 4, 2023
@dmitry-markin
Copy link
Contributor

Sorry for delayed reply, I've been busy with something else but I'll look into this issue soon.

I need to double-check that updating the reputation in NetworkService is OK. With high peer counts and with lots of message it may become an issue because the GRANDPA/transactions are doing a lot of updates.

I don't think it affects the contention on the mutex in any way. When the reputations were updated asynchronously, we still ended up locking the mutex in NetworkWorker::run() every time we received a request to update a reputation value.

Copy link
Contributor

@dmitry-markin dmitry-markin left a comment

Choose a reason for hiding this comment

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

LGTM! And thanks for replacing who with peer_id — we follow this convention now and gradually move all the networking codebase to it.

substrate/client/network/src/service/traits.rs Outdated Show resolved Hide resolved
@bkchr bkchr enabled auto-merge (squash) December 7, 2023 09:12
auto-merge was automatically disabled December 7, 2023 09:34

Head branch was pushed to by a user without write access

@nazar-pc nazar-pc requested a review from bkchr December 7, 2023 10:44
@dmitry-markin dmitry-markin enabled auto-merge (squash) December 7, 2023 10:59
@dmitry-markin dmitry-markin merged commit 9f3c67b into paritytech:master Dec 7, 2023
115 of 116 checks passed
@nazar-pc nazar-pc deleted the query-current-peer-reputation branch December 7, 2023 11:16
ParthDesai pushed a commit to autonomys/polkadot-sdk that referenced this pull request Dec 10, 2023
# Description

Trivial change that resolves
paritytech#2185.

Since there was a mix of `who` and `peer_id` argument names nearby I
changed them all to `peer_id`.

# Checklist

- [x] My PR includes a detailed description as outlined in the
"Description" section above
- [x] My PR follows the [labeling requirements](CONTRIBUTING.md#Process)
of this project (at minimum one label for `T`
  required)
- [x] I have made corresponding changes to the documentation (if
applicable)
- [ ] I have added tests that prove my fix is effective or that my
feature works (if applicable)

---------

Co-authored-by: Bastian Köcher <git@kchr.de>
ordian added a commit that referenced this pull request Dec 11, 2023
* tsv-disabling: (155 commits)
  Fix failing rc-automation GHA (#2648)
  [ci] Return CI_IMAGE variable (#2647)
  Support querying peer reputation (#2392)
  [ci] Update rust to 1.74 (#2545)
  Relax approval requirements on CI files (#2564)
  Added AllSiblingSystemParachains matcher to be used at a parachain level (#2422)
  Improve polkadot sdk docs (#2631)
  Bridges subtree update (#2602)
  pallet-xcm: add new flexible `transfer_assets()` call/extrinsic (#2388)
  [ci] Move rust-features.sh script to .gitlab folder (#2630)
  Bump parity-db from 0.4.10 to 0.4.12 (#2635)
  sp-core: Rename VrfOutput to VrfPreOutput (#2534)
  chore: fix typo (#2596)
  Bump tracing-core from 0.1.31 to 0.1.32 (#2618)
  chore: fixed std wasm build of xcm (#2535)
  Fix PRdoc that have been previously drafted with older schema (#2623)
  Github Workflow migrations (#1574)
  Bridges update subtree (#2625)
  PVF: Add Secure Validator Mode (#2486)
  statement-distribution: Add tests for incoming acknowledgements (#2498)
  ...
ordian added a commit that referenced this pull request Dec 15, 2023
* tsv-disabling: (155 commits)
  Fix failing rc-automation GHA (#2648)
  [ci] Return CI_IMAGE variable (#2647)
  Support querying peer reputation (#2392)
  [ci] Update rust to 1.74 (#2545)
  Relax approval requirements on CI files (#2564)
  Added AllSiblingSystemParachains matcher to be used at a parachain level (#2422)
  Improve polkadot sdk docs (#2631)
  Bridges subtree update (#2602)
  pallet-xcm: add new flexible `transfer_assets()` call/extrinsic (#2388)
  [ci] Move rust-features.sh script to .gitlab folder (#2630)
  Bump parity-db from 0.4.10 to 0.4.12 (#2635)
  sp-core: Rename VrfOutput to VrfPreOutput (#2534)
  chore: fix typo (#2596)
  Bump tracing-core from 0.1.31 to 0.1.32 (#2618)
  chore: fixed std wasm build of xcm (#2535)
  Fix PRdoc that have been previously drafted with older schema (#2623)
  Github Workflow migrations (#1574)
  Bridges update subtree (#2625)
  PVF: Add Secure Validator Mode (#2486)
  statement-distribution: Add tests for incoming acknowledgements (#2498)
  ...
bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this pull request Mar 25, 2024
# Description

Trivial change that resolves
paritytech#2185.

Since there was a mix of `who` and `peer_id` argument names nearby I
changed them all to `peer_id`.

# Checklist

- [x] My PR includes a detailed description as outlined in the
"Description" section above
- [x] My PR follows the [labeling requirements](CONTRIBUTING.md#Process)
of this project (at minimum one label for `T`
  required)
- [x] I have made corresponding changes to the documentation (if
applicable)
- [ ] I have added tests that prove my fix is effective or that my
feature works (if applicable)

---------

Co-authored-by: Bastian Köcher <git@kchr.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
R0-silent Changes should not be mentioned in any release notes T0-node This PR/Issue is related to the topic “node”.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Add ability to query current peer reputation
4 participants