test: Remove option to make TestChain100Setup non-deterministic #21592

1. 
   MarcoFalke commented at 9:40 am on April 4, 2021: member

   Seems odd to have an option for non-deterministic tests when the goal should be for all tests to be deterministic.
2. fanquake added the label Tests on Apr 4, 2021
3. 
   practicalswift commented at 9:31 pm on April 4, 2021: contributor

   Concept ACK: tests should be deterministic where possible
4. 
   jamesob commented at 6:15 pm on April 5, 2021: member

   Looks good to me - I can’t figure out why the regtest assumeutxo hash changed though… any idea? Also not sure why https://github.com/bitcoin/bitcoin/pull/21592/commits/faf96ef5132fe062af8c02b892109f53904e2a0a is in here (though I’m ACK on that as well, and don’t mind including it).
5. 
   MarcoFalke commented at 6:31 pm on April 5, 2021: member

   The assumeutxo hash includes the blockheader hash, which again includes the merkle root, which again includes all tx hashes, which again include all scriptPubkeys, which are different when compressed pubkeys are used (see the first commit)
6. 
   jamesob commented at 6:40 pm on April 5, 2021: member

   The assumeutxo hash includes the blockheader hash, which again includes the merkle root, which again includes all tx hashes, which again include all scriptPubkeys, which are different when compressed pubkeys are used (see the first commit)

   Ah, missed the bool flip here: https://github.com/bitcoin/bitcoin/pull/21592/commits/faab43d2eab9d6831eb6bcd861473e9fbeb4c908#diff-6a8ef76c60f30a6ca67d9f0e478fd02989c4b7fbc4c3116f80e13d873d5775e6R216

7. MarcoFalke force-pushed on Apr 6, 2021
8. 
   MarcoFalke commented at 6:36 am on April 6, 2021: member

   Also not sure why faf96ef is in here

   Thanks, force pushed to remove third commit.

9. 
   theStack commented at 10:58 pm on April 6, 2021: member

   Concept ACK
10. MarcoFalke force-pushed on Apr 7, 2021
11. 
    MarcoFalke commented at 8:26 am on April 7, 2021: member

    Force pushed to add a commit to remove even more code
12. in src/test/validation_tests.cpp:143 in fa7de46b9c outdated

    142 
    143     const auto out210 = *ExpectedAssumeutxo(210, *params);
    144-    BOOST_CHECK_EQUAL(out210.hash_serialized, uint256S("9c5ed99ef98544b34f8920b6d1802f72ac28ae6e2bd2bd4c316ff10c230df3f2"));
    145-    BOOST_CHECK_EQUAL(out210.nChainTx, (unsigned int)210);
    146+    BOOST_CHECK(uint256{out210.hash_serialized} != uint256::ZERO);
    147+    BOOST_CHECK(out210.nChainTx != unsigned{0});
    

    ---

    ---

    

    jamesob commented at 1:32 pm on April 7, 2021:

    Where else are we verifying that these values come through unmangled? I don’t like the idea of removing these checks just because they’re annoying every once in a while if we’re not verifying this behavior elsewhere.
13. 
    jamesob commented at 1:34 pm on April 7, 2021: member

    IMO you should stick to the spirit of the title of this PR and remove the first commit; removing the chance of non-determinism from tests is a good change, but I don’t see why we should make the assumeutxo tests more permissive just because it removes a little bit more code.
14. test: Use compressed keys in TestChain100Setup

    coinbaseKey.MakeNewKey(true); creates a compressed key and there is no reason
    for the deterministic setup to use uncompressed ones.

    fa732bccb3
15. test: Remove option to make TestChain100Setup non-deterministic

    Seems odd to have an option for non-deterministic tests
    when the goal should be for all tests to be deterministic.
    
    Can be reviewed with `--ignore-all-space`.

    fa6183d776
16. MarcoFalke force-pushed on Apr 8, 2021
17. 
    MarcoFalke commented at 7:03 am on April 8, 2021: member

    Reverted to initial changes for now.
18. 
    jamesob commented at 8:10 pm on April 8, 2021: member

    ACK https://github.com/bitcoin/bitcoin/pull/21592/commits/fa6183d7761eef8bb815aa888f0396e92e275f05
19. 
    practicalswift commented at 5:42 am on April 9, 2021: contributor

    cr ACK fa6183d7761eef8bb815aa888f0396e92e275f05: patch looks deterministic!
20. MarcoFalke merged this on Apr 9, 2021
21. MarcoFalke closed this on Apr 9, 2021

    ---

22. MarcoFalke deleted the branch on Apr 9, 2021
23. fanquake locked this on Apr 9, 2021

Contributors
MarcoFalke practicalswift jamesob theStack

Labels
Tests