As per title. Fix the checks failing due to cleanstack and ctor now
being mandatory in txvalidationcache_tests due to Magnetic Anomaly
activating.
Details
Details
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- fix-unit-tests
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 3926 Build 5924: Bitcoin ABC Buildbot (legacy) Build 5923: arc lint + arc unit
Event Timeline
| 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? |
| src/test/txvalidationcache_tests.cpp | ||
|---|---|---|
| 209 ↗ | (On Diff #5850) | Ah I deleted that comment once on the other branch... Also, no idea about DERSIG |
| src/test/txvalidationcache_tests.cpp | ||
|---|---|---|
| 222 | 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. | |
| src/test/txvalidationcache_tests.cpp | ||
|---|---|---|
| 209 | This explains what the test is testing. Making the test green for the sake of making the test green brings zero value. | |
| src/test/txvalidationcache_tests.cpp | ||
|---|---|---|
| 209 | I'm pretty sure this is still being tested. That's why I duplicated this entire block. It would be helpful if you elaborated. | |
| 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 | ||
|---|---|---|
| 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. |
Comment Actions
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.
| 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. |