to_mark_failed was useful to track last disconnected block so that
it's children can be marked as BLOCK_FAILED_CHILD. since the
previous commit removes BLOCK_FAILED_CHILD usage in InvalidateBlock,
the existing variable invalid_walk_tip is sufficient.
Improve upon the variable name for `invalid_walk_tip` to make the
InvalidateBlock logic easier to read. Block tip before disconnection
is now tracked directly via `disconnected_tip`, and `new_tip`
is the tip after the disconnect.
Co-authored-by: stickies-v <stickies-v@protonmail.com>
- there is no functional difference between BLOCK_FAILED_VALID and BLOCK_FAILED_CHILD
and it's unnecessary code complexity to correctly categorise them.
8cc3ac6c23 validation: Don't use IsValid() to filter for invalid blocks (Martin Zumsande)
86d98b94e5 test: verify that ancestors of a reconsidered block can become the chain tip (stratospher)
3c39a55e64 validation: Add ancestors of reconsiderblock to setBlockIndexCandidates (Martin Zumsande)
Pull request description:
When we call `reconsiderblock` for some block, `Chainstate::ResetBlockFailureFlags` puts the descendants of that block into `setBlockIndexCandidates` (if they meet the criteria, i.e. have more work than the tip etc.), but never put any ancestors into the set even though we do clear their failure flags.
I think that this is wrong, because `setBlockIndexCandidates` should always contain all eligible indexes that have at least as much work as the current tip, which can include ancestors of the reconsidered block. This is being checked by `CheckBlockIndex()`, which could fail if it was invoked after `ActivateBestChain` connects a block and releases `cs_main`:
``` diff
diff --git a/src/validation.cpp b/src/validation.cpp
index 7b04bd9a5b..ff0c3c9f58 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -3551,6 +3551,7 @@ bool Chainstate::ActivateBestChain(BlockValidationState& state, std::shared_ptr<
}
}
// When we reach this point, we switched to a new tip (stored in pindexNewTip).
+ m_chainman.CheckBlockIndex();
if (exited_ibd) {
// If a background chainstate is in use, we may need to rebalance our
```
makes `rpc_invalidateblock.py` fail on master.
Even though we don't currently have a `CheckBlockIndex()` in that place, after `cs_main` is released other threads could invoke it, which is happening in the rare failures of #16444 where an invalid header received from another peer could trigger a `CheckBlockIndex()` call that would fail.
Fix this by adding eligible ancestors to `setBlockIndexCandidates` in `Chainstate::ResetBlockFailureFlags` (also simplifying that function a bit).
Fixes#16444
ACKs for top commit:
achow101:
ACK 8cc3ac6c23
TheCharlatan:
Re-ACK 8cc3ac6c23
stratospher:
reACK 8cc3ac6.
Tree-SHA512: 53f27591916246be4093d64b86a0494e55094abd8c586026b1247e4a36747bc3d6dbe46dc26ee4a22f47b8eb0d9699d13e577dee0e7198145f3c9b11ab2a30b7
To mitigate disk-filling attacks caused by unsafe usages of LogPrintf and
friends, we rate-limit them by passing a should_ratelimit bool that
eventually makes its way to LogPrintStr which may call
LogRateLimiter::Consume. The rate limiting is accomplished by
adding a LogRateLimiter member to BCLog::Logger which tracks source
code locations for the given logging window.
Every hour, a source location can log up to 1MiB of data. Source
locations that exceed the limit will have their logs suppressed for the
rest of the window determined by m_limiter.
This change affects the public LogPrintLevel function if called with
a level >= BCLog::Level::Info.
The UpdateTipLog function has been changed to use the private LogPrintLevel_
macro with should_ratelimit set to false. This allows UpdateTipLog to log
during IBD without hitting the rate limit.
Note that on restart, a source location that was rate limited before the
restart will be able to log until it hits the rate limit again.
Co-Authored-By: Niklas Gogge <n.goeggi@gmail.com>
Co-Authored-By: stickies-v <stickies-v@protonmail.com>
9341b5333a blockstorage: make block read hash checks explicit (Lőrinc)
2371b9f4ee test/bench: verify hash in `ComputeFilter` reads (Lőrinc)
5d235d50d6 net: assert block hash in `ProcessGetBlockData` and `ProcessMessage` (Lőrinc)
Pull request description:
A follow-up to https://github.com/bitcoin/bitcoin/pull/32487#discussion_r2094072165, after which validating the hash of a read block from disk doesn't incur the cost of calculating its hash anymore.
### Summary
This PR adds explicit checks that the read block header's hash matches the one we were expecting.
### Context
After the previous PR, validating a block's hash during read operations became essentially free. This PR leverages that by requiring callers to provide a block's expected hash (or `std::nullopt`), preventing silent failures caused by corrupted or mismatched data. Most `ReadBlock` usages were updated with expected hashes and now fail on mismatch.
### Changes
* added hash assertions in `ProcessGetBlockData` and `ProcessMessage` to validate that the block read from disk matches the expected hash;
* updated tests and benchmark to pass the correct block hash to `ReadBlock()`, ensuring the hash validation is tested - or none if we already expect PoW failure;
* removed the default value for `expected_hash`, requiring an explicit hash for all block reads.
### Why is the hash still optional (but no longer has a default value)
* for header-error tests, where the goal is to trigger failures early in the parsing process;
* for out-of-order orphan blocks, where the child hash isn't available before the initial disk read.
ACKs for top commit:
maflcko:
review ACK 9341b5333a🕙
achow101:
ACK 9341b5333a
hodlinator:
ACK 9341b5333a
janb84:
re ACK 9341b5333a
Tree-SHA512: cf1d4fff4c15e3f8898ec284929cb83d7e747125d4ee759e77d369f1716728e843ef98030be32c8d608956a96ae2fbefa0e801200c333b9eefd6c086ec032e1f
a18e572328 test: more template verification tests (Sjors Provoost)
10c908808f test: move gbt proposal mode tests to new file (Sjors Provoost)
94959b8dee Add checkBlock to Mining interface (Sjors Provoost)
6077157531 ipc: drop BlockValidationState special handling (Sjors Provoost)
74690f4ed8 validation: refactor TestBlockValidity (Sjors Provoost)
Pull request description:
This PR adds the IPC equivalent of the `getblocktemplate` RPC in `proposal` mode.
In order to do so it has `TestBlockValidity` return error reasons as a string instead of `BlockValidationState`. This avoids complexity in IPC code for handling the latter struct.
The new Mining interface method is used in `miner_tests`.
It's not used by the `getblocktemplate` and `generateblock` RPC calls, see https://github.com/bitcoin/bitcoin/pull/31981#discussion_r2096473337
The `inconclusive-not-best-prevblk` check is moved from RPC
code to `TestBlockValidity`.
Test coverage is increased by `mining_template_verification.py`.
Superseedes #31564
## Background
### Verifying block templates (no PoW)
Stratum v2 allows miners to generate their own block template. Pools may wish (or need) to verify these templates. This typically involves comparing mempools, asking miners to providing missing transactions and then reconstructing the proposed block.[^0] This is not sufficient to ensure a proposed block is actually valid. In some schemes miners could take advantage of incomplete validation[^1].
The Stratum Reference Implementation (SRI), currently the only Stratum v2 implementation, collects all missing mempool transactions, but does not yet fully verify the block.[^2]. It could use the `getblocktemplate` RPC in `proposal` mode, but using IPC is more performant, as it avoids serialising up to 4 MB of transaction data as JSON.
(although SRI could use this PR, the Template Provider role doesn't need it, so this is _not_ part of #31098)
[^0]: https://github.com/stratum-mining/sv2-spec/blob/main/06-Job-Declaration-Protocol.md
[^1]: https://delvingbitcoin.org/t/pplns-with-job-declaration/1099/45?u=sjors
[^2]: https://github.com/stratum-mining/stratum/blob/v1.1.0/roles/jd-server/src/lib/job_declarator/message_handler.rs#L196
ACKs for top commit:
davidgumberg:
reACK a18e572328
achow101:
ACK a18e572328
TheCharlatan:
ACK a18e572328
ryanofsky:
Code review ACK a18e572328 just adding another NONFATAL_UNREACHABLE since last review
Tree-SHA512: 1a6c29f45a1666114f10f55aed155980b90104db27761c78aada4727ce3129e6ae7a522d90a56314bd767bd7944dfa46e85fb9f714370fc83e6a585be7b044f1
Comments are expanded.
Return BlockValidationState instead of passing a reference.
Lock Chainman mutex instead of cs_main.
Remove redundant chainparams and pindexPrev arguments.
Drop defaults for checking proof-of-work and merkle root.
The ContextualCheckBlockHeader check is moved to after CheckBlock,
which is more similar to normal validation where context-free checks
are done first.
Validation failure reasons are no longer printed through LogError(),
since it depends on the caller whether this implies an actual bug
in the node, or an externally sourced block that happens to be invalid.
When called from getblocktemplate, via BlockAssembler::CreateNewBlock(),
this method already throws an std::runtime_error if validation fails.
Additionally it moves the inconclusive-not-best-prevblk check from RPC
code to TestBlockValidity.
There is no behavior change when callling getblocktemplate with proposal.
Previously this would return a BIP22ValidationResult which can throw for
state.IsError(). But CheckBlock() and the functions it calls only use
state.IsValid().
The final assert is changed into Assume, with a LogError.
Co-authored-by: <Ryan Ofsky <ryan@ofsky.org>
Dropped the default expected_hash parameter from `ReadBlock()`.
In `blockmanager_flush_block_file` tests, we pass {} since the tests would already fail at PoW validation for corrupted blocks.
In `ChainstateManager::LoadExternalBlockFile`, we pass {} when processing child blocks because their hashes aren't known beforehand.
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
IsValid() also returns false for blocks that have not been
validated yet up to the default validity level of BLOCK_VALID_TRANSACTIONS but
are not marked as invalid - e.g. if we only know the header.
Here, we specifically want to filter for invalid blocks.
Also removes the default arg from IsValid() which is now unused outside
of tests, to prevent this kind of misuse for the future.
Co-authored-by: TheCharlatan <seb.kung@gmail.com>
When we call reconsiderblock for some block, ResetBlockFailureFlags puts the descendants of that block
into setBlockIndexCandidates (if they meet the criteria, i.e. have more work than the tip etc.)
We also clear the failure flags of the ancestors, but we never put any of those into setBlockIndexCandidates
this is wrong and could lead to failures in CheckBlockIndex().
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 adds checks that
1) Descendants of invalid block indexes are also marked invalid
2) m_best_header cannot be invalid, and there can be no valid
block with more work than it.
Before, m_best_header would be calculated only after disconnecting
multiple blocks, letting go of cs_main in the meantime.
This is in preparation for adding checks to CheckBlockIndex()
requiring that m_best_header is the most-work header not known to be invalid.
Co-authored-by: stringintech <stringintech@gmail.com>
Before, they would be marked as invalid only after disconnecting
multiple blocks, letting go of cs_main in the meantime.
This is in preparation for adding a check to
CheckBlockIndex() requiring that descendants of invalid block indexes
are always marked as invalid.
Entries from highpow_outofchain_headers are now only removed if made invalid,
no longer after inserting into setBlockIndexCandidates, because they
might still become invalid later in the second case.
This means that blocks could be inserted multiple times now into
setBlockIndexCandidates - this won't have any effect, so
behavior isn't changed.
We now include blocks without HaveNumChainTxs() / lower validation status
than BLOCK_VALID_TRANSACTIONS. These checks are still performed at the
spot where we use the cache to insert into setBlockIndexCandidates.
This is in preparation for using the cache for more things than
just setBlockIndexCandidates candidates in the following commits.
Co-authored-by: stickies-v <stickies-v@protonmail.com>
When a block it found invalid during acceptance (but before connection)
we now mark its descendants with BLOCK_FAILED_CHILD and update
m_best_header when these things weren't done reliably before.
This does not fix a serious bug because the flags and m_best_header were being set on a best-effort basis before
and not used for anything critical.
Setting these reliably has a slight performance cost (iterating over the
entire block index) but leads to more consistency in validation and allows removing m_failed_blocks in a later commit.
This can only be triggered by providing a block with sufficient PoW
that is otherwise invalid, so it is not a DoS vector.
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
fd290730f5 validation: clean up and clarify CheckInputScripts logic (Cory Fields)
1a37507895 validation: use a lock for CCheckQueueControl (Cory Fields)
c3b0e6c7f4 validation: make CCheckQueueControl's CCheckQueue non-optional (Cory Fields)
4c8c90b556 validation: only create a CCheckQueueControl if it's actually going to be used (Cory Fields)
11fed833b3 threading: add LOCK_ARGS macro (Cory Fields)
Pull request description:
As part of an effort to cleanup our threading primitives and add safe `SharedMutex`/`SharedLock` impls, I'd like to get rid of the last of our legacy `ENTER_CRITICAL_SECTION`/`LEAVE_CRITICAL_SECTION` usage. This, along with a follow-up [after fixing REVERSE_LOCK](https://github.com/bitcoin/bitcoin/pull/32465) will allow us to do that.
This replaces the old macros with an RAII lock, while simplifying `CCheckQueueControl`. It now requires a `CCheckQueue`, and optionality is handled externally. In the case of validation, it is wrapped in a `std::optional`.
It also adds an `LOCK_ARGS` macro for `UniqueLock` initialization which may be helpful elsewhere.
ACKs for top commit:
fjahr:
re-ACK fd290730f5
ryanofsky:
Code review ACK fd290730f5, just removing assert since last review. Thanks for considering all the comments and feedback!
TheCharlatan:
Re-ACK fd290730f5
Tree-SHA512: 54b9db604ee1bda7d11bce1653a88d3dcbc4f525eed6a85abdd4d6409138674af4bb8b00afa4e0d3d29dadd045a3a39de253a45f0ef9c05f56cba1aac5b59303
CheckInputScripts behaves differently depending on whether or not it was called
with a vector for checks. Make this difference clear by calling it differently
depending on whether or not control exists. Though more verbose, it should be
more straightforward to understand what's happening this way.
Also remove parallel_script_checks, as `if(control)` is a better test.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
a04f17a188 doc: warn that CheckBlock() underestimates sigops (Sjors Provoost)
Pull request description:
Counting sigops in the witness requires context that `CheckBlock()` does not have, so it only counts sigops for non-segwit transactions.
It's useful to document, but it should not be a problem.
The commit message contains some historical context.
ACKs for top commit:
ismaelsadeeq:
ACK a04f17a188
ryanofsky:
Code review ACK a04f17a188
Tree-SHA512: 26528367a7f3cfa8540ef0b90f7aa912c8f0bc057428f20a1fd1d4e232dac77747bc20044f0fcb0ffab8a2e1fb3dbe3dab46be749553a917744ddc7a829025cb
Counting sigops in the witness and for p2sh requires
context that CheckBlock() does not have, so it only
counts a subset of sigops.
The check here was introduced by Satoshi as a "cleanup" in
f1e1fb4bde. With the attempted
introduction of OP_EVAL, it was replaced by the check in
ConnectBlock(). Commit e679ec969c
marked this code as a placeholder for backward compatibility.
Then when P2SH replaced OP_EVAL in 922e8e2929
the phrase "compatibility-breaking" was replaced by a simple
observation that before v0.6 this is how sigops were counted.
It's unclear why the check was kept and there were no review comments
about it.
GetAll() is non-const, preventing CheckBlockIndex() from being
const. Rather than add a const GetAll() method, just iterate over
the chainstates directly.
Slight behaviour change by also subjecting non-`IsUsable()`
chainstates to consistency checks.
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
The `fsbridge::get_filesystem_error_message()` function exhibits several
drawbacks:
1. It was introduced in https://github.com/bitcoin/bitcoin/pull/14192 to
account for platform-specific variations in
`boost::filesystem::filesystem_error::what()`. Since migrating to
`std::filesystem`, those discrepancies no longer exist.
2. It fails to display UTF-8 paths correctly on Windows.
3. It relies on `std::wstring_convert`, which was deprecated in C++17
and removed in C++26.
This change removes the `fsbridge::get_filesystem_error_message()`
function, thereby resolving all of the above issues.
Additionally, filesystem error messages now use the "Warning" log level.
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
3c3548a70e validation: clarify final |= BLOCK_FAILED_VALID in InvalidateBlock (Matt Corallo)
aac5488909 validation: correctly update BlockStatus for invalid block descendants (stratospher)
9e29653b42 test: check BlockStatus when InvalidateBlock is used (stratospher)
c99667583d validation: fix traversal condition to mark BLOCK_FAILED_CHILD (stratospher)
Pull request description:
This PR addresses 3 issues related to how `BLOCK_FAILED_CHILD` is set:
1. In `InvalidateBlock()`
- Previously, `BLOCK_FAILED_CHILD` was not being set when it should have been.
- This was due to an incorrect traversal condition, which is fixed in this PR.
2. In `SetBlockFailure()`
- `BLOCK_FAILED_VALID` is now cleared before setting `BLOCK_FAILED_CHILD`.
3. In `InvalidateBlock()`
- if block is already marked as `BLOCK_FAILED_CHILD`, don't mark it as `BLOCK_FAILED_VALID` again.
Also adds a unit test to check `BLOCK_FAILED_VALID` and `BLOCK_FAILED_CHILD` status in `InvalidateBlock()`.
<details>
<summary><h3>looking for feedback on an alternate approach</h3></summary>
<br>
An alternate approach could be removing `BLOCK_FAILED_CHILD` since even though we have a distinction between
`BLOCK_FAILED_VALID` and `BLOCK_FAILED_CHILD` in the codebase, we don't use it for anything. Whenever we check for BlockStatus, we use `BLOCK_FAILED_MASK` which encompasses both of them. See similar discussion in https://github.com/bitcoin/bitcoin/pull/16856.
I have a branch with this approach in https://github.com/stratospher/bitcoin/commits/2025_02_remove_block_failed_child/.
Compared to the version in #16856, it also resets `BLOCK_FAILED_CHILD` already on disk to `BLOCK_FAILED_VALID` when loading from disk so that we won't be in a dirty state in a no-`BLOCK_FAILED_CHILD`-world.
I'm not sure if it's a good idea to remove `BLOCK_FAILED_CHILD` though. would be curious to hear what others think of this approach.
thanks @ mzumsande for helpful discussion regarding this PR!
</details>
ACKs for top commit:
achow101:
ACK 3c3548a70e
TheCharlatan:
Re-ACK 3c3548a70e
mzumsande:
re-ACK 3c3548a70e
Tree-SHA512: 83e0d29dea95b97519d4868135c965b86f6f43be50b15c0bd8f998b3476388fc7cc22b49c0c54ec532ae8222e57dfc436438f0c8e98f54757b384f220488b6a6
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
...
This has no functional affect, as the any CBlockIndex*s which
to_mark_failed is set to will already have been marked failed.
Also prevents a situation where block already marked as
BLOCK_FAILED_CHILD is again unconditionally marked as
BLOCK_FAILED_VALID in the final |= BLOCK_FAILED_VALID.
invalid_block ----------> block_index
- before this commit, only if block_index is not invalid, it will mark
block_index as BLOCK_FAILED_CHILD
- it's possible that block_index encountered is invalid and was marked
as BLOCK_FAILED_VALID previously
- in this case, correctly update BlockStatus of block_index by
clearing BLOCK_FAILED_VALID and then setting it to BLOCK_FAILED_CHILD
this block of code is not reached on master since other than
initialisation, all other iterations have invalid_walk_tip
and to_mark_failed pointers in some form of this layout
where 1, 2, 3 and 4 are block heights.
invalid_walk_tip
↓
1 <- 2 <- 3 <- 4
↑
to_mark_failed
fix it so that blocks are correctly marked as BLOCK_FAILED_CHILD
if it's a descendant of BLOCK_FAILED_VALID block.
eb0724f0de doc: banman: reference past vuln due to unbounded banlist (Antoine Poinsot)
ad616b6c01 doc: net: mention past vulnerability as rationale to limit incoming message size (Antoine Poinsot)
4489117c3f doc: txrequest: point to past censorship vulnerability in tx re-request handling (Antoine Poinsot)
68ac9542c4 doc: net_proc: reference past DoS vulnerability in orphan processing (Antoine Poinsot)
c02d9f6dd5 doc: net_proc: reference past defect regarding invalid GETDATA types (Antoine Poinsot)
5e3d9f21df doc: validation: add a reference to historical header spam vulnerability (Antoine Poinsot)
Pull request description:
It is useful when reading code to have context about why it is written or behaves the way it does. Some instances in this PR may seem obvious but i think nonetheless offer important context to anyone willing to change (or review a change to) this code.
ACKs for top commit:
ryanofsky:
Code review ACK eb0724f0de. No changes since last review other than rebase
Tree-SHA512: 271902f45b8130d44153d793bc1096cd22b6ce05494e67c665a5bc45754e3fc72573d303ec8fc7db4098d473760282ddbf0c1cf316947539501dfd8d7d5b8828
stratospher
Ava Chow
Eugene Siegel
Sjors Provoost
Lőrinc
Martin Zumsande
Ryan Ofsky
fanquake
MarcoFalke
Cory Fields
stickies-v
Andrew Toth
Hennadii Stepanov
Matt Corallo