valuePools (sprout, sapling) monitoring / calculation works incorrectly

[DeckerSU]

Ever since we started using our own versioning system, which is similar to 0.7.x.y, instead of the default ZCash versioning 1.x.y.z (meaning our release has lower versions than the initial), it appears that we have encountered issues with monitoring the valuepools.

define(_CLIENT_VERSION_MAJOR, 0)
define(_CLIENT_VERSION_MINOR, 8)
define(_CLIENT_VERSION_REVISION, 1)
define(_CLIENT_VERSION_BUILD, 1)
...
static const int CLIENT_VERSION =
                           1000000 * CLIENT_VERSION_MAJOR
                         +   10000 * CLIENT_VERSION_MINOR
                         +     100 * CLIENT_VERSION_REVISION
                         +       1 * CLIENT_VERSION_BUILD;

For example, version 0.8.1.1 resulting in nVersion == 80101. And here:

komodo/src/chain.h

Lines 446 to 456 in d9144d0

	// Only read/write nSproutValue if the client version used to create
	// this index was storing them.
	if ((s.GetType() & SER_DISK) && (nVersion >= SPROUT_VALUE_VERSION)) {
	READWRITE(nSproutValue);
	}
	// Only read/write nSaplingValue if the client version used to create
	// this index was storing them.
	if ((s.GetType() & SER_DISK) && (nVersion >= SAPLING_VALUE_VERSION)) {
	READWRITE(nSaplingValue);
	}

We lost read/write of nSproutValue and nSaplingValue for the CDiskBlockIndex, as the values of SPROUT_VALUE_VERSION and SAPLING_VALUE_VERSION is higher than current actual version:

komodo/src/chain.h

Lines 40 to 41 in d9144d0

	static const int SPROUT_VALUE_VERSION = 1001400;
	static const int SAPLING_VALUE_VERSION = 1010100;

As a result this code for updating nChainSproutValue and nChainSaplingValue don't work, because pindex->nSproutValue and pindex->nSaplingValue are boost::none (no read from index):

komodo/src/main.cpp

Lines 6041 to 6094 in d9144d0

	for(const auto& item : vSortedByHeight)
	{
	CBlockIndex* pindex = item.second;
	pindex->nChainWork = (pindex->pprev ? pindex->pprev->nChainWork : 0) + GetBlockProof(*pindex);
	// We can link the chain of blocks for which we've received transactions at some point.
	// Pruned nodes may have deleted the block.
	if (pindex->nTx > 0) {
	if (pindex->pprev) {
	if (pindex->pprev->nChainTx) {
	pindex->nChainTx = pindex->pprev->nChainTx + pindex->nTx;
	if (pindex->pprev->nChainSproutValue && pindex->nSproutValue) {
	pindex->nChainSproutValue = *pindex->pprev->nChainSproutValue + *pindex->nSproutValue;
	} else {
	pindex->nChainSproutValue = boost::none;
	}
	if (pindex->pprev->nChainSaplingValue) {
	pindex->nChainSaplingValue = *pindex->pprev->nChainSaplingValue + pindex->nSaplingValue;
	} else {
	pindex->nChainSaplingValue = boost::none;
	}
	} else {
	pindex->nChainTx = 0;
	pindex->nChainSproutValue = boost::none;
	pindex->nChainSaplingValue = boost::none;
	mapBlocksUnlinked.insert(std::make_pair(pindex->pprev, pindex));
	}
	} else {
	pindex->nChainTx = pindex->nTx;
	pindex->nChainSproutValue = pindex->nSproutValue;
	pindex->nChainSaplingValue = pindex->nSaplingValue;
	}
	}
	// Construct in-memory chain of branch IDs.
	// Relies on invariant: a block that does not activate a network upgrade
	// will always be valid under the same consensus rules as its parent.
	// Genesis block has a branch ID of zero by definition, but has no
	// validity status because it is side-loaded into a fresh chain.
	// Activation blocks will have branch IDs set (read from disk).
	if (pindex->pprev) {
	if (pindex->IsValid(BLOCK_VALID_CONSENSUS) && !pindex->nCachedBranchId) {
	pindex->nCachedBranchId = pindex->pprev->nCachedBranchId;
	}
	} else {
	pindex->nCachedBranchId = SPROUT_BRANCH_ID;
	}
	if (pindex->IsValid(BLOCK_VALID_TRANSACTIONS) && (pindex->nChainTx || pindex->pprev == NULL))
	setBlockIndexCandidates.insert(pindex);
	if (pindex->nStatus & BLOCK_FAILED_MASK && (!pindexBestInvalid || pindex->nChainWork > pindexBestInvalid->nChainWork))
	pindexBestInvalid = pindex;
	if (pindex->pprev)
	pindex->BuildSkip();
	if (pindex->IsValid(BLOCK_VALID_TREE) && (pindexBestHeader == NULL || CBlockIndexWorkComparator()(pindexBestHeader, pindex)))
	pindexBestHeader = pindex;
	}

As a result getblockchaininfo RPC output looks like this:

  "valuePools": [
    {
      "id": "sprout",
      "monitored": false
    },
    {
      "id": "sapling",
      "monitored": true,
      "chainValue": 0.00000000,
      "chainValueZat": 0
    }
  ],

The offer is to change SPROUT_VALUE_VERSION and SAPLING_VALUE_VERSION to 80102 (0.8.1-beta3) in KomodoOcean and komodod simultaneously, otherwise the bootstraps between original komodod and KomodoOcean will be incompatible (of course, it assumes publishing releases 80102, 0.8.1-beta3)

Since monitoring value pools is not a critical issue, we can delay addressing it. However, if we decide to make changes now, both daemons in KomodoOcean and this repository should have the same version and the same read/write activation constants.

[DeckerSU]

Btw, jfyi, currently the value of pools in KMD chain are following:

[
  {
    "id": "sprout",
    "monitored": true,
    "chainValue": 84948.93692198,
    "chainValueZat": 8494893692198
  },
  {
    "id": "sapling",
    "monitored": true,
    "chainValue": 3.07618319,
    "chainValueZat": 307618319
  }
]

Since the KOMODO_SAPLING_DEADLINE on February 15, 2019, we have disabled the ability to create new sprout addresses and perform transfers to or from them. Additionally, all shielded transactions have been disabled on the public chain, as KMD itself is a public chain. These values should remain constant, allowing us to use them for verification purposes and more.