-
Notifications
You must be signed in to change notification settings - Fork 273
Refactor: Unify intrusive pruning point update and apply proof db writes #807
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?
Changes from all commits
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 |
|---|---|---|
|
|
@@ -112,6 +112,15 @@ use self::{services::ConsensusServices, storage::ConsensusStorage}; | |
|
|
||
| use crate::model::stores::selected_chain::SelectedChainStoreReader; | ||
|
|
||
| // --- IMPORTS --- | ||
| use crate::model::stores::{ | ||
| selected_chain::DbSelectedChainStore, | ||
| tips::DbTipsStore, // HeadersStore used to be here | ||
| virtual_state::VirtualStores, | ||
| }; | ||
| use parking_lot::RwLock; | ||
| // ----------------------------- | ||
|
Collaborator
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. same regarding the separator above |
||
|
|
||
| pub struct Consensus { | ||
| // DB | ||
| db: Arc<DB>, | ||
|
|
@@ -480,25 +489,37 @@ impl Consensus { | |
|
|
||
| // Update virtual state based to the new pruning point | ||
| // Updating of the utxoset is done separately as it requires downloading the new utxoset in its entirety. | ||
|
|
||
| // IMPORTANT: This must be active; we need the object! | ||
|
Collaborator
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. It seems this comment is a personal one directed at you. Without your context it is not meaningful. |
||
| let virtual_parents = vec![new_pruning_point]; | ||
| let virtual_state = Arc::new(VirtualState { | ||
| parents: virtual_parents.clone(), | ||
| ghostdag_data: self.services.ghostdag_manager.ghostdag(&virtual_parents), | ||
| ..VirtualState::default() | ||
| }); | ||
| self.virtual_stores.write().state.set_batch(&mut batch, virtual_state).unwrap(); | ||
| // Remove old body tips and insert pruning point as the current tip | ||
|
|
||
| // Remove old body tips (specific to intrusive update) | ||
|
Collaborator
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. likewise, this comment contains info regarding the coding process itself. |
||
| self.body_tips_store.write().delete_all_tips(&mut batch).unwrap(); | ||
| self.body_tips_store.write().init_batch(&mut batch, &virtual_parents).unwrap(); | ||
| // Update selected_chain | ||
| self.selected_chain_store.write().init_with_pruning_point(&mut batch, new_pruning_point).unwrap(); | ||
|
|
||
| // Call the new standalone helper to do the common DB updates | ||
|
Collaborator
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. What is new today won't be new in 2 years. Comments should not have temporal aspects unless well justified. |
||
| write_common_pruning_point_db_updates( | ||
| &self.virtual_stores, | ||
| &self.body_tips_store, | ||
| &self.selected_chain_store, | ||
| &mut batch, | ||
| new_pruning_point, | ||
| virtual_state, // Wir übergeben das oben berechnete Objekt | ||
|
Collaborator
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. German remenants |
||
| )?; | ||
|
|
||
| // It is important to set this flag to false together with writing the batch, in case the node crashes suddenly before syncing of new utxo starts | ||
| self.pruning_meta_stores.write().set_pruning_utxoset_stable_flag(&mut batch, false).unwrap(); | ||
|
|
||
|
Collaborator
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. remove the added empty lines here and below. it makes git history noisy to add these without good reason |
||
| // Store the currently bodyless anticone from the POV of the syncer, for trusted body validation at a later stage. | ||
| let mut anticone = self.services.dag_traversal_manager.anticone(new_pruning_point, [syncer_sink].into_iter(), None)?; | ||
| // Add the pruning point itself which is also missing a body | ||
| anticone.push(new_pruning_point); | ||
| self.pruning_meta_stores.write().set_body_missing_anticone(&mut batch, anticone).unwrap(); | ||
|
|
||
| self.db.write(batch).unwrap(); | ||
| drop(pruning_point_write); | ||
| Ok(()) | ||
|
|
@@ -1426,3 +1447,25 @@ impl ConsensusApi for Consensus { | |
| pruning_meta_read.is_in_transitional_ibd_state() | ||
| } | ||
| } | ||
|
|
||
| pub fn write_common_pruning_point_db_updates( | ||
|
Collaborator
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. If you look above you will something unique in this function from the entire file - this is likely the only function that doesn't have self as an argument. in other words this is a utility function, and could have been near anywhere. Utility functions are meant to be rather general, here however you have many many arguments, of very specific types. Since we are refactoring, we want the good to have good form. You can see the conversation here for some more context https://github.com/kaspanet/rusty-kaspa/pull/702/changes#r2379996647 |
||
| virtual_stores: &Arc<RwLock<VirtualStores>>, | ||
| body_tips_store: &Arc<RwLock<DbTipsStore>>, | ||
| selected_chain_store: &Arc<RwLock<DbSelectedChainStore>>, | ||
| batch: &mut WriteBatch, | ||
| new_pruning_point: Hash, | ||
| virtual_state: Arc<VirtualState>, | ||
| ) -> ConsensusResult<()> { | ||
| // 1. write virtual state | ||
| virtual_stores.write().state.set_batch(batch, virtual_state).unwrap(); | ||
|
|
||
| // 2. reset body tips | ||
| let virtual_parents = vec![new_pruning_point]; | ||
| // now that we have the correct type (TipsStore), it also recognizes “init_batch” again. | ||
| body_tips_store.write().init_batch(batch, &virtual_parents).unwrap(); | ||
|
|
||
| // 3. update Selected Chain | ||
| selected_chain_store.write().init_with_pruning_point(batch, new_pruning_point).unwrap(); | ||
|
|
||
| Ok(()) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,8 +27,8 @@ use crate::{ | |
| headers::HeaderStore, | ||
| reachability::StagingReachabilityStore, | ||
| relations::StagingRelationsStore, | ||
| selected_chain::SelectedChainStore, | ||
| virtual_state::{VirtualState, VirtualStateStore}, | ||
| //selected_chain::SelectedChainStore, | ||
|
Collaborator
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. if not necessary remove, don't comment out |
||
| virtual_state::VirtualState, | ||
| }, | ||
| }, | ||
| processes::{ | ||
|
|
@@ -40,6 +40,8 @@ use crate::{ | |
|
|
||
| use super::PruningProofManager; | ||
|
|
||
| use crate::consensus::write_common_pruning_point_db_updates; | ||
|
|
||
| impl PruningProofManager { | ||
| pub fn apply_proof(&self, proof: PruningPointProof, trusted_set: &[TrustedBlock]) -> PruningImportResult<()> { | ||
| // Following validation of a pruning proof, various consensus storages must be updated | ||
|
|
@@ -143,16 +145,30 @@ impl PruningProofManager { | |
| ghostdag_data: self.ghostdag_manager.ghostdag(&virtual_parents), | ||
| ..VirtualState::default() | ||
| }); | ||
| self.virtual_stores.write().state.set(virtual_state).unwrap(); | ||
|
|
||
| let mut batch = WriteBatch::default(); | ||
| self.body_tips_store.write().init_batch(&mut batch, &virtual_parents).unwrap(); | ||
|
|
||
| // REFACTOR: Use the common helper to apply standard pruning point DB updates | ||
|
Collaborator
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. likewise, this comment contains info regarding the coding process itself. |
||
| // Wir nutzen .unwrap(), weil ein DB-Fehler hier fatal ist (genau wie im Original-Code) | ||
|
Collaborator
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. Some german has survived the previous culling :) |
||
| write_common_pruning_point_db_updates( | ||
| &self.virtual_stores, | ||
| &self.body_tips_store, | ||
| &self.selected_chain_store, | ||
| &mut batch, | ||
| pruning_point, | ||
| virtual_state, | ||
| ) | ||
| .unwrap(); | ||
|
|
||
| // Apply updates specific to the pruning proof flow (not shared with intrusive update) | ||
|
Collaborator
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. likewise, this comment contains info regarding the coding process itself. |
||
| self.headers_selected_tip_store | ||
| .write() | ||
| .set_batch(&mut batch, SortableBlock { hash: pruning_point, blue_work: pruning_point_header.blue_work }) | ||
| .unwrap(); | ||
| self.selected_chain_store.write().init_with_pruning_point(&mut batch, pruning_point).unwrap(); | ||
|
|
||
| self.depth_store.insert_batch(&mut batch, pruning_point, ORIGIN, ORIGIN).unwrap(); | ||
|
|
||
| // Finalize the batch write | ||
|
Collaborator
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. This is again an internal comment of your writing process |
||
| self.db.write(batch).unwrap(); | ||
|
|
||
| Ok(()) | ||
|
|
||
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.
There is no need for something like this