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.
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
This change offers a few advantages, such as:
- a more readable and cleaner `ts_files.cmake` (see the next commit);
- a scoped `ts_files` variable;
- improved code locality;
- no need to adjust the location of the resulting `*.qm` files.
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
PermissionsStartOnly is deprecated [1]. This removes the directives
and instead we prefixes the value of the ExecStartPre directive with
'!', which means the executable, 'chgrp' in this case, is run with
full privileges and able to change the group of /etc/bitcoin.
1: 60b45a80c1/NEWS (L2434)
Setting Group=bitcoin is redundant. It is typically the default group
of the user and if not explicitly specified, systemd will run the
service with the default group of the user.
The `SECP256K1_DISABLE_SHARED` CMake variable has been removed upstream.
This change removes its usage ahead of the next `secp256k1` subtree
update to prevent breakage and simplify integration.
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.
7aa5b67132 ci: remove DEBUG_LOCKORDER from TSAN job (fanquake)
b09af2ce50 ci: instrument libc++ in TSAN job (fanquake)
6653cafd0b ci: allow libc++ instrumentation other than msan (fanquake)
Pull request description:
Allow for instrumenting libc++ with a sanitizer other than MemoryWithOrigins.
Would also close#33087, as with the extra instrumentation, the issue from https://github.com/bitcoin/bitcoin/pull/32862#issuecomment-3114706601 is avoided (also see https://github.com/bitcoin/bitcoin/pull/33081), and we can drop `DEBUG_LOCKORDER`.
ACKs for top commit:
maflcko:
re-ACK 7aa5b67132🦀
dergoegge:
utACK 7aa5b67132
Tree-SHA512: 95f123e37da5e81d571244e4b1cd7658107676f1ea763ff16e5b69f4dfadb88236a577bb2ee52230ff542872c1da151c88fc50aba0f32540e765080120cec55e
9954d6c833 depends: hard-code necessary c(xx)flags rather than setting them per-host (Cory Fields)
Pull request description:
The per-host flag variables hold platform-specific defaults that are ignored when flag environment variables are set, so it was wrong for them to contain -std options relied on by package definitions.
Additionally, these flags (-pipe and -std=xx) will no longer be passed into the CMake build, meaning less duplication in the build summary.
Pulled out of #31920.
ACKs for top commit:
achow101:
ACK 9954d6c833
ryanofsky:
Code review ACK 9954d6c833. No changes since last review other than improving the commit message. Change overall makes sense because it deduplicates host definitions, stops dropping `-std` flags from package builds when custom CFLAGS/CXXFLAGS environment variables are set, and stops passing duplicate flags to cmake that have no effect.
theuni:
ACK 9954d6c833
Tree-SHA512: 62a2a21b741893cf708ca71fb5f0626c30d52685c845f9016f428a5e38fc8515acd4bf2c83635d6505b63830d1c296472026ec3d341c8069f1e490a991b6b5ef
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
3a03f07560 qt: Avoid header circular dependency (Anthony Towns)
25884bd896 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 3a03f07560
Tree-SHA512: 4a7261f04fff9bd8edd4dc2df619c90e06417e19da672dd688a917cd0b9a324a6db7185a47c48f0385713b5e6c45d2204bef58cbe6c77299386136ed5682bd8d
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
cab6736b70 ci: remove ninja-build from MSAN jobs (fanquake)
Pull request description:
It is part of `CI_BASE_PACKAGES`.
ACKs for top commit:
maflcko:
review ACK cab6736b70 🕸
hebasto:
ACK cab6736b70, I have reviewed the code and it looks OK.
Tree-SHA512: 8da5f0b07310a1e003d405ade19408b390781121a317ecc0ebdf48cb63bb3abf39bcfb635e4a43200568e0debabb463c2a3a843705e625fa455609eb3f0ba416
c2ed576d2c 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 c2ed576d2c
marcofleon:
ACK c2ed576d2c
Tree-SHA512: 1dc5fc138f89413c46ed41195940f4c578ef996ce84595271b7433cae8a8f576205b649b493a7ec4804c712327d6c77b1004ba116b0144916377042adaaf6c5f
5888b4a2a5 doc: add note for watch-only wallet migration (rkrux)
Pull request description:
This was suggested in a previous PR #31423.
ACKs for top commit:
achow101:
ACK 5888b4a2a5
brunoerg:
reACK 5888b4a2a5
jonatack:
ACK 5888b4a2a5
Tree-SHA512: 96e51eda30a1f31cfd82ae3296ca97c9236599b18e19086dbde3a908f6fe66af8f2de7aa147bdb9ebccb2059c809a25ddfb0c23da57e1a84a35b62ca0a44e3c3
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
fa45ccc15d doc: Add legacy wallet removal release notes (MarcoFalke)
Pull request description:
This spans over several pulls, so add a single note for all of them.
ACKs for top commit:
glozow:
lgtm ACK fa45ccc15d
achow101:
ACK fa45ccc15d
pablomartin4btc:
ACK fa45ccc15d
janb84:
re ACK fa45ccc15d
Tree-SHA512: e753cc3afbd66a88099ff62c2591aa31d32d784098e433e392c20a8dfd40d5c85807e955b264a287c3778d68605cd7022278886a43cd1635c080d563c88fc0cc
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
1bed0f734b guix: warn SOURCE_DATE_EPOCH set in guix-codesign (will)
Pull request description:
#32678 added a sanity check for this environment variable when running `guix-build` but missed that `guix-codesign` also relies on `SOURCE_DATE_EPOCH`, which can result in non-determinism in the codesigning step: https://github.com/bitcoin-core/guix.sigs/pull/1720#issuecomment-3124332676
To avoid repeating the logic move common functionality into the prelude and call the function in both guix actions.
ACKs for top commit:
fanquake:
ACK 1bed0f734b
Tree-SHA512: ad3de8ab06e7f4ffcee5c02e8185b20879d63a02a614a706ea54da5087cca4ba75817ca1aa95301572c34723317fcc44e4478261ac73dd223ee9fa827e6b35b3
3b23f95e34 ci: limit max stack size to 512 KiB (dergoegge)
2931a87477 ci: limit stack size to 512kb in native macOS jobs (fanquake)
Pull request description:
Picks up #31367.
Closes#29840.
Limit adjustment is moved until after compilation, otherwise compilation might not succeed.
I've used compilation flags to limit the stack size in the native macOS jobs, because trying to use `ulimit` resulted in:
```bash
+ '[' -n 1 ']'
+ ulimit -s 262144
/Users/runner/work/bitcoin/bitcoin/ci/test/03_test_script.sh: line 17: ulimit: stack size: cannot modify limit: Operation not permitted
```
See example failures (`ulimit -s 64`) here: https://github.com/bitcoin/bitcoin/runs/46861548042.
ACKs for top commit:
dergoegge:
utACK 3b23f95e34
Tree-SHA512: 7e00626f3ca9e860d79a301af2427008ce27c329b618e24f95e7a55b284459a446216d2859c2e63be50abb9d4f0d343c12ff50445231652d354f225477928a35
6757052fc4 doc: move `cmake -B build -LH` up in Unix build docs (Bufo)
Pull request description:
#32269 rebased.
> I had trouble building bitcoin core the way I wanted since now more features require a flag while building. IMO it makes sense to make it a bit more prominent in the build docs how to get the needed flags.
> Related issue: https://github.com/bitcoin/bitcoin/issues/32258
ACKs for top commit:
maflcko:
lgtm ACK 6757052fc4
stickies-v:
ACK 6757052fc4
janb84:
ACK 6757052fc4
Tree-SHA512: 8e6dc1e432c067f862560776176112d5c24c4009bdf8e9a4e8d1ea3328b88732188fc4d8a7cd29f8a9ed8a1809a7a8a86d63b7ae3ec4ebae74be466727c8d730
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:
```
62ed1f92ef txgraph: check that DoWork finds optimal if given high budget (tests) (Pieter Wuille)
f3c2fc867f txgraph: add work limit to DoWork(), try optimal (feature) (Pieter Wuille)
e96b00d99e txgraph: make number of acceptable iterations configurable (feature) (Pieter Wuille)
cfe9958852 txgraph: track amount of work done in linearization (preparation) (Pieter Wuille)
6ba316eaa0 txgraph: 1-or-2-tx split-off clusters are optimal (optimization) (Pieter Wuille)
fad0eb091e 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 62ed1f92ef
ismaelsadeeq:
Code review ACK 62ed1f92ef
glozow:
ACK 62ed1f92ef
Tree-SHA512: 5f57d4052e369f3444e72e724f04c02004e0f66e365faa59c9f145323e606508380fc97bb038b68783a62ae9c10757f1b628b3b00b2ce9a46161fca2d4336d73
The per-host flag variables hold platform-specific defaults that are ignored
when flag environment variables are set, so it was wrong for them to contain
-std options relied on by package definitions.
Additionally, these flags (-pipe and -std=xx) will no longer be passed into
the CMake build, meaning less duplication in the build summary.
Pulled out of #31920.
0ce041ea88 tracing: fix pointer argument handling in mempool_monitor.py (deadmanoz)
Pull request description:
The BPF code was incorrectly passing pointer variables by value to `bpf_usdt_readarg()`, causing the function to fail silently and resulting in transaction hashes showing as zeros and reason strings displaying empty strings.
This fix adds the missing reference operator (&) when passing pointer variables to `bpf_usdt_readarg()`, allowing the function to properly write the pointer values and enabling correct display of transaction hashes and removal/rejection reasons.
Fixes the regression introduced in [ec47ba349d](ec47ba349d) where `bpf_usdt_readarg_p` was replaced with `bpf_usdt_readarg` but the calling convention wasn't properly updated for pointer arguments.
**Before: "0000000000000000000000000000000000000000000000000000000000000000" tx hashes, and missing reasons (empty strings) for removal.**
<img width="1683" height="1330" alt="Screenshot 2025-07-29 at 4 30 03 PM" src="https://github.com/user-attachments/assets/71ba88be-dbcc-43a6-bfe7-bd49ae082b13" />
**After: tx hashes show, reasons for removal showing.**
<img width="1682" height="1330" alt="Screenshot 2025-07-29 at 4 29 23 PM" src="https://github.com/user-attachments/assets/03738c75-5526-4c1e-82c2-ba100cdf128a" />
ACKs for top commit:
0xB10C:
tested ACK 0ce041ea88
Tree-SHA512: cb50748fa2cd38be4b0abed36723917c2c82a92f588928bb0650eed0049c121df89b33d53421037b12836a497f30b449fe3d041ff7755a1fd9da43544392cf40
