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

sc-authority-discovery: test addresses_to_publish_respects_existing_p2p_protocol broken #3887

Closed
sandreim opened this issue Mar 29, 2024 · 4 comments · Fixed by #3895
Closed
Labels
T10-tests This PR/Issue is related to tests.

Comments

@sandreim
Copy link
Contributor

---- worker::tests::addresses_to_publish_respects_existing_p2p_protocol stdout ----
thread 'worker::tests::addresses_to_publish_respects_existing_p2p_protocol' panicked at substrate/client/authority-discovery/src/worker/tests.rs:739:5:
assertion left == right failed: Expected Multiaddr from TestNetwork to not be altered.
left: ["/ip6/2001:db8::/tcp/30333/p2p/QmcgpsyWgH8Y8ajJz1Cu72KnS5uo2Aa2LpzU7kinSupNKC"]
right: ["/ip6/2001:db8::/tcp/30333/p2p/12D3KooWNpFd1iMHjm3Q5gqQs4uJufVTDM7GY4aMcFueTWafHkyt"]

fails with #3885

@sandreim sandreim added the T10-tests This PR/Issue is related to tests. label Mar 29, 2024
@sandreim
Copy link
Contributor Author

CC @dmitry-markin @alexggh

@alindima
Copy link
Contributor

CC @lexnv, I think you recently had some changes to authority-discovery: 5ac32ee

@dmitry-markin
Copy link
Contributor

Most likely the test started failing after my PR #3757. I don't know how it sneaked in, probably because it was disabled in the CI.

#3757 changes the logic so that the /p2p/... protocol of the multiaddress is always overwritten with the local PeerID. In the test in question we put some arbitrary PeerID into external address, and of course, it's replaced with the correct PeerID.

I don't think the test makes sense in it's current form. I will create a PR to fix/remove the test and, additionally, to generate an error if it happens that the external address reported by the network backend contains PeerId different to the one of the node (with the libp2p network backend external addresses do not contain any /p2p/... components).

@sandreim
Copy link
Contributor Author

We should be able to merge #3885 once your fix is merged in master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T10-tests This PR/Issue is related to tests.
Projects
None yet
3 participants