Page MenuHomePhabricator

[qa] fix broken unit tests due to magnetic anomaly activating
AbandonedPublic

Authored by schancel on Nov 17 2018, 05:18.

Details

Reviewers
jasonbcox
deadalnix
Fabien
Group Reviewers
Restricted Project
Summary

As per title. Fix the checks failing due to cleanstack and ctor now
being mandatory in txvalidationcache_tests due to Magnetic Anomaly
activating.

Test Plan
make VERBOSE=1 check

Diff Detail

Repository
rABC Bitcoin ABC
Branch
fix-unit-tests
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 3924
Build 5920: Bitcoin ABC Buildbot (legacy)
Build 5919: arc lint + arc unit

Event Timeline

jasonbcox requested changes to this revision.Nov 17 2018, 05:33
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
src/test/txvalidationcache_tests.cpp
209

I think this comment needs updating to reflect the test being checked as valid.

Secondly, why is DERSIG relevant to this test?

This revision now requires changes to proceed.Nov 17 2018, 05:33
schancel added inline comments.
src/test/txvalidationcache_tests.cpp
209

Ah I deleted that comment once on the other branch... Also, no idea about DERSIG

Ensure we're still checking CLEANSTACK enforcement

Remove forgotten comment.

jasonbcox requested changes to this revision.Nov 17 2018, 08:07
jasonbcox added inline comments.
src/test/txvalidationcache_tests.cpp
222 ↗(On Diff #5852)

spend_tx -> dirtystack_tx

Please move the initialization block for spend_tx below this test block. It makes errors like this much easier to catch in the future.

This revision now requires changes to proceed.Nov 17 2018, 08:07
deadalnix requested changes to this revision.Nov 17 2018, 12:30
deadalnix added a subscriber: deadalnix.
deadalnix added inline comments.
src/test/txvalidationcache_tests.cpp
209 ↗(On Diff #5852)

This explains what the test is testing. Making the test green for the sake of making the test green brings zero value.

schancel added inline comments.
src/test/txvalidationcache_tests.cpp
209 ↗(On Diff #5852)

I'm pretty sure this is still being tested. That's why I duplicated this entire block. It would be helpful if you elaborated.

Update some comments and move around initiatlization

deadalnix requested changes to this revision.Nov 18 2018, 01:37
deadalnix added inline comments.
src/test/txvalidationcache_tests.cpp
192 ↗(On Diff #5864)

Why is this required here now ?

208 ↗(On Diff #5864)

Keep comment

208 ↗(On Diff #5864)

This seems that the LOCK should be here.

273 ↗(On Diff #5864)

Why is the test case pretty much duplicated ?

This revision now requires changes to proceed.Nov 18 2018, 01:37
schancel added inline comments.
src/test/txvalidationcache_tests.cpp
192 ↗(On Diff #5864)

It's moved from below. It's locked for all the calls that follow here. There only some initialization that's not necessary before it is.

There's a whole string of like 10 different things that require this lock.

208 ↗(On Diff #5864)

The comment is moved up to the beginning of this block. The comment was changed to match my understanding. We can't check the difference between one set of flags and consensus flags due to them all being consensus now?

273 ↗(On Diff #5864)

Because I need a transaction that doesn't violate cleanstack and is spent in order to run the tests below here.

src/test/txvalidationcache_tests.cpp
192 ↗(On Diff #5864)

I don't think any of the item between here and where it was from before require cs_main.

208 ↗(On Diff #5864)

Keep comment close to where the test happen, which is here.

schancel added inline comments.
src/test/txvalidationcache_tests.cpp
194 ↗(On Diff #5864)

@deadalnix The comment you are worried about is now *here*.

208 ↗(On Diff #5864)

It no longer happens here. It happens in the block above.

208 ↗(On Diff #5864)

Move to here, and in the same place in every below block.

deadalnix requested changes to this revision.Nov 18 2018, 02:20

Ok, you prepare a transaction, you don't need the lock for this and this is not what is tested, just necessary wiring for the test. Move the comment where the test is, not where the wiring is. The test with the block at the end do not do the same anymore - the transaction being included was invalid under rules that are not yet enforced as consensus, which ensure that we don't get cache poisoning.

If ValidateCheckInputsForAllFlags require the lock, then lock in there.

This revision now requires changes to proceed.Nov 18 2018, 02:20

Update once more based on feedback

deadalnix requested changes to this revision.Nov 18 2018, 15:56
deadalnix added inline comments.
src/test/txvalidationcache_tests.cpp
252 ↗(On Diff #5877)

This still doesn't test what it used to. This unit test is not there to check the CLEANSTACK behavior, but the cache behavior. We used to check that an invalid tx with a set of flag will not invalidate block acceptance given it is valid with consensus enforced flags.

This revision now requires changes to proceed.Nov 18 2018, 15:56