eeee55f928 rpc: Fix invalid bech32 handling (MarcoFalke)
Pull request description:
Currently the handling of invalid bech32(m) addresses over RPC has many issues:
* No error for invalid addresses is reported, leading to internal bugs via `CHECK_NONFATAL`, see https://github.com/bitcoin/bitcoin/issues/27723
* The error messages use "data size" (the meaning of which is unclear to the user, because the witness program data and bech32 section data are related but different) when they mean "program size"
Fix all issues. Also, use the BIP 173 and BIP 350 test vectors.
ACKs for top commit:
achow101:
ACK eeee55f928
brunoerg:
crACK eeee55f928
Tree-SHA512: c8639ee49e2a54b740b72d66bc4a40352dd553a6e3220dea9f94e48e33124f21f597a2817cb405d0a4c88d21df1013c0a4877a01370a2d326aa2cff1f9c381a8
d7f359b35e Add tests for parallel compact block downloads (Greg Sanders)
03423f8bd1 Support up to 3 parallel compact block txn fetchings (Greg Sanders)
13f9b20b4c Only request full blocks from the peer we thought had the block in-flight (Greg Sanders)
cce96182ba Convert mapBlocksInFlight to a multimap (Greg Sanders)
a90595478d Remove nBlocksInFlight (Greg Sanders)
86cff8bf18 alias BlockDownloadMap for mapBlocksInFlight (Greg Sanders)
Pull request description:
This is an attempt at mitigating https://github.com/bitcoin/bitcoin/issues/25258 , which is a revival of https://github.com/bitcoin/bitcoin/pull/10984, which is a revival of https://github.com/bitcoin/bitcoin/pull/9447.
This PR attempts to mitigate a single case, where high bandwidth peers can bail us out of a flakey
peer not completing blocks for us. We allow up to 2 additional getblocktxns requests per unique block.
This would hopefully allow the chance for an honest high bandwidth peer to hand us the transactions
even if the first in flight peer stalls out.
In contrast to previous effort:
1) it will not help if subsequent peers send block headers only, so only high-bandwidth peers this time. See: https://github.com/bitcoin/bitcoin/pull/10984/files#diff-6875de769e90cec84d2e8a9c1b962cdbcda44d870d42e4215827e599e11e90e3R1411
2) `MAX_GETBLOCKTXN_TXN_AFTER_FIRST_IN_FLIGHT` is removed, in favor of aiding recovery during turbulent mempools
3) We require one of the 3 block fetching slots to be an outbound peer. This can be the original offering peer, or subsequent compact blocks given by high bandwidth peers.
ACKs for top commit:
sdaftuar:
ACK d7f359b35e
mzumsande:
Code Review ACK d7f359b35e
Tree-SHA512: 54980eac179e30f12a0bd49df147b2c3d63cd8f9401abb23c7baf02f76eeb59f2cfaaa155227990d0d39384de9fa38663f88774e891600a3837ae927f04f0db3
272eb55616 test: fix `include_immature_coinbase` logic in `get_utxos` (brunoerg)
a951c34f17 test: fix `interface_usdt_mempool` by mining a block after each test (brunoerg)
1557bf1196 test: fix mature utxos addition to wallet in `mempool_package_limits` (brunoerg)
60ced9007d test: fix intermittent issue in `feature_bip68_sequence` (brunoerg)
Pull request description:
Fixes#27129
To avoid `bad-txns-premature-spend-of-coinbase` error,
when getting a utxo (using `get_utxo`) to create a new
transaction `get_utxo` shouldn't return (if possible)
by default immature coinbase.
ACKs for top commit:
achow101:
ACK 272eb55616
pinheadmz:
re-ACK 272eb55616
Tree-SHA512: eae821c7833bf084d8b907c94876ed010a7925d2177c3013a0c61b69d9571df006da83397a19487d93b0d1fa415951152f0b8ad0de2a55d86c39f6917934f050
1bce12acd3 test: add test for `descriptorprocesspsbt` RPC (ishaanam)
fb2a3a70e8 rpc: add descriptorprocesspsbt rpc (ishaanam)
Pull request description:
This PR implements an RPC called `descriptorprocesspsbt`. This RPC is based off of `walletprocesspsbt`, but instead of interacting with the wallet to update, sign and finalize a psbt, it instead accepts an array of output descriptors and uses that information along with information from the mempool, txindex, and the utxo set to do so. `utxoupdatepsbt` also updates a psbt in this manner, but doesn't sign or finalize it. Because of this overlap, a helper function that is added in this PR is called by both `utxoupdatepsbt` and `descriptorprocesspsbt`. Whether or not the helper function signs a psbt is dictated by if the HidingSigningProvider passed to it contains any private information. There is also a test added in this PR for this new RPC that uses p2wsh, p2wpkh, and legacy outputs.
Edit: see https://github.com/bitcoin/bitcoin/pull/25796#issuecomment-1228830963
ACKs for top commit:
achow101:
re-ACK 1bce12acd3
instagibbs:
reACK 1bce12acd3
Tree-SHA512: e1d0334739943e71f2ee68b4db7637ebe725da62e7aa4be071f71c7196d2a5970a31ece96d91e372d34454cde8509e95ab0eebd2c8edb94f7d5a781a84f8fc5d
To avoid `bad-txns-premature-spend-of-coinbase` error,
when getting a utxo (using `get_utxo`) to create a new
transaction `get_utxo` shouldn't return by default
immature coinbase.
fa4c16b186 test: Add test to check tx in the last block can be downloaded (MarcoFalke)
fadc8490ab test: Split up test_notfound_on_unannounced_tx test case (MarcoFalke)
Pull request description:
If a peer received an `inv` about a transaction, which was included in a block before receiving the corresponding `getdata`, it can be beneficial to send this transaction to the peer to aid compact block relay.
Add a test for this to avoid breaking it in the future.
ACKs for top commit:
sdaftuar:
ACK fa4c16b186
instagibbs:
ACK fa4c16b186
Tree-SHA512: 1ec16dcc216dd29c849928e753158d45c409612e9ac528db16e5af465bb5b69cd42241ac6f67e5a4583b8e558eeba49fa0a0889a4247b3fb0053b2758eec9490
faf4315c88 test: Return dict in MiniWallet::send_to (MarcoFalke)
Pull request description:
Returning a tuple has many issues:
* If only one value is needed, it can not be indexed by name
* If another value is added to the return value, all call sites need to be updated
Bite the bullet now and update all call sites to fix the above issues.
ACKs for top commit:
brunoerg:
crACK faf4315c88
theStack:
Code-review ACK faf4315c88
stickies-v:
Code review ACK faf4315c88
Tree-SHA512: 8ce1aca237df21f04b3990d0e5fcb49cc408fe6404399d3769a64eae1b5218941157d9785fce1bd9e45140cf70e06c3aa42646ee8f7b57855beb784fc3ef0261
This is achieved by letting the index sync thread wait until
reindex-chainstate is finished.
This also disables the pruning check when reindexing the chainstate (which is
incompatible with prune mode) because there would be no chain at this point
in init.
The index sync code has logic to go back the chain to the forking point, while
also updating index-specific state, which is necessary to prevent
possible corruption of the coinstatsindex.
Also add a test for this (a reorg happens while the index is deactivated)
that would not pass before this change.
fe49f06c0e doc: clarify PR 26076 release note (Sjors Provoost)
bd13dc2f46 Switch hardened derivation marker to h in descriptors (Sjors Provoost)
Pull request description:
This makes it easier to handle descriptor strings manually, especially when importing from another Bitcoin Core wallet.
For example the `importdescriptors` RPC call is easiest to use `h` as the marker: `'["desc": ".../0h/..."]'`, avoiding the need for escape characters. With this change `listdescriptors` will use `h`, so you can copy-paste the result, without having to add escape characters or switch `'` to 'h' manually.
Both markers can still be parsed.
The `hdkeypath` field in `getaddressinfo` is also impacted by this change, except for legacy wallets. The latter is to prevent accidentally breaking ancient software that uses our legacy wallet.
See discussion in #15740
ACKs for top commit:
achow101:
ACK fe49f06c0e
darosior:
re-ACK fe49f06c0e
Tree-SHA512: f78bc873b24a6f7a2bf38f5dd58f2b723e35e6b10e4d65c36ec300e2d362d475eeca6e5afa04b3037ab4bee0bf8ebc93ea5fc18102a2111d3d88fc873c08dc89
f6d7636be4 test: Treat `bitcoin-wallet` binary in the same way as others (Hennadii Stepanov)
dda961cec5 test, refactor: Add `set_binary_paths` function (Hennadii Stepanov)
Pull request description:
This PR makes the `bitcoin-wallet` binary path customizable in the same way how it can be done now with other ones, including `bitcoind`, `bitcoin-cli` and `bitcoin-util`.
ACKs for top commit:
stickies-v:
re-ACK f6d7636be4
Tree-SHA512: 480fae14c5440e530ba78a2be19eaaf642260070435e533fc7ab98ddcc2fcac7ad83f2c7e7c6706db3167e8391d7d4abf8784889796c218c2d5bba043144e787
c371cae07a test, init: perturb file to ensure failure instead of only deleting them (brunoerg)
Pull request description:
In `feature_init.py` there is a TODO about perturbing the files instead of only testing by deleting them.
```py
# TODO: at some point, we should test perturbing the files instead of removing
# them, e.g.
#
# contents = target_file.read_bytes()
# tweaked_contents = bytearray(contents)
# tweaked_contents[50:250] = b'1' * 200
# target_file.write_bytes(bytes(tweaked_contents))
#
# At the moment I can't get this to work (bitcoind loads successfully?) so
# investigate doing this later.
```
This PR adds it by writing into the file random bytes and checking whether it throws an error when starting.
ACKs for top commit:
MarcoFalke:
lgtm ACK c371cae07a
Tree-SHA512: d691eee60b91dd9d1b200588608f56b0a10dccd9761a75254b69e0ba5e5866cae14d2f90cb2bd7ec0f95b0617c2562cd33f20892ffd16355b6df770d3806a0ff
This change makes the `bitcoin-wallet` binary path customizable in the
same way how it can be done now with other ones, including `bitcoind`,
`bitcoin-cli` and `bitcoin-util`.
7e3d4f8e86 test: add coverage to ensure the first arg of scantxoutset is needed (ismaelsadeeq)
Pull request description:
Include a test that checks whether the first argument of scantxoutset RPC call "start" is required.
The rpc call should fail if the "start" argument is not provided.
ACKs for top commit:
MarcoFalke:
lgtm ACK 7e3d4f8e86
Tree-SHA512: 6a456af9f3ccd5437be2edcd61936eb9f9c21ab926a6056c2c11b6b5121d1caca4e1f2ffd09015f9414af152c635a20e1da041eefdef980afbe8a0e8ccce07bd
afc2dd5484 test: various `converttopsbt` check cleanups in rpc_psbt.py (Sebastian Falbesoner)
Pull request description:
In the functional test rpc_psbt.py, some comments around the `converttopsbt` RPC checks are wrong or outdated and can be removed:
> _Error could be either "TX decode failed" (segwit inputs causes
> parsing to fail) or "Inputs must not have scriptSigs and
> scriptWitnesses"_
Decoding a valid TX with at least one input always succeeds with the [heuristic](e352f5ab6b/src/core_read.cpp (L126)), i.e. this comment is not right and we can assert for the error string "Inputs must not have scriptSigs and scriptWitnesses" on the calls below.
> _We must set iswitness=True because the serialized transaction has
> inputs and is therefore a witness transaction_
This is also unneeded (and confusing, w.r.t. "is therefore a witness transaction"?), for a TX with one input there is no need to set the `iswitness` parameter. For sake of completeness, we still keep one variant where iswitness is explicitly set to true.
Lastly, there is a superflous `converttopsbt` call on the raw tx which is the same as just [about ~10 lines above](https://github.com/bitcoin/bitcoin/blob/master/test/functional/rpc_psbt.py#L393-L397), so it can be removed.
ACKs for top commit:
ismaelsadeeq:
tested ACK afc2dd5484
achow101:
ACK afc2dd5484
Tree-SHA512: 467efefdb3f61efdb79145044b90fc8dc2f0c425f078117a99112b0074e7d4a32c34e464f665fbf8de70d06caaa5d4ad5908c1d75d2e7607eccb0837480afab3
fa17767154 test: Simplify feature_fastprune.py (MarcoFalke)
Pull request description:
The goal of the test is a single regression check to see if a RPC times out. It shouldn't do more than calling the RPC (and the minimum work needed to get there).
Fix that by removing all blocktools imports and a `for` loop.
ACKs for top commit:
pinheadmz:
ACK fa17767154
theStack:
ACK fa17767154
Tree-SHA512: c9c0154102199b250015ece53005a14d52d857dfa986f3b02a2cb899f16ac8e040d24eb826f35ba15e5ee22ee6a59bf8f74bb8d576b9a12ac6e888beeaaf81cc
710b83938a rpc: return block hash & height in getbalances, gettransaction & getwalletinfo JSONs (Harris)
Pull request description:
Reopens#18570 and closes#18567.
I have rebased the original PR.
Not sure why the original got closed as it was about to get merged.
ACKs for top commit:
achow101:
ACK 710b83938a
Tree-SHA512: d4478d990be98b1642e9ffb2930987f4a224e8bd64e2e35a5dda927a54c509ec9d712cd0eac35dc2bb89f00a1678e530ce14d7445f1dd93aa3a4cce2bc9b130d
24d55fb9cf test: added coverage to rpc_scantxoutset.py (kevkevin)
Pull request description:
Included a test that checks if an invalid first argument is entered we receive a rpc error. The rpc should fail if "start", "status" or "abort" is not the first command.
Relavant: mentioned in https://github.com/bitcoin/bitcoin/pull/27422
ACKs for top commit:
MarcoFalke:
lgtm ACK 24d55fb9cf
theStack:
ACK 24d55fb9cf
Tree-SHA512: 4b804235d3fa17c7bf492068ab47c1f87cb6cfc1a428c51e273ec059d3c41f581bcc467bb5d6d8bbf2fab14c60cd1c52a30c50009efe1c9b5adee70c88897ad9
82e6e3cae5 test: add ripemd160 to test framework modules list (Sebastian Falbesoner)
Pull request description:
Currently test runner doesn't execute the unit tests of the ripemd160 module, so add it to the list. All other framework modules that contain unit tests are included, as can be easily checked via
`$ git grep unittest.TestCase ./test/functional/test_framework/`
This is a late follow-up to PR #23716 (commit ad3e9e1f21).
ACKs for top commit:
MarcoFalke:
lgtm ACK 82e6e3cae5
Tree-SHA512: 10940e215f728291c7149931a356bfc42795c098bda76d760dfa68f86443a3755e1cd35cb9a8a7b2f48880beb53f3bee3842de2d74bcadd45c7b05c13ff04203
Currently test runner doesn't execute the unit tests of the ripemd160
module, so add it to the list. All other framework modules that contain
unit tests are included, as can be easily checked via
`git grep unittest.TestCase ./test/functional/test_framework/`
This is a late follow-up to PR #23716 (commit
ad3e9e1f21).
Included a test that checks if an invalid first argument is entered we
receive a rpc error. The rpc should fail if "start", "status" or "abort"
is not the first command.
8f14fc8622 test: cover fastprune with excessive block size (Matthew Zipkin)
271c23e87f blockstorage: Adjust fastprune limit if block exceeds blockfile size (Martin Zumsande)
Pull request description:
The debug-only `-fastprune` option used in several tests is not always safe to use:
If a `-fastprune` node receives a block larger than the maximum blockfile size of `64kb` bad things happen: The while loop in `BlockManager::FindBlockPos` never terminates, and the node runs oom because memory for `m_blockfile_info` is allocated in each iteration of the loop.
The same would happen if a naive user used `-fastprune` on anything other than regtest (so this can be tested by syncing on signet for example, the first block that crashes the node is at height 2232).
Change the approach by raising the blockfile size to the size of the block, if that block otherwise wouldn't fit (idea by TheCharlatan).
ACKs for top commit:
ryanofsky:
Code review ACK 8f14fc8622. Added new assert, test, and comment since last review
TheCharlatan:
ACK 8f14fc8622
pinheadmz:
ACK 8f14fc8622
Tree-SHA512: df2fea30613ef9d40ebbc2416eacb574f6d7d96847db5c33dda22a29a2c61a8db831aa9552734ea4477e097f253dbcb6dcb1395d43d2a090cc0588c9ce66eac3
dc14ba08e6 test: remove modinv python util helper function (Fabian Jahr)
Pull request description:
Since #27483 was merged the `modinv()` body is just one line calling pythons own implementation of `pow()`. We can just remove the function as it doesn't seem to add any value. Additionally the comment in the function is now outdated and the test is only testing two ways of doing modular inverse but both using python's `pow()` function.
ACKs for top commit:
theStack:
ACK dc14ba08e6
Tree-SHA512: e8b470c72dc3f9fd53699d0684650517b1ea35ad1d4c01cf9472c80d3e4474c0c72e429c0bd201eb99d204c87eee0d68285e6a388e4c506f30e14b2bff9c1c32
057057a2d7 Add test for `sendmany` rpc that uses `subtractfeefrom` parameter (Yusuf Sahin HAMZA)
Pull request description:
This PR adds test that uses `sendmany` rpc to send **BTC** to multiple addresses using `subtractfeefrom` parameter, then checks receiver addresses balances to make sure fees are subtracted correctly.
ACKs for top commit:
achow101:
ACK 057057a2d7
Tree-SHA512: 51167120d489f0ff7b8b9855424d07cb55a8965984f904643cddf45e7a08c350eaded498c350ec9c660edf72c2f128ec142347c9c79d5043d9f6cd481b15cd7e
9c18992bba test: add coverage for `-bantime` (brunoerg)
Pull request description:
This PR adds test coverage for `-bantime`. This flag sets the time in seconds how long the IP is banned (in the case you don't explicitly set `bantime` when using `setban`).
ACKs for top commit:
MarcoFalke:
lgtm ACK 9c18992bba
Tree-SHA512: e95f8608aa5df9b09cc5577daae662ed79ef5d5c69ee5e704d7c69520b9b51cc142e9e6be69d80356eda25a5215c4770b1a208638560c48cd3bc8f6d195a371f
b922f6b526 rpc: scanblocks, add "completed" flag to the result obj (furszy)
ce50acc54f rpc: scanblocks, do not traverse the whole chain block by block (furszy)
Pull request description:
Coming from https://github.com/bitcoin/bitcoin/pull/23549#pullrequestreview-1105712566
The current `scanblocks` flow walks-through every block in the active chain
until hits the chain tip or processes 10k blocks, then calls `lookupFilterRange`
function to obtain all filters from that particular range.
This is only done to obtain the heights range to look up the block
filters. Which is unneeded.
As `scanblocks` only lookup block filters in the active chain, we can
directly calculate the lookup range heights, by using the chain tip,
without requiring to traverse the chain block by block.
ACKs for top commit:
achow101:
ACK b922f6b526
TheCharlatan:
ACK b922f6b526
Tree-SHA512: 0587e6d9cf87a59184adb2dbc26a4e2bce3a16233594c6c330f69feb49bf7dc63fdacf44fc20308e93441159ebc604c63eb7de19204d3e745a2ff16004892b45
be72663a15 test: bumpfee, add coverage for "send coins back to yourself" (furszy)
7bffec6715 bumpfee: enable send coins back to yourself (furszy)
Pull request description:
Simple example:
1) User_1 sends 0.1 btc to user_2 on a low fee transaction.
2) After few hours, the tx is still in the mempool, user_2
is not interested anymore, so user_1 decides to cancel
it by sending coins back to himself.
3) User_1 has the bright idea of opening the explorer and
copy the change output address of the transaction. Then
call bumpfee providing such output (in the "outputs" arg).
Currently, this is not possible. The wallet fails with
"Unable to create transaction. Transaction must have at least
one recipient" error.
The error reason is because we discard the provided output
from the recipients list and set it inside the coin control
so the process adds it later (when the change is calculated).
But.. there is no later if the tx has no outputs.
ACKs for top commit:
ishaanam:
reACK be72663a15
achow101:
ACK be72663a15
Tree-SHA512: c2c38290a998f9b426a830d9624c7feb730158980ac186f8fb0138d5e200935d6538307bc60a2c3d0b7b6ee2b4ffb77a1e98baf8feb1d20a7d825f6055ac377f
To tell the user whether the process was aborted or not.
Plus, as the process can be aborted prior to the end range,
have also changed the "to_height" result value to return the
last scanned block instead of the end range block.
fac395e5eb ci: Bump ci/lint/Dockerfile (MarcoFalke)
fa6eb65167 test: Use python3.8 pow() (MarcoFalke)
88881cf7ac Bump python minimum version to 3.8 (MarcoFalke)
Pull request description:
There is no pressing reason to drop support for 3.7, however there are several maintenance issues:
* There is no supported operating system that ships 3.7 by default. (debian:buster is EOL and unmaintained to the extent that it doesn't run in the CI environment. See https://github.com/bitcoin/bitcoin/pull/27340#issuecomment-1484988445)
* Compiling python 3.7 from source is also unsupported on at least macos, according to https://github.com/bitcoin/bitcoin/pull/24017#issuecomment-1107820790
* Recent versions of lief require 3.8, see https://github.com/bitcoin/bitcoin/pull/27507#issuecomment-1517561645
Fix all maintenance issues by bumping the minimum.
ACKs for top commit:
RandyMcMillan:
ACK fac395e
fjahr:
ACK fac395e5eb
fanquake:
ACK fac395e5eb
Tree-SHA512: c198decdbbe29d186d73ea3f6549d8a38479383495d14a965a2f9211ce39637b43f13a4c2a5d3bf56e2d468be4bbe49b4ee8e8e19ec69936ff43ddf2b714c712
33fdfc7986 test: perturb anchors.dat to test it doesn't throw an error during initialization (brunoerg)
Pull request description:
Got some inspiration from `feature_init`. This PR tests whether perturbing `anchors.dat` doesn't throw any error during initialization.
3f1f5f6f1e/src/addrdb.cpp (L223-L235)
ACKs for top commit:
MarcoFalke:
lgtm ACK 33fdfc7986
Tree-SHA512: e6584debb37647677581fda08366b45b42803022cc4c4f1d5a7bd5e9e04d64da77656dad2b804855337487bdcfc891f300a2e03668d6122de769dd14f39af9ed
4bdcf57158 test: test banlist database recreation (brunoerg)
Pull request description:
This PR adds test coverage for 'banlist database recreation'. If it wasn't able to read ban db (in `LoadBanlist`), so it should create a new (an empty, ofc) one.
d8bdee0fc8/src/banman.cpp (L28-L45)
ACKs for top commit:
MarcoFalke:
lgtm ACK 4bdcf57158
Tree-SHA512: d9d0cd0c4b3797189dff00d3a634878188e7cf51e78346601fc97e2bf78c495561705214062bb42ab8e491e0d111f8bfcf74dbc801768bc02cf2b45f162aad85
cca4f82b82 test: add coverage for rpc error when trying to rescan beyond pruned data (brunoerg)
Pull request description:
This PR adds test coverage for the following rpc error:
15692e2641/src/wallet/rpc/transactions.cpp (L896-L899)
ACKs for top commit:
MarcoFalke:
lgtm ACK cca4f82b82
aureleoules:
ACK cca4f82b82
Tree-SHA512: 724a055e9f6cddf1935699e8769015115f24f6485a0bd87e8660072ee44a15c1bddfdda848acc101ea7184b7e65a33b5b0d80b563d2ba3ecdab7a631378d6476
