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

Move and refactor the PutBlock method #196

Closed
evan-forbes opened this issue Mar 10, 2021 · 11 comments
Closed

Move and refactor the PutBlock method #196

evan-forbes opened this issue Mar 10, 2021 · 11 comments
Assignees
Labels
C:dht DHT and p2p related issues (IPFS mostly)

Comments

@evan-forbes
Copy link
Member

evan-forbes commented Mar 10, 2021

The PutBlock method was introduced as a method to Block in #178. Having PutBlock where it is now adds network dependencies to the types package, which is undesirable. The original thinking in doing so was so that erasured data could be cached instead of recomputed. While caching the erasured data is still a good idea, it was not implemented in #178 and does not necessarily require PutBlock being a method to Block.

We should probably make PutBlock a function and move it to a different package. This will require a general refactor, which includes exporting the utility function shared by PutBlock and fillDataAvailabilityHeader.

As for where to move it, per a suggestion by @liamsi, I think we should move PutBlock and it's related code to lazyledger-core/p2p/ipld.

ref: #178 (comment)

@liamsi liamsi added the C:dht DHT and p2p related issues (IPFS mostly) label Mar 10, 2021
@evan-forbes
Copy link
Member Author

relevant comment about the original API defined in #170 #178 (comment)

While PutLeaves was not implemented in #178, I think it should be added when we move PutBlock

@evan-forbes
Copy link
Member Author

this code will also have to be moved and consolidated with PutBlock

@evan-forbes evan-forbes changed the title Move the PutBlock method to a different package Move and refactor the PutBlock method Mar 15, 2021
@evan-forbes evan-forbes self-assigned this Mar 15, 2021
@evan-forbes
Copy link
Member Author

I'm linking this comment to remind myself to also include the suggested change into the refactor/move of PutBlock

@evan-forbes
Copy link
Member Author

Another self reminder, wrap this error

@evan-forbes
Copy link
Member Author

evan-forbes commented Apr 30, 2021

More notes:
As pointed out by @Wondertan, it's intuitive to think that calling PutBlock would also fill the data availability header, but this is not the case. In the implementation, filling the DAH and saving the block data to IPFS should be separate, but perhaps PutBlock could could also do this if not done already.

Somewhat unrelated, in #304, we're moving the ipfs API object to the block store in order to retrieve block data, and maybe we can move the PutBlock method their too.

@evan-forbes
Copy link
Member Author

I think we can now close this issue now that #334 is merged. Any other opinions?

@Wondertan
Copy link
Member

I think we can close this once PutBlock starts to return DAHeader

@evan-forbes
Copy link
Member Author

evan-forbes commented May 27, 2021

So we just need to fill the DAH of the passed in *Block if not done so already? Is the goal of this to not fill the DAH twice? If so, should we also disable filling the DAH when filling the Header?

@Wondertan
Copy link
Member

Wondertan commented May 27, 2021

So we just need to fill the DAH of the passed in *Block if not done so already?

Ideally, I would change the interface to receive just types.Data bc that's the only thing used in PutBlock.

Is the goal of this to not fill the DAH twice?

Yeah, not to fill and recompute it twice

If so, should we also disable filling the DAH when filling the Header?

Idk yet

@liamsi liamsi mentioned this issue Jun 2, 2021
17 tasks
@Wondertan
Copy link
Member

Wondertan commented Jun 23, 2021

@evan-forbes, @liamsi is it closed?

@liamsi
Copy link
Member

liamsi commented Jun 23, 2021

Yes, thanks @Wondertan 🙏🏼 I'm sure we can further improve the package structure a bit (similar to changes you started in #427) but the suggestions made here are already implemented.

@liamsi liamsi closed this as completed Jun 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:dht DHT and p2p related issues (IPFS mostly)
Projects
None yet
Development

No branches or pull requests

3 participants