-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Publish indexed transactions with BLAKE2b hashes to IPFS DHT #10468
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
base: master
Are you sure you want to change the base?
Conversation
|
/cmd prdoc --audience node_dev --bump major |
…e_dev --bump major'
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.
This file is taken from https://github.com/zdave-parity/substrate/blob/polkadot-bulletin-chain/client/network/src/ipfs/block_provider.rs The code is checked for correctness, but still should be reviewed.
Big thanks goes to @zdave-parity for solid implementation!
| } | ||
|
|
||
| fn indexed_transaction(&self, _: Block::Hash) -> sp_blockchain::Result<Option<Vec<u8>>> { | ||
| fn indexed_transaction(&self, _: H256) -> sp_blockchain::Result<Option<Vec<u8>>> { |
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.
dq: Does the Block::Hash resolve to something different here? Or we should update the other methods of the trait?
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.
Block::Hash can be any hash internally, including hashes of different size. H256 is a fixed-length 32-byte hash, in this case BLAKE2b-256.
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.
I am glad that you touched on these topics. Please check my thoughts from the previous issue, “IPFS/Bitswap CID filtering” #10538 — one of them is exactly about this hash/block_hash/content_hash question.
| /// | ||
| /// Note that this will only fetch transactions that are indexed by the runtime with | ||
| /// `storage_index_transaction`. | ||
| fn block_indexed_hashes(&self, hash: Block::Hash) -> sp_blockchain::Result<Option<Vec<H256>>>; |
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.
dq: Should we keep generic over the returned values (ie replace H256 with BlockT::Hash)? (I do like the h256 better, asking for consistency)
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.
This is also a BLAKE2b-256 and not a block hash.
| &self, | ||
| hash: Block::Hash, | ||
| ) -> ClientResult<Option<impl Iterator<Item = DbHash>>> { | ||
| let body = match read_db( |
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.
nit: let Some(body) = read_db else { return Ok(None)};
| "Missing indexed transaction {hash:?}", | ||
| ))), | ||
| }) | ||
| .collect::<Result<_, _>>()?, |
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.
nit: The Result type is needed by the compiler because of adding the sp_blockchain::Error?
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.
Result is needed to resolve the first Err yielded by the iterator as an Err of entire sequence.
|
|
||
| /// Logging target for the file. | ||
| const LOG_TARGET: &str = "sub-libp2p::bitswap"; | ||
| const LOG_TARGET: &str = "sub-libp2p::ipfs::bitswap"; |
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.
nit: I would keep sub-libp2p::bitswap since grafana does support only 1 subtarget
|
|
||
| loop { | ||
| tokio::select! { | ||
| change = changes.next(), if inflight_queries.len() < MAX_INFLIGHT_QUERIES => { |
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.
nit: We could add tiny handler methods for these code blocks. Currently code under tokio::select! is not formatted:
change = changes.next() => {
self.handle_change(change: Change)
}
fn handle_change(self, change: Change) {
if inflight_queries.len() >= MAX { return; }
}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.
This is not the same, because , if ... in tokio::select! pauses polling the stream and not just discards the Change. But yeah, I will look into formatting the select! in a nicer way.
| match change { | ||
| None => { | ||
| debug!(target: LOG_TARGET, "BlockProvider terminated, terminating IpfsDht"); | ||
| return |
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.
nit: Should we exit sooner here regardless of if inflight_queries.len() < MAX_INFLIGHT_QUERIES?
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.
It will delay the exit (requiring the query slot to empty), but in the end this is abnormal termination and won't make a big difference of when we exit.
| continue; | ||
| } | ||
|
|
||
| random_walk_query = Some(self.kademlia_handle.find_node(PeerId::random()).await); |
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.
Is 1 query at a time sufficient to discover the network? I believe this is how libp2p works, but IRRC litep2p does quite a few more
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.
In practice it works even without random walks :)
| if Some(query_id) == random_walk_query { | ||
| trace!(target: LOG_TARGET, "DHT random walk failed"); | ||
|
|
||
| random_walk_query = None; |
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.
For the following example, we'll wait quite a bit without issuing a new query:
- T0: query submited
- T1 (10seconds later): query fails
- T2: (9 minutes 50 seconds later): we resubmit the query
Could we initiate the random walk on this failure instead? This way we always have an inflight query until we can find peers
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.
I can think only about one situation when query fails — no network connectivity. In other cases it is enough for one peer to respond to FIND_NODE for query to yield some peers. In case of no network connectivity initiating the query instantly on failure will lead to busy looping. And, in the end, it is not that important that we "miss" the random walk, as it is not super important (we can do these random walks once per hour with likely the same outcome).
| /// Enable publishing of indexed transactions to IPFS. | ||
| pub ipfs_server: bool, | ||
|
|
||
| /// List of IPFS bootstrap nodes. |
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.
nit: We could mention that only direct requests will be handled here if no bootnods are provided
| // nothing to do. | ||
| debug_assert!(this.fetched_to <= this.finalized_to); | ||
| if this.fetched_to == this.finalized_to { | ||
| return Poll::Pending |
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.
Since we are not saving the waker, we rely entirely on this.finality_notifications.poll_next_unpin(cx) to wake us up, is that intended?
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.
Yes, this is fine — we come here only if finality_notifications yielded Poll::Pending, so it will wake us up.
lexnv
left a comment
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.
LGTM! 🙏
Add
--ipfs-bootnodesflag for specifying IPFS bootnodes. If passed along with--ipfs-server, the node will register as a content provider in IPFS DHT of indexed transactions with BLAKE2b hashes of the last two weeks (or pruning depth if smaller).