istream_iterator eats whitespace charactesr which causes parsing failures for PSBTs that contain the bytes corresponding to those characters. istreambuf_iterator is the correct thing to use here.
This is a regression in 24.0. https://github.com/bitcoin/bitcoin/pull/25001 accidentally changed the original istreambuf_iterator to istream_iterator.
istream_iterator eats whitespace charactesr which causes parsing
failures for PSBTs that contain the bytes corresponding to those
characters.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | furszy, MarcoFalke |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Tested ACK bb5ea1d9
Not needed but wouldn’t hurt to decouple the file read functionality into a util function, so this bug can be covered in a unit test.
211@@ -212,7 +212,7 @@ void WalletFrame::gotoLoadPSBT(bool from_clipboard)
212 return;
213 }
214 std::ifstream in{filename.toLocal8Bit().data(), std::ios::binary};
215- data.assign(std::istream_iterator<unsigned char>{in}, {});
216+ data.assign(std::istreambuf_iterator<char>{in}, {});
This change reverts https://github.com/bitcoin/bitcoin/commit/a65931e3ce66d87b8f83d67ecdbb46f137e6a670 partially, in particular:
std::istream_iterator –> std::istreambuf_iteratorunsigned char –> charBut the local std::vector data is still parameterized by unsigned char: https://github.com/bitcoin-core/gui/blob/bb5ea1d9a954b7b9f443ee8fbbb04549cd0b08a7/src/qt/walletframe.cpp#L195
Is it on purpose?
unsigned char does not appear to work (fails to compile). But I don’t think that data type makes a difference here.
Yeah, std::ifstream is std::basic_ifstream<char>, and passing that to std::istreambuf_iterator can only be done if it is templated <char> as well. C++ (17?) derives this automatically, so you can also write:
0data.assign(std::istreambuf_iterator{in}, {});
This is a regression in 24.0.
It would be nice to add a test to prevent such a regression in the future. @sipa @MarcoFalke
Perhaps you could be interested in reviewing of this PR as an author/a committer of https://github.com/bitcoin/bitcoin/commit/a65931e3ce66d87b8f83d67ecdbb46f137e6a670 in https://github.com/bitcoin/bitcoin/pull/25001.
Docs: https://en.cppreference.com/w/cpp/iterator/istream_iterator#Notes
review ACK bb5ea1d9a954b7b9f443ee8fbbb04549cd0b08a7 🍇
Signature:
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3review ACK bb5ea1d9a954b7b9f443ee8fbbb04549cd0b08a7 🍇
4-----BEGIN PGP SIGNATURE-----
5
6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
7pUg34wv+NFe6qKxea+Z5Jx0PTNpbWXv+nTGMQYsOek6a+pJIe00MuJGO4XIuDJS0
8Mffqqi8fhC83It02w8Z6q9zLSXB094GabCYgFanN5g0AxDdwgvjrEM5nQ9cZj3Vo
9hJHDzVpdgag5i311J+05yeHig9EOnRjxBRgf1p3pv+/Tg3yS/M7UIRKdpmqAxfOA
10HdPMjRQr+Tjm7+/VHwJJpEnE0fPLUZX5NjblKUmG4wDIwlcqz0qYUK8JkAtznYz9
11JJvXxOayoGKNk3IF/UfZs0/cmGlPh+5AJ3asnSIizAITiwIOFaNAIUfprgc4d86V
12fo66d2avigfwHVpKsLsbQep52I0deYo1dItSGvhcYxhtunB/D8eBsMi8b5kk489j
13kV6i1kP/VtUVu2v2Pkgl632aupAcWM62gY3YCl50IfqNfpMgf0uG58NEScZFHmbp
14920c3zvXD9n7SDx3GVQPljssLPBG7WMUfBYmwYP0NtWx28qv250dThWYnOUUoour
151GDZDw4r
16=QuKV
17-----END PGP SIGNATURE-----
