4d4789dffad55b96f1cb96b718cc6923f5344454 net: Prevent node from binding to the same CService (woltx)
Pull request description:
Currently, if the node inadvertently starts with repeated `-bind` options (e.g. `./build/bin/bitcoind -listen -bind=0.0.0.0 -bind=0.0.0.0`), the user will receive a misleading message followed by the node shutdown:
```
[net:error] Unable to bind to 0.0.0.0:8333 on this computer. Bitcoin Core is probably already running.
[error] Unable to bind to 0.0.0.0:8333 on this computer. Bitcoin Core is probably already running.
```
And the user might spend some time looking for a `bitcoind` process or what application is using port 8333, when what happens is that Bitcoin Core successfully connected to port 8333 and then tries again, generating this fatal error.
This PR proposes that repeated `-bind` options have no effect.
ACKs for top commit:
l0rinc:
ACK 4d4789dffad55b96f1cb96b718cc6923f5344454
yuvicc:
re-ACK 4d4789dffad55b96f1cb96b718cc6923f5344454
sipa:
utACK 4d4789dffad55b96f1cb96b718cc6923f5344454
achow101:
ACK 4d4789dffad55b96f1cb96b718cc6923f5344454
vasild:
ACK 4d4789dffad55b96f1cb96b718cc6923f5344454
naiyoma:
Tested ACK 4d4789dffad55b96f1cb96b718cc6923f5344454
Tree-SHA512: f1042c00417da16550403cfcb75cb8b12740e67cf92a1d8e3c007ae81fcf741907088a633129ce12a6a48ad07fc9f320602792cafed73ec33f6306cd854514b4
8b6264768030db1840041abeeaeefd6c227a2644 test: send duplicate blocktxn message in p2p_compactblocks.py (Eugene Siegel)
5e585a0fc4fd68dd7b4982054b34deae2e7aeb89 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 8b6264768030db1840041abeeaeefd6c227a2644
achow101:
ACK 8b6264768030db1840041abeeaeefd6c227a2644
mzumsande:
Code Review ACK 8b6264768030db1840041abeeaeefd6c227a2644
Tree-SHA512: d43a6f652161d4f7e6137f207a3e95259fc51509279d20347b1698c91179c39c8fcb75d2668b13a6b220f478a03578573208a415804be1d8843acb057fa1a73a
c76797481155754329ec6a6f58e8402569043944 clang-tidy: Fix critical warnings (Fabian Jahr)
54dc34ec2279378c78fa2d9155668e39e20decda index: Remove unused coinstatsindex recovery code (Fabian Jahr)
37c4fba1f4c18632bceb16f41f5a8f1a61fb9096 index: Check BIP30 blocks when rewinding Coinstatsindex (Fabian Jahr)
51df9de8e5b9c8ecd8339d95b630f312fcb9414e doc: Add release note for 30469 (Fabian Jahr)
bb8d673183294a43c716ff8738da2492f3d7a94b test: Add coinstatsindex compatibility test (Fabian Jahr)
b2e8b64ddc351124ac1390ee906a8fcd2781ca50 index, refactor: Append blocks to coinstatsindex without db read (Fabian Jahr)
431a076ae6e3cc32a8725d4a01483d27c5081a34 index: Fix coinstatsindex overflow issue (Fabian Jahr)
84e813a02bb7b3c735ae413f06c0fc156bfeb7ac index, refactor: DRY coinbase check (Fabian Jahr)
fab842b3248744fb0030486f64d3febe815f8377 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 c76797481155754329ec6a6f58e8402569043944
TheCharlatan:
ACK c76797481155754329ec6a6f58e8402569043944
mzumsande:
Tested / Code Review ACK c76797481155754329ec6a6f58e8402569043944
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`
Currently, if the user inadvertently starts the node with duplicate bind options,
such as `-bind=0.0.0.0 -bind=0.0.0.0`, it will cause a fatal error with the
misleading message "Bitcoin Core is probably already running".
This commit adds early validation to detect duplicate bindings across all binding
configurations (-bind, -whitebind, and onion bindings) before attempting to bind.
When duplicates are detected, the node terminates with a clear, specific error
message: "Duplicate binding configuration for address <addr>. Please check your
-bind, -bind=...=onion and -whitebind settings."
The validation catches duplicates both within the same option type (e.g.,
`-bind=X -bind=X`) and across different types (e.g., `-bind=X -whitebind=Y@X`),
helping users identify and fix configuration mistakes.
2885bd0e1c4fc863a7f28ff0fd353f5cffb03442 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 2885bd0e1c4fc863a7f28ff0fd353f5cffb03442. 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 2885bd0e1c4fc863a7f28ff0fd353f5cffb03442
kevkevinpal:
ACK [2885bd0](2885bd0e1c)
achow101:
ACK 2885bd0e1c4fc863a7f28ff0fd353f5cffb03442
janb84:
ACK 2885bd0e1c4fc863a7f28ff0fd353f5cffb03442
Zero-1729:
crACK 2885bd0e1c4fc863a7f28ff0fd353f5cffb03442
jonatack:
ACK 2885bd0e1c4fc863a7f28ff0fd353f5cffb03442
hodlinator:
ACK 2885bd0e1c4fc863a7f28ff0fd353f5cffb03442
w0xlt:
ACK 2885bd0e1c
optout21:
ACK 2885bd0e1c4fc863a7f28ff0fd353f5cffb03442
Tree-SHA512: a9d2a64ab96b3dd7f3a1a29622930054fd5c56e573bc96330f4ef3327dc024b21b3fbc8a698d17aea7c76f57f0c2ccd6403b2df344ae2f69c645ceb8b6fa54a5
a602f6fb7bf5f9e57299f4d6e246c82379fad8d2 test: index with an unclean restart after a reorg (Martin Zumsande)
01b95ac6f496e24e525b2fc9d69ee8b543da65ff 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 a602f6fb7bf5f9e57299f4d6e246c82379fad8d2
stickies-v:
re-ACK a602f6fb7bf5f9e57299f4d6e246c82379fad8d2
Tree-SHA512: 2559ea3fe066caf746a54ad7daac5031332f3976848e937c3dc8b35fa2ce925674115d8742458bf3703b3916f04f851c26523b6b94aeb1da651ba5a1b167a419
5c8bf7b39e9bffba7c4d5778b56b1ebe72f5ea1b doc: add release notes for version 3 transactions (ishaanam)
4ef8065a5e3d2fe9cd3d7a71224ef2ca2e7b495a test: add truc wallet tests (ishaanam)
5d932e14dbe41c349ab41f88088398e0ab10d335 test: extract `bulk_vout` from `bulk_tx` so it can be used by wallet tests (ishaanam)
2cb473d9f2152e33bab2c3c626801deb7841aa20 rpc: Support version 3 transaction creation (Bue-von-hon)
4c20343b4d318be62086676e0898e56221500de1 rpc: Add transaction min standard version parameter (Bue-von-hon)
c5a2d080116270ecd0414c14eb412fa30eaaedaf wallet: don't return utxos from multiple truc txs in AvailableCoins (ishaanam)
da8748ad626fc5813eb06244630e12c8ceb3cedf wallet: limit v3 tx weight in coin selection (ishaanam)
85c54106156f5bbac87f4442a0a27f1b9187125b wallet: mark unconfirmed v3 siblings as mempool conflicts (ishaanam)
0804fc3cb11089000d3b0e8bed41df0b0bf5fff1 wallet: throw error at conflicting tx versions in pre-selected inputs (ishaanam)
cc155226fee1f5c9a40ec37f6276e45c9c42b26a wallet: set m_version in coin control to default value (ishaanam)
2e9617664e70b5e586c485e7c65ce342ffd66cdf wallet: don't include unconfirmed v3 txs with children in available coins (ishaanam)
ec2676becdf488f7a1151345a019c05dec926308 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 5c8bf7b39e9
achow101:
ACK 5c8bf7b39e9bffba7c4d5778b56b1ebe72f5ea1b
rkrux:
ACK 5c8bf7b39e9bffba7c4d5778b56b1ebe72f5ea1b
Tree-SHA512: da8aea51c113e193dd0b442eff765bd6b8dc0e5066272d3e52190a223c903f48788795f32c554f268af0d2607b5b8c3985c648879cb176c65540837c05d0abb5
60d1042b9a4db8daf9fffdc29053652e99b7126e wallet: Remove unused `WalletFeature` enums (woltx)
66de58208a713e16f0d48bceed4d7496eae4b05b wallet: Remove `CWallet::nWalletVersion` and related functions (woltx)
7cda3d0f5bdca64b11f966a60167cde5451071a3 wallet: Remove `IsFeatureSupported()` and `CanSupportFeature()` (woltx)
ba0158522981287f2fde83f38392baac0216b0b4 wallet: `MigrateToDescriptor` no longer calls `CanSupportFeature` (woltx)
63acee279756e72f96fda14a9963281860bf318b wallet: Remove `GetClosestWalletFeature()` (woltx)
e27da3150b48ccf106ba93044bd28c6d1f505421 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 60d1042b9a4db8daf9fffdc29053652e99b7126e 🐾
achow101:
ACK 60d1042b9a4db8daf9fffdc29053652e99b7126e
pablomartin4btc:
ACK 60d1042b9a4db8daf9fffdc29053652e99b7126e
Tree-SHA512: 1a7ad8e15d57df8f66545776e7d178a2cd5312c87769a29770588375e3de5f24247aab9919acf004ed3eca16d08ba595b5f1c7b2b3eef7752e89d9c295624583
fab2980bdc55b5c77f574f879a6ab62db5eda427 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 fab2980bdc55b5c77f574f879a6ab62db5eda427
ajtowns:
crACK fab2980bdc55b5c77f574f879a6ab62db5eda427
davidgumberg:
untested crACK fab2980bdc
Tree-SHA512: e90b66f7423b639356daace476942ce83e65e70466544394cbe2f15738bdbf716163eaf590c64c5448f9b41aeeaafe3342c48c6a7a478678a70b0310ca94e11d
c0d91fc69c67e6f7123326d4f3caeac069d2637b Add release note for #33050 and #33183 error string changes (Antoine Poinsot)
b3f781a0ef4b763ef7ba8b5b20871a7707ec090e contrib: adapt max reject string size in tracing demo (Antoine Poinsot)
9a04635432183c437829339dbf10e7d702581010 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 c0d91fc69c67e6f7123326d4f3caeac069d2637b
davidgumberg:
lgtm ACK c0d91fc69c
ajtowns:
utACK c0d91fc69c67e6f7123326d4f3caeac069d2637b
Crypt-iQ:
utACK c0d91fc69c67e6f7123326d4f3caeac069d2637b
janb84:
utACK c0d91fc69c67e6f7123326d4f3caeac069d2637b
instagibbs:
ACK c0d91fc69c67e6f7123326d4f3caeac069d2637b
Tree-SHA512: b463e633c57dd1eae7c49d23239a59066a672f355142ec194982eddc927a7646bc5cde583dc8d6f45075bf5cbb96dbe73f7e339e728929b0eff356b674d1b68c
ba84a25deec0b3b9b94ee51b373e715fec995791 [doc] update mempool-replacements.md for incremental relay feerate change (glozow)
18720bc5d5b4d3acf91060859180d72cbfdf59b7 [doc] release note for min feerate changes (glozow)
6da5de58cabc4133c379baa50845e30e5bc6b3e4 [policy] lower default minrelaytxfee and incrementalrelayfee to 100sat/kvB (glozow)
2e515d2897eaa5a9b012eb78aef105e1cf80d42b [prep/test] make wallet_fundrawtransaction's minrelaytxfee assumption explicit (glozow)
457cfb61b5323a13218b3cfb5a6a6d8b3a7c5f7f [prep/util] help MockMempoolMinFee handle more precise feerates (glozow)
3eab8b724044dc321f70e5eed66b149713158a04 [prep/test] replace magic number 1000 with respective feerate vars (glozow)
5f2df0ef78be7b24798d0983c9b962740608f1f4 [miner] lower default -blockmintxfee to 1sat/kvB (glozow)
d6213d6aa114aeed6804a585491d741386fd2739 [doc] assert that default min relay feerate and incremental are the same (glozow)
1fbee5d7b61b83e68e4230c8a97ca308de92c4c3 [test] explicitly check default -minrelaytxfee and -incrementalrelayfee (glozow)
72dc18467dbfc16cdbda2dd109b087243b397799 [test] RBF rule 4 for various incrementalrelayfee settings (glozow)
85f498893f54ea7d84f2bdf12aa35d198edf8a72 [test] check bypass of minrelay for various minrelaytxfee settings (glozow)
e5f896bb1f052fb8c7811c6024cb49143b427512 [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 ba84a25deec0b3b9b94ee51b373e715fec995791
gmaxwell:
ACK ba84a25deec0b3b9b94ee51b373e715fec995791
jsarenik:
Tested ACK ba84a25deec0b3b9b94ee51b373e715fec995791
darosior:
ACK ba84a25deec0b3b9b94ee51b373e715fec995791
ajtowns:
ACK ba84a25deec0b3b9b94ee51b373e715fec995791
davidgumberg:
crACK ba84a25dee
w0xlt:
ACK ba84a25dee
caesrcd:
reACK ba84a25deec0b3b9b94ee51b373e715fec995791
ismaelsadeeq:
re-ACK ba84a25deec0b3b9b94ee51b373e715fec995791
Tree-SHA512: b4c35e8b506b1184db466551a7e2e48bb1e535972a8dbcaa145ce3a8bfdcc70a8807dc129460f129a9d31024174d34077154a387c32f1a3e6831f6fa5e9c399e
5c74a0b397cb3db94761bad78801eed4544155b9 config: add DEBUG_ONLY -logratelimit (Eugene Siegel)
9f3b017bcc067bba1d1682a5d4e65b5450dc10c4 test: logging_filesize_rate_limit improvements (stickies-v)
350193e5e2efabb3eb66197b91869b946ec5428c test: don't leak log category mask across tests (stickies-v)
05d7c22479bf96bab9f8c8b8fa90368429ad2c88 test: add ReadDebugLogLines helper function (stickies-v)
3d630c2544e19480268426cda245796d4ce34ac3 log: make m_limiter a shared_ptr (stickies-v)
e8f9c37a3b4c9c88baddb556c4b33a4cbba1f614 log: clean up LogPrintStr_ and Reset, prefix all logs with "[*]" when there are suppressions (Eugene Siegel)
3c7cae49b692bb6bf5cae5ee23479091bed0b8be log: change LogLimitStats to struct LogRateLimiter::Stats (Eugene Siegel)
8319a134684df2240057a5e8afaa6ae441fb8a58 log: clarify RATELIMIT_MAX_BYTES comment, use RATELIMIT_WINDOW (Eugene Siegel)
5f70bc80df06ca85d44e8201d47e7086e971fdea log: remove const qualifier from arguments in LogPrintFormatInternal (Eugene Siegel)
b8e92fb3d4137f91fe6a54829867fc54357da648 log: avoid double hashing in SourceLocationHasher (Eugene Siegel)
616bc22f131132b9239ef362dca8c6bce000a539 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 5c74a0b397cb3db94761bad78801eed4544155b9
achow101:
ACK 5c74a0b397cb3db94761bad78801eed4544155b9
l0rinc:
Code review ACK 5c74a0b397cb3db94761bad78801eed4544155b9
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-
876dbdfb4702410dfd4037614dc9298a0c09c63e tests: drop expect_disconnect behaviour for tx relay (Anthony Towns)
b29ae9efdfeeff774e32ee433ce67d8ed8ecd49f validation: only check input scripts once (Anthony Towns)
266dd0e10d08c0bfde63205db15d6c210a021b90 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 876dbdfb4702410dfd4037614dc9298a0c09c63e
darosior:
re-ACK 876dbdfb4702410dfd4037614dc9298a0c09c63e
sipa:
re-ACK 876dbdfb4702410dfd4037614dc9298a0c09c63e
glozow:
ACK 876dbdfb4702410dfd4037614dc9298a0c09c63e
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.
83950275eddacac56c58a7a3648ed435a5593328 qa: unit test sighash caching (Antoine Poinsot)
b221aa80a081579b8d3b460e3403f7ac0daa7139 qa: simple differential fuzzing for sighash with/without caching (Antoine Poinsot)
92af9f74d74e76681f7d98f293eab226972137b4 script: (optimization) introduce sighash midstate caching (Pieter Wuille)
8f3ddb0bccebc930836b4a6745a7cf29b41eb302 script: (refactor) prepare for introducing sighash midstate cache (Pieter Wuille)
9014d4016ad9351cb59b587541895e55f5d589cc 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 83950275eddacac56c58a7a3648ed435a5593328
dergoegge:
Code review ACK 83950275eddacac56c58a7a3648ed435a5593328
darosior:
re-ACK 83950275eddacac56c58a7a3648ed435a5593328
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.
27aefac42505e9c083fa131d3d7edbec7803f3c0 validation: detect witness stripping without re-running Script checks (Antoine Poinsot)
2907b58834ab011f7dd0c42d323e440abd227c25 policy: introduce a helper to detect whether a transaction spends Segwit outputs (Antoine Poinsot)
eb073209db9efdbc2c94bc1f535a27ec6b20d954 qa: test witness stripping in p2p_segwit (Antoine Poinsot)
Pull request description:
Since it was introduced in 4eb515574e1012bc8ea5dafc3042dcdf4c766f26 (#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 27aefac42505e9c083fa131d3d7edbec7803f3c0
Crypt-iQ:
re-ACK 27aefac42505e9c083fa131d3d7edbec7803f3c0
glozow:
reACK 27aefac42505e9c083fa131d3d7edbec7803f3c0
Tree-SHA512: 70cf76b655b52bc8fa2759133315a3f11140844b6b80d9de3c95f592050978cc01a87bd2446e3a9c25cc872efea7659d6da3337b1a709511771fece206e9f149
