d7fca5c171f450621112457ea6f7c99b2e21d354 clusterlin: add big comment explaning the relation between tests (Pieter Wuille)
b64e61d2de65026c46f70cb91e3cb2213c336be0 clusterlin: abstract try-permutations into ExhaustiveLinearize function (Pieter Wuille)
1fa55a64ed18a0a3defbed33f6bd355929f67b48 clusterlin tests: verify that chunks are minimal (Pieter Wuille)
da23ecef29b7e6a1e03de9697a6bdf6fbe944ef7 clusterlin tests: support non-empty ReadTopologicalSubset() (Pieter Wuille)
94f3e17c33e6fe63c4f791de7aacfb912059e394 clusterlin tests: compare with fuzz-provided linearizations (Pieter Wuille)
5f92ebee0d2401d0d7b5c3466d66a4b09e51ccb0 clusterlin tests: compare with fuzz-provided topological sets (Pieter Wuille)
6e37824ac390835d3d9c82d197f58e992ccc584f clusterlin tests: optimize clusterlin_simple_linearize (Pieter Wuille)
98c1c88b6f8d928b3893e2910ce0c400a6fd1928 clusterlin tests: separate testing of SimpleLinearize and Linearize (Pieter Wuille)
10e90f7aef9cff6fa63dc15adbe22e68f76aac58 clusterlin tests: make SimpleCandidateFinder always find connected (Pieter Wuille)
a38c38951e10a61af80e3bca1e1ae04de978d5c0 clusterlin tests: separate testing of Search- and SimpleCandidateFinder (Pieter Wuille)
77a432ee704b4a83d56135ed10cf2adf4b3c18af clusterlin tests: count SimpleCandidateFinder iterations better (Pieter Wuille)
Pull request description:
Part of the cluster mempool project: #30289
The current cluster linearization fuzz tests contain two tests which combine testing of production code with testing of the test code itself:
* `clusterlin_search_finder`: establishes the correctness of `SearchCandidateFinder` by comparing against both `SimpleCandidateFinder` and `ExhaustiveCandidateFinder` (which is even more simple than `SimpleCandidateFinder`). If `SimpleCandidateFinder` works correctly, then this comparison with `ExhaustiveCandidateFinder` is redundant. If it isn't, we ought to find that in a test specific to `SimpleCandidateFinder` rather than as a side-effect of testing `SearchCandidateFinder`. Split this functionality out into a new `clusterlin_simple_finder`.
* `clusterlin_linearize`: establishes the correctness of `Linearize` by comparing against both `SimpleLinearize` and literally every valid linearization for the cluster. Again, if `SimpleLinearize` works correctly, then this comparison with all valid linearizations is redundant, and if it isn't we should find it in a test for `SimpleLinearize`. Do so by splitting off that functionality into `clusterlin_simple_linearize`.
After that, a few general improvements to the affected tests are made (comparing with linearizations and subsets read from the fuzz input, plus a performance improvement).
ACKs for top commit:
marcofleon:
Re ACK d7fca5c171f450621112457ea6f7c99b2e21d354
ismaelsadeeq:
re-ACK d7fca5c171f450621112457ea6f7c99b2e21d354
monlovesmango:
ACK d7fca5c171f450621112457ea6f7c99b2e21d354
Tree-SHA512: 33cb76bd9b9547a5f3ee231fa452e928f064ad03af98e3d9e64246eb972f2b026c13e7367257ccdac1ae57982ee8ef98c907684588ecbb4bc4c82cbec160b3e8
fa2fbaa4a29f80d3c7d5f0ad6b64035c3156dd12 bench: Avoid tmp files in pwd (MarcoFalke)
Pull request description:
It is a bit confusing that one bench run, when aborted, could leave behind temp files in the current working directory. It is similarly confusing to delete those files in the next run of bench.
Fix all issues by using `BasicTestingSetup`, which provides a proper temp folder to use and also cleans up after itself.
Can be tested via:
```
( echo 'my file content' > streams_tmp ) && ls streams_tmp && ./bld-cmake/bin/bench_bitcoin --filter=FindByte && ls streams_tmp
```
Previously the file would be deleted, now it is kept.
ACKs for top commit:
stickies-v:
ACK fa2fbaa4a29f80d3c7d5f0ad6b64035c3156dd12
Tree-SHA512: 33798030f990d1b4c95be4682d8dbfad95e8716d5fc0b99d65937196f2ced1ba649193c2adba4155f4eec9fd06e16be6667f3c3705af1880f47b2ff57a76243b
fa0528479d5e37833fa66395c94d4611aa9270f6 test: Add missing convert_to_json_for_cli (MarcoFalke)
Pull request description:
Currently the tests are failing on current master, if they use the `--usecli` flag. See https://github.com/bitcoin/bitcoin/runs/45676472375, https://cirrus-ci.com/task/5707897310543872.
This can be reproduced locally via:
```
./bld-cmake/test/functional/wallet_reorgsrestore.py --usecli
```
Fix it by adding the missing `hash_or_height=self.convert_to_json_for_cli(tip)` for the value that could either be a string (needs quotes in json), or a number (does not need quotes in json).
ACKs for top commit:
fanquake:
ACK fa0528479d5e37833fa66395c94d4611aa9270f6
Tree-SHA512: 3d6deafca1249b2266cfabcd883edc9daaf985c417035a4b0223da4693f4165f8c9ce91a0e128d626000c10c32fe31f323f4b3f6ea0d0b3a771237a4f1d4cf44
fa4d68cf97b6d77dbb559facbc425376e2c51f62 Turn rpcauth.py test into functional test (MarcoFalke)
Pull request description:
Currently the `rpcauth-test.py` is problematic, because:
* The boilerplate for the test runner is duplicate or inconsistent with the other (functional) tests. Specifically `ConfigParser`.
* The cmake/ci behavior is brittle and can silently fail, as explained in https://github.com/bitcoin/bitcoin/issues/31476.
* Outside of ctest, this single test has to be run manually and separately, which is easy to forget.
* If the test is manually called, it runs single threaded, when it could just run in parallel with the other functional tests.
* It is also the only "unit" test written in Python, but not called by the functional test runner.
Fix all issues by turning it into a functional test.
ACKs for top commit:
l0rinc:
ACK fa4d68cf97b6d77dbb559facbc425376e2c51f62
janb84:
LGTM ACK fa4d68cf97b6d77dbb559facbc425376e2c51f62
w0xlt:
ACK fa4d68cf97
Tree-SHA512: a3b2b03be31c33288dee23c544b33ec43295e796c2047777597ceb86acce9f697478e32d891aa986c1d7d5749d62eded65eeb858e9d7479bda7a400eb1167040
8cc3ac6c2328873e57c31f237d194c5f22b6748a validation: Don't use IsValid() to filter for invalid blocks (Martin Zumsande)
86d98b94e5469e6476dc87f54b8ac25074603f0c test: verify that ancestors of a reconsidered block can become the chain tip (stratospher)
3c39a55e64bedfc384f6a7b9de1410bc8b46a9bb validation: Add ancestors of reconsiderblock to setBlockIndexCandidates (Martin Zumsande)
Pull request description:
When we call `reconsiderblock` for some block, `Chainstate::ResetBlockFailureFlags` puts the descendants of that block into `setBlockIndexCandidates` (if they meet the criteria, i.e. have more work than the tip etc.), but never put any ancestors into the set even though we do clear their failure flags.
I think that this is wrong, because `setBlockIndexCandidates` should always contain all eligible indexes that have at least as much work as the current tip, which can include ancestors of the reconsidered block. This is being checked by `CheckBlockIndex()`, which could fail if it was invoked after `ActivateBestChain` connects a block and releases `cs_main`:
``` diff
diff --git a/src/validation.cpp b/src/validation.cpp
index 7b04bd9a5b..ff0c3c9f58 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -3551,6 +3551,7 @@ bool Chainstate::ActivateBestChain(BlockValidationState& state, std::shared_ptr<
}
}
// When we reach this point, we switched to a new tip (stored in pindexNewTip).
+ m_chainman.CheckBlockIndex();
if (exited_ibd) {
// If a background chainstate is in use, we may need to rebalance our
```
makes `rpc_invalidateblock.py` fail on master.
Even though we don't currently have a `CheckBlockIndex()` in that place, after `cs_main` is released other threads could invoke it, which is happening in the rare failures of #16444 where an invalid header received from another peer could trigger a `CheckBlockIndex()` call that would fail.
Fix this by adding eligible ancestors to `setBlockIndexCandidates` in `Chainstate::ResetBlockFailureFlags` (also simplifying that function a bit).
Fixes#16444
ACKs for top commit:
achow101:
ACK 8cc3ac6c2328873e57c31f237d194c5f22b6748a
TheCharlatan:
Re-ACK 8cc3ac6c2328873e57c31f237d194c5f22b6748a
stratospher:
reACK 8cc3ac6.
Tree-SHA512: 53f27591916246be4093d64b86a0494e55094abd8c586026b1247e4a36747bc3d6dbe46dc26ee4a22f47b8eb0d9699d13e577dee0e7198145f3c9b11ab2a30b7
1b5c545e82fe3cf5027f16b43e2306aeb8d4ef9b wallet, test: best block locator matches scan state follow-ups (rkrux)
Pull request description:
Few follows-ups from #30221: Use `SetLastBlockProcessedInMem` more in `AttachChain`, add not null locator check in `WriteBestBlock`. Add log and few assertions in `wallet_reorgstore` test.
ACKs for top commit:
achow101:
ACK 1b5c545e82fe3cf5027f16b43e2306aeb8d4ef9b
pablomartin4btc:
cr-ACK 1b5c545e82fe3cf5027f16b43e2306aeb8d4ef9b
Tree-SHA512: 34edde55beef5714cea2e1131c29b57da2dc32ea091cd81878014de503c128f02c3ab88aee1e456541d7937e033dca5a81b03e9e2888cf781d71b62ad9b5ca5c
922adf66ac7420e21ea171d3586ea84554e5d91b mempool: use `FeeFrac` for calculating regular score (Sebastian Falbesoner)
3322b3a05954a1bac9f49a92e3bb35d2cbd20029 mempool: use `FeeFrac` for calculating ancestor score (Sebastian Falbesoner)
ac9c113bd2a2bb5001391a486d010189368f51f3 mempool: use `FeeFrac` for calculating descendant score (Sebastian Falbesoner)
Pull request description:
Rather than determining fee-rates for the mempool index scores and comparators manually in a rather tedious way (even involving floating-points), use the `FeeFrac` class [1] to simplify and deduplicate the code. Note that though this is intended to be a refactoring PR, there might be subtle differences in behaviour due to floating-point arithmetic involved in the original code (to avoid overflows at the cost of precision loss), but these shouldn't matter.
[1] introduced in PR #29242, commit ce8e22542ed0b4fa5794d3203207146418d59473
ACKs for top commit:
ismaelsadeeq:
Code review ACK 922adf66ac7420e21ea171d3586ea84554e5d91b
glozow:
ACK 922adf66ac7420e21ea171d3586ea84554e5d91b
Tree-SHA512: 6c3a9436f2be668aa8561b40c1b93efa7dc97b4ef354e98233ac3d3286a88804668164a55f2fcce4239fee5830e4e70f520e6285b667b87baa65c7cec09159cf
4c772cbd83e502a1339e8993d192ea6416ecd45c doc: add release notes for new rate limiting logging behavior (Eugene Siegel)
d541409a64c60d127ff912dad9dea949d45dbd8c log: Add rate limiting to LogPrintf, LogInfo, LogWarning, LogError, LogPrintLevel (Eugene Siegel)
a6a35cc0c23d0d529bfeb2f40d83d61f15ca7b40 log: use std::source_location in place of __func__, __FILE__, __LINE__ (Eugene Siegel)
afb9e39ec5552e598a5febaa81820d5509b7c5d2 log: introduce LogRateLimiter, LogLimitStats, Status (Eugene Siegel)
df7972a6cfd919b972bcbba07de85f7797898529 test: Mark ~DebugLogHelper as noexcept(false) (Eugene Siegel)
Pull request description:
This revives the work done by dergoegge in https://github.com/bitcoin/bitcoin/pull/21603. The approach is similar — this PR uses `std::source_location` under the hood now that we can use c++20 features. It also resets the rate limiting statistics via the `CScheduler`. The logging functions have also changed slightly since that PR was opened, so work has been done to preserve the intent of the original rate limiting change. I have tried to give commit attribution where possible.
**Approach:**
Each source code location is given an hourly logging quota of 1MiB of logging per hour. Logging is only dropped from source locations that exceed the quota.
- Only logging to disk is rate limited. Logging to console is not rate limited.
- Printing with the category argument is not rate limited.
- `UpdateTip: new best=[…]` is logged without rate limiting. High log volume is expected for that source location during IBD.
- When logging is restarted a tally of how many bytes were dropped is printed.
- All logs will be prefixed with [*] if there is at least one source location that is currently being suppressed.
I've repurposed the old logging rpc mentioned in #21603 in another branch for testing [here](https://github.com/Crypt-iQ/bitcoin/tree/log_ratelimiting_05192025_rpc). This can be used to log from source locations and test out the new changes in logging behavior. Note that the `setmocktime` RPC needs to be used to set the mock time past the current clock time to reset the logging messages.
Example usage:
```
bitcoin-cli -regtest excessivelog 1 1048500 # log just under 1MiB
bitcoin-cli -regtest excessivelog 1 100 # this should get the total amount logged above 1MiB
# and the rate limiting logic should kick in
bitcoin-cli -regtest excessivelog 2 1048500
bitcoin-cli -regtest excessivelog 2 100 # trigger rate limiting from another location
bitcoin-cli -regtest mockscheduler 3600 # fast-forward the scheduler
bitcoin-cli -regtest excessivelog 1 100 # this should trigger logging to resume and will log the source locations that were reset
```
Example output:
```
2025-07-02T22:03:56Z [warning] Excessive logging detected from rpc/node.cpp:142 (RPCHelpMan excessivelog()): >1048576 bytes logged during the last time window of 3600s. Suppressing logging to disk from this source location until time window resets. Console logging unaffected. Last log entry.
[*] 2025-07-02T22:03:56Z aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
2025-07-02T22:04:58Z (mocktime: 2262-04-11T23:47:15Z) Restarting logging from rpc/node.cpp:142 (RPCHelpMan excessivelog()): 121 bytes were dropped during the last 3600s.
2025-07-02T22:04:58Z (mocktime: 2262-04-11T23:47:15Z) Restarting logging from rpc/node.cpp:139 (RPCHelpMan excessivelog()): 121 bytes were dropped during the last 3600s.
```
ACKs for top commit:
maflcko:
re-ACK 4c772cbd83e502a1339e8993d192ea6416ecd45c 🕚
glozow:
reACK 4c772cb
stickies-v:
re-ACK 4c772cbd83, no changes except release notes update
Tree-SHA512: d07087cd0f2b188100b51c9b8c3da376fa24ec3612a2a284bd83f650bba0ea409f9fa0acd5f3b10f45e664ef4fdf3abc97ed3da08098d2beb599cc83e3fc4504
To mitigate disk-filling attacks caused by unsafe usages of LogPrintf and
friends, we rate-limit them by passing a should_ratelimit bool that
eventually makes its way to LogPrintStr which may call
LogRateLimiter::Consume. The rate limiting is accomplished by
adding a LogRateLimiter member to BCLog::Logger which tracks source
code locations for the given logging window.
Every hour, a source location can log up to 1MiB of data. Source
locations that exceed the limit will have their logs suppressed for the
rest of the window determined by m_limiter.
This change affects the public LogPrintLevel function if called with
a level >= BCLog::Level::Info.
The UpdateTipLog function has been changed to use the private LogPrintLevel_
macro with should_ratelimit set to false. This allows UpdateTipLog to log
during IBD without hitting the rate limit.
Note that on restart, a source location that was rate limited before the
restart will be able to log until it hits the rate limit again.
Co-Authored-By: Niklas Gogge <n.goeggi@gmail.com>
Co-Authored-By: stickies-v <stickies-v@protonmail.com>
The std::source_location conveniently stores the file name, line number,
and function name of a source code location. We switch to using it instead
of the __func__ identifier and the __FILE__ and __LINE__ macros.
BufferedLog is changed to have a std::source_location member, replacing the
source_file, source_line, and logging_function members. As a result,
MemUsage no longer explicitly counts source_file or logging_function as the
std::source_location memory usage is included in the MallocUsage call.
This also changes the behavior of -logsourcelocations as std::source_location
includes the entire function signature. Because of this, the functional test
feature_config_args.py must be changed to no longer include the function
signature as the function signature can differ across platforms.
Co-Authored-By: Niklas Gogge <n.goeggi@gmail.com>
Co-Authored-By: stickies-v <stickies-v@protonmail.com>
LogRateLimiter will be used to keep track of source locations and our
current time-based logging window. It contains an unordered_map and a
m_suppressions_active bool to track source locations. The map is keyed
by std::source_location, so a custom Hash function (SourceLocationHasher)
and custom KeyEqual function (SourceLocationEqual) is provided.
SourceLocationHasher uses CSipHasher(0,0) under the hood to get a
uniform distribution.
A public Reset method is provided so that a scheduler (e.g. the
"b-scheduler" thread) can periodically reset LogRateLimiter's state when
the time window has elapsed.
The LogRateLimiter::Consume method checks if we have enough available
bytes in our rate limiting budget to log an additional string. It
returns a Status enum that denotes the rate limiting status and can
be used by the caller to emit a warning, skip logging, etc.
The Status enum has three states:
- UNSUPPRESSED (logging was successful)
- NEWLY_SUPPRESSED (logging was succcesful, next log will be suppressed)
- STILL_SUPPRESSED (logging was unsuccessful)
LogLimitStats counts the available bytes left for logging per source
location for the current logging window. It does not track actual source
locations; it is used as a value in m_source_locations.
Also exposes a SuppressionsActive() method so the logger can use
that in a later commit to prefix [*] to logs whenenever suppressions
are active.
Co-Authored-By: Niklas Gogge <n.goeggi@gmail.com>
Co-Authored-By: stickies-v <stickies-v@protonmail.com>
We mark ~DebugLogHelper as noexcept(false) to be able to catch the
exception it throws. This lets us use it in test in combination with
BOOST_CHECK_THROW and BOOST_CHECK_NO_THROW to check that certain log
messages are (not) logged.
Co-Authored-By: Niklas Gogge <n.goeggi@gmail.com>
6d19815cd44031ff2b45fc9532f579cd81c62749 rest: replace `rf_names[0].rf` by `RESTResponseFormat::UNDEF` for code clarity (Eval EXEC)
Pull request description:
I'm reviewing the bitcoin's rest.cpp source code.
In the function: `ParseDataFormat`, `rf_names[0].rf` is actualy `RESTResponseFormat::UNDEF`:
e3f416dbf7/src/rest.cpp (L48-L57)
so it would be more clarity and code readability to use `return RESTResponseFormat::UNDEF;` to replace `return rf_names[0].rf;`
ACKs for top commit:
maflcko:
lgtm ACK 6d19815cd44031ff2b45fc9532f579cd81c62749
brunoerg:
code review ACK 6d19815cd44031ff2b45fc9532f579cd81c62749
Tree-SHA512: 420454f1cc09db44c1d76423d8623a0b8865d41d6c34015844ff83d78a9373e3e26f3f62818d1502b33eb063caf904750e858b74ddecd76750577ae82b64b0c1
4e69aa5701a2dad3805ea26718e6a406adb8b748 doc: fix `BlockConnected` incorrect comment (ismaelsadeeq)
Pull request description:
This is a simple PR that fixes the `BlockConnected` validation interface notification comment, which incorrectly states that a vector of transactions removed from the mempool is as a parameter of the method.
Originally, this was the case when the method was first introduced in https://github.com/bitcoin/bitcoin/pull/9725
However, the method has since changed, and this is no longer accurate. Keeping the outdated comment is now misleading.
This PR removes the information about the method parameters from the docstring, aligning it with the style of other notifications methods. As noticed in this PR, comments listing parameters can become stale and go uncorrected.
Therefore, this PR simply removes the inaccurate comment without listing the current returned values.
ACKs for top commit:
l0rinc:
ACK 4e69aa5701a2dad3805ea26718e6a406adb8b748
maflcko:
lgtm ACK 4e69aa5701a2dad3805ea26718e6a406adb8b748
Tree-SHA512: 3737313f7a9da55c67c78ce01bab5005946f4e1fccbb471560ff3af8c8275cb5cf876f6c53400c93f0ba1fdf134f28766ed573cbe62903127a3129ca8ce88db6
fcfd3db563e89fd79820a4cdfa102d624d801de1 remove RPCTimerInterface and RPCRunLater (Matthew Zipkin)
8a1765795fd3bff79d790102ca7cefa8fd9b204c use WalletContext scheduler for walletpassphrase callback (Matthew Zipkin)
Pull request description:
This removes the dependency on libevent for events scheduled by RPC commands, like re-locking a wallet some time after decryption with walletpassphrase. Since walletpassphrase is currently the only RPC that does this, `RPCRunLater`, `RPCTimerInterface` and all related methods are left unused, and deleted in the second commit. Any future RPC that needs to execute a callback in the future can follow the pattern in this PR and just use a scheduler from node or wallet context.
This is an alternative approach to #32796, described in https://github.com/bitcoin/bitcoin/pull/32796#issuecomment-3014309449
ACKs for top commit:
fjahr:
Code Review ACK fcfd3db563e89fd79820a4cdfa102d624d801de1
achow101:
ACK fcfd3db563e89fd79820a4cdfa102d624d801de1
furszy:
ACK fcfd3db563e89fd79820a4cdfa102d624d801de1
Tree-SHA512: 04f5e9c3f73f598c3d41d6e35bb59c64c7b93b03ad9fce3c40901733147ce7764f41f475fef1527d44af18f722759996a31ca83b48cb52153795d5022fecfd14
fa946520d229ae45b30519bccc9eaa2c47b4a093 refactor: Use structured binding for-loop (MarcoFalke)
eeeec1579ec5a3aa7b10ff62f87d197ae311a666 rpc: Use type-safe exception to pass RPC help (MarcoFalke)
Pull request description:
The current "catch-all" `catch (const std::exception& e)` in `CRPCTable::help` is problematic, because it could catch exceptions unrelated to passing the help string up.
Fix this by using a dedicated exception type.
ACKs for top commit:
l0rinc:
tested ACK fa946520d229ae45b30519bccc9eaa2c47b4a093 (edited)
achow101:
ACK fa946520d229ae45b30519bccc9eaa2c47b4a093
rkrux:
re-ACK fa946520d229ae45b30519bccc9eaa2c47b4a093
Tree-SHA512: 23dac6e0fe925561bfbf421e6a7441d546eed8c1492ac41ca4ed7dfcd12f4d2ef39c35f105a0291aac511365d98f08fbdc9a4f0bf627172873b8f23c2be45e76
4207d9bf823bea9f94b484ebf3c9274ca781b245 test: feature_init, ensure indexes are synced prior to perturbing files (furszy)
abd07cf7332e4e15fa4ac0e756f3610b0c7b4b6f test: feature_init, only init what's needed per perturbation/deletion round (furszy)
Pull request description:
Aims to solve #32600. Found it while working on #26966 (this was really annoying there).
This ensures the node is index-synced before perturbing files.
If the index sync gets interrupted before it starts, the database could be empty,
making any following perturbation ineffective (which explains why the node
does not abort during startup in the #32600 logs).
Also, the first commit avoids initializing components not under test.
This reduces log flooding, which helped in understanding the issue.
Patch to reproduce the issue on master using `feature_init.py` (this simulates
a node shutting down before the index starts syncing):
```
diff --git a/src/index/base.cpp b/src/index/base.cpp
--- a/src/index/base.cpp(revision 1e03052c3fefb188f047e72548f2c6b0cc019e50)
+++ b/src/index/base.cpp(date 1751293306725)
@@ -185,6 +185,7 @@
void BaseIndex::Sync()
{
const CBlockIndex* pindex = m_best_block_index.load();
+ m_interrupt();
if (!m_synced) {
std::chrono::steady_clock::time_point last_log_time{0s};
std::chrono::steady_clock::time_point last_locator_write_time{0s};
```
ACKs for top commit:
maflcko:
lgtm ACK 4207d9bf823bea9f94b484ebf3c9274ca781b245 🍄
achow101:
ACK 4207d9bf823bea9f94b484ebf3c9274ca781b245
hodlinator:
ACK 4207d9bf823bea9f94b484ebf3c9274ca781b245
Tree-SHA512: c8c89c7af9d473a12756b6a59b97f8fb473500181620eb96ecc10da954fe185d13fbb1d00a4ecb181e8daf149ec93cc547e292da0877522a4d23425fa7fd646b
afaaba69eddd50bc22b94ca7c0f9195773aaa111 test: refactor out same-txid-diff-wtxid tx to reuse in other tests (stratospher)
Pull request description:
It's useful to easily create transactions with same txid, different wtxid and valid witness for testing scenarios in other places in the codebase (ex: private broadcast connections, see https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2055915118)
So refactor out the current `same-txid-diff-wtxid` transaction in `mempool_accept_wtxid.py` so that it can be reused.
ACKs for top commit:
maflcko:
review ACK afaaba69eddd50bc22b94ca7c0f9195773aaa111 📎
theStack:
ACK afaaba69eddd50bc22b94ca7c0f9195773aaa111
vasild:
ACK afaaba69eddd50bc22b94ca7c0f9195773aaa111
Tree-SHA512: 0fc51ac326725d4abe76a15b6b5be55d070b96c303c444f4dd31c2b0a82f266836382389a123a7f6a71aa35e61fbfae27f843b31cc19474e49f3dc82f36ebf73
b1a8ac07e91dd1d305fcbc16ea931d60e46c0055 doc: Release note for removed watchonly parameters and results (Ava Chow)
15710869e19e707ef03492c55030dcefa16269d8 wallet: Remove ISMINE_WATCH_ONLY (Ava Chow)
4439bf4b41a6997d4d965f00a8c40efa9cf6895b wallet, spend: Remove fWatchOnly from CCoinControl (Ava Chow)
1337c72198a7d32935431d64e9e58c12f9003abc wallet, rpc: Remove watchonly from RPCs (Ava Chow)
e81d95d435744e48615973dc22acce1a291bd20d wallet: Remove watchonly balances (Ava Chow)
d20dc9c6aae089ab926fd135febd69a8f0744a18 wallet: Wallets without private keys cannot grind R (Ava Chow)
9991f49c38c084967ca66791d838c99b42f000eb test: Watchonly wallets should estimate larger size (Ava Chow)
Pull request description:
Descriptor wallets do not use the watchonly behavior as it is not possible to mix watchonly and non-watchonly in a descriptor wallet. With legacy wallets now removed, all of the watchonly handling and reporting code is no longer needed. This PR removes watchonly options and results from the RPCs and the handling of watchonly things from the wallet's internals.
With all of the watchonly things removed, ISMINE_WATCH_ONLY is removed as well.
Split from #32523
Depends on #32594 for tests that are easier to read
ACKs for top commit:
Eunovo:
ACK b1a8ac07e9
maflcko:
re-ACK b1a8ac07e91dd1d305fcbc16ea931d60e46c0055 🌈
rkrux:
ACK b1a8ac07e91dd1d305fcbc16ea931d60e46c0055
furszy:
light code review ACK b1a8ac07e91dd1d305fcbc16ea931d60e46c0055
Tree-SHA512: bc87f37a13294f7208991be8f93899b49e5bdf87c70e0f66d9c4cb09c03be6c202320406f27e9a35aa2f57319d19a3f0c07d5e5ddbc97c7edab165b1656d6612
1632fc104be8f171f59a023800b2f9f20d0a3cff txgraph: Track multiple potential would-be clusters in Trim (improvement) (Pieter Wuille)
4608df37e02abde09da5d6668dcd4dc5a6e7c569 txgraph: add Trim benchmark (benchmark) (Pieter Wuille)
9c436ff01cffe51e34d0f29c445db11bb807c6c3 txgraph: add fuzz test scenario that avoids cycles inside Trim() (tests) (Pieter Wuille)
938e86f8fecd65ca90b97e6cf896f8c59fb590ba txgraph: add unit test for TxGraph::Trim (tests) (glozow)
a04e205ab03efa105f7886449c2c9316f67a85c1 txgraph: Add ability to trim oversized clusters (feature) (Pieter Wuille)
eabcd0eb6fca5790ea19e7c1613ae06df8d21918 txgraph: remove unnecessary m_group_oversized (simplification) (Greg Sanders)
19b14e61eae7f0f0bfc8d17b06cc0e3ebd1f994d txgraph: Permit transactions that exceed cluster size limit (feature) (Pieter Wuille)
c4287b9b71c6d5222bcd0d2af3185de93ce76078 txgraph: Add ability to configure maximum cluster size/weight (feature) (Pieter Wuille)
Pull request description:
Part of cluster mempool (#30289).
During reorganisations, it is possible that dependencies get added which would result in clusters that violate policy limits (cluster count, cluster weight), when linking the new from-block transactions to the old from-mempool transactions. Unlike RBF scenarios, we cannot simply reject the changes when they are due to received blocks. To accommodate this, add a `TxGraph::Trim()`, which removes some subset of transactions (including descendants) in order to make all resulting clusters satisfy the limits.
Conceptually, the way this is done is by defining a rudimentary linearization for the entire would-be too-large cluster, iterating it from beginning to end, and reasoning about the counts and weights of the clusters that would be reached using transactions up to that point. If a transaction is encountered whose addition would violate the limit, it is removed, together with all its descendants.
This rudimentary linearization is like a merge sort of the chunks of the clusters being combined, but respecting topology. More specifically, it is continuously picking the highest-chunk-feerate remaining transaction among those which have no unmet dependencies left. For efficiency, this rudimentary linearization is computed lazily, by putting all viable transactions in a heap, sorted by chunk feerate, and adding new transactions to it as they become viable.
The `Trim()` function is rather unusual compared to the `TxGraph` functionality added in previous PRs, in that `Trim()` makes it own decisions about what the resulting graph contents will be, without good specification of how it makes that decision - it is just a best-effort attempt (which is improved in the last commit). All other `TxGraph` mutators are simply to inform the graph about changes the calling mempool code decided on; this one lets the decision be made by txgraph.
As part of this, the "oversized" property is expanded to also encompass a configurable cluster weight limit (in addition to cluster count limit).
ACKs for top commit:
instagibbs:
reACK 1632fc104be8f171f59a023800b2f9f20d0a3cff
glozow:
reACK 1632fc104be via range-diff
ismaelsadeeq:
reACK 1632fc104be8f171f59a023800b2f9f20d0a3cff 🛰️
Tree-SHA512: ccacb54be8ad622bd2717905fc9b7e42aea4b07f824de1924da9237027a97a9a2f1b862bc6a791cbd2e1a01897ad2c7c73c398a2d5ccbce90bfbeac0bcebc9ce
de4eef52d123b781b833841a9765d1788010ac6b threading: use correct mutex name in reverse_lock fatal error messages (Cory Fields)
Pull request description:
"Now that REVERSE_LOCK requires the name of the actual mutex, it can be used for better error messages." - theuni
This is a follow-up to this comment https://github.com/bitcoin/bitcoin/pull/32465#issuecomment-2981287545
I just cherry-picked the commit 85c2848eb575f4abaa81fdd4e8f3b2048693dd98
ACKs for top commit:
theuni:
Re-ACK de4eef52d123b781b833841a9765d1788010ac6b
TheCharlatan:
ACK de4eef52d123b781b833841a9765d1788010ac6b
Tree-SHA512: 1109381e1f0589093f7c737cb1ebd1c43324a9e1ea34b5f05a9171d06ab44cca0c5ead43c581f6e37ded1f0463ab8a280f3319c288d39a4625109b5c08a7cb68
9b75cfda4d62a0a3bde402503244dd57e1621a12 test: retain the intended behavior of `feature_fee_estimation.py` nodes (ismaelsadeeq)
5c1236f04a24716b2cbd9b9b283863d3a8a6fa87 test: fix incorrect subtest in `feature_fee_estimation.py` (ismaelsadeeq)
Pull request description:
Attempt to fix#32461
In the `estimatesmartfee` RPC, we return the maximum of the following: the feerate estimate for the target, `minrelaytxfee`, and `mempoolminfee`.
9a05b45da6/src/rpc/fees.cpp (L85)
The test `test_feerate_mempoolminfee`, originally introduced in ea31caf6b4, is incorrect.
It should calculate the fee rate ceiling by taking the maximum of the custom `minrelaytxfee`, `mempoolminfee`, and the highest fee rate observed during the test (`check_smart_estimates`). This is necessary because:
* There is no guarantee that the generated fee rates will exceed both `minrelaytxfee` and `mempoolminfee`.
* Users can start a node with custom fee settings.
Due to the non-deterministic nature of the `feature_fee_estimation.py` test, it often passes by chance. The randomly generated fees typically include a value higher than the custom `minrelaytxfee`, inadvertently hiding the issue.
Issue #32461 identified a random seeds that consistently fails the test because the generated fees never exceed the custom `minrelaytxfee`:
e.g
```
build/test/functional/feature_fee_estimation.py --random=3450808900320758527
```
This PR has two commits which :
* Correctly fixes the test by calculating the fee rate ceiling as the maximum of the node `minrelaytxfee`, `mempoolminfee`, and the highest seen fee rate, when verifying smart fee estimates.
* Improves the subtest name and comment for clarity.
* Restores the original test behavior by appending 4000 WU to the custom `blockmaxweight`.
ACKs for top commit:
achow101:
ACK 9b75cfda4d62a0a3bde402503244dd57e1621a12
glozow:
ACK 9b75cfda4d62a0a3bde402503244dd57e1621a12
theStack:
Light ACK 9b75cfda4d62a0a3bde402503244dd57e1621a12
Tree-SHA512: 0f7fb0496b50a399b58f6fb1afd95414fad454795fbc0046e22dfc54a2062ae0c519a12ebfeb6ad7ef547438868d99eca2351c0d19d0346adaadb500eff6f15f
c10e382d2a3b76b70ebb8a4eb5cd99fc9f14d702 flatfile: check whether the file has been closed successfully (Vasil Dimov)
4bb5dd78ea4b578922a3316b37b486f96cb0beec util: check that a file has been closed before ~AutoFile() is called (Vasil Dimov)
8bb34f07df9ad45faf25c32c99a4dd70759b25be Explicitly close all AutoFiles that have been written (Vasil Dimov)
a69c4098b273b6db5d2212ba91cfc713c1634c5d rpc: take ownership of the file by WriteUTXOSnapshot() (Hodlinator)
Pull request description:
`fclose(3)` may fail to flush the previously written data to disk, thus a failing `fclose(3)` is as serious as a failing `fwrite(3)`.
Previously the code ignored `fclose(3)` failures. This PR improves that by changing all users of `AutoFile` that use it to write data to explicitly close the file and handle a possible error.
---
Other alternatives are:
1. `fflush(3)` after each write to the file (and throw if it fails from the `AutoFile::write()` method) and hope that `fclose(3)` will then always succeed. Assert that it succeeds from the destructor 🙄. Will hurt performance.
2. Throw nevertheless from the destructor. Exception within the exception in C++ I think results in terminating the program without a useful message.
3. (this is implemented in the latest incarnation of this PR) Redesign `AutoFile` so that its destructor cannot fail. Adjust _all_ its users 😭. For example, if the file has been written to, then require the callers to explicitly call the `AutoFile::fclose()` method before the object goes out of scope. In the destructor, as a sanity check, assume/assert that this is indeed the case. Defeats the purpose of a RAII wrapper for `FILE*` which automatically closes the file when it goes out of scope and there are a lot of users of `AutoFile`.
4. Pass a new callback function to the `AutoFile` constructor which will be called from the destructor to handle `fclose()` errors, as described in https://github.com/bitcoin/bitcoin/pull/29307#issuecomment-2243842400. My thinking is that if that callback is going to only log a message, then we can log the message directly from the destructor without needing a callback. If the callback is going to do more complicated error handling then it is easier to do that at the call site by directly calling `AutoFile::fclose()` instead of getting the `AutoFile` object out of scope (so that its destructor is called) and inspecting for side effects done by the callback (e.g. set a variable to indicate a failed `fclose()`).
ACKs for top commit:
l0rinc:
ACK c10e382d2a3b76b70ebb8a4eb5cd99fc9f14d702
achow101:
ACK c10e382d2a3b76b70ebb8a4eb5cd99fc9f14d702
hodlinator:
re-ACK c10e382d2a3b76b70ebb8a4eb5cd99fc9f14d702
Tree-SHA512: 3994ca57e5b2b649fc84f24dad144173b7500fc0e914e06291d5c32fbbf8d2b1f8eae0040abd7a5f16095ddf4e11fe1636c6092f49058cda34f3eb2ee536d7ba
0a1af4418ed2a135001cc0e10f0af44cbd64e521 doc: Add workaround for vcpkg issue with paths with embedded spaces (Hennadii Stepanov)
Pull request description:
This PR is an alternative to https://github.com/bitcoin/bitcoin/pull/32397 suggested in https://github.com/bitcoin/bitcoin/pull/32397#issuecomment-2883438040.
ACKs for top commit:
sipsorcery:
ACK 0a1af4418ed2a135001cc0e10f0af44cbd64e521.
Tree-SHA512: 09efc3f8b8c38eb8d8de1022c5b11685fba334cdb950209d35b3b0d907fa50de3f85f45c66ac3670b5b104d5bb0b01461c36a0c1e595327b5e62c6abca323deb
4f10a57671c19cacca630b2401e42a213aacff1b depends: Override host compilers for FreeBSD and OpenBSD (Hennadii Stepanov)
Pull request description:
When building depends on FreeBSD/OpenBSD `aarch64`, the host compilers default to `default_host_{CC,CXX}`, which resolves to `gcc`/`g++`. This is incorrect on these systems, where Clang is the default system compiler.
To ensure proper compiler selection, this PR adopts the same approach used for `darwin`:c1d4253d31/depends/builders/darwin.mk (L12-L14)
Fixes https://github.com/bitcoin/bitcoin/issues/32691.
ACKs for top commit:
Sjors:
ACK 4f10a57671c19cacca630b2401e42a213aacff1b
Tree-SHA512: 66ace60b63671f2ab7facb84776298e1489aa0a984559c9a22323adaaedcc86f67158078b18c333cb7bdc2b17a4573e321ee7361596bc941db060c5659014069
67dc7523f3e103c8359b546d38f28c1feb2b9b34 cmake, test: Disable tests instead of ignoring them (Hennadii Stepanov)
bb9157db5d3920d971e365e416de4e23866c715a cmake, refactor: Switch to `Python3::Interpreter` imported target (Hennadii Stepanov)
Pull request description:
This PR:
1. Switches to a modern CMake approach by using the `Python3::Interpreter` imported target, which is more robust than using variables.
2. Disables the `util_rpcauth_test` test explicitly instead of silently ignoring it.
A build and test log for the case when Python is unavailable is provided below:
```
$ cmake -B build
$ cmake --build build -j 16
$ ctest --test-dir build -j $(nproc) -R "^util"
Internal ctest changing into directory: /bitcoin/build
Test project /bitcoin/build
Start 115: util_tests
Start 117: util_trace_tests
Start 114: util_string_tests
Start 116: util_threadnames_tests
Start 1: util_rpcauth_test
1/5 Test #1: util_rpcauth_test ................***Not Run (Disabled) 0.00 sec
2/5 Test #114: util_string_tests ................ Passed 0.11 sec
3/5 Test #117: util_trace_tests ................. Passed 0.11 sec
4/5 Test #116: util_threadnames_tests ........... Passed 0.11 sec
5/5 Test #115: util_tests ....................... Passed 0.13 sec
100% tests passed, 0 tests failed out of 4
Total Test time (real) = 0.13 sec
The following tests did not run:
1 - util_rpcauth_test (Disabled)
```
ACKs for top commit:
purpleKarrot:
ACK 67dc7523f3e103c8359b546d38f28c1feb2b9b34
janb84:
tACK 67dc7523f3e103c8359b546d38f28c1feb2b9b34
Tree-SHA512: 5fc7ebe31ac03f4b8a53ecfcfc1cace0f647a1d2c989651988edae96bdfbbe2dee171714e57cb028e65ead1bb40806a82d9821746451dbf005538601fd33ea88
666016e56b28b77f798dc85c767b95c1ca0abfae ci: use --usecli in one of the CI jobs (Martin Zumsande)
7ea248a020997fcbbd6d62a4ec191aa858c463ca test: Disable several (sub)tests with cli (Martin Zumsande)
f420b6356b6f886638282a6aa9309b9982768775 test: skip subtests that check for wrong types with cli (Martin Zumsande)
6530d0015b958412825e1b8ae2aaefeee4372f08 test: add function to convert to json for height_or_hash params (Martin Zumsande)
54d28722baeac84950bbe6da1d315b9202012259 test: Don't send empty named args with cli (Martin Zumsande)
cca422060e96abbdca68a931fd45653738923caa test: convert tuple to json for cli (Martin Zumsande)
af34e980866e16970e0c4b837f56cd29038ae8bc test: make rpc_psbt.py usable with --usecli (Martin Zumsande)
8f8ce9e1740dfb5249b20d2bf759c23080367553 test: rename .rpc to ._rpc and remove unnecessary uses (Martin Zumsande)
5b088859863224a94514c78ea841d7314ec8fa50 test: enable functional tests with large rpc args for cli (Martin Zumsande)
7d5352ac7373e2beb9af671cef0a1a1e9ecb6658 test: use -stdin for large rpc commands (Martin Zumsande)
6c364e0c10de4d8036e696b3726bb0acffb94617 test: Enable various tests for usage with cli (Martin Zumsande)
Pull request description:
Fixes#32264
I looked into all current failures listed in the issue, as well all tests that are already disabled for the cli with `self.supports_cli = False`. There are several reasons why existing tests fail with `--usecli` on many systems, the most important ones are:
- Most common reason is that the test executes a RPC call with a large arg that exceeds `MAX_ARG_STRLEN` of the OS, which is usually 128kb on linux: This is fixed by using `-stdin` for these large calls (idea by 0xB10C)
- they test specifically the rpc interface - nothing to do there except disabling.
- Some functional test submit wrong types to params on purpose to test the error message (which is different when using the cli) - deactivated these specific subtests locally for the cli when there is just one or two of them, deactivated the entire tests when there are more spots
- When python sets `None` for an arg, the cli converts this to 'null' in `arg_to_cli`. This is fine e.g. for boolean args, but doesn't work for strings where it's interpreted as the string 'null'. Bypass this for named args by not including args in case the value is `None` for the cli is used (it's effectively the same as leaving the optional arg out).
- the `height_or_hash` param used in some RPC needs to be converted to a JSON (effectively adding full quotes).
- Some tests were marked with `self.supports_cli = False` in the past but run fine on master today - enabled those.
In total, this PR fixes all tests that fail on master and reduces the number of tests that are deactivated (`self.supports_cli = False`) from 40 to 21.
It also adds `--usecli` to one CI job (multiprocess, i686, DEBUG) to detect regressions.
ACKs for top commit:
maflcko:
re-ACK 666016e56b28b77f798dc85c767b95c1ca0abfae 🔀
pinheadmz:
re-ACK 666016e56b28b77f798dc85c767b95c1ca0abfae
Tree-SHA512: 7a1efd212649ca100b236a1239294d40ecd36e2720e3b173a230b14545bb40b135111db7fed8a0d1448120f5387da146a03f1912e2028c8d03a0b6a3ca8761b0
d6aaffcb11adcf47480fcc5081af9dcb732decf3 test: check P2SH sigop count for coinbase tx (brunoerg)
Pull request description:
We currently do not test that `GetP2SHSigOpCount` returns 0 for coinbase transactions (see line L129 at https://corecheck.dev/mutation/src/consensus/tx_verify.cpp). This PR addresses it.
ACKs for top commit:
darosior:
That said, i guess unit-tested dead consensus code is better than not-unit-tested dead consensus code. utACK d6aaffcb11adcf47480fcc5081af9dcb732decf3
theStack:
ACK d6aaffcb11adcf47480fcc5081af9dcb732decf3
w0xlt:
ACK d6aaffcb11
ishaanam:
ACK d6aaffcb11adcf47480fcc5081af9dcb732decf3
pablomartin4btc:
ACK d6aaffcb11adcf47480fcc5081af9dcb732decf3
Tree-SHA512: a7d7306f064bb2ec7e93e92625848ae38e150ebb67bde37cd15be1038816b154e867ad21ecd2685d8de5341b67e3b768d30b7654e27b541f33e8f9d63e52261d
Avoids initializing and syncing components not under test.
This not only speeds up execution a bit but also helps isolate
and debug issues more easily, as logs aren't flooded with
unrelated details.
ec004cdb86e6471915e1033f390c76ee0428e415 test: Use rehash() in outbound eviction block-relay (pablomartin4btc)
26598ed21ea7228c4ecf85da24527c88f9c1f1c1 test: Clarify roles in outbound eviction comments (pablomartin4btc)
Pull request description:
This change avoids relying on `tip_header.hash`, which is `None` when the header is deserialized from hex during `CBlockHeader()` construction.
Instead, `tip_header.rehash()` explicitly computes the hash, making the test behavior more robust.
Using the explicit `rehash()` avoids depending on `wait_for_getheaders()` falling back to any received message, thus making the test more deterministic.
This is a follow-up to #32742.
Also, as noted in a previous review [comment](https://github.com/bitcoin/bitcoin/pull/32742#pullrequestreview-2923802386), "_the hash field is wrong either way, simply due to being the wrong type (it is an optional hex string), as opposed to an optional int_".
---
The first commit intention is to improve clarity around the tests purpose, helping reviewers follow what's being verified and why. What started as a small comment during review of #32742 led me reviewing and try to improve most relevant tests comments for consistency and correctness.
ACKs for top commit:
achow101:
ACK ec004cdb86e6471915e1033f390c76ee0428e415
theStack:
lgtm ACK ec004cdb86e6471915e1033f390c76ee0428e415 #️⃣
yuvicc:
ACK ec004cdb86e6471915e1033f390c76ee0428e415
danielabrozzoni:
ACK ec004cdb86e6471915e1033f390c76ee0428e415
Tree-SHA512: 6a14dedfdc425cd806f63443b3b9f79df69a7717452739f5d7fef1b2bdba23402670d63cf1d6b66c9f1a6b460d4d4a6f185426d0a4982fa95115a234cd6baef7
b78990734621b8fe46c68a6e7edaf1fbd2f7d351 wallet: migration, avoid creating spendable wallet from a watch-only legacy wallet (furszy)
e86d71b749c08bde6002b9aa2baee824975a518a wallet: refactor, dedup wallet re-loading code (furszy)
1de423e0a08bbc63eed36c8772e9ef8b48e80fb8 wallet: introduce method to return all db created files (furszy)
d04f6a97ba9a55aa9455e1a805feeed4d630f59a refactor: remove sqlite dir path back-and-forth conversion (furszy)
Pull request description:
Currently, the migration process creates a brand-new descriptor wallet with no
connection to the user's legacy wallet when the legacy wallet lacks key material
and contains only watch-only scripts. This behavior is not aligned with user
expectations. If the legacy wallet contains only watch-only scripts, the migration
process should only generate a watch-only wallet instead.
TODO List:
* Explain that `migratewallet` renames the watch-only after migration, and
also that the wallet will not have keys enabled.
ACKs for top commit:
achow101:
ACK b78990734621b8fe46c68a6e7edaf1fbd2f7d351
pablomartin4btc:
tACK b78990734621b8fe46c68a6e7edaf1fbd2f7d351
rkrux:
LGTM ACK b78990734621b8fe46c68a6e7edaf1fbd2f7d351
Tree-SHA512: 1d583ac4b206fb477e9727daf4b5ad9c3e18b12d40e1ab4a61e8565da44c3d0327c892b51cf47b4894405d122e414cefb6b6366c357e02a74a7ca96e06762d83
In the existing Trim function, as soon as the set of accepted transactions
would exceed the max cluster size or count limit, the acceptance loop is
stopped, removing all later transactions. However, it is possible that by
excluding some of those transactions the would-be cluster splits apart into
multiple would-clusters. And those clusters may well permit far more
transactions before their limits are reached.
Take this into account by using a union-find structure inside TrimTxData to
keep track of the count/size of all would-be clusters that would be formed
at any point, and only reject transactions which would cause these resulting
partitions to exceed their limits.
This is not an optimization in terms of CPU usage or memory; it just
improves the quality of the transactions removed by Trim().
Trim internally builds an approximate dependency graph of the merged cluster,
replacing all existing dependencies within existing clusters with a simple
linear chain of dependencies. This helps keep the complexity of the merging
operation down, but may result in cycles to appear in the general case, even
though in real scenarios (where Trim is called for stitching re-added mempool
transactions after a reorg back to the existing mempool transactions) such
cycles are not possible.
Add a test that specifically targets Trim() but in scenarios where it is
guaranteed not to have any cycles. It is a special case, is much more a
whitebox test than a blackbox test, and relies on randomness rather than
fuzz input. The upside is that somewhat stronger properties can be tested.
Co-authored-by: Greg Sanders <gsanders87@gmail.com>
During reorganisations, it is possible that dependencies get add which
result in clusters that violate limits (count, size), when linking the
new from-block transactions to the old from-mempool transactions.
Unlike RBF scenarios, we cannot simply reject these policy violations
when they are due to received blocks. To accomodate this, add a Trim()
function to TxGraph, which removes transactions (including descendants)
in order to make all resulting clusters satisfy the limits.
In the initial version of the function added here, the following approach
is used:
- Lazily compute a naive linearization for the to-be-merged cluster (using
an O(n log n) algorithm, optimized for far larger groups of transactions
than the normal linearization code).
- Initialize a set of accepted transactions to {}
- Iterate over the transactions in this cluster one by one:
- If adding the transaction to the set makes it exceed the max cluster size
or count limit, stop.
- Add the transaction to the set.
- Remove all transactions from the cluster that were not included in the set
(note that this necessarily includes all descendants too, because they
appear later in the naive linearization).
Co-authored-by: Greg Sanders <gsanders87@gmail.com>
This removes the restriction added in the previous commit that individual
transactions do not exceed the max cluster size limit.
With this change, the responsibility for enforcing cluster size limits can
be localized purely in TxGraph, without callers (and in particular, tests)
needing to duplicate the enforcement for individual transactions.
