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
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
This was in fact untested until now. This can be checked with the following diff.
```diff
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 0c4a89c44cb..f8b9adf910a 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -1822,7 +1822,7 @@ void PeerManagerImpl::MaybePunishNodeForBlock(NodeId nodeid, const BlockValidati
}
case BlockValidationResult::BLOCK_INVALID_HEADER:
case BlockValidationResult::BLOCK_INVALID_PREV:
- if (peer) Misbehaving(*peer, message);
+ if (!via_compact_block && peer) Misbehaving(*peer, message);
return;
// Conflicting (but not necessarily invalid) data or different policy:
case BlockValidationResult::BLOCK_MISSING_PREV:
```
b6d4688f77 [doc] reword comments in test_mid_package_replacement (glozow)
f3a613aa5b [cleanup] delete brittle test_mid_package_eviction (glozow)
c3cd7fcb2c [doc] remove references to now-nonexistent Finalize() function (glozow)
d8140f5f05 don't make a copy of m_non_base_coins (glozow)
98ba2b1db2 [doc] MemPoolAccept coins views (glozow)
ba02c30b8a [doc] always CleanupTemporaryCoins after a mempool trim (glozow)
Pull request description:
Deletes `test_mid_package_eviction` that is brittle and already covered in other places. It was introduced in #28251 addressing 2 issues: (1) calling `LimitMempoolSize()` in the middle of package validation and (2) not updating coins view cache when the mempool contents change, leading to "disappearing coins."
(1) If you let `AcceptSingleTransaction` call `LimitMempoolSize` in the middle of package validation, you should get a failure in `test_mid_package_eviction_success` (the package is rejected):
```
diff --git a/src/validation.cpp b/src/validation.cpp
index f2f6098e214..4bd6f059849 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -1485,7 +1485,7 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef
FinalizeSubpackage(args);
// Limit the mempool, if appropriate.
- if (!args.m_package_submission && !args.m_bypass_limits) {
+ if (!args.m_bypass_limits) {
LimitMempoolSize(m_pool, m_active_chainstate.CoinsTip());
// If mempool contents change, then the m_view cache is dirty. Given this isn't a package
// submission, we won't be using the cache anymore, but clear it anyway for clarity.
```
Mempool modifications have a pretty narrow interface since #31122 and `TrimToSize()` cannot be called while there is an outstanding mempool changeset. So I think there is a low likelihood of accidentally reintroducing this problem and not immediately hitting e.g. a fuzzer crash on this line b53fab1467/src/txmempool.cpp (L1143)
(2) If you remove the `CleanupTemporaryCoins()` call from `ClearSubPackageState()` you should get a failure from `test_mid_package_replacement`:
```
diff --git a/src/validation.cpp b/src/validation.cpp
index f2f6098e214..01b904b69ef 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -779,7 +779,7 @@ private:
m_subpackage = SubPackageState{};
// And clean coins while at it
- CleanupTemporaryCoins();
+ // CleanupTemporaryCoins();
}
};
```
I also added/cleaned up the documentation about coins views to hopefully make it extremely clear when people should `CleanupTemporaryCoins`.
ACKs for top commit:
instagibbs:
reACK b6d4688f77
sdaftuar:
utACK b6d4688f77
marcofleon:
ACK b6d4688f77
Tree-SHA512: 79c68e263013b1153520f5453e6b579b8fe7e1d6a9952b1ac2c3c3c017034e6d21d7000a140bba4cc9d2ce50ea3a84cc6f91fd5febc52d7b3fa4f797955d987d
Submit the block with an invalid transaction Script again, leading to
CACHED_INVALID being returned by AcceptBlockHeader(). Ensure that also this
code path does not lead to a disconnection.
This was previously untested, as can be checked with the following diff:
```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:
```
The current test to exercise a block with an invalid transaction actually
creates a block with an invalid coinbase witness, which is checked early and
for which MaybePunishNodeForBlock() is not called.
Add a test case with an invalid regular transaction, which will lead
CheckInputScripts to return a CONSENSUS error and MaybePunishNodeForBlock() to
be called, appropriately not disconnecting upon an invalid compact block. This
was until now untested as can be checked with the following diff:
```diff
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 0c4a89c44cb..d243fb88d4b 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:
{
```
Finally, note this failure is cached (unlike the malleated witness failure),
which will be used in the following commits.
The error being checked here is BLOCK_MUTATED, as returned by IsBlockMutated()
in FillBlock(). Dropping the fourth transaction from the block is unnecessary
and would make testing of other block validation failures in following commits
more verbose.
1c10b7351e RPC: Return permitbaremultisig and maxdatacarriersize in getmempoolinfo (Kristaps Kaupe)
Pull request description:
Other node relay settings like `fullrbf` and `minrelaytxfee` are already returned, makes sense to add these two too.
ACKs for top commit:
ajtowns:
ACK 1c10b7351e
maflcko:
lgtm ACK 1c10b7351e
theStack:
ACK 1c10b7351e
Tree-SHA512: 1750d7d12de511f0ac34922ea9c58c4b9b55c3aaf22109abfd7dbe01ad1eb7b48fb4a6b074a0baf0e55ee2270fcc969b6830e499ff33adbcd0b9c761fb25e563
c5c1960f93 doc: Add release notes for changes in RPCs (pablomartin4btc)
90fd5acbe5 rpc, test: Fix error message in getdescriptoractivity (pablomartin4btc)
39fef1d203 test: Add missing logging info for each test (pablomartin4btc)
53ac704efd rpc, test: Fix error message in unloadwallet (pablomartin4btc)
1fc3a8e8e7 rpc, test: Add EnsureUniqueWalletName tests (pablomartin4btc)
b635bc0896 rpc, util: Add EnsureUniqueWalletName (pablomartin4btc)
Pull request description:
Currently, `unloadwallet` RPC call fails with a JSON parsing error when no `wallet_name` argument is provided. This behavior is misleading because the error originates from a low-level JSON type mismatch, rather than clearly indicating that the wallet name or RPC endpoint (`-rpcwallet=...`) is missing. Also, found out that the [issue](https://github.com/bitcoin/bitcoin/pull/13111#issuecomment-398831543) was noticed during its implementation but never addressed.
In addition, I've verified all RPC commands calls finding that `getdescriptoractivity` had the same problem, but related to the array input types (blockhashes & descriptors), so I've corrected that RPC as well. For consistency I've added the missing logging info for each test case in `test/functional/rpc_getdescriptoractivity.py` in preparation for the new test.
**_-Before_**
```
./build/bin/bitcoin-cli -regtest -datadir=/tmp/btc unloadwallet
error code: -3
error message:
JSON value of type number is not of expected type string
```
```
./build/bin/bitcoin-cli -regtest -datadir=/tmp/btc getdescriptoractivity
error code: -3
error message:
JSON value of type null is not of expected type array
```
```
./build/bin/bitcoin-cli -regtest -datadir=/tmp/btc getdescriptoractivity '[]'
error code: -3
error message:
JSON value of type null is not of expected type array
```
**_-After_**
```
./build/bin/bitcoin-cli -regtest -datadir=/tmp/btc unloadwallet
error code: -8
error message:
Either the RPC endpoint wallet or the wallet name parameter must be provided
```
```
./build/bin/bitcoin-cli -regtest -datadir=/tmp/btc getdescriptoractivity
error code: -1
error message:
getdescriptoractivity ["blockhash",...] [scanobjects,...] ( include_mempool )
Get spend and receive activity associated with a set of descriptors for a set of blocks. This command pairs well with the `relevant_blocks` output of `scanblocks()`.
This call may take several minutes. If you encounter timeouts, try specifying no RPC timeout (bitcoin-cli -rpcclienttimeout=0)
Arguments:
1. blockhashes (json array, required) The list of blockhashes to examine for activity. Order doesn't matter. Must be along main chain or an error is thrown.
[
"blockhash", (string) A valid blockhash
...
]
2. scanobjects (json array, required) Array of scan objects. Every scan object is either a string descriptor or an object:
[
"descriptor", (string) An output descriptor
{ (json object) An object with output descriptor and metadata
"desc": "str", (string, required) An output descriptor
"range": n or [n,n], (numeric or array, optional, default=1000) The range of HD chain indexes to explore (either end or [begin,end])
},
...
]
3. include_mempool (boolean, optional, default=true) Whether to include unconfirmed activity
...
```
```
./build/bin/bitcoin-cli -regtest -datadir=/tmp/btc getdescriptoractivity '[]'
error code: -1
error message:
getdescriptoractivity ["blockhash",...] [scanobjects,...] ( include_mempool )
...
```
ACKs for top commit:
achow101:
ACK c5c1960f93
stickies-v:
re-ACK c5c1960f93
furszy:
ACK c5c1960f93
Tree-SHA512: e831ff1acbfd15d2ce3a69bb408cce94664c0b63b2aa2f4627a05c6c052241ae3b5cc238219ef1b30afb489a4a3f4c3030e2168b0c8f08b4d20805d050d810f5
It is redundant with -logsourcelocations and the log messages are
clearer without it.
Also, remove a double-space.
Also, add braces around `if` touched in the next commit.
This tiny behavior change requires a test fixup.
This rule was originally introduced along with a very early proposal for
package relay as a way to verify that the "correct"
child-with-unconfirmed-parents package was provided for a transaction,
where correctness was defined as all of the transactions unconfirmed
parents. However, we are not planning to introduce a protocol where
peers would be asked to send these packages.
This rule has downsides: if a transaction has multiple parents but only
1 that requires package CPFP to be accepted, the current rule prevents
us from accepting that package. Even if the other parents are already in
mempool, the p2p logic will only submit the 1p1c package, which fails
this check. See the test in p2p_1p1c_network.py
060695c22a test: Failed load after migrate should restore backup (MarcoFalke)
8a4cfddf23 wallet: Set migrated wallet name only on success (Ava Chow)
Pull request description:
After a wallet is migrated and we are trying to load it, if it could not be loaded, don't try to set the wallet name. Otherwise we have a segfault.
This can be tested by migrated a legacy wallet from another network (e.g. trying to migrate a testnet wallet on mainnet). The fixed behavior is return an error and restore the backup.
ACKs for top commit:
davidgumberg:
ACK 060695c22a
furszy:
ACK 060695c22a
rkrux:
ACK 060695c22a
w0xlt:
reACK 060695c22a
pablomartin4btc:
ACK 060695c22a
Tree-SHA512: f4289e0b3dedef0a3d734c18604f2fd0df0dc65e9641bc342cfa45088d2540a3f6631bbea8bdd394b2631fa83b38e8ac37c83cfc4b53b19dcbd0b820a9beb6e4
We just need enough transactions to push us above the orphanage limits
and trigger trimming. Reducing the number of transactions cuts the
runtime of this test significantly.
faa3e68411 test: Log KeyboardInterrupt as exception (MarcoFalke)
fa30b34026 test: Do not pass tests on unhandled exceptions (MarcoFalke)
Pull request description:
Currently the functional tests are problematic, because they pass, even if they encounter an unhanded exception.
Fix this by handling all exceptions: Catch `BaseException` as fallback and mark it as failure.
Can be tested via:
```diff
diff --git a/test/functional/wallet_disable.py b/test/functional/wallet_disable.py
index da6e5d408f..ecc41fb041 100755
--- a/test/functional/wallet_disable.py
+++ b/test/functional/wallet_disable.py
@@ -19,6 +19,7 @@ class DisableWalletTest (BitcoinTestFramework):
self.wallet_names = []
def run_test (self):
+ import sys;sys.exit("fatal error")
# Make sure wallet is really disabled
assert_raises_rpc_error(-32601, 'Method not found', self.nodes[0].getwalletinfo)
x = self.nodes[0].validateaddress('3J98t1WpEZ73CNmQviecrnyiWrnqRhWNLy')
```
Previously, the test would pass. With this patch, it would fail.
ACKs for top commit:
enirox001:
Looks good to me—ACK faa3e68
stickies-v:
re-ACK faa3e68411
pablomartin4btc:
tACK faa3e68411
Tree-SHA512: 11ecd5201982e2c776e48d98834b17c15a415306a95524bc702daeba20a316aac797748e9592be8db575597804f149ee7ef104416037cc9e5891758625810e2d
Mark blockhashes and scanobjects arguments as required, so the user receives
a clear help message when either is missing.
Added a new functional test for this use case.
Co-authored-by: stickies-v <stickies-v@users.noreply.github.com>
Add self.log.info(...) calls at the beginning of each test
in GetBlocksActivityTest.
This improves readability and provides debugging information
by logging the purpose of each test upon its correct
execution.
This is in preparation for the next commit, which adds a new test
with log info, and it would look inconsistent without this commit.
