-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
peer reputation with separate app & gossipsub score #1028
peer reputation with separate app & gossipsub score #1028
Conversation
…beak/peer_reputation
Note: the scores/config/params for both, gossipsub and app scoring are NOT finalized, this is just the approach, the barebones for our Peer Reputation. We can discuss and (later?) adjust all the parameters however we decide it's for the best |
…of github.com:FuelLabs/fuel-core into leviathanbeak/peer_reputation_with_separate_app_score
…of github.com:FuelLabs/fuel-core into leviathanbeak/peer_reputation_with_separate_app_score
Can review once CI is passing again |
…beak/peer_reputation_with_separate_app_score
…/peer_reputation_with_separate_app_score
…of github.com:FuelLabs/fuel-core into leviathanbeak/peer_reputation_with_separate_app_score
Added peer reputation decay + some refactors regarding the flow of peer reporting |
…of github.com:FuelLabs/fuel-core into leviathanbeak/peer_reputation_with_separate_app_score
} = connection_established; | ||
|
||
self.heartbeat | ||
.on_swarm_event(FromSwarm::ConnectionEstablished( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of these functions are just forwarding events. A helper function could reduce duplication
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of these events need to be extracted separately and passed down to two different behaviours.
I don't see a way to reduce "duplication" since all events are different, this is caused because parent ConnectionHandler
is different than the children behaviours'. So it first needs to be extracted and sent down separately to the children.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made a draft PR here which extracts these conversions out into separate functions: #1109
For the events that are simply pass-throughs, I wonder if the helper methods like .map_handler
and .map_out
could be used as a catch-all 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That looks good, but I understood that freesig meant de-duplicating forwarding of events in on_swarm_event
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
)) | ||
} | ||
IdentifyEvent::Error { peer_id, error } => { | ||
debug!(target: "fuel-p2p", "Identification with peer {:?} failed => {}", peer_id, error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not a good idea to set the target to the wrong thing. The target should be this module not fuel-core
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm it says fuel-p2p
not fuel-core
or we're looking at a different thing?
…e_app_score # Conflicts: # crates/services/p2p/src/p2p_service.rs # crates/services/p2p/src/peer_manager.rs
score: new_score, | ||
}) | ||
} else { | ||
log_missing_peer(&peer_id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This log message might be confusing if the peer is connected as a reserved peer, but they are reported for some reason.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you meant specifically in the context of update_peer_score_with()
, that method works only with non_reserved_connected_peers
HashMap so no reserved peer will be logged from here.
If you meant log_missing_peer
in general, the (reserved) peer could still not be connected at a given point when an action was tried, so it logs that the peer is missing/disconnected.
} | ||
|
||
// `Behaviour` that reports events about peers | ||
pub struct PeerReportBehaviour { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems a little confusing to call this peer report behavior, as it implies this service handles reputation. However, it seems like it handles nearly all the info for each peer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I mostly aimed at "peer reporting" as in report peer events, ie disconnected, connected, identified etc. I am definitely open to a better name
Co-authored-by: Brandon Kite <brandonkite92@gmail.com>
let peer_score = peer_report.get_score_from_report(); | ||
|
||
self.request_sender | ||
.try_send(TaskRequest::RespondWithPeerReport { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about renaming RespondWithPeerReport
-> ApplicationReport
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer we keep RespondWith
and as discussed, the report is about the peer, ApplicationReport
would suggest otherwise
Attempt to partially address @freesig's comment about using a helper function: #1028 (comment) This extracts the conversion of network behaviour events out of the poll function and into separate functions. --------- Co-authored-by: Elvis <elvisdedic@outlook.com>
…of github.com:FuelLabs/fuel-core into leviathanbeak/peer_reputation_with_separate_app_score
Deals with #1028 (comment) Pass the Punisher to PeerManager so it can deal with the banishment internally :)
The change bumps the version to `0.18.0` and exposes `sync_max_get_header` and `sync_max_get_txns` in the helm chart. # Release 0.18.0 ## Overview A new release brings: - Optimization for the execution based on the performance from beta 3 and on internal benchmarks. Improved metrics gathering. - Stabilization and better test coverage of the `fuel-vm`. We removed almost all unsafe code and added test cases for each opcode. Fixed some edge cases with memory in the `fuel-vm`. - Fully integrated Merkle trees and filled all malleable fields in the transactions. - Added retryable messages, removed redundant fields from it, and updated the API to support a new commitment schema. ## What's Changed ### Breaking - All unsafe functions were replaced with safe analog in the `fuel-crypto` - FuelLabs/fuel-vm#346 - `$hp` holds the address of the last available byte in a heap, while previously it was `$hp - 1` - FuelLabs/fuel-vm#377 - Each variant in the `fuel_tx::Input` enum now has its own type - FuelLabs/fuel-vm#364 - Message nonce is unified and now `Bytes32` everywhere - FuelLabs/fuel-vm#394 - Removed the `message_id` field from all places - FuelLabs/fuel-vm#397, FuelLabs/fuel-vm#373, - Unified the block height in all places with the introduction of a new `BlockHeigh` - FuelLabs/fuel-vm#410 - Make SMO instruction take data ptr as an argument - FuelLabs/fuel-vm#404 - Now the chain id affects the signature and predicate's owner and should be passed into the `sign` function - FuelLabs/fuel-vm#406 - Updated the `produce_blocks` endpoint to accept the start time and the number of blocks. All new blocks will use the previous timestamp as a base - #1059 - The `fuel-core` stores only unspent coins and messages, so all API that previously returned spent coins is affected - Prune owned coin idx when inputs are spent by @Voxelot in #1055 - The message proof API has been changed to be compatible with a new version - #1071 - The `fuel-core` now has retryable messages and coin messages. Retryable messages can only be consumed during successful transaction execution. The coin message acts as common coins. `resouces_to_spend` API was replaced with `coins_to_spend` that returns a new `CoinType` type. - #1067 ## All changes * adding fuel-core service monitor to helm chart by @rfuelsh in #1037 * Adding specific app selector by @rfuelsh in #1039 * use sticky sessions for GQL requests to sentries by @Voxelot in #1042 * Add ingress service name to helm chart by @rfuelsh in #1043 * Added test to verify the iteration over owned transactions by @xgreenx in #1041 * Change sentry lb to use clusterIP by @Voxelot in #1045 * Fixed Tempfile dependency by @ControlCplusControlV in #1048 * Write contract code in raw by @freesig in #994 * attempt to use buildjet runners by @Voxelot in #1017 * Set tx pointer during block production by @Voxelot in #1054 * Used `BASE_AMOUNT` for test with bob to pay for fee by @xgreenx in #1057 * Prepare GraphQL Crate for extraction by @ControlCplusControlV in #1012 * Support large contract deployments over p2p by @Voxelot in #1062 * fix yaml and add linting job by @Voxelot in #1063 * Actualized the ABI for message to be compatible with last version of the Solidity contracts. by @xgreenx in #1060 * feat: Contract state and assets merkle data storage by @bvrooman in #1008 * Take into account the previous block timestamp during block production by @xgreenx in #1059 * Prune owned coin idx when inputs are spent by @Voxelot in #1055 * Retrayable messages fuel-core part by @xgreenx in #1067 * chore: Additional Tests for Contract States and Balances by @bvrooman in #1073 * Rust 1.68.1 & Sparse Registry by @Voxelot in #1077 * Nginx tuneup by @Voxelot in #1080 * chore: Update balance and state Merkleization by @bvrooman in #1084 * Support sticky session in the client by @xgreenx in #1079 * Added e2e test to run 1000 `dry_run` by @xgreenx in #1091 * Use docker.io auth credentials to overcome rate limiting by @Voxelot in #1092 * honeycomb tracing subscription by @Voxelot in #1083 * Update withdrawal proof API to support proving from a block header committed to L1 by @xgreenx in #1071 * Upstream v0.17.8 by @Voxelot in #1102 * use chainid for transactions and predicates by @Voxelot in #1107 * fix the dry-run e2e test to actually perform dry runs by @Voxelot in #1111 * peer reputation with separate app & gossipsub score by @leviathanbeak in #1028 * Adding sentry data synchronization by @rfuelsh in #1115 * Capture metrics for graphql api by @Voxelot in #1113 * Update cargo audit and add to commit by @freesig in #1152 * add task params by @leviathanbeak in #1159 * Took into account that block creation could take some time by @xgreenx in #1162 * Applying optimizations from `0.17.11` release by @xgreenx in #1158 **Full Changelog**: v0.17.3...v0.18.0
The approach is mostly detailed in Notion.
These are the first steps of our Peer Reputation approach, there are certain todos left, could be added later on or if needed I can add them with this PR.
Todo:
Ref #943