refactor: Reduce number of LoadChainstate parameters and return values #25308

ryanofsky · 2022-06-08T13:31:03Z

Replace long LoadChainstate parameters list with options struct. Replace long list of return values with simpler error strings.

No changes in behavior. Motivation is just to make libbitcoin_kernel API easier to use and more future-proof, and make internal code clearer and more maintainable.

DrahtBot · 2022-06-08T15:00:36Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25499 (Use steady clock for all millis bench logging by MarcoFalke)

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.

src/kernel/checks.cpp

Suggested bitcoin#25308 (comment)

ryanofsky

Updated 5ceae5c -> 1066e9c (pr/retcode.2 -> pr/retcode.3, compare) making a few more cleanups and enabling IWYU
Updated 1066e9c -> 3981f5e (pr/retcode.3 -> pr/retcode.4, compare) fixing unintended change in catch statement

src/kernel/checks.cpp

Suggested bitcoin#25308 (comment)

jamesob · 2022-06-08T18:12:01Z

Concept ACK! Code looks shorter and more readable.

dongcarl · 2022-06-16T17:53:28Z

I really want to avoid reintroducing stringy error returns and coupling with util/translation if at all possible, why do you prefer error strings?

Suggested bitcoin#25308 (comment)

ryanofsky · 2022-06-16T18:39:04Z

I really want to avoid reintroducing stringy error returns and coupling with util/t 8000 ranslation if at all possible, why do you prefer error strings?

From most important to least important reasons:

Better user experience. Users can see complete information about an error instead of an error code with no context (flashback to https://en.wikipedia.org/wiki/Blue_screen_of_death#/media/File:Windows_9X_BSOD.png)
Better developer experience. Developers can call an API and have maybe 3-4 cases they need to handle, not 11 cases they need to decide what to do about.
Better separation of concerns and encapsulation. The too-narrow error codes that were here exposed implementation details of low level code to high level code
Maintainability. Getting rid of these error codes saves many lines of code. It makes code clearer, because putting error messages at error sites puts the error descriptions at the error site, not in some separate place. Putting error strings at error sites avoids possibilities of error codes getting mismapped or falling through unhandled (things we have no test coverage for, would be unlikely to be detected in manual testing, and could become broken at any time)
API compatibility. Related to the previous point about encapsulation, it's nice to use an API that doesn't break compatibility every time an internal implementation detail is changed, and some new error case is added, or an old one is removed

Error codes can be great. Error strings can be great. Right tool for the job.

Rebased 3981f5e -> f3ec82f (pr/retcode.4 -> pr/retcode.5, compare) due to conflict with #25306

dongcarl

Concept ACK. I've thought about this a bit, and I think I'm convinced that this current way can work.

The key point that convinced me was: what users of LoadChainstate care about discerning between is not what particular internal failure happened, but rather what to do about the failure, which is nicely encapsulated in InitStatus.

Originally, my thought was that requiring callers to match against strings to determine what to do is footgun-y. However, if a case like that ever arises, it simply means that InitStatus should be modified to indicate the right course of action.

In SanityChecks's case, any sanity check failure indicates to the caller that something is catastrophically wrong and the only course of action is to bail, so there's no need for an additional Status enum.

Thanks for fixing the IWYU for src/kernel btw, I've been meaning to do that.

dongcarl · 2022-06-27T21:34:04Z

src/kernel/checks.cpp


 namespace kernel {

-std::optional<SanityCheckError> SanityChecks(const Context&)
+std::optional<bilingual_str> SanityChecks(const Context&)


Could we just use std::string here? I don't want to introduce further coupling with translation.

re: #25308 (comment)

In commit "refactor: Reduce number of SanityChecks return values" (0b9bb46)

Could we just use std::string here? I don't want to introduce further coupling with translation.

We could actually use std::string instead of bilingual_str in this commit (not in the earlier commit), but I think it is better to use bilingual_str for reasons described in the other reply that commit.

dongcarl · 2022-06-27T21:34:18Z

src/node/chainstate.h

+enum class InitStatus { SUCCESS, FAILURE, FAILURE_INCOMPATIBLE_DB, INTERRUPTED };
+
+//! Status code and optional error string.
+using InitResult = std::tuple<InitStatus, bilingual_str>;


Could we just use std::string here? I don't want to introduce further coupling with translation.

re: #25308 (comment)

In commit "refactor: Reduce number of LoadChainstate return values" (ed3c27a)

Could we just use std::string here?

It's not possible to do this without changing behavior. Error messages like "Error opening block database" and "Incorrect or no genesis block found. Wrong datadir for network?" are translated and can't be returned through std::string.

I don't want to introduce further coupling with translation.

I think there are only upsides not downsides to using bilingual_str for messages that may be shown to the users.
Using bilingual_str instead of std::string only adds a dependency on the bilingual_str struct struct which is a simple pair of std::string's. This is a very minimal dependency, and it doesn't require the caller to use translations or the kernel library to provide them. It just allows translations to be passed along if they are present.

In general, I think it is a good practice to use bilingual_str for user messages, whether or not they will be translated. Using the bilingual_str type distinguishes user messages from other types of strings, and helps future-proof code so API's don't have to change if translations are added or removed. The choice of whether or not to translate an string can be context dependent, and may depending on how frequently a specific message will be shown or how complicated it is to translate. Just using bilingual_str everywhere means APIs can pass translations along and not be tied to these implementation details.

src/bitcoin-chainstate.cpp

src/node/chainstate.h

fanquake · 2022-06-28T09:21:08Z

src/kernel/coinstats.cpp


+#include <assert.h>


We want to use/are migrating to the C++ headers. I know IWYU suggests <assert.h> (have been meaning to make it suggest the newer headers), but it will be equally as happy with <cassert>. Thanks for fixing these up in any case.

re: #25308 (comment)

In commit "ci: Enable IWYU in src/kernel directory" (f3ec82f)

We want to use/are migrating to the C++ headers. I know IWYU suggests <assert.h> (have been meaning to make it suggest the newer headers), but it will be equally as happy with <cassert>. Thanks for fixing these up in any case.

Thanks, switched to cassert

maflcko · 2022-07-19T16:51:01Z

ACK fb31664 🚤

Show signature

Signature:

Suggested bitcoin#25308 (comment)

ryanofsky · 2022-07-19T22:01:28Z

Rebased fb31664 -> 1e761a0 (pr/retcode.10 -> pr/retcode.11, compare) due to conflicts with #25494 and #25645

maflcko · 2022-07-20T05:48:08Z

ACK 1e761a0 🕚

Show signature

Signature:

…ters an…

This is a draft commit trying to make some improvements to the `BResult` class to make it more usable. Adds a `util::Result` class with the following improvements (and implements `BResult` in terms of it so existing code doesn't have to change): - Supports returning error strings and error information at the same time. This functionality is needed by the `LoadChainstate` function in bitcoin#25308 or any function that can fail in multiple ways which need to be handled differently. And it means Result class is compatible with rust-style error handling and pattern matching, unlike BResult. - Makes Result class provide same operators and methods as std::optional, so it doesn't require calling less familiar `HasRes`/`GetObj`/`ReleaseObj` methods, and so error reporting functionality can be easily added or dropped from existing code by switching between util::Result and std::optional. - Supports `Result<void>` return values, so it is possible to return error reporting information from functions in a uniform way even if they don't have other return values. - Supports `Result<bilingual_str>` return values. Drops ambiguous and potentially misleading `BResult` constructor that treats any bilingual string as an error, and any other type as a non-error. Adds `util::Error` constructor so errors have to be explicitly constructed and not any `bilingual_str` will be turned into one. - Puts `src/util/` code in the `util::` namespace so naming reflects code organization and it is obvious where the class is coming from. Drops "B" from name because it is undocumented what it stands for (bilingual?) - Adds unit tests.

Suggested bitcoin#25308 (comment)

* rpc, refactor: Add `getblock_prevout` This change eliminates memory usage spike when compiling with Visual Studio 2022 (at least in Cirrus CI environment). Easy to review using `git diff --color-moved-ws=allow-indentation-change --color-moved=dimmed-zebra` * rpc, refactor: Add `decodepsbt_inputs` This change eliminates memory usage spike when compiling with Visual Studio 2022 (at least in Cirrus CI environment). Easy to review using `git diff --color-moved-ws=allow-indentation-change --color-moved=dimmed-zebra` * rpc, refactor: Add `decodepsbt_outputs` This change eliminates memory usage spike when compiling with Visual Studio 2022 (at least in Cirrus CI environment). Easy to review using `git diff --color-moved-ws=allow-indentation-change --color-moved=dimmed-zebra` * build: Increase MS Visual Studio minimum version Visual Studio 2022 with `/std:c++20` supports designated initializers. * refactor: Drop no longer needed `util/designator.h` * Remove my key from trusted-keys * refactor: add most of src/util to iwyu These files change infrequently, and not much header shuffling is required. We don't add everything in src/util/ yet, because IWYU makes some dubious suggestions, which I'm going to follow up with upstream. * Introduce generic 'Result' class Useful to encapsulate the function result object (in case of having it) or, in case of failure, the failure reason. This let us clean lot of boilerplate code, as now instead of returning a boolean and having to add a ref arg for the return object and another ref for the error string. We can simply return a 'BResult<Obj>'. Example of what we currently have: ``` bool doSomething(arg1, arg2, arg3, arg4, &result, &error_string) { do something... if (error) { error_string = "something bad happened"; return false; } result = goodResult; return true; } ``` Example of what we will get with this commit: ``` BResult<Obj> doSomething(arg1, arg2, arg3, arg4) { do something... if (error) return {"something happened"}; // good return {goodResult}; } ``` This allows a similar boilerplate cleanup on the function callers side as well. They don't have to add the extra pre-function-call error string and result object declarations to pass the references to the function. * wallet: refactor, include 'FeeCalculation' inside 'CreatedTransactionResult' * send: refactor CreateTransaction flow to return a BResult<CTransactionRef> * wallet: refactor GetNewDestination, use BResult * test: refactor: pass absolute fee in `create_lots_of_big_transactions` helper * doc: update the URLs to thread functions in developer-notes ThreadMapPort() does not appear on doxygen.bitcoincore.org because it is inside `#ifdef`. * test: speedup wallet_coinbase_category.py No need to create a chain for it (nor use the cache). * wallet: Precompute Txdata after setting PSBT inputs' UTXOs If we are given a PSBT that is missing one or more input UTXOs, our PrecomputedTransactionData will be incorrect and missing information that it should otherwise have, and therefore we may not produce a signature when we should. To avoid this problem, we can do the precomputation after we have set the UTXOs the wallet is able to set for the PSBT. Also adds a test for this behavior. * Address comments remaining from bitcoin#25353 * move-only: Version handshake to libtest_util * move-only: InitializeNode to handshake helper * [test] persist prioritisation of transactions not in mempool * wallet: change `ScanForWalletTransactions` to use `Ticks()` * scripted-diff: [test] Rename BIP125_SEQUENCE_NUMBER to MAX_BIP125_RBF_SEQUENCE -BEGIN VERIFY SCRIPT- sed -i 's:BIP125_SEQUENCE_NUMBER:MAX_BIP125_RBF_SEQUENCE:g' $(git grep -l BIP125_SEQUENCE_NUMBER ./test) -END VERIFY SCRIPT- * test: Remove duplicate MAX_BIP125_RBF_SEQUENCE constant * Prepare BResult for non-copyable types * refactor: Return BResult from restoreWallet * Remove atomic for m_last_getheaders_timestamp This variable is only used in a single thread, so no atomic or mutex is necessary to guard it. * test: remove unnecessary parens * test/mempool_persist: Test manual savemempool when -persistmempool=0 * depends: update urls for dmg tools These repos have migrated from https://github.com/al45tair/ to https://github.com/dmgbuild/, so update our URLs to point to the new location. Note that GitHub is also already performing the redirect automatically. * Expose underlying clock in CThreadInterrupt Overloading sleep_for is not needed, as * seconds and minutes can be converted to milliseconds by the compiler, not needing a duration_cast * std::condition_variable::wait_for will convert milliseconds to the duration type of the underlying clock So simply expose the clock. * refactor: Make FEELER_SLEEP_WINDOW type safe (std::chrono) * refactor: Default options in walletcreatefundedpsbt to VOBJ instead of VNULL This should not change behavior and makes the code consistent with other places. * univalue: Throw exception on invalid pushes over silent ignore * rpc: Select int-UniValue constructor for enum value in upgradewallet RPC UniValue does not have a constructor for enum values, however the compiler will decay the enum into an int and select that constructor. Avoid this compiler magic and clarify the code by explicitly selecting the int-constructor. This is needed for the next commit. * miniscript: don't check for top level validity at parsing time Letting the caller perform the checks allows for finer-grained error reporting. * miniscript: add a helper to find the first insane sub with no child This is helpful for finer grained descriptor parsing error: when there are multiple errors to report in a Miniscript descriptor start with the "smallest" fragments: the ones closer to be a leaf. Co-Authored-By: Pieter Wuille <pieter@wuille.net> * qa: better error reporting on descriptor parsing error A nit, but was helpful when writing unit tests for Miniscript parsing * Miniscript support in output descriptors Miniscript descriptors are defined under P2WSH context (either `wsh()` or `sh(wsh())`). Only sane Miniscripts are accepted, as insane ones (although valid by type) can have surprising behaviour with regard to malleability guarantees and resources limitations. As Miniscript descriptors are longer and more complex than "legacy" descriptors, care was taken in error reporting to help a user determine for what reason a provided Miniscript is insane. Co-authored-by: Pieter Wuille <pieter.wuille@gmail.com> * qa: functional test Miniscript watchonly support * univalue: Avoid narrowing and verbose int constructors As UniValue provides several constructors for integral types, the compiler is unable to select one if the passed type does not exactly match. This is unintuitive for developers and forces them to write verbose and brittle code. For example, there are many places where an unsigned int is cast to a signed int. While the cast is safe in practice, it is still needlessly verbose and confusing as the value can never be negative. In fact it might even be unsafe if the unsigned value is large enough to map to a negative signed one. * Move ChainstateManagerOpts into kernel:: namespace It should have been there in the first place. * Use designated initializers for ChainstateManager::Options This wasn't available at the time when ChainstateManager::Options was introduced but is helpful to be explicit and ensure correctness. * [net processing] Add m_our_services and m_their_services to Peer Track services offered by us and the peer in the Peer object. * [tests] Connect peer in outbound_slow_chain_eviction by sending p2p messages Prior to this commit, the peer was connected, and then the services and connectivity fields in the CNode object were manually set. Instead, send p2p `version` and `verack` messages, and have net_processing's internal logic set the state of the node. This ensures that the node's internal state is consistent with how it would be set in the live code. Prior to this commit, `dummyNode1.nServices` was set to `NODE_NONE` which was not a problem since `CNode::fClient` and `CNode::m_limited_node` are default initialised to false. Now that we are doing the actual version handshake, the values of `fClient` and `m_limited_node` are set during the handshake and cause the test to fail if we do not set `dummyNode1.nServices` to a reasonable value (NODE_NETWORK | NODE_WITNESS). * [net processing] Remove fClient and m_limited_node fClient is replaced by CanServeBlocks(), and m_limited_node is replaced by IsLimitedPeer(). * [net processing] Replace fHaveWitness with CanServeWitnesses() * [net processing] Remove CNode::nServices Use Peer::m_their_services instead * [net] Return CService from GetLocalAddrForPeer and GetLocalAddress * [net processing] Remove CNode::nLocalServices * fix gettxout help text * Release notes for Miniscript support in P2WSH descriptors * DumpMempool: Use std::chrono instead of weird int64_t arthmetics This makes it so that DumpMempool doesn't depend on MICRO anymore * DumpMempool: Pass in dump_path, stop using gArgs Also introduce node::{ShouldPersistMempool,MempoolPath} helper functions in node/mempool_persist_args.{h,cpp} which are used by non-kernel DumpMempool callers to determine whether or not to automatically dump the mempool and where to dump it to. * scripted-diff: Rename m_is_loaded -> m_load_tried m_is_loaded/IsLoaded() doesn't actually indicate whether or not the mempool was successfully, loaded, but rather if a load has been attempted and did not result in a catastrophic ShutdownRequested. -BEGIN VERIFY SCRIPT- find_regex="\bm_is_loaded\b" \ && git grep -l -E "$find_regex" \ | xargs sed -i -E "s@$find_regex@m_load_tried@g" find_regex="\bIsLoaded\b" \ && git grep -l -E "$find_regex" \ | xargs sed -i -E "s@$find_regex@GetLoadTried@g" find_regex="\bSetIsLoaded\b" \ && git grep -l -E "$find_regex" \ | xargs sed -i -E "s@$find_regex@SetLoadTried@g" -END VERIFY SCRIPT- * mempool: Improve comments for [GS]etLoadTried Also change the param name for SetLoadTried to load_tried. * mempool: Use NodeClock+friends for LoadMempool * Disallow encryption of watchonly wallets Watchonly wallets do not have any private keys to encrypt. It does not make sense to encrypt such wallets, so disable the option to encrypt them. This avoids an assertion that can be hit when encrypting watchonly descriptor wallets. * Move FopenFn to fsbridge namespace [META] In a future commit in this patchset, it will be used by more than just validation, and it needs to align with fopen anyway. * test/fuzz: Invoke LoadMempool via CChainState Not only does this increase coverage, it is also more correct in that when ::LoadMempool is called with a mempool and chainstate, it calls AcceptToMemoryPool with just the chainstate. AcceptToMemoryPool will then act on the chainstate's mempool via CChainState::GetMempool, which may be different from the mempool originally passed to ::LoadMempool. (In this fuzz test's case, it definitely is different) Also, move DummyChainstate to its own file since it's now used by the validation_load_mempool fuzz test to replace CChainState's m_mempool. * LoadMempool: Pass in load_path, stop using gArgs Also: 1. Have CChainState::LoadMempool and ::ThreadImport take in paths and pass it through untouched to LoadMempool. 2. Make LoadMempool exit early if the load_path is empty. 3. Adjust the call to ::ThreadImport in ::AppInitMain to correctly pass in an empty path if mempool persistence is disabled. * Move DEFAULT_PERSIST_MEMPOOL out of libbitcoinkernel It is no longer used by anything inside libbitcoinkernel, move it to node/mempool_persist_args.h where it belongs. * Move {Load,Dump}Mempool to kernel namespace Also: 1. Add the newly introduced kernel/mempool_persist.cpp to IWYU CI script 2. Add chrono mapping for iwyu * build: pass -fno-lto when building expat Otherwise it's autoconf endianess check will fail to determine what the endianess is.. * build: Use Link Time Optimization for Qt code on Linux See: https://www.qt.io/blog/2019/01/02/qt-applications-lto * fuzz: Fix assert bug in txorphan target * doc: remove references to downstream Having references to downstream no-longer make sense now that we've unsubtree'd. * refactor: integrate no_nul into univalue unitester * refactor: remove BOOST_*_TEST_ macros * move-only: Move UniValue::getInt definition to keep class with definitions only Can be reviewed with the git options --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space * univalue: Return more detailed type check error messages * Add symlinks for hardcoded Makefiles in out of tree builds * build: Check for std::atomic::exchange rather than std::atomic_exchange Our usage of std::atomic is with it's own exchange function, not std::atomic_exchange. So we should be looking specifically for that function. Additionally, -pthread and -lpthread have an effect on whether -latomic will be needed, so the atomics check needs to use these flags as well. This will make the flags in use better match what is actually used when linking. This removes the need for -latomic for riscv builds, which resolves a guix cross architecture reproducibility issue. * build: Fix autoconf variable names for tools found by `AC_PATH_TOOL` See the `AC_PATH_TOOL` macro implementation. * depends: default to using GCC tool wrappers (with GCC) This improves support for LTO by using gcc wrappers for ar, nm, ranlib, that correctly setup plugin arguments for LTO. Other HOSTS are using clang. * validation: remove unused using directives The following were unused from the node namespace: - BLOCKFILE_CHUNK_SIZE - nPruneTarget - OpenBlockFile - UNDOFILE_CHUNK_SIZE * refactor: remove unused using directives * tidy: use misc-unused-using-decls https://clang.llvm.org/extra/clang-tidy/checks/misc/unused-using-decls.html * refactor: Make mapBlocksUnknownParent local, and rename it Co-authored-by: Larry Ruane <larryruane@gmail.com> * interfaces, refactor: Add more block information to block connected notifications Add new interfaces::BlockInfo struct to be able to pass extra block information (file and undo information) to indexes which they are updated to use high level interfaces::Chain notifications. This commit does not change behavior in any way. * indexes, refactor: Pass Chain interface instead of CChainState class to indexes Passing abstract Chain interface will let indexes run in separate processes. This commit does not change behavior in any way. * indexes, refactor: Remove CBlockIndex* uses in coinstatsindex LookUpOne function This commit does not change behavior in any way. * indexes, refactor: Remove CBlockIndex* uses in index Init methods Replace overriden index Init() methods that use the best block CBlockIndex* pointer with pure CustomInit() callbacks that are passed the block hash and height. This gets rid of more CBlockIndex* pointer uses so indexes can work outside the bitcoin-node process. It also simplifies the initialization call sequence so index implementations are not responsible for initializing the base class. There is a slight change in behavior here since now the best block pointer is loaded and checked before the custom index init functions are called instead of while they are called. * indexes, refactor: Remove CBlockIndex* uses in index WriteBlock methods Replace WriteBlock method with CustomAppend and pass BlockInfo struct instead of CBlockIndex* pointer This commit does not change behavior in any way. * indexes, refactor: Remove CBlockIndex* uses in index Rewind methods Replace Rewind method with CustomRewind and pass block hashes and heights instead of CBlockIndex* pointers This commit does not change behavior in any way. * indexes, refactor: Remove CChainState use in index CommitInternal method Replace CommitInternal method with CustomCommit and use interfaces::Chain instead of CChainState to generate block locator. This commit does not change behavior in any way, except in the (m_best_block_index == nullptr) case, which was added recently in bitcoin#24117 as part of an ongoing attempt to prevent index corruption if bitcoind is interrupted during startup. New behavior in that case should be slightly better than the old behavior (skipping the entire custom+base commit now vs only skipping the base commit previously) and this might avoid more cases of corruption. * refactor: Use chainman() helper consistently in ChainImpl * guix: Drop repetition of option's default value * Fix `-Wparentheses` gcc warning * depends: modify FastFixedDtoa optimisation flags This fixes a non-determinism issue in the asm produced for this function when cross-compiling on x86_64 and aarch64 for the arm-linux-gnueabihf HOST. Related to bitcoin#21194. * Add missing includes to node/chainstate This is needed for the next commit * Add missing includes They are needed, otherwise the next commit will not compile * Remove unused includes from dbwrapper.h * Remove unused includes in rpc/fees.cpp IWYU confirms that they are unused * refactor: store by OutputType in CoinsResult Store COutputs by OutputType in CoinsResult. The struct stores vectors of `COutput`s by `OutputType` for more convenient access * refactor: use CoinsResult struct in SelectCoins Pass the whole CoinsResult struct to SelectCoins instead of only a vector. This means we now have to remove preselected coins from each OutputType vector and shuffle each vector individually. Pass the whole CoinsResult struct to AttemptSelection. This involves moving the logic in AttemptSelection to a newly named function, ChooseSelectionResult. This will allow us to run ChooseSelectionResult over each OutputType in a later commit. This ensures the backoffs work properly. Update unit and bench tests to use CoinResult. * scripted-diff: rename `FromBinary` helper to `from_binary` (signet miner) -BEGIN VERIFY SCRIPT- sed -i s/FromBinary/from_binary/g ./contrib/signet/miner -END VERIFY SCRIPT- * refactor: move `from_binary` helper from signet miner to test framework Can be easily reviewed with `--color-moved=dimmed-zebra`. * refactor: move PSBT(Map) helpers from signet miner to test framework Can be easily reviewed with `--color-moved=dimmed-zebra`. * test: add constants for PSBT key types (BIP 174) Also take use of the constants in the signet miner to get rid of magic numbers and increase readability and maintainability. * refactor: move helper `random_bytes` to util library Can be easily reviewed with `--color-moved=dimmed-zebra`. * test: add test for decoding PSBT with per-input preimage types * wallet: run coin selection by `OutputType` Run coin selection on each OutputType separately, choosing the best solution according to the waste metric. This is to avoid mixing UTXOs that are of different OutputTypes, which can hurt privacy. If no single OutputType can fund the transaction, then coin selection considers the entire wallet, potentially mixing (current behavior). This is done inside AttemptSelection so that all OutputTypes are considered at each back-off in coin selection. * test: functional test for new coin selection logic Create a wallet with mixed OutputTypes and send a volley of payments, ensuring that there are no mixed OutputTypes in the txs. Finally, verify that OutputTypes are mixed only when necessary. * test: add unit test for AvailableCoins test that UTXOs are bucketed correctly after running AvailableCoins * refactor: Reduce number of LoadChainstate parameters * refactor: Reduce number of LoadChainstate return values 3D11 * refactor: Reduce number of SanityChecks return values * ci: Enable IWYU in src/kernel directory Suggested bitcoin#25308 (comment) * refactor: move compat.h into compat/ * compat: document FD_SETSIZE redefinition for WIN32 * compat: remove unused WSA* definitions * compat: extract and document MAX_PATH * compat: document S_I* defines when building for Windows * compat: document sockopt_arg_type definition * compat: document error-code mapping See: https://docs.microsoft.com/en-us/windows/win32/winsock/windows-sockets-error-codes-2 * compat: document redefining ssize_t when using MSVC See: https://docs.microsoft.com/en-us/windows/win32/winprog/windows-data-types#ssize_t https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_types.h.html * Add HashWriter without ser-type and ser-version The moved parts can be reviewed with "--color-moved=dimmed-zebra". * Use HashWriter where possible * ci: better pin to dwarf4 in valgrind job Use `-gdwarf` and also set CFLAGS. I was seeing Valgrind issues otherwise. * contrib: remove unneeded valgrind suppressions * doc: BaseIndex sync behavior with empty datadir Make a note about a potentially confusing behavior with `BaseIndex::m_synced`; if the user starts bitcoind with an empty datadir and an index enabled, BaseIndex will consider itself synced (as a degenerate case). This affects how indices are built during IBD (relying solely on BlockConnected signals vs. using ThreadSync()). * refactor: Fix iwyu on node/chainstate * CBlockIndex: ensure phashBlock is not nullptr before dereferencing and remove a now-redundant assert preceding a GetBlockHash() caller. This protects against UB here, and in case of failure (which would indicate a consensus bug), the debug log will print bitcoind: chain.h:265: uint256 CBlockIndex::GetBlockHash() const: Assertion `phashBlock != nullptr' failed. Aborted instead of Segmentation fault * CDiskBlockIndex: remove unused ToString() class member and mark its inherited CBlockIndex#ToString public interface member as deleted, to disallow calling it in the derived CDiskBlockIndex class. * CDiskBlockIndex: rename GetBlockHash() to ConstructBlockHash() and mark the inherited CBlockIndex#GetBlockHash public interface member as deleted, to disallow calling it in the derived CDiskBlockIndex class. Here is a failing test on master demonstrating the inconsistent behavior of the current design: calling the same inherited public interface functions on the same CDiskBlockIndex object should yield identical behavior. ```diff diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp index 6dc522b..dac3840 100644 --- a/src/test/validation_chainstatemanager_tests.cpp +++ b/src/test/validation_chainstatemanager_tests.cpp @@ -240,6 +240,15 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_activate_snapshot, TestChain100Setup) const CBlockIndex* tip = chainman.ActiveTip(); BOOST_CHECK_EQUAL(tip->nChainTx, au_data.nChainTx); + // CDiskBlockIndex "is a" CBlockIndex, as it publicly inherits from it. + // Test that calling the same inherited interface functions on the same + // object yields identical behavior. + CDiskBlockIndex index{tip}; + CBlockIndex *pB = &index; + CDiskBlockIndex *pD = &index; + BOOST_CHECK_EQUAL(pB->GetBlockHash(), pD->GetBlockHash()); + BOOST_CHECK_EQUAL(pB->ToString(), pD->ToString()); + ``` The GetBlockHash() test assertion only passes on master because the different methods invoked by the current design happen to return the same result. If one of the two is changed, it fails like the ToString() assertion does. Redefining inherited non-virtual functions is well-documented as incorrect design to avoid inconsistent behavior (see Scott Meyers, "Effective C++", Item 36). Class usage is confusing when the behavior depends on the pointer definition instead of the object definition (static binding happening where dynamic binding was expected). This can lead to unsuspected or hard-to-track bugs. Outside of critical hot spots, correctness usually comes before optimisation, but the current design dates back to main.cpp and it may possibly have been chosen to avoid the overhead of dynamic dispatch. This solution does the same: the class sizes are unchanged and no vptr or vtbl is added. There are better designs for doing this that use composition instead of inheritance or that separate the public interface from the private implementations. One example of the latter would be a non-virtual public interface that calls private virtual implementation methods, i.e. the Template pattern via the Non-Virtual Interface (NVI) idiom. * refactor: move CBlockIndex#ToString() from header to implementation which allows dropping tinyformat.h from the header file. * gui: Fix translator comment for Restore Wallet QInputDialog This also changes the window title name from `Restore Name` to `Restore Wallet`. * test: support passing PSBTMaps directly to PSBT ctor This will allow to create simple PSBTs as short one-liners, without the need to have three individual assignments (globals, inputs, outputs). * test: check that combining PSBTs with different txs fails * fuzz: Remove no-op SetMempoolConstraints * Bugfix: RPC/blockchain: Correct type of "value" in getblock docs; add missing "desc" * RPC: Document "asm" and "hex" fields for scripts * test: remove unused if statements * refactor: Make CTransaction constructor explicit It involves calculating two hashes, so the performance impact should be made explicit. Also, add the module to iwyu. * fuzz: refactor: Replace NullUniValue with UniValue{} This is needed for the scripted-diff to compile in the next commit * scripted-diff: Replace NullUniValue with UniValue::VNULL This is required for removing the UniValue copy constructor. -BEGIN VERIFY SCRIPT- sed -i 's/return NullUniValue/return UniValue::VNULL/g' $(git grep -l NullUniValue ':(exclude)src/univalue') -END VERIFY SCRIPT- * psbt: Fix unsigned integer overflow * fix comment spellings from the codespell lint test/lint/all-lint.py includes the codespell lint * depends: always use correct ar for win qt If we don't set this explicitly, then qt will still use it's default windows ar, when building with LTO (when we want it to use gcc-ar). So set `QMAKE_LIB` which is used for win32, and defaults to `ar -rc`. This way we always get the correct ar. Issue can be seen building in Guix with LTO. i.e: ```bash x86_64-w64-mingw32-ar: .obj/release/hb-blob.o: plugin needed to handle lto object ``` * refactor: Remove not needed std::max * scripted-diff: Rename addrman time symbols -BEGIN VERIFY SCRIPT- ren() { sed -i "s:\<$1\>:$2:g" $(git grep -l "\<$1\>" ./src ./test); } ren nLastTry m_last_try ren nLastSuccess m_last_success ren nLastGood m_last_good ren nLastCountAttempt m_last_count_attempt ren nSinceLastTry since_last_try ren nTimePenalty time_penalty ren nUpdateInterval update_interval ren fCurrentlyOnline currently_online -END VERIFY SCRIPT- * util: Add HoursDouble * Add ChronoFormatter to serialize * Add type-safe AdjustedTime() getter to timedata Also, fix includes. The getter will be used in a future commit. * refactor: Use type-safe std::chrono for addrman time * refactor: remove unnecessary string initializations * tidy: enable readability-redundant-string-init See: https://releases.llvm.org/14.0.0/tools/clang/tools/extra/docs/clang-tidy/checks/readability-redundant-string-init.html * depends: expat 2.4.8 * depends: re-enable using -flto when building expat * script: actually trigger the optimization in BuildScript The counter is an optimization over calling `ret.empty()`. It was suggested that the compiler would realize `cnt` is only `0` on the first iteration, and not actually emit the check and conditional. This optimization was actually not triggered at all, since we incremented `cnt` at the beginning of the first iteration. Fix it by incrementing at the end instead. This was reported by Github user "Janus". * test: Drop unused boost workaround * refactor: log `nEvicted` message in `LimitOrphans` then return void `LimitOrphans()` can log expired tx and it should log evicted tx as well instead of returning the number for caller to print the message. Since `LimitOrphans()` now return void, the redundant assertion check in fuzz test is also removed. * [unit tests] individual RBF Rules in isolation Test each component of the RBF policy in isolation. Unlike the RBF functional tests, these do not rely on things like RPC results, mempool submission, etc. * guix: enable SSP for RISC-V glibc (2.27) Pass `--enable-stack-protector=all` when building the glibc used for the RISC-V toolchain, to enable stack smashing protection on all functions, in the glibc code. * guix: pass enable-bind-now to glibc Both glibcs we build support `--enable-bind-now`: Disable lazy binding for installed shared objects and programs. This provides additional security hardening because it enables full RELRO and a read-only global offset table (GOT), at the cost of slightly increased program load times. See: https://www.gnu.org/software/libc/manual/html_node/Configuring-and-compiling.html * guix: enable hardening options in GCC Build Pass `--enable-default-pie` and `--enable-default-ssp` when configuring our GCCs. This achieves the following: --enable-default-pie Turn on -fPIE and -pie by default. --enable-default-ssp Turn on -fstack-protector-strong by default. Note that this isn't a replacement for passing hardneing flags ourselves, but introduces some redundency, and there isn't really a reason to not build a more "hardenings enabled" toolchain by default. See also: https://gcc.gnu.org/install/configure.html * libxcb: use a patch instead of sed To remove the unneeded pthread-stubs requirements. * tidy: run clang-tidy in quiet mode Co-authored-by: fanquake <fanquake@gmail.com> Co-authored-by: MacroFake <falke.marco@gmail.com> Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Co-authored-by: Pieter Wuille <pieter@wuille.net> Co-authored-by: Andrew Chow <achow101-github@achow101.com> Co-authored-by: furszy <matiasfurszyfer@protonmail.com> Co-authored-by: Sebastian Falbesoner <sebastian.falbesoner@gmail.com> Co-authored-by: Vasil Dimov <vd@FreeBSD.org> Co-authored-by: Antoine Riard <dev@ariard.me> Co-authored-by: glozow <gloriajzhao@gmail.com> Co-authored-by: w0xlt <94266259+w0xlt@users.noreply.github.com> Co-authored-by: Suhas Daftuar <sdaftuar@gmail.com> Co-authored-by: Carl Dong <contact@carldong.me> Co-authored-by: Antoine Poinsot <darosior@protonmail.com> Co-authored-by: Pieter Wuille <pieter.wuille@gmail.com> Co-authored-by: John Newbery <john@johnnewbery.com> Co-authored-by: dergoegge <n.goeggi@gmail.com> Co-authored-by: Marnix <93143998+MarnixCroes@users.noreply.github.com> Co-authored-by: chinggg <24590067+chinggg@users.noreply.github.com> Co-authored-by: Pablo Greco <psgreco@gmail.com> Co-authored-by: eugene <elzeigel@gmail.com> Co-authored-by: Larry Ruane <larryruane@gmail.com> Co-authored-by: Ryan Ofsky <ryan@ofsky.org> Co-authored-by: josibake <josibake@protonmail.com> Co-authored-by: Russell Yanofsky <russ@yanofsky.org> Co-authored-by: James O'Beirne <james.obeirne@pm.me> Co-authored-by: Jon Atack <jon@atack.com> Co-authored-by: Luke Dashjr <luke-jr+git@utopios.org> Co-authored-by: Aurèle Oulès <aurele@oules.com> Co-authored-by: Greg Weber <gregwebs@users.noreply.github.com>

* rpc, refactor: Add `getblock_prevout` This change eliminates memory usage spike when compiling with Visual Studio 2022 (at least in Cirrus CI environment). Easy to review using `git diff --color-moved-ws=allow-indentation-change --color-moved=dimmed-zebra` * rpc, refactor: Add `decodepsbt_inputs` This change eliminates memory usage spike when compiling with Visual Studio 2022 (at least in Cirrus CI environment). Easy to review using `git diff --color-moved-ws=allow-indentation-change --color-moved=dimmed-zebra` * rpc, refactor: Add `decodepsbt_outputs` This change eliminates memory usage spike when compiling with Visual Studio 2022 (at least in Cirrus CI environment). Easy to review using `git diff --color-moved-ws=allow-indentation-change --color-moved=dimmed-zebra` * build: Increase MS Visual Studio minimum version Visual Studio 2022 with `/std:c++20` supports designated initializers. * refactor: Drop no longer needed `util/designator.h` * Remove my key from trusted-keys * refactor: add most of src/util to iwyu These files change infrequently, and not much header shuffling is required. We don't add everything in src/util/ yet, because IWYU makes some dubious suggestions, which I'm going to follow up with upstream. * Introduce generic 'Result' class Useful to encapsulate the function result object (in case of having it) or, in case of failure, the failure reason. This let us clean lot of boilerplate code, as now instead of returning a boolean and having to add a ref arg for the return object and another ref for the error string. We can simply return a 'BResult<Obj>'. Example of what we currently have: ``` bool doSomething(arg1, arg2, arg3, arg4, &result, &error_string) { do something... if (error) { error_string = "something bad happened"; return false; } result = goodResult; return true; } ``` Example of what we will get with this commit: ``` BResult<Obj> doSomething(arg1, arg2, arg3, arg4) { do something... if (error) return {"something happened"}; // good return {goodResult}; } ``` This allows a similar boilerplate cleanup on the function callers side as well. They don't have to add the extra pre-function-call error string and result object declarations to pass the references to the function. * wallet: refactor, include 'FeeCalculation' inside 'CreatedTransactionResult' * send: refactor CreateTransaction flow to return a BResult<CTransactionRef> * wallet: refactor GetNewDestination, use BResult * test: refactor: pass absolute fee in `create_lots_of_big_transactions` helper * doc: update the URLs to thread functions in developer-notes ThreadMapPort() does not appear on doxygen.bitcoincore.org because it is inside `#ifdef`. * test: speedup wallet_coinbase_category.py No need to create a chain for it (nor use the cache). * wallet: Precompute Txdata after setting PSBT inputs' UTXOs If we are given a PSBT that is missing one or more input UTXOs, our PrecomputedTransactionData will be incorrect and missing information that it should otherwise have, and therefore we may not produce a signature when we should. To avoid this problem, we can do the precomputation after we have set the UTXOs the wallet is able to set for the PSBT. Also adds a test for this behavior. * Address comments remaining from bitcoin#25353 * move-only: Version handshake to libtest_util * move-only: InitializeNode to handshake helper * [test] persist prioritisation of transactions not in mempool * wallet: change `ScanForWalletTransactions` to use `Ticks()` * scripted-diff: [test] Rename BIP125_SEQUENCE_NUMBER to MAX_BIP125_RBF_SEQUENCE -BEGIN VERIFY SCRIPT- sed -i 's:BIP125_SEQUENCE_NUMBER:MAX_BIP125_RBF_SEQUENCE:g' $(git grep -l BIP125_SEQUENCE_NUMBER ./test) -END VERIFY SCRIPT- * test: Remove duplicate MAX_BIP125_RBF_SEQUENCE constant * Prepare BResult for non-copyable types * refactor: Return BResult from restoreWallet * Remove atomic for m_last_getheaders_timestamp This variable is only used in a single thread, so no atomic or mutex is necessary to guard it. * test: remove unnecessary parens * test/mempool_persist: Test manual savemempool when -persistmempool=0 * depends: update urls for dmg tools These repos have migrated from https://github.com/al45tair/ to https://github.com/dmgbuild/, so update our URLs to point to the new location. Note that GitHub is also already performing the redirect automatically. * Expose underlying clock in CThreadInterrupt Overloading sleep_for is not needed, as * seconds and minutes can be converted to milliseconds by the compiler, not needing a duration_cast * std::condition_variable::wait_for will convert milliseconds to the duration type of the underlying clock So simply expose the clock. * refactor: Make FEELER_SLEEP_WINDOW type safe (std::chrono) * refactor: Default options in walletcreatefundedpsbt to VOBJ instead of VNULL This should not change behavior and makes the code consistent with other places. * univalue: Throw exception on invalid pushes over silent ignore * rpc: Select int-UniValue constructor for enum value in upgradewallet RPC UniValue does not have a constructor for enum values, however the compiler will decay the enum into an int and select that constructor. Avoid this compiler magic and clarify the code by explicitly selecting the int-constructor. This is needed for the next commit. * miniscript: don't check for top level validity at parsing time Letting the caller perform the checks allows for finer-grained error reporting. * miniscript: add a helper to find the first insane sub with no child This is helpful for finer grained descriptor parsing error: when there are multiple errors to report in a Miniscript descriptor start with the "smallest" fragments: the ones closer to be a leaf. Co-Authored-By: Pieter Wuille <pieter@wuille.net> * qa: better error reporting on descriptor parsing error A nit, but was helpful when writing unit tests for Miniscript parsing * Miniscript support in output descriptors Miniscript descriptors are defined under P2WSH context (either `wsh()` or `sh(wsh())`). Only sane Miniscripts are accepted, as insane ones (although valid by type) can have surprising behaviour with regard to malleability guarantees and resources limitations. As Miniscript descriptors are longer and more complex than "legacy" descriptors, care was taken in error reporting to help a user determine for what reason a provided Miniscript is insane. Co-authored-by: Pieter Wuille <pieter.wuille@gmail.com> * qa: functional test Miniscript watchonly support * univalue: Avoid narrowing and verbose int constructors As UniValue provides several constructors for integral types, the compiler is unable to select one if the passed type does not exactly match. This is unintuitive for developers and forces them to write verbose and brittle code. For example, there are many places where an unsigned int is cast to a signed int. While the cast is safe in practice, it is still needlessly verbose and confusing as the value can never be negative. In fact it might even be unsafe if the unsigned value is large enough to map to a negative signed one. * Move ChainstateManagerOpts into kernel:: namespace It should have been there in the first place. * Use designated initializers for ChainstateManager::Options This wasn't available at the time when ChainstateManager::Options was introduced but is helpful to be explicit and ensure correctness. * [net processing] Add m_our_services and m_their_services to Peer Track services offered by us and the peer in the Peer object. * [tests] Connect peer in outbound_slow_chain_eviction by sending p2p messages Prior to this commit, the peer was connected, and then the services and connectivity fields in the CNode object were manually set. Instead, send p2p `version` and `verack` messages, and have net_processing's internal logic set the state of the node. This ensures that the node's internal state is consistent with how it would be set in the live code. Prior to this commit, `dummyNode1.nServices` was set to `NODE_NONE` which was not a problem since `CNode::fClient` and `CNode::m_limited_node` are default initialised to false. Now that we are doing the actual version handshake, the values of `fClient` and `m_limited_node` are set during the handshake and cause the test to fail if we do not set `dummyNode1.nServices` to a reasonable value (NODE_NETWORK | NODE_WITNESS). * [net processing] Remove fClient and m_limited_node fClient is replaced by CanServeBlocks(), and m_limited_node is replaced by IsLimitedPeer(). * [net processing] Replace fHaveWitness with CanServeWitnesses() * [net processing] Remove CNode::nServices Use Peer::m_their_services instead * [net] Return CService from GetLocalAddrForPeer and GetLocalAddress * [net processing] Remove CNode::nLocalServices * fix gettxout help text * Release notes for Miniscript support in P2WSH descriptors * DumpMempool: Use std::chrono instead of weird int64_t arthmetics This makes it so that DumpMempool doesn't depend on MICRO anymore * DumpMempool: Pass in dump_path, stop using gArgs Also introduce node::{ShouldPersistMempool,MempoolPath} helper functions in node/mempool_persist_args.{h,cpp} which are used by non-kernel DumpMempool callers to determine whether or not to automatically dump the mempool and where to dump it to. * scripted-diff: Rename m_is_loaded -> m_load_tried m_is_loaded/IsLoaded() doesn't actually indicate whether or not the mempool was successfully, loaded, but rather if a load has been attempted and did not result in a catastrophic ShutdownRequested. -BEGIN VERIFY SCRIPT- find_regex="\bm_is_loaded\b" \ && git grep -l -E "$find_regex" \ | xargs sed -i -E "s@$find_regex@m_load_tried@g" find_regex="\bIsLoaded\b" \ && git grep -l -E "$find_regex" \ | xargs sed -i -E "s@$find_regex@GetLoadTried@g" find_regex="\bSetIsLoaded\b" \ && git grep -l -E "$find_regex" \ | xargs sed -i -E "s@$find_regex@SetLoadTried@g" -END VERIFY SCRIPT- * mempool: Improve comments for [GS]etLoadTried Also change the param name for SetLoadTried to load_tried. * mempool: Use NodeClock+friends for LoadMempool * Disallow encryption of watchonly wallets Watchonly wallets do not have any private keys to encrypt. It does not make sense to encrypt such wallets, so disable the option to encrypt them. This avoids an assertion that can be hit when encrypting watchonly descriptor wallets. * Move FopenFn to fsbridge namespace [META] In a future commit in this patchset, it will be used by more than just validation, and it needs to align with fopen anyway. * test/fuzz: Invoke LoadMempool via CChainState Not only does this increase coverage, it is also more correct in that when ::LoadMempool is called with a mempool and chainstate, it calls AcceptToMemoryPool with just the chainstate. AcceptToMemoryPool will then act on the chainstate's mempool via CChainState::GetMempool, which may be different from the mempool originally passed to ::LoadMempool. (In this fuzz test's case, it definitely is different) Also, move DummyChainstate to its own file since it's now used by the validation_load_mempool fuzz test to replace CChainState's m_mempool. * LoadMempool: Pass in load_path, stop using gArgs Also: 1. Have CChainState::LoadMempool and ::ThreadImport take in paths and pass it through untouched to LoadMempool. 2. Make LoadMempool exit early if the load_path is empty. 3. Adjust the call to ::ThreadImport in ::AppInitMain to correctly pass in an empty path if mempool persistence is disabled. * Move DEFAULT_PERSIST_MEMPOOL out of libbitcoinkernel It is no longer used by anything inside libbitcoinkernel, move it to node/mempool_persist_args.h where it belongs. * Move {Load,Dump}Mempool to kernel namespace Also: 1. Add the newly introduced kernel/mempool_persist.cpp to IWYU CI script 2. Add chrono mapping for iwyu * build: pass -fno-lto when building expat Otherwise it's autoconf endianess check will fail to determine what the endianess is.. * build: Use Link Time Optimization for Qt code on Linux See: https://www.qt.io/blog/2019/01/02/qt-applications-lto * fuzz: Fix assert bug in txorphan target * doc: remove references to downstream Having references to downstream no-longer make sense now that we've unsubtree'd. * refactor: integrate no_nul into univalue unitester * refactor: remove BOOST_*_TEST_ macros * move-only: Move UniValue::getInt definition to keep class with definitions only Can be reviewed with the git options --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space * univalue: Return more detailed type check error messages * Add symlinks for hardcoded Makefiles in out of tree builds * build: Check for std::atomic::exchange rather than std::atomic_exchange Our usage of std::atomic is with it's own exchange function, not std::atomic_exchange. So we should be looking specifically for that function. Additionally, -pthread and -lpthread have an effect on whether -latomic will be needed, so the atomics check needs to use these flags as well. This will make the flags in use better match what is actually used when linking. This removes the need for -latomic for riscv builds, which resolves a guix cross architecture reproducibility issue. * build: Fix autoconf variable names for tools found by `AC_PATH_TOOL` See the `AC_PATH_TOOL` macro implementation. * depends: default to using GCC tool wrappers (with GCC) This improves support for LTO by using gcc wrappers for ar, nm, ranlib, that correctly setup plugin arguments for LTO. Other HOSTS are using clang. * validation: remove unused using directives The following were unused from the node namespace: - BLOCKFILE_CHUNK_SIZE - nPruneTarget - OpenBlockFile - UNDOFILE_CHUNK_SIZE * refactor: remove unused using directives * tidy: use misc-unused-using-decls https://clang.llvm.org/extra/clang-tidy/checks/misc/unused-using-decls.html * refactor: Make mapBlocksUnknownParent local, and rename it Co-authored-by: Larry Ruane <larryruane@gmail.com> * interfaces, refactor: Add more block information to block connected notifications Add new interfaces::BlockInfo struct to be able to pass extra block information (file and undo information) to indexes which they are updated to use high level interfaces::Chain notifications. This commit does not change behavior in any way. * indexes, refactor: Pass Chain interface instead of CChainState class to indexes Passing abstract Chain interface will let indexes run in separate processes. This commit does not change behavior in any way. * indexes, refactor: Remove CBlockIndex* uses in coinstatsindex LookUpOne function This commit does not change behavior in any way. * indexes, refactor: Remove CBlockIndex* uses in index Init methods Replace overriden index Init() methods that use the best block CBlockIndex* pointer with pure CustomInit() callbacks that are passed the block hash and height. This gets rid of more CBlockIndex* pointer uses so indexes can work outside the bitcoin-node process. It also simplifies the initialization call sequence so index implementations are not responsible for initializing the base class. There is a slight change in behavior here since now the best block pointer is loaded and checked before the custom index init functions are called instead of while they are called. * indexes, refactor: Remove CBlockIndex* uses in index WriteBlock methods Replace WriteBlock method with CustomAppend and pass BlockInfo struct instead of CBlockIndex* pointer This commit does not change behavior in any way. * indexes, refactor: Remove CBlockIndex* uses in index Rewind methods Replace Rewind method with CustomRewind and pass block hashes and heights instead of CBlockIndex* pointers This commit does not change behavior in any way. * indexes, refactor: Remove CChainState use in index CommitInternal method Replace CommitInternal method with CustomCommit and use interfaces::Chain instead of CChainState to generate block locator. This commit does not change behavior in any way, except in the (m_best_block_index == nullptr) case, which was added recently in bitcoin#24117 as part of an ongoing attempt to prevent index corruption if bitcoind is interrupted during startup. New behavior in that case should be slightly better than the old behavior (skipping the entire custom+base commit now vs only skipping the base commit previously) and this might avoid more cases of corruption. * refactor: Use chainman() helper consistently in ChainImpl * guix: Drop repetition of option's default value * Fix `-Wparentheses` gcc warning * depends: modify FastFixedDtoa optimisation flags This fixes a non-determinism issue in the asm produced for this function when cross-compiling on x86_64 and aarch64 for the arm-linux-gnueabihf HOST. Related to bitcoin#21194. * Add missing includes to node/chainstate This is needed for the next commit * Add missing includes They are needed, otherwise the next commit will not compile * Remove unused includes from dbwrapper.h * Remove unused includes in rpc/fees.cpp IWYU confirms that they are unused * refactor: store by OutputType in CoinsResult Store COutputs by OutputType in CoinsResult. The struct stores vectors of `COutput`s by `OutputType` for more convenient access * refactor: use CoinsResult struct in SelectCoins Pass the whole CoinsResult struct to SelectCoins instead of only a vector. This means we now have to remove preselected coins from each OutputType vector and shuffle each vector individually. Pass the whole CoinsResult struct to AttemptSelection. This involves moving the logic in AttemptSelection to a newly named function, ChooseSelectionResult. This will allow us to run ChooseSelectionResult over each OutputType in a later commit. This ensures the backoffs work properly. Update unit and bench tests to use CoinResult. * scripted-diff: rename `FromBinary` helper to `from_binary` (signet miner) -BEGIN VERIFY SCRIPT- sed -i s/FromBinary/from_binary/g ./contrib/signet/miner -END VERIFY SCRIPT- * refactor: move `from_binary` helper from signet miner to test framework Can be easily reviewed with `--color-moved=dimmed-zebra`. * refactor: move PSBT(Map) helpers from signet miner to test framework Can be easily reviewed with `--color-moved=dimmed-zebra`. * test: add constants for PSBT key types (BIP 174) Also take use of the constants in the signet miner to get rid of magic numbers and increase readability and maintainability. * refactor: move helper `random_bytes` to util library Can be easily reviewed with `--color-moved=dimmed-zebra`. * test: add test for decoding PSBT with per-input preimage types * wallet: run coin selection by `OutputType` Run coin selection on each OutputType separately, choosing the best solution according to the waste metric. This is to avoid mixing UTXOs that are of different OutputTypes, which can hurt privacy. If no single OutputType can fund the transaction, then coin selection considers the entire wallet, potentially mixing (current behavior). This is done inside AttemptSelection so that all OutputTypes are considered at each back-off in coin selection. * test: functional test for new coin selection logic Create a wallet with mixed OutputTypes and send a volley of payments, ensuring that there are no mixed OutputTypes in the txs. Finally, verify that OutputTypes are mixed only when necessary. * test: add unit test for AvailableCoins test that UTXOs are bucketed correctly after running AvailableCoins * refactor: Reduce number of LoadChainstate parameters * refactor: Reduce number of LoadChainstate return values * refactor: Reduce number of SanityChecks return values * ci: Enable IWYU in src/kernel directory Suggested bitcoin#25308 (comment) * refactor: move compat.h into compat/ * compat: document FD_SETSIZE redefinition for WIN32 * compat: remove unused WSA* definitions * compat: extract and document MAX_PATH * compat: document S_I* defines when building for Windows * compat: document sockopt_arg_type definition * compat: document error-code mapping See: https://docs.microsoft.com/en-us/windows/win32/winsock/windows-sockets-error-codes-2 * compat: document redefining ssize_t when using MSVC See: https://docs.microsoft.com/en-us/windows/win32/winprog/windows-data-types#ssize_t https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_types.h.html * Add HashWriter without ser-type and ser-version The moved parts can be reviewed with "--color-moved=dimmed-zebra". * Use HashWriter where possible * ci: better pin to dwarf4 in valgrind job Use `-gdwarf` and also set CFLAGS. I was seeing Valgrind issues otherwise. * contrib: remove unneeded valgrind suppressions * doc: BaseIndex sync behavior with empty datadir Make a note about a potentially confusing behavior with `BaseIndex::m_synced`; if the user starts bitcoind with an empty datadir and an index enabled, BaseIndex will consider itself synced (as a degenerate case). This affects how indices are built during IBD (relying solely on BlockConnected signals vs. using ThreadSync()). * refactor: Fix iwyu on node/chainstate * CBlockIndex: ensure phashBlock is not nullptr before dereferencing and remove a now-redundant assert preceding a GetBlockHash() caller. This protects against UB here, and in case of failure (which would indicate a consensus bug), the debug log will print bitcoind: chain.h:265: uint256 CBlockIndex::GetBlockHash() const: Assertion `phashBlock != nullptr' failed. Aborted instead of Segmentation fault * CDiskBlockIndex: remove unused ToString() class member and mark its inherited CBlockIndex#ToString public interface member as deleted, to disallow calling it in the derived CDiskBlockIndex class. * CDiskBlockIndex: rename GetBlockHash() to ConstructBlockHash() and mark the inherited CBlockIndex#GetBlockHash public interface member as deleted, to disallow calling it in the derived CDiskBlockIndex class. Here is a failing test on master demonstrating the inconsistent behavior of the current design: calling the same inherited public interface functions on the same CDiskBlockIndex object should yield identical behavior. ```diff diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp index 6dc522b..dac3840 100644 --- a/src/test/validation_chainstatemanager_tests.cpp +++ b/src/test/validation_chainstatemanager_tests.cpp @@ -240,6 +240,15 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_activate_snapshot, TestChain100Setup) const CBlockIndex* tip = chainman.ActiveTip(); BOOST_CHECK_EQUAL(tip->nChainTx, au_data.nChainTx); + // CDiskBlockIndex "is a" CBlockIndex, as it publicly inherits from it. + // Test that calling the same inherited interface functions on the same + // object yields identical behavior. + CDiskBlockIndex index{tip}; + CBlockIndex *pB = &index; + CDiskBlockIndex *pD = &index; + BOOST_CHECK_EQUAL(pB->GetBlockHash(), pD->GetBlockHash()); + BOOST_CHECK_EQUAL(pB->ToString(), pD->ToString()); + ``` The GetBlockHash() test assertion only passes on master because the different methods invoked by the current design happen to return the same result. If one of the two is changed, it fails like the ToString() assertion does. Redefining inherited non-virtual functions is well-documented as incorrect design to avoid inconsistent behavior (see Scott Meyers, "Effective C++", Item 36). Class usage is confusing when the behavior depends on the pointer definition instead of the object definition (static binding happening where dynamic binding was expected). This can lead to unsuspected or hard-to-track bugs. Outside of critical hot spots, correctness usually comes before optimisation, but the current design dates back to main.cpp and it may possibly have been chosen to avoid the overhead of dynamic dispatch. This solution does the same: the class sizes are unchanged and no vptr or vtbl is added. There are better designs for doing this that use composition instead of inheritance or that separate the public interface from the private implementations. One example of the latter would be a non-virtual public interface that calls private virtual implementation methods, i.e. the Template pattern via the Non-Virtual Interface (NVI) idiom. * refactor: move CBlockIndex#ToString() from header to implementation which allows dropping tinyformat.h from the header file. * gui: Fix translator comment for Restore Wallet QInputDialog This also changes the window title name from `Restore Name` to `Restore Wallet`. * test: support passing PSBTMaps directly to PSBT ctor This will allow to create simple PSBTs as short one-liners, without the need to have three individual assignments (globals, inputs, outputs). * test: check that combining PSBTs with different txs fails * fuzz: Remove no-op SetMempoolConstraints * Bugfix: RPC/blockchain: Correct type of "value" in getblock docs; add missing "desc" * RPC: Document "asm" and "hex" fields for scripts * test: remove unused if statements * refactor: Make CTransaction constructor explicit It involves calculating two hashes, so the performance impact should be made explicit. Also, add the module to iwyu. * fuzz: refactor: Replace NullUniValue with UniValue{} This is needed for the scripted-diff to compile in the next commit * scripted-diff: Replace NullUniValue with UniValue::VNULL This is required for removing the UniValue copy constructor. -BEGIN VERIFY SCRIPT- sed -i 's/return NullUniValue/return UniValue::VNULL/g' $(git grep -l NullUniValue ':(exclude)src/univalue') -END VERIFY SCRIPT- * psbt: Fix unsigned integer overflow * fix comment spellings from the codespell lint test/lint/all-lint.py includes the codespell lint * depends: always use correct ar for win qt If we don't set this explicitly, then qt will still use it's default windows ar, when building with LTO (when we want it to use gcc-ar). So set `QMAKE_LIB` which is used for win32, and defaults to `ar -rc`. This way we always get the correct ar. Issue can be seen building in Guix with LTO. i.e: ```bash x86_64-w64-mingw32-ar: .obj/release/hb-blob.o: plugin needed to handle lto object ``` * refactor: Remove not needed std::max * scripted-diff: Rename addrman time symbols -BEGIN VERIFY SCRIPT- ren() { sed -i "s:\<$1\>:$2:g" $(git grep -l "\<$1\>" ./src ./test); } ren nLastTry m_last_try ren nLastSuccess m_last_success ren nLastGood m_last_good ren nLastCountAttempt m_last_count_attempt ren nSinceLastTry since_last_try ren nTimePenalty time_penalty ren nUpdateInterval update_interval ren fCurrentlyOnline currently_online -END VERIFY SCRIPT- * util: Add HoursDouble * Add ChronoFormatter to serialize * Add type-safe AdjustedTime() getter to timedata Also, fix includes. The getter will be used in a future commit. * refactor: Use type-safe std::chrono for addrman time * refactor: remove unnecessary string initializations * tidy: enable readability-redundant-string-init See: https://releases.llvm.org/14.0.0/tools/clang/tools/extra/docs/clang-tidy/checks/readability-redundant-string-init.html * depends: expat 2.4.8 * depends: re-enable using -flto when building expat * script: actually trigger the optimization in BuildScript The counter is an optimization over calling `ret.empty()`. It was suggested that the compiler would realize `cnt` is only `0` on the first iteration, and not actually emit the check and conditional. This optimization was actually not triggered at all, since we incremented `cnt` at the beginning of the first iteration. Fix it by incrementing at the end instead. This was reported by Github user "Janus". * test: Drop unused boost workaround * refactor: log `nEvicted` message in `LimitOrphans` then return void `LimitOrphans()` can log expired tx and it should log evicted tx as well instead of returning the number for caller to print the message. Since `LimitOrphans()` now return void, the redundant assertion check in fuzz test is also removed. * [unit tests] individual RBF Rules in isolation Test each component of the RBF policy in isolation. Unlike the RBF functional tests, these do not rely on things like RPC results, mempool submission, etc. * guix: enable SSP for RISC-V glibc (2.27) Pass `--enable-stack-protector=all` when building the glibc used for the RISC-V toolchain, to enable stack smashing protection on all functions, in the glibc code. * guix: pass enable-bind-now to glibc Both glibcs we build support `--enable-bind-now`: Disable lazy binding for installed shared objects and programs. This provides additional security hardening because it enables full RELRO and a read-only global offset table (GOT), at the cost of slightly increased program load times. See: https://www.gnu.org/software/libc/manual/html_node/Configuring-and-compiling.html * guix: enable hardening options in GCC Build Pass `--enable-default-pie` and `--enable-default-ssp` when configuring our GCCs. This achieves the following: --enable-default-pie Turn on -fPIE and -pie by default. --enable-default-ssp Turn on -fstack-protector-strong by default. Note that this isn't a replacement for passing hardneing flags ourselves, but introduces some redundency, and there isn't really a reason to not build a more "hardenings enabled" toolchain by default. See also: https://gcc.gnu.org/install/configure.html * libxcb: use a patch instead of sed To remove the unneeded pthread-stubs requirements. * tidy: run clang-tidy in quiet mode Co-authored-by: fanquake <fanquake@gmail.com> Co-authored-by: MacroFake <falke.marco@gmail.com> Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Co-authored-by: Pieter Wuille <pieter@wuille.net> Co-authored-by: Andrew Chow <achow101-github@achow101.com> Co-authored-by: furszy <matiasfurszyfer@protonmail.com> Co-authored-by: Sebastian Falbesoner <sebastian.falbesoner@gmail.com> Co-authored-by: Vasil Dimov <vd@FreeBSD.org> Co-authored-by: Antoine Riard <dev@ariard.me> Co-authored-by: glozow <gloriajzhao@gmail.com> Co-authored-by: w0xlt <94266259+w0xlt@users.noreply.github.com> Co-authored-by: Suhas Daftuar <sdaftuar@gmail.com> Co-authored-by: Carl Dong <contact@carldong.me> Co-authored-by: Antoine Poinsot <darosior@protonmail.com> Co-authored-by: Pieter Wuille <pieter.wuille@gmail.com> Co-authored-by: John Newbery <john@johnnewbery.com> Co-authored-by: dergoegge <n.goeggi@gmail.com> Co-authored-by: Marnix <93143998+MarnixCroes@users.noreply.github.com> Co-authored-by: chinggg <24590067+chinggg@users.noreply.github.com> Co-authored-by: Pablo Greco <psgreco@gmail.com> Co-authored-by: eugene <elzeigel@gmail.com> Co-authored-by: Larry Ruane <larryruane@gmail.com> Co-authored-by: Ryan Ofsky <ryan@ofsky.org> Co-authored-by: josibake <josibake@protonmail.com> Co-authored-by: Russell Yanofsky <russ@yanofsky.org> Co-authored-by: James O'Beirne <james.obeirne@pm.me> Co-authored-by: Jon Atack <jon@atack.com> Co-authored-by: Luke Dashjr <luke-jr+git@utopios.org> Co-authored-by: Aurèle Oulès <aurele@oules.com> Co-authored-by: Greg Weber <gregwebs@users.noreply.github.com>

Suggested bitcoin/bitcoin#25308 (comment)

DrahtBot added the Refactoring label Jun 8, 2022

maflcko reviewed Jun 8, 2022

View reviewed changes

src/kernel/checks.cpp Show resolved Hide resolved

DrahtBot mentioned this pull request Jun 8, 2022

logging: add LogPrintfCategory to log unconditionally with category #25306

Merged

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Jun 8, 2022

ci: Enable IWYU in src/kernel directory

1066e9c

Suggested bitcoin#25308 (comment)

ryanofsky force-pushed the pr/retcode branch from 5ceae5c to 1066e9c Compare June 8, 2022 16:41

ryanofsky commented Jun 8, 2022

View reviewed changes

src/kernel/checks.cpp Show resolved Hide resolved

This was referenced Jun 8, 2022

Severity-based logging -- parent PR #25203

Closed

indexes: Read the locator's top block during init, allow interaction with reindex-chainstate #25193

Merged

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Jun 8, 2022

ci: Enable IWYU in src/kernel directory

3981f5e

Suggested bitcoin#25308 (comment)

ryanofsky force-pushed the pr/retcode branch from 1066e9c to 3981f5e Compare June 8, 2022 16:58

DrahtBot mentioned this pull request Jun 9, 2022

refactor: Split util/system into exception, shell, and fs-specific files #25152

Closed

DrahtBot added the Needs rebase label Jun 14, 2022

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Jun 16, 2022

ci: Enable IWYU in src/kernel directory

f3ec82f

Suggested bitcoin#25308 (comment)

ryanofsky force-pushed the pr/retcode branch from 3981f5e to f3ec82f Compare June 16, 2022 18:38

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Jun 16, 2022

ci: Enable IWYU in src/kernel directory

3135de1

Suggested bitcoin#25308 (comment)

DrahtBot removed the Needs rebase label Jun 16, 2022

This was referenced Jun 22, 2022

assumeutxo: add init and completion logic #24232

Closed

CChainState -> Chainstate #24513

Merged

ci: add unused-using-decls to clang-tidy #25466

Merged

[kernel 3a/n] Decouple CTxMemPool from ArgsManager #25290

Merged

dongcarl reviewed Jun 27, 2022

View reviewed changes

src/node/chainstate.h Outdated Show resolved Hide resolved

fanquake reviewed Jun 28, 2022

View reviewed changes

This was referenced Jun 29, 2022

Use steady clock for all millis bench logging #25499

Merged

wallet: more accurate target for large transactions #25497

Merged

Revert "bnb: exit selection when best_waste is 0" #25495

Merged

DrahtBot removed the Needs rebase label Jul 19, 2022

ryanofsky and others added 4 commits July 19, 2022 15:54

refactor: Reduce number of LoadChainstate parameters

3b91d4b

refactor: Reduce number of LoadChainstate return values

b3e7de7

refactor: Reduce number of SanityChecks return values

6db6552

ci: Enable IWYU in src/kernel directory

1e761a0

Suggested bitcoin#25308 (comment)

DrahtBot added the Needs rebase label Jul 19, 2022

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Jul 19, 2022

ci: Enable IWYU in src/kernel directory

fb95175

Suggested bitcoin#25308 (comment)

ryanofsky force-pushed the pr/retcode branch from fb31664 to 1e761a0 Compare July 19, 2022 22:01

DrahtBot removed the Needs rebase label Jul 19, 2022

maflcko merged commit 0897b18 into bitcoin:master Jul 20, 2022

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 20, 2022

Merge bitcoin#25308: refactor: Reduce number of LoadChainstate parame…

51ff6f8

…ters an…

ryanofsky mentioned this pull request Jul 21, 2022

refactor: Add util::Result failure values, multiple error and warning messages #25665

Open

stickies-v pushed a commit to stickies-v/bitcoin that referenced this pull request Aug 2, 2022

ci: Enable IWYU in src/kernel directory

917b82d

Suggested bitcoin#25308 (comment)

Rspigler pushed a commit to Rspigler/bitcoin that referenced this pull request Aug 21, 2022

ci: Enable IWYU in src/kernel directory

0effd01

Suggested bitcoin#25308 (comment)

furszy mentioned this pull request Sep 7, 2022

wallet: bugfix, load a wallet with an unknown/corrupt descriptor causes a fatal error #26021

Merged

janus pushed a commit to BitgesellOfficial/bitgesell that referenced this pull request Jan 20, 2023

ci: Enable IWYU in src/kernel directory

f082991

Suggested bitcoin/bitcoin#25308 (comment)

bitcoin locked and limited conversation to collaborators Jul 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor: Reduce number of LoadChainstate parameters and return values #25308

refactor: Reduce number of LoadChainstate parameters and return values #25308

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!


		#include <assert.h>

refactor: Reduce number of LoadChainstate parameters and return values #25308

refactor: Reduce number of LoadChainstate parameters and return values #25308

Uh oh!

Conversation

Uh oh!

Uh oh!

Conflicts

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!