Conversation
Scripts refactor
task3r
changed the title
|
If we disable merkle tree, receipts and policy validation, we get basically the same perf as previously (running on C-ACI, for the reasons we discussed). 
|
task3r
changed the title
| } | ||
|
|
||
| fn validate_qcs( | ||
| chain: &Vec<ProtoBlock>, |
There was a problem hiding this comment.
Having the receipt chain be the whole blocks seems expensive.
(Not necessarily something that needs fixed, but probably a good place to look for optimisations)
There was a problem hiding this comment.
From the rest of this function it might be sufficient to just have the merkle inclusion proof of the previous chained block and the hash?
There was a problem hiding this comment.
A ProtoBlock is just a ref, not the actual block Sorry, I misrembered. Nevertheless, I though you were worried about copying and not the actual receipt structure. We can discuss this with Shubham next time we meet, but he was explicit that we could not simply use the hash of the txs (in this case the merkle root) to prove chain consistency.
There was a problem hiding this comment.
No worries, but the concern is that this is a lot of copying of reasonably large amounts of memory for every transaction in the scitt demo.
| ) -> Result<(), String> { | ||
| let ctx = Context::full(runtime).unwrap(); | ||
| ctx.with::<_, Result<(), String>>(|ctx| { | ||
| if let Err(err) = ctx.eval::<(), _>(policy).catch(&ctx) { |
There was a problem hiding this comment.
Possible optimisation: Cache the runtime with the policy evaluated.
|
|
||
| let globals = ctx.globals(); | ||
| let apply_fn: Function = globals.get("apply").unwrap(); | ||
| let result: Result<Value, _> = apply_fn.call((phdr_arg,)); |
There was a problem hiding this comment.
| let result: Result<Value, _> = apply_fn.call((phdr_arg,)); | |
| let result: Result<Value, _> = apply_fn.call((phdr_arg)); |
There was a problem hiding this comment.
That is needed for type matching.
error[E0277]: the trait bound `rquickjs::Object<'_>: IntoArgs<'_>` is not satisfied
--> src/consensus/engines/scitt.rs:529:54
|
529 | let result: Result<Value, _> = apply_fn.call((phdr_arg));
| ---- ^^^^^^^^^^ the trait `IntoArgs<'_>` is not implemented for `rquickjs::Object<'_>`
| |
| required by a bound introduced by this call
|
= help: the following other types implement trait `IntoArgs<'js>`:
()
(A, B)
(A, B, C)
(A, B, C, D)
(A, B, C, D, E)
(A, B, C, D, E, F)
(A, B, C, D, E, F, G)
(A,)
| return; | ||
| } | ||
|
|
||
| // warn!("Byzantine commit from {} to {}. QC {}", old_bci, new_bci, qc.n); |
src/consensus/app.rs
Outdated
|
|
||
|
|
||
| impl<'a, E: AppEngine + Send + Sync + 'a> Application<'a, E> { | ||
| impl<'a, E: AppEngine + Send + Sync + 'a + 'static> Application<'a, E> { |
There was a problem hiding this comment.
Is static lifetime necessary here?
There was a problem hiding this comment.
+1 on this. If the compiler complains about AppEngine not living long enough, suggest setting where 'a: 'static
src/consensus/app.rs
Outdated
| return; | ||
| } | ||
|
|
||
| #[allow(unused_mut)] |
There was a problem hiding this comment.
Surely this is unnecessary?
There was a problem hiding this comment.
Consider changing to:
let results = {
#[cfg(feature = "concurrent_validation")]
self.engine.write().await.handle_byz_commit(blocks)
#[cfg(not(feature = "concurrent_validation"))]
self.engine.handle_byz_commit(blocks)
};This is more idiomatic in rust.
| Ok(()) | ||
| } | ||
|
|
||
| #[allow(unused_mut)] // ignore warning for unused muts (needed for policy validation, otherwise we would need to copy stuff around needlessly) |
There was a problem hiding this comment.
Is there a better solution (inverse of a COW or something?)
There was a problem hiding this comment.
You mean like when the batch.clone() is sent to the app?
Probably can pin the RawBatch in heap and just pass a pointer.
(Ref to how CachedBlock works)
| pub type ProofChain = Vec<ProtoBlock>; | ||
| pub struct ReceiptBuilder { | ||
| // The chain of blocks from the requested block to the subsequent auditQC | ||
| pub chain: ProofChain, |
There was a problem hiding this comment.
As mentioned elsewhere, this seems very expensive for every receipt.
There was a problem hiding this comment.
Suggest changing to
pub type ProofChain = Vec<CachedBlock>;CachedBlock is pinned in the heap. Vec is in effect just Vec of pointers, instead of here where there is a bunch of memcpy involved potentially.
| crypto::{default_hash, hash, HashType, Sha}, | ||
| proto::consensus::ProtoBlock, utils::unwrap_tx_list, | ||
| }; | ||
|
|
There was a problem hiding this comment.
I assume that you have searched for this, but to double check that there wasn't a good pre-existing merkle proof library?
src/crypto/merkle.rs
Outdated
| impl MerkleTree { | ||
| pub fn new(leaves: Vec<HashType>) -> Self { | ||
| let n_leaves = leaves.len(); | ||
| let n_padded_leaves = n_leaves.next_power_of_two(); |
There was a problem hiding this comment.
This is possibly expensive, but in practise afaicr the blocks are guaranteed to be a power of 2 already?
The alternative is to push the overhanging portion up the tree:
ie [0,1,2,3,4] is hashed H(H(H(0,1),H(2,3)), 4) rather than H(H(0,1),H(2,3)),H(H(4,0),H(0,0)))
There was a problem hiding this comment.
+1 It is more standard to push the nodes up the tree.
| MerkleInclusionProof::new(proof) | ||
| } | ||
|
|
||
| /// benchmarks show that this is actually slightly slower than calling `generate_inclusion_proof` N times... left it for future reference |
There was a problem hiding this comment.
Something something vectorisation something something :)
There was a problem hiding this comment.
The crypto service.rs is already quite crowded. But you can use crypto service to do things in parallel if you want.
Not guaranteed to speed up anything. :-)
|
@task3r Left some meta comments in reply to the @cjen1-msft 's comments. Let me know if I can be of any help. |
* fix formatting * remove static lifetime from app * remove padding from merkle proofs
3 participants
No description provided.