_Motivation_
Currently, the wallet uses a fSubtractFeeAmount (SFFO) flag on the
recipients objects for all forms of sending calls. According to the
commit discussion, this flag was chiefly introduced to permit sweeping
without manually calculating the fees of transactions. However, the flag
leads to unintuitive behavior and makes it more complicated to test
many wallet RPCs exhaustively. We proposed to introduce a dedicated
`sendall` RPC with the intention to cover this functionality.
Since the proposal, it was discovered in further discussion that our
proposed `sendall` rpc and SFFO have subtly different scopes of
operation.
• sendall:
Use _specific UTXOs_ to pay a destination the remainder after fees.
• SFFO:
Use a _specific budget_ to pay an address the remainder after fees.
While `sendall` will simplify cases of spending from specific UTXOs,
emptying a wallet, or burning dust, we realized that there are some
cases in which SFFO is used to pay other parties from a limited budget,
which can often lead to the creation of change outputs. This cannot be
easily replicated using `sendall` as it would require manual computation
of the appropriate change amount.
As such, sendall cannot replace all uses of SFFO, but it still has a
different use case and will aid in simplifying some wallet calls and
numerous wallet tests.
_Sendall call details_
The proposed sendall call builds a transaction from a specific subset of
the wallet's UTXO pool (by default all of them) and assigns the funds to
one or more receivers. Receivers can either be specified with a specific
amount or receive an equal share of the remaining unassigned funds. At
least one recipient must be provided without assigned amount to collect
the remainder. The `sendall` call will never create change. The call has
a `send_max` option that changes the default behavior of spending all
UTXOs ("no UTXO left behind"), to maximizing the output amount of the
transaction by skipping uneconomic UTXOs. The `send_max` option is
incompatible with providing a specific set of inputs.
fa0758e145 test: Add diamond-shape prioritisetransaction test (MarcoFalke)
fa450c18db test: Rework create_self_transfer_multi (MarcoFalke)
Pull request description:
Looks like there is no test for diamonds, only for chains (in `mempool_packages.py`)
ACKs for top commit:
jamesob:
ACK fa0758e145
Tree-SHA512: d261184a81df77d24fc256f58ad5ed4a13b7cd4e33f74c8b79495c761ff417817602d8e5d4f63f4bb1000ac63f89bbfa54d8d8994a7b2bb2e8a484c467330984
45e67b2695 test: invalid -i2psam will raise an init error (brunoerg)
Pull request description:
This PR adds test coverage (at `feature_proxy.py`) for the following init error:
2f0f056e08/src/init.cpp (L1791)
It starts the node with an invalid -i2psam (`-i2psam=invalidhere`) and test if it raises an error when initializing.
ACKs for top commit:
dunxen:
Code review ACK 45e67b2
Tree-SHA512: b24e3f6e7a9316b9ebc0b6c8bcf1315faff60a9e258d7bb3dbeb9f6695a728bb3083aea2f81114072fe13822bfca34d4a0f44f229825f7c97a81619d810010c0
89bb25d22a test: check localaddresses in getnetworkinfo for nodes with proxy (brunoerg)
Pull request description:
This PR adds test coverage for the field `localaddresses` for `getnetworkinfo`. In this case, it verifies if this field is empty for all nodes since they are using proxy.
Reference:
515200298b/src/init.cpp (L449)
ACKs for top commit:
jonatack:
ACK 89bb25d22a
Tree-SHA512: 3c765c7060b6972c1ae5a1104734cd7669b650b5f6aa4f623f4299567732260da5083fef306a7c1e71c931f5d1396f24abad251d95c3d82b1f3ee0efee7fcd1f
9053f64fcb [doc] release notes for random change target (glozow)
46f2fed6c5 [wallet] remove MIN_CHANGE (glozow)
a44236addd [wallet] randomly generate change targets (glozow)
1e52e6bd0a refactor coin selection for parameterizable change target (glozow)
Pull request description:
Closes#24458 - the wallet always chooses 1 million sats as its change target, making it easier to fingerprint transactions created by the Core wallet. Instead of using a fixed value, choose one randomly each time (within a range). Using 50ksat (around $20) as the lower bound and `min(1 million sat, 2 * average payment value)` as the upper bound.
RFC: If the payment is <25ksat, this doesn't work, so we're using the range (payment amount, 50ksat) instead.
ACKs for top commit:
achow101:
ACK 9053f64fcb
Xekyo:
reACK 9053f64fcb
Tree-SHA512: 45ce5d064697065549473347648e29935733f3deffc71a6ab995449431f60302d1f9911a0994dfdb960b48c48b5d8859f168b396ff2a62db67d535a7db041d35
da2bc865d6 [wallet] don't create long chains by default (glozow)
Pull request description:
Default mempool policy doesn't let you have chains longer than 25 transactions. This is locally configurable of course, but it's not really safe to assume that a chain longer than 25 transactions will propagate. Thus, the wallet should probably avoid creating such transactions by default; set `DEFAULT_WALLET_REJECT_LONG_CHAINS` to true.
Closes#9752Closes#10004
ACKs for top commit:
MarcoFalke:
re-ACK da2bc865d6 only change is fixing typos in tests 🎏
Tree-SHA512: 65d8e4ec437fe928adf554aa7e819a52e0599b403d5310895f4e371e99bbc838219b3097c4d2f775bc870ac617ef6b4227b94291f2b376f824f14e8f2b152f31
0000ff0d6b test: move-only: Move all generate* tests to a single file (MarcoFalke)
Pull request description:
Seems a bit overkill to spread tests for the `generate*` methods over several files. Combining them into a single file has also a nice side-effect of requiring less node (re)starts, which are expensive in valgrind.
ACKs for top commit:
glozow:
utACK 0000ff0d6b
Tree-SHA512: 8269eb05649a871011bbfbd1838d0f7d1dac4a35b3b198fc43fe85131fda8a53803b75da78cbf422eabf086006dee4421e622fbe706f6781a3848b989024001b
1066d10f71 scripted-diff: rename TxRelay members (John Newbery)
575bbd0dea [net processing] Move tx relay data to Peer (John Newbery)
785f55f7ee [net processing] Move m_wtxid_relay to Peer (John Newbery)
36346703f8 [net] Add CNode.m_relays_txs and CNode.m_bloom_filter_loaded (John Newbery)
Pull request description:
This continues the work of moving application layer data into net_processing, by moving all tx data into the new Peer object added in #19607.
For motivation, see #19398.
ACKs for top commit:
dergoegge:
ACK 1066d10f71 - This is a good layer separation improvement with no behavior changes.
glozow:
utACK 1066d10f71
Tree-SHA512: 0c9d6b8a0a05e2d816b6d6588b7df133842ec960ae67667813422aa7bd8eb5308599c714f3822a98ddbdf364ffab9050b055079277ba4aff24092557ff99ebcc
cccc1e70b8 Enforce Taproot script flags whenever WITNESS is set (MarcoFalke)
fa42299411 Remove nullptr check in GetBlockScriptFlags (MarcoFalke)
faadc606c7 refactor: Pass const reference instead of pointer to GetBlockScriptFlags (MarcoFalke)
Pull request description:
Now that Taproot is active, it makes sense to enforce its rules on all blocks, even historic ones, regardless of the deployment status.
### Benefits:
(With "script flags" I mean "taproot script verification flags".)
* Script flags are known ahead for all blocks (even blocks not yet created) and do not change. This may benefit static analysis, code review, and development of new script features that build on Taproot.
* Any future bugs introduced in the deployment code won't have any effect on the script flags, as they are independent of deployment.
* Enforcing the taproot rules regardless of the deployment status makes testing easier because invalid blocks after activation are also invalid before activation. So there is no need to differentiate the two cases.
* It gives belt-and-suspenders protection against a practically expensive and theoretically impossible IBD reorg attack where the node is eclipsed. While `nMinimumChainWork` already protects against this, the cost for a few months worth of POW might be lowered until a major version release of Bitcoin Core reaches EOL. The needed work for the attack is the difference between `nMinimumChainWork` and the work at block 709632.
For reference, previously the same was done for P2SH and WITNESS in commit 0a8b7b4b33.
### Implementation:
I found one block which fails verification with the flags applied, so I added a `TaprootException`, similar to the `BIP16Exception`.
For reference, the debug log:
```
ERROR: ConnectBlock(): CheckInputScripts on b10c007c60e14f9d087e0291d4d0c7869697c6681d979c6639dbd960792b4d41 failed with non-mandatory-script-verify-flag (Witness program was passed an empty witness)
BlockChecked: block hash=0000000000000000000f14c35b2d841e986ab5441de8c585d5ffe55ea1e395ad state=non-mandatory-script-verify-flag (Witness program was passed an empty witness)
InvalidChainFound: invalid block=0000000000000000000f14c35b2d841e986ab5441de8c585d5ffe55ea1e395ad height=692261 log2_work=92.988459 date=2021-07-23T08:24:20Z
InvalidChainFound: current best=0000000000000000000067b17a4c0ffd77c29941b15ad356ca8f980af137a25d height=692260 log2_work=92.988450 date=2021-07-23T07:47:31Z
ERROR: ConnectTip: ConnectBlock 0000000000000000000f14c35b2d841e986ab5441de8c585d5ffe55ea1e395ad failed, non-mandatory-script-verify-flag (Witness program was passed an empty witness)
```
Hint for testing, make sure to set `-noassumevalid`.
### Considerations
Obviously this change can lead to consensus splits on the network in light of massive reorgs. Currently the last block before Taproot activation, that is the last block without the Taproot script flags set, is only buried by a few days of POW. However, when and if this patch is included in the next major release, it will be buried by a few months of POW. BIP90 considerations apply when looking at reorgs this large.
ACKs for top commit:
Sjors:
tACK cccc1e70b8
achow101:
ACK cccc1e70b8
laanwj:
Code review ACK cccc1e70b8
ajtowns:
ACK cccc1e70b8 ; code review; wrote a "getblockscriptflags" rpc to quickly check that blocks just had bit 17 (taproot) added; review of earlier revisions had established non-exception blocks do validate with taproot rules enabled.
jamesob:
ACK cccc1e70b8 ([`jamesob/ackr/23536.1.MarcoFalke.enforce_taproot_script_f`](https://github.com/jamesob/bitcoin/tree/ackr/23536.1.MarcoFalke.enforce_taproot_script_f))
Tree-SHA512: 00044de68939caef6420ffd588c1291c041a8b397c80a3df1e3e3487fbeae1821d23975c51c95e44e774558db76f943b00b4e27cbd0213f64a9253116dc6edde
58a14795b8 test: passing -onlynet=onion with -onion=0/-noonion raises expected init error (Jon Atack)
7000f66d36 test: passing -onlynet=onion without -proxy/-onion raises expected init error (Jon Atack)
8332e6e4cf test: passing invalid -onion raises expected init error (Jon Atack)
d5edb08708 test: passing invalid -proxy raises expected init error (Jon Atack)
bd57dcbaf2 test: hoist proxy out of 2 network loops in feature_proxy.py (Jon Atack)
afdf2de282 test: add CJDNS to LimitedAndReachable_Network unit tests (Jon Atack)
2b7a8180a9 net, init: assert each network reachability is true by default (Jon Atack)
Pull request description:
Adds missing network reachability test coverage and an assertion during init, noticed while reviewing #22834:
- assert during init that each network reachability is true by default
- add CJDNS to the `LimitedAndReachable_Network` unit tests
- hoist proxy out of two network loops in feature_proxy.py
- test that passing invalid `-proxy` raises expected init error
- test that passing invalid `-onion` raises expected init error
- test that passing `-onlynet=onion` without `-proxy` and `-onion` raises expected init error
- test that passing `-onlynet=onion` with `-onion=0` and with `-noonion` raises expected init error
ACKs for top commit:
vasild:
ACK 58a14795b8
brunoerg:
ACK 58a14795b8
dongcarl:
Code Review ACK 58a14795b8
Tree-SHA512: bdee6dd0c12bb63591ce7c9321fe77b509ab1265123054e774adc38a187746dddafe1627cbe89e990bcc78b45e194bfef8dc782710d5b217e2e2106ab0158827
bc90b8d869 [move only] remove `is_wallet_compiled` checks (josibake)
0bfbf7fb24 test: use MiniWallet in `interfaces_zmq` (josibake)
Pull request description:
While working on #24584 , `interface_zmq` started failing due to coin selection not running deterministically. The test doesn't actually need the wallet, so this PR migrates it to use MiniWallet
_Note for reviewers:_ the second commit moves large chunks of code out of an if block, so it may be helpful to review with something that ignores whitespace, e.g `git diff -w master`
ACKs for top commit:
vincenzopalazzo:
ACK bc90b8d869
Tree-SHA512: c618e23d00635d72dafdef28e68cbc88b9cc2030d4898fc5b7eac926fd621684c1958c075ed167192716b18308da5a0c1f1393396e31b99d0d3bde78b78fefc5
The external input test with specifying input weight would make a
pessimistic estimate of the input weight. However this would result in a
test failure as it is sometimes too pessimistic when an ECDSA signature
ends up being smaller than usual. To correct this, we can calculate the
input weight more accurately.
b2813980b8 init: disallow reindex-chainstate when pruning (Martin Zumsande)
Pull request description:
The combination of `-reindex-chainstate` and `-prune` currently makes the node stuck in an endless loop:
- `LoadChainstate()` will wipe the existing chainstate (so we have no genesis block anymore). It won't clean up unusable block files by calling `CleanupBlockRevFiles()` as for full `-reindex`.
- `ThreadImport()` has [logic](91d12344b1/src/node/blockstorage.cpp (L855)) of reloading Genesis after reindexing. This is what makes full `-reindex` work with `-prune` but it's not executed for `-reindex-chainstate`.
- Since we still don't have a genesis block, init will wait for it forever in an endless loop ([code](91d12344b1/src/init.cpp (L1630-L1640))).
Fix this by disallowing `-reindex-chainstate` together with `-prune`. This is discouraged in the help for `-reindex-chainstate` anyway ("When in pruning mode or if blocks on disk might be corrupted, use full -reindex instead.") but wasn't enforced.
Fixes#24242
ACKs for top commit:
MarcoFalke:
cr ACK b2813980b8
Tree-SHA512: 7220842daaf9a4f972d82b13b81fdeac2833bf5e665c5b0f8eaf6a4bcd0725c8e97d19ec956ca4b730065a983475bb3a2732713d338f4caf8666ccbf63d4d988
fa76d8d4d7 test: Actually print TSan tracebacks (MarcoFalke)
Pull request description:
Commit 5e5138a721 made the TSan logs to be printed before returning an error from the ci script.
However, it seems that on Cirrus CI, the `--failfast` option will kill not only all python process and bitcoind child process, but also the parent CI bash script, rendering the `trap` inefficient. I believe this bug was introduced in commit 451b96f7d2.
ACKs for top commit:
fanquake:
utACK fa76d8d4d7
Tree-SHA512: 686f889d38a343882cb62ad6e0c2080196330e7cc7086891a7ff66d9443b455c82ba8d7e4a5cc42daa0513b0ad2743055bfe90e2f6ac88a910ee3b663fabddcd
2b6dd4e75b test: use MiniWallet for mempool_package_onemore.py (Sebastian Falbesoner)
eb3c5c4ef2 test: MiniWallet: add helper methods `{send,create}_self_transfer_multi` (Sebastian Falbesoner)
Pull request description:
This PR enables one more of the non-wallet functional tests (mempool_package_onemore.py) to be run even with the Bitcoin Core wallet disabled by using the MiniWallet instead, as proposed in https://github.com/bitcoin/bitcoin/issues/20078. For this purpose helper methods `MiniWallet.{create,send}_self_transfer_multi` are introduced which serve as a replacement for `chain_transaction`. With this, it should be also quite straight-forward to change the larger related test `mempool_packages.py` to use MiniWallet.
ACKs for top commit:
MarcoFalke:
ACK 2b6dd4e75b💾
Tree-SHA512: 0c97fa0519ca5eaa6df8953a04678aa8a6a66905a82db6ff40042a675d0c0682aee829a48db84e4e7983d8f766875021f0d39d65e12889342610b8861bc29cd5
2726b60a3a test: use MiniWallet for rpc_createmultisig.py (Ayush Sharma)
Pull request description:
This PR enables one of the non-wallet functional tests (rpc_createmultisig.py) to be run even with the Bitcoin Core wallet disabled by using the MiniWallet instead, as proposed in #20078 .
ACKs for top commit:
danielabrozzoni:
re-ACK 2726b60a3a
Tree-SHA512: fb0ef22d3f1c161ca5963cb19ce76533ac3941f15102fc0aa2286ef3bec48f219e5934d504b41976f9f295fb6ca582b737e0fea896df4eb964cdaba1b2c91650
fa7a576391 test: Run non-wallet tests only once (MarcoFalke)
Pull request description:
I don't see why non-wallet tests should run for two wallet configs, even though they never use a wallet.
ACKs for top commit:
achow101:
ACK fa7a576391
Tree-SHA512: 2a135acf3c3c83a2704ae11f40c72882b23a676828647be1a066653c4d00e4523704f377eb8745c6386829601cc5d643abdce376831c1db91a07e999e1d5e01f
fa8593f898 test: Fix generate calls and comments in feature_segwit (MarcoFalke)
Pull request description:
There are currently a few incorrect comments: Block `432` is mined "twice" (The second one is actually 433).
There isn't any need to mine this many blocks anyway, so remove a few calls.
ACKs for top commit:
theStack:
Tested ACK fa8593f898
Tree-SHA512: b034077b85e6c978a80aa4de493797b4ae451d686cfb3e4fe40f37a38f41f7cb886f8e00a1c245a284be3502164b17414097fcb0bef66d155a1c1db5cfbe9e8f
fa48ea3067 Use MiniWallet in feature_coinstatsindex (MarcoFalke)
fab61437f6 test: Refactor MiniWallet get_utxo helper (MarcoFalke)
Pull request description:
Allows the test to be run even without a wallet compiled
ACKs for top commit:
josibake:
ACK fa48ea3067
ayush933:
tACK fa48ea3 . The test runs successfully with the wallet disabled.
willcl-ark:
tACK fa48ea3067 both with and without wallet compiled in.
Tree-SHA512: e04e04ea0f236c062d6be68909ece2770130ce1d5343823893073d95aebc6eedb1ad1dc5bc41e5b0cb0bf2cd9018bb1d668f0e7f5f1101ed4e0b007ed6b00f69
61152183ab wallet: Add a deprecation warning for newly created legacy wallets (Andrew Chow)
Pull request description:
As we slowly deprecate legacy wallets, we need to warn users that are making new legacy wallets that their wallet type is going to be unsupported in the future.
ACKs for top commit:
jonatack:
ACK 61152183ab
S3RK:
reACK 61152183ab
theStack:
ACK 61152183ab
Tree-SHA512: e89bfb8168869542498958f0c9a2ab302dfd43287f8a49e7d9e09f60438a567bb8b7219a4e569797ee819b30b624f532fcc0b70c6aa0edcb392a301b8ce8b541
Check that within 24h addr of a given node is forwarded
to the same peer(s), and then the destination is
rotated every 24h.
Co-authored-by: Jon Atack <jon@atack.com>
5d7c69b887 rpc: rename getdeploymentinfo status-next to status_next (Jon Atack)
Pull request description:
Rename the `status-next` field to `status_next` in getdeploymentinfo before the RPC is released in v23.
Before
```
Result:
{ (json object)
"hash" : "str", (string) requested block hash (or tip)
"height" : n, (numeric) requested block height (or tip)
"deployments" : { (json object)
"xxxx" : { (json object) name of the deployment
"type" : "str", (string) one of "buried", "bip9"
"height" : n, (numeric, optional) height of the first block which the rules are or will be enforced (only for "buried" type, or "bip9" type with "active" status)
"active" : true|false, (boolean) true if the rules are enforced for the mempool and the next block
"bip9" : { (json object, optional) status of bip9 softforks (only for "bip9" type)
"bit" : n, (numeric, optional) the bit (0-28) in the block version field used to signal this softfork (only for "started" and "locked_in" status)
"start_time" : xxx, (numeric) the minimum median time past of a block at which the bit gains its meaning
"timeout" : xxx, (numeric) the median time past of a block at which the deployment is considered failed if not yet locked in
"min_activation_height" : n, (numeric) minimum height of blocks for which the rules may be enforced
"status" : "str", (string) status of deployment at specified block (one of "defined", "started", "locked_in", "active", "failed")
"since" : n, (numeric) height of the first block to which the status applies
"status-next" : "str", (string) status of deployment at the next block
"statistics" : { (json object, optional) numeric statistics about signalling for a softfork (only for "started" and "locked_in" status)
"period" : n, (numeric) the length in blocks of the signalling period
"threshold" : n, (numeric, optional) the number of blocks with the version bit set required to activate the feature (only for "started" status)
"elapsed" : n, (numeric) the number of blocks elapsed since the beginning of the current period
"count" : n, (numeric) the number of blocks with the version bit set in the current period
"possible" : true|false (boolean, optional) returns false if there are not enough blocks left in this period to pass activation threshold (only for "started" status)
},
"signalling" : "str" (string) indicates blocks that signalled with a # and blocks that did not with a -
}
}
}
}
```
After
```
Result:
{ (json object)
"hash" : "str", (string) requested block hash (or tip)
"height" : n, (numeric) requested block height (or tip)
"deployments" : { (json object)
"xxxx" : { (json object) name of the deployment
"type" : "str", (string) one of "buried", "bip9"
"height" : n, (numeric, optional) height of the first block which the rules are or will be enforced (only for "buried" type, or "bip9" type with "active" status)
"active" : true|false, (boolean) true if the rules are enforced for the mempool and the next block
"bip9" : { (json object, optional) status of bip9 softforks (only for "bip9" type)
"bit" : n, (numeric, optional) the bit (0-28) in the block version field used to signal this softfork (only for "started" and "locked_in" status)
"start_time" : xxx, (numeric) the minimum median time past of a block at which the bit gains its meaning
"timeout" : xxx, (numeric) the median time past of a block at which the deployment is considered failed if not yet locked in
"min_activation_height" : n, (numeric) minimum height of blocks for which the rules may be enforced
"status" : "str", (string) status of deployment at specified block (one of "defined", "started", "locked_in", "active", "failed")
"since" : n, (numeric) height of the first block to which the status applies
"status_next" : "str", (string) status of deployment at the next block
"statistics" : { (json object, optional) numeric statistics about signalling for a softfork (only for "started" and "locked_in" status)
"period" : n, (numeric) the length in blocks of the signalling period
"threshold" : n, (numeric, optional) the number of blocks with the version bit set required to activate the feature (only for "started" status)
"elapsed" : n, (numeric) the number of blocks elapsed since the beginning of the current period
"count" : n, (numeric) the number of blocks with the version bit set in the current period
"possible" : true|false (boolean, optional) returns false if there are not enough blocks left in this period to pass activation threshold (only for "started" status)
},
"signalling" : "str" (string) indicates blocks that signalled with a # and blocks that did not with a -
}
}
}
}
```
Top commit has no ACKs.
Tree-SHA512: 4facfd7af3cfb7b6f5495758c4387602802f5e39d9270b162d17350a7f954eab0b74d895f17f0d8dfbc7814d36db7cff56d08c42728432885ea6f4e37aea4aa8
40e871d9b4 [miner] always assume we can create witness blocks (glozow)
Pull request description:
Given the low possibility of a reorg reverting the segwit soft fork, there is no longer a need to check whether segwit is active to see if it's okay to add to the block template (see also #23512, #21009, etc). `TestBlockValidity()` is also run on the block template at the end of `CreateNewBlock()`, so any invalid block would be caught there.
ACKs for top commit:
gruve-p:
ACK 40e871d9b4
jnewbery:
utACK 40e871d9b4, although I disagree about changing the test for segwit transaction in mempool before activagtion, instead of just removing it: https://github.com/bitcoin/bitcoin/pull/24421#discussion_r822933721.
achow101:
ACK 40e871d9b4
theStack:
Code-review ACK 40e871d9b4
Tree-SHA512: bf4860bf2bed8339622d05228d11d60286edb0c32a9a3c434b8d154913c07ea56e50649f4af7009c2a1c6a58a81d2299ab43b41a6f16dee7d08cc89cc1603019
db27ac9354 tests: Ensure sorted/multi_a descriptors always generate different addrs (Andrew Chow)
Pull request description:
Sometimes the multi_a and sortedmulti_a descriptors will produce some of the same addresses in the tests. This causes the wallets to start generating addresses at a different index as they detect that one of the addresses is used. This subsequently causes a test failure.
To avoid this problem, use descriptors that will produce unique addresses by putting one of the multi_a in a different branch.
ACKs for top commit:
ajtowns:
ACK db27ac9354
theStack:
Tested ACK db27ac9354
Tree-SHA512: 0f57822bf4c7c79da304f092d7d43d6118e78a087cbeb0766fbbf634dc27911ae723d5d41350884d3b63a24d3b3817944f7e5fa534afb849161dd008a1e4a62f
7abd8b21ba doc: include wtxid in TransactionDescriptionString (brunoerg)
2d596bce6f doc: add wtxid info in release-notes (brunoerg)
a5b66738f1 test: add wtxid in expected_fields for wallet_basic (brunoerg)
e8c659a297 wallet: add wtxid in WalletTxToJSON (brunoerg)
7482b6f895 wallet: add GetWitnessHash() (brunoerg)
Pull request description:
This PR add `wtxid` in `WalletTxToJSON` which allows to return this field in `listsinceblock`, `listtransactions` and `gettransaction` (RPCs).
ACKs for top commit:
achow101:
re-ACK 7abd8b21ba
w0xlt:
crACK 7abd8b2
luke-jr:
re-utACK 7abd8b21ba
Tree-SHA512: f86f2dbb5e38e7b19932006121802f47b759d31bdbffe3263d1db464f6a3a30fddd68416f886a44f6d3a9fd570f7bd4f8d999737ad95c189e7ae5e8ec1ffbdaa
fa097d074b addrman: Log too low compat value (MarcoFalke)
Pull request description:
Before this patch, when writing a negative `lowest_compatible` value, it would be read as a positive value. For example `-32` will be read as `224`. There is generally nothing wrong with that. Though, similarly there shouldn't be anything wrong with refusing to read a negative value. I find the code after this patch more logical than before. Also, this allows dropping a file-wide sanitizer suppression.
In practice none of this should ever happen. Bitcoin Core would never write a negative `lowest_compatible` in normal operation, unless the file storage is later corrupted by external influence.
ACKs for top commit:
mzumsande:
re-ACK fa097d074b
Tree-SHA512: 9aae7b8fe666f52f667f149667025e0160cef1a793cc4d392e36608f65c2bee8096da429235118f40a3368f327aabe30f3732ae78c5874648ea6f423f2687b65
Sometimes the multi_a and sortedmulti_a descriptors will produce some of
the same addresses in the tests. This causes the wallets to start
generating addresses at a different index as they detect that one of
the addresses is used. This subsequently causes a test failure.
To avoid this problem, use descriptors that will produce unique
addresses by putting one of the multi_a in a different branch.
4828d53ecc Add (sorted)multi_a descriptors to doc/descriptors.md (Pieter Wuille)
b5f33ac1f8 Simplify wallet_taproot.py functional test (Pieter Wuille)
eb0667ea96 Add tests for (sorted)multi_a derivation/signing (Pieter Wuille)
c17c6aa08d Add signing support for (sorted)multi_a scripts (Pieter Wuille)
3eed6fca57 Add multi_a descriptor inference (Pieter Wuille)
79728c4a3d Add (sorted)multi_a descriptor and script derivation (Pieter Wuille)
25e95f9ff8 Merge/generalize IsValidMultisigKeyCount/GetMultisigKeyCount (Pieter Wuille)
Pull request description:
This adds a new `multi_a(k,key_1,key_2,...,key_n)` (and corresponding `sortedmulti_a`) descriptor for k-of-n policies inside `tr()`. Semantically it is very similar to the existing `multi()` descriptor, but with the following changes:
* The corresponding script is `<key1> OP_CHECKSIG <key2> OP_CHECKSIGADD <key3> OP_CHECKSIGADD ... <key_n> OP_CHECKSIGADD <k> OP_NUMEQUAL`, rather than the traditional `OP_CHECKMULTISIG`-based script, making it usable inside the `tr()` descriptor.
* The keys can optionally be specified in x-only notation.
* Both the number of keys and the threshold can be as high as 999; this is the limit due to the consensus stacksize=1000 limit
I expect that this functionality will later be replaced with a miniscript-based implementation, but I don't think it's necessary to wait for that.
Limitations:
* The wallet code will for not estimate witness size incorrectly for script path spends, which may result in a (dramatic) fee underpayment with large multi_a scripts.
* The multi_a script construction is (slightly) suboptimal for n-of-n (where a `<key1> OP_CHECKSIGVERIFY ... <key_n-1> OP_CHECKSIGVERIFY <key_n> OP_CHECKSIG` would be better). Such a construction is not included here.
ACKs for top commit:
achow101:
ACK 4828d53ecc
gruve-p:
ACK 4828d53ecc
sanket1729:
code review ACK 4828d53ecc
darosior:
Code review ACK 4828d53ecc
Tree-SHA512: 5dcd434b79585f0ff830f7d501d27df5e346f5749f47a3109ec309ebf2cbbad0e1da541eec654026d911ab67fd7cf7793fab0f765628d68d81b96ef2a4d234ce
If `-bind=` is provided then we would bind only to a particular address
and should not add all the other addresses of the machine to the list of
local addresses.
Fixes https://github.com/bitcoin/bitcoin/issues/20184 (case 4.)
`GetListenPort()` uses a simple logic: "if `-port=P` is given, then we
must be listening on `P`, otherwise we must be listening on `8333`".
This is however not true if `-bind=` has been provided with `:port` part
or if `-whitebind=` has been provided. Thus, extend `GetListenPort()` to
return the port from `-bind=` or `-whitebind=`, if any.
Fixes https://github.com/bitcoin/bitcoin/issues/20184 (cases 1. 2. 3. 5.)
Rename `CaptureMessage()` to `CaptureMessageToFile()` and introduce a
`std::function` variable called `CaptureMessage` whose value can be
changed by unit tests, should they need to inspect message contents.
c4d76c6faa tests: Tests for inactive HD chains (Andrew Chow)
8077862c5e wallet: Refactor TopUp to be able to top up inactive chains too (Andrew Chow)
70134eb34f wallet: Properly set hd chain counters when loading (Andrew Chow)
961b9e4e40 wallet: Parse hdKeypath if key_origin is not available (Andrew Chow)
0652ee73ec Add size check on meta.key_origin.path (Rob Fielding)
Pull request description:
Currently inactive HD chains are only derived from at the time a key in that chain is found to have been used. However, at that time, the wallet may not be able to derive keys (e.g. it is locked). Currently we would just move on and not derive any new keys, however this could result in missing funds.
This PR resolves this problem by adding memory only variables to `CHDChain` which track the highest known index. `TopUp` is modified to always try to top up the inactive HD chains, and this process will use the new variables to determine how much to top up. In this way, after an encrypted wallet is unlocked, the inactive HD chains will be topped up and hopefully funds will not be missed.
Note that because these variables are not persisted to disk (because `CHDChain`s for inactive HD chains are not written to disk), if an encrypted wallet is not unlocked in the same session as a key from an inactive chain is found to be used, then it will not be topped up later unless more keys are found.
Additionally, wallets which do not have upgraded key metadata will not derive any keys from inactive HD chains. This is resolved by using the derivation path string in `CKeyMetadata.hdKeypath` to determine what indexes to derive.
ACKs for top commit:
laanwj:
Code review ACK c4d76c6faa
Tree-SHA512: b2b572ad7f1b1b2847edece09f7583543d63997e18ae32764e5a27ad608dd64b9bdb2d84ea27137894e986a8e82f047a3dba9c8015b74f5f179961911f0c4095
0eea83a85e scripted-diff: rename `proxyType` to `Proxy` (Vasil Dimov)
e53a8505db net: respect -onlynet= when making outbound connections (Vasil Dimov)
Pull request description:
Do not make outbound connections to hosts which belong to a network
which is restricted by `-onlynet`.
This applies to hosts that are automatically chosen to connect to and to
anchors.
This does not apply to hosts given to `-connect`, `-addnode`,
`addnode` RPC, dns seeds, `-seednode`.
Fixes https://github.com/bitcoin/bitcoin/issues/13378
Fixes https://github.com/bitcoin/bitcoin/issues/22647
Supersedes https://github.com/bitcoin/bitcoin/pull/22651
ACKs for top commit:
naumenkogs:
utACK 0eea83a85e
prayank23:
reACK 0eea83a85e
jonatack:
ACK 0eea83a85e code review, rebased to master, debug built, and did some manual testing with various config options on signet
Tree-SHA512: 37d68b449dd6d2715843fc84d85f48fa2508be40ea105a7f4a28443b318d0b6bd39e3b2ca2a6186f2913836adf08d91038a8b142928e1282130f39ac81aa741b
c7376cc8d7 tests: Test upgrading wallet with privkeys disabled (Andrew Chow)
3d985d4f43 wallet: Don't generate keys when privkeys disabled when upgrading (Andrew Chow)
Pull request description:
When we're upgrading a wallet, we shouldn't be trying to generate new keys for wallets where private keys are disabled.
Fixes#23610
ACKs for top commit:
laanwj:
Code review ACK c7376cc8d7
benthecarman:
tACK c7376cc8d7 this fixed the issue for me
Tree-SHA512: fa07cf37df9196ff98671bb1ce5c9aa0bab46495066b4dab796d7e8e5d5c7adb414ff56adae4fd3e15658a610995bd19a9e1edb00c46144b0df635c5b343f3a6
fa7991601c Fixup style of VerifyDB (MarcoFalke)
fa462ea787 Avoid implicit-integer-sign-change in VerifyLoadedChainstate (MarcoFalke)
Pull request description:
This happens when checking all blocks (`-1`).
To test:
```
./configure CC=clang CXX=clang++ --with-sanitizers=undefined,integer
make
UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" ./test/functional/rpc_blockchain.py
ACKs for top commit:
theStack:
Code-review ACK fa7991601c
brunoerg:
crACK fa7991601c
Tree-SHA512: bcbe6becf2fbedd21bbde83a544122e79465937346802039532143b2e4165784905a8852c0ccb088b964874df5e5550931fdde3629cbcee3ae237f2f63c43a8e
d41ed32153 p2p: Avoid InitError when downgrading peers.dat (junderw)
Pull request description:
fixes#24188 (also see https://github.com/bitcoin/bitcoin/pull/22762#issuecomment-951063826)
When downgrading, a peers.dat with a future version that has a minimum
required version larger than the downgraded Bitcoin Core version would cause an InitError.
This commit changes this behavior to overwrite the existing peers.dat with
a new empty one.
ACKs for top commit:
prayank23:
reACK d41ed32153
kallewoof:
reACK d41ed32153
Tree-SHA512: c8e625fe36ce0b1aab6c8ef7241c8954038bb856f2de27bdc4814dc9a60e51be28815c7d77d0f96eace49687a0cea02deb713978bbd3a5add742f50a675f2a40
fixes#24188
When downgrading, a peers.dat with a future version that has a minimum
required version larger than the downgraded version would cause an InitError.
This commit changes this behavior to overwrite the existing peers.dat with
a new empty one, while creating a backup in peers.dat.bak.
fad7ddf9e3 test: Run symlink regression tests on Windows (MarcoFalke)
Pull request description:
Seems odd to add tests, but not run them on the platform that needs them most.
ACKs for top commit:
laanwj:
Code review ACK fad7ddf9e3
ryanofsky:
Code review ACK fad7ddf9e3, just removing new test. Would be nice if the test could be added later, of course.
Tree-SHA512: 64b235967a38c2eb90657e8d7a0447bcc8ce81d1b75a275b6c48bd42efd9ea7e7939257e484f297ee84598def3738eaeb289561aeba1dd6a99b258d389995139
Given the low possibility of a reorg reverting the segwit soft fork,
there is no need to check whether segwit is active here. Also,
TestBlockValidity is run on the block template after it has been
created.
48742693ac Replace "can not" with "cannot" in docs, user messages, and tests (Jon Atack)
e670edd434 User-facing content fixups from transifex translator feedback (Jon Atack)
Pull request description:
Closes#24366.
ACKs for top commit:
laanwj:
Code review re-ACK 48742693ac
hebasto:
re-ACK 48742693ac, only suggested change since my previous [review](https://github.com/bitcoin/bitcoin/pull/24367#pullrequestreview-885938219).
Tree-SHA512: 4dcdcb417251a413e65fab6070515e13a1267c8e0dbcf521386b842511391f24c84a0c2168fe13458c977682034466509bf2a3453719d4d94d3c568fd9f4adb4
For testing the USDT tracepoint API in the functional tests we
require:
- that we are on a Linux system*
- that Bitcoin Core is compiled with tracepoints
- that bcc and the the Python bcc module [0] is installed
- that we run the tests with the required permissions**
otherwise we skip the tests.
*: We currently only support tracepoints on Linux. Tracepoints are
not compiled on other platforms.
**: Currently, we check for root permissions via getuid == 0. It's
unclear if it's even possible to run the tests a non-root user
with e.g. CAP_BPF, CAP_PERFMON, and access to /sys/kernel/debug/
tracing/. Anyone running these tests as root should carefully
review them first and then run them in a disposable VM.
[0]: https://github.com/iovisor/bcc/blob/master/INSTALL.md
bfcd60f5d5 test: activate all index types in feature_init.py (Martin Zumsande)
0243907fae index: Don't commit without valid m_best_block_index (Martin Zumsande)
Pull request description:
When an index thread receives an interrupt during init before it got to index anything (so `m_best_block_index == nullptr` still), it will still try to commit previous "work" before stopping the thread. That means that `BaseIndex::CommitInternal()` calls `GetLocator(nullptr)`, which returns an locator to the tip ([code](06b6369766/src/chain.cpp (L31-L32))), and saves it to the index DB.
On the next startup, this locator will be read and it will be assumed that we have successfully synced the index to the tip, when in reality we have indexed nothing.
In the case of coinstatsindex, this would lead to a shutdown of bitcoind without any indication what went wrong. For the other indexes, there would be no immediate shutdown, but the index would be corrupt.
This PR fixes this by not committing when `m_best_block_index==nullptr`, and it also adds an error log message to the silent coinstatsindex shutdown path.
This is another small bug found by `feature_init.py` - the second commit enables blockfilterindex and coinstatsindex for this test, enabling coinstatsindex without the first commit would have led to frequent failures.
ACKs for top commit:
fjahr:
reACK bfcd60f5d5
shaavan:
reACK bfcd60f5d5
Tree-SHA512: 8e2bac0fc40cde209518a9e59b597ae0a5a875a2a90898673987c91733718d40e528dada942bf552b58bc021bf46e59da2d0cc5a61045f48f9bae2b1baf6033b
f11dad22a5 test: refactor: remove unneeded bytes<->hex conversions in `byte_to_base58` (Sebastian Falbesoner)
Pull request description:
It seems like the only reason for using hex strings in this method was to have a convenient way to convert to an integer from the input data interpreted as big-endian. In Python3 we have `int.from_bytes(..., 'big')` for that purpose, hence there is no need for that anymore and we can simply operate on bytes only.
ACKs for top commit:
laanwj:
Code review ACK f11dad22a5
Tree-SHA512: 9b1563010066ca74d85139c3b9259e9a5bb49e1f141c30b6506a0445afddb2bde7fd421fdd917dc516956e66f93610e2c21d720817640daee8f57f803be76ee4
It seems like the only reason for using hex strings in this
method was to have a convenient way to convert to an integer
from the input data interpreted as big-endian.
In Python3 we have `int.from_bytes(..., 'big')` for that
purpose, hence there is no need for that anymore and we can
simply operate on bytes only.
d1fab9d5d2 test: Call ceildiv helper with integer (Martin Zumsande)
Pull request description:
On master,
`assert_fee_amount(Decimal("0.00000993"), 217, Decimal("0.00004531"))` passes
`assert_fee_amount(Decimal("0.00000993"), Decimal("217"), Decimal("0.00004531"))` fails.
the reason is that the // operator in `ceildiv(a,b) = -(-a//b)` has a different behavior for Decimals, see [doc](https://docs.python.org/3/library/decimal.html#decimal-objects).
`wallet_send.py` calls this function with Decimals, and I think this is the reason for the failure reported in the OP of #24151 (`wallet_send.py --legacy-wallet` line 332, the numbers used in the example above are from there). However, the other failures reported there cannot be explained by this, so this is just a partial fix.
ACKs for top commit:
ryanofsky:
Code review ACK d1fab9d5d2. Tracking down this problem was a good find, and code seems safer and easier to understand now
Tree-SHA512: 5bf0568cd1a0824f6b1a15a03580b6e9391b4f51112a97c1d00469d255bf6dda45c49a36fa567a5ba9b9973efe1d9cdd480db91965c9f4c2aa963629a8a32cba
a036358994 test: Repair failfast option for test runner (Martin Zumsande)
Pull request description:
Fixes#23990
After #23799, the `--failfast` option in the test runner for the functional tests stopped working, because a second outer loop was introduced, which would have needed a `break` too for the test runner to fail immediately. This also led to the errors reported in #23990.
This provides a straightforward fix for that.
There is also #23995 which is a larger refactor, but that hasn't been updated in a while to fix the failfast issue.
ACKs for top commit:
pg156:
Tested ACK a036358994. I agree adding the `all_passed` flag to break out of the outer loop when needed makes sense. The "failfast" option works after this change.
Tree-SHA512: 3e2f775e36c13d180d32a05cd1cfe0883274e8615cdbbd4e069a9899e9b9ea1091066cf085e93f1c5326bd8ecc6ff524e0dad7c638f60dfdb169fefcdb26ee52
Warning: Replacing fs::system_complete calls with fs::absolute calls
in this commit may cause minor changes in behaviour because fs::absolute
no longer strips trailing slashes; however these changes are believed to
be safe.
Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
438e6f4c33 test: speedup interface_rest.py by whitelisting peers (immediate tx relay) (Sebastian Falbesoner)
11b9684dfd test: use MiniWallet for rest_interface.py (Sebastian Falbesoner)
Pull request description:
This PR enables one more of the non-wallet functional tests (interface_rest.py) to be run even with the Bitcoin Core wallet disabled by using the MiniWallet instead, as proposed in #20078.
Note that the original test sent funds from one node to another and checked node's balances, but the state of a node's wallet is not relevant to any of the REST endpoints, i.e. the replacement is quite stright-forward. In an additional commit, the test is further sped up by using the good ol' immediate tx relay trick (parameter `-whitelist=noban@127.0.0.1`).
ACKs for top commit:
brunoerg:
ACK 438e6f4c33
Tree-SHA512: eac351c5fb7e043c36c193d51545f20f563be9aaa04f3429a2bfb452ae4aa72294d2552800d6cac55c9a3ec2b4f30bcda2abcd74736dec3ed75e7d83c5af437f
3ee6d0788e test: add more wallet conflicts assertions (S3RK)
3b98bf9c43 Revert "Add to spends only transcations from me" (S3RK)
Pull request description:
This reverts commit d04566415e from #22929.
This commit was based on invalid assumption that `mapTxSpends` should contain only outgoing txs and broke wallet conflicts feature.
ACKs for top commit:
achow101:
ACK 3ee6d0788e
Tree-SHA512: bf5a77ced6bac57d5eb85771d9189c53e1edc295d179ed5a1bdce18e365794a9101b4cecf35387b27f67260db3b47f7214e7876e490494529b748cceeb95632d
