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
12 | + 13 | +from test_framework.test_framework import BitcoinTestFramework 14 | + 15 | +class MempoolCompatibilityTest(BitcoinTestFramework): 16 | + def set_test_params(self): 17 | + self.setup_clean_chain = True
If you remove this line, you can also remove the whole "Generate some coins on both wallets" part
It didn't work for me for some reason, though I don't fully understand yet how chain generation works in tests. I'll try this again tomorrow and will get back with an update
I figured it out. I forgot to manually import coinbase keys in setup_nodes(), it works now. Thanks for the suggestion.
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 :)
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.
0 | @@ -0,0 +1,69 @@ 1 | +#!/usr/bin/env python3 2 | +# Copyright (c) 2017-2020 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 | +"""Test that mempool.dat is compatible between versions 6 | + 7 | +Download node binaries: 8 | +contrib/devtools/previous_release.sh -b v0.19.1
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.
20 | + self.skip_if_no_wallet() 21 | + self.skip_if_no_previous_releases() 22 | + 23 | + def setup_nodes(self): 24 | + self.add_nodes(self.num_nodes, versions=[ 25 | + 190100,
Suggest documenting what's interesting about that version, could be a link to Github issue.
0 | @@ -0,0 +1,69 @@ 1 | +#!/usr/bin/env python3 2 | +# Copyright (c) 2017-2020 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 | +"""Test that mempool.dat is compatible between versions
Concept ACK. f0b2579 looks good modulo some comments.
Suggest making a separate PR for cd58338, ideally with a test.
@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.
Suggest making a separate PR for cd58338, ideally with a test.
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 :-)
Squashed the changes, removed log commit
31 | + def setup_network(self): 32 | + self.add_nodes(self.num_nodes, versions=[ 33 | + 150200, # oldest version supported by the test framework 34 | + None, 35 | + ]) 36 | + adjust_bitcoin_conf_for_pre_17(self.nodes[0].bitcoinconf)
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.
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.
tACK 16d4b3fd6d5aad18ebb731a5006a15180d3661ef
P.S. for increased glory you could sign your commits