fa7857ccda5d82739bba16eeb7244433978d28f3 build: Enable -Wbidi-chars=any (MarcoFalke)
Pull request description:
I don't see a use-case for UTF-8 bidirectional control characters in this codebase. So disable them for now.
Ref: https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wbidi-chars_003d
ACKs for top commit:
fanquake:
ACK fa7857ccda5d82739bba16eeb7244433978d28f3
Tree-SHA512: 29cf78a2bd0fd94f919f4cd1d1038009a574b4d011146c69bf94d3c06951606200b7d1f827ac6f2fb4540e8f45118ba72b3ccf6c20ef8048e819974371d8f67a
bcd82b13f4649e57d7d106856aab7b2a6296d728 Remove pkgconfig from toolchain file (TheCharlatan)
319a4e82614283afb3dbc5d38ff3b9d17fb911b3 depends: drop sqlite pkgconfig file (fanquake)
a8fe1fd38bf496356dc4f28963d4edfa75fe04a5 depends: better cleanup after fontconfig (fanquake)
17e79c92607e2e32b48ffd388828184c5d1a65df depends: fully remove libtool archives from Qt build (fanquake)
8ca85651c8350a6edb069eae0f88cf03c6eae0d5 guix: move pkg-config to Linux builds (fanquake)
e3e648cf410d30185927d031c81a85d5fa890b8c depends: drop pkg-config option from Qt build (fanquake)
0d185bd99f9e40913f678af0fc224add2e1d2f14 doc: update depends doc to prefer .cmake outputs (fanquake)
Pull request description:
After #31181, `pkg-config` is no-longer needed for macOS or Windows Guix builds. It's still needed for Linux, as it's used by a Qt subdependency (fontconfig to find freetype). However we should also no-longer need it for Qt itself, when building using depends.
ACKs for top commit:
TheCharlatan:
ACK bcd82b13f4649e57d7d106856aab7b2a6296d728
Tree-SHA512: 89ae68281030d43fcb6c5c96429cd038a21f13a8ca19ea828ada47e8f9f0aa7407854a67c9003652817e47ab9565573b7028342e3e11bb1cca1d823c483081cd
faaaf59f71ede057b2c1d369ef8db973c2f2dbc2 test: Make g_rng_temp_path rand, not dependent on SeedRandomForTest (MarcoFalke)
fa80b08fef0eaa600339caa678fdf80a8aec3ce3 test: Revert to random path element (MarcoFalke)
Pull request description:
The randomness in the path element is required to allow a single fuzz test to run in parallel. Previous releases used a uint256 random value, but 10 random bytes should be sufficient as well, while avoiding a `MAX_PATH` violation on Windows.
The issue was introduced by myself, by suggesting to use the current time in https://github.com/bitcoin/bitcoin/pull/31000#discussion_r1835351305.
ACKs for top commit:
kevkevinpal:
reACK faaaf59f71
hodlinator:
ACK faaaf59f71ede057b2c1d369ef8db973c2f2dbc2
tdb3:
re ACK faaaf59f71ede057b2c1d369ef8db973c2f2dbc2
dergoegge:
ACK faaaf59f71ede057b2c1d369ef8db973c2f2dbc2
Tree-SHA512: f12256c8b353618291030f71bf36eab97a25ffeaa28e36a5f2c6718dfc1fbbc8548c71475edec53d59026f2a779a05778db83f0530dd3e1d1faf6e4fc0ee7d70
111465d72dd35e42361fc2a089036f652417ed37 test: Remove unused attempts parameter from wait_until (Fabian Jahr)
5468a23eb9a3fd2b0c08dbca69fe3df58af42530 test: Add check_interval parameter to wait_until (Fabian Jahr)
16c87d91fd4d7709fa9d8824d5b641ef71821931 test: Introduce ensure_for helper (Fabian Jahr)
Pull request description:
A repeating pattern in the functional tests is that the test sleeps for a while to ensure that a certain condition is still true after some amount of time has elapsed. Most recently a new case of this was added in #30807. This PR here introduces an `ensure` helper to streamline this functionality.
Some approach considerations:
- It is possible to construct this by reusing `wait_until` and wrapping it in `try` internally. However, the logger output of the failing wait would still be printed which seems irritating. So I opted for simplified but similar internals to `wait_until`.
- This implementation starts for a failure in the condition right away which has the nice side-effect that it might give feedback on a failure earlier than is currently the case. However, in some cases, it may be expected that the condition may still be false at the beginning and then turns true until time has run out, something that would work when the test sleeps without checking in a loop. I decided against this design (and even against adding it as an option) because such a test design seems like it would be racy either way.
- I have also been going back and forth on naming. To me `ensure` works well but I am also not a native speaker, happy consider a different name if others don't think it's clear enough.
ACKs for top commit:
maflcko:
re-ACK 111465d72dd35e42361fc2a089036f652417ed37 🍋
achow101:
ACK 111465d72dd35e42361fc2a089036f652417ed37
tdb3:
code review re ACK 111465d72dd35e42361fc2a089036f652417ed37
furszy:
utACK 111465d72dd35e42361fc2a089036f652417ed37
Tree-SHA512: ce01a4f3531995375a6fbf01b27d51daa9d4c3d7cd10381be6e86ec5925d2965861000f7cb4796b8d40aabe3b64c4c27e2811270e4e3c9916689575b8ba4a2aa
b2d536100282bd901d3e0be7f7f4a6966e0ef817 build: Temporarily disable compiling `fuzz/utxo_snapshot.cpp` with MSVC (Hennadii Stepanov)
Pull request description:
This PR suggests a temporary workaround for a compiler bug [introduced](https://github.com/bitcoin/bitcoin/issues/31303) in Visual Studio 2022 version 17.12.
This workaround is required to fix the CI until the upstream compiler bug is resolved.
ACKs for top commit:
maflcko:
lgtm ACK b2d536100282bd901d3e0be7f7f4a6966e0ef817
TheCharlatan:
ACK b2d536100282bd901d3e0be7f7f4a6966e0ef817
brunoerg:
ACK b2d536100282bd901d3e0be7f7f4a6966e0ef817
Tree-SHA512: 8f8b772253f6f61d9e24c6ae8692511c7c1229c7d20a45fe680ad6a3909c59b58b504589f06d3135d9708a61684be4aba6a426f7f5f60020551f52b9090d3030
fa1177e3d7ca9ef003ded4d0c915fa6dc22bd37d 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 fa1177e3d7ca9ef003ded4d0c915fa6dc22bd37d
tdb3:
code review and light test ACK fa1177e3d7ca9ef003ded4d0c915fa6dc22bd37d
rkrux:
tACK fa1177e3d7ca9ef003ded4d0c915fa6dc22bd37d
ryanofsky:
Code review ACK fa1177e3d7ca9ef003ded4d0c915fa6dc22bd37d
Tree-SHA512: 7cc70a49b07dadc386336687b463043e79e94a46d18db0184c9813218536e87e954a1afeb8739d5d8706e7b2f355d3f7984048c7de2725851b463985f1c5369f
0bd53d913c1c2ffd2d0779f01bc51c81537b6992 test: add test for getchaintips behavior with invalid chains (Martin Zumsande)
ccd98ea4c88fc1aa959e41e0686d8dff00a44209 test: cleanup rpc_getchaintips.py (Martin Zumsande)
f5149ddb9b7de3559943d7fda0f440e59413dfb5 validation: mark blocks building on an invalid block as BLOCK_FAILED_CHILD (Martin Zumsande)
783cb7337f72a3c7b2e74efd677a8ff0c375fe10 validation: call RecalculateBestHeader in InvalidChainFound (Martin Zumsande)
9275e9689a426964f5eaee65e356754a0548d926 rpc: call RecalculateBestHeader as part of reconsiderblock (Martin Zumsande)
a51e91783aac0beefcb604be159eb1cb96a39051 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 0bd53d913c1c2ffd2d0779f01bc51c81537b6992
achow101:
ACK 0bd53d913c1c2ffd2d0779f01bc51c81537b6992
TheCharlatan:
Re-ACK 0bd53d913c1c2ffd2d0779f01bc51c81537b6992
Tree-SHA512: 23c2fc42d7c7bb4f9b4ba4949646b3d0031dd29ed15484e436afd66cd821ed48e0f16a1d02f45477b5d0d73a006f6e81a56b82d9721e0dee2e924219f528b445
8610bcef9d030013f9e36cffe0c58dd2cfe85d66 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 e9bfbb5414ab14ca14d8edcfdf77f28c9ed67c33.
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 8610bcef9d030013f9e36cffe0c58dd2cfe85d66
achow101:
ACK 8610bcef9d030013f9e36cffe0c58dd2cfe85d66
vasild:
ACK 8610bcef9d030013f9e36cffe0c58dd2cfe85d66
Tree-SHA512: 4055153f03f0cb60a97ce26157ab9db40a4609dee9f060ed7b06aa8841df5bd8e1a42ff2ac0f20bd69e221e8e67bff062a9a361a291124070a03dd51c609e845
42066f45ff5d48e78a317eda63c035809bd657c6 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 42066f45ff5d48e78a317eda63c035809bd657c6
achow101:
ACK 42066f45ff5d48e78a317eda63c035809bd657c6
hodlinator:
ACK 42066f45ff5d48e78a317eda63c035809bd657c6
Tree-SHA512: 6bbe9d725d4c3396642e55ce48c31baa5339e56838d6d5fb377fb1069daa9292375e7020ceff7da0d78befffc1e984f717b5232217fe911989613480adaa937e
192dac1d3370edd579db235d69c034726f37c8da [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 192dac1d3370edd579db235d69c034726f37c8da
danielabrozzoni:
re-ACK 192dac1
stickies-v:
ACK 192dac1d3370edd579db235d69c034726f37c8da
Tree-SHA512: a5ae7d6d771fbb5b54f23624b4d3429acf07bbe38179a462a078c825d60c89a725ad4e13fe7067eebea7dfec63c56c8f39b5077b0d949d594f500affcc1272d1
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>
ac286e0d1bd4c8dbe6d21cbe8c0a131ad8ba1854 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 ac286e0d1bd4c8dbe6d21cbe8c0a131ad8ba1854
Tree-SHA512: 684fe16e802431109957b9cde441353edeb16ffffde4282310c1a6f104adffc53347d00a2cf3a5969a78803f3177d6056ca37d3b7e8be52c2ec43ec0b9fcf4d9
fe39acf88ff552bfc4a502c99774375b91824bb1 tinyformat: Add compile-time checking for literal format strings (Ryan Ofsky)
184f34f2d0fa2e56ad594966b2b99ff4cf840d95 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 fe39acf88ff552bfc4a502c99774375b91824bb1 🕐
l0rinc:
ACK fe39acf88ff552bfc4a502c99774375b91824bb1
hodlinator:
re-ACK fe39acf88ff552bfc4a502c99774375b91824bb1
Tree-SHA512: f1ddef0c96b9468c5ffe31b42dc19f1922583dd14f2e180b618d992c98614c5cc7db9f9cd917ef503f833bbc7dbec78e4489d0035416dce6840837e1d66d87cb
e80e4c6ff91e27d7d40f099a2d7942c29085234c 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 e80e4c6ff91e27d7d40f099a2d7942c29085234c
naumenkogs:
ACK e80e4c6ff9
dergoegge:
ACK e80e4c6ff91e27d7d40f099a2d7942c29085234c
ajtowns:
ACK e80e4c6ff91e27d7d40f099a2d7942c29085234c
Tree-SHA512: 0af17c4435bb1b5a4f43600da30545cbbe95a7d642419cabdefabfb82b9335d92262c1c48be7ca2f2a024078ae9447161228b6f951d2f508a51159a31947fb54
fa66e0887ca1a1445d8b18ba1fadb12b2d911048 bench: add support for custom data directory (furszy)
ad9c2cceda9cd893c0f754e49f7fca6e417ee95f 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 fa66e0887ca1a1445d8b18ba1fadb12b2d911048
achow101:
ACK fa66e0887ca1a1445d8b18ba1fadb12b2d911048
tdb3:
re ACK fa66e0887ca1a1445d8b18ba1fadb12b2d911048
hodlinator:
re-ACK fa66e0887ca1a1445d8b18ba1fadb12b2d911048
pablomartin4btc:
re-ACK fa66e0887ca1a1445d8b18ba1fadb12b2d911048
Tree-SHA512: 4e87206c07e26fe193c07074ae9eb0cc9c70a58aeea8cf27d18fb5425d77e4b00dbe0e6d6a75c17b427744e9066458b9a84e5ef7b0420f02a4fccb9c5ef4dacc
9c5775c331e02dab06c78ecbb58488542d16dda7 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 9c5775c331e02dab06c78ecbb58488542d16dda7. Reproduced the crash on master and checked that this fixed it. The checks added to `GetAddr_` look reasonable.
mzumsande:
Code Review ACK 9c5775c331e02dab06c78ecbb58488542d16dda7
vasild:
ACK 9c5775c331e02dab06c78ecbb58488542d16dda7
Tree-SHA512: 2957ae561ccc37df71f43c1863216d2e563522ea70b9a4baee6990e0b4a1ddadccabdcb9115c131a9a57480367b5ebdd03e0e3d4c8583792e2b7d1911a0a06d3
5c2e291060cca3be500f3af0f6f2d3fd2177a7c9 bench: Add basic CheckEphemeralSpends benchmark (Greg Sanders)
3f6559fa581b1f78cd9a9ef4dc0169e315ffa6b3 Add release note for ephemeral dust (Greg Sanders)
71a6ab4b33df383642cca49397a88b1606171225 test: unit test for CheckEphemeralSpends (Greg Sanders)
21d28b2f362708dd9206feb9ddc11a352063ef0c fuzz: add ephemeral_package_eval harness (Greg Sanders)
127719f516a6a8bbfb65f09827bbe22190df3a58 test: Add CheckMempoolEphemeralInvariants (Greg Sanders)
e2e30e89ba4b9bdbcabaf5b4346610922f0728bb functional test: Add ephemeral dust tests (Greg Sanders)
4e68f901390d512a9dfaf0de34daf822449e9bd2 rpc: disallow in-mempool prioritisation of dusty tx (Greg Sanders)
e1d3e81ab4d34485f1b82cb9c3b967e92a4e1f15 policy: Allow dust in transactions, spent in-mempool (Greg Sanders)
04b2714fbbc4a019d23743a488b9f9b42652617b functional test: Add new -dustrelayfee=0 test case (Greg Sanders)
Pull request description:
A replacement for https://github.com/bitcoin/bitcoin/pull/29001
Now that we have 1P1C relay, TRUC transactions and sibling eviction, it makes sense to retarget this feature more narrowly by not introducing a new output type, and simple focusing on the feature of allowing temporary dust in the mempool.
Users of this can immediately use dust outputs as:
1. Single keyed anchor (can be shared by multiple parties)
2. Single unkeyed anchor, ala P2A
Which is useful when the parent transaction cannot have fees for technical or accounting reasons.
What I'm calling "keyed" anchors would be used anytime you don't want a third party to be able to run off with the utxo. As a motivating example, in Ark there is the concept of a "forfeit transaction" which spends a "connector output". The connector output would ideally be 0-value, but you would not want that utxo spend by anyone, because this would cause financial loss for the coordinator of the service: https://arkdev.info/docs/learn/concepts#forfeit-transaction
Note that this specific use-case likely doesn't work as it involves a tree of dust, but the connector idea in general demonstrates how it could be used.
Another related example is connector outputs in BitVM2: https://bitvm.org/bitvm2.html .
Note that non-TRUC usage will be impractical unless the minrelay requirement on individual transactions are dropped in general, which should happen post-cluster mempool.
Lightning Network intends to use this feature post-29.0 if available: https://github.com/lightning/bolts/issues/1171#issuecomment-2373748582
It's also useful for Ark, ln-symmetry, spacechains, Timeout Trees, and other constructs with large presigned trees or other large-N party smart contracts.
ACKs for top commit:
glozow:
reACK 5c2e291060c via range-diff. Nothing but a rebase and removing the conflict.
theStack:
re-ACK 5c2e291060cca3be500f3af0f6f2d3fd2177a7c9
Tree-SHA512: 88e6a6b3b91dc425de47ccd68b7668c8e98c5683712e892c588f79ad639ae95c665e2d5563dd5e5797983e7542cbd1d4353bc90a7298d45a1843b05a417f09f5
Also known as Ephemeral Dust.
We try to ensure that dust is spent in blocks by requiring:
- ephemeral dust tx is 0-fee
- ephemeral dust tx only has one dust output
- If the ephemeral dust transaction has a child,
the dust is spent by by that child.
0-fee requirement means there is no incentive to mine
a transaction which doesn't have a child bringing its
own fees for the transaction package.
83fab3212c91d91fc5502f940c901a07772ff747 test: Add combinerawtransaction test to rpc_createmultisig (Ava Chow)
Pull request description:
The only coverage of combinerawtransaction is in a legacy wallet only test. So also use it in rpc_createmultisig so that this RPC remains tested after the legacy wallet is removed.
Split from #28710
ACKs for top commit:
maflcko:
re-ACK 83fab3212c91d91fc5502f940c901a07772ff747
BrandonOdiwuor:
Re-ACK 83fab3212c91d91fc5502f940c901a07772ff747
Abdulkbk:
ACK 83fab3212c91d91fc5502f940c901a07772ff747
brunoerg:
code review ACK 83fab3212c91d91fc5502f940c901a07772ff747
rkrux:
tACK 83fab3212c91d91fc5502f940c901a07772ff747
Tree-SHA512: 383d88ff6c9b54337ed81c714026e527b0fed41d976959fd5c6863b49d0defa4ea13fdc3d984885c86a2b6380825cd66c17842cc31f20fbec4bc42d86aecbbfa
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.
