-
Notifications
You must be signed in to change notification settings - Fork 700
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
chainHead/follow: Provide multiple block hashes to the initialized event #3445
Conversation
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
) -> Result<InitialBlocks<Block>, SubscriptionManagementError> { | ||
let blockchain = self.backend.blockchain(); | ||
let leaves = blockchain.leaves()?; | ||
let finalized = startup_point.finalized_hash; | ||
let mut pruned_forks = HashSet::new(); | ||
let mut finalized_block_descendants = Vec::new(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a comment related to this PR, but could we do some kind of sanity check of the distance between leaf and finalized block? This code assumes that they are close together, as they should be. However, on parachains for example, the finalization of para blocks is dependent on the relay chain.
If the embedded relay chain node syncs slower while the parachain node is already at the tip, between a leaf and the known finalized block could be millions of blocks (until relay chain has caught up).
This would lead to:
- The tree route being very costly
- Us delivering 1 million blocks via RPC
Maybe we should add some kind of safeguard against such situations.
Certainly an edge case however.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have create this PR to handle that edge-case: #3562 🙏
substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs
Outdated
Show resolved
Hide resolved
@@ -202,13 +212,40 @@ where | |||
|
|||
for pair in blocks.zip(parents) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated to this PR but I don't understand the logic with zip
here.
Parents = blocks.len() + 1
Then the zip
will ignore the last item in blocks is that intended?
Example what I mean:
let mut a = vec![1, 2];
let mut tmp = vec![3, 4];
let b = std::iter::once(99).chain(tmp);
let res: Vec<(usize, usize)> = a.into_iter().zip(b).collect();
assert_eq!(res, vec![(1, 99), (2, 3)]);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, that makes sense! Indeed we are ignoring the last element of the parents
, but that's on purpose since parents = [finalized, blocks[..]]
We are interested in grouping every block reported by blocks
with its parent.
For example:
let blocks = vec!["A", "B", "C"];
let parents = vec!["gensiss", "A", "B", "C"];
for pair in blocks.iter().zip(parents.iter()) {
println!("block={} parent={}", pair.0, pair.1);
}
This should produce the following result:
block=A parent=gensiss
block=B parent=A
block=C parent=B
Let me have a go at this to remove the zip
to make the code easier to read 🙏
substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs
Outdated
Show resolved
Hide resolved
substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs
Outdated
Show resolved
Hide resolved
Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>
Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>
Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>
substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs
Outdated
Show resolved
Hide resolved
The CI pipeline was cancelled due to failure one of the required jobs. |
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
…ent (#3445) This PR extends the Initialized event of the chainHead_follow subscription. Now, the event provides multiple finalized block hashes. This information allows clients that are disconnected, and that want to reconnect, to not lose information about the state of the chain. At the moment, the spec encourages servers to provide at least 1 minute of finalized blocks (~10 blocks). The users are responsible for unpinning these blocks at a later time. This PR tries to report at least 1 finalized block and at most 16 blocks, if they are available. Closes: #3432 cc @paritytech/subxt-team --------- Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>
Removed from queue because it was stuck on flaky test and would run into timeout. |
I am dubious if this actually fixes the problem. It is more of a hail marry. |
…ent (paritytech#3445) This PR extends the Initialized event of the chainHead_follow subscription. Now, the event provides multiple finalized block hashes. This information allows clients that are disconnected, and that want to reconnect, to not lose information about the state of the chain. At the moment, the spec encourages servers to provide at least 1 minute of finalized blocks (~10 blocks). The users are responsible for unpinning these blocks at a later time. This PR tries to report at least 1 finalized block and at most 16 blocks, if they are available. Closes: paritytech#3432 cc @paritytech/subxt-team --------- Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>
#3562) This PR ensure that the distance between any leaf and the finalized block is within a reasonable distance. For a new subscription, the chainHead has to provide all blocks between the leaves of the chain and the finalized block. When the distance between a leaf and the finalized block is large: - The tree route is costly to compute - We could deliver an unbounded number of blocks (potentially millions) (For more details see #3445 (comment)) The configuration of the ChainHead is extended with: - suspend on lagging distance: When the distance between any leaf and the finalized block is greater than this number, the subscriptions are suspended for a given duration. - All active subscriptions are terminated with the `Stop` event, all blocks are unpinned and data discarded. - For incoming subscriptions, until the suspended period expires the subscriptions will immediately receive the `Stop` event. - Defaults to 128 blocks - suspended duration: The amount of time for which subscriptions are suspended - Defaults to 30 seconds cc @paritytech/subxt-team --------- Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> Co-authored-by: Sebastian Kunert <skunert49@gmail.com>
#3562) This PR ensure that the distance between any leaf and the finalized block is within a reasonable distance. For a new subscription, the chainHead has to provide all blocks between the leaves of the chain and the finalized block. When the distance between a leaf and the finalized block is large: - The tree route is costly to compute - We could deliver an unbounded number of blocks (potentially millions) (For more details see #3445 (comment)) The configuration of the ChainHead is extended with: - suspend on lagging distance: When the distance between any leaf and the finalized block is greater than this number, the subscriptions are suspended for a given duration. - All active subscriptions are terminated with the `Stop` event, all blocks are unpinned and data discarded. - For incoming subscriptions, until the suspended period expires the subscriptions will immediately receive the `Stop` event. - Defaults to 128 blocks - suspended duration: The amount of time for which subscriptions are suspended - Defaults to 30 seconds cc @paritytech/subxt-team --------- Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> Co-authored-by: Sebastian Kunert <skunert49@gmail.com>
paritytech#3562) This PR ensure that the distance between any leaf and the finalized block is within a reasonable distance. For a new subscription, the chainHead has to provide all blocks between the leaves of the chain and the finalized block. When the distance between a leaf and the finalized block is large: - The tree route is costly to compute - We could deliver an unbounded number of blocks (potentially millions) (For more details see paritytech#3445 (comment)) The configuration of the ChainHead is extended with: - suspend on lagging distance: When the distance between any leaf and the finalized block is greater than this number, the subscriptions are suspended for a given duration. - All active subscriptions are terminated with the `Stop` event, all blocks are unpinned and data discarded. - For incoming subscriptions, until the suspended period expires the subscriptions will immediately receive the `Stop` event. - Defaults to 128 blocks - suspended duration: The amount of time for which subscriptions are suspended - Defaults to 30 seconds cc @paritytech/subxt-team --------- Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> Co-authored-by: Sebastian Kunert <skunert49@gmail.com>
This PR extends the Initialized event of the chainHead_follow subscription.
Now, the event provides multiple finalized block hashes. This information allows clients that are disconnected, and that want to reconnect, to not lose information about the state of the chain.
At the moment, the spec encourages servers to provide at least 1 minute of finalized blocks (~10 blocks). The users are responsible for unpinning these blocks at a later time. This PR tries to report at least 1 finalized block and at most 16 blocks, if they are available.
Closes: #3432
cc @paritytech/subxt-team