simplify when block_sums and spent_index are added to the db (#3253)

* simplify when block_sums and spent_index are added to the db

* fix pool tests

* cleanup

chain/src/chain.rs

@@ -957,7 +957,7 @@ impl Chain {
 				// Save the block_sums (utxo_sum, kernel_sum) to the db for use later.
 				batch.save_block_sums(
 					&header.hash(),
-					&BlockSums {
+					BlockSums {
 						utxo_sum,
 						kernel_sum,
 					},
@@ -1480,7 +1480,7 @@ fn setup_head(
 						// Save the block_sums to the db for use later.
 						batch.save_block_sums(
 							&header.hash(),
-							&BlockSums {
+							BlockSums {
 								utxo_sum,
 								kernel_sum,
 							},
@@ -1542,7 +1542,7 @@ fn setup_head(
 			})?;
 
 			// Save the block_sums to the db for use later.
-			batch.save_block_sums(&genesis.hash(), &sums)?;
+			batch.save_block_sums(&genesis.hash(), sums)?;
 
 			info!("init: saved genesis: {:?}", genesis.hash());
 		}

chain/src/pipe.rs

@@ -23,7 +23,7 @@ use crate::core::pow;
 use crate::error::{Error, ErrorKind};
 use crate::store;
 use crate::txhashset;
-use crate::types::{CommitPos, Options, Tip};
+use crate::types::{Options, Tip};
 use crate::util::RwLock;
 use grin_store;
 use std::sync::Arc;
@@ -121,7 +121,7 @@ pub fn process_block(b: &Block, ctx: &mut BlockContext<'_>) -> Result<Option<Tip
 	let ref mut header_pmmr = &mut ctx.header_pmmr;
 	let ref mut txhashset = &mut ctx.txhashset;
 	let ref mut batch = &mut ctx.batch;
-	let (block_sums, spent) = txhashset::extending(header_pmmr, txhashset, batch, |ext, batch| {
+	txhashset::extending(header_pmmr, txhashset, batch, |ext, batch| {
 		rewind_and_apply_fork(&prev, ext, batch)?;
 
 		// Check any coinbase being spent have matured sufficiently.
@@ -137,13 +137,12 @@ pub fn process_block(b: &Block, ctx: &mut BlockContext<'_>) -> Result<Option<Tip
 		// we can verify_kernel_sums across the full UTXO sum and full kernel sum
 		// accounting for inputs/outputs/kernels in this new block.
 		// We know there are no double-spends etc. if this verifies successfully.
-		// Remember to save these to the db later on (regardless of extension rollback)
+		verify_block_sums(b, batch)?;
-		let block_sums = verify_block_sums(b, batch)?;
 
 		// Apply the block to the txhashset state.
 		// Validate the txhashset roots and sizes against the block header.
 		// Block is invalid if there are any discrepencies.
-		let spent = apply_block_to_txhashset(b, ext, batch)?;
+		apply_block_to_txhashset(b, ext, batch)?;
 
 		// If applying this block does not increase the work on the chain then
 		// we know we have not yet updated the chain to produce a new chain head.
@@ -154,13 +153,14 @@ pub fn process_block(b: &Block, ctx: &mut BlockContext<'_>) -> Result<Option<Tip
 			ext.extension.force_rollback();
 		}
 
-		Ok((block_sums, spent))
+		Ok(())
 	})?;
 
-	// Add the validated block to the db along with the corresponding block_sums.
+	// Add the validated block to the db.
-	// We do this even if we have not increased the total cumulative work
+	// Note we do this in the outer batch, not the child batch from the extension
-	// so we can maintain multiple (in progress) forks.
+	// as we only commit the child batch if the extension increases total work.
-	add_block(b, &block_sums, &spent, &ctx.batch)?;
+	// We want to save the block to the db regardless.
+	add_block(b, &ctx.batch)?;
 
 	// If we have no "tail" then set it now.
 	if ctx.batch.tail().is_err() {
@@ -404,7 +404,8 @@ fn verify_coinbase_maturity(
 
 /// Verify kernel sums across the full utxo and kernel sets based on block_sums
 /// of previous block accounting for the inputs|outputs|kernels of the new block.
-fn verify_block_sums(b: &Block, batch: &store::Batch<'_>) -> Result<BlockSums, Error> {
+/// Saves the new block_sums to the db via the current batch if successful.
+fn verify_block_sums(b: &Block, batch: &store::Batch<'_>) -> Result<(), Error> {
 	// Retrieve the block_sums for the previous block.
 	let block_sums = batch.get_block_sums(&b.header.prev_hash)?;
 
@@ -419,10 +420,15 @@ fn verify_block_sums(b: &Block, batch: &store::Batch<'_>) -> Result<BlockSums, E
 	let (utxo_sum, kernel_sum) =
 		(block_sums, b as &dyn Committed).verify_kernel_sums(overage, offset)?;
 
-	Ok(BlockSums {
+	batch.save_block_sums(
-		utxo_sum,
+		&b.hash(),
-		kernel_sum,
+		BlockSums {
-	})
+			utxo_sum,
+			kernel_sum,
+		},
+	)?;
+
+	Ok(())
 }
 
 /// Fully validate the block by applying it to the txhashset extension.
@@ -431,25 +437,17 @@ fn apply_block_to_txhashset(
 	block: &Block,
 	ext: &mut txhashset::ExtensionPair<'_>,
 	batch: &store::Batch<'_>,
-) -> Result<Vec<CommitPos>, Error> {
+) -> Result<(), Error> {
-	let spent = ext.extension.apply_block(block, batch)?;
+	ext.extension.apply_block(block, batch)?;
 	ext.extension.validate_roots(&block.header)?;
 	ext.extension.validate_sizes(&block.header)?;
-	Ok(spent)
+	Ok(())
 }
 
 /// Officially adds the block to our chain (possibly on a losing fork).
-/// Adds the associated block_sums and spent_index as well.
 /// Header must be added separately (assume this has been done previously).
-fn add_block(
+fn add_block(b: &Block, batch: &store::Batch<'_>) -> Result<(), Error> {
-	b: &Block,
-	block_sums: &BlockSums,
-	spent: &Vec<CommitPos>,
-	batch: &store::Batch<'_>,
-) -> Result<(), Error> {
 	batch.save_block(b)?;
-	batch.save_block_sums(&b.hash(), block_sums)?;
-	batch.save_spent_index(&b.hash(), spent)?;
 	Ok(())
 }
 

chain/src/store.rs

@@ -318,7 +318,7 @@ impl<'a> Batch<'a> {
 	}
 
 	/// Save block_sums for the block.
-	pub fn save_block_sums(&self, h: &Hash, sums: &BlockSums) -> Result<(), Error> {
+	pub fn save_block_sums(&self, h: &Hash, sums: BlockSums) -> Result<(), Error> {
 		self.db
 			.put_ser(&to_key(BLOCK_SUMS_PREFIX, &mut h.to_vec())[..], &sums)
 	}

chain/src/txhashset/txhashset.rs

@@ -933,9 +933,8 @@ impl<'a> Extension<'a> {
 	/// Apply a new block to the current txhashet extension (output, rangeproof, kernel MMRs).
 	/// Returns a vec of commit_pos representing the pos and height of the outputs spent
 	/// by this block.
-	pub fn apply_block(&mut self, b: &Block, batch: &Batch<'_>) -> Result<Vec<CommitPos>, Error> {
+	pub fn apply_block(&mut self, b: &Block, batch: &Batch<'_>) -> Result<(), Error> {
 		let mut affected_pos = vec![];
-		let mut spent = vec![];
 
 		// Apply the output to the output and rangeproof MMRs.
 		// Add pos to affected_pos to update the accumulator later on.
@@ -949,12 +948,14 @@ impl<'a> Extension<'a> {
 		// Remove the output from the output and rangeproof MMRs.
 		// Add spent_pos to affected_pos to update the accumulator later on.
 		// Remove the spent output from the output_pos index.
+		let mut spent = vec![];
 		for input in b.inputs() {
 			let spent_pos = self.apply_input(input, batch)?;
 			affected_pos.push(spent_pos.pos);
 			batch.delete_output_pos_height(&input.commitment())?;
 			spent.push(spent_pos);
 		}
+		batch.save_spent_index(&b.hash(), &spent)?;
 
 		for kernel in b.kernels() {
 			self.apply_kernel(kernel)?;
@@ -966,7 +967,7 @@ impl<'a> Extension<'a> {
 		// Update the head of the extension to reflect the block we just applied.
 		self.head = Tip::from_header(&b.header);
 
-		Ok(spent)
+		Ok(())
 	}
 
 	fn apply_to_bitmap_accumulator(&mut self, output_pos: &[u64]) -> Result<(), Error> {

pool/tests/common.rs

@@ -83,7 +83,7 @@ impl ChainAdapter {
 			utxo_sum,
 			kernel_sum,
 		};
-		batch.save_block_sums(&header.hash(), &block_sums).unwrap();
+		batch.save_block_sums(&header.hash(), block_sums).unwrap();
 
 		batch.commit().unwrap();