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

Replace BlockID with the HeaderHash #287

Closed
wants to merge 2 commits into from
Closed

Conversation

evan-forbes
Copy link
Member

@evan-forbes evan-forbes commented Apr 20, 2021

Description

This draft PR is starting the process of using only the header hash instead of BlockID, along with a few necessary changes that also closer adhere to the spec. As expected, everything is breaking, but I think I'm ~1/2 done to getting it to compile.

Closes: #184

@evan-forbes evan-forbes self-assigned this Apr 20, 2021
@evan-forbes
Copy link
Member Author

@liamsi I wasn't quite sure what to do with this block retrieving logic in the store package. I'm guessing that we're replacing a portion of this with RetrieveBlockData, but what about the rest?

@liamsi
Copy link
Member

liamsi commented Apr 20, 2021

I'm guessing that we're replacing a portion of this with RetrieveBlockData, but what about the rest?

Yeah, you are right. Most places like these and others will be replaced by RetrieveBlockData. That was the main reason @marbar3778's store PR #218 (#182) was blocked (as RetrieveBlockData didn't exist when he started it). After Marko finishes that PR the tendermint store won't even store the parts requested in the portion you've linked.
That said, for some nodes calling RetrieveBlock will be very similar to the portion you've linked: they will request shares (instead of Parts) from their local ipld store (because e.g. they already downloaded it during consensus), for other nodes it means requesting the block from the network. The latter might not always be feasible (as the latency might be too large) or not desired in all circumstances. There is no general answer to this: we will have to look and understand each such place and decide to either remove it, replace it with only returning the header (where possible), or, Header+state transition relevant Txs (not Messages), or, actually call RetrieveBlockData (and potentially request it from the network).

In some cases it might be worth to rethink the current logic, e.g. for some RPC endpoints (see #195).
To move forward quickly we can simply replace all such occurrences with RetrieveBlockData and revisit each place in a separate PR (or all places together in one PR).

@evan-forbes
Copy link
Member Author

evan-forbes commented Apr 21, 2021

In some cases it might be worth to rethink the current logic, e.g. for some RPC endpoints (see #195).
To move forward quickly we can simply replace all such occurrences with RetrieveBlockData and revisit each place in a separate PR (or all places together in one PR).

@liamsi I think I can split this into a separate PR, will we need #218 to do this properly?

@liamsi
Copy link
Member

liamsi commented Apr 21, 2021

will we need #218 to do this properly?

I don't think so. Without #218 it would just mean that we are storing the block data twice: once in the IPFS data store and another time in the tendermint store.

@liamsi
Copy link
Member

liamsi commented Aug 17, 2021

Closing as this is no longer required.

@liamsi liamsi closed this Aug 17, 2021
@evan-forbes evan-forbes deleted the evan/replace-block-id branch September 2, 2022 04:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BlockID: replace with header hash + add/use DA header instead of PartsSetHeader
2 participants