Merge bitcoin/bitcoin#25285: Add AutoFile without ser-type and ser-version and use it where possible

facc2fa7b8 Use AutoFile where possible (MacroFake)
6666803c89 streams: Add AutoFile without ser-type and ser-version (MacroFake)

Pull request description:

  This was done in the context of https://github.com/bitcoin/bitcoin/pull/25284 , but I think it also makes sense standalone.

  The basic idea is that serialization type should not be initialized when it is not needed. Same for the serialization version.

  So do this here for `AutoFile`. `CAutoFile` remains in places where it is not yet possible.

ACKs for top commit:
  laanwj:
    Code review ACK facc2fa7b8
  fanquake:
    ACK facc2fa7b8

Tree-SHA512: d82d024d55af57565ac53d9d1517afafc12b46964effba0332de62a6c77869356fa77f89e6d4834438fff44c45b64fccdf5a1358bfea03e28dfe55013b3c099d
This commit is contained in:
fanquake 2022-07-20 09:13:00 +01:00
commit 895937edb2
No known key found for this signature in database
GPG key ID: 2EEB9F5CC09526C1
19 changed files with 89 additions and 77 deletions

View file

@ -133,7 +133,7 @@ bool BlockFilterIndex::CustomCommit(CDBBatch& batch)
const FlatFilePos& pos = m_next_filter_pos;
// Flush current filter file to disk.
CAutoFile file(m_filter_fileseq->Open(pos), SER_DISK, CLIENT_VERSION);
AutoFile file{m_filter_fileseq->Open(pos)};
if (file.IsNull()) {
return error("%s: Failed to open filter file %d", __func__, pos.nFile);
}
@ -147,7 +147,7 @@ bool BlockFilterIndex::CustomCommit(CDBBatch& batch)
bool BlockFilterIndex::ReadFilterFromDisk(const FlatFilePos& pos, const uint256& hash, BlockFilter& filter) const
{
CAutoFile filein(m_filter_fileseq->Open(pos, true), SER_DISK, CLIENT_VERSION);
AutoFile filein{m_filter_fileseq->Open(pos, true)};
if (filein.IsNull()) {
return false;
}
@ -179,7 +179,7 @@ size_t BlockFilterIndex::WriteFilterToDisk(FlatFilePos& pos, const BlockFilter&
// If writing the filter would overflow the file, flush and move to the next one.
if (pos.nPos + data_size > MAX_FLTR_FILE_SIZE) {
CAutoFile last_file(m_filter_fileseq->Open(pos), SER_DISK, CLIENT_VERSION);
AutoFile last_file{m_filter_fileseq->Open(pos)};
if (last_file.IsNull()) {
LogPrintf("%s: Failed to open filter file %d\n", __func__, pos.nFile);
return 0;
@ -205,7 +205,7 @@ size_t BlockFilterIndex::WriteFilterToDisk(FlatFilePos& pos, const BlockFilter&
return 0;
}
CAutoFile fileout(m_filter_fileseq->Open(pos), SER_DISK, CLIENT_VERSION);
AutoFile fileout{m_filter_fileseq->Open(pos)};
if (fileout.IsNull()) {
LogPrintf("%s: Failed to open filter file %d\n", __func__, pos.nFile);
return 0;

View file

@ -2825,7 +2825,7 @@ void CaptureMessageToFile(const CAddress& addr,
fs::create_directories(base_path);
fs::path path = base_path / (is_incoming ? "msgs_recv.dat" : "msgs_sent.dat");
CAutoFile f(fsbridge::fopen(path, "ab"), SER_DISK, CLIENT_VERSION);
AutoFile f{fsbridge::fopen(path, "ab")};
ser_writedata64(f, now.count());
f.write(MakeByteSpan(msg_type));

View file

@ -161,13 +161,13 @@ public:
unsigned int GetMaxConfirms() const { return scale * confAvg.size(); }
/** Write state of estimation data to a file*/
void Write(CAutoFile& fileout) const;
void Write(AutoFile& fileout) const;
/**
* Read saved state of estimation data from a file and replace all internal data structures and
* variables with this state.
*/
void Read(CAutoFile& filein, int nFileVersion, size_t numBuckets);
void Read(AutoFile& filein, int nFileVersion, size_t numBuckets);
};
@ -390,7 +390,7 @@ double TxConfirmStats::EstimateMedianVal(int confTarget, double sufficientTxVal,
return median;
}
void TxConfirmStats::Write(CAutoFile& fileout) const
void TxConfirmStats::Write(AutoFile& fileout) const
{
fileout << Using<EncodedDoubleFormatter>(decay);
fileout << scale;
@ -400,7 +400,7 @@ void TxConfirmStats::Write(CAutoFile& fileout) const
fileout << Using<VectorFormatter<VectorFormatter<EncodedDoubleFormatter>>>(failAvg);
}
void TxConfirmStats::Read(CAutoFile& filein, int nFileVersion, size_t numBuckets)
void TxConfirmStats::Read(AutoFile& filein, int nFileVersion, size_t numBuckets)
{
// Read data file and do some very basic sanity checking
// buckets and bucketMap are not updated yet, so don't access them
@ -546,7 +546,7 @@ CBlockPolicyEstimator::CBlockPolicyEstimator(const fs::path& estimation_filepath
longStats = std::unique_ptr<TxConfirmStats>(new TxConfirmStats(buckets, bucketMap, LONG_BLOCK_PERIODS, LONG_DECAY, LONG_SCALE));
// If the fee estimation file is present, read recorded estimations
CAutoFile est_file(fsbridge::fopen(m_estimation_filepath, "rb"), SER_DISK, CLIENT_VERSION);
AutoFile est_file{fsbridge::fopen(m_estimation_filepath, "rb")};
if (est_file.IsNull() || !Read(est_file)) {
LogPrintf("Failed to read fee estimates from %s. Continue anyway.\n", fs::PathToString(m_estimation_filepath));
}
@ -904,13 +904,13 @@ CFeeRate CBlockPolicyEstimator::estimateSmartFee(int confTarget, FeeCalculation
void CBlockPolicyEstimator::Flush() {
FlushUnconfirmed();
CAutoFile est_file(fsbridge::fopen(m_estimation_filepath, "wb"), SER_DISK, CLIENT_VERSION);
AutoFile est_file{fsbridge::fopen(m_estimation_filepath, "wb")};
if (est_file.IsNull() || !Write(est_file)) {
LogPrintf("Failed to write fee estimates to %s. Continue anyway.\n", fs::PathToString(m_estimation_filepath));
}
}
bool CBlockPolicyEstimator::Write(CAutoFile& fileout) const
bool CBlockPolicyEstimator::Write(AutoFile& fileout) const
{
try {
LOCK(m_cs_fee_estimator);
@ -935,7 +935,7 @@ bool CBlockPolicyEstimator::Write(CAutoFile& fileout) const
return true;
}
bool CBlockPolicyEstimator::Read(CAutoFile& filein)
bool CBlockPolicyEstimator::Read(AutoFile& filein)
{
try {
LOCK(m_cs_fee_estimator);

View file

@ -20,7 +20,7 @@
#include <string>
#include <vector>
class CAutoFile;
class AutoFile;
class CTxMemPoolEntry;
class TxConfirmStats;
@ -220,11 +220,11 @@ public:
EXCLUSIVE_LOCKS_REQUIRED(!m_cs_fee_estimator);
/** Write estimation data to a file */
bool Write(CAutoFile& fileout) const
bool Write(AutoFile& fileout) const
EXCLUSIVE_LOCKS_REQUIRED(!m_cs_fee_estimator);
/** Read estimation data from a file */
bool Read(CAutoFile& filein)
bool Read(AutoFile& filein)
EXCLUSIVE_LOCKS_REQUIRED(!m_cs_fee_estimator);
/** Empty mempool transactions on shutdown to record failure to confirm for txs still in mempool */

View file

@ -2307,7 +2307,7 @@ static RPCHelpMan dumptxoutset()
}
FILE* file{fsbridge::fopen(temppath, "wb")};
CAutoFile afile{file, SER_DISK, CLIENT_VERSION};
AutoFile afile{file};
if (afile.IsNull()) {
throw JSONRPCError(
RPC_INVALID_PARAMETER,
@ -2328,7 +2328,7 @@ static RPCHelpMan dumptxoutset()
UniValue CreateUTXOSnapshot(
NodeContext& node,
CChainState& chainstate,
CAutoFile& afile,
AutoFile& afile,
const fs::path& path,
const fs::path& temppath)
{

View file

@ -55,7 +55,7 @@ void CalculatePercentilesByWeight(CAmount result[NUM_GETBLOCKSTATS_PERCENTILES],
UniValue CreateUTXOSnapshot(
node::NodeContext& node,
CChainState& chainstate,
CAutoFile& afile,
AutoFile& afile,
const fs::path& path,
const fs::path& tmppath);

View file

@ -465,35 +465,28 @@ public:
};
/** Non-refcounted RAII wrapper for FILE*
*
* Will automatically close the file when it goes out of scope if not null.
* If you're returning the file pointer, return file.release().
* If you need to close the file early, use file.fclose() instead of fclose(file).
*/
class CAutoFile
class AutoFile
{
private:
const int nType;
const int nVersion;
protected:
FILE* file;
public:
CAutoFile(FILE* filenew, int nTypeIn, int nVersionIn) : nType(nTypeIn), nVersion(nVersionIn)
{
file = filenew;
}
explicit AutoFile(FILE* filenew) : file{filenew} {}
~CAutoFile()
~AutoFile()
{
fclose();
}
// Disallow copies
CAutoFile(const CAutoFile&) = delete;
CAutoFile& operator=(const CAutoFile&) = delete;
AutoFile(const AutoFile&) = delete;
AutoFile& operator=(const AutoFile&) = delete;
void fclose()
{
@ -504,14 +497,14 @@ public:
}
/** Get wrapped FILE* with transfer of ownership.
* @note This will invalidate the CAutoFile object, and makes it the responsibility of the caller
* @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; }
/** 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
* CAutoFile outlives use of the passed pointer.
* AutoFile outlives use of the passed pointer.
*/
FILE* Get() const { return file; }
@ -522,40 +515,62 @@ public:
//
// Stream subset
//
int GetType() const { return nType; }
int GetVersion() const { return nVersion; }
void read(Span<std::byte> dst)
{
if (!file)
throw std::ios_base::failure("CAutoFile::read: file handle is nullptr");
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) ? "CAutoFile::read: end of file" : "CAutoFile::read: fread failed");
throw std::ios_base::failure(feof(file) ? "AutoFile::read: end of file" : "AutoFile::read: fread failed");
}
}
void ignore(size_t nSize)
{
if (!file)
throw std::ios_base::failure("CAutoFile::ignore: file handle is nullptr");
if (!file) throw std::ios_base::failure("AutoFile::ignore: file handle is nullptr");
unsigned char data[4096];
while (nSize > 0) {
size_t nNow = std::min<size_t>(nSize, sizeof(data));
if (fread(data, 1, nNow, file) != nNow)
throw std::ios_base::failure(feof(file) ? "CAutoFile::ignore: end of file" : "CAutoFile::read: fread failed");
throw std::ios_base::failure(feof(file) ? "AutoFile::ignore: end of file" : "AutoFile::read: fread failed");
nSize -= nNow;
}
}
void write(Span<const std::byte> src)
{
if (!file)
throw std::ios_base::failure("CAutoFile::write: file handle is nullptr");
if (!file) throw std::ios_base::failure("AutoFile::write: file handle is nullptr");
if (fwrite(src.data(), 1, src.size(), file) != src.size()) {
throw std::ios_base::failure("CAutoFile::write: write failed");
throw std::ios_base::failure("AutoFile::write: write failed");
}
}
template <typename T>
AutoFile& operator<<(const T& obj)
{
if (!file) throw std::ios_base::failure("AutoFile::operator<<: file handle is nullptr");
::Serialize(*this, obj);
return *this;
}
template <typename T>
AutoFile& operator>>(T&& obj)
{
if (!file) throw std::ios_base::failure("AutoFile::operator>>: file handle is nullptr");
::Unserialize(*this, obj);
return *this;
}
};
class CAutoFile : public AutoFile
{
private:
const int nType;
const int nVersion;
public:
CAutoFile(FILE* filenew, int nTypeIn, int nVersionIn) : AutoFile{filenew}, nType(nTypeIn), nVersion(nVersionIn) {}
int GetType() const { return nType; }
int GetVersion() const { return nVersion; }
template<typename T>
CAutoFile& operator<<(const T& obj)
{

View file

@ -41,26 +41,26 @@ BOOST_AUTO_TEST_CASE(flatfile_open)
// Write first line to file.
{
CAutoFile file(seq.Open(FlatFilePos(0, pos1)), SER_DISK, CLIENT_VERSION);
AutoFile file{seq.Open(FlatFilePos(0, pos1))};
file << LIMITED_STRING(line1, 256);
}
// Attempt to append to file opened in read-only mode.
{
CAutoFile file(seq.Open(FlatFilePos(0, pos2), true), SER_DISK, CLIENT_VERSION);
AutoFile file{seq.Open(FlatFilePos(0, pos2), true)};
BOOST_CHECK_THROW(file << LIMITED_STRING(line2, 256), std::ios_base::failure);
}
// Append second line to file.
{
CAutoFile file(seq.Open(FlatFilePos(0, pos2)), SER_DISK, CLIENT_VERSION);
AutoFile file{seq.Open(FlatFilePos(0, pos2))};
file << LIMITED_STRING(line2, 256);
}
// Read text from file in read-only mode.
{
std::string text;
CAutoFile file(seq.Open(FlatFilePos(0, pos1), true), SER_DISK, CLIENT_VERSION);
AutoFile file{seq.Open(FlatFilePos(0, pos1), true)};
file >> LIMITED_STRING(text, 256);
BOOST_CHECK_EQUAL(text, line1);
@ -72,7 +72,7 @@ BOOST_AUTO_TEST_CASE(flatfile_open)
// Read text from file with position offset.
{
std::string text;
CAutoFile file(seq.Open(FlatFilePos(0, pos2)), SER_DISK, CLIENT_VERSION);
AutoFile file{seq.Open(FlatFilePos(0, pos2))};
file >> LIMITED_STRING(text, 256);
BOOST_CHECK_EQUAL(text, line2);
@ -81,7 +81,7 @@ BOOST_AUTO_TEST_CASE(flatfile_open)
// Ensure another file in the sequence has no data.
{
std::string text;
CAutoFile file(seq.Open(FlatFilePos(1, pos2)), SER_DISK, CLIENT_VERSION);
AutoFile file{seq.Open(FlatFilePos(1, pos2))};
BOOST_CHECK_THROW(file >> LIMITED_STRING(text, 256), std::ios_base::failure);
}
}

View file

@ -18,7 +18,7 @@ FUZZ_TARGET(autofile)
{
FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
FuzzedAutoFileProvider fuzzed_auto_file_provider = ConsumeAutoFile(fuzzed_data_provider);
CAutoFile auto_file = fuzzed_auto_file_provider.open();
AutoFile auto_file{fuzzed_auto_file_provider.open()};
LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 10000) {
CallOneOf(
fuzzed_data_provider,
@ -53,8 +53,6 @@ FUZZ_TARGET(autofile)
});
}
(void)auto_file.Get();
(void)auto_file.GetType();
(void)auto_file.GetVersion();
(void)auto_file.IsNull();
if (fuzzed_data_provider.ConsumeBool()) {
FILE* f = auto_file.release();

View file

@ -76,7 +76,7 @@ FUZZ_TARGET_INIT(policy_estimator, initialize_policy_estimator)
}
{
FuzzedAutoFileProvider fuzzed_auto_file_provider = ConsumeAutoFile(fuzzed_data_provider);
CAutoFile fuzzed_auto_file = fuzzed_auto_file_provider.open();
AutoFile fuzzed_auto_file{fuzzed_auto_file_provider.open()};
block_policy_estimator.Write(fuzzed_auto_file);
block_policy_estimator.Read(fuzzed_auto_file);
}

View file

@ -26,7 +26,7 @@ FUZZ_TARGET_INIT(policy_estimator_io, initialize_policy_estimator_io)
{
FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
FuzzedAutoFileProvider fuzzed_auto_file_provider = ConsumeAutoFile(fuzzed_data_provider);
CAutoFile fuzzed_auto_file = fuzzed_auto_file_provider.open();
AutoFile fuzzed_auto_file{fuzzed_auto_file_provider.open()};
// Re-using block_policy_estimator across runs to avoid costly creation of CBlockPolicyEstimator object.
static CBlockPolicyEstimator block_policy_estimator{FeeestPath(*g_setup->m_node.args)};
if (block_policy_estimator.Read(fuzzed_auto_file)) {

View file

@ -358,17 +358,16 @@ public:
class FuzzedAutoFileProvider
{
FuzzedDataProvider& m_fuzzed_data_provider;
FuzzedFileProvider m_fuzzed_file_provider;
public:
FuzzedAutoFileProvider(FuzzedDataProvider& fuzzed_data_provider) : m_fuzzed_data_provider{fuzzed_data_provider}, m_fuzzed_file_provider{fuzzed_data_provider}
FuzzedAutoFileProvider(FuzzedDataProvider& fuzzed_data_provider) : m_fuzzed_file_provider{fuzzed_data_provider}
{
}
CAutoFile open()
AutoFile open()
{
return {m_fuzzed_file_provider.open(), m_fuzzed_data_provider.ConsumeIntegral<int>(), m_fuzzed_data_provider.ConsumeIntegral<int>()};
return AutoFile{m_fuzzed_file_provider.open()};
}
};

View file

@ -39,13 +39,13 @@ FUZZ_TARGET_INIT(utxo_snapshot, initialize_chain)
Assert(!chainman.SnapshotBlockhash());
{
CAutoFile outfile{fsbridge::fopen(snapshot_path, "wb"), SER_DISK, CLIENT_VERSION};
AutoFile outfile{fsbridge::fopen(snapshot_path, "wb")};
const auto file_data{ConsumeRandomLengthByteVector(fuzzed_data_provider)};
outfile << Span{file_data};
}
const auto ActivateFuzzedSnapshot{[&] {
CAutoFile infile{fsbridge::fopen(snapshot_path, "rb"), SER_DISK, CLIENT_VERSION};
AutoFile infile{fsbridge::fopen(snapshot_path, "rb")};
SnapshotMetadata metadata;
try {
infile >> metadata;

View file

@ -16,7 +16,7 @@
#include <boost/test/unit_test.hpp>
const auto NoMalleation = [](CAutoFile& file, node::SnapshotMetadata& meta){};
const auto NoMalleation = [](AutoFile& file, node::SnapshotMetadata& meta){};
/**
* Create and activate a UTXO snapshot, optionally providing a function to
@ -32,7 +32,7 @@ CreateAndActivateUTXOSnapshot(node::NodeContext& node, const fs::path root, F ma
WITH_LOCK(::cs_main, height = node.chainman->ActiveHeight());
fs::path snapshot_path = root / fs::u8path(tfm::format("test_snapshot.%d.dat", height));
FILE* outfile{fsbridge::fopen(snapshot_path, "wb")};
CAutoFile auto_outfile{outfile, SER_DISK, CLIENT_VERSION};
AutoFile auto_outfile{outfile};
UniValue result = CreateUTXOSnapshot(
node, node.chainman->ActiveChainstate(), auto_outfile, snapshot_path, snapshot_path);
@ -42,7 +42,7 @@ CreateAndActivateUTXOSnapshot(node::NodeContext& node, const fs::path root, F ma
// Read the written snapshot in and then activate it.
//
FILE* infile{fsbridge::fopen(snapshot_path, "rb")};
CAutoFile auto_infile{infile, SER_DISK, CLIENT_VERSION};
AutoFile auto_infile{infile};
node::SnapshotMetadata metadata;
auto_infile >> metadata;

View file

@ -193,7 +193,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_activate_snapshot, TestChain100Setup)
// Should not load malleated snapshots
BOOST_REQUIRE(!CreateAndActivateUTXOSnapshot(
m_node, m_path_root, [](CAutoFile& auto_infile, SnapshotMetadata& metadata) {
m_node, m_path_root, [](AutoFile& auto_infile, SnapshotMetadata& metadata) {
// A UTXO is missing but count is correct
metadata.m_coins_count -= 1;
@ -204,22 +204,22 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_activate_snapshot, TestChain100Setup)
auto_infile >> coin;
}));
BOOST_REQUIRE(!CreateAndActivateUTXOSnapshot(
m_node, m_path_root, [](CAutoFile& auto_infile, SnapshotMetadata& metadata) {
m_node, m_path_root, [](AutoFile& auto_infile, SnapshotMetadata& metadata) {
// Coins count is larger than coins in file
metadata.m_coins_count += 1;
}));
BOOST_REQUIRE(!CreateAndActivateUTXOSnapshot(
m_node, m_path_root, [](CAutoFile& auto_infile, SnapshotMetadata& metadata) {
m_node, m_path_root, [](AutoFile& auto_infile, SnapshotMetadata& metadata) {
// Coins count is smaller than coins in file
metadata.m_coins_count -= 1;
}));
BOOST_REQUIRE(!CreateAndActivateUTXOSnapshot(
m_node, m_path_root, [](CAutoFile& auto_infile, SnapshotMetadata& metadata) {
m_node, m_path_root, [](AutoFile& auto_infile, SnapshotMetadata& metadata) {
// Wrong hash
metadata.m_base_blockhash = uint256::ZERO;
}));
BOOST_REQUIRE(!CreateAndActivateUTXOSnapshot(
m_node, m_path_root, [](CAutoFile& auto_infile, SnapshotMetadata& metadata) {
m_node, m_path_root, [](AutoFile& auto_infile, SnapshotMetadata& metadata) {
// Wrong hash
metadata.m_base_blockhash = uint256::ONE;
}));

View file

@ -198,7 +198,7 @@ std::vector<bool> DecodeAsmap(fs::path path)
{
std::vector<bool> bits;
FILE *filestr = fsbridge::fopen(path, "rb");
CAutoFile file(filestr, SER_DISK, CLIENT_VERSION);
AutoFile file{filestr};
if (file.IsNull()) {
LogPrintf("Failed to open asmap file from disk\n");
return bits;

View file

@ -4721,7 +4721,7 @@ const AssumeutxoData* ExpectedAssumeutxo(
}
bool ChainstateManager::ActivateSnapshot(
CAutoFile& coins_file,
AutoFile& coins_file,
const SnapshotMetadata& metadata,
bool in_memory)
{
@ -4816,7 +4816,7 @@ static void FlushSnapshotToDisk(CCoinsViewCache& coins_cache, bool snapshot_load
bool ChainstateManager::PopulateAndValidateSnapshot(
CChainState& snapshot_chainstate,
CAutoFile& coins_file,
AutoFile& coins_file,
const SnapshotMetadata& metadata)
{
// It's okay to release cs_main before we're done using `coins_cache` because we know

View file

@ -818,7 +818,7 @@ private:
//! Internal helper for ActivateSnapshot().
[[nodiscard]] bool PopulateAndValidateSnapshot(
CChainState& snapshot_chainstate,
CAutoFile& coins_file,
AutoFile& coins_file,
const node::SnapshotMetadata& metadata);
/**
@ -907,7 +907,7 @@ public:
//! - Move the new chainstate to `m_snapshot_chainstate` and make it our
//! ChainstateActive().
[[nodiscard]] bool ActivateSnapshot(
CAutoFile& coins_file, const node::SnapshotMetadata& metadata, bool in_memory);
AutoFile& coins_file, const node::SnapshotMetadata& metadata, bool in_memory);
//! The most-work chain.
CChainState& ActiveChainstate() const;

View file

@ -95,7 +95,7 @@ class AddrmanTest(BitcoinTestFramework):
with open(peers_dat, "wb") as f:
f.write(serialize_addrman()[:-1])
self.nodes[0].assert_start_raises_init_error(
expected_msg=init_error("CAutoFile::read: end of file.*"),
expected_msg=init_error("AutoFile::read: end of file.*"),
match=ErrorMatch.FULL_REGEX,
)