Optimizes coin selection by performing the "group outputs"
procedure only once, outside the "attempt selection" process.
Avoiding the repeated execution of the 'GroupOutputs' operation
that occurs on each coin eligibility filters (up to 8 of them);
then for every coin vector type plus one for all the coins together.
This also let us not perform coin selection over coin eligibility
filtered groups that don't add new elements.
(because, if the previous round failed, and the subsequent one has
the same coins, then this new round will fail again).
The 'GroupOutputs()' function performs the same
calculations for only-positive and mixed groups,
the only difference is that when we look for
only-positive groups, we discard negative utxos.
So, instead of wasting resources calling GroupOutputs()
for positive-only first, then call it again to include
the negative ones in the result, we can execute
GroupOutputs() only once, including in the response
both group types (positive-only and mixed).
The following scenarios are covered:
1) 10 UTXO with the same script:
partial spends is enabled --> outputs must not be grouped.
2) 10 UTXO with the same script:
partial spends disabled --> outputs must be grouped.
3) 20 UTXO, 10 one from scriptA + 10 from scriptB:
a) if partial spends is enabled --> outputs must not be grouped.
b) if partial spends is not enabled --> 2 output groups expected (one per script).
3) Try to add a negative output (value - fee < 0):
a) if "positive_only" is enabled --> negative output must be skipped.
b) if "positive_only" is disabled --> negative output must be added.
4) Try to add a non-eligible UTXO (due not fulfilling the min depth target for
"not mine" UTXOs) --> it must not be added to any group
5) Try to add a non-eligible UTXO (due not fulfilling the min depth target for
"mine" UTXOs) --> it must not be added to any group
6) Surpass the 'OUTPUT_GROUP_MAX_ENTRIES' size and verify that a second partial
group gets created.
Extract the logic that decides whether the new or the tried table is going to
be searched to the beginning of the function.
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
master branch:
0m36.86s real 0m03.26s user 0m01.69s system
0m35.71s real 0m03.78s user 0m01.64s system
0m45.76s real 0m03.12s user 0m01.27s system
PR branch:
0m13.04s real 0m02.66s user 0m00.93s system
0m14.08s real 0m02.81s user 0m00.82s system
0m14.05s real 0m02.50s user 0m00.93s system
8847ce44e0713350a6d3524f62eaeb10ba548bae util: add missing include and fix function signature (Cory Fields)
Pull request description:
ping hebasto
Discovered while testing pre-compiled header support with CMake: https://github.com/theuni/bitcoin/commits/cmake-pch-poc. Compilation of that branch fails without this fix and succeeds with it.
Similar to the fix in #27144.
The problem of having a default argument in the definition was masked by the missing include. Using PCH forces that include, so we end up with the compiler error we should've been getting all along.
ACKs for top commit:
fanquake:
ACK 8847ce44e0713350a6d3524f62eaeb10ba548bae
Tree-SHA512: 5eb9a6691ee37cbc5033a48aedcbf5c93af3b234614ae14c3fcc858f1ee505f630ad68f8bbb69ffa280080c8d0f91451362cb3819290b741ce906b2b3224a622
987f1bb41c0a8c54422066e10d1c63e19c4df66d Fixed a couple of typos in comments to make linter happy (hernanmarino)
Pull request description:
While working on a different PR, I stumbled upon a couple of typos being reported by the linter and fixed them.
ACKs for top commit:
kristapsk:
utACK 987f1bb41c0a8c54422066e10d1c63e19c4df66d
Tree-SHA512: bf53e150bb7c6b59a9dc270bdf748bf8fb0430f35d1d0a68ef70e9345d322470c4a0b5cd7f2c3cfc81ce087bb6624d820d5f4501f58aaa5806f358ddd4e0b551
And not hide it inside the `OutputGroup::Insert` method.
This method does not return anything if insertion fails.
We can know before calling `Insert` whether the coin
will be accepted or not.
545ff924ab6303ffabd91fdfc4f0a4962daf133c refactor: use string_view for RPC named argument values (stickies-v)
7727603e44f8f674e0fc8389e78047e2b56e6052 refactor: reduce unnecessary complexity in ParseNonRFCJSONValue (stickies-v)
1d02e599012721549d4c20b1b37fcc5ee7b961b6 test: add cases to JSON parsing (stickies-v)
Pull request description:
Inspired by MarcoFalke's [comment](https://github.com/bitcoin/bitcoin/pull/26506#discussion_r1036149426). Main purpose of this PR is to minimize copying (potentially large) RPC named arguments when calling `.substr()` by using `std::string_view` instead of `std::string`. Furthermore, cleans up the code by removing unnecessary complexity in `ParseNonRFCJSONValue()` (done first to avoid refactoring required to concatenate `string` and `string_view`), updates some naming and adds a few test cases. Should not introduce any behaviour change.
## Questions
- ~Was there actually any merit to `ParseNonRFCJSONValue()` surrounding the value with brackets and then parsing it as an array? I don't see it, and the new approach doesn't fail any tests. Still a bit suspicious about it though.~
- Cleared up by https://github.com/bitcoin/bitcoin/pull/26506#pullrequestreview-1211984059
- If there are no objections to 7727603e44f8f674e0fc8389e78047e2b56e6052, I think we should follow up with a PR to rename `ParseNonRFCJSONValue()` to a local `Parse()` helper function (that throws if invalid), remove it from `client.h` and merge the test coverage we currently have on `ParseNonRFCJSONValue()` with the coverage we have on `UniValue::read()`.
ACKs for top commit:
ryanofsky:
Code review ACK 545ff924ab6303ffabd91fdfc4f0a4962daf133c
MarcoFalke:
review ACK 545ff924ab6303ffabd91fdfc4f0a4962daf133c 📻
Tree-SHA512: b1c89fb010ac9c3054b023cac1acbba2a539a09cf39a7baffbd7f7571ee268d5a6d98701c7ac10d68a814526e8fd0fe96ac1d1fb072f272033e415b753f64a5c
9c371e50a225e158264b008ba05c9e95055d9800 doc: Update Transifex links and slug format in Release Process (Hennadii Stepanov)
Pull request description:
Reflected the recent changes in Transifex's [workflow](https://github.com/bitcoin/bitcoin/pull/26321) and on its website.
ACKs for top commit:
jarolrod:
ACK 9c371e50a225e158264b008ba05c9e95055d9800
Tree-SHA512: d11f2fa197bba2e3198ca255d80edd7178e8074dc84b03262054d01fa33c8160ca492a1358e9da1e7d32cec21a298593322d10e4cb045ef99ccec2cbe901cb63
2c9eb4afe1f583aafa552b2711b149f17ef8320f guix: use cmake-minimal over cmake (fanquake)
1475515312856afe3f19a95f2c32bc80c7c54484 guix: use coreutils-minimal over coreutils (fanquake)
444562141504ff7f0bb071d6e7bf7f511517e372 guix: use bash-minimal over bash (fanquake)
Pull request description:
Minimal versions of the same packages, that should still be sufficient for our use:
> (define-public bash-minimal
;; A stripped-down Bash for non-interactive use.
> (define-public coreutils-minimal
;; Coreutils without its optional dependencies.
> ;;; This minimal variant of CMake does not include the documentation. It is
;;; used by the cmake-build-system.
(define-public cmake-minimal
ACKs for top commit:
TheCharlatan:
ACK 2c9eb4afe1f5
Sjors:
tACK 2c9eb4afe1f583aafa552b2711b149f17ef8320f
achow101:
ACK 2c9eb4afe1f583aafa552b2711b149f17ef8320f
hebasto:
ACK 2c9eb4afe1f583aafa552b2711b149f17ef8320f,
Tree-SHA512: f91ca9e088b8346b20c2affc80870c31640de3aedcfcc0fb98a5e82c77ef64537870b88552f26759d31d8d0956b1fd685e6c25d5acbc92f5feaececd1a7dd37e
9172cc672ea99eac6d0210e8b793ca030c20e179 qt: Update translation source file (Hennadii Stepanov)
7b0cbf444d2ecdd2a6c0754990bf677ab1152dab qt: Bump Transifex slug for 25.x (Hennadii Stepanov)
369023d22def0917fd879f52f86cf6a4945498ca qt: Periodic translation updates from Transifex (Hennadii Stepanov)
Pull request description:
This PR follows our [Release Process](https://github.com/bitcoin/bitcoin/blob/master/doc/release-process.md).
Required to open Transifex translations for 25.0 on 2023-03-01 as it's [planned](https://github.com/bitcoin/bitcoin/issues/26549).
**NOTE.** Translations for the following languages for the latest 24.x Transifex resource have been effectively cancelled/damaged/vandalized:
- German (de) by [nesbonk83](https://www.transifex.com/user/profile/nesbonk83/) on 2023-01-27
- Dutch (nl) by [bram00767](https://www.transifex.com/user/profile/bram00767/) on 2022-12-17
- Spanish, Mexico (es_MX) by [VCFNFT](https://www.transifex.com/user/profile/VCFNFT/) on 2022-08-08
The first commit ignores changes to translations mentioned above.
ACKs for top commit:
jarolrod:
ACK 9172cc672ea99eac6d0210e8b793ca030c20e179
Tree-SHA512: 85641facecd11526bbcde934b43629aba1b856c4f97272a956c2ce194af8a1723325a160a0a518fc052af9373f853204848b58d3c0a3bea09788fccfc5d9f557
56e37e71a2538a240cc360678aeb752d17bd8f45 Make miniscript fuzzers avoid script size limit (Pieter Wuille)
bcec5ab4ff1039c0c309dbbb9953adbd0a4f3e88 Make miniscript fuzzers avoid ops limit (Pieter Wuille)
213fffa5138229eac2d4a9eda0f643fe90870378 Enforce type consistency in miniscript_stable fuzz test (Pieter Wuille)
e1f30414c6b9434048e089ccc3ec4f475f980c60 Simplify miniscript fuzzer NodeInfo struct (Pieter Wuille)
5abb0f5ac37e8a17072d5989a025227035fdc7e6 Do base type propagation in miniscript_stable fuzzer (Pieter Wuille)
Pull request description:
This adds a number of improvements to the miniscript fuzzers that all amount to rejecting invalid or overly big miniscripts early on:
* Base type propagation in the miniscript_stable fuzzers prevents constructing a large portion of miniscripts that would be illegal, with just a little bit of type logic in the fuzzer. The fuzzer input format is unchanged.
* Ops and script size tracking in GenNode means that too-large scripts (either due to script size limit or ops limit) will be detected on the fly during fuzz input processing, before actually constructing the scripts.
Closes #27147.
ACKs for top commit:
darosior:
re-ACK 56e37e71a2
dergoegge:
tACK 56e37e71a2538a240cc360678aeb752d17bd8f45
Tree-SHA512: 245584adf9a6644a35fe103bc81b619e5b4f5d467571a761b5809d08b1dec48f7ceaf4d8791ccd8208b45c6b309d2ccca23b3d1ec5399df76cd5bf88f2263280
Add common InitConfig function to deduplicate bitcoind and bitcoin-qt code
reading config files and creating the datadir.
There are a few minor changes in behavior:
- In bitcoin-qt, when there is a problem reading the configuration file, the
GUI error text has changed from "Error: Cannot parse configuration file:" to
"Error reading configuration file:" to be consistent with bitcoind.
- In bitcoind, when there is a problem reading the settings.json file, the
error text has changed from "Failed loading settings file" to "Settings
file could not be read" to be consistent with bitcoin-qt.
- In bitcoind, when there is a problem writing the settings.json file, the
error text has changed from "Failed saving settings file" to "Settings file
could not be written" to be consistent with bitcoin-qt.
- In bitcoin-qt, if there datadir is not accessible (e.g. no permission to read),
there is an normal error dialog showing "Error: filesystem error: status:
Permission denied [.../settings.json]", instead of an uncaught exception
This is only used in the current PR to avoid ugly
`strprintf(Untranslated("%s:\n%s"), str, MakeUnorderedList(details)`
boilerplate in init code. But in the future the function could be extended and
more widely used to include more details in GUI error messages or display them
in a more readable way, see code comment.
Previous bilingual_str tinyformat::format accepted bilingual format strings,
but not bilingual arguments. Extend it to accept both. This is useful when
embedding one translated string inside another translated string, for example:
`strprintf(_("Error: %s"), message)` which would fail previously if `message`
was a bilingual_str.
Some InitError calls had trailing \n characters, causing double newlines in
error output. After this change InitError calls consistently output one newline
instead of two. Appearance of messages in the GUI does not seem to be affected.
Can be tested with:
src/bitcoind -regtest -datadir=noexist
src/qt/bitcoin-qt -regtest -datadir=noexist
-BEGIN VERIFY SCRIPT-
git grep -l InitError src/ | xargs sed -i 's/\(InitError(.*\)\\n"/\1"/'
-END VERIFY SCRIPT-
29b62c01c8d211475ea9dd1a1093820f0a86c06d valgrind: remove libsecp256k1 suppression (fanquake)
Pull request description:
I am no-longer been able to recreate this issue, atleast after the most recent libsecp256k1 changes. Can someone else confirm?
ACKs for top commit:
MarcoFalke:
lgtm ACK 29b62c01c8d211475ea9dd1a1093820f0a86c06d
sipa:
utACK 29b62c01c8d211475ea9dd1a1093820f0a86c06d
Tree-SHA512: e61e5b88ac2af3c779f23d999938bec4497f6433a029c07dd57a9481fe9cc104d8d8f63586910b29f41e66bbf0ed094bc7539c0df84754a1783c4b1b15af072e
75db62ba4cae048e742ca02dc6a52b3b3d6727de refactor: Move calculation logic out from `CheckSequenceLocksAtTip()` (Hennadii Stepanov)
3bc434f4590758db673e1bd4ebf1906ea632f593 refactor: Add `CalculateLockPointsAtTip()` function (Hennadii Stepanov)
Pull request description:
This PR is follow up for bitcoin/bitcoin#22677 and bitcoin/bitcoin#23683.
On master (013daed9acca1b723f599d63ab36b9c2a5c60e5f) it is not obvious that `CheckSequenceLocksAtTip()` function can modify its `LockPoints* lp` parameter which leads to https://github.com/bitcoin/bitcoin/pull/22677#discussion_r762040101.
This PR:
- separates the lockpoint calculate logic from `CheckSequenceLocksAtTip()` function into a new `CalculateLockPointsAtTip()` one
- cleans up the `CheckSequenceLocksAtTip()` function interface
- makes code easier to reason about (hopefully)
ACKs for top commit:
achow101:
ACK 75db62ba4cae048e742ca02dc6a52b3b3d6727de
stickies-v:
re-ACK 75db62b
Tree-SHA512: 072c3fd9cd1e1b0e0bfc8960a67b01c80a9f16d6778f374b6944ade03a020415ce8b8ab2593b0f5e787059c8cf90af798290b4c826785d41955092f6e12e7486
9a9d5da11fa6033f82dcf8e2298aee29587f5396 refactor: Stop using gArgs global in system.cpp (Ryan Ofsky)
b20b34f5b33230fe253c81008496bd9b13fd6ecf refactor: Use new GetConfigFilePath function (Ryan Ofsky)
Pull request description:
Most of the code in `util/system.cpp` that was hardcoded to use the global `ArgsManager` instance `gArgs` has been changed to stop using it (for example in https://github.com/bitcoin/bitcoin/pull/20092). But a few hardcoded references to `gArgs` remain. This commit removes the last ones so these functions aren't reading or writing global state.
Noticed these `gArgs` references while reviewing #27073
ACKs for top commit:
achow101:
ACK 9a9d5da11fa6033f82dcf8e2298aee29587f5396
stickies-v:
ACK 9a9d5da11
willcl-ark:
tACK 9a9d5da11
Tree-SHA512: 2c74b0d5fc83e9ed2ec6562eb26ec735512f75db8876a11a5d5f04e6cdbe0cd8beec19894091aa2cbf29319194d2429ccbf8036f5520ecc394f6fe89a0079a7b
a984beeca10e3ae1ceb3ea53e4dea778160e7079 [ci] change lint to bookworm for git v2.38 (glozow)
Pull request description:
Since 5497c14, verify-commits.py uses `git merge-tree` which requires git v2.38 or later. Fix the lint jobs on master (e.g. https://cirrus-ci.com/task/4971007513985024).
ACKs for top commit:
achow101:
ACK a984beeca10e3ae1ceb3ea53e4dea778160e7079
hebasto:
re-ACK a984beeca10e3ae1ceb3ea53e4dea778160e7079
Tree-SHA512: dd50598aefb6f9c86cf221dea27fcc521e335cb182f7a3abcb3215d3991794354085be3e07c133ab74ca8b957e3a6acc43722899165957b3d898867d6253ebdc
fb0dbe94233ec509570cbba3118cf62d8e60842b docs: GetDataDirNet and GetDataDirBase don't create datadir (stickies-v)
Pull request description:
Since #27073, the behaviour of `GetDataDir()` [changed](https://github.com/bitcoin/bitcoin/pull/27073/files#diff-19427b0dd1a791adc728c82e88f267751ba4f1c751e19262cac03cccd2822216L435-L443) to only return the datadir path, but not create it if non-existent. This also changed the behaviour of `GetDataDirNet()` and `GetDataDirBase()` but the docs do not yet reflect that.
ACKs for top commit:
TheCharlatan:
ACK fb0dbe94233ec509570cbba3118cf62d8e60842b
theStack:
ACK fb0dbe94233ec509570cbba3118cf62d8e60842b
willcl-ark:
ACK fb0dbe942
Tree-SHA512: 3f10f4871df59882f3649c6d3b2362cae2f8a01ad0bd0c636c5608b0d177d279a2e8712930b819d6d3912e91fa6447b9e54507c33d8afe427f7f39002b013bfb
3141eab9c669488a2e7fef5f60d356ac92294922 test: add functional test for ScanAndUnlinkAlreadyPrunedFiles (Andrew Toth)
e252909e561e47d75cb3a892657662a139f6532c test: add unit test for ScanAndUnlinkAlreadyPrunedFiles (Andrew Toth)
77557dda4a123515d0fa2a545ee21d7c43a66988 prune: scan and unlink already pruned block files on startup (Andrew Toth)
Pull request description:
There are a few cases where we can mark a block and undo file as pruned in our block index, but not actually remove the files from disk.
1. If we call `FindFilesToPrune` or `FindFilesToPruneManual` and crash before `UnlinkPrunedFiles`.
2. If on Windows there is an open file handle to the file somewhere else when calling `fs::remove` in `UnlinkPrunedFiles` (https://en.cppreference.com/w/cpp/filesystem/remove, https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-deletefilew#remarks). This could be from another process, or if we are calling `ReadBlockFromDisk`/`ReadRawBlockFromDisk` without having a lock on `cs_main` (which has been allowed since ccd8ef65f9).
This PR mitigates this by scanning all pruned block files on startup after `LoadBlockIndexDB` and unlinking them again.
ACKs for top commit:
achow101:
ACK 3141eab9c669488a2e7fef5f60d356ac92294922
pablomartin4btc:
re-ACK with added functional test 3141eab9c669488a2e7fef5f60d356ac92294922.
furszy:
Code review ACK 3141eab9
theStack:
Code-review ACK 3141eab9c669488a2e7fef5f60d356ac92294922
Tree-SHA512: 6c73bc57838ad1b7e5d441af3c4d6bf4c61c4382e2b86485e57fbb74a61240710c0ceeceb8b4834e610ecfa3175c6955c81ea4b2285fee11ca6383f472979d8d
Use the same technique as is using in the FromString miniscript parser to
predict the final script size of the miniscript being generated in the
miniscript_stable and miniscript_smart fuzzers (by counting every unexplored
sub node as 1 script byte, which is possible because every leaf node always
adds at least 1 byte). This allows bailing out early if the script being
generated would exceed the maximum allowed size (before actually constructing
the miniscript, as that may happen only significantly later potentially).
Also add a self-check to make sure this predicted script size matches that
of generated scripts.
Keep track of the total number of ops the constructed script will have
during miniscript_stable and miniscript_smart fuzzers' GenNode, so it
can abort early if the 201 ops limit would be exceeded.
Also add a self-check that the final constructed node has the predicted
ops size limit, so we know the fuzzer's logic for keeping track of this
is correct.
Since we now keep track of all expected child node types (even if rudimentary)
in both miniscript_stable and miniscript_smart fuzzers, there is no need anymore
for the former shortcut NodeInfo constructors without sub types.
Keep track of which base type (B, K, V, or W) is desired in the miniscript_stable
ConsumeStableNode function. This allows aborting early if the constructed node
won't have the right type.
Note that this does not change the fuzzer format; the meaning of inputs in
ConsumeStableNode is unmodified. The only change is that often the fuzzer will
abort early.
The direct motivation is preventing recursing v: wrappers, which are the only
fragment type that does not otherwise increase the overall minimum possible script
size. In a later commit this will be exploited to prevent overly-large scripts from
being constructed.
Since #27073, the behaviour of GetDataDir changed to only return
the datadir path, but not create it. This also changed the behaviour
of GetDataDirNet and GetDataDirBase but the docs do not yet reflect
that.
c5825e14f8999a8c5f5121027af9e07ac51ab42e doc: add explanation for fail_on_insufficient_dbcache (Ryan Ofsky)
7dff7da4f5eafa89546565a63362e57516e4064e init: Return more fitting ChainStateLoadStatus if verification was interrupted (Martin Zumsande)
Pull request description:
This addresses two outstanding comments by ryanofsky from #25574:
* return `ChainstateLoadStatus::INTERRUPTED` instead of `ChainstateLoadStatus::SUCCESS` if verification was stopped by an interrupt. This would coincide with straightforward expectation, and it avoids a misleading [log entry](c5825e14f8/src/init.cpp (L1526)) in `init` for the block index load time (because that would include the verificiation, which didn't complete). It shouldn't affect node behavior otherwise because the shutdown signal would be caught in init anyway. In test, this would lead to an assert ([link](c5825e14f8/src/test/util/setup_common.cpp (L230))), which also makes more sense because benign interrupts are not expected there during init.
This can be tested by setting a large value for `-checkblocks`, interrupting the node during block verification and observing the log.
https://github.com/bitcoin/bitcoin/pull/25574#discussion_r1110050930
* add documentation for `require_full_verification` https://github.com/bitcoin/bitcoin/pull/25574#discussion_r1110031541
ACKs for top commit:
MarcoFalke:
thanks lgtm ACK c5825e14f8999a8c5f5121027af9e07ac51ab42e
Tree-SHA512: ca1c71a1b046d30083337dd9ef6d52e66fa1ac8c4ecd807716e4aa6a894179a81df41caee916fa30997fd6e0b284412a3c8f2919d19c29d826fb580ffb89fd73
ff4a73aea2b8346f48f75e1130a36749f70e9049 depends: use FORTIFY_SOURCE=3 with libevent (fanquake)
Pull request description:
Use `FORTIFY_SOURCE=3` when building libevent in depends. I've upstreamed a change to switch libevent from using =2 to =3 as well: https://github.com/libevent/libevent/pull/1418.
Solves half of #27038, by giving us some fortified funcs in `bitcoin-cli`.
ACKs for top commit:
TheCharlatan:
ACK ff4a73aea2b8346f48f75e1130a36749f70e9049
Tree-SHA512: eaf692ec92b288f0cb524c011fc81529f58efa4c43d418a7b3ae7108eba2bccba708a81a28ac6d063267be80ca615637c6e3fccc02497d7367af2eaae0e8d812
Most of the code in util/system.cpp that was hardcoded to use the global
ArgsManager instance `gArgs` has been changed to work with explicit ArgsManager
instances (for example in https://github.com/bitcoin/bitcoin/pull/20092). But a
few hardcoded references to `gArgs` remain. This commit removes the last ones
so these functions aren't reading or writing global state.
New function was introduced by willcl-ark <will@256k1.dev> in commit
56e370fbb9413260723c598048392219b1055ad0 from
https://github.com/bitcoin/bitcoin/pull/27073 and removes some duplicate code.
