diff --git a/ci/README.md b/ci/README.md index aa89a179f74b..901bb1643d14 100644 --- a/ci/README.md +++ b/ci/README.md @@ -10,7 +10,7 @@ If the repository is not a fresh git clone, you might have to clean files from p The ci needs to perform various sysadmin tasks such as installing packages or writing to the user's home directory. While most of the actions are done inside a docker container, this is not possible for all. Thus, cache directories, such as the depends cache, previous release binaries, or ccache, are mounted as read-write into the docker container. While it should be fine to run -the ci system locally on you development box, the ci scripts can generally be assumed to have received less review and +the ci system locally on your development box, the ci scripts can generally be assumed to have received less review and testing compared to other parts of the codebase. If you want to keep the work tree clean, you might want to run the ci system in a virtual machine with a Linux operating system of your choice. diff --git a/contrib/guix/README.md b/contrib/guix/README.md index cb7eb134a2e6..602ab1f2ea86 100644 --- a/contrib/guix/README.md +++ b/contrib/guix/README.md @@ -247,7 +247,7 @@ details. * _**SDK_PATH**_ Set the path where _extracted_ SDKs can be found. This is passed through to - the depends tree. Note that this is should be set to the _parent_ directory of + the depends tree. Note that this should be set to the _parent_ directory of the actual SDK (e.g. `SDK_PATH=$HOME/Downloads/macOS-SDKs` instead of `$HOME/Downloads/macOS-SDKs/Xcode-26.1.1-17B100-extracted-SDK-with-libcxx-headers`). diff --git a/contrib/tracing/README.md b/contrib/tracing/README.md index 4e8b0c47936c..8c293e25df9a 100644 --- a/contrib/tracing/README.md +++ b/contrib/tracing/README.md @@ -155,7 +155,7 @@ $ python3 contrib/tracing/log_raw_p2p_msgs.py ./src/dashd ``` Logging raw P2P messages. -Messages larger that about 32kb will be cut off! +Messages larger than about 32kb will be cut off! Some messages might be lost! outbound msg 'inv' from peer 4 (outbound-full-relay, XX.XXX.XX.4:8333) with 253 bytes: 0705000000be2245c8f844c9f763748e1a7… … diff --git a/contrib/tracing/log_raw_p2p_msgs.py b/contrib/tracing/log_raw_p2p_msgs.py index 05428968f8fb..beb311cf00bd 100755 --- a/contrib/tracing/log_raw_p2p_msgs.py +++ b/contrib/tracing/log_raw_p2p_msgs.py @@ -166,7 +166,7 @@ def handle_outbound(_, data, size): bpf["outbound_messages"].open_perf_buffer(handle_outbound) print("Logging raw P2P messages.") - print("Messages larger that about 32kb will be cut off!") + print("Messages larger than about 32kb will be cut off!") print("Some messages might be lost!") while True: try: diff --git a/doc/release-notes-27302.md b/doc/release-notes-27302.md new file mode 100644 index 000000000000..e67a6c8b061e --- /dev/null +++ b/doc/release-notes-27302.md @@ -0,0 +1,4 @@ +Configuration +--- + +- `bitcoind` and `bitcoin-qt` will now raise an error on startup if a datadir that is being used contains a bitcoin.conf file that will be ignored, which can happen when a datadir= line is used in a bitcoin.conf file. The error message is just a diagnostic intended to prevent accidental misconfiguration, and it can be disabled to restore the previous behavior of using the datadir while ignoring the bitcoin.conf contained in it. diff --git a/src/Makefile.test.include b/src/Makefile.test.include index b7d7cc5cde14..a424f9ddac7e 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -332,6 +332,7 @@ test_fuzz_fuzz_SOURCES = \ test/fuzz/netbase_dns_lookup.cpp \ test/fuzz/node_eviction.cpp \ test/fuzz/p2p_transport_serialization.cpp \ + test/fuzz/package_eval.cpp \ test/fuzz/parse_hd_keypath.cpp \ test/fuzz/parse_numbers.cpp \ test/fuzz/parse_script.cpp \ diff --git a/src/chain.h b/src/chain.h index 4236e5a7adff..2f2de9243f5e 100644 --- a/src/chain.h +++ b/src/chain.h @@ -10,14 +10,19 @@ #include #include #include +#include #include #include +#include +#include +#include +#include #include /** * Maximum amount of time that a block timestamp is allowed to exceed the - * current network-adjusted time before the block will be accepted. + * current time before the block will be accepted. */ static constexpr int64_t MAX_FUTURE_BLOCK_TIME = 2 * 60 * 60; diff --git a/src/init.cpp b/src/init.cpp index 8d516f2cdb9a..fd91e936042c 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -581,6 +581,7 @@ void SetupServerArgs(ArgsManager& argsman) argsman.AddArg("-dbbatchsize", strprintf("Maximum database write batch size in bytes (default: %u)", nDefaultDbBatchSize), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::OPTIONS); argsman.AddArg("-dbcache=", strprintf("Maximum database cache size MiB (%d to %d, default: %d). In addition, unused mempool memory is shared for this cache (see -maxmempool).", nMinDbCache, nMaxDbCache, nDefaultDbCache), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); argsman.AddArg("-includeconf=", "Specify additional configuration file, relative to the -datadir path (only useable from configuration file, not command line)", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); + argsman.AddArg("-allowignoredconf", strprintf("For backwards compatibility, treat an unused %s file in the datadir as a warning, not an error.", BITCOIN_CONF_FILENAME), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); argsman.AddArg("-loadblock=", "Imports blocks from external file on startup", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); argsman.AddArg("-maxmempool=", strprintf("Keep the transaction memory pool below megabytes (default: %u)", DEFAULT_MAX_MEMPOOL_SIZE), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); argsman.AddArg("-maxorphantxsize=", strprintf("Maximum total size of all orphan transactions in megabytes (default: %u)", DEFAULT_MAX_ORPHAN_TRANSACTIONS_SIZE), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 31d40bd80008..3eee9f011cd8 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -37,6 +37,7 @@ #include #include #include +#include #include #include @@ -65,7 +66,6 @@ #include #include -#include #include #include #include diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index e113e79b86cf..34a085061771 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -400,6 +400,23 @@ bool BlockManager::LoadBlockIndexDB() return true; } +void BlockManager::ScanAndUnlinkAlreadyPrunedFiles() +{ + AssertLockHeld(::cs_main); + if (!m_have_pruned) { + return; + } + + std::set block_files_to_prune; + for (int file_number = 0; file_number < m_last_blockfile; file_number++) { + if (m_blockfile_info[file_number].nSize == 0) { + block_files_to_prune.insert(file_number); + } + } + + UnlinkPrunedFiles(block_files_to_prune); +} + const CBlockIndex* BlockManager::GetLastCheckpoint(const CCheckpointData& data) { const MapCheckpoints& checkpoints = data.mapCheckpoints; @@ -575,11 +592,14 @@ uint64_t BlockManager::CalculateCurrentUsage() void UnlinkPrunedFiles(const std::set& setFilesToPrune) { + std::error_code ec; for (std::set::iterator it = setFilesToPrune.begin(); it != setFilesToPrune.end(); ++it) { FlatFilePos pos(*it, 0); - fs::remove(BlockFileSeq().FileName(pos)); - fs::remove(UndoFileSeq().FileName(pos)); - LogPrint(BCLog::BLOCKSTORE, "Prune: %s deleted blk/rev (%05u)\n", __func__, *it); + const bool removed_blockfile{fs::remove(BlockFileSeq().FileName(pos), ec)}; + const bool removed_undofile{fs::remove(UndoFileSeq().FileName(pos), ec)}; + if (removed_blockfile || removed_undofile) { + LogPrint(BCLog::BLOCKSTORE, "Prune: %s deleted blk/rev (%05u)\n", __func__, *it); + } } } diff --git a/src/node/blockstorage.h b/src/node/blockstorage.h index f8bce3088813..ad05e8b57ba7 100644 --- a/src/node/blockstorage.h +++ b/src/node/blockstorage.h @@ -169,6 +169,13 @@ class BlockManager bool WriteBlockIndexDB() EXCLUSIVE_LOCKS_REQUIRED(::cs_main); bool LoadBlockIndexDB() EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + /** + * Remove any pruned block & undo files that are still on disk. + * This could happen on some systems if the file was still being read while unlinked, + * or if we crash before unlinking. + */ + void ScanAndUnlinkAlreadyPrunedFiles() EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + CBlockIndex* AddToBlockIndex(const CBlockHeader& block, const uint256& hash, CBlockIndex*& best_header, enum BlockStatus nStatus = BLOCK_VALID_TREE) EXCLUSIVE_LOCKS_REQUIRED(cs_main); diff --git a/src/node/miner.cpp b/src/node/miner.cpp index 6b267353cd5f..e85daf48beba 100644 --- a/src/node/miner.cpp +++ b/src/node/miner.cpp @@ -19,9 +19,9 @@ #include #include #include -#include #include #include +#include #include #include diff --git a/src/qt/test/test_main.cpp b/src/qt/test/test_main.cpp index 466c347c19fb..d4a8477bf1ec 100644 --- a/src/qt/test/test_main.cpp +++ b/src/qt/test/test_main.cpp @@ -72,6 +72,9 @@ int main(int argc, char* argv[]) gArgs.ForceSetArg("-upnp", "0"); gArgs.ForceSetArg("-natpmp", "0"); + std::string error; + if (!gArgs.ReadConfigFiles(error, true)) QWARN(error.c_str()); + // Prefer the "minimal" platform for the test instead of the normal default // platform ("xcb", "windows", or "cocoa") so tests can't unintentionally // interfere with any background GUIs and don't require extra resources. diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index 24b63fe83eec..0266da75f188 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -39,6 +39,7 @@ #include #include #include +#include #include #include #include diff --git a/src/test/blockmanager_tests.cpp b/src/test/blockmanager_tests.cpp index 6b29b76e2184..321c86ebfafe 100644 --- a/src/test/blockmanager_tests.cpp +++ b/src/test/blockmanager_tests.cpp @@ -12,6 +12,8 @@ using node::BlockManager; using node::BLOCK_SERIALIZATION_HEADER_SIZE; +using node::MAX_BLOCKFILE_SIZE; +using node::OpenBlockFile; // use BasicTestingSetup here for the data directory configuration, setup, and cleanup BOOST_FIXTURE_TEST_SUITE(blockmanager_tests, BasicTestingSetup) @@ -42,4 +44,45 @@ BOOST_AUTO_TEST_CASE(blockmanager_find_block_pos) BOOST_CHECK_EQUAL(actual.nPos, BLOCK_SERIALIZATION_HEADER_SIZE + ::GetSerializeSize(params->GenesisBlock(), CLIENT_VERSION) + BLOCK_SERIALIZATION_HEADER_SIZE); } +BOOST_FIXTURE_TEST_CASE(blockmanager_scan_unlink_already_pruned_files, TestChain100Setup) +{ + // Cap last block file size, and mine new block in a new block file. + const auto& chainman = Assert(m_node.chainman); + auto& blockman = chainman->m_blockman; + const CBlockIndex* old_tip{WITH_LOCK(chainman->GetMutex(), return chainman->ActiveChain().Tip())}; + WITH_LOCK(chainman->GetMutex(), blockman.GetBlockFileInfo(old_tip->GetBlockPos().nFile)->nSize = MAX_BLOCKFILE_SIZE); + CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())); + + // Prune the older block file, but don't unlink it + int file_number; + { + LOCK(chainman->GetMutex()); + file_number = old_tip->GetBlockPos().nFile; + blockman.PruneOneBlockFile(file_number); + } + + const FlatFilePos pos(file_number, 0); + + // Check that the file is not unlinked after ScanAndUnlinkAlreadyPrunedFiles + // if m_have_pruned is not yet set + WITH_LOCK(chainman->GetMutex(), blockman.ScanAndUnlinkAlreadyPrunedFiles()); + BOOST_CHECK(!AutoFile(OpenBlockFile(pos, true)).IsNull()); + + // Check that the file is unlinked after ScanAndUnlinkAlreadyPrunedFiles + // once m_have_pruned is set + blockman.m_have_pruned = true; + WITH_LOCK(chainman->GetMutex(), blockman.ScanAndUnlinkAlreadyPrunedFiles()); + BOOST_CHECK(AutoFile(OpenBlockFile(pos, true)).IsNull()); + + // Check that calling with already pruned files doesn't cause an error + WITH_LOCK(chainman->GetMutex(), blockman.ScanAndUnlinkAlreadyPrunedFiles()); + + // Check that the new tip file has not been removed + const CBlockIndex* new_tip{WITH_LOCK(chainman->GetMutex(), return chainman->ActiveChain().Tip())}; + BOOST_CHECK_NE(old_tip, new_tip); + const int new_file_number{WITH_LOCK(chainman->GetMutex(), return new_tip->GetBlockPos().nFile)}; + const FlatFilePos new_pos(new_file_number, 0); + BOOST_CHECK(!AutoFile(OpenBlockFile(new_pos, true)).IsNull()); +} + BOOST_AUTO_TEST_SUITE_END() diff --git a/src/test/fuzz/package_eval.cpp b/src/test/fuzz/package_eval.cpp new file mode 100644 index 000000000000..4c81c0b6799b --- /dev/null +++ b/src/test/fuzz/package_eval.cpp @@ -0,0 +1,294 @@ +// Copyright (c) 2023 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +using node::NodeContext; + +namespace { + +const TestingSetup* g_setup; +std::vector g_outpoints_coinbase_init_mature; + +struct MockedTxPool : public CTxMemPool { + void RollingFeeUpdate() EXCLUSIVE_LOCKS_REQUIRED(!cs) + { + LOCK(cs); + lastRollingFeeUpdate = GetTime(); + blockSinceLastRollingFeeBump = true; + } +}; + +void initialize_tx_pool() +{ + static const auto testing_setup = MakeNoLogFileContext(); + g_setup = testing_setup.get(); + + for (int i = 0; i < 2 * COINBASE_MATURITY; ++i) { + COutPoint prevout{MineBlock(g_setup->m_node, P2WSH_OP_TRUE)}; + if (i < COINBASE_MATURITY) { + // Remember the txids to avoid expensive disk access later on + g_outpoints_coinbase_init_mature.push_back(prevout); + } + } + SyncWithValidationInterfaceQueue(); +} + +struct OutpointsUpdater final : public CValidationInterface { + std::set& m_mempool_outpoints; + + explicit OutpointsUpdater(std::set& r) + : m_mempool_outpoints{r} {} + + void TransactionAddedToMempool(const CTransactionRef& tx, uint64_t /* mempool_sequence */) override + { + // for coins spent we always want to be able to rbf so they're not removed + + // outputs from this tx can now be spent + for (uint32_t index{0}; index < tx->vout.size(); ++index) { + m_mempool_outpoints.insert(COutPoint{tx->GetHash(), index}); + } + } + + void TransactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason, uint64_t /* mempool_sequence */) override + { + // outpoints spent by this tx are now available + for (const auto& input : tx->vin) { + // Could already exist if this was a replacement + m_mempool_outpoints.insert(input.prevout); + } + // outpoints created by this tx no longer exist + for (uint32_t index{0}; index < tx->vout.size(); ++index) { + m_mempool_outpoints.erase(COutPoint{tx->GetHash(), index}); + } + } +}; + +struct TransactionsDelta final : public CValidationInterface { + std::set& m_added; + + explicit TransactionsDelta(std::set& a) + : m_added{a} {} + + void TransactionAddedToMempool(const CTransactionRef& tx, uint64_t /* mempool_sequence */) override + { + // Transactions may be entered and booted any number of times + m_added.insert(tx); + } + + void TransactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason, uint64_t /* mempool_sequence */) override + { + // Transactions may be entered and booted any number of times + m_added.erase(tx); + } +}; + +void MockTime(FuzzedDataProvider& fuzzed_data_provider, const Chainstate& chainstate) +{ + const auto time = ConsumeTime(fuzzed_data_provider, + chainstate.m_chain.Tip()->GetMedianTimePast() + 1, + std::numeric_limitsnTime)>::max()); + SetMockTime(time); +} + +CTxMemPool MakeMempool(FuzzedDataProvider& fuzzed_data_provider, const NodeContext& node) +{ + // Take the default options for tests... + CTxMemPool::Options mempool_opts{MemPoolOptionsForTest(node)}; + + + // ...override specific options for this specific fuzz suite + mempool_opts.limits.ancestor_count = fuzzed_data_provider.ConsumeIntegralInRange(0, 50); + mempool_opts.limits.ancestor_size_vbytes = fuzzed_data_provider.ConsumeIntegralInRange(0, 202) * 1'000; + mempool_opts.limits.descendant_count = fuzzed_data_provider.ConsumeIntegralInRange(0, 50); + mempool_opts.limits.descendant_size_vbytes = fuzzed_data_provider.ConsumeIntegralInRange(0, 202) * 1'000; + mempool_opts.max_size_bytes = fuzzed_data_provider.ConsumeIntegralInRange(0, 200) * 1'000'000; + mempool_opts.expiry = std::chrono::hours{fuzzed_data_provider.ConsumeIntegralInRange(0, 999)}; + nBytesPerSigOp = fuzzed_data_provider.ConsumeIntegralInRange(1, 999); + + mempool_opts.estimator = nullptr; + mempool_opts.check_ratio = 1; + mempool_opts.require_standard = fuzzed_data_provider.ConsumeBool(); + + // ...and construct a CTxMemPool from it + return CTxMemPool{mempool_opts}; +} + +FUZZ_TARGET(tx_package_eval, .init = initialize_tx_pool) +{ + FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size()); + const auto& node = g_setup->m_node; + auto& chainstate{static_cast(node.chainman->ActiveChainstate())}; + + MockTime(fuzzed_data_provider, chainstate); + + // All RBF-spendable outpoints outside of the unsubmitted package + std::set mempool_outpoints; + std::map outpoints_value; + for (const auto& outpoint : g_outpoints_coinbase_init_mature) { + Assert(mempool_outpoints.insert(outpoint).second); + outpoints_value[outpoint] = 50 * COIN; + } + + auto outpoints_updater = std::make_shared(mempool_outpoints); + RegisterSharedValidationInterface(outpoints_updater); + + CTxMemPool tx_pool_{MakeMempool(fuzzed_data_provider, node)}; + MockedTxPool& tx_pool = *static_cast(&tx_pool_); + + chainstate.SetMempool(&tx_pool); + + LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 300) + { + Assert(!mempool_outpoints.empty()); + + std::vector txs; + + // Make packages of 1-to-26 transactions + const auto num_txs = (size_t) fuzzed_data_provider.ConsumeIntegralInRange(1, 26); + std::set package_outpoints; + while (txs.size() < num_txs) { + + // Last transaction in a package needs to be a child of parents to get further in validation + // so the last transaction to be generated(in a >1 package) must spend all package-made outputs + // Note that this test currently only spends package outputs in last transaction. + bool last_tx = num_txs > 1 && txs.size() == num_txs - 1; + + // Create transaction to add to the mempool + const CTransactionRef tx = [&] { + CMutableTransaction tx_mut; + tx_mut.nVersion = CTransaction::CURRENT_VERSION; + tx_mut.nLockTime = fuzzed_data_provider.ConsumeBool() ? 0 : fuzzed_data_provider.ConsumeIntegral(); + // Last tx will sweep all outpoints in package + const auto num_in = last_tx ? package_outpoints.size() : fuzzed_data_provider.ConsumeIntegralInRange(1, mempool_outpoints.size()); + const auto num_out = fuzzed_data_provider.ConsumeIntegralInRange(1, mempool_outpoints.size() * 2); + + auto& outpoints = last_tx ? package_outpoints : mempool_outpoints; + + Assert(!outpoints.empty()); + + CAmount amount_in{0}; + for (size_t i = 0; i < num_in; ++i) { + // Pop random outpoint + auto pop = outpoints.begin(); + std::advance(pop, fuzzed_data_provider.ConsumeIntegralInRange(0, outpoints.size() - 1)); + const auto outpoint = *pop; + outpoints.erase(pop); + // no need to update or erase from outpoints_value + amount_in += outpoints_value.at(outpoint); + + // Create input + const auto sequence = ConsumeSequence(fuzzed_data_provider); + const auto script_sig = CScript{}; + const auto script_wit_stack = std::vector>{WITNESS_STACK_ELEM_OP_TRUE}; + CTxIn in; + in.prevout = outpoint; + in.nSequence = sequence; + in.scriptSig = script_sig; + in.scriptWitness.stack = script_wit_stack; + + tx_mut.vin.push_back(in); + } + const auto amount_fee = fuzzed_data_provider.ConsumeIntegralInRange(0, amount_in); + const auto amount_out = (amount_in - amount_fee) / num_out; + for (int i = 0; i < num_out; ++i) { + tx_mut.vout.emplace_back(amount_out, P2WSH_OP_TRUE); + } + // TODO vary transaction sizes to catch size-related issues + auto tx = MakeTransactionRef(tx_mut); + // Restore previously removed outpoints, except in-package outpoints + if (!last_tx) { + for (const auto& in : tx->vin) { + Assert(outpoints.insert(in.prevout).second); + } + // Cache the in-package outpoints being made + for (size_t i = 0; i < tx->vout.size(); ++i) { + package_outpoints.emplace(tx->GetHash(), i); + } + } + // We need newly-created values for the duration of this run + for (size_t i = 0; i < tx->vout.size(); ++i) { + outpoints_value[COutPoint(tx->GetHash(), i)] = tx->vout[i].nValue; + } + return tx; + }(); + txs.push_back(tx); + } + + if (fuzzed_data_provider.ConsumeBool()) { + MockTime(fuzzed_data_provider, chainstate); + } + if (fuzzed_data_provider.ConsumeBool()) { + tx_pool.RollingFeeUpdate(); + } + if (fuzzed_data_provider.ConsumeBool()) { + const auto& txid = fuzzed_data_provider.ConsumeBool() ? + txs.back()->GetHash() : + PickValue(fuzzed_data_provider, mempool_outpoints).hash; + const auto delta = fuzzed_data_provider.ConsumeIntegralInRange(-50 * COIN, +50 * COIN); + tx_pool.PrioritiseTransaction(txid, delta); + } + + // Remember all added transactions + std::set added; + auto txr = std::make_shared(added); + RegisterSharedValidationInterface(txr); + const bool bypass_limits = fuzzed_data_provider.ConsumeBool(); + + // When there are multiple transactions in the package, we call ProcessNewPackage(txs, test_accept=false) + // and AcceptToMemoryPool(txs.back(), test_accept=true). When there is only 1 transaction, we might flip it + // (the package is a test accept and ATMP is a submission). + auto single_submit = txs.size() == 1 && fuzzed_data_provider.ConsumeBool(); + + const auto result_package = WITH_LOCK(::cs_main, + return ProcessNewPackage(chainstate, tx_pool, txs, /*test_accept=*/single_submit)); + // If something went wrong due to a package-specific policy, it might not return a + // validation result for the transaction. + if (result_package.m_state.GetResult() != PackageValidationResult::PCKG_POLICY) { + auto it = result_package.m_tx_results.find(txs.back()->GetWitnessHash()); + Assert(it != result_package.m_tx_results.end()); + Assert(it->second.m_result_type == MempoolAcceptResult::ResultType::VALID || + it->second.m_result_type == MempoolAcceptResult::ResultType::INVALID || + it->second.m_result_type == MempoolAcceptResult::ResultType::MEMPOOL_ENTRY); + } + + const auto res = WITH_LOCK(::cs_main, return AcceptToMemoryPool(chainstate, txs.back(), GetTime(), bypass_limits, /*test_accept=*/!single_submit)); + const bool accepted = res.m_result_type == MempoolAcceptResult::ResultType::VALID; + + SyncWithValidationInterfaceQueue(); + UnregisterSharedValidationInterface(txr); + + // There is only 1 transaction in the package. We did a test-package-accept and a ATMP + if (single_submit) { + Assert(accepted != added.empty()); + Assert(accepted == res.m_state.IsValid()); + if (accepted) { + Assert(added.size() == 1); + Assert(txs.back() == *added.begin()); + } + } else { + // This is empty if it fails early checks, or "full" if transactions are looked at deeper + Assert(result_package.m_tx_results.size() == txs.size() || result_package.m_tx_results.empty()); + } + } + + UnregisterSharedValidationInterface(outpoints_updater); + + WITH_LOCK(::cs_main, tx_pool.check(chainstate.CoinsTip(), chainstate.m_chain.Height() + 1)); +} +} // namespace diff --git a/src/test/util/script.h b/src/test/util/script.h index 81f94afec0ea..1a2b1eb806c8 100644 --- a/src/test/util/script.h +++ b/src/test/util/script.h @@ -16,6 +16,18 @@ static const CScript P2SH_OP_TRUE{ << ToByteVector(CScriptID{CScript{} << OP_TRUE}) << OP_EQUAL}; +static const std::vector EMPTY{}; +static const CScript P2WSH_EMPTY{ + CScript{} + << OP_0 + << ToByteVector([] { + uint256 hash; + CSHA256().Write(EMPTY.data(), EMPTY.size()).Finalize(hash.begin()); + return hash; + }())}; +static const std::vector> P2WSH_EMPTY_TRUE_STACK{{static_cast(OP_TRUE)}, {}}; +static const std::vector> P2WSH_EMPTY_TWO_STACK{{static_cast(OP_2)}, {}}; + /** Flags that are not forbidden by an assert in script validation */ bool IsValidFlagCombination(unsigned flags); diff --git a/src/timedata.h b/src/timedata.h index 3779486c1f88..0b6fcc883293 100644 --- a/src/timedata.h +++ b/src/timedata.h @@ -5,11 +5,8 @@ #ifndef BITCOIN_TIMEDATA_H #define BITCOIN_TIMEDATA_H -#include - #include #include -#include #include #include diff --git a/src/validation.cpp b/src/validation.cpp index b7528e9b41f3..409b446495c3 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -4693,6 +4693,8 @@ bool ChainstateManager::LoadBlockIndex() bool ret{m_blockman.LoadBlockIndexDB()}; if (!ret) return false; + m_blockman.ScanAndUnlinkAlreadyPrunedFiles(); + std::vector vSortedByHeight{m_blockman.GetAllBlockIndices()}; std::sort(vSortedByHeight.begin(), vSortedByHeight.end(), CBlockIndexHeightOnlyComparator()); diff --git a/test/functional/README.md b/test/functional/README.md index 2586c66c8368..76e5d16cceb9 100644 --- a/test/functional/README.md +++ b/test/functional/README.md @@ -37,6 +37,10 @@ don't have test cases for. `set_test_params()`, `add_options()` and `setup_xxxx()` methods at the top of the subclass, then locally-defined helper methods, then the `run_test()` method. - Use `f'{x}'` for string formatting in preference to `'{}'.format(x)` or `'%s' % x`. +- Use `platform.system()` for detecting the running operating system and `os.name` to + check whether it's a POSIX system (see also the `skip_if_platform_not_{linux,posix}` + methods in the `BitcoinTestFramework` class, which can be used to skip a whole test + depending on the platform). #### Naming guidelines diff --git a/test/functional/feature_bind_extra.py b/test/functional/feature_bind_extra.py index 5de9ff203cc2..a6d76854a6d6 100755 --- a/test/functional/feature_bind_extra.py +++ b/test/functional/feature_bind_extra.py @@ -7,15 +7,12 @@ that bind happens on the expected ports. """ -import sys - from test_framework.netutil import ( addr_to_hex, get_bind_addrs, ) from test_framework.test_framework import ( BitcoinTestFramework, - SkipTest, ) from test_framework.util import ( assert_equal, @@ -32,12 +29,11 @@ def set_test_params(self): self.bind_to_localhost_only = False self.num_nodes = 2 - def setup_network(self): + def skip_test_if_missing_module(self): # Due to OS-specific network stats queries, we only run on Linux. - self.log.info("Checking for Linux") - if not sys.platform.startswith('linux'): - raise SkipTest("This test can only be run on Linux.") + self.skip_if_platform_not_linux() + def setup_network(self): loopback_ipv4 = addr_to_hex("127.0.0.1") # Start custom ports by reusing unused p2p ports diff --git a/test/functional/feature_config_args.py b/test/functional/feature_config_args.py index 9bdadfe794c1..672fc615943e 100755 --- a/test/functional/feature_config_args.py +++ b/test/functional/feature_config_args.py @@ -5,9 +5,14 @@ """Test various command line arguments and configuration file parameters.""" import os +import pathlib +import platform +import re +import tempfile import time from test_framework.test_framework import BitcoinTestFramework +from test_framework.test_node import ErrorMatch from test_framework import util @@ -74,7 +79,7 @@ def test_config_file_parser(self): util.write_config(main_conf_file_path, n=0, chain='', extra_config=f'includeconf={inc_conf_file_path}\n') with open(inc_conf_file_path, 'w', encoding='utf-8') as conf: conf.write('acceptnonstdtxn=1\n') - self.nodes[0].assert_start_raises_init_error(extra_args=[f"-conf={main_conf_file_path}"], expected_msg='Error: acceptnonstdtxn is not currently supported for main chain') + self.nodes[0].assert_start_raises_init_error(extra_args=[f"-conf={main_conf_file_path}", "-allowignoredconf"], expected_msg='Error: acceptnonstdtxn is not currently supported for main chain') with open(inc_conf_file_path, 'w', encoding='utf-8') as conf: conf.write('nono\n') @@ -108,6 +113,41 @@ def test_config_file_parser(self): with open(inc_conf_file2_path, 'w', encoding='utf-8') as conf: conf.write('') # clear + def test_config_file_log(self): + # Disable this test for windows currently because trying to override + # the default datadir through the environment does not seem to work. + if platform.system() == "Windows": + return + + self.log.info('Test that correct configuration path is changed when configuration file changes the datadir') + + # Create a temporary directory that will be treated as the default data + # directory by bitcoind. + env, default_datadir = util.get_temp_default_datadir(pathlib.Path(self.options.tmpdir, "test_config_file_log")) + default_datadir.mkdir(parents=True) + + # Write a bitcoin.conf file in the default data directory containing a + # datadir= line pointing at the node datadir. + node = self.nodes[0] + conf_text = pathlib.Path(node.bitcoinconf).read_text() + conf_path = default_datadir / "bitcoin.conf" + conf_path.write_text(f"datadir={node.datadir}\n{conf_text}") + + # Drop the node -datadir= argument during this test, because if it is + # specified it would take precedence over the datadir setting in the + # config file. + node_args = node.args + node.args = [arg for arg in node.args if not arg.startswith("-datadir=")] + + # Check that correct configuration file path is actually logged + # (conf_path, not node.bitcoinconf) + with self.nodes[0].assert_debug_log(expected_msgs=[f"Config file: {conf_path}"]): + self.start_node(0, ["-allowignoredconf"], env=env) + self.stop_node(0) + + # Restore node arguments after the test + node.args = node_args + def test_invalid_command_line_options(self): self.nodes[0].assert_start_raises_init_error( expected_msg='Error: Error parsing command line arguments: Can not set -proxy with no value. Please specify value with -proxy=value.', @@ -278,6 +318,55 @@ def test_connect_with_seednode(self): unexpected_msgs=seednode_ignored): self.restart_node(0, extra_args=[connect_arg, '-seednode=fakeaddress2']) + def test_ignored_conf(self): + self.log.info('Test error is triggered when the datadir in use contains a bitcoin.conf file that would be ignored ' + 'because a conflicting -conf file argument is passed.') + node = self.nodes[0] + with tempfile.NamedTemporaryFile(dir=self.options.tmpdir, mode="wt", delete=False) as temp_conf: + temp_conf.write(f"datadir={node.datadir}\n") + node.assert_start_raises_init_error([f"-conf={temp_conf.name}"], re.escape( + f'Error: Data directory "{node.datadir}" contains a "bitcoin.conf" file which is ignored, because a ' + f'different configuration file "{temp_conf.name}" from command line argument "-conf={temp_conf.name}" ' + f'is being used instead.') + r"[\s\S]*", match=ErrorMatch.FULL_REGEX) + + # Test that passing a redundant -conf command line argument pointing to + # the same bitcoin.conf that would be loaded anyway does not trigger an + # error. + self.start_node(0, [f'-conf={node.datadir}/bitcoin.conf']) + self.stop_node(0) + + def test_ignored_default_conf(self): + # Disable this test for windows currently because trying to override + # the default datadir through the environment does not seem to work. + if platform.system() == "Windows": + return + + self.log.info('Test error is triggered when bitcoin.conf in the default data directory sets another datadir ' + 'and it contains a different bitcoin.conf file that would be ignored') + + # Create a temporary directory that will be treated as the default data + # directory by bitcoind. + env, default_datadir = util.get_temp_default_datadir(pathlib.Path(self.options.tmpdir, "home")) + default_datadir.mkdir(parents=True) + + # Write a bitcoin.conf file in the default data directory containing a + # datadir= line pointing at the node datadir. This will trigger a + # startup error because the node datadir contains a different + # bitcoin.conf that would be ignored. + node = self.nodes[0] + (default_datadir / "bitcoin.conf").write_text(f"datadir={node.datadir}\n") + + # Drop the node -datadir= argument during this test, because if it is + # specified it would take precedence over the datadir setting in the + # config file. + node_args = node.args + node.args = [arg for arg in node.args if not arg.startswith("-datadir=")] + node.assert_start_raises_init_error([], re.escape( + f'Error: Data directory "{node.datadir}" contains a "bitcoin.conf" file which is ignored, because a ' + f'different configuration file "{default_datadir}/bitcoin.conf" from data directory "{default_datadir}" ' + f'is being used instead.') + r"[\s\S]*", env=env, match=ErrorMatch.FULL_REGEX) + node.args = node_args + def run_test(self): self.test_log_buffer() self.test_args_log() @@ -287,7 +376,10 @@ def run_test(self): self.test_config_file_parser() + self.test_config_file_log() self.test_invalid_command_line_options() + self.test_ignored_conf() + self.test_ignored_default_conf() # Remove the -datadir argument so it doesn't override the config file self.nodes[0].args = [arg for arg in self.nodes[0].args if not arg.startswith("-datadir")] diff --git a/test/functional/feature_init.py b/test/functional/feature_init.py index ce12319933de..89d5c1e645df 100755 --- a/test/functional/feature_init.py +++ b/test/functional/feature_init.py @@ -3,9 +3,9 @@ # Distributed under the MIT software license, see the accompanying # file COPYING or http://www.opensource.org/licenses/mit-license.php. """Stress tests related to node initialization.""" -import os from pathlib import Path from random import randint +import platform import shutil from test_framework.test_framework import BitcoinTestFramework, SkipTest @@ -37,7 +37,7 @@ def run_test(self): # and other approaches (like below) don't work: # # os.kill(node.process.pid, signal.CTRL_C_EVENT) - if os.name == 'nt': + if platform.system() == 'Windows': raise SkipTest("can't SIGTERM on Windows") self.stop_node(0) diff --git a/test/functional/feature_notifications.py b/test/functional/feature_notifications.py index c0fd38ddcfad..2e5def424ef6 100755 --- a/test/functional/feature_notifications.py +++ b/test/functional/feature_notifications.py @@ -5,6 +5,7 @@ # file COPYING or http://www.opensource.org/licenses/mit-license.php. """Test the -alertnotify, -blocknotify, -chainlocknotify, -instantsendnotify and -walletnotify options.""" import os +import platform from test_framework.address import ADDRESS_BCRT1_UNSPENDABLE @@ -16,13 +17,13 @@ # Linux allow all characters other than \x00 # Windows disallow control characters (0-31) and /\?%:|"<> -FILE_CHAR_START = 32 if os.name == 'nt' else 1 +FILE_CHAR_START = 32 if platform.system() == 'Windows' else 1 FILE_CHAR_END = 128 -FILE_CHARS_DISALLOWED = '/\\?%*:|"<>' if os.name == 'nt' else '/' +FILE_CHARS_DISALLOWED = '/\\?%*:|"<>' if platform.system() == 'Windows' else '/' UNCONFIRMED_HASH_STRING = 'unconfirmed' def notify_outputname(walletname, txid): - return txid if os.name == 'nt' else f'{walletname}_{txid}' + return txid if platform.system() == 'Windows' else f'{walletname}_{txid}' class NotificationsTest(DashTestFramework): @@ -169,7 +170,7 @@ def expect_wallet_notify(self, tx_details): # Universal newline ensures '\n' on 'nt' assert_equal(text[-1], '\n') text = text[:-1] - if os.name == 'nt': + if platform.system() == 'Windows': # On Windows, echo as above will append a whitespace assert_equal(text[-1], ' ') text = text[:-1] diff --git a/test/functional/feature_remove_pruned_files_on_startup.py b/test/functional/feature_remove_pruned_files_on_startup.py new file mode 100755 index 000000000000..e7cb6cd23174 --- /dev/null +++ b/test/functional/feature_remove_pruned_files_on_startup.py @@ -0,0 +1,55 @@ +#!/usr/bin/env python3 +# Copyright (c) 2022 The Bitcoin Core developers +# Distributed under the MIT software license, see the accompanying +# file COPYING or http://www.opensource.org/licenses/mit-license.php. +"""Test removing undeleted pruned blk files on startup.""" + +import platform +import os +from test_framework.test_framework import BitcoinTestFramework + +class FeatureRemovePrunedFilesOnStartupTest(BitcoinTestFramework): + def set_test_params(self): + self.num_nodes = 1 + self.extra_args = [["-fastprune", "-prune=1"]] + + def mine_batches(self, blocks): + n = blocks // 250 + for _ in range(n): + self.generate(self.nodes[0], 250) + self.generate(self.nodes[0], blocks % 250) + self.sync_blocks() + + def run_test(self): + blk0 = os.path.join(self.nodes[0].datadir, self.nodes[0].chain, 'blocks', 'blk00000.dat') + rev0 = os.path.join(self.nodes[0].datadir, self.nodes[0].chain, 'blocks', 'rev00000.dat') + blk1 = os.path.join(self.nodes[0].datadir, self.nodes[0].chain, 'blocks', 'blk00001.dat') + rev1 = os.path.join(self.nodes[0].datadir, self.nodes[0].chain, 'blocks', 'rev00001.dat') + self.mine_batches(800) + fo1 = os.open(blk0, os.O_RDONLY) + fo2 = os.open(rev1, os.O_RDONLY) + fd1 = os.fdopen(fo1) + fd2 = os.fdopen(fo2) + self.nodes[0].pruneblockchain(600) + + # Windows systems will not remove files with an open fd + if platform.system() != 'Windows': + assert not os.path.exists(blk0) + assert not os.path.exists(rev0) + assert not os.path.exists(blk1) + assert not os.path.exists(rev1) + else: + assert os.path.exists(blk0) + assert not os.path.exists(rev0) + assert not os.path.exists(blk1) + assert os.path.exists(rev1) + + # Check that the files are removed on restart once the fds are closed + fd1.close() + fd2.close() + self.restart_node(0) + assert not os.path.exists(blk0) + assert not os.path.exists(rev1) + +if __name__ == '__main__': + FeatureRemovePrunedFilesOnStartupTest().main() diff --git a/test/functional/rpc_bind.py b/test/functional/rpc_bind.py index 91c624222dc7..9a8990161568 100755 --- a/test/functional/rpc_bind.py +++ b/test/functional/rpc_bind.py @@ -4,8 +4,6 @@ # file COPYING or http://www.opensource.org/licenses/mit-license.php. """Test running dashd with the -rpcbind and -rpcallowip options.""" -import sys - from test_framework.netutil import all_interfaces, addr_to_hex, get_bind_addrs, test_ipv6_local from test_framework.test_framework import BitcoinTestFramework, SkipTest from test_framework.util import assert_equal, assert_raises_rpc_error, get_rpc_proxy, rpc_port, rpc_url @@ -17,6 +15,10 @@ def set_test_params(self): self.num_nodes = 1 self.supports_cli = False + def skip_test_if_missing_module(self): + # due to OS-specific network stats queries, this test works only on Linux + self.skip_if_platform_not_linux() + def setup_network(self): self.add_nodes(self.num_nodes, None) @@ -61,14 +63,9 @@ def run_allowip_test(self, allow_ips, rpchost, rpcport): self.stop_nodes() def run_test(self): - # due to OS-specific network stats queries, this test works only on Linux if sum([self.options.run_ipv4, self.options.run_ipv6, self.options.run_nonloopback]) > 1: raise AssertionError("Only one of --ipv4, --ipv6 and --nonloopback can be set") - self.log.info("Check for linux") - if not sys.platform.startswith('linux'): - raise SkipTest("This test can only be run on linux.") - self.log.info("Check for ipv6") have_ipv6 = test_ipv6_local() if not have_ipv6 and not (self.options.run_ipv4 or self.options.run_nonloopback): diff --git a/test/functional/test_framework/p2p.py b/test/functional/test_framework/p2p.py index a890b59c7186..2cb93b37c780 100755 --- a/test/functional/test_framework/p2p.py +++ b/test/functional/test_framework/p2p.py @@ -24,6 +24,7 @@ from collections import defaultdict from io import BytesIO import logging +import platform import struct import sys import threading @@ -786,7 +787,7 @@ def __init__(self): NetworkThread.listeners = {} NetworkThread.protos = {} - if sys.platform == 'win32': + if platform.system() == 'Windows': asyncio.set_event_loop_policy(asyncio.WindowsSelectorEventLoopPolicy()) NetworkThread.network_event_loop = asyncio.new_event_loop() diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py index cbe42cd1de88..e5eb2b6a3c44 100755 --- a/test/functional/test_framework/test_node.py +++ b/test/functional/test_framework/test_node.py @@ -12,13 +12,13 @@ import json import logging import os.path +import platform import re import subprocess import tempfile import time import urllib.parse import shlex -import sys import collections from pathlib import Path @@ -222,7 +222,7 @@ def __getattr__(self, name): assert self.rpc_connected and self.rpc is not None, self._node_msg("Error: no RPC connection") return getattr(RPCOverloadWrapper(self.rpc, descriptors=self.descriptors), name) - def start(self, extra_args=None, *, cwd=None, stdout=None, stderr=None, **kwargs): + def start(self, extra_args=None, *, cwd=None, stdout=None, stderr=None, env=None, **kwargs): """Start the node.""" if extra_args is None: extra_args = self.extra_args @@ -251,6 +251,8 @@ def start(self, extra_args=None, *, cwd=None, stdout=None, stderr=None, **kwargs # add environment variable LIBC_FATAL_STDERR_=1 so that libc errors are written to stderr and not the terminal subp_env = dict(os.environ, LIBC_FATAL_STDERR_="1") + if env is not None: + subp_env.update(env) self.process = subprocess.Popen(all_args, env=subp_env, stdout=stdout, stderr=stderr, cwd=cwd, **kwargs) @@ -573,7 +575,7 @@ def test_success(cmd): cmd, shell=True, stderr=subprocess.DEVNULL, stdout=subprocess.DEVNULL) == 0 - if not sys.platform.startswith('linux'): + if platform.system() != 'Linux': self.log.warning("Can't profile with perf; only available on Linux platforms") return None diff --git a/test/functional/test_framework/util.py b/test/functional/test_framework/util.py index 7cdcbc8983d8..255d47b1f370 100644 --- a/test/functional/test_framework/util.py +++ b/test/functional/test_framework/util.py @@ -13,15 +13,17 @@ import json import logging import os +import pathlib import random import shutil +import platform import re import time import urllib.parse from . import coverage from .authproxy import AuthServiceProxy, JSONRPCException -from typing import Callable, Optional +from typing import Callable, Optional, Tuple logger = logging.getLogger("TestFramework.utils") @@ -432,6 +434,22 @@ def get_datadir_path(dirname, n): return os.path.join(dirname, "node" + str(n)) +def get_temp_default_datadir(temp_dir: pathlib.Path) -> Tuple[dict, pathlib.Path]: + """Return os-specific environment variables that can be set to make the + GetDefaultDataDir() function return a datadir path under the provided + temp_dir, as well as the complete path it would return.""" + if platform.system() == "Windows": + env = dict(APPDATA=str(temp_dir)) + datadir = temp_dir / "Bitcoin" + else: + env = dict(HOME=str(temp_dir)) + if platform.system() == "Darwin": + datadir = temp_dir / "Library/Application Support/Bitcoin" + else: + datadir = temp_dir / ".bitcoin" + return env, datadir + + def append_config(datadir, options): with open(os.path.join(datadir, "dash.conf"), 'a', encoding='utf8') as f: for option in options: diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index f97e9261a223..74fed74d905d 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -17,6 +17,7 @@ import configparser import datetime import os +import platform import time import shutil import signal @@ -42,7 +43,7 @@ CROSS = "x " CIRCLE = "o " -if os.name == 'nt': #type:ignore +if platform.system() == 'Windows': #type:ignore import ctypes kernel32 = ctypes.windll.kernel32 # type: ignore ENABLE_VIRTUAL_TERMINAL_PROCESSING = 4 @@ -403,6 +404,7 @@ 'p2p_permissions.py', 'feature_blocksdir.py', 'wallet_startup.py', + 'feature_remove_pruned_files_on_startup.py', 'p2p_i2p_ports.py', 'p2p_i2p_sessions.py', 'feature_config_args.py', diff --git a/test/functional/wallet_multiwallet.py b/test/functional/wallet_multiwallet.py index b610347496e2..423cd9efb22c 100755 --- a/test/functional/wallet_multiwallet.py +++ b/test/functional/wallet_multiwallet.py @@ -9,9 +9,9 @@ from threading import Thread from decimal import Decimal import os +import platform import shutil import stat -import sys import time from test_framework.authproxy import JSONRPCException @@ -143,7 +143,7 @@ def wallet_file(name): # should raise rpc error if wallet path can't be created err_code = -4 if self.options.descriptors else -1 - assert_raises_rpc_error(err_code, "filesystem error:" if sys.platform != 'win32' else "create_directories:", self.nodes[0].createwallet, "w8/bad") + assert_raises_rpc_error(err_code, "filesystem error:" if platform.system() != 'Windows' else "create_directories:", self.nodes[0].createwallet, "w8/bad") # check that all requested wallets were created self.stop_node(0) diff --git a/test/lint/run-lint-format-strings.py b/test/lint/run-lint-format-strings.py index 07b0c86979d6..06daaa3d709a 100755 --- a/test/lint/run-lint-format-strings.py +++ b/test/lint/run-lint-format-strings.py @@ -244,20 +244,32 @@ def count_format_specifiers(format_string): 3 >>> count_format_specifiers("foo %d bar %i foo %% foo %*d foo") 4 + >>> count_format_specifiers("foo %5$d") + 5 + >>> count_format_specifiers("foo %5$*7$d") + 7 """ assert type(format_string) is str format_string = format_string.replace('%%', 'X') - n = 0 - in_specifier = False - for i, char in enumerate(format_string): - if char == "%": - in_specifier = True + n = max_pos = 0 + for m in re.finditer("%(.*?)[aAcdeEfFgGinopsuxX]", format_string, re.DOTALL): + # Increase the max position if the argument has a position number like + # "5$", otherwise increment the argument count. + pos_num, = re.match(r"(?:(^\d+)\$)?", m.group(1)).groups() + if pos_num is not None: + max_pos = max(max_pos, int(pos_num)) + else: n += 1 - elif char in "aAcdeEfFgGinopsuxX": - in_specifier = False - elif in_specifier and char == "*": + + # Increase the max position if there is a "*" width argument with a + # position like "*7$", and increment the argument count if there is a + # "*" width argument with no position. + star, star_pos_num = re.match(r"(?:.*?(\*(?:(\d+)\$)?)|)", m.group(1)).groups() + if star_pos_num is not None: + max_pos = max(max_pos, int(star_pos_num)) + elif star is not None: n += 1 - return n + return max(n, max_pos) def main():