a0abcbd382 doc: Mention multipath specifier (Ava Chow)
0019f61fc5 tests: Test importing of multipath descriptors (Ava Chow)
f97d5c137d wallet, rpc: Allow importdescriptors to import multipath descriptors (Ava Chow)
32dcbca3fb rpc: Allow importmulti to import multipath descriptors correctly (Ava Chow)
64dfe3ce4b wallet: Move internal to be per key when importing (Ava Chow)
1692245525 tests: Multipath descriptors for scantxoutset and deriveaddresses (Ava Chow)
cddc0ba9a9 rpc: Have deriveaddresses derive receiving and change (Ava Chow)
360456cd22 tests: Multipath descriptors for getdescriptorinfo (Ava Chow)
a90eee444c tests: Add unit tests for multipath descriptors (Ava Chow)
1bbf46e2da descriptors: Change Parse to return vector of descriptors (Ava Chow)
0d640c6f02 descriptors: Have ParseKeypath handle multipath specifiers (Ava Chow)
a5f39b1034 descriptors: Change ParseScript to return vector of descriptors (Ava Chow)
0d55deae15 descriptors: Add DescriptorImpl::Clone (Ava Chow)
7e86541f72 descriptors: Add PubkeyProvider::Clone (Ava Chow)
Pull request description:
It is convenient to have a descriptor which specifies both receiving and change addresses in a single string. However, as discussed in https://github.com/bitcoin/bitcoin/issues/17190#issuecomment-895515768, it is not feasible to use a generic multipath specification like BIP 88 due to combinatorial blow up and that it would result in unexpected descriptors.
To resolve that problem, this PR proposes a targeted solution which allows only a single pair of 2 derivation indexes to be inserted in the place of a single derivation index. So instead of two descriptor `wpkh(xpub.../0/0/*)` and `wpkh(xpub.../0/1/*)` to represent receive and change addresses, this could be written as `wpkh(xpub.../0/<0;1>/*)`. The multipath specifier is of the form `<NUM;NUM>`. Each `NUM` can have its own hardened specifier, e.g. `<0;1h>` is valid. The multipath specifier can also only appear in one path index in the derivation path.
This results in the parser returning two descriptors. The first descriptor uses the first `NUM` in all pairs present, and the second uses the second `NUM`. In our implementation, if a multipath descriptor is not provided, a pair is still returned, but the second element is just `nullptr`.
The wallet will not output the multipath descriptors (yet). Furthermore, when a multipath descriptor is imported, it is expanded to the two descriptors and each imported on its own, with the second descriptor being implicitly for internal (change) addresses. There is no change to how the wallet stores or outputs descriptors (yet).
Note that the path specifier is different from what was proposed. It uses angle brackets and the semicolon because these are unused characters available in the character set and I wanted to avoid conflicts with characters already in use in descriptors.
Closes#17190
ACKs for top commit:
darosior:
re-ACK a0abcbd382
mjdietzx:
reACK a0abcbd382
pythcoiner:
reACK a0abcbd
furszy:
Code review ACK a0abcbd
glozow:
light code review ACK a0abcbd382
Tree-SHA512: 84ea40b3fd1b762194acd021cae018c2f09b98e595f5e87de5c832c265cfe8a6d0bc4dae25785392fa90db0f6301ddf9aea787980a29c74f81d04b711ac446c2
41051290ab cmake: Ignore build subdirectories within source directory (Hennadii Stepanov)
6ce50fd9d0 doc: Update for CMake-based build system (Hennadii Stepanov)
9730288a0c ci: Migrate CI scripts to CMake (Hennadii Stepanov)
c360837ca5 cmake, lint: Adjust `lint_includes_build_config` (Hennadii Stepanov)
3885441ee0 cmake: Add presets for native Windows builds (Hennadii Stepanov)
7681746b20 cmake: Add vcpkg manifest file (Hennadii Stepanov)
8b6f1c4353 cmake: Add `Coverage` and `CoverageFuzz` scripts (Hennadii Stepanov)
65bdbc1ff2 cmake: Add `docs` build target (Hennadii Stepanov)
fb75ebbc33 cmake: Add compiler diagnostic flags (Hennadii Stepanov)
e821f0a37a cmake: Migrate Guix build scripts to CMake (Hennadii Stepanov)
747adb6ffe cmake: Add `Maintenance` module (Hennadii Stepanov)
1f60b30df0 cmake: Add `APPEND_{CPP,C,CXX,LD}FLAGS` cache variables (Hennadii Stepanov)
2b43c45b13 cmake: Add `AddWindowsResources` module (Hennadii Stepanov)
973a3b0c5d cmake: Implement `install` build target (Hennadii Stepanov)
84ac35cfd4 cmake: Add cross-compiling support (Hennadii Stepanov)
0d01c228a7 build: Generate `toolchain.cmake` in depends (Hennadii Stepanov)
91a799247d depends: Add host-specific `cmake_system_version` variables (Hennadii Stepanov)
9b31209b4c depends: Rename `cmake_system` -> `cmake_system_name` (Hennadii Stepanov)
4a5208a81d Revert "build, qt: Do not install *.prl files" (Hennadii Stepanov)
6522af62af depends: Amend handling flags environment variables (Hennadii Stepanov)
90cec4d251 cmake: Add `MULTIPROCESS` option (Hennadii Stepanov)
bb1a450dcb cmake: Build `bitcoin-chainstate` executable (Hennadii Stepanov)
aed38ea58c cmake: Build `bitcoinkernel` library (Hennadii Stepanov)
975d67369b cmake: Build `test_bitcoin-qt` executable (Hennadii Stepanov)
10fcc668a3 cmake: Add `WITH_DBUS` option (Hennadii Stepanov)
5bb5a4bc75 cmake: Add `libqrencode` optional package support (Hennadii Stepanov)
57a6e2ef4a cmake: Build `bitcoin-qt` executable (Hennadii Stepanov)
30f642952c cmake: Add `WERROR` option (Hennadii Stepanov)
c98d4a4c34 cmake: Add `REDUCE_EXPORTS` option (Hennadii Stepanov)
a01cb6e63f cmake: Add `HARDENING` option (Hennadii Stepanov)
a8a2e364ac cmake: Add Python-based tests (Hennadii Stepanov)
3d85379570 cmake: Add fuzzing options (Hennadii Stepanov)
908530e312 cmake: Add `SANITIZERS` option (Hennadii Stepanov)
8bb0e85631 cmake: Build `bench_bitcoin` executable (Hennadii Stepanov)
801735163a cmake: Add external signer support (Hennadii Stepanov)
353e0c9e96 cmake: Add `systemtap-sdt` optional package support (Hennadii Stepanov)
d2fda82b49 cmake: Add `libzmq` optional package support (Hennadii Stepanov)
ae7b39a0e1 cmake: Add `libminiupnpc` optional package support (Hennadii Stepanov)
6480e1dcdb cmake: Add `libnatpmp` optional package support (Hennadii Stepanov)
e73e9304a1 cmake: Build `bitcoin-util` executable (Hennadii Stepanov)
027c6d7caa cmake: Build `bitcoin-tx` executable (Hennadii Stepanov)
d10c5c34c3 cmake: Add wallet functionality (Hennadii Stepanov)
ab2e99b0d9 cmake: Create test suite for `ctest` (Hennadii Stepanov)
959370bd76 cmake: Build `test_bitcoin` executable (Hennadii Stepanov)
b27bf9700d cmake: Build `bitcoin-cli` executable (Hennadii Stepanov)
a9813df826 cmake: Build `bitcoind` executable (Hennadii Stepanov)
97829ce2d5 cmake: Add `FindLibevent` module (Hennadii Stepanov)
3118e40c61 cmake: Build `bitcoin_consensus` library (Hennadii Stepanov)
809a2f1929 cmake: Build `bitcoin_util` static library (Hennadii Stepanov)
0a9a521a70 cmake: Build `bitcoin_crypto` library (Hennadii Stepanov)
958971f476 cmake: Build `univalue` static library (Hennadii Stepanov)
752747fda8 cmake: Generate `obj/build.h` header (Hennadii Stepanov)
1f0a78edf3 cmake: Build `minisketch` static library (Hennadii Stepanov)
12bfbc8154 cmake: Build `leveldb` static library (Hennadii Stepanov)
51985c5304 cmake: Build `crc32c` static library (Hennadii Stepanov)
db7a198f29 cmake: Build `secp256k1` subtree (Hennadii Stepanov)
dbb7ed14e8 cmake: Add `ccache` support (Hennadii Stepanov)
cedfdf6c72 cmake: Redefine/adjust per-configuration flags (Hennadii Stepanov)
b6b5e732c8 cmake: Add global compiler and linker flags (Hennadii Stepanov)
f98327931b cmake: Add `TryAppendLinkerFlag` module (Hennadii Stepanov)
4a0af29697 cmake: Add `TryAppendCXXFlags` module (Hennadii Stepanov)
35cffc497d cmake: Add POSIX threads support (Hennadii Stepanov)
fd72d00ffe cmake: Add position independent code support (Hennadii Stepanov)
07069e2bb0 cmake: Add introspection module (Hennadii Stepanov)
27d687fc1f cmake: Add `config/bitcoin-config.h` support (Hennadii Stepanov)
fe5cdace5f cmake: Print compiler and linker flags in summary (Hennadii Stepanov)
70683884c5 cmake: Introduce interface libraries to encapsulate common flags (Hennadii Stepanov)
a2317e27b7 cmake: Add root `CMakeLists.txt` file (Hennadii Stepanov)
Pull request description:
This PR introduces a new CMake-based build system, which is a drop-in replacement for the current Autotools-based build system.
ML announcement: https://groups.google.com/g/bitcoindev/c/hgKkfQWzrTo
As discussed during the recent CoreDev meetup in April, the switch from Autotools to CMake is intended to happen as soon as possible after branching 28.x off, which means that 29.0 will be built using CMake.
This PR branch is essentially the [staging branch](https://github.com/hebasto/bitcoin/tree/cmake-staging), with every change reviewed and tested by a group of contributors, including (in alphabetical order):
- [**achow101**](https://github.com/achow101)
- [**fanquake**](https://github.com/fanquake)
- [**maflcko**](https://github.com/maflcko)
- [**m3dwards**](https://github.com/m3dwards)
- [**pablomartin4btc**](https://github.com/pablomartin4btc)
- [**real-or-random**](https://github.com/real-or-random)
- [**ryanofsky**](https://github.com/ryanofsky)
- [**sipsorcery**](https://github.com/sipsorcery)
- [**TheCharlatan**](https://github.com/TheCharlatan)
- [**theStack**](https://github.com/theStack)
- [**theuni**](https://github.com/theuni)
- [**vasild**](https://github.com/vasild)
Reviewing in a separate staging repo was suggested in https://github.com/bitcoin/bitcoin/pull/27060#issuecomment-1431798320.
The accompanying changes to the OSS-Fuzz project are available in https://github.com/hebasto/oss-fuzz/pull/8.
Please refer to the [build options parity table](https://gist.github.com/hebasto/2ef97d3a726bfce08ded9df07f7dab5e). The "auto" value is no longer available; non-default values must be specified explicitly. Additionally, the new default values have been chosen to suit the everyday build experience for the majority of developers.
System requirements for using the CMake-based build system:
- CMake >= 3.22 (if not available in your system's repository, it can be downloaded from https://cmake.org/download/)
- a build tool of your choice:
- any Make (GNU Make is no longer a requirement); GNU Make is still required to build depends
- Ninja (https://ninja-build.org/)
- MSBuild
- Xcode
A note for Windows users: The default installation of the latest version of MSVC 17.10.4 includes both CMake 3.28.3 and the vcpkg package manager).
---
We, the build system developers, kindly ask reviewers to refrain from making suggestions that are not directly related to the migration process or can be implemented separately. Bugs in the scripts and errors in the updated documentation should be the focus of this PR. Please be advised that comments not aligned with this PR's goal may be ignored.
Thank you all for your understanding.
ACKs for top commit:
maflcko:
review ACK 41051290ab🐥
sipsorcery:
ACK 41051290ab.
vasild:
ACK 41051290ab
TheCharlatan:
ACK 41051290ab
pablomartin4btc:
tACK 41051290ab
i-am-yuvi:
tACK [`4105129`](41051290ab)
theuni:
ACK 41051290ab.
fanquake:
ACK 41051290ab
Tree-SHA512: 6c1445054436c6c00ad63bfa0f19d64091a2b25c9bd694f85bf2218ac358ffb774d6c000685b3ca1e9b50401babed989fa2a0694b774c211d226bfd1944c9b39
setup_clean_chain=True is deleted so it uses the default.
Also, vout is now returned from send_to_address,
so now there is no need to fetch it manually
Also remove not-needed code that was used with the old
transaction handling.
18d65d2772 test: use uint256::FromUserHex for RANDOM_CTX_SEED (stickies-v)
6819e5a329 node: use uint256::FromUserHex for -assumevalid parsing (stickies-v)
2e58fdb544 util: remove unused IsHexNumber (stickies-v)
8a44d7d3c1 node: use uint256::FromUserHex for -minimumchainwork parsing (stickies-v)
70e2c87737 refactor: add uint256::FromUserHex helper (stickies-v)
85b7cbfcbe test: unittest chainstatemanager_args (stickies-v)
Pull request description:
Since fad2991ba0, `uint256S` has been [deprecated](fad2991ba0 (diff-800776e2dda39116e889839f69409571a5d397de048a141da7e4003bc099e3e2R138)) because it is less robust than the `base_blob::FromHex()` introduced in [the same PR](https://github.com/bitcoin/bitcoin/pull/30482). Specifically, it tries to recover from length-mismatches, recover from untrimmed whitespace, 0x-prefix and garbage at the end, instead of simply requiring exactly 64 hex-only characters. _(see also #30532)_
This PR carves out the few `uint256S` callsites that may potentially prove a bit more controversial to change because they deal with user input and backwards incompatible behaviour change.
The main behaviour change introduced in this PR is:
- `-minimumchainwork` will raise an error when input is longer than 64 hex digits
- `-assumevalid` will raise an error when input contains invalid hex characters, or when it is longer than 64 hex digits
- test: the optional RANDOM_CTX_SEED env var will now cause tests to abort when it contains invalid hex characters, or when it is longer than 64 hex digits
After this PR, the remaining work to remove `uint256S` completely is almost entirely mechanical and/or test related. I will open that PR once #30560 is merged because it builds on that.
ACKs for top commit:
hodlinator:
re-ACK 18d65d2772
l0rinc:
ACK 18d65d2772
achow101:
ACK 18d65d2772
ryanofsky:
Code review ACK 18d65d2772. Very nice change that cleans up the API, adds checking for invalid values, makes parsing of values more consistent, and adds test coverage.
Tree-SHA512: ec118ea3d56e1dfbc4c79acdbfc797f65c4d2107b0ca9577c848b4ab9b7cb8d05db9f3c7fe8441a09291aca41bf671572431f4eddc59be8fb53abbea76bbbf86
fa5b58ea01 test: Avoid intermittent block download timeout in p2p_ibd_stalling (MarcoFalke)
Pull request description:
Fixes#30704
The goal of the test is to check the stalling timeout, not the block download timeout.
On extremely slow hardware (for example qemu virtual hardware), downloading the 1023 blocks may take longer than the block download timeout.
Fix it by pinning the time using mocktime, and only advance it when testing the stalling timeout.
ACKs for top commit:
tdb3:
CR ACK fa5b58ea01
brunoerg:
utACK fa5b58ea01
Tree-SHA512: 9a9221f264bea52be5e9fe81fd319f5a6970cd315cc5e9f5e2e049c5d84619b19b9f6f075cda8d34565c2d6c17a88fb57e195c66c271e40f73119a77caecb6d7
Because we don't have type checking for command-line/settings/config
args, strings are interpreted as 'false' for non-boolean args.
By convention, this "forces" us to interpret negated strings as 'true',
which conflicts with the negated option definition in all the settings
classes (they expect negated options to always be false and ignore any
other value preceding them). Consequently, when retrieving all "wallet"
values from the command-line/settings/config, we also fetch the negated
string boolean value, which is not of the expected 'string' type.
This mismatch leads to an internal fatal error, resulting in an unclean
shutdown during initialization. Furthermore, this error displays a poorly
descriptive error message:
"JSON value of type bool is not of expected type string"
This commit fixes the fatal error by ensuring that only string values are
returned in the "wallet" settings list, failing otherwise. It also improves
the clarity of the returned error message.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
e1d5dd732d test: check xor.dat recreated when missing (tdb3)
d1610962bf test: add null block xor key (tdb3)
1ad999b9da refactor: lift NUM_XOR_BYTES (tdb3)
d8399584dd refactor: move read_xor_key() to TestNode (tdb3)
d43948c3ef refactor: use unlink rather than os.remove (tdb3)
c8176f758b test: add blocks_key_path (tdb3)
Pull request description:
Builds on PR #30657.
Refactors `read_xor_key()` from `util.py` to `test_node.py` (comment https://github.com/bitcoin/bitcoin/pull/30657#discussion_r1723358327)
Adds a check that `xor.dat` is created when missing (comment https://github.com/bitcoin/bitcoin/pull/30657#discussion_r1717724161)
Help states:
```
-blocksxor
Whether an XOR-key applies to blocksdir *.dat files. The created XOR-key
will be zeros for an existing blocksdir or when `-blocksxor=0` is
set, and random for a freshly initialized blocksdir. (default: 1)
```
ACKs for top commit:
maflcko:
ACK e1d5dd732d
achow101:
ACK e1d5dd732d
theStack:
re-ACK e1d5dd732d
brunoerg:
reACK e1d5dd732d
Tree-SHA512: 325912ef646ec88e0a58e1ece263a2b04cbc06497e8fe5fcd603e509e80c6bcf84b09dd52dfac60e23013f07fc2b2f6db851ed0598649c3593f452c4a1424bd9
fa5aeab3cb test: Avoid duplicate curl call in get_previous_releases.py (MarcoFalke)
Pull request description:
Seems odd having to translate `404` to "Binary tag was not found". Also, it seems odd to write a for-loop over a list with one item.
Fix both issues by just using a single call to `curl --fail ...`.
Can be tested with: `test/get_previous_releases.py -b v99.99.99`
Before:
```
Releases directory: releases
Fetching: https://bitcoincore.org/bin/bitcoin-core-99.99.99/bitcoin-99.99.99-x86_64-linux-gnu.tar.gz
% Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed
0 286k 0 0 0 0 0 0 --:--:-- 0:00:01 --:--:-- 0
Binary tag was not found
```
After:
```
Releases directory: releases
Fetching: https://bitcoincore.org/bin/bitcoin-core-99.99.99/bitcoin-99.99.99-x86_64-linux-gnu.tar.gz
% Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed
0 286k 0 0 0 0 0 0 --:--:-- 0:00:01 --:--:-- 0
curl: (22) The requested URL returned error: 404
ACKs for top commit:
fanquake:
ACK fa5aeab3cb
brunoerg:
utACK fa5aeab3cb
tdb3:
tested ACK fa5aeab3cb
Tree-SHA512: d5d31e0bccdd9de9b4a8ecf2e69348f4e8cee773050c8259b61db1ce5de73f6fbfffbe8c4d2571f7bef2de29cb42fd244573deebfbec614e487e76ef41681b9c
Removes dependency on unsafe and deprecated uint256S.
This makes parsing more strict, by returning an error
when the input contains non-hex characters, or when it
contains more than 64 hex digits.
Also make feature_assumevalid.py more robust by using CBlock.hash
which is guaranteed to be 64 characters long, as opposed to the
variable-length hex(CBlock.sha256)
Removes dependency on unsafe and deprecated uint256S.
This makes parsing more strict, by returning an error
when the input contains more than 64 hex digits.
cccc5bfd35 test: Enable detect_leaks=1 in ASAN_OPTIONS explicitly (MarcoFalke)
Pull request description:
It should be enabled by default, but being explicit can't hurt.
ACKs for top commit:
fanquake:
ACK cccc5bfd35
Tree-SHA512: ed284abd05c7a99c30b509844aa75785a5ccb506d8296a71347b4c328750a6a4ed1f87e7a3ec36ab17f27b467c033cc8ca5eb5e2b951f2ae7473327c5eb1ddae
59ff17e5af miner: adjust clock to timewarp rule (Sjors Provoost)
e929054e12 Add timewarp attack mitigation test (Sjors Provoost)
e85f386c4b consensus: enable BIP94 on regtest (Sjors Provoost)
dd154b0568 consensus: lower regtest nPowTargetTimespan to 144 (Sjors Provoost)
Pull request description:
Because #30647 reduced the timewarp attack threshold from 7200s to 600s, our miner code will fail to propose a block template (on testnet4) if the last block of the previous period has a timestamp two hours in the future. This PR fixes that and also adds a test.
The non-test changes in the last commit should be in v28, otherwise miners have to patch it themselves. If necessary I can split that out into a separate PR, but I prefer to get the tests in as well.
In order to add the test, we activate BIP94 on regtest.
In order for the test to run faster, we reduce its difficulty retarget period to 144, the same number that's already used for softfork activation logic. Regtest does not actually adjust its difficulty, so this change has no effect (except for `getnetworkhashps`, see commit).
An alternative approach would be to run this test on testnet4, by hardcoding its first 2015 in the test suite. But since the timewarp mitigation is a serious candidate for a future mainnet softfork, it seems better to just deploy it on regtest.
The next commits add a test and fix the miner code.
The `MAX_TIMEWARP` constant is moved to `consensus.h` so both validation and miner code have access to it.
ACKs for top commit:
achow101:
ACK 59ff17e5af
fjahr:
ACK 59ff17e5af
glozow:
ACK 59ff17e5af
Tree-SHA512: 50af9fdcba9b0d5c57e1efd5feffd870bd11b5318f1f8b0aabf684657f2d33ab108d5f00b1475fe0d38e8e0badc97249ef8dda20c7f47fcc1698bc1008798830
917e70a620 test: assumeutxo: check that UTXO-querying RPCs operate on snapshot chainstate (Sebastian Falbesoner)
Pull request description:
Inspired by some manual testing I did for #28553, this PR checks that RPCs which explicitly query the UTXO set database (i.e. `gettxoutsetinfo`, `scantxoutset` and `gettxout`) operate on the snapshot chainstate as expected.
ACKs for top commit:
fjahr:
utACK 917e70a620
achow101:
ACK 917e70a620
tdb3:
ACK 917e70a620
Tree-SHA512: 40ecd1c5dd879234df1667fa5444a1fbbee9b7c456f597dc982d1a2bce46fe9107711b005ab829e570ef919a4914792f72f342d71d92bad2ae9434b5e68d5bd3
This currently has no effect due to fPowNoRetargeting,
except for the getnetworkhashps when called with -1.
It will when the next commit enforces the timewarp attack mitigation on regtest.
Previous rpcauth behavior was to sometimes
ignore empty -rpcauth= settings, and other times
treat them as errors.
Empty rpcauth is now consistently treated
as an error and prevents bitcoind from starting.
Updates associated test cases.
Also updates to non-deprecated logging macro.
Co-Authored-By: Luke Dashjr <luke-jr+git@utopios.org>
Co-Authored-By: Ryan Ofsky <ryan@ofsky.org>
77ff0ec1f1 contrib: support reading XORed blocks in linearize-data.py script (Sebastian Falbesoner)
Pull request description:
This PR is a small follow-up for #28052, adding support for the block linearization script to handle XORed blocksdir *.dat files. Note that if no xor.dat file exists, the XOR pattern is set to all-zeros, in order to still support blockdirs that have been created with versions earlier than 28.x.
Partly fixes issue #30599.
ACKs for top commit:
achow101:
ACK 77ff0ec1f1
tdb3:
ACK 77ff0ec1f1
hodlinator:
ACK 77ff0ec1f1
Tree-SHA512: 011eb02e2411de373cbbf4b26db4640fc693a20be8c2430529fba6e36a3a3abfdfdc3b005d330f9ec2846bfad9bfbf34231c574ba99289ef37dd51a68e6e7f3d
6b2dcba076 wallet: List sqlite wallets with empty string name (Ava Chow)
3ddbdd1815 wallet: Ignore .bak files when listing wallet files (Ava Chow)
Pull request description:
When the default wallet is migrated, we do not rename the wallet so we end up having a descriptor wallet with the empty string as its name and the wallet.dat file in the root of the walletdir. This is supposed to be an unsupported configuration and there is no other way to achieve this (other than file copying), but the wallet loading code does not disallow loading such wallets. However `listwalletdir` does not currently list the default wallet if it is sqlite. This is confusing to users, so change `listwalletdir` to include these wallets.
Additionally, the migration of the default wallet, and of any plain wallet files in the walletdir, produces a backup file in the walletdir itself. Since these backups are a BDB file, `listwalletdir` will detect them as being another wallet that we could open, but this is erroneous and could lead to confusion and potentially funds loss if both the backup and the migrated wallet are in use simultaneously. To reduce the likelihood of this issue, don't list these wallets in `listwalletdir`.
***
Possibly we could have more stringent checks on loading to resolve these issues, but I'm concerned that that will just confuse users and gratuitously break things that already worked.
Since the original intent was to disallow default wallets for sqlite/descriptors, a possible alternative would be to prevent people from loading such wallets and change migration to rename those wallets. However, given that this behavior with migrating default wallets has existed since default wallet migration was fixed, I think that making such a change would be confusing and break things for no good reason. Although perhaps we should still do the renaming.
For the backups, we could also change loading to refuse to load any wallet named with `.bak` (or `.legacy.bak`) as such wallets can still be loaded by giving the path to them directly, which some users may do to "restore" the backup. However restricting what can be loaded based on filename seems a little heavyhanded. It wouldn't be funds loss though since the correct way to restore the backup is with `restorewallet`.
ACKs for top commit:
fjahr:
Code review ACK 6b2dcba076
furszy:
Code ACK 6b2dcba076
glozow:
ACK 6b2dcba076
Tree-SHA512: 0b033f6ed55830f8a054afea3fb2cf1fa82a94040053ebfaf123bda36c99f45d3f01a2aec4ed02fed9c61bb3d320b047ed892d7f6644b5a356a7bc5974b10cff
00618e8745 assumeutxo: Drop block height from metadata (Fabian Jahr)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/issues/30514 which has more context and shows how the issue can be reproduced. Since the value in question is removed, there is no test to add to reproduce anything.
This is an alternative approach to #30516 with much of the [code being suggested there](https://github.com/bitcoin/bitcoin/pull/30516#discussion_r1689146902).
ACKs for top commit:
maflcko:
re-ACK 00618e8745🎌
achow101:
ACK 00618e8745
theStack:
Code-review ACK 00618e8745
ismaelsadeeq:
Re-ACK 00618e8745
mzumsande:
ACK 00618e8745
Tree-SHA512: db9575247bae838ad7742a27a216faaf55bb11e022f9afdd05752bb09bbf9614717d0ad64304ff5722a16bf41d8dea888af544e4ae26dcaa528c1add0269a4a8
Although it is not explicitly possible to create a default wallet with
descriptors, it is possible to migrate a default wallet and have it end
up being a default wallet with descriptors. These wallets should be
listed by ListDatabases so that it appears in wallet directory listings
to avoid user confusion.
Migration creates backup files in the wallet directory with .bak as the
extension. This pollutes the output of listwalletdir with backup files
that most users should not need to care about.
6bfa26048d testnet: Add timewarp attack prevention for Testnet4 (Fabian Jahr)
0100907ca1 testnet: Add Testnet4 difficulty adjustment rules fix (Fabian Jahr)
74a04f9e7a testnet: Introduce Testnet4 (Fabian Jahr)
Pull request description:
To supplement the [ongoing conceptual discussion about a testnet reset](https://groups.google.com/g/bitcoindev/c/9bL00vRj7OU/m/9yCPo3uUBwAJ) I have drafted a move to v4 including a fix to the difficulty adjustment mechanism, which was part of the motivation that started the discussion.
Conceptual considerations:
- The conceptual discussion about doing a testnet4 or softforking the fix into testnet3 is outside of the scope of this PR and I would ask reviewers to contribute their opinions on this on the ML instead. However, I am happy to adapt this PR to a softfork change on testnet3 if there is consensus for that instead.
- The difficulty adjustment fix suggested here touches the `CalculateNextWorkRequired` function and uses the same logic used in `GetNextWorkRequired` to find the last previous block that was not mined with difficulty 1 under the exceptionf. An alternative fix briefly mentioned on the mailing list by Jameson Lopp would be to "restrict the special testnet minimum difficulty rule so that it can't be triggered on the block right before a difficulty retarget". That would also fix the issue but I find my suggestion here a bit more elegant.
ACKs for top commit:
jsarenik:
tACK 6bfa26048d
achow101:
ACK 6bfa26048d
murchandamus:
tACK 6bfa26048d
Tree-SHA512: 0b8b69a621406a944da5be551b863d065358ba94d85dd3b80d83c412660e230ee93b27316081fbee9b4851cc4ff8585db64c7dfa26cb5148ac835663f2712c3d
Refactor satoshi_round function to accept different rounding modes.
Updated call site to use the revised `satoshi_round` function.
Co-authored-by: Kate Salazar <52637275+katesalazar@users.noreply.github.com>
e9de0a76b9 doc: release note for 30212 (willcl-ark)
87b1880525 rpc: clarify ALREADY_IN_CHAIN rpc errors (willcl-ark)
Pull request description:
Closes: #19363
Renaming this error improves clarity around the returned error both internally and externally when a transactions' outputs are already found in the utxo set (`TransactionError::ALREADY_IN_CHAIN -> TransactionError::ALREADY_IN_UTXO_SET`)
ACKs for top commit:
tdb3:
ACK e9de0a76b9
ismaelsadeeq:
ACK e9de0a76b9
ryanofsky:
Code review ACK e9de0a76b9.
Tree-SHA512: 7d2617200909790340951fe56a241448f9ce511900777cb2a712e8b9c0778a27d1f912b460f82335844224f1abb4322bc898ca076440959edade55c082a09237
fa895c7283 mingw: Document mode wbx workaround (MarcoFalke)
fa359255fe Add -blocksxor boolean option (MarcoFalke)
fa7f7ac040 Return XOR AutoFile from BlockManager::Open*File() (MarcoFalke)
Pull request description:
Currently the *.dat files in the blocksdir store the data received from remote peers as-is. This may be problematic when a program other than Bitcoin Core tries to interpret them by accident. For example, an anti-virus program or other program may scan them and move them into quarantine, or delete them, or corrupt them. This may cause Bitcoin Core to fail a reorg, or fail to reply to block requests (via P2P, RPC, REST, ...).
Fix this, similar to https://github.com/bitcoin/bitcoin/pull/6650, by rolling a random XOR pattern over the dat files when writing or reading them.
Obviously this can only protect against programs that accidentally and unintentionally are trying to mess with the dat files. Any program that intentionally wants to mess with the dat files can still trivially do so.
The XOR pattern is only applied when the blocksdir is freshly created, and there is an option to disable it (on creation), so that people can disable it, if needed.
ACKs for top commit:
achow101:
ACK fa895c7283
TheCharlatan:
Re-ACK fa895c7283
hodlinator:
ACK fa895c7283
Tree-SHA512: c92a6a717da83bc33a9b8671a779eeefde2c63b192362ba1d71e6535ee31d08e2802b74acc908345197de9daac6930e4771595ee25b09acd5a67f7ea34854720
fa530ec543 rpc: Return precise loadtxoutset error messages (MarcoFalke)
faa5c86dbf refactor: Use untranslated error message in ActivateSnapshot (MarcoFalke)
Pull request description:
The error messages should never happen in normal operation. However, if
they do, they are helpful to return to the user to debug the issue. For
example, to notice a truncated file.
This fixes https://github.com/bitcoin/bitcoin/issues/28621
Also includes a minor refactor commit.
ACKs for top commit:
fjahr:
Code review ACK fa530ec543
ryanofsky:
Code review ACK fa530ec543, just adjusting error messages a little since last review. (Thanks!)
Tree-SHA512: 224968c9b13d082ca2ed1f6a8fcc5f51ff16d6c96bd38c3679699505b54337b99cccaf7a8474391f6b11f9ccb101977b4e626898c1217eae95802e290cf105f1
When using `sendrawtransaction` the ALREADY_IN_CHAIN error help string
may be confusing.
Rename TransactionError::ALREADY_IN_CHAIN to
TransactionError::ALREADY_IN_UTXO_SET and update the rpc help string.
Remove backwards compatibility alias as no longer required.
fa3ea3b83c test: Fix intermittent issue in p2p_v2_misbehaving.py (MarcoFalke)
55555574d1 net: Log accepted connection after m_nodes.push_back (MarcoFalke)
Pull request description:
Fix the two issues reported in https://github.com/bitcoin/bitcoin/pull/30468/files#r1688444784:
* Delay a debug log line for consistency.
* Fix an intermittent test issue.
They are completely separate fixes, but both `net` related.
ACKs for top commit:
0xB10C:
Code Review ACK fa3ea3b83c
stratospher:
tested ACK fa3ea3b.
Tree-SHA512: cd6b6e164b317058a305a5c3e38c56c9a814a7469039e1143f1d7addfbc91b0a28506873356b373d97448b46cb6fbe94a1309df82e34c855540b241a09489e8b
75648cea5a test: add P2A ProduceSignature coverage (Greg Sanders)
7998ce6b20 Add release note for P2A output feature (Greg Sanders)
71c9b02a04 test: add P2A coverage for decodescript (Greg Sanders)
1349e9ec15 test: Add anchor mempool acceptance test (Greg Sanders)
9d89209937 policy: stop 3rd party wtxid malleability of anchor spend (Greg Sanders)
b60aaf8b23 policy: make anchor spend standard (Greg Sanders)
455fca86cf policy: Add OP_1 <0x4e73> as a standard output type (Greg Sanders)
Pull request description:
This is a sub-feature taken out of the original proposal for ephemeral anchors #30239
This PR makes *spending* of `OP_1 <0x4e73>` (i.e. `bc1pfeessrawgf`) standard. Creation of this output type is already standard.
Any future witness output types are considered relay-standard to create, but not to spend. This preserves upgrade hooks, such as a completely new output type for a softfork such as BIP341. It also gives us a bit of room to use a new output type for policy uses.
This particular sized witness program has no other known use-cases (https://bitcoin.stackexchange.com/a/110664/17078), s it affords insufficient cryptographic security for a secure commitment to data, such as a script or a public key. This makes this type of output "keyless", or unauthenticated.
As a witness program, the `scriptSig` of the input MUST be blank, by BIP141. This helps ensure txid-stability of the spending transaction, which may be required for smart contracting wallets. If we do not use segwit, a miner can simply insert an `OP_NOP` in the `scriptSig` without effecting the result of program execution.
An additional relay restriction is to disallow non-empty witness data, which an adversary may use to penalize the "honest" transactor when RBF'ing the transaction due to the incremental fee requirement of RBF rules.
The intended use-case for this output type is to "anchor" the transaction with a spending child to bring exogenous CPFP fees into the transaction package, encouraging the inclusion of the package in a block. The minimal size of creation and spending of this output makes it an attractive contrast to outputs like `p2sh(OP_TRUE)` and `p2wsh(OP_TRUE)` which
are significantly larger in vbyte terms.
Combined with TRUC transactions which limits the size of child transactions significantly, this is an attractive option for presigned transactions that need to be fee-bumped after the fact.
ACKs for top commit:
sdaftuar:
utACK 75648cea5a
theStack:
re-ACK 75648cea5a
ismaelsadeeq:
re-ACK 75648cea5a via [diff](e7ce6dc070..75648cea5a)
glozow:
ACK 75648cea5a
tdb3:
ACK 75648cea5a
Tree-SHA512: d529de23d20857e6cdb40fa611d0446b49989eaafed06c28280e8fd1897f1ed8d89a4eabbec1bbf8df3d319910066c3dbbba5a70a87ff0b2967d5205db32ad1e
fa46a1b74b test: Avoid CScript() as default function argument (MarcoFalke)
fadf621825 test: Make leaf_script mandatory when scriptpath is set in TaprootSignatureMsg (MarcoFalke)
Pull request description:
Unlike other function calls in default arguments, CScript should not cause any issues in the tests, because they are const.
However, this change allows to enable the "function-call-in-default-argument (B008)" lint rule, which will help to catch severe test bugs, such as https://github.com/bitcoin/bitcoin/issues/30543#issuecomment-2259260024 .
The lint rule will be enabled in a follow-up, when all violations are fixed.
ACKs for top commit:
instagibbs:
utACK fa46a1b74b
theStack:
lgtm ACK fa46a1b74b
ismaelsadeeq:
Tested ACK fa46a1b74b
Tree-SHA512: bc68b15121d50ead0fc70ad772360a7829908aedeaff8426efcb8a67f33117f67d26b4f5da94fa735dd8de9c9ff65fc10a29323f1b12f238b75486fa7cc32a89
afd237bb5d [fuzz] Harness for version handshake (dergoegge)
a90ab4aec9 scripted-diff: Rename lazily initialized bloom filters (dergoegge)
82de1bc478 [net processing] Lazily initialize m_recent_confirmed_transactions (dergoegge)
fa0c87f19c [net processing] Lazily initialize m_recent_rejects_reconsiderable (dergoegge)
662e8db2d3 [net processing] Lazily initialize m_recent_rejects (dergoegge)
Pull request description:
This adds a fuzzing harness dedicated to the version handshake. To avoid determinism issues, the harness creates necessary components each iteration (addrman, peerman, etc). A harness like this would have easily caught https://bitcoincore.org/en/2024/07/03/disclose-timestamp-overflow/.
As a performance optimization, this PR includes a change to `PeerManager` to lazily initialize various filters (to avoid large unnecessary memory allocations each iteration).
ACKs for top commit:
brunoerg:
ACK afd237bb5d
marcofleon:
Tested ACK afd237bb5d. I compared the coverage of `net_processing` from this harness to the `process_message` and `process_messages` harnesses to see the differences. This target hits more specific parts of the version handshake. The stability looks good as well, at about 94%.
glozow:
utACK afd237bb5d lazy blooms look ok
mzumsande:
Code Review ACK afd237bb5d
Tree-SHA512: 62bba20aec0cd220e62368354891f9790b81ad75e8adf7b22a76a6d4663bd26aedc4cae8083658a75ea9043d60aad3f0e58ad36bd7bbbf93ff1d16e317bf15cc
This does not cause any issues, because CScript in the tests are const.
However, this change allows to enable the
"function-call-in-default-argument (B008)" lint rule.
a6efc7e16e test: fix intermittent failures in feature_proxy.py (Martin Zumsande)
Pull request description:
Fixes#29871
If addnode connections are made with v2transport and the peer immediately disconnects us, reconnections with v1 are scheduled. This could interfere with later checks depending on timing. Avoid this by using `v2transport=False` in the addnode rpc - this test isn't about the message layer anyway, so running it with v2 would add no value.
ACKs for top commit:
maflcko:
ACK a6efc7e16e
tdb3:
cr re ACK a6efc7e16e
Tree-SHA512: 39353a392e75e4c6257d971ceecb65fb76ec6d3b121a087869831c24b767a18f57e2ae2968da445c7fa731cb03053c90df37dd2cd6e86f786ad4121bc68ca235
This removes the default value, because there should not be a use-case
to fall back to a an empty leaf_script by default. (If there was, it
could trivially be added back)
In python, if the default value is a mutable object (here: a class)
its shared over all instances, so that one instance being changed
would affect others to be changed as well.
This was likely the source of various intermittent bugs in the
functional tests.
If addnode connections are made with v2transport and the peer immediately disconnects us, reconnections
with v1 are scheduled. This could interfere with later checks depending on timing. Avoid this by using
`v2transport=False` in the addnode rpc - this test isn't about the message layer anyway, so running it
with v2 would add no value.
17845e7f21 rpc: add utxo's blockhash and number of confirmations to scantxoutset output (Luis Schwab)
Pull request description:
This PR resolves#30478 by adding two fields to the `scantxoutset` RPC:
- blockhash: the blockhash that an UTXO was created
- confirmations: the number of confirmations an UTXO has relative to the chaintip.
The rationale for the first field is that a blockhash is a much more reliable identifier than the height:
> When using the scantxoutset RPC, the current behaviour is to show the block height of the UTXO. This is not optimal, as block height is ambiguous, especially in the case of a block reorganization happening at the same instant of the query. In this case, an UTXO that does not exist would be assumed to exist, unless the chain's tip hash is recorded before the scan, and make sure it still exists after, as per https://github.com/bitcoindevkit/bdk/issues/895#issuecomment-1475766797 comment by evanlinjin.
The second one was suggested by maflcko, and I agree it's useful for human users:
> While touching this, another thing to add could be the number of confirmations? I understand that this wouldn't help machine consumers of the interface, but human callers may find it useful?
This will yield an RPC output like so:
```diff
bitcoin-cli scantxoutset start "[\"addr(bc1q5q9344vdyjkcgv79ve3tldz4jmx4lf7knmnx6r)\"]"
{
"success": true,
"txouts": 185259116,
"height": 853622,
"bestblock": "00000000000000000002e97d9be8f0ddf31829cf873061b938c10b0f80f708b2",
"unspents": [
{
"txid": "fae435084345fe26e464994aebc6544875bca0b897bf4ce52a65901ae28ace92",
"vout": 0,
"scriptPubKey": "0014a00b1ad58d24ad8433c56662bfb45596cd5fa7d6",
"desc": "addr(bc1q5q9344vdyjkcgv79ve3tldz4jmx4lf7knmnx6r)#smk4xmt7",
"amount": 0.00091190,
"coinbase": false,
"height": 852741,
+ "blockhash": "00000000000000000002eefe7e7db44d5619c3dace4c65f3fdcd2913d4945c13",
+ "confirmations": 882
}
],
"total_amount": 0.00091190
}
```
ACKs for top commit:
sipa:
utACK 17845e7f21
Eunovo:
ACK 17845e7f21
tdb3:
ACK 17845e7f21
Tree-SHA512: 02366d0004e5d547522115ef0efe6794a35978db53dda12c675cfae38197bf43f0bf89ca99a3d79e3d2cff95186015fe1ab764abb8ab82bda440ae9302ad973b
The error messages should never happen in normal operation. However, if
they do, they are helpful to return to the user to debug the issue. For
example, to notice a truncated file.
e4b0dabb21 test: add functional test for tagged MiniWallet instances (Sebastian Falbesoner)
3162c917e9 test: fix MiniWallet internal key derivation for tagged instances (Sebastian Falbesoner)
c9f7364ab2 test: fix MiniWallet script-path spend (missing parity bit in leaf version) (Sebastian Falbesoner)
7774c314fb test: refactor: return TaprootInfo from P2TR address creation routine (Sebastian Falbesoner)
Pull request description:
This PR fixes a dormant bug in MiniWallet that exists since support for P2TR was initially added in #23371 (see commit 041abfebe4).
In the course of spending the output, the leaf version byte of the control block in the witness stack doesn't set the parity bit, i.e. we were so far just lucky that the used combinations of relevant data (internal pubkey, leaf script / version) didn't result in a tweaked pubkey with odd y-parity. If that was the case, we'd get the following validation error:
`mandatory-script-verify-flag-failed (Witness program hash mismatch) (-26)`
Since MiniWallets can now optionally be tagged (#29939), resulting in different internal pubkeys, the issue is more prevalent now. Fix it by passing the parity bit, as specified in BIP341.
Can be tested with the following patch (fails on master, succeeds on PR):
```diff
diff --git a/test/functional/test_framework/mempool_util.py b/test/functional/test_framework/mempool_util.py
index 148cc935ed..7ebe858681 100644
--- a/test/functional/test_framework/mempool_util.py
+++ b/test/functional/test_framework/mempool_util.py
@@ -42,7 +42,7 @@ def fill_mempool(test_framework, node):
# Generate UTXOs to flood the mempool
# 1 to create a tx initially that will be evicted from the mempool later
# 75 transactions each with a fee rate higher than the previous one
- ephemeral_miniwallet = MiniWallet(node, tag_name="fill_mempool_ephemeral_wallet")
+ ephemeral_miniwallet = MiniWallet(node, tag_name="fill_mempool_ephemeral_wallet3")
test_framework.generate(ephemeral_miniwallet, 1 + num_of_batches * tx_batch_size)
# Mine enough blocks so that the UTXOs are allowed to be spent
```
In addition to that, another bug is fixed where the internal key derivation failed, as not every pseudorandom hash results in a valid x-only pubkey. Fix this by treating the hash result as private key and calculate the x-only public key out of that, to be used then as internal key.
Fixes#30528.
ACKs for top commit:
glozow:
ACK e4b0dabb21
rkrux:
reACK [e4b0dab](e4b0dabb21)
hodlinator:
ACK e4b0dabb21
Tree-SHA512: a16f33f76bcb1012857cc3129438a9f6badf28aa2b1d25696da0d385ba5866b46de0f1f93ba777ed9263fe6952f98d7d9c44ea0c0170a2bcc86cbef90bf6ac58
fac0c3d4bf doc: Add release notes for two pull requests (MarcoFalke)
fa7b57e5f5 refactor: Replace ParseHashStr with FromHex (MarcoFalke)
fa90777245 rest: Reject truncated hex txid early in getutxos parsing (MarcoFalke)
fab6ddbee6 refactor: Expose FromHex in transaction_identifier (MarcoFalke)
fad2991ba0 refactor: Implement strict uint256::FromHex() (MarcoFalke)
fa103db2bb scripted-diff: Rename SetHex to SetHexDeprecated (MarcoFalke)
fafe4b8051 test: refactor: Replace SetHex with uint256 constructor directly (MarcoFalke)
Pull request description:
In `rest_getutxos` truncated txids such as `aa` or `ff` are accepted. This is brittle at best.
Fix it by rejecting any truncated (or overlarge) input.
----
Review note: This also starts a major refactor to rework hex parsing in Bitcoin Core, meaning that a few refactor commits are included as well. They are explained individually in the commit message and the work will be continued in the future.
ACKs for top commit:
stickies-v:
re-ACK fac0c3d4bf - only doc and test updates to address review comments, thanks!
hodlinator:
ACK fac0c3d4bf
Tree-SHA512: 473feb3fcf6118443435d1dd321006135b0b54689bfbbcb1697bb5811a449bef51f475c715de6911ff3c4ea3bdb75f601861ff93347bc4414d6b9e5298105dd7
25bf86a225 [test]: ensure `estimatesmartfee` default mode is `economical` (ismaelsadeeq)
41a2545046 [fees]: change `estimatesmartfee` default mode to `economical` (ismaelsadeeq)
Pull request description:
Fixes#30009
This PR changes the `estimatesmartfee` default mode to `economical`.
This was also suggested on IRC https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2024-04-26#1021609
- `conservative` mode: This is the `estimatesmartfee` RPC mode which considers a longer history of blocks. It potentially returns a higher fee rate and is more likely to be sufficient for the desired target, but it is not as responsive to short-term drops in the prevailing fee market.
- `economical` mode: This is the `estimatesmartfee` RPC mode where estimates are potentially lower and more responsive to short-term drops in the prevailing fee market.
Since users are likely to use the default mode, this change will reduce overestimation for many users. The conservative mode remains available for those who wish to opt-in.
For an in-depth analysis of how significantly the `conservative` mode overestimates, see
https://delvingbitcoin.org/t/bitcoind-policy-estimator-modes-analysis/964.
ACKs for top commit:
instagibbs:
reACK 25bf86a225
glozow:
ACK 25bf86a225
willcl-ark:
ACK 25bf86a225
Tree-SHA512: 78ebda667eb9c8f87dcc2f0e6c14968bd1de30358dc77a13611b186fb8427ad97d9f537bad6e32e0a1aa477ccd8c64fee4d41e19308ef3cb184ff1664e6ba8a6
Without the fix, the test could fail intermittently. For example:
node0 2024-07-22T16:31:54.104994Z [httpworker.0] [rpc/request.cpp:232] [parse] [rpc] ThreadRPCServer method=setmocktime user=__cookie__
test 2024-07-22T16:31:54.291000Z TestFramework (INFO): Sending first 4 bytes of ellswift which match network magic
test 2024-07-22T16:31:54.292000Z TestFramework (INFO): If a response is received, assertion failure would happen in our custom data_received() function
test 2024-07-22T16:31:54.292000Z TestFramework.p2p (DEBUG): Connecting to Bitcoin Node: 127.0.0.1:12644
test 2024-07-22T16:31:54.293000Z TestFramework.p2p (DEBUG): Connected & Listening: 127.0.0.1:12644
test 2024-07-22T16:31:54.588000Z TestFramework.p2p (DEBUG): sending 4050 bytes of garbage data
test 2024-07-22T16:31:54.588000Z TestFramework (INFO): Sending remaining ellswift and garbage which are different from V1_PREFIX. Since a response is
test 2024-07-22T16:31:54.588000Z TestFramework (INFO): expected now, our custom data_received() function wouldn't result in assertion failure
node0 2024-07-22T16:31:55.523868Z (mocktime: 2024-07-22T16:31:54Z) [net] [net.cpp:3764] [CNode] [net] Added connection peer=0
node0 2024-07-22T16:31:55.625145Z (mocktime: 2024-07-22T16:31:54Z) [net] [net.cpp:1814] [CreateNodeFromAcceptedSocket] [net] connection from 127.0.0.1:45154 accepted
node0 2024-07-22T16:31:55.625769Z (mocktime: 2024-07-22T16:31:54Z) [http] [httpserver.cpp:305] [http_request_cb] [http] Received a POST request for / from 127.0.0.1:33320
node0 2024-07-22T16:31:55.626543Z (mocktime: 2024-07-22T16:31:54Z) [httpworker.1] [rpc/request.cpp:232] [parse] [rpc] ThreadRPCServer method=getpeerinfo user=__cookie__
test 2024-07-22T16:31:55.818000Z TestFramework (ERROR): Unexpected exception caught during testing
Traceback (most recent call last):
File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 132, in main
self.run_test()
File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/p2p_v2_misbehaving.py", line 133, in run_test
self.test_earlykeyresponse()
File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/p2p_v2_misbehaving.py", line 151, in test_earlykeyresponse
self.wait_until(lambda: node0.getpeerinfo()[-1]["bytesrecv"] > 4)
File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 791, in wait_until
return wait_until_helper_internal(test_function, timeout=timeout, timeout_factor=self.options.timeout_factor)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/util.py", line 289, in wait_until_helper_internal
if predicate():
^^^^^^^^^^^
File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/p2p_v2_misbehaving.py", line 151, in <lambda>
self.wait_until(lambda: node0.getpeerinfo()[-1]["bytesrecv"] > 4)
~~~~~~~~~~~~~~~~~~~^^^^
IndexError: list index out of range
d63ef73800 test: Add loadtxoutset test with tip on snapshot block (Fabian Jahr)
c2f86d4bcb test: Remove already resolved assumeutxo todo comments (Fabian Jahr)
Pull request description:
The first commit removes three Todos that have been addressed previously (see commit message for details).
The second message resolves another todo by adding the missing test case. This is a special case of "the tip has more work than the snapshot" where the tip is the same block as the snapshot base block.
Related to #28648.
ACKs for top commit:
jrakibi:
ACK [d63ef73](d63ef73800)
achow101:
ACK d63ef73800
maflcko:
ACK d63ef73800
alfonsoromanz:
Re ACK d63ef73800
Tree-SHA512: 8d5a25fc0b26531db3a9740132694138f2103b7b42eeb1d4a64095bfc901c1372e23601c0855c7def84c8a4e185d10611e4e830c4e479f1b663ae6ed53abb130
a8e3af1a82 qa: Do not assume running `feature_asmap.py` from source directory (Hennadii Stepanov)
9bf7ca6cad qa: Consider `cache` and `config.ini` relative to invocation directory (Hennadii Stepanov)
a0473442d1 scripted-diff: Add `__file__` argument to `BitcoinTestFramework.init()` (Hennadii Stepanov)
Pull request description:
This PR includes changes split from https://github.com/bitcoin/bitcoin/pull/30454. They improve the functional test framework, allowing users to [run individual functional tests](https://github.com/hebasto/bitcoin/issues/146) from the build directory in the new CMake-based build system.
This functionality is not available for out-of-source builds using the current Autotools-based build system, which always requires write permissions for the source directory. Nevertheless, this PR can be tested as suggested in https://github.com/bitcoin/bitcoin/pull/30463#issuecomment-2232618421:
1. Make an out-of-source build:
```
$ ./autogen.sh
$ mkdir ../build && cd ../build
$ ../bitcoin/configure
$ make
```
2. Create a symlink in the build directory to a functional test:
```
$ ln --symbolic ../../../bitcoin/test/functional/wallet_disable.py ./test/functional/
```
3. Run this symlink:
```
$ ./test/functional/wallet_disable.py
```
The last command fails on the master branch:
```
Traceback (most recent call last):
File "/home/hebasto/git/build/./test/functional/wallet_disable.py", line 31, in <module>
DisableWalletTest().main()
^^^^^^^^^^^^^^^^^^^
File "/home/hebasto/git/bitcoin/test/functional/test_framework/test_framework.py", line 106, in __init__
self.parse_args()
File "/home/hebasto/git/bitcoin/test/functional/test_framework/test_framework.py", line 210, in parse_args
config.read_file(open(self.options.configfile))
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
FileNotFoundError: [Errno 2] No such file or directory: '/home/hebasto/git/bitcoin/test/config.ini'
```
and succeeds with this PR.
ACKs for top commit:
maflcko:
tested ACK a8e3af1a82🎨
glozow:
ACK a8e3af1a82, tested with the steps in op
stickies-v:
ACK a8e3af1a82
Tree-SHA512: 899e4efc09edec13ea3f5b47825d03173fb21d3569c360deda7fa6a56b99b4d24e09ad4f0883bad1ee926b1c706e47ba07c6a6160c63c07c82b3cf4ae5816e91
Also pulls out the guarding assert and calls it explicitly before the test function is called. This is already done before the existing call of the test function so it was not needed there.
- "Valid snapshot file, but referencing a snapshot block that turns out
to be invalid, or has an invalid parent" has been addressed in #30267
- "An ancestor of snapshot block" - If chain tip refers to blocks in this context then any successful load is addressing this because if we have synced past the snapshot base block we fail because we don't need assumeutxo anymore. And if this is about headers then this is the `test_headers_not_synced()` case.
- "A descendant of the snapshot block" - If this refers to blocks the
`test_snapshot_with_less_work()` addressed this and if it is just headers in this case again it would be represented in all of the successful loads in the test.
Co-authored-by: Alfonso Roman Zubeldia <alfonsoromanz24@gmail.com>
55b6d7be68 validation: Don't load a snapshot if it's not in the best header chain. (Martin Zumsande)
Pull request description:
This was suggested by me in the discussion of #30288, which has more context.
If the snapshot is not an ancestor of the most-work header (`m_best_header`), syncing from that alternative chain leading to `m_best_header` should be prioritised. Therefore it's not useful loading the snapshot in this situation.
If the other chain turns out to be invalid or the chain with the snapshot retrieves additional headers so that it's the most-work one again (see functional test), `m_best_header` will change and loading the snapshot will be possible again.
Because of the work required to generate a conflicting headers chain, a situation with two conflicting chains should only be possible under extreme circumstances, such as major forks.
ACKs for top commit:
fjahr:
re-ACK 55b6d7be68
achow101:
ACK 55b6d7be68
alfonsoromanz:
Re ACK 55b6d7be68
Tree-SHA512: 4fbea5ab1038ae353fc949a186041cf9b397e7ce4ac59ff36f881c9437b4f22ada922490ead5b2661389eb1ca0f3d1e7e7e6a4261057678643e71594a691ac36
fac932bf93 refactor: Use util::Split to avoid a harmless unsigned-integer-overflow (MarcoFalke)
fab54db9f1 rest: Reject negative outpoint index in getutxos parsing (MarcoFalke)
Pull request description:
In `rest_getutxos` outpoint indexes such as `+N` or `-N` are accepted. This should be harmless, because any index out of range should be treated as a non-existent utxo. However, a negative index can't exist ever, so it seems better to reject all signs, whether `+` or `-`.
ACKs for top commit:
achow101:
ACK fac932bf93
hodlinator:
ut-ACK fac932bf93
tdb3:
re ACK fac932bf93
danielabrozzoni:
ACK fac932bf93
brunoerg:
reACK fac932bf93
Tree-SHA512: 8f1a75248cb61e1c4beceded6ed170db83b07f30fbcf93a26acfffc00ec4546572366eff87907a7e1423d7d3a2a9e57a0a7a9bacb787c86463f842d7161c16bc
faed5d3870 test: Non-Shy version sender (MarcoFalke)
Pull request description:
After `add_outbound_p2p_connection`, the test framework normally sends a version message only in reply to a received version. This is fine, but the protocol does not require this and tolerates a version to be sent earlier.
However, this is untested, and the missing test coverage leads to bugs being missed. For example https://github.com/bitcoin/bitcoin/pull/30394#pullrequestreview-2166824948
Fix it by adding a test.
ACKs for top commit:
brunoerg:
ACK faed5d3870
tdb3:
ACK faed5d3870
theStack:
tACK faed5d3870
glozow:
ACK faed5d3870
Tree-SHA512: dbf527a39c932e994a1e8248ba78058000811a4bf69275278f1fd1e545716ac4d2d3be5dcf362976bbafa2a49f91d13e3601daf71d29e9c556179b01af62c03c
734076c6de [wallet, rpc]: add `max_tx_weight` to tx funding options (ismaelsadeeq)
b6fc5043c1 [wallet]: update the data type of `change_output_size`, `change_spend_size` and `tx_noinputs_size` to `int` (ismaelsadeeq)
baab0d2d43 [doc]: update reason for deducting change output weight (ismaelsadeeq)
7f61d31a5c [refactor]: update coin selection algorithms input parameter `max_weight` name (ismaelsadeeq)
Pull request description:
This PR taken over from #29264
The PR added an option `max_tx_weight` to transaction funding RPC's that ensures the resulting transaction weight does not exceed the specified `max_tx_weight` limit.
If `max_tx_weight` is not given `MAX_STANDARD_TX_WEIGHT` is used as the max threshold.
This PR addressed outstanding review comments in #29264
For more context and rationale behind this PR see https://delvingbitcoin.org/t/lightning-transactions-with-v3-and-ephemeral-anchors/418/11?u=instagibbs
ACKs for top commit:
achow101:
ACK 734076c6de
furszy:
utACK 734076c6de
rkrux:
reACK [734076c](734076c6de)
Tree-SHA512: 013501aa443d239ee2ac01bccfc5296490c27b4edebe5cfca6b96c842375e895e5cfeb5424e82e359be581460f8be92095855763a62779a18ccd5bdfdd7ddce7
