-
Notifications
You must be signed in to change notification settings - Fork 743
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
authority-discovery: Ignore multi-addresses without IP from DHT records #6545
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
In kusama we also receive addresses of form:
Have added extra checks to accommodate this 🙏 |
if !address.iter().any(|protocol| std::matches!(protocol, Protocol::P2p(_))) { | ||
address.push(Protocol::P2p(litep2p::PeerId::from(peer).into())); | ||
if !address.is_external_address_valid(peer) { | ||
log::warn!( |
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.
Will downgrade to debug after more testing, here and below
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
); | ||
continue | ||
}; | ||
if self.litep2p.add_known_address(peer.into(), iter::once(address.clone().into())) == 0usize { |
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.
With this PR deployed, we receive the following warnings for adding addresses (only on Kusama validators):
sub-libp2p: couldn't add known address (/ip4/127.0.0.1/tcp/30333/p2p/12D3KooWRjoe4poRBkBtuJ5JJGYanS7PWTfPc8sN4zvQ3gFEfSP6) for PeerId("12D3KooWRjoe4poRBkBtuJ5JJGYanS7PWTfPc8sN4zvQ3gFEfSP6"), unsupported transport
All warnings coming from this are related to addresses pointing to the localhost.
Will let this deployment run for a bit and then I believe its safe to turn these into debug
Triage Report Versi-Network
Debug Logs# Publish
2024-11-25 18:30:35.292 DEBUG tokio-runtime-worker sub-authority-discovery: Publishing authority DHT record peer_id='12D3KooWKew6C37uUVcTHKBTvHaCE8v5G2Kv5TzhVP2GdYgWDRBS' with addresses='["/ip4/../tcp/30333"]'
# Fetching
2024-11-25 18:30:33.829 DEBUG tokio-runtime-worker sub-authority-discovery: Found addresses for authority Public(f8694fbdf03070992f07164cccf7298c42994add1d8d01e6bf785abd778ae060 (16ciAFQU...)): {"/ip4/../tcp/30333/p2p/12D3KooWD8SxxoLeeMh8iADpj3nQosizZBWpjZZ6m9nKnpe7SsHJ", "/ip4/../tcp/30333/p2p/12D3KooWD8SxxoLeeMh8iADpj3nQosizZBWpjZZ6m9nKnpe7SsHJ"} Would love another look over this 🙏 |
// Verify the external address is valid. | ||
let multiaddr: sc_network_types::multiaddr::Multiaddr = addr.clone().into(); | ||
if multiaddr.is_external_address_valid() && | ||
multiaddr.ensure_peer_id(self.local_peer_id.into()).is_some() |
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.
Just cross-checking if the following is intended:
ensure_peer_id
modifies multiaddr
, but the new value (which may contain p2p part) may get skipped.
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.
Yep, unfortunately libp2p API forces us to pass the event with a reference to the multiaddr (&'a multiaddr
). I think this should be fine as long as we validate the address here. We also re-add /p2p/[peer]
in the authority-discovery where this matters in publishing records 🤔
/cmd prdoc --audience node_dev --bump patch --force |
Signed-off-by: Alexandru Vasile <[email protected]>
All GitHub workflows were cancelled due to failure one of the required jobs. |
Looks like the |
Kusama has several authority-discovery records published without a proper mutlti-address format.
authority="CzHcp6nEsT4NJuAbf4yCS2J8KKYpvuAe63bkWrPWhjaaT6w" .. addresses={.., /p2p/12D3KooWFegWJubWCeJCbWpDEAwukQmXKaWTyRgRRZ8NNdzmuxSr, ..}
In the previous example, an multiaddress of form
/p2p/12D3KooWFegWJubWCeJCbWpDEAwukQmXKaWTyRgRRZ8NNdzmuxSr
cannot establish a connection to the authority.This PR changes the following:
/p2p/[peer]
formatNext Steps
Closes: #6518
cc @paritytech/networking