This adds a bitset module that implements a BitSet<N> class, a variant
of std::bitset with a few additional features that cannot be implemented
in a wrapper without performance loss (specifically, finding first and
last bit set, or iterating over all set bits).
As per doc/developer-notes#logging, LogError should be used for
severe problems that require the node to shut down.
Co-Authored-By: stickies-v <stickies-v@protonmail.com>
fa780e1c25 build: Remove --enable-gprof (MarcoFalke)
Pull request description:
It is unclear what benefit this option has, given that:
* `gprof` requires re-compilation (`perf` and other tools can instead be used on existing executables)
* `gprof` requires hardening to be disabled
* `gprof` doesn't work with `clang`
* `perf` is documented in the dev-notes, and test notes, and embedded into the functional test framework; `gprof` isn't
* Anyone really wanting to use it could pass the required flags to `./configure`
* I couldn't find any mention of the use of `gprof` in the discussions in this repo, apart from the initial pull request adding it (cfaac2a60f)
* Keeping it means that it needs to be maintained and ported to CMake
Fix all issues by removing it.
ACKs for top commit:
TheCharlatan:
ACK fa780e1c25
hebasto:
ACK fa780e1c25, I have reviewed the code and it looks OK.
willcl-ark:
crACK fa780e1c25
Tree-SHA512: 0a9ff363ac2bec8b743878a4e3147f18bc16823d00c5007568432c36320bd0199b13b6d0ce828a9a83c2cc434c058afaa64eb2eccfbd93ed85b81ce10c41760c
0d3ef83433 ci: Use relative paths in `win64-native` CI job consistently (Hennadii Stepanov)
501aceefcf ci: Remove no longer needed workaround for GHA Windows images (Hennadii Stepanov)
Pull request description:
This PR:
1. Removes no longer needed workaround for GHA Windows images.
GHA Windows images previously had multiple VC Build Tools installed, which required specifying the `VCPKG_PLATFORM_TOOLSET_VERSION` explicitly to avoid linker errors. This issue has been resolved as per
https://github.com/actions/runner-images/issues/9701.
2. Switches all references to temporary files to relative ones for consistency and readability.
ACKs for top commit:
sipsorcery:
ACK 0d3ef83433.
maflcko:
ACK 0d3ef83433
Tree-SHA512: e832b87fc6dee1e9d1eb452797f16b732e776c2ecdbe3dc9e64cc48ce9b5b89c569d5b96b999423ae1261ff4bf684b7003af84d8024ef5260682f531c4e8ff5e
15796d4b61 build: warn on self-assignment (Cory Fields)
53372f2176 refactor: disable self-assign warning for tests (Cory Fields)
Pull request description:
Belt-and suspenders after #30234. Self-assignment should be safe _and_ discouraged.
We used to opt out of this warning because something deep in our serialization/byteswapping code could self-assign, but that doesn't appear to be the case anymore.
ACKs for top commit:
maflcko:
ACK 15796d4b61
fanquake:
ACK 15796d4b61 - not a huge fan of inline pragma usage, but this seems fine, given it's to work around an already-fixed compiler bug, and we'll only be carrying it for a shortish time in any case.
Tree-SHA512: 1f95f7c730b974ad1da55ebd381040bac312f2f380fff9d569ebab91d7c1963592a84d1613d81d96238c6f5a66aa40deebba68a76f6b24b02150d0a77c769654
fab01b5220 refactor: performance-for-range-copy in psbt.h (MarcoFalke)
Pull request description:
A copy of the partial signatures is not required before serializing them.
For reference, this was found by switching the codebase to `std::span` (not sure why it wasn't found with `Span` though):
```
./psbt.h:240:23: error: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy,-warnings-as-errors]
240 | for (auto sig_pair : partial_sigs) {
| ^
| const &
ACKs for top commit:
tdb3:
ACK fab01b5220
theStack:
utACK fab01b5220
Tree-SHA512: b55513d8191118499716684190ee568d171b50ae9171d246ca6e047f0cfd3ec14c9453d721e88af55e47bb41fa66cbafdbfb47bc5f9b8d82789e0a9b634b371b
1f6ab1215b minor: remove unnecessary semicolons from RPC content type examples (Matthew Zipkin)
b225295298 test: use json-rpc 2.0 in all functional tests by default (Matthew Zipkin)
391843b029 bitcoin-cli: use json-rpc 2.0 (Matthew Zipkin)
d39bdf3397 test: remove unused variable in interface_rpc.py (Matthew Zipkin)
0ead71df8c doc: update and link for JSON-RPC 2.0 (Matthew Zipkin)
Pull request description:
This is a follow-up to #27101.
- Addresses [post-merge comments ](https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1606723428)
- bitcoin-cli uses JSON-RPC 2.0
- functional tests use JSON-RPC 2.0 by default (exceptions are in the regression tests added by #27101)
ACKs for top commit:
tdb3:
ACK 1f6ab1215b
cbergqvist:
ACK 1f6ab1215b
Tree-SHA512: 49bf14c70464081280216ece538a2f5ec810bac80a86a83ad3284f0f1b017edf755a1a74a45be279effe00218170cafde7c2de58aed07097a95c2c6b837a6b6c
In order to ensure that the change of nVersion to a uint32_t in the
previous commit has no effect, rename nVersion to version in this commit
so that reviewers can easily spot if a spot was missed or if there is a
check somewhere whose semantics have changed.
This is a just a mechanical change, renaming and inverting the meaning
of the indexing variable.
"m_blockfiles_indexed" is a more straightforward name for this variable
because this variable just indicates whether or not
<datadir>/blocks/blk?????.dat files have been indexed in the
<datadir>/blocks/index LevelDB database. The name "m_reindexing" was
more confusing, it could be true even if -reindex was not specified, and
false when it was specified. Also, the previous name unnecessarily
required thinking about the whole reindexing process just to understand
simple checks in validation code about whether blocks were indexed.
The motivation for this change is to follow up on previous commits,
moving away from having multiple variables called "reindex" internally,
and instead naming variables individually after what they do and
represent.
Before this change continuing a reindex without the -reindex flag set
would leave the block and coins db intact, but discard the data of the
optional indexes. While not a bug per se, wiping the data again is
wasteful, both in terms of having to write it again, and potentially
leading to longer startup times.
When initially running a reindex, both the block index and any further
activated indexes are wiped. On an index's Init(), both the best block
stored by the index and the chain's tip are null. An index's m_synced
member is therefore true. This means that it will process blocks through
validation events while the reindex is running.
Currently, if the reindex is continued without the user re-specifying
the reindex flag, the block index is preserved but further index data is
wiped. This leads to the stored best block being null, but the chain tip
existing. The m_synced member will be set to false. The index will not
process blocks through the validation interface, but instead use the
background sync once the reindex is completed.
If the index is preserved (this change) after a restart its best block
may potentially match the chain tip. The m_synced member will be set to
true and the index can process validation events during the rest of the
reindex.
Drop confusing kernel options:
BlockManagerOpts::reindex
ChainstateLoadOptions::reindex
ChainstateLoadOptions::reindex_chainstate
Replacing them with more straightforward options:
ChainstateLoadOptions::wipe_block_tree_db
ChainstateLoadOptions::wipe_chainstate_db
Having two options called "reindex" which did slightly different things
was needlessly confusing (one option wiped the block tree database, and
the other caused block files to be rescanned). Also the previous set of
options did not allow rebuilding the block database without also
rebuilding the chainstate database, when it should be possible to do
those independently.
30a01134cd [doc] update bips.md for 431 (glozow)
9dbe6a03f0 [test] wallet uses CURRENT_VERSION which is 2 (glozow)
539404fe0f [policy] make v3 transactions standard (glozow)
052ede75af [refactor] use TRUC_VERSION in place of 3 (glozow)
Pull request description:
Make `nVersion=3` (which is currently nonstandard on mainnet) standard.
Note that we will treat these transactions as Topologically Restricted Until Confirmation (TRUC). Spec is in BIP 431 and implementation is in #28948, #29306, and #29873
See #27463 for overall project tracking, and #29319 for information about relevance to cluster mempool.
ACKs for top commit:
sdaftuar:
utACK 30a01134c
achow101:
ACK 30a01134cd
instagibbs:
utACK 30a01134cd
murchandamus:
ACK 30a01134cd
ismaelsadeeq:
ACK 30a01134cd🛰️
Tree-SHA512: 2a4aec0442c860e792a061d83e36483c1f1b426f946efbdf664c8db97a596e498b535707e1d3a900218429486ea69fd4552e3d476526a6883cbd5556c6534b48
7b8eea067f tests: add fuzz tests for VecDeque (Pieter Wuille)
62fd24af6a util: add VecDeque (Pieter Wuille)
Pull request description:
Extracted from #30126.
This adds a `VecDeque` data type, inspired by `std::deque`, but backed by a single allocated memory region used as a ring buffer instead of a linked list of arrays. This gives better memory locality and less allocation overhead, plus better guarantees (some C++ standard library implementations, though not libstdc++ and libc++, use a separate allocation per element in a deque).
It is intended for the candidate set search queue in #30126, but may be useful as a replacement for `std::deque` in other places too. It's not a full drop-in replacement, as I did not add iteration support which is unnecessary for the intended use case, but nothing prevents adding that if needed.
Everything is tested through a simulation-based fuzz test that compares the behavior with normal `std::deque` equivalent operations, both for trivially-copyable/destructible types and others.
ACKs for top commit:
instagibbs:
reACK 7b8eea067f
cbergqvist:
re-ACK 7b8eea067f
hebasto:
re-ACK 7b8eea067f, I've verified changes since my recent [review](https://github.com/bitcoin/bitcoin/pull/30161#pullrequestreview-2103018546) with
glozow:
ACK 7b8eea067f
Tree-SHA512: 1b62f3ba1a43a1293d8c9de047e2399442e74c46de2df81406151fe27538716ce265f35fb6779ee56d77a39cddf8fb4b4e15bda8f04ebf3b149e2f05fa55cb21
It does not control any actual logic and the log message as well as the
comment are obsolete, since no database initialization takes place there
anymore. Log messages indicating when indexes and chainstate databases
are loaded exist in other places.
Reverts a bug introduced in b47bd95920
"kernel: De-globalize fReindex". The change leads to a GUI user being
prompted to re-index on a chainstate loading failure more than once as
well as the node actually not reindexing if the user chooses to. Fix
this by setting the reindexing option instead of the atomic, which can
be safely re-used to indicate that a reindex should be attempted.
The bug specifically is caused by the chainman, and thus the blockman
and its m_reindexing atomic being destroyed on every iteration of
the for loop.
The reindex option for ChainstateLoadOptions is currently also set in a
confusing way. By using the reindex atomic, it is not obvious in which
scenario it is true or false.
The atomic is controlled by both the user passing the -reindex option,
the user chosing to reindex if something went wrong during chainstate
loading when running the gui, and by reading the reindexing flag from
the block tree database in LoadBlockIndexDB. In practice this read is
done through the chainstate module's CompleteChainstateInitialization's
call to LoadBlockIndex. Since this is only done after the reindex option
is set already, it does not have an effect on it.
Make this clear by using the reindex option from the blockman opts which
is only controlled by the user.
GHA Windows images previously had multiple VC Build Tools installed,
which required specifying the `VCPKG_PLATFORM_TOOLSET_VERSION`
explicitly to avoid linker errors. This issue has been resolved as per
https://github.com/actions/runner-images/issues/9701.
fa52e13ee8 test: Remove struct.pack from almost all places (MarcoFalke)
fa826db477 scripted-diff: test: Use int.to_bytes over struct packing (MarcoFalke)
faf2a975ad test: Use int.to_bytes over struct packing (MarcoFalke)
faf3cd659a test: Normalize struct.pack format (MarcoFalke)
Pull request description:
`struct.pack` has many issues:
* The format string consists of characters that may be confusing and may need to be looked up in the documentation, as opposed to using easy to understand self-documenting code.
This lead to many test bugs, which weren't hit, which is fine, but still confusing. Ref: https://github.com/bitcoin/bitcoin/pull/29400, https://github.com/bitcoin/bitcoin/pull/29399, https://github.com/bitcoin/bitcoin/pull/29363, fa3886b7c6, ...
Fix all issues by using the built-in `int` helpers `to_bytes` via a scripted diff.
Review notes:
* For `struct.pack` and `int.to_bytes` the error behavior is the same, although the error messages are not identical.
* Two `struct.pack` remain. One for float serialization in a C++ code comment, and one for native serialization.
ACKs for top commit:
achow101:
ACK fa52e13ee8
rkrux:
tACK [fa52e13](fa52e13ee8)
theStack:
Code-review ACK fa52e13ee8
Tree-SHA512: ee80d935b68ae43d1654b047e84ceb80abbd20306df35cca848b3f7874634b518560ddcbc7e714e2e7a19241e153dee64556dc4701287ae811e26e4f8c57fe3e
This is an STL-like container that interface-wise looks like std::deque, but
is backed by a (fixed size, with vector-like capacity/reserve) circular buffer.
Belt-and suspenders after #30234. Self-assignment should be safe _and_
discouraged.
We used to opt out of this warning because something deep in our
serialization/byteswapping code could self-assign, but that doesn't appear to
be the case anymore.
This new test uses the `vExtraTxnForCompact` (`extra_txn`) vector of
optional orphan/conflicted/etc. transactions to provide a transaction
in a compact block that was not otherwise present in our mempool.
This also covers an improbable nullptr deref bug addressed in
bf031a517c (#29752) where the
`extra_txn` vec/circular-buffer was sometimes null-initialized and
not yet filled when dereferenced in `PartiallyDownloadedBlock::InitData`.
232928b58a build: no-longer allow GCC-10 in C++20 check (fanquake)
Pull request description:
Reverts part of fa67f096bd, now that we require a minimum of GCC 11.
See also:
https://github.com/bitcoin/bitcoin/pull/28349#issuecomment-1745143612.
ACKs for top commit:
maflcko:
utACK 232928b58a
theuni:
utACK 232928b58a
Tree-SHA512: 10e0adac2dd6e455aaf97ebfe73c7586430349fc27ac435bc6c0d99a4934a380398d14467aacd9cedf371345da291366b3ab2c3be7db5d97e21ad6212b2c7890
