e8c31f135c tests: Test for bumping single output transaction (Andrew Chow)
4f4d4407e3 test: Test bumpfee reduce_output (Andrew Chow)
7d83502d3d bumpfee: Allow original change position to be specified (Andrew Chow)
Pull request description:
When bumping the transaction fee, we will try to find the change output of the transaction in order to have an output whose value we can modify so that we can meet the target feerate. However we do not always find the change output which can cause us to unnecessarily add an additional output to the transaction. We can avoid this issue by outsourcing the determination of change to the user if they so desire.
This PR adds a `orig_change_pos` option to bumpfee which the user can use to specify the index of the change output.
Fixes#11233Fixes#20795
ACKs for top commit:
ismaelsadeeq:
re ACK e8c31f135c
pinheadmz:
re-ACK e8c31f135c
furszy:
Code review ACK e8c31f13
Tree-SHA512: 3a230655934af17f7c1a5953fafb5ef0d687c21355cf284d5e98fece411f589cd69ea505f06d6bdcf82836b08d268c366ad2dd30ae3d71541c9cdf94d1f698ee
This fixes a bug in the linter:
"""
Python's open(...) seems to be used to open text files without explicitly specifying encoding='utf8':
test/functional/test_framework/test_node.py: with open(self.debug_log_path, **kwargs) as dl:
"""
read() fails in text mode when the unicode hasn't been fully written
yet. Fixes: "wallet_importdescriptors.py: can't decode bytes in position
228861-228863: unexpected end of data"
(https://github.com/bitcoin/bitcoin/issues/28030)
20b49460b3 test: remove race in the user-agent reception check (Vasil Dimov)
Pull request description:
In `add_p2p_connection()` we connect to `bitcoind` from the Python client and check that it has received our version string.
This check looked up the last/newest entry from `getpeerinfo` RPC, assuming that it must be the connection we have just opened. But this will not be the case if a new inbound or outbound connection is made to/from `bitcoind` in the meantime.
Instead of the last entry in `getpeerinfo`, check all and find the one which corresponds to our connection using our outgoing address:port tuple which is unique.
ACKs for top commit:
jonatack:
re-ACK 20b49460b3
MarcoFalke:
Concept ACK 20b49460b3
Tree-SHA512: 61fd3359ef11ea830021ede0e745497a7b60690c32d21c47cd12ff79f78907bb45e922c9d01e5d7ff614a8cd5e4560d39a3fc86b01b200429773a23ace3917e4
Simultaneously opening the file in read and write mode would
lead to opening of an empty file instead of perturbing the existing
file.
Also, revert to the previous state after each perturbation so that
each perturbation is applied in isolation.
fa2f18ad8e ci: Use DOCKER_BUILDKIT for lint image (MarcoFalke)
Pull request description:
Currently the lint docker/podman image has many issues:
* It relies on an EOL debian version.
* It relies on a debian version different from the one used in the CI lint task.
* It relies on the legacy docker build command, which requires the user to make `cd ./ci/lint/` before the build step.
* It doesn't use the `.python-version` file, but a hardcoded version.
Fix all issues by using the recommended `DOCKER_BUILDKIT=1` to generate the image.
Also:
* Rename `/tmp/python` to `/python_build`.
* Compress all `pip install` commands into one.
* Bump `.python-version`.
ACKs for top commit:
jamesob:
ACK fa2f18ad8e
Tree-SHA512: 804b384904ad753845667998841cc7825f4229933ca2c42af021384713486ec3cca80ba58612d37557fba7ee1921439dacca5e1236aac0557dd75bd9a2f1875d
c7db88af71 descriptor: assert we never parse a sane miniscript with no pubkey (Antoine Poinsot)
a49402a9ec qa: make sure we don't let unspendable Miniscript descriptors be imported (Antoine Poinsot)
639e3b6c97 descriptor: refuse to parse unspendable miniscript descriptors (Antoine Poinsot)
e3280eae1b miniscript: make GetStackSize() and GetOps() return optionals (Antoine Poinsot)
Pull request description:
`IsSane()` in Miniscript does not ensure a Script is actually spendable. This is an issue as we would accept any sane Miniscript when parsing a descriptor. Fix this by explicitly checking a Miniscript descriptor is both sane and spendable when parsing it.
This bug was exposed due to a check added in #22838 (https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1226859880) that triggered a fuzz crash (https://github.com/bitcoin/bitcoin/pull/22838#issuecomment-1612510057).
ACKs for top commit:
sipa:
utACK c7db88af71
achow101:
ACK c7db88af71
Tree-SHA512: e79bc9f7842e98a4e8f358f05811fca51b15b4b80a171c0d2b17cf4bb1f578a18e4397bc2ece9817d392e0de0196ee6a054b7318441fd3566dd22e1f03eb64a5
faf8be7c32 test: Disable known broken USDT test (MarcoFalke)
Pull request description:
The failure is known and running into more failures doesn't help anyone. Not disabling the test would be a waste of CPU and developer time.
https://github.com/bitcoin/bitcoin/issues/27380
Top commit has no ACKs.
Tree-SHA512: d0469153b00d6b30e10a21bcd52d508fcf9f796ff2468f59aff75020a82c718bcae85caf4b58397dea6fd9e210b501353fd51567f979c6b57d3b1bb23d318216
5cf44275c8 test: refactor: deduplicate legacy ECDSA signing for tx inputs (Sebastian Falbesoner)
Pull request description:
There are several instances in functional tests and the framework (MiniWallet, feature_block.py, p2p_segwit.py) where we create a legacy ECDSA signature for a certain transaction's input by doing the following steps:
1. calculate the `LegacySignatureHash` with the desired sighash type
2. create the actual digital signature by calling `ECKey.sign_ecdsa` on the signature message hash calculated above
3. put the DER-encoded result as CScript data push into tx input's scriptSig
Create a new helper `sign_input_legacy` which hides those details and takes only the necessary parameters (tx, input index, relevant scriptPubKey, private key, sighash type [SIGHASH_ALL by default]). For further convenience, the signature is prepended to already existing data-pushes in scriptSig, in order to avoid rehashing the transaction after calling the new signing function.
ACKs for top commit:
dimitaracev:
ACK `5cf4427`
achow101:
ACK 5cf44275c8
pinheadmz:
ACK 5cf44275c8
Tree-SHA512: 8f0e4fb2c3e0f84fac5dbc4dda87973276242b0f628034272a7f3e45434c1e17dd1b26a37edfb302dcaf380dbfe98b0417391ace5e0ac9720155d8fba702031e
faf902858d test: Check expected_stderr after stop (MarcoFalke)
Pull request description:
This fixes a bug where stderr wasn't checked for the shutdown sequence.
Fix that by waiting for the shutdown to finish and then check stderr.
ACKs for top commit:
theStack:
ACK faf902858d
Tree-SHA512: a70cd1e6cda84d542782e41e8b59741dbcd472c0d0575bcef5cbfd1418473ce94efe921481d557bae3fbbdd78f1c49c09c48872883c052d87c5c9a9a51492692
The Socks5 server we use in the test framework would disconnect
by default immediately after the handshake and sometimes would
not register as a connected peer by bitcoind.
By moving the 'StartIndexes()' call into the 'initload'
thread, we can remove the threads active wait. Optimizing
the available resources.
The only difference with the current state is that now the
indexes threads will only be started when they can process
work and not before it.
The thread does not only load blocks, it loads the mempool and,
in a future commit, will start the indexes as well.
Also, renamed the 'ThreadImport' function to 'ImportBlocks'
And the 'm_load_block' class member to 'm_thread_load'.
-BEGIN VERIFY SCRIPT-
sed -i "s/ThreadImport/ImportBlocks/g" $(git grep -l ThreadImport -- ':!/doc/')
sed -i "s/loadblk/initload/g" $(git grep -l loadblk -- ':!/doc/release-notes/')
sed -i "s/m_load_block/m_thread_load/g" $(git grep -l m_load_block)
-END VERIFY SCRIPT-
7ecc29a0b7 test: wallet, add coverage for addressbook migration (furszy)
a277f8357a wallet: migration bugfix, persist empty labels (furszy)
1b64f6498c wallet: migration bugfix, clone 'send' record label to all wallets (furszy)
Pull request description:
Addressing two specific bugs encountered during the wallet migration process, related to the address book, and improves the test coverage for it.
Bug 1: Non-Cloning of External 'Send' Records
The external 'send' records were not being correctly cloned to all wallets.
Bug 2: Persistence of Empty Labels
As address book entries without associated db label records can be treated as change (the `label` field inside the `CAddressBookData` class is optional, `nullopt` labels make `CAddressBookData ::IsChange()` return true), we must persist empty labels during the migration process.
The user might have called `setlabel` with an "" string for an external address and that must be retained during migration.
ACKs for top commit:
achow101:
ACK 7ecc29a0b7
Tree-SHA512: b8a8483a4178a37c49af11eb7ba8a82ca95e54a6cd799e155e33f9fbe7f37b259e28372c77d6944d46b6765f9eaca6b8ca8d1cdd9d223120a3653e4e41d0b6b7
fa1e27fe8e fuzz: Generate rpc fuzz targets individually (MarcoFalke)
Pull request description:
The `rpc` fuzz target was added more than two years ago in e45863166f. However, the bug https://github.com/bitcoin/bitcoin/issues/27913 was only found recently. Thus, it is pretty clear that fuzz engines can't deal with a search space that is too broad and can be extended in too many directions.
Fix that by limiting the search space to each RPC method name and then iterate over all names, instead of letting the fuzz engine do the iteration.
With this, the bug can be found in seconds, as opposed to years of CPU time (or never).
ACKs for top commit:
brunoerg:
ACK fa1e27fe8e
dergoegge:
ACK fa1e27fe8e
Tree-SHA512: 45ccba842367650d010320603153276b1b303deda9ba8c6bb31a4d2473b00aa5bca866db95f541485d65efd8276e2575026968c037872ef344fa33cf45bcdcd7
6eb33bd0c2 kernel: Add fatalError method to notifications (TheCharlatan)
7320db96f8 kernel: Add flushError method to notifications (TheCharlatan)
3fa9094b92 scripted-diff: Rename FatalError to FatalErrorf (TheCharlatan)
edb55e2777 kernel: Pass interrupt reference to chainman (TheCharlatan)
e2d680a32d util: Add SignalInterrupt class and use in shutdown.cpp (TheCharlatan)
Pull request description:
Get rid of all `ShutdownRequested` calls in validation code by introducing an interrupt object that applications can use to cancel long-running kernel operations.
Replace all `AbortNode` calls in validation code with new fatal error and flush error notifications so kernel applications can be notified about failures and choose how to handle them.
---
This pull request is part of the `libbitcoinkernel` project https://github.com/bitcoin/bitcoin/issues/27587https://github.com/orgs/bitcoin/projects/3 and more specifically its "Step 2: Decouple most non-consensus code from libbitcoinkernel".
The pull request mostly allows dropping the kernel dependency on shutdown.cpp. The only dependency left after this is a `StartShutdown` call which will be removed in followup PR https://github.com/bitcoin/bitcoin/pull/27711. This PR also drops the last reference to the `uiInterface` global in kernel code. The process of moving the `uiInterface` out of the kernel was started in https://github.com/bitcoin/bitcoin/pull/27636.
This pull request contains a subset of patches originally proposed in #27711. It will be part of a series of changes required to make handling of interrupts (or in other words the current shutdown procedure) in the kernel library more transparent and less reliable on global mutable state. The set of patches contained here was originally proposed by @ryanofsky [here](https://github.com/bitcoin/bitcoin/pull/27711#issuecomment-1580779869).
ACKs for top commit:
achow101:
light ACK 6eb33bd0c2
hebasto:
ACK 6eb33bd0c2, I have reviewed the code and it looks OK.
ryanofsky:
Code review ACK 6eb33bd0c2. No changes since last review other than rebase.
Tree-SHA512: 7d2d05fa4805428a09466d43c11ae32946cbb25aa5e741b1eec9cd142e4de4bb311e13ebf1bb125ae490c9d08274f2d56c93314e10f3d69e7fec7445e504987c
In `add_p2p_connection()` we connect to `bitcoind` from the Python
client and check that it has received our version string.
This check looked up the last/newest entry from `getpeerinfo` RPC,
assuming that it must be the connection we have just opened. But this
will not be the case if a new inbound or outbound connection is made
to/from `bitcoind` in the meantime.
Instead of the last entry in `getpeerinfo`, check all and find the one
which corresponds to our connection using our outgoing address:port
tuple which is unique.
Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
Co-authored-by: Jon Atack <jon@atack.com>
8fbb6e99bf wallet: Give deprecation warning when loading a legacy wallet (Andrew Chow)
Pull request description:
Next step in legacy wallet deprecation.
ACKs for top commit:
S3RK:
reACK 8fbb6e99bf
jonatack:
re-ACK 8fbb6e99bf
Tree-SHA512: 902984b09452926cf199f06e5fb56e4985325cdd5e0dcc829992158488f42d5fbc33e9a30a29303feac24c8315193e8d31712022e2a0503abd6b67169a0027f4
There are several instances in functional tests and the framework
(MiniWallet, feature_block.py, p2p_segwit.py) where we create a legacy
ECDSA signature for a certain transaction's input by doing the following
steps:
1) calculate the `LegacySignatureHash` with the desired sighash type
2) create the actual digital signature by calling `ECKey.sign_ecdsa`
on the signature message hash calculated above
3) put the DER-encoded result as CScript data push into
tx input's scriptSig
Create a new helper `sign_input_legacy` which hides those details and
takes only the necessary parameters (tx, input index, relevant
scriptPubKey, private key, sighash type [SIGHASH_ALL by default]). For
further convenience, the signature is prepended to already existing
data-pushes in scriptSig, in order to avoid rehashing the transaction
after calling the new signing function.
4f4d039a98 test: add ellswift test vectors from BIP324 (stratospher)
a31287718a test: Add ellswift unit tests (stratospher)
714fb2c02a test: Add python ellswift implementation to test framework (stratospher)
Pull request description:
Built on top of https://github.com/bitcoin/bitcoin/pull/26222.
This PR introduces Elligator swift encoding and decoding in the functional test framework. It's used in #24748 for writing p2p encryption tests.
ACKs for top commit:
sipa:
ACK 4f4d039a98
theStack:
ACK 4f4d039a98🐊
Tree-SHA512: 32bc8e88f715f2cd67dc04cd38db92680872072cb3775478e2c30da89aa2da2742992779ea14da2f1faca09228942cfbd86d6957402b24bf560244b389e03540
6c97757a48 script: appease spelling linter (Jon Atack)
1316119ce7 script: update ignored-words.txt (Jon Atack)
146c861da2 script: update linter dependencies (Jon Atack)
92408224a4 test: fix PEP484 no implicit optional argument types errors (Jon Atack)
f86a301433 script, test: add missing python type annotations (Jon Atack)
Pull request description:
With these updates, `./test/lint/lint-python.py` and `./test/lint/lint-spelling.py` should be green again for developers using relatively recent Python dependencies, in particular mypy 0.991 (released 11/2022) and later. Please see the commit messages for details.
ACKs for top commit:
fanquake:
ACK 6c97757a48
Tree-SHA512: 8a46a4d36d5978affdcecf4f2ace20ca1b52d483e098304911a2169afe60ccb9b042fa90c04b762d94f3ce53d2cafe6f24476ae839867a770c7f31e7e7242d99
Fix warnings for these files when ./test/lint/lint-python.py is run using
mypy 0.991 (released 11/2022) and later:
$ test/lint/lint-python.py
test/functional/test_framework/coverage.py:23: error: Incompatible default for argument "coverage_logfile" (default has type "None", argument has type "str") [assignment]
test/functional/test_framework/coverage.py:23: note: PEP 484 prohibits implicit Optional. Accordingly, mypy has changed its default to no_implicit_optional=True
test/functional/test_framework/util.py:318: error: Incompatible default for argument "timeout" (default has type "None", argument has type "int") [assignment]
test/functional/test_framework/util.py:318: note: PEP 484 prohibits implicit Optional. Accordingly, mypy has changed its default to no_implicit_optional=True
test/functional/test_framework/util.py:318: error: Incompatible default for argument "coveragedir" (default has type "None", argument has type "str") [assignment]
test/functional/interface_rest.py:67: error: Incompatible default for argument "query_params" (default has type "None", argument has type "dict[str, Any]") [assignment]
test/functional/interface_rest.py:67: note: PEP 484 prohibits implicit Optional. Accordingly, mypy has changed its default to no_implicit_optional=True
Verified using https://github.com/hauntsaninja/no_implicit_optional
For details, see:
https://mypy-lang.blogspot.com/2022/11/mypy-0990-released.html
Fix warnings for these files when ./test/lint/lint-python.py is run using
mypy 0.991 (released 11/2022) and later:
"By default the bodies of untyped functions are not checked, consider using
--check-untyped-defs [annotation-unchecked]"
For details, see:
https://mypy-lang.blogspot.com/2022/11/mypy-0990-released.html
fae7c50d20 test: Run fuzz tests on macOS (MarcoFalke)
Pull request description:
Any reason not to?
ACKs for top commit:
jamesob:
Github ACK fae7c50d20
dergoegge:
utACK fae7c50d20
Tree-SHA512: e45122d73fafb17cea312258314b826cb0745e08daadd28465f687ec02d4c127d2f8cbe20179a9fff5712038850c02c968abb4838fa088b7555e28709317d3a3
aaaa3aefbd test: Use TestNode *_path properties where possible (MarcoFalke)
dddd89962b test: Allow pathlib.Path as RPC argument via authproxy (MarcoFalke)
fa41614a0a scripted-diff: Use wallets_path and chain_path where possible (MarcoFalke)
fa493fadfb test: Use wallet_dir lambda in wallet_multiwallet test where possible (MarcoFalke)
Pull request description:
It seems inconsistent, fragile and verbose to:
* Call `get_datadir_path` to recreate the path that already exists as field in TestNode
* Call `os.path.join` with the hardcoded chain name or `self.chain` to recreate the TestNode `chain_path` property
* Sometimes even use the hardcoded node dir name (`"node0"`)
Fix all issues by using the TestNode properties.
ACKs for top commit:
willcl-ark:
re-ACK aaaa3aefbd
theStack:
Code-review ACK aaaa3aefbd🌊
Tree-SHA512: e4720278085beb8164e1fe6c1aa18f601558a9263494ce69a83764c1487007de63ebb51d1b1151862dc4d5b49ded6162a5c1553cd30ea1c28627d447db4d8e72
d4fb58ae8a test: EC: optimize scalar multiplication of G by using lookup table (Sebastian Falbesoner)
1830dd8820 test: add secp256k1 module with FE (field element) and GE (group element) classes (Pieter Wuille)
Pull request description:
This PR rewrites a portion of `test_framework/key.py`, in a compatible way, by introducing classes that encapsulate field element and group element logic, in an attempt to be more readable and reusable.
To maximize readability, the group element logic does not use Jacobian coordinates. Instead, group elements just store (affine) X and Y coordinates directly. To compensate for the performance loss this causes, field elements are represented as fractions. This undoes most, but not all, of the performance loss, and there is a few % slowdown (as measured in `feature_taproot.py`, which heavily uses this).
The upside is that the implementation for group laws (point doubling, addition, subtraction, ...) is very close to the mathematical description of elliptic curves, and this extends to potential future extensions (e.g. ElligatorSwift as needed by #27479).
ACKs for top commit:
achow101:
ACK d4fb58ae8a
theStack:
re-ACK d4fb58ae8a
stratospher:
tested ACK d4fb58a. really liked how this PR makes the secp256k1 code in the tests more intuitive and easier to follow!
Tree-SHA512: 9e0d65d7de0d4fb35ad19a1c19da7f41e5e1db33631df898c6d18ea227258a8ba80c893dab862b0fa9b0fb2efd0406ad4a72229ee26d7d8d733dee1d56947f18
FatalError replaces what previously was the AbortNode function in
shutdown.cpp.
This commit is part of the libbitcoinkernel project and further removes
the shutdown's and, more generally, the kernel library's dependency on
interface_ui with a kernel notification method. By removing interface_ui
from the kernel library, its dependency on boost is reduced to just
boost::multi_index. At the same time it also takes a step towards
de-globalising the interrupt infrastructure.
Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
Co-authored-by: TheCharlatan <seb.kung@gmail.com>
This is done in preparation for the next commit where a new FatalError
function is introduced. FatalErrorf follows common convention to append
'f' for functions accepting format arguments.
-BEGIN VERIFY SCRIPT-
sed -i 's/FatalError/FatalErrorf/g' $( git grep -l 'FatalError')
-END VERIFY SCRIPT-
32e2ffc393 Remove the syscall sandbox (fanquake)
Pull request description:
After initially being merged in #20487, it's no-longer clear that an internal syscall sandboxing mechanism is something that Bitcoin Core should have/maintain, especially when compared to better maintained/supported alterantives, i.e [firejail](https://github.com/netblue30/firejail).
There is more related discussion in #24771.
Note that given where it's used, the sandbox also gets dragged into the kernel.
If it's removed, this should not require any sort of deprecation, as this was only ever an opt-in, experimental feature.
Closes#24771.
ACKs for top commit:
davidgumberg:
crACK 32e2ffc393
achow101:
ACK 32e2ffc393
dergoegge:
ACK 32e2ffc393
Tree-SHA512: 8cf71c5623bb642cb515531d4a2545d806e503b9d57bfc15a996597632b06103d60d985fd7f843a3c1da6528bc38d0298d6b8bcf0be6f851795a8040d71faf16
On my machine, this speeds up the functional test feature_taproot.py by
a factor of >1.66x (runtime decrease from 1m16.587s to 45.334s).
Co-authored-by: Pieter Wuille <pieter@wuille.net>
54877253c8 test: avoid sporadic MINIMALDATA failure in feature_taproot.py (fixes#27595) (Sebastian Falbesoner)
Pull request description:
The functional test feature_taproot.py fails in some rare cases on the execution of the following `"branched_codesep"` spending script (can be reproduced via `$ ./test/functional/feature_taproot.py --randomseed 9048710178866422833` on master / 137a98c5a2):
9d85c03620/test/functional/feature_taproot.py (L741)
The problem occurs if the first data-push (having random content with a random length in the range [0, 510]) has a length of 1 and the single byte has value of [1...16] or [-1]; in this case, the data-push is not minimally encoded by test framework's CScript class (i.e. doesn't use the special op-codes OP_1...OP_16 or OP_1NEGATE) and the script interpreter throws an SCRIPT_ERR_MINIMALDATA error:
```
test_framework.authproxy.JSONRPCException: non-mandatory-script-verify-flag (Data push larger than necessary) (-26)
```
Background: the functional test framework's CScript class translates passed bytes/bytearrays always to data pushes using OP_PUSHx/OP_PUSHDATA{1,2,4} op-codes (see `CScript.__coerce_instance(...)`). E.g. the expression `CScript(bytes([1]))` yields `bytes([OP_PUSH1, 1])` instead of the minimal-encoded `bytes([OP_1])`.
Fix this by adapting the random-size range to [2,...], i.e. never pass byte-arrays below length two to be pushed.
Closes#27595.
ACKs for top commit:
instagibbs:
ACK 54877253c8
sipa:
utACK 54877253c8
achow101:
ACK 54877253c8
Tree-SHA512: 3ffad89b2c3985c20702242192e744c9b10188bff880efaf3c38424a00fa07bd4608d8c948678ff9cdbb4e1e5b06696c7f55407ee10bb05edbb3ee03aa599cdc
30778124b8 net: Give seednodes time before falling back to fixed seeds (Martin Zumsande)
Pull request description:
`-seednode` is an alternative bootstrap mechanism - when choosing it, we make a `AddrFetch` connection to the specified peer, gather addresses from them, and then disconnect. Presumably, if users specify a seednode they prefer addresses from that node over fixed seeds.
However, when disabling dns seeds and specifiying `-seednode`, `CConnman::ProcessAddrFetch()` immediately removes the entry from `m_addr_fetches` (before the seednode could give us addresses) - and once `m_addr_fetches` is empty, `ThreadOpenConnections` will add fixed seeds, resulting in a "race" between the fixed seeds and seednodes filling up AddrMan.
This PR suggests to check for any provided `-seednode` arg instead of using the size of `m_addr_fetches`, thus delaying the querying of fixed seeds for 1 minute when specifying any seednode (as we already do for `addnode` peers).
That way, we actually give the seednodes a chance for to provide us with addresses before falling back to fixed seeds.
This can be tested with `bitcoind -debug=net -dnsseed=0 -seednode=(...)` on a node without `peers.dat` and observing the debug log.
ACKs for top commit:
ajtowns:
utACK 30778124b8
achow101:
ACK 30778124b8
dergoegge:
Code review ACK 30778124b8
sr-gi:
ACK [3077812](30778124b8) with a tiny nit, feel free to ignore it
Tree-SHA512: 96446eb34c0805f10ee158a00a3001a07029e795ac40ad5638228d426e30e9bb836c64ac05d145f2f9ab23ec5a528f3a416e3d52ecfdfb0b813bd4b1ebab3c01
Even though we expect these functions to only produce one event,
we still keep a counter to check if that's true. By simply storing
all the events, we can remove the counters and make debugging
easier, by allowing pdb to access the events.
By storing the events instead of doing the comparison inside the
handle_utxocache_* functions, we simplify the overall logic and
potentially making debugging easier, by allowing pdb to access the
events.
Mostly a refactor, but changes logging behaviour slightly by not
raising and not calling self.log.exception("Assertion failed")
61f4b9b7ad Manage exceptions in bcc callback functions (virtu)
Pull request description:
Address #27380 (and similar future issues) by handling failed `assert_equal()` assertions in bcc callback functions
### Problem
Exceptions are not propagated in ctype callback functions used by bcc. This means an AssertionError exception raised by `assert_equal()` to signal a failed assertion is not getting caught and properly logged. Instead, the error is logged to stdout and execution of the callback stops.
The current workaround to check whether all `assert_equal()` assertions in a callback succeeded is to increment a success counter after the assertions (which only gets incremented if none exception is raised and stops execution). Then, outside the callback, the success counter can be used to check whether a callback executed successfully.
One issue with the described workaround is that when an exception occurs, there is no way of telling which of the `assert_equal()` statements caused the exception; moreover, there is no way of inspecting how the pieces of data that got compared in `assert_equal()` differed (often a crucial clue when debugging what went wrong).
This problem is happening in #27380: Sporadically, in the `mempool:rejected` test, execution does not reach the end of the callback function and the success counter is not incremented. Thus, the test fails when comparing the counter to its expected value of one. Without knowing which of the asserts failed any why it failed, this issue is hard to debug.
### Solution
Two fixes come to mind. The first involves having the callback function make event data accessible outside the callback and inspecting the event using `assert_equal()` outside the callback. This solution still requires a counter in the callback in order to tell whether a callback was actually executed or if instead the call to perf_buffer_poll() timed out.
The second fix entails wrapping all relevant `assert_equal()` statements inside callback functions into try-catch blocks and manually logging AssertionErrors. While not as elegant in terms of design, this approach can be more pragmatic for more complex tests (e.g., ones involving multiple events, events of different types, or the order of events).
The solution proposed here is to select the most pragmatic fix on a case-by-case basis: Tests in `interface_usdt_net.py`, `interface_usdt_mempool.py` and `interface_usdt_validation.py` have been refactored to use the first approach, while the second approach was chosen for `interface_usdt_utxocache.py` (partly to provide a reference for the second approach, but mainly because the utxocache tests are the most intricate tests, and refactoring them to use the first approach would negatively impact their readability). Lastly, `interface_usdt_coinselection.py` was kept unchanged because it does not use `assert_equal()` statements inside callback functions.
ACKs for top commit:
0xB10C:
Reviewed the changes since my last review. ACK 61f4b9b7ad. I've tested that the combined log contains both exceptions by modifying `interface_usdt_utxocache.py`.
willcl-ark:
utACK 61f4b9b
stickies-v:
utACK 61f4b9b7a
Tree-SHA512: 85cdaabf370d4f09a9eab6af9ce7c796cd9d08cb91f38f021f71adda34c5f643331022dd09cadb95be2185dad6016c95cbb8942e41e4fbd566a49bf431c5141a
Also, fix a few bugs:
* Error: RPC command "enumeratesigners" not found in RPC_COMMANDS_SAFE_FOR_FUZZING or RPC_COMMANDS_NOT_SAFE_FOR_FUZZING. Please update test/fuzz/rpc.cpp.
* in run_once: ...format(" ".join(result.args), ... TypeError: sequence item 2: expected str instance, PosixPath found
28fff06afe test: Make linter to look for `BOOST_ASSERT` macros (Hennadii Stepanov)
47fe551e52 test: Kill `BOOST_ASSERT` (Hennadii Stepanov)
Pull request description:
One of the goals of https://github.com/bitcoin/bitcoin/pull/27783 was to get rid of the `BOOST_ASSERT` macros instead of including the `boost/assert.hpp` headers. See https://github.com/bitcoin/bitcoin/pull/27783#discussion_r1210612717.
It turns out that a couple of those macros sneaked into the codebase in https://github.com/bitcoin/bitcoin/pull/27790.
This PR makes the linter guard against new instances of the `BOOST_ASSERT` macros and replaces the current ones.
ACKs for top commit:
kevkevinpal:
ACK [28fff06](28fff06afe)
stickies-v:
ACK 28fff06af
TheCharlatan:
ACK 28fff06afe
Tree-SHA512: 371f613592cf677afe0196d18c83943c6c8f1e998f57b4ff3ee58bfeff8636e4dac1357840d8611b4f7b197def94df10fe1a8ca3282b00b7b4eff4624552dda8
1a572ce7d6 test: refactor: introduce `generate_keypair` helper with WIF support (Sebastian Falbesoner)
Pull request description:
In functional tests it is a quite common scenario to generate fresh elliptic curve keypairs, which is currently a bit cumbersome as it involves multiple steps, e.g.:
privkey = ECKey()
privkey.generate()
privkey_wif = bytes_to_wif(privkey.get_bytes())
pubkey = privkey.get_pubkey().get_bytes()
Simplify this by providing a new `generate_keypair` helper function that returns the private key either as `ECKey` object or as WIF-string (depending on the boolean `wif` parameter) and the public key as byte-string; these formats are what we mostly need (currently we don't use `ECPubKey` objects from generated keypairs anywhere).
With this, most of the affected code blocks following the pattern above can be replaced by one-liners, e.g.:
privkey, pubkey = generate_keypair(wif=True)
Note that after this commit, the only direct uses of `ECKey` remain in situations where we want to set the private key explicitly, e.g. in MiniWallet (test/functional/test_framework/wallet.py) or the test for the signet miner script (test/functional/tool_signet_miner.py).
ACKs for top commit:
instagibbs:
ACK 1a572ce7d6
kevkevinpal:
reACK [1a572ce](1a572ce7d6)
stratospher:
ACK 1a572ce7. neat to have this since keypair generation is done in lots of places.
Tree-SHA512: ceb695ba7b34dc9f65357b55be03e67609e7e13a178083d405284eff4d8d3c5cea4fb0b6632658604a533f38ebfefc33e0c375995cc21ebc7843442ad764287b
0000f55293 ci: Run fuzz target even if input folder is empty (MarcoFalke)
Pull request description:
This should catch trivial integer sanitizer bugs if the author and all reviewers forget to look for them.
ACKs for top commit:
brunoerg:
reACK 0000f55293
dergoegge:
reACK 0000f55293
Tree-SHA512: f139b9d56f0cf1aae339c2890721c77c88d1fea77b73d492c1386ec99b4f393c5b664029919ff4a22e4e8a2929f085699a148c6acc2cc3e40df8a72fd39ff474
Instead of passing the datadir and chain name to os.path.join, just use
the existing properties, which are the same.
-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended 's|\.datadir, self\.chain, .wallets.|.wallets_path|g' $(git grep -l '\.datadir, self\.chain,')
sed -i --regexp-extended 's|\.datadir, self\.chain,|.chain_path,|g' $(git grep -l '\.datadir, self\.chain,')
-END VERIFY SCRIPT-
Seems odd to hardcode all parent directory names in the path for no good
reason.
Also, add wallet_path property to TestNode.
Also, rework wallet_backup.py test for scripted-diff in the next commit.
fa76f0d0ef refactor: Make m_count_with_* in CTxMemPoolEntry int64_t, drop UBSAN supp (MarcoFalke)
Pull request description:
This is a refactor as long as no signed integer overflow appears. In normal operation and absent bugs, signed integer overflow should never happen in the touched code paths.
The main benefit of this refactor is to drop the file-wide ubsan suppression `unsigned-integer-overflow:txmempool.cpp`.
For now, this only changes the internal private representation and the publicly returned type remains `uint64_t`.
ACKs for top commit:
glozow:
ACK fa76f0d0ef
ryanofsky:
Code review ACK fa76f0d0ef
Tree-SHA512: a09e33a915d60c65d369d44ba1a45ce4a6a76e6dc2bea43216ba02b5eab0b74e214b2c7cc44360493f2c483d18d96e4636b7a75b23050976efc80e38de852c39
a1e653828b test: Add test for migrating default wallet and plain file wallet (Andrew Chow)
bdbe3fd76b wallet: Generated migrated wallet's path from walletdir and name (Andrew Chow)
Pull request description:
This PR fixes an assertion error that is hit during the setup of the new database during migration of a wallet that was not contained in a wallet dir. Also added a test for this case as well as one for migrating the default wallet.
ACKs for top commit:
ryanofsky:
Code review ACK a1e653828b
furszy:
ACK a1e65382
Tree-SHA512: 96b218c0de8567d8650ec96e1bf58b0f8ca4c4726f5efc6362453979b56b9d569baea0bb09befb3a5aed8d16d29bf75ed5cd8ffc432bbd4cbcad3ac5574bc479
daa5a658c0 refactor: rename BCLog::BLOCKSTORE to BLOCKSTORAGE (Jon Atack)
cf622b214b doc: release note re raising on invalid -debug/debugexclude/loglevel (Jon Atack)
6cb1c66041 init: remove config option names from translated -loglevel strings (Jon Atack)
2547829272 test: -loglevel raises on invalid values (Jon Atack)
a9c295888b init: raise on invalid loglevel config option (Jon Atack)
b0c3995393 test: -debug and -debugexclude raise on invalid values (Jon Atack)
4c3c19d943 init: raise on invalid debug/debugexclude config options (Jon Atack)
Pull request description:
and rename BCLog::BLOCKSTORE to BLOCKSTORAGE so the enum is the same as its value like the other BCLog enums.
Per discussion in bitcoin-core-dev IRC today from https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2023-05-11#921458.
ACKs for top commit:
achow101:
ACK daa5a658c0
ryanofsky:
Code review ACK daa5a658c0. Just translated string template cleanup since last review
pinheadmz:
re-ACK daa5a658c0
Tree-SHA512: 4c107a93d8e8ce4e2ee81d44aec672526ca354ec390b241221067f68204beac8b4ba7a65748bcfa124ff2245c4307fa9243ec4fe0b464d0fa69c787fb322c3cc
d2b39e09bc test: ensure old fee_estimate.dat not read on restart and flushed (ismaelsadeeq)
cf219f29f3 tx fees, policy: read stale fee estimates with a regtest-only option (ismaelsadeeq)
3eb241a141 tx fees, policy: do not read estimates of old fee_estimates.dat (ismaelsadeeq)
5b886f2b43 tx fees, policy: periodically flush fee estimates to fee_estimates.dat (ismaelsadeeq)
Pull request description:
Fixes#27555
The issue arises when an old `fee_estimates.dat` file is sometimes read during initialization.
Or after an unclean shutdown, the latest fee estimates are not flushed to `fee_estimates.dat`.
If the fee estimates in the old file are old, they can cause transactions to become stuck in the mempool.
This PR ensures that nodes do not use stale estimates from the old file during initialization. If `fee_estimates.dat`
has not been updated for 60 hours or more, it is considered stale and will not be read during initialization. To avoid
having old estimates, the `fee_estimates.dat` file will be flushed periodically every hour. As mentioned #27555
> "The immediate improvement would be to store fee estimates to disk once an hour or so to reduce the chance of having an old file. From there, this case could probably be detected, and refuse to serve estimates until we sync."
In addition, I will follow-up PR to persist the `mempoolminfee` across restarts.
ACKs for top commit:
willcl-ark:
ACK d2b39e09bc
instagibbs:
reACK d2b39e09bc
glozow:
ACK d2b39e09bc. One nit if you follow up.
Tree-SHA512: 4f6e0c296995d0eea5cf80c6aefdd79b7295a6a0ba446f2166f32afc105fe4f831cfda1ad3abd13c5c752b4fbea982cf4b97eaeda2af1fd7184670d41edcfeec
In functional tests it is a quite common scenario to generate fresh
elliptic curve keypairs, which is currently a bit cumbersome as it
involves multiple steps, e.g.:
privkey = ECKey()
privkey.generate()
privkey_wif = bytes_to_wif(privkey.get_bytes())
pubkey = privkey.get_pubkey().get_bytes()
Simplify this by providing a new `generate_keypair` helper function that
returns the private key either as `ECKey` object or as WIF-string
(depending on the boolean `wif` parameter) and the public key as
byte-string; these formats are what we mostly need (currently we don't
use `ECPubKey` objects from generated keypairs anywhere).
With this, most of the affected code blocks following the pattern above
can be replaced by one-liners, e.g.:
privkey, pubkey = generate_keypair(wif=True)
Note that after this commit, the only direct uses of `ECKey` remain in
situations where we want to set the private key explicitly, e.g. in
MiniWallet (test/functional/test_framework/wallet.py) or the test for
the signet miner script (test/functional/tool_signet_miner.py).
Exceptions are not propagated in ctype callback functions used by bcc.
This means an AssertionError exception raised by check_equal() to signal
a failed assertion is not getting caught and properly logged. Instead,
the error is logged to stdout and execution of the handler stops.
The current workaround to check whether all check_equal() assertions in
a callback succeeded is to increment a success counter after the
assertions (which only gets incremented if none exception is raised and
stops execution). Then, outside the callback, the success counter can be
used to check whether a callback executed successfully.
One issue with the described workaround is that when an exception
occurs, there is no way of telling which of the check_equal() statements
caused the exception; moreover, there is no way of inspecting how the
pieces of data that got compared in check_equal() differed (often
a crucial clue when debugging what went wrong).
Two fixes to this problem come to mind. The first involves having the
callback function make event data accessible outside the callback and
inspecting the event using check_equal() outside the callback. This
solution still requires a counter in the callback to tell whether
a callback was actually executed or if instead the call to
perf_buffer_poll() timed out.
The second fix entails wrapping all relevant check_equal() statements
inside callback functions into try-catch blocks and manually logging
AssertionErrors. While not as elegant in terms of design, this approach
can be more pragmatic for more complex tests (e.g., ones involving
multiple events, events of different types, or the order of events).
The solution proposed here is to select the most pragmatic fix on
a case-by-case basis: Tests in interface_usdt_net.py,
interface_usdt_mempool.py and interface_usdt_validation.py have been
refactored to use the first approach, while the second approach was
chosen for interface_usdt_utxocache.py (partly to provide a reference
for the second approach, but mainly because the utxocache tests are the
most intricate tests, and refactoring them to use the first approach
would negatively impact their readability). Lastly,
interface_usdt_coinselection.py was kept unchanged because it does not
use check_equal() statements inside callback functions.
5524fa00fa doc: add release note about removal of `deprecatedrpc=walletwarningfield` flag (Sebastian Falbesoner)
5c77db7354 Restorewallet/createwallet help documentation fixups/improvements (Jon Atack)
a00ae31fcc rpc: remove deprecated "warning" field from {create,load,restore,unload}wallet (Sebastian Falbesoner)
Pull request description:
The "warning" string field for wallet creating/loading RPCs (`createwallet`, `loadwallet`, `unloadwallet` and `restorewallet`) has been deprecated with the configuration option `-deprecatedrpc=walletwarningfield` in PR #27279 (released in v25.0). For the next release v26.0, the field and the configuration option can be removed.
ACKs for top commit:
achow101:
ACK 5524fa00fa
jonatack:
ACK 5524fa00fa
Tree-SHA512: 8212f72067d08095304018b8a95d2ebef630004b65123483fbbfb078cc5709c2d825bbc35b16ea5f6b28ae7377347382d7e9afaf7bdbf0575d2c229d970784de
After initially being merged in #20487, it's no-longer clear that an
internal syscall sandboxing mechanism is something that Bitcoin Core
should have/maintain, especially when compared to better
maintained/supported alterantives, i.e firejail.
Note that given where it's used, the sandbox also gets dragged into the
kernel.
There is some related discussion in #24771.
This should not require any sort of deprecation, as this was only ever
an opt-in, experimental feature.
Closes#24771.
a97c59f12d test: p2p: check misbehavior for non-continuous headers messages (Sebastian Falbesoner)
Pull request description:
This PR adds missing test coverage for a peer sending a `headers` message where the headers don't connect to each other, which should be treated as misbehaving (not disconnecting though, as the score increase is only 20). The relevant code path is `PeerManagerImpl::ProcessHeadersMessage` -> `PeerManagerImpl::CheckHeadersPoW` -> `PeerManagerImpl::CheckHeadersAreContinuous`:
17acb2782a/src/net_processing.cpp (L2415-L2419)17acb2782a/src/net_processing.cpp (L2474-L2484)
ACKs for top commit:
sr-gi:
ACK a97c59f12d
achow101:
ACK a97c59f12d
instagibbs:
ACK a97c59f12d
Tree-SHA512: 3f8d6a2492e5c8b63c7b11be2e4ec455f83581b2c58f2d4e705baadfe8d7c6377296d6cd0eda679d291a13d8930b09443f8e3d183795df34b780c703d5d3aeb3
This commit adds tests to ensure that old fee_estimates.dat files
are not read and that fee_estimates are periodically flushed to the
fee_estimates.dat file.
Additionaly it tests the -regtestonly option -acceptstalefeeestimates.
This is a refactor as long as no signed integer overflow appears. In
normal operation and absent bugs, signed integer overflow should never
happen in the touched code paths.
The main benefit of this refactor is to drop the file-wide ubsan
suppression unsigned-integer-overflow:txmempool.cpp.
For now, this only changes the internal private representation and the
publicly returned type remains uint64_t.
cdba23db35 wallet: Document blank flag use in descriptor wallets (Ryan Ofsky)
43310200dc wallet: Ensure that the blank wallet flag is unset after imports (Andrew Chow)
e9379f1ffa rpc, wallet: Include information about blank flag (Andrew Chow)
Pull request description:
The `blank` wallet flag is used to indicate that the wallet intentionally does not have any keys, scripts, or descriptors, and it prevents the automatic generation of those things for such a wallet. Once the wallet contains any of those data, it is unnecessary, and possibly incorrect, to have `blank` set. This PR fixes a few places where this was not properly happening. It also adds a test for this unset behavior.
ACKs for top commit:
S3RK:
reACK cdba23db35
ryanofsky:
Code review ACK cdba23db35. Only change since last review is dropping the commit which makes createwallet RPC set BLANK flag automatically when DISABLE_PRIVATE_KEYS flag is set
Tree-SHA512: 85bc2a9754df0531575d5c8f4ad7e8f38dcd50083dc29b3283dacf56feae842e81f34654c5e1781f2dadb0560ff80e454bbc8ca3b2d1fab1b236499ae9abd7da
3ef756a5b5 Remove txmempool implicit-integer-sign-change sanitizer suppressions (Hennadii Stepanov)
d2f6d2a95a Use `int32_t` type for most transaction size/weight values (Hennadii Stepanov)
Pull request description:
From bitcoin/bitcoin#23957 which has been incorporated into this PR:
> A file-wide suppression is problematic because it will wave through future violations, potentially bugs.
>
> Fix that by using per-statement casts.
>
> This refactor doesn't change behavior because the now explicit casts were previously done implicitly.
>
> Similar to commit 8b5a4de904
ACKs for top commit:
achow101:
ACK 3ef756a5b5
0xB10C:
ACK 3ef756a5b5. I've focused my testing and code review on the tracepoint related changes. The docs, the test, and the mempool_monitor.py demo script are updated. I ran the `interface_usdt_mempool.py` test and the `mempool_monitor.py` script. The `mempool_monitor.py` output looks correct.
Xekyo:
codereview ACK 3ef756a5b5
ryanofsky:
Code review ACK 3ef756a5b5. Since last review, just rebased with more type changes in test and tracing code
Tree-SHA512: 397407f72165b6fb85ff1794eb1447836c4f903efed1a05d7a9704c88aa9b86f330063964370bbd59f6b5e322e04e7ea8e467805d58dce381e68f7596433330f
ee2417ed61 test: fix intermittent failure in p2p_leak_tx.py (Martin Zumsande)
Pull request description:
Fixes#27860
The problem was that the replacement tx `tx_b` would sometimes be sent out to the inbound peer after the `notfound`, so that threre would be an unexpected `tx` message and the test fails.
```
node0 2023-06-12T12:48:24.903204Z [msghand] [net.cpp:2856] [PushMessage] [net] sending notfound (73 bytes) peer=1
node0 2023-06-12T12:48:24.903916Z [msghand] [net.cpp:2856] [PushMessage] [net] sending tx (133 bytes) peer=1
File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/p2p_leak_tx.py", line 74, in test_notfound_on_replaced_tx
assert "tx" not in inbound_peer.last_message
```
Fix this by letting the peer wait for the initial broadcast of the replacement tx before continuing with the test.
ACKs for top commit:
MarcoFalke:
lgtm ACK ee2417ed61
Tree-SHA512: ecc8fb44cac6097a949e4ee622f6f654f49851d7966359532ab3af4c5ed9d587bf08110820b473a616cde3ae6fc8d0da9bb3cee39347655a8c433e819d4d1065
7d452d826a test: add coverage for `/deploymentinfo` passing a blockhash (brunoerg)
ce887eaf49 rest: bugfix, fix crash error when calling `/deploymentinfo` (brunoerg)
Pull request description:
Calling `/deploymentinfo` passing a valid blockhash makes bitcoind to crash. It happens because we're pushing a JSON value of type array when it expects type object. See:
```cpp
jsonRequest.params = UniValue(UniValue::VARR);
```
```cpp
jsonRequest.params.pushKV("blockhash", hash_str);
```
This PR fixes it by changing `pushKV` to `push_back` and adds more test coverage.
ACKs for top commit:
achow101:
ACK 7d452d826a
stickies-v:
ACK 7d452d826a
Tree-SHA512: f01551e556aba2380c3eaed0bc59057304302c202d317d7c1eec5f7ef839851f672aed80819a8719cb1cbbad2aad735d6d44314ac7d6d98bff8217f5a16c312b
61c569ab60 refactor: decouple early return commands from AppInit (furszy)
4927167f85 gui: return EXIT_FAILURE on post-init fatal errors (furszy)
3b2c61e819 Return EXIT_FAILURE on post-init fatal errors (furszy)
3c06926cf2 refactor: index: use `AbortNode` in fatal error helper (Sebastian Falbesoner)
9ddf7e03a3 move ThreadImport ABC error to use AbortNode (furszy)
Pull request description:
It seems odd to return `EXIT_SUCCESS` when the node aborted execution due a fatal internal error
or any post-init problem that triggers an unrequested shutdown.
e.g. blocks or coins db I/O errors, disconnect block failure, failure during thread import (external
blocks loading process error), among others.
ACKs for top commit:
TheCharlatan:
ACK 61c569ab60
ryanofsky:
Code review ACK 61c569ab60
pinheadmz:
ACK 61c569ab60
theStack:
Code-review ACK 61c569ab60
Tree-SHA512: 18a59c3acc1c6d12cbc74a20a401e89659740c6477fccb59070c9f97922dfe588468e9e5eef56c5f395762187c34179a5e3954aa5b844787fa13da2e666c63d3
It seems odd to return `EXIT_SUCCESS` when the node aborted
execution due a fatal internal error or any post-init problem
that triggers an unrequested shutdown.
e.g. blocks or coins db I/O errors, disconnect block failure,
failure during thread import (external blocks loading process
error), among others.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
67b7fecacd [mempool] clear mapDeltas entry if prioritisetransaction sets delta to 0 (glozow)
c1061acb9d [functional test] prioritisation is not removed during replacement and expiry (glozow)
0e5874f0b0 [functional test] getprioritisedtransactions RPC (glozow)
99f8046829 [rpc] add getprioritisedtransactions (glozow)
9e9ca36c80 [mempool] add GetPrioritisedTransactions (glozow)
Pull request description:
Add an RPC to get prioritised transactions (also tells you whether the tx is in mempool or not), helping users clean up `mapDeltas` manually. When `CTxMemPool::PrioritiseTransaction` sets a delta to 0, remove the entry from `mapDeltas`.
Motivation / Background
- `mapDeltas` entries are never removed from mapDeltas except when the tx is mined in a block or conflicted.
- Mostly it is a feature to allow `prioritisetransaction` for a tx that isn't in the mempool {yet, anymore}. A user can may resbumit a tx and it retains its priority, or mark a tx as "definitely accept" before it is seen.
- Since #8448, `mapDeltas` is persisted to mempool.dat and loaded on restart. This is also good, otherwise we lose prioritisation on restart.
- Note the removal due to block/conflict is only done when `removeForBlock` is called, i.e. when the block is received. If you load a mempool.dat containing `mapDeltas` with transactions that were mined already (e.g. the file was saved prior to the last few blocks), you don't delete them.
- Related: #4818 and #6464.
- There is no way to query the node for not-in-mempool `mapDeltas`. If you add a priority and forget what the value was, the only way to get that information is to inspect mempool.dat.
- Calling `prioritisetransaction` with an inverse value does not remove it from `mapDeltas`, it just sets the value to 0. It disappears on a restart (`LoadMempool` checks if delta is 0), but that might not happen for a while.
Added together, if a user calls `prioritisetransaction` very regularly and not all those transactions get mined/conflicted, `mapDeltas` might keep lots of entries of delta=0 around. A user should clean up the not-in-mempool prioritisations, but that's currently difficult without keeping track of what those txids/amounts are.
ACKs for top commit:
achow101:
ACK 67b7fecacd
theStack:
Code-review ACK 67b7fecacd
instagibbs:
code review ACK 67b7fecacd
ajtowns:
ACK 67b7fecacd code review only, some nits
Tree-SHA512: 9df48b622ef27f33db1a2748f682bb3f16abe8172fcb7ac3c1a3e1654121ffb9b31aeaad5570c4162261f7e2ff5b5912ddc61a1b8beac0e9f346a86f5952260a
The functional test feature_taproot.py fails in some rare cases on the
execution of the `"branched_codesep"` spending script. The problem
occurs if the first data-push (having random content with a random
length in the range [0, 510]) has a length of 1 and the single byte has
value of [1...16] or [-1]; in this case, the data-push is not minimally
encoded by test framework's CScript class (i.e. doesn't use the special
op-codes OP_1...OP_16 or OP_1NEGATE) and the script interpreter throws
an SCRIPT_ERR_MINIMALDATA error:
```
test_framework.authproxy.JSONRPCException: non-mandatory-script-verify-flag (Data push larger than necessary) (-26)
```
Background:
The functional test framework's CScript class translates passed
bytes/bytearrays always to data pushes using OP_PUSHx/OP_PUSHDATA{1,2,4}
op-codes. E.g. the expression `CScript(bytes([1]))` yields
`bytes([OP_PUSH1, 1])` instead of the minimal-encoded `bytes([OP_1])`.
Fix this by adapting the random-size range to [2,...], i.e. never pass
byte-arrays below length two to be pushed.
Closes#27595.
a7b46a1fea test: added coverage to mining_basic.py (kevkevin)
Pull request description:
Included a test that checks if we call submitblock with block.vtx.empty() then it throws an rpc deserialization error, currently we only test if !block.vtx->IsCoinBase() throws an rpc deserialization error
I've tested to make sure this actually doing what I intended by breaking up this if block into two if blocks with different error messages and running the functional test
322ec63b01/src/rpc/mining.cpp (L963)
This change should increase the test coverage for the `submitblock()` rpc in `./src/rpc/mining.cpp`
ACKs for top commit:
theStack:
ACK a7b46a1fea
Tree-SHA512: 4078cb1fa879cc9e34438319f73085b521b90a5a95348b23e494cf8e5ac792ec426bc0e1a63e949645e16afebe54c5f35a194f02e20b7273871163d89a5c44e6
2cd28e9fef rpc: Add check for unintended option/parameter name clashes (Ryan Ofsky)
95d7de0964 test: Update python tests to use named parameters instead of options objects (Ryan Ofsky)
96233146dd RPC: Allow RPC methods accepting options to take named parameters (Ryan Ofsky)
702b56d2a8 RPC: Add add OBJ_NAMED_PARAMS type (Ryan Ofsky)
Pull request description:
Allow RPC methods which take an `options` parameter (`importmulti`, `listunspent`, `fundrawtransaction`, `bumpfee`, `send`, `sendall`, `walletcreatefundedpsbt`, `simulaterawtransaction`), to accept the options as named parameters, without the need for nested JSON objects.
This makes it possible to make calls like:
```sh
src/bitcoin-cli -named bumpfee txid fee_rate=10
```
instead of
```sh
src/bitcoin-cli -named bumpfee txid options='{"fee_rate": 10}'
```
RPC help is also updated to show options as top level named arguments instead of as nested objects.
<details><summary>diff</summary>
<p>
```diff
@@ -15,16 +15,17 @@
Arguments:
1. txid (string, required) The txid to be bumped
-2. options (json object, optional)
+2. options (json object, optional) Options object that can be used to pass named arguments, listed below.
+
+Named Arguments:
- {
- "conf_target": n, (numeric, optional, default=wallet -txconfirmtarget) Confirmation target in blocks
+conf_target (numeric, optional, default=wallet -txconfirmtarget) Confirmation target in blocks
- "fee_rate": amount, (numeric or string, optional, default=not set, fall back to wallet fee estimation)
+fee_rate (numeric or string, optional, default=not set, fall back to wallet fee estimation)
Specify a fee rate in sat/vB instead of relying on the built-in fee estimator.
Must be at least 1.000 sat/vB higher than the current transaction fee rate.
WARNING: before version 0.21, fee_rate was in BTC/kvB. As of 0.21, fee_rate is in sat/vB.
- "replaceable": bool, (boolean, optional, default=true) Whether the new transaction should still be
+replaceable (boolean, optional, default=true) Whether the new transaction should still be
marked bip-125 replaceable. If true, the sequence numbers in the transaction will
be left unchanged from the original. If false, any input sequence numbers in the
original transaction that were less than 0xfffffffe will be increased to 0xfffffffe
@@ -32,11 +33,10 @@
still be replaceable in practice, for example if it has unconfirmed ancestors which
are replaceable).
- "estimate_mode": "str", (string, optional, default="unset") The fee estimate mode, must be one of (case insensitive):
+estimate_mode (string, optional, default="unset") The fee estimate mode, must be one of (case insensitive):
"unset"
"economical"
"conservative"
- }
Result:
{ (json object)
```
</p>
</details>
**Review suggestion:** To understand this PR, it is probably easiest to review the commits in reverse order because the last commit shows the external API changes, the middle commit shows the internal API changes, and the first commit contains the low-level implementation.
ACKs for top commit:
achow101:
ACK 2cd28e9fef
Tree-SHA512: 50f6e78fa622826dab3f810400d8c1a03a98a090b1f2fea79729c58ad8cff955554bd44c2a5975f62a526b900dda68981862fd7d7d05c17f94f5b5d847317436
This fixes an intermittent error, caused by blocks arriving
out of order due to how compact block relay may revert to headers
processing when the tip hasn't caught up, and resulting in slightly
different pruning behavior.
Making sure that all blocks from the previous tests are synced before
generating more blocks makes this impossible.
See Issue #27749 for more details.
015cc5e588 lint: stop ignoring LIEF imports (fanquake)
Pull request description:
Type stubs are now available as of 0.13.0.
See https://github.com/lief-project/LIEF/issues/650.
ACKs for top commit:
TheCharlatan:
ACK 015cc5e588
Tree-SHA512: ebb754f293c2a61a0ef64c3552f7c700ceb3054b50fd3f1573e4a9e87773ddeba47bd9875f6ab055043012dbc20aeb71e4d76cd3da535c76651dfb1fbfc66e89
fa6b11a556 test: Throw error when -signetchallenge is non-hex (MarcoFalke)
Pull request description:
Instead of silently parsing non-hex to an empty challenge, throw an error.
Also, add missing includes while touching the file.
ACKs for top commit:
kevkevinpal:
ACK [fa6b11a](fa6b11a556)
kallewoof:
ACK fa6b11a
TheCharlatan:
Nice, ACK fa6b11a556
Tree-SHA512: 018ebbbf819ba7cdf0c6dd294fdfaa5ddb81b87058a8b9c57b96066d5b07e1656fd78f18e3cef375aebefa191fa515c2c70bc764880fa05f98f526334431a616
89df7987c2 Add wallets_conflicts (Antoine Riard)
dced203162 wallet, tests: mark unconflicted txs as inactive (ishaanam)
096487c4dc wallet: introduce generic recursive tx state updating function (ishaanam)
Pull request description:
This implements a fix for #7315. Previously when a block was disconnected any transactions that were conflicting with transactions mined in that block were not updated to be marked as inactive. The fix implemented here is described on the [Bitcoin DevWiki](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Transaction-Conflict-Tracking#idea-refresh-conflicted). A test which tested the previous behavior has also been updated.
Second attempt at #17543
ACKs for top commit:
achow101:
ACK 89df7987c2
rajarshimaitra:
tACK 89df7987c2.
glozow:
ACK 89df7987c2
furszy:
Tested ACK 89df7987
Tree-SHA512: 3133b151477e8818302fac29e96de30cd64c09a8fe3a7612074a34ba1a332e69148162e6cb3f1074d9d9c9bab5ef9996967d325d8c4c99ba42b5fe3b66a60546
ffffe622e9 test: Move test_chain_listunspent wallet check from mempool_packages to wallet_basic (MarcoFalke)
Pull request description:
This fixes a bug.
On master:
```
$ ./test/functional/mempool_packages.py --legacy-wallet
File "./test/functional/mempool_packages.py", line 52, in run_test
self.nodes[0].importaddress(self.wallet.get_address())
test_framework.authproxy.JSONRPCException: Bech32m addresses cannot be imported into legacy wallets (-5)
```
On this pull, all tests pass.
ACKs for top commit:
glozow:
ACK ffffe622e9, thanks for changing! Nice to remove wallet from another non-wallet test.
Tree-SHA512: 842c3b7c2e90285a155b8ed9924ef0c99f7773892be4f1847e5d7ece79c914ea5acee0d71de2ce46c354ee95fb74a03c20c0afb5e49c0b8e1c0ce406df963650
eefe56967b bugfix: Fix incorrect debug.log config file path (Ryan Ofsky)
3746f00be1 init: Error if ignored bitcoin.conf file is found (Ryan Ofsky)
398c3719b0 lint: Fix lint-format-strings false positives when format specifiers have argument positions (Ryan Ofsky)
Pull request description:
Show an error on startup if a bitcoin datadir that is being used contains a `bitcoin.conf` file that is ignored. There are two cases where this could happen:
- One case reported in [#27246 (comment)](https://github.com/bitcoin/bitcoin/issues/27246#issuecomment-1470006043) happens when a `bitcoin.conf` file in the default datadir (e.g. `$HOME/.bitcoin/bitcoin.conf`) has a `datadir=/path` line that sets different datadir containing a second `bitcoin.conf` file. Currently the second `bitcoin.conf` file is ignored with no warning.
- Another way this could happen is if a `-conf=` command line argument points to a configuration file with a `datadir=/path` line and that path contains a `bitcoin.conf` file, which is currently ignored.
This change only adds an error message and doesn't change anything about way settings are applied. It also doesn't trigger errors if there are redundant `-datadir` or `-conf` settings pointing at the same configuration file, only if they are pointing at different files and one file is being ignored.
ACKs for top commit:
pinheadmz:
re-ACK eefe56967b
willcl-ark:
re-ACK eefe56967b
TheCharlatan:
ACK eefe56967b
Tree-SHA512: 939a98a4b271b5263d64a2df3054c56fcde94784edf6f010d78693a371c38aa03138ae9cebb026b6164bbd898d8fd0845a61a454fd996e328fd7bcf51c580c2b
eeee55f928 rpc: Fix invalid bech32 handling (MarcoFalke)
Pull request description:
Currently the handling of invalid bech32(m) addresses over RPC has many issues:
* No error for invalid addresses is reported, leading to internal bugs via `CHECK_NONFATAL`, see https://github.com/bitcoin/bitcoin/issues/27723
* The error messages use "data size" (the meaning of which is unclear to the user, because the witness program data and bech32 section data are related but different) when they mean "program size"
Fix all issues. Also, use the BIP 173 and BIP 350 test vectors.
ACKs for top commit:
achow101:
ACK eeee55f928
brunoerg:
crACK eeee55f928
Tree-SHA512: c8639ee49e2a54b740b72d66bc4a40352dd553a6e3220dea9f94e48e33124f21f597a2817cb405d0a4c88d21df1013c0a4877a01370a2d326aa2cff1f9c381a8
d7f359b35e Add tests for parallel compact block downloads (Greg Sanders)
03423f8bd1 Support up to 3 parallel compact block txn fetchings (Greg Sanders)
13f9b20b4c Only request full blocks from the peer we thought had the block in-flight (Greg Sanders)
cce96182ba Convert mapBlocksInFlight to a multimap (Greg Sanders)
a90595478d Remove nBlocksInFlight (Greg Sanders)
86cff8bf18 alias BlockDownloadMap for mapBlocksInFlight (Greg Sanders)
Pull request description:
This is an attempt at mitigating https://github.com/bitcoin/bitcoin/issues/25258 , which is a revival of https://github.com/bitcoin/bitcoin/pull/10984, which is a revival of https://github.com/bitcoin/bitcoin/pull/9447.
This PR attempts to mitigate a single case, where high bandwidth peers can bail us out of a flakey
peer not completing blocks for us. We allow up to 2 additional getblocktxns requests per unique block.
This would hopefully allow the chance for an honest high bandwidth peer to hand us the transactions
even if the first in flight peer stalls out.
In contrast to previous effort:
1) it will not help if subsequent peers send block headers only, so only high-bandwidth peers this time. See: https://github.com/bitcoin/bitcoin/pull/10984/files#diff-6875de769e90cec84d2e8a9c1b962cdbcda44d870d42e4215827e599e11e90e3R1411
2) `MAX_GETBLOCKTXN_TXN_AFTER_FIRST_IN_FLIGHT` is removed, in favor of aiding recovery during turbulent mempools
3) We require one of the 3 block fetching slots to be an outbound peer. This can be the original offering peer, or subsequent compact blocks given by high bandwidth peers.
ACKs for top commit:
sdaftuar:
ACK d7f359b35e
mzumsande:
Code Review ACK d7f359b35e
Tree-SHA512: 54980eac179e30f12a0bd49df147b2c3d63cd8f9401abb23c7baf02f76eeb59f2cfaaa155227990d0d39384de9fa38663f88774e891600a3837ae927f04f0db3
272eb55616 test: fix `include_immature_coinbase` logic in `get_utxos` (brunoerg)
a951c34f17 test: fix `interface_usdt_mempool` by mining a block after each test (brunoerg)
1557bf1196 test: fix mature utxos addition to wallet in `mempool_package_limits` (brunoerg)
60ced9007d test: fix intermittent issue in `feature_bip68_sequence` (brunoerg)
Pull request description:
Fixes#27129
To avoid `bad-txns-premature-spend-of-coinbase` error,
when getting a utxo (using `get_utxo`) to create a new
transaction `get_utxo` shouldn't return (if possible)
by default immature coinbase.
ACKs for top commit:
achow101:
ACK 272eb55616
pinheadmz:
re-ACK 272eb55616
Tree-SHA512: eae821c7833bf084d8b907c94876ed010a7925d2177c3013a0c61b69d9571df006da83397a19487d93b0d1fa415951152f0b8ad0de2a55d86c39f6917934f050
1bce12acd3 test: add test for `descriptorprocesspsbt` RPC (ishaanam)
fb2a3a70e8 rpc: add descriptorprocesspsbt rpc (ishaanam)
Pull request description:
This PR implements an RPC called `descriptorprocesspsbt`. This RPC is based off of `walletprocesspsbt`, but instead of interacting with the wallet to update, sign and finalize a psbt, it instead accepts an array of output descriptors and uses that information along with information from the mempool, txindex, and the utxo set to do so. `utxoupdatepsbt` also updates a psbt in this manner, but doesn't sign or finalize it. Because of this overlap, a helper function that is added in this PR is called by both `utxoupdatepsbt` and `descriptorprocesspsbt`. Whether or not the helper function signs a psbt is dictated by if the HidingSigningProvider passed to it contains any private information. There is also a test added in this PR for this new RPC that uses p2wsh, p2wpkh, and legacy outputs.
Edit: see https://github.com/bitcoin/bitcoin/pull/25796#issuecomment-1228830963
ACKs for top commit:
achow101:
re-ACK 1bce12acd3
instagibbs:
reACK 1bce12acd3
Tree-SHA512: e1d0334739943e71f2ee68b4db7637ebe725da62e7aa4be071f71c7196d2a5970a31ece96d91e372d34454cde8509e95ab0eebd2c8edb94f7d5a781a84f8fc5d
To avoid `bad-txns-premature-spend-of-coinbase` error,
when getting a utxo (using `get_utxo`) to create a new
transaction `get_utxo` shouldn't return by default
immature coinbase.
fa4c16b186 test: Add test to check tx in the last block can be downloaded (MarcoFalke)
fadc8490ab test: Split up test_notfound_on_unannounced_tx test case (MarcoFalke)
Pull request description:
If a peer received an `inv` about a transaction, which was included in a block before receiving the corresponding `getdata`, it can be beneficial to send this transaction to the peer to aid compact block relay.
Add a test for this to avoid breaking it in the future.
ACKs for top commit:
sdaftuar:
ACK fa4c16b186
instagibbs:
ACK fa4c16b186
Tree-SHA512: 1ec16dcc216dd29c849928e753158d45c409612e9ac528db16e5af465bb5b69cd42241ac6f67e5a4583b8e558eeba49fa0a0889a4247b3fb0053b2758eec9490
faf4315c88 test: Return dict in MiniWallet::send_to (MarcoFalke)
Pull request description:
Returning a tuple has many issues:
* If only one value is needed, it can not be indexed by name
* If another value is added to the return value, all call sites need to be updated
Bite the bullet now and update all call sites to fix the above issues.
ACKs for top commit:
brunoerg:
crACK faf4315c88
theStack:
Code-review ACK faf4315c88
stickies-v:
Code review ACK faf4315c88
Tree-SHA512: 8ce1aca237df21f04b3990d0e5fcb49cc408fe6404399d3769a64eae1b5218941157d9785fce1bd9e45140cf70e06c3aa42646ee8f7b57855beb784fc3ef0261
This is achieved by letting the index sync thread wait until
reindex-chainstate is finished.
This also disables the pruning check when reindexing the chainstate (which is
incompatible with prune mode) because there would be no chain at this point
in init.
The index sync code has logic to go back the chain to the forking point, while
also updating index-specific state, which is necessary to prevent
possible corruption of the coinstatsindex.
Also add a test for this (a reorg happens while the index is deactivated)
that would not pass before this change.
Test the case of a tx being conflicted by multiple
txs with different depths. The conflicted tx is also spent by
a child tx for which confirmation status is tied to the parent's.
After a reorg of conflicting txs, the conflicted status should be
undone properly.
Co-authored-by: furszy <mfurszy@protonmail.com>
In `blockDisconnected`, for each transaction in the block, look
for any wallet transactions spending the same inputs. If any of
these transactions were marked conflicted, they are now marked as
inactive.
Co-authored-by: ariard <antoine.riard@gmail.com>
It's unnecessary to keep the data around, as it doesn't do anything. If
prioritisetransaction is called again, we'll make a new entry in
mapDeltas.
These entries are only deleted when the transaction is mined or conflicted
from a block (i.e. not in replacement or eviction), are persisted in
mempool.dat, and never expire. If node operators use the RPC to
regularly prioritise/de-prioritise transactions, these (meaningless)
map entries may hang around forever and take up valuable mempool memory.
Included a test that checks if we call submitblock with
block.vtx.empty() then it throws an rpc deserialization error, currently
we only test if !block.vtx->IsCoinBase() throws an rpc deserialization
error
fe49f06c0e doc: clarify PR 26076 release note (Sjors Provoost)
bd13dc2f46 Switch hardened derivation marker to h in descriptors (Sjors Provoost)
Pull request description:
This makes it easier to handle descriptor strings manually, especially when importing from another Bitcoin Core wallet.
For example the `importdescriptors` RPC call is easiest to use `h` as the marker: `'["desc": ".../0h/..."]'`, avoiding the need for escape characters. With this change `listdescriptors` will use `h`, so you can copy-paste the result, without having to add escape characters or switch `'` to 'h' manually.
Both markers can still be parsed.
The `hdkeypath` field in `getaddressinfo` is also impacted by this change, except for legacy wallets. The latter is to prevent accidentally breaking ancient software that uses our legacy wallet.
See discussion in #15740
ACKs for top commit:
achow101:
ACK fe49f06c0e
darosior:
re-ACK fe49f06c0e
Tree-SHA512: f78bc873b24a6f7a2bf38f5dd58f2b723e35e6b10e4d65c36ec300e2d362d475eeca6e5afa04b3037ab4bee0bf8ebc93ea5fc18102a2111d3d88fc873c08dc89
f6d7636be4 test: Treat `bitcoin-wallet` binary in the same way as others (Hennadii Stepanov)
dda961cec5 test, refactor: Add `set_binary_paths` function (Hennadii Stepanov)
Pull request description:
This PR makes the `bitcoin-wallet` binary path customizable in the same way how it can be done now with other ones, including `bitcoind`, `bitcoin-cli` and `bitcoin-util`.
ACKs for top commit:
stickies-v:
re-ACK f6d7636be4
Tree-SHA512: 480fae14c5440e530ba78a2be19eaaf642260070435e533fc7ab98ddcc2fcac7ad83f2c7e7c6706db3167e8391d7d4abf8784889796c218c2d5bba043144e787
c371cae07a test, init: perturb file to ensure failure instead of only deleting them (brunoerg)
Pull request description:
In `feature_init.py` there is a TODO about perturbing the files instead of only testing by deleting them.
```py
# TODO: at some point, we should test perturbing the files instead of removing
# them, e.g.
#
# contents = target_file.read_bytes()
# tweaked_contents = bytearray(contents)
# tweaked_contents[50:250] = b'1' * 200
# target_file.write_bytes(bytes(tweaked_contents))
#
# At the moment I can't get this to work (bitcoind loads successfully?) so
# investigate doing this later.
```
This PR adds it by writing into the file random bytes and checking whether it throws an error when starting.
ACKs for top commit:
MarcoFalke:
lgtm ACK c371cae07a
Tree-SHA512: d691eee60b91dd9d1b200588608f56b0a10dccd9761a75254b69e0ba5e5866cae14d2f90cb2bd7ec0f95b0617c2562cd33f20892ffd16355b6df770d3806a0ff
This change makes the `bitcoin-wallet` binary path customizable in the
same way how it can be done now with other ones, including `bitcoind`,
`bitcoin-cli` and `bitcoin-util`.
7e3d4f8e86 test: add coverage to ensure the first arg of scantxoutset is needed (ismaelsadeeq)
Pull request description:
Include a test that checks whether the first argument of scantxoutset RPC call "start" is required.
The rpc call should fail if the "start" argument is not provided.
ACKs for top commit:
MarcoFalke:
lgtm ACK 7e3d4f8e86
Tree-SHA512: 6a456af9f3ccd5437be2edcd61936eb9f9c21ab926a6056c2c11b6b5121d1caca4e1f2ffd09015f9414af152c635a20e1da041eefdef980afbe8a0e8ccce07bd
afc2dd5484 test: various `converttopsbt` check cleanups in rpc_psbt.py (Sebastian Falbesoner)
Pull request description:
In the functional test rpc_psbt.py, some comments around the `converttopsbt` RPC checks are wrong or outdated and can be removed:
> _Error could be either "TX decode failed" (segwit inputs causes
> parsing to fail) or "Inputs must not have scriptSigs and
> scriptWitnesses"_
Decoding a valid TX with at least one input always succeeds with the [heuristic](e352f5ab6b/src/core_read.cpp (L126)), i.e. this comment is not right and we can assert for the error string "Inputs must not have scriptSigs and scriptWitnesses" on the calls below.
> _We must set iswitness=True because the serialized transaction has
> inputs and is therefore a witness transaction_
This is also unneeded (and confusing, w.r.t. "is therefore a witness transaction"?), for a TX with one input there is no need to set the `iswitness` parameter. For sake of completeness, we still keep one variant where iswitness is explicitly set to true.
Lastly, there is a superflous `converttopsbt` call on the raw tx which is the same as just [about ~10 lines above](https://github.com/bitcoin/bitcoin/blob/master/test/functional/rpc_psbt.py#L393-L397), so it can be removed.
ACKs for top commit:
ismaelsadeeq:
tested ACK afc2dd5484
achow101:
ACK afc2dd5484
Tree-SHA512: 467efefdb3f61efdb79145044b90fc8dc2f0c425f078117a99112b0074e7d4a32c34e464f665fbf8de70d06caaa5d4ad5908c1d75d2e7607eccb0837480afab3
Rather than doing the open/read/hash-steps manually in the affected
functional tests, we can just use the `sha256sum_file` helper from the
utils module instead.
Note that for the tool_wallet.py test, the used hash is changed from
sha1 to sha256, but as the only purpose is to detect file content
changes, this doesn't matter. Also, the optimization using `memoryview`
is overkill here, as the opened file has only a size of 24KiB and
determining the hash doesn't take longer than a few hundred
micro-seconds on my machine.
fa17767154 test: Simplify feature_fastprune.py (MarcoFalke)
Pull request description:
The goal of the test is a single regression check to see if a RPC times out. It shouldn't do more than calling the RPC (and the minimum work needed to get there).
Fix that by removing all blocktools imports and a `for` loop.
ACKs for top commit:
pinheadmz:
ACK fa17767154
theStack:
ACK fa17767154
Tree-SHA512: c9c0154102199b250015ece53005a14d52d857dfa986f3b02a2cb899f16ac8e040d24eb826f35ba15e5ee22ee6a59bf8f74bb8d576b9a12ac6e888beeaaf81cc
710b83938a rpc: return block hash & height in getbalances, gettransaction & getwalletinfo JSONs (Harris)
Pull request description:
Reopens#18570 and closes#18567.
I have rebased the original PR.
Not sure why the original got closed as it was about to get merged.
ACKs for top commit:
achow101:
ACK 710b83938a
Tree-SHA512: d4478d990be98b1642e9ffb2930987f4a224e8bd64e2e35a5dda927a54c509ec9d712cd0eac35dc2bb89f00a1678e530ce14d7445f1dd93aa3a4cce2bc9b130d
24d55fb9cf test: added coverage to rpc_scantxoutset.py (kevkevin)
Pull request description:
Included a test that checks if an invalid first argument is entered we receive a rpc error. The rpc should fail if "start", "status" or "abort" is not the first command.
Relavant: mentioned in https://github.com/bitcoin/bitcoin/pull/27422
ACKs for top commit:
MarcoFalke:
lgtm ACK 24d55fb9cf
theStack:
ACK 24d55fb9cf
Tree-SHA512: 4b804235d3fa17c7bf492068ab47c1f87cb6cfc1a428c51e273ec059d3c41f581bcc467bb5d6d8bbf2fab14c60cd1c52a30c50009efe1c9b5adee70c88897ad9
82e6e3cae5 test: add ripemd160 to test framework modules list (Sebastian Falbesoner)
Pull request description:
Currently test runner doesn't execute the unit tests of the ripemd160 module, so add it to the list. All other framework modules that contain unit tests are included, as can be easily checked via
`$ git grep unittest.TestCase ./test/functional/test_framework/`
This is a late follow-up to PR #23716 (commit ad3e9e1f21).
ACKs for top commit:
MarcoFalke:
lgtm ACK 82e6e3cae5
Tree-SHA512: 10940e215f728291c7149931a356bfc42795c098bda76d760dfa68f86443a3755e1cd35cb9a8a7b2f48880beb53f3bee3842de2d74bcadd45c7b05c13ff04203
Currently test runner doesn't execute the unit tests of the ripemd160
module, so add it to the list. All other framework modules that contain
unit tests are included, as can be easily checked via
`git grep unittest.TestCase ./test/functional/test_framework/`
This is a late follow-up to PR #23716 (commit
ad3e9e1f21).
Included a test that checks if an invalid first argument is entered we
receive a rpc error. The rpc should fail if "start", "status" or "abort"
is not the first command.
8f14fc8622 test: cover fastprune with excessive block size (Matthew Zipkin)
271c23e87f blockstorage: Adjust fastprune limit if block exceeds blockfile size (Martin Zumsande)
Pull request description:
The debug-only `-fastprune` option used in several tests is not always safe to use:
If a `-fastprune` node receives a block larger than the maximum blockfile size of `64kb` bad things happen: The while loop in `BlockManager::FindBlockPos` never terminates, and the node runs oom because memory for `m_blockfile_info` is allocated in each iteration of the loop.
The same would happen if a naive user used `-fastprune` on anything other than regtest (so this can be tested by syncing on signet for example, the first block that crashes the node is at height 2232).
Change the approach by raising the blockfile size to the size of the block, if that block otherwise wouldn't fit (idea by TheCharlatan).
ACKs for top commit:
ryanofsky:
Code review ACK 8f14fc8622. Added new assert, test, and comment since last review
TheCharlatan:
ACK 8f14fc8622
pinheadmz:
ACK 8f14fc8622
Tree-SHA512: df2fea30613ef9d40ebbc2416eacb574f6d7d96847db5c33dda22a29a2c61a8db831aa9552734ea4477e097f253dbcb6dcb1395d43d2a090cc0588c9ce66eac3
dc14ba08e6 test: remove modinv python util helper function (Fabian Jahr)
Pull request description:
Since #27483 was merged the `modinv()` body is just one line calling pythons own implementation of `pow()`. We can just remove the function as it doesn't seem to add any value. Additionally the comment in the function is now outdated and the test is only testing two ways of doing modular inverse but both using python's `pow()` function.
ACKs for top commit:
theStack:
ACK dc14ba08e6
Tree-SHA512: e8b470c72dc3f9fd53699d0684650517b1ea35ad1d4c01cf9472c80d3e4474c0c72e429c0bd201eb99d204c87eee0d68285e6a388e4c506f30e14b2bff9c1c32
057057a2d7 Add test for `sendmany` rpc that uses `subtractfeefrom` parameter (Yusuf Sahin HAMZA)
Pull request description:
This PR adds test that uses `sendmany` rpc to send **BTC** to multiple addresses using `subtractfeefrom` parameter, then checks receiver addresses balances to make sure fees are subtracted correctly.
ACKs for top commit:
achow101:
ACK 057057a2d7
Tree-SHA512: 51167120d489f0ff7b8b9855424d07cb55a8965984f904643cddf45e7a08c350eaded498c350ec9c660edf72c2f128ec142347c9c79d5043d9f6cd481b15cd7e
9c18992bba test: add coverage for `-bantime` (brunoerg)
Pull request description:
This PR adds test coverage for `-bantime`. This flag sets the time in seconds how long the IP is banned (in the case you don't explicitly set `bantime` when using `setban`).
ACKs for top commit:
MarcoFalke:
lgtm ACK 9c18992bba
Tree-SHA512: e95f8608aa5df9b09cc5577daae662ed79ef5d5c69ee5e704d7c69520b9b51cc142e9e6be69d80356eda25a5215c4770b1a208638560c48cd3bc8f6d195a371f
b922f6b526 rpc: scanblocks, add "completed" flag to the result obj (furszy)
ce50acc54f rpc: scanblocks, do not traverse the whole chain block by block (furszy)
Pull request description:
Coming from https://github.com/bitcoin/bitcoin/pull/23549#pullrequestreview-1105712566
The current `scanblocks` flow walks-through every block in the active chain
until hits the chain tip or processes 10k blocks, then calls `lookupFilterRange`
function to obtain all filters from that particular range.
This is only done to obtain the heights range to look up the block
filters. Which is unneeded.
As `scanblocks` only lookup block filters in the active chain, we can
directly calculate the lookup range heights, by using the chain tip,
without requiring to traverse the chain block by block.
ACKs for top commit:
achow101:
ACK b922f6b526
TheCharlatan:
ACK b922f6b526
Tree-SHA512: 0587e6d9cf87a59184adb2dbc26a4e2bce3a16233594c6c330f69feb49bf7dc63fdacf44fc20308e93441159ebc604c63eb7de19204d3e745a2ff16004892b45
be72663a15 test: bumpfee, add coverage for "send coins back to yourself" (furszy)
7bffec6715 bumpfee: enable send coins back to yourself (furszy)
Pull request description:
Simple example:
1) User_1 sends 0.1 btc to user_2 on a low fee transaction.
2) After few hours, the tx is still in the mempool, user_2
is not interested anymore, so user_1 decides to cancel
it by sending coins back to himself.
3) User_1 has the bright idea of opening the explorer and
copy the change output address of the transaction. Then
call bumpfee providing such output (in the "outputs" arg).
Currently, this is not possible. The wallet fails with
"Unable to create transaction. Transaction must have at least
one recipient" error.
The error reason is because we discard the provided output
from the recipients list and set it inside the coin control
so the process adds it later (when the change is calculated).
But.. there is no later if the tx has no outputs.
ACKs for top commit:
ishaanam:
reACK be72663a15
achow101:
ACK be72663a15
Tree-SHA512: c2c38290a998f9b426a830d9624c7feb730158980ac186f8fb0138d5e200935d6538307bc60a2c3d0b7b6ee2b4ffb77a1e98baf8feb1d20a7d825f6055ac377f
To tell the user whether the process was aborted or not.
Plus, as the process can be aborted prior to the end range,
have also changed the "to_height" result value to return the
last scanned block instead of the end range block.
fac395e5eb ci: Bump ci/lint/Dockerfile (MarcoFalke)
fa6eb65167 test: Use python3.8 pow() (MarcoFalke)
88881cf7ac Bump python minimum version to 3.8 (MarcoFalke)
Pull request description:
There is no pressing reason to drop support for 3.7, however there are several maintenance issues:
* There is no supported operating system that ships 3.7 by default. (debian:buster is EOL and unmaintained to the extent that it doesn't run in the CI environment. See https://github.com/bitcoin/bitcoin/pull/27340#issuecomment-1484988445)
* Compiling python 3.7 from source is also unsupported on at least macos, according to https://github.com/bitcoin/bitcoin/pull/24017#issuecomment-1107820790
* Recent versions of lief require 3.8, see https://github.com/bitcoin/bitcoin/pull/27507#issuecomment-1517561645
Fix all maintenance issues by bumping the minimum.
ACKs for top commit:
RandyMcMillan:
ACK fac395e
fjahr:
ACK fac395e5eb
fanquake:
ACK fac395e5eb
Tree-SHA512: c198decdbbe29d186d73ea3f6549d8a38479383495d14a965a2f9211ce39637b43f13a4c2a5d3bf56e2d468be4bbe49b4ee8e8e19ec69936ff43ddf2b714c712
33fdfc7986 test: perturb anchors.dat to test it doesn't throw an error during initialization (brunoerg)
Pull request description:
Got some inspiration from `feature_init`. This PR tests whether perturbing `anchors.dat` doesn't throw any error during initialization.
3f1f5f6f1e/src/addrdb.cpp (L223-L235)
ACKs for top commit:
MarcoFalke:
lgtm ACK 33fdfc7986
Tree-SHA512: e6584debb37647677581fda08366b45b42803022cc4c4f1d5a7bd5e9e04d64da77656dad2b804855337487bdcfc891f300a2e03668d6122de769dd14f39af9ed
4bdcf57158 test: test banlist database recreation (brunoerg)
Pull request description:
This PR adds test coverage for 'banlist database recreation'. If it wasn't able to read ban db (in `LoadBanlist`), so it should create a new (an empty, ofc) one.
d8bdee0fc8/src/banman.cpp (L28-L45)
ACKs for top commit:
MarcoFalke:
lgtm ACK 4bdcf57158
Tree-SHA512: d9d0cd0c4b3797189dff00d3a634878188e7cf51e78346601fc97e2bf78c495561705214062bb42ab8e491e0d111f8bfcf74dbc801768bc02cf2b45f162aad85
cca4f82b82 test: add coverage for rpc error when trying to rescan beyond pruned data (brunoerg)
Pull request description:
This PR adds test coverage for the following rpc error:
15692e2641/src/wallet/rpc/transactions.cpp (L896-L899)
ACKs for top commit:
MarcoFalke:
lgtm ACK cca4f82b82
aureleoules:
ACK cca4f82b82
Tree-SHA512: 724a055e9f6cddf1935699e8769015115f24f6485a0bd87e8660072ee44a15c1bddfdda848acc101ea7184b7e65a33b5b0d80b563d2ba3ecdab7a631378d6476
0c520679ab doc: add release notes for `abandoned` field in `gettransaction` and `listtransactions` (brunoerg)
a1aaa7f51f rpc, wallet: add `abandoned` field for all categories of transactions in ListTransactions (brunoerg)
Pull request description:
Fixes#25130
ACKs for top commit:
achow101:
re-ACK 0c520679ab
Tree-SHA512: 1864460d76decab7898737c96517d722055eb8f81ca52248fe1035723258c6cd4a93251e06a86ecbbb0b0a80af1466b2c86fb142ace4ccb74cc40d5dc3967d7f
bf77fc9cb4 [test] mempool full in package accept (glozow)
b51ebccc28 [validation] set PackageValidationState when mempool full (glozow)
563a2ee4f5 [policy] disallow transactions under min relay fee, even in packages (glozow)
c4554fe894 [test] package cpfp bumps parents <mempoolminfee but >=minrelaytxfee (glozow)
ac463e87df [test util] mock mempool minimum feerate (glozow)
Pull request description:
Part of package relay, see #27463.
Note that this still allows packages to bump transactions that are below the dynamic mempool minimum feerate, which means this still solves the "mempool is congested and my presigned 1sat/vB tx is screwed" problem for all transactions.
On master, the package policy (only accessible through regtest-only RPC submitpackage) allows 0-fee (or otherwise below min relay feerate) transactions if they are bumped by a child. However, with default package limits, we don't yet have a DoS-resistant way of ensuring these transactions remain bumped throughout their time in the mempool. Primarily, the fee-bumping child may later be replaced by another transaction that doesn't bump the parent(s). The parent(s) could potentially stay bumped by other transactions, but not enough to ever be selected by the `BlockAssembler` (due to `blockmintxfee`).
For example, (tested [here](https://github.com/glozow/bitcoin/commits/26933-motivation)):
- The mempool accepts 24 below-minrelayfeerate transactions ("0-fee parents"), all bumped by a single high-fee transaction ("the fee-bumping child"). The fee-bumping child also spends a confirmed UTXO.
- Two additional children are added to each 0-fee parent. These children each pay a feerate slightly above the minimum relay feerate (e.g. 1.9sat/vB) such that, for each 0-fee parent, the total fees of its two children divided by the total size of the children and parent is above the minimum relay feerate.
- If a block template is built now, all transactions would be selected.
- A transaction replaces the the fee-bumping child, spending only the confirmed UTXO and not any of the outputs from the 0-fee parents.
- The 0-fee parents now each have 2 children. Their descendant feerates are above minrelayfeerate, which means that they remain in the mempool, even if the mempool evicts all below-minrelayfeerate packages.
- If a block template is built now, none of the 0-fee parents or their children would be selected.
- Even more low-feerate descendants can be added to these below-minrelayfeerate packages and they will not be evicted until they expire or the mempool reaches capacity.
Unless we have a DoS-resistant way of ensuring package CPFP-bumped transactions are always bumped, allowing package CPFP to bump below-minrelayfeerate transactions can result in these problematic situations. See #27018 which proposes a partial solution with some limitations, and contains discussion about potential improvements to eviction strategy. While no adequate solution exists, for now, avoid these situations by requiring all transactions to meet min relay feerate.
ACKs for top commit:
ajtowns:
reACK bf77fc9cb4
instagibbs:
re-ACK bf77fc9cb4
Tree-SHA512: 28940f41493a9e280b010284316fb8caf1ed7b2090ba9a4ef8a3b2eafc5933601074b142f4f7d4e3c6c4cce99d3146f5c8e1393d9406c6f2070dd41c817985c9
The test `feature_addrman.py` currently serializes the addrdb without
specifying endianness for `int`s, so the machine's native byte order is used (see
https://docs.python.org/3/library/struct.html#byte-order-size-and-alignment)
and the generated `peers.dat` would be invalid on big-endian systems.
Fix this by explicitly specifying little-endian serialization via the
`<` character in `struct.pack(...)`.
These routines look fancy, but do nothing more than converting between
byte objects of length 32 to/from integers in little endian byte order
and can be replaced by simple one-liners, using the int.{from,to}_bytes
methods (available since Python 3.2).
6e9f8bb050 rpc, tests: in `utxoupdatepsbt` also look for the transaction in the txindex (ishaanam)
a5b4883fb4 rpc: extract psbt updating logic into ProcessPSBT (ishaanam)
Pull request description:
Previously the `utxoupdatepsbt` RPC, added in #13932, only updated the inputs spending segwit outputs with the `witness_utxo`, because the full transaction is needed for legacy inputs. Before this RPC looked for just the utxos in the utxo set and the mempool. This PR makes it so that if the user has txindex enabled, then the full transaction is looked for there for all inputs. If it is not found in the txindex or txindex isn't enabled, then the mempool is checked for the full transaction. If the transaction an input is spending from is still not found at that point, then the utxo set is searched and the inputs spending segwit outputs are updated with just the utxo.
ACKs for top commit:
achow101:
ACK 6e9f8bb050
Xekyo:
ACK 6e9f8bb050
Tree-SHA512: 078db3c37a1ecd5816d80a42e8bd1341e416d661f508fa5fce0f4e1249fefd7b92a0d45f44957781cb69d0953145ef096ecdd4545ada39062be27742402dac6f
Currently debug.log will show the wrong bitcoin.conf config file path when
bitcoind is invoked without -conf or -datadir arguments, and there's a default
bitcoin.conf file which specifies another datadir= location. When this happens,
the debug.log will include an incorrect "Config file:" line referring to a
bitcoin.conf file in the other datadir, instead of the referring to the actual
configuration file in the default datadir which was parsed.
The bad log print was reported and originally fixed in
https://github.com/bitcoin/bitcoin/pull/27303 by
Matthew Zipkin <pinheadmz@gmail.com>
This PR takes a slightly different approach to fixing the bug, trying to avoid
future bugs by not allowing the GetConfigFilePath function to be called before
the the configuration is parsed, and deleting GetConfigFile function which
could be confused with GetConfigFilePath. It also includes a test for the bug
which the original fix did not have.
Co-authored-by: Matthew Zipkin <pinheadmz@gmail.com>
Show an error on startup if a bitcoin datadir that is being used contains a
`bitcoin.conf` file that is ignored. There are two cases where this could
happen:
- One case reported in
https://github.com/bitcoin/bitcoin/issues/27246#issuecomment-1470006043
happens when a bitcoin.conf file in the default datadir (e.g.
$HOME/.bitcoin/bitcoin.conf) has a "datadir=/path" line that sets different
datadir containing a second bitcoin.conf file. Currently the second
bitcoin.conf file is ignored with no warning.
- Another way this could happen is if a -conf= command line argument points
to a configuration file with a "datadir=/path" line and that specified path
contains a bitcoin.conf file, which is currently ignored.
This change only adds an error message and doesn't change anything about way
settings are applied. It also doesn't trigger errors if there are redundant
-datadir or -conf settings pointing at the same configuration file, only if
they are pointing at different files and one file is being ignored.
Do not error on valid format specifications like strprintf("arg2=%2$s arg1=%1$s arg2=%2$s", arg1, arg2);
Needed to avoid lint error in upcoming commit: https://cirrus-ci.com/task/4755032734695424?logs=lint#L221
Additionally tested with python -m doctest test/lint/run-lint-format-strings.py
be55f545d5 move-only: Extract common/args and common/config.cpp from util/system (TheCharlatan)
Pull request description:
This pull request is part of the `libbitcoinkernel` project https://github.com/bitcoin/bitcoin/issues/24303https://github.com/bitcoin/bitcoin/projects/18 and more specifically its "Step 2: Decouple most non-consensus code from libbitcoinkernel". It is part of a series of patches splitting up the `util/system` files. Its preceding pull request is https://github.com/bitcoin/bitcoin/pull/27254.
The pull request contains an extraction of ArgsManager related functions from util/system into their own common/ file.
The background of this commit is an ongoing effort to decouple the libbitcoinkernel library from the ArgsManager. The ArgsManager belongs into the common library, since the kernel library should not depend on it. See [doc/design/libraries.md](https://github.com/bitcoin/bitcoin/blob/master/doc/design/libraries.md) for more information on this rationale.
ACKs for top commit:
MarcoFalke:
re-ACK be55f545d5🚲
ryanofsky:
Code review ACK be55f545d5. Just small cleanups since the last review.
hebasto:
ACK be55f545d5, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 90eb03334af0155b823030b4f2ecf286d35058d700ee2ddbbaa445be19e31eb0fe982656f35bd14ecee3ad2c3d0db3746855cb8f3777eff7253713e42873e111
* The GCC suppression was fixed in gcc-11, which is available on all LTS
releases of Linux distros.
* The feerate suppression was likely fixed and does not trigger anymore.
If it was to trigger again, the underlying bug should be fixed instead
of suppressing it.
* The bench suppression does not trigger anymore.
Also, add comments to tsan suppressions on how to reproduce.
This is an extraction of ArgsManager related functions from util/system
into their own common file.
Config file related functions are moved to common/config.cpp.
The background of this commit is an ongoing effort to decouple the
libbitcoinkernel library from the ArgsManager. The ArgsManager belongs
into the common library, since the kernel library should not depend on
it. See doc/design/libraries.md for more information on this rationale.
Prior to PR #27468 (commit 11422cc572) all
call-sites of `GetQueryParameter(...)` in the REST module could trigger
a crash. Add missing test cases for all possible code-paths as a
regression test, as a foundation for possible follow-up fixes (which aim
to resolve this issue in a more general and robust way).
e07dd5fff9 test: fix bumpfee 'spend_one_input' occasional failure (furszy)
Pull request description:
CI test failure, in master: https://cirrus-ci.com/task/5975232842825728.
In #27469https://cirrus-ci.com/task/6452468402356224
Most of the subtests in `wallet_bumpfee.py` expect to find spendable UTXOs of 0.001 btc in the rbf wallet. They use the `spend_one_input()` method which fails if none of them exist.
The sporadic failure comes from the recently added `test_feerate_checks_replaced_outputs` subtest that can spend all them. Leaving the next subtests with no 0.001 UTXOs to spend.
To solve it, this PR moves the recently added case into a "context independent subtests" section.
Which is placed at the end to not affect other cases.
ACKs for top commit:
achow101:
ACK e07dd5fff9
pablomartin4btc:
ACK e07dd5fff9.
Tree-SHA512: c150ed6fcfbb6f1502eaf6370a57aae0da991c0fc48e8bb3d446805f4336abba5d80ff0de26094914da95432dd0255030fe527001af4510dfdcefbc7230f14d6
`evhttp_uri_parse` can return a nullptr, for example when the URI
contains invalid characters (e.g. "%").
`GetQueryParameterFromUri` passes the output of `evhttp_uri_parse`
straight into `evhttp_uri_get_query`, which means that anyone calling
a REST endpoint in which query parameters are used (e.g. `rest_headers`)
can cause a segfault.
This bugfix is designed to be minimal and without additional behaviour change.
Follow-up work should be done to resolve this in a more general and robust way,
so not every endpoint has to handle it individually.
Most of the subtests in wallet_bumpfee.py expect to
find spendable UTXOs of 0.001 btc in the rbf wallet
(they use the 'spend_one_input()' method that tries
to spend one of them and if it doesn't find any, it
throws an exception).
The sporadic failure comes from the recently added
'test_feerate_checks_replaced_outputs' subtest that
can spend all them. Leaving the next subtests with
no 0.001 UTXOs to spend.
To solve it, this PR moves the recently added case
into a "context independent subtests" section.
Which is placed at the end to not affect other cases.
d52fa1b0a5 tests: Make sure that bumpfee feerate checks work when replacing outputs (Andrew Chow)
be177c15a4 bumpfee: Check the correct feerate when replacing outputs (Andrew Chow)
Pull request description:
When replacing the outputs of a transaction during `bumpfee`, it is possible to accidentally create a transaction that will not be accepted into the mempool as it does not meet the incremental relay fee requirements. This occurs because the size estimation used for checking the provided feerate does not account for the replaced outputs; it instead uses the original outputs. When the replaced outputs is significantly different from the original, there can be a large difference in estimated transaction sizes that can make a transaction miss the absolute fee requirements for the incremental relay fee. Unfortunately we do not currently inform the user when the bumped transaction fails to relay, so they could use `bumpfee` and think the transaction has been bumped when it actually has not.
This issue is resolved by replacing the outputs before doing the size estimation, and also updating the feerate checker to use the actual fee values when calculating the required minimum fee.
Also added a test for this scenario.
ACKs for top commit:
ishaanam:
reACK d52fa1b0a5
Xekyo:
reACK d52fa1b0a5
Tree-SHA512: d18301b587465322dd3fb1bb86496c3675265a56072047576e2baa5cf907dd3b54778f30721f662f0c235709a5568427c18542eb7efbfb6fdd9f481fe676c66b
7ccdd741fe test: fix importmulti/importdescriptors assertion (Jon Atack)
19d888ce40 rpc: move WALLET_FLAG_CAVEATS to the compilation unit of its caller (Jon Atack)
01df011ca2 doc: release note for wallet RPCs "warning" field deprecation (Jon Atack)
9ea8b3739a test: createwallet "warning" field deprecation test (Jon Atack)
645d7f75ac rpc: deprecate "warning" field in {create,load,unload,restore}wallet (Jon Atack)
2f4a926e95 test: add test coverage for "warnings" field in createwallet (Jon Atack)
4a1e479ca6 rpc: add "warnings" field to RPCs {create,load,unload,restore}wallet (Jon Atack)
079d8cdda8 rpc: extract wallet "warnings" fields to a util helper (Jon Atack)
f73782a903 doc: fix/improve warning helps in {create,load,unload,restore}wallet (Jon Atack)
Pull request description:
Based on discussion and concept ACKed in #27138, add a `warnings` field to RPCs createwallet, loadwallet, unloadwallet, and restorewallet as a JSON array of strings to replace the `warning` string field in these 4 RPCs. The idea is to more gracefully handle multiple warning messages and for consistency with other wallet RPCs. Then, deprecate the latter fields, which represent all the remaining RPC `warning` fields.
The first commit f73782a903 implements https://github.com/bitcoin/bitcoin/pull/27138#issuecomment-1474789198 as an alternative to #27138. One of those two could potentially be backported to our currently supported releases.
ACKs for top commit:
achow101:
ACK 7ccdd741fe
1440000bytes:
utACK 7ccdd741fe
vasild:
ACK 7ccdd741fe
pinheadmz:
re-ACK 7ccdd741fe
Tree-SHA512: 314e0a4c41fa383d95e2817bfacf359d449e460529d235c3eb902851e2f4eacbabe646d9a5a4beabc4964cdfabf6397ed8301366a58d344a2f787f83b75e9d64
Instead of storing and passing around fixed strings for the purpose of
an address, use an enum.
This also rationalizes the CAddressBookData struct, documenting all fields and
making them public, and simplifying the representation to avoid bugs like
https://github.com/bitcoin/bitcoin/pull/26761#discussion_r1134615114 and make
it not possible to invalid address data like change addresses with labels.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
When replacing the outputs of a transaction, we can end up with
fees that are drastically different from the original. This tests that
the feerate checks we perform will properly detect when the bumping tx
will have an insufficient feerate.
Include a test that checks whether the first argument of
scantxoutset RPC call is required. The rpc call should fail if
the "start" argument is not provided.
This makes it easier to handle descriptor strings manually. E.g. an RPC call that takes an array of descriptors can now use '["desc": ".../0h/..."]'.
Both markers can still be parsed. The default for new descriptors is changed to h. In normalized form h is also used. For private keys the chosen marker is preserved in a round trip.
The hdkeypath field in getaddressinfo is also impacted by this change.
f842ed9a40 test: refactor: replace unnecessary `BytesIO` uses (Sebastian Falbesoner)
Pull request description:
Rather than needing to create intermediate stream variables, we can use helper functions like `tx_from_hex` instead or access the result directly, leading both to increased readability and less code.
ACKs for top commit:
stickies-v:
ACK f842ed9a40
brunoerg:
crACK f842ed9a40
aureleoules:
ACK f842ed9a40 - It seems that these are the only instances that can be changed and it simplifies test code.
Tree-SHA512: 7f4fd7a26720d1988bf27f66c817ff6cd7060350a3be62d28e7848c768fd43578719ca475193d4057ccf4f2458af18564fd513fe3a1d458a11c799927c12bd65
Rather than needing to create intermediate stream variables, we can use
helper functions like `tx_from_hex` instead or access the result
directly, leading both to increased readability and less code.
Also, move the burden of checking for a timeout to the client and
disable the timeout on the server. This should avoid intermittent issues
in slow tests (for example mining_getblocktemplate_longpoll.py, or
feature_dbcrash.py), or possibly when the server is running slow (for
example in valgrind). There shouldn't be any downside in tests caused
by a high rpcservertimeout.
71b3e9b0ad sanitizers: remove GetRNGState lsan suppression (fanquake)
Pull request description:
I am no-longer seeing this, testing with the native_asan job over `x86_64` (Ubuntu 22.04) and `aarch64` (Fedora 37).
Can anyone recreate the false-positive?
ACKs for top commit:
MarcoFalke:
lgtm ACK 71b3e9b0ad
hebasto:
ACK 71b3e9b0ad, tested on Ubuntu 22.04 x86_64.
Tree-SHA512: 63020327d61acd6c94c6c278c9c4d72aedc10253fa172bcf9353bcad4c28d068bee824969eb3ce92152244831df8fe92cffae536453c8073a4fda74dfdfbcefa
e669833943 test: dedup package limit checks via decorator in mempool_package_limits.py (Sebastian Falbesoner)
72f25e238c test: refactor: use Satoshis for fees in mempool_package_limits.py (Sebastian Falbesoner)
Pull request description:
The subtests in the functional test mempool_package_limits.py all follow the same pattern:
1. first, check that the mempool is currently empty
2. create and submit certain single txs to the mempool, prepare list of hex transactions
3. check that `testmempoolaccept` on the package hex fails with a "package-mempool-limits" error on each tx result
4. after mining a block, check that submitting the package succeeds
Note that steps 1,3,4 are identical for each of the subtests and only step 2 varies, so this might be a nice opportunity to deduplicate code by using a newly introduced decorator which executes the necessary before and after the essential part of the subtest. This also makes it easier to add new subtests without having to copy-paste those parts once again.
In addition, the first commit switches the fee unit from BTC to Satoshis, which allows to get rid of some imports (`COIN` and `Decimal`) and a comment for the `test_desc_size_limits` subtest is fixed (s/25KvB/21KvB/).
ACKs for top commit:
ismaelsadeeq:
ACK e669833943
glozow:
utACK e669833943
Tree-SHA512: 84a85e739de7387391c13bd46aeb015a74302ea7c6f0ca3d4e2b1b487d38df390dc118eb5b1c11d3e4206bff316a4dab60ef6b25d8feced672345d4e36ffd205
e47ce42f67 refactor: use address_to_scriptpubkey to retrieve addresses scriptpubkey (ismaelsadeeq)
4142d19d74 refactor: move address_to_scriptpubkey to address.py (ismaelsadeeq)
Pull request description:
PR #27269 enables the function address_to_scriptpubkey() to decode all address types and return their corresponding scriptpubkeys. As a result, there is no longer any need to call getaddressinfo or validateaddress RPCs in order to retrieve an address scriptpubkey, as explained in the comments on this pull request (see https://github.com/bitcoin/bitcoin/pull/27269#pullrequestreview-1353681933 and https://github.com/bitcoin/bitcoin/pull/27269#issuecomment-1481016118).
Instead of using RPC calls, this update replaces the process of obtaining an address scriptPubkey with the address_to_scriptpubkey method, resulting in improved performance for functional tests.
ACKs for top commit:
josibake:
re-ACK e47ce42f67
theStack:
ACK e47ce42f67🌱
Tree-SHA512: 05285349a7d5ce7097b8f2582e573a5135c6deef85ea9936f68f6ce94e9ebb1d84d94f7fc7e5ed833a698e01585addd80deb52e6338f8aee985bf14db45417d2
This avoids having to convert from BTC to Sats and needs less imports.
Also specify the tx's target size in vsize rather than in weight, which
allows us to specify the fee-rate by a simple multiplication, rather
than having another magic number for it.
This commit updates the code by replacing the RPC call used to
decode an address and retrieve its corresponding scriptpubkey
with the address_to_scriptpubkey function. address_to_scriptpubkey
function can now decode all addresses formats, which makes
it more efficient to use.
The COINBASE_MATURITY constant in blocktools.py is imported in wallet.py.
However, importing address_to_scriptpubkey to blocktools.py will
generate a circular import error. Since the method is related to
addresses, it is best to move it to address.py, which will also
fix the circular import error.
Update imports of address_to_scriptpubkey accordingly.
8aab5157c5 test: wallet_create_tx.py fix race (furszy)
Pull request description:
Fixes#27316
Because wallets are internally synchronized through the validation interface,
and the interface dispatches events on a worker thread, it is possible for a
transaction created by the first wallet to not arrive to the second wallet
before the second wallet attempts to use one of its outputs. This is because
we do not wait for the `BroadcastTransaction` callback during the wallet's
"submit to mempool" process. To address this in the tests, we need to
manually sync the validation queue.
ACKs for top commit:
josibake:
ACK 8aab5157c5
theStack:
ACK 8aab5157c5
Tree-SHA512: 76364370ab292a5c3ea1ed61cd353fc626a9e9cd6ce18464c24da1b3dcb34b65006e2bc42b84bbd25af03f9449231990bf789504728972db3217b569099eb309
In the functional test rpc_psbt.py, some comments around the
`converttopsbt` RPC checks are wrong or outdated and can be
removed:
> Error could be either "TX decode failed" (segwit inputs causes
> parsing to fail) or "Inputs must not have scriptSigs and
> scriptWitnesses"
Decoding a valid TX with at least one input always succeeds with the
heuristic, i.e. this comment is not right and we can assert for the
error string "Inputs must not have scriptSigs and scriptWitnesses"
on the calls below.
> We must set iswitness=True because the serialized transaction has
> inputs and is therefore a witness transaction
This is also unneeded (and confusing, w.r.t. "is therefore a witness
transaction"?), for a TX with one input there is no need to set the
`iswitness` parameter. For sake of completeness, we still keep one
variant where iswitness is explicitly set to true.
Lastly, there is a superflous `converttopsbt` call on the raw tx which
is the same as just about ~10 lines above, so it can be removed.
d178082996 test: add bech32 decoding support to address_to_scriptpubkey() (ismaelsadeeq)
aac8793c7a test: test_bech32_decode in address.py (ismaelsadeeq)
Pull request description:
[rpc_scantxoutset.py](e695d8536e/test/functional/rpc_scantxoutset.py (L26)) sendtodestination only sends to legacy addresses and scriptPubkeys because [wallet.py](e695d8536e/test/functional/test_framework/wallet.py (L415)) address_to_scriptpubkey does not support conversion of segwit address.
This update enables address_to_scriptpubkey to support the conversion of testnet segwit addresses to scriptPubkeys.
This change will enable [rpc_scantxoutset.py](e695d8536e/test/functional/rpc_scantxoutset.py (L22)) ScantxoutsetTest to have more test coverage by adding more sendtodestination calls with bech32 and bech32m testnet addresses, then test the bech32 and bech32m derivation subsets UTXO amount in [Test extended key derivation](e695d8536e/test/functional/rpc_scantxoutset.py (L84)).
I will add the test coverage in a subsequent Pull request.
ACKs for top commit:
josibake:
ACK d178082996
theStack:
ACK d178082996✔️
willcl-ark:
ACK d17808299
Tree-SHA512: 312c20ce192c648faf7dd178622700c9b871d755db56c246250e25508c3c19e7b02c0ae901dda11a1794629b9a9429c877168c05e1c4c1dbf41493316e30e7e9
Because wallets are internally synchronized
through the validation interface, and the
interface dispatches events on a worker thread,
it is possible for a transaction created by the
first wallet to not arrive at the second wallet
before the second wallet attempts to use one of
its outputs. This is because we do not wait for
the BroadcastTransaction callback during the wallet's
"submit to mempool" process. To address this in the
tests, we need to sync the validation queue.
f3221d373a test: add wallet too-long-mempool-chain error coverage (furszy)
acf0119d24 wallet: return error msg for too-long-mempool-chain failure (furszy)
Pull request description:
Fixes#23144.
We currently return a general "Insufficient funds" from Coin
Selection when we actually skipped unconfirmed UTXOs that
surpassed the mempool ancestors limit.
This PR make the error clearer by returning:
"Unconfirmed UTXOs are available, but spending them creates
a chain of transactions that will be rejected by the mempool"
Also, added an early return from Coin Selection if the sum of
the discarded coins decreases the available balance below the
target amount.
ACKs for top commit:
achow101:
ACK f3221d373a
S3RK:
Code review ACK f3221d373a
Xekyo:
ACK f3221d373a
Tree-SHA512: 13e5824b75ac302280ff894560a4ebf32a74f32fe49ef8281f2bc99c0104b92cef33d3b143c6e131f3a07eafe64533af7fc60abff585142c134b9d6e531a6a66
fa0696e786 test: Replace threading with concurrent.futures (MarcoFalke)
Pull request description:
`threading` has no easy way to get the return value or exception once the target function stops. Not checking the return value or exception can make tests more fragile and failures harder to debug.
Fix this by checking the return value (or exception) by wrapping the function execution into a future and calling `result()` on it.
Can be reviewed with `--ignore-all-space`.
(There are still some uses of `threading` around, because some tests do expect an exception to be thrown and caught in the target function)
ACKs for top commit:
ishaanam:
ACK fa0696e786
stickies-v:
ACK fa0696e786
Tree-SHA512: d9ddf6b3c530cd8c485a030a3c84d4e03d3e9f9ea8240b050afcd566a884f5cabe816ac56910cec9ea9fa299239e5abb99e672dda05a74974f61bb68dc3c1d65
fa18504d57 rpc: Add submit option to generateblock (MarcoFalke)
fab9a08e14 refactor: Replace block_hash with block_out (MarcoFalke)
Pull request description:
When submit is turned off, a block can be generated and returned as hex, to be used for further tests. For example, it can be submitted on a different node, on a different interface (like p2p), or just never submitted and be used for other testing purposes.
ACKs for top commit:
instagibbs:
ACK fa18504d57
TheCharlatan:
tACK fa18504d57
Tree-SHA512: 1b2ab6b71bb7e155c6482d75f5373f4e77de6446cb16bc2dfd19e7a4075b3a6ad87d7ad7a049a9eed934cb71574acfd27202f54c8bb3b03fac869f2e95db7ee5
Adds bech32_to_bytes() which can decode a bech32 address and return the
version as an `int` and the payload in bytes.
bech32_to_bytes() is used by the test_bech32_decode unit test to test
decoding of segwit addresses.
4b7aec2951 Add mempool tracepoints (virtu)
Pull request description:
This PR adds multiple mempool tracepoints.
| tracepoint | description |
| ------------- | ------------- |
| `mempool:added` | Is called when a transaction enters the mempool |
| `mempool:removed` | ... when a transaction is removed from the mempool |
| `mempool:replaced` | ... when a transaction is replaced in the mempool |
| `mempool:rejected` | ... when a transaction is rejected from entering the mempool |
The tracepoints are further documented in `docs/tracing.md`. Usage is demonstrated in the example script `contrib/tracing/mempool_monitor.py`. Interface tests are provided in `test/functional/interface_usdt_mempool.py`.
The rationale for passing the removal reason as a string instead of numerically is that the benefits of not having to maintain a redundant enum-string mapping seem to outweigh the small cost of string generation. The reject reason is passed as string as well, although in this instance the string does not have to be generated but is readily available.
ACKs for top commit:
0xB10C:
ACK 4b7aec2951
achow101:
ACK 4b7aec2951
Tree-SHA512: 6deb3ba2d1a061292fb9b0f885f7a5c4d11b109b838102d8a8f4828cd68f5cd03fa3fc64adc6fdf54a08a1eaccce261b0aa90c2b8c33cd5fd3828c8f74978958
Tracepoints for added, removed, replaced, and rejected transactions.
The removal reason is passed as string instead of a numeric value, since
the benefits of not having to maintain a redundant enum-string mapping
seem to outweigh the small cost of string generation. The reject reason
is passed as string as well, although here the string does not have to
be generated but is readily available.
So far, tracepoint PRs typically included two demo scripts: a naive
bpftrace script to show raw tracepoint data and a bcc script for a more
refined view. However, as some of the ongoing changes to bpftrace
introduce a certain degree of unreliability (running some of the
existing bpftrace scripts was not possible with standard kernels and
bpftrace packages on latest stable Ubuntu, Debian, and NixOS), this PR
includes only a single bcc script that fuses the functionality of former
bpftrace and bcc scripts.
6d24d1ef2b test: check that sigop limit also affects ancestor/descendant size (Sebastian Falbesoner)
Pull request description:
This is a follow-up to #27171, adding a check that the sigop-limit vsize logic is also respected for {ancestor,descendant}size calculation (as suggested in https://github.com/bitcoin/bitcoin/pull/27171#pullrequestreview-1331143909). For simplicity, we use a one-parent-one-child cluster here and only check for the case that the sigop-limit equivalent size is larger than the serialized vsize.
ACKs for top commit:
glozow:
code review ACK 6d24d1ef2b, thanks for taking!
Tree-SHA512: dc65e455d06cfef1f1d6a53b959f99ec1ca3fe51c98dc1ed5826614b5619773d34aff0171c43a0ede4fd45605b2eb7a9278e027196128bb7ad8586b859f1cf70
Previously only the segwit utxos being spent by the psbt were looked for and
added to the psbt. Now, the full transaction corresponding to each of these
utxos (legacy and segwit) is looked for in the txindex and mempool and added
to the psbt. If txindex is disabled and the transaction is not in the mempool,
then we fall back to getting just the utxo (if segwit) from the utxo set.
fa1eb0ecae test: Make the unlikely race in p2p_invalid_messages impossible (MarcoFalke)
Pull request description:
After `add_p2p_connection` both sides have the verack processed.
However the pong from conn in reply to the ping from the node has not
been processed and recorded in totalbytesrecv.
Flush the pong from conn by sending a ping from conn.
This should make the unlikely race impossible.
ACKs for top commit:
mzumsande:
ACK fa1eb0ecae
pinheadmz:
ACK fa1eb0ecae
Tree-SHA512: 44166587572e8c0c758cac460fcfd5cf403b2883880128b13dc62e7f74ca5cb8f145bb68a903df177ff0e62faa360f913fd409b009d4cd1360f1f4403ade39ae
3dd2f6461b test: psbt: check non-witness UTXO removal for segwit v1 input (Sebastian Falbesoner)
dd78e3fa43 test: speedup rpc_psbt.py by whitelisting peers (immediate tx relay) (Sebastian Falbesoner)
e194e3e93d test: PSBT: eliminate magic numbers for global unsigned tx key (0) (Sebastian Falbesoner)
Pull request description:
This PR adds missing test coverage for dropping non-witness UTXOs from PSBTs for segwit v1+ inputs (see commit 103c6fd279). The formerly [disabled](4600479058) method `test_utxo_conversion` is re-enabled and adapted to spend a Taproot (`bech32m`) instead of a wrapped SegWit (`p2sh-segwit`) output. Note that in contrast to the original test, we have to add the non-witness UTXO manually here using the test framework's PSBT module, since the constructing node knows that the output is segwit v1 and hence doesn't add the non-witness UTXO in the first place (see also [BIP371]( https://github.com/bitcoin/bips/blob/master/bip-0371.mediawiki#user-content-UTXO_Types)).
I strongly assume that most wallets would behave the same as Bitcoin Core here and wouldn't create PSBTs with non-witness UTXOs for Taproot inputs, but it's still good to test everything works as expected if it's still done and that the non-witness UTXO is simply dropped in that case.
The first two commits contain a small refactor (magic number elimination in PSBT module) and test speedup of ~2-3x (using whitelisting peers / immediate tx relay).
ACKs for top commit:
achow101:
ACK 3dd2f6461b
instagibbs:
ACK 3dd2f6461b
Tree-SHA512: b8d7f7ea5d7d21def024b70dfca61991cc96a4193be8857018b4d7cf3ca1465d185619fd4a77623803d9da309aa489c53273e9b7683d970ce12e2399b5b50031
1ff5d61dfd doc: add mempool/contents rest verbose and mempool_sequence args (Andrew Toth)
52a31dccc9 tests: mempool/contents verbose and mempool_sequence query params tests (Andrew Toth)
a518fff0f2 rest: add verbose and mempool_sequence query params for mempool/contents (Andrew Toth)
Pull request description:
The verbose mempool json response can get very large. This adds an option to return the non-verbose response of just the txids. It is identical to the rpc response so the diff here is minimal. This also adds the mempool_sequence parameter for rpc consistency. Verbose defaults to true to remain backwards compatible.
It uses query parameters to be compatible with the efforts in https://github.com/bitcoin/bitcoin/issues/25752.
ACKs for top commit:
achow101:
ACK 1ff5d61dfd
stickies-v:
re-ACK [1ff5d61](1ff5d61dfd)
pablomartin4btc:
tested ACK 1ff5d61dfd.
Tree-SHA512: 1bf08a7ffde2e7db14dc746e421feedf17d84c4b3f1141e79e36feb6014811dfde80e1d8dbc476c15ff705de2d3c967b3081dcd80536d76b7edf888f1a92e9d1
05eeba2c5f [test] Add manual prune startup test case (dergoegge)
4517419628 [util] Avoid integer overflow in CheckDiskSpace (dergoegge)
Pull request description:
Starting a fresh node with `-prune=1` causes an integer overflow to happen in `CheckDiskSpace` ([here](f7bdcfc83f/src/init.cpp (L1633-L1648))) because `nPruneTarget` is to the max `uint64_t` value.
```
node1 stderr util/system.cpp:138:51: runtime error: unsigned integer overflow: 52428800 + 18446744073709551615 cannot be represented in type 'unsigned long'
#0 0x564a482b5088 in CheckDiskSpace(fs::path const&, unsigned long) src/./src/util/system.cpp:138:51
#1 0x564a4728dc59 in AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*) src/./src/init.cpp:1639:14
#2 0x564a47256e6a in AppInit(node::NodeContext&, int, char**) src/./src/bitcoind.cpp:221:43
#3 0x564a47256087 in main src/./src/bitcoind.cpp:265:13
#4 0x7fcb7cbffd8f (/lib/x86_64-linux-gnu/libc.so.6+0x29d8f) (BuildId: 69389d485a9793dbe873f0ea2c93e02efaa9aa3d)
#5 0x7fcb7cbffe3f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x29e3f) (BuildId: 69389d485a9793dbe873f0ea2c93e02efaa9aa3d)
#6 0x564a471957f4 in _start (/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/bitcoind+0xca07f4) (BuildId: 035cb22302d37317a630900a15a26ecb326d395c)
SUMMARY: UndefinedBehaviorSanitizer: unsigned-integer-overflow util/system.cpp:138:51 in
```
I think side stepping the overflow for this specific case, is better than adding an exception to the UB suppresions file.
ACKs for top commit:
MarcoFalke:
ACK 05eeba2c5f🥝
john-moffett:
ACK 05eeba2c5f
Tree-SHA512: 1d8e6bcb49818139f04b5ab2cbef7f9b422bf0c38a804cd532b6bd0ba4c4fd07f959ba977e59896343f213086c8ecc48180f50d006638dc84649c66ec379d58a
fa27cf4cc7 test: Default timeout factor to 4 under --valgrind (MarcoFalke)
Pull request description:
valgrind will incur a slowdown of at least 2, so increase the default timeout factor.
This should reduce the number of reported issues. See also https://github.com/bitcoin/bitcoin/issues/27112#issuecomment-1455762739
ACKs for top commit:
fanquake:
ACK fa27cf4cc7 - I still see at least one actual issue when running the functional tests under `--valgrind` (outside the CI system), but will follow up separately with that. Increasing the timeout here seems fine.
Tree-SHA512: 4467559a3bfd98f5735f300f6ed54c68f951191d65a2b1294d71d72cc5d0864964de562d5dfa0a4855fc541ccb269a538b7aeb3d408d2d012a5369513397c395
89cd20cbed test: add coverage for sigop limit policy (`-bytespersigop` setting) (Sebastian Falbesoner)
Pull request description:
This PR adds missing test coverage for the `-bytespersigop` option, which determines how pre-taproot signature operations (OP_CHECKSIG{VERIFY}, OP_CHECKMULTIGSIG{VERIFY}) affect fee handling calculations. The setting was introduced in PR #7081 for mitigating the [sigop spam attack](https://bitcointalk.org/index.php?topic=1166928.0); the initial implementation rejected txs exceeding the limit, but was changed in #8365 later to account for higher sizes in the mempool (i.e. exceeding the sigop limit is possible, but has to be compensated by higher fees).
For each combination of `-bytespersigop` setting and sigops count, the test first creates a P2WSH spending transaction with a witness script that puts sigops in a non-executing branch (OP_FALSE OP_IF OP_CHECKMULTISIG ... OP_CHECKSIG ... OP_ENDIF). This tx is then bumped up to reach exactly the _sig-op limit equivalent vsize_ by padding its datacarrier output. Based on that, increasing the tx's vsize should still reflect a vsize increase in the mempool, while a decrease of the tx's vsize should lead to the mempool treating the tx's vsize to be the _sig-op limit equivalent vsize_, since the limit was exceeded.
I assume that this parameter is almost never set explicitly by users (also it is not relevant for taproot spends), but it doesn't hurt to have a test for it. See also https://bitcoin.stackexchange.com/a/87958 for another explanation.
ACKs for top commit:
glozow:
light review ACK 89cd20cbed
MarcoFalke:
nice ACK 89cd20cbed📁
Tree-SHA512: 06998ce93bf9d5ce6143db2996a43f13990c415f97afe684227ad469349e73952bf4f6c871c1e6349e07606f4d45db64408848873a86a89481cdca5a134e5e60
faa671591f test: Use self.wait_until over wait_until_helper (MarcoFalke)
Pull request description:
`wait_until_helper` is a "private" helper, not intended to be used directly, because it doesn't scale the timeout with the timeout factor. Fix this by replacing it with a call to `self.wait_until`, which does the scaling.
ACKs for top commit:
theStack:
Code-review ACK faa671591f
Tree-SHA512: 70705f309f83ffd6ea5d090218195d05b868624d909106863372f861138b5a70887070b25beb25044ae1b44250345e45c9cc11191ae7aeca2ad37801a0f62f61
fe329dc936 test: Add test for getblockfrompeer on pruned nodes (Fabian Jahr)
cd761e6b2c rpc: Add note on guarantees to getblockfrompeer (Fabian Jahr)
Pull request description:
These are additions to `getblockfrompeer` that I already [suggested on the original PR](https://github.com/bitcoin/bitcoin/pull/20295#pullrequestreview-817157738).
The two commits do the following:
1. Add a test for `getblockfrompeer` usage on pruned nodes. This is important because many use-cases for `getblockfrompeer` are in a context of a pruned node.
2. Add some information on how long the users of pruned nodes can expect the block to be available after they have used the RPC. I think the behavior is not very intuitive for users and I would not be surprised if users expect the block to be available indefinitely.
ACKs for top commit:
Sjors:
re-utACK fe329dc936
MarcoFalke:
review ACK fe329dc936🍉
stratospher:
ACK fe329dc.
brunoerg:
re-ACK fe329dc936
Tree-SHA512: a686bd8955d9c3baf365db384e497d6ee1aa9ce2fdb0733fe6150f7e3d94bae19d55bc1b347f1c9f619e749e18b41a52b9f8c0aa2042dd311a968a4b5d251fac
fa0abcdafe test: Flatten miniwallet array and remove random fee in longpoll (MarcoFalke)
Pull request description:
* Using a single MiniWallet is enough.
* A random fee isn't needed either.
ACKs for top commit:
theStack:
re-ACK fa0abcdafe
Tree-SHA512: 77b99885b3f0d325d067838122114be57ec999ebc82912de6a22c33e2ba28a341c5e053c5bbc424b9922c2616562289a57c7156bd3b431d779182c2e472da59c
b082f28101 rpc, wallet: use the same `next_index` in listdescriptors and importdescriptors (w0xlt)
Pull request description:
Currently `listdescriptors` RPC uses `next` key to represent `WalletDescriptor::next_index` while `importdescriptors` uses `next_index`. This creates two different descriptor formats.
This PR changes `listdescriptors` to use the same key as `importdescriptors`.
ACKs for top commit:
achow101:
ACK b082f28101
aureleoules:
reACK b082f28101
Tree-SHA512: c29ec59051878e614d749ed6dc85e5c14ad00db0e8fcbce3f5066d1aae85ef07ca70f02920299e48d191b7387024fe224b0054c4191a5951cb805106f7b8e37b
802cc1ef53 Deduplicate bitcoind and bitcoin-qt init code (Ryan Ofsky)
d172b5c671 Add InitError(error, details) overload (Ryan Ofsky)
3db2874bd7 Extend bilingual_str support for tinyformat (Ryan Ofsky)
c361df90b9 scripted-diff: Remove double newlines after some init errors (Ryan Ofsky)
Pull request description:
Add common InitConfig function to deduplicate bitcoind and bitcoin-qt code reading config files and creating the datadir.
Noticed the duplicate code while reviewing #27073 and want to remove it because difference in bitcoin-qt and bitcoind behavior make it hard to evaluate changes like #27073
There are a few minor changes in behavior:
- In bitcoin-qt, when there is a problem reading the configuration file, the GUI error text has changed from "Error: Cannot parse configuration file:" to "Error reading configuration file:" to be consistent with bitcoind.
- In bitcoind, when there is a problem reading the settings.json file, the error text has changed from "Failed loading settings file" to "Settings file could not be read" to be consistent with bitcoin-qt.
- In bitcoind, when there is a problem writing the settings.json file, the error text has changed from "Failed saving settings file" to "Settings file could not be written" to be consistent with bitcoin-qt.
- In bitcoin-qt, if there datadir is not accessible (e.g. no permission to read), there is an normal error dialog showing "Error: filesystem error: status: Permission denied [.../settings.json]", instead of an uncaught exception.
ACKs for top commit:
Sjors:
Light review ACK 802cc1ef53
TheCharlatan:
ACK 802cc1ef53
achow101:
ACK 802cc1ef53
Tree-SHA512: 9c78d277e9ed595fa8ce286b97d2806e1ec06ddbbe7bd3434bd9dd7b456faf8d989f71231e97311f36edb9caaec645a50c730bd7514b8e0fe6e6f7741b13d981
60978c8080 test: Reduce extended timeout on abortnode test (Fabian Jahr)
660bdbf785 http: Release server before waiting for event base loop exit (João Barbosa)
8c6d007c80 http: Track active requests and wait for last to finish (João Barbosa)
Pull request description:
This revives #19420. Since promag is not so active at the moment, I can support this to finally get it merged.
The PR is rebased and comments by jonatack have been addressed.
Once this is merged, I will also reopen#19434.
ACKs for top commit:
achow101:
ACK 60978c8080
stickies-v:
re-ACK [60978c8](60978c8080)
hebasto:
ACK 60978c8080
Tree-SHA512: eef0fe1081e9331b95cfafc71d82f2398abd1d3439dac5b2fa5c6d9c0a3f63ef19adde1c38c88d3b4e7fb41ce7c097943f1815c10e33d165918ccbdec512fe1c
master branch:
0m36.86s real 0m03.26s user 0m01.69s system
0m35.71s real 0m03.78s user 0m01.64s system
0m45.76s real 0m03.12s user 0m01.27s system
PR branch:
0m13.04s real 0m02.66s user 0m00.93s system
0m14.08s real 0m02.81s user 0m00.82s system
0m14.05s real 0m02.50s user 0m00.93s system
Previous bilingual_str tinyformat::format accepted bilingual format strings,
but not bilingual arguments. Extend it to accept both. This is useful when
embedding one translated string inside another translated string, for example:
`strprintf(_("Error: %s"), message)` which would fail previously if `message`
was a bilingual_str.
3141eab9c6 test: add functional test for ScanAndUnlinkAlreadyPrunedFiles (Andrew Toth)
e252909e56 test: add unit test for ScanAndUnlinkAlreadyPrunedFiles (Andrew Toth)
77557dda4a prune: scan and unlink already pruned block files on startup (Andrew Toth)
Pull request description:
There are a few cases where we can mark a block and undo file as pruned in our block index, but not actually remove the files from disk.
1. If we call `FindFilesToPrune` or `FindFilesToPruneManual` and crash before `UnlinkPrunedFiles`.
2. If on Windows there is an open file handle to the file somewhere else when calling `fs::remove` in `UnlinkPrunedFiles` (https://en.cppreference.com/w/cpp/filesystem/remove, https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-deletefilew#remarks). This could be from another process, or if we are calling `ReadBlockFromDisk`/`ReadRawBlockFromDisk` without having a lock on `cs_main` (which has been allowed since ccd8ef65f9).
This PR mitigates this by scanning all pruned block files on startup after `LoadBlockIndexDB` and unlinking them again.
ACKs for top commit:
achow101:
ACK 3141eab9c6
pablomartin4btc:
re-ACK with added functional test 3141eab9c6.
furszy:
Code review ACK 3141eab9
theStack:
Code-review ACK 3141eab9c6
Tree-SHA512: 6c73bc57838ad1b7e5d441af3c4d6bf4c61c4382e2b86485e57fbb74a61240710c0ceeceb8b4834e610ecfa3175c6955c81ea4b2285fee11ca6383f472979d8d
7013da07fb Add release note for PR#25943 (David Gumberg)
04f270b435 Add test for unspendable transactions and parameter 'maxburnamount' to sendrawtransaction. (David Gumberg)
Pull request description:
This PR adds a user configurable, zero by default parameter — `maxburnamount` — to `sendrawtransaction`. This PR makes bitcoin core reject transactions that contain unspendable outputs which exceed `maxburnamount`. closes#25899.
As a result of this PR, `sendrawtransaction` will by default block 3 kinds of transactions:
1. Those that begin with `OP_RETURN` - (datacarriers)
2. Those whose lengths exceed the script limit.
3. Those that contain invalid opcodes.
The user is able to configure a `maxburnamount` that will override this check and allow a user to send a potentially unspendable output into the mempool.
I see two legitimate use cases for this override:
1. Users that deliberately use `OP_RETURN` for datacarrier transactions that embed data into the blockchain.
2. Users that refuse to update, or are unable to update their bitcoin core client would be able to make use of new opcodes that their client doesn't know about.
ACKs for top commit:
glozow:
reACK 7013da07fb
achow101:
re-ACK 7013da07fb
Tree-SHA512: f786a796fb71a587d30313c96717fdf47e1106ab4ee0c16d713695e6c31ed6f6732dff6cbc91ca9841d66232166eb058f96028028e75c1507324426309ee4525
2f84ad7b9e docs: add ramdisk guide for running tests on OSX (Matthew Zipkin)
Pull request description:
Using a ramdisk on OSX sped up the test suite by about 5x (using default `jobs=4`) on my M1 macbook pro running macOS Monterey 12.3.1. This PR adds the relevant OSX commands following the Linux directions.
Default:
```
8204 s (accumulated)
Runtime: 2104 s
```
following commands from the PR:
```
1606 s (accumulated)
Runtime: 421 s
```
ramdisk + `jobs=32`:
```
2090 s (accumulated)
Runtime: 85 s
```
ACKs for top commit:
jonatack:
ACK 2f84ad7b9e
willcl-ark:
ACK 2f84ad7b9e
brunoerg:
utACK 2f84ad7b9e
Tree-SHA512: 37a9903c8ac2cbfaa91e7e73fc96ef65042ff4b15763d452af7b8615255adf03429ad01cf85265a99dd569290c1d69c05a393d616868c05c190b60b053820786
0af16e7134 doc: add release note for #25574 (Martin Zumsande)
57ef2a4812 validation: report if pruning prevents completion of verification (Martin Zumsande)
0c7785bb25 init, validation: Improve handling if VerifyDB() fails due to insufficient dbcache (Martin Zumsande)
d6f781f1cf validation: return VerifyDBResult::INTERRUPTED if verification was interrupted (Martin Zumsande)
6360b5302d validation: Change return value of VerifyDB to enum type (Martin Zumsande)
Pull request description:
`VerifyDB()` can fail to complete due to insufficient dbcache at the level 3 checks. This PR improves the error handling in this case in the following ways:
- The rpc `-verifychain` now returns false if the check can't be completed due to insufficient cache
- During init, we only log a warning if the default values for `-checkblocks` and `-checklevel` are taken and the check doesn't complete. However, if the user actively specifies one of these args, we return with an InitError if we can't complete the check.
This PR also changes `-verifychain` RPC to return `false` if the verification didn't finish due to missing block data (pruning) or due to being interrupted by the node being shutdown.
Previously, this PR also included a fix for a possible assert during verification - this was done in #27009 (now merged).
ACKs for top commit:
achow101:
ACK 0af16e7134
ryanofsky:
Code review ACK 0af16e7134. Only small suggested changes since the last review, like renaming some of the enum values. I did leave more suggestions, but they are not very important and could be followups
john-moffett:
ACK 0af16e7134
MarcoFalke:
lgtm re-ACK 0af16e7134 🎚
Tree-SHA512: 84b4f767cf9bfbafef362312757c9bf765b41ae3977f4ece840e40c52a2266b1457832df0cdf70440be0aac2168d9b58fc817238630b0b6812f3836ca950bc0e
c3b4b5a142 test: Replace 0xC0 constant (roconnor-blockstream)
Pull request description:
Instead it should be the named constant `LEAF_VERSION_TAPSCRIPT`.
ACKs for top commit:
instagibbs:
ACK c3b4b5a142
theStack:
ACK c3b4b5a142
Tree-SHA512: c00be584ea2d0e7c01bf5620da0da1f37e5b5298ef95df48d91d137c8c542f5d91be158d45392cf2ba8874bf27bd12924e2eed395773b49d091e3028de3356a2
4bbf5ddd44 Detailed error message for passphrases with null chars (John Moffett)
b4bdabc223 doc: Release notes for 27068 (John Moffett)
4b1205ba37 Test case for passphrases with null characters (John Moffett)
00a0861181 Pass all characters to SecureString including nulls (John Moffett)
Pull request description:
`SecureString` is a `std::string` specialization with a secure allocator. However, in practice it's treated like a C- string (no explicit length and null-terminated). This can cause unexpected and potentially insecure behavior. For instance, if a user enters a passphrase with embedded null characters (which is possible through Qt and the JSON-RPC), it will ignore any characters after the first null, potentially giving the user a false sense of security.
Instead of assigning to `SecureString` via `std::string::c_str()`, assign it via a `std::string_view` of the original. This explicitly captures the size and still doesn't make any extraneous copies in memory.
Note to reviewers, the following all compile identically in recent `GCC` (x86-64 and ARM64) with `-O2` (and `-std=c++17`):
```C++
std::string orig_string;
std::cin >> orig_string;
SecureString s;
s.reserve(100);
// The following all compile identically
s = orig_string;
s = std::string_view{orig_string};
s.assign(std::string_view{orig_string});
s.assign(orig_string.data(), orig_string.size());
```
So it's largely a matter of preference. However, one thing to keep in mind is that we want to avoid making unnecessary copies of any sensitive data in memory.
Something like `SecureString s{orig_string};` is still invalid and probably unwanted in our case, since it'd get treated as a short string and optimized away from the secure allocator. I presume that's the reason for the `reserve()` calls.
Fixes#27067.
ACKs for top commit:
achow101:
re-ACK 4bbf5ddd44
stickies-v:
re-ACK [4bbf5dd](4bbf5ddd44)
furszy:
utACK 4bbf5ddd
Tree-SHA512: 47a96905a82ca674b18076a20a388123beedf70e9de73e42574ea68afbb434734e56021835dd9b148cdbf61709926b487cc95e9021d9bc534a7c93b3e143d2f7
4d84eaec82 Raise PRNG seed log to INFO. (roconnor-blockstream)
Pull request description:
Some build infrastructure, such as Nix, will delete failed builds by default, keeping only the log (stdout/stderr) of the failed build.
For flaky tests, it would be very helpful to have the PRNG seed in the default log in order to redo the failed test.
By simply raising the PRNG seed logging to INFO, we can, by default, record the seed in the log of every build.
ACKs for top commit:
MarcoFalke:
lgtm ACK 4d84eaec82
theStack:
ACK 4d84eaec82
Tree-SHA512: 3ccb4a4e7639a3babc3b2a6456a6d0bffc090da34e4545b317f7bfbed4e9950d1b38ea5b2a90c37ccb49b3454bdeff03a6aaf86770b9c4dd14b26320aba50b94
9486509be6 wallet, rpc: Update migratewallet help text for encrypted wallets (Andrew Chow)
aaf02b5721 tests: Tests for migrating wallets by name, and providing passphrase (Andrew Chow)
7fd125b27d wallet: Be able to unlock the wallet for migration (Andrew Chow)
6bdbc5ff59 rpc: Allow users to specify wallet name for migratewallet (Andrew Chow)
dbfa345403 wallet: Allow MigrateLegacyToDescriptor to take a wallet name (Andrew Chow)
Pull request description:
`migratewallet` currently operates on wallets that are already loaded, however this is not necessarily required, and in the future, not possible once the legacy wallet is removed. So we need to also be able to give the wallet name to migrate.
Additionally, the passphrase is required when migrating a wallet. Since a wallet may not be loaded when we migrate, and as we currently unload wallets when migrating, we need the passphrase to be given to `migratewallet` in order to migrate encrypted wallets.
Fixes#27048
ACKs for top commit:
john-moffett:
reACK 9486509be6
pinheadmz:
ACK 9486509be6
furszy:
ACK 9486509b
Tree-SHA512: 35e2ba69a148e129a41e20d7fb99c4cab7947b1b7e7c362f4fd06ff8ac6e79e476e07207e063ba5b80e1a33e2343f4b4f1d72d7930ce80c34571c130d2f5cff4
61bb4e783b lint: enable E722 do not use bare except (Leonardo Lazzaro)
Pull request description:
Improve test code and enable E722 lint check.
If you want to catch all exceptions that signal program errors, use except Exception: (bare except is equivalent to except BaseException:).
Reference: https://peps.python.org/pep-0008/#programming-recommendations
ACKs for top commit:
MarcoFalke:
lgtm ACK 61bb4e783b
Tree-SHA512: c7497769d5745fa02c78a20f4a0e555d8d3996d64af6faf1ce28e22ac1d8be415b98e967294679007b7bda2a9fd04031a9d140b24201e00257ceadeb5c5d7665
6a5b348f2e test: test rescanning encrypted wallets (ishaanam)
493b813e17 wallet: ensure that the passphrase is not deleted from memory when being used to rescan (ishaanam)
66a86ebabb wallet: keep track of when the passphrase is needed when rescanning (ishaanam)
Pull request description:
Wallet passphrases are needed to top up the keypool of encrypted wallets
during a rescan. The following RPCs need the passphrase when rescanning:
- `importdescriptors`
- `rescanblockchain`
The following RPCs use the information about whether or not the
passphrase is being used to ensure that full rescans are able to
take place (meaning the following RPCs should not be able to run
if a rescan requiring the wallet to be unlocked is taking place):
- `walletlock`
- `encryptwallet`
- `walletpassphrasechange`
`m_relock_mutex` is also introduced so that the passphrase is not
deleted from memory when the timeout provided in
`walletpassphrase` is up and the wallet is still rescanning.
Fixes#25702, #11249
Thanks to achow101 for coming up with the idea of using a new mutex to solve this issue and for answering related questions.
ACKs for top commit:
achow101:
ACK 6a5b348f2e
hernanmarino:
ACK 6a5b348f2e
furszy:
Tested ACK 6a5b348f
Tree-SHA512: 0b6db692714f6f94594fa47249f5ee24f85713bfa70ac295a7e84b9ca6c07dda65df7b47781a2dc73e5b603a8725343a2f864428ae20d3e126c5b4802abc4ab5
Some build infrastructure, such as Nix, will delete failed builds by default, keeping only the log of the failed build.
For flaky tests, it would be very helpful to have the PRNG seed in the default log in order to redo the failed test.
By simply raising the PRNG seed logging to INFO, we can, by default, record the seed in the log of every build.
14b4921a91 wallet: reuse change dest when recreating TX with avoidpartialspends (Matthew Zipkin)
Pull request description:
Closes https://github.com/bitcoin/bitcoin/issues/27051
When the wallet creates a transaction internally, it will also create an alternative that spends using destination groups and see if the fee difference is negligible. If it costs the user the same to send the grouped version, we send it (even if the user has `avoidpartialspends` set to `false` which is default). This patch ensures that the second transaction creation attempt re-uses the change destination selected by the first attempt. Otherwise, the first change address remains reserved, will not be used in the second attempt, and then will never be used by the wallet, leaving gaps in the BIP44 chain.
If the user had `avoidpartialspends` set to true, there is no second version of the created transaction and the change addresses are not affected.
I believe this behavior was introduced in https://github.com/bitcoin/bitcoin/pull/14582
ACKs for top commit:
achow101:
ACK 14b4921a91
Tree-SHA512: a3d56f251ff4b333fc11325f30d05513e34ab0a2eb703fadd0ad98d167ae074493df1a24068298336c6ed2da6b31aa2befa490bc790bbc260ed357c8f2397659
73ec4b2a83 tests: decodescript can infer descriptors for scripts >520 bytes (Andrew Chow)
7cc7822371 rpc: Use FlatSigningProvider in decodescript (Andrew Chow)
Pull request description:
`FillableSigningProvider` limits scripts to 520 bytes even though segwit allows scripts to be larger than that. We can avoid this limit by using a `FlatSigningProvider` so that such larger scripts can be decoded.
Fixes#27111
ACKs for top commit:
instagibbs:
ACK 73ec4b2a83
Tree-SHA512: c0e6d21025e2da864471989ac94c54e127d05459b9b048f34a0da8d76d8e372d5472a2e667ba2db74d6286e3e6faa55486ffa9232a068b519afa676394031d5a
1819564c21 test: fix intermittent issue in `p2p_disconnect_ban` (brunoerg)
Pull request description:
Fixes#26808
When `node0` calls `disconnectnode` to disconnect `node1`, we should check in `node1` if it worked, because for `node0` the informations in `getpeerinfo` may be updated before really completing the disconnection.
ACKs for top commit:
MarcoFalke:
lgtm ACK 1819564c21
Tree-SHA512: 53a386fc38e2faa6f6da3536e76857ff4b6f55e2590d73fe857b3fe5d0f3ff92c5c7e4abd50ab4be250cb2106a4d14ad95d4809ea60c6e00ed3ac0e71255b0b0
14302a4802 test: fix test abort for high timeout values (and `--timeout-factor 0`) (Sebastian Falbesoner)
Pull request description:
On master, the functional tests's option `--timeout-factor 0` (which according to the test docs and parameter description should disable the RPC timeouts) currently fails, same as high values like `--timeout-factor 999999`:
```
$ ./test/functional/wallet_basic.py --timeout-factor 0
2022-08-29T01:26:39.561000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_f24yxzp5
2022-08-29T01:26:40.262000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/home/honey/bitcoin/test/functional/test_framework/test_framework.py", line 549, in start_nodes
node.wait_for_rpc_connection()
File "/home/honey/bitcoin/test/functional/test_framework/test_node.py", line 234, in wait_for_rpc_connection
rpc.getblockcount()
File "/home/honey/bitcoin/test/functional/test_framework/coverage.py", line 49, in __call__
return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
File "/home/honey/bitcoin/test/functional/test_framework/authproxy.py", line 142, in __call__
response, status = self._request('POST', self.__url.path, postdata.encode('utf-8'))
File "/home/honey/bitcoin/test/functional/test_framework/authproxy.py", line 107, in _request
self.__conn.request(method, path, postdata, headers)
File "/usr/local/lib/python3.9/http/client.py", line 1285, in request
self._send_request(method, url, body, headers, encode_chunked)
File "/usr/local/lib/python3.9/http/client.py", line 1331, in _send_request
self.endheaders(body, encode_chunked=encode_chunked)
File "/usr/local/lib/python3.9/http/client.py", line 1280, in endheaders
self._send_output(message_body, encode_chunked=encode_chunked)
File "/usr/local/lib/python3.9/http/client.py", line 1040, in _send_output
self.send(msg)
File "/usr/local/lib/python3.9/http/client.py", line 980, in send
self.connect()
File "/usr/local/lib/python3.9/http/client.py", line 946, in connect
self.sock = self._create_connection(
File "/usr/local/lib/python3.9/socket.py", line 844, in create_connection
raise err
File "/usr/local/lib/python3.9/socket.py", line 832, in create_connection
sock.connect(sa)
OSError: [Errno 22] Invalid argument
```
This is caused by a high timeout value that Python's HTTP(S) client library can't cope with. Fix this by clamping down the connection's set timeout value in AuthProxy. The change can easily be tested by running an arbitrary test with `--timeout-factor 0` on master (should fail), on this PR (should pass) and on this PR with the clamping value increased by 1 (should fail).
// EDIT: The behaviour was observed on OpenBSD 7.1 and Python 3.9.12.
ACKs for top commit:
MarcoFalke:
lgtm ACK 14302a4802
Tree-SHA512: 6469e8ac699f1bb7dea11d5fb8b3ae54d895bb908570587c5631144cd41fe980ca0b1e6d0b7bfa07983307cba15fb26ae92e6766375672bf5be838d8e5422dbc
When `node0` calls `disconnectnode` to disconnect `node1`, we should check in `node1` if it worked, because for `node0` the informations in `getpeerinfo` may be updated before really completing the disconnection.
2555a3950f p2p: ProcessAddrFetch(-seednode) is unnecessary if -connect is specified (Dhruv Mehta)
Pull request description:
If the user runs: `bitcoind -connect=X -seednode=Y`, I _think_ it is safe to ignore `-seednode`. A more populated `addrman` (via `getaddr` calls to peers in `-seednode`) is not useful in this configuration: `addrman` entries are used to initiate new outbound connections when slots are open, or to open feeler connections and keep `addrman` from getting stale. This is all done in a part of `ThreadOpenConnections` (below [this line](https://github.com/bitcoin/bitcoin/blob/master/src/net.cpp#L1803)) which is never executed when `-connect` is supplied. With `-connect`, `ThreadOpenConnections` will run [this loop](https://github.com/bitcoin/bitcoin/blob/master/src/net.cpp#L1785) and exit thread execution when interrupted.
Reviewers may also find it relevant that when `-connect` is used, we [soft disable](https://github.com/bitcoin/bitcoin/blob/master/src/init.cpp#L800) `-dnsseed` in init.cpp perhaps for the same reason i.e. seeding is not useful with `-connect`.
Running `ProcessAddrFetch` does not seem to have downside except developer confusion AFAICT. I was confused by this and felt it might affect other new bitcoiners too. If there is strong preference to not remove the line, I'd also be happy to just leave a comment there mentioning `ADDR_FETCH`/`-seednode` is irrelevant when used with `-connect`.
If this change is accepted, the node will still make `getaddr` calls to peers in `-connect` and expand `addrman`. However, disabling those `getaddr` calls would leak information about the node's configuration.
ACKs for top commit:
mzumsande:
Code Review ACK 2555a3950f
achow101:
ACK 2555a3950f
vasild:
ACK 2555a3950f
Tree-SHA512: 9187a0cff58db8edeca7e15379b1c121e7ebe8c38fb82f69e3dae8846ee94c92a329d79025e0f023c7579b2d86e7dbf756e4e30e90a72236bfcd2c00714180b3
30a3230e86 script: remove out-of-date snprintf TODO (Jon Atack)
0e015146bd net: remove orphaned CSubNet::SanityCheck() (Jon Atack)
Pull request description:
`CSubNet::SanityCheck()` was added in #20140, and not removed in #22570 when it became orphaned code.
Also, remove an out-of-date `snprintf` TODO that was resolved in #27036, and fix up 2 words to make the spelling linter green again.
ACKs for top commit:
fanquake:
ACK 30a3230e86
pinheadmz:
ACK 30a3230e86
brunoerg:
crACK 30a3230e86
Tree-SHA512: f91a2a5af902d3b82ab496f19deeac17d58dbf72a8016e880ea61ad858b66e7ea0ae70b964c4032018eb3252cc34ac5fea163131c6a7f1baf87fc9ec9b5833d8
4c8ecccdcd test: add tests for `outputs` argument to `bumpfee`/`psbtbumpfee` (Seibart Nedor)
c0ebb98382 wallet: add `outputs` arguments to `bumpfee` and `psbtbumpfee` (Seibart Nedor)
a804f3cfc0 wallet: extract and reuse RPC argument format definition for outputs (Seibart Nedor)
Pull request description:
This implements a modification of the proposal in #22007: instead of **adding** outputs to the set of outputs in the original transaction, the outputs given by `outputs` argument **completely replace** the outputs in the original transaction.
As noted below, this makes it easier to "cancel" a transaction or to reduce the amounts in the outputs, which is not the case with the original proposal in #22007, but it seems from the discussion in this PR that the **replace** behavior is more desirable than **add** one.
ACKs for top commit:
achow101:
ACK 4c8ecccdcd
1440000bytes:
Code Review ACK 4c8ecccdcd
ishaanam:
reACK 4c8ecccdcd
Tree-SHA512: 31361f4a9b79c162bda7929583b0a3fd200e09f4c1a5378b12007576d6b14e02e9e4f0bab8aa209f08f75ac25a1f4805ad16ebff4a0334b07ad2378cc0090103
Since migration reloads the wallet, the wallet will always be locked
unless the passphrase is given. migratewallet can now take the
passphrase in order to unlock the wallet for migration.
7a83aa0982 test: add coverage for unparsable `-maxuploadtarget` (brunoerg)
Pull request description:
This PR adds test coverage for the following error:
7386da7a0b/src/init.cpp (L1096-L1099)
Top commit has no ACKs.
Tree-SHA512: c115b2b4d2d0eb2316bf9fafd7e0046aa18c9650062779b3a82d6145d188765bff5317f4ca5f79607732fde6d83e1f67756ac20a12c98d060ee68d8acc20c76e
741908afc1 test: previous releases: add v24.0.1 (Sebastian Falbesoner)
Pull request description:
The same procedure as every release (see dba1231672 [v23.0] and d8b705f1ca [v22.0]), only a little simpler now: thanks to #25650, the previous release fetch script defaults to downloading/building the necessary tags, i.e. we don't need to extend the tag list in the CI scripts and test/README.md anymore.
ACKs for top commit:
Sjors:
tACK 741908afc1
Tree-SHA512: a5426e989bd0bba42aa13e7d4cf60f792bf36bd9a6cdb6ef5799f7574d9a8a20979244627bbd0c6219630367e7fd73bac9e677814bc50233f64592ad035e713e
6c7a17a8e0 psbt: support externally provided preimages for Miniscript satisfaction (Antoine Poinsot)
840a396029 qa: add a "smart" Miniscript fuzz target (Antoine Poinsot)
17e3547241 qa: add a fuzz target generating random nodes from a binary encoding (Antoine Poinsot)
611e12502a qa: functional test Miniscript signing with key and timelocks (Antoine Poinsot)
d57b7f2021 refactor: make descriptors in Miniscript functional test more readable (Antoine Poinsot)
0a8fc9e200 wallet: check solvability using descriptor in AvailableCoins (Antoine Poinsot)
560e62b1e2 script/sign: signing support for Miniscripts with hash preimage challenges (Antoine Poinsot)
a2f81b6a8f script/sign: signing support for Miniscript with timelocks (Antoine Poinsot)
61c6d1a844 script/sign: basic signing support for Miniscript descriptors (Antoine Poinsot)
4242c1c521 Align 'e' property of or_d and andor with website spec (Pieter Wuille)
f5deb41780 Various additional explanations of the satisfaction logic from Pieter (Pieter Wuille)
22c5b00345 miniscript: satisfaction support (Antoine Poinsot)
Pull request description:
This makes the Miniscript descriptors solvable.
Note this introduces signing support for much more complex scripts than the wallet was previously able to solve, and the whole tooling isn't provided for a complete Miniscript integration in the wallet. Particularly, the PSBT<->Miniscript integration isn't entirely covered in this PR.
ACKs for top commit:
achow101:
ACK 6c7a17a8e0
sipa:
utACK 6c7a17a8e0 (to the extent that it's not my own code).
Tree-SHA512: a71ec002aaf66bd429012caa338fc58384067bcd2f453a46e21d381ed1bacc8e57afb9db57c0fb4bf40de43b30808815e9ebc0ae1fbd9e61df0e7b91a17771cc
that was resolved in PR27036 "test: Remove last uses of snprintf and simplify"
and while here, fix up 2 words in docs to make the spelling linter green again.
dee8549be3 test: simplify and speedup mempool_updatefromblock.py by using MiniWallet (Sebastian Falbesoner)
Pull request description:
This PR simplifies the functional test mempool_updatefromblock.py by using MiniWallet in order to avoid manual low-level tx creation (signing, outputs selection, fee calculation). Most of the tedious work is done by the method `MiniWallet.send_self_transfer_multi` (calling `create_self_transfer_multi` internally) which supports spending a given set of UTXOs and creating a certain number of outputs.
As a nice side-effect, the test's performance increases significantly (~3.5x on my system):
```
master
1m56.80s real 1m50.10s user 0m06.36s system
PR
0m32.34s real 0m30.26s user 0m01.41s system
```
The arguments `start_input_txid` and `end_address` have been removed from the `transaction_graph_test` method, as they are currently unused and I don't see them being needed for future tests.
ACKs for top commit:
brunoerg:
crACK dee8549be3
MarcoFalke:
lgtm ACK dee8549be3🚏
Tree-SHA512: 9f6da634bdc8c272f9a2af1cddaa364ee371d4e95554463a066249eecebb668d8c6cb123ec8a5404c41b3291010c0c8806a8a01dd227733cec03e73aa93b0103
511aa4f1c7 Add unit test for ChaCha20's new caching (Pieter Wuille)
fb243d25f7 Improve test vectors for ChaCha20 (Pieter Wuille)
93aee8bbda Inline ChaCha20 32-byte specific constants (Pieter Wuille)
62ec713961 Only support 32-byte keys in ChaCha20{,Aligned} (Pieter Wuille)
f21994a02e Use ChaCha20Aligned in MuHash3072 code (Pieter Wuille)
5d16f75763 Use ChaCha20 caching in FastRandomContext (Pieter Wuille)
38eaece67b Add fuzz test for testing that ChaCha20 works as a stream (Pieter Wuille)
5f05b27841 Add xoroshiro128++ PRNG (Martin Leitner-Ankerl)
12ff72476a Make unrestricted ChaCha20 cipher not waste keystream bytes (Pieter Wuille)
6babf40213 Rename ChaCha20::Seek -> Seek64 to clarify multiple of 64 (Pieter Wuille)
e37bcaa0a6 Split ChaCha20 into aligned/unaligned variants (Pieter Wuille)
Pull request description:
This is an alternative to #25354 (by my benchmarking, somewhat faster), subsumes #25712, and adds additional test vectors.
It separates the multiple-of-64-bytes-only "core" logic (which becomes simpler) from a layer around which performs caching/slicing to support arbitrary byte amounts. Both have their uses (in particular, the MuHash3072 code can benefit from multiple-of-64-bytes assumptions), plus the separation results in more readable code. Also, since FastRandomContext effectively had its own (more naive) caching on top of ChaCha20, that can be dropped in favor of ChaCha20's new built-in caching.
I thought about rebasing #25712 on top of this, but the changes before are fairly extensive, so redid it instead.
ACKs for top commit:
ajtowns:
ut reACK 511aa4f1c7
dhruv:
tACK crACK 511aa4f1c7
Tree-SHA512: 3aa80971322a93e780c75a8d35bd39da3a9ea570fbae4491eaf0c45242f5f670a24a592c50ad870d5fd09b9f88ec06e274e8aa3cefd9561d623c63f7198cf2c7
772671245d test: p2p: check that headers message with invalid proof-of-work disconnects peer (Sebastian Falbesoner)
Pull request description:
One of the earliest anti-DoS checks done after receiving and deserializing a `headers` message from a peer is verifying whether the proof-of-work is valid (called in method `PeerManagerImpl::ProcessHeadersMessage`):
f227e153e8/src/net_processing.cpp (L2752-L2762)
The called method `PeerManagerImpl::CheckHeadersPoW` calls `Misbehaving` with a score of 100, i.e. leading to an immediate disconnect of the peer:
f227e153e8/src/net_processing.cpp (L2368-L2372)
This PR adds a simple test for both the misbehaving log and the resulting disconnect. For creating a block header with invalid proof-of-work, we first create one that is accepted by the node (the difficulty field `nBits` is copied from the genesis block) and based on that the nonce is modified until we have block header hash prefix that is too high to fulfill even the minimum difficulty.
ACKs for top commit:
Sjors:
ACK 772671245d
achow101:
ACK 772671245d
brunoerg:
crACK 772671245d
furszy:
Code review ACK 77267124 with a non-blocking speedup.
Tree-SHA512: 680aa7939158d1dc672b90aa6554ba2b3a92584b6d3bcb0227776035858429feb8bc66eed18b47de0fe56df7d9b3ddaee231aaeaa360136603b9ad4b19e6ac11
ab4efad51b test: fix immediate tx relay in wallet_groups.py (Sebastian Falbesoner)
Pull request description:
In the functional test wallet_groups.py we whitelist peers on all nodes (`-whitelist=noban@127.0.0.1`) to enable immediate tx relay for fast mempool synchronization. However, considering that this setting only applies to inbound peers and the default test topology looks like this:
```
node0 <--- node1 <---- node2 <--- ... <-- nodeN
```
txs propagate fast only from lower- to higher-numbered nodes (i.e. "left to right" in the above diagram) and take long from higher- to lower-numbered nodes ("right to left") since in the latter direction we only have outbound peers, where the trickle relay is still active. As a consequence, if a tx is submitted from any node other than node0, the mempool synchronization can take quite long.
This PR fixes this by simply adding another connection from node0 to the last node, leading to a ~2-3x speedup (5 runs measured via `time ./test/functional/wallet_groups.py` are shown):
```
master:
0m53.31s real 0m08.22s user 0m05.60s system
0m32.85s real 0m07.44s user 0m04.08s system
0m46.40s real 0m09.18s user 0m04.23s system
0m46.96s real 0m11.10s user 0m05.74s system
0m57.23s real 0m10.53s user 0m05.59s system
PR:
0m19.64s real 0m09.58s user 0m05.50s system
0m18.05s real 0m07.77s user 0m04.03s system
0m18.99s real 0m07.90s user 0m04.25s system
0m17.49s real 0m07.56s user 0m03.92s system
0m18.11s real 0m07.74s user 0m03.88s system
```
Note that in most tests this is not a problem since txs very often originate from node0.
ACKs for top commit:
brunoerg:
utACK ab4efad51b
Tree-SHA512: 12675357e6eb5a18383f2bfe719a184c0790863b37a98749d8e757dd5dc3a36212e16a81f0a192340c11b793eda00db359c7011f46f7c27e3a093af4f5b62147
We'll need a better integration of the hash preimages PSBT fields to
satisfy Miniscript with such challenges from the RPC.
Thanks to Greg Sanders for his examples and suggestions to improve this
test.
Try to solve a script using the Miniscript satisfier if the legacy
solver fails under P2WSH context. Only solve public key and public key
hash challenges for now.
We don't entirely replace the raw solver and especially rule out trying to
solve CHECKMULTISIG-based multisigs with the Miniscript satisfier since
some features, such as the transaction input combiner, rely on the
specific behaviour of the former.
741c215b5f test: remove unused vars in `feature_block` (brunoerg)
Pull request description:
There is no need to assign `self.next_block` to variables if we're not using its return value. Most cases touched here, we're reassigning it right after with the value from `self.update_block`.
Top commit has no ACKs.
Tree-SHA512: 25bbea2a09f38c3a3483fa363f024d2a8edd06a00cccc93cef99e489b9a3485d58bbd6a1ed2dddc00f1cebec7e63aed8ad95701a2645ce20a0db9b69573c20a7
c9ba4f9ecb test: Add test for file system permissions (Hennadii Stepanov)
581f16ef34 Apply default umask in `SetupEnvironment()` (Hennadii Stepanov)
8a6219e543 Remove `-sysperms` option (Hennadii Stepanov)
Pull request description:
On master (1e7564eca8) docs say:
```
$ ./src/bitcoind -help | grep -A 3 sysperms
-sysperms
Create new files with system default permissions, instead of umask 077
(only effective with disabled wallet functionality)
```
Basing on that, one could expect that running `bitcoind` first time will create data directory and `wallets/` subdirectory with safe 0700 permissions.
But that is not the case:
```
$ stat .bitcoin | grep id
Access: (0775/drwxrwxr-x) Uid: ( 1000/ hebasto) Gid: ( 1000/ hebasto)
$ stat .bitcoin/wallets | grep id
Access: (0775/drwxrwxr-x) Uid: ( 1000/ hebasto) Gid: ( 1000/ hebasto)
```
Both directories, in fact, are created with system default permissions.
With this PR:
```
$ stat .bitcoin/wallets | grep id
Access: (0700/drwx------) Uid: ( 1000/ hebasto) Gid: ( 1000/ hebasto)
$ stat .bitcoin/wallets | grep id
Access: (0700/drwx------) Uid: ( 1000/ hebasto) Gid: ( 1000/ hebasto)
```
---
This PR:
- is alternative to bitcoin/bitcoin#13389
- fixesbitcoin/bitcoin#15902
- fixesbitcoin/bitcoin#22595
- closesbitcoin/bitcoin#13371
- reverts bitcoin/bitcoin#4286
Changes in behavior: removed `-sysperms` command-line argument / configure option. The related discussions are here:
- https://github.com/bitcoin/bitcoin/pull/13389#issuecomment-395306690
- https://github.com/bitcoin/bitcoin/pull/13389#issuecomment-539906114
- https://github.com/bitcoin/bitcoin/pull/13389#discussion_r279160472
If users rely on non-default access permissions, they could use `chmod`.
ACKs for top commit:
john-moffett:
ACK c9ba4f9ecb
willcl-ark:
ACK c9ba4f9ecb
Tree-SHA512: 96c745339e6bd0e4d7bf65daf9a721e2e1945b2b0ab74ca0f66576d0dc358b5de8eb8cdb89fe2160f3b19c39d2798bb8b291784316085dc73a27102d3415bd57
b8032293e6 Remove use of snprintf and simplify (John Moffett)
Pull request description:
These are the only remaining uses of `snprintf` in our project, and they can cause unexpected issues -- for example, see https://github.com/bitcoin/bitcoin/issues/27014. Change them to use our `ToString` (which uses a locale-independent version of `std::to_string`) to convert an `int` to `std::string`. Also remove resulting unused parts of `StringContentsSerializer`.
Closes https://github.com/bitcoin/bitcoin/issues/27014
ACKs for top commit:
Sjors:
tACK b8032293e6, fixes#27014.
Tree-SHA512: c903977e654711929decafe8887d0de13b38a340d7082875acc5d41950d834dcfde074e9cabecaf5f9a760f62c34322297b4b156af29761650ef5803b1a54b59
One test case uses snprintf to convert an
int to a string. Change it to use ToString
(which uses a locale-independent version of
std::to_string). Also remove unnecessary
parts of StringContentsSerializer.
The descriptor inference logic would previously always use a dummy
signing provider and would never analyze the witness script of a P2WSH
scriptPubKey.
Note even a valid Miniscript might not always be decodable from Script
without more contextual information (for instance the key preimage for a
pk_h).
6d31900e52 wallet: migrate wallet, exit early if no legacy data exist (furszy)
Pull request description:
The process first creates a backup file then return an error,
without removing the recently created file, when notices that
the db is already running sqlite.
ACKs for top commit:
john-moffett:
ACK 6d31900e52
achow101:
ACK 6d31900e52
ishaanam:
crACK 6d31900e52
Tree-SHA512: 9fb52e80de96e129487ab91bef13647bc4570a782003b1e37940e2a00ca26283fd24ad39bdb63a984ae0a56140b518fd0d74aa2fc59ab04405b2c179b7d3c54a
b530d9605d test: refactor: introduce `replace_in_config` helper (Sebastian Falbesoner)
Pull request description:
Currently two functional tests (p2p_permissions.py and wallet_crosschain.py) include quite similar code for substituting strings in a TestNode's bitcoind configuration file, so refactoring that out to a dedicated helper method seems to make sense (probably other tests could need that too in the future).
ACKs for top commit:
kouloumos:
ACK b530d9605d
Tree-SHA512: 5ca65a2ef3292460e5720d5c6acf7326335001858e8f71ab054560117f9479dbadb1dacd8c9235f67f3fcfd08dbc761b62704f379cbf619bba8804f16a25bc7b
Xoroshiro128++ is a fast non-cryptographic random generator.
Reference implementation is available at https://prng.di.unimi.it/
Co-Authored-By: Pieter Wuille <pieter@wuille.net>
b0fa5989e1 test: Check that orphaned coinbase unconf spend is still abandoned (Andrew Chow)
9addbd7890 wallet: Automatically abandon orphaned coinbases and their children (Andrew Chow)
Pull request description:
When a block is reorged out of the main chain, any descendants of the coinbase will no longer be valid. Currently they are only marked as inactive, which means that our balance calculations will still include them. In order to be excluded from the balance calculation, they need to either be abandoned or conflicted. This PR goes with the abandoned method.
Note that even when they are included in balance calculations, coin selection will not select outputs belonging to these transactions because they are not in the mempool.
Fixes#14148
ACKs for top commit:
furszy:
ACK b0fa5989 with a not-blocking nit.
aureleoules:
reACK b0fa5989e1
ishaanam:
ACK b0fa5989e1
Tree-SHA512: 68f12e7aa8df392d8817dc6ac0becce8dbe83837bfa538f47027e6730e5b2e1b1a090cfcea2dc598398fdb66090e02d321d799f087020d7e1badcf96e598c3ac
39b93649c4 test: add functional test for IBD stalling logic (Martin Zumsande)
0565951f34 p2p: Make block stalling timeout adaptive (Martin Zumsande)
Pull request description:
During IBD, there is the following stalling mechanism if we can't proceed with assigning blocks from a 1024 lookahead window because all of these blocks are either already downloaded or in-flight: We'll mark the peer from which we expect the current block that would allow us to advance our tip (and thereby move the 1024 window ahead) as a possible staller. We then give this peer 2 more seconds to deliver a block (`BLOCK_STALLING_TIMEOUT`) and if it doesn't, disconnect it and assign the critical block we need to another peer.
Now the problem is that this second peer is immediately marked as a potential staller using the same mechanism and given 2 seconds as well - if our own connection is so slow that it simply takes us more than 2 seconds to download this block, that peer will also be disconnected (and so on...), leading to repeated disconnections and no progress in IBD. This has been described in #9213, and I have observed this when doing IBD on slower connections or with Tor - sometimes there would be several minutes without progress, where all we did was disconnect peers and find new ones.
The `2s` stalling timeout was introduced in #4468, when blocks weren't full and before Segwit increased the maximum possible physical size of blocks - so I think it made a lot of sense back then.
But it would be good to revisit this timeout now.
This PR makes the timout adaptive (idea by sipa):
If we disconnect a peer for stalling, we now double the timeout for the next peer (up to a maximum of 64s). If we connect a block, we half it again up to the old value of 2 seconds. That way, peers that are comparatively slower will still get disconnected, but long phases of disconnecting all peers shouldn't happen anymore.
Fixes#9213
ACKs for top commit:
achow101:
ACK 39b93649c4
RandyMcMillan:
Strong Concept ACK 39b93649c4
vasild:
ACK 39b93649c4
naumenkogs:
ACK 39b93649c4
Tree-SHA512: 85bc57093b2fb1d28d7409ed8df5a91543909405907bc129de7c6285d0810dd79bc05219e4d5aefcb55c85512b0ad5bed43a4114a17e46c35b9a3f9a983d5754
8609f24be2 test: refactor: simplify p2p_eviction.py by using MiniWallet (Sebastian Falbesoner)
7aa4b32cd4 test: refactor: simplify p2p_tx_download.py by using MiniWallet (Sebastian Falbesoner)
Pull request description:
Similar to #26892, this PR simplies the functional tests p2p_tx_download.py and p2p_eviction.py by using MiniWallet in order to avoid manual low-level tx creation. For the latter, rather than mining 100 blocks manually, the pre-mined chain of the test framework is used.
These instances were found via `$ git grep signrawtransactionwithkey ./test/functional`. AFAICT, there are no other instances where MiniWallet could replace tx creation trivially.
ACKs for top commit:
MarcoFalke:
review ACK 8609f24be2
Tree-SHA512: dfb0103fe7f0625d125e8e4408baed8bfc1ff579954af17d0ead5277e05f933b2c2d98a0093e8109e947635f1718d5c58d837ab825f26077fac0a40575bd3e6f
fafeddfe0e rpc: Throw more user friendly arg type check error (MarcoFalke)
Pull request description:
The arg type check error doesn't list which arg (position or name) failed. Fix that.
ACKs for top commit:
stickies-v:
ACK fafeddfe0e - although I think the functional test isn't in a logical place (but not blocking)
Tree-SHA512: 17425aa145aab5045940ec74fff28f0e3b2b17ae55f91c4bb5cbcdff0ef13732f8e31621d85964dc2c04333ea37dbe528296ac61be27541384b44e37957555c8
b9d5674541 init: Remove sensitive flag from rpcbind (Andrew Chow)
Pull request description:
`-rpcbind` is currently flagged as a sensitive option which means that its value will be masked when the command line args are written to the debug.log file. However this is not useful as if `rpcbind` is actually activated, the bound IP addresses will be written to the log anyways. The test `feature_config_args.py` did not catch this contradiction as the test node was not started with `rpcallowip` and so `rpcbind` was not acted upon.
This also brings `rpcbind` inline with `bind` as that is not flagged as sensitive either.
ACKs for top commit:
Sjors:
re-utACK b9d5674541
willcl-ark:
ACK b9d5674
theStack:
ACK b9d5674541
Tree-SHA512: 50ab5ad2e18ae70649deb1ac429d404b5f5c41f32a4943b2041480580152df22e72d4aae493379d0b23fcb649ab342376a82119760fbf6dfdcda659ffd3e244a
