Spend coinbase error

[tearodactyl]

In reviewing debug.log ERROR messages for Zero 2.0.4 on Ubuntu 18.04, found a single instance of: 2019-04-16 11:02:13 ERROR: CheckInputs(): tried to spend coinbase at depth 719

Line 2049 in main.cpp, this may be innocuous, but if not, can pursue further. This is the testnet3, and I did not generated transactions on it.

[tearodactyl]

Re-running testnet3 synch entirely from scratch shows no such message, somewhat surprisingly.
Synch of the mainnet also does not demonstrate such a scenario.

Unrelated, but a block from Sept 2018 produced the following error message but apparently was accepted anyway:

2019-04-19 06:01:52 UpdateTip: new best=000000324f6390b27c55822ffb7c418d9120060773a7d397d4192ff5330cad9a  height=412279  log2_work=40.114032  tx=779026  date=2018-09-15 17:37:32 progress=1.000000  cache=180.7MiB(162037tx)
2019-04-19 06:01:52 ERROR: ContextualCheckBlock: founders reward missing
2019-04-19 06:01:52 ERROR: ProcessNewBlock: AcceptBlock FAILED
2019-04-19 06:01:52 Misbehaving: 213.239.212.246:23801 (0 -> 100) BAN THRESHOLD EXCEEDED
2019-04-19 06:01:52 UpdateTip: new best=0000005b23dec67871aaa1956ef0d8bfff8f459d4ef469552a1118d4a2193e7a  height=412280  log2_work=40.114041  tx=779028  date=2018-09-15 17:41:03 progress=1.000000  cache=180.7MiB(162036tx)

[CryptoForge]

This is interesting and I suspect misleading. The Founders address check doesn't apply to blocks below 412300 so it is not required on the block in question. Lets increase the logging around this check and re-sync to see if we can reproduce the error. Let also check that peer and see what protocol version they are on.

[tearodactyl]

There is a bunch of corner cases during sync that Zcash has been trying to get right since last July and again in November and likely it is still not entirely correct.

Getting it right can be very important. Any possibility of a node rejecting or accepting an incoming block due to an ambiguity in accepting transactions within it, due to the stateful and yet asynchronous nature of the sync, with possible confusion about the 'era', may lead to a fork, Whether accidental or malicious, it could prevent recovery from an extended chain split.

Philosophically, a syncing node should not be passing any transactions, as it is clearly not equipped to validate them well against a double spend, which is the whole point, perhaps not even accept them into its mempool. Bitcoin style of P2P makes no guarantees regarding individual transaction. Whether a syncing node should be attempting to originate transactions, I'm uncertain, but would lean toward a NO, though that may impact 'user experience'. It is not ready to mine, so there may be a hint there.

Whether such a node should be passing blocks is unclear, as it cannot properly validate transactions included. Disabling passing of blocks here may get in the way of P2P information propagation during the restart of the chain. Seems that passing empty blocks is fine by this logic.

A frequent set of corner cases is due to the syncing node not knowing prior to getting to processing Sprout block height which 'era' it is running in right now. Since the Zcash logic hardcodes 'overwinter' and 'sapling', they will hit these issues again, with the upgrades coming after Sapling. I'm not convinced that this reasoning is correct, as there could have been a time when Sapling support was compiled in but prior to the fork, but that scenario is long past. A node compiled from any codebase from this year can assume a Sapling or later 'era' on Zcash 'mainnet' and no node with pre-Sapling behavior would successfully run on it. There may be a wrinkle with testnets used to exercise pre-fork states and fork transitions, but hopefully that can be accounted for.

Having issues with how the 'synced' state is determined in IsInitialBlockDownload(). There are separate fImporting and fReindex flags in play, however the state of those is maintained. The code plays some trick with latching the 'false' return value and locking, but I see no uplock. Exactly when latchToFalse.store() at the very end of the function is being hit, after checking a whole bunch of scenarios, resulting in return(false) or even true, is unclear and I cannot tell if it is hit at all, since the function can start returning false due to one of the earlier checks always hitting. If latching is appropriate and the reentrancy concern is a real, I'd use code likely to result in just a couple of instructions, like:

  static uint32 pastInitial = FALSE;
  if (pastInitial)
    return false;
.
.
.
  pastInitial = TRUE;
  return false;

Codebase has a confusing aborted attempt to pass to ContextualCheckTransaction() an optional fifth parameter (function pointer) named isInitBlockDownload(). It is initialized to IsInitialBlockDownload() in the declaration, then never used. Meanwhile IsInitialBlockDownload() is used directly in 12 places in main.cpp and 3 times in other .cpp files.

This needs digesting, some parts filed with Zcash, perhaps the rest captured in a separate Issue for Zero.

[CryptoForge]

213.239.212.246:23801 appears to be the same node referenced #11