60d1042b9a wallet: Remove unused `WalletFeature` enums (woltx)
66de58208a wallet: Remove `CWallet::nWalletVersion` and related functions (woltx)
7cda3d0f5b wallet: Remove `IsFeatureSupported()` and `CanSupportFeature()` (woltx)
ba01585229 wallet: `MigrateToDescriptor` no longer calls `CanSupportFeature` (woltx)
63acee2797 wallet: Remove `GetClosestWalletFeature()` (woltx)
e27da3150b wallet: Remove `GetVersion()` (woltx)
Pull request description:
This PR incorporates the suggestion provided by PRabahy and pablomartin4btc in https://github.com/bitcoin/bitcoin/pull/32944 of removing `CWallet::nWalletVersion` and several related functions, such as `SetMinVersion()`, `GetVersion()`, `GetClosestWalletFeature()`, `IsFeatureSupported()`, `CanSupportFeature()`, etc ...
This field is no longer used in the descriptor wallet and there is still a lot of code related to it, so the changes here provide a good cleanup in the wallet code.
Built on top of https://github.com/bitcoin/bitcoin/pull/32944
ACKs for top commit:
maflcko:
review ACK 60d1042b9a🐾
achow101:
ACK 60d1042b9a
pablomartin4btc:
ACK 60d1042b9a
Tree-SHA512: 1a7ad8e15d57df8f66545776e7d178a2cd5312c87769a29770588375e3de5f24247aab9919acf004ed3eca16d08ba595b5f1c7b2b3eef7752e89d9c295624583
fab2980bdc assumevalid: log every script validation state change (Lőrinc)
Pull request description:
The `-assumevalid` option skips script verification for a specified block and all its ancestors during Initial Block Download.
Many new [users are surprised](https://github.com/bitcoin/bitcoin/issues/32832) when this suddenly slows their node to a halt.
This commit adds a log message to clearly indicate when this optimization ends and full validation begins (and vice versa).
<details>
<summary>Testing instructions</summary>
The behavior can easily be tested by adding this before the new log:
```C++
// TODO hack to enable/disable script checks based on block height for testing purposes
if (pindex->nHeight < 100) fScriptChecks = false;
else if (pindex->nHeight < 200) fScriptChecks = true;
else if (pindex->nHeight < 300) fScriptChecks = false;
else if (pindex->nHeight < 400) fScriptChecks = true;
```
and exercise the new code with:
```bash
cmake -B build && cmake --build build && mkdir -p demo && build/bin/bitcoind -datadir=demo -stopatheight=500 | grep 'signature validation'
```
showing something like:
* Disabling signature validations at block #1 (00000000839a8e6886ab5951d76f411475428afc90947ee320161bbf18eb6048).
* Enabling signature validations at block #100 (000000007bc154e0fa7ea32218a72fe2c1bb9f86cf8c9ebf9a715ed27fdb229a).
* Disabling signature validations at block #200 (000000008f1a7008320c16b8402b7f11e82951f44ca2663caf6860ab2eeef320).
* Enabling signature validations at block #300 (0000000062b69e4a2c3312a5782d7798b0711e9ebac065cd5d19f946439f8609).
</details>
ACKs for top commit:
achow101:
ACK fab2980bdc
ajtowns:
crACK fab2980bdc
davidgumberg:
untested crACK fab2980bdc
Tree-SHA512: e90b66f7423b639356daace476942ce83e65e70466544394cbe2f15738bdbf716163eaf590c64c5448f9b41aeeaafe3342c48c6a7a478678a70b0310ca94e11d
c0d91fc69c Add release note for #33050 and #33183 error string changes (Antoine Poinsot)
b3f781a0ef contrib: adapt max reject string size in tracing demo (Antoine Poinsot)
9a04635432 scripted-diff: validation: rename mandatory errors into block errors (Antoine Poinsot)
Pull request description:
This is a followup to #33050 now that it's merged. Using "block"/"mempool" as the error reason is clearer to a user than "mandatory"/"non-mandatory". The "non-mandatory" errors got renamed to "mempool" in #33050 (see https://github.com/bitcoin/bitcoin/pull/33050#discussion_r2230103371). This takes care of the second part of the renaming.
ACKs for top commit:
fjahr:
utACK c0d91fc69c
davidgumberg:
lgtm ACK c0d91fc69c
ajtowns:
utACK c0d91fc69c
Crypt-iQ:
utACK c0d91fc69c
janb84:
utACK c0d91fc69c
instagibbs:
ACK c0d91fc69c
Tree-SHA512: b463e633c57dd1eae7c49d23239a59066a672f355142ec194982eddc927a7646bc5cde583dc8d6f45075bf5cbb96dbe73f7e339e728929b0eff356b674d1b68c
ba84a25dee [doc] update mempool-replacements.md for incremental relay feerate change (glozow)
18720bc5d5 [doc] release note for min feerate changes (glozow)
6da5de58ca [policy] lower default minrelaytxfee and incrementalrelayfee to 100sat/kvB (glozow)
2e515d2897 [prep/test] make wallet_fundrawtransaction's minrelaytxfee assumption explicit (glozow)
457cfb61b5 [prep/util] help MockMempoolMinFee handle more precise feerates (glozow)
3eab8b7240 [prep/test] replace magic number 1000 with respective feerate vars (glozow)
5f2df0ef78 [miner] lower default -blockmintxfee to 1sat/kvB (glozow)
d6213d6aa1 [doc] assert that default min relay feerate and incremental are the same (glozow)
1fbee5d7b6 [test] explicitly check default -minrelaytxfee and -incrementalrelayfee (glozow)
72dc18467d [test] RBF rule 4 for various incrementalrelayfee settings (glozow)
85f498893f [test] check bypass of minrelay for various minrelaytxfee settings (glozow)
e5f896bb1f [test] check miner doesn't select 0fee transactions (glozow)
Pull request description:
ML post for discussion about the general concept, how this impacts the wider ecosystem, philosophy about minimum feerates, etc: https://delvingbitcoin.org/t/changing-the-minimum-relay-feerate/1886
This PR is inspired by #13922 and #32959 to lower the minimum relay feerate in response to bitcoin's exchange rate changes in the last ~10 years. It lowers the default `-minrelaytxfee` and `-incrementalrelayfee`, and knocks `-blockmintxfee` down to the minimum nonzero setting. Also adds some tests for the settings and pulls in #32750.
The minimum relay feerate is a DoS protection rule, representing a price on the network bandwidth used to relay transactions that have no PoW. While relay nodes don't all collect fees, the assumption is that if nodes on the network use their resources to relay this transaction, it will reach a miner and the attacker's money will be spent once it is mined. The incremental relay feerate is similar: it's used to price the relay of replacement transactions (the additional fees need to cover the new transactions at this feerate) and evicted transactions (following a trim, the new mempool minimum feerate is the package feerate of what was removed + incremental).
Also note that many nodes on the network have elected to relay/mine lower feerate transactions. Miners (some say up to 85%) are choosing to mine these low feerate transactions instead of leaving block space unfilled, but these blocks have extremely poor compact block reconstruction rates with nodes that rejected or didn't hear about those transactions earlier.
- https://github.com/bitcoin/bitcoin/pull/33106#issuecomment-3155627414
- https://x.com/caesrcd/status/1947022514267230302
- https://mempool.space/block/00000000000000000001305770e0aa279dcd8ba8be18c3d5cf736a26f77e06fd
- https://mempool.space/block/00000000000000000001b491649ec030aa8e003e1f4f9d3b24bb99ba16f91e97
- https://x.com/mononautical/status/1949452586391855121
While it wouldn't make sense to loosen DoS restrictions recklessly in response to these events, I think the current price is higher than necessary, and this motivates us changing the default soon. Since the minimum relay feerate defines an amount as too small based on what it costs the attacker, it makes sense to consider BTC's conversion rate to what resources you can buy in the "real world."
Going off of [this comment](https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3095260286) and [this comment](https://github.com/bitcoin/bitcoin/pull/33106#issuecomment-3142444090)
- Let's say an attacker wants to use/exhaust the network's bandwidth, and has the choice between renting resources from a commercial provider and getting the network to "spam" itself it by sending unconfirmed transactions. We'd like the latter to be more expensive than the former.
- The bandwidth for relaying a transaction across the network is roughly its serialized size (plus relay overhead) x number of nodes. A 1000vB transaction is 1000-4000B serialized. With 100k nodes, that's 0.1-0.4GB
- If the going rate for ec2 bandwidth is 10c/GB, that's like 1-4c per kvB of transaction data
- Then a 1000vB transaction should pay at least 4c
- $0.04 USD is 40 satoshis at 100k USD/BTC
- Baking in some margin for changes in USD/BTC conversion rate, number of nodes (and thus bandwidth), and commercial service costs, I think 50-100 satoshis is on the conservative end but in the right ballpark
- At least 97% of the recent sub-1sat/vB transactions would be accepted with a new threshold of 0.1sat/vB: https://github.com/bitcoin/bitcoin/pull/33106#issuecomment-3156213089
List of feerates that are changed and why:
- min relay feerate: significant conversion rate changes, see above
- incremental relay feerate: should follow min relay feerate, see above
- block minimum feerate: shouldn’t be above min relay feerate, otherwise the node accepts transactions it will never mine. I've knocked it down to the bare minimum of 1sat/kvB. Now that we no longer have coin age priority (removed in v0.15), I think we can leave it to the `CheckFeeRate` policy rule to enforce a minimum entry price, and the block assembly code should just fill up the block with whatever it finds in mempool.
List of feerates that are not changed and why:
- dust feerate: this feerate cannot be changed as flexibly as the minrelay feerate. A much longer record of low feerate transactions being mined is needed to motivate a decrease there.
- maxfeerate (RPC, wallet): I think the conversion rate is relevant as well, but out of scope for this PR
- minimum feerate returned by fee estimator: should be done later. In the past, we've excluded new policy defaults from fee estimation until we feel confident they represent miner policy (e.g. #9519). Also, the fee estimator itself doesn't have support for sub-1sat/vB yet.
- all wallet feerates (mintxfee, fallbackfee, discardfee, consolidatefeerate, WALLET_INCREMENTAL_RELAY_FEE, etc.): should be done later. Our standard procedure is to do wallet changes at least 1 release after policy changes.
ACKs for top commit:
achow101:
ACK ba84a25dee
gmaxwell:
ACK ba84a25dee
jsarenik:
Tested ACK ba84a25dee
darosior:
ACK ba84a25dee
ajtowns:
ACK ba84a25dee
davidgumberg:
crACK ba84a25dee
w0xlt:
ACK ba84a25dee
caesrcd:
reACK ba84a25dee
ismaelsadeeq:
re-ACK ba84a25dee
Tree-SHA512: b4c35e8b506b1184db466551a7e2e48bb1e535972a8dbcaa145ce3a8bfdcc70a8807dc129460f129a9d31024174d34077154a387c32f1a3e6831f6fa5e9c399e
5c74a0b397 config: add DEBUG_ONLY -logratelimit (Eugene Siegel)
9f3b017bcc test: logging_filesize_rate_limit improvements (stickies-v)
350193e5e2 test: don't leak log category mask across tests (stickies-v)
05d7c22479 test: add ReadDebugLogLines helper function (stickies-v)
3d630c2544 log: make m_limiter a shared_ptr (stickies-v)
e8f9c37a3b log: clean up LogPrintStr_ and Reset, prefix all logs with "[*]" when there are suppressions (Eugene Siegel)
3c7cae49b6 log: change LogLimitStats to struct LogRateLimiter::Stats (Eugene Siegel)
8319a13468 log: clarify RATELIMIT_MAX_BYTES comment, use RATELIMIT_WINDOW (Eugene Siegel)
5f70bc80df log: remove const qualifier from arguments in LogPrintFormatInternal (Eugene Siegel)
b8e92fb3d4 log: avoid double hashing in SourceLocationHasher (Eugene Siegel)
616bc22f13 test: remove noexcept(false) comment in ~DebugLogHelper (Eugene Siegel)
Pull request description:
Followups to #32604.
There are two behavior changes:
- prefixing with `[*]` is done to all logs (regardless of `should_ratelimit`) per [this comment](https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2195710943).
- a DEBUG_ONLY `-disableratelimitlogging` flag is added by default to functional tests so they don't encounter rate limiting.
ACKs for top commit:
stickies-v:
re-ACK 5c74a0b397
achow101:
ACK 5c74a0b397
l0rinc:
Code review ACK 5c74a0b397
Tree-SHA512: d32db5fcc28bb9b2a850f0048c8062200a3725b88f1cd9a0e137da065c0cf9a5d22e5d03cb16fe75ea7494801313ab34ffec7cf3e8577cd7527e636af53591c4
2b00030af8 interfaces, chain, refactor: Remove inaccurate getActiveChainLocator (pablomartin4btc)
110a0f405c interfaces, chain, refactor: Remove unused getTipLocator (pablomartin4btc)
Pull request description:
Remove `Chain::getTipLocator`, `Chain::GetLocator()`, and `Chain::getActiveChainLocator`:
- `Chain::getTipLocator` is no longer used.
- `Chain::GetLocator`, replaced its call by `GetLocator()`, which uses `LocatorEntries`, avoiding direct access to the chain itself (change suggested by l0rinc while reviewing this PR to maintain consistency with the overall refactoring).
- `Chain::getActiveChainLocator`, whose name was misleading, has functionality redundant with Chain::findBlock.
- Additionally, the comment for getActiveChainLocator became inaccurate following changes in commit ed470940cd (from PR #25717).
This is a [follow-up](https://github.com/bitcoin/bitcoin/pull/29652#issuecomment-3151665095) to #29652.
ACKs for top commit:
achow101:
ACK 2b00030af8
furszy:
ACK 2b00030af8
stickies-v:
ACK 2b00030af8
w0xlt:
ACK 2b00030af8
Tree-SHA512: b12ba6a15feeaeec692d69204a6e155e3af43edfac25597dabf14cacca1e4a2152574816e58dc544f39043c5721f5e707acf544f4541d3b9c0f8c0c40069215e
67e186deb0 doc: update wallet build instruction (Sjors Provoost)
Pull request description:
Sqlite and the wallet are no longer optional, but they can still be opted out of. This PR updates the build instructions accordingly.
Updating this text now reduces churn in #31802.
ACKs for top commit:
jonatack:
ACK 67e186deb0
achow101:
ACK 67e186deb0
w0xlt:
reACK 67e186deb0
pablomartin4btc:
ACK 67e186deb0
janb84:
ACK 67e186deb0
Tree-SHA512: ea87a83c9fcb884f9ca380a006a93ede3cd1af5eb2bb5ccb54fc8124faa532e9473ae2461d775439da59e86178fbd04983113b43ce3d4c382cabe27866acd027
de0675f9de refactor: Move `transaction_identifier.h` to primitives (marcofleon)
6f068f65de Remove implicit uint256 conversion and comparison (marcofleon)
9c24cda72e refactor: Convert remaining instances from uint256 to Txid (marcofleon)
d2ecd6815d policy, refactor: Convert uint256 to Txid (marcofleon)
f6c0d1d231 mempool, refactor: Convert uint256 to Txid (marcofleon)
aeb0f78330 refactor: Convert `mini_miner` from uint256 to Txid (marcofleon)
326f244724 refactor: Convert RPCs and `merkleblock` from uint256 to Txid (marcofleon)
49b3d3a92a Clean up `FindTxForGetData` (marcofleon)
Pull request description:
This is the final leg of the [type safety refactor](https://github.com/bitcoin/bitcoin/pull/32189).
All of these changes are straightforward `uint256` --> `Txid` along with any necessary explicit conversions. Also, `transaction_identifier.h` is moved to primitives in the last commit, as `Txid` and `Wtxid` become fundamental types after this PR.
ACKs for top commit:
stickies-v:
re-ACK de0675f9de, no changes since a20724d926 except address review nits.
janb84:
re ACK de0675f9de
dergoegge:
re-ACK de0675f9de
theStack:
Code-review ACK de0675f9de
Tree-SHA512: 2413160fca7ab146a8d79d18ce3afcf7384cacc73c513d41928904aa453b4dd7a350064cee71e9c5d015da5904c7c81ac17603e50a47441ebc5b0c653235dd08
The Script errors were last touched in 2020. This value was calculated after that
in 2022 (commit 4b7aec2951). The previous commit
made the size of the largest reject reason string 4 characters smaller ("mandatory"
became "block"), so adapt the constant.
Using "block" or "mempool" as the prefix in place of "mandatory" or "non-mandatory" is clearer
to a user. "non-mandatory" was renamed into "mempool" as part of #33050. This takes care of the
other half of this renaming as a scripted diff.
-BEGIN VERIFY SCRIPT-
sed -i 's/mandatory-script-verify/block-script-verify/g' $(git grep -l mandatory-script-verify)
-END VERIFY SCRIPT-
776a163374 guix: increase maximum allowed (runtime) GCC to 7 (fanquake)
Pull request description:
Fixes:
```bash
/distsrc-base/distsrc-d3b8a54a8120-x86_64-linux-gnu/build/bin/bitcoind: symbol __divmodti4 from unsupported version GCC_7.0.0(14)
/distsrc-base/distsrc-d3b8a54a8120-x86_64-linux-gnu/build/bin/bitcoind: failed IMPORTED_SYMBOLS
```
which is occuring after #32750. I can't see any supported distro that is shipping a new enough glibc (2.31), but a GCC older than 7.0.
Fixes#33177.
ACKs for top commit:
hebasto:
ACK 776a163374.
Tree-SHA512: 8e5a77c509eb6164314fdb644ea199916e151eb0c7f48703f3a2bdedf0dea29b7f402ceacb2aaf42ebffba59080cefbb84253b2721047d973a851090447ba3b5
The getActiveChainLocator method name was misleading, and its functionality
duplicated `Chain::findBlock`. This commit removes the method and replaces
all its usages with direct `Chain::findBlock` calls.
Additionally, the comment of getActiveChainLocator has been outdated since
commit ed47094 from #25717.
Finally, in CWallet::ScanForWalletTransactions, the findBlock calls are now
unified into a single call at the start of the function.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
Co-authored-by: Matias Furszyfer <mfurszy@protonmail.com>
Also removed CChain::GetLocator() and replaced its call
with GetLocator() which uses LocatorEntries instead.
Co-authored-by: ryanofsky <ryan@ofsky.org>
Co-authored-by: l0rinc <l0rinc@users.noreply.github.com>
876dbdfb47 tests: drop expect_disconnect behaviour for tx relay (Anthony Towns)
b29ae9efdf validation: only check input scripts once (Anthony Towns)
266dd0e10d net_processing: drop MaybePunishNodeForTx (Anthony Towns)
Pull request description:
Because we do not discourage nodes for transactions we consider non-standard, we don't get any DoS protection from this check in adversarial scenarios, so remove the check entirely both to simplify the code and reduce the risk of splitting the network due to changes in tx relay policy.
Then, because we no longer make use of the distinction between consensus and standardness failures during script validation, don't re-validate each script with only-consensus rules, reducing the cost to us of transactions that we won't relay.
ACKs for top commit:
achow101:
ACK 876dbdfb47
darosior:
re-ACK 876dbdfb47
sipa:
re-ACK 876dbdfb47
glozow:
ACK 876dbdfb47
Tree-SHA512: 8bb0395766dde54fc48f7077b80b88e35581aa6e3054d6d65735965147abefffa7348f0850bb3d46f6c2541fd384ecd40a00a57fa653adabff8a35582e2d1811
db225cea56 wallet, refactor: Replace GetDisplayName() with LogName() (Ryan Ofsky)
01737883b3 wallet: Translate [default wallet] string in progress messages (Ryan Ofsky)
Pull request description:
Noticed while reviewing https://github.com/bitcoin/bitcoin/pull/31287#discussion_r1843809721 that the [default wallet] part of progress messages remains untranslated while the rest of the string is translated.
Fix this in all places where `CWallet::ShowProgress` (which has a cancel button) and `Chain::showProgress` (which doesn't have a cancel button) are called by making "default wallet" into a translated string.
ACKs for top commit:
achow101:
ACK db225cea56
pablomartin4btc:
ACK db225cea56
furszy:
utACK db225cea56
Tree-SHA512: 3e76e22ee692a7403d61c66615f56d0fa5f7883dd47553bcaec2f9ffd942daaa90ceb61830206bece50da53dcd737b6438c36bcb086030b2deb68c44172f3931
- Add helper functions and structs to improve readability and
reusability of test code
- Make tests more specific by comparing all produced log lines with
expected log lines instead of relying on approximations or proxies.
db3228042b util: detect and warn when using exFAT on macOS (willcl-ark)
Pull request description:
exFAT is known to cause intermittent corruption on MacOS.
Therefore we should warn when using this fs format for either the blocks or data directories.
See #28552 for more context.
ACKs for top commit:
l0rinc:
ACK db3228042b
marcofleon:
reACK db3228042b
ismaelsadeeq:
reACK db3228042b
Tree-SHA512: e4453a8e24b35c135e4eb0b4e47fe0c80f8b54700f458909c403aa37a0d2979ee165347bcd76e48e4d1ae5d3bae13f50e6afe714e33226a52f907b95df9d3b46
Fixes:
```bash
/distsrc-base/distsrc-d3b8a54a8120-x86_64-linux-gnu/build/bin/bitcoind: symbol __divmodti4 from unsupported version GCC_7.0.0(14)
/distsrc-base/distsrc-d3b8a54a8120-x86_64-linux-gnu/build/bin/bitcoind: failed IMPORTED_SYMBOLS
```
which is occuring after #32750. I can't see any supported distro that is
shipping a new enough glibc (2.31), but a GCC older than 7.0.
Let's say an attacker wants to use/exhaust the network's bandwidth, and
has the choice between renting resources from a commercial provider and
getting the network to "spam" itself it by sending unconfirmed
transactions. We'd like the latter to be more expensive than the former.
The bandwidth for relaying a transaction across the network is roughly
its serialized size (plus relay overhead) x number of nodes. A 1000vB
transaction is 1000-4000B serialized. With 100k nodes, that's 0.1-0.4GB
If the going rate for commercial services is 10c/GB, that's like 1-4c per kvB
of transaction data, so a 1000vB transaction should pay at least $0.04.
At a price of 120k USD/BTC, 100sat is about $0.12. This price allows us
to tolerate a large decrease in the conversion rate or increase in the
number of nodes.
Back when we implemented coin age priority as a miner policy, miners
mempools might admit transactions paying very low fees, but then want to
set a higher fee for block inclusion. However, since coin age priority
was removed in v0.15, the block assembly policy is solely based on fees,
so we do not need to apply minimum feerate rules in multiple places. In
fact, the block assembly policy ignoring transactions that are added to
the mempool is likely undesirable as we waste resources accepting and
storing this transaction.
Instead, rely on mempool policy to enforce a minimum entry feerate to
the mempool (minrelaytxfee). Set the minimum block feerate to the
minimum non-zero amount (1sat/kvB) so it collects everything it finds in
mempool into the block.
Moves the file from `src/util` to `src/primitives`. Now that the
refactor is complete, Txid and Wtxid are fundamental types, so it
makes sense for them to reside in `src/primitives`.
These remaining miscellaneous changes were identified by commenting out
the `operator const uint256&` conversion and the `Compare(const uint256&)`
method from `transaction_identifier.h`.
ca64b71ed5 test: fix scripts in `blockfilter_basic_test` (UdjinM6)
Pull request description:
`std::vector` fill ctor is like this:
```
// Constructs a vector with `count` copies of elements with value `value`.
explicit vector( size_type count, const T& value = T(), const Allocator& alloc = Allocator() ); // (until C++11)
vector( size_type count, const T& value, const Allocator& alloc = Allocator() ); // (since C++11)(constexpr since C++20)
```
https://en.cppreference.com/w/cpp/container/vector/vector.html
i.e. `std::vector<unsigned char>(0, 65)` means a vector with `0` copies of `65` which feels wrong. I believe `count` and `value` were swapped in `blockfilter_basic_test` scripts.
ACKs for top commit:
furszy:
ACK ca64b71ed5
pablomartin4btc:
ACK ca64b71ed5
janb84:
ACK ca64b71ed5
Tree-SHA512: 2cfc7f09788b0a1afdffc9cd6663204c7f1775dabdbe1046cdcd42936c479658c348cb46e0d8835645e6c508e8b40a598cbe6534084b6780a6b60378bcbd0f96
83950275ed qa: unit test sighash caching (Antoine Poinsot)
b221aa80a0 qa: simple differential fuzzing for sighash with/without caching (Antoine Poinsot)
92af9f74d7 script: (optimization) introduce sighash midstate caching (Pieter Wuille)
8f3ddb0bcc script: (refactor) prepare for introducing sighash midstate cache (Pieter Wuille)
9014d4016a tests: add sighash caching tests to feature_taproot (Pieter Wuille)
Pull request description:
This introduces a per-txin cache for sighash midstate computation to the script interpreter for legacy (bare), P2SH, P2WSH, and (as collateral effect, but not actually useful) P2WPKH. This reduces the impact of certain types of quadratic hashing attacks that use standard transactions. It is not known to improve the situation for attacks involving non-standard transaction attacks.
The cache works by remembering for each of the 6 sighash modes a `(scriptCode, midstate)` tuple, which gives a midstate `CSHA256` object right before the appending of the sighash type itself (to permit all 256, rather than just the 6 ones that match the modes). The midstate is only reused if the `scriptCode` matches. This works because - within a single input - only the sighash type and the `scriptCode` affect the actual sighash used.
The PR implements two different approaches:
* The initial commits introduce the caching effect always, for both consensus and relay relation validation. Despite being primarily intended for improving the situation for standard transactions only, I chose this approach as the code paths are already largely common between the two, and this approach I believe involves fewer code changes than a more targetted approach, and furthermore, it should not hurt (it may even help common multisig cases slightly).
* The final commit changes the behavior to only using the cache for non-consensus script validation. I'm open to feedback about whether adding this commit is worth it.
Functional tests are included that construct contrived cases with many sighash types (standard and non-standard ones) and `OP_CODESEPARATOR`s in all script types (including P2TR, which isn't modified by this PR).
ACKs for top commit:
achow101:
ACK 83950275ed
dergoegge:
Code review ACK 83950275ed
darosior:
re-ACK 83950275ed
Tree-SHA512: 65ae8635429a4d563b19969bac8128038ac2cbe01d9c9946abd4cac3c0780974d1e8b9aae9bb83f414e5d247a59f4a18fef5b37d93ad59ed41b6f11c3fe05af4
