a0eafc10f94362408f54195ffd5a9237dc1ef638 functional test: Deduplicate assert_mempool_contents() (Hodlinator)
Pull request description:
Recently added `mempool_util` implementation probably evolved in parallel with the package RBF one before being submitted as part of ephemeral dust in e2e30e89ba4b9bdbcabaf5b4346610922f0728bb (related comments: https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1825278134, https://github.com/bitcoin/bitcoin/pull/31279#pullrequestreview-2445579323).
ACKs for top commit:
instagibbs:
ACK a0eafc10f94362408f54195ffd5a9237dc1ef638
achow101:
ACK a0eafc10f94362408f54195ffd5a9237dc1ef638
l0rinc:
ACK a0eafc10f94362408f54195ffd5a9237dc1ef638
theStack:
ACK a0eafc10f94362408f54195ffd5a9237dc1ef638
Tree-SHA512: 25ea807d7c041c18be0e4f424131419365d7c1e0fc6c4fb7ac7289c2f8196fd341ff2a2a3ea88df2c3a389edb4571a5fb889efc1b0204c65f7e09ef8f608d0d3
92d3d691f097ead8e5ae571eb9bf691133a6aa49 fuzz: Implement G_TEST_GET_FULL_NAME (Hodlinator)
Pull request description:
Catching up to bench & unit tests. Makes for more orderly paths for fuzz tests using `BasicTestingSetup`.
### Before
```
/tmp/test_common bitcoin/0748ae43ef8fa80703bc/regtest/blocks/xor.dat
```
### After
```
/tmp/test_common bitcoin/tx_pool_standard/f18b3744625e0600eb0c/regtest/blocks/xor.dat
```
ACKs for top commit:
kevkevinpal:
ACK [92d3d69](92d3d691f0)
furszy:
utACK 92d3d691f097ead8e5ae571eb9bf691133a6aa49
tdb3:
ACK 92d3d691f097ead8e5ae571eb9bf691133a6aa49
dergoegge:
utACK 92d3d691f097ead8e5ae571eb9bf691133a6aa49
brunoerg:
code review ACK 92d3d691f097ead8e5ae571eb9bf691133a6aa49
Tree-SHA512: 5e83970b111232adece10f79e3a43d0c3c49ab635763e2a4b420f1336cbb8fee94aab751264ddec01ac8363166636e3b29cfe3b2969fc28c8dd6b31bda351950
fe3457ccfff9a022d9f183e18217422e2e1f7689 ci: note that we should install pkgconf in future (fanquake)
8d203480b338c7504887777a8857e91708deadc7 doc: migrate from pkg-config to pkgconf in macOS build docs (fanquake)
Pull request description:
Migrate the macOS build docs and CI from `pkg-config` to `pkgconf`. As the former now just redirects to the later.
Upstream is currently mass-migrating its formula. i.e https://github.com/Homebrew/homebrew-core/pull/198317.
Fixes#31334.
ACKs for top commit:
maflcko:
ACK fe3457ccfff9a022d9f183e18217422e2e1f7689 🍭
hebasto:
re-ACK fe3457ccfff9a022d9f183e18217422e2e1f7689.
Tree-SHA512: 6e337acb6767d163491149b6ae7181d7d7042bc11cdc745eb6f52d4df6d7a19c4f6daa000b314acd9178f97e670aba145f829e48b1b3033117d7e39cdd3af177
Recently added mempool_util implementation probably evolved in parallel with the package RBF one before being submitted as part of ephemeral dust in e2e30e89ba4b9bdbcabaf5b4346610922f0728bb.
Brew has migrated to using the later:
```bash
brew info pkg-config
==> pkgconf: stable 2.3.0 (bottled), HEAD
Package compiler and linker metadata toolkit
https://github.com/pkgconf/pkgconf
```
9aa50152c1cfa1c41215b2b51ed7a516aa67137a Add destroy to BlockTemplate schema (Sjors Provoost)
Pull request description:
This ensures that if a client no longer needs a block template, the node can clear its memory as soon as possible.
A block template may hold on to transactions that are no longer in the mempool, so this can be significant.
This has a trivial silent merge conflict with #31283 because it also used the number `@9` (gaps are not allowed). I'll rebase whichever is merged last.
ACKs for top commit:
TheCharlatan:
Re-ACK 9aa50152c1cfa1c41215b2b51ed7a516aa67137a
ryanofsky:
Code review ACK 9aa50152c1cfa1c41215b2b51ed7a516aa67137a
Tree-SHA512: 393571b4455969cba71c7572feaeff4503738205331ab198b9181c156c75eb65933a8e5ceff66fc06d1efb8f2528bdb254e5eee7df75735b912526de1e7b166d
When BasicTestingSetup is used in fuzz-tests it will now create test directories containing the fuzz target names. Example:
/tmp/test_common bitcoin/tx_package_eval/153d7906294f7d0606a7/
This is already implemented for bench and unit tests.
5736d1ddacc4019101e7a5170dd25efbc63b622a tracing: pass if replaced by tx/pkg to tracepoint (0xb10c)
a4ec07f1944999c2eead41d08d7dd4fc3aa71243 doc: add comments for CTxMemPool::ChangeSet (Suhas Daftuar)
83f814b1d1100baac9dca9c176f89b0ec2555dbc Remove m_all_conflicts from SubPackageState (Suhas Daftuar)
d3c8e7dfb63f7986a1f9654ea2393aabe3cd78da Ensure that we don't add duplicate transactions in rbf fuzz tests (Suhas Daftuar)
d7dc9fd2f7bc675256687b9c55fdbec9cc8ac781 Move CalculateChunksForRBF() to the mempool changeset (Suhas Daftuar)
284a1d33f1dcbc3b3404ea40a948ff6600239613 Move prioritisation into changeset (Suhas Daftuar)
446b08b599bc492bbec10ccc2292aee6f90c58e7 Don't distinguish between direct conflicts and all conflicts when doing cluster-size-2-rbf checks (Suhas Daftuar)
b53041021abc4f9ee7203341413e8676e2d5a7ca Duplicate transactions are not permitted within a changeset (Suhas Daftuar)
b447416fddcb8c8647391502cca3dbfd1552e02e Public mempool removal methods Assume() no changeset is outstanding (Suhas Daftuar)
2b30f4d36c86f775ac637b171d27d42a02309c5b Make RemoveStaged() private (Suhas Daftuar)
18829194ca68152ac0b38d34e94b9265ee74c410 Enforce that there is only one changeset at a time (Suhas Daftuar)
7fb62f7db60c7d793828ae45f87bc3f5c63cc989 Apply mempool changeset transactions directly into the mempool (Suhas Daftuar)
34b6c5833d11ea84fbd4b891e06408f6f4ca6fac Clean up FinalizeSubpackage to avoid workspace-specific information (Suhas Daftuar)
57983b8add72a04721d3f2050c063a3c4d8683ed Move LimitMempoolSize to take place outside FinalizeSubpackage (Suhas Daftuar)
01e145b9758f1df14a7ea18058ba9577bf88e459 Move changeset from workspace to subpackage (Suhas Daftuar)
802214c0832de00f24268183f7763fa984ba7903 Introduce mempool changesets (Suhas Daftuar)
87d92fa340195d9c87be3d023ca133b90b3b7d4e test: Add unit test coverage of package rbf + prioritisetransaction (Suhas Daftuar)
15d982f91e6b0f145c9dd4edf29827cfabb37a3f Add package hash to package-rbf log message (Suhas Daftuar)
Pull request description:
part of cluster mempool: #30289
It became clear while working on cluster mempool that it would be helpful for transaction validation if we could consider a full set of proposed changes to the mempool -- consisting of a set of transactions to add, and a set of transactions (ie conflicts) to simultaneously remove -- and perform calculations on what the mempool would look like if the proposed changes were to be applied. Two specific examples of where we'd like to do this:
- Determining if ancestor/descendant/TRUC limits would be violated (in the future, cluster limits) if either a single transaction or a package of transactions were to be accepted
- Determining if an RBF would make the mempool "better", however that idea is defined, both in the single transaction and package of transaction cases
In preparation for cluster mempool, I have pulled this reworking of the mempool interface out of #28676 so it can be reviewed on its own. I have not re-implemented ancestor/descendant limits to be run through the changeset, since with cluster mempool those limits will be going away, so this seems like wasted effort. However, I have rebased #28676 on top of this branch so reviewers can see what the new mempool interface could look like in the cluster mempool setting.
There are some minor behavior changes here, which I believe are inconsequential:
- In the package validation setting, transactions would be added to the mempool before the `ConsensusScriptChecks()` are run. In theory, `ConsensusScriptChecks()` should always pass if the `PolicyScriptChecks()` have passed and it's just a belt-and-suspenders for us, but if somehow they were to diverge then there could be some small behavior change from adding transactions and then removing them, versus never adding them at all.
- The error reporting on `CheckConflictTopology()` has slightly changed due to no longer distinguishing between direct conflicts and indirect conflicts. I believe this should be entirely inconsequential because there shouldn't be a logical difference between those two ideas from the perspective of this function, but I did have to update some error strings in some tests.
- Because, in a package setting, RBFs now happen as part of the entire package being accepted, the logging has changed slightly because we do not know which transaction specifically evicted a given removed transaction.
- Specifically, the "package hash" is now used to reference the set of transactions that are being accepted, rather than any single txid. The log message relating to package RBF that happen in the `TXPACKAGES` category has been updated as well to include the package hash, so that it's possible to see which specific set of transactions are being referenced by that package hash.
- Relatedly, the tracepoint logging in the package rbf case has been updated as well to reference the package hash, rather than a transaction hash.
ACKs for top commit:
naumenkogs:
ACK 5736d1ddac
instagibbs:
ACK 5736d1ddacc4019101e7a5170dd25efbc63b622a
ismaelsadeeq:
reACK 5736d1ddacc4019101e7a5170dd25efbc63b622a
glozow:
ACK 5736d1ddacc
Tree-SHA512: 21810872e082920d337c89ac406085aa71c5f8e5151ab07aedf41e6601f60a909b22fbf462ef3b735d5d5881e9b76142c53957158e674dd5dfe6f6aabbdf630b
a6ca8f324396522e9748c9a7bbefb3bf1c74a436 fuzz: Fix difficulty target generation in p2p_headers_presync (marcofleon)
fa327c77e34e0cfb7994842c23f539ab11bf5d3b util: Add ConsumeArithUInt256InRange fuzzing helper (marcofleon)
Pull request description:
In the `p2p_headers_presync` fuzz target, this assertion failed:
```
assert(total_work < chainman.MinimumChainWork());
```
Input that triggered the failure: [p2ppresync_crash.txt](https://github.com/user-attachments/files/17620203/p2ppresync_crash.txt)
The test previously used `ConsumeIntegralInRange` to generate header difficulty targets within a hardcoded range. The fuzzer found specific values in that range that correspond to very low thresholds due to how [`SetCompact`][setcompact-link] works. The total work of a long enough test chain ended up exceeding `MinimumChainWork`.
Fix this by adding a new `ConsumeArithUInt256InRange` helper function and use it in the fuzz test to generate target values within the originally intended range. The target is then converted to an `nBits` value using `GetCompact()`.
For some more context, see https://github.com/bitcoin/bitcoin/pull/30918.
[setcompact-link]: 6463117a29/src/arith_uint256.h (L251-L271)
ACKs for top commit:
instagibbs:
ACK a6ca8f3243
dergoegge:
Code review ACK a6ca8f324396522e9748c9a7bbefb3bf1c74a436
brunoerg:
code review ACK a6ca8f324396522e9748c9a7bbefb3bf1c74a436
Tree-SHA512: 92013d9d37bd3f11992ee678ba9745196efbdc4d773fd14994116629260bea46ffc9fa3923d443af7b623d39c6211900ce98a349c62ad1976e12312c37ef9df0
637f437a1642ce845d7f750b45b71320a82d83b5 doc: remove PR Review Club frequency (Gabriele Bocchi)
Pull request description:
The PR Review Club is mentioned as weekly in the CONTRIBUTING.md file, but it is held monthly as per the official [Bitcoin Core PR Review Club website](https://bitcoincore.reviews/). This PR updates the documentation to just remove the frequency.
ACKs for top commit:
fanquake:
ACK 637f437a1642ce845d7f750b45b71320a82d83b5
Tree-SHA512: 27bf8a0e32edd8bedb5301ceb3c744ff4629403292a7ad00b633921f36278443ae297cd53708a533b1d6e6eab863b831e11247b95277b94cce28e3d5ddb7d9b9
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
This ensures that if a client no longer needs a block template,
the node can clear its memory as soon as possible.
A block template may hold on to transactions that are no longer
in the mempool, so this can be significant.
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
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.
