Test of BIP9 fork activation of mtp, csv, sequence_lock

[NicolasDorier]

#6, while adding lots of tests relating to the implementation of each BIP, removed the old tests which were created for testing the SF activation logic.

I'm not comfortable with the new test which is hard to read, so I expected to keep the old one.
I removed code duplication.

As an added bonus, next SF activation test will be easily created.

[morcos]

@NicolasDorier can you take a look at the test in 7648 again. It was built off of the the original RPC tests in that pull, which I think contain similar tests for the activation logic that you have in this test. Is there anything in particular that you think is missing?

[NicolasDorier]

I reviewed your test, and it is great. My only complain is that it test more (and thus harder to read) than the narrow case of testing proper activation of a soft fork via BIP9.

Your test is a shotgun testing csv bips, this one is chirurgical and only catch codepaths added by bitcoin#7648, it is not meant to test csv bips which have already their test in their respective PRs.

Moreover, with my test, when we'll want to test segwit activation for example, it is trivial to do it by just adding a line at https://github.com/btcdrak/bitcoin/pull/8/files#diff-98a8abf7f80dbe5eda93bbbbb4348e80R192

Thus I'd like both of them.

[morcos]

My thought is that it's not necessary to have multiple tests testing the soft fork activation logic. For instance there are 2 bip65 tests, both of which test the ISM logic, which is already well tested, and none which actually test the bip65 logic itself. I think that accident was more likely to happen because people saw there were bip65 rpc tests and didn't notice they were essentially just repeats of the ISM testing.

I do agree it could be nicer to have the bip9 activation logic as its own test , but then it should only be tested once, and not be repeated for future activations. In that case we could slightly simplify the bip68-112-113 rpc test to remove the various tests around activation.

I'd like to avoid having too many unmaintained and repetitive RPC tests which both become a time sink and make it more difficult to figure out what we are actually testing. That's why I wanted to replace the repeated tests of BIP 9 logic which obscured the fact that we weren't testing BIP's 68,112, or 113.

BTW, there is already an RPC test for segwit, but we do need to flush out a bigger P2P one, I think @sdaftuar is working on that.

[NicolasDorier]

I do agree it could be nicer to have the bip9 activation logic as its own test , but then it should only be tested once, and not be repeated for future activations. In that case we could slightly simplify the bip68-112-113 rpc test to remove the various tests around activation.

I agree, but frankly I don't think it is worth the effort to change the bip68-112-113 rpc tests now. It does not hurt to have redundancy in the tests, those are rarely changed.

Think about the time saved for future soft fork: There would be two kinds of tests, the one which checks that the ScriptVerify flags act as expected on all weird case (like the current tests on segwit) and the other would just test the ISM boundary in 5 lines of code in bip9-softforks.py test file.

I can work on BIP9 segwit activation later this week on top of this test to show how easy it is to check BIP9 activation with it.
Basically segwit is already very much tested already. Adding the BIP9 activation test should be easy and not requiring retesting everything again.

The less important reason I'd like to keep those tests is that I reviewed, wrote, ran and debugged them a lot more (with @btcdrak) than your test.

[morcos]

The point I'm trying to make is its not necessary to test the activation for each soft fork. (I assume you mean bip9 and not ISM)

This has been a point of irritation for me because I kept pointing out that BIP 112 at least lacked any RPC testing and more and more copies of BIP 9 testing kept showing up.

In any case, I'm tired of arguing about it. As long as it's only one additional test specifically for bip9 soft fork activation, I don't object, even if it is redundant.

Sent from my iPhone

On Mar 20, 2016, at 1:06 PM, Nicolas Dorier notifications@github.com wrote:

I do agree it could be nicer to have the bip9 activation logic as its own test , but then it should only be tested once, and not be repeated for future activations. In that case we could slightly simplify the bip68-112-113 rpc test to remove the various tests around activation.

I agree, but frankly I don't think it is worth the effort to change the bip68-112-113 rpc tests now. It does not hurt to have redundancy in the tests, those are rarely changed.

Think about the time saved for future soft fork: There would be two kinds of tests, the one who try the ScriptVerify flags specifically as current tests on segwit and testing the ISM boundary would only be a matter of adding 5 lines of code in bip9-softforks.py test file.

I can work on BIP9 segwit activation later this week on top of this test to show how easy it is to check ISM testing with it.
Basically segwit is already very much tested already. Adding the BIP9 activation test should be easy and not requiring retesting everything again.

The less important reason I'd like to keep those tests is that I reviewed, wrote, ran and debugged them a lot more (with @btcdrak) than your test.

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub

[NicolasDorier]

This has been a point of irritation for me because I kept pointing out that BIP 112 at least lacked any RPC testing and more and more copies of BIP 9 testing kept showing up.

I'm not trying to irritate you, just trying to understand a bit why it is problematic. BIP112 and BIP68 were extensively tested already on bitcoin#7524 and bitcoin#7184

The test you made, as far as I see, does not go through new code path that were previously untested. (testing the same thing through RPC does not increase the test code coverage)
BIP68_112_113Test bundle at the same time all what was already tested before + BIP9 activation threshold, that the tests we did covered.

I think tests of BIPs and test of activation should be separated into different tests. But removing BIP68_112_113Test, even if there is test duplication with the previous BIP's PR tests, and the bip9-softforks.py is a waste. And it is only a test, there is no need to bikeshed on it.

I'm surprised you are so irritated of having overlapping test, all those tests increase confidence in the code, and most test don't need to be touched at all once done correctly.
I thought what bothered you when you removed the old tests was the code duplication between the 3 bip activation tests, which is why I fixed it and proposed this PR.

It's better to have overlapped test as we test the code through different point of view.
I think, you are overreacting on the maintainance burden.

[morcos]

I didn't mean that you had been irritating me. I was just trying to explain why I was a bit exhausted by this subject a priori.

Some things aren't tested well with the unit tests. For instance BIP 68 has consensus critical calculation of prevheights that needs to be separately tested in consensus code. I'm not sure BIP 112 or 113 had any RPC/P2P tests at all, and I think they were important for being comfortable with the testing coverage. It's the same logic by which both of us think we should have regression tests for BIP 9 activation even though that is extensively tested in unit tests.

But you are right, my biggest objection was having 3 BIP 9 activation tests. One is good.

[btcdrak]

Ok guys, I've cherry-picked it.