test: Remove option to make TestChain100Setup non-deterministic #21592
Conversation
|
Concept ACK: tests should be deterministic where possible |
|
Looks good to me - I can't figure out why the regtest assumeutxo hash changed though... any idea? Also not sure why faf96ef is in here (though I'm ACK on that as well, and don't mind including it). |
|
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: faab43d#diff-6a8ef76c60f30a6ca67d9f0e478fd02989c4b7fbc4c3116f80e13d873d5775e6R216 |
maflcko
force-pushed
the
2104-testCleanup
branch
from
faf96ef to
fac9957
Compare
Thanks, force pushed to remove third commit. |
|
Concept ACK |
maflcko
force-pushed
the
2104-testCleanup
branch
from
fac9957 to
fa333f8
Compare
|
Force pushed to add a commit to remove even more code |
src/test/validation_tests.cpp
Outdated
There was a problem hiding this comment.
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.
|
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. |
coinbaseKey.MakeNewKey(true); creates a compressed key and there is no reason for the deterministic setup to use uncompressed ones.
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`.
maflcko
force-pushed
the
2104-testCleanup
branch
from
fa333f8 to
fa6183d
Compare
|
Reverted to initial changes for now. |
|
ACK fa6183d |
|
cr ACK fa6183d: patch looks deterministic! |
5 participants
Seems odd to have an option for non-deterministic tests
when the goal should be for all tests to be deterministic.