4be81e9746e9e18923386d6f4945a33885fd98a7 feature_taproot: sample tx version border values more (Greg Sanders)
Pull request description:
Currently if the version 3 is selected for an otherwise standard spender, the test will fail. It's unlikely but possible, so change the test to update expectations and sample more aggressively on border values to instigate failures much quicker in the future if another version is made standard.
ACKs for top commit:
maflcko:
lgtm ACK 4be81e9746e9e18923386d6f4945a33885fd98a7
darosior:
ACK 4be81e9746e9e18923386d6f4945a33885fd98a7
Tree-SHA512: 53267a201aaa495bea9d624930a19e40af3633427b6715965f43b9e1a060b2c9f19c8b10c8168778349fa50715e44cb8e5e9d2ce477d5f324ca8ed28ff7996cd
941b8f54c0d35d3243bb6083f3b52681d1b9a555 ci: run get_previous_releases as part of test cross win job (Max Edwards)
5e2182140bcd26afbe89b13d1d83ece6d5a89731 test: increment mocked time for migrating wallet backups (Max Edwards)
5174565802f426db1e9c8200a9421f82f61a5c99 ci: disable feature_unsupported_utxo_db functional test (Max Edwards)
3dc90d69a64f8bb39af27fa755683589b1bc76a7 test: remove mempool.dat before copying (Max Edwards)
67a6b20d5030ee51bf6d5e0fd77c9bdc8bafa96b test: add windows support to get previous releases script (Max Edwards)
1a1b478ca31be1f670754a47da17863271e46b7b scripted-diff: rename tarball to archive (Max Edwards)
4f06dc848460c887ad8337702ed900ba78725906 test: remove building from source from get prev releases script (Max Edwards)
Pull request description:
This PR updates the `test/get_previous_releases.py` script to also work on Windows by changing to be pure python rather than using unix tools such as `curl` and `tar`.
This enables additional functional tests to run such as `wallet_migration.py`, `mempool_compatability.py` and `wallet_backwards_compatibility.py`.
Unfortunately `feature_unsupported_utxo_db.py` _could_ run but this test requires Bitcoin `v0.14.3` which will not run under windows with emojis in the data directory (as the functional test runner has by default) . This test could be run as it's own step in the ci workflow file and would pass but as it's quite an old version / feature I have assumed it's not worth worrying about and best just to exclude.
Two tests needed to be slightly modified to run under windows. Both were issues with trying to overwrite a file that already exists which windows seems to be more strict on than the unix based systems.
Finally, building from source has been dropped from the `get_previous_releases.py` script. This had not been updated after the move to cmake and so it was assumed that nobody could have been using that feature.
ACKs for top commit:
maflcko:
re-ACK 941b8f54c0d35d3243bb6083f3b52681d1b9a555 🍪
achow101:
ACK 941b8f54c0d35d3243bb6083f3b52681d1b9a555
hodlinator:
re-ACK 941b8f54c0d35d3243bb6083f3b52681d1b9a555
Tree-SHA512: 22933d0ec278b9b0ffcd2a8e90026e1a3631b00186e7f78bd65be925049021e319367d488c36a82ab526a07b264bac18c2777f87ca1174b231ed49fed56d11cb
215e5999e2070a38c68e343c5c3f1dc37d567f58 wallet: Remove unused CachedTxGet{Available,Immature}Credit (Ava Chow)
49675de035e7c668d6857a32d929b7e1e85e83e3 wallet: Have GetDebit use the wallet's TXO set (Ava Chow)
17d453cb3a6fdbb0ebc8538c228c89712c989499 wallet: Recompute wallet TXOs after descriptor migration (Ava Chow)
764016eb2259676d834cf6829f5b0e04b135d407 wallet: Retrieve TXO directly in FetchSelectedInputs (Ava Chow)
c1801b78f1c11e596378a44458a484b6a9de71d8 wallet: Use wallet's TXO set in AvailableCoins (Ava Chow)
dde7cbe105ba6daaa636466d0fa3a83d15609417 wallet: Change balance calculation to use m_txos (Ava Chow)
96e7a89c5e0bf5e1a4497d8296e8841edd7ebbf1 wallet: Recalculate the wallet's txos after any imports (Ava Chow)
ae888c38d080c9c41925faeb53044e5bd0b7f9ee wallet: Exit IsTrustedTx early if wtx is already in trusted_parents (Ava Chow)
ae0876ec4273a8b978e2c602435a9ed25f48c976 wallet: Keep track of transaction outputs owned by the wallet (Ava Chow)
0f269bc48c3905c0782c9d175ad487b27ebaf54b walletdb: Load Txs last (Ava Chow)
5cc32ee2a7addb38ae4a4c97d306d0c5d9cc2d5e test: Test for balance update due to untracked output becoming spendable (Ava Chow)
8222341d4f9c2b7c572b27dea16036d6ea372067 wallet: MarkDirty after AddWalletDescriptor (Ava Chow)
e02f2d331ce6d9c5b6b8f62a79dd40695d2283a2 bench: Have AvailableCoins benchmark include a lot of unrelated utxos (Ava Chow)
Pull request description:
Currently, the wallet is not actually aware about its own transaction outputs. Instead, it will iterate all of the transactions stored in `mapWallet`, and then all of the outputs of those transactions, in order to figure out what belongs to it for the purposes of coin selection and balance calculation. For balance calculation, there is caching that results in it only iterating all of the transactions, but not all of the outputs. However when the cache is dirty, everything is iterated. This is especially problematic for wallets that have a lot of transactions, or transactions that have a lot of unrelated outputs (as may occur with coinjoins or batched payments).
This PR helps to resolve this issue by making the wallet track all of the outputs that belong to it in a new member `m_txos`. Note that this includes outputs that may have already been spent. Both balance calculation (`GetBalance`) and coin selection (`AvailableCoins`) are updated to iterate `m_txos`. This is generally faster since it ignores all of the unrelated outputs, and it is not slower as in the worst case of wallets containing only single output transactions, it's exactly the same number of outputs iterated.
`m_txos` is memory only, and it is populated during wallet loading. When each transaction is loaded, all of its outputs are checked to see if it is `IsMine`, and if so, an entry added to `m_txos`. When new transactions are received, the same procedure is done.
Since imports can change the `IsMine` status of a transaction (although they can only be "promoted" from watchonly to spendable), all of the import RPCs will be a bit slower as they re-iterate all transactions and all outputs to update `m_txos`.
Each output in `m_txos` is stored in a new `WalletTXO` class. This class contains references to the parent `CWalletTx` and the `CTxOut` itself. It also caches the `IsMine` value of the txout. This should be safe as `IsMine` should not change unless there are imports. This allows us to have additional performance improvements in places that use these `WalletTXO`s as they can use the cached `IsMine` rather than repeatedly calling `IsMine` which can be expensive.
The existing `WalletBalance` benchmark demonstrates the performance improvement that this PR makes. The existing `WalletAvailableCoins` benchmark doesn't as all of the outputs used in that benchmark belong to the test wallet. I've updated that benchmark to have a bunch of unrelated outputs in each transaction so that the difference is demonstrated.
This is part of a larger project to have the wallet actually track and store a set of its UTXOs.
Built on #24914 as it requires loading the txs last in order for `m_txos` to be built correctly.
***
## Benchmarks:
Master:
| ns/op | op/s | err% | ins/op | cyc/op | IPC | bra/op | miss% | total | benchmark
|--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:----------
| 34,590,013.00 | 28.91 | 0.0% | 812,669,269.00 | 148,360,642.50 | 5.478 | 18,356,853.00 | 0.2% | 0.76 | `WalletAvailableCoins`
| 3,193.46 | 313,139.91 | 0.4% | 96,868.06 | 13,731.82 | 7.054 | 26,238.01 | 0.1% | 0.01 | `WalletBalanceClean`
| 26,871.18 | 37,214.59 | 3.3% | 768,179.50 | 115,544.39 | 6.648 | 154,171.09 | 0.1% | 0.01 | `WalletBalanceDirty`
| 3,177.30 | 314,732.47 | 0.2% | 96,868.06 | 13,646.20 | 7.099 | 26,238.01 | 0.1% | 0.01 | `WalletBalanceMine`
| 10.73 | 93,186,952.53 | 0.1% | 157.00 | 46.14 | 3.403 | 36.00 | 0.0% | 0.01 | `WalletBalanceWatch`
| 590,497,920.00 | 1.69 | 0.1% |12,761,692,005.00 |2,536,899,595.00 | 5.030 | 129,124,399.00 | 0.7% | 6.50 | `WalletCreateEncrypted`
| 182,929,529.00 | 5.47 | 0.0% |4,199,271,397.00 | 785,477,302.00 | 5.346 | 75,363,377.00 | 1.1% | 2.01 | `WalletCreatePlain`
| 699,337.20 | 1,429.93 | 0.7% | 18,054,294.00 | 3,005,072.20 | 6.008 | 387,756.60 | 0.3% | 0.04 | `WalletCreateTxUseOnlyPresetInputs`
| 32,068,583.80 | 31.18 | 0.5% | 562,026,110.00 | 137,457,635.60 | 4.089 | 90,667,459.40 | 0.3% | 1.78 | `WalletCreateTxUsePresetInputsAndCoinSelection`
| 36.62 | 27,306,578.40 | 0.5% | 951.00 | 157.05 | 6.056 | 133.00 | 0.0% | 0.01 | `WalletIsMineDescriptors`
| 35.00 | 28,569,989.42 | 0.7% | 937.00 | 150.33 | 6.233 | 129.00 | 0.0% | 0.01 | `WalletIsMineMigratedDescriptors`
| 203,284,889.00 | 4.92 | 0.0% |4,622,691,895.00 | 872,875,275.00 | 5.296 | 90,345,002.00 | 1.2% | 1.02 | `WalletLoadingDescriptors`
| 1,165,766,084.00 | 0.86 | 0.0% |24,139,316,211.00 |5,005,218,705.00 | 4.823 |2,664,455,775.00 | 0.1% | 1.17 | `WalletMigration`
PR:
| ns/op | op/s | err% | ins/op | cyc/op | IPC | bra/op | miss% | total | benchmark
|--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:----------
| 33,975,750.50 | 29.43 | 0.1% | 794,719,150.50 | 145,763,550.00 | 5.452 | 16,036,630.50 | 0.2% | 0.75 | `WalletAvailableCoins`
| 2,442.01 | 409,498.46 | 0.2% | 60,782.04 | 10,500.60 | 5.788 | 9,492.01 | 0.3% | 0.01 | `WalletBalanceClean`
| 2,763.12 | 361,909.21 | 0.2% | 61,493.05 | 11,859.48 | 5.185 | 9,625.01 | 0.2% | 0.01 | `WalletBalanceDirty`
| 2,347.98 | 425,898.72 | 0.3% | 60,782.04 | 10,082.73 | 6.028 | 9,492.01 | 0.2% | 0.01 | `WalletBalanceMine`
| 11.67 | 85,654,630.36 | 0.2% | 176.00 | 50.18 | 3.508 | 40.00 | 0.0% | 0.01 | `WalletBalanceWatch`
| 590,119,519.00 | 1.69 | 0.1% |12,754,398,258.00 |2,534,998,522.00 | 5.031 | 129,078,027.00 | 0.7% | 6.50 | `WalletCreateEncrypted`
| 183,124,790.00 | 5.46 | 0.1% |4,199,212,926.00 | 786,323,886.00 | 5.340 | 75,354,437.00 | 1.1% | 2.02 | `WalletCreatePlain`
| 669,643.00 | 1,493.33 | 0.1% | 17,213,904.20 | 2,877,336.40 | 5.983 | 394,292.80 | 0.3% | 0.04 | `WalletCreateTxUseOnlyPresetInputs`
| 26,205,987.80 | 38.16 | 0.8% | 365,551,340.80 | 112,376,905.20 | 3.253 | 65,684,276.20 | 0.4% | 1.44 | `WalletCreateTxUsePresetInputsAndCoinSelection`
| 34.75 | 28,778,846.38 | 0.1% | 937.00 | 149.41 | 6.271 | 129.00 | 0.0% | 0.01 | `WalletIsMineDescriptors`
| 29.91 | 33,428,072.85 | 0.2% | 920.00 | 128.63 | 7.152 | 126.00 | 0.0% | 0.01 | `WalletIsMineMigratedDescriptors`
| 202,437,985.00 | 4.94 | 0.1% |4,626,686,256.00 | 869,439,274.00 | 5.321 | 90,961,305.00 | 1.1% | 1.02 | `WalletLoadingDescriptors`
| 1,158,394,152.00 | 0.86 | 0.0% |24,143,589,972.00 |4,971,946,380.00 | 4.856 |2,665,355,654.00 | 0.1% | 1.16 | `WalletMigration`
ACKs for top commit:
davidgumberg:
untested reACK 215e599
murchandamus:
reACK 215e5999e20
ishaanam:
reACK 215e5999e2070a38c68e343c5c3f1dc37d567f58
w0xlt:
reACK 215e5999e2
Tree-SHA512: d6b929de56f67930678db654e46f15fb71008390189c701a026b2d76af8f14a7c9769e49835ce7e2b6515d2934a77aad8de0b1a82231a2e1de5337de25db9629
Currently if the version 3 is selected for an otherwise
standard spender, the test will fail. It's unlikely but
possible, so change the test to update expectations and
sample more aggressively on border values to instigate
failures much quicker in the future if another version is
made standard.
fa2163159511a099ecffde2bfddf9cfe33eb9c76 test: Use self.log (MarcoFalke)
fa346f7797ae9fd3ff0e874ea96416c3fe2b4ba5 test: Move error string into exception (MarcoFalke)
fa1986181f246d75b1fcc814353377eeb2256fa0 test: Remove useless catch-throw (MarcoFalke)
fa2f1c55b7da729eb6bb9f88bf48459235cc2664 move-only util data to test/functional/data/util (MarcoFalke)
faa18bf287fc02339b7af29d54af862a289bf96d test: Turn util/test_runner into functional test (MarcoFalke)
fa955154c773c5129f9594982343b440d05957e2 test: Add missing skip_if_no_bitcoin_tx (MarcoFalke)
fac9db6eb0c64333cd202e1053a4dd5fd27344f1 test: Add missing tx util to Binaries (MarcoFalke)
fa91835ec6ad723dc4a379a900ae9331e07a0fba test: Use lowercase env var as attribute name (MarcoFalke)
fac49094cdb12bd566df5a00832462491a839f81 test: Remove duplicate ConfigParser (MarcoFalke)
Pull request description:
The `test/util/test_runner.py` has many issues:
* The boilerplate for the test runner is duplicate or inconsistent with the other (functional) tests. For example, logging options, `ConfigParser` handling, `Binaries` handling ...
* The cmake/ci behavior is brittle and can silently fail, as explained in https://github.com/bitcoin/bitcoin/issues/31476
* corecheck (and likely other places that manually run the tests) completely forget to run it
* If the test is manually called, it runs single threaded, when it could just run in parallel with the other functional tests
Fix all issues by removing the util test_runner and moving the test logic into a new functional test file.
ACKs for top commit:
janb84:
re ACK fa2163159511a099ecffde2bfddf9cfe33eb9c76
brunoerg:
re-ACK fa2163159511a099ecffde2bfddf9cfe33eb9c76
hebasto:
re-ACK fa2163159511a099ecffde2bfddf9cfe33eb9c76, additional feedback has been addressed since my previous [review](https://github.com/bitcoin/bitcoin/pull/32697#pullrequestreview-2940350432).
Tree-SHA512: 694e647887801f002843a74011035d5ed3dfed091d3f0ae18e812a16a4680e04e60e50de0a92af7e047e8ddd6ff5a7834c690f16fd42b74ebc1674bf9989406f
c48846ec4169f749d28da05de849c43a488c3a70 doc: add release notes for #32540 (Roman Zeyde)
d4e212e8a69ea118acb6caa1a7efe64a77bdfdd2 rest: fetch spent transaction outputs by blockhash (Roman Zeyde)
Pull request description:
Today, it is possible to fetch a block's spent prevouts in order to build an external index by using the `/rest/block/BLOCKHASH.json` endpoint. However, its performance is low due to JSON serialization overhead.
We can significantly optimize it by adding a new [REST API](https://github.com/bitcoin/bitcoin/blob/master/doc/REST-interface.md) endpoint, using a binary response format (returning a collection of spent txout lists, one per each block transaction):
```
$ BLOCKHASH=00000000000000000002a7c4c1e48d76c5a37902165a270156b7a8d72728a054
$ ab -k -c 1 -n 100 http://localhost:8332/rest/block/$BLOCKHASH.json
Document Length: 13278152 bytes
Requests per second: 3.53 [#/sec] (mean)
Time per request: 283.569 [ms] (mean)
$ ab -k -c 1 -n 10000 http://localhost:8332/rest/spenttxouts/$BLOCKHASH.bin
Document Length: 195591 bytes
Requests per second: 254.47 [#/sec] (mean)
Time per request: 3.930 [ms] (mean)
```
Currently, this PR is being used and tested by Bindex[^1].
This PR would allow to improve the performance of external indexers such as electrs[^2], ElectrumX[^3], Fulcrum[^4] and Blockbook[^5].
[^1]: https://github.com/romanz/bindex-rs
[^2]: https://github.com/romanz/electrs (also [blockstream.info](https://github.com/Blockstream/electrs) and [mempool.space](https://github.com/mempool/electrs) forks)
[^3]: https://github.com/spesmilo/electrumx
[^4]: https://github.com/cculianu/Fulcrum
[^5]: https://github.com/trezor/blockbook
ACKs for top commit:
maflcko:
re-ACK c48846ec4169f749d28da05de849c43a488c3a70 📶
TheCharlatan:
Re-ACK c48846ec4169f749d28da05de849c43a488c3a70
achow101:
ACK c48846ec4169f749d28da05de849c43a488c3a70
Tree-SHA512: cf423541be90d6615289760494ae849b7239b69427036db6cc528ac81df10900f514471d81a460125522c5ffa31e9747ddfca187a1f93151e4ae77fe773c6b7b
dd8447f70faf6419b4617da3c1b57098e9cd66a6 test: fix catchup loop in outbound eviction functional test (Sebastian Falbesoner)
Pull request description:
In the course of working on an equivalent of #32421 for the `CBlockHeader` class, I noticed that the [catchup loop in the outbound eviction functional test](19765dca19/test/functional/p2p_outbound_eviction.py (L86-L103)) currently has a small flaw: the contained waiting for a `getheaders` message
19765dca19/test/functional/p2p_outbound_eviction.py (L98-L99)
only waits for _any_ such message instead of one with the intended block hash after the first iteration. The reason is that the `prev_prev_hash` variable is set incorrectly, since the `tip_header` instance is not updated and its field `.hash` is None [1]. Fix that by updating `tip_header` after generating a new block and also use the correct field on it -- we want the tip header's previous hash (`.hashPrevBlock`), which will be the previous-previous hash in the next iteration as intended.
Can be demonstrated by adding a debug output for `prev_prev_hash`, e.g.
```diff
diff --git a/test/functional/p2p_outbound_eviction.py b/test/functional/p2p_outbound_eviction.py
index 30ac85e32f..9886a49512 100755
--- a/test/functional/p2p_outbound_eviction.py
+++ b/test/functional/p2p_outbound_eviction.py
@@ -85,6 +85,7 @@ class P2POutEvict(BitcoinTestFramework):
self.log.info("Keep catching up with the old tip and check that we are not evicted")
for i in range(10):
+ print(f"i={i}, prev_prev_hash={prev_prev_hash}")
# Generate an additional block so the peers is 2 blocks behind
prev_header = from_hex(CBlockHeader(), node.getblockheader(best_block_hash, False))
best_block_hash = self.generateblock(node, output="raw(42)", transactions=[])["hash"]
```
master branch
```
...
i=0, prev_prev_hash=21722572577213525620063947414919931742473663114977483853465070858884938201585
i=1, prev_prev_hash=None
i=2, prev_prev_hash=None
i=3, prev_prev_hash=None
i=4, prev_prev_hash=None
i=5, prev_prev_hash=None
i=6, prev_prev_hash=None
i=7, prev_prev_hash=None
i=8, prev_prev_hash=None
i=9, prev_prev_hash=None
...
```
PR branch
```
...
i=0, prev_prev_hash=21722572577213525620063947414919931742473663114977483853465070858884938201585
i=1, prev_prev_hash=23204083306104595181276643925327085197417756603258684897360269464191973063397
i=2, prev_prev_hash=18117007775254206852722585270408843074799046031613422902091537272077477361634
i=3, prev_prev_hash=30556804635951812756130312631227721973553160707632138130845362630877961299882
i=4, prev_prev_hash=16476515948153779819467376247405243058769281687868039119037064816106574626111
i=5, prev_prev_hash=14965506521435221774966695805624206855826023174786191695076697927307467053159
i=6, prev_prev_hash=14510815979277079515923749862202324542606166669768865640616202929053689167149
i=7, prev_prev_hash=15360268707191667685151951417759114642582372006627142890517655217275478262166
i=8, prev_prev_hash=55984929479619644661389829786223559362979512070332438490054115824374865094074
i=9, prev_prev_hash=6591573629906616262191232272909118561529534571119028248829355592878183757083
...
```
[1] that's in my opinion another example how caching hashes is confusing and easy to be misused; it's better to remove it and just compute the hash on-the-fly, so returning None is not even possible anymore
ACKs for top commit:
maflcko:
lgtm ACK dd8447f70faf6419b4617da3c1b57098e9cd66a6
mzumsande:
Code Review ACK dd8447f70faf6419b4617da3c1b57098e9cd66a6
pablomartin4btc:
cr-ACK dd8447f70faf6419b4617da3c1b57098e9cd66a6
Tree-SHA512: bd8e786b52e3e96661453006140d6b8fad5a35f1c8d38243c61df52b19c97cd3800404745a2f9603bcdf0006e9780b4f15f8f7e4fa34ff07d52dba04d87b68d0
c3fe85e2d6dd4f251a62a99fd891b0fa370f9712 wallet, rpc, test: Remove deprecated getunconfirmedbalance (Ava Chow)
0ec255139be3745a135386e9db957fe81bc3d833 wallet, rpc: Remove deprecated balances from getwalletinfo (Ava Chow)
Pull request description:
`getwalletinfo` result fields `balance`, `immature_balance`, and `unconfirmed_balance`, and the `getunconfirmedbalance` RPC have all been deprecated since 0.19.0. It's been long enough that they should either be removed or undeprecated. The functionality provided by these RPCs is provided by `getbalances`.
ACKs for top commit:
davidgumberg:
ACK c3fe85e2d6
rkrux:
ACK c3fe85e2d6dd4f251a62a99fd891b0fa370f9712
BrandonOdiwuor:
ACK c3fe85e2d6dd4f251a62a99fd891b0fa370f9712 removing the deprecated `balance, unconfirmed_balance, immature_balance` fields from `getwalletinfo` and `getunconfirmedbalance` RPCs, as this infomation can be found on the `getbalances` RPC
w0xlt:
reACK c3fe85e2d6
Tree-SHA512: c7c4acfd9cabc7517ba813b95281a6c6a717a417312afd9346298669b4f7bd37724ad977148ce42db7fd47fc3d1f5a8482d8ff2e7b9cb74756b171a5b8b91ef2
47237cd1938058b29fdec242c3a37611e255fda0 wallet, rpc: Output wallet flags in getwalletinfo (Ava Chow)
bc2a26b296238cbead6012c071bc7741c40fbd02 wallet: Add GetWalletFlags (Ava Chow)
69f588a99a7a79d1d72300bc0f2c8475f95f6c6a wallet: Set upgraded descriptor cache flag for newly created wallets (Ava Chow)
Pull request description:
Newly created wallets will always have an upgraded descriptor cache, so set those.
Also, to verify this behavior, add a new `flags` field to `getwalletinfo` and check that in the functional tests.
Split from #32489
ACKs for top commit:
Sjors:
ACK 47237cd1938058b29fdec242c3a37611e255fda0
w0xlt:
ACK 47237cd193
rkrux:
ACK 47237cd1938058b29fdec242c3a37611e255fda0
Tree-SHA512: 97c7f85b858efe5ced9b8aafb6cd7c1a547de6f8013b82bfc75bc567cf73c9db5e168e3980355756541305520022fd776b8d4d240d3fb34ed86c27d2acaf4863
The wallet backups performed before migration use the time as part of
their filename. As the time is mocked, increment it between migration
attempts to prevent file name conflicts which is a problem on Windows.
Using the get_previous_releases.py script to build from source only works for
releases prior to v29 due to removal of Autotools (in favor of CMake). It also
does not support building on Windows, and we are adding support for downloading
Windows release binaries in later commits of this PR.
As there were no complaints during review, it is assumed nobody uses this
functionality.
578ea3eedb285519762087a4b27d953d8f61667f test: round difficulty and networkhashps (Sjors Provoost)
Pull request description:
Both are rational numbers. Client software should only use them to display information to humans. Followup calculations should use the underlying values such as target.
Therefore it's not necessary to test the handling of these floating point values. Round them down to avoid spurious test failures.
Fixes#32515
ACKs for top commit:
Prabhat1308:
Code Review ACK [`578ea3e`](578ea3eedb)
achow101:
ACK 578ea3eedb285519762087a4b27d953d8f61667f
w0xlt:
Code review ACK 578ea3eedb
janb84:
ACK 578ea3eedb285519762087a4b27d953d8f61667f
Tree-SHA512: 5fc63c73ad236b7cd55c15da0f1d1e6b45e4289d252147a86717bf77d79f897f42c3e38aa514df6a4a8deca10c87a8710b61b454c533ad56b0daf738365f426c
b184f5c87c418ea49429e4ce0520c655d458306b test: update BIP340 test vectors and implementation (variable-length messages) (Sebastian Falbesoner)
Pull request description:
This PR updates the Schnorr signatures implementation in the functional test framework to the latest BIP changes (see https://github.com/bitcoin/bips/pull/1446,commit 200f9b26fe0a2f235a2af8b30c4be9f12f6bc9cb) and syncs the [test vectors](https://github.com/bitcoin/bips/blob/master/bip-0340/test-vectors.csv) accordingly. Practically, we probably don't need non-32-bytes message signing/verifying any time soon, but it seems good practice anyways to update.
ACKs for top commit:
stratospher:
ACK b184f5c.
achow101:
ACK b184f5c87c418ea49429e4ce0520c655d458306b
real-or-random:
utACK b184f5c87c418ea49429e4ce0520c655d458306b
jonasnick:
utACK b184f5c87c418ea49429e4ce0520c655d458306b
Tree-SHA512: b566823aa0f1cd7151215178c57551d772b338d022ccb2807a0df2670df6d59c4b63a6fc936708ccf2922c7e59f474f544adaafc4aea731bfd896250c0d45fa6
272cd09b796a36596b325277bb43cb47b19c8e12 log: Use warning level while scanning wallet dir (MarcoFalke)
17776443675ddf804f92042883ad36ed040438c3 qa, wallet: Verify warning when failing to scan (Hodlinator)
893e51ffeb0543e1c8d33e83b20c56f02d2b793c wallet: Correct dir iteration error handling (Hodlinator)
Pull request description:
Make wallet DB properly detect and report failure to scan wallet directory. Seems to have been broken since moving from Boost to `std::filesystem`.
Found while reviewing: https://github.com/bitcoin/bitcoin/pull/31410#pullrequestreview-2604068753
ACKs for top commit:
achow101:
ACK 272cd09b796a36596b325277bb43cb47b19c8e12
maflcko:
re-ACK 272cd09b796a36596b325277bb43cb47b19c8e12 🍽
rkrux:
tACK 272cd09b796a36596b325277bb43cb47b19c8e12
Tree-SHA512: 969afde2e37f885ed0c823dc36d2dbeaa0378639849c6a26f8ac67b4f1997eea95bbcae6d58aef5b716807210f37eb166c0cda7ba1d6caffd34249970833af3a
e285e691b7a311e278f89e9fe423716de1ee268b test: Fix list index out of range error in feature_bip68_sequence.py (zaidmstrr)
Pull request description:
Fixes [#32334](https://github.com/bitcoin/bitcoin/issues/32334)
The test `feature_bip68_sequence.py` fails with `IndexError: list index out of range` error due to a mismatch between the number of inputs requested (at random) and the number of UTXOs available. The error is reproducible with the randomseed:
```
$ ./build/test/functional/feature_bip68_sequence.py --randomseed 6169832640268785903
```
This PR adds a valid upper bound to randomly select the inputs.
ACKs for top commit:
maflcko:
lgtm ACK e285e691b7a311e278f89e9fe423716de1ee268b
Prabhat1308:
re-ACK [`e285e69`](e285e691b7)
theStack:
ACK e285e691b7a311e278f89e9fe423716de1ee268b
Tree-SHA512: 2e5e19d5db2880915f556ed4444abed94e9ceb1ecee5f857df5616040c850dae682aaa4ade3060c48acb16676df92ba81c3af078c1958965e9e874e7bb489388
a18e57232867d946bc35769632fed49e1bf1464f test: more template verification tests (Sjors Provoost)
10c908808fb80cd4fbde9d377079951b91944755 test: move gbt proposal mode tests to new file (Sjors Provoost)
94959b8deedcff98a55c87b5e473890b2e7a3b16 Add checkBlock to Mining interface (Sjors Provoost)
6077157531c1cec6dea8e6f90b4df8ef7b5cec4e ipc: drop BlockValidationState special handling (Sjors Provoost)
74690f4ed82b1584abb07c0387db0d924c4c0cab validation: refactor TestBlockValidity (Sjors Provoost)
Pull request description:
This PR adds the IPC equivalent of the `getblocktemplate` RPC in `proposal` mode.
In order to do so it has `TestBlockValidity` return error reasons as a string instead of `BlockValidationState`. This avoids complexity in IPC code for handling the latter struct.
The new Mining interface method is used in `miner_tests`.
It's not used by the `getblocktemplate` and `generateblock` RPC calls, see https://github.com/bitcoin/bitcoin/pull/31981#discussion_r2096473337
The `inconclusive-not-best-prevblk` check is moved from RPC
code to `TestBlockValidity`.
Test coverage is increased by `mining_template_verification.py`.
Superseedes #31564
## Background
### Verifying block templates (no PoW)
Stratum v2 allows miners to generate their own block template. Pools may wish (or need) to verify these templates. This typically involves comparing mempools, asking miners to providing missing transactions and then reconstructing the proposed block.[^0] This is not sufficient to ensure a proposed block is actually valid. In some schemes miners could take advantage of incomplete validation[^1].
The Stratum Reference Implementation (SRI), currently the only Stratum v2 implementation, collects all missing mempool transactions, but does not yet fully verify the block.[^2]. It could use the `getblocktemplate` RPC in `proposal` mode, but using IPC is more performant, as it avoids serialising up to 4 MB of transaction data as JSON.
(although SRI could use this PR, the Template Provider role doesn't need it, so this is _not_ part of #31098)
[^0]: https://github.com/stratum-mining/sv2-spec/blob/main/06-Job-Declaration-Protocol.md
[^1]: https://delvingbitcoin.org/t/pplns-with-job-declaration/1099/45?u=sjors
[^2]: https://github.com/stratum-mining/stratum/blob/v1.1.0/roles/jd-server/src/lib/job_declarator/message_handler.rs#L196
ACKs for top commit:
davidgumberg:
reACK a18e572328
achow101:
ACK a18e57232867d946bc35769632fed49e1bf1464f
TheCharlatan:
ACK a18e57232867d946bc35769632fed49e1bf1464f
ryanofsky:
Code review ACK a18e57232867d946bc35769632fed49e1bf1464f just adding another NONFATAL_UNREACHABLE since last review
Tree-SHA512: 1a6c29f45a1666114f10f55aed155980b90104db27761c78aada4727ce3129e6ae7a522d90a56314bd767bd7944dfa46e85fb9f714370fc83e6a585be7b044f1
9dfc61d95f0082672a9b90528386e6bcd7014a78 test: detect no external signer connected (Sjors Provoost)
0a4ee93529d68a31f3ba6c7c6009954be47bbbd6 wallet: use PSBTError::EXTERNAL_SIGNER_NOT_FOUND (Sjors Provoost)
8ba2f9b7c8a6c6a91cc718d256354f7a73083b68 refactor: use util::Result for GetExternalSigner() (Sjors Provoost)
Pull request description:
When attempting to sign a transaction involving an external signer, if the device isn't connected we throw an `std::runtime_error`. This prevents the (mainly GUI) code that's actually supposed to handle this case from running.
This PR returns a `PSBTError::EXTERNAL_SIGNER_NOT_FOUND` instead of throwing.
The first commit is a refactor to have `GetExternalSigner()` return a `util::Result<ExternalSigner>` so the caller can decide how to handle the error. There are two other places where call `GetExternalSigner()` which this PR doesn't change (which I think is fine there).
Before:
After (the translation already exist):
Fixes#32426
Additionally use `LogWarning` instead of `std::cerr` for both a missing signer and failure to sign.
ACKs for top commit:
achow101:
ACK 9dfc61d95f0082672a9b90528386e6bcd7014a78
brunoerg:
code review ACK 9dfc61d95f0082672a9b90528386e6bcd7014a78
Tree-SHA512: 22515f4f0b4f50cb0ef532b729e247f11a68be9c90e384942d4277087b2e76806a1cdaa57fb51d5883dacf0a428e5279674aab37cce8c0d3d7de0f96346b8233
Additionally this commit gives each test its
own function.
The assert_submitblock helper is absorbed into
assert_template.
Review hint:
git show --color-moved=dimmed-zebra
0def84d407facd319b52826d013cad0d5fc8dbf5 test: Verify parent_desc in RPCs (Ava Chow)
2554cee988fb2ddf65428b354a238f1a4efc1aca test: Enable default wallet for wallet_descriptor.py (Ava Chow)
3fc9d9f241a44ab64774aa9ddc3ded4bb589ed5a wallet, rpc: Push the normalized parent descriptor (Ava Chow)
Pull request description:
Instead of prividing the descriptor string as stored in the db, use the normalized descriptor as is done for getaddressinfo's parent_desc field.
Split from #32489
ACKs for top commit:
Sjors:
re-utACK 0def84d407
rkrux:
ACK 0def84d407facd319b52826d013cad0d5fc8dbf5
w0xlt:
reACK 0def84d407
Tree-SHA512: 575c5b545d6f0aa7e135696b7a55c004e754fca4dd35dd9cf71b0b45b49a2e86e7b20570e768534d587005953bb893645379ec1ba4f98cfd26811f9c2f17de2d
The catchup loop in the outbound eviction functional test currently has
a small flaw, as the contained waiting for a `getheaders` message just
waits for any such message instead of one with the intended block hash.
The reason is that the `prev_prev_hash` variable is set incorrectly,
since the `tip_header` instance is not updated and its field `.hash` is
None. Fix that by updating `tip_header` and use the correct field -- we
want the tip header's previous hash (`.hashPrevBlock`).
4ef6253017672a74c584e6e9b449ffff79f67273 test: avoid unneeded (w)txid hex -> integer conversions (Sebastian Falbesoner)
472f3770aec89bf0783609094108ba277a46d339 scripted-diff: test: rename CTransaction `.getwtxid()` -> `wtxid_hex` for consistency (Sebastian Falbesoner)
81af4334e8f903601dac503426eaccc1d3527f9c test: rename CTransaction `.sha256` -> `.txid_int` for consistency (Sebastian Falbesoner)
ce83924237127fe6d32b63c362b57789e4628954 test: rename CTransaction `.rehash()`/`.hash` -> `.txid_hex` for consistency (Sebastian Falbesoner)
e9cdaefb0a80988cfba2dc5ddd96b84a204d5519 test: introduce and use CTransaction `.wtxid_int` property (Sebastian Falbesoner)
9b3dce24a3336a02563412b541a3fb2003e92506 test: remove bare CTransaction `.rehash()`/`.calc_sha256()` calls (Sebastian Falbesoner)
a2724e3ea392cdbd5526735516862b17bf687624 test: remove txid caching in CTransaction class (Sebastian Falbesoner)
Pull request description:
In the functional test framework, determining a (w)txid for a `CTransaction` instance is currently rather confusing and footgunny due to inconsistent naming/interfaces (see table below) and statefulness involved. This PR aims to improve that by:
* removing the (w)txid caching mechanism, in order to avoid the need to call additional rehashing functions (`.rehash()`/`.calculate_sha256()`, see first two commits and https://github.com/bitcoin/bitcoin/pull/32050#discussion_r1993286997). This change in theory decreases the performance, as the involved serialization and hashing involved might be called more often than previously, but I couldn't find a functional test where this leads to a measurable run-time increase on my machine.
* introduce consistent naming that shows the type of the returned txid, i.e. hex string vs. test-framework-internal representation [currently integers] (see remaining commits)
Summary table showing (w)txid determaination before/after this PR:
| Task | master | PR |
|:-----------------------|:-----------------------|:-------------|
| get TXID (hex string) | `.rehash()` / `.hash`[1] | `.txid_hex` |
| get TXID (integer) | `.sha256`[1] | `.txid_int` |
| get WTXID (hex string) | `.getwtxid()` | `.wtxid_hex` |
| get WTXID (integer) | `.calc_sha256(True)` | `.wtxid_int` |
Unfortunately, most renames can't be done with a scripted-diff, as the property names (`.hash`, `.sha256`) are also used for blocks and other message types. The PR is rather invasive and touches a lot of files, but I think it's worth to do it, also to make life easier for new contributors. Future tasks like e.g. doing the same overhaul for block (header) objects or getting rid of the integer representation (see https://github.com/bitcoin/bitcoin/pull/32050) become easier should become easier after this one.
[1] = returned value might be out-of-date, if rehashing function wasn't called after modification
ACKs for top commit:
maflcko:
re-ACK 4ef6253017672a74c584e6e9b449ffff79f67273 🏈
achow101:
ACK 4ef6253017672a74c584e6e9b449ffff79f67273
marcofleon:
code review ACK 4ef6253017672a74c584e6e9b449ffff79f67273
Tree-SHA512: 4b472c31d169966b6f6878911a8404d25bf3e503b6e8ef30f36a7415d21ad4bc1265083af2d3ead6edfcd9fac9ccb0a8be57e1b0739ad431b836413070d7d583
Both are rational numbers. Client software should only use them to
display information to humans. Followup calculations should use the
underlying values such as target.
Therefore it's not necessary to test the handling of these floating
point values. Round them down to avoid spurious test failures.
Fixes#32515
Rather than determining a CTransaction's (w)txid as an integer by
converting it's hex value, it can be directly accessed via the
introduced `.{w,}txid_int` property.
e98c51fcce9ae3f441a416cab32a5c85756c6c64 doc: update tor.md to mention the new -proxy=addr:port=tor (Vasil Dimov)
ca5781e23a8f299ff4f143d2355218f551e65944 config: allow setting -proxy per network (Vasil Dimov)
Pull request description:
`-proxy=addr:port` specifies the proxy for all networks (except I2P). Previously only the Tor proxy could have been specified separately via `-onion=addr:port`.
Make it possible to specify separately the proxy for IPv4, IPv6, Tor and CJDNS by e.g. `-proxy=addr:port=ipv6`. Or remove the proxy for a given network, e.g. `-proxy=0=cjdns`.
Resolves: https://github.com/bitcoin/bitcoin/issues/24450
ACKs for top commit:
pinheadmz:
ACK e98c51fcce9ae3f441a416cab32a5c85756c6c64
caesrcd:
reACK e98c51fcce
danielabrozzoni:
Code Review ACK e98c51fcce9ae3f441a416cab32a5c85756c6c64
1440000bytes:
ACK e98c51fcce
Tree-SHA512: 0cb590cb72b9393cc36357e8bd7861514ec4c5bc044a154e59601420b1fd6240f336ab538ed138bc769fca3d17e03725d56de382666420dc0787895d5bfec131
f16c8c67bf137e64fbeea1242431baaa915a5c53 tests: Expand HTTP coverage to assert libevent behavior (Matthew Zipkin)
Pull request description:
These commits are cherry-picked from #32061 and part of a project to [remove libevent](https://github.com/bitcoin/bitcoin/issues/31194).
This PR only adds functional tests to `interface_http` to cover some HTTP server behaviors we inherit from libevent, in order to maintain those behaviors when we replace libevent with our own HTTP server.
1. Pipelining: The server must respond to requests from a client in the order in which they were received [RFC 7230 6.3.2](https://www.rfc-editor.org/rfc/rfc7230#section-6.3.2)
2. `-rpcservertimeout` config option which sets the amount of time the server will keep an idle client connection alive
3. "Chunked" Transfer-Encoding: Allows a client to send a request in pieces, without the `Content-Length` header [RFC 7230 4.1](https://www.rfc-editor.org/rfc/rfc7230#section-4.1)
ACKs for top commit:
achow101:
ACK f16c8c67bf137e64fbeea1242431baaa915a5c53
vasild:
ACK f16c8c67bf137e64fbeea1242431baaa915a5c53
polespinasa:
ACK f16c8c67bf137e64fbeea1242431baaa915a5c53
fjahr:
utACK f16c8c67bf137e64fbeea1242431baaa915a5c53
Tree-SHA512: 405b59431b4d2bf118fde04b270865dee06ef980ab120d9cc1dce28e5d65dfd880a57055b407009d22f4de614bc3eebdb3e203bcd39e86cb14fbfd62195ed06a
