The format string is evaluated differently on modern compilers (clang 10 and later, as well as gcc 10 and later).
Work around the behaviour change in compilers by pinning the underlying type of the format arguments.
Can be tested by observing a failing test when running against master compiled with clang 10 or gcc 10 (or later).
Also add a regression test.
Context for other reviewers: c42f/tinyformat#82
I think the changes here make sense even without the issue (and whether it is a bug or not).
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
53+
54+ self.log.info("Check that addrman from future cannot be read")
55+ self.stop_node(0)
56+ write_addrman(peers_dat, lowest_compatible=111)
57+ with self.nodes[0].assert_debug_log([
58+ f'ERROR: DeserializeDB: Deserialize or I/O error - Unsupported format of addrman database: 1. It is compatible with formats >=111, but the maximum supported by this version of {self.config["environment"]["PACKAGE_NAME"]} is 3.',
nit, line length
0- f'ERROR: DeserializeDB: Deserialize or I/O error - Unsupported format of addrman database: 1. It is compatible with formats >=111, but the maximum supported by this version of {self.config["environment"]["PACKAGE_NAME"]} is 3.',
1+ "ERROR: DeserializeDB: Deserialize or I/O error - Unsupported format of "
2+ "addrman database: 1. It is compatible with formats >=111, but the maximum "
3+ f"supported by this version of {self.config['environment']['PACKAGE_NAME']} is 3.",
ACK fab0b55cf060c2b14fae5cee13f0a2dcaebde892 verified the test fails on master as expected only at line 61 (assertion fixed by the code change); the last two test additions pass as expected
02021-09-06T13:36:53.543000Z TestFramework (INFO): Check that addrman from future cannot be read
12021-09-06T13:36:55.715000Z TestFramework (ERROR): Assertion failed
2Traceback (most recent call last):
3 File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 131, in main
4 self.run_test()
5 File "/home/jon/projects/bitcoin/bitcoin/test/functional/feature_addrman.py", line 61, in run_test
6 self.start_node(0)
7 File "/usr/lib/python3.9/contextlib.py", line 126, in __exit__
8 next(self.gen)
9 File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_node.py", line 400, in assert_debug_log
10 self._raise_assertion_error('Expected messages "{}" does not partially match log:\n\n{}\n\n'.format(str(expected_msgs), print_log))
11 File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_node.py", line 166, in _raise_assertion_error
12 raise AssertionError(self._node_msg(msg))
13AssertionError: [node 0] Expected messages "['ERROR: DeserializeDB: Deserialize or I/O error - Unsupported format of addrman database: 1. It is compatible with formats >=111, but the maximum supported by this version of Bitcoin Core is 3.', 'Recreating peers.dat']" does not partially match log:
14...
15 - 2021-09-06T13:36:53.840640Z [init] [util/system.h:51] [error] ERROR: DeserializeDB: Deserialize or I/O error - Unsupported format of addrman database: \x01. It is compatible with formats >=111, but the maximum supported by this version of Bitcoin Core is 3.: iostream error
241@@ -242,9 +242,9 @@ void CAddrMan::Unserialize(Stream& s_)
242 const uint8_t lowest_compatible = compat - INCOMPATIBILITY_BASE;
243 if (lowest_compatible > FILE_FORMAT) {
244 throw std::ios_base::failure(strprintf(
245- "Unsupported format of addrman database: %u. It is compatible with formats >=%u, "
246- "but the maximum supported by this version of %s is %u.",
247- format, lowest_compatible, PACKAGE_NAME, static_cast<uint8_t>(FILE_FORMAT)));
248+ "Unsupported format of addrman database: %u. It is compatible with formats >=%u, "
249+ "but the maximum supported by this version of %s is %u.",
250+ uint8_t{format}, uint8_t{lowest_compatible}, PACKAGE_NAME, uint8_t{FILE_FORMAT}));
lowest_compatible seems unnecessary.
facce4c looks ok but unrelated.
Right. Didn’t seem worth it to open a separate pull for this “trivial style” fix. I found it while working on this pull, so at least it is loosely related :sweat_smile: