-
Notifications
You must be signed in to change notification settings - Fork 990
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
[WIP] Perf: Kernel Sync Performance #1987
Changes from 10 commits
34394c1
a8a2d4d
9d212f7
90c2660
eb87f7a
6eea9ba
6082089
d41774c
f940a07
6fc61a4
e180629
f523bdb
12c56ef
fcc0323
98fa264
b570a22
e7fcf2d
8df281c
202e609
07aacf6
14a8852
79c5790
d468f59
acff2c2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,7 +27,7 @@ use core::consensus; | |
use core::core::hash::{Hash, Hashed}; | ||
use core::core::verifier_cache::VerifierCache; | ||
use core::core::Committed; | ||
use core::core::{Block, BlockHeader, BlockSums}; | ||
use core::core::{Block, BlockHeader, BlockSums, TxKernel}; | ||
use core::global; | ||
use core::pow; | ||
use error::{Error, ErrorKind}; | ||
|
@@ -276,6 +276,45 @@ pub fn sync_block_headers( | |
} | ||
} | ||
|
||
/// Process the kernels. | ||
/// This is only ever used during sync. | ||
pub fn sync_kernels( | ||
first_kernel_index: u64, | ||
kernels: &Vec<TxKernel>, | ||
ctx: &mut BlockContext, | ||
) -> Result<(), Error> { | ||
if let Some(kernel) = kernels.first() { | ||
debug!( | ||
"pipe: sync_kernels: {} kernels from {} at {}", | ||
kernels.len(), | ||
kernel.hash(), | ||
first_kernel_index | ||
); | ||
} else { | ||
return Ok(()); | ||
} | ||
|
||
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? | ||
|
||
for kernel in kernels { | ||
// Ensure kernel is self-consistent | ||
kernel.verify()?; | ||
|
||
// Apply the kernel to the kernel MMR. | ||
extension.apply_kernel(kernel)?; | ||
|
||
// DAVID: If kernel is last in block, validate root. | ||
// Use RewindableKernelView? Or modify validate_kernel_history? | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd consider pushing that whole loop down in txhashset in some There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also shouldn't there be some MMR reconstruction after this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
||
Ok(()) | ||
})?; | ||
} | ||
Ok(()) | ||
} | ||
|
||
/// Process block header as part of "header first" block propagation. | ||
/// We validate the header but we do not store it or update header head based | ||
/// on this. We will update these once we get the block back after requesting | ||
|
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.
That would make sense.