fa1177e3d7 refactor: Avoid std::string format strings (MarcoFalke)
Pull request description:
This changes some unchecked `std::string` format strings to use string literals, which are `consteval` checked at compile-time.
Split out, because it is used in several pull requests.
ACKs for top commit:
l0rinc:
ACK fa1177e3d7
tdb3:
code review and light test ACK fa1177e3d7
rkrux:
tACK fa1177e3d7
ryanofsky:
Code review ACK fa1177e3d7
Tree-SHA512: 7cc70a49b07dadc386336687b463043e79e94a46d18db0184c9813218536e87e954a1afeb8739d5d8706e7b2f355d3f7984048c7de2725851b463985f1c5369f
0bd53d913c test: add test for getchaintips behavior with invalid chains (Martin Zumsande)
ccd98ea4c8 test: cleanup rpc_getchaintips.py (Martin Zumsande)
f5149ddb9b validation: mark blocks building on an invalid block as BLOCK_FAILED_CHILD (Martin Zumsande)
783cb7337f validation: call RecalculateBestHeader in InvalidChainFound (Martin Zumsande)
9275e9689a rpc: call RecalculateBestHeader as part of reconsiderblock (Martin Zumsande)
a51e91783a validation: add RecalculateBestHeader() function (Martin Zumsande)
Pull request description:
`m_best_header` (the most-work header not known to be on an invalid chain) can be wrong in the context of invalidation / reconsideration of blocks. This can happen naturally (a valid header is received and stored in our block tree db; when the full block arrives, it is found to be invalid) or triggered by the user with the `invalidateblock` / `reconsiderblock` rpc.
We don't currently use `m_best_header` for any critical things (see OP of #16974 for a list that still seems up-to-date), so it being wrong affects mostly rpcs.
This PR proposes to recalculate it if necessary by looping over the block index and finding the best header. It also suggest to mark headers between an invalidatetd block and the previous `m_best_header` as invalid, so they won't be considered in the recalculation.
It adds tests to `rpc_invalidateblock.py` and `rpc_getchaintips.py` that fail on master.
One alternative to this suggested in the past would be to introduce a continuous tracking of header tips (#12138).
While this might be more performant, it is also more complicated, and situations where we need this data are only be remotely triggerable by paying the cost of creating a valid PoW header for an invalid block.
Therefore I think it isn't necessary to optimise for performance here, plus the solution in this PR doesn't perform any extra steps in the normal node operation where no invalidated blocks are encountered.
Fixes #26245
ACKs for top commit:
fjahr:
reACK 0bd53d913c
achow101:
ACK 0bd53d913c
TheCharlatan:
Re-ACK 0bd53d913c
Tree-SHA512: 23c2fc42d7c7bb4f9b4ba4949646b3d0031dd29ed15484e436afd66cd821ed48e0f16a1d02f45477b5d0d73a006f6e81a56b82d9721e0dee2e924219f528b445
8610bcef9d ci: skip Github CI on branch pushes for forks (Sjors Provoost)
Pull request description:
When a contributor maintains a fork of the repo, any pull request they make to their own fork, or to the main repository, will trigger two CI runs one for the branch push and one for the pull request.
After this PR when `SKIP_BRANCH_PUSH` is set, pushes made to git branches inside fork repositories will no longer trigger CI runs, unless the git branches are associated with PRs in the fork repository, or the main repository.
The same behaviour was added for Cirrus in e9bfbb5414.
Note to maintainers: `SKIP_BRANCH_PUSH=true` needs to be set for the GUI repo to maintain existing behaviour.
ACKs for top commit:
m3dwards:
ACK 8610bcef9d
achow101:
ACK 8610bcef9d
vasild:
ACK 8610bcef9d
Tree-SHA512: 4055153f03f0cb60a97ce26157ab9db40a4609dee9f060ed7b06aa8841df5bd8e1a42ff2ac0f20bd69e221e8e67bff062a9a361a291124070a03dd51c609e845
42066f45ff Refactor SipHash_32b benchmark to improve accuracy and avoid optimization issues (Lőrinc)
Pull request description:
This PR stems from the discussions in https://github.com/bitcoin/bitcoin/pull/30317#discussion_r1649187336
The previous benchmark for `SipHash` was slightly less accurate in representing real-world usage and allowed for potential compiler optimizations that could invalidate the benchmark.
This change aims to ensure the benchmark produces more realistic results.
By modifying the initial values and only incrementing the bytes of `val`, the benchmark should reflects a more typical usage patterns - and prevent the compiler from optimizing away the calculations.
-------
On my M1 processor the benchmark's speed changed significantly (but the CI seems to produce the same result as before):
> cmake -B build -DCMAKE_BUILD_TYPE=Release -DBUILD_BENCH=ON && cmake --build build -j10 &&
./build/src/bench/bench_bitcoin --filter=SipHash_32b --min-time=1000
Before:
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 35.15 | 28,445,856.66 | 0.2% | 1.10 | `SipHash_32b`
After (note that only the benchmark changed):
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 22.05 | 45,350,886.64 | 0.3% | 1.10 | `SipHash_32b`
ACKs for top commit:
maflcko:
review ACK 42066f45ff
achow101:
ACK 42066f45ff
hodlinator:
ACK 42066f45ff
Tree-SHA512: 6bbe9d725d4c3396642e55ce48c31baa5339e56838d6d5fb377fb1069daa9292375e7020ceff7da0d78befffc1e984f717b5232217fe911989613480adaa937e
192dac1d33 [refactor] Cleanup BlockAssembler mempool usage (TheCharlatan)
Pull request description:
The `addPackageTxs` method of the `BlockAssembler` currently has access to two mempool variables, as an argument and as a member. Clean this up and clarify that they both are the same mempool instance by removing the argument and instead only using the member variable in the method.
This was noticed in this PR review: https://github.com/bitcoin/bitcoin/pull/25223#discussion_r898164322.
ACKs for top commit:
achow101:
ACK 192dac1d33
danielabrozzoni:
re-ACK 192dac1
stickies-v:
ACK 192dac1d33
Tree-SHA512: a5ae7d6d771fbb5b54f23624b4d3429acf07bbe38179a462a078c825d60c89a725ad4e13fe7067eebea7dfec63c56c8f39b5077b0d949d594f500affcc1272d1
After port collisions are no longer tolerated but lead to
a startup failure in v28.0, local setups of multiple nodes,
each with a different -port value would not be possible anymore
due to collision of the onion default port - even if the nodes
were using tor or not interested in receiving onion inbound connections.
Fix this by deriving the onion listening port to be -port + 1.
(idea by vasild / laanwj)
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
Pass literal format strings instead of std::string so formats can be
checked at compile time.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
Co-authored-by: stickies-v <stickies-v@protonmail.com>
ac286e0d1b doc: Fix grammatical errors in multisig-tutorial.md (secp512k2)
Pull request description:
This pull request fixes grammatical errors in the `multisig-tutorial.md` document.
ACKs for top commit:
Abdulkbk:
ACK ac286e0d1b
Tree-SHA512: 684fe16e802431109957b9cde441353edeb16ffffde4282310c1a6f104adffc53347d00a2cf3a5969a78803f3177d6056ca37d3b7e8be52c2ec43ec0b9fcf4d9
fe39acf88f tinyformat: Add compile-time checking for literal format strings (Ryan Ofsky)
184f34f2d0 util: Support dynamic width & precision in ConstevalFormatString (Ryan Ofsky)
Pull request description:
Add compile-time checking for literal format strings passed to `strprintf` and `tfm::format` to make sure the right number of format arguments are passed.
There is still no compile-time checking if non-literal `std::string` or `bilingual_str` format strings are passed, but this is improved in other PRs:
- [#31061](https://github.com/bitcoin/bitcoin/pull/31061) implements compile-time checking for translated strings
- [#31072](https://github.com/bitcoin/bitcoin/pull/31072) increases compile-time checking by using literal strings as format strings, instead of `std::string` and `bilingual_str`
- [#31149](https://github.com/bitcoin/bitcoin/pull/31149) may drop the `std::string` overload for `strprintf` to [require](https://github.com/bitcoin/bitcoin/pull/31149#issuecomment-2444579999) compile-time checking
ACKs for top commit:
maflcko:
re-ACK fe39acf88f🕐
l0rinc:
ACK fe39acf88f
hodlinator:
re-ACK fe39acf88f
Tree-SHA512: f1ddef0c96b9468c5ffe31b42dc19f1922583dd14f2e180b618d992c98614c5cc7db9f9cd917ef503f833bbc7dbec78e4489d0035416dce6840837e1d66d87cb
e80e4c6ff9 validation: Remove RECENT_CONSENSUS_CHANGE validation result (TheCharlatan)
Pull request description:
The *_RECENT_CONSENSUS_CHANGE variants in the validation result enumerations were always unused. They seem to have been kept around speculatively for a soft fork after segwit, however they were never used for taproot either. This points at them not having a clear purpose. Based on the original pull requests' comments their usage was never entirely clear:
https://github.com/bitcoin/bitcoin/pull/11639#issuecomment-370234133https://github.com/bitcoin/bitcoin/pull/15141#discussion_r271039747
Since they are part of the validation interface and need to be exposed by the kernel library keeping them around may also be confusing to future users of the library.
ACKs for top commit:
sipa:
ACK e80e4c6ff9
naumenkogs:
ACK e80e4c6ff9
dergoegge:
ACK e80e4c6ff9
ajtowns:
ACK e80e4c6ff9
Tree-SHA512: 0af17c4435bb1b5a4f43600da30545cbbe95a7d642419cabdefabfb82b9335d92262c1c48be7ca2f2a024078ae9447161228b6f951d2f508a51159a31947fb54
With --skip-missing-binaries, instead of stopping the execution of
gen-manpages.py when a binary is not found, continue generating
manpages for the available binaries and skip the missing ones.
fa66e0887c bench: add support for custom data directory (furszy)
ad9c2cceda test, bench: specialize working directory name (furszy)
Pull request description:
Expands the benchmark framework with the existing `-testdatadir` arg,
enabling the ability to change the benchmark data directory.
This is useful for running benchmarks on different storage devices, and
not just under the OS `/tmp/` directory.
A good use case is #28574, where we are benchmarking the wallet
migration process on an HDD.
ACKs for top commit:
maflcko:
re-ACK fa66e0887c
achow101:
ACK fa66e0887c
tdb3:
re ACK fa66e0887c
hodlinator:
re-ACK fa66e0887c
pablomartin4btc:
re-ACK fa66e0887c
Tree-SHA512: 4e87206c07e26fe193c07074ae9eb0cc9c70a58aeea8cf27d18fb5425d77e4b00dbe0e6d6a75c17b427744e9066458b9a84e5ef7b0420f02a4fccb9c5ef4dacc
The mempool:replaced tracepoint now reports either a txid or a
package hash (previously it always was a txid). To let users know
if a txid or package hash is passed, a boolean argument is added
the the tracepoint.
In the functional test, a ctypes.Structure class for MempoolReplaced
is introduced as Python warns the following when not explcitly
casting it to a ctype:
Type: 'bool' not recognized. Please define the data with ctypes manually.
Rather than individually calling addUnchecked for each transaction added in a
changeset (after removing all the to-be-removed transactions), instead we can
take advantage of boost::multi_index's splicing features to extract and insert
entries directly from the staging multi_index into mapTx.
This has the immediate advantage of saving allocation overhead for mempool
entries which have already been allocated once. This also means that the memory
locations of mempool entries will not change when transactions go from staging
to the main mempool.
Additionally, eliminate addUnchecked and require all new transactions to enter
the mempool via a CTxMemPoolChangeSet.
9c5775c331 addrman: cap the `max_pct` to not exceed the maximum number of addresses (brunoerg)
Pull request description:
Fixes#31234
This PR fixes a bad alloc issue in `GetAddresses` by capping the value `max_pct`. In practice, values greater than 100 should be treated as 100 since it's the percentage of addresses to return. Also, it limites the value `max_pct` in connman target to exercise values between 0 and 100.
ACKs for top commit:
adamandrews1:
Code Review ACK 9c5775c331
marcofleon:
Tested ACK 9c5775c331. Reproduced the crash on master and checked that this fixed it. The checks added to `GetAddr_` look reasonable.
mzumsande:
Code Review ACK 9c5775c331
vasild:
ACK 9c5775c331
Tree-SHA512: 2957ae561ccc37df71f43c1863216d2e563522ea70b9a4baee6990e0b4a1ddadccabdcb9115c131a9a57480367b5ebdd03e0e3d4c8583792e2b7d1911a0a06d3
