DevP2p: Get node IP address and udp port from Socket, if not included in PING packet #10705

seunlanlege · 2019-05-31T17:16:41Z

closes #10613

… packet

parity-cla-bot · 2019-05-31T17:16:45Z

It looks like @seunlanlege signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

dvdplm · 2019-05-31T19:22:15Z

cc @fjl

fjl · 2019-06-01T22:18:43Z

The 'from' field in Ping shouldn't be used at all to determine where to send the response. I recommend sending Pong back to the UDP packet source address unconditionally.

The only thing 'from' is useful for is checking whether the remote node is aware of its own endpoint.

(Edit: s/to/from)

tomusdrw

I don't think it's correct currently, we should probably handle the error from NodeEndpoint::from_rlp and then fallback to from: SocketAddr if it's not correct.

@fjl The PONG packet is sent to the address that we got from the Socket not to from/to field of the PING packet. We do use the TCP port from from (if available) to know what TCP port to connect to.
So I think the handling is fine.

AFAICT the point of this PR is to handle a case where from is an invalid address (nulls) - in such case we fallback to socket address for UDP and only use that node for discovery (never for sync/TCP)

util/network-devp2p/src/discovery.rs

fjl · 2019-06-03T16:13:24Z

Sorry, misread the code a bit. The PR claims to fix #10613, I guess the fix is that early return is avoided if the 'from' field is invalid?

seunlanlege · 2019-06-04T11:01:10Z

@fjl yes

ngotchac

For the bootnodes to be useful, they should be added to the node_buckets actually. Here, they don't do anything.
What you should do I think is call self.update_node(node) in way that it adds it the the node_buckets but don't appear in the TableUpdates returned value. Does that make sense?

ngotchac

LGTM

util/network-devp2p/src/discovery.rs

util/network-devp2p/src/node_table.rs

Agreed with Tomask

tomusdrw

Looks good, just needs the documentation to be sorted out.

util/network-devp2p/src/discovery.rs

util/network-devp2p/src/node_table.rs

util/network-devp2p/src/discovery.rs

ngotchac · 2019-06-05T15:18:09Z

util/network-devp2p/src/discovery.rs

@@ -728,7 +743,7 @@ impl<'a> Discovery<'a> {
 		trace!(target: "discovery", "Got {} Neighbours from {:?}", results_count, &from);
 		for r in rlp.at(0)?.iter() {
 			let endpoint = NodeEndpoint::from_rlp(&r)?;
-			if !endpoint.is_valid() {
+			if !endpoint.is_valid_sync_node() {


Shouldn't it be is_valid_bootnode here?

endpoint.is_valid_sync_node == endpoint.is_valid

Well that's kind of my point. Here we are receiving neighbors, and I guess it would be possible that some of those are actually bootnodes. I didn't think of that earlier, but here I think you should revisit let endpoint = NodeEndpoint::from_rlp(&r)?; and endpoint.is_valid_sync_node to take this case into account: we want to add those to our discovery list, without adding them to our syncing list.

util/network-devp2p/src/discovery.rs

dvdplm

Good stuff. Minor nitpicks added.

util/network-devp2p/src/discovery.rs

util/network-devp2p/src/node_table.rs

util/network-devp2p/src/discovery.rs

util/network-devp2p/src/node_table.rs

Co-Authored-By: David <dvdplm@gmail.com>

…me-parent * master: updater: fix static id hashes initialization (#10755) Use fewer threads for snapshotting (#10752) Die error_chain, die (#10747) Fix deprectation warnings on nightly (#10746) fix docker tags for publishing (#10741) DevP2p: Get node IP address and udp port from Socket, if not included in PING packet (#10705) ethcore: enable ECIP-1054 for classic (#10731)

* master: updater: fix static id hashes initialization (#10755) Use fewer threads for snapshotting (#10752) Die error_chain, die (#10747) Fix deprectation warnings on nightly (#10746) fix docker tags for publishing (#10741) DevP2p: Get node IP address and udp port from Socket, if not included in PING packet (#10705) ethcore: enable ECIP-1054 for classic (#10731) Stop breaking out of loop if a non-canonical hash is found (#10729) Refactor Clique stepping (#10691) Use RUSTFLAGS to set the optimization level (#10719) SecretStore: non-blocking wait of session completion (#10303) removed secret_store folder (#10722) SecretStore: expose restore_key_public in HTTP API (#10241) Revert "enable lto for release builds (#10717)" (#10721) enable lto for release builds (#10717) Merge `Notifier` and `TransactionsPoolNotifier` (#10591)

* master: Enable aesni (#10756) remove support of old SS db formats (#10757) [devp2p] Don't use `rust-crypto` (#10714) updater: fix static id hashes initialization (#10755) Use fewer threads for snapshotting (#10752) Die error_chain, die (#10747) Fix deprectation warnings on nightly (#10746) fix docker tags for publishing (#10741) DevP2p: Get node IP address and udp port from Socket, if not included in PING packet (#10705) ethcore: enable ECIP-1054 for classic (#10731) Stop breaking out of loop if a non-canonical hash is found (#10729) Refactor Clique stepping (#10691) Use RUSTFLAGS to set the optimization level (#10719) SecretStore: non-blocking wait of session completion (#10303) removed secret_store folder (#10722) SecretStore: expose restore_key_public in HTTP API (#10241) Revert "enable lto for release builds (#10717)" (#10721) enable lto for release builds (#10717) Merge `Notifier` and `TransactionsPoolNotifier` (#10591)

…-even * master: [devp2p] Update to 2018 edition (#10716) Add a way to signal shutdown to snapshotting threads (#10744) Enable aesni (#10756) remove support of old SS db formats (#10757) [devp2p] Don't use `rust-crypto` (#10714) updater: fix static id hashes initialization (#10755) Use fewer threads for snapshotting (#10752) Die error_chain, die (#10747) Fix deprectation warnings on nightly (#10746) fix docker tags for publishing (#10741) DevP2p: Get node IP address and udp port from Socket, if not included in PING packet (#10705) ethcore: enable ECIP-1054 for classic (#10731)

…p/chore/aura-log-validator-set-in-epoch-manager * dp/chore/aura-warn-when-validators-is-1-or-even: [devp2p] Update to 2018 edition (#10716) Add a way to signal shutdown to snapshotting threads (#10744) Enable aesni (#10756) remove support of old SS db formats (#10757) [devp2p] Don't use `rust-crypto` (#10714) updater: fix static id hashes initialization (#10755) Use fewer threads for snapshotting (#10752) Die error_chain, die (#10747) Fix deprectation warnings on nightly (#10746) fix docker tags for publishing (#10741) Update ethcore/src/engines/validator_set/simple_list.rs DevP2p: Get node IP address and udp port from Socket, if not included in PING packet (#10705) ethcore: enable ECIP-1054 for classic (#10731)

holiman · 2019-06-20T09:21:18Z

Could you LMK when this is rolled out on the bootnodes? Currently, I get timeouts trying to check them

INFO [06-20|11:18:13.129] New local node record                    seq=1 id=9e0289db6e78a121 ip=127.0.0.1 udp=35546 tcp=0
node didn't respond: RPC timeout
INFO [06-20|11:18:13.670] New local node record                    seq=1 id=b183029189ff0be4 ip=127.0.0.1 udp=44773 tcp=0
node didn't respond: RPC timeout
INFO [06-20|11:18:14.209] New local node record                    seq=1 id=6eb9f21b386f3185 ip=127.0.0.1 udp=60007 tcp=0
node didn't respond: RPC timeout
INFO [06-20|11:18:14.747] New local node record                    seq=1 id=6bba547ab04d6e52 ip=127.0.0.1 udp=56096 tcp=0
node didn't respond: RPC timeout

… in PING packet (#10705) * get node IP address and udp port from Socket, if not included in PING packet * prevent bootnodes from being added to host nodes * code corrections * code corrections * code corrections * code corrections * docs * code corrections * code corrections * Apply suggestions from code review Co-Authored-By: David <dvdplm@gmail.com>

* ethcore/res: activate atlantis classic hf on block 8772000 (#10766) * fix docker tags for publishing (#10741) * fix: aura don't add `SystemTime::now()` (#10720) This commit does the following: - Prevent overflow in `verify_timestamp()` by not adding `now` to found faulty timestamp - Use explicit `CheckedSystemTime::checked_add` to prevent potential consensus issues because SystemTime is platform depedent - remove `#[cfg(not(time_checked_add))]` conditional compilation * Update version * Treat empty account the same as non-exist accounts in EIP-1052 (#10775) * DevP2p: Get node IP address and udp port from Socket, if not included in PING packet (#10705) * get node IP address and udp port from Socket, if not included in PING packet * prevent bootnodes from being added to host nodes * code corrections * code corrections * code corrections * code corrections * docs * code corrections * code corrections * Apply suggestions from code review Co-Authored-By: David <dvdplm@gmail.com> * Add a way to signal shutdown to snapshotting threads (#10744) * Add a way to signal shutdown to snapshotting threads * Pass Progress to fat_rlps() so we can abort from there too. * Checking for abort in a single spot * Remove nightly-only weak/strong counts * fix warning * Fix tests * Add dummy impl to abort snapshots * Add another dummy impl for TestSnapshotService * Remove debugging code * Return error instead of the odd Ok(()) Switch to AtomicU64 * revert .as_bytes() change * fix build * fix build maybe

* ethcore/res: activate atlantis classic hf on block 8772000 (#10766) * fix docker tags for publishing (#10741) * merge-backports * Update version * remove clique engine from backports * Reset blockchain properly (#10669) * delete BlockDetails from COL_EXTRA * better proofs * added tests * PR suggestions * adds rpc error message for --no-ancient-blocks (#10608) * adds error message for --no-ancient-blocks, closes #10261 * Apply suggestions from code review Co-Authored-By: seunlanlege <seunlanlege@gmail.com> * Treat empty account the same as non-exist accounts in EIP-1052 (#10775) * fix: aura don't add `SystemTime::now()` (#10720) This commit does the following: - Prevent overflow in `verify_timestamp()` by not adding `now` to found faulty timestamp - Use explicit `CheckedSystemTime::checked_add` to prevent potential consensus issues because SystemTime is platform depedent - remove `#[cfg(not(time_checked_add))]` conditional compilation * DevP2p: Get node IP address and udp port from Socket, if not included in PING packet (#10705) * get node IP address and udp port from Socket, if not included in PING packet * prevent bootnodes from being added to host nodes * code corrections * code corrections * code corrections * code corrections * docs * code corrections * code corrections * Apply suggestions from code review Co-Authored-By: David <dvdplm@gmail.com> * Revert "fix: aura don't add `SystemTime::now()` (#10720)" This reverts commit f104784. * Add a way to signal shutdown to snapshotting threads (#10744) * Add a way to signal shutdown to snapshotting threads * Pass Progress to fat_rlps() so we can abort from there too. * Checking for abort in a single spot * Remove nightly-only weak/strong counts * fix warning * Fix tests * Add dummy impl to abort snapshots * Add another dummy impl for TestSnapshotService * Remove debugging code * Return error instead of the odd Ok(()) Switch to AtomicU64 * revert .as_bytes() change * fix build * fix build maybe

dvdplm · 2019-06-27T10:16:54Z

Could you LMK when this is rolled out on the bootnodes?

@holiman should be up now. Thank you for reporting this! :)

get node IP address and udp port from Socket, if not included in PING…

fb102a7

… packet

seunlanlege requested review from ngotchac and tomusdrw May 31, 2019 17:16

dvdplm approved these changes May 31, 2019

View reviewed changes

tomusdrw reviewed Jun 3, 2019

View reviewed changes

util/network-devp2p/src/discovery.rs Outdated Show resolved Hide resolved

tomusdrw added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. M4-core ⛓ Core client code / Rust. labels Jun 3, 2019

ngotchac suggested changes Jun 3, 2019

View reviewed changes

util/network-devp2p/src/discovery.rs Show resolved Hide resolved

prevent bootnodes from being added to host nodes

351b257

ngotchac suggested changes Jun 4, 2019

View reviewed changes

seunlanlege added 2 commits June 4, 2019 14:54

code corrections

706dceb

code corrections

783a66d

ngotchac previously approved these changes Jun 4, 2019

View reviewed changes

tomusdrw suggested changes Jun 4, 2019

View reviewed changes

util/network-devp2p/src/discovery.rs Outdated Show resolved Hide resolved

tomusdrw reviewed Jun 4, 2019

View reviewed changes

util/network-devp2p/src/discovery.rs Outdated Show resolved Hide resolved

tomusdrw reviewed Jun 4, 2019

View reviewed changes

util/network-devp2p/src/discovery.rs Outdated Show resolved Hide resolved

util/network-devp2p/src/discovery.rs Show resolved Hide resolved

util/network-devp2p/src/discovery.rs Outdated Show resolved Hide resolved

code corrections

9400dd1

ngotchac reviewed Jun 5, 2019

View reviewed changes

util/network-devp2p/src/node_table.rs Show resolved Hide resolved

code corrections

2056d6e

tomusdrw approved these changes Jun 5, 2019

View reviewed changes

util/network-devp2p/src/discovery.rs Show resolved Hide resolved

util/network-devp2p/src/node_table.rs Show resolved Hide resolved

util/network-devp2p/src/node_table.rs Show resolved Hide resolved

ngotchac suggested changes Jun 5, 2019

View reviewed changes

docs

56e9eb3

seunlanlege force-pushed the node-discover branch from 8e44e5b to 56e9eb3 Compare June 5, 2019 15:43

code corrections

cd43652

code corrections

2633494

ngotchac suggested changes Jun 6, 2019

View reviewed changes

util/network-devp2p/src/discovery.rs Show resolved Hide resolved

ngotchac approved these changes Jun 11, 2019

View reviewed changes

util/network-devp2p/src/discovery.rs Show resolved Hide resolved

dvdplm approved these changes Jun 11, 2019

View reviewed changes

dvdplm added B1-patch-beta 🕷🕷 A8-looksgood 🦄 Pull request is reviewed well. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Jun 11, 2019

Apply suggestions from code review

cdf80c5

Co-Authored-By: David <dvdplm@gmail.com>

dvdplm merged commit 1c076af into master Jun 12, 2019

ordian deleted the node-discover branch June 17, 2019 10:34

holiman mentioned this pull request Jun 20, 2019

params: add new bootnodes ethereum/go-ethereum#19680

Closed

ordian added the B0-patch-stable 🕷 Pull request should also be back-ported to the stable branch. label Jun 25, 2019

ordian added this to the 2.6 milestone Jun 25, 2019

s3krit mentioned this pull request Jun 25, 2019

Beta 2.5.3 #10776

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DevP2p: Get node IP address and udp port from Socket, if not included in PING packet #10705

DevP2p: Get node IP address and udp port from Socket, if not included in PING packet #10705

seunlanlege commented May 31, 2019

parity-cla-bot commented May 31, 2019

dvdplm commented May 31, 2019

fjl commented Jun 1, 2019 •

edited

Loading

tomusdrw left a comment

fjl commented Jun 3, 2019

seunlanlege commented Jun 4, 2019

ngotchac left a comment

ngotchac left a comment

tomusdrw left a comment

ngotchac Jun 5, 2019

seunlanlege Jun 5, 2019

ngotchac Jun 5, 2019

dvdplm left a comment

holiman commented Jun 20, 2019

dvdplm commented Jun 27, 2019

DevP2p: Get node IP address and udp port from Socket, if not included in PING packet #10705

DevP2p: Get node IP address and udp port from Socket, if not included in PING packet #10705

Conversation

seunlanlege commented May 31, 2019

parity-cla-bot commented May 31, 2019

dvdplm commented May 31, 2019

fjl commented Jun 1, 2019 • edited Loading

tomusdrw left a comment

Choose a reason for hiding this comment

fjl commented Jun 3, 2019

seunlanlege commented Jun 4, 2019

ngotchac left a comment

Choose a reason for hiding this comment

ngotchac left a comment

Choose a reason for hiding this comment

tomusdrw left a comment

Choose a reason for hiding this comment

ngotchac Jun 5, 2019

Choose a reason for hiding this comment

seunlanlege Jun 5, 2019

Choose a reason for hiding this comment

ngotchac Jun 5, 2019

Choose a reason for hiding this comment

dvdplm left a comment

Choose a reason for hiding this comment

holiman commented Jun 20, 2019

dvdplm commented Jun 27, 2019

fjl commented Jun 1, 2019 •

edited

Loading