test: mempool compatibility test #19153
Conversation
7c24bae to
fe4a271
Compare
|
Concept ACK What a nice first-time contribution! Warm welcome as a contributor @S3RK - I hope to see more great contributions from you in the future :) |
fe4a271 to
cd58338
Compare
|
Concept ACK. I don't think we can always guarantee that the mempool file will be backwards compatible, but it's good to have a test to be sure that compatibility at least isn't accidentally broken. |
| """Test that mempool.dat is compatible between versions | ||
|
|
||
| Download node binaries: | ||
| contrib/devtools/previous_release.sh -b v0.19.1 |
There was a problem hiding this comment.
nit: I prefer to have the same instruction in all test files that require previous releases, because otherwise other tests fail when they have some versions but not all. Try:
contrib/devtools/previous_release.sh -b v0.19.1 v0.18.1 v0.17.1 v0.16.3 v0.15.2
Only v0.19.1 is required by this test. The rest is used in other backwards compatibility tests.
|
|
||
| def setup_nodes(self): | ||
| self.add_nodes(self.num_nodes, versions=[ | ||
| 190100, |
There was a problem hiding this comment.
Suggest documenting what's interesting about that version, could be a link to Github issue.
| # Copyright (c) 2017-2020 The Bitcoin Core developers | ||
| # Distributed under the MIT software license, see the accompanying | ||
| # file COPYING or http://www.opensource.org/licenses/mit-license.php. | ||
| """Test that mempool.dat is compatible between versions |
There was a problem hiding this comment.
Suggest adding a comment like what @laanwj said:
I don't think we can always guarantee that the mempool file will be backwards compatible, but it's good to have a test to be sure that compatibility at least isn't accidentally broken.
|
@Sjors thanks for the review. I've pushed a fixup commit to address your suggestions. Once the PR is ready to be merged I will squash it into the original commit.
I'm not sure this tiny change deserves a PR and a separate discussion on its own 😄. To clarify the impact to log chattiness, the code path in question is executed only during initial mempool load, so it won't repeatedly produce tons of logs. But maybe I'm failing to consider some other possible negative outcomes. |
|
The new changes look good. Log chattiness could be one concern, e.g. when loading a 1 GB mempool with broken entries, but also there's no test. Touching validation.cpp generally instills fear :-) |
aec13a6 to
16d4b3f
Compare
|
Squashed the changes, removed log commit |
| 150200, # oldest version supported by the test framework | ||
| None, | ||
| ]) | ||
| adjust_bitcoin_conf_for_pre_17(self.nodes[0].bitcoinconf) |
There was a problem hiding this comment.
If you believe the code is unclear, we need to think of a better way to fix it rather than putting the same comment before each function call. For example: give this function a better name. But as for me, I think the function name speaks for itself and it doesn't need any clarifying comment.
There was a problem hiding this comment.
I think it's fine to call this without comment; it's always necessary for nodes before 0.17. Ideally the test framework (add_nodes) takes care of it automagically.
There was a problem hiding this comment.
tACK 16d4b3f
P.S. for increased glory you could sign your commits
| 150200, # oldest version supported by the test framework | ||
| None, | ||
| ]) | ||
| adjust_bitcoin_conf_for_pre_17(self.nodes[0].bitcoinconf) |
There was a problem hiding this comment.
I think it's fine to call this without comment; it's always necessary for nodes before 0.17. Ideally the test framework (add_nodes) takes care of it automagically.
16d4b3f test: mempool.dat compatibility between versions (Ivan Metlushko) Pull request description: Rationale: Verify mempool.dat compatibility between versions The format of mempool.dat has been changed in bitcoin#18038 The tests verifies the fix made in bitcoin#18807 and ensures that the file format is compatible between current version and v0.19.1 The test verifies both backward and forward compatibility. This PR also adds a log when we fail to add a tx loaded from mempool.dat. It was useful when debugging this test and could be potentially useful to debug other scenarios as well. Closes bitcoin#19037 ACKs for top commit: Sjors: tACK 16d4b3f Tree-SHA512: 00a38bf528c6478cb0da467af216488f83c1e3ca4d9166c109202ea8284023e99d87a3d6e252c4d88d08d9b5ed1a730b3e1970d6e5c0aef526fa7ced40de7490
Rationale: Verify mempool.dat compatibility between versions
The format of mempool.dat has been changed in #18038
The tests verifies the fix made in #18807 and ensures that the file format is compatible between current version and v0.19.1
The test verifies both backward and forward compatibility.
This PR also adds a log when we fail to add a tx loaded from mempool.dat.
It was useful when debugging this test and could be potentially useful to debug other scenarios as well.
Closes #19037