BugFix: g_IsSaplingActive flag is not initialized at startup.

[furszy]

Small bug introduced in #1814, the flag is set on every new tip but not initialized at startup.
Discovered rebasing #1798 on top of master and running any sapling functional test. As g_IsSaplingActive is false at startup, the transaction hash is different, making the merkle tree hash different, failing in VerifyDB.

[random-zebra]

Nice catch.
Don't particularly like it inside LoadBlockIndexDB though.
I think it would be cleaner to do it during AppInit2: in step 7, after reading the block index, when we get the chain height and initialize the money supply (before verifying the blocks).

[furszy]

I think it would be cleaner to do it during AppInit2: in step 7, after reading the block index, when we get the chain height and initialize the money supply (before verifying the blocks).

I don't see it cleaner there. It wouldn't be following the same workflow in both scenarios (flag initialization and update).
As is now, in both scenarios, the flag is being set right after chainActive.SetTip with a reason. It's the only two places in the whole sources where chainTip gets updated (without counting UnloadBlockIndex).
So we can ensure that any new introduced code will be having the flag update (there by the proper tx serialization). If we move it to another place, there is a possibility of introducing changes in-between SetTip and the flag initialization that will not work properly.

[random-zebra]

I don't see it cleaner there.

It would have been cleaner in my opinion, as it would have been next to the initialization of other globals (depending on chainActive), and not directly inside a function called LoadBlockIndexDB (while not having anything to do with the block index DB itself, nor the action of loading it from disk).

Anyway, not a big deal of course... can stay as is if you prefer, but consider that we might want to introduce global atomic bools for other enforcement heights as well (zerocoin, zerocoinv2, v4, etc) in the future. Setting all of them in the same place (would remove the need to pass the height, or access chainActive, locking cs_main, in lots of functions).

[furszy]

Anyway, not a big deal of course... can stay as is if you prefer, but consider that we might want to introduce global atomic bools for other enforcement heights as well (zerocoin, zerocoinv2, v4, etc) in the future. Setting all of them in the same place (would remove the need to pass the height, or access chainActive, locking cs_main, in lots of functions).

I'm not sure why we would be implementing global flags for every upgrade, that would just make the code harder to unit test and read. All of that can continue using the network upgrade flow.

And btw, i'm actually not happy with this global flag (same as I'm not happy with any other global flag), but well, it's just a temporary solution to v5 upgrade due to how square is our serialization framework at the moment.

Moving forward (possibly after v5), we can decouple the serialization/unserialization methods from the templated serialization framework (upstream did it as well after introducing SegWit support) and introduce an arg flag to enable/disable v2 tx parsing from the caller side.

[furszy]

rebased on master, solved a conflict / non-conflict situation..

ready for review.

[random-zebra]

utACK 520d889

[Fuzzbawls]

utACK 520d889