Also rewrite CheckBlockIndex() to perform tests on all chainstates.
This increases sanity-check coverage, as any place in our code where we were
invoke CheckBlockIndex() on a single chainstate will now invoke the sanity
checks on all chainstates.
This change also tightens up the checks on setBlockIndexCandidates and
mapBlocksUnlinked, to more precisely match what we aim for even in the presence
of assumed-valid blocks.
When using assumeutxo and multiple chainstates are active, the background
chainstate should consider all HAVE_DATA blocks that are ancestors of the
snapshotted block and that have more work than the tip as potential candidates.
When using assumeutxo, we only need the background chainstate to consider
blocks that are on the chain leading to the snapshotted block.
Note that this introduces the new invariant that we can only have an assumeutxo
snapshot where the snapshotted blockhash is in our block index. Unknown block
hashes that are somehow passed in will cause assertion failures when processing
new blocks.
Includes test fixes and improvements by Andrew Chow and Fabian Jahr.
Separate the notion of which blocks are stored on disk, and what data is in our
block index, from what tip a chainstate might be able to get to. We can use
chainstate-agnostic data to determine when to store a block on disk (primarily,
an anti-DoS set of criteria) and let the chainstates figure out for themselves
when a block is of interest for being a candidate tip.
Note: some of the invariants in CheckBlockIndex are modified, but more work is
needed (ie to move CheckBlockIndex to ChainstateManager, as most of what
CheckBlockIndex is doing is checking the consistency of the block index, which
is outside of Chainstate).
Block arrival information (and the preciousblock RPC, a related concept) are
both chainstate-agnostic, so these are moved to ChainstateManager. This should
just be a refactor, without any observable behavior changes.
This change drops the last kernel dependency on shutdown.cpp. It also adds new
hooks for libbitcoinkernel applications to be able to interrupt kernel
operations when the chain tip changes.
This is a refactoring that does not affect behavior. (Looking at the code it
can appear like the new break statement in the ActivateBestChain function is a
change in behavior, but actually the previous StartShutdown call was indirectly
triggering a break before, because it was causing m_chainman.m_interrupt to be
true. The new code just makes the break more obvious.)
bitcoin/bitcoin#23381 refactored PolicyScriptChecks to store the
precomputed txdata in the workspace, so I have added the genesis hash to
the transform of the workspaces in AcceptMultipleTransactions.
I don't think there was a specific test for this, so during review it
may be worth checking this thoroughly. We also need a test to cover
this.
FatalError replaces what previously was the AbortNode function in
shutdown.cpp.
This commit is part of the libbitcoinkernel project and further removes
the shutdown's and, more generally, the kernel library's dependency on
interface_ui with a kernel notification method. By removing interface_ui
from the kernel library, its dependency on boost is reduced to just
boost::multi_index. At the same time it also takes a step towards
de-globalising the interrupt infrastructure.
Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
Co-authored-by: TheCharlatan <seb.kung@gmail.com>
This and the following commit seek to decouple the libbitcoinkernel
library from the shutdown code. As a library, it should it should have
its own flexible interrupt infrastructure without relying on node-wide
globals.
The commit takes the first step towards this goal by de-globalising
`ShutdownRequested` calls in kernel code.
Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
Co-authored-by: TheCharlatan <seb.kung@gmail.com>
1c7d08b9ac validation: Stricter assumeutxo error handling in InvalidateCoinsDBOnDisk (Ryan Ofsky)
9047337d36 validation: Stricter assumeutxo error handling in LoadChainstate (Ryan Ofsky)
Pull request description:
There are two places in assumeutxo code where it is calling `AbortNode` to trigger asynchronous shutdowns without returning errors to calling functions.
One case, in `LoadChainstate`, happens when snapshot validation succeeds, and there is an error trying to replace the background chainstate with the snapshot chainstate.
The other case, in `InvalidateCoinsDBOnDisk`, happens when snapshot validatiion fails, and there is an error trying to remove the snapshot chainstate.
In both cases the node is being forced to shut down, so it makes sense for these functions to raise errors so callers can know that an error happened without having to infer it from the shutdown state.
Noticed these cases while reviewing #27861, which replaces the `AbortNode` function with a `FatalError` function.
ACKs for top commit:
achow101:
ACK 1c7d08b9ac
TheCharlatan:
ACK 1c7d08b9ac
jamesob:
ACK 1c7d08b9ac ([`jamesob/ackr/27862.1.ryanofsky.validation_stricter_assu`](https://github.com/jamesob/bitcoin/tree/ackr/27862.1.ryanofsky.validation_stricter_assu))
Tree-SHA512: fb1dcde3fa0e77b4ba0c48507d289552b939c2866781579c8e994edc209abc3cd29cf81c89380057199323a8eec484956abb1fd3a43c957ecd0e7f7bbfd63fd8
Currently InvalidateCoinsDBOnDisk is calling AbortNode without an error to the
caller if it fails. Change it to return just return util::Result, and update
the caller to handle the error itself.
This causes the secondary error to be shown below the main error instead of the
other way around.
