test: previous releases: add v24.0.1 #26586
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsNo conflicts as of last run. |
|
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 |
|
Sounds good to hold off a bit. Meanwhile it needs a rebase after #26589. I like how small this commit could be now that |
|
Concept NACK. Besides the bug, I'm not sure there's anything notably different between v23 and v24 to warrant adding v24 here. |
|
@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). |
|
Though, |
Probably not, but it's a low cost check.
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. |
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. |
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.) |
|
Instead of hardcoding the version numbers in |
|
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. |
|
Noting that if this is still going ahead, it should be updated to use 24.0.1. |
2c7c280 to
02ba6e8
Compare
|
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). |
| # Add new version after each release: | ||
| self.extra_args = [ | ||
| ["-addresstype=bech32", "-whitelist=noban@127.0.0.1"], # Pre-release: use to mine blocks. noban for immediate tx relay | ||
| ["-nowallet", "-walletrbf=1", "-addresstype=bech32", "-whitelist=noban@127.0.0.1"], # Pre-release: use to receive coins, swap wallets, etc | ||
| ["-nowallet", "-walletrbf=1", "-addresstype=bech32", "-whitelist=noban@127.0.0.1"], # v24.0.1 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Unrelated: The same goes for the other versions in this file, so someone could do that in a separate pull
There was a problem hiding this comment.
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.
02ba6e8 to
741908a
Compare
|
Not sure what to do here. @Sjors? |
|
tACK 741908a 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.
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.
I tend to agree. In any case we can always prune versions in a followup. |
The same procedure as every release (see dba1231 [v23.0] and d8b705f [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.