read() fails in text mode when the unicode hasn't been fully written
yet. Fixes: "wallet_importdescriptors.py: can't decode bytes in position
228861-228863: unexpected end of data"
(https://github.com/bitcoin/bitcoin/issues/28030)
5cf44275c8ca8c32d238f37f717d78e9823f44c2 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 5cf44275c8ca8c32d238f37f717d78e9823f44c2
pinheadmz:
ACK 5cf44275c8ca8c32d238f37f717d78e9823f44c2
Tree-SHA512: 8f0e4fb2c3e0f84fac5dbc4dda87973276242b0f628034272a7f3e45434c1e17dd1b26a37edfb302dcaf380dbfe98b0417391ace5e0ac9720155d8fba702031e
faf902858d38150caa8991b0ab9d7cfee2905684 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 faf902858d38150caa8991b0ab9d7cfee2905684
Tree-SHA512: a70cd1e6cda84d542782e41e8b59741dbcd472c0d0575bcef5cbfd1418473ce94efe921481d557bae3fbbdd78f1c49c09c48872883c052d87c5c9a9a51492692
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-
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.
4f4d039a98370a91e3cd5977352a9a4b260aa06b test: add ellswift test vectors from BIP324 (stratospher)
a31287718aebad847b232eff59adc16c166c99e8 test: Add ellswift unit tests (stratospher)
714fb2c02ab4bfdd8a5a4f420036ece217c8b474 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 4f4d039a98370a91e3cd5977352a9a4b260aa06b
theStack:
ACK 4f4d039a98370a91e3cd5977352a9a4b260aa06b 🐊
Tree-SHA512: 32bc8e88f715f2cd67dc04cd38db92680872072cb3775478e2c30da89aa2da2742992779ea14da2f1faca09228942cfbd86d6957402b24bf560244b389e03540
6c97757a480b6e71a0750330d69ff18ac7cc6127 script: appease spelling linter (Jon Atack)
1316119ce7ba3de4135bbf1e5ac28c9ea26f62e1 script: update ignored-words.txt (Jon Atack)
146c861da2e4236997bee3eed6110a5016a8b86b script: update linter dependencies (Jon Atack)
92408224a4cb2f454465d5ff8445c247f2c4318a test: fix PEP484 no implicit optional argument types errors (Jon Atack)
f86a3014338de6a2204bbdda10795b75ef6654c0 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 6c97757a480b6e71a0750330d69ff18ac7cc6127
Tree-SHA512: 8a46a4d36d5978affdcecf4f2ace20ca1b52d483e098304911a2169afe60ccb9b042fa90c04b762d94f3ce53d2cafe6f24476ae839867a770c7f31e7e7242d99
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
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
aaaa3aefbdfca1c9243057eeefdc19940e60bf18 test: Use TestNode *_path properties where possible (MarcoFalke)
dddd89962b26b5593860d016586ee8feb5aeea24 test: Allow pathlib.Path as RPC argument via authproxy (MarcoFalke)
fa41614a0abc05cbfbf76d6af3a186ab8d79c3f2 scripted-diff: Use wallets_path and chain_path where possible (MarcoFalke)
fa493fadfb0ac73b7c0ee308f6623213702ae6f4 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 aaaa3aefbdfca1c9243057eeefdc19940e60bf18
theStack:
Code-review ACK aaaa3aefbdfca1c9243057eeefdc19940e60bf18 🌊
Tree-SHA512: e4720278085beb8164e1fe6c1aa18f601558a9263494ce69a83764c1487007de63ebb51d1b1151862dc4d5b49ded6162a5c1553cd30ea1c28627d447db4d8e72
d4fb58ae8ae9772d025ead184ef8f2c0ea50df3e test: EC: optimize scalar multiplication of G by using lookup table (Sebastian Falbesoner)
1830dd8820fb90bac9aea32000e47d7eb1a99e1b 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 d4fb58ae8ae9772d025ead184ef8f2c0ea50df3e
theStack:
re-ACK d4fb58ae8ae9772d025ead184ef8f2c0ea50df3e
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
32e2ffc39374f61bb2435da507f285459985df9e 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 32e2ffc39374f61bb2435da507f285459985df9e
dergoegge:
ACK 32e2ffc39374f61bb2435da507f285459985df9e
Tree-SHA512: 8cf71c5623bb642cb515531d4a2545d806e503b9d57bfc15a996597632b06103d60d985fd7f843a3c1da6528bc38d0298d6b8bcf0be6f851795a8040d71faf16
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>
Seems odd to hardcode all parent directory names in the path for no good
reason.
Also, add wallet_path property to TestNode.
Also, rework wallet_backup.py test for scripted-diff in the next commit.
In functional tests it is a quite common scenario to generate fresh
elliptic curve keypairs, which is currently a bit cumbersome as it
involves multiple steps, e.g.:
privkey = ECKey()
privkey.generate()
privkey_wif = bytes_to_wif(privkey.get_bytes())
pubkey = privkey.get_pubkey().get_bytes()
Simplify this by providing a new `generate_keypair` helper function that
returns the private key either as `ECKey` object or as WIF-string
(depending on the boolean `wif` parameter) and the public key as
byte-string; these formats are what we mostly need (currently we don't
use `ECPubKey` objects from generated keypairs anywhere).
With this, most of the affected code blocks following the pattern above
can be replaced by one-liners, e.g.:
privkey, pubkey = generate_keypair(wif=True)
Note that after this commit, the only direct uses of `ECKey` remain in
situations where we want to set the private key explicitly, e.g. in
MiniWallet (test/functional/test_framework/wallet.py) or the test for
the signet miner script (test/functional/tool_signet_miner.py).
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.
Note that given where it's used, the sandbox also gets dragged into the
kernel.
There is some related discussion in #24771.
This should not require any sort of deprecation, as this was only ever
an opt-in, experimental feature.
Closes#24771.
It seems odd to return `EXIT_SUCCESS` when the node aborted
execution due a fatal internal error or any post-init problem
that triggers an unrequested shutdown.
e.g. blocks or coins db I/O errors, disconnect block failure,
failure during thread import (external blocks loading process
error), among others.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
eefe56967b4eb4b5144325cde4f40fc1cbde3e65 bugfix: Fix incorrect debug.log config file path (Ryan Ofsky)
3746f00be1b732a04976fc70cbb0661f97bbbd99 init: Error if ignored bitcoin.conf file is found (Ryan Ofsky)
398c3719b02197ad92fded20f6ff83b364747297 lint: Fix lint-format-strings false positives when format specifiers have argument positions (Ryan Ofsky)
Pull request description:
Show an error on startup if a bitcoin datadir that is being used contains a `bitcoin.conf` file that is ignored. There are two cases where this could happen:
- One case reported in [#27246 (comment)](https://github.com/bitcoin/bitcoin/issues/27246#issuecomment-1470006043) happens when a `bitcoin.conf` file in the default datadir (e.g. `$HOME/.bitcoin/bitcoin.conf`) has a `datadir=/path` line that sets different datadir containing a second `bitcoin.conf` file. Currently the second `bitcoin.conf` file is ignored with no warning.
- Another way this could happen is if a `-conf=` command line argument points to a configuration file with a `datadir=/path` line and that path contains a `bitcoin.conf` file, which is currently ignored.
This change only adds an error message and doesn't change anything about way settings are applied. It also doesn't trigger errors if there are redundant `-datadir` or `-conf` settings pointing at the same configuration file, only if they are pointing at different files and one file is being ignored.
ACKs for top commit:
pinheadmz:
re-ACK eefe56967b4eb4b5144325cde4f40fc1cbde3e65
willcl-ark:
re-ACK eefe56967b
TheCharlatan:
ACK eefe56967b4eb4b5144325cde4f40fc1cbde3e65
Tree-SHA512: 939a98a4b271b5263d64a2df3054c56fcde94784edf6f010d78693a371c38aa03138ae9cebb026b6164bbd898d8fd0845a61a454fd996e328fd7bcf51c580c2b
To avoid `bad-txns-premature-spend-of-coinbase` error,
when getting a utxo (using `get_utxo`) to create a new
transaction `get_utxo` shouldn't return by default
immature coinbase.
This change makes the `bitcoin-wallet` binary path customizable in the
same way how it can be done now with other ones, including `bitcoind`,
`bitcoin-cli` and `bitcoin-util`.
fac395e5eb2cd3210ba6345f777a586a9bec84e3 ci: Bump ci/lint/Dockerfile (MarcoFalke)
fa6eb6516727a8675dc6e46634d8343e282528ab test: Use python3.8 pow() (MarcoFalke)
88881cf7ac029aea660c2413ca8e2a5136fcd41b Bump python minimum version to 3.8 (MarcoFalke)
Pull request description:
There is no pressing reason to drop support for 3.7, however there are several maintenance issues:
* There is no supported operating system that ships 3.7 by default. (debian:buster is EOL and unmaintained to the extent that it doesn't run in the CI environment. See https://github.com/bitcoin/bitcoin/pull/27340#issuecomment-1484988445)
* Compiling python 3.7 from source is also unsupported on at least macos, according to https://github.com/bitcoin/bitcoin/pull/24017#issuecomment-1107820790
* Recent versions of lief require 3.8, see https://github.com/bitcoin/bitcoin/pull/27507#issuecomment-1517561645
Fix all maintenance issues by bumping the minimum.
ACKs for top commit:
RandyMcMillan:
ACK fac395e
fjahr:
ACK fac395e5eb2cd3210ba6345f777a586a9bec84e3
fanquake:
ACK fac395e5eb2cd3210ba6345f777a586a9bec84e3
Tree-SHA512: c198decdbbe29d186d73ea3f6549d8a38479383495d14a965a2f9211ce39637b43f13a4c2a5d3bf56e2d468be4bbe49b4ee8e8e19ec69936ff43ddf2b714c712
These routines look fancy, but do nothing more than converting between
byte objects of length 32 to/from integers in little endian byte order
and can be replaced by simple one-liners, using the int.{from,to}_bytes
methods (available since Python 3.2).
Show an error on startup if a bitcoin datadir that is being used contains a
`bitcoin.conf` file that is ignored. There are two cases where this could
happen:
- One case reported in
https://github.com/bitcoin/bitcoin/issues/27246#issuecomment-1470006043
happens when a bitcoin.conf file in the default datadir (e.g.
$HOME/.bitcoin/bitcoin.conf) has a "datadir=/path" line that sets different
datadir containing a second bitcoin.conf file. Currently the second
bitcoin.conf file is ignored with no warning.
- Another way this could happen is if a -conf= command line argument points
to a configuration file with a "datadir=/path" line and that specified path
contains a bitcoin.conf file, which is currently ignored.
This change only adds an error message and doesn't change anything about way
settings are applied. It also doesn't trigger errors if there are redundant
-datadir or -conf settings pointing at the same configuration file, only if
they are pointing at different files and one file is being ignored.
Also, move the burden of checking for a timeout to the client and
disable the timeout on the server. This should avoid intermittent issues
in slow tests (for example mining_getblocktemplate_longpoll.py, or
feature_dbcrash.py), or possibly when the server is running slow (for
example in valgrind). There shouldn't be any downside in tests caused
by a high rpcservertimeout.
This commit updates the code by replacing the RPC call used to
decode an address and retrieve its corresponding scriptpubkey
with the address_to_scriptpubkey function. address_to_scriptpubkey
function can now decode all addresses formats, which makes
it more efficient to use.
The COINBASE_MATURITY constant in blocktools.py is imported in wallet.py.
However, importing address_to_scriptpubkey to blocktools.py will
generate a circular import error. Since the method is related to
addresses, it is best to move it to address.py, which will also
fix the circular import error.
Update imports of address_to_scriptpubkey accordingly.
d178082996dc3000f42816f89afcf3fa4d31e159 test: add bech32 decoding support to address_to_scriptpubkey() (ismaelsadeeq)
aac8793c7a2ee7630641dd74be6ba2ead50c2aee test: test_bech32_decode in address.py (ismaelsadeeq)
Pull request description:
[rpc_scantxoutset.py](e695d8536e/test/functional/rpc_scantxoutset.py (L26)) sendtodestination only sends to legacy addresses and scriptPubkeys because [wallet.py](e695d8536e/test/functional/test_framework/wallet.py (L415)) address_to_scriptpubkey does not support conversion of segwit address.
This update enables address_to_scriptpubkey to support the conversion of testnet segwit addresses to scriptPubkeys.
This change will enable [rpc_scantxoutset.py](e695d8536e/test/functional/rpc_scantxoutset.py (L22)) ScantxoutsetTest to have more test coverage by adding more sendtodestination calls with bech32 and bech32m testnet addresses, then test the bech32 and bech32m derivation subsets UTXO amount in [Test extended key derivation](e695d8536e/test/functional/rpc_scantxoutset.py (L84)).
I will add the test coverage in a subsequent Pull request.
ACKs for top commit:
josibake:
ACK d178082996
theStack:
ACK d178082996dc3000f42816f89afcf3fa4d31e159 ✔️
willcl-ark:
ACK d17808299
Tree-SHA512: 312c20ce192c648faf7dd178622700c9b871d755db56c246250e25508c3c19e7b02c0ae901dda11a1794629b9a9429c877168c05e1c4c1dbf41493316e30e7e9
Adds bech32_to_bytes() which can decode a bech32 address and return the
version as an `int` and the payload in bytes.
bech32_to_bytes() is used by the test_bech32_decode unit test to test
decoding of segwit addresses.
3dd2f6461b4bb28b2b212c691a3df28ac793ad91 test: psbt: check non-witness UTXO removal for segwit v1 input (Sebastian Falbesoner)
dd78e3fa439d57e148a2a5e312021da962c4a394 test: speedup rpc_psbt.py by whitelisting peers (immediate tx relay) (Sebastian Falbesoner)
e194e3e93dd0665181bafeb162bf4c9f3621d6f1 test: PSBT: eliminate magic numbers for global unsigned tx key (0) (Sebastian Falbesoner)
Pull request description:
This PR adds missing test coverage for dropping non-witness UTXOs from PSBTs for segwit v1+ inputs (see commit 103c6fd2791f7e73eeab7f3900fbedd5b550211d). The formerly [disabled](4600479058) method `test_utxo_conversion` is re-enabled and adapted to spend a Taproot (`bech32m`) instead of a wrapped SegWit (`p2sh-segwit`) output. Note that in contrast to the original test, we have to add the non-witness UTXO manually here using the test framework's PSBT module, since the constructing node knows that the output is segwit v1 and hence doesn't add the non-witness UTXO in the first place (see also [BIP371]( https://github.com/bitcoin/bips/blob/master/bip-0371.mediawiki#user-content-UTXO_Types)).
I strongly assume that most wallets would behave the same as Bitcoin Core here and wouldn't create PSBTs with non-witness UTXOs for Taproot inputs, but it's still good to test everything works as expected if it's still done and that the non-witness UTXO is simply dropped in that case.
The first two commits contain a small refactor (magic number elimination in PSBT module) and test speedup of ~2-3x (using whitelisting peers / immediate tx relay).
ACKs for top commit:
achow101:
ACK 3dd2f6461b4bb28b2b212c691a3df28ac793ad91
instagibbs:
ACK 3dd2f6461b4bb28b2b212c691a3df28ac793ad91
Tree-SHA512: b8d7f7ea5d7d21def024b70dfca61991cc96a4193be8857018b4d7cf3ca1465d185619fd4a77623803d9da309aa489c53273e9b7683d970ce12e2399b5b50031
fa27cf4cc7c24aa00a66dffabab849d4b3cb1c41 test: Default timeout factor to 4 under --valgrind (MarcoFalke)
Pull request description:
valgrind will incur a slowdown of at least 2, so increase the default timeout factor.
This should reduce the number of reported issues. See also https://github.com/bitcoin/bitcoin/issues/27112#issuecomment-1455762739
ACKs for top commit:
fanquake:
ACK fa27cf4cc7c24aa00a66dffabab849d4b3cb1c41 - I still see at least one actual issue when running the functional tests under `--valgrind` (outside the CI system), but will follow up separately with that. Increasing the timeout here seems fine.
Tree-SHA512: 4467559a3bfd98f5735f300f6ed54c68f951191d65a2b1294d71d72cc5d0864964de562d5dfa0a4855fc541ccb269a538b7aeb3d408d2d012a5369513397c395
