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

test: allow producing block on top of arbitrary prev block #10093

Merged
merged 9 commits into from
Nov 6, 2023

Conversation

Longarithm
Copy link
Member

For stateless validation, chunk execution will be delayed until next block is processed: #9982. This impacts several tests assuming that block processing includes chunk processing as well. For these tests, we need to produce and process one more block to get execution results like ChunkExtra and ExecutionOutcome.

To allow production of longer forks, I want to extend client API by produce_block_on which can produce a block not just on top of head, but on top of any existing block. As this block isn't immediately saved or processed, it even doesn't break any guarantees.

Testing

Impacted tests should still pass. Later stateless validation PRs will rely on it.

@Longarithm Longarithm requested a review from a team as a code owner November 3, 2023 14:08
@@ -3044,6 +3049,12 @@ fn test_fork_receipt_ids() {
env.clients[0].process_block_test(block.clone().into(), Provenance::NONE).unwrap();
}

// Ensure that in stateless validation protocol receipts in fork blocks are executed.
let b3 = env.clients[0].produce_block_on(last_height + 3, *block1.hash()).unwrap().unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: block3 and block4 since that's how 1 and 2 are, for uniformity?

@wacban
Copy link
Contributor

wacban commented Nov 6, 2023

Ah, finally, near gets proper support for forks, a feature long awaited by the community :)

Copy link
Contributor

@wacban wacban left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -477,20 +477,19 @@ impl Client {

fn should_reschedule_block(
Copy link
Contributor

Choose a reason for hiding this comment

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

While you're at it, can you add some comments please?

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do in following PR.

Comment on lines +585 to +586
height: BlockHeight,
prev_hash: CryptoHash,
Copy link
Contributor

Choose a reason for hiding this comment

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

So just for my understanding, height and prev_hash are not tied together are they? As in height can be arbitrary number as long as it is greater than the height of the block with hash prev_hash.

Copy link
Member Author

Choose a reason for hiding this comment

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

Depends on what you mean by "tied together". Yeah, height is not necessarily height(prev_hash)+1 anymore, but prev_hash is still hash of previous block for block being produced.

@Longarithm
Copy link
Member Author

To be completely clear, after that clients still will not produce blocks for arbitrary heights during normal production behaviour. It is test-only feature.

@Longarithm Longarithm added this pull request to the merge queue Nov 6, 2023
Merged via the queue into near:master with commit a26635a Nov 6, 2023
20 of 21 checks passed
@Longarithm Longarithm deleted the produce-block-on branch November 6, 2023 12:21
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.

3 participants