a60f863d3e scripted-diff: Replace GenTxidVariant with GenTxid (marcofleon)
c8ba199598 Remove old GenTxid class (marcofleon)
072a198ea4 Convert remaining instances of GenTxid to GenTxidVariant (marcofleon)
1b528391c7 Convert `txrequest` to GenTxidVariant (marcofleon)
bde4579b07 Convert `txdownloadman_impl` to GenTxidVariant (marcofleon)
c876a892ec Replace GenTxid with Txid/Wtxid overloads in `txmempool` (marcofleon)
de858ce2be move-only: make GetInfo a private CTxMemPool member (stickies-v)
eee473d9f3 Convert `CompareInvMempoolOrder` to GenTxidVariant (marcofleon)
243553d590 refactor: replace get_iter_from_wtxid with GetIter(const Wtxid&) (stickies-v)
fcf92fd640 refactor: make CTxMemPool::GetIter strongly typed (marcofleon)
11d28f21bb Implement GenTxid as a variant (marcofleon)
Pull request description:
Part of the [type safety refactor](https://github.com/bitcoin/bitcoin/pull/32189).
This PR changes the GenTxid class to a variant, which holds both Txids and Wtxids. This provides compile-time type safety and eliminates the manual type check (bool m_is_wtxid). Variables that can be either a Txid or a Wtxid are now using the new GenTxid variant, instead of uint256.
ACKs for top commit:
w0xlt:
ACK a60f863d3e
dergoegge:
Code review ACK a60f863d3e
maflcko:
review ACK a60f863d3e🎽
theStack:
Code-review ACK a60f863d3e
Tree-SHA512: da9b73b7bdffee2eb9281a409205519ac330d3336094d17681896703fbca8099608782c9c85801e388e4d90af5af8abf1f34931f57bbbe6e9674d802d6066047
fa894b0f3e log: Properly log warnings with warn loglevel in addrdb (MarcoFalke)
Pull request description:
The logging in addrdb is confusing, because it uses `LogPrintf` (info level) to log warnings.
Fix this by properly using the `warn` level, where needed. Also, drop unused trailing `\n` while touching the lines.
ACKs for top commit:
stickies-v:
ACK fa894b0f3e
dergoegge:
utACK fa894b0f3e
Tree-SHA512: 96d3823623ea8e1698e8cb541ca97cbab7b2a9934b2f894884171045abbca7be796f07965082e997001c97d06d1e0c4d13b29354eb4fe71c3a2ee680eada5516
fa8862723c fuzz: CheckGlobals in init (MarcoFalke)
fa26bfde98 test: Avoid resetting mocktime in testing setup (MarcoFalke)
fa6b45fa8e Add SetMockTime for time_point types (MarcoFalke)
Pull request description:
(Tracking issue https://github.com/bitcoin/bitcoin/issues/29018)
During fuzzing, `AppInitParameterInteraction` may actually disable a previously set mocktime. This is confusing and can also cause non-determinism.
Fix this issue, by
* fixing the erroneous `-mocktime` parsing in `AppInitParameterInteraction`.
* adding the missing `SetMockTime` calls to the affected fuzz init functions.
* adding a `CheckGlobals` to the fuzz init, to prevent this issue in the future.
This can be tested by
* Cherry-picking the `CheckGlobals`-commit onto current master and observing a fuzz failure in the touched fuzz targets.
* Reverting the touched fuzz fixups and observing a fuzz failure for each target.
ACKs for top commit:
w0xlt:
ACK fa8862723c
dergoegge:
utACK fa8862723c
Tree-SHA512: 5a9400f0467c82fa224713af4cc2b525afbefefc7c3f419077110925ad7af6c7fda3dcd2b50f7facf0ee7df2547c6ac20336906d707adcdfd1d652a9d9a735fe
f43571010e Resolve guix non-determinism with emplace_back instead of push_back (Ava Chow)
Pull request description:
For some reason, building x86_64-w64-mingw32 on x86_64 and aarch64 results in a single instruction difference which can be traced down to prevector.h:174. The ultimate caller of this is the copy constructor for a prevector that ends up being called by std::vector::push_back in walletmodel.cpp:183. By replacing the push_back with an emplace_back, somehow this non-determinism goes away.
Closes#32923
ACKs for top commit:
l0rinc:
code review ACK f43571010e
Sjors:
utACK f43571010e
maflcko:
lgtm ACK f43571010e
Tree-SHA512: 5bf0571f32cb72efc0c533e16d2704cfc3a79bcef2943f0892743572808610fb00ca8ab41223897536f8e5090bf4030735be910942de8116652d02bc3f231e2e
150b5c99ca wallet: replace `reload_wallet` with inline functionality (rkrux)
0f86da382d wallet: remove dead code in legacy wallet migration (rkrux)
Pull request description:
A discussion on a previous [PR 32481](https://github.com/bitcoin/bitcoin/pull/32481#discussion_r2145152084) related to legacy wallet dead
code removal made me realize that checking if the legacy
wallet was loaded prior to the start of the migration is not
required ever since legacy wallets can't be loaded in the first
place. I also verified that the `load_on_start` persistent
setting can also not cause the legacy wallets to be loaded, which
further makes the case for removal of the above mentioned checks
during migration.
The current test coverage also shows these lines uncovered.
ACKs for top commit:
achow101:
ACK 150b5c99ca
furszy:
ACK 150b5c99ca
Tree-SHA512: 9bc7043cac1f4051228557208895e43648de3c7ffae6860c0676d1aa2db3a8ed3a09d1f9defacd96ca50bbb9699ba86652ccb0c5e55cc88be248a1fe727c13d9
d7fca5c171 clusterlin: add big comment explaning the relation between tests (Pieter Wuille)
b64e61d2de clusterlin: abstract try-permutations into ExhaustiveLinearize function (Pieter Wuille)
1fa55a64ed clusterlin tests: verify that chunks are minimal (Pieter Wuille)
da23ecef29 clusterlin tests: support non-empty ReadTopologicalSubset() (Pieter Wuille)
94f3e17c33 clusterlin tests: compare with fuzz-provided linearizations (Pieter Wuille)
5f92ebee0d clusterlin tests: compare with fuzz-provided topological sets (Pieter Wuille)
6e37824ac3 clusterlin tests: optimize clusterlin_simple_linearize (Pieter Wuille)
98c1c88b6f clusterlin tests: separate testing of SimpleLinearize and Linearize (Pieter Wuille)
10e90f7aef clusterlin tests: make SimpleCandidateFinder always find connected (Pieter Wuille)
a38c38951e clusterlin tests: separate testing of Search- and SimpleCandidateFinder (Pieter Wuille)
77a432ee70 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 d7fca5c171
ismaelsadeeq:
re-ACK d7fca5c171
monlovesmango:
ACK d7fca5c171
Tree-SHA512: 33cb76bd9b9547a5f3ee231fa452e928f064ad03af98e3d9e64246eb972f2b026c13e7367257ccdac1ae57982ee8ef98c907684588ecbb4bc4c82cbec160b3e8
For some reason, building x86_64-w64-mingw32 on x86_64 and aarch64
results in a single instruction difference which can be traced down to
prevector.h:174. The ultimate caller of this is the copy constructor for
a prevector that ends up being called by std::vector::push_back in
walletmodel.cpp:183. By replacing the push_back with an emplace_back,
somehow this non-determinism goes away.
fa2fbaa4a2 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 fa2fbaa4a2
Tree-SHA512: 33798030f990d1b4c95be4682d8dbfad95e8716d5fc0b99d65937196f2ced1ba649193c2adba4155f4eec9fd06e16be6667f3c3705af1880f47b2ff57a76243b
8cc3ac6c23 validation: Don't use IsValid() to filter for invalid blocks (Martin Zumsande)
86d98b94e5 test: verify that ancestors of a reconsidered block can become the chain tip (stratospher)
3c39a55e64 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 8cc3ac6c23
TheCharlatan:
Re-ACK 8cc3ac6c23
stratospher:
reACK 8cc3ac6.
Tree-SHA512: 53f27591916246be4093d64b86a0494e55094abd8c586026b1247e4a36747bc3d6dbe46dc26ee4a22f47b8eb0d9699d13e577dee0e7198145f3c9b11ab2a30b7
1b5c545e82 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 1b5c545e82
pablomartin4btc:
cr-ACK 1b5c545e82
Tree-SHA512: 34edde55beef5714cea2e1131c29b57da2dc32ea091cd81878014de503c128f02c3ab88aee1e456541d7937e033dca5a81b03e9e2888cf781d71b62ad9b5ca5c
922adf66ac mempool: use `FeeFrac` for calculating regular score (Sebastian Falbesoner)
3322b3a059 mempool: use `FeeFrac` for calculating ancestor score (Sebastian Falbesoner)
ac9c113bd2 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 ce8e22542e
ACKs for top commit:
ismaelsadeeq:
Code review ACK 922adf66ac
glozow:
ACK 922adf66ac
Tree-SHA512: 6c3a9436f2be668aa8561b40c1b93efa7dc97b4ef354e98233ac3d3286a88804668164a55f2fcce4239fee5830e4e70f520e6285b667b87baa65c7cec09159cf
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>
Now that we are storing `CTxMemPool::CompareDepthAndScore` parameters using
`std::variant` we have no portable zero-overhead way of accessing them,
so use `std::visit` and drop `bool wtxid` in-parameter.
Co-authored-by: stickies-v <stickies-v@protonmail.com>
6d19815cd4 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 6d19815cd4
brunoerg:
code review ACK 6d19815cd4
Tree-SHA512: 420454f1cc09db44c1d76423d8623a0b8865d41d6c34015844ff83d78a9373e3e26f3f62818d1502b33eb063caf904750e858b74ddecd76750577ae82b64b0c1
Also, update related comments because a reload is not happening
anymore. It is done because the legacy wallets could not have been
loaded prior to migration, so I don't think a reload is happening
post a successful migration, it's just load IMO.
4e69aa5701 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 4e69aa5701
maflcko:
lgtm ACK 4e69aa5701
Tree-SHA512: 3737313f7a9da55c67c78ce01bab5005946f4e1fccbb471560ff3af8c8275cb5cf876f6c53400c93f0ba1fdf134f28766ed573cbe62903127a3129ca8ce88db6
fcfd3db563 remove RPCTimerInterface and RPCRunLater (Matthew Zipkin)
8a1765795f 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 fcfd3db563
achow101:
ACK fcfd3db563
furszy:
ACK fcfd3db563
Tree-SHA512: 04f5e9c3f73f598c3d41d6e35bb59c64c7b93b03ad9fce3c40901733147ce7764f41f475fef1527d44af18f722759996a31ca83b48cb52153795d5022fecfd14
fa946520d2 refactor: Use structured binding for-loop (MarcoFalke)
eeeec1579e 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 fa946520d2 (edited)
achow101:
ACK fa946520d2
rkrux:
re-ACK fa946520d2
Tree-SHA512: 23dac6e0fe925561bfbf421e6a7441d546eed8c1492ac41ca4ed7dfcd12f4d2ef39c35f105a0291aac511365d98f08fbdc9a4f0bf627172873b8f23c2be45e76
b1a8ac07e9 doc: Release note for removed watchonly parameters and results (Ava Chow)
15710869e1 wallet: Remove ISMINE_WATCH_ONLY (Ava Chow)
4439bf4b41 wallet, spend: Remove fWatchOnly from CCoinControl (Ava Chow)
1337c72198 wallet, rpc: Remove watchonly from RPCs (Ava Chow)
e81d95d435 wallet: Remove watchonly balances (Ava Chow)
d20dc9c6aa wallet: Wallets without private keys cannot grind R (Ava Chow)
9991f49c38 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 b1a8ac07e9🌈
rkrux:
ACK b1a8ac07e9
furszy:
light code review ACK b1a8ac07e9
Tree-SHA512: bc87f37a13294f7208991be8f93899b49e5bdf87c70e0f66d9c4cb09c03be6c202320406f27e9a35aa2f57319d19a3f0c07d5e5ddbc97c7edab165b1656d6612
1632fc104b txgraph: Track multiple potential would-be clusters in Trim (improvement) (Pieter Wuille)
4608df37e0 txgraph: add Trim benchmark (benchmark) (Pieter Wuille)
9c436ff01c txgraph: add fuzz test scenario that avoids cycles inside Trim() (tests) (Pieter Wuille)
938e86f8fe txgraph: add unit test for TxGraph::Trim (tests) (glozow)
a04e205ab0 txgraph: Add ability to trim oversized clusters (feature) (Pieter Wuille)
eabcd0eb6f txgraph: remove unnecessary m_group_oversized (simplification) (Greg Sanders)
19b14e61ea txgraph: Permit transactions that exceed cluster size limit (feature) (Pieter Wuille)
c4287b9b71 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 1632fc104b
glozow:
reACK 1632fc104b via range-diff
ismaelsadeeq:
reACK 1632fc104b🛰️
Tree-SHA512: ccacb54be8ad622bd2717905fc9b7e42aea4b07f824de1924da9237027a97a9a2f1b862bc6a791cbd2e1a01897ad2c7c73c398a2d5ccbce90bfbeac0bcebc9ce
de4eef52d1 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 de4eef52d1
TheCharlatan:
ACK de4eef52d1
Tree-SHA512: 1109381e1f0589093f7c737cb1ebd1c43324a9e1ea34b5f05a9171d06ab44cca0c5ead43c581f6e37ded1f0463ab8a280f3319c288d39a4625109b5c08a7cb68
c10e382d2a flatfile: check whether the file has been closed successfully (Vasil Dimov)
4bb5dd78ea util: check that a file has been closed before ~AutoFile() is called (Vasil Dimov)
8bb34f07df Explicitly close all AutoFiles that have been written (Vasil Dimov)
a69c4098b2 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 c10e382d2a
achow101:
ACK c10e382d2a
hodlinator:
re-ACK c10e382d2a
Tree-SHA512: 3994ca57e5b2b649fc84f24dad144173b7500fc0e914e06291d5c32fbbf8d2b1f8eae0040abd7a5f16095ddf4e11fe1636c6092f49058cda34f3eb2ee536d7ba
67dc7523f3 cmake, test: Disable tests instead of ignoring them (Hennadii Stepanov)
bb9157db5d 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 67dc7523f3
janb84:
tACK 67dc7523f3
Tree-SHA512: 5fc7ebe31ac03f4b8a53ecfcfc1cace0f647a1d2c989651988edae96bdfbbe2dee171714e57cb028e65ead1bb40806a82d9821746451dbf005538601fd33ea88
A discussion on a previous PR 32481 related to legacy wallet dead
code removal made me realize that checking if the legacy
wallet was loaded prior to the start of the migration is not
required ever since legacy wallets can't be loaded in the first
place. I also verified that the `load_on_start` persistent
setting can also not cause the legacy wallets to be loaded, which
further makes the case for removal of the above mentioned checks
during migration.
The current test coverage also shows these lines uncovered.
d6aaffcb11 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 d6aaffcb11
theStack:
ACK d6aaffcb11
w0xlt:
ACK d6aaffcb11
ishaanam:
ACK d6aaffcb11
pablomartin4btc:
ACK d6aaffcb11
Tree-SHA512: a7d7306f064bb2ec7e93e92625848ae38e150ebb67bde37cd15be1038816b154e867ad21ecd2685d8de5341b67e3b768d30b7654e27b541f33e8f9d63e52261d
b789907346 wallet: migration, avoid creating spendable wallet from a watch-only legacy wallet (furszy)
e86d71b749 wallet: refactor, dedup wallet re-loading code (furszy)
1de423e0a0 wallet: introduce method to return all db created files (furszy)
d04f6a97ba 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 b789907346
pablomartin4btc:
tACK b789907346
rkrux:
LGTM ACK b789907346
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().
