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
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.
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.
