652424ad162b63d73ecb6bd65bd26946e90c617f test: additional test coverage for script_verify_flags (Anthony Towns)
417437eb01ac014c57aca47f44d7f8d3da351987 script/verify_flags: extend script_verify_flags to 64 bits (Anthony Towns)
3cbbcb66efc39c6566ab31836e4eb582b77581d2 script/interpreter: make script_verify_flag_name an ordinary enum (Anthony Towns)
bddcadee82daf3ed1441820a0ffc4c5ef78f64f1 script/verify_flags: make script_verify_flags type safe (Anthony Towns)
a5ead122fe060e7e582914dcb7acfaeee7a8ac48 script/interpreter: introduce script_verify_flags typename (Anthony Towns)
4577fb2b1e098c3f560b1ff50a37ebfef2af5f32 rpc: have getdeploymentinfo report script verify flags (Anthony Towns)
a3986935f073be799a35dfa92ab5004e12b35467 validation: export GetBlockScriptFlags() (Anthony Towns)
5db8cd2d37eba3ca6abc66386a3b9dc2185fa3ce Move mapFlagNames and FormatScriptFlags logic to script/interpreter.h (Anthony Towns)
Pull request description:
We currently use 21 of 32 possible bits for `SCRIPT_VERIFY_*` flags, with open PRs that may use 8 more (#29247, #31989, #32247, #32453). The mutinynet fork that has included many experimental soft fork features is [already reusing bits here](d4a86277ed/src/script/interpreter.h (L175-L195)). Therefore, bump this to 64 bits.
In order to make it easier to update this logic in future, this PR also introduces a dedicated type for the script flags, and disables implicit conversion between that type and the underlying integer type. To make verifying that this change doesn't cause flags to disappear, this PR also resurrects the changes from #28806 so that the script flags that are consensus enforced on each block can be queried via getdeploymentinfo.
ACKs for top commit:
instagibbs:
reACK 652424ad16
achow101:
ACK 652424ad162b63d73ecb6bd65bd26946e90c617f
darosior:
ACK 652424ad162b63d73ecb6bd65bd26946e90c617f
theStack:
Code-review ACK 652424ad162b63d73ecb6bd65bd26946e90c617f 🎏
Tree-SHA512: 7b30152196cdfdef8b9700b571b7d7d4e94d28fbc5c26ea7532788037efc02e4b1d8de392b0b20507badfdc26f5c125f8356a479604a9149b8aae23a7cf5549f
3635d62f5a935801e26a0d5fa2cb5e2dbbb42f9b chain: make use of pskip in LastCommonAncestor (optimization) (Pieter Wuille)
2e09d66fbb7bb253ce90ffcda026ce58426ba4e4 tests: add unit tests for CBlockIndex::GetAncestor and LastCommonAncestor (Pieter Wuille)
Pull request description:
In theory, the `LastCommonAncestor` function in chain.cpp can take $\mathcal{O}(n)$ time, walking over the entire chain, if the forking point is very early, which could take ~milliseconds. I expect this to be very rare in normal occurrences, but it seems nontrivial to reason about worst cases as it's accessible from several places in net_processing.
This PR modifies the algorithm to make use of the `CBlockIndex::pskip` skip pointers to find the forking point in sublinear time (a simulation shows that for heights up to $34 \cdot 4^k - 2$ and $k \geq 8$, no more than $k^2 + 10k + 13$ steps are ever needed), in a way that should be nearly free - at worst the same number of memory accesses should be made, with a tiny increase in computation.
As it appears we didn't really have tests for this function, unit tests are added for that function as well as `CBlockIndex::GetAncestor()`.
This is inspired by https://github.com/bitcoin/bitcoin/pull/32180#discussion_r2394877881
ACKs for top commit:
optout21:
ACK 3635d62f5a935801e26a0d5fa2cb5e2dbbb42f9b
achow101:
ACK 3635d62f5a935801e26a0d5fa2cb5e2dbbb42f9b
vasild:
ACK 3635d62f5a935801e26a0d5fa2cb5e2dbbb42f9b
mzumsande:
Code Review ACK 3635d62f5a935801e26a0d5fa2cb5e2dbbb42f9b
furszy:
ACK 3635d62f5a935801e26a0d5fa2cb5e2dbbb42f9b
stratospher:
ACK 3635d62f5a935801e26a0d5fa2cb5e2dbbb42f9b.
Tree-SHA512: f9b7dea1e34c1cc1ec1da3fb9e90c4acbf4aaf0f04768844f538201efa6b11eeeefc97b720509e78c21878977192e2c4031fd8974151667e2e756247002b8164
a4f929696490 Merge bitcoin-core/libmultiprocess#224: doc: fix typos
f4344ae87da0 Merge bitcoin-core/libmultiprocess#222: test, ci: Fix threadsanitizer errors in mptest
1434642b3804 doc: fix typos
73d22ba2e930 test: Fix tsan race in thread busy test
b74e1bba014d ci: Use tsan-instrumented cap'n proto in sanitizers job
c332774409ad test: Fix failing exception check in new thread busy test
ca3c05d56709 test: Use KJ_LOG instead of std::cout for logging
7eb1da120ab6 ci: Use tsan-instrumented libcxx in sanitizers job
ec86e4336e98 Merge bitcoin-core/libmultiprocess#220: Add log levels and advertise them to users via logging callback
515ce93ad349 Logging: Pass LogData struct to logging callback
213574ccc43d Logging: reclassify remaining log messages
e4de0412b430 Logging: Break out expensive log messages and classify them as Trace
408874a78fdc Logging: Use new logging macros
67b092d835cd Logging: Disable logging if messsage level is less than the requested level
d0a1ba7ebf21 Logging: add log levels to mirror Core's
463a8296d188 Logging: Disable moving or copying Logger
83a2e10c0b03 Logging: Add an EventLoop constructor to allow for user-specified log options
58cf47a7fc8c Merge bitcoin-core/libmultiprocess#221: test default PassField impl handles output parameters
db03a663f514 Merge bitcoin-core/libmultiprocess#214: Fix crash on simultaneous IPC calls using the same thread
afcc40b0f1e8 Merge bitcoin-core/libmultiprocess#213: util+doc: Clearer errors when attempting to run examples + polished docs
6db669628387 test In|Out parameter
29cf2ada75ea test default PassField impl handles output parameters
1238170f68e8 test: simultaneous IPC calls using same thread
eb069ab75d83 Fix crash on simultaneous IPC calls using the same thread
ec03a9639ab5 doc: Precision and typos
2b4348193551 doc: Where possible, remove links to ryanofsky/bitcoin/
286fe469c9c9 util: Add helpful error message when failing to execute file
git-subtree-dir: src/ipc/libmultiprocess
git-subtree-split: a4f92969649018ca70f949a09148bccfeaecd99a
1aaaaa078bb2efed126e3f41ecf7c81ccf005818 fuzz: Drop unused workaround after Apple-Clang bump (MarcoFalke)
fadad7a49477cd61fbbfe20a0a61023c2d4d70a1 Drop support for EOL macOS 13 (MarcoFalke)
Pull request description:
Now that macOS 13 is EOL (https://en.wikipedia.org/wiki/MacOS_Ventura), it seems odd to still support it.
(macOS Ventura 13.7.8 received its final security update on 20 Aug 2025: https://support.apple.com/en-us/100100)
This patch will only be released in version 31.x, another 6 months out from now.
So:
* Update the depends build and release note template to drop EOL macOS 13.
* As a result, update the earliest Xcode to version 16 in CI.
* Also, bump the macOS CI runner to version 15, to avoid issues when version 14 will be at its EOL in about 1 year.
This also allows to drop a small workaround in the fuzz tests and unlocks libcpp hardening (https://github.com/bitcoin/bitcoin/pull/33462)
ACKs for top commit:
stickies-v:
re-ACK 1aaaaa078bb2efed126e3f41ecf7c81ccf005818
l0rinc:
code review ACK 1aaaaa078bb2efed126e3f41ecf7c81ccf005818
hodlinator:
re-ACK 1aaaaa078bb2efed126e3f41ecf7c81ccf005818
hebasto:
ACK 1aaaaa078bb2efed126e3f41ecf7c81ccf005818.
Tree-SHA512: 6d247a8432ef8ea8c6ff2a221472b278f8344346b172980299507f9898bb9e8e16480c128b1f4ca692bcbcc393da2b2fd6895ac5f118bc09e0f30f910529d20c
c76de2eea18076f91dd80b52f66ba790f071a2b1 net: support overriding the proxy selection in ConnectNode() (Vasil Dimov)
Pull request description:
Normally `ConnectNode()` would choose whether to use a proxy and which one. Make it possible to override this from the callers and same for `OpenNetworkConnection()` - pass down the proxy to `ConnectNode()`.
Document both functions.
This is useful if we want to open connections to IPv4 or IPv6 peers through the Tor SOCKS5 proxy.
Also have `OpenNetworkConnection()` return whether the connection succeeded or not. This can be used when the caller needs to keep track of how many (successful) connections were opened.
---
This is part of [#29415 Broadcast own transactions only via short-lived Tor or I2P connections](https://github.com/bitcoin/bitcoin/pull/29415). Putting it in its own PR to reduce the size of #29415 and because it does not depend on the other commits from there.
ACKs for top commit:
stratospher:
ACK c76de2e.
optout21:
ACK c76de2eea18076f91dd80b52f66ba790f071a2b1
mzumsande:
Code Review ACK c76de2eea18076f91dd80b52f66ba790f071a2b1
andrewtoth:
ACK c76de2eea18076f91dd80b52f66ba790f071a2b1
Tree-SHA512: 1d266e4280cdb1d0599971fa8b5da58b1b7451635be46abb15c0b823a1e18cf6e7bcba4a365ad198e6fd1afee4097d81a54253fa680c8b386ca6b9d68d795ff0
0f7d4ee4e8281ed141a6ebb7e0edee7b864e4dcf p2p: Use different inbound inv timer per network (Martin Zumsande)
94db966a3bb52a3677eb5f762447202ed3889f0f net: use generic network key for addrcache (Martin Zumsande)
Pull request description:
Currently, `NextInvToInbounds` schedules each round of `inv` at the same time for all inbound peers. It's being done this way because with a separate timer per peer (like it's done for outbounds), an attacker could do multiple connections to learn about the time a transaction arrived. (#13298).
However, having a single timer for inbounds of all networks is also an obvious fingerprinting vector: Connecting to a suspected pair of privacy-network and clearnet addresses and observing the `inv` pattern makes it trivial to confirm or refute that they are the same node.
This PR changes it such that a separate timer is used for each network.
It uses the existing method from `getaddr` caching and generalizes it to be saved in a new field `m_network_key` in `CNode` which will be used for both `getaddr` caching and `inv` scheduling, and can also be used for any future anti-fingerprinting measures.
ACKs for top commit:
sipa:
utACK 0f7d4ee4e8281ed141a6ebb7e0edee7b864e4dcf
stratospher:
reACK 0f7d4ee.
naiyoma:
Tested ACK 0f7d4ee4e8281ed141a6ebb7e0edee7b864e4dcf
danielabrozzoni:
reACK 0f7d4ee4e8
Tree-SHA512: e197c3005b2522051db432948874320b74c23e01e66988ee1ee11917dac0923f58c1252fa47da24e68b08d7a355d8e5e0a3ccdfa6e4324cb901f21dfa880cd9c
By using the pskip pointer, which regularly allows jumping back much faster
than pprev, the forking point between two CBlockIndex entries can be found
much faster.
A simulation shows that no more than 136 steps are needed to jump anywhere
within the first 2^20 block heights, and on average 65 jumps for uniform
forking points around that height.
06df14ba75be5f48cf9c417424900ace17d1cf4d test: add more TRUC reorg coverge (Greg Sanders)
26e71c237d9d2197824b547f55ee3a0a60149f92 Mempool: Do not enforce TRUC checks on reorg (Greg Sanders)
bbe8e9063c15dc230553e0cbf16d603f5ad0e4cf fuzz: don't bypass_limits for most mempool harnesses (Greg Sanders)
Pull request description:
This was the intended behavior but our tests didn't cover the scenario where in-block transactions themselves violate TRUC topological constraints.
The behavior in master will potentially lead to many erroneous evictions during a reorg, where evicted TRUC packages may be very high feerate and make sense to mine all together in the next block and are well within the normal anti-DoS chain limits.
This issue exists since the merge of https://github.com/bitcoin/bitcoin/pull/28948/files#diff-97c3a52bc5fad452d82670a7fd291800bae20c7bc35bb82686c2c0a4ea7b5b98R956
ACKs for top commit:
sdaftuar:
ACK 06df14ba75be5f48cf9c417424900ace17d1cf4d
glozow:
ACK 06df14ba75b
ismaelsadeeq:
Code review ACK 06df14ba75be5f48cf9c417424900ace17d1cf4d
Tree-SHA512: bdb6e4dd622ed8b0b11866263fff559fcca6e0ca1c56a884cca9ac4572f0026528a63a9f4c8a0660df2f5efe0766310a30e5df1d6c560f31e4324ea5d4b3c1a8
Normally `ConnectNode()` would choose whether to use a proxy and which
one. Make it possible to override this from the callers and same for
`OpenNetworkConnection()` - pass down the proxy to `ConnectNode()`.
Document both functions.
This is useful if we want to open connections to IPv4 or IPv6 peers
through the Tor SOCKS5 proxy.
Also have `OpenNetworkConnection()` return whether the connection
succeeded or not. This can be used when the caller needs to keep track
of how many (successful) connections were opened.
87e7f37918d42c28033e9f684db52f94eeed617b doc: clarify peer address in getpeerinfo and addnode RPC help (Vasil Dimov)
2a4450ccbbe30f6522c3108f136b2b867b2a87fe net: change FindNode() to not return a node and rename it (Vasil Dimov)
4268abae1a1d06f2c4bd26b85b3a491719217fae net: avoid recursive m_nodes_mutex lock in DisconnectNode() (Vasil Dimov)
3a4d1a25cf949eb5f27d6dfd4e1b4a966b2cde75 net: merge AlreadyConnectedToAddress() and FindNode(CNetAddr) (Vasil Dimov)
Pull request description:
`CConnman::FindNode()` would lock `m_nodes_mutex`, find the node in `m_nodes`, release the mutex and return the node. The current code is safe but it is a dangerous interface where a caller may end up using the node returned from `FindNode()` without owning `m_nodes_mutex` and without having that node's reference count incremented.
Change `FindNode()` to return a boolean since all but one of its callers used its return value to check whether a node exists and did not do anything else with the return value.
Remove a recursive lock on `m_nodes_mutex`.
Rename `FindNode()` to better describe what it does.
ACKs for top commit:
achow101:
ACK 87e7f37918d42c28033e9f684db52f94eeed617b
furszy:
Code review ACK 87e7f37918d42c28033e9f684db52f94eeed617b
hodlinator:
re-ACK 87e7f37918d42c28033e9f684db52f94eeed617b
Tree-SHA512: 44fb64cd1226eca124ed1f447b4a1ebc42cc5c9e8561fc91949bbeaeaa7fa16fcfd664e85ce142e5abe62cb64197c178ca4ca93b3b3217b913e3c498d0b7d1c9
The returned value in `getpeerinfo/addr` could be a hostname as well as
an IP address and the `:port` part could be missing. It is displayed
from `CNode::m_addr_name` which could have been set from RPC `addnode`
where the argument is allowed to be a hostname and an optional port.
All callers of `CConnman::FindNode()` use its return value `CNode*` only
as a boolean null/notnull. So change that method to return `bool`.
This removes the dangerous pattern of handling a `CNode` object (the
return value of `FindNode()`) without holding `CConnman::m_nodes_mutex`
and without having that object's reference count incremented for the
duration of the usage.
Also rename the method to better describe what it does.
Have `CConnman::DisconnectNode()` iterate `m_nodes` itself instead of
using `FindNode()`. This avoids recursive mutex lock and drops the only
caller of `FindNode()` which used the return value for something else
than a boolean found/notfound.
0802398e749c5e16fa7085cd87c91a31bbe043bd fuzz: make it possible to mock (fuzz) CThreadInterrupt (Vasil Dimov)
6d9e5d130d2e1d052044e9a72d44cfffb5d3c771 fuzz: add CConnman::SocketHandler() to the tests (Vasil Dimov)
3265df63a48db187e0d240ce801ee573787fed80 fuzz: add CConnman::InitBinds() to the tests (Vasil Dimov)
91cbf4dbd864b65ba6b107957f087d1d305914b2 fuzz: add CConnman::CreateNodeFromAcceptedSocket() to the tests (Vasil Dimov)
50da7432ec1e5431b243aa30f8a9339f8e8ed97d fuzz: add CConnman::OpenNetworkConnection() to the tests (Vasil Dimov)
e6a917c8f8e0f1a0fa71dc9bbb6e1074f81edea3 fuzz: add Fuzzed NetEventsInterface and use it in connman tests (Vasil Dimov)
e883b37768812d96feec207a37202c7d1b603c1f fuzz: set the output argument of FuzzedSock::Accept() (Vasil Dimov)
Pull request description:
Extend `CConnman` fuzz tests to also exercise the methods `OpenNetworkConnection()`, `CreateNodeFromAcceptedSocket()`, `InitBinds()` and `SocketHandler()`.
Previously fuzzing those methods would have resulted in real socket functions being called in the operating system which is undesirable during fuzzing. Now that https://github.com/bitcoin/bitcoin/pull/21878 is complete all those are mocked to a fuzzed socket and a fuzzed DNS resolver (see how `CreateSock` and `g_dns_lookup` are replaced in the first commit).
ACKs for top commit:
achow101:
ACK 0802398e749c5e16fa7085cd87c91a31bbe043bd
jonatack:
Review re-ACK 0802398e749c5e16fa7085cd87c91a31bbe043bd
dergoegge:
Code review ACK 0802398e749c5e16fa7085cd87c91a31bbe043bd
Tree-SHA512: a717d4e79f42bacf2b029c821fdc265e10e4e5c41af77cd4cb452cc5720ec83c62789d5b3dfafd39a22cc8c0500b18169aa7864d497dded729a32ab863dd6c4d
451ba9ada41f687c0e4bb34d5925374a68a8f8a3 datacarrier: Undeprecate configuration option (Anthony Towns)
Pull request description:
Removes the deprecation for the `datacarrier` and `datacarriersize` options by reverting commit 0b4048c73385166144d0b3e76beb9a2ac4cc1eca from https://github.com/bitcoin/bitcoin/pull/32406
**Many current Bitcoin Core users want to continue using this option**
This statement is based on public postings from many Bitcoin Core users and not a formal survey. AJ Towns’ observation from [#32406](0b4048c733 (r2084024874)) that “_for now there seem to be a bunch of users who like the option_” has only become more apparent in the months since.
**The deprecation intent is unclear to users**
This echo’s Ava Chow’s comment from #32714 that “_IMO we should not have removal warnings if there is no current plan to actually remove them._” In months since that comment, partially due to increased feedback from Bitcoin Core users wanting to keep this option, there is even less likelihood of a near term plan to remove these options. That leaves Bitcoin Core users in an unclear situation: the option could be removed in the next version or perhaps never. Removing the deprecation gives clarity for their planning purposes. Deprecating the option in the future, preferably with a removal schedule to better inform users, would still be possible.
**Minimal downsides to removing deprecation**
As a best practice, Bitcoin Core has avoided an option when the developers cannot articulate when they should be used. There is non-zero maintenance cost to keeping this code around (although leaving the options deprecated for a long time has the same effect). “Don’t offer users footguns” is also a good principle, but with this option, there seems to be only small impacts that can quickly be remedied by changing the option value by Bitcoin Core users. There already exist in Bitcoin Core more potentially-user-harmful options/values than what datacarrier might cause.
ACKs for top commit:
ajtowns:
ACK 451ba9ada41f687c0e4bb34d5925374a68a8f8a3
darosior:
That said, certain users care strongly about using those options. In these conditions, i do not see the project removing the option anytime soon. Therefore i think it's technically incorrect (and confusing) to mark it as deprecated. utACK 451ba9ada41f687c0e4bb34d5925374a68a8f8a3 on removing the deprecation.
instagibbs:
crACK 451ba9ada41f687c0e4bb34d5925374a68a8f8a3
Raimo33:
ACK 451ba9ada41f687c0e4bb34d5925374a68a8f8a3
Ademan:
utACK 451ba9a
ryanofsky:
Code review ACK 451ba9ada41f687c0e4bb34d5925374a68a8f8a3
marcofleon:
ACK 451ba9ada41f687c0e4bb34d5925374a68a8f8a3
achow101:
ACK 451ba9ada41f687c0e4bb34d5925374a68a8f8a3
moonsettler:
ACK 451ba9ada4
ismaelsadeeq:
utACK 451ba9ada41f687c0e4bb34d5925374a68a8f8a3 🛰️
jonatack:
ACK 451ba9ada41f687c0e4bb34d5925374a68a8f8a3
Zero-1729:
crACK 451ba9ada41f687c0e4bb34d5925374a68a8f8a3
vasild:
ACK 451ba9ada41f687c0e4bb34d5925374a68a8f8a3
Tree-SHA512: b83fc509f5dd820976596e1ae9fb69a22ada567e0e0ac88da5fc5e940a46d8894b40cc70c3eff2cbdabd4da5ec913f0d18c1632fc906f210b308855868410699
Currently nodes schedule their invs to all inbound peers at the same time.
It is trivial to make use this timing pattern for fingerprinting
identities on different networks. Using a separate timers for each network will
make the fingerprinting harder.
Not enforcing TRUC topology on reorg was the intended
behavior, but the appropriate bypass argument was not
checked.
This mistake means we could potentially invalidate a long
chain of perfectly incentive-compatible transactions that
were made historically, including subsequent non-TRUC
transactions, all of which may have been very high feerate.
Lastly, it wastes CPU cycles doing topology checks since
this behavior cannot actually enforce the topology in
general for the reorg setting.
Using bypass_limits=true is essentially fuzzing part of a
reorg only, and results in TRUC invariants unable to be
checked. Remove most instances of bypassing limits, leaving
one harness able to do so.
The removed statements were logged up to two or three times for each loaded
wallet. The SQLite version only needs to be logged once.
The full wallet path is dropped, since the existing unconditional
logging while loading wallets is sufficient (also reduces anonymization
efforts in case of sharing logs).
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
`CConnman::AlreadyConnectedToAddress()` is the only caller of
`CConnman::FindNode(CNetAddr)`, so merge the two in one function.
The unit test that checked whether `AlreadyConnectedToAddress()` ignores
the port is now unnecessary because now the function takes a `CNetAddr`
argument. It has no access to the port.
1ff9e929489e625a603e8755b8efe849feda1f16 key: use static context for libsecp256k1 calls where applicable (Sebastian Falbesoner)
Pull request description:
The dynamically created [signing context](2d6a0c4649/src/key.cpp (L19)) for libsecp256k1 calls is only needed for functions that involve generator point multiplication with a secret key, i.e. different variants of public key creation and signing. The API docs hint to those by stating "[(not secp256k1_context_static)](b475654302/include/secp256k1.h (L645))" for the context parameter. In our case that applies to the following calls:
- `secp256k1_ec_pubkey_create`
- `secp256k1_keypair_create`
- `secp256k1_ellswift_create`
- `secp256k1_ecdsa_sign`
- `secp256k1_ecdsa_sign_recoverable`
- `secp256k1_schnorrsig_sign32`
- `ec_seckey_export_der` (not a direct secp256k1 function, but calls `secp256k1_ec_pubkey_create` inside)
For all the other secp256k1 calls we can simply use the static context. This is done for consistency to other calls that already use `secp256k1_context_static`, and also to reduce dependencies on the global signing context variable. Looked closer at this in the course of reviewing #29675, where some functions used the signing context that didn't need to, avoiding a move to another module (see https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2333831377).
ACKs for top commit:
Eunovo:
ACK 1ff9e92948
furszy:
ACK 1ff9e929489e625a603e8755b8efe849feda1f16
rkrux:
crACK 1ff9e929489e625a603e8755b8efe849feda1f16
Tree-SHA512: f091efa56c358057828f3455d4ca9ce40ec0d35f3e38ab147fe3928bb5dbf7ffbc27dbf97b71937828ab95ea4e9be5f96d89a2d29e2aa18df4542aae1b33e258
316a0c513278d53cb25f42ea502d20691962aad6 rpc: addpeeraddress: throw on invalid IP (John Moffett)
Pull request description:
Right now we return an opaque `{"success" : false}` in `addpeeraddress` for an empty or invalid IP. This changes it to throw `RPC_CLIENT_INVALID_IP_OR_SUBNET` with the error message `Invalid IP address`. Tests updated to match.
ACKs for top commit:
sipa:
utACK 316a0c513278d53cb25f42ea502d20691962aad6
achow101:
ACK 316a0c513278d53cb25f42ea502d20691962aad6
vasild:
ACK 316a0c513278d53cb25f42ea502d20691962aad6
pablomartin4btc:
tACK 316a0c513278d53cb25f42ea502d20691962aad6
Tree-SHA512: 79a8ce127d0a24b2eb1f31bc3294b895d0c6424032a6b49168259e0e94aff69723d067adf1b4dc3c9b79e597531e5b65e4b8fc5a8e21fba0b81f99168de12b96
453b0fa286e5dce0af682b7b73684dd6415a50de bitcoin: Make wrapper not require -m (Ryan Ofsky)
29e836fae660d9a89c54a094ae1a032e6a88c334 test: add tool_bitcoin to test bitcoin wrapper behavior (Ryan Ofsky)
0972f5504021b482b27523fd3bcb8036cf6b439c init: add exe name to bitcoind, bitcoin-node -version output to be able to distinguish these in tests (Ryan Ofsky)
Pull request description:
This change makes the `bitcoin` command respect IPC command line options and _bitcoin.conf_ settings, so IPC listening can be enabled by just running `bitcoin node -ipcbind=unix` or `bitcoin node` with `ipcbind=unix` in the configuration file, and there is no longer a need to specify a multiprocess `-m` option like `bitcoin -m node [...]`
sipa and theuni in #31802 pointed out that users shouldn't be exposed to multiprocess implementation details just to use IPC features, so current need to specify the `bitcoin -m` option in conjunction with `-ipcbind` could be seen as a design mistake and not just a usage inconvenience.
This PR also adds a dedicated functional test for the `bitcoin` wrapper command and to make sure it calls the right binaries and test the new functionality.
---
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/issues/28722).
ACKs for top commit:
Sjors:
re-ACK 453b0fa286e5dce0af682b7b73684dd6415a50de
achow101:
ACK 453b0fa286e5dce0af682b7b73684dd6415a50de
TheCharlatan:
Re-ACK 453b0fa286e5dce0af682b7b73684dd6415a50de
Tree-SHA512: 9e49cb7e183fd220fa7a4e8ac68cef55f3cb2ccec40ad2a9d3e3f31db64c4953db8337f8caf7fce877bc97002ae97568dcf47ee269a06ca1f503f119bfe392c1
df67bb6fd84c393eaf00f19074085ee080546bd3 test: Remove convert_to_json_for_cli (Ava Chow)
44a493e150a706ec10899d0fcbc029e7466e5e81 cli: Allow arguments to be both strings and json (Ava Chow)
Pull request description:
There are some RPCs where the argument can be either JSON that needs to be parsed, or a string that we can pass straight through. However, `bitcoin-cli` would always parse those arguments as JSON which makes for some cumbersome argument passing when using those RPCs. Notably, `hash_or_height` in `getblockstats` and `gettxoutsetinfo` do this, and results in a more cumbersome command of `bitcoin-cli getblockstats '"<hash>"'`. Otherwise, using a normal invocation of `bitcoin-cli getblockstats <hash>` results in `error: Error parsing JSON`. This PR marks those particular options as also being a string so that when `bitcoin-cli` fails to parse the argument as JSON, it will assume that the argument is a string and pass it straight through.
ACKs for top commit:
ryanofsky:
Code review ACK df67bb6fd84c393eaf00f19074085ee080546bd3, just rebased since last review. I do still think it would be good to improve the test (https://github.com/bitcoin/bitcoin/pull/33230#discussion_r2369570345)
rkrux:
Light code review, lgtm ACK df67bb6fd84c393eaf00f19074085ee080546bd3
mzumsande:
Code Review ACK df67bb6fd84c393eaf00f19074085ee080546bd3
Tree-SHA512: 6c488570fbb24d0cf10508416c56accfc7af5163b7a7187d22d78c812424a9e3ecc95906d3e295fbf6af54bf80903aa448fd879dd6a9944ba8b4d1a33eb29ef2
b807dfcdc5929c314d43b790c9e705d5bf0a86e8 miner: fix `addPackageTxs` unsigned integer overflow (ismaelsadeeq)
Pull request description:
This PR fixes an unsigned integer overflow in the `addPackageTxs` method of the `BlockAssembler`.
The overflow is a rare edge case that might occur on master when a miner reserves 2000 WU and wants to create an block to be empty.
i.e, by starting with `-blockmaxweight=2000`, `-blockreservedweight=2000`, or just `blockmaxweight=2000`, and then calling the mining interface `createNewBlock` with `blockReservedWeight` set to `2000`.
Instead of bailing out after going through transactions equivalent to `MAX_CONSECUTIVE_FAILURES`, the loop never breaks until all mempool transactions are visited.
See https://github.com/bitcoin/bitcoin/pull/33421#issuecomment-3324859282
The fix avoids the overflow by using addition instead adding `BLOCK_FULL_ENOUGH_WEIGHT_DELTA` to the block weight and comparing it with `m_options.nBlockMaxWeight`.
Another alternative that preserves the same structure is to use `static_cast`. See c9530cf35d.
This fix can be tested by cherry-picking the commits from #33421 without the static cast fix and running:
```bash
echo "AQAAAAAAA
AAnJycnAAAAAAAAAAAAAAAAAA" | base64 --decode > miner.crash
FUZZ=block_template_cache ./build_fuzz/bin/fuzz miner.crash
```
---
This is part of a larger inconsistency in how size/weight is represented in the codebase. It may be worth defining a dedicated type for size/weight.
ACKs for top commit:
glozow:
nice, utACK b807dfcdc5929c314d43b790c9e705d5bf0a86e8
furszy:
Code ACK b807dfcdc5929c314d43b790c9e705d5bf0a86e8
Tree-SHA512: c1d2f7e500f9b0624a4c22a146921a1644017065e6c94d0c5027486392321f5de26c61751a24765e025e45b34c535adfd6d0e2ac809dea6846b99f37d13043c9
bf7996cbc3becf329d8b1cd2f1007fec9b3a3188 rpc: fix getblock(header) returns target for tip (Sjors Provoost)
4c3c1f42cf705e039751395799240da33ca969bd test: add block 2016 to mock mainnet (Sjors Provoost)
Pull request description:
A `target` field was added to the `getblock` and `getblockheader` RPC calls in #31583, but it mistakingly always used the tip value.
This PR fixes it to return the target for the given block. Because regtest does not have difficulty adjustment, the mainnet test is expanded to cover the fix.
A preliminary commit deals with mining block 2016 that's needed for the test. It also:
- renames the `create_coinbase` `retarget_period` argument to `halving_period`. Before #31583 this was hardcoded for regtest where these values are the same.
- drops unused `fees` argument from `mine` helper
- expands the CPU miner instructions for generating the alternative mainnet chain
Fixes#33440
ACKs for top commit:
sipa:
utACK bf7996cbc3becf329d8b1cd2f1007fec9b3a3188
luke-jr:
crACK bf7996cbc3becf329d8b1cd2f1007fec9b3a3188
TheCharlatan:
ACK bf7996cbc3becf329d8b1cd2f1007fec9b3a3188
ismaelsadeeq:
Code review ACK bf7996cbc3becf329d8b1cd2f1007fec9b3a3188
Tree-SHA512: 2a2e11efd91f4aaccf9d2ec4dff9fd82c366b8a7e797ce5981dca2e6f08028f69154f4e6a27aef20d78b0e6c3304416789267c2fad42d7aa5072f8537d0c8b0d
88b0647f027a608acb61ec32329d19f8e5b0a9fd wallet: Always write last hardened cache flag in migrated wallets (Ava Chow)
8a08eef645eeb3e1991a80480c5ee232bfceeb37 tests: Check that the last hardened cache upgrade occurs (Ava Chow)
Pull request description:
#32597 set the descriptor cache upgraded flag for newly created wallets, but migrated wallets still did not have the flag set when they are migrated. For consistency, and to avoid an unnecessary upgrade, we should be setting this flag for migrated wallets.
The flag would end up being set anyways at the end of migration when the wallet is reloaded as it would perform the automatic upgrade at that time. However, this is unnecessary and we should just set it from the get go.
This PR also adds a couple tests to verify that the flag is being set, and that the upgrade is being performed.
ACKs for top commit:
cedwies:
re-ACK 88b0647
rkrux:
lgtm ACK 88b0647f027a608acb61ec32329d19f8e5b0a9fd
pablomartin4btc:
ACK 88b0647f027a608acb61ec32329d19f8e5b0a9fd
Tree-SHA512: 7d0850db0ae38eedd1e6a3bfaa548c6c612182291059fb1a47279a4c4984ee7914ecd02d8c7e427ef67bf9f5e67cbc57a7ae4412fad539e1bf3e05c512a60d69
2427939935f3e6669be6bf553be89639e0afabaa test: forbid copying of DebugLogHelper (Daniel Pfeifer)
d6aa266d432f24c1f1bf7ece64aeba342cabeaf2 test: don't throw from the destructor of DebugLogHelper (Vasil Dimov)
Pull request description:
Throwing an exception from the destructor of a class is a bad practice because the destructor will be called when an object of that type is alive on the stack and another exception is thrown, which will result in "exception during the exception". This would terminate the program without any messages.
Instead print the message to the standard error output and call `std::abort()`.
---
This change is part of https://github.com/bitcoin/bitcoin/pull/26812. It is an improvement on its own, so creating a separate PR for it following the discussion at https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2345091587. Getting it in will reduce the size of #26812.
ACKs for top commit:
Crypt-iQ:
crACK 2427939
l0rinc:
Code review reACK 2427939935f3e6669be6bf553be89639e0afabaa
optout21:
crACK 2427939935f3e6669be6bf553be89639e0afabaa
furszy:
utACK 2427939935f3e6669be6bf553be89639e0afabaa
Tree-SHA512: 918c1e40d2db4ded6213cd78a18490ad10a9f43c0533df64bdf09f0b216715415030e444712981e4407c32ebf552fbb0e3cce718e048df10c2b8937caf015564
The generic key can also be used in other places
where behavior between different network identities should
be uncorrelated to avoid fingerprinting.
This also changes RANDOMIZER_ID - since it is not
being persisted to disk, there are no compatibility issues.
2738b63e025d240618b3c72c28243c3e4d7d9c79 test: validate behaviour of getpeerinfo last_inv_sequence and inv_to_send (Anthony Towns)
77b2ebb811824899f56976f8e3113914706edc97 rpc/net: report per-peer last_inv_sequence (Anthony Towns)
adefb51c5437667696cacaf163ea08b39e961358 rpc/net: add per-peer inv_to_send sizes (Anthony Towns)
Pull request description:
Adds per-peer entries to `getpeerinfo` for the size of the inv_to_send queue and the mempool sequence number as at the last INV. Can be helpful for debugging tx relay performance and privacy/fingerprinting issues.
ACKs for top commit:
sipa:
utACK 2738b63e025d240618b3c72c28243c3e4d7d9c79
instagibbs:
ACK 2738b63e025d240618b3c72c28243c3e4d7d9c79
Tree-SHA512: e3c9c52e8e38b099d405a177ffba6783c5821cc5ce1432b98218843e00906986ce2141dcd5b04a67006c328211a672e519fa3390e012688499bfc9ac99767599
cad9a7fd7370f6df38370569f9d2de6ac6b7137a rpc: Always return per-wtxid entries in submitpackage tx-results (John Moffett)
Pull request description:
Follow-up to #28848
When `submitpackage` produced no per-transaction result for a member, the RPC set `"error": "unevaluated"` but then continued without inserting the entry into `tx-results`, making it impossible for callers to know which `wtxids` were unevaluated.
This inserts the error result before continuing, updates help text, and adjusts functional tests to expect entries for all submitted `wtxids`.
ACKs for top commit:
instagibbs:
ACK cad9a7fd7370f6df38370569f9d2de6ac6b7137a
glozow:
ACK cad9a7fd7370f6df38370569f9d2de6ac6b7137a
Tree-SHA512: 8df5c9b3d1f17aaf0311c38f028ae4b55d4c52a660f85171f105c4f65d130b14ab00698ac5d7c27403a0c37fff391c154c3ad44cc99ba4d549d9c30751b8360f
56791b582958e905e5ba5cbf172a8ea7dad1a8b0 test: split out `system_ram_tests` to signal when total ram cannot be determined (Lőrinc)
337a6e738616781f81504275bac8ed7bcf8068df system: improve handling around GetTotalRAM() (Vasil Dimov)
Pull request description:
1. Fix unused variable warning (https://github.com/bitcoin/bitcoin/pull/33333#discussion_r2362493046)
2. Enable `GetTotalRAM()` on other platforms where it was tested to work.
3. Skip the `GetTotalRAM()` unit test on unsupported platforms.
Prior discussion: https://github.com/bitcoin/bitcoin/pull/33333#discussion_r2362493046
ACKs for top commit:
l0rinc:
ACK 56791b582958e905e5ba5cbf172a8ea7dad1a8b0
hebasto:
ACK 56791b582958e905e5ba5cbf172a8ea7dad1a8b0.
Tree-SHA512: bc419aa55edad77473dbcf810f02d02fa0c45a6355a93d17f7881051117b753c584296ab3840893270ecdc9bb2bee0fe4e070607c6560b794e97a25da733c47d
This patch achieves two things:
1. Fix unused variable warning (https://github.com/bitcoin/bitcoin/pull/33333#discussion_r2362493046)
2. Enable GetTotalRAM() on other platforms where it was tested to work.
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
A target field was added to the getblock and getblockheader RPC calls in bitcoin#31583, but it mistakingly always used the tip value.
Because regtest does not have difficulty adjustment, a test is added for mainnet instead.
6a33970fef1b7b4d634f28277607b882958c95ac fuzz: Reduce iterations in slow targets (marcofleon)
Pull request description:
The `mini_miner`, `txdownloadman`, `txdownloadman_impl`, and `tx_pool_standard` fuzz targets are all slow-running targets. Fix this by reducing the iteration count in the `LIMITED_WHILE` loops.
This should help decrease the run time of the fuzz CI jobs. See https://github.com/bitcoin/bitcoin/pull/33425.
Addresses https://github.com/bitcoin/bitcoin/issues/32870 as well.
ACKs for top commit:
Crypt-iQ:
crACK 6a33970fef1b7b4d634f28277607b882958c95ac
dergoegge:
utACK 6a33970fef1b7b4d634f28277607b882958c95ac
enirox001:
Concept ACK 6a33970
brunoerg:
ACK 6a33970fef1b7b4d634f28277607b882958c95ac
Tree-SHA512: d03d687507f497e587f7199866266298ca67d9843985dc96d1c957a6fbffb3c6cd5144a4876c471b84c84318295b0438908c745f3a4ac0254dca3e72655ecc14
b81f37031c8f2ccad9346f1b65ee0f8083c44796 p2p: Increase tx relay rate (Anthony Towns)
Pull request description:
In the presence of smaller transactions on the network, blocks can sustain a higher relay rate than 7tx/second. In this event, the per-peer inventory queues can grow too large.
This commit bumps the rate up to 14 tx/s (for inbound peers), increasing the safety margin by a factor of 2.
Outbound peers continue to receive relayed transactions at 2.5x the rate of inbound peers, for a rate of 35tx/second.
ACKs for top commit:
sipa:
ACK b81f37031c8f2ccad9346f1b65ee0f8083c44796
achow101:
ACK b81f37031c8f2ccad9346f1b65ee0f8083c44796
darosior:
utACK b81f37031c8f2ccad9346f1b65ee0f8083c44796.
glozow:
utACK b81f37031c8f2ccad9346f1b65ee0f8083c44796
Tree-SHA512: 854ea0824d5f4c629f1dceb9ee61cc9226c8f0d4d26664737e68db917f65341d4800362ab55ed32673db920b2b59aa116b4cb9ee063367b2e43c94a904b41c08
When submitpackage produced no per-transaction result for a member,
the RPC previously set "error": "unevaluated" but then continued
without inserting the entry into tx-results, making it impossible for
callers to know which wtxids were unevaluated.
Insert the placeholder result before continuing, update help text, and
adjust functional tests to expect entries for all submitted wtxids.
