Build previous releases and run functional tests

Sjors commented at 7:35 pm on January 9, 2018: member

This PR adds binaries for 0.17, 0.18 and 0.19 to Travis and runs a basic block propagation test.

Includes test for upgrading v0.17.1 wallets and opening master wallets with older versions.

Usage:

0contrib/devtools/previous_release.sh -f -b v0.19.0.1 v0.18.1 v0.17.1
1test/functional/backwards_compatibility.py

Travis caches these earlier releases, so it should be able to run these tests with little performance impact.

Additional scenarios where it might be useful to run tests against earlier releases:

creating a wallet with #11403’s segwit implementation, copying it to an older node and making sure the user didn’t lose any funds (although this PR doesn’t support v0.15.1)
future consensus changes
P2P changes (e.g. to make sure we don’t accidentally ban old nodes)

ryanofsky commented at 8:27 pm on January 9, 2018: member

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.

Sjors commented at 8:33 pm on January 9, 2018: member

@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?

ryanofsky commented at 8:42 pm on January 9, 2018: member

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

sdaftuar commented at 9:16 pm on January 9, 2018: member

@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.

ryanofsky commented at 9:38 pm on January 9, 2018: member

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.

sdaftuar commented at 9:49 pm on January 9, 2018: member

@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.

jnewbery commented at 10:00 pm on January 9, 2018: member

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?

sdaftuar commented at 10:09 pm on January 9, 2018: member

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.

sdaftuar commented at 10:12 pm on January 9, 2018: member

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.

jnewbery commented at 10:16 pm on January 9, 2018: member

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).

Sjors force-pushed on Jan 10, 2018

Sjors commented at 4:26 pm on January 10, 2018: member

@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.

in .travis.yml:16 in f46bc95f19 outdated

 6@@ -7,6 +7,7 @@ cache:
 7   - depends/built
 8   - depends/sdk-sources
 9   - $HOME/.ccache
10+  - build/releases

MarcoFalke commented at 4:33 pm on January 10, 2018:

That cache should be covered by .ccache, no?

Sjors commented at 10:17 am on January 11, 2018:

will look into ccache behavior.

MarcoFalke commented at 6:15 pm on January 11, 2018:

Forget what I said. It is probably better to cache the resulting binary than the individual obj files, since we are building specific tags that don’t change. For master or other branch build, the commits often only change parts of the code, so the other obj files can be cached and re-used. For tag builds everything is static, so caching the binaries works as well.

in test/functional/backwards_compatibility.py:43 in f46bc95f19 outdated

29+
30+    def setup_nodes(self):
31+        """Override this method to customize test node setup"""
32+        self.add_nodes(self.num_nodes, self.extra_args, None, None, [
33+            None,
34+            "build/releases/v0.15.1/src/bitcoind",

MarcoFalke commented at 4:46 pm on January 10, 2018:

Would be nice to be able to pass those paths in. Could overwrite add_options in test_framework.

Sjors commented at 10:09 am on January 11, 2018:

Do you mean making --srcdir accept multiple paths? That’s probably a useful PR by itself, I made a note.

in test/functional/backwards_compatibility.py:31 in f46bc95f19 outdated

26+            ["-vbparams=segwit:0:0", "-vbparams=csv:0:0"], # v0.15.1
27+            ["-vbparams=segwit:0:0", "-vbparams=csv:0:0"] # v0.14.2
28+        ]
29+
30+    def setup_nodes(self):
31+        """Override this method to customize test node setup"""

MarcoFalke commented at 4:46 pm on January 10, 2018:

Comment seems misplaced

Sjors commented at 10:16 am on January 11, 2018:

Removed.

in test/functional/backwards_compatibility.py:32 in f46bc95f19 outdated

27+            ["-vbparams=segwit:0:0", "-vbparams=csv:0:0"] # v0.14.2
28+        ]
29+
30+    def setup_nodes(self):
31+        """Override this method to customize test node setup"""
32+        self.add_nodes(self.num_nodes, self.extra_args, None, None, [

MarcoFalke commented at 4:47 pm on January 10, 2018:

Use named args for those unnamed arguments to provide documentation and robustness against future interface changes.

Sjors commented at 10:16 am on January 11, 2018:

Done.

MarcoFalke commented at 4:50 pm on January 10, 2018: member

Concept ACK. Just noting that every change to the rpc interface is a breaking change in compatibility tests, that potentially needs attention. So we need to run those tests in the pull request travis run.

jonasschnelli commented at 7:01 pm on January 10, 2018: contributor

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.

fanquake added the label Tests on Jan 11, 2018

meshcollider commented at 9:15 am on January 11, 2018: contributor

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

in .travis.yml:58 in f46bc95f19 outdated

71@@ -70,7 +72,7 @@ script:
72     - BITCOIN_CONFIG_ALL="--disable-dependency-tracking --prefix=$TRAVIS_BUILD_DIR/depends/$HOST --bindir=$OUTDIR/bin --libdir=$OUTDIR/lib"
73     - if [ -z "$NO_DEPENDS" ]; then depends/$HOST/native/bin/ccache --max-size=$CCACHE_SIZE; fi
74     - test -n "$USE_SHELL" && eval '"$USE_SHELL" -c "./autogen.sh"' || ./autogen.sh
75-    - mkdir build && cd build
76+    - mkdir -p build && cd build

snuggs commented at 9:52 am on January 11, 2018:

Nice nit @MarcoFalke!

Sjors force-pushed on Jan 11, 2018

Sjors commented at 10:32 am on January 11, 2018: member

@MarcoFalke wrote:

we need to run those tests in the pull request travis run

They ran for this PR, so I assume they’ll run on every PR once this is merged?

Sjors force-pushed on Jan 12, 2018

Sjors renamed this:
~~[WIP] Build previous releases and run functional tests~~
Build previous releases and run functional tests
on Jan 12, 2018

Sjors commented at 7:12 pm on January 12, 2018: member

I found this quite useful in testing the recently merged SegWit wallet changes in WIP #12152.

I didn’t need more changes to the build script and Travis configuration for those tests. On other hand in order to swap wallets in a test I’ll need more involved changes to the test framework as well as dynamic loading of wallets. So I’ll keep the tests in this PR trivial.

With the above caveats, noting this is probably only maintainable for the most recent one or two releases, I think it’s ready for review.

Once everyone is happy with the code, someone with Travis privileges should trigger a new build with a completely wiped cache (I can also add and drop a commit that uses the -r flag to force a rebuild, but that’s less rigorous).

Sjors force-pushed on Apr 11, 2018

Sjors commented at 3:30 pm on April 11, 2018: member

Rebased. I added support for v0.16.0 and removed v0.15.1 and v0.14.2 since they’re no longer compatible with the test framework (can perhaps be patched later).

Sjors force-pushed on Apr 11, 2018

in .travis.yml:58 in dfd5c57cae outdated

62@@ -62,6 +63,7 @@ before_script:
63     - if [ -z "$NO_DEPENDS" ]; then make $MAKEJOBS -C depends HOST=$HOST $DEP_OPTS; fi
64     # Start xvfb if needed, as documented at https://docs.travis-ci.com/user/gui-and-headless-browsers/#Using-xvfb-to-Run-Tests-That-Require-a-GUI
65     - if [ "$NEED_XVFB" = 1 ]; then export DISPLAY=:99.0; /sbin/start-stop-daemon --start --pidfile /tmp/custom_xvfb_99.pid --make-pidfile --background --exec /usr/bin/Xvfb -- :99 -ac; fi
66+    - if [ "$COMPILE_PREVIOUS_VERSIONS" = "true" ]; then contrib/devtools/previous_release.sh -d -f v0.16.0; fi

MarcoFalke commented at 3:00 pm on April 29, 2018:

Just noting that in case we don’t apply local patches to the previous release, we might as well just fetch the gitian result from one of the download sites. That would also save us from having to deal with caching the binaries et al.

Sjors commented at 11:49 am on April 30, 2018:

Maybe fetching from a download site could be an optional argument for previous_release.sh?

Sjors commented at 12:48 pm on May 15, 2018:

I’m worried that if we go for the binary download approach, the compile from source stuff would no longer be maintained. I could remove that entirely, but then some small future change to the test framework might force us to put it back in. I don’t know how likely that is, but caching these files doesn’t seem like a huge burden either.

Sjors force-pushed on May 15, 2018

Sjors commented at 12:49 pm on May 15, 2018: member

Rebased due to small conflict in travis.yml.

Sjors commented at 9:17 am on May 23, 2018: member

Travis failed after 50 minutes due to a timeout. Preventing that would be an argument in favor of just fetching binaries.

MarcoFalke added the label Needs rebase on Jun 6, 2018

Sjors force-pushed on Jul 6, 2018

Sjors commented at 12:45 pm on July 6, 2018: member

Rebased, and switched the test from v0.16.0 to v0.16.1

Getting another timeout during the build, so I’ll add support to just fetch the binaries.

Sjors force-pushed on Jul 6, 2018

Sjors commented at 2:01 pm on July 6, 2018: member

It now fetches the v0.16.1 binary instead of compiling, which should speed up Travis.

Unfortunately now the test framework seems unable to connect to the RPC, at least on my Mac.

Sjors force-pushed on Jul 6, 2018

DrahtBot removed the label Needs rebase on Jul 7, 2018

Sjors force-pushed on Jul 7, 2018

Sjors commented at 12:48 pm on July 7, 2018: member

Same problem on Travis: test framework can’t connect to the v0.16.1 RPC for some reason. @jnewbery any recent changes which could explain that? Or perhaps there’s something about the release binaries that gets in the way?

Sjors force-pushed on Jul 7, 2018

MarcoFalke commented at 7:42 am on July 8, 2018: member

@Sjors I believe you could add some prints in wait_for_rpc_connection to find out why it fails.

jnewbery commented at 3:13 pm on July 9, 2018: member

If you can repro this locally, I’d recommend adding a breakpoint immediately before the call to wait_for_rpc_connection() and then seeing if you can connect to the RPC manually. Perhaps your binaries aren’t picking up all the correct config and command line arguments?

DrahtBot added the label Needs rebase on Jul 24, 2018

Sjors force-pushed on Aug 26, 2018

Sjors commented at 7:03 pm on August 26, 2018: member

Rebased. I’m just going to wait for the 0.17.0 release rather than try to debug why v0.16.2 doesn’t play nicely. Based on local testing that should work.

DrahtBot removed the label Needs rebase on Aug 26, 2018

DrahtBot added the label Needs rebase on Aug 27, 2018

Sjors force-pushed on Aug 28, 2018

DrahtBot removed the label Needs rebase on Aug 28, 2018

Sjors force-pushed on Aug 28, 2018

Sjors commented at 3:39 pm on August 28, 2018: member

Rebased again. Moved to the x86_64 Linux (no depends, only system libs) host since that one has less to do.

in contrib/devtools/previous_release.sh:62 in 39c428481b outdated

57+for tag in "$@"
58+do
59+  RELEASE_PATH="$PWD/build/releases/$tag"
60+
61+  if [ "$DELETE_EXISTING" -eq "1" ]; then
62+    rm -rf $RELEASE_PATH

practicalswift commented at 2:50 pm on September 11, 2018:

Looks a bit risky. Can this be done in a safer manner? :-)

Sjors commented at 2:59 pm on September 11, 2018:

I can leave out the -f.

in contrib/devtools/previous_release.sh:77 in 39c428481b outdated

72+      if [ -z $(git tag -l "$tag") ]; then
73+        echo "Tag $tag not found"
74+        exit 1
75+      fi
76+
77+      git clone . $RELEASE_PATH

practicalswift commented at 2:50 pm on September 11, 2018:

Double quote to prevent globbing and word splitting. Please fix throughout in this PR :-)

Verify with shellcheck contrib/devtools/previous_release.sh | grep SC2086

Sjors commented at 4:24 am on October 14, 2018:

Fixed, except for ./configure $CONFIG_FLAGS, but $CONFIG_FLAGS itself is created with double quotes.

DrahtBot added the label Needs rebase on Sep 16, 2018

Sjors force-pushed on Oct 14, 2018

Sjors commented at 7:19 am on October 14, 2018: member

Now that v0.17.0 binaries are available, this PR works again.

Rebased, added double quotes, refactored script to make rm a bit safer. The backwards compatibility test is skipped if build/releases is absent.

It runs on Travis machine 7:

And is skipped elsewhere:

fanquake removed the label Needs rebase on Oct 14, 2018

jnewbery commented at 1:57 pm on October 15, 2018: member

I’m not convinced that the extra machinery that this PR introduces is worth the additional test coverage that it provides. At the moment, the test only checks that a v0.17 node can sync with blocks generated on a master branch node. I’d expect an inconsistency that crass would be caught very quickly regardless of this test. On the other hand, extending the test to cover more complex scenarios will probably run into the difficulties enumerated by Suhas here: #12134 (comment). Any such test would also require maintenance every time there’s a new release.

In general, I think that cross-version compatibility testing is definitely worthwhile, but I’m not convinced that it should live in the core repo and run in travis. What would your thoughts be about running this style of tests separately from our CI?

ryanofsky commented at 4:10 pm on October 15, 2018: member

This change seems like it’d be useful in order to write tests for wallet upgrade code, where we have some issues to fix like #14422. If we aren’t going to merge this, how can we write tests for code that handles upgrades or downgrades?

Sjors commented at 5:38 am on October 16, 2018: member

The main use case, which I think is worth some overhead, is testing soft forks before they’re deployed. We could have the test craft more interesting transactions and blocks on master.

Some aspects of wallet upgrades can be tested without this, e.g. we can store older wallet payloads in the test folder. This PR lets you test opening wallets on a downgraded node. Node downgrades in general can be tested this way, by copying the datadir.

As long as I occasionally rebase this PR it should be useful even if it’s not merged.

jnewbery commented at 12:58 pm on October 16, 2018: member

The main use case, which I think is worth some overhead, is testing soft forks before they’re deployed. We could have the test craft more interesting transactions and blocks on master.

As long as I occasionally rebase this PR it should be useful even if it’s not merged.

Yes, this sounds reasonable. I don’t think the current test case adds much, but if there’s a fork in future, a test using this infrastructure would be very useful.

Some aspects of wallet upgrades can be tested without this, e.g. we can store older wallet payloads in the test folder. This PR lets you test opening wallets on a downgraded node. Node downgrades in general can be tested this way, by copying the datadir.

Great - that sounds like a much better way of testing wallet upgrades.

in test/functional/feature_backwards_compatibility.py:5 in 552df51c9b outdated

0@@ -0,0 +1,64 @@
1+#!/usr/bin/env python3
2+# Copyright (c) 2018 The Bitcoin Core developers
3+# Distributed under the MIT software license, see the accompanying
4+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
5+"""Backwards compatibity functional test

practicalswift commented at 8:29 pm on October 16, 2018:

Should be “compatibility” :-)

in test/functional/feature_backwards_compatibility.py:60 in 552df51c9b outdated

55+    def run_test(self):
56+        self.nodes[0].generate(101)
57+
58+        sync_blocks(self.nodes)
59+
60+        # Santity check the test framework:

practicalswift commented at 8:30 pm on October 16, 2018:

Should be “sanity” :-)

Sjors force-pushed on Oct 17, 2018

DrahtBot commented at 11:13 am on October 20, 2018: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

jtimon commented at 4:45 pm on October 22, 2018: contributor

Concept ACK, perhaps the python test could optionally take a custom value even if it “build/releases/v0.17.0/bin/” as default?

in test/functional/feature_backwards_compatibility.py:28 in cb62a1c54e outdated

23+from test_framework.util import (
24+    assert_equal,
25+    sync_blocks
26+)
27+
28+class BackwardsCompatibityTest(BitcoinTestFramework):

practicalswift commented at 6:11 am on November 23, 2018:

Should be “BackwardsCompatibilityTest” :-)

in test/functional/feature_backwards_compatibility.py:64 in cb62a1c54e outdated

59+
60+        # Sanity check the test framework:
61+        res = self.nodes[self.num_nodes - 1].getblockchaininfo()
62+        assert_equal(res['blocks'], 101)
63+if __name__ == '__main__':
64+    BackwardsCompatibityTest().main()

practicalswift commented at 6:11 am on November 23, 2018:

Same typo here :-)

practicalswift changes_requested

DrahtBot added the label Needs rebase on Nov 23, 2018

Sjors commented at 11:41 am on November 30, 2018: member

(I’ll rebase and fix above nits after 0.17.1 final binaries are up)

Sjors force-pushed on Jan 5, 2019

Sjors commented at 7:20 pm on January 5, 2019: member

Rebased for v0.17.1

ef7c54c1eceb0eb938dd4e8312283c7433fe0c8f added a wait argument to stop (cc @promag), which causes the v0.17.1 node to fail upon shutdown. I worked around this by adding an optional version to TestNode().

Sjors force-pushed on Jan 5, 2019

in .travis/test_06_script.sh:27 in b823630434 outdated

22@@ -23,7 +23,7 @@ else
23 fi
24 END_FOLD
25 
26-mkdir build
27+mkdir -p build

Empact commented at 6:29 am on January 6, 2019:

nit: maybe a comment to indicate the -p is because the dir may already exist (or so I presume)?

in contrib/devtools/previous_release.sh:85 in b823630434 outdated

75+  esac
76+fi
77+
78+pushd "$PWD/build/releases" || exit 1
79+
80+  for tag in "$@"

Empact commented at 6:31 am on January 6, 2019:

nit: whitespace

Sjors commented at 11:23 am on January 6, 2019:

I’m indenting pushd so it’s more clear what directory the script is in. Or is it another whitespace issue?

practicalswift commented at 11:28 am on January 6, 2019:

Please remove the indenting for the pushd. It is non-idiomatic :-)

Sjors commented at 12:35 pm on January 6, 2019:

See also this discussion: https://github.com/mvdan/sh/issues/293

What do you think of the indenting with brackets suggestion?

0pushd somewhere || exit 1
1{
2   do something
3}
4popd || exit 1

practicalswift commented at 12:49 pm on January 6, 2019:

@Sjors Brackets make it clear that the indentation is intentional :-)

DrahtBot removed the label Needs rebase on Jan 6, 2019

Sjors force-pushed on Jan 7, 2019

jnewbery commented at 9:38 pm on January 7, 2019: member

@Sjors - what’s your intention for this branch? Do you want it to get merged into master, or are you maintaining it as a separate branch for your own testing? (if so perhaps you could add a [do not merge] tag to the PR title)

The version logic that you’ve added to TestNode is fairly limited now, but I expect that if you want to maintain this branch and have it remain compatible with different versions of bitcoind, the complexity will start to creep up. I think this kind of testing is useful, particularly for any future softfork, but my personal preference would still be to not have this merged into master.

Sjors commented at 1:12 pm on January 9, 2019: member

@jnewbery I would like it to get merged, but I’m willing to maintain it for the time being even if it doesn’t get merged. I think people were worried about maintainability if this is merged. So far it’s not too bad in my experience.

I don’t think the goal is to support an infinite number of old bitcoind versions, so complexity can probably be kept low by dropping old versions and simplifying the test patches.

DrahtBot added the label Needs rebase on Feb 1, 2019

Sjors commented at 11:21 am on February 2, 2019: member

I’ll rebase after 0.18 binaries are up, unless we want to merge this sooner.

I might also add a test for #15226: create a blank wallet in 0.18 and master and check that they can’t be opened by 0.17.

Sjors force-pushed on Feb 20, 2019

DrahtBot removed the label Needs rebase on Feb 20, 2019

Sjors force-pushed on Feb 20, 2019

Sjors commented at 8:44 pm on February 20, 2019: member

Rebased and prepared for v0.18.0 release (just need to uncomment a few lines after the binaries are up, but it can be merged before that if we want).

I added tests for opening regular, private key disabled and blank wallets in v0.17.1 (cc @achow101, @instagibbs). They work by creating wallets in v0.18.0 and on master, copying and then opening with v0.18.0 and v0.17.1.

I also added a test to upgrade a v0.17.1 wallet to master, in particular to check if the key origin data is upgraded.

Sjors force-pushed on Feb 20, 2019

DrahtBot added the label Needs rebase on Feb 25, 2019

Sjors force-pushed on Mar 7, 2019

DrahtBot removed the label Needs rebase on Mar 7, 2019

Sjors force-pushed on May 3, 2019

Sjors commented at 1:19 pm on May 3, 2019: member

It now uses v0.18.0 release.

practicalswift commented at 1:26 pm on May 3, 2019: contributor

Concept ACK

Nice work!

instagibbs commented at 3:51 pm on June 12, 2019: member

and the test will be busted, and we won’t have a good way to fix it

I think this is an argument for keeping the scope reduced but not convincing to keep out entirely. I sincerely hope that the functional suite can support semi-modern versions with respect to block sync checks, for example.

concept ACK

lack of wallet upgrade tests are some of the most painful points right now

DrahtBot added the label Needs rebase on Jul 7, 2019

Sjors force-pushed on Jul 8, 2019

DrahtBot removed the label Needs rebase on Jul 8, 2019

DrahtBot added the label Needs rebase on Aug 5, 2019

Sjors force-pushed on Aug 9, 2019

Sjors commented at 6:57 pm on August 9, 2019: member

Rebased to to support the latest v0.18.1. Added a version conditional for -logthreadnames (#15849).

DrahtBot removed the label Needs rebase on Aug 9, 2019

DrahtBot added the label Needs rebase on Aug 15, 2019

Sjors force-pushed on Aug 15, 2019

DrahtBot removed the label Needs rebase on Aug 15, 2019

Sjors commented at 7:47 pm on August 15, 2019: member

Rebased after #16582, moved Travis machine 8, because machine 7 has --disable-wallet. Travis is having a hard time at the moment though; I tried a few restarts of machine 5 and 8 in vain.

Sjors force-pushed on Aug 16, 2019

Sjors commented at 10:44 am on August 16, 2019: member

Expanded wallet tests for #16624.

Sjors force-pushed on Aug 16, 2019

Sjors commented at 9:00 pm on August 16, 2019: member

Moving releases to host-specific directory on Travis. Added a check to prevent the test from being silently skipped when it can’t find the release binaries.

Sjors commented at 10:23 am on August 17, 2019: member

I’m confused by Travis / Docker file system organization. See e.g. this log.

The releases are downloaded to /home/travis/build/bitcoin/bitcoin/build/bitcoin-x86_64-unknown-linux-gnu/releases, which is cached. I don’t know yet if the cache works, because so far my builds have failed so the cache didn’t get stored. If it works, you should see “Using cached v0.18.1” and “Using cached v0.17.1” in the log.

feature_backwards_compatibility.py complains that it can’t find /home/travis/build/bitcoin/bitcoin/build/bitcoin-x86_64-unknown-linux-gnu/releases. I don’t understand why. Does stuff get moved around or am I accidentally downloading stuff into a Docker volume that the test runner doesn’t see?

Sjors force-pushed on Aug 17, 2019

Sjors commented at 12:19 pm on August 17, 2019: member

Hopefully solved by mounting the releases folder as another Docker volume (moved it to $TRAVIS_BUILD_DIR/releases/$HOST).

Sjors commented at 1:40 pm on August 17, 2019: member

Getting closer! This time the wallet test actually ran and failed: Wallet file verification failed: wallet.dat corrupt, salvage failed (-4) (log). This happens when 0.18 tries to load a (blank) wallet from master. Documented in #16640.

Sjors force-pushed on Aug 17, 2019

DrahtBot added the label Needs rebase on Aug 19, 2019

Sjors force-pushed on Aug 19, 2019

DrahtBot removed the label Needs rebase on Aug 19, 2019

DrahtBot added the label Needs rebase on Aug 29, 2019

Sjors force-pushed on Aug 29, 2019

DrahtBot removed the label Needs rebase on Aug 29, 2019

DrahtBot added the label Needs rebase on Oct 3, 2019

Sjors force-pushed on Oct 9, 2019

Sjors commented at 7:11 pm on October 9, 2019: member

Rebased and added tests for v0.19.0 (using release candidate).

DrahtBot removed the label Needs rebase on Oct 9, 2019

DrahtBot added the label Needs rebase on Oct 17, 2019

Sjors force-pushed on Oct 27, 2019

DrahtBot removed the label Needs rebase on Oct 27, 2019

DrahtBot added the label Needs rebase on Nov 8, 2019

Sjors force-pushed on Nov 25, 2019

Dianne1404 approved

fanquake removed the label Needs rebase on Nov 25, 2019

Sjors commented at 7:38 pm on November 25, 2019: member

Rebased again. Now using v0.19.0.1

DrahtBot added the label Needs rebase on Dec 3, 2019

Sjors force-pushed on Dec 6, 2019

DrahtBot removed the label Needs rebase on Dec 6, 2019

Sjors force-pushed on Dec 6, 2019

DrahtBot added the label Needs rebase on Dec 7, 2019

Sjors force-pushed on Dec 7, 2019

DrahtBot removed the label Needs rebase on Dec 7, 2019

DrahtBot added the label Needs rebase on Dec 10, 2019

Sjors force-pushed on Dec 11, 2019

DrahtBot removed the label Needs rebase on Dec 11, 2019

DrahtBot added the label Needs rebase on Dec 17, 2019

Sjors force-pushed on Jan 9, 2020

DrahtBot removed the label Needs rebase on Jan 9, 2020

DrahtBot added the label Needs rebase on Jan 22, 2020

Sjors force-pushed on Jan 23, 2020

DrahtBot removed the label Needs rebase on Jan 23, 2020

[scripts] build earlier releases ae379cf7d1

Sjors force-pushed on Feb 11, 2020

Sjors commented at 7:05 pm on February 11, 2020: member

Rebased. Reminder to self: cherry-pick https://github.com/Sjors/bitcoin/commit/bacf55990905c0257c7bc45bbbd2b028716efc7f after #18067 is merged.

in contrib/devtools/previous_release.sh:149 in 6c530f550e outdated

144+        rm "bitcoin-${tag:1}-$PLATFORM.tar.gz"
145+      fi
146+    fi
147+  done
148+}
149+popd || exit 1

MarcoFalke commented at 7:34 pm on February 11, 2020:

I don’t think we have anyone interested in tests who can also review bash scripts. Maybe rewrite this in python?

Sjors commented at 8:50 pm on February 11, 2020:

Added to my todo list, but equally happy to (see someone else) do this in a followup.

MarcoFalke added this to the milestone 0.20.0 on Feb 11, 2020

in ci/test/05_before_script.sh:40 in 6c530f550e outdated

34@@ -35,3 +35,8 @@ if [ -z "$NO_DEPENDS" ]; then
35   fi
36   DOCKER_EXEC $SHELL_OPTS make $MAKEJOBS -C depends HOST=$HOST $DEP_OPTS
37 fi
38+if [ "$TEST_PREVIOUS_RELEASES" = "true" ]; then
39+  BEGIN_FOLD previous-versions
40+  DOCKER_EXEC contrib/devtools/previous_release.sh -f -b -t "$PREVIOUS_RELEASES_DIR" v0.17.1 v0.18.1 v0.19.0.1

MarcoFalke commented at 7:42 pm on February 11, 2020:

I think -b implies that -f is unused. So why pass it?

Sjors commented at 8:50 pm on February 11, 2020:

It was a left-over from when this PR still compiled from source. Removed.

in ci/test/04_install.sh:66 in 6c530f550e outdated

62@@ -62,6 +63,7 @@ if [ -z "$RUN_CI_ON_HOST" ]; then
63                   --mount type=bind,src=$BASE_ROOT_DIR,dst=/ro_base,readonly \
64                   --mount type=bind,src=$CCACHE_DIR,dst=$CCACHE_DIR \
65                   --mount type=bind,src=$DEPENDS_DIR,dst=$DEPENDS_DIR \
66+                  --mount type=bind,src=$PREVIOUS_RELEASES_DIR,dst=$PREVIOUS_RELEASES_DIR \

MarcoFalke commented at 7:43 pm on February 11, 2020:

Not sure if it is worth it to cache this folder. No strong opinion though. If you keep this, please fixup the ci/README.md where it says “cache directories, such as …”

Sjors commented at 8:49 pm on February 11, 2020:

In my experience the release download is pretty slow. I updated the ci/README

in test/functional/feature_backwards_compatibility.py:51 in 6c530f550e outdated

46+
47+        releases_path = os.getenv("PREVIOUS_RELEASES_DIR") or os.getcwd() + "/releases"
48+        if not os.path.isdir(releases_path):
49+            if os.getenv("TEST_PREVIOUS_RELEASES") == "true":
50+                raise AssertionError("TEST_PREVIOUS_RELEASES=1 but releases missing: " + releases_path)
51+            raise SkipTest("This test requires binaries for previous releases")

MarcoFalke commented at 7:46 pm on February 11, 2020:

I’d rather have both of these an Assertion instead of a silent pass

Sjors commented at 8:49 pm on February 11, 2020:

That seems inconsistent with the way we skip wallet tests, i.e. the test runner calls each test file and each test file decides if needs to be skipped.

in test/functional/feature_backwards_compatibility.py:251 in 6c530f550e outdated

246+
247+        # Open the wallets in v0.18
248+        node_v18.loadwallet("w1")
249+        wallet = node_v18.get_wallet_rpc("w1")
250+        info = wallet.getwalletinfo()
251+        assert(info['private_keys_enabled'])

MarcoFalke commented at 7:47 pm on February 11, 2020:

style-nit:

0        assert info['private_keys_enabled']

(Applies throughout this file)

Sjors commented at 8:49 pm on February 11, 2020:

Done

in test/functional/test_framework/test_framework.py:372 in 6c530f550e outdated

368@@ -369,7 +369,7 @@ def run_test(self):
369 
370     # Public helper methods. These can be accessed by the subclass test scripts.
371 
372-    def add_nodes(self, num_nodes, extra_args=None, *, rpchost=None, binary=None):
373+    def add_nodes(self, num_nodes, extra_args=None, *, rpchost=None, binary=None, binarycli=None, versions=None):

MarcoFalke commented at 7:48 pm on February 11, 2020:

style-nit:

0    def add_nodes(self, num_nodes, extra_args=None, *, rpchost=None, binary=None, binary_cli=None, versions=None):

Sjors commented at 8:49 pm on February 11, 2020:

Done

MarcoFalke approved

MarcoFalke commented at 7:49 pm on February 11, 2020: member

ACK, some style nits

[tests] check v0.17.1 and v0.18.1 backwards compatibility 8b1460dbd1

[scripts] support release candidates of earlier releases c7ca630896

[tests] add wallet backwards compatility tests 9d9390dab7

[test] add v0.17.1 wallet upgrade test b769cd142d

[test] add 0.19 backwards compatibility tests c456145b2c

Sjors force-pushed on Feb 11, 2020

MarcoFalke commented at 2:19 pm on February 12, 2020: member

ACK c456145b2c65f580683df03bf10cd39000cf24d5 🔨

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK c456145b2c65f580683df03bf10cd39000cf24d5 🔨
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUiCZgwAwkfNVLMvxlvocPpi0dT2TM8hPUkhwYfpWFP2GFvo6BDwivVgIgRd+Wdm
 8O3xDZ+b+KbNfGotMRjZgz0zWpwjCef5kaJBdrG8Je9xZeU3gPmVDZMFDvoQkE7Lv
 9RnRhui33JaSr+TGiPHMEtHOra6+Y3frY77SFqeJw8I6SPUca0uhCllULq9ejT6k/
10QeMGdZIWyRcghbvqEJNsOyUi+bqeAtl8N4YI78WCR+IsnX463RA/ky49aRvnCPma
117r8jIV548gb0AlE05C6xZzcU9Iqd6XVXCDEDIDxdmWQCmK2S3RCKHUWqKLOX5Z54
12u58dXa/lPi79iztdiSeuZFsSBSOprq8/0OCjQy+VrMUa/DumKhAE0sLTTwdDjUke
13mz6fVGUIGS5czt4QKBt2TxtPsNtfg+WGrmCXQp31h0Ds2S5+3Y8IfSpLNHXFxC9U
14YnnG8FgCOvXz9OwhHqptpZVKYak3C3j76ulLWpkAXdVYTLXEXdRjJ0fOYGyeA1b+
15L6l6wWU2
16=vdCU
17-----END PGP SIGNATURE-----

Timestamp of file with hash 9f2d9c2708f034d9a99d437d02cbd47fb5e0b16b9e7cfc969396186bdeabfbbf -

MarcoFalke referenced this in commit caa2f3af29 on Feb 12, 2020

MarcoFalke merged this on Feb 12, 2020

MarcoFalke closed this on Feb 12, 2020

Sjors deleted the branch on Feb 12, 2020

DrahtBot locked this on Feb 15, 2022

Build previous releases and run functional tests #12134

Conflicts