ad132761fc49c38769c09653a265fdbc3b93eda5 [allocators] Apply manual ASan poisoning to PoolResource (dergoegge)
Pull request description:
Currently ASan will not detect use-after-free issues for memory allocated by a `PoolResource`. This is because ASan is only aware of the memory chunks allocated by `PoolResource` but not the individual "sub-chunks" within.
E.g. this test will not produce an ASan error even though the referenced coin has been deallocated:
```c++
diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp
index c46144b34b..aa6ca15ce1 100644
--- a/src/test/coins_tests.cpp
+++ b/src/test/coins_tests.cpp
@@ -508,6 +508,17 @@ BOOST_FIXTURE_TEST_CASE(updatecoins_simulation_test, UpdateTest)
BOOST_CHECK(spent_a_duplicate_coinbase);
}
+BOOST_AUTO_TEST_CASE(asan_uaf)
+{
+ CCoinsMapMemoryResource cache_coins_memory_resource{};
+ CCoinsMap map(0, SaltedOutpointHasher(/*deterministic=*/true), CCoinsMap::key_equal{}, &cache_coins_memory_resource);
+ COutPoint outpoint{};
+ map.emplace(outpoint, Coin{});
+ auto& coin = map.at(outpoint);
+ map.erase(outpoint);
+ coin.coin.nHeight = 1;
+}
+
BOOST_AUTO_TEST_CASE(ccoins_serialization)
{
// Good example
```
Fix this by applying [manual ASan poisoning](https://github.com/google/sanitizers/wiki/AddressSanitizerManualPoisoning) for memory allocated by `PoolResource`:
* Newly allocated chunks are poisoned as a whole
* "Sub-chunks" are unpoisoned/re-poisoned during allocation/deallocation
With the poisoning applied, ASan catches the issue in the test above:
```
$ ./build_unit/bin/test_bitcoin --run_test="coins_tests/asan_uaf"
Running 1 test case...
=================================================================
==366064==ERROR: AddressSanitizer: use-after-poison on address 0x7f99c3204870 at pc 0x55569dab6f8a bp 0x7ffe0210e4d0 sp 0x7ffe0210e4c8
READ of size 4 at 0x7f99c3204870 thread T0 (b-test)
```
ACKs for top commit:
achow101:
ACK ad132761fc49c38769c09653a265fdbc3b93eda5
marcofleon:
code review ACK ad132761fc49c38769c09653a265fdbc3b93eda5
Tree-SHA512: eb5e80bfa9509225e784151807bd8aa21fb0826ca1781dfe81b1d60bd3766019384ea3f9cb8e53398fde2f4e994a9c201b5a9962b4d279d7e52bb60e8961be11
1252eeb997df2eb12c33d92eb1a5c9d6643a67ff rpc: fix getpeerinfo ping duration unit docs (0xb10c)
Pull request description:
The docs have been incorrect since a3789c700b5a43efd4b366b4241ae840d63f2349 (released in v25; master since Sept. 2022). Noticed while setting up monitoring using getpeerinfo.
0cb1ed2b7c/src/rpc/net.cpp (L249-L257)
ACKs for top commit:
luke-jr:
utACK 1252eeb997df2eb12c33d92eb1a5c9d6643a67ff
maflcko:
lgtm ACK 1252eeb997df2eb12c33d92eb1a5c9d6643a67ff
jonatack:
ACK 1252eeb997df2eb12c33d92eb1a5c9d6643a67ff
theStack:
ACK 1252eeb997df2eb12c33d92eb1a5c9d6643a67ff
janb84:
ACK 1252eeb997df2eb12c33d92eb1a5c9d6643a67ff
Tree-SHA512: 33f576336b2a4d9533f51f4641d564ee59ef692c5fa9a3cad239fc31465883d5da534bfd0e069be1e1d688e5f0dea3fe6850be19bf35335041b8f414d08f7f09
The getpeerinfo docs incorrectly specified the ping durations as
milliseconds. This was incorrectly changed in a3789c700b5a43efd4b366b4241ae840d63f2349
(released in v25; master since Sept. 2022). The correct duration unit
is seconds.
Also, remove the documentation of the getpeerinfo RPC response from the
ping RPC since it's incomplete. Better to just reference the getpeerinfo
RPC and it's documenation for this.
e017ef3c7eb775e2cf999674df341be56f7ba72d init: make `-blockmaxweight` startup option debug-only (ismaelsadeeq)
Pull request description:
This PR updates `-blockmaxweight` startup option to be debug-only so that it will be hidden from help text.
The option is currently unlikely to be used on mainnet, after the addition of the new `blockreservedweight` option. however it can be useful for test and signet network see https://github.com/bitcoin/bitcoin/pull/32654#issuecomment-2925674473
ACKs for top commit:
Sjors:
tACK e017ef3c7eb775e2cf999674df341be56f7ba72d
fjahr:
ACK e017ef3c7eb775e2cf999674df341be56f7ba72d
polespinasa:
tACK e017ef3c7eb775e2cf999674df341be56f7ba72d
Tree-SHA512: 6c18781826b2f96b13b70b7f1624481f5971746a613079d0d9528366f274ba657a02611f134d7a64f35ecb7e5faf2e3cd025458b04574ac68f804372f6eb715f
a3cf623364e84819bc16fd407b80d8dba46bbcb5 test: Test max_selection_weight edge cases (Murch)
57fe8acc8a8471ec41ac5f3c3764458431880e4e test: Check max_weight_exceeded error (Murch)
Pull request description:
I tested all of the reported surviving mutants that @brunoerg reported in https://gist.github.com/brunoerg/834063398d5002f738506d741513e310.
I found that all Mutants except for 12, 14, 17, 37, and 39 were now being caught by one of the existing tests. This fixes Mutants 14, 37, and 39.
Mutant 17 is not fixed, because I consider it acceptable that running BnB for 100,001 instead of 100,000 comparisons doesn’t cause an issue, and Mutant 12 is not yet fixed, because at `fee` = `long_term_fee`, the waste of inputs is 0 and only excess matters, and I haven’t evaluated yet, whether it needs to be fixed.
ACKs for top commit:
achow101:
ACK a3cf623364e84819bc16fd407b80d8dba46bbcb5
jlest01:
ACK a3cf623364
brunoerg:
code review ACK a3cf623364e84819bc16fd407b80d8dba46bbcb5
Tree-SHA512: db67c52127ed98f809f64a903c6b3a012e56cf665a0cd851457af7c85c37ec3af8bb72035d7ad370dd883f99cf3014464e3576559899e37c1d6ee01230511754
cc33e4578946c68d6d333f35c48f8cbf3f75f6cf test: improve assertion for SRD max weight test (yancy)
Pull request description:
Replace generic assertion with a result specific assertion showing the correctness of the solution found. If the max weight parameter is exceeded, the least valuable `UTXOs` are removed from the result. Therefore, only the most valued _encountered_ `UTXO's` are selected. While the smallest set would include all the most valued `UTXO's`, in the case of the test there is one high value `UTXO` that is never found before the target value is reached.
Correct the test comment to be more specific about why the assertion is a good result.
ACKs for top commit:
murchandamus:
ACK cc33e4578946c68d6d333f35c48f8cbf3f75f6cf
furszy:
ACK cc33e4578946c68d6d333f35c48f8cbf3f75f6cf
Tree-SHA512: bad224063ba830c27fba1b7b80e411ac7cd6c3edcb60bade4e6e3010f3b5d360a921de742c7c20efea8fa839d7939f338270658f66bbcebedebe5c5c8a3e8f9b
c0642e558a02319ade33dc1014e7ae981663ea46 [fuzz] fix latency score check in txorphan_protected (glozow)
3d4d4f0d92d42809e74377e4380abdc70f74de5d scripted-diff: rename "ann" variables to "latency_score" (monlovesmango)
3b924489238220710326e9031c7aaa0d606c9064 [doc] comment fixups for orphanage changes (glozow)
1384dbaf6d0bfcdb05f97e1e3cb3d5e498bee505 [config] emit warning for -maxorphantx, but allow it to be set (glozow)
b10c55b298d4d2b7dddfecdbeb0edc624b8e6eb2 fix up TxOrphanage lower_bound sanity checks (glozow)
cfd71c67043a2a46950fd3f055afbe4a93922f75 scripted-diff: rename TxOrphanage outpoints index (glozow)
edb97bb3f151600f00c94a2732d2595446011295 [logging] add logs for inner loop of LimitOrphans (glozow)
8a58d0e87d70580ae47da228e2f88cd53c40c675 scripted-diff: rename OrphanTxBase to OrphanInfo (glozow)
cc50f2f0df6e6e2cc9b9aeb3c3c8e1c78fa5be1d [cleanup] replace TxOrphanage::Size() with CountUniqueOrphans (glozow)
ed24e016969098c486f413f4f57dcffe35241785 [optimization] Maintain at most 1 reconsiderable announcement per wtxid (Pieter Wuille)
af7402ccfa7f19177b5f422f596a3ab2bd1e9633 [refactor] make TxOrphanage keep itself trimmed (glozow)
d1fac25ff3c3ac090b68e370efc6dd9374b6ad3b [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 c0642e558a02319ade33dc1014e7ae981663ea46
marcofleon:
Nice, ACK c0642e558a02319ade33dc1014e7ae981663ea46
Tree-SHA512: f298eae92cf906ed5e4f15a24eeffa7b9e620bcff457772cd77522dd9f0b3b183ffc976871b1b0e6fe93009e64877d518e53d4b9e186e0df58fc16d17f6de90a
In the `txgraph` fuzz test, the `CommitStaging` step updates the
`SimTxGraph` levels simply by erasing the front (=main) one in the
`sims` vector, i.e. the staging level instance takes the place of the
main level instance. This also includes the `real_is_optimal` flag
(reflecting whether the corresponding real graph is known to be
optimally linearized), without taking into account that this flag
should only be set if _both_ levels before the commiting are optimal.
E.g. in case of #33097, the main level is not optimally linearized,
while the staging level is, and due to the incorrect propagation of the
latter to the simulation incorrectly assumes that the main level is
optimal, leading to the assertion fail. Fix this by setting the flag
in the resulting main level explicitly.
Resolves the fuzzing assertion fail in issue #33097.
-BEGIN VERIFY SCRIPT-
sed -i 's/max_global_ann/max_global_latency_score/g' src/node/txorphanage.cpp
sed -i 's/max_global_ann/max_global_latency_score/g' src/node/txorphanage.h
sed -i 's/max_global_ann/max_global_latency_score/g' src/test/orphanage_tests.cpp
sed -i 's/max_global_ann/max_global_latency_score/g' src/test/fuzz/txorphan.cpp
sed -i 's/max_global_ann/max_global_latency_score/g' src/bench/txorphanage.cpp
sed -i 's/max_ann/max_lat/g' src/node/txorphanage.cpp
-END VERIFY SCRIPT-
This introduces an invariant that TxOrphanageImpl never holds more than one
announcement with m_reconsider=true for a given wtxid. This avoids duplicate
work, both in the caller might otherwise reconsider the same transaction multiple
times before it is ready, and internally in AddChildrenToWorkSet, which might
otherwise iterate over all announcements multiple times.
ea17a9423fb431a86d36927b02d3624f654fd867 [doc] release note for relaxing requirement of all unconfirmed parents present (glozow)
12f48d5ed302e92a334dbe971c66df467d402655 test: add chained 1p1c propagation test (Greg Sanders)
525be56741cff5f89d596b8a0c44df00f2209bcb [unit test] package submission 2p1c with 1 parent missing (glozow)
f24771af0581e8117bd638227469e37cc69c5103 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 ea17a9423fb431a86d36927b02d3624f654fd867
instagibbs:
ACK ea17a9423fb431a86d36927b02d3624f654fd867
Tree-SHA512: c2231761ae7b2acea10a96735e7a36c646f517964d0acb59bacbae1c5a1950e0223458b84c6d5ce008f0c1d53c1605df0fb3cd0064ee535ead006eb7c0fa625b
fac90e5261b811739ada56e06ea793a12f9c2c3d test: Check that the GUI interactive reindex works (MarcoFalke)
faaaddaaf8e5a63f19c4fc66aa79134987775f96 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 fac90e5261b811739ada56e06ea793a12f9c2c3d
TheCharlatan:
ACK fac90e5261b811739ada56e06ea793a12f9c2c3d
mzumsande:
Tested ACK fac90e5261b811739ada56e06ea793a12f9c2c3d
Tree-SHA512: 9f744d36e7cdd3f5871764386ec5a5cca1ae144f1bacc26c07e60313c2bdacdc5fca351aa185cb51359540eea4534dda17e4fb6073ad90f91ba0a6936faeead8
3a03f075606b19e411b8bd19870242e0e0b58fcb qt: Avoid header circular dependency (Anthony Towns)
25884bd89684262b94e09c9904c1560d5ba87d86 qt, refactor: Move `FreespaceChecker` class into its own module (Hennadii Stepanov)
Pull request description:
For some reason, the MOC compiler in older versions of Qt 6 fails to parse `qt/intro.cpp`, as noted in [this comment](https://github.com/bitcoin/bitcoin/pull/32998#issuecomment-3082011233).
This PR proposes a move-only refactoring to simplify the source structure by eliminating the need for the inline `#include <qt/intro.moc>`, thereby effectively working around the issue.
Required for https://github.com/bitcoin/bitcoin/pull/32998.
ACKs for top commit:
ajtowns:
ACK 3a03f075606b19e411b8bd19870242e0e0b58fcb
Tree-SHA512: 4a7261f04fff9bd8edd4dc2df619c90e06417e19da672dd688a917cd0b9a324a6db7185a47c48f0385713b5e6c45d2204bef58cbe6c77299386136ed5682bd8d
c6e2c31c55123cc97b4400bcbf3c37a39b067a22 rpc: unhide waitfor{block,newblock,blockheight} (Sjors Provoost)
0786b7509acd7e160345eea5fc25acd3c795d01c 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 c6e2c31c55123cc97b4400bcbf3c37a39b067a22
ryanofsky:
Code review ACK c6e2c31c55123cc97b4400bcbf3c37a39b067a22. Just rebased and tweaked documentation since last review.
glozow:
utACK c6e2c31c55123cc97b4400bcbf3c37a39b067a22
Tree-SHA512: 84a0c94cb9a2e4449e7a395cf3dce1650626bd852e30e0e238a1aafae19d57bf440bfac226fd4da44eaa8d1b2fa4a8c1177b6c716235ab862a72ff5bf8fc67ac
c2ed576d2caf282a51aa9df5df55d844ab1da794 fuzz: cover BanMan::IsDiscouraged (brunoerg)
Pull request description:
This PR adds fuzz coverage for the `IsDiscouraged` function in the banman target. This is the only function missing from `BanMan`.
ACKs for top commit:
maflcko:
lgtm ACK c2ed576d2caf282a51aa9df5df55d844ab1da794
marcofleon:
ACK c2ed576d2caf282a51aa9df5df55d844ab1da794
Tree-SHA512: 1dc5fc138f89413c46ed41195940f4c578ef996ce84595271b7433cae8a8f576205b649b493a7ec4804c712327d6c77b1004ba116b0144916377042adaaf6c5f
76fe0e59ec4a5b0c5b18f46bfcbbf99628d74d77 test: Migration of a wallet ending in `../` (David Gumberg)
f0bb3d50fef08d9f981eac45841ef2df7444031b test: Migration of a wallet ending in `/` (David Gumberg)
41faef5f80d69ed984af685bd9d2a43268009fb6 test: Migration fail recovery w/ `../` in path (David Gumberg)
63c6d364376907c10b9baa3c6f4d72e3f1881abc test: Migration of a wallet with `../` in path. (David Gumberg)
70f1c99c901de64d6ccea793b7e267e20dfa49cf wallet: Fix migration of wallets with pathnames. (David Gumberg)
f6ee59b6e2995a3916fb4f0d4cbe15ece2054494 wallet: migration: Make backup in walletdir (David Gumberg)
e22c3599c6772730e72e17fc68c99feea09b4d29 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 76fe0e59ec4a5b0c5b18f46bfcbbf99628d74d77
achow101:
ACK 76fe0e59ec4a5b0c5b18f46bfcbbf99628d74d77
ryanofsky:
Code review ACK 76fe0e59ec4a5b0c5b18f46bfcbbf99628d74d77. 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
aac0b6dd79b0db1e9d42a6f466709a61cfd1f69f test: test sendall and send do anti-fee-sniping (ishaanam)
20802c7b65f4b196a3ed0aca876974153db55d9d 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 aac0b6dd79b0db1e9d42a6f466709a61cfd1f69f
murchandamus:
ACK aac0b6dd79b0db1e9d42a6f466709a61cfd1f69f
glozow:
ACK aac0b6dd79b0db1e9d42a6f466709a61cfd1f69f
Tree-SHA512: d4f1b43b5bda489bdba46b0af60e50bff0de604a35670e6ea6e1de2b539f16b3f68805492f51d6d2078d421b63432ca22a561a5721d1a37686f2e48284e1e646
62ed1f92efff42bc79c50935e6dbd9da4e072020 txgraph: check that DoWork finds optimal if given high budget (tests) (Pieter Wuille)
f3c2fc867fc4332dfed0a3766997433e1676dbe3 txgraph: add work limit to DoWork(), try optimal (feature) (Pieter Wuille)
e96b00d99ebe27eeadba88841db32b2b8e741433 txgraph: make number of acceptable iterations configurable (feature) (Pieter Wuille)
cfe9958852be0e0763c924bdbadc37e784f5aee5 txgraph: track amount of work done in linearization (preparation) (Pieter Wuille)
6ba316eaa0321faf94eb31769deb18781ff9667c txgraph: 1-or-2-tx split-off clusters are optimal (optimization) (Pieter Wuille)
fad0eb091e58f345b922a49335c60bbeae6d5c6f txgraph: reset quality when merging clusters (bugfix) (Pieter Wuille)
Pull request description:
Part of #30289. Builds on top of #31553.
So far, the `TxGraph::DoWork()` function took no parameters, and just made all clusters reach the "acceptable" internal quality level by performing a minimum number of improvement iterations on it, but:
* Did not attempt to go beyond that.
* Was broken, as the QualityLevel of optimal clusters that merge together was not being reset.
Fix this by adding an argument to `DoWork()` to control how much work it is allowed to do right now, which will first be used to get all clusters to the acceptable level, and if more budget remains, use it to try to get some or all clusters optimal. The function will now return `true` if all clusters are known to be optimal (and thus no further work remains). This is verified in the tests, by remembering whether the graph is optimal, and if it is at the end of the simulation run, verify that the overall linearization cannot be improved further.
ACKs for top commit:
instagibbs:
ACK 62ed1f92efff42bc79c50935e6dbd9da4e072020
ismaelsadeeq:
Code review ACK 62ed1f92efff42bc79c50935e6dbd9da4e072020
glozow:
ACK 62ed1f92efff42bc79c50935e6dbd9da4e072020
Tree-SHA512: 5f57d4052e369f3444e72e724f04c02004e0f66e365faa59c9f145323e606508380fc97bb038b68783a62ae9c10757f1b628b3b00b2ce9a46161fca2d4336d73
b6d4688f77df9e31fd64e2be300f55bb8e944bd0 [doc] reword comments in test_mid_package_replacement (glozow)
f3a613aa5bb780f5d85821fa00742f5a99df81b5 [cleanup] delete brittle test_mid_package_eviction (glozow)
c3cd7fcb2cd9f9911f8251a6f29120f65c63ea37 [doc] remove references to now-nonexistent Finalize() function (glozow)
d8140f5f050076bfa129169c1223cab94b243a6e don't make a copy of m_non_base_coins (glozow)
98ba2b1db2eb81e08b550ba0d5069a9289f30e13 [doc] MemPoolAccept coins views (glozow)
ba02c30b8a635db88100916a1bb9c40fb0b47ab6 [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 b6d4688f77df9e31fd64e2be300f55bb8e944bd0
marcofleon:
ACK b6d4688f77df9e31fd64e2be300f55bb8e944bd0
Tree-SHA512: 79c68e263013b1153520f5453e6b579b8fe7e1d6a9952b1ac2c3c3c017034e6d21d7000a140bba4cc9d2ce50ea3a84cc6f91fd5febc52d7b3fa4f797955d987d
Simplifies `m_tx_inventory_to_send` a bit by making it a set of Wtxids.
Wtxid relay is prevalent throughout the network, so the complexity of
dealing with a GenTxid in this data structure isn't necessary.
For peers that aren't wtxid relay, the txid is now retrieved from our
mempool entry when the inv is constructed.
1c10b7351e194fc788766347f65f4512f61f05e8 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 1c10b7351e194fc788766347f65f4512f61f05e8
maflcko:
lgtm ACK 1c10b7351e194fc788766347f65f4512f61f05e8
theStack:
ACK 1c10b7351e194fc788766347f65f4512f61f05e8
Tree-SHA512: 1750d7d12de511f0ac34922ea9c58c4b9b55c3aaf22109abfd7dbe01ad1eb7b48fb4a6b074a0baf0e55ee2270fcc969b6830e499ff33adbcd0b9c761fb25e563
251d02084688c67523e9ec92ec79ee657454ab93 init, wallet: replace hardcoded output types with `FormatAllOutputTypes` (Sebastian Falbesoner)
e3ba0757a9410336e904a1b108d86165f347fc52 rpc, wallet: replace remaining hardcoded output types with `FormatAllOutputTypes` (Sebastian Falbesoner)
Pull request description:
This PR takes use of the `FormatAllOutputTypes` helper (introduced in PR #32432, commit 8cc9845b8ddf4f93a02c622e7df8d1095dc1a640) to get rid of the remaining hardcoded output types in wallet RPC and command line arguments documentation [1]. Note that it can't be used in the [`createmultisig` RPC](fc162299f0/src/rpc/output_script.cpp (L100)), as this one is only for pre-taproot output types and hence doesn't contain "bech32m" in the list.
[1] instances were found via `$ git grep legacy.*p2sh-segwit ./src/rpc/ ./src/wallet/`
ACKs for top commit:
nervana21:
tACK [251d020](251d020846)
maflcko:
review ACK 251d02084688c67523e9ec92ec79ee657454ab93 🌨
pablomartin4btc:
re-utACK 251d02084688c67523e9ec92ec79ee657454ab93
rkrux:
crACK 251d02084688c67523e9ec92ec79ee657454ab93
Tree-SHA512: 23d1025d068f3a44b115a34b217b808fcae59fc574e35a899f0d43a85512935c90675d2e98c621287e02adc3a9f4a08289a26596c66e2122262af0cd2dfbde72
Previously, the assertion only showed that a result was found, however
made no assertion about the quality of the result.
Remove comment about what UTXOs are selected and what are not
since the test does not reflect that.
Co-authored-by: Mark "Murch" Erhardt <murch@murch.one>
Co-authored-by: Andrew Toth <andrewstoth@gmail.com>
Co-authored-by: Sebastian Falbesoner <sebastian.falbesoner@gmail.com>
Co-authored-by: Anthony Towns <aj@erisian.com.au>
faa1c3e80d95552bdc2c0e717065ebf8d510138f Revert "Merge bitcoin/bitcoin#32343: common: Close non-std fds before exec in RunCommandJSON" (MarcoFalke)
Pull request description:
After a fork() in a multithreaded program, the child can safely
call only async-signal-safe functions (see [signal-safety(7)](https://www.man7.org/linux/man-pages/man7/signal-safety.7.html))
until such time as it calls execv.
The standard library (`std` namespace) is not async-signal-safe. Also, `throw`, isn't.
There was an alternative implementation using `readdir` (https://github.com/bitcoin/bitcoin/pull/32529), but that isn't async-signal-safe either, and that implementation was still using `throw`.
So temporarily revert this feature.
A follow-up in the future can add it back, using only async-signal-safe functions, or by using a different approach.
Fixes https://github.com/bitcoin/bitcoin/issues/32524
Fixes https://github.com/bitcoin/bitcoin/issues/33015
Fixes https://github.com/bitcoin/bitcoin/issues/32855
For reference, a failure can manifest in the GCC debug mode:
* While `fork`ing, a debug mode mutex is held (by any other thread).
* The `fork`ed child tries to use the stdard libary before `execv` and deadlocks.
This may look like the following:
```
(gdb) thread apply all bt
Thread 1 (Thread 0xf58f4b40 (LWP 774911) "b-httpworker.2"):
#0 0xf7f4f589 in __kernel_vsyscall ()
#1 0xf79e467e in ?? () from /lib32/libc.so.6
#2 0xf79eb582 in pthread_mutex_lock () from /lib32/libc.so.6
#3 0xf7d93bf2 in ?? () from /lib32/libstdc++.so.6
#4 0xf7d93f36 in __gnu_debug::_Safe_iterator_base::_M_attach(__gnu_debug::_Safe_sequence_base*, bool) () from /lib32/libstdc++.so.6
#5 0x5668810a in __gnu_debug::_Safe_iterator_base::_Safe_iterator_base (this=0xf58f13ac, __seq=0xf58f13f8, __constant=false) at /bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/debug/safe_base.h:91
#6 0x56ddfb50 in __gnu_debug::_Safe_iterator<__gnu_cxx::__normal_iterator<int*, std::__cxx1998::vector<int, std::allocator<int> > >, std::__debug::vector<int, std::allocator<int> >, std::forward_iterator_tag>::_Safe_iterator (this=0xf58f13a8, __i=3, __seq=0xf58f13f8) at /bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/debug/safe_iterator.h:162
#7 0x56ddfacb in __gnu_debug::_Safe_iterator<__gnu_cxx::__normal_iterator<int*, std::__cxx1998::vector<int, std::allocator<int> > >, std::__debug::vector<int, std::allocator<int> >, std::bidirectional_iterator_tag>::_Safe_iterator (this=0xf58f13a8, __i=3, __seq=0xf58f13f8) at /bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/debug/safe_iterator.h:539
#8 0x56ddfa5b in __gnu_debug::_Safe_iterator<__gnu_cxx::__normal_iterator<int*, std::__cxx1998::vector<int, std::allocator<int> > >, std::__debug::vector<int, std::allocator<int> >, std::random_access_iterator_tag>::_Safe_iterator (this=0xf58f13a8, __i=3, __seq=0xf58f13f8) at /bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/debug/safe_iterator.h:687
#9 0x56ddd3f6 in std::__debug::vector<int, std::allocator<int> >::begin (this=0xf58f13f8) at /bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/debug/vector:300
#10 0x57d83701 in subprocess::detail::Child::execute_child (this=0xf58f156c) at ./util/subprocess.h:1372
#11 0x57d80a7c in subprocess::Popen::execute_process (this=0xf58f1cd8) at ./util/subprocess.h:1231
#12 0x57d6d2b4 in subprocess::Popen::Popen<subprocess::input, subprocess::output, subprocess::error, subprocess::close_fds> (this=0xf58f1cd8, cmd_args="fake.py enumerate", args=..., args=..., args=..., args=...) at ./util/subprocess.h:964
#13 0x57d6b597 in RunCommandParseJSON (str_command="fake.py enumerate", str_std_in="") at ./common/run_command.cpp:27
#14 0x57a90547 in ExternalSigner::Enumerate (command="fake.py", signers=std::__debug::vector of length 0, capacity 0, chain="regtest") at ./external_signer.cpp:28
#15 0x56defdab in enumeratesigners()::$_0::operator()(RPCHelpMan const&, JSONRPCRequest const&) const (this=0xf58f2ba0, self=..., request=...) at ./rpc/external_signer.cpp:51
...
(truncated, only one thread exists)
```
ACKs for top commit:
fanquake:
ACK faa1c3e80d95552bdc2c0e717065ebf8d510138f
darosior:
ACK faa1c3e80d95552bdc2c0e717065ebf8d510138f
Tree-SHA512: 602da5f2eba08d7fe01ba19baf411e287ae27fe2d4b82f41734e05b7b1d938ce94cc0041e86ba677284fa92838e96ebee687023ff28047e2b036fd9a53567e0a
The current `prevector` size of 28 bytes (chosen to fill the `sizeof(CScript)` aligned size) was introduced in 2015 (https://github.com/bitcoin/bitcoin/pull/6914) before SegWit and TapRoot.
However, the increasingly common `P2WSH` and `P2TR` scripts are both 34 bytes, and are forced to use heap (re)allocation rather than efficient inline storage.
The core trade-off of this change is to eliminate heap allocations for common 34-36 byte scripts at the cost of increasing the base memory footprint of all `CScript` objects by 8 bytes (while still respecting peak memory usage defined by `-dbcache`).
Increasing the `prevector` size allows these scripts to be stored inline, avoiding extra heap allocations, reducing potential memory fragmentation, and improving performance during cache flushes. Massif analysis confirms a lower stable memory usage after flushing, suggesting the elimination of heap allocations outweighs the larger base size for common workloads.
Due to memory alignment, increasing the `prevector` size to 36 bytes doesn't change the overall `sizeof(CScript)` compared to an increase to 34 bytes, allowing us to include `P2PK` scripts as well at no additional memory cost.
Performance benchmarks for AssumeUTXO load and flush show:
* Small dbcache (450MB): ~1-3% performance improvement (despite more frequent flushes)
* Large dbcache (4500MB): ~6-8% performance improvement due to fewer heap allocations (and basically the number of flushes)
* Very large dbcache (4500MB): ~5-6% performance improvement due to fewer heap allocations (and memory limit not being reached, so there's no memory penalty)
Full IBD and reindex-chainstate with larger `dbcache` values also show an overall ~3-4% speedup.
Co-authored-by: Ava Chow <github@achow101.com>
Co-authored-by: Andrew Toth <andrewstoth@gmail.com>
Co-authored-by: maflcko <6399679+maflcko@users.noreply.github.com>
Verifies that script types are correctly allocated using prevector's direct or indirect storage based on their size:
Direct allocated script types (size ≤ 28 bytes):
* OP_RETURN (small)
* P2WPKH
* P2SH
* P2PKH
Indirect allocated script types (size > 28 bytes):
* P2WSH
* P2TR
* P2PK
* MULTISIG (small)
This test provides a baseline for verifying changes to prevector's inline capacity.
The `CHECK_SCRIPT_STATIC_SIZE` and `CHECK_SCRIPT_DYNAMIC_SIZE` macros were added to differentiate the two cases - while preserving the correct source code line in case of failure.
