6869fb4170 net: Block v2->v1 transport downgrade if !CConnman::fNetworkActive (Hodlinator)
Pull request description:
We might have just set `CNode::fDisconnect` in the first loop because of `!CConnman::fNetworkActive`.
Attempting to reconnect using v1 transport just because `fNetworkActive` was set to `false` at the "right" stage in the v2 handshake does not make sense.
Issue [discovered](https://github.com/bitcoin/bitcoin/pull/31633#discussion_r1930908304) by davidgumberg.
ACKs for top commit:
davidgumberg:
Tested and Reviewed ACK 6869fb4170
mabu44:
ACK 6869fb4170
stratospher:
ACK 6869fb4. I've reviewed the code but don't have strong preference for this branch vs master since only functional change is just a single log not being printed in a low probability scenario (we happen to be attempting v2 connection when P2P network activity is being turned off).
vasild:
ACK 6869fb4170
Tree-SHA512: 54f596e54c5a6546f2c3fec2609aa8d10dec3adcf1001ca16666d8b374b8d79d64397f46c90d9b3915b4e91a5041b6ced3044fd2a5b4fb4aa7282eb51f61296a
a015b7e13d test: Add expected result assertions (yancy)
Pull request description:
~This is a trivial addition to the test suit, however it shouldn't be required to add debug statements and manually run the tests if someone needs to know the results of this test.~
Add an assertion for the values returned. The goal of the test is to show that a minimal weight selection of UTXOs is returned by coin-grinder. Since there are multiple possible solutions, the added assertion shows that coin-grinder finds the solution with the lowest weight. Without this assertion, it's ambiguous whether or not coin-grinder is returning the solution with the lowest weight.
Remove the check that a result is returned since the expected result assertion implies a result.
ACKs for top commit:
janb84:
re ACK [a015b7e](https://github.com/bitcoin/bitcoin/commit/a015b7e13daacdfb6db0eada50563dec70c5afb2)
murchandamus:
ACK a015b7e13d
Tree-SHA512: ee3c2688b4a4a07ab209f7655c3956e62a1084419df5e87c27d751a38ff64d4c3457df2317f8077149a6947cdb05b249975de2b8f0e18ca8b17b41f4735fb1c6
2f2ab47bf7 Release notes (Pol Espinasa)
bf194c920c wallet, rpc: deprecate settxfee and paytxfee (Pol Espinasa)
Pull request description:
**Summary**
This PR deprecates the settxfee RPC and paytxfee setting, marking it for removal in Bitcoin Core 31.0.
**Motivation**
The PR was initially motivated by https://github.com/bitcoin/bitcoin/issues/31088. The intention was to create a new function `settxfeerate` to allow users to set a static fee rate in `sat/vB` instead of `btc/kvB`.
The `settxfee` RPC allows users to set a static fee rate for all transactions created by the wallet. However, in a dynamic fee environment, this can lead to poor fee choices, either overpaying when the mempool is empty or underpaying when congestion is high. The preferred approach is to rely on fee estimation, which is designed to adapt to network conditions, and is the one by default. Same argument apply for `paytxfee` setting.
During discussion the consensus was that static fee settings are a footgun and that users should instead specify the fee rate per transaction if they don't want to rely on the fee estimation. Given this, rather than introducing a `settxfeerate` alternative, this PR goes towards removing `settxfee` and `paytxfee` entirely.
**Key Changes**
`settxfee` and `paytxfee` is now deprecated and will be removed in Bitcoin Core 31.0.
Users should rely on fee estimation or explicitly specify a fee rate when constructing transactions.
**Impact on Users**
If users currently use settxfee or paytxfee, they should transition to specifying fees per transaction.
No immediate breakage in 30.0 (must use `-deprecatedrpc=settxfee`), but `settxfee` and `paytxfee` will be removed in 31.0.
**Alternative Approaches Considered**
A settxfeerate alternative (using sat/vB) was initially proposed but ultimately rejected in favor of deprecating static fee setting entirely.
**Notes for removal**
- When removing paytxfee we should also update txconfirmtarget startup option help text.
- Get back the comment from `rpc_deprecated.py` test. [+info](https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1998876768)
ACKs for top commit:
fjahr:
re-ACK 2f2ab47bf7
ismaelsadeeq:
re-ACK 2f2ab47bf7
rkrux:
Concept and utACK 2f2ab47bf7
Tree-SHA512: 0272812cbe5a519737c5d0683acc2072e67559792b4a6472bca8b23426e5ce9e88a3a1eba987feda70a082b8b474b3126893848628d7bf11e1520357b18c8d3e
d423fd9ec8 cli, bugfix: for -getinfo, replace IsArgSet() with GetBoolArg() (Jon Atack)
e99e41b307 cli, refactor: simplify public-only classes with structs (Jon Atack)
fdbfd250fb cli, refactor: deduplicate NetworkStringToId() (Jon Atack)
be82139b2a cli, refactor: simplify DetailsRequested() (Jon Atack)
Pull request description:
These have been accumulating over the past few years.
Each is described in its commit message.
ACKs for top commit:
pablomartin4btc:
re-ACK d423fd9ec8
hodlinator:
Code review ACK d423fd9ec8
l0rinc:
ACK d423fd9ec8
ryanofsky:
Code review ACK d423fd9ec8, just running clang-format and updating commit messages since last review
Tree-SHA512: a8e5f7827cef308186d5a7c3a627d2cd8f57437f4465d181986e5d3274ff0e2b9faac252dd55d9257d66a7aa99fca62b3000cdc0988d23385df20ff1f870a046
eb0724f0de doc: banman: reference past vuln due to unbounded banlist (Antoine Poinsot)
ad616b6c01 doc: net: mention past vulnerability as rationale to limit incoming message size (Antoine Poinsot)
4489117c3f doc: txrequest: point to past censorship vulnerability in tx re-request handling (Antoine Poinsot)
68ac9542c4 doc: net_proc: reference past DoS vulnerability in orphan processing (Antoine Poinsot)
c02d9f6dd5 doc: net_proc: reference past defect regarding invalid GETDATA types (Antoine Poinsot)
5e3d9f21df doc: validation: add a reference to historical header spam vulnerability (Antoine Poinsot)
Pull request description:
It is useful when reading code to have context about why it is written or behaves the way it does. Some instances in this PR may seem obvious but i think nonetheless offer important context to anyone willing to change (or review a change to) this code.
ACKs for top commit:
ryanofsky:
Code review ACK eb0724f0de. No changes since last review other than rebase
Tree-SHA512: 271902f45b8130d44153d793bc1096cd22b6ce05494e67c665a5bc45754e3fc72573d303ec8fc7db4098d473760282ddbf0c1cf316947539501dfd8d7d5b8828
7edaf8b64c Benchmark Chainstate::ConnectBlock duration (Eunovo)
Pull request description:
Introduce benchmarks to evaluate ConnectBlock performance for:
- Blocks containing only Schnorr signatures
- Blocks containing both Schnorr and ECDSA signatures
- Blocks containing only ECDSA signatures
The benchmarks in this PR, focus on signature validation. Additional benchmarks may be added in the future to assess other aspects of ConnectBlock.
This is the first step toward implementing Batch Verification of Schnorr Signatures in Core. It provides a way to test and measure the performance improvements of batch verification on Core.
For more details on batch validation, refer to the [batch-verify module on secp](https://github.com/bitcoin-core/secp256k1/pull/1134) and [batch-verify on core](https://github.com/bitcoin/bitcoin/pull/29491).
ACKs for top commit:
josibake:
reACK https://github.com/bitcoin/bitcoin/commit/7edaf8b64cb2d59ada22042fee62a417e52368b8
fjahr:
utACK 7edaf8b64c
l0rinc:
ACK 7edaf8b64c
Tree-SHA512: 883c8a5e4e4de401ffb9ac9b6789b7fe0737afefbdaf02c6d7e1645392efc4f0d2d28b423ba7e34366a33608e0835793f5e7a1312b5c8063de14446319529cc7
d190f0facc test, contrib: Fix signer/miner command line escaping (Ryan Ofsky)
0d2eefca8b test, refactor: Add TestNode.binaries to hold binary paths (Ryan Ofsky)
Pull request description:
Add new `TestNode.binaries` object to manage paths to bitcoin binaries.
The `binaries` object makes it possible for the test framework to exercise the bitcoin wrapper executable introduced in https://github.com/bitcoin/bitcoin/pull/31375 and also makes it easier in general to add new binaries, and new options and environment variables controlling how they are invoked, because logic for invoking them that was previously spread out is now consolidated in one place.
These changes were originally part of #31375 but made that PR harder to review because they were unrelated to the other changes there. If this PR can get merged first, python changes in #31375 will be simple, and the test framework changes here should also get a higher quality review.
ACKs for top commit:
maflcko:
re-review-ACK d190f0facc🍓
Sjors:
ACK d190f0facc
vasild:
ACK d190f0facc
Tree-SHA512: 5a6c0553cd2822585810d827ef1c1772cbf3097d3336daf733f8378dd3da79c00fc3721e50ed0f7455908fbd7a509e9739f9be33f588d6bc1aaa400b9d75c650
21e9d39a37 docs: add release notes for 31603 (brunoerg)
a8b548d75d test: `getdescriptorinfo`/`importdescriptors` with whitespace in pubkeys (brunoerg)
c7afca3d62 test: descriptor: check whitespace into keys (brunoerg)
cb722a3cea descriptor: check whitespace in ParsePubkeyInner (brunoerg)
50856695ef test: fix descriptors in `ismine_tests` (brunoerg)
Pull request description:
Currently, we successfully parse descriptors which contains spaces in the beginning or end of the public/private key within a fragment (e.g. `pk( KEY)`, `pk(KEY )` or `pk( KEY )`). I have noticed that one of the reasons is that the `DecodeBase58` function simply ignore these whitespaces.
This PR changes the `ParsePubkeyInner ` to reject pubkeys that contain a whitespace at the beginning and/or at the end. We will only check the whitespace in some RPCs (e.g. `importdescriptors`), but an already imported descriptor won't be affected by this check, especially because we store descriptors from `ToString`.
For context: https://github.com/brunoerg/bitcoinfuzz/issues/72
ACKs for top commit:
rkrux:
tACK 21e9d39a37
darosior:
re-ACK 21e9d39a37
sipa:
utACK 21e9d39a37
Tree-SHA512: 54f48a89a235517e5cdc29a46dceeb7dabbee93c7616a166288ff3f90131808eb0ece43b0797a11fe827a5f7bd51d65e3e75c16789b0a42020934cabb684cc8f
Pass bitcoin binary command lines from test framework to signet/miner utility
using shell escaping so they are unambigous and don't get mangled if they
contain spaces.
This change is not needed for tests to pass currently, but is a useful change
to avoid CI failures in followup PR
https://github.com/bitcoin/bitcoin/pull/31375 and to avoid other bugs.
Add new TestNode.binaries object to manage paths to bitcoin binaries.
Having this object makes it possible for the test framework to exercise the
bitcoin wrapper executable introduced in
https://github.com/bitcoin/bitcoin/pull/31375 and also makes it easier to add
new binaries and options and environment variables controlling how they are
invoked, because logic for invoking them that was previously spread out is now
consolidated in one place.
Co-authored-by: Sjors Provoost <sjors@sprovoost.nl>
e5ff4e416e qa: use a clearer and documented amount error in malleated snapshot (Antoine Poinsot)
b34fdb5ade test: introduce output amount (de)compression routines (Sebastian Falbesoner)
a7911ed101 test: introduce VARINT (de)serialization routines (Sebastian Falbesoner)
Pull request description:
The `feature_assumeutxo.py` functional test checks various errors with malleated snapshots. Some of these cases are brittle or use confusing and undocumented values. Fix one of those by using a clear, documented and forward-compatible value.
I ran across those when working on an unrelated changeset which affected the snapshot. It took me a while to understand where the seemingly magic byte string was coming from, so i figured it was worth proposing this patch on its own for the sake of making the test more maintainable.
See commit messages for details.
ACKs for top commit:
janb84:
re ACK [e5ff4e4](https://github.com/bitcoin/bitcoin/commit/e5ff4e416ecc8a51367022eb8a7a291f8cbc0c65)
theStack:
ACK e5ff4e416e
fjahr:
Code review ACK e5ff4e416e
i-am-yuvi:
tACK e5ff4e416e
Tree-SHA512: 60f022b7176836ce05e8f287b436329d7ca6460f3fcd95f78cd24e07a95a7d4d9cbbb68a117916a113fe451732b09a012d300fe860ff33d61823eca797ceddaf
11f8ab140f test: wallet, coverage for crash on dup block disconnection during unclean shutdown (Martin Zumsande)
9ef429b6ae wallet: fix crash on double block disconnection (furszy)
Pull request description:
The wallet crashes if it processes the same block disconnection event twice in a row due
to an incompatible coinbase transaction state.
This happens because `disconnectBlock` provides `TxStateInactive` without the "abandoned"
flag for coinbase transactions to `SyncTransaction`, while `AddToWallet()` internally modifies
it to retain the abandoned state.
The crash flow is as follows:
1) On the first disconnection, the transaction state transitions from "confirmed" to
"inactive," bypassing the state equality check since the provided state differs. Then,
`AddToWallet` internally updates the state to "inactive + abandoned"
2) On the second disconnection, as we provide only the "inactive" state
to `SyncTransaction()`, the state equality assertion fails and crashes the wallet.
Reviewers Note:
The crash can easily be replicated by cherry-picking the test commit in master.
ACKs for top commit:
mzumsande:
Code Review ACK 11f8ab140f
rkrux:
ACK 11f8ab140f
pinheadmz:
ACK 11f8ab140f
Tree-SHA512: 971069bca562f0afb06c34a2516842d01b5cbc2b18ed851392aa3caa3bb7488f4a84a5d017ea334e6361261d3c44aa597cc67a1d4fa16781f85e081f3d1f8771
cadbd4137d miner: have waitNext return after 20 min on testnet (Sjors Provoost)
d4020f502a Add waitNext() to BlockTemplate interface (Sjors Provoost)
Pull request description:
This PR introduces `waitNext()`. It waits for either the tip to update or for fees at the top of the mempool to rise sufficiently. It then returns a new template, with which the caller can rinse and repeat.
On testnet3 and testnet4 the difficulty drops after 20 minutes, so the second ensures that a new template is returned in that case.
Alternative approach to #31003, suggested in https://github.com/bitcoin/bitcoin/issues/31109#issuecomment-2451942362
ACKs for top commit:
ryanofsky:
Code review ACK cadbd4137d. Main change since last review is adding back a missing `m_interrupt` check in the waitNext loop. Also made various code cleanups in both commits.
ismaelsadeeq:
Code review ACK cadbd4137d
vasild:
ACK cadbd4137d
Tree-SHA512: c5a40053723c1c1674449ba1e4675718229a2022c8b0a4853b12a2c9180beb87536a1f99fde969a0ef099bca9ac69ca14ea4f399d277d2db7f556465ce47de95
This commit does not change behavior, it just changes code to handle -noconnect
values explicitly with IsArgNegated() instead of implicitly with IsArgSet(),
and adds comments to make it clear what behavior is intended when -noconnect is
specified.
Make sure -noconnect has same effect as -connect for disabling DNS seeding and
listening by default, and warning about -dnsseed being ignored with the -proxy
setting.
Initial implementation of https://github.com/bitcoin/bitcoin/pull/30529
accidentally broke this behavior, so having coverage may be useful to make sure
it does not break again.
Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
This commit does not change behavior because negation of -signetseednode and
-signetchallenge parameters has been disallowed since these were introduced in
#18267, so calling IsArgSet() is equivalent to checking if GetArgs() returns a
non-empty list.
This commit does not change behavior, it just drops unnecessary IsArgSet()
calls for -debug, -loglevel, and -vbparams options. The calls are unnecessary
because GetArgs() already returns empty arrays if these arguments are not
specified.
Treat specifying -noexternalip the same as not specifying -externalip, instead
of causing it to soft-set the -discover default to false.
Before this change, was -noexternalip basically an undocumented synonym for
-nodiscover.
After this change, specifying -noexternalip just clears previously specifed
-externalip options, restoring default behavior as if they were not were
specified.
The previous -noexternalip behavior wasn't neccessarily bad, but it was
undocumented, redundant with the -nodiscover option, and inconsistent with
behavior of other list options.
Treat specifying -noonlynet the same as not specifying -onlynet, instead of
marking all networks unreachable.
Before this change, specifying -noonlynet cleared list of reachable networks
and did not allow connecting to any network. It was basically an undocumented
synonym for -noconnect.
After this change, specifying -nononlynet just clears previously specifed
-onlynet options and allows connecting to all networks, restoring default
behavior as if no -onlynet options were specified.
Before this change, there was no way to restore default behavior once an
-onlynet option was specified. So for example, if a config file specifed
onlynet settings, they couldn't be reset on the command line without disabling
the entire config file.
The previous -noonlynet behavior wasn't neccessarily bad, but it was
undocumented, redundant with the -noconnect option, inconsistent with behavior
of other list options, and inconsistent with being able to use the command line
to selectively override config options. It was also probably unintended,
arising from use of the IsArgSet() method and its interaction with negated
options.
Treat specifying -norpcwallet the same as not specifying any -rpcwallet option,
instead of treating it like -rpcwallet=0 with 0 as the wallet name.
This restores previous behavior before 743077544b
from https://github.com/bitcoin/bitcoin/pull/18594, which inadvertently changed
it.
Treat specifying -norpcwhitelist the same as not specifying -rpcwhitelist,
instead of behaving almost the same but flipping the default
-rpcwhitelistdefault value.
This is confusing because before this change if -norpcwhitelist was specified
it would block users from calling any RPC methods.
Treat specifying -norpcbind and -norpcallowip the same as not specifying
-rpcbind or -rpcallowip, instead of failing to bind to localhost and failing to
show warnings.
Also add code comment to clarify what intent of existing code is.
Treat specifying -noseednode the same as not specifying any -seednode value,
instead of enabling the seed node timeout and log messages, and waiting longer
to add other seeds.
7afeaa2469 test: `-debug=0` and `-debug=none` behave similarly to `-nodebug` (Daniela Brozzoni)
a8fedb36a7 logging: Ensure -debug=0/none behaves consistently with -nodebug (Daniela Brozzoni)
d39d521d86 test: `-nodebug` clears previously set debug options (Daniela Brozzoni)
Pull request description:
Previously, -nodebug cleared all prior -debug configurations in the command line while allowing subsequent debug options to be applied.
However, -debug=0 and -debug=none completely disabled debugging, even for categories specified afterward.
This commit ensures consistency by making -debug=0 and -debug=none behave like -nodebug: they now clear previously set debug configurations but do not disable debugging for categories specified later.
See https://github.com/bitcoin/bitcoin/pull/30529#discussion_r1930956563
ACKs for top commit:
hodlinator:
re-ACK 7afeaa2469
ryanofsky:
Code review ACK 7afeaa2469. Nicely implemented change with test and release notes, and I like how the test is implemented as the first commit.
maflcko:
review ACK 7afeaa2469👡
Tree-SHA512: c69b17ff10da6c88636bd01918366dd408832e70f2d0a7b951e9619089e89c39282db70398ba2542d3aa69a2fe6b6a0a01638b3225aff79d234d84d3067f2caa
2ffea09820 build: disable bitcoin-node if daemon is not built (Sjors Provoost)
Pull request description:
When building for fuzzing with multiprocess enabled, we were still trying to build `bitcoin-node`. This PR fixes that, by applying a similar check as for `bitcoin-gui`.
Before:
```
cmake -B build -DBUILD_FOR_FUZZING=ON -DWITH_MULTIPROCESS=ON
...
Configure summary
=================
Executables:
bitcoind ............................ OFF
bitcoin-node (multiprocess) ......... ON
bitcoin-qt (GUI) .................... OFF
bitcoin-gui (GUI, multiprocess) ..... OFF
...
cmake --build build
...
[ 84%] Built target bitcoin-node
```
After:
```
bitcoin-node (multiprocess) ......... OFF
```
And no `bitcoin-node` target gets built (not to be confused with `bitcoin_node`).
ACKs for top commit:
hebasto:
ACK 2ffea09820.
ryanofsky:
Code review ACK 2ffea09820
laanwj:
Code review ACK 2ffea09820
Tree-SHA512: bdb0b62049f77929d5c084bf98a076e9933de91eb30853ed89edd23cc81b3d4aec4cd57c9a9e21cf1d6930885f8c408dda830db6884b4e326c7fb348f1fbab4c
b448b01494 test: add a mocked Sock that allows inspecting what has been Send() to it (Vasil Dimov)
f1864148c4 test: put the generic parts from StaticContentsSock into a separate class (Vasil Dimov)
4b58d55878 test: move the implementation of StaticContentsSock to .cpp (Vasil Dimov)
Pull request description:
Put the generic parts from `StaticContentsSock` into a separate class `ZeroSock` so that they can be reused in other mocked `Sock` implementations.
Add a new `DynSock` whose `Recv()` and `Send()` methods can be controlled after the object is created. To achieve that, the caller/creator of `DynSock` provides to its constructor two pipes (FIFOs) - recv-pipe and send-pipe. Whatever data is written to recv-pipe is later received by `DynSock::Recv()` method and whatever data is written to the socket using `DynSock::Send()` can later be found in the send-pipe. For convenience there are also two methods to send and receive `CNetMessage`s.
---
This is used in https://github.com/bitcoin/bitcoin/pull/26812 (first two commits from that PR).
Extracting as a separate PR suggested here: https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1619152037.
ACKs for top commit:
Sjors:
re-ACK b448b01494
jonatack:
re-ACK b448b01494
pinheadmz:
ACK b448b01494
Tree-SHA512: 4a36f038192ec4ef63366cbe1a38ae70e7e015630c9f7c44926b756b20ab8c08138acae41801f23b30f6629c7059c1f81e001806e86584ff1bf1fa5b44d9caec
8888ee4403 ci: Allow build dir on CI host (MarcoFalke)
Pull request description:
This is required to pass cross builds on to a different machine after the build.
See for example https://github.com/bitcoin/bitcoin/pull/31176, but this pull will also allow someone to implement it outside this repo.
ACKs for top commit:
davidgumberg:
lgtm ACK https://github.com/bitcoin/bitcoin/commit/8888ee4403fa62972c49e18752695d15fd32c0b0
hebasto:
re-ACK 8888ee4403.
Tree-SHA512: a1e2c32bc1b95efbd0b48287ac5b49e0e1bacbf5a5800845be5352bbdd3e17fa478e90348b2e94e95cf3ae863cdf75ab444089376588f6f8eec438f73a4b5b97
a4df12323c doc: add release notes (Sjors Provoost)
c75872ffdd test: use DIFF_1_N_BITS in tool_signet_miner (tdb3)
4131f322ac test: check difficulty adjustment using alternate mainnet (Sjors Provoost)
c4f68c12e2 Use OP_0 for BIP34 padding in signet and tests (Sjors Provoost)
cf0a62878b rpc: add next to getmininginfo (Sjors Provoost)
2d18a078a2 rpc: add target and bits to getchainstates (Sjors Provoost)
f153f57acc rpc: add target and bits to getblockchaininfo (Sjors Provoost)
baa504fdfa rpc: add target to getmininginfo result (Sjors Provoost)
2a7bfebd5e Add target to getblock(header) in RPC and REST (Sjors Provoost)
341f932516 rpc: add GetTarget helper (Sjors Provoost)
d20d96fa41 test: use REGTEST_N_BITS in feature_block (tdb3)
7ddbed4f9f rpc: add nBits to getmininginfo (Sjors Provoost)
ba7b9f3d7b build: move pow and chain to bitcoin_common (Sjors Provoost)
c4cc9e3e9d consensus: add DeriveTarget() to pow.h (Sjors Provoost)
Pull request description:
**tl&dr for consensus-code only reviewers**: the first commit splits `CheckProofOfWorkImpl()` in order to create a `DeriveTarget()` helper. The rest of this PR does not touch consensus code.
There are three ways to represent the proof-of-work in a block:
1. nBits
2. Difficulty
3. Target
The latter notation is useful when you want to compare share work against either the pool target (to get paid) or network difficulty (found an actual block). E.g. for difficulty 1 which corresponds to an nBits value of `0x00ffff`:
```
share hash: f6b973257df982284715b0c7a20640dad709d22b0b1a58f2f88d35886ea5ac45
target: 7fffff0000000000000000000000000000000000000000000000000000000000
```
It's immediately clear that the share is invalid because the hash is above the target.
This type of logging is mostly done by the pool software. It's a nice extra convenience, but not very important. It impacts the following RPC calls:
1. `getmininginfo` displays the `target` for the tip block
2. `getblock` and `getblockheader` display the `target` for a specific block (ditto for their REST equivalents)
The `getdifficulty` method is a bit useless in its current state, because what miners really want to know if the difficulty for the _next_ block. So I added a boolean argument `next` to `getdifficulty`. (These values are typically the same, except for the first block in a retarget period. On testnet3 / testnet4 they change when no block is found after 20 minutes).
Similarly I added a `next` object to `getmininginfo` which shows `bit`, `difficulty` and `target` for the next block.
In order to test the difficulty transition, an alternate mainnet chain with 2016 blocks was generated and used in `mining_mainnet.py`. The chain is deterministic except for its timestamp and nonce values, which are stored in `mainnet_alt.json`.
As described at the top, this PR introduces a helper method `DeriveTarget()` which is split out from `CheckProofOfWorkImpl`. The proposed `checkblock` RPC in #31564 needs this helper method internally to figure out the consensus target.
Finally, this PR moves `pow.cpp` and `chain.cpp` from `bitcoin_node` to `bitcoin_common`, in order to give `rpc/util.cpp` (which lives in `bitcoin_common`) access to `pow.h`.
ACKs for top commit:
ismaelsadeeq:
re-ACK a4df12323c
tdb3:
code review re ACK a4df12323c
ryanofsky:
Code review ACK a4df12323c. Only overall changes since last review were dropping new `gettarget` method and dropping changes to `getdifficulty`, but there were also various internal changes splitting and rearranging commits.
Tree-SHA512: edef5633590379c4be007ac96fd1deda8a5b9562ca6ff19fe377cb552b5166f3890d158554c249ab8345977a06da5df07866c9f42ac43ee83dfe3830c61cd169
fa3c787b62 fuzz: Abort when global PRNG is used before SeedRand::ZEROS (MarcoFalke)
Pull request description:
This adds one more check to abort when global PRNG is used before SeedRand::ZEROS in fuzz tests. This is achieved by carving out the two remaining uses. First, `g_rng_temp_path_init`, and second the random fallback for `RANDOM_CTX_SEED`, which isn't used in fuzz tests anyway.
Requested in https://github.com/bitcoin/bitcoin/pull/31521#issuecomment-2554669015
Can be tested by reverting fadd568931 and observing an abort when running the `utxo_total_supply` fuzz target.
ACKs for top commit:
marcofleon:
ACK fa3c787b62
hodlinator:
re-ACK fa3c787b62
ryanofsky:
Code review ACK fa3c787b62. This adds a new check to make that sure that RNG is never seeded during fuzzing after the RNG has been used. Together with existing checks which ensure RNG can only be seeded with zeroes during fuzzing, and that RNG must was seeded at some point if used after fuzzing, this implies it must have been seeded by zeros before being used.
Tree-SHA512: 2614928d31c310309bd9021b3e5637b35f64196020fbf9409e978628799691d0efd3f4cf606be9a2db0ef60b010f890c2e70c910eaa2934a7fbf64cd1598fe22
223081ece6 scripted-diff: rename block and undo functions for consistency (Lőrinc)
baaa3b2846 refactor,blocks: remove costly asserts and modernize affected logs (Lőrinc)
fa39f27a0f refactor,blocks: deduplicate block's serialized size calculations (Lőrinc)
dfb2f9d004 refactor,blocks: inline `WriteBlockToDisk` (Lőrinc)
42bc491465 refactor,blocks: inline `UndoWriteToDisk` (Lőrinc)
86b85bb11f bench: add SaveBlockBench (Lőrinc)
34f9a0157a refactor,bench: rename bench/readblock.cpp to bench/readwriteblock.cpp (Lőrinc)
Pull request description:
`UndoWriteToDisk` and `WriteBlockToDisk` were delegating a subset of their functionality to single-use methods that didn't optimally capture a meaningful chunk of the algorithm, resulting in calculating things twice (serialized size, header size).
This change inlines the awkward methods (asserting that all previous behavior was retained), and in separate commits makes the usages less confusing.
Besides making the methods slightly more intuitive, the refactorings reduce duplicate calculations as well.
The speed difference is insignificant for now (~0.5% for the new `SaveBlockToDiskBench`), but are a cleanup for follow-ups such as https://github.com/bitcoin/bitcoin/pull/31539
ACKs for top commit:
ryanofsky:
Code review ACK 223081ece6. Since last review, "Save" was renamed to "Write", uint32_t references were dropped, some log statements and comments were improved as suggested, and a lot of tweaks made to commits and commit messages which should make this easier to review.
hodlinator:
ACK 223081ece6
TheCharlatan:
ACK 223081ece6
andrewtoth:
ACK 223081ece6
Tree-SHA512: 951bc8ad3504c510988afd95c561e3e259c6212bd14f6536fe56e8eb5bf5c35c32a368bbdb1d5aea1acc473d7e5bd9cdcde02008a148b05af1f955e413062d5c
fa83bec78e refactor: Allow std::byte in Read(LE/BE) (MarcoFalke)
Pull request description:
Starting with C++17, `std::byte` is often (not always) a better choice over `uint8_t` for new code.
However, the existing codebase discourages the use of `std::byte`, when helpers such as `ReadLE32` are used. This is because calling code will be cluttered with byte-casts.
Fix it by allowing `std::byte` pointers in `ReadLE32` (and friends).
ACKs for top commit:
sipa:
utACK fa83bec78e
fjahr:
Code review ACK fa83bec78e
theuni:
utACK fa83bec78e
l0rinc:
ACK fa83bec78e
Tree-SHA512: 83604dc9df9ad447ad1b6f81f1e1844554c2c5331fcb78bdba1300e050d9dcbe9cf7a1b2dd250772bb23a8bf02a4ec26441012fe2f4bcc670ef31c15151adb15
This is a documentation-only change following up on suggestions made in the
#30526 review.
Motivation for this change is that I was recently reviewing #31583, which
reminded me how confusing the arithmetic blob code was and made me want to
write better comments.
5709718b83 coins: warn on shutdown for big UTXO set flushes (Lőrinc)
Pull request description:
Split out of https://github.com/bitcoin/bitcoin/pull/30611#issuecomment-2549027130
Setting a large `-dbcache` size postpones the index writes until the coins cache size exceeds the specified limit. This causes the final flush after manual termination to seemingly hang forever (e.g. tens of minutes for 20 GiB); Now that the `dbcache` upper cap has been lifted, this will become even more apparent, so a warning will be shown when large UTXO sets are flushed (currently >1 GiB), such as:
> 2024-12-18T18:25:03Z Flushed fee estimates to fee_estimates.dat.
> 2024-12-18T18:25:03Z [warning] Flushing large (1 GiB) UTXO set to disk, it may take several minutes
> 2024-12-18T18:25:09Z Shutdown: done
---
You can reproduce it by starting `bitcoind` with a large `-dbcache`:
> mkdir demo && cmake -B build -DCMAKE_BUILD_TYPE=Release && cmake --build build -j$(nproc) && build/src/bitcoind -datadir=demo **-dbcache=10000**
Waiting until the used memory is over 1 GiB
> 2024-12-18T18:25:02Z UpdateTip: [...] progress=0.069009 cache=**1181.1MiB**(8827981txo)
And cancelling the process from the terminal:
> ^C2024-12-18T18:25:03Z tor: Thread interrupt
> [...]
> 2024-12-18T18:25:03Z **[warning] Flushing large (1 GiB) UTXO set to disk, it may take several minutes*
ACKs for top commit:
sipa:
utACK 5709718b83
tdb3:
re ACK 5709718b83
1440000bytes:
ACK https://github.com/bitcoin/bitcoin/pull/31534/commits/5709718b830161b7c2ba0db545ef0cfa98423597
danielabrozzoni:
tACK 5709718b83
Tree-SHA512: 608cf797de788501ccb2986508c155f5660c5f6f7a414524bfcc2820cfa9ebe3da558d13f2317d1f121a82d49ffe1e711a1152c743c22dab9f9807363f4ed8d5
06443b8f28 net: clarify if we ever sent or received from peer (Sjors Provoost)
1d01ad4d73 net: add LogIP() helper, use in net_processing (Sjors Provoost)
937ef9eb40 net_processing: use CNode::DisconnectMsg helper (Sjors Provoost)
ad224429f8 net: additional disconnection logging (Sjors Provoost)
Pull request description:
While debugging unexpected disconnections, possibly related to #28331, I found some additional [net] logging to be useful.
All cases where we disconnect now come with a log message that has the word `disconnecting`:
* all calls to `CloseSocketDisconnect()` log `disconnecting peer=…`
* wherever we set `pnode->fDisconnect = true;`
* for all `InactivityCheck` cases (which in turn sets `fDisconnect`)
* replaces "dropping" with "disconnecting" in `Network not active, dropping peer=…`
A few exceptions are listed here: https://github.com/bitcoin/bitcoin/pull/28521#discussion_r1890824361
I changed `CloseSocketDisconnect()` to no longer log `disconnecting`, and instead have all the call sites do so.
This PR introduces two helper functions on `CNode`: `DisconnectMsg` and `LogIP`. The second and third commit use these helpers in `net_processing.cpp` so these disconnect messages are more consistent now (e.g. some didn't log the IP). No new messages are added there though.
The `LogIP()` helper is rarely used outside of a disconnect event, but it's available for future use.
Any `LogPrint` this PR touches is replaced with `LogDebug` (superseded by #30750), and every `LogPrintf ` with `LogInfo`.
ACKs for top commit:
davidgumberg:
reACK https://github.com/bitcoin/bitcoin/commit/06443b8f28bcec4315cec262eb03343dee5465a6
vasild:
ACK 06443b8f28
danielabrozzoni:
ACK 06443b8f28
hodlinator:
ACK 06443b8f28
Tree-SHA512: 525f4c11568616e1d48455a3fcab9e923da7432377fe9230468c15403d2e9b7ce712112df8fbd547cfec01dce0d1f26107cfc1b90f78cfc1fe13e08d57b08464
fadd568931 fuzz: Fix misplaced SeedRand::ZEROS (MarcoFalke)
Pull request description:
After commit fae63bf130 this must be placed even before test_setup. This is nice, because it makes the usage consistently appear in the first line.
The change is moving a `SeedRandomForTest(SeedRand::ZEROS)` to happen earlier. This is fine, because it will either have no effect, or make the code more deterministic, because after commit fae63bf, no other re-seeding other than `ZEROS` can happen in fuzz tests.
ACKs for top commit:
marcofleon:
Re ACK fadd568931
brunoerg:
code review ACK fadd568931
hodlinator:
ACK fadd568931
Tree-SHA512: 54eadf19a1e850157a280fb252ece8797f37a9a50d3b0a01aa2c267bacbe8ef4ddea6cf3faadcbaa4ab9f53148edf08e3cee5dfb3eae928db582adf8373a5206
81cea5d4ee Ensure m_tip_block is never ZERO (Sjors Provoost)
e058544d0e Make m_tip_block an std::optional (Sjors Provoost)
Pull request description:
Suggested in https://github.com/bitcoin/bitcoin/pull/31297#discussion_r1844244309
ACKs for top commit:
fjahr:
re-ACK 81cea5d4ee
tdb3:
code review re ACK 81cea5d4ee
l0rinc:
ACK 81cea5d4ee
Tree-SHA512: 31a75ba29e3d567bab32e4e7925a419d9d7a4d2d85ed1c1012116d8d22adc14d31d5b4ce5f6c499c994188dcd26a01cced05be74f94c892fc90ae17a6783a472
c991cea1a0 Remove processNewBlock() from mining interface (Sjors Provoost)
9a47852d88 Remove getTransactionsUpdated() from mining interface (Sjors Provoost)
bfc4e029d4 Remove testBlockValidity() from mining interface (Sjors Provoost)
Pull request description:
There are three methods in the mining interface that can be dropped. The Template Provider doesn't need them and other application should probably not use them either.
1. `processNewBlock()` was added in 7b4d3249ce, but became unnecessary with the introduction of interfaces::BlockTemplate::submitSolution in 7b4d3249ce.
Dropping it was suggested in https://github.com/bitcoin/bitcoin/pull/30200#issuecomment-2404460342
2. `getTransactionsUpdated()`: this is used in the implementation of #31003 `waitFeesChanged`. It's not very useful generically because the mempool updates very frequently.
3. `testBlockValidity()`: it might be useful for mining application to have a way to check the validity of a block template they modified, but the Stratum v2 Template Provider doesn't do that, and this method is a bit brittle (e.g. the block needs to build on the tip).
ACKs for top commit:
TheCharlatan:
Re-ACK c991cea1a0
ryanofsky:
Code review ACK c991cea1a0. Since last review, just rebased to avoid conflicts in surrounding code, and edited a commit message
tdb3:
code review ACK c991cea1a0
Tree-SHA512: 2138e54f920b26e01c068b24498c6a210c5c4358138dce0702ab58185d9ae148a18f04c97ac9f043646d40f8031618d80a718a176b1ce4779c237de6fb9c4a67
facb4d010c refactor: Move GuessVerificationProgress into ChainstateManager (MarcoFalke)
Pull request description:
Currently the function is standalone, which means any passed-in data like `TxData` or the block pointer needs to be taken from the `ChainstateManager` and passed in. This is currently verbose and may become even more verbose if the function is reworked in the future. As the function can not be called without a `ChainstateManager` in production code anyway, make it a member function on the class.
ACKs for top commit:
ryanofsky:
Code review ACK facb4d010c. Nice cleanup, that should make this code less awkward to work with
TheCharlatan:
ACK facb4d010c
danielabrozzoni:
reACK facb4d010c
Tree-SHA512: b17977e12cd7c6e308c47e6a1aa920acecd4442696e46d1f30bd7c201e9898ca2d581ff0bf2cc9f7334e146c1b0c50925adb849c8c17f65dcdf6877be1c5f776
fa9e0489f5 refactor: Use immediate lambda to work around GCC bug 117966 (MarcoFalke)
Pull request description:
Currently the libstdc++ debug mode can only be used with version 11, or 15 (and later), due to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=117966
This seems restrictive.
Add a temporary workaround for now, which makes the global (temporary) `std::span` local to a lambda.
ACKs for top commit:
theuni:
utACK fa9e0489f5
hebasto:
ACK fa9e0489f5, tested on Ubuntu 24.10.
vasild:
ACK fa9e0489f5
Tree-SHA512: 0cc54f089f329592f7a92a6f938b7de46c92d5362615310748225a42789e858e871432721e3101271b00871d523af5fbaadba2f52433fe79e928b1d1253931f6
fae63bf130 fuzz: Clarify that only SeedRandomStateForTest(SeedRand::ZEROS) is allowed (MarcoFalke)
fa18acb457 fuzz: Abort when using global PRNG without re-seed (MarcoFalke)
fa7809aeab fuzz: Add missing SeedRandomStateForTest(SeedRand::ZEROS) (MarcoFalke)
Pull request description:
This is the first step toward improving fuzz stability and determinism (https://github.com/bitcoin/bitcoin/issues/29018).
A fuzz target using the global test-only PRNG will now abort if the seed is re-used across fuzz inputs.
Also, temporarily add `SeedRandomStateForTest(SeedRand::ZEROS)` to all affected fuzz targets. This may slow down the libfuzzer leak detector, but it will disable itself after some time, or it can be disabled explicitly with `-detect_leaks=0`.
In a follow-up, each affected fuzz target can be stripped of the global random use and a local `RandomMixin` (or similar) can be added instead.
(Can be tested by removing any one of the re-seed calls and observing a fuzz abort)
ACKs for top commit:
hodlinator:
ACK fae63bf130
dergoegge:
utACK fae63bf130
marcofleon:
Tested ACK fae63bf130
Tree-SHA512: 4a0db69af7f715408edf4f8b08b44f34ce12ee2c79d33b336ad19a6e6bd079c4ff7c971af0a3efa428213407c1171f4e2837ec6a2577086c2f94cd15618a0892
f86678156a Check leaves size maximum in MerkleComputation (Sjors Provoost)
4d57288246 refactor: use CTransactionRef in submitSolution (Sjors Provoost)
2e81791d90 Drop TransactionMerklePath default position arg (Sjors Provoost)
39d3b538e6 Rename merkle branch to path (Sjors Provoost)
Pull request description:
This PR implements the refactors suggested in https://github.com/bitcoin/bitcoin/pull/30955#pullrequestreview-2354931253.
ACKs for top commit:
tdb3:
code review re-ACK f86678156a
itornaza:
re ACK f86678156a
ryanofsky:
Code review ACK f86678156a only changes since last review are a whitespace change and adding an Assume statement to check for size_t -> uint32_t overflows
Tree-SHA512: 661b5d5d0e24b2269bf33ab1484e37c36e67b32a7796d77ca3b1856d3043378b081ad43c32a8638b46fa8c0de51c823fd9747dd9fc81f958f20d327bf330a47c
52fd1511a7 test: drop scriptPubKeyIn arg from CreateNewBlock (Sjors Provoost)
ff41b9e296 Drop script_pub_key arg from createNewBlock (Sjors Provoost)
7ab733ede4 rpc: rename coinbase_script to coinbase_output_script (Sjors Provoost)
Pull request description:
Providing a script for the coinbase transaction is only done in test code and for (unoptimized) CPU solo mining.
Production miners use the `getblocktemplate` RPC which omits the coinbase transaction entirely from its block template, leaving it to external (pool) software to construct it.
This commit removes the `script_pub_key argument` from `createNewBlock()` in the Mining interface.
A coinbase script can still be passed via `BlockCreateOptions` instead. Tests are modified to do so.
ACKs for top commit:
ryanofsky:
Code review ACK 52fd1511a7. No change since last review other than rebase
TheCharlatan:
Re-ACK 52fd1511a7
vasild:
ACK 52fd1511a7
Tree-SHA512: c4b3a53774d9a5dc90950e77f47a64dbb68f971baffbb9a0d8f59332ef8e52d0c039130c925bde73135b3d0e79e65d91d1df30dc4cff13f32d8a72e5c56669d8
c93bf0e6e2 test: Add missing %c character test (Hodlinator)
76cca4aa6f test: Document non-parity between tinyformat and ConstevalFormatstring (Hodlinator)
533013cba2 test: Prove+document ConstevalFormatString/tinyformat parity (Hodlinator)
b81a465995 refactor test: Profit from using namespace + using detail function (Hodlinator)
Pull request description:
Clarifies and puts the extent of parity under test.
Broken out from #30546 based on https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1755013263 and https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756495304.
ACKs for top commit:
maflcko:
re-ACK c93bf0e6e2 🗜
l0rinc:
ACK c93bf0e6e2
ryanofsky:
Code review ACK c93bf0e6e2. Just a few cleanups tweaking function declarations and commit comments and consolidating some test cases since last review.
Tree-SHA512: 5ecc893b26cf2761c0009861be392ec4c4fceb0ef95052a2f6f9df76b2e459cfb3f9e257f61be07c3bb2ecc6e525e72c5ca853be1f63b70b52785323d3db6b42
55347a5018 test: Rework migratewallet to use previous release (v28.0) (Ava Chow)
f42ec0f3bf wallet: Check specified wallet exists before migration (Ava Chow)
Pull request description:
This PR reworks wallet_migration.py to use previous releases to produce legacy wallets for testing so that the test will continue to work once legacy wallets are removed.
Split from #28710
ACKs for top commit:
maflcko:
re-ACK 55347a5018🥊
rkrux:
re-ACK 55347a5
Tree-SHA512: f90a2f475febc73d29e8ad3cb20d134c368a40a3b5934c3e4aaa77ae704af6314d4dd2e85c261142bd60a201902ac4ba00b8e2443d3cef7c8cc45d23281fa831
This makes code more consistent and makes it easier to add compile-time checking to
enforce that format strings contain the right specifiers, because it stops
using Untranslated() to create the format string, so the Untranslated()
function will not need to get involved in formatting.
-BEGIN VERIFY SCRIPT-
quote='"[^"]+"'
quotes="(?:$quote|\\s)*"
nonparens="[^()]*"
single_level_paren="\($nonparens\)"
double_level_paren="\($nonparens\($nonparens\)$nonparens\)"
exprs="(?:$double_level_paren|$single_level_paren|$nonparens)*"
git grep -l 'Untranslated' | xargs perl -0777 -i -pe "s/strprintf\((\\W*)Untranslated\(($quotes)\)($exprs)(\))/Untranslated(\1strprintf(\2\3))/gs"
-END VERIFY SCRIPT-
50cce20013 test, refactor: Compact ccoins_access and ccoins_spend (Lőrinc)
0a159f0914 test, refactor: Remove remaining unbounded flags from coins_tests (Lőrinc)
c0b4b2c1ee test: Validate error messages on fail (Lőrinc)
d5f8d607ab test: Group values and states in tests into CoinEntry wrappers (Lőrinc)
ca74aa7490 test, refactor: Migrate GetCoinsMapEntry to return MaybeCoin (Lőrinc)
15aaa81c38 coins, refactor: Remove direct GetFlags access (Lőrinc)
6b733699cf coins, refactor: Assume state after SetClean in AddFlags to prevent dangling pointers (Lőrinc)
fc8c282022 coins, refactor: Make AddFlags, SetDirty, SetFresh static (Lőrinc)
cd0498eabc coins, refactor: Split up AddFlags to remove invalid states (Lőrinc)
Pull request description:
Similarly to https://github.com/bitcoin/bitcoin/pull/30849, this cleanup is intended to de-risk https://github.com/bitcoin/bitcoin/pull/30673#discussion_r1739909068 by simplifying the coin cache public interface.
`CCoinsCacheEntry` provided general access to its internal flags state, even though, in reality, it could only be `clean`, `fresh`, `dirty`, or `fresh|dirty` (in the follow-up, we will remove `fresh` without `dirty`).
Once it was marked as `dirty`, we couldn’t set the state back to clean with `AddFlags(0)`—tests explicitly checked against that.
This PR refines the public interface to make this distinction clearer and to make invalid behavior impossible, rather than just checked by tests. We don't need extensive access to the internals of `CCoinsCacheEntry`, as many tests were simply validating invalid combinations in this way.
The last few commits contain significant test refactorings to make `coins_tests` easier to change in follow-ups.
ACKs for top commit:
andrewtoth:
Code Review ACK 50cce20013
laanwj:
Code review ACK 50cce20013
ryanofsky:
Code review ACK 50cce20013. Looks good! Thanks for the followups.
Tree-SHA512: c0d65f1c7680b4bb9cd368422b218f2473c2ec75a32c7350a6e11e8a1601c81d3c0ae651b9f1dae08400fb4e5d43431d9e4ccca305a718183f9a936fe47c1a6c
This change manually removes two strprintf(Untranslated...) calls. All
remaining calls are removed in the next scripted-diff commit.
Removing these calls makes code more consistent and makes it easier to
implement compile-time checking enforcing that format strings contain valid
specifiers, by avoiding the need for the Untranslated() function to be involved
in formatting.
Additionally, using + and += instead of strprintf here makes code a little
shorter, and more type-safe because + unlike strprintf only works on strings of
the same type, making it less likely english strings and bilingual strings will
be unintentionally combined.
This could produce an english error message containing non-english string
fragments if PopulateAndValidateSnapshot started returning any translated
strings in the future. This change is also needed to make the next
scripted-diff commit work.
Instead just concatenate already formatted strings. This allows untranslated
format strings to be checked at compile time now, and translated format strings
to be checked at compile time in #31061.
fa3e074304 refactor: Tidy fixups (MarcoFalke)
fa72646f2b move-only: Detail_CheckNumFormatSpecifiers and G_TRANSLATION_FUN (MarcoFalke)
faff8403f0 refactor: Pick translated string after format (MarcoFalke)
Pull request description:
The changes are required for https://github.com/bitcoin/bitcoin/pull/31061, however they also make sense on their own. For example, they are fixing up an `inline namespace`, which lead to compile errors otherwise (can be tested by observing the compile error after reverting the changes to `src/util/strencodings.h`). Also, a unit test comment is fixed.
ACKs for top commit:
ryanofsky:
Code review ACK fa3e074304. Nice changes! These should allow related PRs to be simpler.
l0rinc:
ACK fa3e074304
hodlinator:
cr-ACK fa3e074304
Tree-SHA512: 37371181a348610442186b5fbb7a6032d0caf70aae566002ad60be329a3131a2b89f28f6c51e10872079f987986925dc8c0611bde639057bee4f572d2b9ba92a
fa5e706459 ci: Skip broken Wine64 tests by default (MarcoFalke)
Pull request description:
I don't think the unit tests run in Wine after the Windows cross-compilation have ever shown a true positive since the MSVC task was added. However, they are a source of frequent false-positives.
Thus, disable them by default for now. Anyone can still enable them by setting `RUN_UNIT_TESTS=true`.
A follow-up could run them on real Windows, see https://github.com/bitcoin/bitcoin/pull/31176.
Conceptually there are many other nightly tasks, which rarely find issues and are not run by default, like the valgrind or s390x tasks. So putting the Wine unit tests in the same bucket should be fine.
ACKs for top commit:
hebasto:
ACK fa5e706459, to avoid false-positives.
willcl-ark:
ACK fa5e706459
Tree-SHA512: 6bd54470e4d5ce18923c5d724aba0dbf475d053d7097d3f87e822a455cc537b6ce5f0dfcc8ccd0719c12c5d0c8fc7355a6c84185a6c9b5d484d98aee763d0c49
409d0d6293 test: enable running individual independent functional test methods (ismaelsadeeq)
Pull request description:
- Some test methods in the functional test framework are independent and do not require any prior context or setup in `run_test`.
- This commit adds a new option for running these specific methods within a test file, allowing them to be executed individually without running the entire test suite.
- Using this option reduces the time you need to wait before the test you are interested in starts executing.
- The functionality added by this PR can be achieved manually by commenting out code, but having a pragmatic option to do this is more convenient.
Note: Running test methods that require arguments or context will fail.
**Example Usage**:
```zsh
build/test/functional/feature_reindex.py --test_methods continue_reindex_after_shutdown
```
```zsh
build/test/functional/feature_config_args.py --test_methods test_log_buffer test_args_log test_connect_with_seednode
```
ACKs for top commit:
maflcko:
review ACK 409d0d6293
rkrux:
reACK 409d0d6293
ryanofsky:
Code review ACK 409d0d6293. This seems like a good step towards making it easy to run independent tests quickly. I think ideally there would be some naming convention or @ annotation added to test methods that can run independently, so the test framework could provide more functionality like being able to list test methods, being able to show command lines to quickly reproduce problems when tests fails, and calling test methods automatically instead of requiring individual tests to call them. But these ideas are all compatible with the new `--test_methods` option
Tree-SHA512: b0daac7c3b322e6fd9b946962335d8279e8cb004ff76f502c8d597b9c4b0073840945be198a79d44c5aaa64bda421429829d5c84ceeb8c6139eb6ed079a35878
9aa50152c1 Add destroy to BlockTemplate schema (Sjors Provoost)
Pull request description:
This ensures that if a client no longer needs a block template, the node can clear its memory as soon as possible.
A block template may hold on to transactions that are no longer in the mempool, so this can be significant.
This has a trivial silent merge conflict with #31283 because it also used the number `@9` (gaps are not allowed). I'll rebase whichever is merged last.
ACKs for top commit:
TheCharlatan:
Re-ACK 9aa50152c1
ryanofsky:
Code review ACK 9aa50152c1
Tree-SHA512: 393571b4455969cba71c7572feaeff4503738205331ab198b9181c156c75eb65933a8e5ceff66fc06d1efb8f2528bdb254e5eee7df75735b912526de1e7b166d
fa1177e3d7 refactor: Avoid std::string format strings (MarcoFalke)
Pull request description:
This changes some unchecked `std::string` format strings to use string literals, which are `consteval` checked at compile-time.
Split out, because it is used in several pull requests.
ACKs for top commit:
l0rinc:
ACK fa1177e3d7
tdb3:
code review and light test ACK fa1177e3d7
rkrux:
tACK fa1177e3d7
ryanofsky:
Code review ACK fa1177e3d7
Tree-SHA512: 7cc70a49b07dadc386336687b463043e79e94a46d18db0184c9813218536e87e954a1afeb8739d5d8706e7b2f355d3f7984048c7de2725851b463985f1c5369f
fafbf8acf4 Make G_FUZZING constexpr, require -DBUILD_FOR_FUZZING=ON to execute a fuzz target (MarcoFalke)
fae3cf0ffa ci: Temporarily disable macOS/Windows fuzz step (MarcoFalke)
Pull request description:
`g_fuzzing` is used inside `Assume` at runtime, causing significant overhead in hot paths. See https://github.com/bitcoin/bitcoin/issues/31178
One could simply remove the `g_fuzzing` check from the `Assume`, but this would make fuzzing a bit less useful. Also, it would be unclear if `g_fuzzing` adds a runtime overhead in other code paths today or in the future.
Fix all issues by making `G_FUZZING` equal to the build option `BUILD_FOR_FUZZING`, and for consistency in fuzzing, require it to be set when executing any fuzz target.
Fixes https://github.com/bitcoin/bitcoin/issues/31178
Temporarily this drops fuzzing from two CI tasks, but they can be re-added in a follow-up with something like https://github.com/bitcoin/bitcoin/pull/31073
ACKs for top commit:
marcofleon:
Tested ACK fafbf8acf4
davidgumberg:
I still ACK https://github.com/bitcoin/bitcoin/pull/31191/commits/fafbf8acf419d5e2ca307e5804099361ca7471af for fixing the regression measured in #31178.
ryanofsky:
Code review ACK fafbf8acf4 but approach -0, because this approach means libraries built for fuzz testing do not function correctly if used in a release, and libraries built for releases are mostly useless for fuzz testing. So I would like to at least consider other solutions to this problem even if we go with this one.
dergoegge:
utACK fafbf8acf4
Tree-SHA512: 124fc2e8b35e0c4df414436556a7a0a36cd1bec4b3000b40dcf2ab8c85f32e0610bf7f70d2fd79223d62f3c3665b6c09da21241654c7b9859461b8ca340d5421
This is needed in the next commit to add compile-time checking to strprintf
calls, because bitcoin-cli.cpp uses dynamic width in many format strings.
This change is easiest to review ignoring whitespace.
Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
Co-authored-by: l0rinc <pap.lorinc@gmail.com>
fa22e5c430 refactor: Remove dead code that assumed tip == nullptr (MarcoFalke)
fa2e443965 refactor: Replace g_genesis_wait_cv with m_tip_block_cv (MarcoFalke)
fa7f52af1a refactor: Use wait_for predicate to check for interrupt (MarcoFalke)
5ca28ef28b refactor: Split up NodeContext shutdown_signal and shutdown_request (Ryan Ofsky)
fad8e7fba7 bugfix: Mark m_tip_block_cv as guarded by m_tip_block_mutex (MarcoFalke)
fa18586c29 refactor: Add missing GUARDED_BY(m_tip_block_mutex) (MarcoFalke)
fa4c075033 doc: Clarify waitTipChanged docs (MarcoFalke)
Pull request description:
`g_genesis_wait_cv` is similar to `m_tip_block_cv` but shuffling everything through a redundant `boost::signals2`.
So remove it, along with some other dead code, as well as minor fixups.
ACKs for top commit:
ryanofsky:
Code review ACK fa22e5c430 (just rebased since last review)
Sjors:
ACK fa22e5c430
TheCharlatan:
ACK fa22e5c430
Tree-SHA512: a2cb59b651aaf85a3574723adfe403487566788ad945933b0458816ccc841fce08ca77b31afbd2d6adb5bf1deed7229c028bee74fb4bbaf6576e9edcfa0ad817
Previously the check-deps.sh would write information about unexpected
dependencies to stderr, but return exit code 0, so the error would be ignored
by CI. Now it will return code 1 and cause CI to fail if unexpected
dependencies are detected.
Instead of having a single NodeContext::shutdown member that is used both to
request shutdowns and check if they have been requested, use separate members
for each. Benefits of this change:
1. Should make code a little clearer and easier to search because it is easier
to see which parts of code are triggering shutdowns and which parts are just
checking to see if they were triggered.
2. Makes it possible for init.cpp to specify additional code to run when a
shutdown is requested, like signalling the m_tip_block_cv condition variable.
Motivation for this change was to remove hacky NodeContext argument and
m_tip_block_cv access from the StopRPC function, so StopRPC can just be
concerned with RPC functionality, not other node functionality.
fa5bc450d5 util: Use compile-time check for LogConnectFailure (MarcoFalke)
fa7087b896 util: Use compile-time check for FatalErrorf (MarcoFalke)
faa62c0112 util: Add ConstevalFormatString (MarcoFalke)
fae7b83eb5 lint: Remove forbidden functions from lint-format-strings.py (MarcoFalke)
Pull request description:
The `test/lint/lint-format-strings.py` was designed to count the number of format specifiers and assert that they are equal to the number of parameters passed to the format function. The goal seems reasonable, but the implementation has many problems:
* It is written in Python, meaning that C++ code can not be parsed correctly. Currently it relies on brittle regex and string parsing.
* Apart from the parsing errors, there are also many logic errors. For example, `count_format_specifiers` allows a mix of positional specifiers and non-positional specifiers, which can lead to runtime format bugs. Also, `count_format_specifiers` silently skipped over "special" format specifiers, which are valid in tinyformat, which again can lead to runtime format bugs being undetected.
* The brittle logic has a history of breaking in pull requests that are otherwise fine. This causes the CI to fail and the pull request being blocked from progress until the bug in the linter is fixed, or the code is rewritten to work around the bug.
* It is only run in the CI, or when the developer invokes the script. It would be better if the developer got the error message at compile-time, directly when writing the code.
Fix all issues by using a `consteval` checked format string in `FatalErrorf` and `LogConnectFailure`.
This is the first step toward https://github.com/bitcoin/bitcoin/issues/30530 and a follow-up will apply the approach to the other places.
ACKs for top commit:
stickies-v:
re-ACK fa5bc450d5
l0rinc:
ACK fa5bc450d5
hodlinator:
ACK fa5bc450d5
ryanofsky:
Code review ACK fa5bc450d5
Tree-SHA512: d6189096b16083143687ed1b1559cf4f92f97dd87bc5d00673e44f4fb9fce7bb7b215cfdfc39b6e6a24f0b75a79a03ededce966639e554f7172e1fc22cf015ae
1eac96a503 Compare FromUserHex result against other hex validators and parsers (Lőrinc)
19947863e1 Use BOOST_CHECK_EQUAL for optional, arith_uint256, uint256, uint160 (Lőrinc)
743ac30e34 Add std::optional support to Boost's equality check (Lőrinc)
Pull request description:
Enhanced `FromUserHex` coverage by:
* Added `std::optional` support to `BOOST_CHECK_EQUAL`, allowing direct comparisons of `std::optional<T>` with other `T` expected values.
* Increased fuzz testing for hex parsing to validate against other hex validators and parsers.
----
* Use BOOST_CHECK_EQUAL for https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1706637780 arith_uint256, uint256, uint160
Example error before:
> unknown location:0: fatal error: in "validation_chainstatemanager_tests/chainstatemanager_args": std::bad_optional_access: bad_optional_access
test/validation_chainstatemanager_tests.cpp:781: last checkpoint
after:
> test/validation_chainstatemanager_tests.cpp:801: error: in "validation_chainstatemanager_tests/chainstatemanager_args": check set_opts({"-assumevalid=0"}).assumed_valid_block == uint256::ZERO has failed [std::nullopt != 0000000000000000000000000000000000000000000000000000000000000000]
ACKs for top commit:
stickies-v:
re-ACK 1eac96a503
ryanofsky:
Code review ACK 1eac96a503. Only changes since last review were auto type and fuzz test tweaks.
hodlinator:
ACK 1eac96a503
Tree-SHA512: f1d2c65f0ee4e97830700be5b330189207b11ed0c89a8cebf0f97d43308402a6b3732e10130c79a0c044f7d2eeabfb5359990825aadf02c4ec19428dcd982b00
43cd83b0c7 test: move uint256_tests/operator_with_self to arith_uint256_tests (stickies-v)
c6c994cb2b test: remove test-only uint160S (stickies-v)
62cc4656e2 test: remove test-only uint256S (stickies-v)
adc00ad728 test: remove test-only arith_uint256S (stickies-v)
f51b237723 refactor: rpc: use uint256::FromHex for ParseHashV (stickies-v)
Pull request description:
_Continuation of #30569._
Since https://github.com/bitcoin/bitcoin/commit/fad2991ba073de0bd1f12e42bf0fbaca4a265508, `uint256S()` has been [deprecated](https://github.com/bitcoin/bitcoin/pull/30482/commits/fad2991ba073de0bd1f12e42bf0fbaca4a265508#diff-800776e2dda39116e889839f69409571a5d397de048a141da7e4003bc099e3e2R138) because it is less robust than the `base_blob::FromHex()` introduced in https://github.com/bitcoin/bitcoin/pull/30482. Specifically, it tries to recover from length-mismatches, recover from untrimmed whitespace, 0x-prefix and garbage at the end, instead of simply requiring exactly 64 hex-only characters. (see also https://github.com/bitcoin/bitcoin/pull/30532)
This PR removes `uint256S()` (and `uint160S()`) completely, with no non-test behaviour change.
Specifically, the main changes in this PR are:
- the (minimal) last non-test usage of `uint256S()` in `ParseHashV()` is removed without behaviour change, which can partially be verified by cherry-picking and/or modifying [this test commit](https://github.com/stickies-v/bitcoin/commit/1f2b0fa86db2ed65476b68417aa1bf4c26026a19)).
- the test usage of `uint{160,256}S()` is removed, largely replacing it with `uint{160,256}::FromHex()` where applicable, potentially modifying the test by removing non-hex characters or dropping the test entirely if removing non-hex characters makes it redundant
- the now unused `uint{160,256}S()` functions are removed completely.
- unit test coverage on converting `uint256` <-> `arith_uint256` through `UintToArith256()` and `ArithToUint256()` is beefed up, and `arith_uint256` tests are moved to `arith_uint256_tests.cpp`, removing the `uint256_tests.cpp` dependency on `uint256h`, mirroring how the code is structured.
_Note: `uint256::FromUserHex()` exists to more leniently construct uint256 from user input, allowing "0x" prefixes and too-short-input, as safer alternative to `uint256S()` where necessary._
ACKs for top commit:
l0rinc:
reACK 43cd83b0c7
hodlinator:
re-ACK 43cd83b0c7
ryanofsky:
Code review ACK 43cd83b0c7. Only code change is a small refactoring which looks good. The rest of the PR is all test changes, which I only lightly reviewed, but seem to be positive and do what's described
Tree-SHA512: 48147a4c6af671597df0f72c1b477ae4631cd2cae4645ec54d0e327611ff302c9899e344518c81242cdde82930f6ad23a3a7e6e0b80671816e9f457b9de90a5c
d4c7c4009d init: error out if -maxconnections is negative (Sergi Delgado Segura)
c773649481 init: improves file descriptors accounting and docs (Sergi Delgado Segura)
29008a7ff4 init: fixes fd accounting regarding poll/select (Sergi Delgado Segura)
Pull request description:
The current logic for file descriptor accounting is pretty convoluted and hard to follow. This is partially caused by the lack of documentation plus non-intuitive variable naming (which was more intuitive when fewer things were accounted for, but
hasn't aged well). This has led to this accounting being error-prone and hard to maintain (as shown in the first commit of this PR).
Redefine some of the constants, plus document what are we accounting for so this can be extended more easily
Fixes#18911
ACKs for top commit:
sr-gi:
> ACK [d4c7c40](https://github.com/bitcoin/bitcoin/commit/d4c7c4009da14c84576d43ab4a1241967cfa5ffc)
naumenkogs:
ACK d4c7c4009d
vasild:
ACK d4c7c4009d
TheCharlatan:
ACK d4c7c4009d
Tree-SHA512: 1524d10c8ad8f354f6ab9c244699adbcdae2dd7aba37de5b8f9e177c629e8a2cce0f6e8117e076dde3a592f5283bd30a4201db96a3c011e335c02d1fde7414bc
Script was failing when called on libraries that do not import symbols, because
bash pipefail option was specified, and grep was used in some pipelines to
filter symbols, and grep returns status 1 when it doesn't match any lines. This
could cause the script to fail on some systems and configurations, such as the
clang-tidy CI configuration
https://cirrus-ci.com/task/4801670352207872?logs=ci#L6191 where the
libbitcoin_crypto_x86_shani.a library does not import symbols.
Fix check-deps.sh to check for weak symbols so it can detect when an exported
template function is used from another library.
In a previous version of this commit, this change caused an invalid dependency
in the consensus library on the TryParseHex template function from the util
library to be detected, and a suppression was added here. But #30377 removed
the invalid dependency so the suppression is no longer needed.
The invalid dependency and problem detecting weak symbol usage was originally
reported by Hennadii Stepanov in
https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-2209258843
8756ccd712 scripted-diff: Replace ParseHex[<std::byte>]("str") -> "str"_hex[_u8] (Hodlinator)
9cb687351f refactor: Prepare for ParseHex -> ""_hex scripted-diff (Hodlinator)
50bc017040 refactor: Hand-replace some ParseHex -> ""_hex (Hodlinator)
5b74a849cf util: Add consteval ""_hex[_v][_u8] literals (l0rinc)
dc5f6f6812 test refactor: util_tests - parse_hex clean up (Hodlinator)
2b5e6eff36 refactor: Make XOnlyPubKey tolerate constexpr std::arrays (Hodlinator)
403d86f1cc refactor: vector -> span in CCrypter (Hodlinator)
bd0830bbd4 refactor: de-Hungarianize CCrypter (Hodlinator)
d99c816971 refactor: Improve CCrypter related lines (Hodlinator)
7e1d9a8468 refactor: Enforce lowercase hex digits for consteval uint256 (Hodlinator)
Pull request description:
Motivation:
* Validates and converts the hex string into bytes at compile time instead of at runtime like `ParseHex()`.
* Eliminates runtime dependencies: https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2214432177, https://github.com/bitcoin/bitcoin/pull/30048#discussion_r1592108480
* Has stricter requirements than `ParseHex()` (disallows whitespace and uppercase hex digits) and replaces it in a bunch of places.
* Makes it possible to derive other compile time constants.
* Minor: should shave off a few runtime CPU cycles.
`""_hex` produces `std::array<std::byte>` as the momentum in the codebase is to use `std::byte` over `uint8_t`.
Also makes `uint256` hex string constructor disallow uppercase hex digits. Discussed: https://github.com/bitcoin/bitcoin/pull/30560#discussion_r1701323070
Surprisingly does not change the size of the Guix **bitcoind** binary (on x86_64-linux-gnu) by 1 single byte.
Spawned already merged PRs: #30436, #30482, #30532, #30560.
ACKs for top commit:
l0rinc:
ACK 8756ccd712
stickies-v:
Rebase re-ACK 8756ccd712, no changes since a096215c9c71a2ec03e76f1fd0bcdda0727996e0
ryanofsky:
Code review ACK 8756ccd712, just rebasing since last review and taking advantage of CScript constructors in #29369, also tweaking a code comment
Tree-SHA512: 9b2011b7c37e0ef004c669f8601270a214b388916316458370f5902c79c2856790b1b2c7c123efa65decad04886ab5eff95644301e0d84358bb265cf1f8ec195
ec973dd197 refactor: remove un-tested early returns (josibake)
72a5822d43 tests: add tests for KeyPair (josibake)
cebb08b121 refactor: move SignSchnorr to KeyPair (josibake)
c39fd39ba8 crypto: add KeyPair wrapper class (josibake)
5d507a0091 tests: add key tweak smoke test (josibake)
f14900b6e4 bench: add benchmark for signing with a taptweak (josibake)
Pull request description:
Broken out from #28201
---
The wallet returns an untweaked internal key for taproot outputs. If the output commits to a tree of scripts, this key needs to be tweaked with the merkle root. Even if the output does not commit to a tree of scripts, BIP341/342 recommend commiting to a hash of the public key.
Previously, this logic for applying the taptweak was implemented in the `CKey::SignSchnorr` method.
This PR moves introduces a KeyPair class which wraps a `secp256k1_keypair` type and refactors SignSchnorr to use this new KeyPair. The KeyPair class is created with an optional merkle_root argument and the logic from BIP341 is applied depending on the state of the merkle_root argument.
The motivation for this refactor is to be able to use the tap tweak logic outside of signing, e.g. in silent payments when retrieving the private key (see #28201).
Outside of silent payments, since we almost always convert a `CKey` to a `secp256k1_keypair` when doing anything with taproot keys, it seems generally useful to have a way to model this type in our code base.
ACKs for top commit:
paplorinc:
ACK ec973dd197 - will happily reack if you decide to apply @ismaelsadeeq's suggestions
ismaelsadeeq:
Code review ACK ec973dd197
itornaza:
trACK ec973dd197
theStack:
Code-review ACK ec973dd197
Tree-SHA512: 34947e3eac39bd959807fa21b6045191fc80113bd650f6f08606e4bcd89aa17d6afd48dd034f6741ac4ff304b104fa8c1c1898e297467edcf262d5f97425da7b
fa18fc7050 log: Remove NOLINT(bitcoin-unterminated-logprintf) (MarcoFalke)
Pull request description:
`NOLINT(bitcoin-unterminated-logprintf)` is used to document a missing trailing `\n` char in the format string. This has many issues:
* It is just documentation, assuming that a trailing `\n` ends up in the formatted string. It is not enforced at compile-time, so it is brittle.
* If the newline was truly missing and `NOLINT(bitcoin-unterminated-logprintf)` were used to document a "continued" line, the log stream would be racy/corrupt, because any other thread may inject a log message in the meantime.
* If the newline was accidentally missing, nothing is there to correct the mistake.
* The intention of all code is to always end a log line with a new line. However, historic code exists to deal with the case where the new line was missing (`m_started_new_line`). This is problematic, because the presumed dead code has to be maintained (https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1682963306).
Fix almost all issues by removing the `NOLINT(bitcoin-unterminated-logprintf)`, ensuring that a new line is always present.
A follow-up will remove the dead logging code.
ACKs for top commit:
TheCharlatan:
ACK fa18fc7050
ryanofsky:
Code review ACK fa18fc7050
Tree-SHA512: bf8a83723cca84e21187658edc19612da79c34f7ef2e1f6e9353e7ba70e4ecc0a878a2ae32290045fb90cba9a44451e35341a36ef2ec1169d13592393aa4a8ca
e9de0a76b9 doc: release note for 30212 (willcl-ark)
87b1880525 rpc: clarify ALREADY_IN_CHAIN rpc errors (willcl-ark)
Pull request description:
Closes: #19363
Renaming this error improves clarity around the returned error both internally and externally when a transactions' outputs are already found in the utxo set (`TransactionError::ALREADY_IN_CHAIN -> TransactionError::ALREADY_IN_UTXO_SET`)
ACKs for top commit:
tdb3:
ACK e9de0a76b9
ismaelsadeeq:
ACK e9de0a76b9
ryanofsky:
Code review ACK e9de0a76b9.
Tree-SHA512: 7d2617200909790340951fe56a241448f9ce511900777cb2a712e8b9c0778a27d1f912b460f82335844224f1abb4322bc898ca076440959edade55c082a09237
Ryan Ofsky