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
9141e4395a rpc, docs: Add note for commands that supports only legacy wallets (Yusuf Sahin HAMZA)
Pull request description:
Refs #25363, apparently issue is not updated since over a month, so i decided to put the same `importaddress` note in #25368 to other rpc commands that needs this note.
Note is added for following commands:
- `importprivkey`
- `importpubkey`
- `importwallet`
- `dumpprivkey`
- `dumpwallet`
- `importmulti`
- `addmultisigaddress`
- `sethdseed`
ACKs for top commit:
achow101:
ACK 9141e4395a
Tree-SHA512: f3dc05d26577fd8dbe2bd69cb5c14ffccebacd6010402af44427b3d01be8484895dfcf33d55dfa766eadb7f9f3bae5cc4c2add3ac816a2ac60e8beb5a97527f3
a5986e82dd refactor: Remove CAddressBookData::destdata (Ryan Ofsky)
5938ad0bdb wallet: Add DatabaseBatch::ErasePrefix method (Ryan Ofsky)
Pull request description:
This is cleanup that doesn't change external behavior. Benefits of the cleanup are:
- Removes awkward `StringMap` intermediate representation for wallet address metadata.
- Simplifies `CWallet`, deals with used address and received request serialization in `walletdb.cpp` instead of higher level wallet code
- Adds test coverage and documentation
This PR doesn't change externally observable behavior. Internally, the only change in behavior is that `EraseDestData` deletes rows directly from the database because they are no longer stored in memory. This is more direct and efficient because it uses a single lookup and scan instead of multiple lookups.
Motivation for this cleanup is making changes like #18550, #18192, #13756 easier to reason about and less likely to result in unintended behavior and bugs
---
This PR is a rebased copy of #18608. For some reason that PR is locked and couldn't be reopened or commented on.
This PR is an alternative to #27215 with differences described in https://github.com/bitcoin/bitcoin/pull/27215#pullrequestreview-1329028143
ACKs for top commit:
achow101:
ACK a5986e82dd
furszy:
Code ACK a5986e82
Tree-SHA512: 6bd3e402f1f60263fbd433760bdc29d04175ddaf8307207c4a67d59f6cffa732e176ba57886e02926f7a1615dce0ed9cda36c8cbc6430aa8e5b56934c23f3fe7
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.
The current flow walks-through every block in the active chain until
hits the chain tip or processes 10k blocks, then calls
`lookupFilterRange()` to obtain all the 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.
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
96bf0bca4a test: simplify uint256 (de)serialization routines (Sebastian Falbesoner)
Pull request description:
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).
ACKs for top commit:
MarcoFalke:
lgtm ACK 96bf0bca4a
brunoerg:
crACK 96bf0bca4a
Tree-SHA512: f3031502d61a936147867ad8a0efa841a9bbdd2acf8781653036889a38524f4f1a5c86b1e07157bf2d9663097e7b84be6846678d0883d2a334beafd87e9510f0
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).
10a354f174 test: prevent intermittent failures (Amiti Uttarwar)
Pull request description:
Follow up to #27214 - add an address to the tried table before the new table to make sure a new table collision is not possible.
ACKs for top commit:
mzumsande:
Code review ACK 10a354f174 - the fix is what I suggested [here](https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1169169601) and should make these intermittent failures impossible.
Tree-SHA512: 24099f02e1915395130065af0ef6a2a1893955d222517d156d928765541d9c427da00172a9b5a540163f4d6aae93ca3882e8267eeb35ecc595d42178abc6191c
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
63c0c4ff10 depends: reuse _config_opts for CMake options (Cory Fields)
Pull request description:
Context: I'm [currently experimenting with building our depends with CMake when possible](https://github.com/theuni/bitcoin/commits/depends-cmake) as part of our future transition to CMake. Specifically zmq, libevent, libnatpmp, and miniupnpc all have existing CMake buildsystems. Building them with CMake will allow us to drop several deps that we currently have (autoconf, automake, pkg-config, etc) which would be unfortunate to carry over after the switch-over.
But that's not relevant for this PR. This is just a very simple change that makes the above work easier to experiment with as it [adds a needed feature for CMake packages](5733dc2000 (diff-e6ed342a25092e0a6d0308e0bfd826044578847132cc6726ac4afa2ca767b61aR20)). It's a no-op for the current builds.
---
From commit description:
This will allow us to use the existing machinery for filtering by arch, os, debug/release, etc.
For example, the following becomes possible for libevent when building with CMake:
`$(package)_config_opts_release=-DEVENT__DISABLE_DEBUG_MODE`
Now the define is only set when not building depends with DEBUG=1
ACKs for top commit:
hebasto:
ACK 63c0c4ff10
Tree-SHA512: 17d123219d2f98c017880196966ffebd5d09d3e2db8e443e227eb7e49da46e9d971fbe7fd2b99a324fc367e1bbe0ac3cd15b399bce98dd87fbb144d767e15fe7
faa7144d3c fuzz: re-enable prioritisetransaction & analyzepsbt RPC (MarcoFalke)
Pull request description:
The linked issue seems fixed, so it should be fine to re-enable
ACKs for top commit:
dergoegge:
utACK faa7144d3c
Tree-SHA512: a681c726fceacc27ab5a03d455c7808d33f3cb11fe7d253d455526568af840b29f0c3c1d97c54785ef9277e7891a3aa742ac73ccd3cf115b7606eba50864aaa9
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.
