test: mempool compatibility test #19153
Conversation
S3RK
changed the title
S3RK
force-pushed
the
feature/mempool_compatibility_test
branch
from
7c24bae to
fe4a271
Compare
maflcko
left a comment
maflcko
left a comment
There was a problem hiding this comment.
Nice first contribution! ACK with or without the nit addressed
|
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 :) |
S3RK
force-pushed
the
feature/mempool_compatibility_test
branch
from
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. |
Sjors
left a comment
Sjors
left a comment
There was a problem hiding this comment.
Concept ACK. f0b2579 looks good modulo some comments.
Suggest making a separate PR for cd58338, ideally with a test.
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.
There was a problem hiding this comment.
Suggest documenting what's interesting about that version, could be a link to Github issue.
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 :-) |
S3RK
force-pushed
the
feature/mempool_compatibility_test
branch
from
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.
Sjors
left a comment
Sjors
left a comment
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
7 participants
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