From 980378eb6549b37d3b28cf1123a3867bdec1df84 Mon Sep 17 00:00:00 2001
From: Antioch Peverell <30642645+antiochp@users.noreply.github.com>
Date: Sun, 8 Jul 2018 17:37:09 +0100
Subject: [PATCH] fix misbehaving simulnet fastsync test (#1227)

* fix misbehaving simulnet fastsync test
cleanup redundant cutoff vs bitmap params is rewind and check_compact

* make sure we do not verify full kernel history on a writeable txhashset extension
rework simulnet simulate_fast_sync test to be more robust

* fixup store tests

* sleep for a bit longer to give nodes time to update
their sync_state correctly

* tweak timing of simulate_block_propagation
---
 chain/src/chain.rs            | 19 ++++++++----
 chain/src/txhashset.rs        | 55 ++++++++++-------------------------
 chain/src/types.rs            |  2 +-
 core/src/core/pmmr.rs         |  4 +--
 core/tests/vec_backend/mod.rs |  1 -
 servers/src/common/types.rs   | 20 +++++++++++--
 servers/src/grin/sync.rs      | 16 +++++++---
 servers/tests/simulnet.rs     | 53 ++++++++++++++++++++++++---------
 servers/tests/stratum.rs      |  2 +-
 src/bin/tui/status.rs         |  1 +
 store/src/leaf_set.rs         | 18 ++++++++++--
 store/src/pmmr.rs             |  9 ++----
 store/tests/pmmr.rs           | 28 +++++++++---------
 store/tests/utxo_set_perf.rs  |  2 +-
 14 files changed, 134 insertions(+), 96 deletions(-)

diff --git a/chain/src/chain.rs b/chain/src/chain.rs
index 9f6121b9b..e91f7f663 100644
--- a/chain/src/chain.rs
+++ b/chain/src/chain.rs
@@ -425,15 +425,17 @@ impl Chain {
 			return Ok(());
 		}
 
+		// We want to validate the full kernel history here for completeness.
+		let skip_kernel_hist = false;
+
 		let mut txhashset = self.txhashset.write().unwrap();
 
 		// Now create an extension from the txhashset and validate against the
 		// latest block header. Rewind the extension to the specified header to
 		// ensure the view is consistent.
 		txhashset::extending_readonly(&mut txhashset, |extension| {
-			// TODO - is this rewind guaranteed to be redundant now?
 			extension.rewind(&header, &header)?;
-			extension.validate(&header, skip_rproofs, &NoStatus)?;
+			extension.validate(&header, skip_rproofs, skip_kernel_hist, &NoStatus)?;
 			Ok(())
 		})
 	}
@@ -546,22 +548,25 @@ impl Chain {
 
 		// validate against a read-only extension first (some of the validation
 		// runs additional rewinds)
+		debug!(LOGGER, "chain: txhashset_write: rewinding and validating (read-only)");
 		txhashset::extending_readonly(&mut txhashset, |extension| {
 			extension.rewind(&header, &header)?;
-			extension.validate(&header, false, status)
+			extension.validate(&header, false, false, status)?;
+			Ok(())
 		})?;
 
 		// all good, prepare a new batch and update all the required records
+		debug!(LOGGER, "chain: txhashset_write: rewinding and validating a 2nd time (writeable)");
 		let mut batch = self.store.batch()?;
 		txhashset::extending(&mut txhashset, &mut batch, |extension| {
-			// TODO do we need to rewind here? We have no blocks to rewind
-			// (and we need them for the pos to unremove)
 			extension.rewind(&header, &header)?;
-			extension.validate(&header, false, status)?;
+			extension.validate(&header, false, true, status)?;
 			extension.rebuild_index()?;
 			Ok(())
 		})?;
 
+		debug!(LOGGER, "chain: txhashset_write: finished validating and rebuilding");
+
 		status.on_save();
 		// replace the chain txhashset with the newly built one
 		{
@@ -578,6 +583,8 @@ impl Chain {
 		}
 		batch.commit()?;
 
+		debug!(LOGGER, "chain: txhashset_write: finished committing the batch (head etc.)");
+
 		self.check_orphans(header.height + 1);
 
 		status.on_done();
diff --git a/chain/src/txhashset.rs b/chain/src/txhashset.rs
index cbe9166ee..1806d89a4 100644
--- a/chain/src/txhashset.rs
+++ b/chain/src/txhashset.rs
@@ -233,7 +233,6 @@ impl TxHashSet {
 		let horizon = current_height.saturating_sub(global::cut_through_horizon().into());
 		let horizon_header = self.commit_index.get_header_by_height(horizon)?;
 
-		let rewind_add_pos = output_pos_to_rewind(&horizon_header, &head_header)?;
 		let rewind_rm_pos =
 			input_pos_to_rewind(self.commit_index.clone(), &horizon_header, &head_header)?;
 
@@ -249,14 +248,12 @@ impl TxHashSet {
 
 			self.output_pmmr_h.backend.check_compact(
 				horizon_header.output_mmr_size,
-				&rewind_add_pos,
 				&rewind_rm_pos.1,
 				clean_output_index,
 			)?;
 
 			self.rproof_pmmr_h.backend.check_compact(
 				horizon_header.output_mmr_size,
-				&rewind_add_pos,
 				&rewind_rm_pos.1,
 				&prune_noop,
 			)?;
@@ -453,14 +450,9 @@ impl<'a> Extension<'a> {
 		kernel_pos: u64,
 		rewind_rm_pos: &Bitmap,
 	) -> Result<(), Error> {
-		let latest_output_pos = self.output_pmmr.unpruned_size();
-		let rewind_add_pos: Bitmap = ((output_pos + 1)..(latest_output_pos + 1))
-			.map(|x| x as u32)
-			.collect();
 		self.rewind_to_pos(
 			output_pos,
 			kernel_pos,
-			&rewind_add_pos,
 			rewind_rm_pos,
 		)?;
 		Ok(())
@@ -475,9 +467,7 @@ impl<'a> Extension<'a> {
 	/// new tx).
 	pub fn apply_raw_tx(&mut self, tx: &Transaction) -> Result<(), Error> {
 		// This should *never* be called on a writeable extension...
-		if !self.rollback {
-			panic!("attempted to apply a raw tx to a writeable txhashset extension");
-		}
+		assert!(self.rollback, "applied raw_tx to writeable txhashset extension");
 
 		// Checkpoint the MMR positions before we apply the new tx,
 		// anything goes wrong we will rewind to these positions.
@@ -769,7 +759,6 @@ impl<'a> Extension<'a> {
 		// undone during rewind).
 		// Rewound output pos will be removed from the MMR.
 		// Rewound input (spent) pos will be added back to the MMR.
-		let rewind_add_pos = output_pos_to_rewind(block_header, head_header)?;
 		let rewind_rm_pos =
 			input_pos_to_rewind(self.commit_index.clone(), block_header, head_header)?;
 		if !rewind_rm_pos.0 {
@@ -780,7 +769,6 @@ impl<'a> Extension<'a> {
 		self.rewind_to_pos(
 			block_header.output_mmr_size,
 			block_header.kernel_mmr_size,
-			&rewind_add_pos,
 			&rewind_rm_pos.1,
 		)
 	}
@@ -791,7 +779,6 @@ impl<'a> Extension<'a> {
 		&mut self,
 		output_pos: u64,
 		kernel_pos: u64,
-		rewind_add_pos: &Bitmap,
 		rewind_rm_pos: &Bitmap,
 	) -> Result<(), Error> {
 		trace!(
@@ -807,13 +794,13 @@ impl<'a> Extension<'a> {
 		self.new_output_commits.retain(|_, &mut v| v <= output_pos);
 
 		self.output_pmmr
-			.rewind(output_pos, rewind_add_pos, rewind_rm_pos)
+			.rewind(output_pos, rewind_rm_pos)
 			.map_err(&ErrorKind::TxHashSetErr)?;
 		self.rproof_pmmr
-			.rewind(output_pos, rewind_add_pos, rewind_rm_pos)
+			.rewind(output_pos, rewind_rm_pos)
 			.map_err(&ErrorKind::TxHashSetErr)?;
 		self.kernel_pmmr
-			.rewind(kernel_pos, rewind_add_pos, rewind_rm_pos)
+			.rewind(kernel_pos, &Bitmap::create())
 			.map_err(&ErrorKind::TxHashSetErr)?;
 		Ok(())
 	}
@@ -882,6 +869,7 @@ impl<'a> Extension<'a> {
 		&mut self,
 		header: &BlockHeader,
 		skip_rproofs: bool,
+		skip_kernel_hist: bool,
 		status: &T,
 	) -> Result<((Commitment, Commitment)), Error>
 	where
@@ -911,7 +899,9 @@ impl<'a> Extension<'a> {
 
 		// Verify kernel roots for all past headers, need to be last as it rewinds
 		// a lot without resetting
-		self.verify_kernel_history(header)?;
+		if !skip_kernel_hist {
+			self.verify_kernel_history(header)?;
+		}
 
 		Ok((output_sum, kernel_sum))
 	}
@@ -1040,14 +1030,15 @@ impl<'a> Extension<'a> {
 		Ok(())
 	}
 
+	// Special handling to make sure the whole kernel set matches each of its
+	// roots in each block header, without truncation. We go back header by
+	// header, rewind and check each root. This fixes a potential weakness in
+	// fast sync where a reorg past the horizon could allow a whole rewrite of
+	// the kernel set.
 	fn verify_kernel_history(&mut self, header: &BlockHeader) -> Result<(), Error> {
-		// Special handling to make sure the whole kernel set matches each of its
-		// roots in each block header, without truncation. We go back header by
-		// header, rewind and check each root. This fixes a potential weakness in
-		// fast sync where a reorg past the horizon could allow a whole rewrite of
-		// the kernel set.
+		assert!(self.rollback, "verified kernel history on writeable txhashset extension");
+
 		let mut current = header.clone();
-		let empty_bitmap = Bitmap::create();
 		loop {
 			current = self.commit_index.get_block_header(&current.previous)?;
 			if current.height == 0 {
@@ -1055,7 +1046,7 @@ impl<'a> Extension<'a> {
 			}
 			// rewinding kernels only further and further back
 			self.kernel_pmmr
-				.rewind(current.kernel_mmr_size, &empty_bitmap, &empty_bitmap)
+				.rewind(current.kernel_mmr_size, &Bitmap::create())
 				.map_err(&ErrorKind::TxHashSetErr)?;
 			if self.kernel_pmmr.root() != current.kernel_root {
 				return Err(ErrorKind::InvalidTxHashSet(format!(
@@ -1096,20 +1087,6 @@ pub fn zip_write(root_dir: String, txhashset_data: File) -> Result<(), Error> {
 		.map_err(|ze| ErrorKind::Other(ze.to_string()).into())
 }
 
-/// Given a block header to rewind to and the block header at the
-/// head of the current chain state, we need to calculate the positions
-/// of all outputs we need to "undo" during a rewind.
-/// The MMR is append-only so we can simply look for all positions added after
-/// the rewind pos.
-fn output_pos_to_rewind(
-	block_header: &BlockHeader,
-	head_header: &BlockHeader,
-) -> Result<Bitmap, Error> {
-	let marker_to = head_header.output_mmr_size;
-	let marker_from = block_header.output_mmr_size;
-	Ok(((marker_from + 1)..=marker_to).map(|x| x as u32).collect())
-}
-
 /// Given a block header to rewind to and the block header at the
 /// head of the current chain state, we need to calculate the positions
 /// of all inputs (spent outputs) we need to "undo" during a rewind.
diff --git a/chain/src/types.rs b/chain/src/types.rs
index a98d1bf78..e31565ae9 100644
--- a/chain/src/types.rs
+++ b/chain/src/types.rs
@@ -49,7 +49,7 @@ pub struct TxHashSetRoots {
 /// blockchain tree. References the max height and the latest and previous
 /// blocks
 /// for convenience and the total difficulty.
-#[derive(Serialize, Deserialize, Debug, Clone)]
+#[derive(Serialize, Deserialize, Debug, Clone, PartialEq)]
 pub struct Tip {
 	/// Height of the tip (max height of the fork)
 	pub height: u64,
diff --git a/core/src/core/pmmr.rs b/core/src/core/pmmr.rs
index d8030c84d..81a245642 100644
--- a/core/src/core/pmmr.rs
+++ b/core/src/core/pmmr.rs
@@ -67,7 +67,6 @@ where
 	fn rewind(
 		&mut self,
 		position: u64,
-		rewind_add_pos: &Bitmap,
 		rewind_rm_pos: &Bitmap,
 	) -> Result<(), String>;
 
@@ -301,7 +300,6 @@ where
 	pub fn rewind(
 		&mut self,
 		position: u64,
-		rewind_add_pos: &Bitmap,
 		rewind_rm_pos: &Bitmap,
 	) -> Result<(), String> {
 		// Identify which actual position we should rewind to as the provided
@@ -312,7 +310,7 @@ where
 			pos += 1;
 		}
 
-		self.backend.rewind(pos, rewind_add_pos, rewind_rm_pos)?;
+		self.backend.rewind(pos, rewind_rm_pos)?;
 		self.last_pos = pos;
 		Ok(())
 	}
diff --git a/core/tests/vec_backend/mod.rs b/core/tests/vec_backend/mod.rs
index 942ee324a..3e56e8935 100644
--- a/core/tests/vec_backend/mod.rs
+++ b/core/tests/vec_backend/mod.rs
@@ -121,7 +121,6 @@ where
 	fn rewind(
 		&mut self,
 		position: u64,
-		_rewind_add_pos: &Bitmap,
 		_rewind_rm_pos: &Bitmap,
 	) -> Result<(), String> {
 		self.elems = self.elems[0..(position as usize) + 1].to_vec();
diff --git a/servers/src/common/types.rs b/servers/src/common/types.rs
index f3a176a52..751cd8743 100644
--- a/servers/src/common/types.rs
+++ b/servers/src/common/types.rs
@@ -24,6 +24,7 @@ use core::{core, pow};
 use p2p;
 use pool;
 use store;
+use util::LOGGER;
 use wallet;
 
 /// Error type wrapping underlying module errors.
@@ -259,6 +260,8 @@ impl Default for StratumServerConfig {
 #[derive(Debug, Clone, Copy, Eq, PartialEq)]
 #[allow(missing_docs)]
 pub enum SyncStatus {
+	/// Initial State (we do not yet know if we are/should be syncing)
+	Initial,
 	/// Not syncing
 	NoSync,
 	/// Downloading block headers
@@ -295,11 +298,12 @@ impl SyncState {
 	/// Return a new SyncState initialize to NoSync
 	pub fn new() -> SyncState {
 		SyncState {
-			current: RwLock::new(SyncStatus::NoSync),
+			current: RwLock::new(SyncStatus::Initial),
 		}
 	}
 
-	/// Whether the current state matches any active syncing operation
+	/// Whether the current state matches any active syncing operation.
+	/// Note: This includes our "initial" state.
 	pub fn is_syncing(&self) -> bool {
 		*self.current.read().unwrap() != SyncStatus::NoSync
 	}
@@ -311,7 +315,19 @@ impl SyncState {
 
 	/// Update the syncing status
 	pub fn update(&self, new_status: SyncStatus) {
+		if self.status() == new_status {
+			return;
+		}
+
 		let mut status = self.current.write().unwrap();
+
+		debug!(
+			LOGGER,
+			"sync_state: sync_status: {:?} -> {:?}",
+			*status,
+			new_status,
+		);
+
 		*status = new_status;
 	}
 }
diff --git a/servers/src/grin/sync.rs b/servers/src/grin/sync.rs
index 33ca14b5d..f8ce69264 100644
--- a/servers/src/grin/sync.rs
+++ b/servers/src/grin/sync.rs
@@ -72,11 +72,19 @@ pub fn run_sync(
 		.spawn(move || {
 			let mut si = SyncInfo::new();
 
-			// initial sleep to give us time to peer with some nodes
-			if !skip_sync_wait {
+			{
+				// Initial sleep to give us time to peer with some nodes.
+				// Note: Even if we have "skip_sync_wait" we need to wait a
+				// short period of time for tests to do the right thing.
+				let wait_secs = if skip_sync_wait {
+					3
+				} else {
+					30
+				};
+
 				awaiting_peers.store(true, Ordering::Relaxed);
 				let mut n = 0;
-				while peers.more_work_peers().len() < 4 && n < 30 {
+				while peers.more_work_peers().len() < 4 && n < wait_secs {
 					thread::sleep(Duration::from_secs(1));
 					n += 1;
 				}
@@ -305,7 +313,7 @@ fn needs_syncing(
 					);
 
 					let _ = chain.reset_head();
-					return (false, 0);
+					return (false, most_work_height);
 				}
 			}
 		} else {
diff --git a/servers/tests/simulnet.rs b/servers/tests/simulnet.rs
index a856aa358..351c5d463 100644
--- a/servers/tests/simulnet.rs
+++ b/servers/tests/simulnet.rs
@@ -25,6 +25,7 @@ mod framework;
 use std::default::Default;
 use std::{thread, time};
 
+use core::core::hash::Hashed;
 use core::global::{self, ChainTypes};
 
 use framework::{config, stratum_config, LocalServerContainerConfig, LocalServerContainerPool,
@@ -190,7 +191,6 @@ fn simulate_block_propagation() {
 
 	// start mining
 	servers[0].start_test_miner(None);
-	let _original_height = servers[0].head().height;
 
 	// monitor for a change of head on a different server and check whether
 	// chain height has changed
@@ -204,7 +204,7 @@ fn simulate_block_propagation() {
 		if count == 5 {
 			break;
 		}
-		thread::sleep(time::Duration::from_millis(100));
+		thread::sleep(time::Duration::from_millis(1_000));
 	}
 	for n in 0..5 {
 		servers[n].stop();
@@ -228,12 +228,21 @@ fn simulate_full_sync() {
 	s1.start_test_miner(None);
 	thread::sleep(time::Duration::from_secs(8));
 
-	#[ignore(unused_mut)] // mut needed?
-	let mut conf = framework::config(1001, "grin-sync", 1000);
-	let s2 = servers::Server::new(conf).unwrap();
-	while s2.head().height < 4 {
-		thread::sleep(time::Duration::from_millis(100));
+	let s2 = servers::Server::new(framework::config(1001, "grin-sync", 1000)).unwrap();
+
+	// Get the current header from s1.
+	let s1_header = s1.chain.head_header().unwrap();
+
+	// Wait for s2 to sync up to and including the header from s1.
+	while s2.head().height < s1_header.height {
+		thread::sleep(time::Duration::from_millis(1_000));
 	}
+
+	// Confirm both s1 and s2 see a consistent header at that height.
+	let s2_header = s2.chain.get_block_header(&s1_header.hash()).unwrap();
+	assert_eq!(s1_header, s2_header);
+
+	// Stop our servers cleanly.
 	s1.stop();
 	s2.stop();
 }
@@ -250,19 +259,35 @@ fn simulate_fast_sync() {
 	let test_name_dir = "grin-fast";
 	framework::clean_all_output(test_name_dir);
 
+	// start s1 and mine enough blocks to get beyond the fast sync horizon
 	let s1 = servers::Server::new(framework::config(2000, "grin-fast", 2000)).unwrap();
-	// mine a few blocks on server 1
 	s1.start_test_miner(None);
-	thread::sleep(time::Duration::from_secs(8));
+
+	while s1.head().height < 21 {
+		thread::sleep(time::Duration::from_millis(1_000));
+	}
 
 	let mut conf = config(2001, "grin-fast", 2000);
 	conf.archive_mode = Some(false);
-	let s2 = servers::Server::new(conf).unwrap();
-	while s2.head().height != s2.header_head().height || s2.head().height < 20 {
-		thread::sleep(time::Duration::from_millis(1000));
-	}
-	let _h2 = s2.chain.get_header_by_height(1).unwrap();
 
+	let s2 = servers::Server::new(conf).unwrap();
+	while s2.head().height < 21 {
+		thread::sleep(time::Duration::from_millis(1_000));
+	}
+
+	// Get the current header from s1.
+	let s1_header = s1.chain.head_header().unwrap();
+
+	// Wait for s2 to sync up to and including the header from s1.
+	while s2.head().height < s1_header.height {
+		thread::sleep(time::Duration::from_millis(1_000));
+	}
+
+	// Confirm both s1 and s2 see a consistent header at that height.
+	let s2_header = s2.chain.get_block_header(&s1_header.hash()).unwrap();
+	assert_eq!(s1_header, s2_header);
+
+	// Stop our servers cleanly.
 	s1.stop();
 	s2.stop();
 }
diff --git a/servers/tests/stratum.rs b/servers/tests/stratum.rs
index 0b9b4b98d..31c4d75d5 100644
--- a/servers/tests/stratum.rs
+++ b/servers/tests/stratum.rs
@@ -87,7 +87,7 @@ fn basic_stratum_server() {
 	workers.remove(4);
 
 	// Swallow the genesis block
-	thread::sleep(time::Duration::from_secs(1)); // Wait for the server to broadcast
+	thread::sleep(time::Duration::from_secs(5)); // Wait for the server to broadcast
 	let mut response = String::new();
 	for n in 0..workers.len() {
 		let _result = workers[n].read_line(&mut response);
diff --git a/src/bin/tui/status.rs b/src/bin/tui/status.rs
index c7195e0a1..6d02afbe2 100644
--- a/src/bin/tui/status.rs
+++ b/src/bin/tui/status.rs
@@ -81,6 +81,7 @@ impl TUIStatusListener for TUIStatusView {
 				"Waiting for peers".to_string()
 			} else {
 				match stats.sync_status {
+					SyncStatus::Initial => "Initializing".to_string(),
 					SyncStatus::NoSync => "Running".to_string(),
 					SyncStatus::HeaderSync {
 						current_height,
diff --git a/store/src/leaf_set.rs b/store/src/leaf_set.rs
index de341a796..03cdcc66f 100644
--- a/store/src/leaf_set.rs
+++ b/store/src/leaf_set.rs
@@ -102,14 +102,20 @@ impl LeafSet {
 	pub fn removed_pre_cutoff(
 		&self,
 		cutoff_pos: u64,
-		rewind_add_pos: &Bitmap,
 		rewind_rm_pos: &Bitmap,
 		prune_list: &PruneList,
 	) -> Bitmap {
 		let mut bitmap = self.bitmap.clone();
 
-		// Now "rewind" using the rewind_add_pos and rewind_rm_pos bitmaps passed in.
+		// First remove pos from leaf_set that were
+		// added after the point we are rewinding to.
+		let marker_from = cutoff_pos;
+		let marker_to = self.bitmap.maximum() as u64;
+		let rewind_add_pos: Bitmap = ((marker_from + 1)..=marker_to).map(|x| x as u32).collect();
 		bitmap.andnot_inplace(&rewind_add_pos);
+
+		// Then add back output pos to the leaf_set
+		// that were removed.
 		bitmap.or_inplace(&rewind_rm_pos);
 
 		// Invert bitmap for the leaf pos and return the resulting bitmap.
@@ -119,10 +125,16 @@ impl LeafSet {
 	}
 
 	/// Rewinds the leaf_set back to a previous state.
-	pub fn rewind(&mut self, rewind_add_pos: &Bitmap, rewind_rm_pos: &Bitmap) {
+	/// Removes all pos after the cutoff.
+	/// Adds back all pos in rewind_rm_pos.
+	pub fn rewind(&mut self, cutoff_pos: u64, rewind_rm_pos: &Bitmap) {
 		// First remove pos from leaf_set that were
 		// added after the point we are rewinding to.
+		let marker_from = cutoff_pos;
+		let marker_to = self.bitmap.maximum() as u64;
+		let rewind_add_pos: Bitmap = ((marker_from + 1)..=marker_to).map(|x| x as u32).collect();
 		self.bitmap.andnot_inplace(&rewind_add_pos);
+
 		// Then add back output pos to the leaf_set
 		// that were removed.
 		self.bitmap.or_inplace(&rewind_rm_pos);
diff --git a/store/src/pmmr.rs b/store/src/pmmr.rs
index fdc65baf5..caacc4ad8 100644
--- a/store/src/pmmr.rs
+++ b/store/src/pmmr.rs
@@ -152,12 +152,11 @@ where
 	fn rewind(
 		&mut self,
 		position: u64,
-		rewind_add_pos: &Bitmap,
 		rewind_rm_pos: &Bitmap,
 	) -> Result<(), String> {
 		// First rewind the leaf_set with the necessary added and removed positions.
 		if self.prunable {
-			self.leaf_set.rewind(rewind_add_pos, rewind_rm_pos);
+			self.leaf_set.rewind(position, rewind_rm_pos);
 		}
 
 		// Rewind the hash file accounting for pruned/compacted pos
@@ -328,7 +327,6 @@ where
 	pub fn check_compact<P>(
 		&mut self,
 		cutoff_pos: u64,
-		rewind_add_pos: &Bitmap,
 		rewind_rm_pos: &Bitmap,
 		prune_cb: P,
 	) -> io::Result<bool>
@@ -343,7 +341,7 @@ where
 
 		// Calculate the sets of leaf positions and node positions to remove based
 		// on the cutoff_pos provided.
-		let (leaves_removed, pos_to_rm) = self.pos_to_rm(cutoff_pos, rewind_add_pos, rewind_rm_pos);
+		let (leaves_removed, pos_to_rm) = self.pos_to_rm(cutoff_pos, rewind_rm_pos);
 
 		// 1. Save compact copy of the hash file, skipping removed data.
 		{
@@ -418,14 +416,12 @@ where
 	fn pos_to_rm(
 		&self,
 		cutoff_pos: u64,
-		rewind_add_pos: &Bitmap,
 		rewind_rm_pos: &Bitmap,
 	) -> (Bitmap, Bitmap) {
 		let mut expanded = Bitmap::create();
 
 		let leaf_pos_to_rm = self.leaf_set.removed_pre_cutoff(
 			cutoff_pos,
-			rewind_add_pos,
 			rewind_rm_pos,
 			&self.prune_list,
 		);
@@ -467,4 +463,3 @@ fn removed_excl_roots(removed: Bitmap) -> Bitmap {
 		})
 		.collect()
 }
-
diff --git a/store/tests/pmmr.rs b/store/tests/pmmr.rs
index 8bba43ded..fe322e2b1 100644
--- a/store/tests/pmmr.rs
+++ b/store/tests/pmmr.rs
@@ -124,7 +124,7 @@ fn pmmr_compact_leaf_sibling() {
 
 	// aggressively compact the PMMR files
 	backend
-		.check_compact(1, &Bitmap::create(), &Bitmap::create(), &prune_noop)
+		.check_compact(1, &Bitmap::create(), &prune_noop)
 		.unwrap();
 
 	// check pos 1, 2, 3 are in the state we expect after compacting
@@ -182,7 +182,7 @@ fn pmmr_prune_compact() {
 
 	// compact
 	backend
-		.check_compact(2, &Bitmap::create(), &Bitmap::create(), &prune_noop)
+		.check_compact(2, &Bitmap::create(), &prune_noop)
 		.unwrap();
 
 	// recheck the root and stored data
@@ -228,7 +228,7 @@ fn pmmr_reload() {
 
 		// now check and compact the backend
 		backend
-			.check_compact(1, &Bitmap::create(), &Bitmap::create(), &prune_noop)
+			.check_compact(1, &Bitmap::create(), &prune_noop)
 			.unwrap();
 		backend.sync().unwrap();
 
@@ -241,7 +241,7 @@ fn pmmr_reload() {
 		backend.sync().unwrap();
 
 		backend
-			.check_compact(4, &Bitmap::create(), &Bitmap::create(), &prune_noop)
+			.check_compact(4, &Bitmap::create(), &prune_noop)
 			.unwrap();
 		backend.sync().unwrap();
 
@@ -340,7 +340,7 @@ fn pmmr_rewind() {
 
 	// and compact the MMR to remove the pruned elements
 	backend
-		.check_compact(6, &Bitmap::create(), &Bitmap::create(), &prune_noop)
+		.check_compact(6, &Bitmap::create(), &prune_noop)
 		.unwrap();
 	backend.sync().unwrap();
 
@@ -354,7 +354,7 @@ fn pmmr_rewind() {
 	// rewind and check the roots still match
 	{
 		let mut pmmr: PMMR<TestElem, _> = PMMR::at(&mut backend, mmr_size);
-		pmmr.rewind(9, &Bitmap::of(&vec![11, 12, 16]), &Bitmap::create())
+		pmmr.rewind(9, &Bitmap::of(&vec![11, 12, 16]))
 			.unwrap();
 		assert_eq!(pmmr.unpruned_size(), 10);
 
@@ -399,7 +399,7 @@ fn pmmr_rewind() {
 
 	{
 		let mut pmmr: PMMR<TestElem, _> = PMMR::at(&mut backend, 10);
-		pmmr.rewind(5, &Bitmap::create(), &Bitmap::create())
+		pmmr.rewind(5, &Bitmap::create())
 			.unwrap();
 		assert_eq!(pmmr.root(), root1);
 	}
@@ -440,7 +440,7 @@ fn pmmr_compact_single_leaves() {
 
 	// compact
 	backend
-		.check_compact(2, &Bitmap::create(), &Bitmap::create(), &prune_noop)
+		.check_compact(2, &Bitmap::create(), &prune_noop)
 		.unwrap();
 
 	{
@@ -453,7 +453,7 @@ fn pmmr_compact_single_leaves() {
 
 	// compact
 	backend
-		.check_compact(2, &Bitmap::create(), &Bitmap::create(), &prune_noop)
+		.check_compact(2, &Bitmap::create(), &prune_noop)
 		.unwrap();
 
 	teardown(data_dir);
@@ -484,7 +484,7 @@ fn pmmr_compact_entire_peak() {
 
 	// compact
 	backend
-		.check_compact(2, &Bitmap::create(), &Bitmap::create(), &prune_noop)
+		.check_compact(2, &Bitmap::create(), &prune_noop)
 		.unwrap();
 
 	// now check we have pruned up to and including the peak at pos 7
@@ -557,7 +557,7 @@ fn pmmr_compact_horizon() {
 
 		// compact
 		backend
-			.check_compact(4, &Bitmap::create(), &Bitmap::of(&vec![1, 2]), &prune_noop)
+			.check_compact(4, &Bitmap::of(&vec![1, 2]), &prune_noop)
 			.unwrap();
 		backend.sync().unwrap();
 
@@ -612,7 +612,7 @@ fn pmmr_compact_horizon() {
 
 		// compact some more
 		backend
-			.check_compact(9, &Bitmap::create(), &Bitmap::create(), &prune_noop)
+			.check_compact(9, &Bitmap::create(), &prune_noop)
 			.unwrap();
 	}
 
@@ -675,7 +675,7 @@ fn compact_twice() {
 
 	// compact
 	backend
-		.check_compact(2, &Bitmap::create(), &Bitmap::create(), &prune_noop)
+		.check_compact(2, &Bitmap::create(), &prune_noop)
 		.unwrap();
 
 	// recheck the root and stored data
@@ -704,7 +704,7 @@ fn compact_twice() {
 
 	// compact
 	backend
-		.check_compact(2, &Bitmap::create(), &Bitmap::create(), &prune_noop)
+		.check_compact(2, &Bitmap::create(), &prune_noop)
 		.unwrap();
 
 	// recheck the root and stored data
diff --git a/store/tests/utxo_set_perf.rs b/store/tests/utxo_set_perf.rs
index f36a9bbb0..9d2e566dc 100644
--- a/store/tests/utxo_set_perf.rs
+++ b/store/tests/utxo_set_perf.rs
@@ -83,7 +83,7 @@ fn test_leaf_set_performance() {
 		let from_pos = x * 1_000 + 1;
 		let to_pos = from_pos + 1_000;
 		let bitmap: Bitmap = (from_pos..to_pos).collect();
-		leaf_set.rewind(&Bitmap::create(), &bitmap);
+		leaf_set.rewind(1_000_000, &bitmap);
 	}
 	assert_eq!(leaf_set.len(), 1_000_000);
 	println!(