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

Merge master into discv5.2 #234

Merged
merged 20 commits into from
Feb 21, 2024
Merged

Conversation

ackintosh
Copy link
Member

@ackintosh ackintosh commented Feb 13, 2024

Description

Merge lastest master branch into discv5.2.

Notes & open questions

I have updated the Config::session_cache_capacity type from usize to NonZeroUsize in 6d4fce2, for the following reasons:

Change checklist

  • Self-review
  • Documentation updates if relevant
  • Tests if relevant

AgeManning and others added 19 commits July 19, 2023 19:56
* goodbye prefix

* adjust docs
* reject extra data

* reduce diff
* Replace `Handler::spawn` with `build_handler()` and `Handler::start()`

* Test the handler's states after the handler has been terminated

* Remove expected response on handle_auth_message()

* Rename variables for readability
* Change `port` from u16 to NonZeroU16

* Fix tests

* Fix test: the PONG port can't be zero

* Fix clippy warnings
Co-authored-by: ackintosh <[email protected]>
Co-authored-by: Diva M <[email protected]>
Co-authored-by: Age Manning <[email protected]>
* remove log for timed out query. This is always informed in the callback

* expand common logs, unify info on startup

* adjust auth header log

* Update src/service.rs

* Appease clippy

* Realised I was wrong. Don't need this log, my bad

* fmt

---------

Co-authored-by: Age Manning <[email protected]>
# Conflicts:
#	Cargo.toml
#	src/config.rs
#	src/error.rs
#	src/handler/mod.rs
#	src/handler/session.rs
#	src/handler/tests.rs
#	src/rpc.rs
#	src/service.rs
#	src/service/test.rs
since the argument of LruCache::new is NonZeroUsize.
@ackintosh ackintosh marked this pull request as draft February 13, 2024 22:11
@ackintosh ackintosh marked this pull request as ready for review February 14, 2024 20:41
Copy link
Contributor

@emhane emhane left a comment

Choose a reason for hiding this comment

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

keeping up to date, very nice. I found one weird thing though from master, left a suggestion. the fix is out of scope of this pr, but could be cherry-picked to master if included.

enable_packet_filter: false,
request_timeout: Duration::from_secs(1),
vote_duration: Duration::from_secs(30),
query_peer_timeout: Duration::from_secs(2),
query_timeout: Duration::from_secs(60),
request_retries: 1,
session_timeout: Duration::from_secs(86400),
session_cache_capacity: 1000,
session_cache_capacity: NonZeroUsize::new(1000).expect("infallible"),
Copy link
Contributor

Choose a reason for hiding this comment

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

appropriate. always wondering what message to write when calling expect on this with a value > 0.


pub(super) struct ActiveRequests {
/// A list of raw messages we are awaiting a response from the remote.
active_requests_mapping: HashMapDelay<NodeAddress, RequestCall>,
active_requests_mapping: HashMap<NodeAddress, Vec<RequestCall>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

parallelisation, cool

Comment on lines +230 to +232
// Seed is chosen such that all nodes are in the 256th distance of the first node.
let seed = 1652;
let mut keypairs = generate_deterministic_keypair(5, seed);
Copy link
Contributor

Choose a reason for hiding this comment

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

very useful, didn't know this function was available

Comment on lines +1516 to +1518
let received = nodes_response.received_nodes.len();
let expected = distances.len();
warn!(%node_id, %addr, %error, %received, %expected, "FINDNODE request failed with partial results");
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't this wording a bit misleading? distances and nodes don't map 1-1. we can receive more than one node in the response at any distance in the requested range of distances.

Suggested change
let received = nodes_response.received_nodes.len();
let expected = distances.len();
warn!(%node_id, %addr, %error, %received, %expected, "FINDNODE request failed with partial results");
let received = nodes_response.received_nodes.len();
warn!(%node_id, %addr, %error, %received, ?distances, "FINDNODE request failed with partial results");

@AgeManning AgeManning merged commit 088a171 into sigp:discv5.2 Feb 21, 2024
6 checks passed
@ackintosh ackintosh deleted the discv5.2-merge-master branch February 28, 2024 06:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants