f6bdbcf79d lint: Grep for `AUTO` test suites in file names (rustaceanrob)
Pull request description:
Tests without a fixture did not have their file names linted because the grep matches on `BOOST_FIXTURE`. Updates to match `BOOST_FIXTURE` or `BOOST_TEST`.
ACKs for top commit:
l0rinc:
ACK f6bdbcf79d
achow101:
ACK f6bdbcf79d
hebasto:
ACK f6bdbcf79d.
Tree-SHA512: dd1763b6ac90fa87b7e0d2faa56d1c7beedb1e2d37d16367c60ebcadd155f5955113fff7cf5c0ce5eaa9e63aeeb67ffff2c8e081f7c23978cb072207f072f2ef
3f44f9aef7 test: Add coverage for m_blocks_unlinked invariant in LoadBlockIndex (marcofleon)
0e4b0bacec validation: Don't add pruned blocks to m_blocks_unlinked on startup (marcofleon)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/issues/35050
The `m_blocks_unlinked` map keeps track of blocks that have transactions but whose parent (or any ancestor) does not. This happens when a block is received before its parent, or during a reorg, when `FindMostWorkChain()` encounters a block whose ancestors were pruned.
The bug this PR addresses is a rare interaction of these two cases, which happens on startup when `BlockManager::LoadBlockIndex()` rebuilds `m_blocks_unlinked`. The check there only considers whether a block has transactions, and pruned blocks keep `nTx > 0` but clear `BLOCK_HAVE_DATA`. So if there's a pruned block on a stale fork whose parent has no transactions, that block is added to `m_blocks_unlinked` without having data on disk. This violates an [assertion](ad3f73862b/src/validation.cpp (L5352)) in `CheckBlockIndex()`.
Get rid of this unintended case by gating on `BLOCK_HAVE_DATA` before adding to `m_blocks_unlinked`.
ACKs for top commit:
achow101:
ACK 3f44f9aef7
sedited:
Re-ACK 3f44f9aef7
stratospher:
ACK 3f44f9a. nice!
Tree-SHA512: 275d0f8588524c01c4e701c8635973cd4a086d31c10d252a498c1ef668bdb3895ba1cae265dbe88f8983ca7ddbe32247824753c7c1f49e59c8bce0df377b784c
fa2afba28b p2p: Release m_peer_mutex early in InitiateTxBroadcastToAll (MarcoFalke)
Pull request description:
The `InitiateTxBroadcastToAll` method holds the `m_peer_mutex` while updating the bloom filters for all peers. This is perfectly fine, because updating the bloom filters is fast. Though, from a style-perspective, the lock does not need to be held for the whole function. Also, holding the lock longer, may confuse Tsan into a lock-order inversion false-positive (ref: https://github.com/bitcoin/bitcoin/issues/19303#issuecomment-1514926359).
So "fix" both issues in this style-refactor.
ACKs for top commit:
xyzconstant:
Code review ACK fa2afba28b
shuv-amp:
ACK fa2afba28b
danielabrozzoni:
Code Review ACK fa2afba28b
sedited:
ACK fa2afba28b
Tree-SHA512: c47849a4c3cc11c74b61fec3425db8ec7f78db4ca43d7bf3145ce640f7b0872701c09495f0dfe77109d09d5716d920ad3d7308483fe41564c30867b3e80432e7
5b65e31270 test: remove two unnecessary nodes from the test (rkrux)
Pull request description:
A discussion in the review of #35443 PR brought this test to my attention.
The test needs multiple wallets that can be created on a single node, multiple nodes are not required.
As there is a cost associated with setting-up and tearing-down nodes, this patch helps in reducing the test time as well.
ACKs for top commit:
ekzyis:
ACK 5b65e31270
polespinasa:
lgtm ACK 5b65e31270
sedited:
ACK 5b65e31270
Tree-SHA512: f6b4a96b9beee968ef5438fd9db582a48834ff36ba27c19dd7012902528fa713424212530e34cc16b58c19c023f1accd2b89fe846ef2cc36677c24e160c5b817
Tests without a fixture did not have their file names linted because the
grep matches on `BOOST_FIXTURE`. Updates to match `BOOST_FIXTURE` or
`BOOST_TEST`.
Co-authored-by: l0rinc <pap.lorinc@gmail.com>
BIP434 defines FEATURE messages which are sent between VERSION and VERACK
to indicate support for new P2P protocol features. This commit provides
the infrastructure for easily using BIP434 negotiation when implementing
such new P2P protocol features. Note that advertised protocol version
is bumped to 70017, as per BIP434's specification.
The test needs multiple wallets that can be created on a single node, multiple
nodes are not required.
As there is a cost associated with setting-up and tearing-down nodes, this patch
helps in reducing the test time as well.
bf0d257c11 net: un-default the OpenNetworkConnection()'s proxy_override argument (Eugene Siegel)
5a3756d150 test: add a regression test for private broadcast v1 retries (Vasil Dimov)
ab35a028ed test: make reusable filling of a node's addrman (Vasil Dimov)
2333be9cbc test: make reusable starting a standalone P2P listener (Vasil Dimov)
2ffa81fac4 test: make reusable SOCKS5 server starting (Vasil Dimov)
32d072a49f doc: add release notes for #35319 (Vasil Dimov)
d01b461f71 net: ensure no direct private broadcast connections (Vasil Dimov)
fd230f942d net: use the proxy if overriden when doing v2->v1 reconnections (Vasil Dimov)
Pull request description:
This PR includes https://github.com/bitcoin/bitcoin/pull/35319 and on top of that adds a regression functional test.
The functional test exercises the relevant code paths without modifying non-test code. To do that it does:
* Add a bunch of IPv4 addresses to the node's addrman (they will be added without P2P_V2 flag).
* Get them to report P2P_V2 in their service flags and connect to each one, so that the flags
in addrman are updated to contain P2P_V2.
* Get one successful connection to a Tor peer (.onion) so that bitcoind assumes the configured
Tor proxy works and is indeed a proxy to the Tor network. This will make it open private
broadcast connections also to IPv4 addresses via that proxy.
* Start some private broadcast connections.
* Remember the destination IPv4 address of the first connection and get it to fail the v2
transport.
* Wait for a subsequent connection also through the Tor proxy to the same IPv4 and expect
it to be v1, i.e. the v2->v1 downgrade retry.
The test fails without the fix - the v1 retry never arrives to the Tor proxy. And passes with the fix. The fix is in the first commit here and in https://github.com/bitcoin/bitcoin/pull/35319, can remove it by `git show fd230f942d | git apply -R`.
ACKs for top commit:
Crypt-iQ:
reACK bf0d257c11
andrewtoth:
ACK bf0d257c11
instagibbs:
ACK bf0d257c11
sedited:
utACK bf0d257c11
Tree-SHA512: 11e89be36577199e0312e5e63efeac04e295faaba1cf1c13a30e683d35f473c8dbb419d1897b0333c2e993c10637adecafcf90fe08c812065c793cbc903744c9
7735c13488 test: run bitcoin-cli -ipcconnect check under valgrind with -datadir (Michael Dietz)
Pull request description:
This case invokes bitcoin-cli via raw subprocess.run() without -datadir, so it reads the default datadir's bitcoin.conf (e.g. ~/.bitcoin) and fails whenever that real config is unusable. Pass the node's -datadir so the check reads the test's own bitcoin.conf and depends only on the build's IPC support, not the host environment.
ACKs for top commit:
maflcko:
lgtm ACK 7735c13488
sedited:
ACK 7735c13488
Tree-SHA512: 5264433e2cd1747b9b9b4437b80f1849b5fa01620dd958c34c124925270dfca85c3809186285bf1dd70a8358552e1ebb589ec84093e11bb0d66cfe10f04dde81
107d4178d9 versionbits: update VersionBitsCache doc comment to match current behaviour (Antoine Poinsot)
94e3ac0b21 doc: release notes and bips doc update for #34779 (Antoine Poinsot)
1d5240574a qa: test we don't warn for ignored unknown version bits deployments (Antoine Poinsot)
f802edf57c versionbits: Limit live activation params and activation warnings per BIP323 (Anthony Towns)
Pull request description:
This implements https://github.com/bitcoin/bips/pull/2116, which repurposes 24 version bits as extra nonce space for miners rather than soft fork deployment coordination. 24 bits allows a miner to perform up to 72 PH before needing a fresh job from its controller. The current 16 bits in use by miners only allow up to 280 TH, which [apparently led some ASIC designers to start rolling the timestamp field](https://github.com/bitaxeorg/ESP-Miner/pull/1553#issuecomment-3937736319) on their beefier machines.
Mailing list discussion available [here](https://gnusha.org/pi/bitcoindev/6fa0cb45-37d6-4b41-9ff8-03730fd96d6e@mattcorallo.com/). A previous shot at this is https://github.com/bitcoin/bitcoin/pull/13972 (with a smaller extranonce space).
This change only affects the warning logic.
ACKs for top commit:
ajtowns:
ACK 107d4178d9
achow101:
ACK 107d4178d9
sedited:
Re-ACK 107d4178d9
optout21:
ACK 107d4178d9
Tree-SHA512: cfaf5d7de1e8c020a4d7f4b1096b6c3e0e3b41ea840a4652ebcdabc345c5c557161c8304f1d7d6de541a2bf1df3c855ad7b64e49dd8c8af3937876d134bb5aba
2ef6679c2c test: Check that MuSig2 signing does not reuse nonces (Ava Chow)
bb05986c0a musig: Include pubnonce in session id (Ava Chow)
Pull request description:
It is safe to have multiple musig signing sessions over the same message so long as the nonces used are different. Including the pubnonce in the session id allows for multiple simultaneous signing sessions over the same message, rather than asserting when the user tries to do this.
The second commit tests this behavior, both ensuring that there is no crash, and verifying that both sessions produce unique nonces and signatures to verify that no reuse is occurring.
Lastly, the assertion in `SetMuSig2SecNonce` is retained as hitting it now would indicate that a nonce has been reused. We prefer to assert and crash rather than do something that is highly likely to leak a private key.
Fixes#35250
ACKs for top commit:
rkrux:
lgtm ACK 2ef6679c2c
junbyjun1238:
utACK 2ef6679c2c
theStack:
ACK 2ef6679c2c
Tree-SHA512: 9fb60b68ebe0ea9656408afb65b9ec9f280632e1bb84a4821b074c8d8569847845f7c29da800c757b9ddf3aa31aa890dd9e3646cf119917a714e7daf20be2198
This case invoked bitcoin-cli via raw subprocess.run() without the
valgrind wrapper (bypassing valgrind) and without -datadir (so it read
the default datadir's bitcoin.conf, e.g. ~/.bitcoin, and failed whenever
that real config was unusable). Build the command like the rest of the
framework: prepend binaries.valgrind_cmd and pass the node's -datadir so
the check runs under valgrind and depends only on the build's IPC
support, not the host environment.
c8b8c275fa test: Improve loopback address check in `rcp_bind.py` (xyzconstant)
Pull request description:
A [loopback address](https://www.geeksforgeeks.org/computer-networks/what-is-a-loopback-address/) can range from `127.0.0.0` to `127.255.255.255`. This commit relaxes the loopback check in `rpc_bind.py` by checking whether an IP address (from `all_interfaces()`) starts with `'127.'` instead of strictly matching `'127.0.0.1'`.
Programs like VPNs might add an extra loopback address (e.g., 127.1.130.83), which failed under the previous state. These addresses will now pass with this update.
---
**For context:** I found this while running tests with the Mullvad daemon active. Mullvad adds a custom lo0 interface like `inet 127.141.11.239 netmask 0xff000000` that failed with `--nonloopback`, which should not be the case since the address is a valid loopback IP.
ACKs for top commit:
maflcko:
lgtm ACK c8b8c275fa
willcl-ark:
ACK c8b8c275fa
Tree-SHA512: 3b82002d6bc90cfc4023dd0274a40970abb2dc6a9ced77dd97e275b31340bb657d5222bb55a768ebf71047ac1521dd4ba77fb427398f7cc9857738bcd16c5818
9c1fcaca5c wallet, test: fix sendall anti-fee-sniping when locktime is not specified (rkrux)
8877eec726 wallet: allow anti-fee-sniping in sendall RPC while not relying on RBF default (rkrux)
Pull request description:
Partially fixes https://github.com/bitcoin/bitcoin/issues/32661.
Prerequisite to #35405.
The test change in the second commit would fail without the presence
of the first commit.
ACKs for top commit:
maflcko:
review ACK 9c1fcaca5c🍅
xyzconstant:
Tested ACK 9c1fcaca5c
polespinasa:
code review ACK 9c1fcaca5c
sedited:
ACK 9c1fcaca5c
Tree-SHA512: 048c1a6c64c104f8c27053e4dea139fb8740f30096f4d85daf23aa53a9ad21f0120151a7d1d6ea19b12d174976999335b3f9ac863800629774d8030b7f34918f
A loopback address can range from `127.0.0.0` to `127.255.255.255`.
This commit relaxes the loopback check in `rpc_bind.py` by checking whether
an IP address (from `all_interfaces()`) starts with `'127.'` instead of
strictly matching `'127.0.0.1'`.
Programs like VPNs might add an extra loopback address (e.g., 127.1.130.83),
which failed under the previous state. These addresses will now pass with this update.
fad585b6e5 test: Wait for node exit after crash in verify_utxo_hash (MarcoFalke)
faf1475514 ci: Exclude feature_dbcrash.py under --v2transport --usecli (MarcoFalke)
fac27d702f test: Fix feature_dbcrash.py --usecli intermittent error (MarcoFalke)
fa09de8b68 test: [refactor] Simplify submit_block_catch_error (MarcoFalke)
Pull request description:
This fixes a small intermittent issue that snuck in via commit fa8d4d5c35.
Generally, it seems tedious and brittle trying to enumerate all possible exception types on all platforms and all test configs, all possible errno values, etc.
So just treat any `Exception` as crash, and confirm it in the test. The test would still fail, if a crash did not happen after an Exception, but the failure may be minimally more tedious to debug. I think this is fine, because failures should be rare and having simpler and more flexible test code is preferable.
----
Also, disable the test for now in CI, because it is quite slow.
ACKs for top commit:
willcl-ark:
ACK fad585b6e5
Tree-SHA512: 6ff69a53908b22904780f29def473f8e26c5b608d89420ac76768d06ea3e3394b5d3f4d488100f9d47f943ff5778a555302ccdaebc11fda7c009205b6a6ca05c
ac09260982 test: restore JSONRPCException error format (rkrux)
Pull request description:
This is a follow-up to PR #34575.
ACKs for top commit:
maflcko:
lgtm ACK ac09260982
Tree-SHA512: 15979f4e2c07993f283640ebfe570e9f8d3842a23a8118042f5b618273e0da8a01bcabe1ec90b6cc49ebf28e9819d1b4f077ac18f62f681a4d4f58ad8e11bdb1
This particular test case only needs to ensure that locktime is not
specified in the RPC request, it doesn't need to rely on the wallet optin
RBF default that causes the test to pass coincidentally.
Co-authored-by: maflcko <6399679+maflcko@users.noreply.github.com>
This is a follow-up to PR 34575.
Copy is done so that checking of error["code"] in test_node.py
while handling this exception doesn't fail.
Co-authored-by: maflcko <6399679+maflcko@users.noreply.github.com>
Extract the part of `p2p_private_broadcast.py` that starts
listening on a `P2PConnection` object (or its children classes)
and put it into `test_framework/p2p.py`.
Extract the part of `p2p_private_broadcast.py` that configures and
starts the SOCKS5 server into a reusable function and put it into
`test_framework/socks5.py`.
Use bind port 0 to let the OS pick an available port instead of
hackishly assuming that `p2p_port(N)` is available where N is more
than the number of the nodes the test uses.
f701cd159a doc: fix typo in release notes of #34917 (rkrux)
7bc39e3d08 wallet, test: add wallet_deprecated_rbf.py for walletrbf deprecated keys & options (rkrux)
2cbbcb5659 wallet, test: remove -deprecatedrpc=bip125 from wallet_send.py (rkrux)
307134bd7e wallet, test: remove -deprecatedrpc=bip125 from wallet_migration.py (rkrux)
3ec550d168 wallet, test: remove -deprecatedrpc=bip125 from wallet_basic.py (rkrux)
a52ea9bff9 wallet, test: remove -walletrbf startup option from wallet_backwards_compatibility.py (rkrux)
42330922dd wallet, test: remove -walletrbf startup option from wallet_backwards_compatibility.py (rkrux)
8cb6e405d8 wallet, test: remove -walletrbf startup option from wallet_listtransactions.py (rkrux)
0ee94b2fef wallet, test: remove -deprecatedrpc=bip125 from wallet_listtransactions.py (rkrux)
5e833e068d wallet, test: -walletrbf startup option from wallet_bumpfee.py (rkrux)
a2a2b1745f wallet, test: remove -walletrbf startup option from rpc_psbt.py (rkrux)
a3fe455a95 wallet: refactor to read -walletrbf only once instead of twice (rkrux)
Pull request description:
Prerequisite to #35404 and #35405.
All these changes address the points raised in the review of PR #34917
here: https://github.com/bitcoin/bitcoin/pull/34917#pullrequestreview-4362148900.
Essentially updating the existing wallet functional tests without using
the -deprecatedrpc=bip125 and -walletrbf startup options. Instead,
these two are added and tested via a singular new
wallet_deprecated_rbf.py test that can be removed easily later when
these startup options are completely removed from the wallet post
deprecation.
ACKs for top commit:
maflcko:
review ACK f701cd159a🌄
achow101:
ACK f701cd159a
Tree-SHA512: 700785062b5de8ee3b6c4f50570b769d56c6c4960f2b6e2a2e71be8085c6b51eaeb34fb158fae76f812fe82791aaa0c0277f964f0472cb0784b86caabe6d4ec9
451fdd26a4 test: wallet: Constructing a DSPKM that can't TopUp() throws. (David Gumberg)
32946e0291 wallet: Setup new autogenerated descriptors on construction (Ava Chow)
e20aaff70f wallet: Construct ExternalSignerSPKM with the new descriptor (Ava Chow)
aa4f7823aa wallet: include keys when constructing DescriptorSPKM during import (Ava Chow)
6538f69135 fuzz: Skip adding descriptor to wallet if it cannot be expanded (Ava Chow)
8be5ee554b test: wallet: Check that loading wallet with both unencrypted and encrypted keys fails. (David Gumberg)
80b0c25992 wallet: Load everything into DescSPKM on construction (Ava Chow)
f713fd1725 refactor: wallet: Don't reuse WALLET_BLANK flag for born-encrypted wallets. (David Gumberg)
cd912c4e10 wallet: Consolidate generation setup callers into one function (Ava Chow)
0301c758ea wallet migration, fuzz: Migrate hd seed once (Ava Chow)
Pull request description:
Instead of constructing ScriptPubKeyMans with no data, and then loading data as we find it, we should gather everything first and then load it all on construction. If there actually is no data and we want to setup generation, then that should also occur in a constructor rather than afterwards.
This change is only applied to DescriptorScriptPubKeyMan and ExternalSignerScriptPubKeyMan, and should be done for any ScriptPubKeyMans added in the future. I don't think it's really worth it to do this for LegacyScriptPubKeyMan since it would make loading performance worse (or cause layer violations) and it's (supposed to be) going away soon.
ACKs for top commit:
polespinasa:
ACK 451fdd26a4
davidgumberg:
re crACK 451fdd26a4
w0xlt:
ACK 451fdd26a4
Tree-SHA512: 58a889bf7c77d5da78041907a76a1958207f95a19bec8dc4d86d4e4108d256a729e0949c0973f7d447178f78a7fd4268cda71d358cae4dec5a76dc453b5283af
b86c1c443d test: add coverage for migrating ancient wallets (furszy)
fd44d48b24 wallet: fix ancient wallets migration (furszy)
Pull request description:
We currently fail migration if the wallet does not contain the best block locator.
This is a problem for wallets created before https://github.com/bitcoin/bitcoin/pull/152, which are not storing such record.
Missing this record is not an error. it simply means the wallet will scan the chain prior
to finish migration.
ACKs for top commit:
achow101:
ACK b86c1c443d
w0xlt:
reACK b86c1c443d
sedited:
Re-ACK b86c1c443d
Tree-SHA512: 5226934e16d32f3337c432a84e1adce9985518e52c62abfa4a8d6b3d857d4b5c6aa99ac90e84ae6772983ceaf7a67e128ff7e0e174843fcb892728b9be4653cf
3962138cc0 test: add IPC submitBlock functional test (woltx)
5b60f69e40 mining: add submitBlock IPC method to Mining interface (woltx)
813b4a80d7 refactor: introduce SubmitBlock helper (w0xlt)
Pull request description:
This PR adds a `submitBlock` method to the IPC Mining interface, equivalent to the `submitblock` RPC. It accepts a serialized block over IPC, validates/processes it via the normal block-processing path.
The method uses the same result shape as `checkBlock`: `bool` + `reason/debug out-params`. It reports duplicate, inconclusive, and invalid-block rejection details, and initializes reason/debug on every call.
Closes#34626
ACKs for top commit:
Sjors:
ACK 3962138cc0
optout21:
reACK 3962138cc0
ryanofsky:
Code review ACK 3962138cc0. Just rebased since and made suggested changes since last review.
Tree-SHA512: 705cbb89972a80b6ff0ab75a78f686983d6077c97f1758795efe5b8968f01065ebef664ac850eae2bc86af8964efa2a68e8dfc677209c312856650f9387ed006
Catch any Exception in verify_utxo_hash and let restart_node verify the
crash via wait_for_node_exit.
(Also, use named args in restart_node, while touching this test)
Catching any Exception covers possible subprocess.CalledProcessError
that may happen in a --usecli run. E.g.
TestFramework (INFO): Verifying utxo hash matches for all nodes
TestFramework.bitcoincli (DEBUG): Running bitcoin-cli ['-datadir=/tmp/bitcoin_func_test_gzufs0ht/node0', '-rpcclienttimeout=240', '-rpcconnect=127.0.0.1', '-rpcport=20963', 'gettxoutsetinfo']
TestFramework.bitcoincli (DEBUG): Running bitcoin-cli ['-datadir=/tmp/bitcoin_func_test_gzufs0ht/node1', '-rpcclienttimeout=240', '-rpcconnect=127.0.0.1', '-rpcport=20964', 'gettxoutsetinfo']
TestFramework (ERROR): Called Process failed with stdout='error: timeout on transient error: Could not connect to the server 127.0.0.1:20964 (error code 1 - "EOF reached")
Make sure the bitcoind server is running and that you are connecting to the correct RPC port.
Use "bitcoin-cli -help" for more info.
'; stderr='None';
Traceback (most recent call last):
File "./test/functional/test_framework/test_framework.py", line 143, in main
self.run_test()
~~~~~~~~~~~~~^^
File "./test/functional/feature_dbcrash.py", line 273, in run_test
self.verify_utxo_hash()
~~~~~~~~~~~~~~~~~~~~~^^
File "./test/functional/feature_dbcrash.py", line 182, in verify_utxo_hash
nodei_utxo_hash = self.nodes[i].gettxoutsetinfo()['hash_serialized_3']
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^
File "./test/functional/test_framework/test_node.py", line 963, in __call__
return self.cli.send_cli(self.command, *args, **kwargs)
~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "./test/functional/test_framework/test_node.py", line 1043, in send_cli
raise subprocess.CalledProcessError(returncode, p_args, output=cli_stderr)
Make it catch any Exception and let the caller verify it.
This refactor does not change any behavior, but the code is simpler,
more flexible and still correct, because wait_for_node_exit enforces the
crash to happen.
Test the new Mining.submitBlock IPC method:
- Invalid block (bad version) returns failure with reason
- Valid block (with a real mempool tx) is accepted and propagates
- Duplicate block returns failure with "duplicate" reason
- Witness commitment without coinbase witness nonce is rejected
(bad-witness-nonce-size), confirming no auto-fix behavior
- submitBlock then submitSolution: duplicate is accepted (submitSolution
returns true for already-known blocks)
- submitSolution then submitBlock interaction (duplicate)
Build candidate blocks from BlockTemplate data in the existing coinbase and
submission test, then exercise checkBlock(), submitSolution(), and
submitBlock() against those candidates. submitBlock() uses an isolated IPC
node for cases that would otherwise affect the main submitSolution() and
checkBlock() assertions.
