Page MenuHomePhabricator

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

Authored by schancel on Sat, Nov 17, 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 OK
Unit
No Unit Test Coverage
Build Status
Buildable 3948
Build 5968: Bitcoin ABC Teamcity Staging
Build 5967: arc lint + arc unit

Event Timeline

schancel created this revision.Sat, Nov 17, 05:18
Herald added a reviewer: Restricted Project. · View Herald TranscriptSat, Nov 17, 05:19
jasonbcox requested changes to this revision.Sat, Nov 17, 05:33
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
src/test/txvalidationcache_tests.cpp
209 ↗(On Diff #5850)

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.Sat, Nov 17, 05:33
schancel marked an inline comment as done.Sat, Nov 17, 05:43
schancel added inline comments.
src/test/txvalidationcache_tests.cpp
209 ↗(On Diff #5850)

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

schancel updated this revision to Diff 5851.Sat, Nov 17, 06:42

Ensure we're still checking CLEANSTACK enforcement

schancel updated this revision to Diff 5852.Sat, Nov 17, 06:56

Remove forgotten comment.

jasonbcox requested changes to this revision.Sat, Nov 17, 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.Sat, Nov 17, 08:07
deadalnix requested changes to this revision.Sat, Nov 17, 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 marked an inline comment as done.Sat, Nov 17, 20:08
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.

schancel updated this revision to Diff 5857.Sat, Nov 17, 20:09

Update some comments and move around initiatlization

schancel updated this revision to Diff 5858.Sat, Nov 17, 20:29

Add one more assertion

Fabien accepted this revision as: Fabien.Sat, Nov 17, 22:11
jasonbcox accepted this revision.Sun, Nov 18, 00:42
deadalnix requested changes to this revision.Sun, Nov 18, 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.Sun, Nov 18, 01:37
schancel marked 3 inline comments as done.Sun, Nov 18, 01:46
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.

deadalnix added inline comments.Sun, Nov 18, 01:57
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 updated this revision to Diff 5873.Sun, Nov 18, 02:11

Address feedback from @deadalnix

schancel marked 3 inline comments as done.Sun, Nov 18, 02:12
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.Sun, Nov 18, 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.Sun, Nov 18, 02:20
schancel updated this revision to Diff 5876.Sun, Nov 18, 04:20

Update once more based on feedback

deadalnix requested changes to this revision.Sun, Nov 18, 15:56
deadalnix added inline comments.
src/test/txvalidationcache_tests.cpp
252

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.Sun, Nov 18, 15:56
schancel abandoned this revision.Mon, Nov 19, 09:32