feat: implement post-state-root chunk production #9537
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pugachAG
changed the title
pugachAG
force-pushed
the
post-state-root-produce-block
branch
3 times, most recently
from
0a7edd0 to
5315641
Compare
pugachAG
force-pushed
the
post-state-root-produce-block
branch
4 times, most recently
from
3d95429 to
d15674a
Compare
pugachAG
changed the title
pugachAG
force-pushed
the
post-state-root-produce-block
branch
3 times, most recently
from
5f4f692 to
596e025
Compare
pugachAG
force-pushed
the
post-state-root-produce-block
branch
from
596e025 to
8012638
Compare
pugachAG
requested review from
nikurt,
Longarithm and
robin-near
and removed request for
nikurt
Longarithm
approved these changes
Oct 2, 2023
chain/client/src/client.rs
Outdated
Comment on lines
1002
to
1013
| // Receipts proofs root is calculating here | ||
| // | ||
| // For each subset of incoming_receipts_into_shard_i_from_the_current_one | ||
| // we calculate hash here and save it | ||
| // and then hash all of them into a single receipts root | ||
| // | ||
| // We check validity in two ways: | ||
| // 1. someone who cares about shard will download all the receipts | ||
| // and checks that receipts_root equals to all receipts hashed | ||
| // 2. anyone who just asks for one's incoming receipts | ||
| // will receive a piece of incoming receipts only | ||
| // with merkle receipts proofs which can be checked locally |
There was a problem hiding this comment.
Looks like this comment belongs to calculate_receipts_root. Could you move it there and remove it from produce_pre_state_root_chunk as well? I'd appreciate if you prettify it as well.
There was a problem hiding this comment.
good point, I was too lazy to actually read it, so I just assumed that it corresponds to the piece of code below 😄
I've moved it as a rustdoc to calculate_receipts_root and tried to improve both formating and wording there, please take a look.
chain/chain/src/chain.rs
Outdated
Comment on lines
853
to
858
| // TODO(post-state-root): this misses outgoing receipts from the last block before | ||
| // the switch to post-state-root. Incoming receipts for that block corresponds to the | ||
| // outgoing receipts from the previous block, but incoming receipts for the next block | ||
| // include outgoing receipts for that block. These receipts can be obtained from the db | ||
| // using get_outgoing_receipts_for_shard since we currently track all shard. This will | ||
| // be implemented later along with an intergation test to reproduce the issue. |
There was a problem hiding this comment.
Even though you explained it to me previously, it was hard to understand this by the comment. I came up with some reformulation which worked better for me - WDYT?
Suggested change
| // TODO(post-state-root): this misses outgoing receipts from the last block before | |
| // the switch to post-state-root. Incoming receipts for that block corresponds to the | |
| // outgoing receipts from the previous block, but incoming receipts for the next block | |
| // include outgoing receipts for that block. These receipts can be obtained from the db | |
| // using get_outgoing_receipts_for_shard since we currently track all shard. This will | |
| // be implemented later along with an intergation test to reproduce the issue. | |
| // TODO(post-state-root): this is NOT correct for chunks ONLY for the first block B after | |
| // switching to post-state-root. In general case incoming receipts for post-state-root | |
| // chunks are derived from outgoing receipts for chunks in the previous block. However, | |
| // the previous block for B has only incoming receipts, as required by pre-state-root | |
| // logic. | |
| // To get incoming receipts in this edge case we can call get_outgoing_receipts_for_shard | |
| // which obtains receipts from DB directly. This is enough because currently each node | |
| // tracks all shards. | |
| // This will be implemented later along with an integration test to reproduce the issue. |
There was a problem hiding this comment.
thanks for the suggestion, I've tried to make it more detailed, please let me know if that looks more understandable
There was a problem hiding this comment.
We had an offline chat. Got ideas to
- point out that we talk about
DBCol::IncomingReceiptspopulation and remind its definition which is ~ receipts TO execute which node received at the block B. So, for pre-state-root, when we receive block B, it gives us outgoing receipts from B-1 ("prev"). For post-state-root, chunk at block B stores outgoing receipts caused by itself, so they go to B as well. - reverse direction of arrow :)
pugachAG
force-pushed
the
post-state-root-produce-block
branch
5 times, most recently
from
ec2280f to
34ba52f
Compare
pugachAG
force-pushed
the
post-state-root-produce-block
branch
from
34ba52f to
039527d
Compare
nikurt
pushed a commit
that referenced
this pull request
Oct 11, 2023
This PR adds chunk production with post-state-root. This means applying transactions and receipts before producing the chunk and including the necessary fields in the header. The code is not used anywhere yet, that will be implemented as a separate PR. The issues marked with `TODO(post-state-root)` will be resolved separately. Part of #9450.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds chunk production with post-state-root. This means applying transactions and receipts before producing the chunk and including the necessary fields in the header.
The code is not used anywhere yet, that will be implemented as a separate PR. The issues marked with
TODO(post-state-root)will be resolved separately.Part of #9450.