[IPFS/Bitswap] Improvement Suggestions

[bkontur]

Summary

Maybe it’s not worth refactoring right now, but the main idea is to make BitswapServer more generic so it can work with mechanisms other than transaction_index and pallet-transaction-storage (for example, the future JAM DA support?). Although, perhaps the intended direction is that anything you want to propagate or expose to IPFS/Bitswap must go into transaction_index.

In any case, I’m sure we can at least improve the documentation and the handling of the content_hash type.

cc: @dmitry-markin @lexnv

Context

While working on adding support for multiple hashing algorithms for Bitswap (#10416) to improve compatibility with Bulletin (paritytech/polkadot-bulletin-chain#100), I identified a couple of areas that could be improved.

Essentially, the [PR](#10468) touches most of the necessary changes.

Suggestions

Clean the content_hash usage from Block::Hash to [u8; 32]

1. Trait/host function TransactionIndex uses context_hash: [u8; 32] here.
2. enum IndexOperation uses hash as hash: Vec<u8>,
3. db/backend stores it as pub type DbHash = sp_core::H256; (converted from IndexOperation.hash: Vec<u8> -> potential/hypothetical bug when H256::from_slice could take just last 32-byte (?!))
4. client api (rpc, runtime api) uses Block::Hash (which is the trait HashOutput)

The result is that BitswapServer server uses this hack/workaround:

let mut hash = Block::Hash::default();
hash.as_mut().copy_from_slice(&cid.hash().digest()[0..32]);
let transaction = match self.client.indexed_transaction(hash) {

We want to support multiple hashing algorithms with IPFS (sha2, keccak, blake2b, etc.), so using Block::Hash::default() feels odd — especially since our blocks typically use BlakeTwo256, also the BitswapServer does not need to know the notion of "Block", it just cares about content_hash and return bytes.

From this perspective, it might make sense to introduce a type alias such as pub type ContentHash = sp_core::H256 (if we want to stick to the 32-byte hashes), or perhaps make it generic using something like HashOutput - something like struct BitswapServer<Block: BlockT> -> struct BitswapServer<ContentHash: HashOutput> ?

Extract trait for indexed_transaction / has_indexed_transaction

We have there functions in the trait BlockBackend

// hash -> content hash of indexed tx
fn indexed_transaction(&self, hash: Block::Hash)
// hash -> content hash of indexed tx
fn has_indexed_transaction(&self, hash: Block::Hash)
// hash -> block hash
fn block_indexed_body(&self, hash: Block::Hash)

At the minimum, we should update/align docs to avoid confusion what hash is block hash and what content hash. We should consider also extracting indexed_transaction/has_indexed_transaction to separate trait (ContentIndexProvider (?!)) and implement tuple feature for this trait (to play also with block_indexed_body, which reads the tx index). This way we could easily support more "transaction indexes" or more content_hash indexes, for example this could potentially be useful when going for JAM, where we could have something like this or HOP protocol

let content_provider_client: ContentIndexProvider = (
   TransactionIndexProvider, // actual support for `transaction_index` for `pallet-transaction-storage`
   JamDaContentProvider,
   HopProtocolProvider,
)

// Actual Bitswap handler which uses 
struct BitswapServer(client: ContentIndexProvider) {
...
     let transaction = match self.client.indexed_transaction(hash)
...
}