After the normal optimization process finishes, and finds an optimal
spanning forest, run a second process (while computation budget remains)
to split chunks into minimal equal-feerate chunks.
2cade5d5d1 [miniminer] stop assuming ancestor fees >= self fees (glozow)
Pull request description:
These assertions exist to detect double-deducting values when we update descendants. However, negative fees are possible with `prioritisetransaction` so it doesn't make sense to check this.
Leave the check for sizes because those are never negative.
Fixes#34234
ACKs for top commit:
instagibbs:
ACK 2cade5d5d1
dergoegge:
utACK 2cade5d5d1
Tree-SHA512: 935bbc8bd9a0d508eea43bb49aa43c22735e3f2c1012598f6843e229c13b76e44f9fd3eb8b61c437fa0b32353b4e7b15afa3e31002bdfa382d3d711d16419fde
301d9eea66 qt: Remove "Starting Block" from Peer Detail. Following Deprecation in `bitcoin#34197` (WakeTrainDev)
Pull request description:
the `startingheight` rpc field got deprecated in https://github.com/bitcoin/bitcoin/pull/34197
this pr removes it from peer detail
ACKs for top commit:
maflcko:
review lgtm ACK 301d9eea66
theStack:
ACK 301d9eea66
hebasto:
ACK 301d9eea66, I verified `forms/debugwindow.ui` using Qt Designer.
Tree-SHA512: b870b4cff8ead073a17d171c01c46fc7e750c0343b4578ffb63abc8f40b33abdf08beb6733fead5307ef5d48b078b60d29ac0e0e41190a98f50f92154f0878cf
f78f6f1dc8 wallettool: do not use fs::remove_all in createfromdump cleanup (Ava Chow)
Pull request description:
As pointed out in https://github.com/bitcoin/bitcoin/pull/34156#issuecomment-3716728670, it is possible for `createfromdump` to also accidentally delete the entire wallets directory if the wallet name is the empty string and the dumpfile contains a checksum error.
This is also fixed by removing the files created by only removing the directory for named wallets, and avoiding the use of `fs::remove_all`.
ACKs for top commit:
waketraindev:
lgtm ACK f78f6f1dc8
polespinasa:
code review and tACK f78f6f1dc8
rkrux:
Code review and tACK f78f6f1dc8
willcl-ark:
ACK f78f6f1dc8
pablomartin4btc:
ACK f78f6f1dc8
Tree-SHA512: ff1e7668131ec3632c67d990c99e8fddff28605e7e553c7e20695e61017c88476c3636e22f2007e763a00d527e80e4d1d3d45409f6678d28729b8397430bfe7a
b7c34d08dd test: coverage for migration failure when last sync is beyond prune height (furszy)
82caa8193a wallet: migration, fix watch-only and solvables wallets names (furszy)
d70b159c42 wallet: improve post-migration logging (furszy)
f011e0f068 test: restorewallet, coverage for existing dirs, unnamed wallet and prune failure (furszy)
36093bde63 test: add coverage for unnamed wallet migration failure (furszy)
f4c7e28e80 wallet: fix unnamed wallet migration failure (furszy)
4ed0693a3f wallet: RestoreWallet failure, erase only what was created (furszy)
Pull request description:
Minimal fix for #34128.
The issue occurs during the migration of a legacy unnamed wallet
(the legacy "default" wallet). When the migration fails, the cleanup
logic is triggered to roll back the state, which involves erasing the
newly created descriptor wallets directories. Normally, this only
affects the parent directories of named wallets, since they each
reside in their own directories. However, because the unnamed
wallet resides directly in the top-level `/wallets/` folder, this
logic accidentally deletes the main directory.
The fix ensures that only the wallet.dat file of the unnamed wallet
is touched and restored, preserving the wallet in BDB format and
leaving the main `/wallets/` directory intact.
#### Story Line:
#32273 fixed a different set of issues and, in doing so, uncovered
this one.
Before the mentioned PR, backups were stored in the same directory
as the wallet.dat file. On a migration failure, the backup was then
copied to the top-level `/wallets/` directory. For the unnamed legacy
wallet, the wallet directory is the `/wallets/` directory, so the source
and destination paths were identical. As a result, we threw early in the
`fs::copy_file` call ([here](https://github.com/bitcoin/bitcoin/blob/29.x/src/wallet/wallet.cpp#L4572)) because the file already existed, as we
were trying to copy the file onto itself. This caused the cleanup logic
to abort early on and never reach the removal line.
#### Testing Notes:
Cherry-pick the test commit on top of master and run it. You will
see the failure and realize the reason by reading the test code.
ACKs for top commit:
achow101:
ACK b7c34d08dd
davidgumberg:
crACK b7c34d08dd
w0xlt:
ACK b7c34d08dd
willcl-ark:
ACK b7c34d08dd
Tree-SHA512: d0be14c0ed6417f999c3f2f429652c2407097d0cc18453c91653e57ae4b5375b327ad3b2553d9ea6ff46a3ae00cdbd5ab325b94eba763072c4fc5a773b85618b
fafbc70d48 rpc: [wallet] Use unsigned type for tx version in sendall (MarcoFalke)
Pull request description:
It is confusing to parse the unsigned tx version as a signed type. Also, it makes it harder to use the integer sanitizer.
Can be tested via:
* Build with the flags `-DCMAKE_C_COMPILER='clang' -DCMAKE_CXX_COMPILER='clang++' -DSANITIZERS=undefined,integer,float-divide-by-zero`
* Set the existing suppressions: `export UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=0:report_error_type=1"`
* Start the RPC server, e.g. `./bld-cmake/bin/bitcoin-qt -datadir=/tmp -regtest -server`
* Call the sendall RPC, e.g. `./bld-cmake/bin/bitcoin-cli -datadir=/tmp -regtest -named sendall '["bcrt1qlrt3xps4wxpfcjmljrayr2ualczmnfvd4vzdq3"]' fee_rate=1.234 version=-1`
Before:
```
src/wallet/rpc/spend.cpp:1470:42: runtime error: implicit conversion from type 'int' of value -1 (32-bit, signed) to type 'uint32_t' (aka 'unsigned int') changed the value to 4294967295 (32-bit, unsigned)
Invalid parameter, version out of range(1~3)
```
After:
```
JSON integer out of range
ACKs for top commit:
bensig:
ACK fafbc70d48
achow101:
ACK fafbc70d48
rkrux:
utACK fafbc70d48
theStack:
ACK fafbc70d48
Tree-SHA512: bb7cf54e9691ad2591646b138ffdfac95bf77c5234d489f4e4f2c60b41bdc14cdc18a030fecb0a6ac64e55e4c69b37835afd334f87d8a44b8df6cda053e8fefb
Because the default wallet has no name, the watch-only and solvables
wallets created during migration end up having no name either.
This fixes it by applying the same prefix name we use for the backup
file for an unnamed default wallet.
Before: watch-only wallet named "_watchonly"
After: watch-only wallet named "default_wallet_watchonly"
Right now, after migration the last message users see is "migration completed",
but the migration isn't actually finished yet. We still need to load the new wallets
to ensure consistency, and if that fails, the migration will be rolled back. This
can be confusing for users.
This change logs the post-migration loading step and if a wallet fails to load and
the migration will be rolled back.
When migrating any legacy unnamed wallet, a failed migration would
cause the cleanup logic to remove its parent directory. Since this
type of legacy wallet lives directly in the main '/wallets/' folder,
this resulted in unintentionally erasing all wallets, including the
backup file.
To be fully safe, we will no longer call `fs::remove_all`. Instead,
we only erase the individual db files we have created, leaving
everything else intact. The created wallets parent directories are
erased only if they are empty.
As part of this last change, `RestoreWallet` was modified to allow
an existing directory as the destination, since we no longer remove
the original wallet directory (we only remove the files we created
inside it). This also fixes the restore of top-level default wallets
during failures, which were failing due to the directory existence
check that always returns true for the /wallets/ directory.
This bug started after:
f6ee59b6e2
Previously, the `fs::copy_file` call was failing for top-level wallets,
which prevented the `fs::remove_all` call from being reached.
Track what RestoreWallet creates so only those files and directories
are removed during a failure and nothing else. Preexisting paths
must be left untouched.
Note:
Using fs::remove_all() instead of fs::remove() in RestoreWallet does
not cause any problems currently, but the change is necessary for the
next commit which extends RestoreWallet to work with existing directories,
which may contain files that must not be deleted.
1808b5aaf7 clusterlin: remove unused FixLinearization (cleanup) (Pieter Wuille)
34a77138b7 txgraph: permit non-topological clusters to defer fixing (optimization) (Pieter Wuille)
3380e0cbb5 txgraph: use PostLinearize less prior to linearizing (Pieter Wuille)
62dd88624a txgraph: drop NEEDS_SPLIT_ACCEPTABLE (simplification) (Pieter Wuille)
01ffcf464a clusterlin: support fixing linearizations (feature) (Pieter Wuille)
Pull request description:
Part of #30289, follow-up to #32545.
This gets rid of `FixLinearization()` by integrating the functionality into `Linearize()`, and makes txgraph exploit that (by delaying fixing of clusters until their first re-linearization). It also reduces (but does not eliminate) the number of calls to `PostLinearize`, as the SFL linearization effectively performs something very similar to postlinearization when loading in an existing linearization already.
ACKs for top commit:
instagibbs:
reACK 1808b5aaf7
marcofleon:
code review ACK 1808b5aaf7
Tree-SHA512: 81cd9549de2968f5126079cbb532e2cb052ea8157c9c9ce37fd39ad2294105d7c79ee8d946c3d8f7af5b2119299a232c448b42a33e1e43ccc778a5b52957e387
5b7bf47f9b doc: p2p: replace last remaining "command" terminology with "message type" (Sebastian Falbesoner)
Pull request description:
This small PR is (presumably) the final one in a long series of replacing the confusing "command" terminology with "message type" when referring to the header field of P2P messages, see #18533, #18937, #24078, #24141 and #31163.
The instances were found manually via `$ git grep -i command`, hope I didn't miss any.
ACKs for top commit:
l0rinc:
ACK 5b7bf47f9b
billymcbip:
ACK 5b7bf47f9b
maflcko:
lgtm ACK 5b7bf47f9b
Tree-SHA512: b895873b82f904c2ee9a81b4a2fbb365b60c57f04587ded5ddc7907d209520acb6073f5dd1a19cb2ae6aadab3c85a5ac751c8c398ce7c0e29314eea54e61295c
fa65bc0e79 test: Run bench sanity checks in parallel with functional tests (MarcoFalke)
fa9fdbce79 test: Pass bench exe into test framework utils (MarcoFalke)
Pull request description:
The ctest target `bench_sanity_check` has many issues:
* With sanitizers enabled, it is one of the slowest targets, often taking several minutes. See https://github.com/bitcoin/bitcoin/issues/32770#issuecomment-2984264066.
* There is no insight from ctest into how long each individual sanity check takes.
* On a timeout, or OOM issue, there is no insight into which sub-bench failed. The failure will generally just look like `75/153 Test #9: bench_sanity_check ...................***Failed 770.84 sec out of memory`
* Places that can't use ctest (like the Windows-cross CI task) have to explicitly run it, or risk forgetting to run it.
* All benchmarks are run sequentially, when they could run in parallel instead.
Both issues can lead to CI timeouts and leave CPU unused during testing.
Fix all issues by running it as part of the functional tests instead. This is similar to the rpcauth tests (https://github.com/bitcoin/bitcoin/pull/32881) and util tests [bitcoin-tx, and bitcoin-util] (https://github.com/bitcoin/bitcoin/pull/32697).
ACKs for top commit:
achow101:
ACK fa65bc0e79
l0rinc:
Tested ACK fa65bc0e79
janb84:
tACK fa65bc0e79
willcl-ark:
ACK fa65bc0e79
Tree-SHA512: d27e363b7896a7543a4ee8df41a56e58b74f07d4f296e2e5ee293fc91817d0be310e26905755fb94d44417d94fa29ad4cc5d4aa19e78d25d41bc2d9e0948c034
4ce3f4a265 rpc, net: deprecate `startingheight` field of `getpeerinfo` RPC (Sebastian Falbesoner)
Pull request description:
This PR deprecates the "startingheight" result field of the `getpeerinfo` RPC, following the discussion in #33990.
Rationale: the reported starting height of a peer in the VERSION message is untrusted, and it doesn't seem to be useful anymore (after #20624), so deprecating the corresponding field seems reasonable. After that, it can be removed, along with the `m_starting_height` field of the Peer / CNodeStats structs, as it is sufficient to show the reported height only once at connection in the debug log.
ACKs for top commit:
optout21:
crACK 4ce3f4a265
achow101:
ACK 4ce3f4a265
fjahr:
utACK 4ce3f4a265
rkrux:
crACK 4ce3f4a265
janb84:
cr ACK 4ce3f4a265
Tree-SHA512: b296a28d30084fd35c67a2162e85576e3365e5d6fffe5b1add500034c1850604ee8c37b61afe812bfab8a7cc20f6a9e22db445e3c371311a5f82a777e5700ebf
5805a8b540 psbt: detect invalid MuSig2 pubkeys in deserialization (rkrux)
Pull request description:
Throw error while deserializing PSBT if invalid pubkeys are passed
as a MuSig2 aggregate or participant.
Should fix#33999 & #34201 by throwing error at the very start while decoding
an invalid PSBT that should subsequently not allow the MuSig2
signing operation to take place, thereby avoiding the crash.
ACKs for top commit:
fjahr:
utACK 5805a8b540
achow101:
ACK 5805a8b540
Tree-SHA512: 4741db96b278e9f3d532e1873af9530a70bbc7a8d3625b9e1c07001acc472fc10cbb79995c16bc4d06cc568ef98fe8d2b8e8d87b617dc05d7554085ffb92dfef
With the new SFL algorithm, the process of loading an existing linearization into the
SFL state is very similar to what PostLinearize does. This means there is little benefit
to performing an explicit PostLinearize step before linearizing inside txgraph. Instead,
it seems better to use our allotted CPU time to perform more SFL optimization steps.
With the SFL algorithm, we will practically be capable of keeping
most if not all clusters optimal. With that, it seems less valuable
to avoid doing work after splitting an acceptable cluster, because by
doing some work we may get it to OPTIMAL.
This reduces the complexity of the code a bit as well.
The reported starting height of a peer in the VERSION message is
untrusted, and it doesn't seem to be useful anymore (after #20624),
so deprecating the corresponding "startingheight" field seems
reasonable. After that, it can be removed, along with the
`m_starting_height` field of the Peer / CNodeStats structs, as it is
sufficient to show the reported height only once at connection in the
debug log.
6da6f503a6 refactor: Let CCoinsViewCache::BatchWrite return void (TheCharlatan)
Pull request description:
CCoinsViewCache::BatchWrite always returns true if called from a backed cache, so just return void instead. Also return void from ::Sync and ::Flush.
This allows for dropping a FatalError condition and simplifying some dead error handling code a bit.
Since we now no longer exercise the "error path" when returning from `CCoinsView::BatchWrite`, make the method clear the cache instead. This should only be exercised by tests and not change production behaviour. This might slightly improve the coins_view fuzz test's ability to generate better coverage.
ACKs for top commit:
l0rinc:
ACK 6da6f503a6
andrewtoth:
re-ACK 6da6f503a6
achow101:
ACK 6da6f503a6
w0xlt:
ACK 6da6f503a6
Tree-SHA512: dfaa325b0cf8108910aebf1b27434aaddb639d10d860e96797c77ea42eca9035a54a7dc1d6a5d4eae2b75fcc9356206d3d5672243d2c906e80d19024c8b95408
76c092ff80 wallet: warn against accidental unsafe older() import (Sjors Provoost)
592157b759 test: move SEQUENCE_LOCKTIME flags to script (Sjors Provoost)
Pull request description:
[BIP 379](https://github.com/bitcoin/bips/blob/master/bip-0379.md) ([Miniscript](https://bitcoin.sipa.be/miniscript/)) allows relative height and time locks that have no consensus meaning in [BIP 68](https://github.com/bitcoin/bips/blob/master/bip-0068.mediawiki) (relative timelocks) / [BIP 112](https://github.com/bitcoin/bips/blob/master/bip-0112.mediawiki) (`CHECKSEQUENCEVERIFY`). This is (ab)used by some protocols, e.g. [by Lightning to encode extra data](https://delvingbitcoin.org/t/exploring-extended-relative-timelocks/1818/23), but is unsafe when used unintentionally: `older(65536)` is equivalent to `older(1)`.
This PR emits a warning when `importdescriptors` contains such a descriptor.
The first commit makes `SEQUENCE_LOCKTIME` flags reusable by other tests.
The main commit adds the `ForEachNode` helper to `miniscript.h` which is then used in the `MiniscriptDescriptor` constructor to check for `Fragment::OLDER` with unsafe values. These are stored in `m_warnings`, which the RPC code then collects via `Warnings()`.
It adds both a unit and functional test.
---
A previous version of this PR prevented the import, unless the user opted in with an `unsafe` flag. It also used string parsing in the RPC code.
---
Based on:
- [x] https://github.com/bitcoin/bitcoin/pull/33914
ACKs for top commit:
pythcoiner:
reACK 76c092ff80
achow101:
ACK 76c092ff80
rkrux:
lgtm re-ACK 76c092ff80
brunoerg:
reACK 76c092ff80
Tree-SHA512: 8e944e499bd4a43cc27eeb889f262b499b9b07aa07610f4a415ccb4e34a9110f9946646f446a54ac5bf17494d8d96a89e4a1fa278385db9b950468f27283e17a
658d38106a policy: remove constant parameter from `IsWellFormedPackage` (Lőrinc)
Pull request description:
`IsWellFormedPackage()` already claims: "parents must appear before children." In practice the `require_sorted` argument was always passed as `true`, making the false-path dead code. It was introduced that way from the beginning in https://github.com/bitcoin/bitcoin/pull/28758/files#diff-f30090b30c9489972ee3f1181c302cf3a484bb890bade0fd7c9ca92ea8d347f6R79.
Remove the unused parameter, updating callers/tests.
ACKs for top commit:
billymcbip:
tACK 658d38106a
instagibbs:
ACK 658d38106a
Tree-SHA512: 8b86dda7e2e1f0d48947ff258f0a3b6ec60676f54d4b506604d24e15c8b6465358ed2ccf174c7620125f5cad6bfc4df0bc482d920e5fc4cd0e1d72a9b16eafa5
fab1f4b800 rpc: [mempool] Remove erroneous Univalue integral casts (MarcoFalke)
Pull request description:
Casting without reason can only be confusing (because it is not needed), or wrong (because it does the wrong thing).
For example, the added test that adds a positive chunk prioritization will fail:
```
AssertionError: not(-1.94936096 == 41.000312)
```
Fix all issues by removing the erroneous casts, and by adding a test to check against regressions.
ACKs for top commit:
rkrux:
tACK fab1f4b800
pablomartin4btc:
ACK fab1f4b800
glozow:
ACK fab1f4b800
Tree-SHA512: b03c888ec07a8bdff25f7ded67f253b2a8edd83adf08980416e2ac8ac1b36ad952cc5828be833d19f64a55abab62d7a1c6f181bc5f1388ed08cc178b4aaec6ee
fa66e2d07a refactor: [rpc] Remove confusing and brittle integral casts (MarcoFalke)
Pull request description:
When constructing an UniValue from integral values, historically (long ago), in some cases casts where needed. With the current UniValue constructor, only very few are actually needed.
Keeping the unused casts around is:
* confusing, because code readers do not understand why they are needed
* brittle, because some may copy them into new places, where they will lead to hard-to-find logic bugs, such as the ones fixed in pull https://github.com/bitcoin/bitcoin/pull/34112
So fix all issues by removing them, except for a few cases, where casting was required:
* `ret.pushKV("coinbase", static_cast<bool>(coin->fCoinBase));`, or
* `static_cast<std::underlying_type_t<decltype(info.nServices)>>(info.nServices)`.
This hardening refactor does not fix any bugs and does not change any behavior.
ACKs for top commit:
sedited:
ACK fa66e2d07a
rkrux:
ACK fa66e2d07a
Tree-SHA512: 13c9c59ad021ea03cdabe10d58850cef96d792634c499e62227cc2e7e5cace066ebd9a8ef3f979eaba98cadf8a525c6e6df909a07115559c0450bd9fc3a9763e
44e006d438 [kernel] Expose reusable PrecomputedTransactionData in script valid (Josh Doman)
Pull request description:
This PR exposes a reusable `PrecomputedTransactionData` object in script validation using libkernel.
Currently, libkernel computes `PrecomputedTransactionData` each time `btck_script_pubkey_verify` is called, exposing clients to quadratic hashing when validating a transaction with multiple inputs. By externalizing `PrecomputedTransactionData` and making it reusable, libkernel can eliminate this attack vector.
I discussed this problem in [this issue](https://github.com/TheCharlatan/rust-bitcoinkernel/issues/46). The design of this PR is inspired by @sedited's comments.
The PR introduces three new APIs for managing the `btck_PrecomputedTransactionData` object:
```c
/**
* @brief Create precomputed transaction data for script verification.
*
* @param[in] tx_to Non-null.
* @param[in] spent_outputs Nullable for non-taproot verification. Points to an array of
* outputs spent by the transaction.
* @param[in] spent_outputs_len Length of the spent_outputs array.
* @return The precomputed data, or null on error.
*/
btck_PrecomputedTransactionData* btck_precomputed_transaction_data_create(
const btck_Transaction* tx_to,
const btck_TransactionOutput** spent_outputs, size_t spent_outputs_len) BITCOINKERNEL_ARG_NONNULL(1);
/**
* @brief Copy precomputed transaction data.
*
* @param[in] precomputed_txdata Non-null.
* @return The copied precomputed transaction data.
*/
btck_PrecomputedTransactionData* btck_precomputed_transaction_data_copy(
const btck_PrecomputedTransactionData* precomputed_txdata) BITCOINKERNEL_ARG_NONNULL(1);
/**
* Destroy the precomputed transaction data.
*/
void btck_precomputed_transaction_data_destroy(btck_PrecomputedTransactionData* precomputed_txdata);
```
The PR also modifies `btck_script_pubkey_verify` so that it accepts `precomputed_txdata` instead of `spent_outputs`:
```c
/**
* @brief Verify if the input at input_index of tx_to spends the script pubkey
* under the constraints specified by flags. If the
* `btck_ScriptVerificationFlags_WITNESS` flag is set in the flags bitfield, the
* amount parameter is used. If the taproot flag is set, the precomputed data
* must contain the spent outputs.
*
* @param[in] script_pubkey Non-null, script pubkey to be spent.
* @param[in] amount Amount of the script pubkey's associated output. May be zero if
* the witness flag is not set.
* @param[in] tx_to Non-null, transaction spending the script_pubkey.
* @param[in] precomputed_txdata Nullable if the taproot flag is not set. Otherwise, precomputed data
* for tx_to with the spent outputs must be provided.
* @param[in] input_index Index of the input in tx_to spending the script_pubkey.
* @param[in] flags Bitfield of btck_ScriptVerificationFlags controlling validation constraints.
* @param[out] status Nullable, will be set to an error code if the operation fails, or OK otherwise.
* @return 1 if the script is valid, 0 otherwise.
*/
int btck_script_pubkey_verify(
const btck_ScriptPubkey* script_pubkey,
int64_t amount,
const btck_Transaction* tx_to,
const btck_PrecomputedTransactionData* precomputed_txdata,
unsigned int input_index,
btck_ScriptVerificationFlags flags,
btck_ScriptVerifyStatus* status) BITCOINKERNEL_ARG_NONNULL(1, 3);
```
As before, an error is thrown if the taproot flag is set and `spent_outputs` is not provided in `precomputed_txdata` (or `precomputed_txdata` is null). For simple single-input non-taproot verification, `precomputed_txdata` may be null, and the kernel will construct the precomputed data on-the-fly.
Both the C++ wrapper and the test suite are updated with the new API. Tests cover both `precomputed_txdata` reuse and nullability.
Appreciate feedback on this concept / approach!
ACKs for top commit:
sedited:
Re-ACK 44e006d438
stringintech:
ACK 44e006d
Tree-SHA512: 1ed435173e6ff4ec82bc603194cf182c685cb79f167439a442b9b179a32f6c189c358f04d4cb56d153fab04e3424a11b73c31680e42b87b8a6efcc3ccefc366c
5646e6c0d3 index: restrict index helper function to namespace (Martin Zumsande)
032f3503e3 index, refactor: deduplicate LookUpOne (Martin Zumsande)
a67d3eb91d index: deduplicate Hash / Height handling (Martin Zumsande)
Pull request description:
The logic for `DBHashKey` / `DBHeightKey` handling and lookup of entries is shared by `coinstatsindex` and `blockfilterindex`, leading to many lines of duplicated code. De-duplicate this by moving the logic to `index/db_key.h` (using templates for the index-specific `DBVal`).
ACKs for top commit:
fjahr:
re-ACK 5646e6c0d3
furszy:
utACK 5646e6c0d3
sedited:
ACK 5646e6c0d3
Tree-SHA512: 6f41684d6a9fd9bb01239e9f2e39a12837554f247a677eadcc242f0c1a2d44a79979f87249c4e0305ef1aa708d7056e56dfc40e1509c6d6aec2714f202fd2e09
e44dec027c add release note about supporing non-TRUC <minrelay txns (Greg Sanders)
1488315d76 policy: Allow any transaction version with < minrelay (Greg Sanders)
Pull request description:
Prior to cluster mempool, a policy was in place that
disallowed non-TRUC transactions from being
TX_RECONSIDERABLE in a package setting if it was below
minrelay. This was meant to simplify reasoning about mempool
trimming requirements with non-trivial transaction
topologies in the mempool. This is no longer a concern
post-cluster mempool, so this is relaxed.
In effect, this makes 0-value parent transactions relayable
through the network without the TRUC restrictions and
thus the anti-pinning protections.
ACKs for top commit:
ajtowns:
ACK e44dec027c - lgtm
ismaelsadeeq:
ACK e44dec027c
Tree-SHA512: 6fd1a2429c55ca844d9bd669ea797e29eca3f544f0b5d3484743d3c1cdf4364f7c7a058aaf707bcfd94b84c621bea03228cb39487cbc23912b9e0980a1e5b451
This function is a duplicate of HasEncryptionKeys().
-BEGIN VERIFY SCRIPT-
sed -i '/bool IsCrypted() const;/d' src/wallet/wallet.h
sed -i '/^bool CWallet::IsCrypted() const$/,/^}$/{/^}$/N;d;}' src/wallet/wallet.cpp
sed -i --regexp-extended 's/IsCrypted\(\)/HasEncryptionKeys()/g' $(git ls-files '*.cpp' '*.h')
-END VERIFY SCRIPT-
217dbbbb5e test: Add musig failure scenarios (Fabian Jahr)
c9519c260b musig: Check session id reuse (Fabian Jahr)
e755614be5 sign: Remove duplicate sigversion check (Fabian Jahr)
0f7f0692ca musig: Move MUSIG_CHAINCODE to musig.cpp (Fabian Jahr)
Pull request description:
This is a follow-up to #29675 and primarily adds test coverage for some of the most prominent failure cases in the last commit.
The following commits address a few left-over nit comments that didn't make it in before merge.
ACKs for top commit:
achow101:
ACK 217dbbbb5e
rkrux:
lgtm ACK 217dbbb
Tree-SHA512: d73807bc31791ef1825c42f127c7ddfbc70b2b7cf782bc11341666e32e86b787ffc7aed64caea992909cef3a85fc6629282d8209c173aadec77f72fd0da96c45
1ed8e76165 rpc, doc: clarify the response of listtransactions RPC (rkrux)
Pull request description:
I noticed this behaviour while perf testing PR #27286 and it was not something that I expected, updating the doc to make it present in the RPCHelp command.
ACKs for top commit:
achow101:
ACK 1ed8e76165
furszy:
ACK 1ed8e76165
musaHaruna:
ACK [1ed8e76](1ed8e76165) since my last review. New changes looks good, it's much easier to understand as well, looking at it from a user's perspective.
Tree-SHA512: 893a8e259201ac2140f46f827d81e681d2ec478c9571cceb10864aaa1b941991ce2263357d7c2b0024c04a9f8fbc372a020104b26e022c96289d271675947033
56750c4f87 iwyu, clang-format: Sort includes (Hennadii Stepanov)
2c78814e0e ci: Add IWYU job (Hennadii Stepanov)
94e4f04d7c cmake: Fix target name (Hennadii Stepanov)
0f81e00519 cmake: Make `codegen` target dependent on `generate_build_info` (Hennadii Stepanov)
73f7844cdb iwyu: Add patch to prefer C++ headers over C counterparts (Hennadii Stepanov)
7a65437e23 iwyu: Add patch to prefer angled brackets over quotes for includes (Hennadii Stepanov)
Pull request description:
This PR separates the IWYU checks into its own CI job to provide faster feedback to developers. No other changes are made to the treatment of IWYU warnings. The existing “tidy” CI job will no longer run IWYU.
See also the discussion of https://github.com/bitcoin/bitcoin/pull/33779, specifically this [comment](https://github.com/bitcoin/bitcoin/pull/33779#issuecomment-3491515263):
> Maybe a better approach would be to run the enforced sections in a separate, faster job? Some of the linters are already a bit annoying to invoke locally, so I usually just run the lint job. Doing the same for the includes seems fine to me.
Based on ideas from https://github.com/bitcoin/bitcoin/pull/32953.
ACKs for top commit:
maflcko:
review ACK 56750c4f87🌄
sedited:
ACK 56750c4f87
Tree-SHA512: af15326b6d0c5d1e11346ac64939644936c65eb9466cd1a17ab5da347d39aef10f7ab33b39fbca31ad291b0b4b54639b147b24410f4f86197e4a776049882694
d7de5b109f logs: show reindex progress in `ImportBlocks` (Lőrinc)
Pull request description:
### Summary
When triggering a reindex, users have no indication of progress.
### Fix
This patch precomputes the total number of block files so progress can be shown.
Instead of only displaying which block file is being processed, it now shows the percent complete.
### Reproducer + expected results
```bash
cmake -B build -DCMAKE_BUILD_TYPE=Release && make -C build -j && ./build/bin/bitcoind -datadir=demo -reindex
```
Before, the block files were shown one-by-one, there's no way to see how much work is left:
```
Reindexing block file blk00000.dat...
Loaded 119920 blocks from external file in 1228ms
Reindexing block file blk00001.dat...
Loaded 10671 blocks from external file in 284ms
Reindexing block file blk00002.dat...
Loaded 5459 blocks from external file in 263ms
Reindexing block file blk00003.dat...
Loaded 5595 blocks from external file in 267ms
```
After the change we add a percentage:
```
Reindexing block file blk00000.dat (0% complete)...
Loaded 119920 blocks from external file in 1255ms
Reindexing block file blk00001.dat (1% complete)...
Loaded 10671 blocks from external file in 303ms
Reindexing block file blk00002.dat (2% complete)...
Loaded 5459 blocks from external file in 278ms
Reindexing block file blk00003.dat (3% complete)...
Loaded 5595 blocks from external file in 285ms
```
ACKs for top commit:
enirox001:
Concept ACK d7de5b1
rkrux:
lgtm ACK d7de5b109f
danielabrozzoni:
tACK d7de5b109f - code reviewed and tested on my archival node.
maflcko:
review ACK d7de5b109f💇
Tree-SHA512: 359a539b781ad8b73e2a616c951567062a76be27cf90e5b88bb5309295af9cd7994e327f185bacc1482b43b892b38329593b4043a5e71d8800e3e4b7a3954310
This should avoid having to include interfaces/chain.h from a kernel
module. interfaces/chain.h in turn includes a bunch of non-kernel
headers, that break the desired library topology and might introduce
entanglement regressions.
Specifically gets rid of batchpriority, chainparams, script/sign.h and
system includes.
Also take the opportunity of cleaning up the headers for the effected
files and adding them to the iwyu-enforced set.
fa4cb13b52 test: [doc] Manually unify stale headers (MarcoFalke)
fa5f297748 scripted-diff: [doc] Unify stale copyright headers (MarcoFalke)
Pull request description:
Historically, the upper year range in file headers was bumped manually
or with a script.
This has many issues:
* The script is causing churn. See for example commit 306ccd4, or
drive-by first-time contributions bumping them one-by-one. (A few from
this year: https://github.com/bitcoin/bitcoin/pull/32008,
https://github.com/bitcoin/bitcoin/pull/31642,
https://github.com/bitcoin/bitcoin/pull/32963, ...)
* Some, or likely most, upper year values were wrong. Reasons for
incorrect dates could be code moves, cherry-picks, or simply bugs in
the script.
* The upper range is not needed for anything.
* Anyone who wants to find the initial file creation date, or file
history, can use `git log` or `git blame` to get more accurate
results.
* Many places are already using the `-present` suffix, with the meaning
that the upper range is omitted.
To fix all issues, this bumps the upper range of the copyright headers
to `-present`.
Further notes:
* Obviously, the yearly 4-line bump commit for the build system (c.f.
b537a2c02a) is fine and will remain.
* For new code, the date range can be fully omitted, as it is done
already by some developers. Obviously, developers are free to pick
whatever style they want. One can list the commits for each style.
* For example, to list all commits that use `-present`:
`git log --format='%an (%ae) [%h: %s]' -S 'present The Bitcoin'`.
* Alternatively, to list all commits that use no range at all:
`git log --format='%an (%ae) [%h: %s]' -S '(c) The Bitcoin'`.
<!--
* The lower range can be wrong as well, so it could be omitted as well,
but this is left for a follow-up. A previous attempt was in
https://github.com/bitcoin/bitcoin/pull/26817.
ACKs for top commit:
l0rinc:
ACK fa4cb13b52
rkrux:
re-ACK fa4cb13b52
janb84:
ACK fa4cb13b52
Tree-SHA512: e5132781bdc4417d1e2922809b27ef4cf0abb37ffb68c65aab8a5391d3c917b61a18928ec2ec2c75ef5184cb79a5b8c8290d63e949220dbeab3bd2c0dfbdc4c5
