`IWYU pragma: export` enforces the transitive inclusion of the headers,
which undermines the purpose of IWYU.
The remained cases seem useful and could be considered separately:
- `<cassert>` in `util/check.h`
- `<filesystem>` in `util/fs.h`
- `<chrono>` in `util/time.h`
a5a8c4139c811e697b3c0b4d87737e04b60c53c8 ci, iwyu: Fix warnings in `src/kernel` and treat them as errors (Hennadii Stepanov)
Pull request description:
Now seems like a good time to update the includes in `src/kernel`.
ACKs for top commit:
maflcko:
review ACK a5a8c4139c811e697b3c0b4d87737e04b60c53c8 🍱
purpleKarrot:
ACK a5a8c4139c811e697b3c0b4d87737e04b60c53c8
sedited:
ACK a5a8c4139c811e697b3c0b4d87737e04b60c53c8
Tree-SHA512: ba401b27b03dee66d52d0b348972268e162506c4bafa40f408349173b68c40a11f20ca24f46c98945515e1d5c84f740d6e6784f7e4c799df46ab816cf5d11483
fa64d8424b8de49e219bffb842a33d484fb03212 refactor: Enforce readability-avoid-const-params-in-decls (MarcoFalke)
faf0c2d942c8de7868a3fd3afc7fc9ea700c91d4 refactor: Avoid copies by using const references or by move-construction (MarcoFalke)
Pull request description:
Top level `const` in declarations is problematic for many reasons:
* It is often a typo, where one wanted to denote a const reference. For example `bool PSBTInputSignedAndVerified(const PartiallySignedTransaction psbt, ...` is missing the `&`. This will create a redundant copy of the value.
* In constructors it prevents move construction.
* It can incorrectly imply some data is const, like in an imaginary example `std::span<int> Shuffle(const std::span<int>);`, where the `int`s are *not* const.
* The compiler ignores the `const` from the declaration in the implementation.
* It isn't used consistently anyway, not even on the same line.
Fix some issues by:
* Using a const reference to avoid a copy, where read-only of the value is intended. This is only done for values that may be expensive to copy.
* Using move-construction to avoid a copy
* Applying `readability-avoid-const-params-in-decls` via clang-tidy
ACKs for top commit:
l0rinc:
diff reACK fa64d8424b8de49e219bffb842a33d484fb03212
hebasto:
ACK fa64d8424b8de49e219bffb842a33d484fb03212, I have reviewed the code and it looks OK.
sedited:
ACK fa64d8424b8de49e219bffb842a33d484fb03212
Tree-SHA512: 293c000b4ebf8fdcc75259eb0283a2e4e7892c73facfb5c3182464d6cb6a868b7f4a6682d664426bf2edecd665cf839d790bef0bae43a8c3bf1ddfdd3d068d38
dd904298c13b14ef518e24fa63c6d0962f4a2de0 gui: Show an error message if the restored wallet name is empty (Ava Chow)
Pull request description:
The Restore Wallet dialog rejects wallet names that are empty, but was doing so silently. This is confusing, we should be presenting an error message to the user.
ACKs for top commit:
hebasto:
ACK dd904298c13b14ef518e24fa63c6d0962f4a2de0. Tested on Fedora 43.
Tree-SHA512: f4b60f32d1c2550dbce8613f25d29a92588b1ecfc8e8e5dac691a6bdb21a77508288a904539b68333d96bde5ebb993912253f4a293e4c583891f553d95762e77
d09a19fd41cb71a5d1c10297763e72bc551a7d3a test: add coverage for issue 34206 (Greg Sanders)
4c7cfd37ad9517334a0848e6778243ddef1843f4 wallet: remove erroneous-on-reorg Assume() (Greg Sanders)
Pull request description:
Resolves https://github.com/bitcoin/bitcoin/issues/34206
I'm not certain the test is worth keeping, but included it for now to show minimal example that crashes without fix. Can be removed.
ACKs for top commit:
bensig:
ACK d09a19fd41cb71a5d1c10297763e72bc551a7d3a
dergoegge:
utACK d09a19fd41cb71a5d1c10297763e72bc551a7d3a
Tree-SHA512: 7eac19e97be6db8e38af396c406066fdcec532332e685a38bb33f0a988701c7bd5a0967f51426737fd56972847b761a3d873495928ff66efa8512fb267a9622b
fac70ea8b5bb33d05a47c36f2c5f1d79f119315c fuzz: Exclude too expensive inputs in miniscript_string target (MarcoFalke)
fa907864786056258302a611bf4df0319138a71b iwyu: Fix includes for test/fuzz/util/descriptor module (MarcoFalke)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/issues/30498
Accepting "expensive" fuzz inputs which have no real use-case is problematic, because it prevents the fuzz engine from spending time on the next useful fuzz input.
For example this one will take several seconds (the flamegraph shows the time is spent in minscipt `NoDupCheck`):
```
curl -fLO '41bae50cff'
FUZZ=miniscript_string /usr/bin/time ./bld-cmake/bin/fuzz ./41bae50cffd1741150a1b330d02ab09f46ff8cd1
```
Inspecting the inputs shows that it has many sub frags, so rejecting based on `HasTooManySubFrag` should be sufficient.
ACKs for top commit:
darosior:
ACK fac70ea8b5bb33d05a47c36f2c5f1d79f119315c
brunoerg:
code review ACK fac70ea8b5bb33d05a47c36f2c5f1d79f119315c
dergoegge:
utACK fac70ea8b5bb33d05a47c36f2c5f1d79f119315c
Tree-SHA512: 7f1e0d9ce24d67ec63e5b7c2dd194efa51f38beb013564690afe0f920e5ff1980c85ce344828c0dc3f34b6851db7fe72a76b1a775c6d51c94fb91431834f453b
da56ef239b12786e3a177afda14352dda4a70bc6 clusterlin: minimize chunks (feature) (Pieter Wuille)
Pull request description:
Part of #30289.
This was split off from #34023, because it's not really an optimization but a feature. The feature existed pre-SFL, so this brings SFL to parity in terms of functionality with the old code.
The idea is that while optimality - as achieved by SFL before this PR - guarantees a linearization whose feerate diagram is optimal, it may be possible to split chunks into smaller equal-feerate parts. This is desirable because even though it doesn't change the diagram, it provides more flexibility for optimization (binpacking is easier when the pieces are smaller).
Thus, this PR introduces the stronger notion of "minimality": optimal chunks, which are also split into their smallest possible pieces. To accomplish that, an additional step in the SFL algorithm is added which aims to split chunks into minimal equal-feerate parts where possible, without introducing circular dependencies between them. It works based on the observation that if an (already otherwise optimal) chunk has a way of being split into two equal-feerate parts, and T is a given transaction in the chunk, then we can find the split in two steps:
* One time, pretend T has $\epsilon$ higher feerate than it really has. If a split exists with T in the top part, this will find it.
* The other time, pretend T has $\epsilon$ lower feerate than it really has. If a split exists with T in the bottom part, this will find it.
So we try both on each found optimal chunk. If neither works, the chunk is minimal. If one works, recurse into the split chunks to split them further.
ACKs for top commit:
instagibbs:
reACK da56ef239b
marcofleon:
crACK da56ef239b12786e3a177afda14352dda4a70bc6
Tree-SHA512: 2e94d6b78725f5f9470a939dedef46450b85c4e5e6f30cba0b038622ec2b417380747e8df923d1f303706602ab6d834350716df9678de144f857e3a8d163f6c2
fa3df5271232ee342c225da183be95dc47bde77f bench: Require semicolon after BENCHMARK(foo) (MarcoFalke)
fa8938f08c9a9da81a482bccb6bfe86f37a5a841 bench: Remove incorrect __LINE__ in BENCHMARK macro (MarcoFalke)
fa51a28a948dbab7109f660bf11dbfd389c839ed scripted-diff: Remove priority_level from BENCHMARK macro (MarcoFalke)
fa790c3eeaae1bb600ae59d013b170087ea5fb0e bench: Remove -priority-level= option (MarcoFalke)
Pull request description:
The option was added in https://github.com/bitcoin/bitcoin/pull/26158, when the project was using an autotools-based build system. However, in the meantime this option is unused:
* First, commit 27f11217ca63e0f8f78f14db139150052dcd9962 removed the option from one CI task
* Then https://github.com/bitcoin/bitcoin/pull/32310 removed the option from CMakeList.txt, because:
* they only run as a sanity check (fastest version)
* no one otherwise runs them, not even CI
* issues have been missed due to this
Finally, after commit 0ad4376a49fae6f705128b326ba92317cb8e0639, I don't see a single reason to keep this option, so remove it.
Also, there is a commit to turn a silent ignore of duplicate bench names into an error.
ACKs for top commit:
achow101:
ACK fa3df5271232ee342c225da183be95dc47bde77f
l0rinc:
ACK fa3df5271232ee342c225da183be95dc47bde77f
hebasto:
re-ACK fa3df5271232ee342c225da183be95dc47bde77f, only suggested changes since my recent [review](https://github.com/bitcoin/bitcoin/pull/34210#pullrequestreview-3652414135).
Tree-SHA512: 68a314bff551fa878196d5a615d41d71e1c8c504135e6fc555659aa9f0c8786957d49ba038448e933554a8bc54caea2ddd7d628042c5627bf3bf37628210f8fb
7b5d256af4a0f954a919604ed4346db3a814fb6d test: Add bitcoin-chainstate test for assumeutxo functionality (stringintech)
2bc32656498517fe58bd41dcbd0afd306d51d4b0 Fix `ChainstateManager::AddChainstate()` assertion crash (stringintech)
5f3d6bdb6659dba16941e6d6a05fd883d3f49a9d Add regtest support to bitcoin-chainstate tool (stringintech)
Pull request description:
This PR adds functional test coverage for the bitcoin-chainstate tool loading a datadir initialized with an assumeutxo snapshot.
The PR also includes:
- Fix for assertion crash in `ChainstateManager::AddChainstate()` when `prev_chainstate` has no initialized mempool (required for the test to pass)
- `-regtest` flag support for bitcoin-chainstate to enable the testing
This work started while experimenting with the bitcoin-chainstate tool and how the kernel API (#30595) behaved when loading a datadir containing assumeutxo data, during the time that PR was still under review. sedited suggested opening a PR to add this test coverage.
ACKs for top commit:
achow101:
ACK 7b5d256af4a0f954a919604ed4346db3a814fb6d
theStack:
Concept and code-review ACK 7b5d256af4a0f954a919604ed4346db3a814fb6d
sedited:
Re-ACK 7b5d256af4a0f954a919604ed4346db3a814fb6d
Tree-SHA512: 5d3b0050cf2d53144b5f65451c991d5e212117b4541ae1368ecf58fde5f3cca4f018aad6ae32257b9ebb1c28b926424fbcff496ba5487cdc4eb456cea6db8b24
792e2edf57ab31ae5c6f98acf33af8f67506630f p2p: first addr self-announcement in separate msg (0xb10c)
Pull request description:
This makes sure the initial address self-announcement a node sends to a peer happends in a separate P2P message. This has benefits for both inbound and outbound connections:
For inbound connections from a peer to us, previously, we might send the self-announcement along with our response to a GETADDR request. However, the self-announcement might replace an address from the GETADDR response. This isn't clean.
For outbound connections from us to a peer, previously, it could have happend that we send the self-announcement along with other addresses. Since shortly after connection open, the peer might only have one rate-limiting token for us, and the addresses are shuffeld on arrival, it's possible that the self-announcement gets rate-limited. However, note that these rate-limitings seem to be rare in practice.
This is inspired by and based on https://github.com/bitcoin/bitcoin/pull/33699#issuecomment-3462287763. The discussion there should be helpful for reviewers.
ACKs for top commit:
bensig:
ACK 792e2edf57ab31ae5c6f98acf33af8f67506630f
achow101:
ACK 792e2edf57ab31ae5c6f98acf33af8f67506630f
fjahr:
Code review ACK 792e2edf57ab31ae5c6f98acf33af8f67506630f
frankomosh:
Code Review ACK [792e2ed](792e2edf57)
Tree-SHA512: e3d39b1e3ae6208b54df4b36c624a32d70a442e01681f49e0c8a65076a818b5bf203c2e51011dc32edbbe3637b3c0b5f18de26e3461c288aa3806646a209a260
de4242f47476769d0a7f3e79e8297ed2dd60d9a4 refactor: Use reference for chain_start in HeadersSyncState (Daniela Brozzoni)
e37555e5401f9fca39ada0bd153e46b2c7ebd095 refactor: Use initializer list in CompressedHeader (Daniela Brozzoni)
0488bdfefe92b2c9a924be9244c91fe472462aab refactor: Remove unused parameter in ReportHeadersPresync (Daniela Brozzoni)
256246a9fa5b05141c93aeeb359394b9c7a80e49 refactor: Remove redundant parameter from CheckHeadersPoW (Daniela Brozzoni)
ca0243e3a6d77d2b218749f1ba113b81444e3f4a refactor: Remove useless CBlock::GetBlockHeader (Pieter Wuille)
45686522224598bed9923e60daad109094d7bc29 refactor: Use std::span in HasValidProofOfWork (Daniela Brozzoni)
4066bfe561a45f61a3c9bf24bec7f600ddcc7467 refactor: Compute work from headers without CBlockIndex (Daniela Brozzoni)
0bf6139e194f355d121bb2aea74715d1c4099598 p2p: Avoid an IsAncestorOfBestHeaderOrTip call (Pieter Wuille)
Pull request description:
This is a partial* revival of #25968
It contains a list of most-unrelated simplifications and optimizations to the code merged in #25717:
- Avoid an IsAncestorOfBestHeaderOrTip call: Just don't call this function when it won't have any effect.
- Compute work from headers without CBlockIndex: Avoid the need to construct a CBlockIndex object just to compute work for a header, when its nBits value suffices for that. Also use some Spans where possible.
- Remove useless CBlock::GetBlockHeader: There is no need for a function to convert a CBlock to a CBlockHeader, as it's a child class of it.
It also contains the following code cleanups, which were suggested by reviewers in #25968:
- Remove redundant parameter from CheckHeadersPoW: No need to pass consensusParams, as CheckHeadersPow already has access to m_chainparams.GetConsensus()
- Remove unused parameter in ReportHeadersPresync
- Use initializer list in CompressedHeader, also make GetFullHeader const
- Use reference for chain_start in HeadersSyncState: chain_start can never be null, so it's better to pass it as a reference rather than a raw pointer
*I decided to leave out three commits that were in #25968 (4e7ac7b94d04e056e9994ed1c8273c52b7b23931, ab52fb4e95aa2732d1a1391331ea01362e035984, 7f1cf440ca1a9c86085716745ca64d3ac26957c0), since they're a bit more involved, and I'm a new contributor. If this PR gets merged, I'll comment under #25968 to note that these three commits are still up for grabs :)
ACKs for top commit:
l0rinc:
ACK de4242f47476769d0a7f3e79e8297ed2dd60d9a4
polespinasa:
re-ACK de4242f47476769d0a7f3e79e8297ed2dd60d9a4
sipa:
ACK de4242f47476769d0a7f3e79e8297ed2dd60d9a4
achow101:
ACK de4242f47476769d0a7f3e79e8297ed2dd60d9a4
hodlinator:
re-ACK de4242f47476769d0a7f3e79e8297ed2dd60d9a4
Tree-SHA512: 1de4f3ce0854a196712505f2b52ccb985856f5133769552bf37375225ea8664a3a7a6a9578c4fd461e935cd94a7cbbb08f15751a1da7651f8962c866146d9d4b
7fc465ece88284c79728cacbc1d4c2fe63c60e1a doc: fix incorrect description of `PackageMempoolChecks` (ismaelsadeeq)
1412b779ad0a7d98396e45676ba75bd8e90446e0 refactor: execute `PackageMempoolChecks` during package rbf only (ismaelsadeeq)
Pull request description:
This is a simple PR that fixes the incorrect description of what is done in `PackageMempoolChecks`
> // Enforce package mempool ancestor/descendant limits (distinct from individual
> // ancestor/descendant limits done in PreChecks) and run Package RBF checks.
After cluster mempool, we no longer enforce ancestor/descendant limits in both `PreChecks` and `PackageMempoolChecks`; instead, cluster limit is enforced in `PackageMempoolChecks`.
This PR fixes the incorrect comment by;
- Making it clear why it is necessary to have two calls of `CheckMempoolPolicyLimts` in both `PackageMempoolChecks` and after in `AcceptMultipleTransactionsInternal` by executing `PackageMempoolChecks` only during package RBF only. No need to jump into the next subroutine when there is no conflict.
- Renames `PackageMempoolChecks` to `PackageRBFChecks`; the method name is self-explanatory now, hence no need for a description comment.
ACKs for top commit:
yashbhutwala:
ACK 7fc465ece88284c79728cacbc1d4c2fe63c60e1a
instagibbs:
ACK 7fc465ece88284c79728cacbc1d4c2fe63c60e1a
glozow:
utACK 7fc465ece88284c79728cacbc1d4c2fe63c60e1a
Tree-SHA512: 38655f9d05be54cadd224fad376da9871a85efc7801306b58d4f51aee658036cdce2ab406143a3439d7211fc9bb0fc86bd330852e8926d79660944872b8fae8d
fa1d17d56c83d6ad89c1f688824ec0dc1c294012 refactor: Use uint64_t over size_t for serialize corruption check in fees.dat (MarcoFalke)
Pull request description:
Serialization should not behave differently on different architectures. See also the related commit 3789215f73466606eb111714f596a2a5e9bb1933.
However, on fees.dat file corruption, 32-bit builds may run into an unsigned integer overflow and report the wrong corruption reason, or may even silently continue after the corruption.
This is a bit hard to reproduce, because 32-bit platforms are rare and most of them don't support running the unsigned integer overflow sanitizer. So the possible options to reproduce are:
* Run on armhf and manually annotate the code to detect the overflow
* Run on i386 with the integer sanitizer (possibly via `podman run -it --rm --platform linux/i386 'debian:trixie'`)
* Run the integer sanitizer on any 64-bit platform and manually replace type in the affected line by `uint32_t`
Afterwards, the steps to reproduce are:
```
export DEBIAN_FRONTEND=noninteractive && apt update && apt install curl wget htop git vim ccache -y && git clone https://github.com/bitcoin/bitcoin.git --depth=1 ./b-c && cd b-c && apt install build-essential cmake pkg-config python3-zmq libzmq3-dev libevent-dev libboost-dev libsqlite3-dev systemtap-sdt-dev libcapnp-dev capnproto libqrencode-dev qt6-tools-dev qt6-l10n-tools qt6-base-dev clang llvm libc++-dev libc++abi-dev -y
cmake -B ./bld-cmake -DCMAKE_BUILD_TYPE=Debug -DCMAKE_C_COMPILER='clang' -DCMAKE_CXX_COMPILER='clang++' -DSANITIZERS=undefined,integer,float-divide-by-zero --preset=dev-mode
cmake --build ./bld-cmake --parallel $(nproc)
curl -fLO '6074731370'
UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" FUZZ=policy_estimator_io ./bld-cmake/bin/fuzz ./607473137013139e3676e30ec4b29639e673fa9b
```
The output will be something like:
```
/b-c/src/policy/fees/block_policy_estimator.cpp:448:25: runtime error: unsigned integer overflow: 346685954 * 219 cannot be represented in type 'unsigned int'
#0 0x5b0b1bbe in TxConfirmStats::Read(AutoFile&, unsigned int) /b-c/bld-cmake/src/./policy/fees/block_policy_estimator.cpp:448:25
#1 0x5b0b7d3f in CBlockPolicyEstimator::Read(AutoFile&) /b-c/bld-cmake/src/./policy/fees/block_policy_estimator.cpp:1037:29
#2 0x592a9783 in policy_estimator_io_fuzz_target(std::span<unsigned char const, 4294967295u>) /b-c/bld-cmake/src/test/fuzz/./test/fuzz/policy_estimator_io.cpp:32:32
#3 0x5896ba8e in void std::__invoke_impl<void, void (*&)(std::span<unsigned char const, 4294967295u>), std::span<unsigned char const, 4294967295u>>(std::__invoke_other, void (*&)(std::span<unsigned char const, 4294967295u>), std::span<unsigned char const, 4294967295u>&&) /usr/lib/gcc/i686-linux-gnu/14/../../../../include/c++/14/bits/invoke.h:61:14
#4 0x5896b8eb in std::enable_if<is_invocable_r_v<void, void (*&)(std::span<unsigned char const, 4294967295u>), std::span<unsigned char const, 4294967295u>>, void>::type std::__invoke_r<void, void (*&)(std::span<unsigned char const, 4294967295u>), std::span<unsigned char const, 4294967295u>>(void (*&)(std::span<unsigned char const, 4294967295u>), std::span<unsigned char const, 4294967295u>&&) /usr/lib/gcc/i686-linux-gnu/14/../../../../include/c++/14/bits/invoke.h:111:2
#5 0x5896b44b in std::_Function_handler<void (std::span<unsigned char const, 4294967295u>), void (*)(std::span<unsigned char const, 4294967295u>)>::_M_invoke(std::_Any_data const&, std::span<unsigned char const, 4294967295u>&&) /usr/lib/gcc/i686-linux-gnu/14/../../../../include/c++/14/bits/std_function.h:290:9
#6 0x59845c95 in std::function<void (std::span<unsigned char const, 4294967295u>)>::operator()(std::span<unsigned char const, 4294967295u>) const /usr/lib/gcc/i686-linux-gnu/14/../../../../include/c++/14/bits/std_function.h:591:9
#7 0x5983a0da in test_one_input(std::span<unsigned char const, 4294967295u>) /b-c/bld-cmake/src/test/fuzz/util/./test/fuzz/fuzz.cpp:88:5
#8 0x5983cb80 in main /b-c/bld-cmake/src/test/fuzz/util/./test/fuzz/fuzz.cpp:271:13
#9 0xf75aecc2 (/lib/i386-linux-gnu/libc.so.6+0x24cc2) (BuildId: 2dc5f2945fad35c1b07d1a5a32520b3c41afaa75)
#10 0xf75aed87 in __libc_start_main (/lib/i386-linux-gnu/libc.so.6+0x24d87) (BuildId: 2dc5f2945fad35c1b07d1a5a32520b3c41afaa75)
#11 0x58932db6 in _start (/b-c/bld-cmake/bin/fuzz+0x235ddb6) (BuildId: 7d8d83a77923f14e99c0de64acbc5f5bfc2cce9b)
SUMMARY: UndefinedBehaviorSanitizer: unsigned-integer-overflow /b-c/src/policy/fees/block_policy_estimator.cpp:448:25
```
Note: This is marked a "refactor", because the code change does not affect 64-bit builds, and on the still remaining rare 32-bit builds today it is extremely unlikely to happen in production.
ACKs for top commit:
bensig:
ACK fa1d17d56c83d6ad89c1f688824ec0dc1c294012
ismaelsadeeq:
utACK fa1d17d56c83d6ad89c1f688824ec0dc1c294012
luke-jr:
Also, utACK fa1d17d56c83d6ad89c1f688824ec0dc1c294012 as an improvement.
Tree-SHA512: 696bf8e0dbe4777c84cb90e313c7f8f9ee90d4b3e64de1222f8472b2d9d0f3a0f6f027fda743dd6ca8c6aab94f404db7a65bb562a76000d9c33a8a39de28d8d4
2f5b1c5f80590ffa6b5a5bcfb21fddb1dc22e852 psbt: Fix `PSBTInputSignedAndVerified` bounds `assert` (Lőrinc)
Pull request description:
This PR fixes an off-by-one in a debug assertion in `PSBTInputSignedAndVerified`.
The function indexes `psbt.inputs[input_index]`, so the assertion must not allow indexing at `psbt.inputs.size()`.
Found during review: https://github.com/bitcoin/bitcoin/pull/31650#discussion_r2685892867
ACKs for top commit:
optout21:
utACK 2f5b1c5f80590ffa6b5a5bcfb21fddb1dc22e852
maflcko:
lgtm ACK 2f5b1c5f80590ffa6b5a5bcfb21fddb1dc22e852
achow101:
ACK 2f5b1c5f80590ffa6b5a5bcfb21fddb1dc22e852
Tree-SHA512: cec613a9a38358d5caa243197d746baa129aebfd7fe697689f28e652f94c4683873c4676d5eb2eb909ea19de5e5f6e54ecc5f3162384a48f6f38a59273667689
fa8d56f9f092fceab7dfb10533c4187e1b5fabfe fuzz: Reject too large descriptor leaf sizes in scriptpubkeyman target (MarcoFalke)
fabac1b3950e4bc9716f9b3c17b8f02952d6b974 fuzz: Reject some more "expensive" descriptors in the scriptpubkeyman target (MarcoFalke)
333333356f431d8ef318f685860d25ff99d4b457 fuzz: [refactor] Use std::span over FuzzBufferType in descriptor utils (MarcoFalke)
Pull request description:
Accepting "expensive" fuzz inputs which have no real use-case is problematic, because it prevents the fuzz engine from spending time on the next useful fuzz input.
Also, this may lead to problems, where the fuzz target can not be run at all on some platforms. See https://github.com/bitcoin/bitcoin/issues/34110.
Fixes https://github.com/bitcoin/bitcoin/issues/34110 by rejecting those useless and expensive inputs (via the third commit)
Can be tested by running the input and checking the time before and after the changes here:
```
curl -fLO '1cf91e0c6b'
FUZZ=scriptpubkeyman time ./bld-cmake/bin/fuzz ./1cf91e0c6bfff9dafcd4db5b0ba36b1e906f4cf5
```
Also, the second commit fixes https://github.com/bitcoin/bitcoin/issues/31066.
ACKs for top commit:
brunoerg:
code review ACK fa8d56f9f092fceab7dfb10533c4187e1b5fabfe
marcofleon:
ACK fa8d56f9f092fceab7dfb10533c4187e1b5fabfe
sipa:
ACK fa8d56f9f092fceab7dfb10533c4187e1b5fabfe
Tree-SHA512: e683cb89c3047358add438508c173f1cf647827bcadc3564ad42c757e4c99b8e9b777213fd38ebeb46f4c89a72363e0642f47435e20df3960eaeb5b8257dbd32
6bb66fcccb5b65eada89578737ecada6f017fc5a test: Improve code coverage for pubkey checks (billymcbip)
Pull request description:
Cover these branches in `IsCompressedOrUncompressedPubKey` and `IsCompressedPubKey`:
- `Non-canonical public key: invalid length for uncompressed key`
- `Non-canonical public key: invalid length for compressed key`
- `Non-canonical public key: invalid prefix for compressed key`
See the missed branches here: https://maflcko.github.io/b-c-cov/total.coverage/src/script/interpreter.cpp.gcov.html
`script_tests` succeed on my end.
ACKs for top commit:
maflcko:
ACK 6bb66fcccb5b65eada89578737ecada6f017fc5a 🌑
rkrux:
code review ACK 6bb66fcccb5b65eada89578737ecada6f017fc5a
darosior:
ACK 6bb66fcccb5b65eada89578737ecada6f017fc5a
Tree-SHA512: f9b8acdc8bbe95559d594e74ed721d27be715754717b1557796168a6e81ce56d5bc20c40da4c0906ef9e1edcd88f202f000e34d8331d9be8d2694067a98996c6
997e7b4d7cf7c4622938798423447375383184c0 init: Fix non-zero code on interrupt (sedited)
Pull request description:
Reported by dergoegge on irc.
An interrupt does not create a failure exit code during normal operation. This should also be the case when interrupt is triggered during initialization. However a failure exit code is currently returned if an interrupt occurs during init. Fix this by making `AppInitMain` return true instead of false on interrupt, which further up the call stack currently sets the `EXIT_FAILURE` code. Also add a check for the interrupt condition during GUI startup. Returning `EXIT_SUCCESS` seems to be the usual behaviour for daemons, see the discussion on IRC for this: https://www.erisian.com.au/bitcoin-core-dev/log-2026-01-08.html#l-146 .
Best reviewed with `--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space`.
ACKs for top commit:
maflcko:
review ACK 997e7b4d7cf7c4622938798423447375383184c0 🔺
janb84:
ACK 997e7b4d7cf7c4622938798423447375383184c0
dergoegge:
utACK 997e7b4d7cf7c4622938798423447375383184c0
Tree-SHA512: c9542e95d9312567e029426a329144b5bc638d8ebc9c966e0246c1bb728d40f56ca425b00c446f5d238067e629c2337d0fe78bcc5a8760424d2ec38a5578e115
- We no longer enforce ancestor/descendant count limit
in both PreChecks and PackageMempoolChecks.
- This commit fixes the incorrect comment by just renaming
`PackageMempoolChecks` to `PackageRBFChecks`
- The method name is self explanatory now; hence no need
for a description comment.
48f57bb35bbdbce509b8ef195de69e2a61a2511e mining: add new getCoinbaseTx() returning a struct (Sjors Provoost)
d59b4cdb5772917ee13e48552d51662160104b62 mining: rename getCoinbaseTx() to ..RawTx() (Sjors Provoost)
Pull request description:
The first commit renames `getCoinbaseTx()` to `getCoinbaseRawTx()` to reflect that it returns a serialised transaction. This does not impact IPC clients, because they do not use the function name.
The second commit then introduces a replacement `getCoinbase()` that provides a struct with everything clients need to construct a coinbase. This avoids clients having to parse and manipulate our dummy transaction.
Deprecate but don't remove `getCoinbaseRawTx()`, `getCoinbaseCommitment()` and `getWitnessCommitmentIndex()`.
After this change we can drop these deprecated methods, which in turn would allow us to clear the dummy transaction from the `getBlock()` result. But that is left for a followup to keep this PR focussed. See https://github.com/Sjors/bitcoin/pull/106 for an approach.
Expand the `interface_ipc.py` functional test to document its usage.
Can be tested using:
- https://github.com/stratum-mining/sv2-tp/pull/59
ACKs for top commit:
ryanofsky:
Code review ACK 48f57bb35bbdbce509b8ef195de69e2a61a2511e. Just rebased and addressed comments and dropped coinbase tx "template" suffix, which is a nice change
ismaelsadeeq:
code review ACK 48f57bb35bbdbce509b8ef195de69e2a61a2511e
vasild:
ACK 48f57bb35bbdbce509b8ef195de69e2a61a2511e
Tree-SHA512: c4f1d752777fb3086a1a0b7b8b06e4205dbe2f3adb41f218855ad1dee952adccc263cf82acd3bf9300cc83c2c64cebd2b27f66a69beee32d325b9a85e3643b0d
This makes the code more consistent.
Also, use "using BenchFunction = ..." while touching the header.
Also, fixup the whitespace after and earlier scripted-diff.
Duplicate benchmarks with the same name are not supported. Expanding the
name with __LINE__ is confusing and brittle, because it makes duplication
bugs silent.
Fix this twofold:
* By enforcing unique benchmarks at compile-time and link-time. For
example, a link failure may now look like:
"mold: error: duplicate symbol: bench_runner_AddrManAdd"
* By enforcing unique benchmarks at run-time. This should never happen,
due to the build-failure, but a failure may look like:
"Assertion `benchmarks().try_emplace(std::move(name), std::move(func)).second' failed."
The Restore Wallet dialog rejects wallet names that are empty, but was
doing so silently. This is confusing, we should be presenting an error
message to the user.
After the normal optimization process finishes, and finds an optimal
spanning forest, run a second process (while computation budget remains)
to split chunks into minimal equal-feerate chunks.
89372213048adf37a47427112a1ff836ee84c50e doc: add release notes for 29415 (Vasil Dimov)
582016fa5f013817db650bbba0a40d9195c18e2e test: add unit test for the private broadcast storage (Vasil Dimov)
e74d54e04896a86cad4e4b1bd9641afcc3a026c2 test: add functional test for private broadcast (Vasil Dimov)
818b780a05db126dcfe7efe12c46c84b5cfc3de6 rpc: use private broadcast from sendrawtransaction RPC if -privatebroadcast is ON (Vasil Dimov)
eab595f9cf13f7cb1d25a0db51409535cfe053b1 net_processing: retry private broadcast (Vasil Dimov)
37b79f9c39db5a4a61d360a6a29c8853bb5c7ac0 net_processing: stop private broadcast of a transaction after round-trip (Vasil Dimov)
2de53eee742da11b0e3f6fc44c39f2b5b5929da1 net_processing: handle ConnectionType::PRIVATE_BROADCAST connections (Vasil Dimov)
30a9853ad35365af8545e8e766d75cf398968480 net_processing: move a debug check in VERACK processing earlier (Vasil Dimov)
d1092e5d48ce67bd517068550c78bfcab062a554 net_processing: modernize PushNodeVersion() (Vasil Dimov)
9937a12a2fd5a0033f37f4dda5d75bfc5f15c3b6 net_processing: move the debug log about receiving VERSION earlier (Vasil Dimov)
a098f37b9e240291077a7f440e9f57e61f30e158 net_processing: reorder the code that handles the VERSION message (Vasil Dimov)
679ce3a0b8df6e8cab07965301382d2036ef2368 net_processing: store transactions for private broadcast in PeerManager (Vasil Dimov)
a3faa6f944a672faccac5dd201c8d33a638d9091 node: extend node::TxBroadcast with a 3rd option (Vasil Dimov)
95c051e21051bd469fda659fe7c495d5e264d221 net_processing: rename RelayTransaction() to better describe what it does (Vasil Dimov)
bb49d26032c57714c62a4b31ff1fdd969751683f net: implement opening PRIVATE_BROADCAST connections (Vasil Dimov)
01dad4efe2b38b7a71c96b6222147f395e0c11d9 net: introduce a new connection type for private broadcast (Vasil Dimov)
94aaa5d31b6ff1d0122319fc70e70a7e27e1a0ba init: introduce a new option to enable/disable private broadcast (Vasil Dimov)
d6ee490e0a9a81b69a4751087918303163ba8869 log: introduce a new category for private broadcast (Vasil Dimov)
Pull request description:
_Parts of this PR are isolated in independent smaller PRs to ease review:_
* [x] _https://github.com/bitcoin/bitcoin/pull/29420_
* [x] _https://github.com/bitcoin/bitcoin/pull/33454_
* [x] _https://github.com/bitcoin/bitcoin/pull/33567_
* [x] _https://github.com/bitcoin/bitcoin/pull/33793_
---
To improve privacy, broadcast locally submitted transactions (from the `sendrawtransaction` RPC) to the P2P network only via Tor or I2P short-lived connections, or to IPv4/IPv6 peers but through the Tor network.
* Introduce a new connection type for private broadcast of transactions with the following properties:
* started whenever there are local transactions to be sent
* opened to Tor or I2P peers or IPv4/IPv6 via the Tor proxy
* opened regardless of max connections limits
* after handshake is completed one local transaction is pushed to the peer, `PING` is sent and after receiving `PONG` the connection is closed
* ignore all incoming messages after handshake is completed (except `PONG`)
* Broadcast transactions submitted via `sendrawtransaction` using this new mechanism, to a few peers. Keep doing this until we receive back this transaction from one of our ordinary peers (this takes about 1 second on mainnet).
* The transaction is stored in peerman and does not enter the mempool.
* Once we get an `INV` from one of our ordinary peers, then the normal flow executes: we request the transaction with `GETDATA`, receive it with a `TX` message, put it in our mempool and broadcast it to all our existent connections (as if we see it for the first time).
* After we receive the full transaction as a `TX` message, in reply to our `GETDATA` request, only then consider the transaction has propagated through the network and remove it from the storage in peerman, ending the private broadcast attempts.
The messages exchange should look like this:
```
tx-sender >--- connect -------> tx-recipient
tx-sender >--- VERSION -------> tx-recipient (dummy VERSION with no revealing data)
tx-sender <--- VERSION -------< tx-recipient
tx-sender <--- WTXIDRELAY ----< tx-recipient (maybe)
tx-sender <--- SENDADDRV2 ----< tx-recipient (maybe)
tx-sender <--- SENDTXRCNCL ---< tx-recipient (maybe)
tx-sender <--- VERACK --------< tx-recipient
tx-sender >--- VERACK --------> tx-recipient
tx-sender >--- INV/TX --------> tx-recipient
tx-sender <--- GETDATA/TX ----< tx-recipient
tx-sender >--- TX ------------> tx-recipient
tx-sender >--- PING ----------> tx-recipient
tx-sender <--- PONG ----------< tx-recipient
tx-sender disconnects
```
Whenever a new transaction is received from `sendrawtransaction` RPC, the node will send it to a few (`NUM_PRIVATE_BROADCAST_PER_TX`) recipients right away. If after some time we still have not heard anything about the transaction from the network, then it will be sent to 1 more peer (see `PeerManagerImpl::ReattemptPrivateBroadcast()`).
A few considerations:
* The short-lived private broadcast connections are very cheap and fast wrt network traffic. It is expected that some of those peers could blackhole the transaction. Just one honest/proper peer is enough for successful propagation.
* The peers that receive the transaction could deduce that this is initial transaction broadcast from the transaction originator. This is ok, they can't identify the sender.
---
<details>
<summary>How to test this?</summary>
Thank you, @stratospher and @andrewtoth!
Start `bitcoind` with `-privatebroadcast=1 -debug=privatebroadcast`.
Create a wallet and get a new address, go to the Signet faucet and request some coins to that address:
```bash
build/bin/bitcoin-cli -chain="signet" createwallet test
build/bin/bitcoin-cli -chain="signet" getnewaddress
```
Get a new address for the test transaction recipient:
```bash
build/bin/bitcoin-cli -chain="signet" loadwallet test
new_address=$(build/bin/bitcoin-cli -chain="signet" getnewaddress)
```
Create the transaction:
```bash
# Option 1: `createrawtransaction` and `signrawtransactionwithwallet`:
txid=$(build/bin/bitcoin-cli -chain="signet" listunspent | jq -r '.[0] | .txid')
vout=$(build/bin/bitcoin-cli -chain="signet" listunspent | jq -r '.[0] | .vout')
echo "txid: $txid"
echo "vout: $vout"
tx=$(build/bin/bitcoin-cli -chain="signet" createrawtransaction "[{\"txid\": \"$txid\", \"vout\": $vout}]" "[{\"$new_address\": 0.00001000}]" 0 false)
echo "tx: $tx"
signed_tx=$(build/bin/bitcoin-cli -chain="signet" signrawtransactionwithwallet "$tx" | jq -r '.hex')
echo "signed_tx: $signed_tx"
# OR Option 2: `walletcreatefundedpsbt` and `walletprocesspsbt`:
# This makes it not have to worry about inputs and also automatically sends back change to the wallet.
# Start `bitcoind` with `-fallbackfee=0.00003000` for instance for 3 sat/vbyte fee.
psbt=$(build/bin/bitcoin-cli -chain="signet" walletcreatefundedpsbt "[]" "[{\"$new_address\": 0.00001000}]" | jq -r '.psbt')
echo "psbt: $psbt"
signed_tx=$(build/bin/bitcoin-cli -chain="signet" walletprocesspsbt "$psbt" | jq -r '.hex')
echo "signed_tx: $signed_tx"
```
Finally, send the transaction:
```bash
raw_tx=$(build/bin/bitcoin-cli -chain="signet" sendrawtransaction "$signed_tx")
echo "raw_tx: $raw_tx"
```
</details>
---
<details>
<summary>High-level explanation of the commits</summary>
* New logging category and config option to enable private broadcast
* `log: introduce a new category for private broadcast`
* `init: introduce a new option to enable/disable private broadcast`
* Implement the private broadcast connection handling on the `CConnman` side:
* `net: introduce a new connection type for private broadcast`
* `net: implement opening PRIVATE_BROADCAST connections`
* Prepare `BroadcastTransaction()` for private broadcast requests:
* `net_processing: rename RelayTransaction to better describe what it does`
* `node: extend node::TxBroadcast with a 3rd option`
* `net_processing: store transactions for private broadcast in PeerManager`
* Implement the private broadcast connection handling on the `PeerManager` side:
* `net_processing: reorder the code that handles the VERSION message`
* `net_processing: move the debug log about receiving VERSION earlier`
* `net_processing: modernize PushNodeVersion()`
* `net_processing: move a debug check in VERACK processing earlier`
* `net_processing: handle ConnectionType::PRIVATE_BROADCAST connections`
* `net_processing: stop private broadcast of a transaction after round-trip`
* `net_processing: retry private broadcast`
* Engage the new functionality from `sendrawtransaction`:
* `rpc: use private broadcast from sendrawtransaction RPC if -privatebroadcast is ON`
* New tests:
* `test: add functional test for private broadcast`
* `test: add unit test for the private broadcast storage`
</details>
---
**This PR would resolve the following issues:**
https://github.com/bitcoin/bitcoin/issues/3828 Clients leak IPs if they are recipients of a transaction
https://github.com/bitcoin/bitcoin/issues/14692 Can't configure bitocoind to only send tx via Tor but receive clearnet transactions
https://github.com/bitcoin/bitcoin/issues/19042 Tor-only transaction broadcast onlynet=onion alternative
https://github.com/bitcoin/bitcoin/issues/24557 Option for receive events with all networks, but send transactions and/or blocks only with anonymous network[s]?
https://github.com/bitcoin/bitcoin/issues/25450 Ability to broadcast wallet transactions only via dedicated oneshot Tor connections
https://github.com/bitcoin/bitcoin/issues/32235 Tor: TX circuit isolation
**Issues that are related, but (maybe?) not to be resolved by this PR:**
https://github.com/bitcoin/bitcoin/issues/21876 Broadcast a transaction to specific nodes
https://github.com/bitcoin/bitcoin/issues/28636 new RPC: sendrawtransactiontopeer
---
Further extensions:
* Have the wallet do the private broadcast as well, https://github.com/bitcoin/bitcoin/issues/11887 would have to be resolved.
* Have the `submitpackage` RPC do the private broadcast as well, [draft diff in the comment below](https://github.com/bitcoin/bitcoin/pull/29415#pullrequestreview-2972293733), thanks ismaelsadeeq!
* Add some stats via RPC, so that the user can better monitor what is going on during and after the broadcast. Currently this can be done via the debug log, but that is not convenient.
* Make the private broadcast storage, currently in peerman, persistent over node restarts.
* Add (optional) random delay before starting to broadcast the transaction in order to avoid correlating unrelated transactions based on the time when they were broadcast. Suggested independently of this PR [here](https://github.com/bitcoin/bitcoin/issues/30471).
* Consider periodically sending transactions that did not originate from the node as decoy, discussed [here](https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2035414972).
* Consider waiting for peer's FEEFILTER message and if the transaction that was sent to the peer is below that threshold, then assume the peer is going to drop it. Then use this knowledge to retry more aggressively with another peer, instead of the current 10 min. See [comment below](https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3258611648).
* It may make sense to be able to override the default policy -- eg so submitrawtransaction can go straight to the mempool and relay, even if txs are normally privately relayed. See [comment below](https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3427086681).
* As a side effect we have a new metric available - the time it takes for a transaction to reach a random node in the network (from the point of view of the private broadcast recipient the tx originator is a random node somewhere in the network). This can be useful for monitoring, unrelated to privacy characteristics of this feature.
---
_A previous incarnation of this can be found at https://github.com/bitcoin/bitcoin/pull/27509. It puts the transaction in the mempool and (tries to) hide it from the outside observers. This turned out to be too error prone or maybe even impossible._
ACKs for top commit:
l0rinc:
code review diff ACK 89372213048adf37a47427112a1ff836ee84c50e
andrewtoth:
ACK 89372213048adf37a47427112a1ff836ee84c50e
pinheadmz:
ACK 89372213048adf37a47427112a1ff836ee84c50e
w0xlt:
ACK 8937221304 with nit https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2654849875
mzumsande:
re-ACK 89372213048adf37a47427112a1ff836ee84c50e
Tree-SHA512: d51dadc865c2eb080c903cbe2f669e69a967e5f9fc64e9a20a68f39a67bf0db6ac2ad682af7fa24ef9f0942a41c89959341a16ba7b616475e1c5ab8e563b9b96
- No need to jump into the next subroutine when there is no conflict.
- This makes it clear why it is necessary to have two calls of
CheckMempoolPolicyLimts in both PackageMempoolChecks and after in
AcceptMultipleTransactionsInternal, there is a possibilty that we
we want to accept multiple transaction but they are not conflicting
with any in-mempool transaction, in that case also we want to check
that they do not bust the cluster limits.
2cade5d5d17010cd89855b26da350d6e54683805 [miniminer] stop assuming ancestor fees >= self fees (glozow)
Pull request description:
These assertions exist to detect double-deducting values when we update descendants. However, negative fees are possible with `prioritisetransaction` so it doesn't make sense to check this.
Leave the check for sizes because those are never negative.
Fixes#34234
ACKs for top commit:
instagibbs:
ACK 2cade5d5d17010cd89855b26da350d6e54683805
dergoegge:
utACK 2cade5d5d17010cd89855b26da350d6e54683805
Tree-SHA512: 935bbc8bd9a0d508eea43bb49aa43c22735e3f2c1012598f6843e229c13b76e44f9fd3eb8b61c437fa0b32353b4e7b15afa3e31002bdfa382d3d711d16419fde
An interrupt does not create a failure exit code during normal
operation. This should also be the case when interrupt is triggered
during initialization. However a failure exit code is currently returned
if an interrupt occurs during init. Fix this by making `AppInitMain` return
true instead of false, which further up the call stack sets the
`EXIT_FAILURE` code. Also add a check for the interrupt condition during
GUI startup.
301d9eea66cadffe2872776b8a0e9b72ec90b9a2 qt: Remove "Starting Block" from Peer Detail. Following Deprecation in `bitcoin#34197` (WakeTrainDev)
Pull request description:
the `startingheight` rpc field got deprecated in https://github.com/bitcoin/bitcoin/pull/34197
this pr removes it from peer detail
ACKs for top commit:
maflcko:
review lgtm ACK 301d9eea66cadffe2872776b8a0e9b72ec90b9a2
theStack:
ACK 301d9eea66cadffe2872776b8a0e9b72ec90b9a2
hebasto:
ACK 301d9eea66cadffe2872776b8a0e9b72ec90b9a2, I verified `forms/debugwindow.ui` using Qt Designer.
Tree-SHA512: b870b4cff8ead073a17d171c01c46fc7e750c0343b4578ffb63abc8f40b33abdf08beb6733fead5307ef5d48b078b60d29ac0e0e41190a98f50f92154f0878cf
They are exactly the same, but the descriptor utils should not prescribe
to use the FuzzBufferType. Using a dedicated type for them clarifies
that the utils are not tied to FuzzBufferType.
Also, while touching the lines, use `const` only where it is meaningful.
f78f6f1dc8e16d5a8a23749e77bc3bf17c91ae42 wallettool: do not use fs::remove_all in createfromdump cleanup (Ava Chow)
Pull request description:
As pointed out in https://github.com/bitcoin/bitcoin/pull/34156#issuecomment-3716728670, it is possible for `createfromdump` to also accidentally delete the entire wallets directory if the wallet name is the empty string and the dumpfile contains a checksum error.
This is also fixed by removing the files created by only removing the directory for named wallets, and avoiding the use of `fs::remove_all`.
ACKs for top commit:
waketraindev:
lgtm ACK f78f6f1dc8e16d5a8a23749e77bc3bf17c91ae42
polespinasa:
code review and tACK f78f6f1dc8e16d5a8a23749e77bc3bf17c91ae42
rkrux:
Code review and tACK f78f6f1dc8e16d5a8a23749e77bc3bf17c91ae42
willcl-ark:
ACK f78f6f1dc8e16d5a8a23749e77bc3bf17c91ae42
pablomartin4btc:
ACK f78f6f1dc8e16d5a8a23749e77bc3bf17c91ae42
Tree-SHA512: ff1e7668131ec3632c67d990c99e8fddff28605e7e553c7e20695e61017c88476c3636e22f2007e763a00d527e80e4d1d3d45409f6678d28729b8397430bfe7a
b7c34d08dd9549a95cffc6ec1ffa4bb4f81e35eb test: coverage for migration failure when last sync is beyond prune height (furszy)
82caa8193a3e36f248dcc949e0cd41def191efac wallet: migration, fix watch-only and solvables wallets names (furszy)
d70b159c42008ac3b63d1c43d99d4f1316d2f1ef wallet: improve post-migration logging (furszy)
f011e0f0680a8c39988ae57dae57eb86e92dd449 test: restorewallet, coverage for existing dirs, unnamed wallet and prune failure (furszy)
36093bde63286e19821a9e62cdff1712b6245dc7 test: add coverage for unnamed wallet migration failure (furszy)
f4c7e28e80bf9af50b03a770b641fd309a801589 wallet: fix unnamed wallet migration failure (furszy)
4ed0693a3f2a427ef9e7ad016930ec29fa244995 wallet: RestoreWallet failure, erase only what was created (furszy)
Pull request description:
Minimal fix for #34128.
The issue occurs during the migration of a legacy unnamed wallet
(the legacy "default" wallet). When the migration fails, the cleanup
logic is triggered to roll back the state, which involves erasing the
newly created descriptor wallets directories. Normally, this only
affects the parent directories of named wallets, since they each
reside in their own directories. However, because the unnamed
wallet resides directly in the top-level `/wallets/` folder, this
logic accidentally deletes the main directory.
The fix ensures that only the wallet.dat file of the unnamed wallet
is touched and restored, preserving the wallet in BDB format and
leaving the main `/wallets/` directory intact.
#### Story Line:
#32273 fixed a different set of issues and, in doing so, uncovered
this one.
Before the mentioned PR, backups were stored in the same directory
as the wallet.dat file. On a migration failure, the backup was then
copied to the top-level `/wallets/` directory. For the unnamed legacy
wallet, the wallet directory is the `/wallets/` directory, so the source
and destination paths were identical. As a result, we threw early in the
`fs::copy_file` call ([here](https://github.com/bitcoin/bitcoin/blob/29.x/src/wallet/wallet.cpp#L4572)) because the file already existed, as we
were trying to copy the file onto itself. This caused the cleanup logic
to abort early on and never reach the removal line.
#### Testing Notes:
Cherry-pick the test commit on top of master and run it. You will
see the failure and realize the reason by reading the test code.
ACKs for top commit:
achow101:
ACK b7c34d08dd9549a95cffc6ec1ffa4bb4f81e35eb
davidgumberg:
crACK b7c34d08dd
w0xlt:
ACK b7c34d08dd
willcl-ark:
ACK b7c34d08dd9549a95cffc6ec1ffa4bb4f81e35eb
Tree-SHA512: d0be14c0ed6417f999c3f2f429652c2407097d0cc18453c91653e57ae4b5375b327ad3b2553d9ea6ff46a3ae00cdbd5ab325b94eba763072c4fc5a773b85618b
fafbc70d48e1fb42c4ed3da609e5f30c4cc39418 rpc: [wallet] Use unsigned type for tx version in sendall (MarcoFalke)
Pull request description:
It is confusing to parse the unsigned tx version as a signed type. Also, it makes it harder to use the integer sanitizer.
Can be tested via:
* Build with the flags `-DCMAKE_C_COMPILER='clang' -DCMAKE_CXX_COMPILER='clang++' -DSANITIZERS=undefined,integer,float-divide-by-zero`
* Set the existing suppressions: `export UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=0:report_error_type=1"`
* Start the RPC server, e.g. `./bld-cmake/bin/bitcoin-qt -datadir=/tmp -regtest -server`
* Call the sendall RPC, e.g. `./bld-cmake/bin/bitcoin-cli -datadir=/tmp -regtest -named sendall '["bcrt1qlrt3xps4wxpfcjmljrayr2ualczmnfvd4vzdq3"]' fee_rate=1.234 version=-1`
Before:
```
src/wallet/rpc/spend.cpp:1470:42: runtime error: implicit conversion from type 'int' of value -1 (32-bit, signed) to type 'uint32_t' (aka 'unsigned int') changed the value to 4294967295 (32-bit, unsigned)
Invalid parameter, version out of range(1~3)
```
After:
```
JSON integer out of range
ACKs for top commit:
bensig:
ACK fafbc70d48e1fb42c4ed3da609e5f30c4cc39418
achow101:
ACK fafbc70d48e1fb42c4ed3da609e5f30c4cc39418
rkrux:
utACK fafbc70d48e1fb42c4ed3da609e5f30c4cc39418
theStack:
ACK fafbc70d48e1fb42c4ed3da609e5f30c4cc39418
Tree-SHA512: bb7cf54e9691ad2591646b138ffdfac95bf77c5234d489f4e4f2c60b41bdc14cdc18a030fecb0a6ac64e55e4c69b37835afd334f87d8a44b8df6cda053e8fefb
