4ef6253017672a74c584e6e9b449ffff79f67273 test: avoid unneeded (w)txid hex -> integer conversions (Sebastian Falbesoner)
472f3770aec89bf0783609094108ba277a46d339 scripted-diff: test: rename CTransaction `.getwtxid()` -> `wtxid_hex` for consistency (Sebastian Falbesoner)
81af4334e8f903601dac503426eaccc1d3527f9c test: rename CTransaction `.sha256` -> `.txid_int` for consistency (Sebastian Falbesoner)
ce83924237127fe6d32b63c362b57789e4628954 test: rename CTransaction `.rehash()`/`.hash` -> `.txid_hex` for consistency (Sebastian Falbesoner)
e9cdaefb0a80988cfba2dc5ddd96b84a204d5519 test: introduce and use CTransaction `.wtxid_int` property (Sebastian Falbesoner)
9b3dce24a3336a02563412b541a3fb2003e92506 test: remove bare CTransaction `.rehash()`/`.calc_sha256()` calls (Sebastian Falbesoner)
a2724e3ea392cdbd5526735516862b17bf687624 test: remove txid caching in CTransaction class (Sebastian Falbesoner)
Pull request description:
In the functional test framework, determining a (w)txid for a `CTransaction` instance is currently rather confusing and footgunny due to inconsistent naming/interfaces (see table below) and statefulness involved. This PR aims to improve that by:
* removing the (w)txid caching mechanism, in order to avoid the need to call additional rehashing functions (`.rehash()`/`.calculate_sha256()`, see first two commits and https://github.com/bitcoin/bitcoin/pull/32050#discussion_r1993286997). This change in theory decreases the performance, as the involved serialization and hashing involved might be called more often than previously, but I couldn't find a functional test where this leads to a measurable run-time increase on my machine.
* introduce consistent naming that shows the type of the returned txid, i.e. hex string vs. test-framework-internal representation [currently integers] (see remaining commits)
Summary table showing (w)txid determaination before/after this PR:
| Task | master | PR |
|:-----------------------|:-----------------------|:-------------|
| get TXID (hex string) | `.rehash()` / `.hash`[1] | `.txid_hex` |
| get TXID (integer) | `.sha256`[1] | `.txid_int` |
| get WTXID (hex string) | `.getwtxid()` | `.wtxid_hex` |
| get WTXID (integer) | `.calc_sha256(True)` | `.wtxid_int` |
Unfortunately, most renames can't be done with a scripted-diff, as the property names (`.hash`, `.sha256`) are also used for blocks and other message types. The PR is rather invasive and touches a lot of files, but I think it's worth to do it, also to make life easier for new contributors. Future tasks like e.g. doing the same overhaul for block (header) objects or getting rid of the integer representation (see https://github.com/bitcoin/bitcoin/pull/32050) become easier should become easier after this one.
[1] = returned value might be out-of-date, if rehashing function wasn't called after modification
ACKs for top commit:
maflcko:
re-ACK 4ef6253017672a74c584e6e9b449ffff79f67273 🏈
achow101:
ACK 4ef6253017672a74c584e6e9b449ffff79f67273
marcofleon:
code review ACK 4ef6253017672a74c584e6e9b449ffff79f67273
Tree-SHA512: 4b472c31d169966b6f6878911a8404d25bf3e503b6e8ef30f36a7415d21ad4bc1265083af2d3ead6edfcd9fac9ccb0a8be57e1b0739ad431b836413070d7d583
f16c8c67bf137e64fbeea1242431baaa915a5c53 tests: Expand HTTP coverage to assert libevent behavior (Matthew Zipkin)
Pull request description:
These commits are cherry-picked from #32061 and part of a project to [remove libevent](https://github.com/bitcoin/bitcoin/issues/31194).
This PR only adds functional tests to `interface_http` to cover some HTTP server behaviors we inherit from libevent, in order to maintain those behaviors when we replace libevent with our own HTTP server.
1. Pipelining: The server must respond to requests from a client in the order in which they were received [RFC 7230 6.3.2](https://www.rfc-editor.org/rfc/rfc7230#section-6.3.2)
2. `-rpcservertimeout` config option which sets the amount of time the server will keep an idle client connection alive
3. "Chunked" Transfer-Encoding: Allows a client to send a request in pieces, without the `Content-Length` header [RFC 7230 4.1](https://www.rfc-editor.org/rfc/rfc7230#section-4.1)
ACKs for top commit:
achow101:
ACK f16c8c67bf137e64fbeea1242431baaa915a5c53
vasild:
ACK f16c8c67bf137e64fbeea1242431baaa915a5c53
polespinasa:
ACK f16c8c67bf137e64fbeea1242431baaa915a5c53
fjahr:
utACK f16c8c67bf137e64fbeea1242431baaa915a5c53
Tree-SHA512: 405b59431b4d2bf118fde04b270865dee06ef980ab120d9cc1dce28e5d65dfd880a57055b407009d22f4de614bc3eebdb3e203bcd39e86cb14fbfd62195ed06a
Since the previous commit, CTransaction object calls to the
methods `.rehash()` and `.calc_sha256()` are effectively no-ops
if the returned value is not used, so we can just remove them.
Rather than txids (represented by the fields `.sha256` and `.hash`)
being stateful, simply compute them on-the-fly. This ensures that
the correct values are always returned and takes the burden of
rehashing from test writers, making the code shorter overall.
In a first step, the fields are kept at the same name with @property
functions as drop-in replacements, for a minimal diff. In later commits,
the names are changed to be more descriptive and indicating the return
type of the txid.
a189d636184b1c28fa4a325b56c1fab8f44527b1 add release note for datacarriersize default change (Greg Sanders)
a141e1bf501bb2660f3a62083a65678250085e56 Add more OP_RETURN mempool acceptance functional tests (Peter Todd)
0b4048c73385166144d0b3e76beb9a2ac4cc1eca datacarrier: deprecate startup arguments for future removal (Greg Sanders)
63091b79e70b8e230a122fa6fb3dac91c80638e7 test: remove unnecessary -datacarriersize args from tests (Greg Sanders)
9f36962b07eff2369577a17c8adeaa0433697e1c policy: uncap datacarrier by default (Greg Sanders)
Pull request description:
Retains the `-datacarrier*` args, marks them as deprecated, and does not require another startup argument for multiple OP_RETURN outputs.
If a user has set `-datacarriersize` the value is "budgeted" across all seen OP_RETURN output scriptPubKeys. In other words the total script bytes stays the same, but can be spread across any number of outputs. This is done to not introduce an additional argument to support multiple outputs.
I do not advise people use the option with custom arguments and it is marked as deprecated to not mislead as a promise to offer it forever. The argument itself can be removed in some future release to clean up the code and minimize footguns for users.
ACKs for top commit:
stickies-v:
re-ACK a189d636184b1c28fa4a325b56c1fab8f44527b1
Sjors:
re-ACK a189d636184b1c28fa4a325b56c1fab8f44527b1
polespinasa:
re-ACK a189d636184b1c28fa4a325b56c1fab8f44527b1
hodlinator:
re-ACK a189d636184b1c28fa4a325b56c1fab8f44527b1
ajtowns:
reACK a189d636184b1c28fa4a325b56c1fab8f44527b1
mzumsande:
re-ACK a189d636184b1c28fa4a325b56c1fab8f44527b1
petertodd:
ACK a189d636184b1c28fa4a325b56c1fab8f44527b1
theStack:
re-ACK a189d636184b1c28fa4a325b56c1fab8f44527b1
1440000bytes:
re-ACK a189d636184b1c28fa4a325b56c1fab8f44527b1
willcl-ark:
ACK a189d636184b1c28fa4a325b56c1fab8f44527b1
dergoegge:
ACK a189d636184b1c28fa4a325b56c1fab8f44527b1
fanquake:
ACK a189d636184b1c28fa4a325b56c1fab8f44527b1
murchandamus:
ACK a189d636184b1c28fa4a325b56c1fab8f44527b1
darosior:
Concept ACK a189d636184b1c28fa4a325b56c1fab8f44527b1.
Tree-SHA512: 3da2f1ef2f50884d4da7e50df2121bf175cb826edaa14ba7c3068a6d5b2a70beb426edc55d50338ee1d9686b9f74fdf9e10d30fb26a023a718dd82fa1e77b038
6ee32aaaca4a46d42594bc16479868c76cc67fd8 test: signet tool genpsbt and solvepsbt commands (Sjors Provoost)
0a99d99fe4cb8432945d37c8e5d221715c538e4f signet: miner skips PSBT step for OP_TRUE (Sjors Provoost)
cdfb70e5a6a9235ea18dd2cb8148b757902dc08e signet: split decode_psbt miner helper (Sjors Provoost)
Pull request description:
[BIP325](https://github.com/bitcoin/bips/blob/master/bip-0325.mediawiki) mentions the following rule:
> In the special case where an empty solution is valid (ie scriptSig and scriptWitness are both empty) this additional commitment can optionally be left out. This special case is to allow non-signet-aware block generation code to be used to test a custom signet chain where the challenge is trivially true.
Such a signet can be created using e.g. `-signetchallenge=51` (`OP_TRUE`). However `contrib/signet/miner` won't omit the commitment.
This PR improves the miner by skipping the PSBT for known trivial scripts (just `OP_TRUE` and trivial pushes for now). This prevents it from appending the 4 byte signet header to the witness commitment, as allowed by the above rule.
---
Previously the script would fail with `PSBT signing failed`, making it difficult to mine. This is no longer the case.
ACKs for top commit:
achow101:
ACK 6ee32aaaca4a46d42594bc16479868c76cc67fd8
theStack:
re-ACK 6ee32aaaca4a46d42594bc16479868c76cc67fd8
danielabrozzoni:
ACK 6ee32aaaca4a46d42594bc16479868c76cc67fd8
Tree-SHA512: e47fbf471f2909286a6c1c073799ea388b9c19551afcce96cf9af45cc48d25c02f1e48e08861a88b604361e2c107a759d5baf393da8a37360de419f31651758a
Datacarrier output script sizes and output counts are now
uncapped by default.
To avoid introducing another startup argument, we modify the
OP_RETURN accounting to "budget" the spk sizes.
If a user has set a custom default, this results in that
budget being spent over the sum of all OP_RETURN outputs'
scripts in the transaction, no longer capping the number
of OP_RETURN outputs themselves. This should allow a
superset of current behavior while respecting the passed
argument in terms of total arbitrary data storage.
Co-authored-by: Anthony Towns <aj@erisian.com.au>
b1ea542ae651cec433910d8c727abc41f17a7678 test: test MAX_SCRIPT_SIZE for block validity (Greg Sanders)
Pull request description:
I don't believe there are direct tests for this.
ACKs for top commit:
achow101:
ACK b1ea542ae651cec433910d8c727abc41f17a7678
TheCharlatan:
ACK b1ea542ae651cec433910d8c727abc41f17a7678
theStack:
ACK b1ea542ae651cec433910d8c727abc41f17a7678
Tree-SHA512: 1d7d3eab9c54977844bf2ca1aa403b070aae0f818db2fb5cae367d1c4d12f1e403b6fdec224af769a2ebb648cbca8bfd0d7df5db2a89fccf256c9c244484eba2
a5ac43d98d1ad3ebed934f2c50208a85aae17e5e doc: Add release notes describing bitcoin wrapper executable (Ryan Ofsky)
258bda80c009a25d1f1bdeffccf9ed1ffde29cb2 doc: Mention bitcoin wrapper executable in documentation (Ryan Ofsky)
d2739d75c911c8bf73a4d3005c57add1ae4a67ae build: add bitcoin.exe to windows installer (Sjors Provoost)
ba649c00063a43b59a63db17b509179a658a8d9a ci: Run multiprocess tests through wrapper executable (Ryan Ofsky)
29bdd743bb843f8b8ed2e426b6df36e9d7e54215 test: Support BITCOIN_CMD environment variable (Ryan Ofsky)
9c8c68891b43053acfe7b8eb9d2e0d2bcfcb4e1e multiprocess: Add bitcoin wrapper executable (Ryan Ofsky)
5076d20fdb70a4bfafc4bdfe8293e347cb6bfa78 util: Add cross-platform ExecVp and GetExePath functions (Ryan Ofsky)
Pull request description:
Intended to make bitcoin command line features more discoverable and allow installing new multiprocess binaries in libexec/ instead of bin/ so they don't cause confusion.
Idea and implementation of this were discussed in https://github.com/bitcoin/bitcoin/issues/30983.
---
Initial implementation of this feature is deliberately minimal so the UX can evolve in response to feedback and there are not too many details to debate and discuss in a single PR. But many improvements are possible or planned:
- Adding manpage and bash completions.
- Showing nicer error messages that detect if an executable isn't installed and suggest how to fix [(comment)](https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2073194474)
- Showing wrapper command lines in subcommand in help output [(comment)](https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2077800405). This could be done conditionally as suggested in the comment or be unconditional.
- Showing wrapper command lines in subcommand error output. There is a bitcoin-cli error pointed out in [(comment)](https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2091152243) that is needlessly confusing.
- Integrating help so `bitcoin help subcommand` invokes `bitcoin subcommand -h`. `bitcoin -h subcommand` should also be supported and be equivalent [(comment)](https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2093116725)
- Adding support for `bitcoin-util` subcommands. Ideal interface would probably be more like `bitcoin grind` not `bitcoin util grind` but this has been punted for now. Supporting subcommands directly would require some ArgsManager modifications
- Adding a dedicated python functional test for the wrapper. Right now there is some CI coverage by setting the `BITCOIN_CMD` variable, but this doesn't cover things like the help output and version output, and support for different directory layouts.
- Better `--multiprocess` (`-m`) / `--monolithic` (`-M`) default selection. Right now, default is monolithic but it probably makes sense to chose more intelligently depending on whether -ipc options are enabled and what binaries are available.
- Maybe parsing `bitcoin.conf` and supporting options to control wrapper behavior like custom locations or preferences or aliases.
- Better command command line usability. Allow combining short options like (`-ah`). Allow fuzzy matching of subcommands or suggestions if you misspell. (suggested by stickies in review club)
- Not directly related to this PR but `bitcoin-cli named` implementation used by the wrapper should do a better job disambiguating named arguments from base64 arguments ending in = as pointed out in [(comment)](https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2091886628)
---
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/issues/28722). A review club meeting for it took place in https://bitcoincore.reviews/31375
ACKs for top commit:
Sjors:
utACK a5ac43d98d1ad3ebed934f2c50208a85aae17e5e
achow101:
ACK a5ac43d98d1ad3ebed934f2c50208a85aae17e5e
vasild:
ACK a5ac43d98d1ad3ebed934f2c50208a85aae17e5e
theStack:
ACK a5ac43d98d1ad3ebed934f2c50208a85aae17e5e
ismaelsadeeq:
fwiw my last review implied an ACK a5ac43d98d1ad3ebed934f2c50208a85aae17e5e
hodlinator:
ACK a5ac43d98d1ad3ebed934f2c50208a85aae17e5e
Tree-SHA512: 570e6a4ff8bd79ef6554da3d01f36c0a7c6d2dd7dace8f8732eca98f4a8bc2284474a9beadeba783114fe2f3dd08b2041b3da7753bae0b7f881ec50668cb821f
7bc64a8859c3644fdf2eeff59f72a778ae60ea3f test: properly check for per-tx sigops limit (Sebastian Falbesoner)
Pull request description:
Currently the per-tx sigops limit standardness check (bounded by `MAX_STANDARD_TX_SIGOPS_COST`, throwing "bad-txns-too-many-sigops" if exceeded):
3f83c744ac/src/validation.cpp (L925-L927)
is only indirectly tested with the much higher per-block consensus limit (`MAX_BLOCK_SIGOPS_COST`):
3f83c744ac/test/functional/data/invalid_txs.py (L236-L242)
I.e. an increase in the per-tx limit up to the per-block one would still pass all of our tests. Refine that by splitting up the invalid tx template `TooManySigops` in a per-block and a per-tx template.
The involved functional tests taking use of these templates are `feature_block.py` and `p2p_invalid_txs.py`. Can be tested by applying e.g.
```diff
diff --git a/src/policy/policy.h b/src/policy/policy.h
index 2151ec13dd..e5766d2a55 100644
--- a/src/policy/policy.h
+++ b/src/policy/policy.h
@@ -37,7 +37,7 @@ static constexpr unsigned int MIN_STANDARD_TX_NONWITNESS_SIZE{65};
/** Maximum number of signature check operations in an IsStandard() P2SH script */
static constexpr unsigned int MAX_P2SH_SIGOPS{15};
/** The maximum number of sigops we're willing to relay/mine in a single tx */
-static constexpr unsigned int MAX_STANDARD_TX_SIGOPS_COST{MAX_BLOCK_SIGOPS_COST/5};
+static constexpr unsigned int MAX_STANDARD_TX_SIGOPS_COST{MAX_BLOCK_SIGOPS_COST/5 + 4};
/** Default for -incrementalrelayfee, which sets the minimum feerate increase for mempool limiting or replacement **/
static constexpr unsigned int DEFAULT_INCREMENTAL_RELAY_FEE{1000};
/** Default for -bytespersigop */
diff --git a/test/functional/mempool_accept.py b/test/functional/mempool_accept.py
```
where the tests succeed on master, but fail on this PR.
(Found by diving deeper into the jungle of current sig-ops limit, as preparation for reviewing the [BIP 54](https://github.com/bitcoin/bips/blob/master/bip-0054.md) draft and related preparatory PRs like #32521).
ACKs for top commit:
fjahr:
tACK 7bc64a8859c3644fdf2eeff59f72a778ae60ea3f
tapcrafter:
tACK 7bc64a8859c3644fdf2eeff59f72a778ae60ea3f
darosior:
ACK 7bc64a8859c3644fdf2eeff59f72a778ae60ea3f
instagibbs:
crACK 7bc64a8859c3644fdf2eeff59f72a778ae60ea3f
Tree-SHA512: 1365409349664a76a1d46b2fa358c0d0609fb17fffdd549423d22b61749481282c928be3c2fb428725735c82d319b4279f703bde01e94e4aec14bab206abb8cf
Covers:
- http pipelining
- rpcservertimeout
Testing this requires adding an option to TestNode to force
the test framework to establish a new HTTP connection for
every RPC. Otherwise, attempting to reuse a persistent connection
would cause framework RPCs during startup and shutdown to fail.
- "chunked" Transfer-Encoding
b104d442277090337ce405d92f1398b7cc9bcdb7 test: Remove RPCOverloadWrapper (Ava Chow)
4d32c19516fdb65b364638fa088b8d7167b438b6 test: Replace importpubkey (Ava Chow)
fe838dd391be669ccd0765b95f81c25fecfd3636 test: Replace usage of addmultisigaddress (Ava Chow)
d3142077794f4e910d3cdc749020d725e30feb24 test: Replace usage of importaddress (Ava Chow)
fcc457573f9b39e6f173a4f51c45d7dbb47e7ab0 test: Replace importprivkey with wallet_importprivkey (Ava Chow)
94c87bbbd06eb9a57930b9f59315533cfbe8f460 test: Remove unnecessary importprivkey from wallet_createwallet (Ava Chow)
Pull request description:
`RPCOverloadWrapper` implemented overloads for legacy wallet only RPCs so that the same function call could be used within tests for both legacy wallets and descriptor wallets. With legacy wallets now removed, there is no need to continue to have these overloads.
For `importaddress`, `addmultisigaddress`, and `importpubkey`, the uses of these are converted to `importdescriptors`.
For `importprivkey`, a new helper function `wallet_imporprivkey` is introduced that does what the overload did. This is mainly to reduce verbosity as `importprivkey` was more widely used throughout the tests.
Some tests that used these RPCs are now also no longer relevant and have been removed.
ACKs for top commit:
Sjors:
ACK b104d442277090337ce405d92f1398b7cc9bcdb7
pablomartin4btc:
cr ACK b104d442277090337ce405d92f1398b7cc9bcdb7
rkrux:
ACK b104d442277090337ce405d92f1398b7cc9bcdb7
w0xlt:
ACK b104d44227
Tree-SHA512: ded2f73829e2ce28466d4a9738eb382783ad990daee5d1859dbc4d354e6f8eec0c483ed5ecb1287fe0dd24ac332065b733a30d71b126b841bd7cd49e9a094b6d
Currently the per-tx sigops limit standardness check (bounded by
`MAX_STANDARD_TX_SIGOPS_COST`, throwing "bad-txns-too-many-sigops"
if exceeded) is only indirectly tested with the much higher per-block
consensus limit (`MAX_BLOCK_SIGOPS_COST`), i.e. an increase in the
limit would still pass all tests. Refine that by splitting up the invalid
tx template `TooManySigops` in a per-block and a per-tx one.
The involved functional tests taking use of these templates are
`feature_block.py` and `p2p_invalid_txs.py`.
Would otherwise cause an OverflowError in feature_framework_startup_failures.py when calling subprocess.run() with 60 * factor.
Fixes#32506
Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
75a185ea3db3177e8e479ee61a39bcb51e08d9a6 test: add skip_if_running_under_valgrind() (fanquake)
Pull request description:
Enable it in the USDT tests. The context (from 0xB10C):
> every time the tracepoint is reached a SIGTRAP is fired.
> No matter the tracepoint contents, even with an empty one.
> Valgrind intercepts SIGTRAP and aborts.
See discussion in #32374.
ACKs for top commit:
maflcko:
lgtm ACK 75a185ea3db3177e8e479ee61a39bcb51e08d9a6
willcl-ark:
ACK 75a185ea3db3177e8e479ee61a39bcb51e08d9a6
Tree-SHA512: 7f45c3049ab39cc514024067bd6ac26598e99202c114b48459834c26c2e1273fa58af693878298e628a10c561b954850e49e76b39567b771bb0c0534a063a524
4b241867567203b204823a4558c2aa5767acf028 test: add test for decoding PSBT with MuSig2 PSBT key types (BIP 373) (Sebastian Falbesoner)
8ba245cb8318c9be375ac2fb2a3e7ded875b4408 test: add constants for MuSig2 PSBT key types (BIP 373) (Sebastian Falbesoner)
Pull request description:
This PR is a follow-up to #31247 (see https://github.com/bitcoin/bitcoin/pull/31247#pullrequestreview-2427834909) and adds a functional test for decoding PSBTs (using the `decodepsbt` RPC) with MuSig2 per-input and per-output types. The first commit adds the new MuSig2 key types to the test frameworks and extends the PSBT serialization to cope with lists of bytestrings.
ACKs for top commit:
achow101:
ACK 4b241867567203b204823a4558c2aa5767acf028
rkrux:
re-ACK 4b24186
Tree-SHA512: f12919f71b3fff74df1d7ddaa8db455b1b139f7abd51d7f3fa5d750fc7dd613454b438c4e0dedad679476d414fa1da43ef1121e486b0bdfd97d5ef8bdf37f060
Enable it in the USDT tests. The context (from 0xB10C):
> every time the tracepoint is reached a SIGTRAP is fired.
> No matter the tracepoint contents, even with an empty one.
> Valgrind intercepts SIGTRAP and aborts.
See discussion in #32374.
Support new BITCOIN_CMD environment variable in functional test to be able to
test the new bitcoin wrapper executable and run other commands through it
instead of calling them directly.
Co-authored-by: Sjors Provoost <sjors@sprovoost.nl>
a58cb3b1c12c8cb75a87375c50f94c4605bb805d qa: sanity check mined block have their coinbase timelocked to height (Antoine Poinsot)
8f2078af6a55448c003b3f7f3021955fbb351caa miner: timelock coinbase transactions (Antoine Poinsot)
788aeebf343526760fa8f3ed969ac3713212a5b6 qa: use prev height as nLockTime for coinbase txs created in unit tests (Antoine Poinsot)
c76dbe9b8b6f03b761a0ef97e1b8cd133b934714 qa: timelock coinbase transactions created in fuzz targets (Antoine Poinsot)
9c94069d8b6cf67a24eb03c51230a4f2b2bf2d64 contrib: timelock coinbase transactions in signet miner (Antoine Poinsot)
a5f52cfcc400ad0adb41a78c65b8abb971e0d622 qa: timelock coinbase transactions created in functional tests (Antoine Poinsot)
Pull request description:
The Consensus Cleanup soft fork proposal includes enforcing that coinbase transactions set their
nLockTime field to the block height minus 1, as well as their nSequence such as to not disable the
timelock. If such a fork were to be activated by Bitcoin users, miners need to be ready to produce
compliant blocks at the risk of losing substantial amounts mining would-be invalid blocks. As miners
are unfamously slow to upgrade, it's good to make this change as early as possible.
Although Bitcoin Core's GBT implementation does not provide the `coinbasetxn` field, and mining
pool software crafts the coinbase on its own, updating the Bitcoin Core mining code is a first step
toward convincing pools to update their (often closed source) code. A possible followup is also to
introduce new fields to GBT. In addition, this first step also makes it possible to test future
Consensus Cleanup changes.
The commit making the change also updates a bunch of seemingly-unrelated tests. This is because those tests were asserting error messages based on the txid of transactions involved, and changing the coinbase transaction structure necessarily changes the txid of all tests' transactions.
ACKs for top commit:
Sjors:
Code review ACK a58cb3b1c12c8cb75a87375c50f94c4605bb805d
achow101:
ACK a58cb3b1c12c8cb75a87375c50f94c4605bb805d
TheCharlatan:
Re-ACK a58cb3b1c12c8cb75a87375c50f94c4605bb805d
Tree-SHA512: a2aae009a187eb760d34435f518a895ee76c6b02a667eb030ddf6bd584da6e8eae2737d974dbf81a928d60c07bcb4820f055adc067e18d8819640db0240bb513
importprivkey was a legacy wallet only RPC which had a helper for
descriptor wallets in tests. Add wallet_importprivkey helper and use it
wherever importprivkey is used (other than backward compatibility tests)
10845cd7cc89fc83b4ee844dc577b391720bba9c qa: Add feature_framework_startup_failures.py (Hodlinator)
28e282ef9ae94ede4aace6b97ff18c66cb72a001 qa: assert_raises_message() - Stop assuming certain structure for exceptions (Hodlinator)
1f639efca5e71a0ff208415d94e408a74778d4db qa: Work around Python socket timeout issue (Hodlinator)
9b24a403fae4b896ff7705519bd48c877b4e621b qa: Only allow calling TestNode.stop() after connecting (Hodlinator)
6ad21b4c0114029d16d334bf8d437834708a295e qa: Include ignored errors in RPC connection timeout (Hodlinator)
879243e81fd55f54739fbdae4d5b8666f5acb9a9 qa refactor: wait_for_rpc_connection - Treat OSErrors the same (Hodlinator)
Pull request description:
Improves handling of startup errors in functional tests and puts tests in place to ensure knock-on errors don't creep in.
- `wait_for_rpc_connection()` now appends specific failures leading up to the `Unable to connect to bitcoind` error to that error message:
`[node 0] Unable to connect to bitcoind after 60s (ignored errors: {'missing_credentials': 1, 'OSError.ECONNREFUSED': 239}, latest error: ConnectionRefusedError(111, 'Connection refused'))`
- Fixes Windows Python issue where `socket.timeout` exceptions end up with unset `errno`-fields.
- Also adds comments, refactors code, improves logging.
The underlying purpose is to ensure developer efficiency in finding root causes of test failures.
Prior iterations of the PR partially focused on fixing the same issue as #31620.
Originally inspired by #30390.
### Testing
Can be tested by reverting either faf2f2c654d9aa18b2f49a157956f9ab0fce302a or fae3bf6b870eb0f9cddd1adac82ba72890806ae3 from #31620, or the "qa: Avoid calling stop-RPC if not connected" from this PR, and running *feature_framework_startup_failures.py*.
ACKs for top commit:
l0rinc:
ACK 10845cd7cc89fc83b4ee844dc577b391720bba9c
ryanofsky:
Code review ACK 10845cd7cc89fc83b4ee844dc577b391720bba9c. Only changes since last review were adding a new commit tweaking assert_raises_message(), extending the new test to have a self-check, and to pass through all options to child tests instead of a hardcoded list of options. I left some cleanup suggestions below but they are not important.
Tree-SHA512: f0235c5cbb6d1bb85d8dc5de492a08a34f6edc83499cbf0a5f9a3824809ff84635888c62c9c01101e3cc9ef9f1cdee2c9ab6537fea6feeb005b29f428caf8b22
Also, support serialization of lists of byte-strings as PSBTMap values,
which will be simply concatenated without any compact-size prefixes
(neither for the individual items nor for the size of the list).
Removes all legacy wallet specific functional tests.
Also removes the --descriptor and --legacy-wallet options as these are
no longer necessary with the legacy wallet removed.
fa86190e6ed2aeda7bcceaf96f52403816bcd751 rpc: Allow fullrbf fee bump (MarcoFalke)
Pull request description:
The RPCs (psbt)bumpfee, and the GUI, reject fee bumps when BIP 125 signalling is absent in the transaction even when the mempool and other RPCs allow them. Fix the confusion by allowing the fee bump.
This is done after fullrbf is always on (https://github.com/bitcoin/bitcoin/pull/30592)
ACKs for top commit:
1440000bytes:
reACK fa86190e6e
achow101:
ACK fa86190e6ed2aeda7bcceaf96f52403816bcd751
w0xlt:
ACK fa86190e6e
rkrux:
reACK fa86190e6ed2aeda7bcceaf96f52403816bcd751
glozow:
ACK fa86190e6ed2aeda7bcceaf96f52403816bcd751
Tree-SHA512: b2ffe8dcadbe71e9be767a16cf8aa0bf383c2de7aa1aee9438d125f444e24f3f7e4f02ddb28981bd3b8b645b6a24a407b4ad6bb0b21946ae637e78f6386e05bf
Observed on local machine running Windows / Python v3.13.1 when overriding rpc_timeout to small values (5- seconds). Next commit performs such overrides.
(Still tolerate calling it on a no longer (self.)running node, as in a node that has been queried for is_node_stopped() and modified state before returning True).
Tests should not attempt to use the non-functioning RPC interface to call stop() unless wait_for_connections() has succeeded.
No longer log and suppress http.client.CannotSendRequest as a consequence of stop()-RPC, as error conditions causing this knock-on issue are now guarded against before the call.
