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

Lookahead: Remove filling of unincluded segment #6075

Closed
wants to merge 3 commits into from

Conversation

skunert
Copy link
Contributor

@skunert skunert commented Oct 15, 2024

The lookahead collator was filling up the unincluded segment by producing one extra block every time when there is space in the unincluded segment.

This increases time to finality for given transactions and the benefit is questionable. In addition, if we use the full execution time of 2s for async-backing parachains and there is a relay chain fork, we will be producing blocks for 8s, which means that we are producing past our slot.

Alternatively, we could introduce a configuration to make users choose this value, however that makes this PR non-backporteable.

cc @bkchr @eskimor

@skunert skunert added T0-node This PR/Issue is related to the topic “node”. T9-cumulus This PR/Issue is related to cumulus. labels Oct 15, 2024
@skunert
Copy link
Contributor Author

skunert commented Oct 15, 2024

/cmd prdoc --audience node_dev

@alexggh
Copy link
Contributor

alexggh commented Oct 15, 2024

This increases time to finality for given transactions and the benefit is questionable.

Thinking about this, in the current implementation because we produce from 0..2 we end-up at the first try with 2 parachains blocks ahead.

With this PR, we can easily end up with 2 blocks ahead, if we missed 2 opportunities of backing the parachain blocks on the relay-chain. Because there is still space in the unincluded segment the collators will produce the blocks ahead and after that the steady state will be as before this PR.

Unfortunately, missing two backing opportunities is probably something will happen over the course of a few hours/days, so I think the parachain will quickly converge to the functioning mode from before this PR.

Comment on lines -360 to -362
// This needs to change to support elastic scaling, but for continuously
// scheduled chains this ensures that the backlog will grow steadily.
for n_built in 0..2 {
Copy link
Member

Choose a reason for hiding this comment

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

What we actually want is that we in total are only always produce two blocks on top of the last backed block. What currently can happen if we missed to back a block, we put another block (assuming unincluded segment len of 3) on top. So, if everything works as expected, we will need the unincluded segment length of 3, but if a block failed to be backed we should go back to 2 from the included block.

@skunert
Copy link
Contributor Author

skunert commented Oct 16, 2024

After reading the comments and thinking about it some more, I come to the conclusion that my initial assumption was wrong. While its not super useful to build this extra blocks, we also don't really gain anything by the proposed change. Will close here.

@skunert skunert closed this Oct 16, 2024
@alexggh
Copy link
Contributor

alexggh commented Oct 17, 2024

After reading the comments and thinking about it some more, I come to the conclusion that my initial assumption was wrong. While its not super useful to build this extra blocks, we also don't really gain anything by the proposed change. Will close here.

So right now, if PB(x) is backed at RB(n), then PB(x) will be created when RB(n-2) is imported, which is full 2 blocks(12s) before it will be backed, we can't reduce unincluded segment to 2 because then it will be created at the begining of RB(n-1) if authoring takes 2s you don't have time to also run the backing protocol, but ....

What if in this loop we do something like sleep(2*seconds * current_unincluded_blocks)), before we create parachain blocks ? In that case we move the creation of PB(x) at RB(n-2) + 4s which is really close to the ideal time you want to start creating the block PB(x) if you want it to be backed at RB(n).

@skunert
Copy link
Contributor Author

skunert commented Oct 17, 2024

What if in this loop we do something like sleep(2*seconds * current_unincluded_blocks)), before we create parachain blocks ? In that case we move the creation of PB(x) at RB(n-2) + 4s which is really close to the ideal time you want to start creating the block PB(x) if you want it to be backed at RB(n).

We could try to improve the timings here a bit, but it is a bit tricky. The building happens in the "main loop" and we might need to build parachain blocks for multiple relay chain forks, each taking 2s. So that needs to be factored in, would become quite finicky.
Next step for the slot based collator is to make it more flexible and extract a signaling task, this will make it much easier to test different timing models, makes more sense to me: #6066

@bkchr bkchr deleted the skunert/lookahead-remove-segment-filling branch October 17, 2024 08:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T0-node This PR/Issue is related to the topic “node”. T9-cumulus This PR/Issue is related to cumulus.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants