0000276b31 test: Remove redundant verack check (MarcoFalke)
Pull request description:
Currently the sync in `connect_nodes` mentions the `version` and `verack` message types, but only checks the `verack`. Neither check is required, as the `pong` check implies both. In case of failure, the debug log will have to be consulted anyway, so the redundant check doesn't add value.
Also clarify in the comments that the goal is to check the flag `fSuccessfullyConnected` indirectly.
ACKs for top commit:
furszy:
utACK 0000276b31
brunoerg:
ACK 0000276b31
tdb3:
ACK 0000276b31
Tree-SHA512: f9ddcb1436d2f70da462a8dd470ecfc90a534dd6507c23877ef7626e7c02326c077001a42ad0171a87fba5c5275d1970d8c5e5d82c56c8412de944856fdfd6db
39d135e79f test: MiniWallet: respect fee_rate for target_weight, use in mempool_limit.py (Sebastian Falbesoner)
b2f0a9f8b0 test: add framework functional test for MiniWallet's tx padding (Sebastian Falbesoner)
c17550bc3a test: MiniWallet: fix tx padding (`target_weight`) for large sizes, improve accuracy (Sebastian Falbesoner)
Pull request description:
MiniWallet allows to create padded transactions that are equal or slightly above a certain `target_weight` (first introduced in PR #25379, commit 1d6b438ef0), which can be useful especially for mempool-related tests, e.g. for policy limit checks or scenarios to trigger mempool eviction. Currently the `target_weight` parameter doesn't play together with `fee_rate` though, as the fee calculation is incorrectly based on the tx vsize before the padding output is added, so the fee-rate is consequently far off. This means users are forced to pass an absolute fee, which can be quite inconvenient and leads to lots of duplicated "calculate absolute fee from fee-rate and vsize" code with the pattern `fee = (feerate / 1000) * (weight // 4)` on the call-sites.
This PR first improves the tx padding itself to be more accurate, adds a functional test for it, and fixes the `fee_rate` treatment for the `{create,send}_self_transfer` methods. (Next step would be to enable this also for the `_self_transfer_multi` methods, but those currently don't even offer a `fee_rate` parameter). Finally, the ability to pass both `target_weight` and `fee_rate` is used in the `mempool_limit.py` functional test. There might be more use-cases in other tests, that could be done in a follow-up.
ACKs for top commit:
rkrux:
tACK [39d135e](39d135e79f)
ismaelsadeeq:
Code Review ACK 39d135e79f🚀
glozow:
light review ACK 39d135e79f
Tree-SHA512: 6bf8e853a921576d463291d619cdfd6a7e74cf92f61933a563800ac0b3c023a06569b581243166906f56b3c5e8858fec2d8a6910d55899e904221f847eb0953d
d1581c6048 test: doc: fix units in tx size standardness test (s/vbytes/weight units) (Sebastian Falbesoner)
Pull request description:
This small fixup PR is a late follow-up for #17947 (commit 4537ba5f21), where the wrong units has been used in the comments for the large tx composition.
ACKs for top commit:
tdb3:
ACK d1581c6048
ismaelsadeeq:
ACK d1581c6048
glozow:
ACK d1581c6048
Tree-SHA512: ea2de42174f9dca0608275ea377c852ebddc5a04a2b32248ce808aea33d7e00cdee3a225b24c0cf426c69646cccbbc31273c62f7bc1647bb3443a61de3b15670
Relax assumptions about in-mempool children of in-mempool
parents. With package RBF, we will allow a package of size
2 with conflicts on its parent and reconsider the parent
if its fee is insufficient on its own.
Consider:
TxA (in mempool) <- TxB (in mempool)
TxA (in mempool) <- TxB' (in package, conflicts with TxB) <-
TxC (in package)
If TxB' fails to RBF TxB due to insufficient feerate, the
package TxB' + TxC will be considered. PackageV3Checks
called on TxB' will see an in-mempool parent TxA, and
see the in-mempool child TxB. We cannot assume there is
no in-mempool sibling, rather detect it and fail normally.
Prior to package RBF, this would have failed on the first
conflict in package.
f68cba29b3 blockman: Replace m_reindexing with m_blockfiles_indexed (Ryan Ofsky)
1b1c6dcca0 test: Add functional test for continuing a reindex (TheCharlatan)
201c1a9282 indexes: Don't wipe indexes again when already reindexing (TheCharlatan)
804f09dfa1 kernel: Add less confusing reindex options (Ryan Ofsky)
e172553223 validation: Remove needs_init from LoadBlockIndex (TheCharlatan)
533eab7d67 bugfix: Streamline setting reindex option (TheCharlatan)
Pull request description:
When restarting `bitcoind` during an ongoing reindex without setting the `-reindex` flag again, the block and coins db is left intact, but any data from the optional indexes is discarded. While not a bug per se, wiping the data again is
wasteful, both in terms of having to write it again, as well as potentially leading to longer startup times. So keep the index data instead when continuing a prior reindex.
Also includes a bugfix and smaller code cleanups around the reindexing code. The bug was introduced in b47bd95920: "kernel: De-globalize fReindex".
ACKs for top commit:
stickies-v:
ACK f68cba29b3
fjahr:
Code review ACK f68cba29b3
furszy:
Code review ACK f68cba29b3
ryanofsky:
Code review ACK f68cba29b3. Only changes since last review were cherry-picking suggested commits that rename variables, improving comments, and making some tweaks to test code.
Tree-SHA512: b252228cc76e9f1eaac56d5bd9e4eac23408e0fc04aeffd97a85417f046229364673ee1ca7410b9b6e7b692b03f13ece17c42a10176da0d7e975a8915deb98ca
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.
