Bugfixes and new unit test.

SHAMap.cpp

@@ -33,7 +33,7 @@ std::stack<SHAMapTreeNode::pointer> SHAMap::getStack(const uint256& id, bool inc
 		uint256 hash=node->getChildHash(branch);
 		if(!hash) return stack;
 
-		node=getNode(node->getChildNodeID(branch), node->getChildHash(branch), false);
+		node=getNode(node->getChildNodeID(branch), hash, false);
 		if(!node)
 		{
 			if(mSynching) return stack;
@@ -210,6 +210,60 @@ SHAMapItem::pointer SHAMap::lastBelow(SHAMapTreeNode::pointer node)
 	} while(1);
 }
 
+SHAMapItem::pointer SHAMap::onlyBelow(SHAMapTreeNode::pointer node)
+{
+	// If there is only one item below this node, return it
+	bool found;
+	while(!node->isLeaf())
+	{
+		found=false;
+		SHAMapTreeNode::pointer nextNode;
+
+		for(int i=0; i<15; i++)
+			if(!node->isEmptyBranch(i))
+			{
+				if(found) return SHAMapItem::pointer(); // two leaves below
+				nextNode=getNode(node->getChildNodeID(i), node->getChildHash(i), false);
+				if(!nextNode) throw SHAMapException(MissingNode);
+				found=true;
+			}
+
+		if(!found)
+		{
+			assert(false);
+			return SHAMapItem::pointer();
+		}
+		node=nextNode;
+	}
+	assert(node->hasItem());
+	return node->peekItem();
+}
+
+void SHAMap::eraseChildren(SHAMapTreeNode::pointer node)
+{ // this node has only one item below it, erase its children
+	bool erase=false;
+	while(!node->isLeaf())
+	{
+		for(int i=0; i<16; i++)
+			if(!node->isEmptyBranch(i))
+			{
+				SHAMapTreeNode::pointer nextNode=getNode(node->getChildNodeID(i), node->getChildHash(i), false);
+				if(erase)
+				{
+					returnNode(node, true);
+					if(mTNByID.erase(*node))
+					assert(false);
+				}
+				erase=true;
+				node=nextNode;
+			}
+	}
+	returnNode(node, true);
+	if(mTNByID.erase(*node)==0)
+		assert(false);
+	return;
+}
+
 SHAMapItem::pointer SHAMap::peekFirstItem()
 {
 	boost::recursive_mutex::scoped_lock sl(mLock);
@@ -302,16 +356,18 @@ bool SHAMap::delItem(const uint256& id)
 { // delete the item with this ID
 	boost::recursive_mutex::scoped_lock sl(mLock);
 
-	std::stack<SHAMapTreeNode::pointer> stack=getStack(id, false);
+	std::stack<SHAMapTreeNode::pointer> stack=getStack(id, true);
 	if(stack.empty()) throw SHAMapException(MissingNode);
 
 	SHAMapTreeNode::pointer leaf=stack.top();
 	stack.pop();
 	if( !leaf || !leaf->hasItem() || (leaf->peekItem()->getTag()!=id) )
 		return false;
+
 	SHAMapTreeNode::TNType type=leaf->getType();
 	returnNode(leaf, true);
-	mTNByID.erase(*leaf);
+	if(mTNByID.erase(*leaf)==0)
+		assert(false);
 
 	uint256 prevHash;
 	while(!stack.empty())
@@ -319,25 +375,46 @@ bool SHAMap::delItem(const uint256& id)
 		SHAMapTreeNode::pointer node=stack.top();
 		stack.pop();
 		returnNode(node, true);
+		assert(node->isInner());
+
 		if(!node->setChildHash(node->selectBranch(id), prevHash))
+		{
+			assert(false);
 			return true;
-		if(!prevHash && !node->isRoot())
+		}
+		if(!node->isRoot())
 		{ // we may have made this a node with 1 or 0 children
 			int bc=node->getBranchCount();
 			if(bc==0)
 			{
+#ifdef DEBUG
+				std::cerr << "delItem makes empty node" << std::endl;
+#endif
 				prevHash=uint256();
-				mTNByID.erase(*node);
+				if(!mTNByID.erase(*node))
+					assert(false);
 			}
 			else if(bc==1)
 			{ // pull up on the thread
-				SHAMapItem::pointer item=firstBelow(node);
-				assert(item);
-				node->setItem(item, type);
+				SHAMapItem::pointer item=onlyBelow(node);
+				if(item)
+				{
+					eraseChildren(node);
+#ifdef ST_DEBUG
+					std::cerr << "Making item node " << node->getString() << std::endl;
+#endif
+					node->setItem(item, type);
+				}
 				prevHash=node->getNodeHash();
+				assert(!!prevHash);
+			}
+			else
+			{
+				prevHash=node->getNodeHash();
+				assert(!!prevHash);
 			}
 		}
-		else prevHash=node->getNodeHash();
+		else assert(stack.empty());
 	}
 	return true;
 }

SHAMap.h

@@ -231,6 +231,8 @@ protected:
 
 	SHAMapItem::pointer firstBelow(SHAMapTreeNode::pointer);
 	SHAMapItem::pointer lastBelow(SHAMapTreeNode::pointer);
+	SHAMapItem::pointer onlyBelow(SHAMapTreeNode::pointer);
+	void eraseChildren(SHAMapTreeNode::pointer);
 
 	bool walkBranch(SHAMapTreeNode::pointer node, SHAMapItem::pointer otherMapItem, bool isFirstMap,
 	    SHAMapDiff& differences, int& maxCount);

SHAMapNodes.cpp

@@ -279,14 +279,16 @@ bool SHAMapTreeNode::setItem(SHAMapItem::pointer& i, TNType type)
 
 SHAMapItem::pointer SHAMapTreeNode::getItem() const
 {
+	assert(isLeaf());
 	return boost::make_shared<SHAMapItem>(*mItem);
 }
 
 int SHAMapTreeNode::getBranchCount() const
 {
+	assert(isInner());
 	int ret=0;
-	for(int i=0; i<16; i++)
-		if(!mHashes[i]) ret++;
+	for(int i=0; i<16; ++i)
+		if(!!mHashes[i]) ++ret;
 	return ret;
 }
 

SHAMapSync.cpp

@@ -294,6 +294,55 @@ bool SHAMap::deepCompare(SHAMap& other)
 	return true;
 }
 
+#ifdef DEBUG
+#define SMS_DEBUG
+#endif
+
+static SHAMapItem::pointer makeRandomAS()
+{
+		Serializer s;
+		for(int d=0; d<8; d++)
+			s.add32(rand());
+		return boost::make_shared<SHAMapItem>(s.getRIPEMD160(), s.peekData());
+}
+
+static bool confuseMap(SHAMap &map, int count)
+{
+	// add a bunch of random states to a map, then remove them
+	// map should be the same
+	uint256 beforeHash=map.getHash();
+
+	std::list<uint256> items;
+
+	for(int i=0; i<count; i++)
+	{
+		SHAMapItem::pointer item=makeRandomAS();
+		items.push_back(item->getTag());
+		if(!map.addItem(*item, false))
+		{
+			std::cerr << "Unable to add item to map" << std::endl;
+			return false;
+		}
+	}
+
+	for(std::list<uint256>::iterator it=items.begin(); it!=items.end(); ++it)
+	{
+		if(!map.delItem(*it))
+		{
+			std::cerr << "Unable to remove item from map" << std::endl;
+			return false;
+		}
+	}
+
+	if(beforeHash!=map.getHash())
+	{
+		std::cerr << "Hashes do not match" << std::endl;
+		return false;
+	}
+
+	return true;
+}
+
 bool SHAMap::syncTest()
 {
 	unsigned int seed;
@@ -302,20 +351,14 @@ bool SHAMap::syncTest()
 
 	SHAMap source, destination;
 
+
 	// add random data to the source map
-	int items=rand()%20000;
+	int items=10000;
 	for(int i=0; i<items; i++)
-	{
-		Serializer s;
-		int dlen=rand()%30+10;
-		for(int d=0; d<dlen; d++)
-			s.add32(rand());
-		uint160 id=s.getRIPEMD160();
-		source.addItem(SHAMapItem(id, s.peekData()), false);
-#ifdef ST_DEBUG
-		std::cerr << "Item: " << id.GetHex() << std::endl;
-#endif
-	}
+		source.addItem(*makeRandomAS(), false);
+
+	if(!confuseMap(source, 350))
+		return false;
 
 	source.setImmutable();
 
@@ -366,10 +409,10 @@ bool SHAMap::syncTest()
 		hashes.clear();
 
 		// get the list of nodes we know we need
-		destination.getMissingNodes(nodeIDs, hashes, 512);
+		destination.getMissingNodes(nodeIDs, hashes, 2048);
 		if(!nodeIDs.size()) break;
 
-#ifdef DEBUG
+#ifdef SMS_DEBUG
 		std::cerr << nodeIDs.size() << " needed nodes" << std::endl;
 #endif
 		
@@ -392,7 +435,7 @@ bool SHAMap::syncTest()
 			return false;
 		}
 
-#ifdef DEBUG
+#ifdef SMS_DEBUG
 		std::cerr << gotNodeIDs.size() << " found nodes" << std::endl;
 #endif
 		for(nodeIDIterator=gotNodeIDs.begin(), rawNodeIterator=gotNodes.begin();
@@ -413,7 +456,7 @@ bool SHAMap::syncTest()
 	} while(1);
 	destination.clearSynching();
 
-#ifdef DEBUG
+#ifdef SMS_DEBUG
 	std::cerr << "SYNCHING COMPLETE " << items << " items, " << nodes << " nodes" << std::endl;
 #endif
 
@@ -424,7 +467,7 @@ bool SHAMap::syncTest()
 		return false;
 	}
 
-#ifdef DEBUG
+#ifdef SMS_DEBUG
 	std::cerr << "SHAMapSync test passed: " << items << " items, " <<
 		passes << " passes, " << nodes << " nodes" << std::endl;
 #endif