- don't log function name
- take into account that GetName() always ends with " index"
- replace deprecated LogPrintf with LogInfo
- remove trailing \n
- adjusted log level where needed
There is no way to report a close error from `AutoFile` destructor.
Such an error could be serious if the file has been written to because
it may mean the file is now corrupted (same as if write fails).
So, change all users of `AutoFile` that use it to write data to
explicitly close the file and handle a possible error.
Moved CBlockUndo disk read lookups from child index classes to
the base index class.
The goal is for child index classes to synchronize only through
events, without directly accessing the chain database.
This change will enable future parallel synchronization mechanisms,
reduce database access (when batched), and contribute toward the
goal of running indexes in a separate process (with no chain
database access).
Besides that, this commit also documents how NextSyncBlock() behaves.
It is not immediately clear this function could return the first
block after the fork point during a reorg.
Move ReadBlock code from CoinStatsIndex::CustomRewind to BaseIndex::Rewind
Move ReadUndo code from CoinStatsIndex::ReverseBlock to BaseIndex::Rewind
This commit does change behavior slightly. Since the new CustomRemove
methods only take a single block at a time instead of a range of
disconnected blocks, when they call CopyHeightIndexToHashIndex they will
now do an index seek for each removed block instead of only seeking once
to the height of the earliest removed block. Seeking instead of scanning
is a little worse for performance if there is a >1 block reorg, but
probably not noticeable unless the reorg is very deep.
This fixes the log output when -logsourcelocations is used.
Also, instead of 'ERROR:', the log will now say '[error]', like other
errors logged with LogError.
-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended 's! error\("([^"]+)"! LogError("\1\\n"!g' $( git grep -l ' error(' ./src/ )
-END VERIFY SCRIPT-
This is needed for the next commit.
-BEGIN VERIFY SCRIPT-
# Separate sed invocations to replace one-line, and two-line error(...) calls
sed -i --regexp-extended 's!( +)return (error\(.*\);)!\1\2\n\1return false;!g' $( git grep -l 'return error(' )
sed -i --null-data --regexp-extended 's!( +)return (error\([^\n]*\n[^\n]*\);)!\1\2\n\1return false;!g' $( git grep -l 'return error(' )
-END VERIFY SCRIPT-
c0bf667912 index: add [nodiscard] attribute to functions writing to the db (furszy)
eef595560e index: coinstats reorg, fail when block cannot be reversed (furszy)
Pull request description:
Found it while reviewing https://github.com/bitcoin/bitcoin/pull/24230#discussion_r1310863359.
During a reorg, continuing execution when a block cannot be reversed leaves the
coinstats index in an inconsistent state.
This was surely overlooked when 'CustomRewind' was implemented.
ACKs for top commit:
ryanofsky:
Code review ACK c0bf667912. Only change since last review is new commit adding [[nodiscard]]
Tree-SHA512: f4fc8522508d23e4fff09a29c935971819b1bd3b2a260e08e2e2b72f9340980d74fbec742a58fe216baf61d27de057c7c8300e8fa075f8507cd1227f128af909
This removes unused includes, primitives/block found manually, and the
others by iwyu:
blockfilter.h should remove these lines:
- #include <serialize.h> // lines 16-16
- #include <undo.h> // lines 18-18
This is a commit in preparation for the next few commits. The functions
are moved to methods to avoid their re-declaration for the purpose of
passing in BlockManager options.
The functions that were now moved into the BlockManager should no longer
use the params as an argument, but instead use the member variable.
In the moved ReadBlockFromDisk and UndoReadFromDisk, change
the function signature to accept a reference to a CBlockIndex instead of
a raw pointer. The pointer is expected to be non-null, so reflect that
in the type.
To allow for the move of functions to BlockManager methods all call
sites require an instantiated BlockManager, or a callback to one.
This is an extraction of ArgsManager related functions from util/system
into their own common file.
Config file related functions are moved to common/config.cpp.
The background of this commit is an ongoing effort to decouple the
libbitcoinkernel library from the ArgsManager. The ArgsManager belongs
into the common library, since the kernel library should not depend on
it. See doc/design/libraries.md for more information on this rationale.
This is an extraction of filesystem related functions from util/system
into their own utility file.
The background of this commit is an ongoing effort to decouple the
libbitcoinkernel library from the ArgsManager defined in system.h.
Moving these functions out of system.h allows including them from a
separate source file without including the ArgsManager definitions from
system.h.
facc2fa7b8 Use AutoFile where possible (MacroFake)
6666803c89 streams: Add AutoFile without ser-type and ser-version (MacroFake)
Pull request description:
This was done in the context of https://github.com/bitcoin/bitcoin/pull/25284 , but I think it also makes sense standalone.
The basic idea is that serialization type should not be initialized when it is not needed. Same for the serialization version.
So do this here for `AutoFile`. `CAutoFile` remains in places where it is not yet possible.
ACKs for top commit:
laanwj:
Code review ACK facc2fa7b8
fanquake:
ACK facc2fa7b8
Tree-SHA512: d82d024d55af57565ac53d9d1517afafc12b46964effba0332de62a6c77869356fa77f89e6d4834438fff44c45b64fccdf5a1358bfea03e28dfe55013b3c099d
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
https://github.com/bitcoin/bitcoin/pull/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 Rewind method with CustomRewind and pass block hashes and
heights instead of CBlockIndex* pointers
This commit does not change behavior in any way.
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.
e734228d85 Update GCSFilter benchmarks (Calvin Kim)
aee9a8140b Add GCSFilterDecodeSkipCheck benchmark (Patrick Strateman)
299023c1d9 Add GCSFilterDecode and GCSBlockFilterGetHash benchmarks. (Patrick Strateman)
b0a53d50d9 Make sanity check in GCSFilter constructor optional (Patrick Strateman)
Pull request description:
This PR picks up the abandoned #19280
BlockFilterIndex was depending on `GolombRiceDecode()` during the filter decode to sanity check that the filter wasn't corrupt. However, we can check for corruption by ensuring that the encoded blockfilter's hash matches up with the one stored in the index database.
Benchmarks that were added in #19280 showed that checking the hash is much faster.
The benchmarks were changed to nanobench and the relevant benchmarks were like below, showing a clear win for the hash check method.
```
| ns/elem | elem/s | err% | ins/elem | bra/elem | miss% | total | benchmark
|--------------------:|--------------------:|--------:|----------------:|---------------:|--------:|----------:|:----------
| 531.40 | 1,881,819.43 | 0.3% | 3,527.01 | 411.00 | 0.2% | 0.01 | `DecodeCheckedGCSFilter`
| 258,220.50 | 3,872.66 | 0.1% | 2,990,092.00 | 586,706.00 | 1.7% | 0.01 | `DecodeGCSFilter`
| 13,036.77 | 76,706.09 | 0.3% | 64,238.24 | 513.04 | 0.2% | 0.01 | `BlockFilterGetHash`
```
ACKs for top commit:
mzumsande:
Code Review ACK e734228d85
theStack:
Code-review ACK e734228d85
stickies-v:
ACK e734228d85
ryanofsky:
Code review ACK e734228d85, with caveat that I mostly paid attention to the main code, not the changes to the benchmark. Only changes since last review were changes to the benchmark code.
Tree-SHA512: 02b86eab7b554e1a57a15b17a4d6d71faa91b556c637b0da29f0c9ee76597a110be8e3b4d0c158d4cab04af0623de18b764837be0ec2a72afcfe1ad9c78a83c6
Add more fs::path operator/ and operator+ overloads to prevent unsafe
string->path conversions on Windows that would cause strings to be
decoded according to the current Windows locale & code page instead of
the correct string encoding.
Update application code to deal with loss of implicit string->path
conversions by calling fs::u8path or fs::PathFromString explicitly, or
by just changing variable types from std::string to fs::path to avoid
conversions altoghther, or make them happen earlier.
In all cases, there's no change in behavior either (1) because strings
only contained ASCII characters and would be decoded the same regardless
of what encoding was used, or (2) because of the 1:1 mapping between
paths and strings using the PathToString and PathFromString functions.
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
819d03b932 refactor: took out unused member functions (Zero)
ed69213c2b build: enable unused member function diagnostic (Zero)
Pull request description:
This PR enables the `-Wunused-member-function` compiler diagnostic, as discussed in #19702.
> **Notice**: The `unused-member-function` diagnostic is only available on clang. Therefore, clang should be used to test this PR.
- [x] Include the `-Wunused-member-function`diagnostic in `./configure.ac`. (ed69213c2b)
- [x] Resolve the reported warnings. (819d03b932)
Currently, enabling this flag no longer reports the following warnings:
> **Note**: output from `make 2>&1 | grep "warning: unused member function" | sort | uniq -c`
```
1 index/blockfilterindex.cpp:54:5: warning: unused member function 'DBHeightKey' [-Wunused-member-function]
2 script/bitcoinconsensus.cpp:50:9: warning: unused member function 'GetType' [-Wunused-member-function]
1 test/util_tests.cpp:1975:14: warning: unused member function 'operator=' [-Wunused-member-function]
```
All tests have passed locally (from `make check` & `src/test/test_bitcoin`).
This PR closes#19702.
ACKs for top commit:
practicalswift:
ACK 819d03b932 - patch still looks correct :)
MarcoFalke:
ACK 819d03b932
pox:
Tested ACK 819d03b932 with clang after `make clean`. No unused member function warnings.
theStack:
tested ACK 819d03b932
Tree-SHA512: 5fdfbbb02b3dc618a90a874a5caa5e01e596fc1d14a209e75a6981f01b253f9bca0cfac8fdd758dd7151986609fb76571c3745124a29cfd4f8cbb8d82a07272e
0187d4c118 [indexes] Add compact block filter headers cache (John Newbery)
Pull request description:
Cache block filter headers at heights of multiples of 1000 in memory.
Block filter headers at height 1000x are checkpointed, and will be the most frequently requested. Cache them in memory to avoid costly disk reads.
ACKs for top commit:
jkczyz:
ACK 0187d4c118
theStack:
ACK 0187d4c118🎉
fjahr:
re-utACK 0187d4c118
laanwj:
code review ACK 0187d4c118
ariard:
Code Review ACK 0187d4c.
Tree-SHA512: 2075ae36901ebcdc4a217eae5203ebc8582181a0831fb7a53a119f031c46bca960a610a38a3d0636a9a405f713efcf4200c85f10c8559fd80139036d89473c56
Cache block filter headers at heights of multiples of 1000 in memory.
Block filter headers at height 1000x are checkpointed, and will be the
most frequently requested. Cache them in memory to avoid costly disk
reads.
