Use upstream version of multihash instead of a fork #1472
Conversation
vmx
force-pushed
the
multihash-upstream
branch
2 times, most recently
from
88033dd
to
ebfbfa7
Compare
|
Is there anything I can help with running it against a live system or is this something the Rust libp2p team will take care of? |
If you're motivated, my idea was to run Polkadot with a local override to this PR, and see if nothing suspicious happens. The most difficult part is waiting for the compilation to finish. |
|
Can I opt-out again :) I thought there might be something automated, you'd need to ping people for. I've never used Polkadot, so I guess that isn't a good task for me as I really can't tell if there's anything suspicious. |
|
Looks like it's working. For note-keeping, here's the small patch to apply to Substrate: diff --git a/client/authority-discovery/src/lib.rs b/client/authority-discovery/src/lib.rs
index 6260ac9a8..ff88765cb 100644
--- a/client/authority-discovery/src/lib.rs
+++ b/client/authority-discovery/src/lib.rs
@@ -499,9 +499,8 @@ where
}
fn hash_authority_id(id: &[u8]) -> Result<libp2p::kad::record::Key> {
- libp2p::multihash::encode(libp2p::multihash::Hash::SHA2256, id)
- .map(|k| libp2p::kad::record::Key::new(&k))
- .map_err(Error::HashingAuthorityId)
+ let digest = libp2p::multihash::Sha2_256::digest(id);
+ Ok(libp2p::kad::record::Key::new(&digest))
}
fn interval_at(start: Instant, duration: Duration) -> Interval { |
core/src/peer_id.rs
Outdated
| //}; | ||
|
|
||
| let canonical = canonical_algorithm.map(|alg| | ||
| multihash::encode(alg, &key_enc).expect("SHA2256 is always supported")); | ||
| alg.hasher().expect("canonical hasher exists").digest(&key_enc)); |
There was a problem hiding this comment.
I would suggest to keep mentioning "SHA2-256". It is more informative than "canonical".
There was a problem hiding this comment.
If the canonical hasher is always sha2-256, should perhaps then the variable also be renamed?
There was a problem hiding this comment.
"canonical" as a variable name sounds good to me so I would leave it as is. After all SHA2-256 is our canonical hash algorithm.
core/src/peer_id.rs
Outdated
|
|
||
| let multihash = multihash::encode(hash_algorithm, &key_enc) | ||
| .expect("identity and sha2-256 are always supported by known public key types"); | ||
| let multihash = hash_algorithm.hasher().expect("hasher exists").digest(&key_enc); |
There was a problem hiding this comment.
@twittner should I also change this message back to "identity and sha2-256 are always supported by known public key types"?
There was a problem hiding this comment.
I would go with "identity and sha2-256 are always supported".
The changes from the libp2p fork have been backported to upstream, hence upstream can now be used instead.
|
I have just rebased an changed the error messages in c976930#diff-d21ea1befb9128247a52e731c8fd3256R79-R83 as suggested by the code review. There were no other changes. |
|
The integration test also fails for me locally on master. Update: locally on master it was a different error. I'm trying again to reproduce it. |
|
I've no idea what the problem is, the integration test works locally. |
|
The integration test failure is obviously not caused by this PR, and we also had it in the other PR. |
The changes from the libp2p fork have been backported to upstream, hence
upstream can now be used instead.