refactor: inline CCoinsViewErrorCatcher into CCoinsViewDB
#34132
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34132. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
597d2dd to
53de82d
Compare
53de82d to
0973a03
Compare
|
close/open dance to retrigger CI - likely caused by GitHub outage |
andrewtoth
left a comment
There was a problem hiding this comment.
Concept ACK
I think getting rid of this indirection is great. It reduces a lot of complexity when thinking about the stack of views.
When reviewing, I think the last 3 commits should be combined into 1. There doesn't seem to be much value in creating and then removing functions in multiple commits.
I would utACK but I wanted clarity on why we have to skip a test when no IPC.
|
Concept ACK |
0973a03 to
dceb671
Compare
|
Rebased and updated the added test to work on 32 bit systems as well. Ready for review again! |
dceb671 to
4ae1f79
Compare
d306374 to
36a01d0
Compare
The error was also reworded to avoid the leading repetition (see expected stderr).
### Reproducers
The test failure can be triggered manually by:
```patch
std::optional<Coin> CCoinsViewErrorCatcher::GetCoin(const COutPoint& outpoint) const
{
- return ExecuteBackedWrapper<std::optional<Coin>>([&]() { return CCoinsViewBacked::GetCoin(outpoint); }, m_err_callbacks);
+ return CCoinsViewBacked::GetCoin(outpoint);
}
```
### Notes
Note that CCoinsViewErrorCatcher::HaveCoin failure isn't tested since it doesn't seem to be used in production code, all tests pass with:
```patch
bool CCoinsViewErrorCatcher::HaveCoin(const COutPoint& outpoint) const
{
- return ExecuteBackedWrapper<bool>([&]() { return CCoinsViewBacked::HaveCoin(outpoint); }, m_err_callbacks);
+ throw "CCoinsViewBacked::HaveCoin";
}
```
### Context:
On 64-bit systems, LevelDB uses mmap which reflects file changes immediately.
On 32-bit systems (i686, armhf), mmap is disabled (kDefaultMmapLimit=0 in leveldb/util/env_posix.cc), so LevelDB uses block cache which serves stale data.
The node must be restarted after corruption to clear the cache.
Corruption is placed at the middle to avoid LevelDB's paranoid_checks verification during database open, and -checkblocks=0 -checklevel=0 skips Bitcoin's block verification.
Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
Inlined `ExecuteBackedWrapper`, removing a lambda indirection and specializing the error logs per usage. The error handling will be pushed down to the call sites in follow ups.
There's only ever a single callback at most, let's assign it instead of pretending we have a list of callbacks. This will be moved to the constructor in the next commit.
36a01d0 to
0d0743d
Compare
| int64_t check_blocks{DEFAULT_CHECKBLOCKS}; | ||
| int64_t check_level{DEFAULT_CHECKLEVEL}; | ||
| std::function<void()> coins_error_cb; | ||
| std::function<void()> read_error_cb{[]{}}; |
There was a problem hiding this comment.
e77ef5c: I don't think this is a rename. You change nullptr to an object?! When changing behavior, you should mention why the behavior is changed, instead of omitting the change.
| // Starting the shutdown sequence and returning false to the caller would be | ||
| // interpreted as 'entry not found' (as opposed to unable to read data), and | ||
| // could lead to invalid interpretation. Just exit immediately, as we can't | ||
| // continue anyway, and all writes should be atomic. |
There was a problem hiding this comment.
029224b: I don't think this is an "inline". You are removing this comment without even mentioning it. When removing code, you should mention why it is not needed.
There was a problem hiding this comment.
I think the ExecuteBackedWrapper can be kept. It seems applicable if it is moved to txdb.cpp as well?
Problem
The current coins database read-error handling adds unnecessary complexity to the critical path:
CCoinsViewErrorCatchersits between the cache and database views, wrapping every read operation with a generic error handlerExecuteBackedWrapperadds another function call layer with lambda captures on each database readstd::vector<std::function<void()>>for callbacks when there's only ever one callback registeredcs_mainlock)This adds unnecessary stack depth and bookkeeping overhead on every coins database read operation (
GetCoin). Also note thatCCoinsViewDB::HaveCoinisn't even called from production code as far as I can tell.Context
This is part of ongoing coins caching cleanup efforts like #34124, #33866, #33018, and #33512.
Fix
This PR collapses the
CCoinsViewErrorCatcherlayer entirely and moves error handling directly intoCCoinsViewDB:ExecuteBackedWrapperandCCoinsViewErrorCatcher, placing try-catch blocks directly inCCoinsViewDB::GetCoinandCCoinsViewDB::HaveCoinstd::vector<std::function<void()>>with a singlestd::function<void()>member, initialized in the constructorChainstate::InitCoinsDBand forwarded toCCoinsViewDBconstructorA new test is also added to reproduce the behavior in case of database corruption. It's added as a first commit to demonstrate that the behavior stays unchanged.