Use confirmations instead of time to decide if tx is "safe" for mining

[UdjinM6]

This should ensure that all nodes arrive at the same decision most of the time.

[codablock]

This basically results in blocks to never get ChainLocked until 6 confirmations when just a single unsafe TX is included. It also makes the whole tracking of firstSeenTime/firstSeenHeight unnecessary, as every TX in a new block has 1 confirmation, no matter when it was seen first.

The idea with the firstSeen time-tracking was that individual nodes would consider a TX safe after not seeing a conflicting IS lock for X minutes. True, the result is that individual nodes will consider TXs to be safe at different times, but this is ok. In that case, one node after another will sign the block, resulting in the signing session to not timeout until no new sig share appears for one minute or after a total of 5 minutes. So, even if nodes sign at different times, the CLSIG should be created eventually.

[UdjinM6]

It also makes the whole tracking of firstSeenTime/firstSeenHeight unnecessary, as every TX in a new block has 1 confirmation, no matter when it was seen first.

It still tracks txes from mempool which are not mined in next few blocks. I think I see where confusion is coming from, new commits should make it a bit clearer I hope and also fix a potential race condition between TrySignChainTip/IsTxSafeForMining and Cleanup (which also exists in current code btw but is less obvious).

[codablock]

Ah ok, now I understand better. But this still leaves us with the problem that a freshly received block that contains a fresh unsafe TX will result in CLs not being signed for the next 4 blocks, no matter how much time passed since the block with the unsafe TX appeared.

In the current solution, if a TX appears in a block that is unknown, signing of CLs will continue after the timeout. So lets say block 5 had an unsafe TX, so no CLSIG. Then block 6 appears after 2.5 minutes, still no CLSIG (for none of the blocks). Then after some time, 7 and 8 appear and total time passed since block 5 reaches 10 minutes (or whatever timeout we choose later), now the current tip is considered safe immediately and all nodes sign the CL, even if block 5 did not reach 6 confirmations yet.

Maybe a hybrid solution works better? We track the block height at which the TX was seen first and then use that blocks time as basis to determine if a TX is known long enough until we consider it safe. Or we track the time of the block where the TX appeared to make things easier.

[UdjinM6]

In the current solution... and total time passed since block 5 reaches 10 minutes (or whatever timeout we choose later)

The problem is that by that time node might receive 6 (fast) blocks already and cleanup could wipe tx out of "seen" map and thus txAge will always be 0 (i.e. tx will always be "unsafe"). The issue is a fundamental difference between the expected/real time and actual blocks which is basically in the nature of the mining. With this PR the only "time" is blocks, so to say.

Maybe a hybrid solution works better? We track the block height at which the TX was seen first and then use that blocks time as basis to determine if a TX is known long enough until we consider it safe. Or we track the time of the block where the TX appeared to make things easier.

This doesn't solve the issue, see above. Also, can't rely on nTime of the block header (if I understand what you are proposing here correctly), it can vary a lot and miners will be able to trick IsTxSafe... to get their blocks locked even if no txes were seen/locked earlier.

[codablock]

The problem is that by that time node might receive 6 (fast) blocks already and cleanup could wipe tx out of "seen" map and thus txAge will always be 0 (i.e. tx will always be "unsafe"). The issue is a fundamental difference between the expected/real time and actual blocks which is basically in the nature of the mining. With this PR the only "time" is blocks, so to say.

In that case the TX will not be checked if its safe or not and the whole block is then assumed to be safe. Look at https://github.com/dashpay/dash/pull/2759/files#diff-80dfe69dedcffa54ffef1075c78ada96R275, which bails out from the whole checking. Only the last 6 (including the to be signed tip) blocks are checked here.

[UdjinM6]

@codablock Hmm.. ok, I stand corrected, this works for signing. But it doesn't work for mining (IsTxSafeForMining()) unless I'm missing smth there as well.

[codablock]

For mining, such a TX wouldn't be in the mempool anymore (as it got mined in a rapid chain). Can you maybe describe the original problem you tried to solve?

[UdjinM6]

Nvm, I figured out where I got it all wrong (Cleanup). Thanks :)