Note that we unfortunately can't use a scripted diff here, as the
`sha256` symbol is also used for other instances (e.g. as function
in hashlib, or in the `UTXO` class in p2p_segwit.py).
Since the previous commit, CBlockHeader/CBlock 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 block hashes (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 block hash.
This is a preparatory commit for removing the header hash
caching in the CBlockHeader class. In order to not lose the
old block hash, necessary for updating the internal state of
the test (represented by `self.block_heights` and `self.blocks`),
we should only modify it within the `update_block` method.
Note that we can't call `.serialize()` directly in
the `.calc_sha256()` method, as this could wrongly lead
to the serialization of the derived class (CBlock) if
called from an instance there.
This check ensures that when migrating a legacy wallet with a direct
filename, the backup file is named as expected.
Co-authored-by: Ava Chow <github@achow101.com>
This test was introduced in #28251 to ensure that the mempool is not
trimmed in the middle of a package evaluation and the m_view cache
is updated when evictions and replacements happen so coins are no longer
visible in subsequent package transactions. These two things have
coverage in other tests as well, and are pretty unlikely to happen.
This test is also brittle: it requires evaluation of the parents in a
particular order, and creates a transaction that itself is not
enough to trigger eviction but will be pushed out immediately by the
package spending from it. While the current magic number 2000 works, we
do not have a way to query remaining space in the mempool if mempool
data structures change, and it can differ across platforms.
4bb4c86599 test: document HOST for get_previous_releases.py (Sjors Provoost)
609203d507 test: stop signing previous releases >= v28.2 (Sjors Provoost)
c6dc2c29f8 test: replace v28.0 with notarized v28.2 (Sjors Provoost)
5bd73d96a3 test: fix macOS detection (Sjors Provoost)
Pull request description:
Since https://github.com/bitcoin/bitcoin/pull/31407 macOS guix builds are signed and notarized. This was included in v29 and backported to 28.x.
This PR bumps the v28.0 previous release binary to v28.2 and adjusts the test that uses it. Additionally it no longer manually code signs binaries >= v28.2.
While testing on an M4 mac and redownloading all the binaries, I noticed that `platform == "arm64-apple-darwin"` doesn't actually work. This initially used `args.platform` in #26694, but that was changed to just `platform` in #32219.
So the first commit switches this to use `args.host`. I manually tested on Intel macOS 13.7.6 that code-signing still isn't needed there (when downloading using a script).
Also documented that you can set `HOST`.
ACKs for top commit:
m3dwards:
ACK 4bb4c86599
maflcko:
review ACK 4bb4c86599🚏
Tree-SHA512: b4803d39a21cb622fd2388a0528b76d2b502956e2505385d3da201143b0afcf6f9d71c8c28937f27b70d2588fb6da677da058bdcd67b90fb53617acc3a727818
61e800e75c test: headers sync timeout (stringintech)
Pull request description:
When reviewing PR #32051 and considering which functional tests might need to be adapted/extended accordingly, I noticed there appears to be limited functional test coverage for header sync timeouts and thought it might be helpful to add one.
This test attempts to cover two scenarios:
1. **Normal peer timeout behavior:** When a peer fails to respond to initial getheaders requests within the timeout period, it should be disconnected and the node should attempt to sync headers from the next available peer.
2. **Noban peer behavior:** When a peer with noban privileges times out, it should remain connected while the node still attempts to sync headers from other peers.
ACKs for top commit:
maflcko:
re-ACK 61e800e75c 🗝
stratospher:
reACK 61e800e7.
Tree-SHA512: b8a867e7986b6f0aa00d81a84b205f2bf8fb2e6047a2e37272e0244229d1f43020e9031467827dabbfe7849a91429f2685e00a25356e2ed477fa1a035fa0b1fd
For the default number of peers (125), allows each to relay a default
descendant package (up to 25-1=24 can be missing inputs) of small (9
inputs or fewer) transactions out of order.
This limit also gives acceptable bounds for worst case LimitOrphans iterations.
Functional tests aren't changed to check for larger cap because it would
make the runtime too long.
Also deletes the now-unused DEFAULT_MAX_ORPHAN_TRANSACTIONS.
node1 (with 24 blocks) causes node0 (with 6 blocks) to silently
reorg. so move the subtest to a point before the 20 blocks are
generated so that node1's state doesn't cause node0 to silently
reorg.
Expiry is going away in a later commit.
This is only an RPC change. Behavior of the orphanage does not change.
Note that getorphantxs is marked experimental.
83bb414557 test: less ambiguous error if bitcoind is missing (Sjors Provoost)
Pull request description:
Before this change, when a functional test is run without building the source, the error message suggested that previous release binaries were missing.
When no previous release version is set, make the error message more specifically about bitcoind.
To test, try this before and after:
```sh
git clean -dfx
cmake -B build
build/test/functional/mining_basic.py
cmake --build build
build/test/functional/mining_basic.py
build/test/functional/wallet_backwards_compatibility.py
test/get_previous_releases.py
build/test/functional/wallet_backwards_compatibility.py
```
ACKs for top commit:
achow101:
ACK 83bb414557
janb84:
ACK 83bb414557
w0xlt:
ACK 83bb414557
Tree-SHA512: c6df65019de99d6c214951cf70944c4ddca9b635c5ab60ac2c47e4589478e9c65d5e079c394ace9b470a7eaeea3c9cf68b7246dd413e802c4a1e071913a7fc32
fa0528479d test: Add missing convert_to_json_for_cli (MarcoFalke)
Pull request description:
Currently the tests are failing on current master, if they use the `--usecli` flag. See https://github.com/bitcoin/bitcoin/runs/45676472375, https://cirrus-ci.com/task/5707897310543872.
This can be reproduced locally via:
```
./bld-cmake/test/functional/wallet_reorgsrestore.py --usecli
```
Fix it by adding the missing `hash_or_height=self.convert_to_json_for_cli(tip)` for the value that could either be a string (needs quotes in json), or a number (does not need quotes in json).
ACKs for top commit:
fanquake:
ACK fa0528479d
Tree-SHA512: 3d6deafca1249b2266cfabcd883edc9daaf985c417035a4b0223da4693f4165f8c9ce91a0e128d626000c10c32fe31f323f4b3f6ea0d0b3a771237a4f1d4cf44
fa4d68cf97 Turn rpcauth.py test into functional test (MarcoFalke)
Pull request description:
Currently the `rpcauth-test.py` is problematic, because:
* The boilerplate for the test runner is duplicate or inconsistent with the other (functional) tests. Specifically `ConfigParser`.
* The cmake/ci behavior is brittle and can silently fail, as explained in https://github.com/bitcoin/bitcoin/issues/31476.
* Outside of ctest, this single test has to be run manually and separately, which is easy to forget.
* If the test is manually called, it runs single threaded, when it could just run in parallel with the other functional tests.
* It is also the only "unit" test written in Python, but not called by the functional test runner.
Fix all issues by turning it into a functional test.
ACKs for top commit:
l0rinc:
ACK fa4d68cf97
janb84:
LGTM ACK fa4d68cf97
w0xlt:
ACK fa4d68cf97
Tree-SHA512: a3b2b03be31c33288dee23c544b33ec43295e796c2047777597ceb86acce9f697478e32d891aa986c1d7d5749d62eded65eeb858e9d7479bda7a400eb1167040
8cc3ac6c23 validation: Don't use IsValid() to filter for invalid blocks (Martin Zumsande)
86d98b94e5 test: verify that ancestors of a reconsidered block can become the chain tip (stratospher)
3c39a55e64 validation: Add ancestors of reconsiderblock to setBlockIndexCandidates (Martin Zumsande)
Pull request description:
When we call `reconsiderblock` for some block, `Chainstate::ResetBlockFailureFlags` puts the descendants of that block into `setBlockIndexCandidates` (if they meet the criteria, i.e. have more work than the tip etc.), but never put any ancestors into the set even though we do clear their failure flags.
I think that this is wrong, because `setBlockIndexCandidates` should always contain all eligible indexes that have at least as much work as the current tip, which can include ancestors of the reconsidered block. This is being checked by `CheckBlockIndex()`, which could fail if it was invoked after `ActivateBestChain` connects a block and releases `cs_main`:
``` diff
diff --git a/src/validation.cpp b/src/validation.cpp
index 7b04bd9a5b..ff0c3c9f58 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -3551,6 +3551,7 @@ bool Chainstate::ActivateBestChain(BlockValidationState& state, std::shared_ptr<
}
}
// When we reach this point, we switched to a new tip (stored in pindexNewTip).
+ m_chainman.CheckBlockIndex();
if (exited_ibd) {
// If a background chainstate is in use, we may need to rebalance our
```
makes `rpc_invalidateblock.py` fail on master.
Even though we don't currently have a `CheckBlockIndex()` in that place, after `cs_main` is released other threads could invoke it, which is happening in the rare failures of #16444 where an invalid header received from another peer could trigger a `CheckBlockIndex()` call that would fail.
Fix this by adding eligible ancestors to `setBlockIndexCandidates` in `Chainstate::ResetBlockFailureFlags` (also simplifying that function a bit).
Fixes#16444
ACKs for top commit:
achow101:
ACK 8cc3ac6c23
TheCharlatan:
Re-ACK 8cc3ac6c23
stratospher:
reACK 8cc3ac6.
Tree-SHA512: 53f27591916246be4093d64b86a0494e55094abd8c586026b1247e4a36747bc3d6dbe46dc26ee4a22f47b8eb0d9699d13e577dee0e7198145f3c9b11ab2a30b7
1b5c545e82 wallet, test: best block locator matches scan state follow-ups (rkrux)
Pull request description:
Few follows-ups from #30221: Use `SetLastBlockProcessedInMem` more in `AttachChain`, add not null locator check in `WriteBestBlock`. Add log and few assertions in `wallet_reorgstore` test.
ACKs for top commit:
achow101:
ACK 1b5c545e82
pablomartin4btc:
cr-ACK 1b5c545e82
Tree-SHA512: 34edde55beef5714cea2e1131c29b57da2dc32ea091cd81878014de503c128f02c3ab88aee1e456541d7937e033dca5a81b03e9e2888cf781d71b62ad9b5ca5c
The std::source_location conveniently stores the file name, line number,
and function name of a source code location. We switch to using it instead
of the __func__ identifier and the __FILE__ and __LINE__ macros.
BufferedLog is changed to have a std::source_location member, replacing the
source_file, source_line, and logging_function members. As a result,
MemUsage no longer explicitly counts source_file or logging_function as the
std::source_location memory usage is included in the MallocUsage call.
This also changes the behavior of -logsourcelocations as std::source_location
includes the entire function signature. Because of this, the functional test
feature_config_args.py must be changed to no longer include the function
signature as the function signature can differ across platforms.
Co-Authored-By: Niklas Gogge <n.goeggi@gmail.com>
Co-Authored-By: stickies-v <stickies-v@protonmail.com>
Before this change, when a functional test is run without building
the source, the error message suggested that previous release binaries
were missing.
When no previous release version is set, make the error message more
specifically about bitcoind.
fcfd3db563 remove RPCTimerInterface and RPCRunLater (Matthew Zipkin)
8a1765795f use WalletContext scheduler for walletpassphrase callback (Matthew Zipkin)
Pull request description:
This removes the dependency on libevent for events scheduled by RPC commands, like re-locking a wallet some time after decryption with walletpassphrase. Since walletpassphrase is currently the only RPC that does this, `RPCRunLater`, `RPCTimerInterface` and all related methods are left unused, and deleted in the second commit. Any future RPC that needs to execute a callback in the future can follow the pattern in this PR and just use a scheduler from node or wallet context.
This is an alternative approach to #32796, described in https://github.com/bitcoin/bitcoin/pull/32796#issuecomment-3014309449
ACKs for top commit:
fjahr:
Code Review ACK fcfd3db563
achow101:
ACK fcfd3db563
furszy:
ACK fcfd3db563
Tree-SHA512: 04f5e9c3f73f598c3d41d6e35bb59c64c7b93b03ad9fce3c40901733147ce7764f41f475fef1527d44af18f722759996a31ca83b48cb52153795d5022fecfd14
4207d9bf82 test: feature_init, ensure indexes are synced prior to perturbing files (furszy)
abd07cf733 test: feature_init, only init what's needed per perturbation/deletion round (furszy)
Pull request description:
Aims to solve #32600. Found it while working on #26966 (this was really annoying there).
This ensures the node is index-synced before perturbing files.
If the index sync gets interrupted before it starts, the database could be empty,
making any following perturbation ineffective (which explains why the node
does not abort during startup in the #32600 logs).
Also, the first commit avoids initializing components not under test.
This reduces log flooding, which helped in understanding the issue.
Patch to reproduce the issue on master using `feature_init.py` (this simulates
a node shutting down before the index starts syncing):
```
diff --git a/src/index/base.cpp b/src/index/base.cpp
--- a/src/index/base.cpp(revision 1e03052c3fefb188f047e72548f2c6b0cc019e50)
+++ b/src/index/base.cpp(date 1751293306725)
@@ -185,6 +185,7 @@
void BaseIndex::Sync()
{
const CBlockIndex* pindex = m_best_block_index.load();
+ m_interrupt();
if (!m_synced) {
std::chrono::steady_clock::time_point last_log_time{0s};
std::chrono::steady_clock::time_point last_locator_write_time{0s};
```
ACKs for top commit:
maflcko:
lgtm ACK 4207d9bf82🍄
achow101:
ACK 4207d9bf82
hodlinator:
ACK 4207d9bf82
Tree-SHA512: c8c89c7af9d473a12756b6a59b97f8fb473500181620eb96ecc10da954fe185d13fbb1d00a4ecb181e8daf149ec93cc547e292da0877522a4d23425fa7fd646b
afaaba69ed test: refactor out same-txid-diff-wtxid tx to reuse in other tests (stratospher)
Pull request description:
It's useful to easily create transactions with same txid, different wtxid and valid witness for testing scenarios in other places in the codebase (ex: private broadcast connections, see https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2055915118)
So refactor out the current `same-txid-diff-wtxid` transaction in `mempool_accept_wtxid.py` so that it can be reused.
ACKs for top commit:
maflcko:
review ACK afaaba69ed📎
theStack:
ACK afaaba69ed
vasild:
ACK afaaba69ed
Tree-SHA512: 0fc51ac326725d4abe76a15b6b5be55d070b96c303c444f4dd31c2b0a82f266836382389a123a7f6a71aa35e61fbfae27f843b31cc19474e49f3dc82f36ebf73
b1a8ac07e9 doc: Release note for removed watchonly parameters and results (Ava Chow)
15710869e1 wallet: Remove ISMINE_WATCH_ONLY (Ava Chow)
4439bf4b41 wallet, spend: Remove fWatchOnly from CCoinControl (Ava Chow)
1337c72198 wallet, rpc: Remove watchonly from RPCs (Ava Chow)
e81d95d435 wallet: Remove watchonly balances (Ava Chow)
d20dc9c6aa wallet: Wallets without private keys cannot grind R (Ava Chow)
9991f49c38 test: Watchonly wallets should estimate larger size (Ava Chow)
Pull request description:
Descriptor wallets do not use the watchonly behavior as it is not possible to mix watchonly and non-watchonly in a descriptor wallet. With legacy wallets now removed, all of the watchonly handling and reporting code is no longer needed. This PR removes watchonly options and results from the RPCs and the handling of watchonly things from the wallet's internals.
With all of the watchonly things removed, ISMINE_WATCH_ONLY is removed as well.
Split from #32523
Depends on #32594 for tests that are easier to read
ACKs for top commit:
Eunovo:
ACK b1a8ac07e9
maflcko:
re-ACK b1a8ac07e9🌈
rkrux:
ACK b1a8ac07e9
furszy:
light code review ACK b1a8ac07e9
Tree-SHA512: bc87f37a13294f7208991be8f93899b49e5bdf87c70e0f66d9c4cb09c03be6c202320406f27e9a35aa2f57319d19a3f0c07d5e5ddbc97c7edab165b1656d6612
9b75cfda4d test: retain the intended behavior of `feature_fee_estimation.py` nodes (ismaelsadeeq)
5c1236f04a test: fix incorrect subtest in `feature_fee_estimation.py` (ismaelsadeeq)
Pull request description:
Attempt to fix#32461
In the `estimatesmartfee` RPC, we return the maximum of the following: the feerate estimate for the target, `minrelaytxfee`, and `mempoolminfee`.
9a05b45da6/src/rpc/fees.cpp (L85)
The test `test_feerate_mempoolminfee`, originally introduced in ea31caf6b4, is incorrect.
It should calculate the fee rate ceiling by taking the maximum of the custom `minrelaytxfee`, `mempoolminfee`, and the highest fee rate observed during the test (`check_smart_estimates`). This is necessary because:
* There is no guarantee that the generated fee rates will exceed both `minrelaytxfee` and `mempoolminfee`.
* Users can start a node with custom fee settings.
Due to the non-deterministic nature of the `feature_fee_estimation.py` test, it often passes by chance. The randomly generated fees typically include a value higher than the custom `minrelaytxfee`, inadvertently hiding the issue.
Issue #32461 identified a random seeds that consistently fails the test because the generated fees never exceed the custom `minrelaytxfee`:
e.g
```
build/test/functional/feature_fee_estimation.py --random=3450808900320758527
```
This PR has two commits which :
* Correctly fixes the test by calculating the fee rate ceiling as the maximum of the node `minrelaytxfee`, `mempoolminfee`, and the highest seen fee rate, when verifying smart fee estimates.
* Improves the subtest name and comment for clarity.
* Restores the original test behavior by appending 4000 WU to the custom `blockmaxweight`.
ACKs for top commit:
achow101:
ACK 9b75cfda4d
glozow:
ACK 9b75cfda4d
theStack:
Light ACK 9b75cfda4d
Tree-SHA512: 0f7fb0496b50a399b58f6fb1afd95414fad454795fbc0046e22dfc54a2062ae0c519a12ebfeb6ad7ef547438868d99eca2351c0d19d0346adaadb500eff6f15f
666016e56b ci: use --usecli in one of the CI jobs (Martin Zumsande)
7ea248a020 test: Disable several (sub)tests with cli (Martin Zumsande)
f420b6356b test: skip subtests that check for wrong types with cli (Martin Zumsande)
6530d0015b test: add function to convert to json for height_or_hash params (Martin Zumsande)
54d28722ba test: Don't send empty named args with cli (Martin Zumsande)
cca422060e test: convert tuple to json for cli (Martin Zumsande)
af34e98086 test: make rpc_psbt.py usable with --usecli (Martin Zumsande)
8f8ce9e174 test: rename .rpc to ._rpc and remove unnecessary uses (Martin Zumsande)
5b08885986 test: enable functional tests with large rpc args for cli (Martin Zumsande)
7d5352ac73 test: use -stdin for large rpc commands (Martin Zumsande)
6c364e0c10 test: Enable various tests for usage with cli (Martin Zumsande)
Pull request description:
Fixes#32264
I looked into all current failures listed in the issue, as well all tests that are already disabled for the cli with `self.supports_cli = False`. There are several reasons why existing tests fail with `--usecli` on many systems, the most important ones are:
- Most common reason is that the test executes a RPC call with a large arg that exceeds `MAX_ARG_STRLEN` of the OS, which is usually 128kb on linux: This is fixed by using `-stdin` for these large calls (idea by 0xB10C)
- they test specifically the rpc interface - nothing to do there except disabling.
- Some functional test submit wrong types to params on purpose to test the error message (which is different when using the cli) - deactivated these specific subtests locally for the cli when there is just one or two of them, deactivated the entire tests when there are more spots
- When python sets `None` for an arg, the cli converts this to 'null' in `arg_to_cli`. This is fine e.g. for boolean args, but doesn't work for strings where it's interpreted as the string 'null'. Bypass this for named args by not including args in case the value is `None` for the cli is used (it's effectively the same as leaving the optional arg out).
- the `height_or_hash` param used in some RPC needs to be converted to a JSON (effectively adding full quotes).
- Some tests were marked with `self.supports_cli = False` in the past but run fine on master today - enabled those.
In total, this PR fixes all tests that fail on master and reduces the number of tests that are deactivated (`self.supports_cli = False`) from 40 to 21.
It also adds `--usecli` to one CI job (multiprocess, i686, DEBUG) to detect regressions.
ACKs for top commit:
maflcko:
re-ACK 666016e56b🔀
pinheadmz:
re-ACK 666016e56b
Tree-SHA512: 7a1efd212649ca100b236a1239294d40ecd36e2720e3b173a230b14545bb40b135111db7fed8a0d1448120f5387da146a03f1912e2028c8d03a0b6a3ca8761b0
Avoids initializing and syncing components not under test.
This not only speeds up execution a bit but also helps isolate
and debug issues more easily, as logs aren't flooded with
unrelated details.
ec004cdb86 test: Use rehash() in outbound eviction block-relay (pablomartin4btc)
26598ed21e test: Clarify roles in outbound eviction comments (pablomartin4btc)
Pull request description:
This change avoids relying on `tip_header.hash`, which is `None` when the header is deserialized from hex during `CBlockHeader()` construction.
Instead, `tip_header.rehash()` explicitly computes the hash, making the test behavior more robust.
Using the explicit `rehash()` avoids depending on `wait_for_getheaders()` falling back to any received message, thus making the test more deterministic.
This is a follow-up to #32742.
Also, as noted in a previous review [comment](https://github.com/bitcoin/bitcoin/pull/32742#pullrequestreview-2923802386), "_the hash field is wrong either way, simply due to being the wrong type (it is an optional hex string), as opposed to an optional int_".
---
The first commit intention is to improve clarity around the tests purpose, helping reviewers follow what's being verified and why. What started as a small comment during review of #32742 led me reviewing and try to improve most relevant tests comments for consistency and correctness.
ACKs for top commit:
achow101:
ACK ec004cdb86
theStack:
lgtm ACK ec004cdb86#️⃣
yuvicc:
ACK ec004cdb86
danielabrozzoni:
ACK ec004cdb86
Tree-SHA512: 6a14dedfdc425cd806f63443b3b9f79df69a7717452739f5d7fef1b2bdba23402670d63cf1d6b66c9f1a6b460d4d4a6f185426d0a4982fa95115a234cd6baef7
b789907346 wallet: migration, avoid creating spendable wallet from a watch-only legacy wallet (furszy)
e86d71b749 wallet: refactor, dedup wallet re-loading code (furszy)
1de423e0a0 wallet: introduce method to return all db created files (furszy)
d04f6a97ba refactor: remove sqlite dir path back-and-forth conversion (furszy)
Pull request description:
Currently, the migration process creates a brand-new descriptor wallet with no
connection to the user's legacy wallet when the legacy wallet lacks key material
and contains only watch-only scripts. This behavior is not aligned with user
expectations. If the legacy wallet contains only watch-only scripts, the migration
process should only generate a watch-only wallet instead.
TODO List:
* Explain that `migratewallet` renames the watch-only after migration, and
also that the wallet will not have keys enabled.
ACKs for top commit:
achow101:
ACK b789907346
pablomartin4btc:
tACK b789907346
rkrux:
LGTM ACK b789907346
Tree-SHA512: 1d583ac4b206fb477e9727daf4b5ad9c3e18b12d40e1ab4a61e8565da44c3d0327c892b51cf47b4894405d122e414cefb6b6366c357e02a74a7ca96e06762d83
