authority-discovery: Populate DHT records with public listen addresses #6298

lexnv · 2024-10-30T16:08:42Z

This PR's main goal is to add public listen addresses to the DHT authorities records. This change improves the discoverability of validators that did not provide the --public-addresses flag.

This PR populates the authority DHT records with public listen addresses if any.

The change effectively ensures that addresses are added to the DHT record in following order:

Public addresses provided by CLI --public-addresses
Maximum of 4 public (global) listen addresses (if any)
Any external addresses discovered from the network (ie from /identify protocol)

While at it, this PR adds the following constraints on the number of addresses:

Total number of addresses cached is bounded at 16 (increased from 10).
A maximum number of 32 addresses are published to DHT records (previously unbounded).
A maximum of 4 global listen addresses are utilized.

This PR also removes the following warning:
WARNING: No public address specified, validator node may not be reachable.

Next Steps

deploy and monitor in versi network

Closes: #6280
Part of: #5266

cc @paritytech/networking

Signed-off-by: Alexandru Vasile <[email protected]>

dmitry-markin · 2024-10-30T16:54:36Z

substrate/client/cli/src/commands/run_cmd.rs

-			eprintln!(
-				"WARNING: No public address specified, validator node may not be reachable.
-				Consider setting `--public-addr` to the public IP address of this node.
-				This will become a hard requirement in future versions."
-			);


Honestly, I would keep this warning, because there is no guarantee we will have a global listen address nor that the external address will be discovered correctly (with libp2p address translation). Especially since there is a practice of "hiding" validators behind firewalls / NATs.

lexnv · 2024-11-01T10:03:37Z

Versi Network Triage Report

Repo	Count	Level	Triage report
polkadot-sdk	10000	error	🥩 for session starting at block .* no BEEFY authority key found in store
polkadot-sdk	144	warn	Report .: . to .. Reason: .. Banned, disconnecting. ( Invalid justification. Banned, disconnecting.)
polkadot-sdk	50	error	Warp sync failed. Continuing with full sync.
polkadot-sdk	50	error	Can't use warp sync mode with a partially synced database. Reverting to full sync mode.
polkadot-sdk	18	warn	Report .: . to .. Reason: .. Banned, disconnecting. ( Peer disconnected with inflight after backoffs. Banned, disconnecting.)
polkadot-sdk	4	error	💔 Called `on_validated_block_announce` with a bad peer ID .*
polkadot-sdk	3	error	Call to `DetermineUndisputedChain` failed
polkadot-sdk	3	warn	Received msg before first active leaves update. This is not expected - message will be dropped.
polkadot-sdk	3	warn	Re-finalized block #.* (.) in the canonical chain, current best finalized is #.

🥩 for session starting at block .* no BEEFY authority key found in store versi is misconfigured after a scale up and down some time ago
Warp sync failed and Can't use warp sync mode with a partially synced database. - believe these are related to deploying a new version and having old databases available

addreses Signed-off-by: Alexandru Vasile <[email protected]>

lexnv · 2024-11-01T12:19:43Z

I have moved the warning to the authority-discovery mechanism instead of removing it.

This has the following implications:

warning is printed only once, with the first authority record publish to DHT if any
libp2p: Local listen addresses are propagated via swarm events, and therefore the libp2p object needs to be polled a few times. On my local machine, the libp2p backend produces listen addresses before authority-discovery is started.
litep2p: Liten addresses are available immediately via API calls

Signed-off-by: Alexandru Vasile <[email protected]>

substrate/client/authority-discovery/src/worker.rs

dmitry-markin · 2024-11-01T13:25:53Z

warning is printed only once, with the first authority record publish to DHT if any

Is it printed soon enough for a node operator to notice?

lexnv · 2024-11-01T15:50:13Z

From my testing it should be printed roughly within 10 seconds:

2024-11-01 15:17:15.675  INFO main sc_cli::runner: Parity Polkadot
...
2024-11-01 15:17:26.671  WARN tokio-runtime-worker sub-authority-discovery: No public addresses configured and no global...

I used cumulus/zombienet/tests/0003-full_node_catching_up.toml to double check this:

alice and bob relay chain validators: they print the warning one single time
charlie parachain collator: no warning
dave and eve parachain full node: no warning
ferdie: cumulus node with embedded light client: no warning

Co-authored-by: Dmitry Markin <[email protected]>

Signed-off-by: Alexandru Vasile <[email protected]>

kianenigma · 2024-11-01T18:39:25Z

Who would be a good second reviewer of this code? I would love to get it approved, but it is too far from my domain to review in a meaningful way.

dmitry-markin · 2024-11-01T19:24:24Z

I would ask @alexggh

lexnv · 2024-11-04T07:58:30Z

Alex and Sebastian are on holiday for quite some time

Would love if maybe @alindima and @bkchr could have a look since I'm not that familiar with collators 🙏

Signed-off-by: Alexandru Vasile <[email protected]>

This reverts commit 49f6d2e.

paritytech-cmd-bot-polkadot-sdk · 2024-11-05T10:14:38Z

Created backport PR for stable2407:

[stable2407] Backport #6298 #6363 with remaining conflicts!

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin backport-6298-to-stable2407
git worktree add --checkout .worktree/backport-6298-to-stable2407 backport-6298-to-stable2407
cd .worktree/backport-6298-to-stable2407
git reset --hard HEAD^
git cherry-pick -x 762f824cb3adf23c72a6645f575057bfb543850b
git push --force-with-lease

#6298) This PR's main goal is to add public listen addresses to the DHT authorities records. This change improves the discoverability of validators that did not provide the `--public-addresses` flag. This PR populates the authority DHT records with public listen addresses if any. The change effectively ensures that addresses are added to the DHT record in following order: 1. Public addresses provided by CLI `--public-addresses` 2. Maximum of 4 public (global) listen addresses (if any) 3. Any external addresses discovered from the network (ie from `/identify` protocol) While at it, this PR adds the following constraints on the number of addresses: - Total number of addresses cached is bounded at 16 (increased from 10). - A maximum number of 32 addresses are published to DHT records (previously unbounded). - A maximum of 4 global listen addresses are utilized. This PR also removes the following warning: `WARNING: No public address specified, validator node may not be reachable.` ### Next Steps - [ ] deploy and monitor in versi network Closes: #6280 Part of: #5266 cc @paritytech/networking --------- Signed-off-by: Alexandru Vasile <[email protected]> Co-authored-by: Dmitry Markin <[email protected]> Co-authored-by: Bastian Köcher <[email protected]> (cherry picked from commit 762f824)

paritytech-cmd-bot-polkadot-sdk · 2024-11-05T10:14:45Z

Successfully created backport PR for stable2409:

[stable2409] Backport #6298 #6364

* master: (129 commits) pallet-revive: Use `RUSTUP_TOOLCHAIN` if set (#6365) [eth-rpc] proxy /health (#6360) [Release|CI/CD] adjust release pipelines (#6366) Bump the known_good_semver group across 1 directory with 3 updates (#6339) Run check semver in MQ (#6287) [Deprecation] deprecate treasury `spend_local` call and related items (#6169) refactor and harden check_core_index (#6217) litep2p: Update litep2p to v0.8.0 (#6353) [pallet-staking] Additional check for virtual stakers (#5985) migrate pallet-remarks to v2 bench syntax (#6291) Remove leftover references of Wococo (#6361) snowbridge: allow account conversion for Ethereum accounts (#6221) authority-discovery: Populate DHT records with public listen addresses (#6298) Bounty Pallet: add `approve_bounty_with_curator` call to `bounties` pallet (#5961) Silent annoying log (#6351) [pallet-revive] rework balance transfers (#6187) `statement-distribution`: RFC103 implementation (#5883) Disable flaky tests reported in #6343 / #6345 (#6346) migrate pallet-recovery to benchmark V2 syntax (#6299) inclusion emulator: correctly handle UMP signals (#6178) ...

michalkucharczyk · 2024-11-05T22:23:34Z

substrate/client/authority-discovery/src/worker.rs

+
+		if let Some(multiaddr::Protocol::P2p(peer_id)) = address.iter().last() {
+			if peer_id != *local_peer_id.as_ref() {
+				error!(


This line for sure breaks this test:

polkadot-sdk/substrate/zombienet/0002-validators-warp-sync/test-validators-warp-sync.zndsl

Line 38 in 52a7325

alice: count of log lines containing "error" is 0 within 10 seconds

Does it need to be error? If so, I can adjust the condition in test to be 1.

I can change it back to warning for now, sorry missed that 🙏

The error is coming from the validator setup:

2024-11-05 20:07:15.454 2024-11-05 18:07:15.454 ERROR tokio-runtime-worker sub-authority-discovery: No public addresses configured and no global listen addresses found. Authority DHT record may contain unreachable addresses. Consider setting `--public-addr` to the public IP address of this node. This will become a hard requirement in future versions for authorities.

Will adjust the testing instead to accommodate this 🙏

This PR fixes the `0002-validators-warp-sync` zombienet test by matching for a specific error case. The following PR introduced a new error for validators that don't have public addresses available for publishing in the DHT: - #6298 The log line was moved from `eprintln!("Warning: ...` in sc-cli to the authority discovery where it is printed properly as an error. cc @paritytech/sdk-node Signed-off-by: Alexandru Vasile <[email protected]>

…6380) This PR ensures that external addresses with different PeerIDs are not propagated to the higher layer of the network code. While at it, this ensures that libp2p only adds the `/p2p/peerid` part to the discovered address if it does not contain it already. This is a followup from: - #6298 cc @paritytech/networking --------- Signed-off-by: Alexandru Vasile <[email protected]> Co-authored-by: Dmitry Markin <[email protected]>

#6298) This PR's main goal is to add public listen addresses to the DHT authorities records. This change improves the discoverability of validators that did not provide the `--public-addresses` flag. This PR populates the authority DHT records with public listen addresses if any. The change effectively ensures that addresses are added to the DHT record in following order: 1. Public addresses provided by CLI `--public-addresses` 2. Maximum of 4 public (global) listen addresses (if any) 3. Any external addresses discovered from the network (ie from `/identify` protocol) While at it, this PR adds the following constraints on the number of addresses: - Total number of addresses cached is bounded at 16 (increased from 10). - A maximum number of 32 addresses are published to DHT records (previously unbounded). - A maximum of 4 global listen addresses are utilized. This PR also removes the following warning: `WARNING: No public address specified, validator node may not be reachable.` ### Next Steps - [ ] deploy and monitor in versi network Closes: #6280 Part of: #5266 cc @paritytech/networking --------- Signed-off-by: Alexandru Vasile <[email protected]> Co-authored-by: Dmitry Markin <[email protected]> Co-authored-by: Bastian Köcher <[email protected]>

…addresses (#6298)" This reverts commit a519c47.

lexnv added 6 commits October 30, 2024 17:59

cli: Revert warning

e42d31d

Signed-off-by: Alexandru Vasile <[email protected]>

authority-discovery: Obtain global addresses from listen addresses

fd68062

Signed-off-by: Alexandru Vasile <[email protected]>

authority-discovery: Chain an deduplicate addresses

36a4a1e

Signed-off-by: Alexandru Vasile <[email protected]>

authority-discovery: Refactor address_without_p2p into a dedicated fn

f1c7ed6

Signed-off-by: Alexandru Vasile <[email protected]>

authority-discovery: Cap max addresses to publish

5ca1f8b

Signed-off-by: Alexandru Vasile <[email protected]>

authority-discovery: Bump max cached addresses to 16

0b091ef

Signed-off-by: Alexandru Vasile <[email protected]>

lexnv self-assigned this Oct 30, 2024

Add prdoc

6bbfce0

Signed-off-by: Alexandru Vasile <[email protected]>

dmitry-markin approved these changes Oct 30, 2024

View reviewed changes

authority-discovery: Warn in authority discovery about non public

58a8432

addreses Signed-off-by: Alexandru Vasile <[email protected]>

lexnv added 4 commits November 1, 2024 14:20

tests: Adress testing

9c0d40d

Signed-off-by: Alexandru Vasile <[email protected]>

authority-disovery: Replace validator with authority

0bb4526

Signed-off-by: Alexandru Vasile <[email protected]>

prdoc: Add more details

9b34c65

Signed-off-by: Alexandru Vasile <[email protected]>

Merge remote-tracking branch 'origin/master' into lexnv/warn-dhtfix

f29ebcb

dmitry-markin reviewed Nov 1, 2024

View reviewed changes

substrate/client/authority-discovery/src/worker.rs Outdated Show resolved Hide resolved

lexnv and others added 2 commits November 1, 2024 17:51

Update substrate/client/authority-discovery/src/worker.rs

1ce39e8

Co-authored-by: Dmitry Markin <[email protected]>

Merge remote-tracking branch 'origin/master' into lexnv/warn-dhtfix

d95fd51

lexnv mentioned this pull request Nov 1, 2024

Invalid warning for parachain node #6280

Closed

Fix clippy

e6f329d

Signed-off-by: Alexandru Vasile <[email protected]>

lexnv and others added 7 commits November 4, 2024 15:34

authority-discovery: Fix build

094b5f5

Signed-off-by: Alexandru Vasile <[email protected]>

authority-discovery: Add docs for warn_public_addresses

53a07d4

Signed-off-by: Alexandru Vasile <[email protected]>

authority-discovery: Add AddressType wrapper for better logs

9fb02e0

Signed-off-by: Alexandru Vasile <[email protected]>

authority-discovery: Log error for all types on mismatched peerId

9fed62e

Signed-off-by: Alexandru Vasile <[email protected]>

Merge branch 'master' into lexnv/warn-dhtfix

0a4e74c

prdoc: Try to remove sc-cli for semvercheck

eab484c

Signed-off-by: Alexandru Vasile <[email protected]>

primitives: Revert panicHook import to fix CI for now

49f6d2e

Signed-off-by: Alexandru Vasile <[email protected]>

lexnv requested a review from koute as a code owner November 4, 2024 16:46

lexnv and others added 2 commits November 4, 2024 18:47

Merge branch 'master' into lexnv/warn-dhtfix

e408fee

Revert "primitives: Revert panicHook import to fix CI for now"

31b3595

This reverts commit 49f6d2e.

lexnv added this pull request to the merge queue Nov 5, 2024

Merged via the queue into master with commit 762f824 Nov 5, 2024
189 of 192 checks passed

lexnv deleted the lexnv/warn-dhtfix branch November 5, 2024 10:13

paritytech-cmd-bot-polkadot-sdk bot mentioned this pull request Nov 5, 2024

[stable2407] Backport #6298 #6363

Closed

paritytech-cmd-bot-polkadot-sdk bot mentioned this pull request Nov 5, 2024

[stable2409] Backport #6298 #6364

Closed

michalkucharczyk reviewed Nov 5, 2024

View reviewed changes

This was referenced Nov 6, 2024

substrate/zombienet: Fix 0002-validators-warp-sync #6379

Merged

net/discovery: Do not propagate external addr with different peerIDs #6380

Merged

lexnv mentioned this pull request Nov 15, 2024

backport: Bring litep2p fixes and latest version to stable2409 #6497

Closed

lexnv added a commit that referenced this pull request Nov 18, 2024

Revert "authority-discovery: Populate DHT records with public listen …

a4720dd

…addresses (#6298)" This reverts commit a519c47.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

authority-discovery: Populate DHT records with public listen addresses #6298

authority-discovery: Populate DHT records with public listen addresses #6298

lexnv commented Oct 30, 2024 •

edited

Loading

dmitry-markin Oct 30, 2024

lexnv commented Nov 1, 2024

lexnv commented Nov 1, 2024

dmitry-markin commented Nov 1, 2024

lexnv commented Nov 1, 2024

kianenigma commented Nov 1, 2024

dmitry-markin commented Nov 1, 2024

lexnv commented Nov 4, 2024

paritytech-cmd-bot-polkadot-sdk bot commented Nov 5, 2024

paritytech-cmd-bot-polkadot-sdk bot commented Nov 5, 2024

michalkucharczyk Nov 5, 2024

lexnv Nov 6, 2024

lexnv Nov 6, 2024

authority-discovery: Populate DHT records with public listen addresses #6298

authority-discovery: Populate DHT records with public listen addresses #6298

Conversation

lexnv commented Oct 30, 2024 • edited Loading

Next Steps

dmitry-markin Oct 30, 2024

Choose a reason for hiding this comment

lexnv commented Nov 1, 2024

Versi Network Triage Report

lexnv commented Nov 1, 2024

dmitry-markin commented Nov 1, 2024

lexnv commented Nov 1, 2024

kianenigma commented Nov 1, 2024

dmitry-markin commented Nov 1, 2024

lexnv commented Nov 4, 2024

paritytech-cmd-bot-polkadot-sdk bot commented Nov 5, 2024

paritytech-cmd-bot-polkadot-sdk bot commented Nov 5, 2024

michalkucharczyk Nov 5, 2024

Choose a reason for hiding this comment

lexnv Nov 6, 2024

Choose a reason for hiding this comment

lexnv Nov 6, 2024

Choose a reason for hiding this comment

lexnv commented Oct 30, 2024 •

edited

Loading