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

sc-client: expose pinning API #1219

Closed
wants to merge 1 commit into from
Closed

Conversation

ordian
Copy link
Member

@ordian ordian commented Aug 28, 2023

Required for #623.

I've considered another design with RAAI similar to lock_block, but opted for a lower-level albeit less safe API. The reason is that the caller can provide a safer abstraction on top (see the follow-up PR).

@ordian ordian added R0-silent Changes should not be mentioned in any release notes T0-node This PR/Issue is related to the topic “node”. T8-parachains_engineering labels Aug 28, 2023
Copy link
Member

@eskimor eskimor left a comment

Choose a reason for hiding this comment

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

Awesome, thanks!

@eskimor
Copy link
Member

eskimor commented Aug 28, 2023

With regards to the safe abstraction, I would strongly consider to also provide it alongside and recommend its usage or something similar - otherwise people will have to implement it themselves or more likely - they don't use safe abstraction.

Personally I would even prefer to only expose a safe API.

EDIT: What happens if a blocks are never unpined?

@bkchr
Copy link
Member

bkchr commented Aug 28, 2023

/// Consume this notification and extract the unpin handle.
///
/// Note: Only use this if you want to keep the block pinned in the backend.
pub fn into_unpin_handle(self) -> UnpinHandle<Block> {
self.unpin_handle
}
this what you want to use to keep the blocks alive. There is no guarantee that the block is still valid when you try to call pin_block.

@ordian
Copy link
Member Author

ordian commented Aug 28, 2023

@bkchr

  • are we guaranteed to receive BlockImportNotification for every block? I think not, which is why we have this function determine_new_blocks
  • we want to pin only inclusion blocks with f+1 against votes, this happens much later after import, but before the dispute concludes and they are pruned

@ordian
Copy link
Member Author

ordian commented Aug 28, 2023

What happens if a blocks are never unpined?

Substrate has its own LruMap and will unpin it eventually. But you can see ChainAPI API in #1220 prevents that from happening.

@bkchr
Copy link
Member

bkchr commented Aug 28, 2023

  • we want to pin only inclusion blocks with f+1 against votes, this happens much later after import, but before the dispute concludes and they are pruned

Fair point, but then just forward the backend. No need to blow up the client even more.

@ordian
Copy link
Member Author

ordian commented Aug 31, 2023

Closing this in favor of using UnpinHandle and #1220 (comment)

@ordian ordian closed this Aug 31, 2023
@ordian ordian deleted the ao-expose-client-pinning-api branch August 31, 2023 10:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
R0-silent Changes should not be mentioned in any release notes T0-node This PR/Issue is related to the topic “node”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants