Commit graph

4983 commits

Author SHA1 Message Date
MarcoFalke
fa63326fbc
test: Fix debug_log_size helper
debug_log_bytes returned "an opaque number when in text mode"
(https://docs.python.org/3/tutorial/inputoutput.html#methods-of-file-objects),
not the number of bytes.

Fix this by using binary mode or text mode (with the same encoding)
consistently when opening the file for ftell() and read().
2023-07-20 09:15:04 +02:00
Martin Zumsande
e667bd68a1 test: fix intermittent failure in wallet_resendwallettransactions.py
Before, it was possible that a resend was triggered right between
eviction the txns from the mempool and the check that they were evicted.
2023-07-19 14:14:59 -04:00
Matthew Zipkin
ba8ab4fc54
test: cover addrv2 support in anchors.dat with a TorV3 address 2023-07-19 13:24:44 -04:00
fanquake
0be2f5481c
Merge bitcoin/bitcoin#27986: test: remove race in the user-agent reception check
20b49460b3 test: remove race in the user-agent reception check (Vasil Dimov)

Pull request description:

  In `add_p2p_connection()` we connect to `bitcoind` from the Python client and check that it has received our version string.

  This check looked up the last/newest entry from `getpeerinfo` RPC, assuming that it must be the connection we have just opened. But this will not be the case if a new inbound or outbound connection is made to/from `bitcoind` in the meantime.

  Instead of the last entry in `getpeerinfo`, check all and find the one which corresponds to our connection using our outgoing address:port tuple which is unique.

ACKs for top commit:
  jonatack:
    re-ACK 20b49460b3
  MarcoFalke:
    Concept ACK 20b49460b3

Tree-SHA512: 61fd3359ef11ea830021ede0e745497a7b60690c32d21c47cd12ff79f78907bb45e922c9d01e5d7ff614a8cd5e4560d39a3fc86b01b200429773a23ace3917e4
2023-07-19 12:32:30 +01:00
Martin Zumsande
d27b9a2248 test: fix feature_init.py file perturbation
Simultaneously opening the file in read and write mode would
lead to opening of an empty file instead of perturbing the existing
file.
Also, revert to the previous state after each perturbation so that
each perturbation is applied in isolation.
2023-07-18 15:54:23 -04:00
Andrew Chow
bc88f3ab90
Merge bitcoin/bitcoin#27997: Descriptors: rule out unspendable miniscript descriptors
c7db88af71 descriptor: assert we never parse a sane miniscript with no pubkey (Antoine Poinsot)
a49402a9ec qa: make sure we don't let unspendable Miniscript descriptors be imported (Antoine Poinsot)
639e3b6c97 descriptor: refuse to parse unspendable miniscript descriptors (Antoine Poinsot)
e3280eae1b miniscript: make GetStackSize() and GetOps() return optionals (Antoine Poinsot)

Pull request description:

  `IsSane()` in Miniscript does not ensure a Script is actually spendable. This is an issue as we would accept any sane Miniscript when parsing a descriptor. Fix this by explicitly checking a Miniscript descriptor is both sane and spendable when parsing it.

  This bug was exposed due to a check added in #22838 (https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1226859880) that triggered a fuzz crash (https://github.com/bitcoin/bitcoin/pull/22838#issuecomment-1612510057).

ACKs for top commit:
  sipa:
    utACK c7db88af71
  achow101:
    ACK c7db88af71

Tree-SHA512: e79bc9f7842e98a4e8f358f05811fca51b15b4b80a171c0d2b17cf4bb1f578a18e4397bc2ece9817d392e0de0196ee6a054b7318441fd3566dd22e1f03eb64a5
2023-07-17 19:16:09 -04:00
MarcoFalke
faf8be7c32
test: Disable known broken USDT test 2023-07-17 13:49:00 +02:00
Andrew Chow
5d8469362a test: Add helper functions for checking node versions 2023-07-14 17:03:30 -04:00
Andrew Chow
357e3f6aa4
Merge bitcoin/bitcoin#28025: test: refactor: deduplicate legacy ECDSA signing for tx inputs
5cf44275c8 test: refactor: deduplicate legacy ECDSA signing for tx inputs (Sebastian Falbesoner)

Pull request description:

  There are several instances in functional tests and the framework (MiniWallet, feature_block.py, p2p_segwit.py) where we create a legacy ECDSA signature for a certain transaction's input by doing the following steps:

  1. calculate the `LegacySignatureHash` with the desired sighash type
  2. create the actual digital signature by calling `ECKey.sign_ecdsa` on the signature message hash calculated above
  3. put the DER-encoded result as CScript data push into tx input's scriptSig

  Create a new helper `sign_input_legacy` which hides those details and takes only the necessary parameters (tx, input index, relevant scriptPubKey, private key, sighash type [SIGHASH_ALL by default]). For further convenience, the signature is prepended to already existing data-pushes in scriptSig, in order to avoid rehashing the transaction after calling the new signing function.

ACKs for top commit:
  dimitaracev:
    ACK `5cf4427`
  achow101:
    ACK 5cf44275c8
  pinheadmz:
    ACK 5cf44275c8

Tree-SHA512: 8f0e4fb2c3e0f84fac5dbc4dda87973276242b0f628034272a7f3e45434c1e17dd1b26a37edfb302dcaf380dbfe98b0417391ace5e0ac9720155d8fba702031e
2023-07-11 17:25:40 -04:00
fanquake
21ed784614
Merge bitcoin/bitcoin#28028: test: Check expected_stderr after stop
faf902858d test: Check expected_stderr after stop (MarcoFalke)

Pull request description:

  This fixes a bug where stderr wasn't checked for the shutdown sequence.

  Fix that by waiting for the shutdown to finish and then check stderr.

ACKs for top commit:
  theStack:
    ACK faf902858d

Tree-SHA512: a70cd1e6cda84d542782e41e8b59741dbcd472c0d0575bcef5cbfd1418473ce94efe921481d557bae3fbbdd78f1c49c09c48872883c052d87c5c9a9a51492692
2023-07-11 10:14:48 +01:00
Matthew Zipkin
b4bee4bbf4
test: add keep_alive option to socks5 proxy in test_framework
The Socks5 server we use in the test framework would disconnect
by default immediately after the handshake and sometimes would
not register as a connected peer by bitcoind.
2023-07-10 10:07:45 -04:00
Matthew Zipkin
5aaf988ccc
test: cover TorV3 address in p2p_addrv2_relay 2023-07-10 10:07:45 -04:00
brunoerg
80f64a3d40
test: add support for all networks in CAddress in messages.py
Also removes TorV2 from messages.py
See https://github.com/bitcoin/bitcoin/pull/22050

Co-authored-by: Matthew Zipkin <pinheadmz@gmail.com>
2023-07-10 10:07:40 -04:00
furszy
fcbdaeef4d
init: don't start indexes sync thread prematurely
By moving the 'StartIndexes()' call into the 'initload'
thread, we can remove the threads active wait. Optimizing
the available resources.

The only difference with the current state is that now the
indexes threads will only be started when they can process
work and not before it.
2023-07-10 10:50:50 -03:00
MarcoFalke
faf902858d
test: Check expected_stderr after stop 2023-07-10 13:45:50 +02:00
furszy
04575106b2
scripted-diff: rename 'loadblk' thread name to 'initload'
The thread does not only load blocks, it loads the mempool and,
in a future commit, will start the indexes as well.

Also, renamed the 'ThreadImport' function to 'ImportBlocks'
And the 'm_load_block' class member to 'm_thread_load'.

-BEGIN VERIFY SCRIPT-

sed -i "s/ThreadImport/ImportBlocks/g" $(git grep -l ThreadImport -- ':!/doc/')
sed -i "s/loadblk/initload/g" $(git grep -l loadblk -- ':!/doc/release-notes/')
sed -i "s/m_load_block/m_thread_load/g" $(git grep -l m_load_block)

-END VERIFY SCRIPT-
2023-07-07 19:31:27 -03:00
Sebastian Falbesoner
bbbb89d238 test: miner: add coverage for -blockmintxfee setting
Co-authored-by: glozow <gloriajzhao@gmail.com>
2023-07-07 15:56:24 +02:00
furszy
7ecc29a0b7
test: wallet, add coverage for addressbook migration 2023-07-06 16:11:55 -03:00
Vasil Dimov
20b49460b3
test: remove race in the user-agent reception check
In `add_p2p_connection()` we connect to `bitcoind` from the Python
client and check that it has received our version string.

This check looked up the last/newest entry from `getpeerinfo` RPC,
assuming that it must be the connection we have just opened. But this
will not be the case if a new inbound or outbound connection is made
to/from `bitcoind` in the meantime.

Instead of the last entry in `getpeerinfo`, check all and find the one
which corresponds to our connection using our outgoing address:port
tuple which is unique.

Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
Co-authored-by: Jon Atack <jon@atack.com>
2023-07-06 17:48:15 +02:00
glozow
ecf3baffc0
Merge bitcoin/bitcoin#27869: wallet: Give deprecation warning when loading a legacy wallet
8fbb6e99bf wallet: Give deprecation warning when loading a legacy wallet (Andrew Chow)

Pull request description:

  Next step in legacy wallet deprecation.

ACKs for top commit:
  S3RK:
    reACK 8fbb6e99bf
  jonatack:
    re-ACK 8fbb6e99bf

Tree-SHA512: 902984b09452926cf199f06e5fb56e4985325cdd5e0dcc829992158488f42d5fbc33e9a30a29303feac24c8315193e8d31712022e2a0503abd6b67169a0027f4
2023-07-06 10:47:41 +01:00
Sebastian Falbesoner
5cf44275c8 test: refactor: deduplicate legacy ECDSA signing for tx inputs
There are several instances in functional tests and the framework
(MiniWallet, feature_block.py, p2p_segwit.py) where we create a legacy
ECDSA signature for a certain transaction's input by doing the following
steps:
    1) calculate the `LegacySignatureHash` with the desired sighash type
    2) create the actual digital signature by calling `ECKey.sign_ecdsa`
       on the signature message hash calculated above
    3) put the DER-encoded result as CScript data push into
       tx input's scriptSig

Create a new helper `sign_input_legacy` which hides those details and
takes only the necessary parameters (tx, input index, relevant
scriptPubKey, private key, sighash type [SIGHASH_ALL by default]). For
further convenience, the signature is prepended to already existing
data-pushes in scriptSig, in order to avoid rehashing the transaction
after calling the new signing function.
2023-07-03 17:33:41 +02:00
Antoine Poinsot
a49402a9ec
qa: make sure we don't let unspendable Miniscript descriptors be imported 2023-07-01 12:12:26 +02:00
fanquake
61d59fed74
Merge bitcoin/bitcoin#24005: test: add python implementation of Elligator swift
4f4d039a98 test: add ellswift test vectors from BIP324 (stratospher)
a31287718a test: Add ellswift unit tests (stratospher)
714fb2c02a test: Add python ellswift implementation to test framework (stratospher)

Pull request description:

  Built on top of https://github.com/bitcoin/bitcoin/pull/26222.

  This PR introduces Elligator swift encoding and decoding in the functional test framework. It's used in #24748 for writing p2p encryption tests.

ACKs for top commit:
  sipa:
    ACK 4f4d039a98
  theStack:
    ACK 4f4d039a98 🐊

Tree-SHA512: 32bc8e88f715f2cd67dc04cd38db92680872072cb3775478e2c30da89aa2da2742992779ea14da2f1faca09228942cfbd86d6957402b24bf560244b389e03540
2023-06-30 19:30:49 +01:00
fanquake
3367e1c850
Merge bitcoin/bitcoin#28009: script, test: python typing and linter updates
6c97757a48 script: appease spelling linter (Jon Atack)
1316119ce7 script: update ignored-words.txt (Jon Atack)
146c861da2 script: update linter dependencies (Jon Atack)
92408224a4 test: fix PEP484 no implicit optional argument types errors (Jon Atack)
f86a301433 script, test: add missing python type annotations (Jon Atack)

Pull request description:

  With these updates, `./test/lint/lint-python.py` and `./test/lint/lint-spelling.py` should be green again for developers using relatively recent Python dependencies, in particular mypy 0.991 (released 11/2022) and later. Please see the commit messages for details.

ACKs for top commit:
  fanquake:
    ACK 6c97757a48

Tree-SHA512: 8a46a4d36d5978affdcecf4f2ace20ca1b52d483e098304911a2169afe60ccb9b042fa90c04b762d94f3ce53d2cafe6f24476ae839867a770c7f31e7e7242d99
2023-06-30 16:20:37 +01:00
Jon Atack
92408224a4 test: fix PEP484 no implicit optional argument types errors
Fix warnings for these files when ./test/lint/lint-python.py is run using
mypy 0.991 (released 11/2022) and later:

$ test/lint/lint-python.py
test/functional/test_framework/coverage.py:23: error: Incompatible default for argument "coverage_logfile" (default has type "None", argument has type "str")  [assignment]
test/functional/test_framework/coverage.py:23: note: PEP 484 prohibits implicit Optional. Accordingly, mypy has changed its default to no_implicit_optional=True
test/functional/test_framework/util.py:318: error: Incompatible default for argument "timeout" (default has type "None", argument has type "int")  [assignment]
test/functional/test_framework/util.py:318: note: PEP 484 prohibits implicit Optional. Accordingly, mypy has changed its default to no_implicit_optional=True
test/functional/test_framework/util.py:318: error: Incompatible default for argument "coveragedir" (default has type "None", argument has type "str")  [assignment]
test/functional/interface_rest.py:67: error: Incompatible default for argument "query_params" (default has type "None", argument has type "dict[str, Any]")  [assignment]
test/functional/interface_rest.py:67: note: PEP 484 prohibits implicit Optional. Accordingly, mypy has changed its default to no_implicit_optional=True

Verified using https://github.com/hauntsaninja/no_implicit_optional

For details, see:

https://mypy-lang.blogspot.com/2022/11/mypy-0990-released.html
2023-06-29 16:14:07 -06:00
Jon Atack
f86a301433 script, test: add missing python type annotations
Fix warnings for these files when ./test/lint/lint-python.py is run using
mypy 0.991 (released 11/2022) and later:

"By default the bodies of untyped functions are not checked, consider using
--check-untyped-defs [annotation-unchecked]"

For details, see:

https://mypy-lang.blogspot.com/2022/11/mypy-0990-released.html
2023-06-29 16:13:51 -06:00
stratospher
4f4d039a98 test: add ellswift test vectors from BIP324
The test vector input file is taken from:
1. https://github.com/bitcoin/bips/blob/master/bip-0324/xswiftec_inv_test_vectors.csv
2. https://github.com/bitcoin/bips/blob/master/bip-0324/ellswift_decode_test_vectors.csv

Co-authored-by: theStack <sebastian.falbesoner@gmail.com>
2023-06-29 23:32:56 +05:30
stratospher
a31287718a test: Add ellswift unit tests
remove util also since unit tests there were removed in #27538

Co-authored-by: theStack <sebastian.falbesoner@gmail.com>
2023-06-29 23:32:56 +05:30
stratospher
714fb2c02a test: Add python ellswift implementation to test framework
Co-authored-by: Pieter Wuille <pieter.wuille@gmail.com>
2023-06-29 23:32:54 +05:30
MarcoFalke
fabd34873c
test: Rename EncodeDecimal to serialization_fallback
The new name better explains that the function handles fallbacks,
without listing all in the function name.
2023-06-29 19:51:43 +02:00
fanquake
e8543629ae
Merge bitcoin/bitcoin#27884: test: Use TestNode datadir_path or chain_path where possible
aaaa3aefbd test: Use TestNode *_path properties where possible (MarcoFalke)
dddd89962b test: Allow pathlib.Path as RPC argument via authproxy (MarcoFalke)
fa41614a0a scripted-diff: Use wallets_path and chain_path where possible (MarcoFalke)
fa493fadfb test: Use wallet_dir lambda in wallet_multiwallet test where possible (MarcoFalke)

Pull request description:

  It seems inconsistent, fragile and verbose to:

  * Call `get_datadir_path` to recreate the path that already exists as field in TestNode
  * Call `os.path.join` with the hardcoded chain name or `self.chain` to recreate the TestNode `chain_path` property
  * Sometimes even use the hardcoded node dir name (`"node0"`)

  Fix all issues by using the TestNode properties.

ACKs for top commit:
  willcl-ark:
    re-ACK aaaa3aefbd
  theStack:
    Code-review ACK aaaa3aefbd 🌊

Tree-SHA512: e4720278085beb8164e1fe6c1aa18f601558a9263494ce69a83764c1487007de63ebb51d1b1151862dc4d5b49ded6162a5c1553cd30ea1c28627d447db4d8e72
2023-06-29 09:51:53 +01:00
Andrew Chow
626d346469
Merge bitcoin/bitcoin#26222: Introduce secp256k1 module with field and group classes to test framework
d4fb58ae8a test: EC: optimize scalar multiplication of G by using lookup table (Sebastian Falbesoner)
1830dd8820 test: add secp256k1 module with FE (field element) and GE (group element) classes (Pieter Wuille)

Pull request description:

  This PR rewrites a portion of `test_framework/key.py`, in a compatible way, by introducing classes that encapsulate field element and group element logic, in an attempt to be more readable and reusable.

  To maximize readability, the group element logic does not use Jacobian coordinates. Instead, group elements just store (affine) X and Y coordinates directly. To compensate for the performance loss this causes, field elements are represented as fractions. This undoes most, but not all, of the performance loss, and there is a few % slowdown (as measured in `feature_taproot.py`, which heavily uses this).

  The upside is that the implementation for group laws (point doubling, addition, subtraction, ...) is very close to the mathematical description of elliptic curves, and this extends to potential future extensions (e.g. ElligatorSwift as needed by #27479).

ACKs for top commit:
  achow101:
    ACK d4fb58ae8a
  theStack:
    re-ACK d4fb58ae8a
  stratospher:
    tested ACK d4fb58a. really liked how this PR makes the secp256k1 code in the tests more intuitive and easier to follow!

Tree-SHA512: 9e0d65d7de0d4fb35ad19a1c19da7f41e5e1db33631df898c6d18ea227258a8ba80c893dab862b0fa9b0fb2efd0406ad4a72229ee26d7d8d733dee1d56947f18
2023-06-28 16:27:55 -04:00
Andrew Chow
caff95a023
Merge bitcoin/bitcoin#27896: Remove the syscall sandbox
32e2ffc393 Remove the syscall sandbox (fanquake)

Pull request description:

  After initially being merged in #20487, it's no-longer clear that an internal syscall sandboxing mechanism is something that Bitcoin Core should have/maintain, especially when compared to better maintained/supported alterantives, i.e [firejail](https://github.com/netblue30/firejail).

  There is more related discussion in #24771.

  Note that given where it's used, the sandbox also gets dragged into the kernel.

  If it's removed, this should not require any sort of deprecation, as this was only ever an opt-in, experimental feature.

  Closes #24771.

ACKs for top commit:
  davidgumberg:
     crACK 32e2ffc393
  achow101:
    ACK 32e2ffc393
  dergoegge:
    ACK 32e2ffc393

Tree-SHA512: 8cf71c5623bb642cb515531d4a2545d806e503b9d57bfc15a996597632b06103d60d985fd7f843a3c1da6528bc38d0298d6b8bcf0be6f851795a8040d71faf16
2023-06-27 18:19:21 -04:00
Sebastian Falbesoner
d4fb58ae8a test: EC: optimize scalar multiplication of G by using lookup table
On my machine, this speeds up the functional test feature_taproot.py by
a factor of >1.66x (runtime decrease from 1m16.587s to 45.334s).

Co-authored-by: Pieter Wuille <pieter@wuille.net>
2023-06-27 09:34:52 -04:00
Pieter Wuille
1830dd8820 test: add secp256k1 module with FE (field element) and GE (group element) classes
These are primarily designed for ease of understanding, not performance.
2023-06-27 09:34:48 -04:00
Andrew Chow
e8c31f135c tests: Test for bumping single output transaction 2023-06-26 17:56:50 -04:00
Andrew Chow
4f4d4407e3 test: Test bumpfee reduce_output 2023-06-26 17:56:50 -04:00
Andrew Chow
8847497161
Merge bitcoin/bitcoin#27631: test: avoid sporadic MINIMALDATA failure in feature_taproot.py (fixes #27595)
54877253c8 test: avoid sporadic MINIMALDATA failure in feature_taproot.py (fixes #27595) (Sebastian Falbesoner)

Pull request description:

  The functional test feature_taproot.py fails in some rare cases on the execution of the following `"branched_codesep"` spending script (can be reproduced via `$ ./test/functional/feature_taproot.py --randomseed 9048710178866422833` on master / 137a98c5a2):

  9d85c03620/test/functional/feature_taproot.py (L741)

  The problem occurs if the first data-push (having random content with a random length in the range [0, 510]) has a length of 1 and the single byte has value of [1...16] or [-1]; in this case, the data-push is not minimally encoded by test framework's CScript class (i.e. doesn't use the special op-codes OP_1...OP_16 or OP_1NEGATE) and the script interpreter throws an SCRIPT_ERR_MINIMALDATA error:

  ```
  test_framework.authproxy.JSONRPCException: non-mandatory-script-verify-flag (Data push larger than necessary) (-26)
  ```

  Background: the functional test framework's CScript class translates passed bytes/bytearrays always to data pushes using OP_PUSHx/OP_PUSHDATA{1,2,4} op-codes (see `CScript.__coerce_instance(...)`). E.g. the expression `CScript(bytes([1]))` yields `bytes([OP_PUSH1, 1])` instead of the minimal-encoded `bytes([OP_1])`.

  Fix this by adapting the random-size range to [2,...], i.e. never pass byte-arrays below length two to be pushed.

  Closes #27595.

ACKs for top commit:
  instagibbs:
    ACK 54877253c8
  sipa:
    utACK 54877253c8
  achow101:
    ACK 54877253c8

Tree-SHA512: 3ffad89b2c3985c20702242192e744c9b10188bff880efaf3c38424a00fa07bd4608d8c948678ff9cdbb4e1e5b06696c7f55407ee10bb05edbb3ee03aa599cdc
2023-06-23 18:54:06 -04:00
Andrew Chow
035ae61c5a
Merge bitcoin/bitcoin#27577: p2p: give seednodes time before falling back to fixed seeds
30778124b8 net: Give seednodes time before falling back to fixed seeds (Martin Zumsande)

Pull request description:

  `-seednode` is an alternative bootstrap mechanism - when choosing it, we make a `AddrFetch` connection to the specified peer, gather addresses from them, and then disconnect. Presumably, if users specify a seednode they prefer addresses from that node over fixed seeds.

  However, when disabling dns seeds and specifiying `-seednode`, `CConnman::ProcessAddrFetch()`  immediately removes the entry from `m_addr_fetches` (before the seednode could give us addresses) - and once `m_addr_fetches`  is empty, `ThreadOpenConnections` will add fixed seeds, resulting in a "race" between the fixed seeds and seednodes filling up AddrMan.

  This PR suggests to check for any provided `-seednode` arg instead of using the size of `m_addr_fetches`, thus delaying the querying of fixed seeds for 1 minute when specifying any seednode (as we already do for `addnode` peers).
  That way, we actually give the seednodes a chance for  to provide us with addresses before falling back to fixed seeds.

  This can be tested with `bitcoind -debug=net -dnsseed=0 -seednode=(...)` on a node without `peers.dat` and observing the debug log.

ACKs for top commit:
  ajtowns:
    utACK 30778124b8
  achow101:
    ACK 30778124b8
  dergoegge:
    Code review ACK 30778124b8
  sr-gi:
    ACK [3077812](30778124b8) with a tiny nit, feel free to ignore it

Tree-SHA512: 96446eb34c0805f10ee158a00a3001a07029e795ac40ad5638228d426e30e9bb836c64ac05d145f2f9ab23ec5a528f3a416e3d52ecfdfb0b813bd4b1ebab3c01
2023-06-23 17:39:58 -04:00
Andrew Chow
8fbb6e99bf wallet: Give deprecation warning when loading a legacy wallet 2023-06-23 16:37:22 -04:00
stickies-v
9f55773a37
test: refactor: usdt_mempool: store all events
Even though we expect these functions to only produce one event,
we still keep a counter to check if that's true. By simply storing
all the events, we can remove the counters and make debugging
easier, by allowing pdb to access the events.
2023-06-23 16:47:43 +01:00
stickies-v
bc43270450
test: refactor: remove unnecessary nonlocal
Since we're only mutating, and not reassigning, we don't need to
declare `events` as `nonlocal`.
2023-06-23 16:47:43 +01:00
stickies-v
326db63a68
test: log sanity check assertion failures 2023-06-23 16:47:43 +01:00
stickies-v
f5525ad680
test: store utxocache events
By storing the events instead of doing the comparison inside the
handle_utxocache_* functions, we simplify the overall logic and
potentially making debugging easier, by allowing pdb to access the
events.

Mostly a refactor, but changes logging behaviour slightly by not
raising and not calling self.log.exception("Assertion failed")
2023-06-23 16:46:46 +01:00
stickies-v
f1b99ac94f
test: refactor: deduplicate handle_utxocache_* logic
Carve out the comparison logic into a helper function to avoid
code duplication.
2023-06-23 16:46:32 +01:00
stickies-v
ad90ba36bd
test: refactor: rename inbound to is_inbound
Makes it easier to recognize this variable represents a flag.
2023-06-23 16:46:15 +01:00
stickies-v
afc0224cdb
test: refactor: remove unnecessary blocks_checked counter
Since we already store all the blocks in `events`, keeping an
additional counter is redundant.
2023-06-23 16:46:02 +01:00
MarcoFalke
fa748c6f2a
test: Fix intermittent issue in mining_getblocktemplate_longpoll.py 2023-06-23 15:21:29 +02:00
fanquake
a7261da479
Merge bitcoin/bitcoin#27831: test: handle failed assert_equal() assertions in bcc callback functions
61f4b9b7ad Manage exceptions in bcc callback functions (virtu)

Pull request description:

  Address #27380 (and similar future issues) by handling failed `assert_equal()` assertions in bcc callback functions

  ### Problem

  Exceptions are not propagated in ctype callback functions used by bcc. This means an AssertionError exception raised by `assert_equal()` to signal a failed assertion is not getting caught and properly logged. Instead, the error is logged to stdout and execution of the callback stops.

  The current workaround to check whether all `assert_equal()` assertions in a callback succeeded is to increment a success counter after the assertions (which only gets incremented if none exception is raised and stops execution). Then, outside the callback, the success counter can be used to check whether a callback executed successfully.

  One issue with the described workaround is that when an exception occurs, there is no way of telling which of the `assert_equal()` statements caused the exception; moreover, there is no way of inspecting how the pieces of data that got compared in `assert_equal()` differed (often a crucial clue when debugging what went wrong).

  This problem is happening in #27380: Sporadically, in the `mempool:rejected` test, execution does not reach the end of the callback function and the success counter is not incremented. Thus, the test fails when comparing the counter to its expected value of one. Without knowing which of the asserts failed any why it failed, this issue is hard to debug.

  ### Solution

  Two fixes come to mind. The first involves having the callback function make event data accessible outside the callback and inspecting the event using `assert_equal()` outside the callback. This solution still requires a counter in the callback in order  to tell whether a callback was actually executed or if instead the call to perf_buffer_poll() timed out.

  The second fix entails wrapping all relevant `assert_equal()` statements inside callback functions into try-catch blocks and manually logging AssertionErrors. While not as elegant in terms of design, this approach can be more pragmatic for more complex tests (e.g., ones involving multiple events, events of different types, or the order of events).

  The solution proposed here is to select the most pragmatic fix on a case-by-case basis: Tests in `interface_usdt_net.py`, `interface_usdt_mempool.py` and `interface_usdt_validation.py` have been refactored to use the first approach, while the second approach was chosen for `interface_usdt_utxocache.py` (partly to provide a reference for the second approach, but mainly because the utxocache tests are the most intricate tests, and refactoring them to use the first approach would negatively impact their readability). Lastly, `interface_usdt_coinselection.py` was kept unchanged because it does not use `assert_equal()` statements inside callback functions.

ACKs for top commit:
  0xB10C:
    Reviewed the changes since my last review. ACK 61f4b9b7ad. I've tested that the combined log contains both exceptions by modifying `interface_usdt_utxocache.py`.
  willcl-ark:
    utACK 61f4b9b
  stickies-v:
    utACK 61f4b9b7a

Tree-SHA512: 85cdaabf370d4f09a9eab6af9ce7c796cd9d08cb91f38f021f71adda34c5f643331022dd09cadb95be2185dad6016c95cbb8942e41e4fbd566a49bf431c5141a
2023-06-22 16:08:15 +01:00
kevkevin
d05be124db
test: added coverage to estimatefee
Added a assert for an rpc error when we try to estimate fee for the max
conf_target
2023-06-22 08:27:38 -05:00