029ba1a21d index: remove CBlockIndex access from CustomAppend() (furszy)
91b7ab6c69 refactor: index, simplify CopyHeightIndexToHashIndex to process single block (furszy)
6f1392cc42 indexes, refactor: Remove remaining CBlockIndex* uses in index Rewind methods (Ryan Ofsky)
0a248708dc indexes, refactor: Stop requiring CBlockIndex type to call IsBIP30Unspendable (Ryan Ofsky)
331a25cb16 test: indexes, avoid creating threads when sync runs synchronously (furszy)
Pull request description:
Combining common refactors from #24230 and #26966, aiming to move both efforts forward while reducing their size and review burden.
Broadly, #24230 focuses on enabling indexes to run in a separate process, and #26966 aims to parallelize the indexes initial synchronization process. A shared prerequisite for both is ensuring that only the base index class interacts with the node’s chain internals - child index classes should instead operate solely through chain events.
This PR moves disk read lookups from child index classes to the base index class. It also includes a few documentation improvements and a test-only code cleanup.
ACKs for top commit:
maflcko:
review ACK 029ba1a21d👡
achow101:
ACK 029ba1a21d
TheCharlatan:
Re-ACK 029ba1a21d
davidgumberg:
ACK 029ba1a21d
mzumsande:
Code Review ACK 029ba1a21d
Tree-SHA512: f073af407fc86f228cb47a32c7bcf2241551cc89ff32059317eb81d5b86fd5fda35f228d2567e0aedbc9fd6826291f5fee05619db35ba44108421ae04d11e6fb
fa9ca13f35 refactor: Sort includes of touched source files (MarcoFalke)
facb152697 scripted-diff: Bump copyright headers after include changes (MarcoFalke)
fae71d30f7 clang-tidy: Apply modernize-deprecated-headers (MarcoFalke)
Pull request description:
Bitcoin Core is written in C++, so it is confusing to sometimes use the deprecated C headers (with the `.h` extension). For example, it is less clear whether `string.h` refers to the file in this repo or the cstring stdlib header (https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2121492797).
The check is currently disabled for headers, to exclude subtree headers.
ACKs for top commit:
l0rinc:
ACK fa9ca13f35
achow101:
ACK fa9ca13f35
janb84:
ACK fa9ca13f35
stickies-v:
ACK fa9ca13f35
Tree-SHA512: 6639608308c598d612e24435aa519afe92d71b955874b87e527245291fb874b67f3ab95d3a0a5125c6adce5eb41c0d62f6ca488fbbfd60a94f2063d734173f4d
f6b782f3aa doc: Improve m_best_header documentation (Martin Zumsande)
ee673b9aa0 validation: remove m_failed_blocks (Martin Zumsande)
ed764ea2b4 validation: Add more checks to CheckBlockIndex() (Martin Zumsande)
9a70883002 validation: in invalidateblock, calculate m_best_header right away (Martin Zumsande)
8e39f2d20d validation: in invalidateblock, mark children as invalid right away (Martin Zumsande)
4c29326183 validation: cache all headers with enough PoW in invalidateblock (Martin Zumsande)
15fa5b5a90 validation: call InvalidBlockFound also from AcceptBlock (Martin Zumsande)
Pull request description:
Some fields in validation are set opportunistically by "best effort":
- The `BLOCK_FAILED_CHILD` status (which means that the block index has an invalid predecessor)
- `m_best_header` (the most-work header not known to be invalid).
This means that there are known situations in which these fields are not set when they should be, or set to wrong values. This is tolerated because the fields are not used for anything consensus-critical and triggering these situations involved creating invalid blocks with valid PoW header, so would have a cost attached. Also, having stricter guarantees for these fields requires iterating over the entire block index, which has some DoS potential, especially with any header above the checkpoint being accepted int he past (see e.g. #11531).
However, there are reasons to change this now:
- RPCs use these fields and can report wrong results
- There is the constant possibility that someone could add code that expects these fields to be correct, especially because it is not well documented that these fields cannot always be relied upon.
- DoS concerns have become less of an issue after #25717 - now an attacker would need to invest much more work because they can't fork off the last checkpoint anymore
This PR continues the work from #30666 to ensure that `BLOCK_FAILED_CHILD` status and `m_best_header` are always correct:
- it adds a call to `InvalidChainFound()` in `AcceptBlock()`.
- it adds checks for `BLOCK_FAILED_CHILD` and `m_best_header` to `CheckBlockIndex()`. In order to be able to do this, the existing cache in the RPC-only `InvalidateBlock()` is adjusted to handle these as well. These are performance optimizations with the goal of avoiding having a call of `InvalidChainFound()` / looping over the block index after each disconnected block.
I also wrote a fuzz test to find possible edge cases violating `CheckBlockIndex`, which I will PR separately soon.
- it removes the `m_failed_blocks` set, which was a heuristic necessary when we couldn't be sure if a given block index had an invalid predecessor or not. Now that we have that guarantee, the set is no longer needed.
ACKs for top commit:
stickies-v:
re-ACK f6b782f3aa
achow101:
reACK f6b782f3aa
ryanofsky:
Code review ACK f6b782f3aa with only minor code & comment updates
TheCharlatan:
Re-ACK f6b782f3aa
Tree-SHA512: 1bee324216eeee6af401abdb683abd098b18212833f9600dbc0a46244e634cb0e6f2a320c937a5675a12af7ec4a7d10fabc1db9e9bc0d9d0712e6e6ca72d084f
After changes in previous commits, we now mark all blocks that descend from an invalid block
immediately as the block is found invalid. This happens both in the AcceptBlock
and ConnectBlock stages of block validation.
As a result, the pindexPrev->nStatus check in AcceptBlockHeader is now sufficient to detect
invalid blocks and checking m_failed_blocks there is no longer necessary.
This can be reproduced according to the developer notes with something
like
( cd ./src/ && ../contrib/devtools/run-clang-tidy.py -p ../bld-cmake -fix -j $(nproc) )
Also, the header related changes were done manually.
fab1e02086 refactor: Pass verification_progress into block tip notifications (MarcoFalke)
fa76b378e4 rpc: Round verificationprogress to exactly 1 for a recent tip (MarcoFalke)
faf6304bdf test: Use mockable time in GuessVerificationProgress (MarcoFalke)
Pull request description:
Some users really seem to care about this. While it shouldn't matter much, the diff is so trivial that it is probably worth doing.
Fixes#31127
One could also consider to split the field into two dedicated ones (https://github.com/bitcoin/bitcoin/issues/28847#issuecomment-1807115357), but this is left for a more involved follow-up and may also be controversial.
ACKs for top commit:
achow101:
ACK fab1e02086
pinheadmz:
ACK fab1e02086
sipa:
utACK fab1e02086
Tree-SHA512: a3c24e3c446d38fbad9399c1e7f1ffa7904490a3a7d12623b44e583b435cc8b5f1ba83b84d29c7ffaf22028bc909c7cec07202b825480449c6419d2a190938f5
3e6ac5bf77 refactor: validation: mark CheckBlockIndex as const (stickies-v)
61a51eccbb validation: don't use GetAll() in CheckBlockIndex() (stickies-v)
d05481df64 refactor: validation: mark SnapshotBase as const (stickies-v)
Pull request description:
While reviewing another PR, I [noticed](https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2056509235) that `ChainstateManager::CheckBlockIndex()` is not a `const` method. To try and assert that this method was not causing any side-effects, I modified the method to make it `const`. It did not surface any errors, but I think it would be good to merge this change regardless, even if `CheckBlockIndex` is only used in regtest.
This PR removes `CheckBlockIndex()`'s calls to non-const `ChainstateManager` methods by marking `SnapshotBase` `const` and ~inlining the `GetAll()` calls (thereby also performing consistency checks on invalid or fully validated `m_disabled==true` chainstates, as slight behaviour change), and finally marks `CheckBlockIndex()` as `const`.
ACKs for top commit:
achow101:
ACK 3e6ac5bf77
mzumsande:
Code Review ACK 3e6ac5bf77
TheCharlatan:
ACK 3e6ac5bf77
Tree-SHA512: 3d3cd351f5af1fab9a9498218ec62dba6e397fc7b5f4868ae0a77dc2b7c813d12c4f53f253f209101a3f6523695014e20c82dfac27cf0035611d5dd29feb80b5
We don't add or maintain these, and they are of little value, as
well as having the effect of polluting diffs.
They are also wrong, i.e DEFAULT_SCRIPTCHECK_THREADS is not in
validation.h.
e976bd3045 validation: add randomness to periodic write interval (Andrew Toth)
2e2f410681 refactor: replace m_last_write with m_next_write (Andrew Toth)
b557fa7a17 refactor: rename fDoFullFlush to should_write (Andrew Toth)
d73bd9fbe4 validation: write chainstate to disk every hour (Andrew Toth)
0ad7d7abdb test: chainstate write test for periodic chainstate flush (Andrew Toth)
Pull request description:
Since #28233, periodically writing the chainstate to disk every 24 hours does not clear the dbcache. Since #28280, periodically writing the chainstate to disk is proportional only to the amount of dirty entries in the cache. Due to these changes, it is no longer beneficial to only write the chainstate to disk every 24 hours. The periodic flush interval was necessary because every write of the chainstate would clear the dbcache. Now, we can get rid of the periodic flush interval and simply write the chainstate along with blocks and block index at least every hour.
Three benefits of doing this:
1. For IBD or reindex-chainstate with a combination of large dbcache setting, slow CPU, slow internet speed/unreliable peers, it could be up to 24 hours until the chainstate is persisted to disk. A power outage or crash could potentially lose up to 24 hours of progress. If there is a very large amount of dirty cache entries, writing to disk when a flush finally does occur will take a very long time. Crashing during this window of writing can cause https://github.com/bitcoin/bitcoin/issues/11600. By syncing every hour in unison with the block index we avoid this problem. Only a maximum of one hour of progress can be lost, and the window for crashing during writing is much smaller. For IBD with lower dbcache settings, faster CPU, or better internet speed/reliable peers, chainstate writes are already triggered more often than every hour so this change will have no effect on IBD.
2. Based on discussion in #28280, writing only once every 24 hours during long running operation of a node causes IO spikes. Writing smaller chainstate changes every hour like we do with blocks and block index will reduce IO spikes.
3. Faster shutdown speeds. All dirty chainstate entries must be persisted to disk on shutdown. If we have a lot of dirty entries, such as when close to 24 hours or if we sync with a large dbcache, it can take a long time to shutdown. By keeping the chainstate clean we avoid this problem.
Inspired by [this comment](https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2121088705).
Resolves https://github.com/bitcoin/bitcoin/issues/11600
ACKs for top commit:
achow101:
ACK e976bd3045
davidgumberg:
utACK e976bd3045
sipa:
utACK e976bd3045
l0rinc:
ACK e976bd3045
Tree-SHA512: 5bccd8f1dea47f9820a3fd32fe3bb6841c0167b3d6870cc8f3f7e2368f124af1a914bca6acb06889cd7183638a8dbdbace54d3237c3683f2b567eb7355e015ee
abe43dfadd doc: release note for #27826 (Sjors Provoost)
f9fa28788e Use LogBlockHeader for compact blocks (Sjors Provoost)
bad7c91479 Log which peer sent us a header (Sjors Provoost)
9d3e39c29c Log block header in net_processing (Sjors Provoost)
Pull request description:
Fixes#27744
Since #27278 we log received headers. For compact blocks we also log which peer sent it (e5ce857634), but not for regular headers. That required an additional refactor, which this PR provides.
Move the logging from validation to net_processing.
This also reduces the number of log entries (under default configuration) per compact block header from 3 to 2: one for the header and one for the connected tip.
The PR introduces a new helper method `LogBlockHeader`.
When receiving a _compact block_ we call `LogBlockHeader` from the exact same place as where we previously logged. So that log message doesn't change. What does change is that we no longer _also_ log from `AcceptBlockHeader`.
When receiving a regular header(s) message, _we only log the last one_. This is a change in behaviour because it was simpler to implement, but it's probably better anyway. It does mean that if a peer sends of a bunch of headers of which _any_ is invalid, we won't log it (here).
Lastly I expanded the code comment explaining why we log this. It initially only covered selfish mining, but we also care about peers sending us headers but not following up (see e.g. #27626).
Example log:
```
2023-06-05T13:12:21Z Saw new header hash=000000000000000000045910263ef84b575ae3af151865238f1e5c619e69c330 height=792964 peer=0
2023-06-05T13:12:23Z UpdateTip: new best=000000000000000000045910263ef84b575ae3af151865238f1e5c619e69c330 height=792964 version=0x20000000 log2_work=94.223098 tx=848176824 date='2023-06-05T13:11:49Z' progress=1.000000 cache=6.4MiB(54615txo)
2023-06-05T13:14:05Z Saw new cmpctblock header hash=00000000000000000003c6fd4ef2e1246a3f9e1fffab7247344f94cadb9de979 height=792965 peer=0
2023-06-05T13:14:05Z UpdateTip: new best=00000000000000000003c6fd4ef2e1246a3f9e1fffab7247344f94cadb9de979 height=792965 version=0x20000000 log2_work=94.223112 tx=848179461 date='2023-06-05T13:13:58Z' progress=1.000000 cache=7.2MiB(61275txo)
2023-06-05T13:14:41Z Saw new header hash=000000000000000000048e6d69c8399992782d08cb57f5d6cbc81a9f996c3f43 height=792966 peer=8
2023-06-05T13:14:42Z UpdateTip: new best=000000000000000000048e6d69c8399992782d08cb57f5d6cbc81a9f996c3f43 height=792966 version=0x2db3c000 log2_work=94.223126 tx=848182944 date='2023-06-05T13:14:35Z' progress=1.000000 cache=8.0MiB(69837txo)
```
ACKs for top commit:
danielabrozzoni:
tACK abe43dfadd
achow101:
ACK abe43dfadd
vasild:
ACK abe43dfadd
Tree-SHA512: 081e0de62cbd8a0b35cf54daaa09e3e6991d0cc9f706ef3eb50908752fe7815de69b367f7313381c90cd8d5de0ae5f532d1cd54948c5c1133b1832f266d9c232
e3014017ba test: add IsActiveAfter tests for versionbits (Anthony Towns)
60950f77c3 versionbits: docstrings for BIP9Info (Anthony Towns)
7565563bc7 tests: refactor versionbits fuzz test (Anthony Towns)
2e4e9b9608 tests: refactor versionbits unit test (Anthony Towns)
525c00f91b versionbits: Expose VersionBitsConditionChecker via impl header (Anthony Towns)
e74a7049b4 versionbits: Expose StateName function (Anthony Towns)
d00d1ed52c versionbits: Split out internal details into impl header (Anthony Towns)
37b9b67a39 versionbits: Simplify VersionBitsCache API (Anthony Towns)
1198e7d2fd versionbits: Move BIP9 status logic for getblocktemplate to versionbits (Anthony Towns)
b1e967c3ec versionbits: Move getdeploymentinfo logic to versionbits (Anthony Towns)
3bd32c2055 versionbits: Move WarningBits logic from validation to versionbits (Anthony Towns)
5da119e5d0 versionbits: Change BIP9Stats to uint32_t types (Anthony Towns)
a679040ec1 consensus/params: Move version bits period/threshold to bip9 param (Anthony Towns)
e9d617095d versionbits: Remove params from AbstractThresholdConditionChecker (Anthony Towns)
9bc41f1b48 versionbits: Use std::array instead of C-style arrays (Anthony Towns)
Pull request description:
Increases the encapsulation/modularity of the versionbits code, moving more of the logic into the versionbits module rather than having it scattered across validation and rpc code. Updates unit/fuzz tests to test the actual code used rather than just a close approximation of it.
ACKs for top commit:
achow101:
ACK e3014017ba
TheCharlatan:
Re-ACK e3014017ba
darosior:
ACK e3014017ba
Tree-SHA512: 2978db5038354b56fa1dd6aafd511099e9c16504d6a88daeac2ff2702c87bcf3e55a32e2f0a7697e3de76963b68b9d5ede7976ee007e45862fa306911194496d
Previously ChainstateManager::AcceptBlockHeader would log when it
saw a new header. This commit moves logging to the call site(s) in
net_processing. The next commits will then log which peer sent it
and whether it was part of a compact block.
This commit changes behavior:
- when multiple headers are received in a single message, only the
last one is logged
- if any of the headers are invalid, the valid ones are not logged
This happens because net_processing calls ProcessNewBlockHeaders
with multiple headers, which then calls AcceptBlockHeader one
header at a time.
Additionally:
- when the header is received via a compact block, there's no more
duplicate log (a later commit also unifies logging code paths)
This allows the clock to be mockable in tests. Also, replace cs_main
with GetMutex() while touching this function.
Also, use the ElapseSteady test helper in the p2p_headers_presync fuzz
target to make it more deterministic.
The m_last_presync_update variable is a global that is not reset in
ResetAndInitialize. However, it is only used for logging, so completely
disable it for now.
Without this patch, the tool would report a diff:
cargo run --manifest-path ./contrib/devtools/deterministic-fuzz-coverage/Cargo.toml -- $PWD/bld-cmake/ $PWD/../qa-assets/fuzz_corpora/ p2p_headers_presync 32
...
4468| 81| auto now = std::chrono::steady_clock::now();
4469| 81| if (now < m_last_presync_update + std::chrono::milliseconds{250}) return;
- ^80
+ ^79
...
2a92702baf init: Use size_t consistently for cache sizes (TheCharlatan)
65cde3621d kernel: Move default cache constants to caches (TheCharlatan)
8826cae285 kernel: Move non-kernel db cache size constants (TheCharlatan)
e758b26b85 kernel: Move kernel-specific cache size options to kernel (TheCharlatan)
d5e2c4a409 fuzz: Add fuzz test for checked and saturating add and left shift (TheCharlatan)
c03a2795a8 util: Add integer left shift helpers (TheCharlatan)
8bd5f8a38c [refactor] init: Simplify coinsdb cache calculation (TheCharlatan)
5db7d4d3d2 doc: Correct docstring describing max block tree db cache (TheCharlatan)
Pull request description:
Carrying non-kernel related fields in the cache sizes for the indexes is confusing for kernel library users. The cache sizes are set currently with magic numbers in bitcoin-chainstate. The comments for the cache size calculations are not completely clear. The constants for the cache sizes are also currently in `txdb.h`, which is not an ideal place for holding all cache size related constants.
Solve these things by moving the kernel-specific cache size fields to their own struct and moving the constants to either the node or the kernel cache sizes.
This slightly changes the way the cache is allocated if (and only if) the txindex and/or blockfilterindex is used. Since they are now given precedence over the block tree db cache, this results in a bit less cache being allocated to the block tree db, coinsdb and coins caches. The effect is negligible though, i.e. cache sizes with default dbcache reported through the logs are:
master:
```
Cache configuration:
* Using 2.0 MiB for block index database
* Using 56.0 MiB for transaction index database
* Using 49.0 MiB for basic block filter index database
* Using 8.0 MiB for chain state database
* Using 335.0 MiB for in-memory UTXO set (plus up to 286.1 MiB of unused mempool space)
```
this PR:
```
Cache configuration:
* Using 2.0 MiB for block index database
* Using 56.2 MiB for transaction index database
* Using 49.2 MiB for basic block filter index database
* Using 8.0 MiB for chain state database
* Using 334.5 MiB for in-memory UTXO set (plus up to 286.1 MiB of unused mempool space)
```
---
This PR is part of the [libbitcoinkernel project](https://github.com/bitcoin/bitcoin/issues/27587).
ACKs for top commit:
stickies-v:
re-ACK 2a92702baf
ryanofsky:
Code review ACK 2a92702baf. Changes since last review are fixing size options to use size_t instead of int64_t again, simplifying CheckedLeftShift more, and making other minor suggested cleanups
hodlinator:
re-ACK 2a92702baf
Tree-SHA512: 98376eaa0660b1b8c096a5ce1f3e7c8c30e7cd6644de36856c2d3e573108cfc9473c93ebb3952b7881047b5ae6c85c5b096e6726f30f35be58b98eca07c8c785
This avoids having to rely on implicit casts when passing them to the
various functions allocating the caches.
This also ensures that if the requested amount of db_cache does not fit
in a size_t, it is clamped to the maximum value of a size_t.
Also take this opportunity to make the total amounts of cache in the
chainstate manager a size_t too.
8f85d36d68 refactor: Clamp worker threads in ChainstateManager constructor (TheCharlatan)
Pull request description:
This ensures the options are applied consistently from contexts where they might not pass through the args manager, such as in some tests, or when used through the kernel library.
This is similar to the patch applied in 09ef322acc, used to make applying the mempool options consistent.
---
This is part of the libbitcoinkernel project https://github.com/bitcoin/bitcoin/issues/27587
ACKs for top commit:
maflcko:
ACK 8f85d36d68 🛳
achow101:
ACK 8f85d36d68
furszy:
Code ACK 8f85d36d68
stickies-v:
ACK 8f85d36d68
Tree-SHA512: 32d7cc177d6726ee9df62ac9eb43e49ba676f35bfcff47834bd97a1e33f2a9ea7be65d0a8a37be149de04e58c9c500ecef730e498f4e3909042324d3136160e9
The check type function now needs to return a std::optional<R> for some type R,
and the check queue overall will return std::nullopt if all individual checks
return that, or one of the non-nullopt values if there is at least one.
For most tests, we use R=int, but for the actual validation code, we make it return
the ScriptError.
This ensures the options are applied consistently from contexts where
they might not pass through the args manager, such as in some tests, or
when used through the kernel library.
This is similar to the patch applied in 09ef322acc.
0bd53d913c test: add test for getchaintips behavior with invalid chains (Martin Zumsande)
ccd98ea4c8 test: cleanup rpc_getchaintips.py (Martin Zumsande)
f5149ddb9b validation: mark blocks building on an invalid block as BLOCK_FAILED_CHILD (Martin Zumsande)
783cb7337f validation: call RecalculateBestHeader in InvalidChainFound (Martin Zumsande)
9275e9689a rpc: call RecalculateBestHeader as part of reconsiderblock (Martin Zumsande)
a51e91783a validation: add RecalculateBestHeader() function (Martin Zumsande)
Pull request description:
`m_best_header` (the most-work header not known to be on an invalid chain) can be wrong in the context of invalidation / reconsideration of blocks. This can happen naturally (a valid header is received and stored in our block tree db; when the full block arrives, it is found to be invalid) or triggered by the user with the `invalidateblock` / `reconsiderblock` rpc.
We don't currently use `m_best_header` for any critical things (see OP of #16974 for a list that still seems up-to-date), so it being wrong affects mostly rpcs.
This PR proposes to recalculate it if necessary by looping over the block index and finding the best header. It also suggest to mark headers between an invalidatetd block and the previous `m_best_header` as invalid, so they won't be considered in the recalculation.
It adds tests to `rpc_invalidateblock.py` and `rpc_getchaintips.py` that fail on master.
One alternative to this suggested in the past would be to introduce a continuous tracking of header tips (#12138).
While this might be more performant, it is also more complicated, and situations where we need this data are only be remotely triggerable by paying the cost of creating a valid PoW header for an invalid block.
Therefore I think it isn't necessary to optimise for performance here, plus the solution in this PR doesn't perform any extra steps in the normal node operation where no invalidated blocks are encountered.
Fixes #26245
ACKs for top commit:
fjahr:
reACK 0bd53d913c
achow101:
ACK 0bd53d913c
TheCharlatan:
Re-ACK 0bd53d913c
Tree-SHA512: 23c2fc42d7c7bb4f9b4ba4949646b3d0031dd29ed15484e436afd66cd821ed48e0f16a1d02f45477b5d0d73a006f6e81a56b82d9721e0dee2e924219f528b445
Without doing so, header-only chains building on a chain that
will be marked as invalid would still be eligible for m_best_header.
This improves both getblockchaininfo and getchaintips behavior.
While this adds an iteration over the entire block index, it can only be
triggered by the user (invalidateblock) or by others at a cost (the
header needs to be accepted in the first place, so it needs valid PoW).
Co-authored-by: TheCharlatan <seb.kung@gmail.com>
It recalculates m_best_header by looping over the entire
block index. Even though this is not very performant, it
will only be used in rare situations that cannot be
triggered by others without a cost:
As part of to invalidateblock / reconsiderblock rpcs, or when a
block with an accepted header with valid PoW turns out to be invalid
later during full validation.
The thread handle is never used by the ChainstateManager, so move it out
and into the node context. Users of the kernel library now no longer
have to manually join the thread when destructing the ChainstateManager.
Because AssumeUTXO nodes prioritize tip synchronization, they relay their local
address through the network before completing the background chain sync.
This, combined with the advertising of full-node service (NODE_NETWORK), can
result in an honest peer in IBD connecting to the AssumeUTXO node (while syncing)
and requesting an historical block the node does not have. This behavior leads to
an abrupt disconnection due to perceived unresponsiveness (lack of response)
from the AssumeUTXO node.
This lack of response occurs because nodes ignore getdata requests when they do
not have the block data available (further discussion can be found in PR 30385).
Fix this by refraining from signaling full-node service support while the
background chain is being synced. During this period, the node will only
signal 'NODE_NETWORK_LIMITED' support. Then, full-node ('NODE_NETWORK')
support will be re-enabled once the background chain sync is completed.
a2955f0979 validation: Use span for ImportBlocks paths (TheCharlatan)
20515ea3f5 validation: Use span for CalculateClaimedHeadersWork (TheCharlatan)
52575e96e7 validation: Use span for ProcessNewBlockHeaders (TheCharlatan)
Pull request description:
Makes it friendlier for potential future users of the kernel library if they do not store the headers in a std::vector, but can guarantee contiguous memory.
Take this opportunity to also change the argument of ImportBlocks previously taking a `std::vector` to a `std::span`.
ACKs for top commit:
stickies-v:
re-ACK a2955f0979 - no changes except further walking the ~file~ path of modernizing variable names.
maflcko:
ACK a2955f0979🕑
achow101:
ACK a2955f0979
danielabrozzoni:
ACK a2955f0979
Tree-SHA512: 8b07f4ad26e270b65600d1968cd78847b85caca5bfbb83fd9860389f26656b1d9a40b85e0990339f50403d18cedcd2456990054f3b8b0bedce943e50222d2709
Makes it friendlier for potential future users of the kernel library if
they do not store the headers in a std::vector, but can guarantee
contiguous memory.
Makes it friendlier for potential future users of the kernel library if
they do not store the headers in a std::vector, but can guarantee
contiguous memory.
The error messages should never happen in normal operation. However, if
they do, they are helpful to return to the user to debug the issue. For
example, to notice a truncated file.
In future, users of the kernel library might run multiple chainstates in
parallel, or create and destroy multiple chainstates over the lifetime
of a process. Having static, mutable variables could lead to state
inconsistencies in these scenarios.
In future, users of the kernel library might run multiple chainstates in
parallel, or create and destroy multiple chainstates over the lifetime
of a process. Having static, mutable variables could lead to state
inconsistencies in these scenarios.
606a7ab862 kernel: De-globalize signature cache (TheCharlatan)
66d74bfc45 Expose CSignatureCache class in header (TheCharlatan)
021d38822c kernel: De-globalize script execution cache hasher (TheCharlatan)
13a3661aba kernel: De-globalize script execution cache (TheCharlatan)
ab14d1d6a4 validation: Don't error if maxsigcachesize exceeds uint32::max (TheCharlatan)
Pull request description:
The validation caches are currently setup independently from where the rest of the validation code is initialized. This makes their ownership semantics unclear. There is also no clear enforcement on when and in what order they need to be initialized. The caches are always initialized in the `BasicTestingSetup` although a number of tests don't actually need them.
Solve this by moving the caches from global scope into the `ChainstateManager` class. This simplifies the usage of the kernel library by no longer requiring manual setup of the caches prior to using the `ChainstateManager`. Tests that need to access the caches can instantiate them independently.
---
This pull request is part of the [libbitcoinkernel project](https://github.com/bitcoin/bitcoin/issues/27587).
ACKs for top commit:
stickies-v:
re-ACK 606a7ab862
glozow:
reACK 606a7ab
ryanofsky:
Code review ACK 606a7ab862. Just small formatting, include, and static_assert changes since last review.
Tree-SHA512: e7f3ee41406e3b233832bb67dc3a63c4203b5367e5daeed383df9cb590f227fcc62eae31311029c077d5e81b273a37a88a364db3dee2efe91bb3b9c9ddc8a42e
Move its ownership to the ChainstateManager class.
Next to simplifying usage of the kernel library by no longer requiring
manual setup of the cache prior to using validation code, it also slims
down the amount of memory allocated by BasicTestingSetup.
Use this opportunity to make SignatureCache RAII styled
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
