Can you explain why exactly the patching is needed in this case? It would definitely seem better to test against unpatched previous releases if possible.

@ryanofsky the regtest parameters were changed to activate SegWit at the genesis block, which causes nodes after #11389 to reject blocks created by v0.15.1 and older nodes (and vice versa). Maybe there's a less drastic way to change the regtest consensus params for these older releases?

Not sure, but is passing -vbparams=segwit:0:0 an option?

@Sjors I've spent some time looking into how feasible it would be to write functional tests that would run against older versions of our software, which I think would be great if we could pull off -- for instance, I thought it would be nice in something like the p2p-segwit.py test for us to use an actual old binary in the upgrade_after_activation test, rather than mocking it with a current binary by setting the bip9 parameters to 0.

However, in my experience this proved to be impractical. It seems like we periodically make changes to the RPC test framework infrastructure itself, in such ways that cause problems with backward compatibility. Here are a few examples I have run into, so you get a flavor of some of the issues:

• sync_blocks() was rewritten to use an rpc call, waitforblockheight, that was introduced in 0.14. So no test that uses sync_blocks can run on a pre-0.14 version of the code (which is basically every test we have!).

• Even if you solve that problem somehow (eg by rewriting sync_blocks, or avoiding it somehow), we also introduced in 0.14 support for RPC named arguments. This also breaks compatibility with pre-0.14 software (so without a patch, I believe no RPC calls work at all).

• I might have the details of this one slightly wrong, but I think we made a change at some point to support parsing numbers as strings, versus floats, and then I think our JSON library started to take advantage of that as well? Not sure if that's exactly right, but I remember having an issue running newer tests against older releases when using RPC calls involving numbers, and needing a patch for that.

So my takeaway from this exercise was that if we want to support running modern tests against older binaries, we need to change the way we do things, so that we (a) have a better split between the python test code, and the python utilities/RPC handlers/etc that allow us to talk to a node, and (b) come up with a way to use older python utility code when talking to older nodes.

So no test that uses sync_blocks can run on a pre-0.14 version of the code (which is basically every test we have!).

I don't understand why this would matter. I think the goal here is just to be able to write a small test that brings up a specific version of bitcoind (0.15) and tests a few things with it, not to be able to run large swathes of the test framework against arbitrary bitcoin releases. It doesn't seem it would require a lot of code to support this, and if the tests are run on travis it doesn't seem like whatever code is added could get broken easily.

@ryanofsky Perhaps I phrased that sentence poorly -- my point wasn't that we'd want to run all the tests against older code (I think that would be absurd), just that we take it for granted that we can call sync_blocks in all sorts of places, so to not have such a basic utility at your disposal can be surprising and hard to work around.

Another way to look at it: imagine you wanted to run a test against 0.15 now, so you write one and you can even use sync_blocks and everything works fine (I agree that this shouldn't be too hard, as 0.15 was very recent). Based on our history, I imagine there will come a time when someone wants to do something which will break the test, because some utility code -- like sync_blocks, or the way we make RPC calls, or something else -- will change, and the test will be busted, and we won't have a good way to fix it.

This seems possible for small, limited tests which don't use the full range of test framework capabilities (for example, if we just want to test downgrading to v0.15 to test segwit wallets). As Suhas has pointed out, there are lots of reasons why this is difficult in the general case, but I think we can work around them for a targeted test cases:

sync_blocks() was rewritten to use an rpc call, waitforblockheight, that was introduced in 0.14.

Not relevant if we only want to test as far back as v0.15, but we can add a waitforblockheight method to TestNode for versions that don't have a waitforblockheight RPC method.

Even if you solve that problem somehow (eg by rewriting sync_blocks, or avoiding it somehow), we also introduced in 0.14 support for RPC named arguments.

We could be careful to not use named arguments in tests where we want to use earlier versions. Or we could add a shim to TestNode to convert named -> positional.

I think we made a change at some point to support parsing numbers as strings, versus floats, and then I think our JSON library started to take advantage of that as well?

As above.

I imagine there will come a time when someone wants to do something which will break the test, because some utility code -- like sync_blocks, or the way we make RPC calls, or something else -- will change, and the test will be busted, and we won't have a good way to fix it.

I expect we'll only want to have tests covering the last one or two releases, so I don't see this as a huge problem.

I think the goal here is ... not to be able to run large swathes of the test framework against arbitrary bitcoin releases.

Yes - I agree that would be almost impossible.

@Sjors - if the goal is only to test official releases, why not download the binaries directly from https://bitcoincore.org/bin/ rather than building them locally?

This is an aside if the goal here is just to make 0.15 work, but:

We could be careful to not use named arguments in tests where we want to use earlier versions. Or we could add a shim to TestNode to convert named -> positional.

The change to support named args actually affected RPC invocations that don't use named args -- I think we send an empty dictionary instead of an empty list in authproxy.py which an old server wouldn't accept.

Anyway I don't mean to be overly pessimistic if you all think we can make something work! I'm definitely in favor of the overall goal, I just haven't come across a simple solution that I thought was robust.

I expect we'll only want to have tests covering the last one or two releases, so I don't see this as a huge problem.

I don't follow this point though -- in something like the p2p-segwit.py test, you'd have to do the upgrade after activation test on an 0.13.0 node or earlier. So if we'd written the test in such a way as to use an old binary, we'd have to just decommission that test at some point...? Obviously replacing with a later binary (that has the feature you want to test upgrade from) would mean you can't do that test anymore.

in something like the p2p-segwit.py test, you'd have to do the upgrade after activation test on an 0.13.0 node or earlier. So if we'd written the test in such a way as to use an old binary, we'd have to just decommission that test at some point...?

Yes - my expectation is that we'd decommission that test at some point. I don't think that testing versions older than a couple of releases is something that we should do as part of travis or developer local testing (although there's nothing stopping anyone from having a custom test rig to do that sort of thing).

@jnewbery compiling from source seems more flexible than fetching binaries, especially if we need to patch things. There might also be configure flags that are optimal for the functional tests. It can be expanded to fetch code from other repositories (on your own test rig).

Downloading binaries would be faster, but it also involves figuring out which OS you're on and checking the checksum, so it's not necessarily easier to write a script for. That said, the one-off build is painfully slow on Travis atm, so I might add an option to just fetch a binary.

@ryanofsky -vbparams=segwit:0:0 -vbparams=csv:0:0 did the trick, as long as I generate blocks on the modern node (which is fine). So no patch needed for v0.15.1 at the moment.

@sdaftuar I ran into the issue you mentioned for versions before v0.14 and added a comment to the test file to warn about that.

I changed the demonstration test to simply check if old nodes sync blocks from the new node. I'll try some SegWit wallet related scenarios in a different PR.

I added a commit that makes one Travis instance compile v0.14.2 and v0.15.1 (cached) and then run the relevant functional test. It works, but I'm a bit confused as to what the right place is on Travis to put these older builds.

Concept ACK, haven't looked at the code.

I think not testing with older versions is the biggest lack in our test env.
Things like the UTXO migration (now done) or testing how "older" nodes act on the (new) p2p network.
Also, up- and downgrading wallets is not covered by our tests and could be with something like this.

Concept ACK, cross version testing will be a very nice addition to the testing suite IMO

Rebased. Reminder to self: cherry-pick Sjors@bacf559 after #18067 is merged.

ACK c456145 🔨

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK c456145b2c65f580683df03bf10cd39000cf24d5 🔨
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUiCZgwAwkfNVLMvxlvocPpi0dT2TM8hPUkhwYfpWFP2GFvo6BDwivVgIgRd+Wdm
O3xDZ+b+KbNfGotMRjZgz0zWpwjCef5kaJBdrG8Je9xZeU3gPmVDZMFDvoQkE7Lv
RnRhui33JaSr+TGiPHMEtHOra6+Y3frY77SFqeJw8I6SPUca0uhCllULq9ejT6k/
QeMGdZIWyRcghbvqEJNsOyUi+bqeAtl8N4YI78WCR+IsnX463RA/ky49aRvnCPma
7r8jIV548gb0AlE05C6xZzcU9Iqd6XVXCDEDIDxdmWQCmK2S3RCKHUWqKLOX5Z54
u58dXa/lPi79iztdiSeuZFsSBSOprq8/0OCjQy+VrMUa/DumKhAE0sLTTwdDjUke
mz6fVGUIGS5czt4QKBt2TxtPsNtfg+WGrmCXQp31h0Ds2S5+3Y8IfSpLNHXFxC9U
YnnG8FgCOvXz9OwhHqptpZVKYak3C3j76ulLWpkAXdVYTLXEXdRjJ0fOYGyeA1b+
L6l6wWU2
=vdCU
-----END PGP SIGNATURE-----