Enable non-sequential block finalization. #21
Conversation
There was a problem hiding this comment.
I hate to sound like a broken record in every PR of this kind, but have you discussed this with upstream, is there an open discussion and/or PR there?
We need a generic solution here, not hacks on top of hacks. paritytech#5119 looks like a related discussion. IMO we should implement and submit an upstream fix for a situation like this.
| Ok(header) => header, | ||
| Err(_) => { | ||
| debug!("Can't find obtain header metatadata: {parent:?}"); | ||
| break; |
There was a problem hiding this comment.
Does this mean we never remove one of the blocks from the database?
There was a problem hiding this comment.
If I understand the code correctly we skip only a missing block.
| @@ -128,6 +128,10 @@ where | |||
| fn clear_block_gap(&self) -> sp_blockchain::Result<()> { | |||
| self.backend.blockchain().clear_block_gap() | |||
| } | |||
|
|
|||
| fn finalize_block(&self, hash: Block::Hash) -> sp_blockchain::Result<()> { | |||
There was a problem hiding this comment.
There is already apply_finality in Substrate, why do we have to add another API on top of this instead of fixing the API we already have?
There was a problem hiding this comment.
Indeed, apply_finality relies on the tree_route which demands existing headers from the genesis in our case. Changing the method seems much more invasive than preparing conditions for its correct work - I'm open to suggestions here though.
|
Also I think by sequential you actually meant contiguous |
There was a problem hiding this comment.
I hate to sound like a broken record in every PR of this kind, but have you discussed this with upstream, is there an open discussion and/or PR there?
We need a generic solution here, not hacks on top of hacks. paritytech#5119 looks like a related discussion. IMO we should implement and submit an upstream fix for a situation like this.
I intended to send upstream displaced_leaves_after_finalizing change part indeed. I wanted to validate the whole solution before posting it upstream because you authored one of the changed part.
The bug seems rather serious so I wanted to have the fix first and start a long discussion with Parity later.
Also I think by sequential you actually meant contiguous
I meant this snippet:
if ensure_sequential_finalization {
self.ensure_sequential_finalization(header, last_finalized)?;
}
| @@ -128,6 +128,10 @@ where | |||
| fn clear_block_gap(&self) -> sp_blockchain::Result<()> { | |||
| self.backend.blockchain().clear_block_gap() | |||
| } | |||
|
|
|||
| fn finalize_block(&self, hash: Block::Hash) -> sp_blockchain::Result<()> { | |||
There was a problem hiding this comment.
Indeed, apply_finality relies on the tree_route which demands existing headers from the genesis in our case. Changing the method seems much more invasive than preparing conditions for its correct work - I'm open to suggestions here though.
| Ok(header) => header, | ||
| Err(_) => { | ||
| debug!("Can't find obtain header metatadata: {parent:?}"); | ||
| break; |
There was a problem hiding this comment.
If I understand the code correctly we skip only a missing block.
Upstream has actually changed things further since, so if you have patches, you should probably build them against upstream, add corresponding tests and open PR upstream. We can then backport it based on feedback and timeline.
I'm convinced discussion should start as soon as you identify what the problem is. Meaning at the very beginning, not at the end of the process.
I see, maybe code should support such finalization then, why is it required to be sequential? |
|
There is an easier way to do that: autonomys/subspace#2972 with the combination of the upstream fixes in |
This PR prepares the polkadot-sdk for enabling non-sequential block finalization in subspace blockchain.
During the snap-sync the code invokes
apply_finalitymethod which implies sequential block order and fails otherwise. Additionally, the recently modifieddisplaced_leaves_after_finalizingtries to find the parent header of the last finalized block and fails after snap-sync(that block is the first available block inserted by the newly addedfinalize_blockAPI and contains no parent header in blockchain db).The proposed changes export
finalize_blockAPI that enables block finalization by disabling sequential block order check, also, the changes soften the error handling fordisplaced_leaves_after_finalizingand allows it to continue after accessing non-existent block header.