cfe5aebc79 rpc: add minconf and maxconf options to sendall (ishaanam)
a07a413466 Wallet/RPC: Allow specifying min & max chain depth for inputs used by fund calls (Juan Pablo Civile)
Pull request description:
This PR adds a "minconf" option to `fundrawtransaction`, `walletcreatefundedpsbt`, and `sendall`.
Alternative implementation of #14641Fixes#14542
Edit: This PR now also adds this option to `send`
ACKs for top commit:
achow101:
ACK cfe5aebc79
Xekyo:
ACK cfe5aebc79
furszy:
diff ACK cfe5aebc, only a non-blocking nit.
Tree-SHA512: 836e610926eec3a62308fba88ddbd6a13d8f4dac37352d0309599f893cde9c1df5e9c298fda6e076493068e4d213e4afa7290a9e3bdb5a95a5d507da3f7b59e8
4159ccd031 test: Run feature_bip68_sequence.py with MiniWallet (Miles Liu)
fc0caaf4aa test: Add "include mempool" flag to MiniWallet rescan_utxos (Miles Liu)
d0a909ae54 test: Add "include immature coinbase" flag to MiniWallet get_utxos (Miles Liu)
e5b9127d9e test: Add signs P2TR and RAWSCRIPT to MiniWallet sign_tx (Miles Liu)
Pull request description:
This PR enables one more of the non-wallet functional tests (feature_bip68_sequence.py) to be run even when no wallet is compiled in by using the MiniWallet instead, as proposed in #20078.
ACKs for top commit:
achow101:
ACK 4159ccd031
MarcoFalke:
review ACK 4159ccd031🤸
Tree-SHA512: e891b189381e961c840b45fc30d058363707fd54c1c4bdc3d29623b03309981f1d3ebfac27e6aecf621bdbcd7e31754a3ef7c53f86346f7dd241c137e64c92bd
282019cd3d refactor: add kernel/cs_main.* (fanquake)
Pull request description:
One place to find / include `cs_main`.
No more:
> // Actually declared in validation.cpp; can't include because of circular dependency.
> extern RecursiveMutex cs_main;
Ultimately, no more need to include `validation.h` (which also includes (heavy/boost filled) `txmempool.h`) everywhere for `cs_main`. See #26087 for another example of why that is useful.
ACKs for top commit:
ajtowns:
ACK 282019cd3d
Tree-SHA512: 142835b794873e7a09c3246d6101843ae81ec0c6295e6873130c98a2abfa5f7282748d0f1a37237a779cc71c3bc0a75d03b20313ef5398c83d4814215cbc8287
d6fc1d6a33 test: add coverage for dust mempool policy (`-dustrelayfee` setting) (Sebastian Falbesoner)
8a5dbe2879 test: add `CScript` method for checking for witness program (Sebastian Falbesoner)
Pull request description:
This PR adds missing test coverage for the `-dustrelayfee` setting, which specifies the fee-rate used to define dust. Output scripts for all common types that are treated as standard by default (P2PK, P2(W)PKH, P2(W)SH, P2TR, bare multisig, null data, unknown witness versions v2+) are created and then checked for dust-mempool-policy each via the `testmempoolaccept` RPC: a tx with an output's nValue equal to the dust threshold should be accepted, one with an nValue of just one 1 satoshi below that should be rejected with reason `dust`. This is repeatedly done for a fixed (but obviously somewhat arbitrary) list of different `-dustrelayfee` settings on a single node, including the default and zero (i.e. no dust limit) settings.
Note that the first commit introduces a necessary `CScript` helper method `IsWitnessProgram` (using PascalCase in Python is likely controversial; in this case the style for the already existing method `GetSigOpCount` was followed, which also refers to a method in the core `CScript` class).
Some historical information about dust, contributed by pablomartin4btc:
"The concept of dust was first introduced in https://github.com/bitcoin/bitcoin/pull/2577. This [commit](eb30d1a5b2) from https://github.com/bitcoin/bitcoin/pull/9380 introduced the -dustrelayfee option. Previous to that PR, the dust feerate was whatever -minrelaytxfee was set to."
ACKs for top commit:
LarryRuane:
ACK d6fc1d6a33
glozow:
ACK d6fc1d6a33
kouloumos:
ACK d6fc1d6a33
Tree-SHA512: 35ea2b2497dfb466395af5665bb217f7250aa7cab9dc43539a5658ab69a454e3623ff58fce7489fcc1105b37f8cb4840a93cec658c5df1de611732bc6439ccad
8cbd926a2c test: refactor: simplify p2p_permissions.py by using MiniWallet (Sebastian Falbesoner)
Pull request description:
This PR simplies the functional test p2p_permissions.py by using MiniWallet in order to avoid manual low-level tx creation. Also, rather than mining 100 blocks manually, the pre-mined chain of the test framework is used, which speeds up the test a little (~2-3 seconds faster on my machine).
ACKs for top commit:
MarcoFalke:
ACK 8cbd926a2c
Tree-SHA512: 36cbc2a0f6fb0251c8696cd017163ed30529690736bafd36e80b53007bd02b9030b68fe93b90dc50323526c8b7d8e0abd8f20890d46ff6015d5c632b64a08535
61360e0cf9 test: Remove redundant function call (Kolby ML)
Pull request description:
Removed unnecessary function call and assignment `get_generate_key()` already calls `key_to_p2pkh()` and stores it in the object as p2pkh_addr.
key.p2pkh_addr is already used for most testcases as well, so it is just a redundant call
ACKs for top commit:
MarcoFalke:
ACK 61360e0cf9
Tree-SHA512: d39117310d6630bad7a78c051e683dbfc77b27f6182014557fb900c1adaf5f1e12039c5a6c0ea50e908d4a8688da05f2deae701f12e06086d17dde86ae275ec5
f4a11d7baf gui: bugfix, catch invalid networks combination crash (furszy)
Pull request description:
The app currently crashes if a network is set inside bitcoin.conf and
another one is provided as param.
The reason is an uncaught runtime_error.
ACKs for top commit:
jarolrod:
tACK f4a11d7baf
johnny9:
tACK f4a11d7baf
john-moffett:
ACK f4a11d7baf
pablomartin4btc:
Tested ACK f4a11d7baf.
hebasto:
ACK f4a11d7baf, tested on Ubuntu 22.04 (Qt 5.15.3).
Tree-SHA512: fc5e26ae0a361e37d53d904cc122d07f064f261b309629c6386cb046ab1b3d2c805cbfe0db8ed3e934af52c6cf0ebb0bef9df9117b4330d9b0ea40c76f9270f9
0f883df7a5 build: fix configuring with only bitcoin-util (fanquake)
Pull request description:
Fixes the issue presented in #25037 in a single (easily backportable) diff, with no additional refactoring/changes.
Can be tested with:
```bash
./configure \
--disable-tests \
--disable-bench \
--without-libs \
--without-daemon \
--without-gui \
--disable-fuzz-binary \
--without-utils \
--enable-util-util
```
ACKs for top commit:
TheCharlatan:
tACK 0f883df7a5
hebasto:
ACK 0f883df7a5, tested on Ubuntu 22.04.
Tree-SHA512: 3682712405c360852c4edd90c171e21302154bf8789252c64083974a5c873cf04d97e8721c7916d5b2dafa6acd2b8dc32deecf550e90e03bcbbabbbbf75ce959
eea73d465e test: skip sqlite3 tests if it isn't available (fanquake)
Pull request description:
Fixes#26819. Related too #26873, which adds the missing documentation.
ACKs for top commit:
brunoerg:
ACK eea73d465e
Tree-SHA512: 9bcf5c642695c39a58d56ef8bf4d62601ab0acd4ad71c9400b9d8a2a336a1cd55b69408a02c583d9a67c5de0286364c9f38debd03ec8316725ad69450d980f0d
2022917223 Add secp256k1_selftest call (Pieter Wuille)
3bfca788b0 Remove explicit enabling of default modules (Pieter Wuille)
4462cb0498 Adapt to libsecp256k1 API changes (Pieter Wuille)
9d47e7b71b Squashed 'src/secp256k1/' changes from 44c2452fd3..21ffe4b22a (Pieter Wuille)
Pull request description:
Now that libsecp256k1 has a release (https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2022-December/021271.html), update the subtree to match it.
The changes themselves are not very impactful for Bitcoin Core, but include:
* It's no longer needed to specify whether contexts are for signing or verification or both (all contexts support everything), so make use of that in this PR.
* Verification operations can use the static context now, removing the need for some infrastructure in pubkey.cpp to make sure a context exists.
* Most modules are now enabled by default, so we can drop explicit enabling for them.
* CI improvements (in particular, MSVC and more recent MacOS)
* Introduction of an internal int128 type, which has no effect for GCC/Clang builds, but enables 128-bit multiplication in MSVC, giving a ~20% speedup there (but still slower than GCC/Clang).
* Release process changes (process documentation, changelog, ...).
ACKs for top commit:
Sjors:
ACK 2022917223, but 4462cb0498 could use more eyes on it.
achow101:
ACK 2022917223
jonasnick:
utACK 2022917223
Tree-SHA512: 8a9fe28852abe74abd6f96fef16a94d5a427b1d99bff4caab1699014d24698aab9b966a5364a46ed1001c07a7c1d825154ed4e6557c7decce952b77330a8616b
dc9bad5192 Change dots to an ellipsis and fix capitalization (John Moffett)
9b158ae73f Update to mention restoring wallet via GUI (John Moffett)
Pull request description:
f9783b0f07 Recently added the ability to restore wallets via the GUI, but the current wallet guide says backups must be restored via RPC.
ACKs for top commit:
kouloumos:
ACK dc9bad5192
jarolrod:
re-ACK dc9bad5
hebasto:
re-ACK dc9bad5192
Tree-SHA512: 325a0023ef10c75073b0288f69c99f01b029b0b7b64ae91e7ef72d4ab1fa4da60fe4cd1b4528c1c0d34617122d9aee3cd9cb32aef05a25493fc01e9ec2e6cc10
fa0584eb82 ci: Stop and remove CI container (MarcoFalke)
fa5dccba32 scripted-diff: ci: Rework docker naming (MarcoFalke)
Pull request description:
This should not affect CI runs that have `DANGER_RUN_CI_ON_HOST=1` set, for example `.cirrus.yml`.
However, when running CI locally with podman or docker, the container is stopped and thus deleted when all tests have passed. This feature was requested in https://github.com/bitcoin/bitcoin/pull/26843#issuecomment-1374445512 and can help to reduce used disk space when running several CI tasks subsequently.
ACKs for top commit:
fanquake:
ACK fa0584eb82
hebasto:
ACK fa0584eb82, tested on Ubuntu 22.04.
Tree-SHA512: 8bc1fabd15dad06d9ab3535d2743556137d512332b377b333ab13dbd7c5911b9295549511c6f3836b3acb7f8a39fb67e5f5249e45e1178d16ad4ed0593d05f04
672f7ad747 doc: remove usages of C++11 (fanquake)
Pull request description:
These were new in C++11, and now they are just our standard library.
ACKs for top commit:
jarolrod:
re-ACK 672f7ad747
hebasto:
re-ACK 672f7ad747
Tree-SHA512: 7e3b8b0346ba29b19e6d8536700ca510e2b543cdeecd9e740bba71ea6d0133dd96cdaeaa00f371f8ef85913ff5aaabe12878255f393dac7d354a8b89b58d050a
fa1bf4e705 test: Fix intermittent timeout in p2p_permissions.py (MarcoFalke)
Pull request description:
The sync is based on `bytesrecv_per_msg["verack"]`. However, the bytes are counted before processing the message, so they are not sufficient to ensure the connection is fully up.
ACKs for top commit:
mzumsande:
ACK fa1bf4e705
aureleoules:
ACK fa1bf4e705
Tree-SHA512: eb1ed537032c76a449b1ed5e42ff062e9b8b3c7e11fde2a5b8183ae0d6fbe31dba39e2c758836160cd8157d9ac5cc1f5d1916415861b8d711b7370c88f5e9790
c28d461834 doc: move errant release note to doc/ (fanquake)
Pull request description:
Release note from #26646 should be in doc/.
Top commit has no ACKs.
Tree-SHA512: 741323c0bc526163d65e441c9e677ee3613ed0c55b1880b99fb63b0f3306aeca0cad83dadef05c23def8d0fc6a224956b27ae1bf4b1ff980a1be2d31feef7e3d
94d0c418c5 build: remove already tested headers from AC_CHECK_HEADERS (fanquake)
Pull request description:
These headers are already included in a default set which are checked early during configure.
We already use at least sys/types.h and unistd.h unconditionally in configure.
ACKs for top commit:
TheCharlatan:
Code review ACK 94d0c418c5
Tree-SHA512: d81ce01d46ef1c226b19a4b906e5b622baeee62f04d75eebd02094585ad4712a6c830314227d3a1c03a5f76be5c659b456bb065c175e3cf391e0ece37fd99ead
f9ce0eadf4 For feebump, ignore abandoned descendant spends (John Moffett)
Pull request description:
Closes#26667
To be eligible for fee-bumping, a transaction must not have any of its outputs (eg - change) spent in other unconfirmed transactions in the wallet. This behavior is currently [enforced](9e229a542f/src/wallet/feebumper.cpp (L25-L28)) and [tested](9e229a542f/test/functional/wallet_bumpfee.py (L270-L286)).
However, this check shouldn't apply to spends in abandoned descendant transactions, as explained by #26667.
`CWallet::IsSpent` already carves out an exception for abandoned transactions, so we can just use that.
I've also added a new test to cover this case.
ACKs for top commit:
Sjors:
re-utACK f9ce0eadf4
achow101:
ACK f9ce0eadf4
furszy:
ACK f9ce0ead
Tree-SHA512: 19d957d1cf6747668bb114e27a305027bfca5a9bed2b1d9cc9e1b0bd4666486c7c4b60b045a7fe677eb9734d746f5de76390781fb1e9e0bceb4a46d20acd1749
04528054fc [bench] BlockAssembler with mempool packages (glozow)
6ce265acf4 [test util] lock cs_main before pool.cs in PopulateMempool (glozow)
8791410662 [test util] randomize fee in PopulateMempool (glozow)
cba5934eb6 [miner] allow bypassing TestBlockValidity (glozow)
c058852308 [refactor] parameterize BlockAssembler::Options in PrepareBlock (glozow)
a2de971ba1 [refactor] add helper to apply ArgsManager to BlockAssembler::Options (glozow)
Pull request description:
Performance of block template building matters as miners likely want to be able to start mining on a block with transactions asap after a block is found. We would want to know if a mempool PR accidentally caused, for example, a 100x slowdown. An `AssembleBlock()` bench exists, but it operates on a mempool with 101 transactions, each with 0 ancestors or descendants and with the same fee. Adding a bench with a more complex mempool is useful because (1) it's more realistic (2) updating packages can potentially cause the algorithm to take a long time.
ACKs for top commit:
kevkevinpal:
Tested ACK [0452805](04528054fc)
achow101:
ACK 04528054fc
stickies-v:
ACK 04528054f
Tree-SHA512: 38c138d6a75616651f9b1faf4e3a1cd833437a486f4e84308fbee958e8462bb570582c88f7ba7ab99d80191e97855ac2cf27c43cc21585d3e4b0e227effe2fb5
0f5fc4f656 doc: fix up -netinfo relaytxes help (Jon Atack)
Pull request description:
Addresses https://github.com/bitcoin/bitcoin/pull/26109#discussion_r995502563 by Marco Falke (thanks!)
ACKs for top commit:
mzumsande:
Code Review ACK 0f5fc4f656
Tree-SHA512: d7345d1a94b15c4ec1a2bb0be5c04c472411d90cefb4c16ed524933d2bfc36816bb7519c2e109b2e41ff451b039dd2ddaa6d5db917ad54745332f2a1d8b85570
c467cfffce test: add coverage for `purpose` arg in `listlabels` (brunoerg)
Pull request description:
This PR adds test coverage for `listlabels` command when specifying the `purpose` (send and receive).
dcdfd72861/src/wallet/rpc/addresses.cpp (L698-L704)
ACKs for top commit:
kristapsk:
ACK c467cfffce
Tree-SHA512: 7e7143c1264692f7b22952e7c70dbe9ed3f5dcd2e3b69962a47be9f9c21b3f4a9089ca87962fbc8ff9116e7d2dbeb7f36d6a132c9ac13724a255cfe1b32373a8
264f9ef17f [validation] return MempoolAcceptResult for every tx on PCKG_TX failure (glozow)
dae81e01e8 [refactor] rename variables in AcceptPackage for clarity (glozow)
da484bc738 [doc] release note effective-feerate and effective-includes RPC results (glozow)
5eab397b98 [validation] remove PackageMempoolAcceptResult::m_package_feerate (glozow)
601bac88cb [rpc] return effective-includes in testmempoolaccept and submitpackage (glozow)
1691eaa818 [rpc] return effective-feerate in testmempoolaccept and submitpackage (glozow)
d6c7b78ef2 [validation] return wtxids of other transactions whose fees were used (glozow)
1605886380 [validation] return effective feerate from mempool validation (glozow)
5d35b4a7de [test] package validation quits early due to non-policy, non-missing-inputs failure (glozow)
be2e4d94e5 [validation] when quitting early in AcceptPackage, set package_state and tx result (glozow)
Pull request description:
This PR fixes a bug and improves the mempool accept interface to return information more predictably.
Bug: In package validation, we first try the transactions individually (see doc/policy/packages.md for more explanation) and, if they all failed for missing inputs and policy-related (i.e. fee) reasons, we'll try package validation. Otherwise, we'll just "quit early" since, for example, if a transaction had an invalid signature, adding a child will not help make it valid. Currently, when we quit early, we're not setting the `package_state` to be invalid, so the caller might think it succeeded. Also, we're returning no results - it makes more sense to return the individual transaction failure. Thanks instagibbs for catching https://github.com/bitcoin/bitcoin/pull/25038#discussion_r1013293248!
Also, make the package results interface generally more useful/predictable:
- Always return the feerate at which a transaction was considered for `CheckFeeRate` in `MempoolAcceptResult::m_effective_feerate` when it was successful. This can replace the current `PackageMempoolAcceptResult::m_package_feerate`, which only sometimes exists.
- Always provide an entry for every transaction in `PackageMempoolAcceptResult::m_tx_results` when the error is `PCKG_TX`.
ACKs for top commit:
instagibbs:
reACK 264f9ef17f
achow101:
ACK 264f9ef17f
naumenkogs:
reACK 264f9ef17f
Tree-SHA512: ce7fd9927a80030317cc6157822596e85a540feff5dbf5eea7c62da2eb50c917cdddc9da1e2ff62cc18b98b27d360151811546bd9d498859679a04bbee090837
DOCKER in names is confusingly used as synonym for "image", "container",
and "ci". Fix the confusion by picking the term that fits the context.
-BEGIN VERIFY SCRIPT-
ren() { sed -i "s:$1:$2:g" $( git grep -l "$1" ) ; }
ren DOCKER_PACKAGES CI_BASE_PACKAGES
# This better reflects that they are the common base for all CI
# containers.
ren DOCKER_ID CI_CONTAINER_ID
# This is according to the documentation of "--detach , -d: Run
# container in background and print container ID".
ren DOCKER_NAME_TAG CI_IMAGE_NAME_TAG
# This avoids confusing with CONTAINER_NAME and clarifies that it is an
# image.
ren DOCKER_ADMIN CI_CONTAINER_CAP
# This clarifies that it is a capability added to the container.
ren DOCKER_CI_CMD_PREFIX CI_EXEC_CMD_PREFIX
# This brings it in line with the CI_EXEC naming.
-END VERIFY SCRIPT-
3784009534 wallet: Skip rescanning if wallet is more recent than tip (Andrew Chow)
Pull request description:
If a wallet has key birthdates that are more recent than the currrent chain tip, or a bestblock height higher than the current tip, we should not attempt to rescan as there is nothing to scan for.
Fixes#26655
ACKs for top commit:
ishaanam:
re-utACK 3784009534
w0xlt:
utACK 3784009534
furszy:
Code review ACK 37840095
Tree-SHA512: f0d90b62940d97d50f21e1e01fa6dcb54409fad819cea4283612825c4d93d733df323cd92787fed43956b0a8e386a5bf88218f1f5749c913398667a5c8f54470
65e78bda7c test: Invalid label name coverage (Aurèle Oulès)
552b51e682 refactor: Add sanity checks in LabelFromValue (Aurèle Oulès)
67e7ba8e1a rpc: Sanitize label name in various RPCs (Aurèle Oulès)
Pull request description:
The following RPCs did not sanitize the optional label name:
- importprivkey
- importaddress
- importpubkey
- importmulti
- importdescriptors
- listsinceblock
Thus is was possible to import an address with a label `*` which should not be possible.
The wildcard label is used for backwards compatibility in the `listtransactions` rpc.
I added test coverage for these RPCs.
ACKs for top commit:
ajtowns:
ACK 65e78bda7c
achow101:
ACK 65e78bda7c
furszy:
diff ACK 65e78bd
stickies-v:
re-ACK 65e78bda7c
theStack:
re-ACK 65e78bda7c
Tree-SHA512: ad99f2824d4cfae352166b76da4ca0069b7c2eccf81aaa0654be25bbb3c6e5d6b005d93960f3f4154155f80e12be2d0cebd5529922ae3d2a36ee4eed82440b31
3076f1815d doc: net: fix link to onion address encoding scheme [ONIONADDRESS] (Sebastian Falbesoner)
Pull request description:
Instead of referring to a fixed line number to a file in master (which is obviously always quickly outdated), use a permalink tied to the latest commit.
ACKs for top commit:
vasild:
ACK 3076f1815d
Tree-SHA512: 7070a7e47d683b1539f33daa4c67093a87d6121a84430a3b24afee139a7f2b3cab32fcdf0bc561f8e177b5ba864a98e4491e08dac90cdd4bd2e4e6b8fa7e4b14
Instead of referring to a fixed line number to a file in master (which
is obviously always quickly outdated), use a permalink tied to the
latest commit.
fae885b98f ci: Run one task with all tests on credits (MarcoFalke)
Pull request description:
This should allow to detect any obvious issues in the tests within 10 minutes of opening a pull request, regardless of the current scheduling load on the Cirrus CI community cluster.
ACKs for top commit:
fanquake:
ACK fae885b98f
Tree-SHA512: 384651364648cd38a126f64a5cc650dd396735c05f7d8b5d5c9a8ee7b3b98e36f5dfb3b1c0563ba4d9f554b5e0e82e13aa38e78092f11856a82fc76270fe6f3e
This makes the interface more predictable and useful. The caller
understands one or more transactions failed, and can learn what happened
with each transaction. We already have this information, so we might as
well return it.
It doesn't make sense to do this for other PackageValidationResult
values because:
- PCKG_RESULT_UNSET: this means everything succeeded, so the individual
failures are no longer accurate.
- PCKG_MEMPOOL_ERROR: something went wrong with the mempool logic;
transaction failures might not be meaningful.
- PCKG_POLICY: this means something was wrong with the package as a
whole. The caller should use the PackageValidationState to find the
error, rather than looking at individual MempoolAcceptResults.
This value creates an extremely confusing interface as its existence is
dependent upon implementation details (whether something was submitted
on its own, etc). MempoolAcceptResult::m_effective_feerate is much more
helpful, as it always exists for submitted transactions.
