allow selection of correct coinbase maturity value based on height

[barrystyle]

782a607 allow selection of correct coinbase maturity value based on height

[Jongjan88]

Pr is working! Now 8.22 connected to the old testnet chain. And mining on it. https://testnetexplorer.digibyteservers.io/ also working.

[gto90]

Thank you for your contribution and glad to see you back my friend!

cACK

[saltedlolly]

Thank you @barrystyle for getting to the bottom of this issue after months of trying to understand it. It's great that the cause has been found before it affected mainnet - a potential disaster has been averted.

Question for the core devs - could we not have a unit test to check for this sort of issue so it does not happen again in future?

Also, I think we should take the RC1 and RC2 binaries down from GitHub given that they have introduced this issue. This will reduce the likelihood of people running them.

Once this and #154 are merged we are hopefully ready for RC3.

[beggsdl]

While this PR addressed a definite issue (thanks @barrystyle), further testing is showing that there are still some things that need to be resolved.

Here are two things I noticed in v8.22 (I tested with the patched version from @Jongjan88, with PR 154 and 157):

1. Coinbase TX will be confirmed after 8 blocks
2. Coinbase TX allowed to be spent before 100 blocks. TX then gets stuck in mempool.

Core wallet version 7.17.3 handled both of these correctly, not allowing a coinbase tx to be spent before 100 blocks.

I did this testing by solo mining to my core wallet. Here is a screenshot of the patched v8 wallet confirming a coinbase TX:

Here is that same coinbase TX in the 7.17.3 wallet:

The transaction detail screen in v8 says it needs to mature for 101 blocks before being spent, but you can see in the Status and Credit, that it is only confirming for 9.

This then affects the Immature and Available balances in the v8 wallet (top) compared to the v7 wallet (bottom):

I was then able to try to prematurely spend (before 100 confirmations) some of the coinbase TX from the v8 wallet (the v7 wallet did not allow me to do this due to insufficient funds):

This 50 DGB transaction is now stuck in mempool of the v8 wallet, even 17 hours after being created.

[saltedlolly]

Thank you @beggsdl. Excellent detective work!

[JaredTate]

Awesome you found this @barrystyle ! Thanks for the contribution. It looks like there might be a couple of other places where COINBASE_MATURITY_2 needs added properly to fix the issue as @beggsdl mentioned above.

txmempool.cpp needs an additional COINBASE_MATURITY_2 entry:

digibyte/src/txmempool.cpp

Lines 535 to 538 in 1c2c656

	unsigned int nMemPoolHeight = active_chainstate.m_chain.Tip()->nHeight + 1;
	if (coin.IsSpent() || (coin.IsCoinBase() && ((signed long)nMemPoolHeight) - coin.nHeight < COINBASE_MATURITY)) {
	txToRemove.insert(it);
	break;

Here is txmempool.cpp COINBASE_MATURITY_2 in 7.17.3:

digibyte/src/txmempool.cpp

Lines 522 to 529 in 258afac

	if (coin.IsSpent() || (coin.IsCoinBase() && ((signed long)nMemPoolHeight) - coin.nHeight < COINBASE_MATURITY)) {
	txToRemove.insert(it);
	break;
	}
	} else {
	if (coin.IsSpent() || (coin.IsCoinBase() && ((signed long)nMemPoolHeight) - coin.nHeight < COINBASE_MATURITY_2)) {
	txToRemove.insert(it);
	break;

Also in src/wallet/wallet.cpp looks like we need change to COINBASE_MATURITY_2 as well:

digibyte/src/wallet/wallet.cpp

Lines 2904 to 2911 in 1c2c656

	int CWalletTx::GetBlocksToMaturity() const
	{
	if (!IsCoinBase())
	return 0;
	int chain_depth = GetDepthInMainChain();
	assert(chain_depth >= 0); // coinbase tx should not be conflicted
	return std::max(0, (COINBASE_MATURITY+1) - chain_depth);
	}

Here is what 7.17.3 shows:

digibyte/src/wallet/wallet.cpp

Lines 4419 to 4426 in 258afac

	int CMerkleTx::GetBlocksToMaturity() const
	{
	if (!IsCoinBase())
	return 0;
	int chain_depth = GetDepthInMainChain();
	assert(chain_depth >= 0); // coinbase tx should not be conflicted
	return std::max(0, (COINBASE_MATURITY_2+1) - chain_depth);
	}

V7.17.3 mentions COINBASE_MATURITY_2 in these locations:

current 8.22 including this PR onlys shows COINBASE_MATURITY_2 in 3 places outside of tests:

I can add additions on to this PR @barrystyle or you can modify yours. Just let me know.

[beggsdl]

Thanks @JaredTate. These are the exact changes I was hoping to test, but have been having trouble doing a build and actually seeing the changes when I run the core wallet.

I also noticed this:

And here is what I found in tests so far. They for the most part use COINBASE_MATURITY (8) and COINBASE_MATURITY_ORIGINAL (100):

I can add others as I make note of them.

[Jongjan88]

@barrystyle @beggsdl @JaredTate Nice work!!

[saltedlolly]

Given that we have a lot of other fixes waiting on this one, I suggest we merge this as is. PRs for the remaining fixes can be made separately. Thank you again @barrystyle for identifying the issue. I am going to approve.

[saltedlolly]

utACK

[Jongjan88]

txmempool.cppa253bed

digibyte/src/txmempool.cpp

Lines 536 to 546 in a253bed

	if (coin.nHeight < Params().GetConsensus().multiAlgoDiffChangeTarget) {
	if (coin.IsSpent() || (coin.IsCoinBase() && ((signed long)nMemPoolHeight) - coin.nHeight < COINBASE_MATURITY)) {
	txToRemove.insert(it);
	break;
	}
	} else {
	if (coin.IsSpent() || (coin.IsCoinBase() && ((signed long)nMemPoolHeight) - coin.nHeight < COINBASE_MATURITY_2)) {
	txToRemove.insert(it);
	break;
	}
	}

wallet/wallet.cppf35de13

digibyte/src/wallet/wallet.cpp

Line 2910 in f35de13

return std::max(0, (COINBASE_MATURITY_2+1) - chain_depth);

Can we Merge this PR? Or first add this and then merge?

[ycagel]

Has there been sufficient testing on this PR before considering merging?

[beggsdl]

Has there been sufficient testing on this PR before considering merging?

Several of us have been running it since the fix was made. It did fix the issue of v8 going onto the longer chain with prematurely spend coins in one of the transactions. Without the txmempool.cpp and wallet.cpp fixes though, you can still attempt to prematurely spend coins from a coinbase transaction. That new tx then gets stuck in mempool. If possible, it would be nice to have these two fixes included so we can officially test all the COINBASE_MATURITY_2 fixes at the same time. COINBASE_MATURITY is also used in src/bench/block_assemble.cpp, but I am not sure if it really needs to be changed there or not.

[Jongjan88]

Has there been sufficient testing on this PR before considering merging?

Yes we tested it a lot. The pr is 100% working. And used by all v8 testnet nodes.

[ycagel]

cACK.

[j50ng]

cACK

[ycagel]

Please reference the following issue:

#161

The work to resolve that issue will be done in another PR.