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.

Sign up for GitHub

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

Jump to bottom

Support querying peer reputation #2392

Merged
merged 5 commits into from
Dec 7, 2023
Merged

Support querying peer reputation #2392

merged 5 commits into from
Dec 7, 2023

Conversation

nazar-pc
Copy link
Contributor

@nazar-pc nazar-pc commented Nov 18, 2023 •
edited by dmitry-markin
Loading

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)

@nazar-pc nazar-pc mentioned this pull request Nov 18, 2023
Closed
2 tasks
@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 nazar-pc force-pushed the query-current-peer-reputation branch from 768b110 to ecd99a5 Compare November 18, 2023 13:54
@nazar-pc
Copy link
Contributor Author

nazar-pc commented Nov 18, 2023 •
edited
Loading

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.

dmitry-markin
dmitry-markin approved these changes Dec 4, 2023
View reviewed changes
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 subspace/polkadot-sdk that referenced this pull request Dec 10, 2023
@nazar-pc @bkchr @ParthDesai
Support querying peer reputation (paritytech#2392)
3b7cf1a 
# 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>
@ParthDesai ParthDesai mentioned this pull request Dec 10, 2023
Merged
ordian added a commit that referenced this pull request Dec 11, 2023
@ordian
Merge branch 'tsv-disabling' into ao-disabling-disputes
80f371e 
* 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)
  ...
This was referenced Dec 12, 2023
Merged
Merged
ordian added a commit that referenced this pull request Dec 15, 2023
@ordian
Merge branch 'tsv-disabling' into ao-disabling-statement-distribution
9842b9e 
* 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)
  ...
@github-actions github-actions bot mentioned this pull request Feb 19, 2024
Closed
@github-actions github-actions bot mentioned this pull request Mar 13, 2024
Closed
bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this pull request Mar 25, 2024
@nazar-pc @bkchr
Support querying peer reputation (paritytech#2392)
39ea1d2 
# 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>
@nazar-pc nazar-pc mentioned this pull request May 28, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dmitry-markin dmitry-markin dmitry-markin approved these changes

@andresilva andresilva Awaiting requested review from andresilva

@bkchr bkchr Awaiting requested review from bkchr

Assignees
No one assigned
Labels
R0-silent Changes should not be mentioned in any release notes T0-node This PR/Issue is related to the topic “node”.
Projects
Networking
Status: Done ✅
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add ability to query current peer reputation
4 participants
@nazar-pc @altonen @bkchr @dmitry-markin