8b62647680 test: send duplicate blocktxn message in p2p_compactblocks.py (Eugene Siegel)
5e585a0fc4 net: check for empty header before calling FillBlock (Eugene Siegel)
Pull request description:
This avoids an Assume crash if multiple blocktxn messages are received. The first call to `FillBlock` would make the header empty via `SetNull` and the call right before the second `FillBlock` would crash [here](689a321976/src/net_processing.cpp (L3333)) since `LookupBlockIndex` won't find anything. Fix that by checking for an empty header before the Assume.
ACKs for top commit:
instagibbs:
reACK 8b62647680
fjahr:
tACK 8b62647680
achow101:
ACK 8b62647680
mzumsande:
Code Review ACK 8b62647680
Tree-SHA512: d43a6f652161d4f7e6137f207a3e95259fc51509279d20347b1698c91179c39c8fcb75d2668b13a6b220f478a03578573208a415804be1d8843acb057fa1a73a
c767974811 clang-tidy: Fix critical warnings (Fabian Jahr)
54dc34ec22 index: Remove unused coinstatsindex recovery code (Fabian Jahr)
37c4fba1f4 index: Check BIP30 blocks when rewinding Coinstatsindex (Fabian Jahr)
51df9de8e5 doc: Add release note for 30469 (Fabian Jahr)
bb8d673183 test: Add coinstatsindex compatibility test (Fabian Jahr)
b2e8b64ddc index, refactor: Append blocks to coinstatsindex without db read (Fabian Jahr)
431a076ae6 index: Fix coinstatsindex overflow issue (Fabian Jahr)
84e813a02b index, refactor: DRY coinbase check (Fabian Jahr)
fab842b324 index, refactor: Rename ReverseBlock to RevertBlock (Fabian Jahr)
Pull request description:
Closes https://github.com/bitcoin/bitcoin/issues/26362
This continues the work that was started with #26426. It fixes the overflow issue by switching the tracked values that are in danger of overflowing from `CAmount` to `arith_uint256`.
The current approach opts for a simple solution to ensure compatibility with datadirs including the previous version of the index: The new version of the index goes into a separate location in the datadir (`index/coinstatsindex/` rather than `index/coinstats/` before, the new naming is more consistent with the naming of the other indexes). There is no explicit concept of versioning of the index which earlier versions of this PR had. Having the two different versions of the index in separate places allows for downgrading of the node without having to rebuild the index. However, there will be a warning printed in the logs if the new code (v30) detects the old index still being present. A future version could delete a left-over legacy index automatically.
The PR also includes several minor improvements but most notably it lets new entries be calculated and stored without needing to read any DB records.
ACKs for top commit:
achow101:
ACK c767974811
TheCharlatan:
ACK c767974811
mzumsande:
Tested / Code Review ACK c767974811
Tree-SHA512: 3fa4a19dd1a01c1b01390247bc9daa6871eece7c1899eac976e0cc21ede09c79c65f758d14daafc46a43c4ddd7055c85fb28ff03029132d48936b248639c6ab9
The index originally stored cumulative values in a CAmount type but this allowed for
potential overflow issues which were observed on Signet. Fix this by
storing the values that are in danger of overflowing in a arith_uint256.
Also turns an unnecessary copy into a reference in RevertBlock and
CustomAppend and gets
rid of the explicit total unspendable tracking which can be calculated
by adding the four categories of unspendables together.
With this change, tests can specify `self.extra_init = [{ipcbind: True}]` to
start a node listening on an IPC socket, instead of needing to choose which
node binary to invoke and what `self.extra_args=[["-ipcbind=..."]]` value to
pass to it.
The eliminates boilerplate code #30437 (interface_ipc_mining.py), #32297
(interface_ipc_cli.py), and #33201 (interface_ipc.py) previously needed in
their test setup.
Set new `BitcoinTestFramework.binary_paths.bitcoin_bin` property with path to
the `bitcoin` wrapper binary. This allows new tests for `bitcoin-mine` in
#30437 and `bitcoin-cli` in #32297 to find the `bitcoin` binary and call
`bitcoin -m` to start nodes with IPC support. This way the new tests can run
whenever the ENABLE_IPC build option is enabled, instead of only running when
the `BITCOIN_CMD` environment variable is set to `bitcoin -m`
2885bd0e1c doc: unify `datacarriersize` warning with release notes (Lőrinc)
Pull request description:
Follow-up to https://github.com/bitcoin/bitcoin/pull/32406
---
The [release notes](a189d63618/doc/release-notes-32406.md (L1)) claim
> [...] marked as deprecated and are expected to be removed in a future release
but the [warning itself](2885bd0e1c/src/init.cpp (L907)) claims
> [...] marked as deprecated. They **will** be removed in a future version.
To be less aggressive (since some have objected against this version online) - and to unify the deprecation warning with the release notes - I have changed the warning to communicate our expectation in a friendlier way.
ACKs for top commit:
cedwies:
ACK 2885bd0
ryanofsky:
Code review ACK 2885bd0e1c. I don't think it is good for the release notes and the runtime warning message to say two different things. I'd also be happy if release notes were updated to match the runtime warning, instead of vice versa. Whatever is more accurate is better.
ajtowns:
ACK 2885bd0e1c
kevkevinpal:
ACK [2885bd0](2885bd0e1c)
achow101:
ACK 2885bd0e1c
janb84:
ACK 2885bd0e1c
Zero-1729:
crACK 2885bd0e1c
jonatack:
ACK 2885bd0e1c
hodlinator:
ACK 2885bd0e1c
w0xlt:
ACK 2885bd0e1c
optout21:
ACK 2885bd0e1c
Tree-SHA512: a9d2a64ab96b3dd7f3a1a29622930054fd5c56e573bc96330f4ef3327dc024b21b3fbc8a698d17aea7c76f57f0c2ccd6403b2df344ae2f69c645ceb8b6fa54a5
a602f6fb7b test: index with an unclean restart after a reorg (Martin Zumsande)
01b95ac6f4 index: don't commit state in BaseIndex::Rewind (Martin Zumsande)
Pull request description:
The committed state of an index should never be ahead of the flushed chainstate.
Otherwise, in the case of an unclean shutdown, the blocks necessary to revert
from the prematurely committed state are not be available, which would corrupt the coinstatsindex in particular.
Instead, the index state will be committed with the next ChainStateFlushed notification.
Fixes#33208
ACKs for top commit:
achow101:
ACK a602f6fb7b
stickies-v:
re-ACK a602f6fb7b
Tree-SHA512: 2559ea3fe066caf746a54ad7daac5031332f3976848e937c3dc8b35fa2ce925674115d8742458bf3703b3916f04f851c26523b6b94aeb1da651ba5a1b167a419
5c8bf7b39e doc: add release notes for version 3 transactions (ishaanam)
4ef8065a5e test: add truc wallet tests (ishaanam)
5d932e14db test: extract `bulk_vout` from `bulk_tx` so it can be used by wallet tests (ishaanam)
2cb473d9f2 rpc: Support version 3 transaction creation (Bue-von-hon)
4c20343b4d rpc: Add transaction min standard version parameter (Bue-von-hon)
c5a2d08011 wallet: don't return utxos from multiple truc txs in AvailableCoins (ishaanam)
da8748ad62 wallet: limit v3 tx weight in coin selection (ishaanam)
85c5410615 wallet: mark unconfirmed v3 siblings as mempool conflicts (ishaanam)
0804fc3cb1 wallet: throw error at conflicting tx versions in pre-selected inputs (ishaanam)
cc155226fe wallet: set m_version in coin control to default value (ishaanam)
2e9617664e wallet: don't include unconfirmed v3 txs with children in available coins (ishaanam)
ec2676becd wallet: unconfirmed ancestors and descendants are always truc (ishaanam)
Pull request description:
This PR Implements the following:
- If creating a v3 transaction, `AvailableCoins` doesn't return unconfirmed v2 utxos (and vice versa)
- `AvailableCoins` doesn't return an unconfirmed v3 utxo if its transaction already has a child
- If a v3 transaction is kicked out of the mempool by a sibling, mark the sibling as a mempool conflict
- Throw an error if pre-selected inputs are of the wrong transaction version
- Allow setting version to 3 manually in `createrawtransaction` (uses commits from #31936)
- Limits a v3 transaction weight in coin selection
Closes#31348
To-Do:
- [x] Test a v3 sibling conflict kicking out one of our transactions from the mempool
- [x] Implement separate size limit for TRUC children
- [x] Test that we can't fund a v2 transaction when everything is v3 unconfirmed
- [x] Test a v3 sibling conflict being removed from the mempool
- [x] Test limiting v3 transaction weight in coin selection
- [x] Simplify tests
- [x] Add documentation
- [x] Test that user-input max weight is not overwritten by truc max weight
- [x] Test v3 in RPCs other than `createrawtransaction`
ACKs for top commit:
glozow:
reACK 5c8bf7b39e
achow101:
ACK 5c8bf7b39e
rkrux:
ACK 5c8bf7b39e
Tree-SHA512: da8aea51c113e193dd0b442eff765bd6b8dc0e5066272d3e52190a223c903f48788795f32c554f268af0d2607b5b8c3985c648879cb176c65540837c05d0abb5
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
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-
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
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.
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
The `-assumevalid` option skips script verification for a specified block and all its ancestors during Initial Block Download.
Many new users are surprised 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).
When using `-assumeutxo`, logging is suppressed for the active assumed-valid chainstate and for the background validation chainstate to avoid the confusing toggles.
-------
> cmake -B build && cmake --build build && mkdir -p demo && build/bin/bitcoind -datadir=demo -stopatheight=500 | grep 'signature validation'
```
2025-08-08T20:59:21Z Disabling signature validations at block #1 (00000000839a8e6886ab5951d76f411475428afc90947ee320161bbf18eb6048).
2025-08-08T20:59:21Z Enabling signature validations at block #100 (000000007bc154e0fa7ea32218a72fe2c1bb9f86cf8c9ebf9a715ed27fdb229a).
2025-08-08T20:59:21Z Disabling signature validations at block #200 (000000008f1a7008320c16b8402b7f11e82951f44ca2663caf6860ab2eeef320).
2025-08-08T20:59:21Z Enabling signature validations at block #300 (0000000062b69e4a2c3312a5782d7798b0711e9ebac065cd5d19f946439f8609).
```
Previously, we would check failing input scripts twice when considering
a transaction for the mempool, in order to distinguish policy failures
from consensus failures. This allowed us both to provide a different
error message and to discourage peers for consensus failures. Because we
are no longer discouraging peers for consensus failures during tx relay,
and because checking a script can be expensive, only do this once.
Also renames non-mandatory-script-verify-flag error to
mempool-script-verify-flag-failed.
Do not discourage nodes even when they send us consensus invalid
transactions.
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.
27aefac425 validation: detect witness stripping without re-running Script checks (Antoine Poinsot)
2907b58834 policy: introduce a helper to detect whether a transaction spends Segwit outputs (Antoine Poinsot)
eb073209db qa: test witness stripping in p2p_segwit (Antoine Poinsot)
Pull request description:
Since it was introduced in 4eb515574e (#18044), the detection of a stripped witness relies on running the Script checks 3 times. In the worst case, this consists in running Script validation for every single input 3 times.
Detection of a stripped witness is necessary because in this case wtxid==txid, and the transaction's wtxid must not be added to the reject filter or it could allow a malicious peer to interfere with txid-based orphan resolution as used in 1p1c package relay.
However it is not necessary to run Script validation to detect a stripped witness (much less so doing it 3 times in a row). There are 3 types of witness program: defined program types (Taproot, P2WPKH and P2WSH), undefined types, and the Pay-to-anchor carve-out.
For defined program types, Script validation with an empty witness will always fail (by consensus). For undefined program types, Script validation is always going to fail regardless of the witness (by standardness). For P2A, an empty witness is never going to lead to a failure.
Therefore it holds that we can always detect a stripped witness without re-running Script validation. However this might lead to more "false positives" (cases where we return witness stripping for an otherwise invalid transaction) than the existing implementation. For instance a transaction with one P2PKH input with an invalid signature and one P2WPKH input with its witness stripped. The existing implementation would treat it as consensus invalid while the implementation in this PR would always consider it witness stripped.
h/t AJ: this essentially implements a variant of https://github.com/bitcoin/bitcoin/pull/33066#issuecomment-3135258539.
ACKs for top commit:
sipa:
re-ACK 27aefac425
Crypt-iQ:
re-ACK 27aefac425
glozow:
reACK 27aefac425
Tree-SHA512: 70cf76b655b52bc8fa2759133315a3f11140844b6b80d9de3c95f592050978cc01a87bd2446e3a9c25cc872efea7659d6da3337b1a709511771fece206e9f149
3543bfdfec test: Fix 'getdescriptoractivity' RPCHelpMan, add test to verify 'spend_vin' is the correct field (Chris Stewart)
Pull request description:
Fixes bug in `getdescriptoractivity` RPC help manual.
Here is the line that pushes `spend_vin` field, there is no `spend_vout` json field.
https://github.com/bitcoin/bitcoin/blob/master/src/rpc/blockchain.cpp#L2757
ACKs for top commit:
nervana21:
tACK 3543bfd
luke-jr:
utACK 3543bfdfec
jonatack:
ACK 3543bfdfec
Tree-SHA512: 2cd543569a87261d8d804d9afe36f8e8ead55839c01da9c4831aea3ced7d1251e6885621e628898105700aae4d76cbb8a682f518f33c1c52163e66f75ec87a61
Ava Chow
Eugene Siegel
Fabian Jahr
Pieter Wuille
Ryan Ofsky
MarcoFalke
merge-script
Martin Zumsande
Lőrinc
merge-script
ishaanam
Bue-von-hon
Antoine Poinsot
Anthony Towns
fuder.eth