From 9999a49b3299bd25dde4805f5c68adef3876057f Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Mon, 12 Jun 2023 11:28:59 +0200 Subject: [PATCH 1/6] Extract util::Xor, Add key_offset option, Add bench --- src/Makefile.bench.include | 3 ++- src/bench/bench.h | 2 +- src/bench/xor.cpp | 24 ++++++++++++++++++++++++ src/streams.h | 37 ++++++++++++++++++++++--------------- 4 files changed, 49 insertions(+), 17 deletions(-) create mode 100644 src/bench/xor.cpp diff --git a/src/Makefile.bench.include b/src/Makefile.bench.include index 10c8389c800..51bfb1e4593 100644 --- a/src/Makefile.bench.include +++ b/src/Makefile.bench.include @@ -52,7 +52,8 @@ bench_bench_bitcoin_SOURCES = \ bench/streams_findbyte.cpp \ bench/strencodings.cpp \ bench/util_time.cpp \ - bench/verify_script.cpp + bench/verify_script.cpp \ + bench/xor.cpp nodist_bench_bench_bitcoin_SOURCES = $(GENERATED_BENCH_FILES) diff --git a/src/bench/bench.h b/src/bench/bench.h index 78196134e78..6065ddf3fc9 100644 --- a/src/bench/bench.h +++ b/src/bench/bench.h @@ -14,7 +14,7 @@ #include #include -#include +#include // IWYU pragma: export /* * Usage: diff --git a/src/bench/xor.cpp b/src/bench/xor.cpp new file mode 100644 index 00000000000..edda74214a2 --- /dev/null +++ b/src/bench/xor.cpp @@ -0,0 +1,24 @@ +// Copyright (c) The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or https://opensource.org/license/mit/. + +#include + +#include +#include + +#include +#include + +static void Xor(benchmark::Bench& bench) +{ + FastRandomContext frc{/*fDeterministic=*/true}; + auto data{frc.randbytes(1024)}; + auto key{frc.randbytes(31)}; + + bench.batch(data.size()).unit("byte").run([&] { + util::Xor(data, key); + }); +} + +BENCHMARK(Xor, benchmark::PriorityLevel::HIGH); diff --git a/src/streams.h b/src/streams.h index 03df20b5db8..4fbbdc573c9 100644 --- a/src/streams.h +++ b/src/streams.h @@ -23,6 +23,27 @@ #include #include +namespace util { +inline void Xor(Span write, Span key, size_t key_offset = 0) +{ + if (key.size() == 0) { + return; + } + key_offset %= key.size(); + + for (size_t i = 0, j = key_offset; i != write.size(); i++) { + write[i] ^= key[j++]; + + // This potentially acts on very many bytes of data, so it's + // important that we calculate `j`, i.e. the `key` index in this + // way instead of doing a %, which would effectively be a division + // for each byte Xor'd -- much slower than need be. + if (j == key.size()) + j = 0; + } +} +} // namespace util + template class OverrideStream { @@ -316,20 +337,7 @@ public: */ void Xor(const std::vector& key) { - if (key.size() == 0) { - return; - } - - for (size_type i = 0, j = 0; i != size(); i++) { - vch[i] ^= std::byte{key[j++]}; - - // This potentially acts on very many bytes of data, so it's - // important that we calculate `j`, i.e. the `key` index in this - // way instead of doing a %, which would effectively be a division - // for each byte Xor'd -- much slower than need be. - if (j == key.size()) - j = 0; - } + util::Xor(MakeWritableByteSpan(*this), MakeByteSpan(key)); } }; @@ -469,7 +477,6 @@ public: } }; - /** Non-refcounted RAII wrapper for FILE* * * Will automatically close the file when it goes out of scope if not null. From fafe2ca0ce842cd8f0d8135fa8c8bac9b2c72da6 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Wed, 12 Jul 2023 08:54:00 +0200 Subject: [PATCH 2/6] refactor: Remove redundant file check from AutoFile shift operators The shift operators will call the write or read member function, which already does the check. Also, call sites are free to directly call ::(Un)Serialize(s, obj) to skip this check, so removing it increases consistency. --- src/streams.h | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/streams.h b/src/streams.h index 4fbbdc573c9..89068bff0dd 100644 --- a/src/streams.h +++ b/src/streams.h @@ -560,7 +560,6 @@ public: template AutoFile& operator<<(const T& obj) { - if (!file) throw std::ios_base::failure("AutoFile::operator<<: file handle is nullptr"); ::Serialize(*this, obj); return *this; } @@ -568,7 +567,6 @@ public: template AutoFile& operator>>(T&& obj) { - if (!file) throw std::ios_base::failure("AutoFile::operator>>: file handle is nullptr"); ::Unserialize(*this, obj); return *this; } @@ -588,9 +586,6 @@ public: template CAutoFile& operator<<(const T& obj) { - // Serialize to this stream - if (!file) - throw std::ios_base::failure("CAutoFile::operator<<: file handle is nullptr"); ::Serialize(*this, obj); return (*this); } @@ -598,9 +593,6 @@ public: template CAutoFile& operator>>(T&& obj) { - // Unserialize from this stream - if (!file) - throw std::ios_base::failure("CAutoFile::operator>>: file handle is nullptr"); ::Unserialize(*this, obj); return (*this); } From fa8d227d58f7baa5a9be1b88930f4813bf6eedb1 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Wed, 12 Jul 2023 08:52:36 +0200 Subject: [PATCH 3/6] doc: Remove comments that just repeat what the code does No need to artificially bloat the code and waste space. --- src/hash.h | 2 -- src/streams.h | 7 ------- 2 files changed, 9 deletions(-) diff --git a/src/hash.h b/src/hash.h index 2e3ed11b438..89c6f0dab9a 100644 --- a/src/hash.h +++ b/src/hash.h @@ -160,7 +160,6 @@ public: template CHashWriter& operator<<(const T& obj) { - // Serialize to this stream ::Serialize(*this, obj); return (*this); } @@ -228,7 +227,6 @@ public: template CHashVerifier& operator>>(T&& obj) { - // Unserialize from this stream ::Unserialize(*this, obj); return (*this); } diff --git a/src/streams.h b/src/streams.h index 89068bff0dd..7806eeaf98a 100644 --- a/src/streams.h +++ b/src/streams.h @@ -58,7 +58,6 @@ public: template OverrideStream& operator<<(const T& obj) { - // Serialize to this stream ::Serialize(*this, obj); return (*this); } @@ -66,7 +65,6 @@ public: template OverrideStream& operator>>(T&& obj) { - // Unserialize from this stream ::Unserialize(*this, obj); return (*this); } @@ -131,7 +129,6 @@ class CVectorWriter template CVectorWriter& operator<<(const T& obj) { - // Serialize to this stream ::Serialize(*this, obj); return (*this); } @@ -172,7 +169,6 @@ public: template SpanReader& operator>>(T&& obj) { - // Unserialize from this stream ::Unserialize(*this, obj); return (*this); } @@ -317,7 +313,6 @@ public: template DataStream& operator<<(const T& obj) { - // Serialize to this stream ::Serialize(*this, obj); return (*this); } @@ -325,7 +320,6 @@ public: template DataStream& operator>>(T&& obj) { - // Unserialize from this stream ::Unserialize(*this, obj); return (*this); } @@ -741,7 +735,6 @@ public: template CBufferedFile& operator>>(T&& obj) { - // Unserialize from this stream ::Unserialize(*this, obj); return (*this); } From fa7724bc9d94c08d8facccd0a067d6a3b27fbbc6 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Wed, 12 Jul 2023 09:40:24 +0200 Subject: [PATCH 4/6] refactor: Modernize AutoFile * Add m_ prefix to the std::FILE member variable * Add std namespace where possible, to avoid confusion with member functions of the same name. * Add AutoFile::feof() member function, to be used in place of std::feof(AutoFile::Get()) * Simplify fclose() in terms of release() * Fix typo in the error message in the ignore member function. --- src/streams.h | 47 ++++++++++++++++++++++++----------------------- 1 file changed, 24 insertions(+), 23 deletions(-) diff --git a/src/streams.h b/src/streams.h index 7806eeaf98a..f9bcf0fe144 100644 --- a/src/streams.h +++ b/src/streams.h @@ -480,73 +480,74 @@ public: class AutoFile { protected: - FILE* file; + std::FILE* m_file; public: - explicit AutoFile(FILE* filenew) : file{filenew} {} + explicit AutoFile(std::FILE* file) : m_file{file} {} - ~AutoFile() - { - fclose(); - } + ~AutoFile() { fclose(); } // Disallow copies AutoFile(const AutoFile&) = delete; AutoFile& operator=(const AutoFile&) = delete; + bool feof() const { return std::feof(m_file); } + int fclose() { - int retval{0}; - if (file) { - retval = ::fclose(file); - file = nullptr; - } - return retval; + if (auto rel{release()}) return std::fclose(rel); + return 0; } /** Get wrapped FILE* with transfer of ownership. * @note This will invalidate the AutoFile object, and makes it the responsibility of the caller * of this function to clean up the returned FILE*. */ - FILE* release() { FILE* ret = file; file = nullptr; return ret; } + std::FILE* release() + { + std::FILE* ret{m_file}; + m_file = nullptr; + return ret; + } /** Get wrapped FILE* without transfer of ownership. * @note Ownership of the FILE* will remain with this class. Use this only if the scope of the * AutoFile outlives use of the passed pointer. */ - FILE* Get() const { return file; } + std::FILE* Get() const { return m_file; } /** Return true if the wrapped FILE* is nullptr, false otherwise. */ - bool IsNull() const { return (file == nullptr); } + bool IsNull() const { return m_file == nullptr; } // // Stream subset // void read(Span dst) { - if (!file) throw std::ios_base::failure("AutoFile::read: file handle is nullptr"); - if (fread(dst.data(), 1, dst.size(), file) != dst.size()) { - throw std::ios_base::failure(feof(file) ? "AutoFile::read: end of file" : "AutoFile::read: fread failed"); + if (!m_file) throw std::ios_base::failure("AutoFile::read: file handle is nullptr"); + if (std::fread(dst.data(), 1, dst.size(), m_file) != dst.size()) { + throw std::ios_base::failure(feof() ? "AutoFile::read: end of file" : "AutoFile::read: fread failed"); } } void ignore(size_t nSize) { - if (!file) throw std::ios_base::failure("AutoFile::ignore: file handle is nullptr"); + if (!m_file) throw std::ios_base::failure("AutoFile::ignore: file handle is nullptr"); unsigned char data[4096]; while (nSize > 0) { size_t nNow = std::min(nSize, sizeof(data)); - if (fread(data, 1, nNow, file) != nNow) - throw std::ios_base::failure(feof(file) ? "AutoFile::ignore: end of file" : "AutoFile::read: fread failed"); + if (std::fread(data, 1, nNow, m_file) != nNow) { + throw std::ios_base::failure(feof() ? "AutoFile::ignore: end of file" : "AutoFile::ignore: fread failed"); + } nSize -= nNow; } } void write(Span src) { - if (!file) throw std::ios_base::failure("AutoFile::write: file handle is nullptr"); - if (fwrite(src.data(), 1, src.size(), file) != src.size()) { + if (!m_file) throw std::ios_base::failure("AutoFile::write: file handle is nullptr"); + if (std::fwrite(src.data(), 1, src.size(), m_file) != src.size()) { throw std::ios_base::failure("AutoFile::write: write failed"); } } From 000019e158ef01f2bedc3fc1589f95e106e817ea Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Wed, 5 Jul 2023 12:44:12 +0200 Subject: [PATCH 5/6] Add AutoFile::detail_fread member function New code can call the method without having first to retrieve the raw FILE* pointer via Get(). Also, move implementation to the cpp file. Can be reviewed with: --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space --- src/Makefile.am | 2 ++ src/streams.cpp | 40 ++++++++++++++++++++++++++++++++++++++++ src/streams.h | 35 +++++++---------------------------- 3 files changed, 49 insertions(+), 28 deletions(-) create mode 100644 src/streams.cpp diff --git a/src/Makefile.am b/src/Makefile.am index 4e9c161c57d..cdf2df4f2e3 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -715,6 +715,7 @@ libbitcoin_util_a_SOURCES = \ logging.cpp \ random.cpp \ randomenv.cpp \ + streams.cpp \ support/cleanse.cpp \ sync.cpp \ util/asmap.cpp \ @@ -958,6 +959,7 @@ libbitcoinkernel_la_SOURCES = \ script/standard.cpp \ shutdown.cpp \ signet.cpp \ + streams.cpp \ support/cleanse.cpp \ support/lockedpool.cpp \ sync.cpp \ diff --git a/src/streams.cpp b/src/streams.cpp new file mode 100644 index 00000000000..16a8e517228 --- /dev/null +++ b/src/streams.cpp @@ -0,0 +1,40 @@ +// Copyright (c) 2009-present The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or https://opensource.org/license/mit/. + +#include +#include + +std::size_t AutoFile::detail_fread(Span dst) +{ + if (!m_file) throw std::ios_base::failure("AutoFile::read: file handle is nullptr"); + return std::fread(dst.data(), 1, dst.size(), m_file); +} + +void AutoFile::read(Span dst) +{ + if (detail_fread(dst) != dst.size()) { + throw std::ios_base::failure(feof() ? "AutoFile::read: end of file" : "AutoFile::read: fread failed"); + } +} + +void AutoFile::ignore(size_t nSize) +{ + if (!m_file) throw std::ios_base::failure("AutoFile::ignore: file handle is nullptr"); + unsigned char data[4096]; + while (nSize > 0) { + size_t nNow = std::min(nSize, sizeof(data)); + if (std::fread(data, 1, nNow, m_file) != nNow) { + throw std::ios_base::failure(feof() ? "AutoFile::ignore: end of file" : "AutoFile::ignore: fread failed"); + } + nSize -= nNow; + } +} + +void AutoFile::write(Span src) +{ + if (!m_file) throw std::ios_base::failure("AutoFile::write: file handle is nullptr"); + if (std::fwrite(src.data(), 1, src.size(), m_file) != src.size()) { + throw std::ios_base::failure("AutoFile::write: write failed"); + } +} diff --git a/src/streams.h b/src/streams.h index f9bcf0fe144..27875775a7f 100644 --- a/src/streams.h +++ b/src/streams.h @@ -13,6 +13,7 @@ #include #include +#include #include #include #include @@ -520,37 +521,15 @@ public: */ bool IsNull() const { return m_file == nullptr; } + /** Implementation detail, only used internally. */ + std::size_t detail_fread(Span dst); + // // Stream subset // - void read(Span dst) - { - if (!m_file) throw std::ios_base::failure("AutoFile::read: file handle is nullptr"); - if (std::fread(dst.data(), 1, dst.size(), m_file) != dst.size()) { - throw std::ios_base::failure(feof() ? "AutoFile::read: end of file" : "AutoFile::read: fread failed"); - } - } - - void ignore(size_t nSize) - { - if (!m_file) throw std::ios_base::failure("AutoFile::ignore: file handle is nullptr"); - unsigned char data[4096]; - while (nSize > 0) { - size_t nNow = std::min(nSize, sizeof(data)); - if (std::fread(data, 1, nNow, m_file) != nNow) { - throw std::ios_base::failure(feof() ? "AutoFile::ignore: end of file" : "AutoFile::ignore: fread failed"); - } - nSize -= nNow; - } - } - - void write(Span src) - { - if (!m_file) throw std::ios_base::failure("AutoFile::write: file handle is nullptr"); - if (std::fwrite(src.data(), 1, src.size(), m_file) != src.size()) { - throw std::ios_base::failure("AutoFile::write: write failed"); - } - } + void read(Span dst); + void ignore(size_t nSize); + void write(Span src); template AutoFile& operator<<(const T& obj) From fa633aa6906f3b130b691568bcd20b2b76bb1cbb Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Mon, 12 Jun 2023 18:30:23 +0200 Subject: [PATCH 6/6] streams: Teach AutoFile how to XOR --- src/streams.cpp | 32 +++++++++++++++++++++--- src/streams.h | 5 ++-- src/test/streams_tests.cpp | 50 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 82 insertions(+), 5 deletions(-) diff --git a/src/streams.cpp b/src/streams.cpp index 16a8e517228..6921dad6773 100644 --- a/src/streams.cpp +++ b/src/streams.cpp @@ -5,10 +5,20 @@ #include #include +#include + std::size_t AutoFile::detail_fread(Span dst) { if (!m_file) throw std::ios_base::failure("AutoFile::read: file handle is nullptr"); - return std::fread(dst.data(), 1, dst.size(), m_file); + if (m_xor.empty()) { + return std::fread(dst.data(), 1, dst.size(), m_file); + } else { + const auto init_pos{std::ftell(m_file)}; + if (init_pos < 0) throw std::ios_base::failure("AutoFile::read: ftell failed"); + std::size_t ret{std::fread(dst.data(), 1, dst.size(), m_file)}; + util::Xor(dst.subspan(0, ret), m_xor, init_pos); + return ret; + } } void AutoFile::read(Span dst) @@ -34,7 +44,23 @@ void AutoFile::ignore(size_t nSize) void AutoFile::write(Span src) { if (!m_file) throw std::ios_base::failure("AutoFile::write: file handle is nullptr"); - if (std::fwrite(src.data(), 1, src.size(), m_file) != src.size()) { - throw std::ios_base::failure("AutoFile::write: write failed"); + if (m_xor.empty()) { + if (std::fwrite(src.data(), 1, src.size(), m_file) != src.size()) { + throw std::ios_base::failure("AutoFile::write: write failed"); + } + } else { + auto current_pos{std::ftell(m_file)}; + if (current_pos < 0) throw std::ios_base::failure("AutoFile::write: ftell failed"); + std::array buf; + while (src.size() > 0) { + auto buf_now{Span{buf}.first(std::min(src.size(), buf.size()))}; + std::copy(src.begin(), src.begin() + buf_now.size(), buf_now.begin()); + util::Xor(buf_now, m_xor, current_pos); + if (std::fwrite(buf_now.data(), 1, buf_now.size(), m_file) != buf_now.size()) { + throw std::ios_base::failure{"XorFile::write: failed"}; + } + src = src.subspan(buf_now.size()); + current_pos += buf_now.size(); + } } } diff --git a/src/streams.h b/src/streams.h index 27875775a7f..5ff952be76d 100644 --- a/src/streams.h +++ b/src/streams.h @@ -482,9 +482,10 @@ class AutoFile { protected: std::FILE* m_file; + const std::vector m_xor; public: - explicit AutoFile(std::FILE* file) : m_file{file} {} + explicit AutoFile(std::FILE* file, std::vector data_xor={}) : m_file{file}, m_xor{std::move(data_xor)} {} ~AutoFile() { fclose(); } @@ -553,7 +554,7 @@ private: const int nVersion; public: - CAutoFile(FILE* filenew, int nTypeIn, int nVersionIn) : AutoFile{filenew}, nType(nTypeIn), nVersion(nVersionIn) {} + explicit CAutoFile(std::FILE* file, int type, int version, std::vector data_xor = {}) : AutoFile{file, std::move(data_xor)}, nType{type}, nVersion{version} {} int GetType() const { return nType; } int GetVersion() const { return nVersion; } diff --git a/src/test/streams_tests.cpp b/src/test/streams_tests.cpp index 55e4f200b1c..52321758245 100644 --- a/src/test/streams_tests.cpp +++ b/src/test/streams_tests.cpp @@ -6,6 +6,7 @@ #include #include #include +#include #include @@ -13,6 +14,55 @@ using namespace std::string_literals; BOOST_FIXTURE_TEST_SUITE(streams_tests, BasicTestingSetup) +BOOST_AUTO_TEST_CASE(xor_file) +{ + fs::path xor_path{m_args.GetDataDirBase() / "test_xor.bin"}; + auto raw_file{[&](const auto& mode) { return fsbridge::fopen(xor_path, mode); }}; + const std::vector test1{1, 2, 3}; + const std::vector test2{4, 5}; + const std::vector xor_pat{std::byte{0xff}, std::byte{0x00}}; + { + // Check errors for missing file + AutoFile xor_file{raw_file("rb"), xor_pat}; + BOOST_CHECK_EXCEPTION(xor_file << std::byte{}, std::ios_base::failure, HasReason{"AutoFile::write: file handle is nullpt"}); + BOOST_CHECK_EXCEPTION(xor_file >> std::byte{}, std::ios_base::failure, HasReason{"AutoFile::read: file handle is nullpt"}); + BOOST_CHECK_EXCEPTION(xor_file.ignore(1), std::ios_base::failure, HasReason{"AutoFile::ignore: file handle is nullpt"}); + } + { + AutoFile xor_file{raw_file("wbx"), xor_pat}; + xor_file << test1 << test2; + } + { + // Read raw from disk + AutoFile non_xor_file{raw_file("rb")}; + std::vector raw(7); + non_xor_file >> Span{raw}; + BOOST_CHECK_EQUAL(HexStr(raw), "fc01fd03fd04fa"); + // Check that no padding exists + BOOST_CHECK_EXCEPTION(non_xor_file.ignore(1), std::ios_base::failure, HasReason{"AutoFile::ignore: end of file"}); + } + { + AutoFile xor_file{raw_file("rb"), xor_pat}; + std::vector read1, read2; + xor_file >> read1 >> read2; + BOOST_CHECK_EQUAL(HexStr(read1), HexStr(test1)); + BOOST_CHECK_EQUAL(HexStr(read2), HexStr(test2)); + // Check that eof was reached + BOOST_CHECK_EXCEPTION(xor_file >> std::byte{}, std::ios_base::failure, HasReason{"AutoFile::read: end of file"}); + } + { + AutoFile xor_file{raw_file("rb"), xor_pat}; + std::vector read2; + // Check that ignore works + xor_file.ignore(4); + xor_file >> read2; + BOOST_CHECK_EQUAL(HexStr(read2), HexStr(test2)); + // Check that ignore and read fail now + BOOST_CHECK_EXCEPTION(xor_file.ignore(1), std::ios_base::failure, HasReason{"AutoFile::ignore: end of file"}); + BOOST_CHECK_EXCEPTION(xor_file >> std::byte{}, std::ios_base::failure, HasReason{"AutoFile::read: end of file"}); + } +} + BOOST_AUTO_TEST_CASE(streams_vector_writer) { unsigned char a(1);