42fb5311b1 rpc: return warnings as an array instead of just a single one (stickies-v)
Pull request description:
The RPC documentation for `getblockchaininfo`, `getmininginfo` and `getnetworkinfo` states that "warnings" returns "any network and blockchain warnings". In practice, only a single warning (i.e. the latest one that is set) is returned, the other ones are ignored.
Fix that by returning all warnings as an array.
As a side benefit, clean up the GetWarnings() logic.
Since this PR changes the RPC result schema, I've added release notes. Users can temporarily revert to the old results by using `-deprecatedrpc=warnings`, until it's removed in a future version.
---
Some historical context from git log:
- when `GetWarnings` was introduced in 401926283a, it was used in the `getinfo` RPC, where only a [single error/warning was returned](401926283a (diff-7442c48d42cd5455a79915a0f00cce5e13359db46437a32b812876edb0a5ccddR250)) (similar to how it is now).
- later on, "warnings" RPC response fields were introduced, e.g. in ef2a3de25c, with the description [stating](ef2a3de25c (diff-1021bd3c74415ad9719bd764ad6ca35af5dfb33b1cd863c0be49bdf52518af54R411)) that it returned "any network warnings" but in practice still only a single warning was returned
ACKs for top commit:
achow101:
re-ACK 42fb5311b1
tdb3:
Re ACK for 42fb5311b1
TheCharlatan:
ACK 42fb5311b1
maflcko:
ACK 42fb5311b1🔺
Tree-SHA512: 4225ed8979cd5f030dec785a80e7452a041ad5703445da79d2906ada983ed0bbe7b15889d663d75aae4a77d92e302c93e93eca185c7bd47c9cce29e12f752bd3
Previously, our Android builds were geared towards generating APKs,
which relied on Qt. However, after migrating to C++20, compiling for
Android became unfeasible due to Qt 5.15's compatibility limitations
with NDK only up to r25, which includes an outdated embedded libc++.
All removed stuff will be reinstated after migrating the build system to
CMake and upgrading Qt to version 6.x."
This also changes behavior if ReadBlockFromDisk or
ReadRawBlockFromDisk fails. Previously, the node would crash
due to an assert. This has been replaced with logging the error,
disconnecting the peer, and returning early.
63317103c9 miniscript: make operator_mst consteval (Pieter Wuille)
Pull request description:
It seems modern compilers don't realize that all invocations of operator""_mst can be evaluated at compile time, despite the `constexpr` keyword.
Since C++20, we can force them to evaluate at compile time using `consteval`, turning all the miniscript type constants into actual compile-time constants.
This should give a nice but not very important speedup for miniscript logic, but it's also a way to start testing C++20 features.
ACKs for top commit:
hebasto:
re-ACK 63317103c9.
theuni:
utACK 63317103c9
Tree-SHA512: bdc9f1a6499b8bb3ca04f1a158c31e6876ba97206f95ee5718f50efd58b5b4e6b8867c07f791848430bfaa130b9676d8a68320b763cda9a340c75527acbfcc9e
976e5d8f7b test: Fix `test/streams_tests.cpp` compilation on SunOS / illumos (Hennadii Stepanov)
Pull request description:
On systems where `int8_t` is defined as `char`, the `{S,Uns}erialize(Stream&, signed char)` functions become undefined.
This PR resolves the issue by testing `{S,Uns}erialize(Stream&, int8_t)` instead.
No behavior change on systems where `int8_t` is defined as `signed char`, which is the case for most other systems.
Fixes https://github.com/bitcoin/bitcoin/issues/29884.
An alternative approach is mentioned in https://github.com/bitcoin/bitcoin/issues/29884#issuecomment-2058434577 as well.
ACKs for top commit:
maflcko:
lgtm ACK 976e5d8f7b
theuni:
ACK 976e5d8f7b. Nice to have the serialization concept actually tested :)
Tree-SHA512: 1033863e584fa8e99a281b236fa01fc919f610a024bcec792116762e28c1c16ee481bd01325c3a0ca9dd9d753176aa63bd9ac7e08a9bbce772db2949d06f6e61
Due to a bug in the legacy wallet, the p2sh maximum script size limit is also imposed
on 'p2sh-segwit' and 'bech32' redeem scripts.
Although redeem scripts over MAX_SCRIPT_ELEMENT_SIZE bytes are technically valid for
segwit output types, we don't want to enable this feature in legacy wallets for the
following reasons:
1) It introduces a compatibility-breaking change requiring downgrade protection; older
wallets would be unable to interact with these "new" legacy wallets.
2) Considering the ongoing deprecation of the legacy spkm, this issue adds another
good reason to transition towards descriptors.
The process currently fails to sign redeem scripts that are longer than
520 bytes. Even when it shouldn't. The 520 bytes redeem scripts limit
is a legacy p2sh rule, and not a segwit limitation.
Segwit redeem scripts are not restricted by the script item size limit.
The reason why this occurs, is the usage of the same keystore used by
the legacy spkm. Which contains blockage for any redeem scripts longer
than the script item size limit.
The multisig script generation process currently fails when
the user exceeds 15 keys, even when it shouldn't. The maximum
number of keys allowed for segwit redeem scripts (p2sh-segwit
and bech32) is 20 keys.
This is because the redeem script placed in the witness is not
restricted by the item size limit.
The reason behind this issue is the utilization of the legacy
p2sh redeem script restrictions on segwit ones. Redeem scripts
longer than 520 bytes are blocked from being inserted into the
keystore, which causes the signing process and the descriptor
inference process to fail.
This occurs because the multisig generation flow uses the same
keystore as the legacy spkm (FillableSigningProvider), which
contains the 520-byte limit.
ffc674595c Replace remaining "520" magic numbers with MAX_SCRIPT_ELEMENT_SIZE (Jon Atack)
Pull request description:
Noticed these while reviewing BIPs yesterday.
It would be clearer and more future-proof to refer to their constant name.
ACKs for top commit:
instagibbs:
ACK ffc674595c
sipa:
ACK ffc674595c
achow101:
ACK ffc674595c
glozow:
ACK ffc674595c, agree it's clearer for these comments to refer to the greppable name of the limit rather than the number
Tree-SHA512: 462afc1c64543877ac58cb3acdb01d42c6d08abfb362802f29f3482d75401a2a8adadbc2facd222a9a9fefcaab6854865ea400f50ad60bec17831d29f7798afe
It seems modern compilers don't realize that all invocations of operator""_mst
can be evaluated at compile time, despite the constexpr keyword.
Since C++20, we can force them to evaluate at compile time, turning all the
miniscript type constants into actual compile-time constants.
It appears that MSVC does not support consteval operator"" when used inside
certain expressions. For the few places where this happens, define a
constant outside the operator call.
Co-Authored-By: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
bd2de7ac59 refactor, test: Always initialize pointer (Hennadii Stepanov)
Pull request description:
This change fixes MSVC warning [C4703](https://learn.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-4-c4703).
All `DisableSpecificWarnings` dropped from `test_bitcoin.vcxproj` as all remained are inherited from `common.init.vcxproj`.
Required to simplify warning suppression porting to the CMake-based build system.
ACKs for top commit:
maflcko:
utACK bd2de7ac59
sipsorcery:
utACK bd2de7ac59.
ryanofsky:
Code review ACK bd2de7ac59
Tree-SHA512: 006db041d3c3697a77d9df14de86cf7c8a10804b45789df01268b2236cf6452e77dc57e89f5d5a6bc26d4b5cd483f0722d6035649c8a523b57954bb1fc810d0c
As described in #10542 (and numerous other places), message signing in
Bitcoin Core only supports message signing using P2PKH addresses, at
least until a new message-signing standard is agreed upon.
Therefore update the possibly-misleading error message presented to the
user in the GUI to detail more specifically the reason their message
cannot be signed, in the case that a non P2PKH address is entered.
b50d127a77 refactor: Make 64-bit shift explicit (Hennadii Stepanov)
Pull request description:
This PR fixes MSVC warning [C4334](https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-3-c4334) in the fuzzing code. Similar to https://github.com/bitcoin/bitcoin/pull/26252.
All `DisableSpecificWarnings` dropped from `fuzz.vcxproj` as all remained are inherited from `common.init.vcxproj`.
Required to simplify warning suppression porting to the CMake-based build system.
ACKs for top commit:
maflcko:
utACK b50d127a77
sipsorcery:
utACK b50d127a77
Tree-SHA512: 18f6082b4234506ad2f9df54e577031b97cdf9f7ef64cad4162f275660716ab73587a97d3af0f778dfd48d2751d8676b5d3381d0aa837fcc60a09704473a9209
8b52e7f628 update comments in cpp-subprocess (check_output references) (Sebastian Falbesoner)
97f159776e remove unused method `Popen::kill` from cpp-subprocess (Sebastian Falbesoner)
908c51fe4a remove commented out code in cpp-subprocess (Sebastian Falbesoner)
ff79adbe05 remove unused templates from cpp-subprocess (Sebastian Falbesoner)
Pull request description:
This PR removes remaining code that is unused within the cpp-subprocess module (templates and commented out code). Happy to add more removals if anyone finds more unused parts. Note that there are some API functions of the `Popen` class that we don't use, e.g. `wait()`, `pid()`, `poll()`, `kill()`, but they sound IMHO common enough to be useful in the future, so not sure how deep we should go there.
ACKs for top commit:
fjahr:
Code review ACK 8b52e7f628
achow101:
ACK 8b52e7f628
hebasto:
ACK 8b52e7f628.
Tree-SHA512: 14c1cd2216185d941923f06fdc7acbeed66cd87e2691d9a352f7309b3e07fe4877b580f598a2e4106f9c48395ed6de00a0bfb5d3c3af9c4624d1956a0f543e99
e504b1fa1f test: Add test case for spending bare multisig (Brandon Odiwuor)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/issues/29113
ACKs for top commit:
ajtowns:
ACK e504b1fa1f ; LGTM and just checking the 1-of-3 case seems fine
maflcko:
utACK e504b1fa1f
achow101:
ACK e504b1fa1f
willcl-ark:
reACK e504b1fa1f
Tree-SHA512: 641a12599efa34e1a3eb65b125318df326628fef3e6886410ea9e63a044664fad7bcad46d1d6f41ddc59630746b9963cedb569c2682b5940b32b9225883da8f2
Adds the following fixups in txorphan fuzz tests:
- Don't bond the output count of the created orphans based on the number of available coins
- Allow duplicate inputs, when applicable, but don't store duplicate outpoints
Rationale
---------
The way the test is currently written, duplicate inputs are allowed based on a random flag (`duplicate_input`).
If the flag is unset, upon selecting an outpoint as input for a new transaction, the input is popped to prevent re-selection,
and later re-added to the collection (once all inputs have been picked). However, the re-addition to the collection is performed independently of whether the flag was set or not.
This means that, if the flag is set, the selected inputs are duplicated which in turn makes these inputs more likely to be re-picked in the following iteration of the loop.
Additionally, both the input and output count of the transaction and bonded to the number of available outpoints. This makes sense for the former, but the latter shouldn't be.
The RPC documentation for `getblockchaininfo`, `getmininginfo` and
`getnetworkinfo` states that "warnings" returns "any network and
blockchain warnings". In practice, only a single warning is returned.
Fix that by returning all warnings as an array.
As a side benefit, cleans up the GetWarnings() logic.
774359b4a9 build, msvc: Compile `test\fuzz\bitdeque.cpp` (Hennadii Stepanov)
85f50a46c5 refactor: Fix "error C2248: cannot access private member" on MSVC (Hennadii Stepanov)
Pull request description:
This PR resolves one point from the https://github.com/bitcoin/bitcoin/pull/29774#issuecomment-2028808614:
> What is the issue with the bitdeque... ?
ACKs for top commit:
maflcko:
lgtm ACK 774359b4a9
sipa:
utACK 774359b4a9
achow101:
ACK 774359b4a9
dergoegge:
utACK 774359b4a9
Tree-SHA512: dba5c0217b915468af08475795437a10d8e8dedfadeb319f36d9b1bf54a91a8b2c61470a6047565855276c2bc8589c7776dc19237610b65b57cc841a303de8b3
82f41d76f1 Added seednode prioritization message to help output (tdb3)
3120a4678a Gives seednode priority over dnsseed if both are provided (Sergi Delgado Segura)
Pull request description:
This is a follow-up of #27577
If both `seednode` and `dnsseed` are provided, the node will start a race between them in order to fetch data to feed the `addrman`.
This PR gives priority to `seednode` over `dnsseed` so if some nodes are provided as seeds, they can be tried before defaulting to the `dnsseeds`
ACKs for top commit:
davidgumberg:
untested reACK 82f41d76f1
itornaza:
tested re-ACK 82f41d76f1
achow101:
ACK 82f41d76f1
cbergqvist:
ACK 82f41d76f1
Tree-SHA512: 4e39e10a7449af6cd9b8f9f6878f846b94bca11baf89ff2d4fbcd4f28293978a6ed71a3a86cea36d49eca891314c834e32af93f37a09c2cc698a878f84d31c62
c6be144c4b Remove timedata (stickies-v)
92e72b5d0d [net processing] Move IgnoresIncomingTxs to PeerManagerInfo (dergoegge)
7d9c3ec622 [net processing] Introduce PeerManagerInfo (dergoegge)
ee178dfcc1 Add TimeOffsets helper class (stickies-v)
55361a15d1 [net processing] Use std::chrono for type-safe time offsets (stickies-v)
038fd979ef [net processing] Move nTimeOffset to net_processing (dergoegge)
Pull request description:
[An earlier approach](1d226ae1f9/) in #28956 involved simplifying and refactoring the network-adjusted time calculation logic, but this was eventually [left out](https://github.com/bitcoin/bitcoin/pull/28956#issuecomment-1904214370) of the PR to make it easier for reviewers to focus on consensus logic changes.
Since network-adjusted time is now only used for warning/informational purposes, cleaning up the logic (building on @dergoegge's approach in #28956) should be quite straightforward and uncontroversial. The main changes are:
- Previously, we would only calculate the time offset from the first 199 outbound peers that we connected to. This limitation is now removed, and we have a proper rolling calculation. I've reduced the set to 50 outbound peers, which seems plenty.
- Previously, we would automatically use the network-adjusted time if the difference was < 70 mins, and warn the user if the difference was larger than that. Since there is no longer any automated time adjustment, I've changed the warning threshold to ~~20~~ 10 minutes (which is an arbitrary number).
- Previously, a warning would only be raised once, and then never again until node restart. This behaviour is now updated to 1) warn to log for every new outbound peer for as long as we appear out of sync, 2) have the RPC warning toggled on/off whenever we go in/out of sync, and 3) have the GUI warn whenever we are out of sync (again), but limited to 1 messagebox per 60 minutes
- no more globals
- remove the `-maxtimeadjustment` startup arg
Closes#4521
ACKs for top commit:
sr-gi:
Re-ACK [c6be144](c6be144c4b)
achow101:
reACK c6be144c4b
dergoegge:
utACK c6be144c4b
Tree-SHA512: 1063d639542e882186cdcea67d225ad1f97847f44253621a8c4b36c4d777e8f5cb0efe86bc279f01e819d33056ae4364c3300cc7400c087fb16c3f39b3e16b96
e518a8bf8a [functional test] opportunistic 1p1c package submission (glozow)
87c5c524d6 [p2p] opportunistically accept 1-parent-1-child packages (glozow)
6c51e1d7d0 [p2p] add separate rejections cache for reconsiderable txns (glozow)
410ebd6efa [fuzz] break out parent functions and add GetChildrenFrom* coverage (glozow)
d095316c1c [unit test] TxOrphanage::GetChildrenFrom* (glozow)
2f51cd680f [txorphanage] add method to get all orphans spending a tx (glozow)
092c978a42 [txpackages] add canonical way to get hash of package (glozow)
c3c1e15831 [doc] restore comment about why we check if ptx HasWitness before caching rejected txid (glozow)
6f4da19cc3 guard against MempoolAcceptResult::m_replaced_transactions (glozow)
Pull request description:
This enables 1p1c packages to propagate in the "happy case" (i.e. not reliable if there are adversaries) and contains a lot of package relay-related code. See https://github.com/bitcoin/bitcoin/issues/27463 for overall package relay tracking.
Rationale: This is "non-robust 1-parent-1-child package relay" which is immediately useful.
- Relaying 1-parent-1-child CPFP when mempool min feerate is high would be a subset of all package relay use cases, but a pretty significant improvement over what we have today, where such transactions don't propagate at all. [1]
- Today, a miner can run this with a normal/small maxmempool to get revenue from 1p1c CPFP'd transactions without losing out on the ones with parents below mempool minimum feerate.
- The majority of this code is useful for building more featureful/robust package relay e.g. see the code in #27742.
The first 2 commits are followups from #29619:
- https://github.com/bitcoin/bitcoin/pull/29619#discussion_r1523094034
- https://github.com/bitcoin/bitcoin/pull/29619#discussion_r1519819257
Q: What makes this short of a more full package relay feature?
(1) it only supports packages in which 1 of the parents needs to be CPFP'd by the child. That includes 1-parent-1-child packages and situations in which the other parents already pay for themselves (and are thus in mempool already when the package is submitted). More general package relay is a future improvement that requires more engineering in mempool and validation - see #27463.
(2) We rely on having kept the child in orphanage, and don't make any attempt to protect it while we wait to receive the parent. If we are experiencing a lot of orphanage churn (e.g. an adversary is purposefully sending us a lot of transactions with missing inputs), we will fail to submit packages. This limitation has been around for 12+ years, see #27742 which adds a token bucket scheme for protecting package-related orphans at a limited rate per peer.
(3) Our orphan-handling logic is somewhat opportunistic; we don't make much effort to resolve an orphan beyond asking the child's sender for the parents. This means we may miss packages if the first sender fails to give us the parent (intentionally or unintentionally). To make this more robust, we need receiver-side logic to retry orphan resolution with multiple peers. This is also an existing problem which has a proposed solution in #28031.
[1]: see this writeup and its links 02ec218c78/bip-0331.mediawiki (propagate-high-feerate-transactions)
ACKs for top commit:
sr-gi:
tACK e518a8bf8a
instagibbs:
reACK e518a8bf8a
theStack:
Code-review ACK e518a8bf8a📦
dergoegge:
light Code review ACK e518a8bf8a
achow101:
ACK e518a8bf8a
Tree-SHA512: 632579fbe7160cb763bbec6d82ca0dab484d5dbbc7aea90c187c0b9833b8d7c1e5d13b8587379edd3a3b4a02a5a1809020369e9cd09a4ebaf729921f65c15943
6a8b2befea refactor: Avoid copying util::Result values (Ryan Ofsky)
834f65e824 refactor: Drop util::Result operator= (Ryan Ofsky)
Pull request description:
This PR just contains the first two commits of #25665.
It disables copying of `util::Result` objects because unnecessary copies are inefficient and not possible after #25665, which makes `util::Result` object move-only.
It disables the assignment operator and replaces it with an `Update()` method, because #25665 adds more information to `util::Result` objects (warning and error messages and failure values) and having an assignment operator that overwrites data instead of merging it would make it easy to accidentally erase existing information while trying to assign new information.
ACKs for top commit:
stickies-v:
re-ACK 6a8b2befea
achow101:
ACK 6a8b2befea
furszy:
re-ACK 6a8b2befea
Tree-SHA512: 3f21af9031d50d6c68cca69133de03080f69b1ddcf8b140bdeb762069f14645209b2586037236d15b6ebd8973af0fbefd7e83144aeb7b84078a4cb4df812f984
cc15c5bfd1 fuzz: don't allow adding duplicate transactions to the mempool (Suhas Daftuar)
Pull request description:
Filter duplicate transaction ids from being added to the mempool in the `partially_downloaded_block` fuzz target.
I think a prerequisite for calling `CTxMemPool::addUnchecked` should be that the underlying txid doesn't already exist in the mempool (otherwise `addUnchecked` would need a way to return failure, which we don't currently have).
ACKs for top commit:
glozow:
utACK cc15c5bfd1 makes sense to me
maflcko:
lgtm ACK cc15c5bfd1
brunoerg:
ACK cc15c5bfd1
dergoegge:
utACK cc15c5bfd1
Tree-SHA512: 85f84ce405aba584e6d00391515f0a86c5648ce8b2da69036e50a6c1f6833d050d09b1972cc5ffbe7c4edb3e5f7f965ef34bd839deeddac27a889cc8d2e53b8f
30a6c99935 rpc: access some args by name (stickies-v)
bbb31269bf rpc: add named arg helper (stickies-v)
13525e0c24 rpc: add arg helper unit test (stickies-v)
Pull request description:
Adds string overloads for the `RPCHelpMan::Arg` and `RPCHelpMan::MaybeArg` helpers to be able to access RPC arguments by name instead of index number. Especially in RPCs with a large number of parameters, this can be quite helpful.
Example usage:
```cpp
const auto action{self.Arg<std::string>("action")};
```
Most of the LoC is adding test coverage and documentation updates. No behaviour change.
An alternative approach to #27788 with significantly less overhaul.
ACKs for top commit:
fjahr:
Code review ACK 30a6c99935
maflcko:
ACK 30a6c99935🥑
ryanofsky:
Code review ACK 30a6c99935. Nice change! Implementation is surprisingly simple and additional unit test coverage is welcome, too.
Tree-SHA512: 4904f5f914fe1d421d32f60edb7c5a028c8ea0f140a2f207a106b4752d441164e073066a6bf2e17693f859fe847815a96609d3cf521e0ac4178d8cd09362ea3d
fae0db555c refactor: Use chrono type for g_mock_time (MarcoFalke)
fa382d3dd0 test: Add missing Assert(mock_time_in >= 0s) to SetMockTime (MarcoFalke)
Pull request description:
Seems odd to have the assert in the *deprecated* function, but not in the other.
Fix this by adding it to the other, and by inlining the deprecated one.
Also, use chrono type for the global mocktime variable.
ACKs for top commit:
davidgumberg:
crACK fae0db555c
stickies-v:
ACK fae0db555c
Tree-SHA512: 630c2917422ff2a7fa307114f95f22ad3c205429ffe36e67f0b2650733e40c876289c1aecebe882a9123d3106db7606bd6eff067ed6e2ecb95765984d3fe8612
Remove obsolete `check_output` references in the comments and remove
the numbering of the Popen API methods, as they don't seem to provide a
value and just make diffs larger for future changes.
Checking the interface name is kind of brittle. In the age of network
namespaces and containers, there is no reason a loopback interface can't
be called differently.
Check for the `IFF_LOOPBACK` flag to detect loopback interface instead.
It's very hard to randomly construct a transaction that would be the
parent of an existing orphanage tx. For functions like
AddChildrenToWorkSet and GetChildren that take orphan parents, use a tx
that was previously constructed.
Copying util::Result values is less efficient than moving them because they
allocate memory and contain strings. Also this is needed to avoid compile
errors in https://github.com/bitcoin/bitcoin/pull/25722 which adds a
std::unique_ptr member to util::Result which implicity disables copying.
fd81a37239 net: attempts to connect to all resolved addresses when connecting to a node (Sergi Delgado Segura)
Pull request description:
This is a follow-up of #28155 motivated by https://github.com/bitcoin/bitcoin/pull/28155#discussion_r1362677038
## Rationale
Prior to this, when establishing a network connection via `CConnman::ConnectNode`, if the connection needed address resolution, a single address would be picked at random from the resolved addresses and our node would try to connect to it. However, this would lead to the behavior of `ConnectNode` being unpredictable when the address was resolved to various ips (e.g. the address resolving to IPv4 and IPv6, but we only support one of them).
This patches the aforementioned behavior by going over all resolved IPs until a valid one is found or until we
exhaust them.
ACKs for top commit:
mzumsande:
re-ACK fd81a37239 (just looked at diff, only small logging change)
achow101:
ACK fd81a37239
vasild:
ACK fd81a37239
Tree-SHA512: fa1ebc5c84fe61dd0a7fe1113ae2d594a75ad661c43ed8984a31fc9bc50f166b2759b0d8d84ee5dc247691eff78c8156fac970af797bbcbf67492eec0353fb58
`util::Result` objects are aggregates that can hold multiple fields with
different information. Currently Result objects can only hold a success value
of an arbitrary type or a single bilingual_str error message. In followup PR
https://github.com/bitcoin/bitcoin/pull/25722, Result objects may be able to
hold both success and failure values of different types, plus error and warning
messages.
Having a Result::operator= assignment operator that completely erases all
existing Result information before assigning new information is potentially
dangerous in this case. For example, code that looks like it is assigning a
warning value could erase previously-assigned success or failure values.
Conversely, code that looks like it is just assigning a success or failure
value could erase previously assigned error and warning messages.
To prevent potential bugs like this, disable Result::operator= assignment
operator.
It is possible in the future we may want to re-enable operator= in limited
cases (such as when implicit conversions are not used) or add a Replace() or
Reset() method that mimicks default operator= behavior. Followup PR
https://github.com/bitcoin/bitcoin/pull/25722 also adds a Result::Update()
method providing another way to update an existing Result object.
Co-authored-by: stickies-v <stickies-v@protonmail.com>
65951e0418 index: race fix, lock cs_main while 'm_synced' is subject to change (Ryan Ofsky)
Pull request description:
Fixes#29831 and #29863. Thanks to Marko for the detailed description of the issue.
The race occurs because a block could be connected and its event signaled in-between reading the 'next block' and setting `m_synced` during the index initial synchronization. This is because `cs_main` is not locked through the process of determining the final index sync state.
To address the issue, the `m_synced` flag set has been moved under `cs_main` guard.
ACKs for top commit:
fjahr:
Code review ACK 65951e0418
achow101:
ACK 65951e0418
ryanofsky:
Code review ACK 65951e0418
Tree-SHA512: 77286e22de164a27939d2681b7baa6552eb75e99c541d3b9631f4340d7dd01742667c86899b6987fd2d97799d959e0a913a7749b2b69d9e50505128cd3ae0e69
992c714451 common: Don't terminate on null character in UrlDecode (Fabian Jahr)
099fa57151 scripted-diff: Modernize name of urlDecode function and param (Fabian Jahr)
8f39aaae41 refactor: Remove hooking code for urlDecode (Fabian Jahr)
650d43ec15 refactor: Replace libevent use in urlDecode with our own code (Fabian Jahr)
46bc6c2aaa test: Add unit tests for urlDecode (Fabian Jahr)
Pull request description:
Fixes#29654 (as a side-effect)
Removing dependencies is a general goal of the project and the xz backdoor has been an additional wake up call recently. Libevent shows many of the same symptoms, few maintainers and slow releases. While libevent can not be removed completely over night we should start removing it’s usage where it's possible, ideally with the end goal to removing it completely.
This is a pretty easy win in that direction. The [`evhttp_uridecode` function from libevent](e0a4574ba2/http.c (L3542)) we were using in `urlDecode` could be easily emulated in fewer LOC. This also ports the [applicable test vectors over from libevent](https://github.com/libevent/libevent/blob/master/test/regress_http.c#L3430).
ACKs for top commit:
achow101:
ACK 992c714451
theStack:
Code-review ACK 992c714451
maflcko:
ACK 992c714451👈
stickies-v:
ACK 992c714451
Tree-SHA512: 78f76ae7ab3b6710eab2aaac20f55eb0da7803e057eaa6220e865f328666a5399ef1a479702aaf630b2f974ad3aa15e2b6adac9c11bc8c3d4be21e8af1667fea
The previous behavior was the result of casting the result returned from the libevent function evhttp_uridecode to std:string but this was probably not intended.
The point of this was to be able to build bitcoin-tx and bitcoin-wallet without libevent, see #18504.
Now that we use our own implementation of urlDecode this is not needed anymore.
Co-authored-by: stickies-v <stickies-v@protonmail.com>
b22901dfa9 Avoid explicitly computing diagram; compare based on chunks (Pieter Wuille)
Pull request description:
This merges the `BuildDiagramFromChunks` and `CompareFeeRateDiagram` introduced in #29242 into a single `CompareChunks` function, which operates on sorted chunk data rather than diagrams, instead computing the diagram on the fly.
This avoids the need for the construction of an intermediary diagram object, and removes the slightly arbitrary "all diagrams must start at (0, 0)" requirement.
Not a big deal, but I think the result is a bit cleaner and not really more complicated.
ACKs for top commit:
glozow:
reACK b22901d
instagibbs:
reACK b22901dfa9
Tree-SHA512: ca37bdf61d9a9cb5435f4da73e97ead33bf65828ad9af49b87336b1ece70db8ced1c21f517fc6eb6d616311c91f3da75ecae6b9bd42547133e3a3c5320b7816d
08f756bd37 Replace locale-dependent `std::strerror` with `SysErrorString` (Hennadii Stepanov)
d8e4ba4d05 refactor: Rename `subprocess.hpp` to follow our header name conventions (Hennadii Stepanov)
Pull request description:
This PR renames the header `*.hpp` --> `*.h` and adjusts the header guard name, which makes it available for processing by linters.
Fixed the following linter warning:
```
The locale dependent function strerror(...) appears to be used:
src/util/subprocess.h: std::runtime_error( err_msg + ": " + std::strerror(err_code) )
Unnecessary locale dependence can cause bugs that are very tricky to isolate and fix. Please avoid using locale-dependent functions if possible.
Advice not applicable in this specific case? Add an exception by updating the ignore list in /bitcoin/test/lint/lint-locale-dependence.py
^---- failure generated from lint-locale-dependence.py
```
ACKs for top commit:
TheCharlatan:
ACK 08f756bd37
Tree-SHA512: 57a2f01c20eb9552481e428a4969bd59e9ada9f784fe1a45cb62aa9c9152c8e950d336854f45af0e2e5dc7c7b2a1fb216c8f832e3d6ccfb457ad71b6e423231e
4d8d21320e sign: don't assume we are parsing a sane Miniscript (Antoine Poinsot)
Pull request description:
The script provided for signature might be externally provided, for instance by way of 'finalizepsbt'. Therefore the script might be ill-crafted, so don't assume pubkeys are always 32 bytes.
Thanks to Niklas for finding this.
FIxes https://github.com/bitcoin/bitcoin/issues/29851.
ACKs for top commit:
achow101:
ACK 4d8d21320e
furszy:
ACK 4d8d21320e with a small nuance that could be tackled in a follow-up by someone else (or never).
Tree-SHA512: 29b7948b56e6dc05eac1014d684f2129ab1d19cb1e5d304216c826b7057c0e1d84ceb18731b91124b680e17d90e38de9f9a5526e4f6ecc3ea816881a6599bb47
fa6ab0d020 rpc: Reword SighashFromStr error message (MarcoFalke)
Pull request description:
Put quotes around the parameter. In theory, `std::quoted` should be used, but that seems overkill.
This should avoid error messages such as `A valid sighash parameter is not a valid sighash parameter. (code -8)`.
Also, it should fix fuzz false positives when searching for internal bugs in the `rpc` fuzz target. For example, `ZGVzY3JpcHRvcnByb2Nlc3Nwc2J0XP9ce1tdXOVJbnRlcm5hbCBidWcgZGV0ZWN0ZWQAXQ0AHfcAXQ1p7TJv`.
ACKs for top commit:
dergoegge:
ACK fa6ab0d020
brunoerg:
utACK fa6ab0d020
Tree-SHA512: e2c0cc0126de61873a863af38b7b0a23d2dadd596ca0418dae2ad091e8acfb6a9d657c376d59187bb008989dc78c6b44fe518590e5217e4049a867b220c9fb18
fb4cc5f423 netbase: clean up Proxy logging (Matthew Zipkin)
Pull request description:
Follow up to #27375 and see https://github.com/bitcoin/bitcoin/pull/29649#issuecomment-2057456834
This removes an extra log message when we can't connect to our own proxy, and another when the proxy is invalid.
## Before #27375 if proxy is unreachable
```
2024-04-15T17:54:51Z connect() to 127.0.0.1:9999 failed after wait: Connection refused (61)
2024-04-15T17:54:52Z connect() to 127.0.0.1:9999 failed after wait: Connection refused (61)
2024-04-15T17:54:52Z connect() to 127.0.0.1:9999 failed after wait: Connection refused (61)
2024-04-15T17:54:53Z connect() to 127.0.0.1:9999 failed after wait: Connection refused (61)
2024-04-15T17:54:53Z connect() to 127.0.0.1:9999 failed after wait: Connection refused (61)
```
## After #27375 if unix proxy is unreachable:
```
2024-04-15T17:54:03Z connect() to /Users/matthewzipkin/Desktop/tor failed: No such file or directory (2)
2024-04-15T17:54:03Z Cannot connect to socket for /Users/matthewzipkin/Desktop/tor
2024-04-15T17:54:04Z connect() to /Users/matthewzipkin/Desktop/tor failed: No such file or directory (2)
2024-04-15T17:54:04Z Cannot connect to socket for /Users/matthewzipkin/Desktop/tor
2024-04-15T17:54:04Z connect() to /Users/matthewzipkin/Desktop/tor failed: No such file or directory (2)
2024-04-15T17:54:04Z Cannot connect to socket for /Users/matthewzipkin/Desktop/tor
2024-04-15T17:54:05Z connect() to /Users/matthewzipkin/Desktop/tor failed: No such file or directory (2)
2024-04-15T17:54:05Z Cannot connect to socket for /Users/matthewzipkin/Desktop/tor
```
## After this PR:
```
2024-04-15T18:18:51Z connect() to /Users/matthewzipkin/Desktop/tor failed: No such file or directory (2)
2024-04-15T18:18:51Z connect() to /Users/matthewzipkin/Desktop/tor failed: No such file or directory (2)
2024-04-15T18:18:52Z connect() to /Users/matthewzipkin/Desktop/tor failed: No such file or directory (2)
2024-04-15T18:18:52Z connect() to /Users/matthewzipkin/Desktop/tor failed: No such file or directory (2)
```
ACKs for top commit:
tdb3:
CR ACK for fb4cc5f423
laanwj:
ACK fb4cc5f423
Tree-SHA512: f07b9f7f2ea9f4bc01780c09f0b076547108294a1fa7d158a0dd48d6d7351569e461e5cccf232b7b1413ce2e3679668e523e5a7c89cd58c909da76d3dcbc34de
4357158c47 wallet: return and display signer error (Sjors Provoost)
dc55531087 wallet: compare address returned by displayaddress (Sjors Provoost)
6c1a2cc09a test: use h marker for external signer mock (Sjors Provoost)
Pull request description:
* HWI returns the requested address: as a sanity check, we now compare that to what we expected
* external signer documentation now reflects that HWI alternatives must implement this check
* both RPC and GUI will now return an error text, rather than just fail (the GUI even failed silently in some cases)
ACKs for top commit:
brunoerg:
ACK 4357158c47
achow101:
ACK 4357158c47
Tree-SHA512: 4f56edf3846745c8e7d08ef55cf29e8bb468256457149377c5f02da097931f9ca0c06bdbd856dc2385cde4fd11e4dc3b634c5a48814ff27f5562c8a25d43da93
13adbf733f remove unneeded environment option from cpp-subprocess (Sebastian Falbesoner)
2088777ba0 remove unneeded cwd option from cpp-subprocess (Sebastian Falbesoner)
03ffb09c31 remove unneeded bufsize option from cpp-subprocess (Sebastian Falbesoner)
79c3036373 remove unneeded close_fds option from cpp-subprocess (Sebastian Falbesoner)
62db8f8e5a remove unneeded session_leader option from cpp-subprocess (Sebastian Falbesoner)
80d008c66d remove unneeded defer_spawn option from cpp-subprocess (Sebastian Falbesoner)
cececad7b2 remove unneeded preexec function option from cpp-subprocess (Sebastian Falbesoner)
633e45b2e2 remove unneeded shell option from cpp-subprocess (Sebastian Falbesoner)
Pull request description:
The newly introduced cpp-subprocess library provides a good number of options for the `Popen` class:
0de63b8b46/src/util/subprocess.hpp (L1009-L1020)
Some of them are either not fully implemented (`shell`, missing an implementation on Windows), implemented in an ugly way (e.g. using "Impoverished, meager, needy, truly needy version of type erasure" for `preexec_func` according to the author's own words) or simply unlikely to be ever needed for our external signer use-case (`defer_spawn`). Instead of maintaining incomplete and/or unneeded code, I'd suggest to get rid of it and only keep support for options if there is a strong reason for it.
ACKs for top commit:
achow101:
ACK 13adbf733f
hebasto:
re-ACK 13adbf733f.
Tree-SHA512: 8270da27891cb659da2ef6062a23f4b86331859b15ac27b79ae7433b14f5bd7efaba621f2b3ba1953708d0f38377a8bd23ef1cc0f28b9c152ac8958dd9eec6b0
c3e632b441 Bugfix: bitcoin-cli: Check length of peer.transport_protocol_type (Luke Dashjr)
Pull request description:
"v" would dereference beyond the string length, and "v10" would show as '1'
Turn both of these cases into a blank, like anything else unexpected currently is.
ACKs for top commit:
sipa:
utACK c3e632b441.
hernanmarino:
utACK c3e632b441
alfonsoromanz:
ACK c3e632b441
achow101:
ACK c3e632b441
Tree-SHA512: f641e4412521adae7c8c8e1f268bdaaa223d9048d8286e3df4b13905faaa0d601155ce581cd649f760cab2acc4122356fa94a44714f1f190845552100105eda0
The script provided for signature might be externally provided, for
instance by way of 'finalizepsbt'. Therefore the script might be
ill-crafted, so don't assume pubkeys are always 32 bytes.
Thanks to Niklas for finding this.
f2e3662e57 net: Decrease nMaxIPs when learning from DNS seeds (laanwj)
Pull request description:
Limit number of IPs learned from a single DNS seed to 32, to prevent the results from one DNS seed from dominating AddrMan. Note that the number of results from a UDP DNS query is bounded to 33 already, but it is possible for it to use TCP where a larger number of results can be returned.
Closes#16070.
ACKs for top commit:
Sjors:
utACK f2e3662e57
achow101:
ACK f2e3662e57
1440000bytes:
utACK f2e3662e57
mzumsande:
utACK f2e3662e57
Tree-SHA512: 3f108c2baba7adfedb8019daaf60aa00e628b38d3942e1319c7183a4683670be01929ced9e6372c8e983c902e8633f81fbef12d7cdcaadd7f77ed729c1019942
21d0e6c7b7 doc: release notes for PR 27679 (Matthew Zipkin)
791dea204e test: cover unix sockets in zmq interface (Matthew Zipkin)
c87b0a0ff4 zmq: accept unix domain socket address for notifier (Matthew Zipkin)
Pull request description:
This is a follow-up to https://github.com/bitcoin/bitcoin/pull/27375, allowing ZMQ notifications to be published to a UNIX domain socket.
Fortunately, libzmq handles unix sockets already, all we really have to do to support it is allow the format in the actual option.
[libzmq](https://libzmq.readthedocs.io/en/latest/zmq_ipc.html) uses the prefix `ipc://` as opposed to `unix:` which is [used by Tor](https://gitlab.torproject.org/tpo/core/tor/-/blob/main/doc/man/tor.1.txt?ref_type=heads#L1475) and now also by [bitcoind](a85e5a7c9a/doc/release-notes-27375.md (L5)) so we need to switch that internally.
As far as I can tell, [LND](d20a764486/zmq.go (L38)) supports `ipc://` and `unix://` (notice the double slashes).
With this patch, LND can connect to bitcoind using unix sockets:
Example:
*bitcoin.conf*:
```
zmqpubrawblock=unix:/tmp/zmqsb
zmqpubrawtx=unix:/tmp/zmqst
```
*lnd.conf*:
```
bitcoind.zmqpubrawblock=ipc:///tmp/zmqsb
bitcoind.zmqpubrawtx=ipc:///tmp/zmqst
```
ACKs for top commit:
laanwj:
Code review ACK 21d0e6c7b7
tdb3:
crACK for 21d0e6c7b7. Changes lgtm. Will follow up with some testing within the next few days as time allows.
achow101:
ACK 21d0e6c7b7
guggero:
Tested and code review ACK 21d0e6c7b7
Tree-SHA512: ffd50222e80dd029d903e5ddde37b83f72dfec1856a3f7ce49da3b54a45de8daaf80eea1629a30f58559f4b8ded0b29809548c0638cd1c2811b2736ad8b73030
016ed248ba fuzz: explicitly cap the vsize of RBFs for diagram checks (Greg Sanders)
Pull request description:
In master we are hitting a case where vsize transactions much larger than max standard size are causing an overflow in not-yet-exposed RBF diagram checking code: https://github.com/bitcoin/bitcoin/pull/29757#issuecomment-2049220195
`ConsumeTxMemPoolEntry` is creating entries with tens of thousands of sigops cost, causing the resulting RBFs to be "overly large".
To fix this I cause the fuzz test to stop adding transactions to the mempool when we reach a potential overflow of `int32_t`.
ACKs for top commit:
glozow:
ACK 016ed248ba
marcofleon:
ACK 016ed248ba. I ran libFuzzer on `package_rbf` on the current master branch until the overflow was encountered. Then I built the PR branch and ran the fuzzer using the crash input.
Tree-SHA512: b3ffc98d2c4598eb3010edd58b9370aab1441aafbb1044c83b2b90c17dfe9135b8de9dba475dd0108863c1ffedede443cd978e95231a41cf1f0715629197fa51
Prior to this, when establishing a network connection via CConnman::ConnectNode,
if the connection needed address resolution, a single address would be picked
at random from the resolved addresses and our node will try to connect to it. However,
this would lead to the behavior of ConnectNode being unpredictable when the address
was resolved to various ips (e.g. the address resolving to IPv4 and IPv6, but we only
support one of them).
This patches the aforementioned behavior by going over all resolved IPs until we find one
we can connect to or until we exhaust them.
On systems where `int8_t` is defined as `char`, the
`{S,Uns}erialize(Stream&, signed char)` functions become undefined.
This change resolves the issue by testing
`{S,Uns}erialize(Stream&, int8_t)` instead.
No behavior change on systems where `int8_t` is defined as
`signed char`, which is the case for most other systems.
c6d1b8de89 gui: change example address from legacy (P2PKH) to bech32m (P2TR) (Sebastian Falbesoner)
Pull request description:
Legacy addresses are less and less common these days and not recommended to use, so it seems senseful to also reflect that in the example addresses and update to the most recent address / output type (bech32m / P2TR). Also, as I couldn't see any value in computing these at runtime, they are pre-generated. This was done with the following Python script, executed in `./test/functional` (it's also included in the commit body, though without the she-bang):
```python
#!/usr/bin/env python3
from test_framework.segwit_addr import CHARSET, decode_segwit_address, encode_segwit_address
from test_framework.messages import sha256
output_key = sha256(b'bitcoin dummy taproot output key')
for network, hrp in [('mainnet', 'bc'), ('signet', 'tb'), ('testnet', 'tb'), ('regtest', 'bcrt')]:
dummy_address = encode_segwit_address(hrp, 1, output_key)
while decode_segwit_address(hrp, dummy_address) != (None, None):
last_char = CHARSET[(CHARSET.index(dummy_address[-1]) + 1) % 32]
dummy_address = dummy_address[:-1] + last_char
print(f'{network:7} example address: {dummy_address}')
```
Note that the last bech32 character is modified in order to make the checksum fail.
master (mainnet):
PR (mainnet):
ACKs for top commit:
maflcko:
lgtm ACK c6d1b8de89
pablomartin4btc:
tACK c6d1b8de89
Tree-SHA512: a53c267a3e0d29b9c41bf043b123e7152fbf297e2322d74ce047ba2582b54768187162d462cc334e91a84874731c2e0793726ad44d9970c10ecfe70a1d4f3f1c
05416422d3 refactor: Avoid implicit-integer-sign-change in processNewTransaction (MarcoFalke)
321f105d08 refactor: Avoid implicit-signed-integer-truncation-or-sign-change in FreedesktopImage (MarcoFalke)
6d8eecd33a refactor: Avoid implicit-integer-sign-change in createTransaction (MarcoFalke)
Pull request description:
This is allowed by the language. However, the `integer` sanitizer complains about it. Thus, fix it, so that the `integer` sanitizer can be used in the future to catch unintended sign changes.
Fixes#805.
ACKs for top commit:
pablomartin4btc:
tACK 05416422d3
hebasto:
ACK 05416422d3, I have reviewed the code and it looks OK.
Tree-SHA512: eaa941479bd7bee196eb8b31d93b8e1db122410cf62e8ec4cbbec35cfd14cc766081c3df5dd14a228e21ad2678d8b8ba0d2649e5934c994a90ae96d8b264b4ce
This ensures that the index does not miss any 'new block' signals
occurring in-between reading the 'next block' and setting 'm_synced'.
Because, if this were to happen, the ignored blocks would never be
indexed, thus stalling the index forever.
fa4c69669e test: Fix failing univalue float test (MarcoFalke)
Pull request description:
Currently the test may fail for some compilers, because `1e-8` may not be possible to represent exactly/consistently.
```
$ ./src/univalue/test/object
object: univalue/test/object.cpp:424: void univalue_readwrite(): Assertion `v.read("0.00000000000000000000000000000000000001e+30 ") && v.get_real() == 1e-8' failed.
Aborted (core dumped)
```
Fixes https://github.com/bitcoin/bitcoin/pull/27256#discussion_r1567356943
ACKs for top commit:
laanwj:
ACK fa4c69669e
stickies-v:
ACK fa4c69669e , thanks for fixing!
Tree-SHA512: dea4f4f843381d5e8ffaa812b2290a11e081b29f8777d041751c4aa9942e60f1f8d2d1a652d9a52b41dec470a490c9fe26ca9bc762dd593c3521b328a8af2826
b7ba60f81a test: add coverage for -reindex and assumeutxo (Martin Zumsande)
e57f951805 init, validation: Fix -reindex option with an existing snapshot (Martin Zumsande)
Pull request description:
In c711ca186f logic was introduced that `-reindex` and `-reindex-chainstate` will delete the snapshot chainstate.
This doesn't work currently, instead of deleting the snapshot chainstate the node crashes with an assert (this can be triggered by applying the added test commit on master).
Fix this, and another bug that would prevent the new active chainstate from having a mempool after `-reindex` has deleted the snapshot (also covered by the test).
ACKs for top commit:
fjahr:
re-ACK b7ba60f81a
hernanmarino:
crACK b7ba60f81a . Good fix
BrandonOdiwuor:
re-ACK b7ba60f81a
byaye:
Tested ACK b7ba60f81a
Tree-SHA512: c168f36997d7677d590af37b10427870f5d30123abf1c76032a16661e486735373bfa7e049e6aca439526fbcb6d619f970bf9d042196c851bf058a75a32fafdc
Both RPC and GUI now render a useful error message instead of (silently) failing.
Replace bool with util::Result<void> to clarify that this either succeeds or returns an error message.
It should never be a nullopt when the transaction result is valid -
Assume() this is the case. However, as a belt-and-suspenders just in
case it is nullopt, use an empty list.
c2e0489b71 [rpc, bugfix] Enforce maximum value for setmocktime (dergoegge)
Pull request description:
The maximum value for our mocktime must be representable in nanoseconds, otherwise we end up with negative values returned from `NodeClock::now()`.
Found through fuzzing:
```
$ echo "c2V0bW9ja3RpbWVcZTptYf9w/3NldG3///////////////9p////ZP///ymL//////89////Nv9L////////LXkBAABpAA==" | base64 --decode > rpc-8cab9148ab4418ebd1923c213e9d3fe9c9b49b39.crash
$ FUZZ=rpc ./src/test/fuzz/fuzz rpc-8cab9148ab4418ebd1923c213e9d3fe9c9b49b39.crash
fuzz_libfuzzer: util/time.cpp:28: static NodeClock::time_point NodeClock::now(): Assertion `ret > 0s' failed.
```
ACKs for top commit:
maflcko:
re-ACK c2e0489b71
brunoerg:
crACK c2e0489b71
glozow:
ACK c2e0489b71
Tree-SHA512: d7e237ca37bedd74a6b085fb6e726a142705371044c77488f593f35afe70aeca756fdba86920294b1d322c7a9b2cde9ce4e1b7d410a6ccc1fd7c6f3a6e77200a
92f88a9629 doc: fixup NAT-PMP help doc (fanquake)
02395edca9 init: remove redundant upnp #ifdef (fanquake)
Pull request description:
This is a very belated followup to #26896 (which removed the configure options for setting the upnp and natpmp runtime default) and corrects the `-help` docs for `-upnp` and `-natpmp`.
ACKs for top commit:
davidgumberg:
ACK 92f88a9629
hernanmarino:
ACK 92f88a9629
Tree-SHA512: 795dc8a8703bf322b5831d845de85f2428ee0dd45d3064b48ff47d147147381af26c0a9d00c596db12009b254763844b209989daf4e7470d20e8a1753b640966
3472e2f5ec Merge sipa/minisketch#81: Avoid overflowing shift by special casing inverse of 1
653d8b2e26 Avoid overflowing shift by special casing inverse of 1
33b7c200b9 Merge sipa/minisketch#80: Add c++20 version of CountBits
4a48f31a37 Merge sipa/minisketch#83: ci: Fix "s390x (big-endian)" task
82b6488acb Add c++20 version of CountBits
0498084d31 ci: Fix "s390x (big-endian)" task
71709dca9e Merge sipa/minisketch#82: ci: Fix `x86_64-w64-mingw32` task
9e6127fa98 Merge sipa/minisketch#74: Avoid >> above type width in BitWriter
ed420bc170 ci: Fix `x86_64-w64-mingw32` task
fe1040f227 Drop -Wno-shift-count-overflow compile flag
154bcd43bd Avoid >> above type width in BitWriter
67b87acdb6 Merge sipa/minisketch#78: ci: Update macOS image for CI
7de7250416 ci: Update macOS image for CI
83d812ea9f Merge sipa/minisketch#73: ci: Use correct variable to designate C++ compiler
e051a7d690 ci: Install wine32 package for Windows tests
2d2c695d78 build: Drop unused `CC` variable
1810fcbd11 ci: Use correct variable to designate C++ compiler
022b959049 Merge sipa/minisketch#77: Add missing include
08443c4892 Add missing include
git-subtree-dir: src/minisketch
git-subtree-split: 3472e2f5ec75ace39ce9243af6b3fee233a67492
4ba1d0b553 fuzz: Add coverage for client_maxfeerate (Greg Sanders)
91d7d8f22a AcceptMultipleTransactions: Fix workspace client_maxfeerate (Greg Sanders)
f3aa5bd5eb fill_mempool: assertions and docsctring update (Greg Sanders)
a3da63e8fe Move fill_mempool to util function (Greg Sanders)
73b68bd8b4 fill_mempool: remove subtest-specific comment (Greg Sanders)
Pull request description:
Bug causes an `Assume()` failure due to the expectation that the individual result should be invalid when done over `submitpackage` via rpc.
Bug introduced by https://github.com/bitcoin/bitcoin/pull/28950 , and I discovered it rebasing https://github.com/bitcoin/bitcoin/pull/28984 since it's easier to hit in that test scenario.
Tests in place were only checking `AcceptSingleTransaction`-level checks due to package evaluation only triggering when minfee is too high for the parent transaction.
Added test along with fix, moving the fill_mempool utility into a common area for re-use.
ACKs for top commit:
glozow:
reACK 4ba1d0b553
theStack:
ACK 4ba1d0b553
ismaelsadeeq:
re-ACK 4ba1d0b553 via [diff](4fe7d150eb..4ba1d0b553)
Tree-SHA512: 3729bdf7f25d04e232f173ccee04ddbb2afdaafa3d04292a01cecf58fb11b3b2bc133e8490277f1a67622b62d17929c242dc980f9bb647896beea4332ee35306
Limit number of IPs learned from a single DNS seed to 32, to prevent the results from
one DNS seed from dominating AddrMan. Note that the number of results from a UDP DNS query is
bounded to 33 already, but it is possible for it to use TCP where a potentially enormous
number of results can be returned.
Closes#16070.
This helper class is an alternative to CMedianFilter, but without a
lot of the special logic and exceptions that we needed while it was
still used for consensus.
d5a715536e build: remove boost::process dependency for building external signer support (Sebastian Falbesoner)
70434b1c44 external_signer: replace boost::process with cpp-subprocess (Sebastian Falbesoner)
cc8b9875b1 Add `cpp-subprocess` header-only library (Hennadii Stepanov)
Pull request description:
Closes https://github.com/bitcoin/bitcoin/issues/24907.
This PR is based on **theStack**'s [work](https://github.com/bitcoin/bitcoin/issues/24907#issuecomment-1466087049).
The `subprocess.hpp` header has been sourced from the [upstream repo](https://github.com/arun11299/cpp-subprocess) with the only modification being the removal of convenience functions, which are not utilized in our codebase.
Windows-related changes will be addressed in subsequent follow-ups.
ACKs for top commit:
achow101:
reACK d5a715536e
Sjors:
re-tACK d5a715536e
theStack:
Light re-ACK d5a715536e
fanquake:
ACK d5a715536e - with the expectation that this code is going to be maintained as our own. Next PRs should:
Tree-SHA512: d7fb6fecc3f5792496204190afb7d85b3e207b858fb1a75efe483c05260843b81b27d14b299323bb667c990e87a07197059afea3796cf218ed8b614086bd3611
a71eadf66b Change MAC_OSX macro to __APPLE__ in crypto package (Lőrinc)
Pull request description:
Split out from https://github.com/bitcoin/bitcoin/pull/29450#issuecomment-2044405345 to avoid the uncertainties and simplify review.
ACKs for top commit:
theuni:
ACK a71eadf66b
fanquake:
ACK a71eadf66b
Tree-SHA512: b6a7bd7ca95585dd9110cefe7c1213f4a1a72bdfc88670abf4a0d9a8bbc12e093544524adce46aa9ca714c472d417f74ca4a678af682ed2488053059434eaa02
a8203e9412 refactor: Simplify `extra_txn` to be a vec of CTransactionRef instead of a vec of pair<Wtxid, CTransactionRef> (AngusP)
c3c18433ae refactor: Use typesafe Wtxid in compact block encoding message, instead of ambiguous uint256. (AngusP)
Pull request description:
The first commit replaces `uint256` with typesafe `Wtxid` (or `Txid`) types introduced in #28107.
The second commit then simplifies the extra tx `vector` to just be of `CTransactionRef`s instead of a `std::pair<Wtxid, CTransactionRef>`, as it's easy to get a `Wtxid` from a transaction ref.
ACKs for top commit:
glozow:
ACK a8203e9412
dergoegge:
ACK a8203e9412
Tree-SHA512: b4ba1423a8059f9fc118782bd8a668213d229c822f22b01267de74d6ea97fe4f2aad46b5da7c0178ecc9905293e9a0eafba1d75330297c055e27fd53c8c8ebfd
03b87a3e64 Drop Windows Socket dependency for `randomenv.cpp` (Hennadii Stepanov)
Pull request description:
This change drops a dependency on the ws2_32 library for our libbitcoinkernel by switching to [`GetComputerName`](https://learn.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-getcomputernamew) function.
ACKs for top commit:
sipsorcery:
utACK 03b87a3e64.
laanwj:
Code review ACK 03b87a3e64.
fanquake:
ACK 03b87a3e64
Tree-SHA512: a4abd5499176634d5f3fbf4e794a7504c40232fb73bd7f41955fbfb2cc7c44bc7ea4518c5203836e52f552c30414c6c3e1b24f0922641dbf1c8377981c0ffaf0
4f273ab436 Change Luke Dashjr seed to dashjr-list-of-p2p-nodes.us (Luke Dashjr)
Pull request description:
To avoid issues with DNS blacklisting, I've setup a separate domain for my DNS seed.
(This time, without a potentially alarming name)
ACKs for top commit:
kevkevinpal:
Concept ACK [4f273ab](4f273ab436), name looks good to me
petertodd:
ACK 4f273ab436
mzumsande:
ACK 4f273ab436
fanquake:
ACK 4f273ab436
Tree-SHA512: 689698e3c735df3ed0c2756a9d4adb5644bb9d8a6954e23d66bfa9d94ee10954f77fb241d9593f750054d731aa1532368a0fc8277884f6c2a98ac47cd0bdeeb7
2d1819455c crypto: chacha20: always use our fallback timingsafe_bcmp rather than libc's (Cory Fields)
Pull request description:
Looking at libc sources, apple and openbsd implementations match our naive fallback. Only FreeBSD (and only x86_64) seems to [implement an optimized version](https://github.com/freebsd/freebsd-src/blob/main/lib/libc/amd64/string/timingsafe_bcmp.S).
It's not worth the hassle of using a platform-specific function for such little gain.
Additionally, as mentioned below, this is the only case outside of sha2 that requires an autoconf check, and I have upcoming PRs to remove the sha2 ones.
Apple's [impl is unoptimized](https://opensource.apple.com/source/Libc/Libc-1244.1.7/string/FreeBSD/timingsafe_bcmp.c.auto.html).
As-is [OpenBSD's impl](https://github.com/openbsd/src/blob/master/lib/libc/string/timingsafe_bcmp.c).
Relevant IRC conversation with sipa:
> \<cfields\> sipa: chacha20poly1305.cpp uses libc's timingsafe_bcmp when possible. But looking around at apple/freebsd/openbsd, I don't see any impl that doesn't use the naive implementation that matches our fallback...
> \<cfields\> is there any reason to belive there's an optimized impl somewhere that we're actually hitting?
> \<cfields\> asking because after cleaning up sha2, timingsafe_bcmp is the last autoconf check that remains in all of crypto. It'd make life easy if we could just always use our internal one.
> \<cfields\> *all of crypto/
> \<sipa\> cfields: let's get rid of the dependency then
> \<sipa\> it's a trivial function
> \<sipa\> and if we need it for some platforms, no real reason not to use it on all
After the above discusstion, I did end up finding the x86_64-optimized FreeBSD impl, but I don't think that's all that significant.
ACKs for top commit:
sipa:
utACK 2d1819455c
fanquake:
ACK 2d1819455c
TheCharlatan:
ACK 2d1819455c
theStack:
ACK 2d1819455c
Tree-SHA512: b9583e19ac2f77c5d572aa5b95bc4b53669d5717e5708babef930644980de7c5d06a9c7decd5c2b559d70b8597328ecfe513375e3d8c3ef523db80012dfe9266
All `CTransactionRef` have `.GetWitnessHash()` that returns a cached `const Wtxid` (since fac1223a56),
so we don't need to pass transaction refs around with their IDs as they're easy to get from a ref.
561a650e0f test: Fix debug recommendation in argsman_tests (Fabian Jahr)
Pull request description:
There are recommendations in the `argsman_tests` comments on how to re-run and debug a test failure to see if it reflects an expected or unexpected change. The command tries to run a test in `util_tests` but this is in `argsman_tests` so the command doesn't work with just copy+paste. I didn't investigate further but I suspect that these tests were moved between files.
ACKs for top commit:
fanquake:
ACK 561a650e0f
Tree-SHA512: b3bb94ba1635c9455149b455f2b30ee37a8067a6242339531ab54d428177a288da29a4a10702652305eb34aa7638f51dad35fa6b0e7b74617e445327b8c4c053
fa9f36baba build: Remove HAVE_GMTIME_R (MarcoFalke)
fa72dcbfa5 refactor: FormatISO8601* without gmtime* (MarcoFalke)
fa2c486afc Revert "time: add runtime sanity check" (MarcoFalke)
Pull request description:
Now that the `ChronoSanityCheck` has passed for everyone with C++17 and is guaranteed by C++20 to always pass, remove it.
Also, remove `gmtime_r` and `gmtime_s` and replace them with `year_month_day`+`hh_mm_ss` from C++20.
ACKs for top commit:
sipa:
utACK fa9f36baba
fanquake:
ACK fa9f36baba - more std lib & even less stuff to port.
Tree-SHA512: a9e7e805b757b7dade0bcc3f95273a7dc4f68622630d74838339789dd203ad7542d36b2e090a93b2bc5a7ecc383207dd7ec82c68147108bdac7ce44f088c8c9a
Looking at apple/freebsd/openbsd sources, their implementations match our naive
fallback. It's not worth the hassle of using a platform-specific function for
no gain.
Fixes: #809
Previously it was possible through the GUI to enter an IP address:port
into the "Proxy IP" configuration box. After the node was restarted the
errant setting would prevent the node starting back up until manually
removed from settings.json.
bbe82c116eFix#29767, set m_synced = true after Commit() (nanlour)
Pull request description:
I think this problem https://github.com/bitcoin/bitcoin/issues/29767#issue-2216373048 is because of
in BaseIndex::Sync
61de64df67/src/index/base.cpp (L163-L168)
Setup m_synced = true; before Commit();
So this may cause a race condition window to BaseIndex::BlockConnected
61de64df67/src/index/base.cpp (L271-L274)
So i try to fix it with move m_synced = true after Commit().
Also see comment of Sync():
61de64df67/src/index/base.h (L151-L156)
I am a newcomer interested in Bitcoin, trying to become a member of the Bitcoin Core development team. Please give me some feedback if you could, as I may be doing something wrong. Thank you!
ACKs for top commit:
fjahr:
Code review ACK bbe82c116e
ryanofsky:
Code review ACK bbe82c116e
Tree-SHA512: 89a09498a232c87ef1e083d4cc4ed9bb15f045ad0624d5d150a87187b2b8a48a41137974dbc7ea5c37f73da90742c43259f5aa7f84b4179eb8d62033e44fa479
b0344c219a logging: remove unused BCLog::UTIL (Vasil Dimov)
d3b3af9034 log: deduplicate category names and improve logging.cpp (Vasil Dimov)
Pull request description:
The code in `logging.cpp` needs to:
* Get the category name given the flag (e.g. `BCLog::PRUNE` -> `"prune"`)
* Get the flag given the category name (e.g. `"prune"` -> `BCLog::PRUNE`)
* Get the list of category names sorted in alphabetical order
Achieve this by using the proper std containers. The result is
* less code (the diff of the first commit is +62 / -129)
* faster code (to linear search and no copy+sort)
* more maintainable code (the categories are no longer duplicated in `LogCategories[]` and `LogCategoryToStr()`)
This behavior is preserved:
`BCLog::NONE` -> `""` (lookup by `LogCategoryToStr()`)
`""` -> `BCLog::ALL` (lookup by `GetLogCategory("")`)
---
Also remove unused `BCLog::UTIL`.
---
These changes (modulo the `BCLog::UTIL` removal) are part of https://github.com/bitcoin/bitcoin/pull/29415 but they make sense on their own and would be good to have them, regardless of the fate of https://github.com/bitcoin/bitcoin/pull/29415. Also, if this is merged, that would reduce the size of https://github.com/bitcoin/bitcoin/pull/29415, thus the current standalone PR.
ACKs for top commit:
davidgumberg:
crACK b0344c219a
pinheadmz:
ACK b0344c219a
ryanofsky:
Code review ACK b0344c219a. Nice cleanup! Having to maintain multiple copies of the same mapping seemed messy and a like a possible footgun. I checked old and new mappings in both directions and confirmed no behavior should be changing.
Tree-SHA512: 57f87a090932f9b33dc8e075d1855dba9b71a3243a0758511745483dec2d9c46d3b532eadab297e78164c9b7caba370986ee380696a45f0778a841082f8e21a7
It's useful to be able to seek to a specific position in a file. Allow
AutoFile to seek by using fseek.
It's also useful to be able to get the current position in a file.
Allow AutoFile to tell by using ftell.
80f8b92f4f remove libbitcoinconsensus (fanquake)
Pull request description:
This was deprecated in `v27.0`, for removal in `v28.0`. See discussion in PR #29189.
ACKs for top commit:
theuni:
Concept ACK and light review ACK 80f8b92f4f. My only hesitation here is that (afaics?) there's now nothing keeping undesired features like threading or globals from working their way into the interpreter in future commits.
m3dwards:
Concept ACK 80f8b92f4f
TheCharlatan:
ACK 80f8b92f4f
hebasto:
ACK 80f8b92f4f, I have reviewed the code and it looks OK.
Tree-SHA512: 17a62118aeb088f2695c892bb32794dfea3061e3cb7d9e8e9f1c06c3ff6f63a7587fa532e37edbb91fbc5a19b12c9a0f8e05fa9e8864aa07f92665375d847e80
601edd8ee8 ci: use codespell 2.2.6 (fanquake)
52fa0d285f doc: fix some typos (crazeteam)
b5ed13a240 doc: Fix typos (RoboSchmied)
Pull request description:
Combines the recent PRs to fix typos so they can be merged.
ACKs for top commit:
brunoerg:
crACK 601edd8ee8
tdb3:
crACK 601edd8ee8
kristapsk:
cr utACK 601edd8ee8
Tree-SHA512: d054b1dad1336d6b9291cc5d5252d4debf6424a993d4edd6a97d7c15055a7fc48a333d30967f72e7dc9c6c1d9a9038ca8bb5e219c529f4c2365ea48404a508d0
ee1b9b231a CalculateFeerateDiagramsForRBF: update misleading description of old diagram contents (Greg Sanders)
a9d42b9aa5 CompareFeerateDiagram: short-circuit comparison when detected as incomparable (Greg Sanders)
cebcced65e remove erroneous CompareFeerateDiagram comment about slope (Greg Sanders)
a0376e1061 unit test: clarify unstated assumption for calc_feerate_diagram_rbf chunking (Greg Sanders)
890cb015f3 s/effected/affected/ (Greg Sanders)
d9391ec095 CalculateFeerateDiagramsForRBF: remove size tie-breaking from chunking conflicts (Greg Sanders)
b684d82d7e fuzz: Add more invariant checks for package_rbf (Greg Sanders)
2a3ada8b21 fuzz: finer grained ImprovesFeerateDiagram check on error result (Greg Sanders)
c377ae9ba0 unit test: improve ImprovesFeerateDiagram coverage with one less vb case (Greg Sanders)
d2bf923eb1 unit test: make calc_feerate_diagram_rbf less brittle (Greg Sanders)
defe023f6e fuzz: add PrioritiseTransaction coverage in diagram checks (Greg Sanders)
216d5ff162 unit test: add coverage showing priority affects diagram check results (Greg Sanders)
a80d80936a unit test: add CheckConflictTopology case for not the only child (Greg Sanders)
69bd18ca80 unit test: check tx4 conflict error message (Greg Sanders)
c0c37f07eb unit test: have CompareFeerateDiagram tested with diagrams both ways (Greg Sanders)
b62e2c0fa5 ImprovesFeerateDiagram: Spelling fix and removal of unused diagram vectors (Greg Sanders)
bb42402945 doc: fix comment about non-existing CompareFeeFrac (Greg Sanders)
Pull request description:
Follow-ups to https://github.com/bitcoin/bitcoin/pull/29242
ACKs for top commit:
glozow:
ACK ee1b9b231a, reviewed the changes and package_rbf fuzzer seems to run fine
murchandamus:
crACK ee1b9b231a
ismaelsadeeq:
Code review ACK ee1b9b231a
willcl-ark:
ACK ee1b9b231a
Tree-SHA512: 8399fe12064fb49b0e4c73258968b57be1d9c2e35701b2d3b0bb67e2e4052e44216358238f92508e4697d0fb6176518d5b885474054d3deda242f669e99262a7
746b6d8839 test: Add test for createwalletdescriptor (Ava Chow)
2402b63062 wallet: Test upgrade of pre-taproot wallet to have tr() descriptors (Ava Chow)
460ae1bf67 wallet, rpc: Add createwalletdescriptor RPC (Ava Chow)
8e1a475062 wallet: Be able to retrieve single key from descriptors (Ava Chow)
85b1fb19dd wallet: Add GetActiveHDPubKeys to retrieve xpubs from active descriptors (Ava Chow)
73926f2d31 wallet, descspkm: Refactor wallet descriptor generation to standalone func (Andrew Chow)
54e74f46ea wallet: Refactor function for single DescSPKM setup (Andrew Chow)
3b09d0eb7f tests: Test for gethdkeys (Ava Chow)
5febe28c9e wallet, rpc: Add gethdkeys RPC (Ava Chow)
66632e5c24 wallet: Add IsActiveScriptPubKeyMan (Ava Chow)
fa6a259985 desc spkm: Add functions to retrieve specific private keys (Ava Chow)
fe67841464 descriptor: Be able to get the pubkeys involved in a descriptor (Ava Chow)
ef6745879d key: Add constructor for CExtKey that takes CExtPubKey and CKey (Ava Chow)
Pull request description:
This PR adds a `createwalletdescriptor` RPC which allows users to add new automatically generated descriptors to their wallet, e.g. to upgrade a 0.21.x wallet to contain a taproot descriptor. This RPC takes 3 arguments: the output type to create a descriptor for, whether the descriptor will be internal or external, and the HD key to use if the user wishes to use a specific key. The HD key is an optional parameter. If it is not specified, the wallet will use the key shared by the active descriptors, if they are all single key. For most users in the expected upgrade scenario, this should be sufficient. In more advanced cases, the user must specify the HD key to use.
Currently, specified HD keys must already exist in the wallet. To make it easier for the user to know, `gethdkeys` is also added to list out the HD keys in use by all of the descriptors in the wallet. This will include all HD keys, whether we have the private key, for it, which descriptors use it and their activeness, and optionally the extended private key. In this way, users with more complex wallets will be still be able to get HD keys from their wallet for use in other scenarios, and if they want to use `createwalletdescriptor`, they can easily get the keys that they can specify to it.
See also https://github.com/bitcoin/bitcoin/pull/26728#issuecomment-1866961865
ACKs for top commit:
Sjors:
re-utACK 746b6d8839
furszy:
ACK 746b6d8
ryanofsky:
Code review ACK 746b6d8839, and this looks ready to merge. There were various suggested changes since last review where main change seems to be switching `gethdkeys` output to use normalized descriptors (removing hardened path components).
Tree-SHA512: f2849101e6fbf1f59cb031eaaaee97af5b1ae92aaab54c5716940d210f08ab4fc952df2725b636596cd5747b8f5beb1a7a533425bc10d09da02659473516fbda
4d5b55735b log: renamed disk to file so wording was more accurate (kevkevin)
b9f04be870 mempool: Log added for dumping mempool transactions to disk (kevkevin)
Pull request description:
Sometimes when shutting off bitcoind it can take a while to dump the mempool transaction onto the disk so
this change adds additional logging to the `DumpMempool` method in `kernel/mempool_persist.cpp`
Motivated by https://github.com/bitcoin/bitcoin/pull/29227 this change
- adds a single new line for the amount of transactions being dumped and the amount of memory being dumped to file
This is in response to https://github.com/bitcoin/bitcoin/pull/29227#issuecomment-1893375082
The logs will now look like this
```
2024-02-09T23:41:52Z DumpAnchors: Flush 2 outbound block-relay-only peer addresses to anchors.dat completed (0.02s)
2024-02-09T23:41:52Z scheduler thread exit
2024-02-09T23:41:52Z Writing 29 mempool transactions to file...
2024-02-09T23:41:52Z Writing 0 unbroadcast transactions to file.
2024-02-09T23:41:52Z Dumped mempool: 0.000s to copy, 0.022s to dump, 0.015 MB dumped to file
2024-02-09T23:41:52Z Flushed fee estimates to fee_estimates.dat.
2024-02-09T23:41:53Z Shutdown: done
```
ACKs for top commit:
maflcko:
cr-ACK 4d5b55735b
glozow:
reACK 4d5b557
Tree-SHA512: 049191e140d00c1ea57debe0138f1c9eb0f9bb0ef8138e2568e6d89e64f45a5d5853ce3b9cc0b28566aab97555b47ddfb0f9199fc8cea6b81e53f50592d5ae6a
5952292133 wallet, rpc: show mempool conflicts in `gettransaction` result (ishaanam)
54e07ee22f wallet: track mempool conflicts (ishaanam)
d64922b590 wallet refactor: use CWalletTx member functions to determine tx state (ishaanam)
ffe5ff1fb6 scripted-diff: wallet: s/TxStateConflicted/TxStateBlockConflicted (ishaanam)
180973a941 test: Add tests for wallet mempool conflicts (ishaanam)
Pull request description:
The `mempool_conflicts` variable is added to `CWalletTx`, it is a set of txids of txs in the mempool conflicting with the wallet tx or a wallet tx's parent. This PR only changes how mempool-conflicted txs are dealt with in memory.
`IsSpent` now returns false for an output being spent by a mempool conflicted transaction where it previously returned true.
A txid is added to `mempool_conflicts` during `transactionAddedToMempool`. A txid is removed from `mempool_conflicts` during `transactionRemovedFromMempool`.
This PR also adds a `mempoolconflicts` field to the `gettransaction` wallet RPC result.
Builds on #27145
Second attempt at #18600
ACKs for top commit:
achow101:
ACK 5952292133
ryanofsky:
Code review ACK 5952292133. Just small suggested changes since last review
furszy:
ACK 59522921
Tree-SHA512: 615779606723dbb6c2e302681d8e58ae2052ffee52d721ee0389746ddbbcf4b4c4afacf01ddf42b6405bc6f883520524186a955bf6b628fe9b3ae54cffc56a29
Upstream repo: https://github.com/arun11299/cpp-subprocess
Commit: 4025693decacaceb9420efedbf4967a04cb028e7
The "Convenience Functions" section is unused in our codebase, so it has
been removed.
7295986778 Unit tests for CalculateFeerateDiagramsForRBF (Greg Sanders)
b767e6bd47 test: unit test for ImprovesFeerateDiagram (Greg Sanders)
7e89b659e1 Add fuzz test for FeeFrac (Greg Sanders)
4d6528a3d6 fuzz: fuzz diagram creation and comparison (Greg Sanders)
e9c5aeb11d test: Add tests for CompareFeerateDiagram and CheckConflictTopology (Greg Sanders)
588a98dccc fuzz: Add fuzz target for ImprovesFeerateDiagram (Greg Sanders)
2079b80854 Implement ImprovesFeerateDiagram (Greg Sanders)
66d966dcfa Add FeeFrac unit tests (Greg Sanders)
ce8e22542e Add FeeFrac utils (Greg Sanders)
Pull request description:
This is a smaller piece of https://github.com/bitcoin/bitcoin/pull/28984 broken off for easier review.
Up to date explanation of diagram checks are here: https://delvingbitcoin.org/t/mempool-incentive-compatibility/553
This infrastructure has two near term applications prior to cluster mempool:
1) Limited Package RBF(https://github.com/bitcoin/bitcoin/pull/28984): We want to allow package RBF only when we know it improves the mempool. This narrowly scoped functionality allows use with v3-like topologies, and will be expanded at some point post-cluster mempool when diagram checks can be done efficiently against bounded cluster sizes.
2) Replacement for single tx RBF(in a cluster size of up to two) against conflicts of up to cluster size two. `ImprovesFeerateDiagram` interface will have to change for this use-case, which is a future direction to solve certain pins and improve mempool incentive compatibility: https://delvingbitcoin.org/t/ephemeral-anchors-and-mev/383#diagram-checks-fix-this-3
And longer-term, this would be the proposed way we would compute incentive compatibility for all conflicts, post-cluster mempool.
ACKs for top commit:
sipa:
utACK 7295986778
glozow:
code review ACK 7295986778
murchandamus:
utACK 7295986778
ismaelsadeeq:
Re-ACK 7295986778
willcl-ark:
crACK 7295986778
sdaftuar:
ACK 7295986778
Tree-SHA512: 79593e5a087801c06f06cc8b73aa3e7b96ab938d3b90f5d229c4e4bfca887a77b447605c49aa5eb7ddcead85706c534ac5eb6146ae2396af678f4beaaa5bea8e
