Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Storage chains: serve transactions over IPFS/bitswap #7963

Merged
13 commits merged into from
Feb 3, 2021
Merged

Conversation

arkpar
Copy link
Member

@arkpar arkpar commented Jan 23, 2021

See #7962

Initially I've tried implementing this as request-response protocol, but found out it is not suitable due to paritytech/polkadot-sdk#542.

polkadot companion: paritytech/polkadot#2315

@arkpar arkpar added A0-please_review Pull request needs code review. B5-clientnoteworthy C1-low PR touches the given topic and has a low impact on builders. labels Jan 23, 2021
@arkpar arkpar mentioned this pull request Jan 23, 2021
7 tasks
@arkpar arkpar requested a review from tomaka January 26, 2021 17:08
@arkpar
Copy link
Member Author

arkpar commented Jan 26, 2021

@tomaka please review or reassign

Copy link
Contributor

@tomaka tomaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not familiar with the bitswap protocol, so I can't tell if the messages being exchanged are correct, but the code looks good to me!

client/network/src/bitswap.rs Outdated Show resolved Hide resolved
client/network/src/bitswap.rs Outdated Show resolved Hide resolved
client/network/src/bitswap.rs Outdated Show resolved Hide resolved
@arkpar
Copy link
Member Author

arkpar commented Jan 27, 2021

I've tested this against js-ipfs. jsipfs cat <hash> works at least

Copy link
Contributor

@cheme cheme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
I wonder a bit if big tx should be split in multiple block.

@@ -95,6 +95,17 @@ pub trait BlockBackend<Block: BlockT> {

/// Get block hash by number.
fn block_hash(&self, number: NumberFor<Block>) -> sp_blockchain::Result<Option<Block::Hash>>;

/// Get single extrinsic by hash.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we name the method transaction.
Also wondering if we should comment the fact that in some case do not have result depending on node configuration (in previous pr column 'TRANSACTION' was only optionally use).

&self,
_hash: &Block::Hash,
) -> sp_blockchain::Result<Option<<Block as BlockT>::Extrinsic>> {
unimplemented!()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will it be implemented later, if not maybe add a parameter message saying that it is unsupported.

client/network/src/behaviour.rs Show resolved Hide resolved
client/db/src/lib.rs Show resolved Hide resolved
client/network/src/bitswap.rs Show resolved Hide resolved
client/network/src/bitswap.rs Show resolved Hide resolved
@arkpar
Copy link
Member Author

arkpar commented Feb 3, 2021

bot merge

@ghost
Copy link

ghost commented Feb 3, 2021

Trying merge.

@ghost ghost merged commit 56c64cf into master Feb 3, 2021
@ghost ghost deleted the a-ipfs-server branch February 3, 2021 07:14
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants