bfcd60f test: activate all index types in feature_init.py (Martin Zumsande)
0243907 index: Don't commit without valid m_best_block_index (Martin Zumsande)

Pull request description:

  When an index thread receives an interrupt during init before it got to index anything (so `m_best_block_index == nullptr` still), it will still try to commit previous "work" before stopping the thread. That means that `BaseIndex::CommitInternal()` calls `GetLocator(nullptr)`, which returns an locator to the tip ([code](https://github.com/bitcoin/bitcoin/blob/06b6369766137756648b3cb62c8f385cca234e69/src/chain.cpp#L31-L32)), and saves it to the index DB.
  On the next startup, this locator will be read and it will be assumed that we have successfully synced the index to the tip, when in reality we have indexed nothing.
  In the case of coinstatsindex, this would lead to a shutdown of bitcoind without any indication what went wrong. For the other indexes, there would be no immediate shutdown, but the index would be corrupt.

  This PR fixes this by not committing when `m_best_block_index==nullptr`, and it also adds an error log message to the silent coinstatsindex shutdown path.

  This is another small bug found by `feature_init.py` - the second commit enables blockfilterindex and coinstatsindex for this test, enabling coinstatsindex without the first commit would have led to frequent failures.

ACKs for top commit:
  fjahr:
    reACK bfcd60f
  shaavan:
    reACK bfcd60f

Tree-SHA512: 8e2bac0fc40cde209518a9e59b597ae0a5a875a2a90898673987c91733718d40e528dada942bf552b58bc021bf46e59da2d0cc5a61045f48f9bae2b1baf6033b

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.

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.

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.

* 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 <[email protected]>

* 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 <[email protected]>

* 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 <[email protected]>

* 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 <[email protected]>
Co-authored-by: MacroFake <[email protected]>
Co-authored-by: Hennadii Stepanov <[email protected]>
Co-authored-by: Pieter Wuille <[email protected]>
Co-authored-by: Andrew Chow <[email protected]>
Co-authored-by: furszy <[email protected]>
Co-authored-by: Sebastian Falbesoner <[email protected]>
Co-authored-by: Vasil Dimov <[email protected]>
Co-authored-by: Antoine Riard <[email protected]>
Co-authored-by: glozow <[email protected]>
Co-authored-by: w0xlt <[email protected]>
Co-authored-by: Suhas Daftuar <[email protected]>
Co-authored-by: Carl Dong <[email protected]>
Co-authored-by: Antoine Poinsot <[email protected]>
Co-authored-by: Pieter Wuille <[email protected]>
Co-authored-by: John Newbery <[email protected]>
Co-authored-by: dergoegge <[email protected]>
Co-authored-by: Marnix <[email protected]>
Co-authored-by: chinggg <[email protected]>
Co-authored-by: Pablo Greco <[email protected]>
Co-authored-by: eugene <[email protected]>
Co-authored-by: Larry Ruane <[email protected]>
Co-authored-by: Ryan Ofsky <[email protected]>
Co-authored-by: josibake <[email protected]>
Co-authored-by: Russell Yanofsky <[email protected]>
Co-authored-by: James O'Beirne <[email protected]>
Co-authored-by: Jon Atack <[email protected]>
Co-authored-by: Luke Dashjr <[email protected]>
Co-authored-by: Aurèle Oulès <[email protected]>
Co-authored-by: Greg Weber <[email protected]>

Summary:
PR description:

> When an index thread receives an interrupt during init before it got to index anything (so `m_best_block_index == nullptr` still), it will still try to commit previous "work" before stopping the thread. That means that `BaseIndex::CommitInternal()` calls `GetLocator(nullptr)`, which returns an locator to the tip (code), and saves it to the index DB.
> On the next startup, this locator will be read and it will be assumed that we have successfully synced the index to the tip, when in reality we have indexed nothing.
> In the case of coinstatsindex, this would lead to a shutdown of bitcoind without any indication what went wrong. For the other indexes, there would be no immediate shutdown, but the index would be corrupt.
>
> This PR fixes this by not committing when `m_best_block_index==nullptr`, and it also adds an error log message to the silent coinstatsindex shutdown path.
>
> This is another small bug found by feature_init.py - the second commit enables blockfilterindex and coinstatsindex for this test, enabling coinstatsindex without the first commit would have led to frequent failures.

commits:
> index: Don't commit without valid m_best_block_index
>
> Also report an error when coinstatsindex init fails.

> test: activate all index types in feature_init.py

This is a backport of [[bitcoin/bitcoin#24117 | core#24117]]
Depends on D12621

Test Plan: `ninja all check-all`

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Subscribers: Fabien

Differential Revision: https://reviews.bitcoinabc.org/D12622

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/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.