24bc46c83b cli: Add warning for duplicate port definition (tdb3)
e208fb5d3b cli: Sanitize ports in rpcconnect and rpcport (tdb3)
Pull request description:
Adds invalid port detection to bitcoin-cli for -rpcconnect and -rpcport.
In addition to detecting malformed/invalid ports (e.g. those outside of the 16-bit port range, not numbers, etc.), bitcoin-cli also now considers usage of port 0 to be invalid. bitcoin-cli previously considered port 0 to be valid and attempted to use it to reach bitcoind.
Functional tests were added for invalid port detection as well as port prioritization.
Additionally, a warning is provided when a port is specified in both -rpcconnect and -rpcport.
This PR is an alternate approach to PR #27820 (e.g. SplitHostPort is unmodified).
Considered an alternative to 127.0.0.1 being specified in functional tests, but at first glance, this might need an update to test_framework/util.py (e.g. rpc_url), which might be left to a future PR.
ACKs for top commit:
S3RK:
light code review ACK 24bc46c83b
achow101:
ACK 24bc46c83b
cbergqvist:
re ACK 24bc46c83b
Tree-SHA512: c83ab6a30a08dd1ac8b368a7dcc2b4f23170f0b61dd67ffcad7bcda05096d333bcb9821fba11018151f55b2929c0a333bfec15b8bb863d83f41fc1974c6efca5
09ef322acc [[refactor]] Check CTxMemPool options in constructor (TheCharlatan)
Pull request description:
The tests should run the same checks on the mempool options that the init code also applies. The downside to this patch is that the log line may now be printed more than once in the for loop.
This was originally noticed here https://github.com/bitcoin/bitcoin/pull/25290#discussion_r900272797.
ACKs for top commit:
stickies-v:
re-ACK 09ef322acc . Fixed unreachable assert and updated docstring, and also added an exception for "-maxmempool must be at least " in the `tx_pool` fuzz test, which makes sense when looking at how the mempool options are constructed in `SetMempoolConstraints`.
achow101:
ACK 09ef322acc
ryanofsky:
Code review ACK 09ef322acc. Just fuzz test error checking fix and updated comment since last review
Tree-SHA512: eb3361411c2db70be17f912e3b14d9cb9c60fb0697a1eded952c3b7e8675b7d783780d45c52e091931d1d80fe0f0280cee98dd57a3100def13af20259d9d1b9e
7cbfd7a7ce refactor: rename (macho) ld64 to lld (fanquake)
d851451705 ci: update deps for macOS cross build (fanquake)
9ebdd5e9e0 depends: update install docs for macOS cross compilation (fanquake)
fb74fd66cb depends: remove no-longer used llvm_* vars from macOS build (fanquake)
9043f12425 depends: no-longer pass -B to clang in macOS cross-compile (fanquake)
f9994b025e depends: remove native LLVM package (fanquake)
e9a44faf14 depends: remove FORCE_USE_SYSTEM_CLANG (fanquake)
9946618f61 guix: use clang-toolchain-18 for macOS build (fanquake)
Pull request description:
Remove `FORCE_USE_SYSTEM_CLANG` in favour of always using the system Clang and lld for macOS cross-compilation; rather than downloading precompiled blobs.
For example, anyone using Ubuntu 24.04 should be able to `apt install clang llvm lld .. etc`, and then cross-compile for macOS using:
```bash
# clang --version
Ubuntu clang version 18.1.3 (1)
make -C depends HOST=arm64-apple-darwin FORCE_USE_SYSTEM_CLANG=1
./autogen.sh
CONFIG_SITE=/path/to/depends/arm64-apple-darwin/share/config.site ./configure
make
# file src/qt/bitcoin-qt
src/qt/bitcoin-qt: Mach-O 64-bit arm64 executable, flags:<NOUNDEFS|DYLDLINK|TWOLEVEL|WEAK_DEFINES|BINDS_TO_WEAK|PIE|HAS_TLV_DESCRIPTORS>
```
Note that the minimum supported version of Clang we will support for macOS cross-compilation will likely be more recent than our other minimum supported version of Clang, due to compiler/linker option usage.
ACKs for top commit:
Sjors:
tACK 7cbfd7a7ce
theuni:
ACK 7cbfd7a7ce
TheCharlatan:
Nice, ACK 7cbfd7a7ce
Tree-SHA512: 1499e29b3b238c5c85278c38e8fb6bb5e7883db3443f62b6bf397c5d761bedbc054962be645a9defce15266f0a969bb4b3ccd28b6e4dd874472857b928f185d1
ab98e6fd03 test: add coverage for errors for `combinerawtransaction` RPC (brunoerg)
Pull request description:
This PR adds test coverage for the following errors for the `combinerawtransaction` RPC:
* Tx decode failed
* Missing transactions
* Input not found or already spent
For reference: https://maflcko.github.io/b-c-cov/total.coverage/src/rpc/rawtransaction.cpp.gcov.html
ACKs for top commit:
maflcko:
lgtm ACK ab98e6fd03
tdb3:
ACK ab98e6fd03
Tree-SHA512: 8a133c25dad2e1b236e0278a88796f60f763e3fd6fbbc080f926bb23f9dcc55599aa242d6e0c4ec15a179d9ded10a1f17ee5b6063719107ea84e6099f10416b2
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.
