2f2ab47bf74f4da37aad75a186cb0bb16e8af579 Release notes (Pol Espinasa)
bf194c920cf768d1339d41aef1441a78e2f5fcbe 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 2f2ab47bf74f4da37aad75a186cb0bb16e8af579
ismaelsadeeq:
re-ACK 2f2ab47bf74f4da37aad75a186cb0bb16e8af579
rkrux:
Concept and utACK 2f2ab47bf74f4da37aad75a186cb0bb16e8af579
Tree-SHA512: 0272812cbe5a519737c5d0683acc2072e67559792b4a6472bca8b23426e5ce9e88a3a1eba987feda70a082b8b474b3126893848628d7bf11e1520357b18c8d3e
fa310cc6f499e3e3dd58dc382e04e9db3f02bc3b 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 fa310cc6f499e3e3dd58dc382e04e9db3f02bc3b
Tree-SHA512: e4a58093ab5b9e280c479b845fecb5d228e65519ea3dc2111b393202225fd0feded423e8812452454b6b9348cb37a9c1b01b9d1b1802e9f4aa76b9e56b4b54ef
d190f0facc8379da7610d7161e532d57c6a7eb96 test, contrib: Fix signer/miner command line escaping (Ryan Ofsky)
0d2eefca8bf3cb64e3f2f912ed32118524430967 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 d190f0facc8379da7610d7161e532d57c6a7eb96 🍓
Sjors:
ACK d190f0facc8379da7610d7161e532d57c6a7eb96
vasild:
ACK d190f0facc8379da7610d7161e532d57c6a7eb96
Tree-SHA512: 5a6c0553cd2822585810d827ef1c1772cbf3097d3336daf733f8378dd3da79c00fc3721e50ed0f7455908fbd7a509e9739f9be33f588d6bc1aaa400b9d75c650
387385ba1edf9febdc75d39bd77b35b29714b3d0 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 387385ba1edf9febdc75d39bd77b35b29714b3d0
0xB10C:
had a quick look, lgtm ACK 387385ba1edf9febdc75d39bd77b35b29714b3d0
theStack:
utACK 387385ba1edf9febdc75d39bd77b35b29714b3d0
brunoerg:
code review ACK 387385ba1edf9febdc75d39bd77b35b29714b3d0
i-am-yuvi:
Great! ACK 387385ba1edf9febdc75d39bd77b35b29714b3d0
Tree-SHA512: 741a3d98288c9999f62bcbaa3806716b0519ec9b521e1e6e17aa458392245f6eff886af6cb601c66f2147e0265ff1eae57cea3dcfd67af93bef6dff25b056935
cec14ee47d71d8dd5ca8f90b760d807c3c8933a5 test: switch wallet_crosschain.py to signet (Sjors Provoost)
9c2951541c22c1e2fb9406095487ce8c5c360769 test: drop testnet4 from wallet_crosschain.py (Sjors Provoost)
Pull request description:
It's sufficient to check only one test network, so this PR reverts the addition of testnet4 from #29775.
Testnet3 is deprecated. Instead of moving to testnet4, which might also be deprecated in the future, use signet.
ACKs for top commit:
fjahr:
utACK cec14ee47d71d8dd5ca8f90b760d807c3c8933a5
maflcko:
lgtm ACK cec14ee47d71d8dd5ca8f90b760d807c3c8933a5 🌰
Tree-SHA512: c5aad6e7d251957f090145eac906f7985fddc3e3ba82df7184d72b961f9c856d324a1065ac98323b75501d136bd7b669fcc2565b9e66b0743eb3f3906ef37570
20fe41e9e83d510fd467f5a999d55a614b16ef89 test: avoid disk space warning for non-regtest (Sjors Provoost)
Pull request description:
`feature_config_args.py` incorrectly assumed that its testnet4 node would not log a disk space warning.
But when #31978 increased `m_assumed_blockchain_size` on testnet4 from 1 to 11 GiB, it triggered this bug on my RAM disk, see https://github.com/bitcoin/bitcoin/tree/master/test#speed-up-test-runs-with-a-ram-disk
This PR fixes the issue by using `-prune` which prevents the warning.
ACKs for top commit:
fjahr:
ACK 20fe41e9e83d510fd467f5a999d55a614b16ef89
maflcko:
lgtm ACK 20fe41e9e83d510fd467f5a999d55a614b16ef89
rkrux:
ACK 20fe41e9e83d510fd467f5a999d55a614b16ef89
Tree-SHA512: f4bbb3ede307e06bf097a3cf7a4099eacc9388e33f551e1d0c4c5f53747bfa593a4b22e5d2e713ce6dd8adf91602fade36fbec9cfc2b250a6b1cf09f11bc8473
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.
Pass bitcoin binary command lines from test framework to signet/miner utility
using shell escaping so they are unambigous and don't get mangled if they
contain spaces.
This change is not needed for tests to pass currently, but is a useful change
to avoid CI failures in followup PR
https://github.com/bitcoin/bitcoin/pull/31375 and to avoid other bugs.
21e9d39a3725cd6107b742f0cb97f65b3640201b docs: add release notes for 31603 (brunoerg)
a8b548d75d9a376c9bb66e06bb918c876416d615 test: `getdescriptorinfo`/`importdescriptors` with whitespace in pubkeys (brunoerg)
c7afca3d62cf5d3ea9b98d5a76e4e54cac07bc3c test: descriptor: check whitespace into keys (brunoerg)
cb722a3cea16a04844c83e56fd6deaa1f0dc0a7e descriptor: check whitespace in ParsePubkeyInner (brunoerg)
50856695ef6c02ecbaa0cf448567355b6b86b510 test: fix descriptors in `ismine_tests` (brunoerg)
Pull request description:
Currently, we successfully parse descriptors which contains spaces in the beginning or end of the public/private key within a fragment (e.g. `pk( KEY)`, `pk(KEY )` or `pk( KEY )`). I have noticed that one of the reasons is that the `DecodeBase58` function simply ignore these whitespaces.
This PR changes the `ParsePubkeyInner ` to reject pubkeys that contain a whitespace at the beginning and/or at the end. We will only check the whitespace in some RPCs (e.g. `importdescriptors`), but an already imported descriptor won't be affected by this check, especially because we store descriptors from `ToString`.
For context: https://github.com/brunoerg/bitcoinfuzz/issues/72
ACKs for top commit:
rkrux:
tACK 21e9d39a3725cd6107b742f0cb97f65b3640201b
darosior:
re-ACK 21e9d39a3725cd6107b742f0cb97f65b3640201b
sipa:
utACK 21e9d39a3725cd6107b742f0cb97f65b3640201b
Tree-SHA512: 54f48a89a235517e5cdc29a46dceeb7dabbee93c7616a166288ff3f90131808eb0ece43b0797a11fe827a5f7bd51d65e3e75c16789b0a42020934cabb684cc8f
Add new TestNode.binaries object to manage paths to bitcoin binaries.
Having this 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 to add
new binaries and 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.
Co-authored-by: Sjors Provoost <sjors@sprovoost.nl>
This reverts the changes to test/functional/wallet_crosschain.py
in commit 74a04f9e7ad6a16988149cc3438b9ce13c91cdb9.
It's sufficient to check only one test network. The next commit
will change that network away from testnet3.
52482cb24400f8c44ba9628aaaecb7c04b11beb2 test: Check datadir cleanup after assumeutxo was successful (Fabian Jahr)
Pull request description:
I noticed that the proper datadir cleanup after a successful restart of an assumutxo node does not seem to be covered in our tests. This is added here.
ACKs for top commit:
l0rinc:
utACK 52482cb24400f8c44ba9628aaaecb7c04b11beb2
mabu44:
Re-ACK 52482cb24400f8c44ba9628aaaecb7c04b11beb2
Prabhat1308:
re-ACK [`52482cb`](52482cb244)
TheCharlatan:
Re-ACK 52482cb24400f8c44ba9628aaaecb7c04b11beb2
Tree-SHA512: cc941afeba250050eaccf5112255d961253fec9b2683545454a0d2fbe4d542178394b301d169a9dd79edbf6b5d478d95282727dbb0aca96ee79d4cd1ff80f19b
3301d2cbe8c3b76c97285d75fa59637cb6952d0b qa: Wait for txindex to avoid race condition (Hodlinator)
9bfb0d75ba10591cc6c9620f9fd1ecc0e55e7a48 qa: Remove unnecessary -txindex args (Hodlinator)
7ac281c19cd3d11f316dbbb3308eabf1ad4f26d6 qa: Add missing coverage of corrupt indexes (Hodlinator)
Pull request description:
- Add synchronization in 3 places where if the Transaction Index happens to be slow, we get rare test failures when querying it for transactions (one such case experienced on Windows, prompting investigation).
- Remove unnecessary TxIndex initialization in some tests.
- Add some test coverage where TxIndex aspect could be tested in feature_init.py.
ACKs for top commit:
fjahr:
re-ACK 3301d2cbe8c3b76c97285d75fa59637cb6952d0b
mzumsande:
Code Review ACK 3301d2cbe8c3b76c97285d75fa59637cb6952d0b
furszy:
Code review ACK 3301d2cbe8c3b76c97285d75fa59637cb6952d0b
Prabhat1308:
Concept ACK [`3301d2c`](3301d2cbe8)
Tree-SHA512: 7c2019e38455f344856aaf6b381faafbd88d53dc88d13309deb718c1dcfbee4ccca7c7f1b66917395503a6f94c3b216a007ad432cc8b93d0309db9805f38d602
36b0713edc4655f6e0c291975d6d280fbc89cf2e test: fix intermittent failure in wallet_reorgsrestore.py (furszy)
Pull request description:
In response to #32066 intermittent failure.
Wait until the node's process has fully stopped before starting a new instance of it.
Same behavior as in the [tool_wallet.py](698f86964c/test/functional/tool_wallet.py (L540)) test.
ACKs for top commit:
maflcko:
lgtm ACK 36b0713edc4655f6e0c291975d6d280fbc89cf2e
Chand-ra:
tACK [36b0713](36b0713edc)
Tree-SHA512: 8e01493ef1fb58589479f3e12d7429d02ca75a2183d5f79d3b6a2fbf13334878926274a20857f1b4729afc1d30b65789daed229ce06ba236b91d949b73f45d5a
02942056fd861581503a8a35a06dcf22d4ba1473 test: fix intermittent failure in p2p_orphan_handling.py (Martin Zumsande)
Pull request description:
If the mocktime is bumped before the node has successfully disconnected the peer, the requests for both parents could be spread over two GETDATAS: The first time `GetRequestsToSend` is invoked it would only request one tx from peer2, because the other one would only be available after peer1 was disconnected and its outstanding txrequest cleared.
So two GETDATAs would be sent, which would make the test fail.
Fixes#31700
ACKs for top commit:
maflcko:
lgtm ACK 02942056fd861581503a8a35a06dcf22d4ba1473
instagibbs:
ACK 02942056fd
Tree-SHA512: 769200898345da197d86d673d9506f08f0a64b72a456e7e7c988ac37450d9c54ec65da1c8447c566c8578f7cfccdc5723ea680e636bfbe0b3d38265e5ef57774
fa9cf38ab666b50b0d8e82cb17f9e5a8a613547d scripted-diff: test: Rename send_message to send_without_ping (MarcoFalke)
fa4356717d6efcc41709f62a5116c62519d1a853 test: Prefer send_and_ping over send_message+sync_with_ping (MarcoFalke)
Pull request description:
`send_message` is problematic, because it is easy to forget a `sync_with_ping` (or other `wait_until`), leading to intermittent test failures. (Example: https://github.com/bitcoin/bitcoin/pull/31837#discussion_r1950370246)
There are more uses of `send_and_ping` in the codebase than `send_message`, so in most cases `send_and_ping` is needed anyway.
For the remaining cases, clearly document that no sync happens by renaming `send_message` to `send_without_ping`.
ACKs for top commit:
instagibbs:
ACK fa9cf38ab6
Tree-SHA512: 31caa6568d292ae3d3dda931a94aaa30cc1205ec2ef537a484393eb55687f86c212f1e751ac4a7636610bdf591502a50995dc63bf02f97be9fdc482072160b07
2819c514825577dc8d73690882f25117d412ed9f test: Use rpc_deprecated only for testing deprecation (Fabian Jahr)
Pull request description:
The comment in `functional/rpc_deprecated.py` says "This test should be used to verify correct behaviour of deprecated RPC methods with and without the -deprecatedrpc flags." I think we can get rid of the "with" part since we can assume that every deprecated RPC is already tested in at least one other functional test. (I didn't look but I could verify in our coverage if someone has doubts about that.) In order for this test to continue working, the flag will need to be used there. Otherwise this seems to prescribe copy+pasting a basic test from another file and I don't see a good reason for that.
ACKs for top commit:
maflcko:
lgtm ACK 2819c514825577dc8d73690882f25117d412ed9f
janb84:
re ACK [2819c51](2819c51482)
polespinasa:
reACK 2819c514825577dc8d73690882f25117d412ed9f
Tree-SHA512: 96edfd07be863ad19f99feb27afbc2c3ad53560c93c93eac8de8d766ad8c46e5aa02fd013fec99af794cbe9adca8e459d5b80b454caea7e67b2388003e010bb6
feature_config_args.py incorrectly assumed that its testnet4 node
would not log a disk space warning.
0683b8ebf33386d5c05140df89df10b1853d7c7e increased m_assumed_blockchain_size
on testnet4 from 1 to 11 GiB which triggers this bug on more
systems, e.g. a RAM disk.
Prevent the warning by setting -prune for these nodes.
Fix the same issue in feature_signet.py
Wait until the node's process has fully stopped before starting a new instance.
Since the same code is used in tool_wallet.py, this consolidates the behavior
into a 'kill_process()' function.
send_message only drops the bytes in a buffer and a sync is needed to
avoid intermittent test issues. Change the name of the method to make
this more apparent during review.
-BEGIN VERIFY SCRIPT-
sed -i 's/send_message(/send_without_ping(/g' $( git grep -l 'send_message(' )
-END VERIFY SCRIPT-
36b6f36ac4724cb2c9ed0e25314c3bbf55e4ebb8 build: require sqlite when building the wallet (Sjors Provoost)
Pull request description:
Require that sqlite is available in order to compile the wallet. Removes instances of `USE_SQLITE` since it is no longer possible to not have sqlite available.
The `NO_SQLITE` option is dropped from depends.
This is another step towards dropping the legacy wallet, extracted from #31250.
ACKs for top commit:
m3dwards:
ACK 36b6f36ac4724cb2c9ed0e25314c3bbf55e4ebb8
davidgumberg:
crACK 36b6f36ac4
hebasto:
re-ACK 36b6f36ac4724cb2c9ed0e25314c3bbf55e4ebb8.
Tree-SHA512: 870a0135671c80c4f28602119eb8637a1ed43b51b1673bfe88425782fb62ec6ef0f3d6baf0d5984d6a243779b0f63423fd4c4dc324ef87bffba13d63e05ad793
3c5d1a468199722da620f1f3d8ae3319980a46d5 Remove checkpoints (marcofleon)
632ae47372de90064f61e3e622d8da766d1d12de update comment on MinimumChainWork check (marcofleon)
Pull request description:
The headers presync logic (only downloading headers that lead to a chain with sufficient work, implemented in https://github.com/bitcoin/bitcoin/pull/25717) should be enough to prevent memory DoS using low-work headers. Therefore, we no longer have any use for checkpoints.
All checkpoints and checkpoint logic are removed in a single commit, to make it easy to revert if necessary.
Some previous discussion can be found in https://github.com/bitcoin/bitcoin/pull/25725. The conclusion at the time was that more testing of the presync logic was needed. Now that we have [unit](https://github.com/bitcoin/bitcoin/blob/master/src/test/headers_sync_chainwork_tests.cpp), [functional](https://github.com/bitcoin/bitcoin/blob/master/test/functional/p2p_headers_sync_with_minchainwork.py), and [fuzz](https://github.com/bitcoin/bitcoin/blob/master/src/test/fuzz/p2p_headers_presync.cpp) tests for this logic, it seems safe to move forward with checkpoint removal.
ACKs for top commit:
Sjors:
Code review ACK 3c5d1a468199722da620f1f3d8ae3319980a46d5
instagibbs:
reACK 3c5d1a468199722da620f1f3d8ae3319980a46d5
dergoegge:
ACK 3c5d1a468199722da620f1f3d8ae3319980a46d5
Tree-SHA512: 051a6f9b82cd0262f4d3be4403906812fc6d1be022731fac16bb1c02bca471f31dfc7fc4b834ab2469e8f087265a6d99e84a1d665823cda1b112363a8e8f337d
If we bump the mocktime before the node has successfully disconnected
the peer, the requests for both parents could be spread over
two GETDATAS, which would make the test fail.
e5ff4e416ecc8a51367022eb8a7a291f8cbc0c65 qa: use a clearer and documented amount error in malleated snapshot (Antoine Poinsot)
b34fdb5ade0b48384636f8c7c9673554bf82cedf test: introduce output amount (de)compression routines (Sebastian Falbesoner)
a7911ed101ff5b6b624a207f1526b1c347bf635f test: introduce VARINT (de)serialization routines (Sebastian Falbesoner)
Pull request description:
The `feature_assumeutxo.py` functional test checks various errors with malleated snapshots. Some of these cases are brittle or use confusing and undocumented values. Fix one of those by using a clear, documented and forward-compatible value.
I ran across those when working on an unrelated changeset which affected the snapshot. It took me a while to understand where the seemingly magic byte string was coming from, so i figured it was worth proposing this patch on its own for the sake of making the test more maintainable.
See commit messages for details.
ACKs for top commit:
janb84:
re ACK [e5ff4e4](e5ff4e416e)
theStack:
ACK e5ff4e416ecc8a51367022eb8a7a291f8cbc0c65
fjahr:
Code review ACK e5ff4e416ecc8a51367022eb8a7a291f8cbc0c65
i-am-yuvi:
tACK e5ff4e416ecc8a51367022eb8a7a291f8cbc0c65
Tree-SHA512: 60f022b7176836ce05e8f287b436329d7ca6460f3fcd95f78cd24e07a95a7d4d9cbbb68a117916a113fe451732b09a012d300fe860ff33d61823eca797ceddaf
11f8ab140fe63857f6a93b81021efda8f90ceeda test: wallet, coverage for crash on dup block disconnection during unclean shutdown (Martin Zumsande)
9ef429b6ae65f6ad3e9ac11c2d9c0a6c52beb865 wallet: fix crash on double block disconnection (furszy)
Pull request description:
The wallet crashes if it processes the same block disconnection event twice in a row due
to an incompatible coinbase transaction state.
This happens because `disconnectBlock` provides `TxStateInactive` without the "abandoned"
flag for coinbase transactions to `SyncTransaction`, while `AddToWallet()` internally modifies
it to retain the abandoned state.
The crash flow is as follows:
1) On the first disconnection, the transaction state transitions from "confirmed" to
"inactive," bypassing the state equality check since the provided state differs. Then,
`AddToWallet` internally updates the state to "inactive + abandoned"
2) On the second disconnection, as we provide only the "inactive" state
to `SyncTransaction()`, the state equality assertion fails and crashes the wallet.
Reviewers Note:
The crash can easily be replicated by cherry-picking the test commit in master.
ACKs for top commit:
mzumsande:
Code Review ACK 11f8ab140fe63857f6a93b81021efda8f90ceeda
rkrux:
ACK 11f8ab140fe63857f6a93b81021efda8f90ceeda
pinheadmz:
ACK 11f8ab140fe63857f6a93b81021efda8f90ceeda
Tree-SHA512: 971069bca562f0afb06c34a2516842d01b5cbc2b18ed851392aa3caa3bb7488f4a84a5d017ea334e6361261d3c44aa597cc67a1d4fa16781f85e081f3d1f8771
18e83534ace7aa2d26bc7dfa521b1d591b66edfa wallet: Replace "non-0" with "non-zero" in translatable error message (Hennadii Stepanov)
Pull request description:
Transifex interprets the "-0" substring as a number in translatable strings. Since not all translations preserve "-0," this triggers a corresponding warning. While this warning could be disabled globally, it is more reasonable to adjust the original string instead.
ACKs for top commit:
davidgumberg:
ACK 18e83534ac
l0rinc:
ACK 18e83534ace7aa2d26bc7dfa521b1d591b66edfa
1440000bytes:
ACK 18e83534ac
BrandonOdiwuor:
Code Review ACK 18e83534ace7aa2d26bc7dfa521b1d591b66edfa
laanwj:
Code review ACK 18e83534ace7aa2d26bc7dfa521b1d591b66edfa
Tree-SHA512: 5c38cfc4b352dbbcc8de5fb907cf988a77a7ecded7a90fe0517bfb9e4cd5097bdeb1aa6edf5d9ca37de54d1d7939d5e49533ec93c403db90d9169ad7732e5124
Require that sqlite is available in order to compile the wallet. Removes
instances of USE_SQLITE since it is no longer possible to not have
sqlite available.
The NO_SQLITE option is dropped from depends.
Co-authored-by: Ava Chow <github@achow101.com>
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
e637dc2c01c3b566e6c51c911c5881a8d206c924 refactor: Replace uint256 type with Wtxid in PackageMempoolAcceptResult struct (marcofleon)
a3baead7cb8376e3b09f1726b8c466648d187524 validation: use wtxid instead of txid in CheckEphemeralSpends (marcofleon)
Pull request description:
This PR addresses a small bug in [`AcceptMultipleTransactions`](45719390a1/src/validation.cpp (L1598)) where a txid was being inserted into a map that should only hold wtxids. `CheckEphemeralSpends` has an out parameter on failure that records that the child transaction did not spend the parent's dust. Instead of using the txid of this child, use its wtxid.
The second commit in this PR is a refactor of the `PackageMempoolAcceptResult` struct to use the `Wtxid` type instead of `uint256`. This helps to prevent errors like this in the future.
ACKs for top commit:
instagibbs:
ACK e637dc2c01
glozow:
ACK e637dc2c01c, hooray for type safety
dergoegge:
Code review ACK e637dc2c01c3b566e6c51c911c5881a8d206c924
Tree-SHA512: 17039efbb241b7741e2610be5a6d6f88f4c1cbe22d476931ec99e43f993d259a1a5e9334e1042651aff49edbdf7b9e1c1cd070a28dcba5724be6db842e4ad1e0
59c4930394cafc939eb396224b3d60d01ba0ce37 qa: Enable feature_init.py on Windows (Hodlinator)
Pull request description:
Windows has been skipped since feature_init.py was added in #23289. Possibly due to poorer support on older Python versions, or attempts to use `CTRL_C_EVENT` (which didn't work in my testing either) instead of `CTRL_BREAK_EVENT`.
ACKs for top commit:
maflcko:
lgtm ACK 59c4930394cafc939eb396224b3d60d01ba0ce37
BrandonOdiwuor:
Code Review ACK 59c4930394cafc939eb396224b3d60d01ba0ce37
hebasto:
ACK 59c4930394cafc939eb396224b3d60d01ba0ce37, I have reviewed the code and it looks OK.
Tree-SHA512: 4f3649b41bcba2e8d03b8dcb1a7a6882edafb2c456db4b0768fc86018e9e9ed7171cb3d3c99e74b4ef38a3fcf3ab5d2f1865bbd49d791f1ce0a246806634e1a7
568fcdddaec2cc8decba5a098257f31729cc1caa scripted-diff: Adjust documentation per top-level target output location (Hennadii Stepanov)
026bb226e96919603af829d0b677779a234a0f6e cmake: Set top-level target output locations (Hennadii Stepanov)
Pull request description:
This PR sets the target output locations to the `bin` and `lib` subdirectories within the build tree, creating a directory structure that mirrors that of the installed targets.
This approach is widely adopted by the large projects, such as [LLVM](e146c1867e/lldb/cmake/modules/LLDBStandalone.cmake (L128-L130)):
```cmake
set(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/bin)
set(CMAKE_LIBRARY_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/lib${LLVM_LIBDIR_SUFFIX})
set(CMAKE_ARCHIVE_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/lib${LLVM_LIBDIR_SUFFIX})
```
The `libsecp256k1` project has also recently [adopted](https://github.com/bitcoin-core/secp256k1/pull/1553) this approach.
With this PR, all binaries are conveniently located. For example, run:
```
$ ./build/bin/fuzz
```
instead of:
```
$ ./build/src/test/fuzz/fuzz
```
On Windows, all required DLLs are now located in the same directory as the executables, allowing to run `bitcoin-chainstate.exe` (which loads `bitcoinkernel.dll`) without the need to copy DLLs or modify the `PATH` variable.
The idea was briefly discussed among the build team during the recent CoreDev meeting.
---
**Warning**: This PR changes build locations of newly built executables like `bitcoind` and `test_bitcoin` from `src/` to `bin/` without deleting previously built executables. A clean build is recommended to avoid accidentally running old binaries.
ACKs for top commit:
theStack:
Light re-ACK 568fcdddaec2cc8decba5a098257f31729cc1caa
ryanofsky:
Code review ACK 568fcdddaec2cc8decba5a098257f31729cc1caa. Only change since last review was rebasing. I'm ok with this PR in its current form if other developers are happy with it. I just personally think it is inappropriate to \*silently\* break an everyday developer workflow like `git pull; make bitcoind`. I wouldn't have a problem with this PR if it triggered an explicit error, or if the problem was limited to less common workflows like changing cmake options in an existing build.
TheCharlatan:
Re-ACK 568fcdddaec2cc8decba5a098257f31729cc1caa
theuni:
ACK 568fcdddaec2cc8decba5a098257f31729cc1caa
Tree-SHA512: 1aa5ecd3cd49bd82f1dcc96c8e171d2d19c58aec8dade4bc329df89311f9e50cbf6cf021d004c58a0e1016c375b0fa348ccd52761bcdd179c2d1e61c105e3b9f
fac1dd9dffba1033245c283bc0468e801c14e910 test: Fix authproxy named args debug logging (MarcoFalke)
Pull request description:
In Python the meaning of `args or argsn` is that `argsn` is fully ignored when `args` is a list with at least one element. However, the RPC server accepts mixed positional and named args in the same RPC.
Fix the debug log by always printing both. Also, add a new `_json_dumps` helper to avoid bloated code.
Can be tested via `--tracerpc` on a call that uses named args mixed with positional args.
ACKs for top commit:
i-am-yuvi:
Tested ACK fac1dd9dffba1033245c283bc0468e801c14e910
rkrux:
tACK fac1dd9dffba1033245c283bc0468e801c14e910
musaHaruna:
Tested ACK [fac1dd9](fac1dd9dff)
ryanofsky:
Code review ACK fac1dd9dffba1033245c283bc0468e801c14e910. Thanks for logging fix. This change should have been included in #19762
Tree-SHA512: ff63fbc2564b2c7589e9294baacf4c7a79f10d593776813392510702ca726e3893a29db3ba261f3aee1789a59bb215d7cb10fc85ca1a02632631d3722ddcdfc5
In the assumeutxo functional tests, the final test case with alternated UTxO data tests the error
raised when deserializing a snapshot that contains a coin with an amount not in range (<0 or
>MAX_MONEY).
The current malleation uses an undocumented byte string and offset which makes it hard to maintain.
In addition, the undocumented offset is set surprisingly high (39 bytes is well into the
serialization of the amount which starts at offset 36). Similarly the value is surprisingly small,
presumably one was adjusted for the other. But there is no comment explaining how they were chosen,
why not in a clearer manner and what they are supposed to represent.
Instead replace this seemingly magic value with a clear one, MAX_MONEY + 1, serialize the whole
value for the amount field at the correct offset, and document the whole thing for the next person
around.
Transifex interprets the "-0" substring as a number in translatable
strings. Since not all translations preserve "-0," this triggers a
corresponding warning. While this warning could be disabled globally, it
is more reasonable to adjust the original string instead.
