-
Notifications
You must be signed in to change notification settings - Fork 251
Consensus level support for cryptographic receipts #725
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
base: master
Are you sure you want to change the base?
Conversation
1) create a witness for inclusion of a leaf in a merkle tree 2) verify the inclusion given a leaf and said witness
improved tests removed legacy code
improved general structure of code simplified test code added ignore for the perfomance dependant test test_writer_reentrance per instructions of Aspect and Michael
Preliminary work on POCHM
Fixed error:changed daa score to blue score for posterity blocks minor refactoring
get_pre_posterity get_post_posterity get posterity_by_bscore. Changed/improved implementation of representative_log_parents More error logic
…various other minor improvements
bug fix regarding accepted txs refactoring a few for loops
legacy_pochm complete
Changed namings removed DB entries for pchmr
merged recent changes
changed a function's name
coderofstuff
left a comment
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.
Still need to wrap my head around the KIP and PR. Initial cosmetic comments first :)
consensus/core/src/receipts.rs
Outdated
| use crate::header::Header; | ||
| use kaspa_hashes::Hash; | ||
| #[derive(Clone)] | ||
| pub struct Pochm { |
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.
Please rename this struct to ProofOfChainMembership. I know what pochm means because I'm familiar with the KIP, but an arbitrary reader of this struct will not have that context immediately.
Also, please add comments to the struct or parts of the struct explaining what this it's intended. Take a different struct for example, it has this:
pub struct PruningPointReply {
/// The most recent pruning sample from POV of the queried block (with distance up to ~F)
pub pruning_sample: Hash,
/// The pruning point of the queried block. I.e., the most recent pruning sample with
/// depth P (except for shortly after the fork where the new P' is gradually reached)
pub pruning_point: Hash,
}
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.
see now
consensus/core/src/api/mod.rs
Outdated
| ) -> ConsensusResult<ProofOfPublication> { | ||
| unimplemented!() | ||
| } | ||
| fn generate_pochm(&self, block: Hash) -> ConsensusResult<Pochm> { |
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.
Please use generate_proof_of_chain_membership
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.
addressed
consensus/core/src/api/mod.rs
Outdated
| fn verify_tx_receipt(&self, receipt: &TxReceipt) -> bool { | ||
| unimplemented!() | ||
| } | ||
| fn verify_proof_of_pub(&self, proof_of_pub: &ProofOfPublication) -> bool { |
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.
Please use verify_proof_of_publication
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.
addressed
consensus/core/src/api/mod.rs
Outdated
| fn verify_proof_of_pub(&self, proof_of_pub: &ProofOfPublication) -> bool { | ||
| unimplemented!() | ||
| } | ||
| fn verify_pochm(&self, chain_purporter: Hash, pochm: &Pochm) -> bool { |
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.
Please use verify_proof_of_chain_membership
consensus/src/consensus/mod.rs
Outdated
| .map(|hash| (hash, self.headers_store.get_compact_header_data(hash).unwrap())) | ||
| .collect_vec() | ||
| } | ||
| /* this logic may not be the most efficient and reliable as of now */ |
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.
Inside this function are several large comment blocks. Please update this into a function comment explaining what this function does - maybe move parts of those large comments below to this function comment.
Take a look at this function elsewhere for an example:
pub fn select_transactions(&mut self) -> Vec<Transaction>
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.
see now
consensus/core/src/receipts.rs
Outdated
| use kaspa_hashes::Hash; | ||
| #[derive(Clone)] | ||
| pub struct Pochm { | ||
| pub hdr_map: HashMap<Hash, Arc<Header>>, |
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.
At a first lecture, I thought hdr was reference to some specific object I wasn't aware of, would it be possible to rename it header instead?
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.
addressed
fixed bug in proof of publication verification
|
I removed proof of publication and proof of chain memberships as it became apparent to me these features' implementation is unsatisfactory, and the correct implementation not immediate. Coupled with the fact that it greatly complicated tests, and that the usecase was always dubious I decided to cut it off in the hopes that it will ease reviewing. An implementation of proof of publication as it previously was can be found at https://github.com/freshair18/rusty-kaspa/tree/crypt_receipts_with_pop. |
Provides consensus layer support for cryptographic receipts based on KIP15.
Additionally:
Supersedes #609, addresses #709.