[WIP] Perf: Kernel Sync Performance #1987
Conversation
…antioch and igno.
…ck_hash (to verify chain) and a first_kernel_index.
DavidBurkett
changed the title
…ly requests synchronously from 1 peer.
…till need to only send Outputs & RangeProofs (no kernels) to peers that support ENHANCED_TXHASHSET_HIST.
chain/src/pipe.rs
Outdated
|
|
||
| if ctx.txhashset.num_kernels() < (first_kernel_index + 1) { | ||
| txhashset::extending(&mut ctx.txhashset, &mut ctx.batch, |extension| { | ||
| // DAVID: Rewind mmr to correct kernel index just to be safe? |
There was a problem hiding this comment.
That would make sense.
chain/src/pipe.rs
Outdated
|
|
||
| // DAVID: If kernel is last in block, validate root. | ||
| // Use RewindableKernelView? Or modify validate_kernel_history? | ||
| } |
There was a problem hiding this comment.
I'd consider pushing that whole loop down in txhashset in some apply_kernels.
There was a problem hiding this comment.
Also shouldn't there be some MMR reconstruction after this?
There was a problem hiding this comment.
My bad, looks like apply does it. But there should be some clearer separation around validation. Here you're verifying each kernel, but the whole state validation will do that again. So we either slip the latter one or this one.
There was a problem hiding this comment.
Depends what you mean by whole state validation. If you're talking about after the TxHashSet is fully downloaded and processed, sure. But then we'd have to go back and start the kernel validation process all over again if something's wrong with the kernel. If instead you're referring to the expected call to "validate_kernel_history" on line 309, then I disagree. It doesn't verify the signature of the kernel. It only verifies that the MMR root matches the header.
There was a problem hiding this comment.
After re-reading, it appears you're talking about verify_kernel_signatures in Extension. Yea, I can probably skip that verify if it's already been done. It'd probably be easiest to store a 'validated' boolean on the kernel. If you're opposed to adding the extra byte (since kernels are all written to disk), I can look into other options.
chain/src/pipe.rs
Outdated
|
|
||
| // DAVID: If kernel is last in block, validate root. | ||
| // Use RewindableKernelView? Or modify validate_kernel_history? | ||
| } |
There was a problem hiding this comment.
Also shouldn't there be some MMR reconstruction after this?
chain/src/pipe.rs
Outdated
|
|
||
| // DAVID: If kernel is last in block, validate root. | ||
| // Use RewindableKernelView? Or modify validate_kernel_history? | ||
| } |
There was a problem hiding this comment.
My bad, looks like apply does it. But there should be some clearer separation around validation. Here you're verifying each kernel, but the whole state validation will do that again. So we either slip the latter one or this one.
|
previous approaches have included (BTC example): verify twice, followed by
changing one to an assertation that is optimized out for production
builds, followed
by removing the really working check. From this, maybe keeping a bool is
wise. The bool can be stripped before serialization to disk.
|
I'm not sure we can strip the bool before serialization to disk. We'd be flushing to disk shortly after first validation I believe, and wouldn't perform the second validation (where boolean is needed) until much later in the sync process when we receive the outputs in rangeproofs as part of TxHashSet download. |
…eight indices haven't been built yet. Using header mmr instead.
|
@antiochp Requested that this be reviewed and merged in several phases. Closing this one and opening one for the first phase. |
Sync Performance: Support requesting kernel MMRs separate from the rest of TxHashSet
#1968