fa4cb13b52030c2e55c6bea170649ab69d75f758 test: [doc] Manually unify stale headers (MarcoFalke)
fa5f29774872d18febc0df38831a6e45f3de69cc 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.
b537a2c02a9921235d1ecf8c3c7dc1836ec68131) 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 fa4cb13b52030c2e55c6bea170649ab69d75f758
rkrux:
re-ACK fa4cb13b52030c2e55c6bea170649ab69d75f758
janb84:
ACK fa4cb13b52030c2e55c6bea170649ab69d75f758
Tree-SHA512: e5132781bdc4417d1e2922809b27ef4cf0abb37ffb68c65aab8a5391d3c917b61a18928ec2ec2c75ef5184cb79a5b8c8290d63e949220dbeab3bd2c0dfbdc4c5
d9319b06cf82664d55f255387a348135fd7f91c7 refactor: unify container presence checks - non-trivial counts (Lőrinc)
039307554eb311ce41648d1f9a12b543f480f871 refactor: unify container presence checks - trivial counts (Lőrinc)
8bb9219b6301215f53e43967d17445aaf1b81090 refactor: unify container presence checks - find (Lőrinc)
Pull request description:
### Summary
Instead of counting occurrences in sets and maps, the C++20 `::contains` method expresses the intent unambiguously and can return early on first encounter.
### Context
Applied clang‑tidy's [readability‑container‑contains](https://clang.llvm.org/extra/clang-tidy/checks/readability/container-contains.html) check, though many cases required manual changes since tidy couldn't fix them automatically.
### Changes
The changes made here were:
| From | To |
|------------------------|------------------|
| `m.find(k) == m.end()` | `!m.contains(k)` |
| `m.find(k) != m.end()` | `m.contains(k)` |
| `m.count(k)` | `m.contains(k)` |
| `!m.count(k)` | `!m.contains(k)` |
| `m.count(k) == 0` | `!m.contains(k)` |
| `m.count(k) != 1` | `!m.contains(k)` |
| `m.count(k) == 1` | `m.contains(k)` |
| `m.count(k) < 1` | `!m.contains(k)` |
| `m.count(k) > 0` | `m.contains(k)` |
| `m.count(k) != 0` | `m.contains(k)` |
> Note that `== 1`/`!= 1`/`< 1` only apply to simple [maps](https://en.cppreference.com/w/cpp/container/map/contains)/[sets](https://en.cppreference.com/w/cpp/container/set/contains) and had to be changed manually.
There are many other cases that could have been changed, but we've reverted most of those to reduce conflict with other open PRs.
-----
<details>
<summary>clang-tidy command on Mac</summary>
```bash
rm -rfd build && \
cmake -B build \
-DCMAKE_C_COMPILER="$(brew --prefix llvm)/bin/clang" \
-DCMAKE_CXX_COMPILER="$(brew --prefix llvm)/bin/clang++" \
-DCMAKE_OSX_SYSROOT="$(xcrun --show-sdk-path)" \
-DCMAKE_C_FLAGS="-target arm64-apple-macos11" \
-DCMAKE_CXX_FLAGS="-target arm64-apple-macos11" \
-DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DBUILD_BENCH=ON -DBUILD_FUZZ_BINARY=ON -DBUILD_FOR_FUZZING=ON
"$(brew --prefix llvm)/bin/run-clang-tidy" -quiet -p build -j$(nproc) -checks='-*,readability-container-contains' | grep -v 'clang-tidy'
```
</details>
Note: this is a take 2 of https://github.com/bitcoin/bitcoin/pull/33094 with fewer contentious changes.
ACKs for top commit:
optout21:
reACK d9319b06cf82664d55f255387a348135fd7f91c7
sedited:
ACK d9319b06cf82664d55f255387a348135fd7f91c7
janb84:
re ACK d9319b06cf82664d55f255387a348135fd7f91c7
pablomartin4btc:
re-ACK d9319b06cf82664d55f255387a348135fd7f91c7
ryanofsky:
Code review ACK d9319b06cf82664d55f255387a348135fd7f91c7. I manually reviewed the full change, and it seems there are a lot of positive comments about this and no more very significant conflicts, so I will merge it shortly.
Tree-SHA512: e4415221676cfb88413ccc446e5f4369df7a55b6642347277667b973f515c3c8ee5bfa9ee0022479c8de945c89fbc9ff61bd8ba086e70f30298cbc1762610fe1
dcd42d6d8f160ae8bc12c152099a6e6473658e30 [test] wallet send 3 generation TRUC (glozow)
e753fadfd01cb8a4a8de3bddc7391481551cca89 [wallet] never try to spend from unconfirmed TRUC that already has ancestors (glozow)
Pull request description:
Addresses https://github.com/bitcoin/bitcoin/issues/33368#issuecomment-3319935660
There is not an explicit check that the to-be-created wallet transaction would be within the {TRUC, normal} ancestor limits. This means that the wallet may create a transaction that violates these limits, but fail to broadcast it in `CommitTransaction`.
This appears to be expected behavior for the normal ancestor limits (and any other situation in which the wallet creates a tx that was rejected by mempool) and AFAIK the transaction will be rebroadcast at some point after the ancestors confirm.
1ed00a0d39/test/functional/wallet_basic.py (L502-L506)
It's a bit complex to address this for the normal ancestor limit, and probably unrealistic for the wallet to check all possible mempool policies in coin selection, but it's quite trivial for TRUC: just skip any unconfirmed UTXOs that have any ancestors. I think it would be much more helpful to the user to say there are insufficient funds.
ACKs for top commit:
achow101:
ACK dcd42d6d8f160ae8bc12c152099a6e6473658e30
monlovesmango:
ACK dcd42d6d8f160ae8bc12c152099a6e6473658e30
rkrux:
lgtm ACK dcd42d6d8f160ae8bc12c152099a6e6473658e30
Tree-SHA512: b4cf9685bf0593c356dc0d6644835d53e3d7089f42b65f647795257dc7f5dac90c5ee493b41ee30a1c1beb880a859db8e049d3c64a43d5ca9b3e6482ff6bddd5
The changes made here were:
| From | To |
|-------------------|------------------|
| `m.count(k)` | `m.contains(k)` |
| `!m.count(k)` | `!m.contains(k)` |
| `m.count(k) == 0` | `!m.contains(k)` |
| `m.count(k) != 0` | `m.contains(k)` |
| `m.count(k) > 0` | `m.contains(k)` |
The commit contains the trivial, mechanical refactors where it doesn't matter if the container can have multiple elements or not
Co-authored-by: Jan B <608446+janb84@users.noreply.github.com>
The values are small enough to fit in size_t, but to avoid having to
think about it, just use uint64_t consistently for all architectures.
On 64-bit systems, this refactor is a no-op. On 32-bit systems, it could
avoid bugs in the theoretical and unexpected case where a 32-bit size_t
is too small and overflows.
Since the only remaining isminetypes are ISMINE_NO and ISMINE_SPENDABLE,
this enum is now just a bool and can be removed. IsMine is changed to
return a bool and any usage of isminetypes and isminefilters are changed
to be the remaining ISMINE_SPENDABLE case.
In descriptor wallets, we consider all outputs to be spendable as we no
longer have mixed mine and watchonly in a wallet. As such,
COutput::spendable is meaningless and can be removed.
Furthermore, CoinFilterParams::only_spendable can be removed as that was
essentially checking for COutput::spendable.
Lastly, AvailableCoinsListUnspent can also be removed as the wrapper is
now only setting the feerate to std::nullopt which is trivial enough that
a dedicated wrapper is not needed.
In future commits we assume that coin_control.m_version has a
value when making sure that we follow truc rules, so we should
give it a default value of CTransaction::CURRENT_VERSION.
de0675f9de5feae1f070840ad7218b1378fb880b refactor: Move `transaction_identifier.h` to primitives (marcofleon)
6f068f65de17951dc459bc8637e5de15b84ca445 Remove implicit uint256 conversion and comparison (marcofleon)
9c24cda72edb2085edfa75296d6b42fab34433d9 refactor: Convert remaining instances from uint256 to Txid (marcofleon)
d2ecd6815d89c9b089b55bc96fdf93b023be8dda policy, refactor: Convert uint256 to Txid (marcofleon)
f6c0d1d23128f742dfdda253752cba7db9bb0679 mempool, refactor: Convert uint256 to Txid (marcofleon)
aeb0f783305c923ee7667c46ca0ff7e1b96ed45c refactor: Convert `mini_miner` from uint256 to Txid (marcofleon)
326f24472487dc7f447839136db2ccf60833e9a2 refactor: Convert RPCs and `merkleblock` from uint256 to Txid (marcofleon)
49b3d3a92a7250e80c56ff8c351cf1670e32c1a2 Clean up `FindTxForGetData` (marcofleon)
Pull request description:
This is the final leg of the [type safety refactor](https://github.com/bitcoin/bitcoin/pull/32189).
All of these changes are straightforward `uint256` --> `Txid` along with any necessary explicit conversions. Also, `transaction_identifier.h` is moved to primitives in the last commit, as `Txid` and `Wtxid` become fundamental types after this PR.
ACKs for top commit:
stickies-v:
re-ACK de0675f9de5feae1f070840ad7218b1378fb880b, no changes since a20724d926d5844168c6a13fa8293df8c8927efe except address review nits.
janb84:
re ACK de0675f9de5feae1f070840ad7218b1378fb880b
dergoegge:
re-ACK de0675f9de5feae1f070840ad7218b1378fb880b
theStack:
Code-review ACK de0675f9de5feae1f070840ad7218b1378fb880b
Tree-SHA512: 2413160fca7ab146a8d79d18ce3afcf7384cacc73c513d41928904aa453b4dd7a350064cee71e9c5d015da5904c7c81ac17603e50a47441ebc5b0c653235dd08
Moves the file from `src/util` to `src/primitives`. Now that the
refactor is complete, Txid and Wtxid are fundamental types, so it
makes sense for them to reside in `src/primitives`.
These remaining miscellaneous changes were identified by commenting out
the `operator const uint256&` conversion and the `Compare(const uint256&)`
method from `transaction_identifier.h`.
aac0b6dd79b0db1e9d42a6f466709a61cfd1f69f test: test sendall and send do anti-fee-sniping (ishaanam)
20802c7b65f4b196a3ed0aca876974153db55d9d wallet, rpc: add anti-fee-sniping to `send` and `sendall` (ishaanam)
Pull request description:
Currently, `send` and `sendall` don't do anti-fee-sniping because they don't use `CreateTransaction`. This PR adds anti-fee-sniping to these RPCs by calling `DiscourageFeeSniping` from `FinishTransaction` when the user does not specify a locktime.
ACKs for top commit:
achow101:
ACK aac0b6dd79b0db1e9d42a6f466709a61cfd1f69f
murchandamus:
ACK aac0b6dd79b0db1e9d42a6f466709a61cfd1f69f
glozow:
ACK aac0b6dd79b0db1e9d42a6f466709a61cfd1f69f
Tree-SHA512: d4f1b43b5bda489bdba46b0af60e50bff0de604a35670e6ea6e1de2b539f16b3f68805492f51d6d2078d421b63432ca22a561a5721d1a37686f2e48284e1e646
6135e0553e6e58fcf506700991fa178f2c50a266 wallet, rpc: Move (Un)LockCoin WalletBatch creation out of RPC (Ava Chow)
Pull request description:
If the locked coin needs to be persisted to the wallet database, insteead of having the RPC figure out when to create a WalletBatch and having LockCoin's behavior depend on it, have LockCoin take whether to persist as a parameter so it makes the batch.
Since unlocking a persisted locked coin requires a database write as well, we need to track whether the locked coin was persisted to the wallet database so that it can erase the locked coin when necessary.
Keeping track of whether a locked coin was persisted is also useful information for future PRs.
Split from #32489
ACKs for top commit:
rkrux:
ACK 6135e05
Sjors:
ACK 6135e0553e6e58fcf506700991fa178f2c50a266
w0xlt:
ACK 6135e0553e
Tree-SHA512: 0e2367fc4d50c62ec41443374b64c4c5ecf679998677df47fb8776cfb44704713bc45547e32e96cd30d1dbed766f5d333efb6f10eb0e71271606638e07e61a01
Instead of searching mapWallet for the preselected inputs, search
m_txos.
wallet_fundrawtransaction.py spends external inputs and needs the change
output to also belong to the test wallet for the oversized tx test.
Instead of iterating every transaction and every output stored in wallet
when trying to figure out what outputs can be spent, iterate the TXO set
which should be a lot smaller.
If the locked coin needs to be persisted to the wallet database,
insteead of having the RPC figure out when to create a WalletBatch and
having LockCoin's behavior depend on it, have LockCoin take whether to
persist as a parameter so it makes the batch.
Since unlocking a persisted locked coin requires a database write as
well, we need to track whether the locked coin was persisted to the
wallet database so that it can erase the locked coin when necessary.
Keeping track of whether a locked coin was persisted is also useful
information for future PRs.
Transifex interprets the "-0" substring as a number in translatable
strings. Since not all translations preserve "-0," this triggers a
corresponding warning. While this warning could be disabled globally, it
is more reasonable to adjust the original string instead.
Before this commit, we would always prepare tracepoint arguments
regardless of the tracepoint being used or not. While we already made
sure not to include expensive arguments in our tracepoints, this
commit introduces gating to make sure the arguments are only prepared
if the tracepoints are actually used. This is a win-win improvement
to our tracing framework. For users not interested in tracing, the
overhead is reduced to a cheap 'greater than 0' compare. As the
semaphore-gating technique used here is available in bpftrace, bcc,
and libbpf, users interested in tracing don't have to change their
tracing scripts while profiting from potential future tracepoints
passing slightly more expensive arguments. An example are mempool
tracepoints that pass serialized transactions. We've avoided the
serialization in the past as it was too expensive.
Under the hood, the semaphore-gating works by placing a 2-byte
semaphore in the '.probes' ELF section. The address of the semaphore
is contained in the ELF note providing the tracepoint information
(`readelf -n ./src/bitcoind | grep NT_STAPSDT`). Tracing toolkits
like bpftrace, bcc, and libbpf increase the semaphore at the address
upon attaching to the tracepoint. We only prepare the arguments and
reach the tracepoint if the semaphore is greater than zero. The
semaphore is decreased when detaching from the tracepoint.
This also extends the "Adding a new tracepoint" documentation to
include information about the semaphores and updated step-by-step
instructions on how to add a new tracepoint.
ec585f11c38bbe277a596dcea3c813e7c6626050 Reserve space for transaction inputs in CreateTransactionInternal (Lőrinc)
c76aaaf90034a15917d02a71e3fdc36e8dd927f6 Reserve space for transaction outputs in CreateTransactionInternal (Lőrinc)
Pull request description:
Reserved memory for the transaction inputs and outputs.
Split out of https://github.com/bitcoin/bitcoin/pull/30050/files#r1597631104
ACKs for top commit:
achow101:
ACK ec585f11c38bbe277a596dcea3c813e7c6626050
TheCharlatan:
ACK ec585f11c38bbe277a596dcea3c813e7c6626050
stickies-v:
ACK ec585f11c38bbe277a596dcea3c813e7c6626050
Tree-SHA512: de399fb19824423467f48af64aa57f41a23cdd00eb17461e0131e4deafdd15e0d2daebf6a0a7ac7728b2fb486b2a54f1a7ef26bbe823c56b2a09f892f6b9a581
734076c6de1781f957c8bc3bf7ed6951920cfcf6 [wallet, rpc]: add `max_tx_weight` to tx funding options (ismaelsadeeq)
b6fc5043c16c2467a2a6768a6ca9b18035fc400f [wallet]: update the data type of `change_output_size`, `change_spend_size` and `tx_noinputs_size` to `int` (ismaelsadeeq)
baab0d2d43049a71dc90176bc4d72062f7b2ce19 [doc]: update reason for deducting change output weight (ismaelsadeeq)
7f61d31a5cec8fc61328bee43f90d3f1dcb0a035 [refactor]: update coin selection algorithms input parameter `max_weight` name (ismaelsadeeq)
Pull request description:
This PR taken over from #29264
The PR added an option `max_tx_weight` to transaction funding RPC's that ensures the resulting transaction weight does not exceed the specified `max_tx_weight` limit.
If `max_tx_weight` is not given `MAX_STANDARD_TX_WEIGHT` is used as the max threshold.
This PR addressed outstanding review comments in #29264
For more context and rationale behind this PR see https://delvingbitcoin.org/t/lightning-transactions-with-v3-and-ephemeral-anchors/418/11?u=instagibbs
ACKs for top commit:
achow101:
ACK 734076c6de1781f957c8bc3bf7ed6951920cfcf6
furszy:
utACK 734076c6de1781f957c8bc3bf7ed6951920cfcf6
rkrux:
reACK [734076c](734076c6de)
Tree-SHA512: 013501aa443d239ee2ac01bccfc5296490c27b4edebe5cfca6b96c842375e895e5cfeb5424e82e359be581460f8be92095855763a62779a18ccd5bdfdd7ddce7
a9c7300135f0188daa5bce5491e2daf2dd8da8ae move-only: refactor CreateTransactionInternal (josibake)
adc6ab25bba42ce9e7ed6bd7599aabb6dead6987 wallet: use CRecipient instead of CTxOut (josibake)
Pull request description:
Broken out from #28201
---
In order to estimate fees properly, we need to know what the final serialized transaction size will be. This PR refactors `CreateTransactionInternal` to:
* Get the serialized size directly from the `CRecipient`: this sets us up in a future PR to calculate the serialized size of silent payment `CTxDestinations` (see 797e21c8c1)
* Use the new `GetSerializeSizeForRecipient` to move the serialize size calculation to *before* coin selection and the output creation to *after* coin selection: this also sets us up for silent payments sending in a future PR in that silent payments outputs cannot be created until after the inputs to the transaction have been selected
Aside from the silent payments use case, I think this structure logically makes more sense. As a reminder, move-only commits are best reviewed with something like `git diff -w --color-moved=dimmed-zebra`
ACKs for top commit:
S3RK:
reACK a9c7300135f0188daa5bce5491e2daf2dd8da8ae
achow101:
ACK a9c7300135f0188daa5bce5491e2daf2dd8da8ae
rkrux:
tACK [a9c7300](a9c7300135)
Tree-SHA512: 412e1764b98f7428c8530c3a68f55e32063d6b66ab2ff613e1c7e12d49b049807cb60055cfe7f7e8ffe7ac7f0f9931427cbfd3efe7d4f97a5a0f6d1bf1aaac58
This allows a transaction's weight to be bound under a certain
weight if possible and desired. This can be beneficial for future
RBF attempts, or whenever a more restricted spend topology is
desired.
Co-authored-by: Greg Sanders <gsanders87@gmail.com>
- This change ensures consistency in transaction size and weight calculation
within the wallet and prevents conversion overflow when calculating
`max_selection_weight`.
`CoinGrinder` will also produce change output, listing all the
Coin selection algorithms that produces change output is not maintainable,
just infer that remaining algorithms all might produce change.
- This commit renames the coin selection algorithms input parameter `max_weight`
to `max_selection_weight` for clarity.
The parameter represent the maximum weight of the UTXOs the coin selection algorithm
should select, not the transaction maximum weight.
- The commit updates the parameter docstring to provide correct description.
- Also updates coin selection unit and fuzzing test variables to match the new name.
72b226882fe2348a9a66aee1d8d21b4e2d275e68 wallet: notify when preset + automatic inputs exceed max weight (furszy)
Pull request description:
Small change. Found it while finishing my review on #29523. This does not interfere with it.
Basically, we are erroring out early when the automatic coin selection process exceeds the maximum weight, but we are not doing so when the user-preselected inputs combined with the wallet-selected inputs exceed the maximum weight.
This change avoids signing all inputs before erroring out and introduces test coverage for `fundrawtransaction`.
ACKs for top commit:
achow101:
ACK 72b226882fe2348a9a66aee1d8d21b4e2d275e68
tdb3:
re ACK for 72b226882fe2348a9a66aee1d8d21b4e2d275e68
rkrux:
tACK [72b2268](72b226882f)
ismaelsadeeq:
utACK 72b226882fe2348a9a66aee1d8d21b4e2d275e68
Tree-SHA512: d77be19231023383a9c79a5d66b642dcbc6ebfc31a363e0b9f063c44898720a7859ec211cdbc0914ac7a3bfdf15e52fb8fc20d97f171431f70492c0f159dbc36
Move the output serialization size and dust calculation into the loop where the
outputs are iterated over to calculate the total sum.
Move the code for adding the the txoutputs to the transaction to after
coin selection.
While this code structure generally follows a more logical flow,
the primary motivation for moving the code for adding outputs to the
transaction sets us up nicely for silent payments (in a future PR):
we need to know the input set before generating the final output scriptPubKeys.
Now that a CRecipient holds a CTxDestination, we can get the serialized
size and determine if the output is dust using the CRecipient directly.
This does not change any current behavior, but provides a nice generalization
that can be used to apply special logic to a CTxDestination serialization
and dust calculations in the future.
Specifically, in a later PR when support for `V0SilentPayment` destinations is
added, we need to use `WitnessV1Taproot` as the scriptPubKey for serialized
size calcuations whenever the `CRecipient` destination is a `V0SilentPayment`
destination.
c7376babd19d0c858fef93ebd58338abd530c1f4 doc: Clarify distinction between util and common libraries in libraries.md (Ryan Ofsky)
4f74c59334d496f28e1a5c0d84c412f9020b366f util: Move util/string.h functions to util namespace (Ryan Ofsky)
4d05d3f3b42a41525aa6ec44b90f543dfab53ecf util: add TransactionError includes and namespace declarations (Ryan Ofsky)
680eafdc74021c1e0893c3a62404e607fd4724f5 util: move fees.h and error.h to common/messages.h (Ryan Ofsky)
02e62c6c9af4beabaeea58fb1ea3ad0dc5094678 common: Add PSBTError enum (Ryan Ofsky)
0d44c44ae33434f366229c612d6edeedf7658963 util: move error.h TransactionError enum to node/types.h (Ryan Ofsky)
9bcce2608dd2515dc35a0f0866abc9d43903c795 util: move spanparsing.h to script/parsing.h (Ryan Ofsky)
6dd2ad47922694d2ab84bad4dac9dd442c5df617 util: move spanparsing.h Split functions to string.h (Ryan Ofsky)
23cc8ddff472d259605d7790ba98a1900e77efab util: move HexStr and HexDigit from util to crypto (TheCharlatan)
6861f954f8ff42c87ad638037adae86a5bd89600 util: move util/message to common/signmessage (Ryan Ofsky)
cc5f29fbea15d33e4d1aa95591253c6b86953fe7 build: move memory_cleanse from util to crypto (Ryan Ofsky)
5b9309420cc9721a0d5745b6ad3166a4bdbd1508 build: move chainparamsbase from util to common (Ryan Ofsky)
ffa27af24da81a97d6c4912ae0e10bc5b6f17f69 test: Add check-deps.sh script to check for unexpected library dependencies (Ryan Ofsky)
Pull request description:
Remove `fees.h`, `errors.h`, and `spanparsing.h` from the util library. Specifically:
- Move `Split` functions from `util/spanparsing.h` to `util/string.h`, using `util` namespace for clarity.
- Move remaining spanparsing functions to `script/parsing.h` since they are used for descriptor and miniscript parsing.
- Combine `util/fees.h` and `util/errors.h` into `common/messages.h` so there is a place for simple functions that generate user messages to live, and these functions are not part of the util library.
Motivation for this change is that the util library is a dependency of the kernel, and we should remove functionality from util that shouldn't be called by kernel code or kernel applications. These changes should also improve code organization and make functions easier to discover. Some of these same moves are (or were) part of #28690, but did not help with code organization, or made it worse, so it is better to move them and clean them up in the same PR so code only has to change one time.
ACKs for top commit:
achow101:
ACK c7376babd19d0c858fef93ebd58338abd530c1f4
TheCharlatan:
Re-ACK c7376babd19d0c858fef93ebd58338abd530c1f4
hebasto:
re-ACK c7376babd19d0c858fef93ebd58338abd530c1f4.
Tree-SHA512: 5bcef16c1255463b1b69270548711e7ff78ca0dd34e300b95e3ca1ce52ceb34f83d9ddb2839e83800ba36b200de30396e504bbb04fa02c6d0c24a16d06ae523d
In order to ensure that the change of nVersion to a uint32_t in the
previous commit has no effect, rename nVersion to version in this commit
so that reviewers can easily spot if a spot was missed or if there is a
check somewhere whose semantics have changed.
bd34dd85e7b8b4cc26d2173d84bbeda2e9c27624 Use `exact_target` shorthand in coinselector_tests (Murch)
7aa7e30441fe77bf8e8092916e36b004bbbfe2a7 Fold GetSelectionWaste() into ComputeAndSetWaste() (Murch)
Pull request description:
PR #26152 moved waste calculation into SelectionResult to be able to correct the waste score on basis of the bump_fee_group_discount for overlapping ancestries. This left two functions with largely overlapping purpose, where one was simply a wrapper of the other. This PR cleans up the overlap, and fixes the double-meaning of `change_cost` where the `GetChange()` function assumed that no change was created when `change_cost` was set to 0. This behavior was exploited in a bunch of tests, but is problematic, because a `change_cost` of 0 is permitted with custom settings for feerate and discard_feerate (i.e. when they’re both 0).
ACKs for top commit:
achow101:
ACK bd34dd85e7b8b4cc26d2173d84bbeda2e9c27624
furszy:
Code ACK bd34dd85e7b8b4cc26d2173d84bbeda2e9c27624
ismaelsadeeq:
Code Review ACK bd34dd85e7b8b4cc26d2173d84bbeda2e9c27624
Tree-SHA512: 83a2688d45d719dc61a64b5180fe136107faccf401a59df65245c05d701748a03e85ed56fde8c9b7ef39a3ab54374dd3718c559bda5b3f55dafedfd7fed25161
