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
A stripped witness is detected as a special case in mempool acceptance to make sure we do not add
the wtxid (which is =txid since witness is stripped) to the reject filter. This is because it may
interfere with 1p1c parent relay which currently uses orphan reconciliation (and originally it was
until wtxid-relay was widely adopted on the network.
This commit adds a test for this special case in the p2p_segwit function test, both when spending
a native segwit output and when spending a p2sh-wrapped segwit output.
Thanks to Eugene Siegel for pointing out the p2sh-wrapped detection did not have test coverage by
finding a bug in a related patch of mine.
c0642e558a [fuzz] fix latency score check in txorphan_protected (glozow)
3d4d4f0d92 scripted-diff: rename "ann" variables to "latency_score" (monlovesmango)
3b92448923 [doc] comment fixups for orphanage changes (glozow)
1384dbaf6d [config] emit warning for -maxorphantx, but allow it to be set (glozow)
b10c55b298 fix up TxOrphanage lower_bound sanity checks (glozow)
cfd71c6704 scripted-diff: rename TxOrphanage outpoints index (glozow)
edb97bb3f1 [logging] add logs for inner loop of LimitOrphans (glozow)
8a58d0e87d scripted-diff: rename OrphanTxBase to OrphanInfo (glozow)
cc50f2f0df [cleanup] replace TxOrphanage::Size() with CountUniqueOrphans (glozow)
ed24e01696 [optimization] Maintain at most 1 reconsiderable announcement per wtxid (Pieter Wuille)
af7402ccfa [refactor] make TxOrphanage keep itself trimmed (glozow)
d1fac25ff3 [doc] 31829 release note (glozow)
Pull request description:
Followup to #31829:
- Release notes
- Have the orphanage auto-trim itself whenever necessary (and test changes) https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2169508690
- Reduce duplicate reconsiderations by keeping track of which txns are already reconsiderable so we only mark it for reconsideration for 1 peer at a time https://github.com/bitcoin/bitcoin/pull/31829#issuecomment-3001627814
- Rename `OrphanTxBase` to `OrphanInfo`
- Get rid of `Size()` method by replacing all calls with `CountUniqueOrphans`
- Rename outpoints index since they point to wtxids, not iterators https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2205557613
- Add more logging in the `LimitOrphans` inner loop to make it easy to see which peers are being trimmed https://github.com/bitcoin/bitcoin/pull/31829#issuecomment-3074385460
ACKs for top commit:
sipa:
utACK c0642e558a
marcofleon:
Nice, ACK c0642e558a
Tree-SHA512: f298eae92cf906ed5e4f15a24eeffa7b9e620bcff457772cd77522dd9f0b3b183ffc976871b1b0e6fe93009e64877d518e53d4b9e186e0df58fc16d17f6de90a
eb65f57f31 [test] setmocktime instead of waiting in 1p1c tests (glozow)
70772dd469 [test] cut the number of transactions involved in 1p1c DoS tests (glozow)
Pull request description:
It was brought to my attention that the runtime of this test is Too Damn High. The test is slow due to the many `wait_for_getdata`s with delays (inbound peer + txid request) and the large volume of messages sent in the dos-related tests. This PR cuts the runtime by about 60% by reducing the number of messages/transactions and using `setmocktime` instead of waiting.
On my machine, master:
```
84.51s user 1.55s system 57% cpu 2:28.53 total
```
After first commit (about 1min faster):
```
28.29s user 0.88s system 35% cpu 1:22.84 total
```
After second commit (about 30sec faster):
```
28.17s user 0.87s system 59% cpu 49.082 total
```
Reviewers should verify that the transactions in the DoS tests are still enough to cause evictions, and that the `bumpmocktime` amounts are not more than necessary.
Alternatives:
- If we don't like mocking the times, we can use outbound connections for all the peers. However, that approach won't improve the runtime as much because we impose a 2-second delay on all txid requests regardless of peer type.
- Note that `noban_tx_relay` is not relevant for this test because all delays are related to downloading, not announcing.
ACKs for top commit:
achow101:
ACK eb65f57f31
w0xlt:
ACK eb65f57f31
Tree-SHA512: 6ffe1f9e5144653e2ded744cec9ddb62ad728c587705542565400a0e8f1fba4843aced4e0d929843874ca7f56f670f5871b7e009ff6be58b791ab24d2e6fcc0e
ea17a9423f [doc] release note for relaxing requirement of all unconfirmed parents present (glozow)
12f48d5ed3 test: add chained 1p1c propagation test (Greg Sanders)
525be56741 [unit test] package submission 2p1c with 1 parent missing (glozow)
f24771af05 relax child-with-unconfirmed-parents rule (glozow)
Pull request description:
Broadens the package validation interface, see #27463 for wider context.
On master, package rules include that (1) the package topology must be child-wth-parents (2) all of the child's unconfirmed parents must be present. This PR relaxes the second rule, leaving the first rule untouched (there are plans to change that as well, but not here).
Original motivation for this rule was based on the idea that we would have a child-with-unconfirmed-parents package relay protocol, and this would verify that the peer provided the "correct" package. For various reasons, we're not planning on doing this. We could potentially do this for ancestor packages (with a similar definition that all UTXOs to make the tx valid are available in this package), but it's also questionable whether it's useful to enforce this.
This rule gets in the way of certain usage of 1p1c package relay currently. If a transaction has multiple parents, of which only 1 requires a package CPFP, this rule blocks the package from relaying. Even if all the non-low-feerate parents are already in mempool, when the p2p logic submits the 1p1c package, it gets rejected for not meeting this rule.
ACKs for top commit:
ishaanam:
re-utACK ea17a9423f
instagibbs:
ACK ea17a9423f
Tree-SHA512: c2231761ae7b2acea10a96735e7a36c646f517964d0acb59bacbae1c5a1950e0223458b84c6d5ce008f0c1d53c1605df0fb3cd0064ee535ead006eb7c0fa625b
c157438116 qa: test that we do disconnect upon a second invalid compact block being announced (Antoine Poinsot)
fb2dcbb160 qa: test cached failure for compact block (Antoine Poinsot)
f12d8b104e qa: test a compact block with an invalid transaction (Antoine Poinsot)
d6c37b28a7 qa: remove unnecessary tx removal from compact block (Antoine Poinsot)
Pull request description:
In thinking about https://github.com/bitcoin/bitcoin/pull/33050 and https://github.com/bitcoin/bitcoin/pull/33012#issuecomment-3111631541, i went through the code paths for peer disconnection upon submitting an invalid block. It turns out that the fact we exempt a peer from disconnection upon submitting an invalid compact block was not properly tested, as can be checked with these diffs:
```diff
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 0c4a89c44c..d243fb88d4 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -1805,10 +1805,10 @@ void PeerManagerImpl::MaybePunishNodeForBlock(NodeId nodeid, const BlockValidati
// The node is providing invalid data:
case BlockValidationResult::BLOCK_CONSENSUS:
case BlockValidationResult::BLOCK_MUTATED:
- if (!via_compact_block) {
+ //if (!via_compact_block) {
if (peer) Misbehaving(*peer, message);
return;
- }
+ //}
break;
case BlockValidationResult::BLOCK_CACHED_INVALID:
{
```
```diff
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 0c4a89c44cb..e8e0c805367 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -1814,10 +1814,10 @@ void PeerManagerImpl::MaybePunishNodeForBlock(NodeId nodeid, const BlockValidati
{
// Discourage outbound (but not inbound) peers if on an invalid chain.
// Exempt HB compact block peers. Manual connections are always protected from discouragement.
- if (peer && !via_compact_block && !peer->m_is_inbound) {
+ //if (peer && !via_compact_block && !peer->m_is_inbound) {
if (peer) Misbehaving(*peer, message);
return;
- }
+ //}
break;
}
case BlockValidationResult::BLOCK_INVALID_HEADER:
```
We do have a test for this, but it actually uses a coinbase witness commitment error, which is checked much earlier in `FillBlock`. This PR adds coverage for the two exemptions in `MaybePunishNodeForBlock`.
ACKs for top commit:
kevkevinpal:
ACK [c157438](c157438116)
nervana21:
tACK [c157438](c157438116)
instagibbs:
crACK c157438116
stratospher:
ACK c157438116.
Tree-SHA512: a77d5a9768c0d73f122b06db2e416e80d0b3c3fd261dae8e340ecec2ae92d947d31988894bc732cb6dad2e338b3c82f33e75eb3280f8b0933b285657cf3b212c
4b80147feb test: Perform backup filename checks in migrate_and_get_rpc (Ava Chow)
Pull request description:
Some test cases were unnecessarily checking the backup filename, which involved setting the mocktime before `migrate_and_get_rpc`. However, this could cause a failure if the test was slow since `migrate_and_get_rpc` also sets the mocktime. Since it also already checks that the backup file is named correctly, there's no need for those tests to also do their own mocktime and filename check.
The CI failure can be reproduced locally by adding a sleep to `migrate_and_get_rpc`:
```diff
diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py
index 704204425c7..e87a6100623 100755
--- a/test/functional/wallet_migration.py
+++ b/test/functional/wallet_migration.py
@@ -129,6 +129,7 @@ class WalletMigrationTest(BitcoinTestFramework):
assert_equal(w["warnings"], ["This wallet is a legacy wallet and will need to be migrated with migratewallet before it can be loaded"])
# Mock time so that we can check the backup filename.
+ time.sleep(1)
mocked_time = int(time.time())
self.master_node.setmocktime(mocked_time)
# Migrate, checking that rescan does not occur
```
Fixes#33096
ACKs for top commit:
fjahr:
reACK 4b80147feb
Sammie05:
tACK 4b80147
pablomartin4btc:
utACK 4b80147feb
rkrux:
ACK 4b80147feb
Tree-SHA512: 045d4acf2ad0b56a7083ff2ee5ef09f0d74ad097c01a290660daca096c71fc07109848024256d84f74abbc87dd52691d160f9968b3654726626d3dbd21a84ab6
Some test cases were unnecessarily checking the backup filename, which
involved setting the mocktime before `migrate_and_get_rpc`. However,
this could cause a failure if the test was slow since
`migrate_and_get_rpc` also sets the mocktime. Since it also already
checks that the backup file is named correctly, there's no need for
those tests to also do their own mocktime and filename check.
fac90e5261 test: Check that the GUI interactive reindex works (MarcoFalke)
faaaddaaf8 init: [gui] Avoid UB/crash in InitAndLoadChainstate (MarcoFalke)
Pull request description:
`InitAndLoadChainstate` is problematic, when called twice in the GUI. This can happen when it returns a failure and the user selects an interactive reindex.
There are several bugs that have been introduced since the last time this was working correctly:
* The first one is a crash (assertion failure), which happens due to a cached tip block in the notifiications from the previous run. See https://github.com/bitcoin/bitcoin/pull/31346#discussion_r2207914726
* The second one is UB (use-after-free), which happens because the block index db in the blockmanager is not reset. See https://github.com/bitcoin/bitcoin/pull/30965#discussion_r2207822121
Fix both bugs by resetting any dirty state in `InitAndLoadChainstate`.
Also, add a test, because I don't really want to keep testing this manually every time. (A failing test run can be seen in https://github.com/bitcoin/bitcoin/pull/32979/checks)
ACKs for top commit:
achow101:
ACK fac90e5261
TheCharlatan:
ACK fac90e5261
mzumsande:
Tested ACK fac90e5261
Tree-SHA512: 9f744d36e7cdd3f5871764386ec5a5cca1ae144f1bacc26c07e60313c2bdacdc5fca351aa185cb51359540eea4534dda17e4fb6073ad90f91ba0a6936faeead8
c6e2c31c55 rpc: unhide waitfor{block,newblock,blockheight} (Sjors Provoost)
0786b7509a rpc: add optional blockhash to waitfornewblock (Sjors Provoost)
Pull request description:
The `waitfornewblock` is inherently racy as the tip may have changed since the last RPC call, and can even change during initial processing of this call.
Add an optional `blockhash` argument so the caller can specify their current tip. Return immediately if our tip is different.
I've made it fail if `LookupBlockIndex` fails. This should never happen if the user got the block hash from our RPC in the first place.
Finally, the `waitfor{block,newblock,blockheight}` RPC methods are no longer hidden in `help`:
- the changes in #30409 ensured these methods _could_ work in the GUI
- #31785 removed the guards that prevented GUI users from using them
- this PR makes `waitfornewblock` reliable
So there's no more reason to hide them.
ACKs for top commit:
TheCharlatan:
Re-ACK c6e2c31c55
ryanofsky:
Code review ACK c6e2c31c55. Just rebased and tweaked documentation since last review.
glozow:
utACK c6e2c31c55
Tree-SHA512: 84a0c94cb9a2e4449e7a395cf3dce1650626bd852e30e0e238a1aafae19d57bf440bfac226fd4da44eaa8d1b2fa4a8c1177b6c716235ab862a72ff5bf8fc67ac
76fe0e59ec test: Migration of a wallet ending in `../` (David Gumberg)
f0bb3d50fe test: Migration of a wallet ending in `/` (David Gumberg)
41faef5f80 test: Migration fail recovery w/ `../` in path (David Gumberg)
63c6d36437 test: Migration of a wallet with `../` in path. (David Gumberg)
70f1c99c90 wallet: Fix migration of wallets with pathnames. (David Gumberg)
f6ee59b6e2 wallet: migration: Make backup in walletdir (David Gumberg)
e22c3599c6 test: wallet: Check direct file backup name. (David Gumberg)
Pull request description:
Support for wallets outside of the default wallet directory was added in #11687, and these external wallets can be specified with paths relative to the wallet directory, e.g. `bitcoin-cli loadwallet ../../mywallet`. In the RPC commands, there is no distinction between a wallet's 'name' and a wallet's 'path'. This PR fixes an issue with wallet backup during migration where the wallet's 'name-path' is used in the backup filename. This goes south when that filename is appended to the directory where we want to put the file and the wallet's 'name' actually gets treated as a path:
```cpp
fs::path backup_filename = fs::PathFromString(strprintf("%s_%d.legacy.bak", (wallet_name.empty() ? "default_wallet" : wallet_name), GetTime()));
fs::path backup_path = this_wallet_dir / backup_filename;
```
Attempting to migrate a wallet with the 'name' `../../../mywallet` results in a backup being placed in `datadir/wallets/../../../mywallet/../../../mywallet_1744683963.legacy.bak`.
If permissions don't exist to write to that folder, migration can fail.
The solution implemented here is to put backup files in the top-level of the node's `walletdir` directory, using the folder name (and in some rare cases the file name) of the wallet to name the backup file:
9fa5480fc4/src/wallet/wallet.cpp (L4254-L4268)
##### Steps to reproduce on master
Build and run `bitcoind` with legacy wallet creation enabled:
```bash
$ cmake -B build -DWITH_BDB=ON && cmake --build build -j $(nproc)
$ ./build/bin/bitcoind -regtest -deprecatedrpc=create_bdb
```
Create a wallet with some relative path specifiers (exercise caution with where this file may be written)
```bash
$ ./build/bin/bitcoin-cli -regtest -named createwallet wallet_name="../../../myrelativewallet" descriptors=false
```
Try to migrate the wallet:
```bash
$ ./build/bin/bitcoin-cli -regtest -named migratewallet wallet_name="../../../myrelativewallet"
```
You will see a message in `debug.log` about trying to backup a file somewhere like: `/home/user/.bitcoin/regtest/wallets/../../../myrelativewallet/../../../myrelativewallet_1744686627.legacy.bak` and migration might fail because `bitcoind` doesn't have permissions to write the backup file.
ACKs for top commit:
pablomartin4btc:
tACK 76fe0e59ec
achow101:
ACK 76fe0e59ec
ryanofsky:
Code review ACK 76fe0e59ec. Nice changes that (1) fix potential errors when names of wallets being migrated contain slashes, and (2) store migration backups in the top-level `-walletdir` instead of in individual wallet subdirectories.
Tree-SHA512: 5cf6ed9f44ac7d204e4e9854edd3fb9b43812e930f76343b142b3c19df3de2ae5ca1548d4a8d26226d537bca231e3a50b3ff0d963c200303fb761f2b4eb3f0d9
aac0b6dd79 test: test sendall and send do anti-fee-sniping (ishaanam)
20802c7b65 wallet, rpc: add anti-fee-sniping to `send` and `sendall` (ishaanam)
Pull request description:
Currently, `send` and `sendall` don't do anti-fee-sniping because they don't use `CreateTransaction`. This PR adds anti-fee-sniping to these RPCs by calling `DiscourageFeeSniping` from `FinishTransaction` when the user does not specify a locktime.
ACKs for top commit:
achow101:
ACK aac0b6dd79
murchandamus:
ACK aac0b6dd79
glozow:
ACK aac0b6dd79
Tree-SHA512: d4f1b43b5bda489bdba46b0af60e50bff0de604a35670e6ea6e1de2b539f16b3f68805492f51d6d2078d421b63432ca22a561a5721d1a37686f2e48284e1e646
