89cd20cbed test: add coverage for sigop limit policy (`-bytespersigop` setting) (Sebastian Falbesoner)
Pull request description:
This PR adds missing test coverage for the `-bytespersigop` option, which determines how pre-taproot signature operations (OP_CHECKSIG{VERIFY}, OP_CHECKMULTIGSIG{VERIFY}) affect fee handling calculations. The setting was introduced in PR #7081 for mitigating the [sigop spam attack](https://bitcointalk.org/index.php?topic=1166928.0); the initial implementation rejected txs exceeding the limit, but was changed in #8365 later to account for higher sizes in the mempool (i.e. exceeding the sigop limit is possible, but has to be compensated by higher fees).
For each combination of `-bytespersigop` setting and sigops count, the test first creates a P2WSH spending transaction with a witness script that puts sigops in a non-executing branch (OP_FALSE OP_IF OP_CHECKMULTISIG ... OP_CHECKSIG ... OP_ENDIF). This tx is then bumped up to reach exactly the _sig-op limit equivalent vsize_ by padding its datacarrier output. Based on that, increasing the tx's vsize should still reflect a vsize increase in the mempool, while a decrease of the tx's vsize should lead to the mempool treating the tx's vsize to be the _sig-op limit equivalent vsize_, since the limit was exceeded.
I assume that this parameter is almost never set explicitly by users (also it is not relevant for taproot spends), but it doesn't hurt to have a test for it. See also https://bitcoin.stackexchange.com/a/87958 for another explanation.
ACKs for top commit:
glozow:
light review ACK 89cd20cbed
MarcoFalke:
nice ACK 89cd20cbed📁
Tree-SHA512: 06998ce93bf9d5ce6143db2996a43f13990c415f97afe684227ad469349e73952bf4f6c871c1e6349e07606f4d45db64408848873a86a89481cdca5a134e5e60
faa671591f test: Use self.wait_until over wait_until_helper (MarcoFalke)
Pull request description:
`wait_until_helper` is a "private" helper, not intended to be used directly, because it doesn't scale the timeout with the timeout factor. Fix this by replacing it with a call to `self.wait_until`, which does the scaling.
ACKs for top commit:
theStack:
Code-review ACK faa671591f
Tree-SHA512: 70705f309f83ffd6ea5d090218195d05b868624d909106863372f861138b5a70887070b25beb25044ae1b44250345e45c9cc11191ae7aeca2ad37801a0f62f61
fe329dc936 test: Add test for getblockfrompeer on pruned nodes (Fabian Jahr)
cd761e6b2c rpc: Add note on guarantees to getblockfrompeer (Fabian Jahr)
Pull request description:
These are additions to `getblockfrompeer` that I already [suggested on the original PR](https://github.com/bitcoin/bitcoin/pull/20295#pullrequestreview-817157738).
The two commits do the following:
1. Add a test for `getblockfrompeer` usage on pruned nodes. This is important because many use-cases for `getblockfrompeer` are in a context of a pruned node.
2. Add some information on how long the users of pruned nodes can expect the block to be available after they have used the RPC. I think the behavior is not very intuitive for users and I would not be surprised if users expect the block to be available indefinitely.
ACKs for top commit:
Sjors:
re-utACK fe329dc936
MarcoFalke:
review ACK fe329dc936🍉
stratospher:
ACK fe329dc.
brunoerg:
re-ACK fe329dc936
Tree-SHA512: a686bd8955d9c3baf365db384e497d6ee1aa9ce2fdb0733fe6150f7e3d94bae19d55bc1b347f1c9f619e749e18b41a52b9f8c0aa2042dd311a968a4b5d251fac
3fa1185dda github: Switch to yaml issue templates (willcl-ark)
Pull request description:
The new YAML templates provide more flexibility and can be designed to extract more information from users when submitting issues, avoiding initial back-and-forth when reports do not include enough background information to begin with.
Key differences:
* YAML format
* Allows us to require responses to certain questions
* Not currently compatible with GitLab (.md only)
This does keep the "Blank Issue" option at the bottom.
Testing this must be done with the master branch of the repo, which is slightly annoying for this repo. I have therefore pushed this to my own fork so that you can see the new templates, along with how the output is rendered in newly-created issues:
[github.com/willcl-ark/bitcoin/issues](https://github.com/willcl-ark/bitcoin/issues?q=is%3Aissue+is%3Aopen+sort%3Aupdated-desc)
I did make some minor changes to some of the template wording, but this change could also be a good time to add/remove additional questions.
This seems like a net-positive for me, setting aside the issue that if we ever migrated away from GitHub these might have to be ported back to *.md (or something else), but that seems easy-enough that this change would be worth it.
Curious to know what others think of this, and whether they would suggest adding any other questions to any of the templates as part of this update?
ACKs for top commit:
achow101:
ACK 3fa1185dda
glozow:
ACK 3fa1185dda
Tree-SHA512: ce7990cd5f951e3839bc54022ce9f4b0ff9ffb8b19754d657d79acf9118bbdc4aba196f872cd5511b81e03993e54dfe4fcb85e89deade024e5a65a336adb638b
4be57a5df1 gui: fix comments for BanTableModel and BanTablePriv::refreshBanlist() (Vasil Dimov)
a981af4e6f gui: use the stored CSubNet entry when unbanning (Vasil Dimov)
Pull request description:
The previous code visualized the `CSubNet` object as string, then parsed that string back to `CSubNet`. This is sub-optimal given that the original `CSubNet` object can be used directly instead.
This avoids calling `LookupSubNet()` from the GUI.
ACKs for top commit:
furszy:
utACK 4be57a5d
mzumsande:
Tested ACK 4be57a5df1
Tree-SHA512: b783c18c9d676aa9486cff2d27039dd5c5ef3f1cc67e5056a2be68e35930926f368f26dacdf4f3d394a1f73e3e28f42dc8a6936cd1765c6e6e60695c7b4d78af
fa0abcdafe test: Flatten miniwallet array and remove random fee in longpoll (MarcoFalke)
Pull request description:
* Using a single MiniWallet is enough.
* A random fee isn't needed either.
ACKs for top commit:
theStack:
re-ACK fa0abcdafe
Tree-SHA512: 77b99885b3f0d325d067838122114be57ec999ebc82912de6a22c33e2ba28a341c5e053c5bbc424b9922c2616562289a57c7156bd3b431d779182c2e472da59c
b082f28101 rpc, wallet: use the same `next_index` in listdescriptors and importdescriptors (w0xlt)
Pull request description:
Currently `listdescriptors` RPC uses `next` key to represent `WalletDescriptor::next_index` while `importdescriptors` uses `next_index`. This creates two different descriptor formats.
This PR changes `listdescriptors` to use the same key as `importdescriptors`.
ACKs for top commit:
achow101:
ACK b082f28101
aureleoules:
reACK b082f28101
Tree-SHA512: c29ec59051878e614d749ed6dc85e5c14ad00db0e8fcbce3f5066d1aae85ef07ca70f02920299e48d191b7387024fe224b0054c4191a5951cb805106f7b8e37b
3e947d7117 doc: remove 'omitted...' doc for rpc getrawtransaction when verbose is 2 (dougEfish)
Pull request description:
Remove optional rpc doc for getrawtransaction when verbose is 2
ACKs for top commit:
stickies-v:
ACK 3e947d7117
Tree-SHA512: b9e970d6ef4a47ec7ca32f5ff1028cc901f1bfdc1571668208505d42f4160733530601b78e469de82a854d3b298a55a81d0a7916bc5db4a43ad6d6a299c55c9e
fa1b4e5c32 Use steady clock in FlushStateToDisk (MarcoFalke)
1111e2f8b4 Use steady clock in SeedStrengthen and FindBestImplementation (MarcoFalke)
Pull request description:
There may be a theoretical deadlock for the duration of the offset when the system clock is adjusted into a past time while executing `SeedStrengthen`.
Fix this by using steady clock.
Do the same in `FindBestImplementation`, which shouldn't be affected, because it discards outlier measurements. However, doing the same there for consistency seems fine.
Do the same in `FlushStateToDisk`, which should make the flushes more steady, if the system clock is adjusted by a large offset.
ACKs for top commit:
john-moffett:
ACK fa1b4e5c32
willcl-ark:
ACK fa1b4e5c3
Tree-SHA512: cc625e796b186accd53222bd64eb57d0512bc7e588312d254349b542bbc5e5daac348ff2b3b3f7dc5ae0bbbae2ec11fdbf3022cf2164211633765a4b0108e83e
2b373fe49d docs: update assumeutxo.md (James O'Beirne)
87a1108c81 test: add snapshot completion unittests (James O'Beirne)
d70919a88f refactor: make MempoolMutex() public (James O'Beirne)
7300ced9de log: add LoadBlockIndex() message for assumedvalid blocks (James O'Beirne)
d96c59cc5c validation: add ChainMan logic for completing UTXO snapshot validation (James O'Beirne)
f2a4f3376f move-only-ish: init: factor out chainstate initialization (James O'Beirne)
637a90b973 add Chainstate::HasCoinsViews() (James O'Beirne)
c29f26b47b validation: add CChainState::m_disabled and ChainMan::isUsable (James O'Beirne)
5ee22cdafd add ChainstateManager.GetSnapshot{BaseHeight,BaseBlock}() (James O'Beirne)
Pull request description:
This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11) (parent PR: https://github.com/bitcoin/bitcoin/pull/15606)
Part two of replacing https://github.com/bitcoin/bitcoin/pull/24232.
---
When a user activates a snapshot, the serialized UTXO set data is used to create an "assumed-valid" chainstate, which becomes active in an attempt to get the node to network tip as quickly as possible. Simultaneously in the background, the already-existing chainstate continues "conventional" IBD to both accumulate full block data and serve as a belt-and-suspenders to validate the assumed-valid chainstate.
Once the background chainstate's tip reaches the base block of the snapshot used, we set `m_stop_use` on that chainstate and immediately take the hash of its UTXO set; we verify that this matches the assumeutxo value in the source code. Note that while we ultimately want to remove this background chainstate, we don't do so until the following initialization process, when we again check the UTXO set hash of the background chainstate, and if it continues to match, we remove the (now unnecessary) background chainstate, and move the (previously) assumed-valid chainstate into its place. We then reinitialize the chainstate in the normal way.
As noted in previous comments, we could do the filesystem operations "inline" immediately when the background validation completes, but that's basically just an optimization that saves disk space until the next restart. It didn't strike me as worth the risk of moving chainstate data around on disk during runtime of the node, though maybe my concerns are overblown.
The final result of this completion process is a fully-validated chain, where the only evidence that the user synced using assumeutxo is the existence of a `base_blockhash` file in the `chainstate` directory.
ACKs for top commit:
achow101:
ACK 2b373fe49d
Tree-SHA512: a204e1d6e6932dd83c799af3606b01a9faf893f04e9ee1a36d63f2f1ccfa9118bdc1c107d86976aa0312814267e6a42074bf3e2bf1dead4b2513efc6d955e13d
Also adjusts the previous snapshot chainstate init tests
to account for the fact that the init process is now attempting to
validate and complete background chainstates whose tip is at the
snapshot base block. We use a DisconnectTip() hack to preserve the
nature of the test.
Trigger completion when a background validation chainstate reaches the
same height as a UTXO snapshot, and handle cleaning up the chainstate
on subsequent startup.
802cc1ef53 Deduplicate bitcoind and bitcoin-qt init code (Ryan Ofsky)
d172b5c671 Add InitError(error, details) overload (Ryan Ofsky)
3db2874bd7 Extend bilingual_str support for tinyformat (Ryan Ofsky)
c361df90b9 scripted-diff: Remove double newlines after some init errors (Ryan Ofsky)
Pull request description:
Add common InitConfig function to deduplicate bitcoind and bitcoin-qt code reading config files and creating the datadir.
Noticed the duplicate code while reviewing #27073 and want to remove it because difference in bitcoin-qt and bitcoind behavior make it hard to evaluate changes like #27073
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.
ACKs for top commit:
Sjors:
Light review ACK 802cc1ef53
TheCharlatan:
ACK 802cc1ef53
achow101:
ACK 802cc1ef53
Tree-SHA512: 9c78d277e9ed595fa8ce286b97d2806e1ec06ddbbe7bd3434bd9dd7b456faf8d989f71231e97311f36edb9caaec645a50c730bd7514b8e0fe6e6f7741b13d981
fa8481b05f util: Work around ParseHex gcc cross compiler bug (MarcoFalke)
Pull request description:
I fail to see how an explicit `ParseHex` template instantiation fails to also instantiate `TryParseHex`.
Nonetheless, to work around a compiler bug, change the explicit instantiation from `ParseHex` to `TryParseHex`. (`ParseHex` is inline anyway and will be instantiated by the compiler either way).
Fixes https://github.com/bitcoin/bitcoin/pull/25227#issuecomment-1456009757 :
```
CXXLD bitcoind
/usr/lib/gcc-cross/powerpc64le-linux-gnu/11/../../../../powerpc64le-linux-gnu/bin/ld: libbitcoin_node.a(libbitcoin_node_a-net_processing.o): in function `(anonymous namespace)::PeerManagerImpl::ProcessMessage(CNode&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, CDataStream&, std::chrono::duration<long, std::ratio<1l, 1000000l> >, std::atomic<bool> const&)':
net_processing.cpp:(.text+0x29660): undefined reference to `std::optional<std::vector<unsigned char, std::allocator<unsigned char> > > TryParseHex<unsigned char>(std::basic_string_view<char, std::char_traits<char> >)'
/usr/lib/gcc-cross/powerpc64le-linux-gnu/11/../../../../powerpc64le-linux-gnu/bin/ld: libbitcoin_node.a(libbitcoin_node_a-rest.o): in function `rest_getutxos(std::any const&, HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)':
rest.cpp:(.text+0x83b4): undefined reference to `std::optional<std::vector<unsigned char, std::allocator<unsigned char> > > TryParseHex<unsigned char>(std::basic_string_view<char, std::char_traits<char> >)'
/usr/lib/gcc-cross/powerpc64le-linux-gnu/11/../../../../powerpc64le-linux-gnu/bin/ld: libbitcoin_node.a(libbitcoin_node_a-torcontrol.o): in function `std::vector<unsigned char, std::allocator<unsigned char> > ParseHex<unsigned char>(std::basic_string_view<char, std::char_traits<char> >)':
torcontrol.cpp:(.text._Z8ParseHexIhESt6vectorIT_SaIS1_EESt17basic_string_viewIcSt11char_traitsIcEE[_Z8ParseHexIhESt6vectorIT_SaIS1_EESt17basic_string_viewIcSt11char_traitsIcEE]+0x2c): undefined reference to `std::optional<std::vector<unsigned char, std::allocator<unsigned char> > > TryParseHex<unsigned char>(std::basic_string_view<char, std::char_traits<char> >)'
/usr/lib/gcc-cross/powerpc64le-linux-gnu/11/../../../../powerpc64le-linux-gnu/bin/ld: libbitcoin_common.a(libbitcoin_common_a-external_signer.o): in function `ExternalSigner::SignTransaction(PartiallySignedTransaction&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&)':
external_signer.cpp:(.text+0x8d84): undefined reference to `std::optional<std::vector<unsigned char, std::allocator<unsigned char> > > TryParseHex<unsigned char>(std::basic_string_view<char, std::char_traits<char> >)'
collect2: error: ld returned 1 exit status
ACKs for top commit:
gruve-p:
ACK fa8481b05f
hebasto:
ACK fa8481b05f, tested on Ubuntu 22.04, gcc 11.3 for the `riscv64-linux-gnu` host.,
Tree-SHA512: 53efa424e7e18d85a2c9ac2267b9370ae3594d9be73da5135a3a79bf07ab50fcc5357cbde09dc0b2a9eb78d78ec37beae0c9f876333b568e678b9d0067bc9e4e
60978c8080 test: Reduce extended timeout on abortnode test (Fabian Jahr)
660bdbf785 http: Release server before waiting for event base loop exit (João Barbosa)
8c6d007c80 http: Track active requests and wait for last to finish (João Barbosa)
Pull request description:
This revives #19420. Since promag is not so active at the moment, I can support this to finally get it merged.
The PR is rebased and comments by jonatack have been addressed.
Once this is merged, I will also reopen#19434.
ACKs for top commit:
achow101:
ACK 60978c8080
stickies-v:
re-ACK [60978c8](60978c8080)
hebasto:
ACK 60978c8080
Tree-SHA512: eef0fe1081e9331b95cfafc71d82f2398abd1d3439dac5b2fa5c6d9c0a3f63ef19adde1c38c88d3b4e7fb41ce7c097943f1815c10e33d165918ccbdec512fe1c
6a302d40df wallet: single output groups filtering and grouping process (furszy)
bd91ed1cb2 wallet: unify outputs grouping process (furszy)
55962001da test: coinselector_tests refactor, use CoinsResult instead of plain std::vector (furszy)
34f54a0a3a wallet: decouple outputs grouping process from each ChooseSelectionResult (furszy)
461f0821a2 refactor: make OutputGroup::m_outputs field a vector of shared_ptr (furszy)
d8e749bb84 test: wallet, add coverage for outputs grouping process (furszy)
06ec8f9928 wallet: make OutputGroup "positive_only" filter explicit (furszy)
Pull request description:
The idea originates from https://github.com/bitcoin/bitcoin/pull/24845#issuecomment-1130310321.
Note:
For clarity, it's recommended to start reviewing from the end result to understand the structure of the flow.
#### GroupOutputs function rationale:
If "Avoid Partial Spends" is enabled, the function gathers outputs with the same script together inside a container. So Coin Selection can treats them as if them were just one possible input and either select them all or not select them.
#### How the Inputs Fetch + Selection process roughly works:
```
1. Fetch user’s manually selected inputs.
2. Fetch wallet available coins (walks through the entire wallet txes map) and insert them into a set of vectors (each vector store outputs from a single type).
3. Coin Selection Process:
Call `AttemptSelection` 8 times. Each of them expands the coin eligibility filter (accepting a larger subset of coins in the calculation) until it founds a solutions or completely fails if no solutions gets founds after the 8 rounds.
Each `AttemptSelection` call performs the following actions:
- For each output type supported by the wallet (P2SH, P2PK, P2WPKH, P2WSH and a combination of all of them):
Call ‘ChooseSelectionResult’ providing the respective, filtered by type, coins vector. Which:
I. Groups the outputs vector twice (one for positive only and a second one who includes the negative ones as well).
- GroupOutputs walks-through the entire inputted coins vector one time at least, + more if we are avoiding partial spends, to generate a vector of OutputGroups.
II. Then performs every coin selection algorithm using the recently created vector of OutputGroup: (1) BnB, (2) knapsack and (3) SRD.
III. Then returns the best solution out of them.
```
We perform the general operation of gathering outputs, with the same script, into a single container inside:
Each coins selection attempt (8 times —> each coin eligibility filter), for each of the outputs vector who were filtered by type (plus another one joining all the outputs as well if needed), twice (one for the positive only outputs effective value and a second one for all of them).
So, in the worst case scenario where no solution is found after the 8 Coin Selection attempts, the `GroupOutputs` function is called 80 times (8 * 5 * 2).
#### Improvements:
This proposal streamlines the process so that the output groups, filtered by coin eligibility and type, are created in a single loop outside of the Coin Selection Process.
The new process is as follows:
```
1. Fetch user’s manually selected inputs.
2. Fetch wallet available coins.
3. Group outputs by each coin eligibility filter and each different output type found.
4. Coin Selection Process:
Call AttemptSelection 8 times. Each of them expands the coin eligibility filter (accepting different output groups) until it founds a solutions or completely fails if no solutions gets founds after the 8 rounds.
Each ‘AttemptSelection’ call performs the following actions:
- For each output type supported by the wallet (P2SH, P2PK, P2WPKH, P2WSH and all of them):
A. Call ‘ChooseSelectionResult’ providing the respective, filtered by type, output group. Which:
I. Performs every coin selection algorithm using the provided vector of OutputGroup: (1) BnB, (2) knapsack and (3) SRD.
II. Then returns the best solution out of them.
```
Extra Note:
The next steps after this PR will be to:
1) Merge `AvailableCoins` and `GroupOutputs` processes.
2) Skip entire coin selection rounds if no new coins are added into the subsequent round.
3) Remove global feerates from the OutputGroup class.
4) Remove secondary "grouped" tx creation from `CreateTransactionInternal` by running Coin Selection results over the aps grouped outputs vs non-aps ones.
ACKs for top commit:
S3RK:
ReACK 6a302d4
achow101:
ACK 6a302d40df
theStack:
re-ACK 6a302d40df🥥
Tree-SHA512: dff849063be328e7d9c358ec80239a6db2cd6131963b511b83699b95b337d3106263507eaba0119eaac63e6ac21c6c42d187ae23d79d9220b90c323d44b01d24
e4ede64fe8 Expand scantxoutset help text to cover tr() and miniscript (Greg Sanders)
Pull request description:
ACKs for top commit:
achow101:
ACK e4ede64fe8
darosior:
webACK e4ede64fe8
Tree-SHA512: 6b5d9e7fccc8242f4534861c1b438ec40fb03fbf5968c5a4af5ddbced73df6d666812053c2e12a1e0a34057f6f4fe11987c8c59d8cdfa48ca7ab7d2720b51ef9
4163093d63 wallet: use Mutex for g_sqlite_mutex instead of GlobalMutex (Vasil Dimov)
Pull request description:
Using `Mutex` provides stronger guarantee than `GlobalMutex` wrt Clang's
thread safety analysis. Thus it is better to reduce the usage of
`GlobalMutex` in favor of `Mutex`.
Using `Mutex` for `g_sqlite_mutex` is ok because its usage is limited in
`wallet/sqlite.cpp` and it does not require propagating the negative
annotations to not relevant code.
ACKs for top commit:
achow101:
ACK 4163093d63
hebasto:
re-ACK 4163093d63
TheCharlatan:
ACK 4163093d63
Tree-SHA512: 4913bcb8437ecf0e6b6cb781d02a6d24ffb4bf3e2e1899fa60785eab41c4c65dbdd9600bcb696290c873661b873ad61e5a4c4f205b7e66fdef2ae17c676cd12f
The previous code visualized the `CSubNet` object as string, then
parsed that string back to `CSubNet`. This is sub-optimal given that
the original `CSubNet` object can be used directly instead.
This avoids calling `LookupSubNet()` from the GUI.
Co-authored-by: furszy <matiasfurszyfer@protonmail.com>
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.
8847ce44e0 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 8847ce44e0
Tree-SHA512: 5eb9a6691ee37cbc5033a48aedcbf5c93af3b234614ae14c3fcc858f1ee505f630ad68f8bbb69ffa280080c8d0f91451362cb3819290b741ce906b2b3224a622
987f1bb41c 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 987f1bb41c
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.
545ff924ab refactor: use string_view for RPC named argument values (stickies-v)
7727603e44 refactor: reduce unnecessary complexity in ParseNonRFCJSONValue (stickies-v)
1d02e59901 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 7727603e44, 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 545ff924ab
MarcoFalke:
review ACK 545ff924ab📻
Tree-SHA512: b1c89fb010ac9c3054b023cac1acbba2a539a09cf39a7baffbd7f7571ee268d5a6d98701c7ac10d68a814526e8fd0fe96ac1d1fb072f272033e415b753f64a5c
9c371e50a2 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 9c371e50a2
Tree-SHA512: d11f2fa197bba2e3198ca255d80edd7178e8074dc84b03262054d01fa33c8160ca492a1358e9da1e7d32cec21a298593322d10e4cb045ef99ccec2cbe901cb63
2c9eb4afe1 guix: use cmake-minimal over cmake (fanquake)
1475515312 guix: use coreutils-minimal over coreutils (fanquake)
4445621415 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 2c9eb4afe1
Sjors:
tACK 2c9eb4afe1
achow101:
ACK 2c9eb4afe1
hebasto:
ACK 2c9eb4afe1,
Tree-SHA512: f91ca9e088b8346b20c2affc80870c31640de3aedcfcc0fb98a5e82c77ef64537870b88552f26759d31d8d0956b1fd685e6c25d5acbc92f5feaececd1a7dd37e
