Page MenuHomePhabricator

[qa] fix p2p-fullblocktest.py to support magnetic anomaly
AbandonedPublic

Authored by jasonbcox on Sun, Nov 18, 08:33.

Details

Reviewers
Fabien
schancel
deadalnix
Group Reviewers
Restricted Project
Summary

Per title, failures associated with the activation of CTOR, CLEANSTACK,
and a change in error message introduced in D2076

Test Plan
make check && ./test/functional/test_runner.py p2p-fullblocktest

Diff Detail

Repository
rABC Bitcoin ABC
Branch
arcpatch-D2085
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 3962
Build 5996: Bitcoin ABC Teamcity Staging
Build 5995: arc lint + arc unit

Event Timeline

schancel created this revision.Sun, Nov 18, 08:33
Herald added a reviewer: Restricted Project. · View Herald TranscriptSun, Nov 18, 08:33
schancel updated this revision to Diff 5880.Sun, Nov 18, 08:50

Fix last test

schancel retitled this revision from [qa] WIP fix p2p-fullblocktest.py to support magnetic anomaly to [qa] fix p2p-fullblocktest.py to support magnetic anomaly.Sun, Nov 18, 08:51
Fabien requested changes to this revision.Sun, Nov 18, 10:11
Fabien added a subscriber: Fabien.
Fabien added inline comments.
src/validation.cpp
3355 ↗(On Diff #5880)

bad-txns-duplicate is already raised on validation.cpp:3122, better change the error name to avoid confusion. From my understanding, the 2 errors are slightly different.

test/functional/test_framework/blocktools.py
31 ↗(On Diff #5880)

Good idea !

This revision now requires changes to proceed.Sun, Nov 18, 10:11
jasonbcox requested changes to this revision.Sun, Nov 18, 15:15
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
test/functional/p2p-fullblocktest.py
561 ↗(On Diff #5880)

numTxes -> numTxs

565 ↗(On Diff #5880)

can be simplified to for i in range(numTxs):

613 ↗(On Diff #5880)

Why the comment if it's accepted?

jasonbcox added inline comments.Sun, Nov 18, 16:25
src/validation.cpp
3355 ↗(On Diff #5880)

This doesn't look related to making the test green. Please focus on that first.

jasonbcox added inline comments.Sun, Nov 18, 16:32
test/functional/p2p-fullblocktest.py
565 ↗(On Diff #5880)

nvm

jasonbcox commandeered this revision.Sun, Nov 18, 16:33
jasonbcox edited reviewers, added: schancel; removed: jasonbcox.
jasonbcox updated this revision to Diff 5887.Sun, Nov 18, 17:05

Fixed according to feedback

jasonbcox marked an inline comment as done.Sun, Nov 18, 17:06
jasonbcox added inline comments.
test/functional/p2p-fullblocktest.py
1311 ↗(On Diff #5887)

This looks wrong, but passes. I don't know why yet.

deadalnix requested changes to this revision.Sun, Nov 18, 18:28
deadalnix added a subscriber: deadalnix.
deadalnix added inline comments.
test/functional/p2p-fullblocktest.py
561 ↗(On Diff #5887)

Ok this is the worst name ever. This is not necessarily creating a long chain, nor it is the goal at all, because we are testing sigops.

1041 ↗(On Diff #5887)

When you see things like this, you should move the comment on the previous line so that formatting isn't screwed up.

This revision now requires changes to proceed.Sun, Nov 18, 18:28
jasonbcox updated this revision to Diff 5894.Sun, Nov 18, 18:49

Feedback

deadalnix added inline comments.Sun, Nov 18, 19:02
test/functional/p2p-fullblocktest.py
608

This changes completely what this test is doing, why ?

793 ↗(On Diff #5887)

Revert

schancel added inline comments.Sun, Nov 18, 19:11
src/validation.cpp
3355 ↗(On Diff #5880)

This *is* a change in the error behavior. It changes the external API.

test/functional/p2p-fullblocktest.py
608

No. It does the same thing AFAICS. The old version was invalid because of LTOR changing transaction indices in the block. So, I change it to generate the same thing twice.

schancel added inline comments.Sun, Nov 18, 19:16
test/functional/p2p-fullblocktest.py
1311 ↗(On Diff #5887)

I didn't add this. Where did it come from?

613 ↗(On Diff #5880)

It was failing with that. I forgot to remove the comment.

jasonbcox abandoned this revision.Sun, Nov 18, 19:37

Superseded by D2088