This change:
1. Collects build artifacts in dedicated locations.
2. Allows running bitcoin-chainstate.exe with bitcoinkernel.dll directly
from the build tree on Windows.
e4dd5a351b test: wallet, abandon coinbase txs and their descendants during startup (furszy)
474139aa9b wallet: abandon inactive coinbase tx and their descendants during startup (furszy)
Pull request description:
Since #26499, we mark coinbase transactions and their descendants as abandoned when a reorg arises through the "block disconnection" signal handler. However, this does not cover all scenarios; external wallets could contain coinbase transactions from blocks the node has not seen yet, or the user could have replaced the chain with an earlier or different version (one without the coinbase chain).
This affects balance calculation as well as mempool rebroadcast (descendants shouldn't be relayed).
Fix this by marking orphaned coinbase transactions and their descendants as abandoned during wallet startup.
ACKs for top commit:
achow101:
ACK e4dd5a351b
rkrux:
tACK e4dd5a351b
mzumsande:
Code Review ACK e4dd5a351b
Tree-SHA512: 461a43de7a6f5a580f2e6e3b56ec9bc92239cd45e850a2ff594ab5488dcd4a507f68fbbf550a33d7173b2add0de80de1e1b3841e1dfab0c95b284212d8ced08a
bb633c9407 tests: add functional test for miniscript decaying multisig (Michael Dietz)
Pull request description:
This is very closely based on [test/functional/wallet_multisig_descriptor_psbt.py](https://github.com/bitcoin/bitcoin/blob/master/test/functional/wallet_multisig_descriptor_psbt.py) both in code and concept. It should serve as some integration testing for Miniscript descriptors, and also documents a simple multisig that starts as 4-of-4 and decays to 3-of-4, 2-of-4, and finally 1-of-4 at block heights (I think in the real world aligning this to halvenings would be nice).
ACKs for top commit:
achow101:
ACK bb633c9407
rkrux:
reACK bb633c9407
hodlinator:
ACK bb633c9407
Tree-SHA512: 1f8e8e50258d45d8f2b882b5f86dcd390d86c543ff4801f397733017102e0854ac387960b6e296bb164603545615d224a4b400247cbbc07bf21b2f4b718ab2ff
4080b66cbe test: add test for utxo-to-sqlite conversion script (Sebastian Falbesoner)
ec99ed7380 contrib: add tool to convert compact-serialized UTXO set to SQLite database (Sebastian Falbesoner)
Pull request description:
## Problem description
There is demand from users to get the UTXO set in form of a SQLite database (#24628). Bitcoin Core currently only supports dumping the UTXO set in a binary _compact-serialized_ format, which was crafted specifically for AssumeUTXO snapshots (see PR #16899), with the primary goal of being as compact as possible. Previous PRs tried to extend the `dumptxoutset` RPC with new formats, either in human-readable form (e.g. #18689, #24202), or most recently, directly as SQLite database (#24952). Both are not optimal: due to the huge size of the ever-growing UTXO set with already more than 80 million entries on mainnet, human-readable formats are practically useless, and very likely one of the first steps would be to put them in some form of database anyway. Directly adding SQLite3 dumping support on the other hand introduces an additional dependency to the non-wallet part of bitcoind and the risk of increased maintenance burden (see e.g. https://github.com/bitcoin/bitcoin/pull/24952#issuecomment-1163551060, https://github.com/bitcoin/bitcoin/issues/24628#issuecomment-1108469715).
## Proposed solution
This PR follows the "external tooling" route by adding a simple Python script for achieving the same goal in a two-step process (first create compact-serialized UTXO set via `dumptxoutset`, then convert it to SQLite via the new script). Executive summary:
- single file, no extra dependencies (sqlite3 is included in Python's standard library [1])
- ~150 LOC, mostly deserialization/decompression routines ported from the Core codebase and (probably the most difficult part) a little elliptic curve / finite field math to decompress pubkeys (essentialy solving the secp256k1 curve equation y^2 = x^3 + 7 for y given x, respecting the proper polarity as indicated by the compression tag)
- creates a database with only one table `utxos` with the following schema:
```(txid TEXT, vout INT, value INT, coinbase INT, height INT, scriptpubkey TEXT)```
- the resulting file has roughly 2x the size of the compact-serialized UTXO set (this is mostly due to encoding txids and scriptpubkeys as hex-strings rather than bytes)
[1] note that there are some rare cases of operating systems like FreeBSD though, where the sqlite3 module has to installed explicitly (see #26819)
A functional test is also added that creates UTXO set entries with various output script types (standard and also non-standard, for e.g. large scripts) and verifies that the UTXO sets of both formats match by comparing corresponding MuHashes. One MuHash is supplied by the bitcoind instance via `gettxoutsetinfo muhash`, the other is calculated in the test by reading back the created SQLite database entries and hashing them with the test framework's `MuHash3072` module.
## Manual test instructions
I'd suggest to do manual tests also by comparing MuHashes. For that, I've written a go tool some time ago which would calculate the MuHash of a sqlite database in the created format (I've tried to do a similar tool in Python, but it's painfully slow).
```
$ [run bitcoind instance with -coinstatsindex]
$ ./src/bitcoin-cli dumptxoutset ~/utxos.dat
$ ./src/bitcoin-cli gettxoutsetinfo muhash <block height returned in previous call>
(outputs MuHash calculated from node)
$ ./contrib/utxo-tools/utxo_to_sqlite.py ~/utxos.dat ~/utxos.sqlite
$ git clone https://github.com/theStack/utxo_dump_tools
$ cd utxo_dump_tools/calc_utxo_hash
$ go run calc_utxo_hash.go ~/utxos.sqlite
(outputs MuHash calculated from the SQLite UTXO set)
=> verify that both MuHashes are equal
```
For a demonstration what can be done with the resulting database, see https://github.com/bitcoin/bitcoin/pull/24952#pullrequestreview-956290477 for some example queries. Thanks go to LarryRuane who gave me to the idea of rewriting this script in Python and adding it to `contrib`.
ACKs for top commit:
ajtowns:
ACK 4080b66cbe - light review
achow101:
ACK 4080b66cbe
romanz:
tACK 4080b66cbe on signet (using [calc_utxo_hash](8981aa3e85/calc_utxo_hash/calc_utxo_hash.go)):
tdb3:
ACK 4080b66cbe
Tree-SHA512: be8aa0369a28c8421a3ccdf1402e106563dd07c082269707311ca584d1c4c8c7b97d48c4fcd344696a36e7ab8cdb64a1d0ef9a192a15cff6d470baf21e46ee7b
a85e8c0e61 doc: Add some general documentation about negated options (Ryan Ofsky)
490c8fa178 doc: Add release notes summarizing negated option behavior changes. (Ryan Ofsky)
458ef0a11b refactor: Avoid using IsArgSet() on -connect list option (Ryan Ofsky)
752ab9c3c6 test: Add test to make sure -noconnect disables -dnsseed and -listen by default (Ryan Ofsky)
3c2920ec98 refactor: Avoid using IsArgSet() on -signetseednode and -signetchallenge list options (Ryan Ofsky)
d05668922a refactor: Avoid using IsArgSet() on -debug, -loglevel, and -vbparams list options (Ryan Ofsky)
3d1e8ca53a Normalize inconsistent -noexternalip behavior (Ryan Ofsky)
ecd590d4c1 Normalize inconsistent -noonlynet behavior (Ryan Ofsky)
5544a19f86 Fix nonsensical bitcoin-cli -norpcwallet behavior (Ryan Ofsky)
6e8e7f433f Fix nonsensical -noasmap behavior (Ryan Ofsky)
b6ab350806 Fix nonsensical -notest behavior (Ryan Ofsky)
6768389917 Fix nonsensical -norpcwhitelist behavior (Ryan Ofsky)
e03409c70f Fix nonsensical -norpcbind and -norpcallowip behavior (Ryan Ofsky)
40c4899bc2 Fix nonsensical -nobind and -nowhitebind behavior (Ryan Ofsky)
5453e66fd9 Fix nonsensical -noseednode behavior (Ryan Ofsky)
Pull request description:
The PR changes behavior of negated `-noseednode`, `-nobind`, `-nowhitebind`, `-norpcbind`, `-norpcallowip`, `-norpcwhitelist`, `-notest`, `-noasmap`, `-norpcwallet`, `-noonlynet`, and `-noexternalip` options, so negating these options just clears previously specified values doesn't have other side effects.
Negating options on the command line can be a useful way of resetting options that may have been set earlier in the command line or config file. But before this change, negating these options wouldn't fully reset them, and would have confusing and undocumented side effects (see commit descriptions for details). Now, negating these options just resets them and behaves the same as not specifying them.
Motivation for this PR is to fix confusing behaviors and also to remove incorrect usages of the `IsArgSet()` function. Using `IsArgSet()` tends to lead to negated option bugs in general, but it especially causes bugs when used with list settings returned by `GetArgs()`, because when these settings are negated, `IsArgSet()` will return true but `GetArgs()` will return an empty list. This PR eliminates all uses of `IsArgSet()` and `GetArgs()` together, and followup PR #17783 makes it an error to use `IsArgSet()` on list settings, since calling `IsArgSet()` is never actually necessary. Most of the changes here were originally made in #17783 and then moved here to be easier to review and avoid a dependency on #16545.
ACKs for top commit:
achow101:
ACK a85e8c0e61
danielabrozzoni:
re-ACK a85e8c0e61
hodlinator:
re-ACK a85e8c0e61
Tree-SHA512: dd4b19faac923aeaa647b1c241d929609ce8242b43e3b7bc32523cc48ec92a83ac0dc5aee79f1eba8794535e0314b96cb151fd04ac973671a1ebb9b52dd16697
fa8e0956c2 rpc: Remove deprecated dummy alias for listtransactions::label (MarcoFalke)
Pull request description:
The RPC arg is not a dummy, but a label, so offering an undocumented alias is inconsistent with all other label interfaces and confusing at best, if not entirely unused.
Fix it by removing the deprecated alias.
This pull is a breaking change, but it should be limited, because it only affects someone using the deprecated named arg on this RPC. I can't imagine anyone doing this, because in all other places where label args are accepted, they are called `label`. If someone really didn't use `label` here as named arg, it would be trivial and less confusing for them to fix it up.
ACKs for top commit:
achow101:
ACK fa8e0956c2
rkrux:
tACK fa8e0956c2
ryanofsky:
Code review ACK fa8e0956c2
Tree-SHA512: 0d0f3f53237ff9fac8c065b7d0a4245f5ff86efa427dbeeca711765494b7315a9d72b44751d346c76422847daf3d7ff90dbccb5ba200b089fb96128bd95da9f0
f0e5e4cdbe test: Add test for rpcwhitelistdefault (naiyoma)
Pull request description:
This PR adds tests for `rpcwhitelistdefault.` The implementation is a continuation of this [PR](https://github.com/bitcoin/bitcoin/pull/17805).
Applied suggestions to include the tests in` rpc_whitelist.py` and to use a single node.
PR covers three test cases:
- rpcwhitelistdefault = 0, no permissions
- rpcwhitelistdefault = 1, no permissions
- rpcwhitelistdefault = 1, with user permissions
I didn't add tests for rpcwhitelistdefault = 0 with user permissions since that is already tested here: [rpc_whitelist.py#L77](https://github.com/bitcoin/bitcoin/blob/master/test/functional/rpc_whitelist.py#L77).
ACKs for top commit:
achow101:
ACK f0e5e4cdbe
ryanofsky:
Code review ACK f0e5e4cdbe. PR seems very clear and simple, moving 1 test and adding 3 new tests.
ismaelsadeeq:
Tested and Code review ACK f0e5e4cdbe
Tree-SHA512: c3652940d2f23746e769ebe834e43dee47b7af8f258cbb133e38663aa8a05a1a8d0194d3008c3a10b0c54d11b5b95420c9cad0aa761c0fc1b9559277443b0696
0a02e7fdea test: deduplicates p2p_tx_download constants (Sergi Delgado Segura)
Pull request description:
Some of the networking constants defined in p2p_tx_download.py are more generally defined in p2p.py
ACKs for top commit:
i-am-yuvi:
re-ACK 0a02e7fdea
maflcko:
review ACK 0a02e7fdea🔖
danielabrozzoni:
re-ACK 0a02e7fdea
tdb3:
re ACK 0a02e7fdea
Tree-SHA512: 05fc114a32b6b42a7c57563a38f1a8921e0bb224c4b124ae9d395c3a1105ae6e9cdfc62f603f4f2dee55cef5f6a6ed400d328740ad84fbd3093c5e0f3fb2982a
bb0879ddab test: check `scanning` field from `getwalletinfo` (brunoerg)
Pull request description:
During a rescan, check that `getwalletinfo` returns properly information (the scanning field) about it.
ACKs for top commit:
maflcko:
lgtm ACK bb0879ddab
arejula27:
ACK [`bb0879d`](bb0879ddab)
achow101:
ACK bb0879ddab
BrandonOdiwuor:
Code Review ACK bb0879ddab
Prabhat1308:
re-ACK [`bb0879d`](bb0879ddab)
Tree-SHA512: 9bca1c1e813bf4f61a5621bdc0a5f5c2bcfb388ffe9dfacb821bf6954f6e0880140d72258dc93ab6b84efb54f55c682a17aebd42f6559d6cfac9998e6bc4e5b9
af76664b12 test: Test migration of a solvable script with no privkeys (Ava Chow)
17f01b0795 test: Test migration of taproot output scripts (Ava Chow)
1eb9a2a39f test: Test migration of miniscript in legacy wallets (Ava Chow)
e8c3efc7d8 wallet migration: Determine Solvables with CanProvide (Ava Chow)
fa1b7cd6e2 migration: Skip descriptors which do not parse (Ava Chow)
440ea1ab63 legacy spkm: use IsMine() to extract watched output scripts (Ava Chow)
b777e84cd7 legacy spkm: Move CanProvide to LegacyDataSPKM (Ava Chow)
b1ab927bbf tests: Test migration of additional P2WSH scripts (Ava Chow)
c39b3cfcd1 test: Extra verification that migratewallet migrates (Ava Chow)
Pull request description:
The legacy wallet `IsMine()` is essentially a black box that would tell us whether the wallet is watching an output script. In order to migrate legacy wallets to descriptor wallets, we need to be able to compute all of the output scripts that a legacy wallet would watch. The original approach for this was to understand `IsMine()` and write a function which would be its inverse. This was partially done in the original migration code, and attempted to be completed in #30328. However, further analysis of `IsMine()` has continued to reveal additional edge cases which make writing an inverse function increasingly difficult to verify correctness.
This PR instead changes migration to utilize `IsMine()` to produce the output scripts by first computing a superset of all of the output scripts that `IsMine()` would watch and testing each script against `IsMine()` to filter for the ones that actually are watched. The superset is constructed by computing all possible output scripts for the keys and scripts in the wallet - for keys, every key could be a P2PK, P2PKH, P2WPKH, and P2SH-P2WPKH; for scripts, every script could be an output script, the redeemScript of a P2SH, the witnessScript of a P2WSH, and the witnessScript of a P2SH-P2WSH.
Additionally, the legacy wallet can contain scripts that are redeemScripts and witnessScripts, while not watching for any output script utilizing that script. These are known as solvable scripts and are migrated to a separate "solvables" wallet. The previous approach to identifying these solvables was similar to identifying output scripts - finding known solvable conditions and computing the scripts. However, this also can miss scripts, so the solvables are now identified in a manner similar to the output scripts but using the function `CanProvide()`. Using the same superset as before, all output scripts which are `ISMINE_NO` are put through `CanProvide()` which will perform a dummy signing and then a key lookup to determine whether the legacy wallet could provide any solving data for the output script. The scripts that pass will have their descriptors inferred and the script included in the solvables wallet.
The main downside of this approach is that `IsMine()` and `CanProvide()` can no longer be deleted. They will need to be refactored to be migration only code instead in #28710.
Lastly, I've added 2 test cases for the edge cases that prompted this change of approach. In particular, miniscript witnessScripts and `rawtr()` output scripts are solvable and signable in a legacy wallet, although never `ISMINE_SPENDABLE`.
ACKs for top commit:
sipa:
Code review ACK af76664b12d8611b606a7e755a103a20542ee539; I did not review the tests in detail.
brunoerg:
code review ACK af76664b12
rkrux:
ACK af76664b12
Tree-SHA512: 7f58a90de6f38fe9801fb6c2a520627072c8d66358652ad0872ff59deb678a82664b99babcfd874288bebcb1487d099a77821f03ae063c2b4cbf2d316e77d141
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>
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.
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
8fe552fe6e test: add missing sync to p2p_tx_download.py (Martin Zumsande)
Pull request description:
If the node hasn't processed the inv from the outbound peer before the mocktime bump, the peer won't be preferred after the other inv timeouts, failing the test . Therefore, add a sync, just like there is one after the `send_message` calls in the previous lines.
Fixes#31833
ACKs for top commit:
maflcko:
lgtm ACK 8fe552fe6e
instagibbs:
ACK 8fe552fe6e
Tree-SHA512: fda935d8a4081b5ecae96f5a73c04f4bb91feaeb09b5c159ffd45cf16668c4345ff268c57f383ba7c7ff544ee07b21f97aa28f257ade809c18b9310837795e7a
b2e9fdc00f test: expect that files may disappear from /proc/PID/fd/ (Vasil Dimov)
Pull request description:
`get_socket_inodes()` calls `os.listdir()` and then iterates on the results using `os.readlink()`. However a file may disappear from the directory after `os.listdir()` and before `os.readlink()` resulting in a `FileNotFoundError` exception.
It is expected that this may happen for `bitcoind` which is running and could open or close files or sockets at any time. Thus ignore the `FileNotFoundError` exception.
ACKs for top commit:
arejula27:
ACK [`b2e9fdc`](b2e9fdc00f)
sipa:
utACK b2e9fdc00f
achow101:
ACK b2e9fdc00f
theuni:
utACK b2e9fdc00f
hodlinator:
ACK b2e9fdc00f
Tree-SHA512: 8eb05393e4de4307a70af446c3fc7e8f7dc3f08bf9d68d74d02b0e4e900cfd4865249f297be31f1fd7b05ffea45eb855c5cfcd75704167950c1deb4f17109f33
This prevents intermittent failures - if the node hasn't processed
the inv from the outbound peer before the mocktime bump, the peer
won't be preferred after the other inv timeouts, failing the test .
Therefore, add a sync, just like there is one after the send_message
calls in the previous lines.
The legacy wallet will be able to solve output scripts where the
redeemScript or witnessScript is known, but does not know any of the
private keys involved in that script. These should be migrated to the
solvables wallet.
386eecff5f doc: add release notes (ismaelsadeeq)
3eaa0a3b66 miner: init: add `-blockreservedweight` startup option (ismaelsadeeq)
777434a2cd doc: rpc: improve `getmininginfo` help text (ismaelsadeeq)
c8acd4032d init: fail to start when `-blockmaxweight` exceeds `MAX_BLOCK_WEIGHT` (ismaelsadeeq)
5bb31633cc test: add `-blockmaxweight` startup option functional test (ismaelsadeeq)
2c7d90a6d6 miner: bugfix: fix duplicate weight reservation in block assembler (ismaelsadeeq)
Pull request description:
* This PR attempts to fix the duplicate coinbase weight reservation issue we currently have.
* Fixes#21950
We reserve 4000 weight units for coinbase transaction in `DEFAULT_BLOCK_MAX_WEIGHT`
7590e93bc7/src/policy/policy.h (L23)
And also reserve additional `4000` weight units in the default `BlockCreationOptions` struct.
7590e93bc7/src/node/types.h (L36-L40)
**Motivation**
- This issue was first noticed during a review here https://github.com/bitcoin/bitcoin/pull/11100#discussion_r136157411)
- It was later reported in issue #21950.
- I also came across the bug while writing a test for building the block template. I could not create a block template above `3,992,000` in the block assembler, and this was not documented anywhere. It took me a while to realize that we were reserving space for the coinbase transaction weight twice.
---
This PR fixes this by consolidating the reservation to be in a single location in the codebase.
This PR then adds a new startup option `-blockreservedweight` whose default is `8000` that can be used to lower or increase the block reserved weight for block header, txs count, coinbase tx.
ACKs for top commit:
Sjors:
ACK 386eecff5f
fjahr:
Code review ACK 386eecff5f
glozow:
utACK 386eecff5f, nonblocking nits. I do think the release notes should be clarified more
pinheadmz:
ACK 386eecff5f
Tree-SHA512: f27efa1da57947b7f4d42b9322b83d13afe73dd749dd9cac49360002824dd41c99a876a610554ac2d67bad7485020b9dcc423a8e6748fc79d6a10de6d4357d4c
Some of the networking constants defined in p2p_tx_download.py are more generally
defined in p2p.py
Also, rename the remaining ones to match ones defined in txdownloadman
1973a9e4f1 test: fixes p2p_ibd_txrelay wait time (Sergi Delgado Segura)
Pull request description:
`p2p_ibd_txrelay` expects no GETDATA to have been received by a peer after announcing a transaction. The reason is that the node is doing IBD, so transaction requests are not replied to. However, the way this is checked is wrong, and the check will pass even if the node **was not** in IBD.
This is due to the mocktime not being properly initialized, so the check is always performed earlier than it should, making it impossible for the request to be there.
This can be checked by modifying the test so the peer **is not doing IBD**, and checking how the test succeeds on that assert (even though it fails later on, given the nature of the test):
```diff
index 882f5b5c13..3a69ae5860 100755
--- a/test/functional/p2p_ibd_txrelay.py
+++ b/test/functional/p2p_ibd_txrelay.py
@@ -34,7 +34,7 @@ NORMAL_FEE_FILTER = Decimal(100) / COIN
class P2PIBDTxRelayTest(BitcoinTestFramework):
def set_test_params(self):
- self.setup_clean_chain = True
+ # self.setup_clean_chain = True
self.num_nodes = 2
self.extra_args = [
["-minrelaytxfee={}".format(NORMAL_FEE_FILTER)],
@@ -43,9 +43,11 @@ class P2PIBDTxRelayTest(BitcoinTestFramework):
def run_test(self):
self.log.info("Check that nodes set minfilter to MAX_MONEY while still in IBD")
- for node in self.nodes:
- assert node.getblockchaininfo()['initialblockdownload']
- self.wait_until(lambda: all(peer['minfeefilter'] == MAX_FEE_FILTER for peer in node.getpeerinfo()))
+ # for node in self.nodes:
+ # assert node.getblockchaininfo()['initialblockdownload']
+ # self.wait_until(lambda: all(peer['minfeefilter'] == MAX_FEE_FILTER for peer in node.getpeerinfo()))
```
ACKs for top commit:
i-am-yuvi:
ACK 1973a9e4f1
glozow:
ACK 1973a9e4f1
Tree-SHA512: c4b3afe9927c5480671ebf5c1f6ee5fc7e3aeefeb13c210fa83587a6c126e1a8e40ad8e46587537d0f4bf06a36bbf2310ca065d685d4d9286e5a446b8d5b2235
3f4b104b1b test: make sure we are on sync with a peer before checking if they have sent a message (Sergi Delgado Segura)
Pull request description:
p2p_orphan_handling checks whether a message has not been requested slightly too soon, making the check always succeed. This passes unnoticed since the expected result is for the message to not have been received, but it will make the test not catch a relevant change that should make it fail.
An easy way to check this is the case is to modify one of the test cases to force a request within the expected time, and check how the request is not seen. After the change, the test would crash as expected:
```diff
index 963d92485c..30ab5f2035 100755
--- a/test/functional/p2p_orphan_handling.py
+++ b/test/functional/p2p_orphan_handling.py
@@ -186,9 +185,12 @@ class OrphanHandlingTest(BitcoinTestFramework):
parent_inv = CInv(t=MSG_WTX, h=int(tx_parent_arrives["tx"].getwtxid(), 16))
assert_equal(len(peer_spy.get_invs()), 0)
peer_spy.assert_no_immediate_response(msg_getdata([parent_inv]))
+ txid = 0xdeadbeef
+ peer_spy.send_and_ping(msg_inv([CInv(t=MSG_WTX, h=txid)]))
# Request would be scheduled with this delay because it is not a preferred relay peer.
self.nodes[0].bumpmocktime(NONPREF_PEER_TX_DELAY)
+ peer_spy.assert_never_requested(int(txid))
peer_spy.assert_never_requested(int(tx_parent_arrives["txid"], 16))
peer_spy.assert_never_requested(int(tx_parent_doesnt_arrive["txid"], 16))
# Request would be scheduled with this delay because it is by txid.
```
It is worth noting that this is not seen in the cases where the message is expected to be received, because in such cases `assert_never_requested` is always after a `wait_....` method, which is already waiting for the node to sync on their end.
ACKs for top commit:
i-am-yuvi:
ACK 3f4b104b1b
instagibbs:
ACK 3f4b104b1b
glozow:
ACK 3f4b104b1b
Tree-SHA512: 321a6605d630bed2217b6374e999dbb84da14138263dd8adf65fe3a6cd7981a50c873beced9cf05cb6d747a912e91017c58e7d4323d25449c87d83095ff4cba9
e3622a9692 tracing: document that peer addrs can be >68 chars (0xb10c)
b19b526758 tracing: log_p2p_connections.bt example (0xb10c)
caa5486574 tracing: connection closed tracepoint (0xb10c)
b2ad6ede95 tracing: add misbehaving conn tracepoint (0xb10c)
68c1ef4f19 tracing: add inbound connection eviction tracepoint (0xb10c)
4d61d52f43 tracing: add outbound connection tracepoint (0xb10c)
85b2603eec tracing: add inbound connection tracepoint (0xb10c)
Pull request description:
This adds five new tracepoints with documentation and tests for network connections:
- established connections with `net:inbound_connection` and `net:outbound_connection`
- closed connections (both closed by us or by the peer) with `net:closed_connnection`
- inbound connections that we choose to evict with `net:evicted_inbound_connection`
- connections that are misbehaving and punished with `net:misbehaving_connection`
I've been using these tracepoints for a few months now to monitor connection lifetimes, re-connection frequency by IP and netgroup, misbehavior, peer discouragement, and eviction and more. Together with the two existing P2P message tracepoints they allow for a good overview of local P2P network activity. Also sort-of addresses https://github.com/bitcoin/bitcoin/pull/22006#discussion_r636775863.
I've been back and forth on which arguments to include. For example, `net:evicted_connection` could also include some of the eviction metrics like e.g. `last_block_time`, `min_ping_time`, ... but I've left them out for now. If wanted, this can be added here or in a follow-up. I've tried to minimize a potential performance impact by measuring executed instructions with `gdb` where possible (method described [here](https://github.com/bitcoin/bitcoin/pull/23724#issuecomment-996919963)). I don't think a few hundred extra instructions are too crucial, as connection opens/closes aren't too frequent (compared to e.g. P2P messages). Note: e.g. `CreateNodeFromAcceptedSocket()` usually executes between 80k and 90k instructions for each new inbound connection.
| tracepoint | instructions |
|----------------------------|--------------------------------------------------------|
| net:inbound_connection | 390 ins |
| net:outbound_connection | between 700 and 1000 ins |
| net:closed_connnection | 473 ins |
| net:evicted_inbound_connection | not measured; likely similar to net:closed_connnection |
| net:misbehaving_connection | not measured |
Also added a bpftrace (tested with v0.14.1) `log_p2p_connections.bt` example script that produces output similar to:
```
Attaching 6 probes...
Logging opened, closed, misbehaving, and evicted P2P connections
OUTBOUND conn to 127.0.0.1:15287: id=0, type=block-relay-only, network=0, total_out=1
INBOUND conn from 127.0.0.1:45324: id=1, type=inbound, network=0, total_in=1
MISBEHAVING conn id=1, message='getdata message size = 50001'
CLOSED conn to 127.0.0.1:15287: id=0, type=block-relay-only, network=0, established=1231006505
EVICTED conn to 127.0.0.1:45324: id=1, type=inbound, network=0, established=1612312312
```
ACKs for top commit:
laanwj:
re-ACK e3622a9692
vasild:
ACK e3622a9692
sipa:
utACK e3622a9692
Tree-SHA512: 1032dcac6fe0ced981715606f82c2db47016407d3accb8f216c978f010da9bc20453e24a167dcc95287f4783b48562ffb90f645bf230990e3df1b9b9a6d4e5d0
723440c5b8 test framework, wallet: rename get_scriptPubKey method to get_output_script (Alfonso Roman Zubeldia)
fa0232a3e0 test: add validation for gettxout RPC response (Alfonso Roman Zubeldia)
Pull request description:
Added a new test in `test/functional/rpc_blockchain.py` to validate the gettxout RPC response. This new test ensures all response elements are verified, including `bestblock`, `confirmations`, `value`, `coinbase`, and `scriptPubKey` details.
Also renamed the method `get_scriptPubKey` from `test/functional/test_framework/wallet.py` to the modern name `get_output_script` as suggested by maflcko (https://github.com/bitcoin/bitcoin/pull/30226#discussion_r1925491846)
ACKs for top commit:
fjahr:
reACK 723440c5b8
maflcko:
lgtm ACK 723440c5b8
brunoerg:
code review ACK 723440c5b8
Tree-SHA512: 3384578909d2e7548cef302c5b8a9fed5b82dfc942892503ad4a05e73f5cceafad1eab3af9a27e54aef3db7631f8935298d6b882c70d2f02a9a75b8e3c209b6c
846a138728 func test: Expand tx download preference tests (Greg Sanders)
Pull request description:
1. Check that outbound nodes are treated the same as whitelisted connections for
the purposes of `getdata` delays
2. Add test case that demonstrates download retries are preferentially
given to outbound (preferred) connections
even when multiple announcements are
considered ready.
`NUM_INBOUND` is a magic number large enough that it should fail over 90% of the time
if the underlying outbound->preferred->PriorityComputer logic was broken. Bumping this
to 100 peers cost another 14 seconds locally for the sub-test, so I made it pretty small.
ACKs for top commit:
i-am-yuvi:
tACK 846a138728 good catch
maflcko:
ACK 846a138728🍕
marcofleon:
lgtm ACK 846a138728
Tree-SHA512: 337aa4dc33b5c0abeb4fe7e4cd5e389f7f53ae25dd991ba26615c16999872542391993020122fd255af4c7163f76c1d1feb2f2d6114f12a364c0360d4d52b8c3
7e0db87d4f test: added additional coverage to waitforblock and waitforblockheight rpc's (kevkevinpal)
Pull request description:
Similar to https://github.com/bitcoin/bitcoin/pull/31746
This adds test coverage to the `waitforblock` and `waitforblockheight` rpc's by adding a test to assert we get an rpc error if we include a negative timeout
ACKs for top commit:
Sjors:
utACK 7e0db87d4f
Prabhat1308:
ACK [7e0db87](7e0db87d4f)
brunoerg:
code review ACK 7e0db87d4f
BrandonOdiwuor:
Code Review ACK 7e0db87d4f
Tree-SHA512: c3b1b3a525e91e0092b783d73d2401126e3b8792a394be00374258f20cf3d619498e6625d3aad5e5ced29509c5eac828ee03c4ee66ba405b91e89be7e8b07311
- This commit renamed coinbase_max_additional_weight to block_reserved_weight.
- Also clarify that the reservation is for block header, transaction count
and coinbase transaction.
faf8fc5487 lint: Call lint_commit_msg from test_runner (MarcoFalke)
fa99728b0c lint: Move commit range printing to test_runner (MarcoFalke)
fa673cf344 lint: Call lint_scripted_diff from test_runner (MarcoFalke)
Pull request description:
The lint `commit-script-check.sh` can not be called from the test_runner at all and must be called manually. Also, some checks require `COMMIT_RANGE` to be set.
Fix all issues by moving two lint checks into the test_runner. Also, the proper commit range is passed to the checks by the test_runner, so that the user no longer has to do it.
ACKs for top commit:
kevkevinpal:
reACK [faf8fc5](faf8fc5487)
willcl-ark:
tACK faf8fc5487
Tree-SHA512: 78018adc618d997508c226c9eee0a4fada3899cdfd91587132ab1c0389aea69127bafc3a900e90e30aca2c6bae9dcd6e6188ef287e91413bc63ee66fb078b1af
9d2d9f7ce2 rpc: Include assumeutxo as a failure reason of rescanblockchain (Fabian Jahr)
595edee169 test, assumeutxo: import descriptors during background sync (Alfonso Roman Zubeldia)
d73ae603d4 rpc: Improve importdescriptor RPC error messages (Fabian Jahr)
27f99b6d63 validation: Don't assume m_chain_tx_count in GuessVerificationProgress (Fabian Jahr)
42d5d53363 interfaces: Add helper function for wallet on pruning (Fabian Jahr)
Pull request description:
A test that is added as part of #30455 uncovered this issue: The `GuessVerificationProgress` function is used during during descriptor import and relies on `m_chain_tx_count`. In #29370 an [`Assume` was added](0fd915ee6b) expecting the `m_chaint_tx_count` to be set. However, as the test uncovered, `GuessVerificationProgress` is called with background sync blocks that have `m_chaint_tx_count = 0` when they have not been downloaded and processed yet.
The simple fix is to remove the `Assume`. Users should not be thrown off by the `Internal bug detected` error. The behavior of `importdescriptor` is kept consistent with the behavior for blocks missing due to pruning.
The test by alfonsoromanz is cherry-picked here to show that the [CI errors](https://cirrus-ci.com/task/5110045812195328?logs=ci#L2535) should be fixed by this change.
This PR also improves error messages returned by the `importdescriptors` and `rescanblockchain` RPCs. The error message now changes depending on the situation of the node, i.e. if pruning is happening or an assumutxo backgroundsync is active.
ACKs for top commit:
achow101:
ACK 9d2d9f7ce2
mzumsande:
Code Review ACK 9d2d9f7ce2
furszy:
Code review ACK 9d2d9f7ce2
Tree-SHA512: b841a9b371e5eb8eb3bfebca35645ff2fdded7a3e5e06308d46a33a51ca42cc4c258028c9958fbbb6cda9bb990e07ab8d8504dd9ec6705ef78afe0435912b365
93747d934b test: Added coverage to the waitfornewblock rpc (kevkevinpal)
Pull request description:
Added a test for the Negative timeout error if the rpc is given a negative value for its timeout arg
This adds coverage to the `waitfornewblock` rpc
you can check to see there is no coverage for this error by doing
`grep -nri "Negative timeout" ./test/`
and nothing shows up, you can also see by manually checking where we call `waitfornewblock` in the functional tests
ACKs for top commit:
Sjors:
tACK 93747d934b
achow101:
ACK 93747d934b
brunoerg:
code review ACK 93747d934b
tdb3:
ACK 93747d934b
Tree-SHA512: 45cf34312412d3691a39f003bcd54791ea16542aa3f5a2674d7499c9cc4039550b2cbd32cc3d4c5fe100d65cb05690594b10a0c42dfab63bcca3dac121bb195b
e1676b08f7 doc: release notes (Sjors Provoost)
0082f6acc1 rpc: have mintime account for timewarp rule (Sjors Provoost)
79d45b10f1 rpc: clarify BIP94 behavior for curtime (Sjors Provoost)
0713548137 refactor: add GetMinimumTime() helper (Sjors Provoost)
Pull request description:
#30681 fixed the `curtime` field of `getblocktemplate` to take the timewarp rule into account. However I forgot to do the same for the `mintime` field, which was hardcoded to use `pindexPrev->GetMedianTimePast()+1`.
This PR adds a helper `GetMinimumTime()` and uses it for the `mintime` field.
#31376 changed the `curtime` field to always account for the timewarp rule. This PR maintains that behavior.
Note that `mintime` now always applies BIP94, including on mainnet. This makes future softfork activation safer.
It could be backported to v28.
ACKs for top commit:
fjahr:
tACK e1676b08f7
achow101:
ACK e1676b08f7
darosior:
utACK e1676b08f7 on the code changes
tdb3:
brief code review re ACK e1676b08f7
TheCharlatan:
ACK e1676b08f7
Tree-SHA512: 0e322d8cc3b8ff770849bce211edcb5b6f55d04e5e0dee0657805049663d758f27423b047ee6363bd8f6c6fead13f974760f48b3321ea86f514f446e1b23231c
152a2dcdef test: fix intermittent timeout in p2p_1p1c_network.py (Martin Zumsande)
Pull request description:
The timeout is due to outstanding txrequests with python peers, which have the same timeout (`60s`) as the mempool sync timeout.
I explained this in more detail in https://github.com/bitcoin/bitcoin/issues/31721#issuecomment-2620169640 and also mentioned there how to reproduce it.
Fix this by disconnecting the python peers after they send their txns, they aren't needed after this point anyway because the main goal of the test is the sync between the 4 full nodes.
Fixes#31721
ACKs for top commit:
achow101:
ACK 152a2dcdef
instagibbs:
reACK 152a2dcdef
marcofleon:
ACK 152a2dcdef
glozow:
reACK 152a2dcdef
Tree-SHA512: 908c58933d8e9fcca91425fce1b7c9c7cb7121a6d26840630e03a442356ad2a327d1e087df72a19caa97024ea827593e10f2ff93838f88939458e73df9857df0
cddcbaf81e RPC: improve SFFO arg parsing, error catching and coverage (furszy)
4f4cd35319 rpc: decouple sendtoaddress 'subtractfeefromamount' boolean parsing (furszy)
Pull request description:
Following changes were made:
1) Catch and signal error for duplicate string destinations.
2) Catch and signal error for invalid value type.
3) Catch and signal error for string destination not found in tx outputs.
4) Improved `InterpretSubtractFeeFromOutputInstructions()` code organization.
5) Added test coverage for all possible error failures.
Also, fixed two PEP 8 warnings at the 'wallet_sendmany.py' file:
- PEP 8: E302 expected 2 blank lines, found 1 at the SendmanyTest class declaration.
- PEP 8: E303 too many blank lines (2) at skip_test_if_missing_module() and set_test_params()
ACKs for top commit:
achow101:
ACK cddcbaf81e
murchandamus:
crACK cddcbaf81e
naiyoma:
TACK [cddcbaf81e)
ismaelsadeeq:
Code review and Tested ACK cddcbaf81e
Tree-SHA512: c9c15582b81101a93987458d155394ff2c9ca42864624c034ee808a31c3a7d7f55105dea98e86fce17d3c7b2c1a6b5b77942da66b287f8b8881a60cde78c1a3c
d45eb3964f test: compare BDB dumps of test framework parser and wallet tool (Sebastian Falbesoner)
01ddd9f646 test: complete BDB parser (handle internal/overflow pages, support all page sizes) (Sebastian Falbesoner)
Pull request description:
This PR adds missing features to our test framework's BDB parser with the goal of hopefully being able to read all legacy wallets that are created with current and past versions of Bitcoin Core. This could be useful both for making review of https://github.com/bitcoin/bitcoin/pull/26606 easier and to also possibly improve our functional tests for the wallet BDB-ro parser by additionally validating it with an alternative implementation. The second commits introduces a test that create a legacy wallet with huge label strings (in order to create overflow pages, i.e. pages needed for key/value data than is larger than the page size) and compares the dump outputs of wallet tool and the extended test framework BDB parser.
It can be exercised via `$ ./test/functional/tool_wallet.py --legacy`. BDB support has to be compiled in (obviously).
For some manual tests regarding different page sizes, the following patch can be used:
```diff
diff --git a/src/wallet/bdb.cpp b/src/wallet/bdb.cpp
index 38cca32f80..1bf39323d3 100644
--- a/src/wallet/bdb.cpp
+++ b/src/wallet/bdb.cpp
@@ -395,6 +395,7 @@ void BerkeleyDatabase::Open()
DB_BTREE, // Database type
nFlags, // Flags
0);
+ pdb_temp->set_pagesize(1<<9); /* valid BDB pagesizes are from 1<<9 (=512) to <<16 (=65536) */
if (ret != 0) {
throw std::runtime_error(strprintf("BerkeleyDatabase: Error %d, can't open database %s", ret, strFile));
```
I verified that the newly introduced test passes with all valid page sizes between 512 and 65536.
ACKs for top commit:
achow101:
ACK d45eb3964f
furszy:
utACK d45eb3964f
brunoerg:
code review ACK d45eb3964f
Tree-SHA512: 9f8ac80452545f4fcd24a17ea6f9cf91b487cfb1fcb99a0ba9153fa4e3b239daa126454e26109fdcb72eb1c76a4ee3b46fd6af21dc318ab67bd12b3ebd26cfdd
p2p_orphan_handling checks whether a message has not been requested slightly
too soon, making the check always succeed. This passes unnoticed since the
expected result is for the message to not have been received, but it will make
the test not catch a relevant change that should make it fail
p2p_ibd_txrelay expects no GETDATA to have been received by a peer after
announcing a transaction. The reason is that the node is doing IBD, so
transaction requests are not replied to. However, the way this is checked
is wrong, and the check will pass even if the node **was not** in IBD.
This is due to the mocktime not being properly initialized, so the check
is always performed earlier than it should, making it impossible for the
request to be there
The timeout is due to outstanding txrequests with
python peers. Fix this by disconnecting these peers
after they send their txns, they aren't needed after
this point anyway.
Previously in getblocktemplate only curtime took the timewarp rule into account.
Mining pool software could use either, though in general it should use curtime.
faf2f2c654 test: Avoid redundant stop and error spam on shutdown (MarcoFalke)
fae3bf6b87 test: Avoid redundant stop and error spam on startup failure (MarcoFalke)
fa0dc09b90 test: Remove --noshutdown flag (MarcoFalke)
fad441fba0 test: Treat leftover process as error (MarcoFalke)
Pull request description:
The `--noshutdown` flag is brittle, confusing, and redundant:
* Someone wanting to inspect the state after a test failure will likely also want to debug the state on the python side, so the option is redundant with `--pdbonfailure`. If there was a use case to replicate `--pdbonfailure` without starting pdb, a dedicated flag could be added for that use case.
* It is brittle to use the flag for a passing test, because it will disable checks in the test. For example, on shutdown LSan will perform a leak check, and the test framework will check that the node did not crash, and it will check that the node did not print errors to stderr.
Fix all issues by removing it.
Also, tidy up startup error messages to be less confusing as a result.
ACKs for top commit:
hodlinator:
re-ACK faf2f2c654
pablomartin4btc:
re tACK faf2f2c654
Tree-SHA512: 46d7ae59c7be88b93f1f9e0b6be21af0fc101e646512e2c5e725682cb18bfec8aa010e0ebe89ce9ffe239e5caac0da5f81cc97b79e738d26ca5fa31930e8e4e3
1b51616f2e test: improve rogue calls in mining functions (i-am-yuvi)
Pull request description:
#31403 follow-up, see [comment](https://github.com/bitcoin/bitcoin/pull/31403#pullrequestreview-2498806354)
- Rename `invalid_call` parameter to `called_by_framework` in `generateblock`, `generatetoaddress` and `generatetodescriptor` mining methods to better express its intended usage.
- Add explicit assertion message clarifying that these functions should only be called by TestFramework itself to maintain proper node synchronization.
ACKs for top commit:
maflcko:
lgtm ACK 1b51616f2e
achow101:
ACK 1b51616f2e
hodlinator:
re-ACK 1b51616f2e
Prabhat1308:
ACK [1b51616](1b51616f2e)
Tree-SHA512: 56832626fe54dcaa07dacb4f9c960c0a83fad3fb12272155114ac697856c59b7f44805e1152eddeec7a5e8f7daf487382dc01b5b9ae2e74b62b2df6bd1f81f77
92787dd52c test: raise an error when target_vsize is below tx virtual size (ismaelsadeeq)
a8780c937f test: raise an error if output value is <= 0 in `create_self_transfer` (ismaelsadeeq)
f6e88931f0 test: test that `create_self_transfer_multi` respects `target_vsize` (ismaelsadeeq)
Pull request description:
This is a simple test PR that does two things:
1. Raise an exception in `_bulk_tx_` when `target_vsize` is too low, i.e., below the tx vsize.
2. Addresses some review comments from https://github.com/bitcoin/bitcoin/pull/30162, which are:
- Raise an error if the output value is less than or equal to zero in `create_self_transfer`.
This prevents creating transactions with a value of 0 or less.
- Add a test to verify that `create_self_transfer_multi` also respects the passed `target_vsize`.
ACKs for top commit:
achow101:
ACK 92787dd52c
theStack:
ACK 92787dd52c
rkrux:
reACK 92787dd52c
glozow:
ACK 92787dd52c
Tree-SHA512: 1f2767f2cf715ed65074c5fff347eec160b142685777d833d5e872cfef364d3dc1916b52ee442e99c7b9a8d514ff62bc67a9899d8854f65a4b93ac3ae300d18e
2656a5658c tests: add a test for the new blocksdir lock (Cory Fields)
bdc0a68e67 init: lock blocksdir in addition to datadir (Cory Fields)
cabb2e5c24 refactor: introduce a more general LockDirectories for init (Cory Fields)
1db331ba76 init: allow a new xor key to be written if the blocksdir is newly created (Cory Fields)
Pull request description:
This probably should've been included in #12653 when `-blocksdir` was introduced. Credit TheCharlatan for noticing that it's missing.
This guards against 2 processes running with separate datadirs but the same blocksdir. I didn't add `walletdir` as I assume sqlite has us covered there.
It's not likely to happen currently, but may be more relevant in the future with applications using the kernel. Note that the kernel does not currently do any dir locking, but it should.
ACKs for top commit:
maflcko:
review ACK 2656a5658c 🏼
kevkevinpal:
ACK [2656a56](2656a5658c)
achow101:
ACK 2656a5658c
tdb3:
Code review and light test ACK 2656a5658c
Tree-SHA512: 3ba17dc670126adda104148e14d1322ea4f67d671c84aaa9c08c760ef778ca1936832c0dc843cd6367e09939f64c6f0a682b0fa23a5967e821b899dff1fff961
8996fef8ae test: p2p: check that INV messages not matching wtxidrelay are ignored (Sebastian Falbesoner)
e0b3336822 test: p2p: fix sending of manual INVs in tx download test (Sebastian Falbesoner)
Pull request description:
The `test_inv_block` sub-test in p2p_tx_download.py has a subtle bug: the manual msg_inv announcements from peers currently have no effect, since they don't match the wtxidrelay setting (=true by default for `P2PInterface` instances) and are hence ignored by the nodes (since 2d282e0c / PR #18044):
e7c4794955/src/net_processing.cpp (L3904-L3911)
Though the test still passes on master, it does so without the intended scenario of asking an additional peer (triggering the GETDATA_TX_INTERVAL delay). Fix this by sending the INV message with MSG_WTX instead of MSG_TX. This increases the test run time by about one minute intentionally.
It might be good to avoid issues like this in the future, happy to add test framework improvements if someone has a concrete idea.
(Got into the topic of tx/wtx announcements via the discussion https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1904121487)
ACKs for top commit:
maflcko:
ACK 8996fef8ae😸
danielabrozzoni:
ACK 8996fef8ae
mzumsande:
Code Review ACK 8996fef8ae
Tree-SHA512: 3da26f9539c89d64c3b0d0579d9af2a6a4577615eed192506e1fb4318421b235f99a6672a497dea3050fba85dad32678f37fd2cda9ecb70cbf52982db37982e8
Trying to shut down a node after a test failure may fail and lead to an
RPC error.
Also, it is confusing to sidestep the existing fallback to kill any
leftover nodes on a test failure.
So just rely on the fallback.
Idea by Hodlinator.
Co-Authored-By: Hodlinator <172445034+hodlinator@users.noreply.github.com>
4da7bfdcc9 test: add coverage for unknown address type for `createwalletdescriptor` (brunoerg)
Pull request description:
Calling `createwalletdescriptor` RPC with an unknown address type throws an error. This PR adds test coverage for it as done for other RPCs (`getnewaddress `, `getrawchangeaddress`, etc).
ACKs for top commit:
maflcko:
lgtm ACK 4da7bfdcc9
rkrux:
tACK 4da7bfdcc9
Tree-SHA512: 490bc3ffeb70b0f26db0a44d3950d7410fef35d4056487f2e82c081fb14ca277a18943c487235e0163a29f90fc741a262c29835beb9f41936affa4e73ddad25f
fa9aced800 test: Check that reindex with prune wipes blk files (MarcoFalke)
fa9593efc2 test: Use high-level python types (MarcoFalke)
Pull request description:
This adds missing test coverage for `CleanupBlockRevFiles`.
ACKs for top commit:
TheCharlatan:
Re-ACK fa9aced800
l0rinc:
ACK fa9aced800
tdb3:
re ACK fa9aced800
Tree-SHA512: b31ff8a896ce344437715e7fb7efdb8cd7e11470e8465d8972fddfdb58ffd78257786c4060e8596cc53b6278f8ac6a9b6eb05a06e9df58b8b240bdaa719a8e5b
d38ade7bc4 qa: Use `sys.executable` when invoking other Python scripts (Hennadii Stepanov)
Pull request description:
This PR fixes the `rpc_signer.py` and `wallet_signer.py` functional tests on systems where `python3` is not available in the `PATH`, causing the shebang `#!/usr/bin/env python3` to fail.
Here are logs on NetBSD 10.0:
- without this PR:
```
$ python3.12 ./build/test/functional/test_runner.py rpc_signer.py wallet_signer.py
Temporary test directory at /tmp/test_runner_₿_🏃_20241219_160538
Remaining jobs: [rpc_signer.py, wallet_signer.py --descriptors]
1/2 - rpc_signer.py failed, Duration: 1 s
stdout:
2024-12-19T16:05:40.012000Z TestFramework (INFO): PRNG seed is: 1833166631173850775
2024-12-19T16:05:40.012000Z TestFramework (INFO): Initializing test directory /tmp/test_runner_₿_🏃_20241219_160538/rpc_signer_1
2024-12-19T16:05:40.754000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/home/hebasto/dev/bitcoin/test/functional/test_framework/util.py", line 160, in try_rpc
fun(*args, **kwds)
File "/home/hebasto/dev/bitcoin/test/functional/test_framework/coverage.py", line 50, in __call__
return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/hebasto/dev/bitcoin/test/functional/test_framework/authproxy.py", line 146, in __call__
raise JSONRPCException(response['error'], status)
test_framework.authproxy.JSONRPCException: RunCommandParseJSON error: process(/home/hebasto/dev/bitcoin/test/functional/mocks/signer.py enumerate) returned 127: env: python3: No such file or directory
(-1)
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "/home/hebasto/dev/bitcoin/test/functional/test_framework/test_framework.py", line 135, in main
self.run_test()
File "/home/hebasto/dev/bitcoin/build/test/functional/rpc_signer.py", line 72, in run_test
assert_raises_rpc_error(-1, 'fingerprint not found',
File "/home/hebasto/dev/bitcoin/test/functional/test_framework/util.py", line 151, in assert_raises_rpc_error
assert try_rpc(code, message, fun, *args, **kwds), "No exception raised"
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/hebasto/dev/bitcoin/test/functional/test_framework/util.py", line 166, in try_rpc
raise AssertionError(
AssertionError: Expected substring not found in error message:
substring: 'fingerprint not found'
error message: 'RunCommandParseJSON error: process(/home/hebasto/dev/bitcoin/test/functional/mocks/signer.py enumerate) returned 127: env: python3: No such file or directory
'.
2024-12-19T16:05:40.756000Z TestFramework (INFO): Stopping nodes
2024-12-19T16:05:40.873000Z TestFramework (WARNING): Not cleaning up dir /tmp/test_runner_₿_🏃_20241219_160538/rpc_signer_1
2024-12-19T16:05:40.873000Z TestFramework (ERROR): Test failed. Test logging available at /tmp/test_runner_₿_🏃_20241219_160538/rpc_signer_1/test_framework.log
2024-12-19T16:05:40.873000Z TestFramework (ERROR):
2024-12-19T16:05:40.873000Z TestFramework (ERROR): Hint: Call /home/hebasto/dev/bitcoin/test/functional/combine_logs.py '/tmp/test_runner_₿_🏃_20241219_160538/rpc_signer_1' to consolidate all logs
2024-12-19T16:05:40.873000Z TestFramework (ERROR):
2024-12-19T16:05:40.873000Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log.
2024-12-19T16:05:40.873000Z TestFramework (ERROR): https://github.com/bitcoin/bitcoin/issues
2024-12-19T16:05:40.873000Z TestFramework (ERROR):
stderr:
Remaining jobs: [wallet_signer.py --descriptors]
2/2 - wallet_signer.py --descriptors failed, Duration: 1 s
stdout:
2024-12-19T16:05:40.014000Z TestFramework (INFO): PRNG seed is: 7530764367977090686
2024-12-19T16:05:40.014000Z TestFramework (INFO): Initializing test directory /tmp/test_runner_₿_🏃_20241219_160538/wallet_signer_0
2024-12-19T16:05:40.526000Z TestFramework (ERROR): JSONRPC error
Traceback (most recent call last):
File "/home/hebasto/dev/bitcoin/test/functional/test_framework/test_framework.py", line 135, in main
self.run_test()
File "/home/hebasto/dev/bitcoin/build/test/functional/wallet_signer.py", line 66, in run_test
self.test_valid_signer()
File "/home/hebasto/dev/bitcoin/build/test/functional/wallet_signer.py", line 83, in test_valid_signer
self.nodes[1].createwallet(wallet_name='hww', disable_private_keys=True, descriptors=True, external_signer=True)
File "/home/hebasto/dev/bitcoin/test/functional/test_framework/test_node.py", line 935, in createwallet
return self.__getattr__('createwallet')(wallet_name, disable_private_keys, blank, passphrase, avoid_reuse, descriptors, load_on_startup, external_signer)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/hebasto/dev/bitcoin/test/functional/test_framework/coverage.py", line 50, in __call__
return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/hebasto/dev/bitcoin/test/functional/test_framework/authproxy.py", line 146, in __call__
raise JSONRPCException(response['error'], status)
test_framework.authproxy.JSONRPCException: RunCommandParseJSON error: process(/home/hebasto/dev/bitcoin/test/functional/mocks/signer.py enumerate) returned 127: env: python3: No such file or directory
(-1)
2024-12-19T16:05:40.528000Z TestFramework (INFO): Stopping nodes
2024-12-19T16:05:40.645000Z TestFramework (WARNING): Not cleaning up dir /tmp/test_runner_₿_🏃_20241219_160538/wallet_signer_0
2024-12-19T16:05:40.646000Z TestFramework (ERROR): Test failed. Test logging available at /tmp/test_runner_₿_🏃_20241219_160538/wallet_signer_0/test_framework.log
2024-12-19T16:05:40.646000Z TestFramework (ERROR):
2024-12-19T16:05:40.646000Z TestFramework (ERROR): Hint: Call /home/hebasto/dev/bitcoin/test/functional/combine_logs.py '/tmp/test_runner_₿_🏃_20241219_160538/wallet_signer_0' to consolidate all logs
2024-12-19T16:05:40.646000Z TestFramework (ERROR):
2024-12-19T16:05:40.646000Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log.
2024-12-19T16:05:40.646000Z TestFramework (ERROR): https://github.com/bitcoin/bitcoin/issues
2024-12-19T16:05:40.646000Z TestFramework (ERROR):
stderr:
TEST | STATUS | DURATION
rpc_signer.py | ✖ Failed | 1 s
wallet_signer.py --descriptors | ✖ Failed | 1 s
ALL | ✖ Failed | 2 s (accumulated)
Runtime: 1 s
```
- with this PR:
```
$ python3.12 ./build/test/functional/test_runner.py rpc_signer.py wallet_signer.py
Temporary test directory at /tmp/test_runner_₿_🏃_20241219_160011
Remaining jobs: [rpc_signer.py, wallet_signer.py --descriptors]
1/2 - rpc_signer.py passed, Duration: 2 s
Remaining jobs: [wallet_signer.py --descriptors]
2/2 - wallet_signer.py --descriptors passed, Duration: 3 s
TEST | STATUS | DURATION
rpc_signer.py | ✓ Passed | 2 s
wallet_signer.py --descriptors | ✓ Passed | 3 s
ALL | ✓ Passed | 5 s (accumulated)
Runtime: 3 s
```
ACKs for top commit:
maflcko:
lgtm ACK d38ade7bc4
stickies-v:
ACK d38ade7bc4 . I have a minor concern about `sys.executable` not being guaranteed to return a valid Python path, but this patch seems good enough as is so no blocker.
Tree-SHA512: 91fe0abc0b7e2b599c5562f8b225ba60f94c5bd6baa77d8df532155ef4d3ef6c6a862cee7f4a7f565ed4bb3251adcda813b4a4f79be1aa6a4ffdfda8b4e53415
Allowing to call the check from the test_runner allows for consistent
error messages and better UX by having a single test_runner for all
checks.
This requires the env var to be set for now. The next commit makes the
commit range optional.
This is similar in structure to test/functional/wallet_multisig_descriptor_psbt.py
both in code and concept. It should serve as some integration testing for
Miniscript descriptors, and also documents a simple multisig that starts as 4-of-4
and decays to 3-of-4, 2-of-4, and finally 1-of-4 at block heights (I think in the
real world aligning this to halvenings would be nice).
For blocks 1 through 15 the script_BIP34_coinbase_height appends OP_1
to comply with BIP34 and avoid bad-cb-length.
This is inconsistent with BlockAssembler::CreateNewBlock() which adds
OP_0 instead.
The utxo_total_supply fuzzer and MinerTestingSetup::Block also use OP_0.
Changing it is required to import the test vectors in the next commit.
It also ensures the test vectors can be regenerated using the CPU miner
at https://github.com/pooler/cpuminer without patches (it uses OP_0).
The same helper is used by the signet miner, so this will impact newly
bootstrapped signets.
86d7135e36 [p2p] only attempt 1p1c when both txns provided by the same peer (glozow)
f7658d9b14 [cleanup] remove p2p_inv from AddTxAnnouncement (glozow)
063c1324c1 [functional test] getorphantxs reflects multiple announcers (glozow)
0da693f7e1 [functional test] orphan handling with multiple announcers (glozow)
b6ea4a9afe [p2p] try multiple peers for orphan resolution (glozow)
1d2e1d709c [refactor] move creation of unique_parents to helper function (glozow)
c6893b0f0b [txdownload] remove unique_parents that we already have (glozow)
163aaf285a [fuzz] orphanage multiple announcer functions (glozow)
22b023b09d [unit test] multiple orphan announcers (glozow)
96c1a822a2 [unit test] TxOrphanage EraseForBlock (glozow)
04448ce32a [txorphanage] add GetTx so that orphan vin can be read (glozow)
e810842acd [txorphanage] support multiple announcers (glozow)
62a9ff1870 [refactor] change type of unique_parents to Txid (glozow)
6951ddcefd [txrequest] GetCandidatePeers (glozow)
Pull request description:
Part of #27463.
(Transaction) **orphan resolution** is a process that kicks off when we are missing UTXOs to validate an unconfirmed transaction. We currently request missing parents by txid; BIP 331 also defines a way to [explicitly request ancestors](https://github.com/bitcoin/bips/blob/master/bip-0331.mediawiki#handle-orphans-better).
Currently, when we find that a transaction is an orphan, we only try to resolve it with the peer who provided the `tx`. If this doesn't work out (e.g. they send a `notfound` or don't respond), we do not try again. We actually can't, because we've already forgotten who else could resolve this orphan (i.e. all the other peers who announced the transaction).
What is wrong with this? It makes transaction download less reliable, particularly for 1p1c packages which must go through orphan resolution in order to be downloaded.
Can we fix this with BIP 331 / is this "duct tape" before the real solution?
BIP 331 (receiver-initiated ancestor package relay) is also based on the idea that there is an orphan that needs resolution, but it's just a new way of communicating information. It's not inherently more honest; you can request ancestor package information and get a `notfound`. So ancestor package relay still requires some kind of procedure for retrying when an orphan resolution attempt fails. See the #27742 implementation which builds on this orphan resolution tracker to keep track of what packages to download (it just isn't rebased on this exact branch). The difference when using BIP 331 is that we request `ancpkginfo` and then `pkgtxns` instead of the parent txids.
Zooming out, we'd like orphan handling to be:
- Bandwidth-efficient: don't have too many requests out at once. As already implemented today, transaction requests for orphan parents and regular download both go through the `TxRequestTracker` so that we don't have duplicate requests out.
- Not vulnerable to censorship: don't give up too easily, use all candidate peers. See e.g. https://bitcoincore.org/en/2024/07/03/disclose_already_asked_for/
- Load-balance between peers: don't overload peers; use all peers available. This is also useful for when we introduce per-peer orphan protection, since each peer will have limited slots.
The approach taken in this PR is to think of each peer who announces an orphan as a potential "orphan resolution candidate." These candidates include:
- the peer who sent us the orphan tx
- any peers who announced the orphan prior to us downloading it
- any peers who subsequently announce the orphan after we have started trying to resolve it
For each orphan resolution candidate, we treat them as having "announced" all of the missing parents to us at the time of receipt of this orphan transaction (or at the time they announced the tx if they do so after we've already started tracking it as an orphan). We add the missing parents as entries to `m_txrequest`, incorporating the logic of typical txrequest processing, which means we prefer outbounds, try not to have duplicate requests in flight, don't overload peers, etc.
ACKs for top commit:
marcofleon:
Code review ACK 86d7135e36
instagibbs:
reACK 86d7135e36
dergoegge:
Code review ACK 86d7135e36
mzumsande:
ACK 86d7135e36
Tree-SHA512: 618d523b86e60c3ea039e88326d50db4e55e8e18309c6a20e8f2b10ed9e076f1de0315c335fd3b8abdabcc8b53cbceb66fb59147d05470ea25b83a2b4bd9c877
fa3efb5729 refactor: Introduce struct to hold a runtime format string (MarcoFalke)
fa6adb0134 lint: Remove unused and broken format string linter (MarcoFalke)
fadc6b9bac refactor: Check translatable format strings at compile-time (MarcoFalke)
fa1d5acb8d refactor: Use TranslateFn type consistently (MarcoFalke)
eeee6cf2ff refactor: Delay translation of _() literals (MarcoFalke)
Pull request description:
All translatable format strings are fixed. This change surfaces errors in them at compile-time.
The implementation achieves this by allowing to delay the translation (or `std::string` construction) that previously happened in `_()` by returning a new type from this function. The new type can be converted to `bilingual_str` where needed.
This can be tested by adding a format string error in an original string literal and observing a new compile-time failure.
Fixes https://github.com/bitcoin/bitcoin/issues/30530
ACKs for top commit:
stickies-v:
re-ACK fa3efb5729
ryanofsky:
Code review ACK fa3efb5729. Since last review added TranslateFn commit, clarified FormatStringCheck documentation, dropped redundant `inline` keyword
Tree-SHA512: 28fa1db11e85935d998031347bd519675d75c171c8323b0ed6cdd0b628c95250bb86b30876946cc48840ded541e95b8a152696f9f2b13a5f28f5673228ee0509
The linter has many implementation bugs and missing features.
Also, it is completely redundant with FormatStringCheck, which
constructs from ConstevalFormatString or a runtime format string.
The `test_inv_block` sub-test in p2p_tx_download.py has a subtle bug:
the manual msg_inv announcements from peers currently have no effect,
since they don't match the wtxidrelay setting (=true by default for
`P2PInterface` instances) and are hence ignored by the nodes (since
2d282e0c / PR #18044). Though the test still passes, it does so without
the intended scenario of asking an additional peer (triggering the
GETDATA_TX_INTERVAL delay). Fix this by sending the INV message with
MSG_WTX instead of MSG_TX. This increases the test run time by about one
minute.
`p2p_dns_seeds.py` would try to connect to the DNS server configured on
the machine and resolve `dummySeed.invalid`.
To block that configure an unavailable proxy which will be used also to
connect to the name server. The test needs 2 successful connections to
other peers (two Python `P2PInterface`s) and they work in spite of the
unavailable proxy because they are on `127.0.0.1` (`NET_UNROUTABLE`) and
the proxy is not used for that.
`feature_config_args.py` uses a proxy address of `1.2.3.4`. This results
in actually trying to open TCP connections over the internet to
`1.2.3.4:9050`.
The test does not need those to succeed so use `127.0.0.1:1` instead.
Also avoid `-noconnect=0` because that is interpreted as `-connect=1`
which is interpreted as `-connect=0.0.0.1` and a connection to
`0.0.0.1:18444` is attempted.
`p2p_seednode.py` would try to connect to `0.0.0.1` and `0.0.0.2` as
seed nodes. This sends outbound TCP packets on a non-loopback interface
to the default router.
Configure an unavailable proxy for all executions of `bitcoind` during
this test. Also change `0.0.0.1` and `0.0.0.2` because connecting to
them would skip the `-proxy=` setting because for such an address:
* `CNetAddr::IsLocal()` is true, thus
* `CNetAddr::IsRoutable()` is false, thus
* `CNetAddr::GetNetwork()` is `NET_UNROUTABLE`, even though
`CNetAddr::m_net` is `NET_IPV4`.
This speeds up the execution time of `p2p_seednode.py`
from 12.5s to 2.5s.
69e95c2b4f tests: Test cleanup of mkeys from wallets without privkeys (Andrew Chow)
2b9279b50a wallet: Remove unused encryption keys from watchonly wallets (Andrew Chow)
813a16a463 wallet: Add HasCryptedKeys (Andrew Chow)
Pull request description:
An earlier version allowed users to create watchonly wallets (wallets without private keys) that were "encrypted". Such wallets would have a stored encryption keys, but nothing would actually be encrypted with them. This can cause unexpected behavior such as https://github.com/bitcoin-core/gui/issues/772.
We can detect such wallets as they will have the disable private keys flag set, no encrypted keys, and encryption keys. For such wallets, we can remove those encryption keys thereby avoiding any issues that may result from this unexpected situation.
ACKs for top commit:
sipa:
utACK 69e95c2b4f.
laanwj:
Code review re-ACK 69e95c2b4f
furszy:
Code review ACK 69e95c2b4f
Tree-SHA512: 901932cd709c57e66c598f011f0105a243b5a8b539db2ef3fcf370dca4cf35ae09bc1110e8fca8353be470f159468855a4dd96b99bc9c1112adc86ccc50e1b9d
589ed1a8ea wallet: migration, avoid loading wallet after failure when it wasn't loaded before (furszy)
Pull request description:
Fixes#31447.
During migration failure, only load wallet back into memory when the wallet was
loaded prior to migration. This fixes the case where BDB is not supported, which
implies that no legacy wallet can be loaded into memory due to the lack of db
writing functionality.
Link to error description https://github.com/bitcoin/bitcoin/issues/31447#issuecomment-2528757140.
This PR also improves migration backup related comments to better document the
current workflow.
ACKs for top commit:
achow101:
ACK 589ed1a8ea
rkrux:
ACK 589ed1a8ea
pablomartin4btc:
tACK 589ed1a8ea
Tree-SHA512: c7a489d2b253c574ee0287b691ebe29fe8d026f659f68a3f6108eca8b4e1e420c67ca7803c6bd70c1e1440791833fabca3afbcf8fe8524c6c9fc08de95b618d0
Trying to immediately shut down a node after a startup failure without
waiting for the RPC to be fully up will in most cases just fail and lead
to an RPC error.
Also, it is confusing to sidestep the existing fallback to kill any
leftover nodes on a test failure.
So just rely on the fallback.
Printing to stderr instead of stdout makes the test_runner.py fail on
leftover processes. This is desired and fine, because a leftover process
should only happen on a test failure anyway.
`get_socket_inodes()` calls `os.listdir()` and then iterates on the
results using `os.readlink()`. However a file may disappear from the
directory after `os.listdir()` and before `os.readlink()` resulting in a
`FileNotFoundError` exception.
It is expected that this may happen for `bitcoind` which is running and
could open or close files or sockets at any time. Thus ignore the
`FileNotFoundError` exception.
If the `releases` directory exists, but still only a subset of the
necessary previous release binaries are available, the test fails by
throwing an exception (sometimes leading to follow-up exceptions like
"AssertionError: [node 0] Error: no RPC connection") and printing out
a stack trace, which can be confusing and at a first glance suggests
that the node crashed or some alike.
Improve this by checking and printing out *all* of the missing release
binaries and failing with an explicit error in this case. Also add an
info on how to download previous releases binaries.
Noticed while testing #30328.
Can be tested by e.g.
$ ./test/get_previous_releases.py -b
$ rm -rf ./releases/v28.0/
$ ./build/test/functional/wallet_migration.py
Now that we track all announcers of an orphan, it's not helpful to
consider an orphan provided by a peer that didn't send us this parent.
It can only hurt our chances of finding the right orphan when there are
multiple candidates.
Adapt the 2 tests in p2p_opportunistic_1p1c.py that looked at 1p1c
packages from different peers. Instead of checking that the right peer
is punished, we now check that the package is not submitted. We can't
use the functional test to see that the package was not considered
because the behavior is indistinguishable (except for the logs).
e8f0e6efaf lint: output-only - Avoid repeated arrows, trim (Hodlinator)
fa9aacf614 lint: Move assertion linter into lint runner (MarcoFalke)
Pull request description:
On failure, this makes the output more consistent with the other linters. Each failure will be marked with an '⚠️ ' emoji and explanation, making it easier to spot.
Also, add --line-number to the filesystem linter.
Also, add newlines after each failing check, to visually separate different failures from each other.
Can be reviewed with:
`--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space`
ACKs for top commit:
davidgumberg:
crACK e8f0e6efaf
hodlinator:
re-ACK e8f0e6efaf
TheCharlatan:
ACK e8f0e6efaf
Tree-SHA512: 9896ff882af9d673ec3e6d2718f877b2fdc8514faba50942fcebacb9de95b1f5b4a5db595e1338fa7f505d06df2df304897350cc55c558c7a85232800e5fd804
b6f0593f43 doc: add release note about testmempoolaccept debug-message (Matthew Zipkin)
f9cac63523 test: cover testmempoolaccept debug-message in RBF test (Matthew Zipkin)
f9650e18ea rbf: remove unecessary newline at end of error string (Matthew Zipkin)
221c789e91 rpc: include verbose reject-details field in testmempoolaccept response (Matthew Zipkin)
Pull request description:
Adds a new field `reject-details` in `testmempoolaccept` responses to include `m_debug_message` from `ValidationState`. This string is the complete error message thrown by the mempool in response to `sendrawtransaction`.
The extra verbosity is helpful to consumers of `testmempoolaccept`, which is sort of a debug tool anyway.
example:
>
> {
> "txid": "07d7a59a7bdad4c3a5070659ea04147c9b755ad9e173c52b6a38e017abf0f5b8",
> "wtxid": "5dc243b1b92ee2f5a43134eb3e23449be03d1abb3d7f3c03c836ed0f13c50185",
> "allowed": false,
> "reject-reason": "insufficient fee",
> "reject-details": "insufficient fee, rejecting replacement 07d7a59a7bdad4c3a5070659ea04147c9b755ad9e173c52b6a38e017abf0f5b8; new feerate 0.00300000 BTC/kvB <= old feerate 0.00300000 BTC/kvB"
> }
ACKs for top commit:
rkrux:
re-ACK b6f0593f43
glozow:
ACK b6f0593f43
Tree-SHA512: 340b8023d59cefa84598879c4efdb7c399a3f62da126e87c595523f302e53d33098fc69da9c5f8c92b7580dc75466c66cea372051f935b197265648fe15c43a3
fa63b8232f test: generateblocks called by multiple threads (MarcoFalke)
fa62c8b1f0 rpc: Extend scope of validation mutex in generateblock (MarcoFalke)
Pull request description:
The mutex (required by TestBlockValidity) must be held after creating the block, until TestBlockValidity is called. Otherwise, it is possible that the chain advances in the meantime and leads to a crash in TestBlockValidity: `Assertion failed: pindexPrev && pindexPrev == chainstate.m_chain.Tip() (validation.cpp: TestBlockValidity: 4338)`
Fixes#31562
ACKs for top commit:
davidgumberg:
reACK fa63b8232f
achow101:
ACK fa63b8232f
ismaelsadeeq:
re-ACK fa63b8232f
mzumsande:
utACK fa63b8232f
Tree-SHA512: 3dfda1192af52546ab11fbffe44af8713073763863f4a63fbcdbdf95b1c6cbeb003dc4b8b29e7ec67362238ad15e07d8f6855832a0c68dc5370254f8cbf9445c
bc43ecaf6d test: add functional test for balance after snapshot completion (Martin Zumsande)
226d03dd61 validation: Send correct notification during snapshot completion (Martin Zumsande)
Pull request description:
After AssumeUtxo background sync is completed in a `ActivateBestChain()` call, the `GetRole()` function called with `BlockConnected()` returns `ChainstateRole::NORMAL` instead of `ChainstateRole::BACKGROUND` for this chainstate.
This would make the wallet (which ignores `BlockConnected` notifications for the background chainstate) process it, change `m_last_block_processed_height` to the (ancient) snapshot height, and display an incorrect balance.
Fix this by caching the chainstate role before calling `ActivateBestChainStep()`.
Also contains a test for this situation that fails on master.
Fixes#31546
ACKs for top commit:
fjahr:
re-ACK bc43ecaf6d
achow101:
ACK bc43ecaf6d
furszy:
Code review ACK bc43ecaf6d
TheCharlatan:
lgtm ACK bc43ecaf6d
Tree-SHA512: c5db677cf3fbab3a33ec127ec6c27c8812299e8368fd3c986bc34d0e515c4eb256f6104479f27829eefc098197de3af75d64ddca636b6b612900a0e21243e4f2
fa0998f0a0 test: Avoid intermittent error in assert_equal(pruneheight_new, 248) (MarcoFalke)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/issues/31446
The test uses the P2P network to sync blocks, which has no inherent guarantee that the blocks are sent and received in the right order, assuming the headers are received first.
This can mean that the first block file is flushed with block at height 249 and block at height 248 is added to the second file. In the log it looks like: `Leaving block file 0: CBlockFileInfo(blocks=249, size=65319, heights=0...249, time=2011-02-02...2024-12-03) (onto 1) (height 248)`. The test assumes that the height of the last pruned block in the first file is 248, expecting it to look like: `Leaving block file 0: CBlockFileInfo(blocks=249, size=65319, heights=0...248, time=2011-02-02...2024-12-09) (onto 1) (height 249) `.
Fix the issue by using a linear dumb sync.
ACKs for top commit:
achow101:
ACK fa0998f0a0
mzumsande:
Code Review ACK fa0998f0a0
i-am-yuvi:
Code Review ACK fa0998f0a0
fjahr:
Code review ACK fa0998f0a0
Tree-SHA512: 59cb4317be6cf9012c9bf7a3e9f5ba96b8b114b30bd2ac42af4fe742cd26a634d685b075f04a84bd782b2a43a342d75bb20a042bd82ad2831dbf844d39517ca2
fa6e599cf9 test: Call generate through test framework only (MarcoFalke)
Pull request description:
The generate RPCs are special in that they should only be called by the test framework itself. This way, they will call the sync function on the nodes, which can avoid intermittent test issues. Also, when the sync is disabled, it will happen explicitly by setting the `sync_fun`.
Apply this rule here, so that all generate calls are written consistently.
ACKs for top commit:
achow101:
ACK fa6e599cf9
rkrux:
tACK fa6e599cf9
hodlinator:
ACK fa6e599cf9
i-am-yuvi:
Tested ACK fa6e599cf9
Tree-SHA512: 31079997f1e17031ecd577904457e0560388aa53cadb1bbda281865271e8e4cf244bc6bf315838a717bf9d6620c201093e30039aa0007bec3629f7ca56abfba3
a2c45ae548 test: report failure during utf8 response decoding (furszy)
Pull request description:
Useful for debugging issues such https://github.com/bitcoin/bitcoin/pull/31241#issuecomment-2462816933.
Prints the entire response content instead of printing only the position of the byte it can't be decoded.
The diff between the error messages can be seen by running the `wallet_migration.py` functional test with the following patch applied:
```
diff --git a/src/wallet/rpc/wallet.cpp b/src/wallet/rpc/wallet.cpp
--- a/src/wallet/rpc/wallet.cpp(revision d65918c5da52c7d5035b4151dee9ffb2e94d4761)
+++ b/src/wallet/rpc/wallet.cpp(date 1731005254673)
@@ -801,7 +801,7 @@
}
UniValue r{UniValue::VOBJ};
- r.pushKV("wallet_name", res->wallet_name);
+ r.pushKV("wallet_name", "\xc3\x28");
if (res->watchonly_wallet) {
r.pushKV("watchonly_name", res->watchonly_wallet->GetName());
}
```
ACKs for top commit:
achow101:
ACK a2c45ae548
theStack:
re-ACK a2c45ae548
rkrux:
tACK a2c45ae548
ismaelsadeeq:
utACK a2c45ae548
Tree-SHA512: 6abb524b5a215c51ec881eea91ebe8174140a88ff3874c8c88676157edae7818801356586a904dbb21b45053183315a6d71dbf917d753611d8e413776b57c484
ecaa786cc1 rpc: add signet_challenge field to getblockchaininfo and getmininginfo (Ash Manning)
Pull request description:
Signet challenges are currently only available via `getblocktemplate` RPC.
`getblockchaininfo` and `getmininginfo` both provide inadequate information to distinguish signets. Since these are the RPCs used to determine the current network, they should also provide the signet challenge for signets.
Test coverage is included in `test/functional/feature_signet.py`.
ACKs for top commit:
sipa:
utACK ecaa786cc1
achow101:
ACK ecaa786cc1
i-am-yuvi:
Concept ACK ecaa786cc1
Sjors:
ACK ecaa786cc1
zaidmstrr:
Tested ACK [ecaa786](ecaa786cc1)
Tree-SHA512: 9ccf4ae634ee74353a2a895efb881fdc62ae703a134ccd219da2cd6080c7d38319e689054584722457a7cc79004bd6022292a3b0b90eaab9f7003564665e1ea4
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 06443b8f28
vasild:
ACK 06443b8f28
danielabrozzoni:
ACK 06443b8f28
hodlinator:
ACK 06443b8f28
Tree-SHA512: 525f4c11568616e1d48455a3fcab9e923da7432377fe9230468c15403d2e9b7ce712112df8fbd547cfec01dce0d1f26107cfc1b90f78cfc1fe13e08d57b08464
