fa86190e6e rpc: Allow fullrbf fee bump (MarcoFalke)
Pull request description:
The RPCs (psbt)bumpfee, and the GUI, reject fee bumps when BIP 125 signalling is absent in the transaction even when the mempool and other RPCs allow them. Fix the confusion by allowing the fee bump.
This is done after fullrbf is always on (https://github.com/bitcoin/bitcoin/pull/30592)
ACKs for top commit:
1440000bytes:
reACK fa86190e6e
achow101:
ACK fa86190e6e
w0xlt:
ACK fa86190e6e
rkrux:
reACK fa86190e6e
glozow:
ACK fa86190e6e
Tree-SHA512: b2ffe8dcadbe71e9be767a16cf8aa0bf383c2de7aa1aee9438d125f444e24f3f7e4f02ddb28981bd3b8b645b6a24a407b4ad6bb0b21946ae637e78f6386e05bf
e261eb8d50 tests: Add BIP 373 test vectors (Ava Chow)
26370c68d0 rpc: Include MuSig2 fields in decodepsbt (Ava Chow)
ff3d460898 psbt: Implement un/ser of musig2 fields (Ava Chow)
Pull request description:
Implements un/serialization of MuSig2 PSBT fields and prepares PSBT to be able to sign for MuSig2 inputs.
Split from #29675
ACKs for top commit:
fjahr:
re-ACK e261eb8d50
theStack:
re-ACK e261eb8d50
rkrux:
tACK e261eb8d50
Tree-SHA512: bb852ad074978847ac4dc656332025e2d4d1025d4283537b89618c7cadd61a8ecd2eff24779b8a014bc8d7b431125060449768192fa05ad0577f29e3c64b2374
fa6a007b8e fuzz: Avoid integer sanitizer warnings in policy_estimator target (MarcoFalke)
Pull request description:
It seems odd to write a fuzz target to trigger integer sanitizer warnings in `CBlockPolicyEstimator::processBlockTx` and then suppress them. If the scenario can happen in reality, the code should be properly fixed to handle the cases. If not, it seems better to fix the fuzz target to not trigger meaningless traces.
Do that here by keeping track of the current height and limiting mempool entries to at most this entry height.
ACKs for top commit:
brunoerg:
ACK fa6a007b8e
dergoegge:
utACK fa6a007b8e
Tree-SHA512: 2092017dc309fb095fe5d43cfb76efb691795f303d567ee919be2b5cac19a944293636229903dc4d1e8b9fe5daf9dc3058544321eff1735f91f804c3baa36cd0
Also, fix the incorrect documention of the 'replaceable' RPC argument
with respect to sequence number handling. The docs were incorrect
before, so the fix could be extracted, but it seems fine to include here
as well.
babb9f5db6 depends: remove non-native libmultiprocess build (Cory Fields)
5d105fb8c3 depends: Switch libmultiprocess packages to use local git subtree (Ryan Ofsky)
9b35518d2f depends, moveonly: split up int_get_build_id function (Ryan Ofsky)
2d373e2707 lint: Add exclusions for libmultiprocess subtree (Ryan Ofsky)
e88ab394c1 doc: Update documentation to explain libmultiprocess subtree (Ryan Ofsky)
d4bc563982 cmake: Fix clang-tidy "no input files" errors (Ryan Ofsky)
abdf3cb645 cmake: Fix warnings from boost headers (Ryan Ofsky)
8532fcb1c3 cmake: Fix ctest mptest "Unable to find executable" errors (Ryan Ofsky)
d597ab1dee cmake: Support building with libmultiprocess subtree (Ryan Ofsky)
69f0d4adb7 scripted-diff: s/WITH_MULTIPROCESS/ENABLE_IPC/ in cmake (Ryan Ofsky)
a2f28e4be9 Squashed 'src/ipc/libmultiprocess/' content from commit 35944ffd23fa (Ryan Ofsky)
d6244f85c5 depends: Update libmultiprocess library to simplify cmake subtree build (Ryan Ofsky)
Pull request description:
This adds the [libmultiprocess](https://github.com/chaincodelabs/libmultiprocess) library and code generator as a subtree in `src/ipc/libmultiprocess` and allows it to be built with the cmake `-DENABLE_IPC` option, which is disabled by default.
This PR does not entirely remove the depends system [libmultiprocess package](https://github.com/bitcoin/bitcoin/blob/master/depends/packages/native_libmultiprocess.mk) because the package is useful when cross compiling. (A cross-compiling cmake build cannot easily build and run a native code generation tool.) However, it does update the depends package to build from the new git subtree, instead of being downloaded separately from github, so the same sources are used to build both the runtime library and the code generator.
This PR includes the following manual changes (not created automatically with `git subtree add`) which just update the build system and documentation:
- [`d6244f85c509` depends: Update libmultiprocess library to simplify cmake subtree build](d6244f85c5)
- [`69f0d4adb72c` scripted-diff: s/WITH_MULTIPROCESS/ENABLE_IPC/ in cmake](69f0d4adb7)
- [`d597ab1dee6b` cmake: Support building with libmultiprocess subtree](d597ab1dee)
- [`8532fcb1c30d` cmake: Fix ctest mptest "Unable to find executable" errors](8532fcb1c3)
- [`abdf3cb6456f` cmake: Fix warnings from boost headers](abdf3cb645)
- [`d4bc5639829f` cmake: Fix clang-tidy "no input files" errors](d4bc563982)
- [`e88ab394c163` doc: Update documentation to explain libmultiprocess subtree](e88ab394c1)
- [`2d373e27071f` lint: Add exclusions for libmultiprocess subtree](2d373e2707)
- [`9b35518d2f3f` depends, moveonly: split up int_get_build_id function](9b35518d2f)
- [`5d105fb8c3ff` depends: Switch libmultiprocess packages to use local git subtree](5d105fb8c3)
- [`babb9f5db641` depends: remove non-native libmultiprocess build](babb9f5db6)
---
Previous minisketch subtree PR #23114 may be useful for comparison
Instructions for subtree verification can be found:
- https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#subtrees
- https://github.com/bitcoin/bitcoin/tree/master/test/lint#git-subtree-checksh
TL&DR:
```sh
git remote add --fetch libmultiprocess https://github.com/chaincodelabs/libmultiprocess.git
test/lint/git-subtree-check.sh -r src/ipc/libmultiprocess
```
---
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/issues/28722).
ACKs for top commit:
Sjors:
re-ACK babb9f5db6
TheCharlatan:
tACK babb9f5db6
vasild:
ACK babb9f5db6
Tree-SHA512: 43d4eecca5aab63e55c613de935965666eaced327f9fe859a0e9c9b85f7685dc16c5c8d6e03e09ca998628c5d468633f4f743529930b037049abe8e0101e0143
fac978fb21 test: Remove fragile and ancient release 0.17 wallet test (MarcoFalke)
Pull request description:
The test checks that the 0.17 wallet rejects wallet files created in "the future".
This is nice, and good to know. However,
* The 0.17 release is ancient and should be unused outside of tests, especially to load future wallets.
* The test intermittently fails, due to ancient RPC server bugs, that were fixed in the meantime. [1]
* Albeit they are not identical, the 0.18 release is still checked in this test, so any theoretical bug that would be caught by 0.17 is hopefully still caught by 0.18 as well.
So fix all issues by removing the test case.
[1] For example from https://api.cirrus-ci.com/v1/task/6161588714995712/logs/ci.log:
```
190/321 - [1mwallet_backwards_compatibility.py --descriptors[0m failed, Duration: 23 s
[17:21:40.700]
[17:21:40.700] [1mstdout:
[17:21:40.700] [0m2025-04-02T21:21:16.575000Z TestFramework (INFO): PRNG seed is: 5772716217847090743
[17:21:40.700] 2025-04-02T21:21:16.580000Z TestFramework (INFO): Initializing test directory /ci_container_base/ci/scratch/test_runner/test_runner_₿_🏃_20250402_210134/wallet_backwards_compatibility_134
[17:21:40.700] 2025-04-02T21:21:26.378000Z TestFramework (INFO): Test wallet backwards compatibility...
[17:21:40.700] 2025-04-02T21:21:33.191000Z TestFramework (INFO): Testing 0.19 addmultisigaddress case (#18075)
[17:21:40.700] 2025-04-02T21:21:33.637000Z TestFramework (INFO): Test that a wallet made on master can be opened on:
[17:21:40.700] 2025-04-02T21:21:33.637000Z TestFramework (INFO): - 250000
[17:21:40.700] 2025-04-02T21:21:34.055000Z TestFramework (INFO): - 240001
[17:21:40.700] 2025-04-02T21:21:34.435000Z TestFramework (INFO): - 230000
[17:21:40.700] 2025-04-02T21:21:34.858000Z TestFramework (INFO): - 220000
[17:21:40.700] 2025-04-02T21:21:35.614000Z TestFramework (INFO): - 210000
[17:21:40.700] 2025-04-02T21:21:35.707000Z TestFramework (INFO): Test descriptor wallet incompatibility on:
[17:21:40.700] 2025-04-02T21:21:35.707000Z TestFramework (INFO): - 200100
[17:21:40.700] 2025-04-02T21:21:35.878000Z TestFramework (INFO): - 190100
[17:21:40.700] 2025-04-02T21:21:36.021000Z TestFramework (INFO): - 180100
[17:21:40.700] 2025-04-02T21:21:36.319000Z TestFramework (INFO): Test descriptor wallet incompatibility with 0.17
[17:21:40.700] 2025-04-02T21:21:37.328000Z TestFramework (INFO): Test that 0.21 cannot open wallet containing tr() descriptors
[17:21:40.700] 2025-04-02T21:21:37.356000Z TestFramework (INFO): Test that a wallet can upgrade to and downgrade from master, from:
[17:21:40.700] 2025-04-02T21:21:37.361000Z TestFramework (INFO): - 250000
[17:21:40.700] 2025-04-02T21:21:37.665000Z TestFramework (INFO): - 240001
[17:21:40.700] 2025-04-02T21:21:37.970000Z TestFramework (INFO): - 230000
[17:21:40.700] 2025-04-02T21:21:38.439000Z TestFramework (INFO): - 220000
[17:21:40.700] 2025-04-02T21:21:38.793000Z TestFramework (INFO): - 210000
[17:21:40.700] 2025-04-02T21:21:39.470000Z TestFramework (INFO): Stopping nodes
[17:21:40.700]
[17:21:40.700]
[17:21:40.700] [1mstderr:
[17:21:40.700] [0mTraceback (most recent call last):
[17:21:40.700] File "/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/test/functional/wallet_backwards_compatibility.py", line 389, in <module>
[17:21:40.700] BackwardsCompatibilityTest(__file__).main()
[17:21:40.700] File "/ci_container_base/test/functional/test_framework/test_framework.py", line 206, in main
[17:21:40.700] exit_code = self.shutdown()
[17:21:40.700] File "/ci_container_base/test/functional/test_framework/test_framework.py", line 379, in shutdown
[17:21:40.700] self.stop_nodes()
[17:21:40.700] File "/ci_container_base/test/functional/test_framework/test_framework.py", line 643, in stop_nodes
[17:21:40.700] node.stop_node(wait=wait, wait_until_stopped=False)
[17:21:40.700] File "/ci_container_base/test/functional/test_framework/test_node.py", line 397, in stop_node
[17:21:40.700] self.stop()
[17:21:40.700] File "/ci_container_base/test/functional/test_framework/coverage.py", line 50, in __call__
[17:21:40.700] return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
[17:21:40.700] File "/ci_container_base/test/functional/test_framework/authproxy.py", line 132, in __call__
[17:21:40.700] response, status = self._request('POST', self.__url.path, postdata.encode('utf-8'))
[17:21:40.700] File "/ci_container_base/test/functional/test_framework/authproxy.py", line 106, in _request
[17:21:40.700] return self._get_response()
[17:21:40.700] File "/ci_container_base/test/functional/test_framework/authproxy.py", line 169, in _get_response
[17:21:40.700] http_response = self.__conn.getresponse()
[17:21:40.700] File "/usr/lib/python3.10/http/client.py", line 1375, in getresponse
[17:21:40.700] response.begin()
[17:21:40.700] File "/usr/lib/python3.10/http/client.py", line 318, in begin
[17:21:40.700] version, status, reason = self._read_status()
[17:21:40.700] File "/usr/lib/python3.10/http/client.py", line 287, in _read_status
[17:21:40.700] raise RemoteDisconnected("Remote end closed connection without"
[17:21:40.700] http.client.RemoteDisconnected: Remote end closed connection without response
[17:21:40.700] [node 10] Cleaning up leftover process
[17:21:40.700] [node 9] Cleaning up leftover process
[17:21:40.700] [node 8] Cleaning up leftover process
[17:21:40.700] [node 7] Cleaning up leftover process
[17:21:40.700] [node 6] Cleaning up leftover process
[17:21:40.700] [node 5] Cleaning up leftover process
[17:21:40.700] [node 4] Cleaning up leftover process
[17:21:40.700] [node 3] Cleaning up leftover process
[17:21:40.700] [node 2] Cleaning up leftover process
[17:21:40.700] [node 1] Cleaning up leftover process
[17:21:40.700] [node 0] Cleaning up leftover process
ACKs for top commit:
laanwj:
Code review ACK fac978fb21
janb84:
Re ACK [fac978f](fac978fb21)
pablomartin4btc:
re ACK fac978fb21
BrandonOdiwuor:
Code Review ACK fac978fb21
Tree-SHA512: 13acdfc6be4293a0ff45ae20b26ba60636e130097da380b7b51716faaa950320462399bef55e74b3cedc82944586dcc1bfd078babb96edb03c4efdb8f40af5a4
459807d566 test: remove strict restrictions on rpc_deprecated (Pol Espinasa)
Pull request description:
Removed the wallet restrictions for `rpc_deprecated.py` and added specific test case for the current deprecated rpc.
`skip_test_if_missing_module` will skip the whole test when the wallet is missing, even if a part of the test is non-wallet related. This PR ensures that other tests not related to wallet can be ran and only this specific test will be skipped if there's no wallet
For more context check https://github.com/bitcoin/bitcoin/pull/31278#discussion_r2011661090
ACKs for top commit:
maflcko:
lgtm ACK 459807d566
rkrux:
ACK 459807d
Tree-SHA512: 922b0fafe8fb5bd88a677ce8be5c3fe2fdd4d0aadcd32cc11738a714cd6f765f07e7e7158c829f8338db0d46a15c030437a1ea09a3187c072bebebb4ca53ad85
f974359e21 test: Add encodable PUSHDATA1 examples to feature_taproot (Greg Sanders)
Pull request description:
Inspired by discussion in https://github.com/bitcoin/bitcoin/pull/31640#issuecomment-2743492906 I made an example adding coverage I think is missing, with some extra commentary that might help future contributors (including myself when I forget how it works again).
Open for suggestions how we can make it more welcoming beyond this.
cc darosior EthanHeilman sipa
ACKs for top commit:
janb84:
Re-ACK [f974359](f974359e21)
rkrux:
ACK f974359e21
Tree-SHA512: 7544d41c39c13d245a8a33522e53f22b4dd7593c069631978303e5a349cd12cf9d45bed648c391618c4732831232c4b82b8de2bf6cba5bf5e1232501db926122
c5a7ffd1e8 preserve llvm profile env (Prabhat Verma)
Pull request description:
While generating `profraw` for fuzz tests using steps in [PR 32206](https://github.com/bitcoin/bitcoin/pull/32206) , the profraw was not being built at the desired location and only one `default.profraw` was being created which was being overwritten for multiple fuzz targets. This PR fixes that.
ACKs for top commit:
maflcko:
lgtm ACK c5a7ffd1e8
mabu44:
ACK c5a7ffd1e8
Tree-SHA512: 11f74caa8cba6f841aa899a5e294f658aed1b6a3d4cf68992609ea99fadb4a092b2350ffacea5c2d5eb377eb10082de018f27a1d6486a72460cb3905aaa15664
Without this change linter produces errors about:
- Use of std::filesystem the libmultiprocess example program.
- Use of locale-dependent functions in example program, in the build time code
generator, and in the runtime library for debug logging.
- Include guards not beginning with BITCOIN_
Removed the wallet restrictions for rpc_deprecated.py and added specific test case for the current deprecated rpc.
skip_test_if_missing_module will skip the whole test when the wallet is missing, even if a part of the test is non-wallet related.
7bb83f6718 test: create assert_not_equal util and add to where imports are needed (kevkevin)
Pull request description:
In the functional tests there are lots of cases where we assert != which we now swap with assert_not_equal to be more readable
This is motivated/uses logic from this PR which was closed https://github.com/bitcoin/bitcoin/pull/28528
This partially helps https://github.com/bitcoin/bitcoin/issues/23119
I've broken it up to just `assert_not_equal` to keep the PR smaller as suggested in https://github.com/bitcoin/bitcoin/pull/28528#issuecomment-1959945805
I can create follow up PR's if this is wanted
ACKs for top commit:
hodlinator:
re-ACK 7bb83f6718
ryanofsky:
Code review ACK 7bb83f6718. Only change since last review is fixing error message formatting and passing it as a keyword argument
janb84:
Re-ACK [7bb83f6](7bb83f6718)
Tree-SHA512: de09f41a690033a5b61e6f861d3bd69a32b889d6655a28fbc0d5cfac9f7ec9c642432967d33913970882b4cfdd47bdd377d0ddc44e25976cbaa49f7f9d8f7b10
aa7a898c23 doc: use testnet4 in developer docs (Sjors Provoost)
6c217d22fd test: use testnet4 in argsman test (Sjors Provoost)
7c200ece80 test: use testnet4 in key_io_valid.json (Sjors Provoost)
d424bd5941 test: drop unused testnet3 magic bytes (Sjors Provoost)
8cfc09fafe test: cover testnet4 magic in assumeutxo.py (Sjors Provoost)
4281e3603a zmq: use testnet4 in zmq_sub.py example (Sjors Provoost)
Pull request description:
In preparation for dropping testnet3 entirely in #31974 this PR migrates a few things to testnet4:
* the ZMQ examples
* developer docs
* various unit tests
* the snapshot magic byte check in `feature_assumeutxo.py`
It drops `testnet3` from `MAGIC_BYTES` in the test framework, since no test uses it.
ACKs for top commit:
fjahr:
re-ACK aa7a898c23
maflcko:
lgtm ACK aa7a898c23🔊
hodlinator:
re-ACK aa7a898c23
Tree-SHA512: 235f74273234e8fb2aedf0017dea5c16bb9813ec7a1f89a51abe85691f09830a5ead834115d7db0936e12e55a40bc81888856a8002fe507c1474407e77f8b9fb
In the functional tests there are lots of cases where we assert != which
this new util will replace, we also are adding the imports and the new assertion
d065208f0f test: get rid of redundant TODO tag (Chandra Pratap)
Pull request description:
The `FEE` parameter in `test/functional/feature_dbcrash.py::generate_small_transaction()` is not a fee rate, but an absolute fee. Hence, it doesn't make sense to replace it with node relay based fee calculation. Get rid of the TODO comment suggesting otherwise.
ACKs for top commit:
maflcko:
lgtm ACK d065208f0f
Tree-SHA512: f2b7f51ffb23de8e14ca071edd731410176a20750115a65db0ae67714389e03ffe1593ce88368e96d211329bd93c772f665de7c3a59b932681bc5b80db908d9f
ca55613fd1 test: Add functional test for bitcoin-chainstate (TheCharlatan)
3f9c716e7f test: Fix docstring for cmake migration (TheCharlatan)
Pull request description:
While the `bitcoin-chainstate` utility is not shipped in a release, it is the only current utility directly using the bitcoin kernel library. Adding a simple test for it is useful for checking that the library is actually usable. The test is also useful in future to demonstrate that the `bitcoin-chainstate` binary using the API for the kernel library introduced in #30595 actually works and offers similar features.
ACKs for top commit:
laanwj:
Code review ACK ca55613fd1
maflcko:
ACK ca55613fd1🎭
kevkevinpal:
ACK ca55613fd1
Tree-SHA512: 282627f5fac868a84aab9962ef2cbd3a8d3941d9f9dc2a3f26db1e7706ffa8051637ab5f8372676800e426e077ca40449a9e3e42a003048472339d81ed81bca8
a82829f37e test: simplify (w)txid checks by avoiding .calc_sha256 calls (Sebastian Falbesoner)
346a099fc1 test: avoid unneeded hash -> uint256 -> hash roundtrips (Sebastian Falbesoner)
Pull request description:
In the functional test framework we currently have a strong tendency to treat and store identifiers that result from hash functions (e.g. (w)txids, block hashes) as integers, which seems an unnatural and confusing choice. Hashes are just pseudo-random sequences of bytes, and there is usually no need to apply integer operations on them; the only exceptions I could think of is PoW-verification of block hashes with the less-than (`<`) operator, or interpreting the byte-string as scalar in the EC-context for e.g. key derivation.
I'd hence argue that most uses of `ser_uint256`/`uint256_from_str` and txid conversions via `int(txid/blockhash, 16)` are potential code smells and should be reduced to a minimum long-term if possible. This PR is a first step into this direction, intentionally kept small with (what I think) uncontroversial changes for demonstration purposes, to check out if other contributors are interested in this. A next step could be to change the classes of primitives (CTransaction, CBlock etc.) and network messages (msg_) to store hash results as actual bytes (maybe in a class wrapping the bytes that offers conversion from/to human-readable strings [1], for easier interaction with RPC calls and debug outputs) rather than ints. But that would of course need larger, potentially more controversial changes, and its questionable if its really worth the effort.
[1] unfortunately, txids and block hashes are shown to user in reverse byte order, so e.g. a txid_bytes->txid_str conversion is not just a simple `txid_bytes.hex()`, but a `txid_bytes[::-1].hex()`
ACKs for top commit:
maflcko:
review ACK a82829f37e🐘
rkrux:
Concept and utACK a82829f37e
ryanofsky:
Code review ACK a82829f37e. Nice changes, and sorry about the false bug report
Tree-SHA512: bb0465802d743a495207800f922b65f49ed0d20552f95bb0bee764944664092aad74812e29df6e01ef40bcb8f9bc6c84c7e9cbbe6f008ee1a14d94ed88e698b4
248fdd88dc test: accept unordered tracepoints in... (willcl-ark)
Pull request description:
We have encountered an instance where the tracepoints were not collected in the same order they were fired (#31951).
Tracepoint ordering is not guaranteed in userspace for a number of reasons.
As this test does not require a strict collection/processing order collect `expected` and `actual` events into dicts and compare them.
This will gracefully handle both the number of events, and out-of-order events should they reoccur in the future.
Fixes: #31951
ACKs for top commit:
0xB10C:
re-ACK 248fdd88dc
laanwj:
Code review ACK 248fdd88dc
Tree-SHA512: 78d1aa936194d386d919ed26133aac3af5fc6d3d0b1fe1e767288d9e6226e2c701d640e71e994a63ccd48344bd2a0db508cb353cdd5ce1f644cd6f7313654623
9f35d4d070 test: fix intermittent timeout in p2p_ibd_stalling.py (Martin Zumsande)
Pull request description:
After sending the headers message add a sync, so that we wait until the header message from the previous peer has been received before connecting additional peers.
In the failed NetBSD run linked in #32090, the second node managed to complete the handshake and send its own headers message before the message from the first node was received.
Fixes#32090
ACKs for top commit:
maflcko:
lgtm ACK 9f35d4d070
Tree-SHA512: 30a98c6ec04f819c892ab5ce76a309df81b3a4644be021f938eefddbd00ec4141e055c3cf735c9e04ce2fe9e950470a99d54cb6be43230110348a802a6a6c252
The 'FEE' parameter in test/functional/feature_dbcrash.py::
generate_small_transaction() is not a fee rate, but an
absolute fee. Hence, it doesn't make sense to replace it
with node relay based fee calculation. Get rid of the TODO
comment suggesting otherwise.
...interface_usdt_utxocache.py
We have encountered an instance where the tracepoints were not collected
in the same order they were fired (#31951).
Tracepoint ordering is not guaranteed in userspace for a number of
reasons.
As this test does not require a strict collection/processing order
collect `expected` and `actual` events into dicts and compare them.
This will gracefully handle both the number of events, and out-of-order
events should they reoccur in the future.
226d81f8b7 mining: drop unused -nFees and sigops from CBlockTemplate (Sjors Provoost)
53ad845fb9 test: check fees and sigops in getblocktemplate (Sjors Provoost)
Pull request description:
For the coinbase `vTxFees` used a dummy value of -nFees.
Similarly the first `vTxSigOpsCost` entry was calculated from
the dummy coinbase transaction.
This was introduced in #2115, but the values were never returned by the RPC or used in a test.
Drop 'm and add code comments to prevent confusion.
This PR also adds test coverage for the `fees` and `sigops` fields in `getblocktemplate`, so it closes#32053.
ACKs for top commit:
ismaelsadeeq:
re-ACK 226d81f8b7
ryanofsky:
Code review ACK 226d81f8b7. New test was added since last review, which seems very cleanly written and fixes some missing coverage.
glozow:
ACK 226d81f8b7
Tree-SHA512: 79c534e6bc4810d29114b04dd6db798877732cb473e773bf3cc28f83d14ee3982392587bd0baa39857bd53a79eae3b730d7a7029b08a9b6c3b5c51f86657ca5d
Replace test_rpcwhitelistdefault_0_no_permissions and
test_rpcwhitelistdefault_1_no_permissions with a single
test_rpcwhitelistdefault_permissions function.
2f2ab47bf7 Release notes (Pol Espinasa)
bf194c920c wallet, rpc: deprecate settxfee and paytxfee (Pol Espinasa)
Pull request description:
**Summary**
This PR deprecates the settxfee RPC and paytxfee setting, marking it for removal in Bitcoin Core 31.0.
**Motivation**
The PR was initially motivated by https://github.com/bitcoin/bitcoin/issues/31088. The intention was to create a new function `settxfeerate` to allow users to set a static fee rate in `sat/vB` instead of `btc/kvB`.
The `settxfee` RPC allows users to set a static fee rate for all transactions created by the wallet. However, in a dynamic fee environment, this can lead to poor fee choices, either overpaying when the mempool is empty or underpaying when congestion is high. The preferred approach is to rely on fee estimation, which is designed to adapt to network conditions, and is the one by default. Same argument apply for `paytxfee` setting.
During discussion the consensus was that static fee settings are a footgun and that users should instead specify the fee rate per transaction if they don't want to rely on the fee estimation. Given this, rather than introducing a `settxfeerate` alternative, this PR goes towards removing `settxfee` and `paytxfee` entirely.
**Key Changes**
`settxfee` and `paytxfee` is now deprecated and will be removed in Bitcoin Core 31.0.
Users should rely on fee estimation or explicitly specify a fee rate when constructing transactions.
**Impact on Users**
If users currently use settxfee or paytxfee, they should transition to specifying fees per transaction.
No immediate breakage in 30.0 (must use `-deprecatedrpc=settxfee`), but `settxfee` and `paytxfee` will be removed in 31.0.
**Alternative Approaches Considered**
A settxfeerate alternative (using sat/vB) was initially proposed but ultimately rejected in favor of deprecating static fee setting entirely.
**Notes for removal**
- When removing paytxfee we should also update txconfirmtarget startup option help text.
- Get back the comment from `rpc_deprecated.py` test. [+info](https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1998876768)
ACKs for top commit:
fjahr:
re-ACK 2f2ab47bf7
ismaelsadeeq:
re-ACK 2f2ab47bf7
rkrux:
Concept and utACK 2f2ab47bf7
Tree-SHA512: 0272812cbe5a519737c5d0683acc2072e67559792b4a6472bca8b23426e5ce9e88a3a1eba987feda70a082b8b474b3126893848628d7bf11e1520357b18c8d3e
fa310cc6f4 test: Fix intermittent issue in p2p_orphan_handling.py (MarcoFalke)
Pull request description:
The test may fail intermittently when the `net` thread is lagging while calling `DeleteNode`. This may result in a split `getdata`, meaning that `peer2.wait_for_parent_requests([int(parent_peekaboo_AB["txid"], 16), int(parent_missing["txid"], 16)])` fails.
Fix it by adding a sync on the `net` thread.
Fixes#31700
ACKs for top commit:
mzumsande:
Code Review ACK fa310cc6f4
Tree-SHA512: e4a58093ab5b9e280c479b845fecb5d228e65519ea3dc2111b393202225fd0feded423e8812452454b6b9348cb37a9c1b01b9d1b1802e9f4aa76b9e56b4b54ef
d190f0facc test, contrib: Fix signer/miner command line escaping (Ryan Ofsky)
0d2eefca8b test, refactor: Add TestNode.binaries to hold binary paths (Ryan Ofsky)
Pull request description:
Add new `TestNode.binaries` object to manage paths to bitcoin binaries.
The `binaries` object makes it possible for the test framework to exercise the bitcoin wrapper executable introduced in https://github.com/bitcoin/bitcoin/pull/31375 and also makes it easier in general to add new binaries, and new options and environment variables controlling how they are invoked, because logic for invoking them that was previously spread out is now consolidated in one place.
These changes were originally part of #31375 but made that PR harder to review because they were unrelated to the other changes there. If this PR can get merged first, python changes in #31375 will be simple, and the test framework changes here should also get a higher quality review.
ACKs for top commit:
maflcko:
re-review-ACK d190f0facc🍓
Sjors:
ACK d190f0facc
vasild:
ACK d190f0facc
Tree-SHA512: 5a6c0553cd2822585810d827ef1c1772cbf3097d3336daf733f8378dd3da79c00fc3721e50ed0f7455908fbd7a509e9739f9be33f588d6bc1aaa400b9d75c650
387385ba1e test: replace assert with assert_equal and assert_greater_than (Chandra Pratap)
Pull request description:
In `test/functional/interface_usdt_net.py`, `assert_equal` is already used to check for equality between objects. Replace `assert.*==` with `assert_equal` and `assert.*>` with `assert_greater_than` to further easify debugging.
Relevant issue: #23119
ACKs for top commit:
maflcko:
lgtm ACK 387385ba1e
0xB10C:
had a quick look, lgtm ACK 387385ba1e
theStack:
utACK 387385ba1e
brunoerg:
code review ACK 387385ba1e
i-am-yuvi:
Great! ACK 387385ba1e
Tree-SHA512: 741a3d98288c9999f62bcbaa3806716b0519ec9b521e1e6e17aa458392245f6eff886af6cb601c66f2147e0265ff1eae57cea3dcfd67af93bef6dff25b056935
