test: previous releases: add v24.0.1

theStack commented at 10:59 pm on November 27, 2022: contributor

The same procedure as every release (see dba123167236a172d2d33861d58aa94a19729671 [v23.0] and d8b705f1caeb3b4a6790cb26e4e5584ca791d965 [v22.0]), only a little simpler now: thanks to #25650, the previous release fetch script defaults to downloading/building the necessary tags, i.e. we don’t need to extend the tag list in the CI scripts and test/README.md anymore.

DrahtBot commented at 10:59 pm on November 27, 2022: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	Sjors
Concept NACK	luke-jr

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

DrahtBot added the label Tests on Nov 27, 2022

maflcko commented at 8:12 am on November 28, 2022: member

I wonder if the release will get wiped, given that there is a bug in the wallet, which will be fixed in a follow-up release on the 24.x branch

Sjors commented at 1:32 pm on November 28, 2022: member

Sounds good to hold off a bit. Meanwhile it needs a rebase after #26589.

I like how small this commit could be now that test/get_previous_releases.py -b automatically fetches all available versions.

luke-jr commented at 11:35 pm on November 28, 2022: member

Concept NACK. Besides the bug, I’m not sure there’s anything notably different between v23 and v24 to warrant adding v24 here.

Sjors commented at 11:01 am on November 29, 2022: member

@luke-jr one reason for the backwards compatibility tests is to ensure there’s no accidental breakage between versions, including wallet upgrades, consensus and p2p bugs. So I think we should add every major release.

Apart from that, #25717 alone should justify adding v24.0 (or whatever ends up widely available for download). On the wallet side this version added watch-only miniscript support (which last time I checked caused problems when downgrading).

maflcko commented at 11:09 am on November 29, 2022: member

Though, test/functional/feature_backwards_compatibility.py doesn’t test watch-only miniscript support. I don’t mind, but I am not too much a fan of a “general” compat test. It is likely so general that it can’t catch any issues. I much more prefer specific tests, like the ones you can find with git grep -l skip_if_no_prev

Sjors commented at 11:22 am on November 29, 2022: member

It is likely so general that it can’t catch any issues.

Probably not, but it’s a low cost check.

doesn’t test watch-only miniscript support

I’ve noticed over the past few years that people would add specific tests later on. It’s slightly easier to do that if the release is already there. But we can also keep this PR open until there’s another PR that needs it. We’ve previously piled several major releases this way. Back then it was more work to rebase than it is now. Every time it’s rebased, the CI runs the checks again, so it’s useful even in an unmerged state.

maflcko commented at 11:25 am on November 29, 2022: member

Probably not, but it’s a low cost check.

Not sure if it will stay low cost forever, if the matrix is append-only with no plan to remove some or all previous releases?

Sjors commented at 11:31 am on November 29, 2022: member

Probably not, but it’s a low cost check.

Not sure if it will stay low cost forever, if the matrix is append-only with no plan to remove some or all previous releases?

We can remove intermediate releases at some point.

luke-jr commented at 9:05 pm on November 29, 2022: member

Probably not, but it’s a low cost check.

It’s not low cost. To run the test at all, you have to keep a built copy of every single release listed. (And no, guix static binaries aren’t a good alternative.)

Sjors commented at 11:34 am on November 30, 2022: member

Instead of hardcoding the version numbers in feature_backwards_compatibility.py we could have it loop over whatever is available under releases/v*. In that case it’s up to the developer which previous releases they want to test. The default of test/get_previous_releases.py would still to fetch all the binaries.

maflcko commented at 11:46 am on November 30, 2022: member

I don’t think it is too important that a modern wallet can be opened with an ancient EOL version. It seems more important to be able to load an ancient wallet with a modern version. So maybe the EOL versions can be dropped and the qa-assets repo hosts some regtest wallets instead?

In any case the test fails intermittently (#24400), so apart from obvious smoke test issues that happen on every iteration, I don’t think anyone even looks into failures here.

fanquake commented at 11:43 am on December 7, 2022: member

Noting that if this is still going ahead, it should be updated to use 24.0.1.

theStack force-pushed on Dec 7, 2022

theStack renamed this:
~~test: previous releases: add v24.0~~
test: previous releases: add v24.0.1
on Dec 7, 2022

theStack commented at 2:30 pm on December 7, 2022: contributor

Rebased on master and updated to v24.0.1 (note that as I’m writing this, the archives on https://bitcoincore.org/bin/ are not available yet for v24.0.1).

in test/functional/feature_backwards_compatibility.py:45 in 02ba6e89df outdated

41+        self.num_nodes = 11
42         # Add new version after each release:
43         self.extra_args = [
44             ["-addresstype=bech32", "-whitelist=noban@127.0.0.1"], # Pre-release: use to mine blocks. noban for immediate tx relay
45             ["-nowallet", "-walletrbf=1", "-addresstype=bech32", "-whitelist=noban@127.0.0.1"], # Pre-release: use to receive coins, swap wallets, etc
46+            ["-nowallet", "-walletrbf=1", "-addresstype=bech32", "-whitelist=noban@127.0.0.1"], # v24.0.1

maflcko commented at 8:27 am on December 9, 2022:

I think it only makes sense to add the latest release for each missing feature. So if you want to add 24.x, you will either need to remove 23.x or explain why 24.x is special, see also 16624e6ff3af4429e571f7a606bbbcac336e067a

maflcko commented at 8:29 am on December 9, 2022:

Unrelated: The same goes for the other versions in this file, so someone could do that in a separate pull

theStack commented at 3:35 am on December 11, 2022:

I think it only makes sense to add the latest release for each missing feature. So if you want to add 24.x, you will either need to remove 23.x or explain why 24.x is special, see also 16624e6

For these wallet backwards compatibility tests, wouldn’t it make sense to always keep at least the recent major releases that haven’t reached EOL yet as bare minimum rather than sacrificing one for another? I don’t think 24.x is special to 23.x at all, but I’d rather verify this assumption (in this case e.g., that wallets created on master can still be opened on both of these versions) rather than potentially miss out on either one of those cases.

ACK on cleaning up the releases list in general, but I think that’s outside of scope for this PR.

DrahtBot added the label Needs rebase on Dec 9, 2022

test: previous releases: add v24.0.1 741908afc1

theStack force-pushed on Dec 11, 2022

DrahtBot removed the label Needs rebase on Dec 11, 2022

fanquake commented at 10:49 am on February 16, 2023: member

Not sure what to do here. @Sjors?

Sjors commented at 12:48 pm on February 16, 2023: member

tACK 741908afc1f9ed2040c18667c75665b300c5dfe7

I checked that it still works on top of current master. That said, there’s no pressing reason to merge this. As we’ve done before, we could just leave the PR open and occasionally rebase it.

apart from obvious smoke test issues that happen on every iteration, I don’t think anyone even looks into failures here

This can also be a reason to not rush merging new releases. This PR itself is where most of the thorough checking happens, at every rebase.

wouldn’t it make sense to always keep at least the recent major releases that haven’t reached EOL yet

I tend to agree. In any case we can always prune versions in a followup.

maflcko merged this on Feb 16, 2023

maflcko closed this on Feb 16, 2023

bitcoin locked this on Feb 16, 2024

test: previous releases: add v24.0.1 #26586

Reviews

Conflicts