71aae72e1f test: test sendall does ancestor aware funding (ishaanam)
36757941a0 wallet, rpc: implement ancestor aware funding for sendall (ishaanam)
544131f3fb rpc, test: test sendall spends unconfirmed change and unconfirmed inputs when specified (ishaanam)
Pull request description:
This PR:
- Adds a functional test that `sendall` spends unconfirmed change
- Adds a functional test that `sendall` spends regular unconfirmed inputs when specified by user
- Adds ancestor aware funding to `sendall` by using `calculateCombinedBumpFee` and adjusting the effective value accordingly
- Adds a functional test for ancestor aware funding in `sendall`
ACKs for top commit:
S3RK:
ACK 71aae72e1f
achow101:
ACK 71aae72e1f
furszy:
ACK 71aae72e1f
Tree-SHA512: acaeb7c65166ce53123a1d6cb5012197202246acc02ef9f37a28154cc93afdbd77c25e840ab79bdc7e0b88904014a43ab1ddea79d5337dc310ea210634ab61f0
bd34dd85e7 Use `exact_target` shorthand in coinselector_tests (Murch)
7aa7e30441 Fold GetSelectionWaste() into ComputeAndSetWaste() (Murch)
Pull request description:
PR #26152 moved waste calculation into SelectionResult to be able to correct the waste score on basis of the bump_fee_group_discount for overlapping ancestries. This left two functions with largely overlapping purpose, where one was simply a wrapper of the other. This PR cleans up the overlap, and fixes the double-meaning of `change_cost` where the `GetChange()` function assumed that no change was created when `change_cost` was set to 0. This behavior was exploited in a bunch of tests, but is problematic, because a `change_cost` of 0 is permitted with custom settings for feerate and discard_feerate (i.e. when they’re both 0).
ACKs for top commit:
achow101:
ACK bd34dd85e7
furszy:
Code ACK bd34dd85e7
ismaelsadeeq:
Code Review ACK bd34dd85e7
Tree-SHA512: 83a2688d45d719dc61a64b5180fe136107faccf401a59df65245c05d701748a03e85ed56fde8c9b7ef39a3ab54374dd3718c559bda5b3f55dafedfd7fed25161
22d0f1a27e [fuzz] Avoid endless waiting in FuzzedSock::{Wait,WaitMany} (marcofleon)
a7fceda68b [fuzz] Make peeking through FuzzedSock::Recv fuzzer friendly (dergoegge)
865cdf3692 [fuzz] Use fuzzer friendly ConsumeRandomLengthByteVector in FuzzedSock::Recv (dergoegge)
Pull request description:
`FuzzedSock` has a few issues that block a fuzzer from making progress. See commit messages for details.
ACKs for top commit:
marcofleon:
Tested ACK 22d0f1a27e
brunoerg:
utACK 22d0f1a27e
Tree-SHA512: 2d66fc94ba58b6652ae234bd1dcd33b7d685b5054fe83e0cd624b053dd51519c23148f43a865ab8c8bc5fc2dc25e701952831b99159687474978a90348faa4c5
ffbc173ca1 depends: Update Boost download link (Hennadii Stepanov)
Pull request description:
The Boost has [migrated](https://github.com/boostorg/boost-tasks/pull/3) their downloads from from boostorg.jfrog.io to archives.boost.io.
So do we.
FWIW, the download speed at my location is much better :)
ACKs for top commit:
maflcko:
ACK ffbc173ca1
Tree-SHA512: 3f66675c390510ecfdacf2ac8af4a0a6bdbdf6fbfe01ed66fbe1b11bc9e935709e5492a754711cd788352c5853738fce755afd0c0480f36d0f8af31fcc108263
This method was introduced as a pre-requirement for the v2 transport
protocol back then (see PR #14047, commit 463921bb), when it was still
BIP151. With the replacement BIP324, this is not needed anymore, and
it's also unlikely that any other proposal would need to negate private
keys at this abstraction level.
(If there is really demand, it's trivial to reintroduce the method.)
3c08e11c3e doc: JSON-RPC request Content-Type is application/json (Luke Dashjr)
Pull request description:
Specify json content type in RPC examples.
Picks up #29946. Which needed rebasing and the commit message fixing,
ACKs for top commit:
laanwj:
ACK 3c08e11c3e
tdb3:
ACK for 3c08e11c3e
Tree-SHA512: 770bbbc0fb324cb63628980b13583cabf02e75079851850170587fb6eca41a70b01dcedaf1926bb6488eb9816a3cc6616fe8cee8c4b7e09aa39b7df5834ca0ec
e3249f2111 fuzz: add more coverage for `ScriptPubKeyMan` (brunoerg)
Pull request description:
This PR adds more coverage for `ScriptPubKeyMan`:
- Check `GetKey` and `HasPrivKey` after adding descriptor key.
- Cover `GetEndRange` and `GetKeyPoolSize`.
- Cover `MarkUnusedAddresses` with the scripts from ScriptPubKeys and `GetMetadata` with the destinations from them.
ACKs for top commit:
marcofleon:
Tested ACK e3249f2111. I ran the updated harness for ~9 hours on an empty corpus, generated a coverage report, and checked that the new functions mentioned were hit. Coverage of `scriptpubkeyman.cpp` increased.
murchandamus:
Tested ACK e3249f2111
Tree-SHA512: cfab91f6c8401174842e79209c0e9225c08f011fe9b41d0a58bcec716ae4545eaf803867f899ed7b5fbcefea45711f91894e36df082ba19732dd310cd9e61a79
For example:
```bash
ADDITIONAL_GUIX_COMMON_FLAGS set in the ENV
ADDITIONAL_GUIX_ENVIRONMENT_FLAGS="--emulate-fhs" ./contrib/guix/guix-build
<snip>
INFO: Building f75199182133 for platform triple x86_64-linux-gnu:
...using reference timestamp: 1716905119
...running at most 10 jobs
...from worktree directory: '/bitcoin'
...bind-mounted in container to: '/bitcoin'
...in build directory: '/bitcoin/guix-build-f75199182133/distsrc-f75199182133-x86_64-linux-gnu'
...bind-mounted in container to: '/distsrc-base/distsrc-f75199182133-x86_64-linux-gnu'
...outputting in: '/bitcoin/guix-build-f75199182133/output/x86_64-linux-gnu'
...bind-mounted in container to: '/outdir-base/x86_64-linux-gnu'
ADDITIONAL FLAGS (if set)
ADDITIONAL_GUIX_COMMON_FLAGS: --no-substitutes
ADDITIONAL_GUIX_ENVIRONMENT_FLAGS: --emulate-fhs
ADDITIONAL_GUIX_TIMEMACHINE_FLAGS:
```
Currently, when the FuzzedDataProvider of a FuzzedSock runs out of data,
FuzzedSock::Wait and WaitMany will simulate endless waiting as the
requested events are never simulated as occured.
Fix this by simulating event occurence when ConsumeBool() returns false
(e.g. when the data provider runs out).
Co-authored-by: dergoegge <n.goeggi@gmail.com>
FuzzedSock only supports peeking at one byte at a time, which is not
fuzzer friendly when trying to receive long data.
Fix this by supporting peek data of arbitrary length instead of only one
byte.
cbd4640ede build: remove --enable-lcov-branch-coverage (fanquake)
Pull request description:
This supports lcov `2.x` in the sense that we are no-longer hardcoding version specific options, and instead will use the `LCOV_OPTS` variable (which is the more flexible thing to do in any case). It's also quite likely that devs are already having to pass extra options to lcov `2.x`, given it's more stringent in terms of coverage generation and error checking. See this thread for an example: https://github.com/linux-test-project/lcov/issues/238.
Tested on one machine (LCOV 2.0, gcc 13.2) with:
```bash
./autogen.sh
./configure --enable-lcov CXXFLAGS="-fprofile-update=prefer-atomic" LCOV_OPTS="--rc branch_coverage=1 --ignore-errors mismatch"
make
make cov
<snip>
Processing file src/netaddress.cpp
lines=521 hit=480 functions=72 hit=72 branches=675 hit=499
Overall coverage rate:
lines......: 81.8% (79362 of 97002 lines)
functions......: 77.8% (10356 of 13310 functions)
branches......: 49.6% (130628 of 263196 branches)
```
and another machine (LCOV 2.1, Clang 18.1.3) with:
```bash
./autogen.sh
./configure --enable-lcov CC=clang CXX=clang++ LCOV_OPTS="--rc branch_coverage=1 --ignore-errors mismatch,inconsistent"
make
make cov
<snip>
Processing file src/util/strencodings.cpp
lines=315 hit=311 functions=38 hit=38 branches=425 hit=357
Overall coverage rate:
source files: 622
lines.......: 79.8% (70311 of 88132 lines)
functions...: 78.1% (13968 of 17881 functions)
branches....: 44.5% (157551 of 354317 branches)
Message summary:
101 warning messages:
count: 1
inconsistent: 100
3528 ignore messages:
inconsistent: 3528
```
Related to #28468.
ACKs for top commit:
theuni:
utACK cbd4640ede
hebasto:
ACK cbd4640ede, tested on Ubuntu 22.04.
Tree-SHA512: 94eb01e0e236a480052749f6107b1d0d2e4f6f70a8eefd55fa9ba3d2f72996c9e8a0f28340698b7ac82e7a71e9cf799b7a53ddb6e435e5e9795f5f98a18820f7
8defc182a3 scripted-diff: Replace nNextSweep with m_next_sweep (marcofleon)
0048680467 increase txorphan harness stability (marcofleon)
Pull request description:
This moves `nNextSweep` from being a static variable in `LimitOrphans` to being a member of the `TxOrphanage` class. This improves the stability of the `txorphan` fuzz harness, as each orphanage (created every iteration) now has its own value for `nNextSweep`.
ACKs for top commit:
maflcko:
utACK 8defc182a3
dergoegge:
Code review ACK 8defc182a3
glozow:
utACK 8defc182a3, I can rebase on this pretty easily
Tree-SHA512: 54d4a5074def764f6c895308b94e417662d2f21f157925421131745f22743907df59971f4ce545063658cd74ec133792cdd8df96ae3e69af8314e9b0ff899d48
a27e1ceb9f depends: consolidate dependency docs (fanquake)
Pull request description:
Adds missing `g++` for macOS. This is needed by Qt:
```bash
Configuring qt...
Creating qmake...
gmake[1]: Entering directory '/bitcoin/depends/work/build/arm64-apple-darwin/qt/5.15.14-4bca24c8f89/qtbase/qmake'
gmake[1]: g++: No such file or directory
gmake[1]: *** [Makefile:250: main.o] Error 127
```
`xz-utils` was also missing (but generally already installed), and is needed for the `.tar.xz` tarballs.
Remove `bsdmainutils`, as this is only needed by the main build system (for tests), and isn't needed to complete a depends build.
ACKs for top commit:
maflcko:
ACK a27e1ceb9f
Tree-SHA512: 720c31d4d4c9b86fda4aace405d528193714dd3e526f38d5b8a83e4b676a433b9c891f01d86d673be9ac848458eda8a89b0981003a42eaa6d97bacc2e914396a
0a3631fc35 depends: fix Qt macOS build with Clang 18 (fanquake)
b018bd779d depends: qt 5.15.14 (fanquake)
Pull request description:
Also adds a patch to Qts internal libpng, to fix compilation using Clang 18, when targetting macOS. I'd like to get this patched, so we can continue working on removing `FORCE_USE_SYSTEM_CLANG` (#30201); otherwise builds will be broken using the default Clang (`18`) on the current Ubuntu LTS (`24.04`).
With this PR, anyone using Ubuntu 24.04 should be able to `apt install clang llvm lld`, and then cross-compile for macOS using:
```bash
# clang --version
Ubuntu clang version 18.1.3 (1)
make -C depends HOST=arm64-apple-darwin FORCE_USE_SYSTEM_CLANG=1
./autogen.sh
CONFIG_SITE=/path/to/depends/arm64-apple-darwin/share/config.site ./configure
make
# file src/qt/bitcoin-qt
src/qt/bitcoin-qt: Mach-O 64-bit arm64 executable, flags:<NOUNDEFS|DYLDLINK|TWOLEVEL|WEAK_DEFINES|BINDS_TO_WEAK|PIE|HAS_TLV_DESCRIPTORS>
```
ACKs for top commit:
TheCharlatan:
ACK 0a3631fc35
theuni:
utACK 0a3631fc35
hebasto:
ACK 0a3631fc35, a new patch indeed fixes cross-compiling on Ubuntu 24.04 with `FORCE_USE_SYSTEM_CLANG=1`.
Tree-SHA512: 711d321b1efbb1aeef802d1d7e72fff8f4e28aa2420d19df9db6f4449fc7d281e1d08ba242ce20122dfe21129e713bd59e7e6ade0b67d7271eea18b39ceb9283
Adds missing `g++` for macOS. This is needed by Qt:
```bash
Configuring qt...
Creating qmake...
gmake[1]: Entering directory '/bitcoin/depends/work/build/arm64-apple-darwin/qt/5.15.14-4bca24c8f89/qtbase/qmake'
gmake[1]: g++: No such file or directory
gmake[1]: *** [Makefile:250: main.o] Error 127
```
`xz-utils` was also missing (but generally already installed), and is
needed for the `.tar.xz` tarballs.
Remove bsdmainutils, as this is only needed by the main build system
(for tests), and isn't needed to complete a depends build.
This removes the need to actually track misbehavior score (see further commit), because any
Misbehaving node will immediately hit the discouragement threshold.
With the Misbehavior score gone for non-connecting headers (see previous
commit), there is no need to only treat headers messages with up to 8
headers as potential BIP130 announcements. BIP130 does not specify such
a limit; it was purely a heuristic.
This misbehavior was originally intended to prevent bandwidth wastage due to
actually observed very broken (but likely non-malicious) nodes that respond
to GETHEADERS with a response unrelated to the request, triggering a request
cycle.
This has however largely been addressed by the previous commit, which causes
non-connecting HEADERS that are received while a GETHEADERS has not been
responded to, to be ignored, as long as they do not time out (2 minutes).
With that, the specific misbehavior is largely irrelevant (for inbound peers,
it is now harmless; for outbound peers, the eviction logic will eventually
kick them out if they're not keeping up with headers at all).
Since https://github.com/bitcoin/bitcoin/pull/25454 we keep track of the last
GETHEADERS request that was sent and wasn't responded to. So far, every incoming
HEADERS message is treated as a response to whatever GETHEADERS was last sent,
regardless of its contents.
This commit makes this tracking more accurate, by only treating HEADERS messages
which (1) are empty, (2) connect to our existing block header tree, or (3) are a
continuation of a low-work headers sync as responses that clear the "outstanding
GETHEADERS" state (m_last_getheaders_timestamp).
That means that HEADERS messages which do not satisfy any of the above criteria
will be ignored, not triggering a GETHEADERS, and potentially (for now, but see
later commit) increase misbehavior score.
4b7d984269 lint: add markdown hyperlink checker (willcl-ark)
Pull request description:
Potential followup to: #30025
This should prevent us reintroducing broken markdown links.
It does not test "online" (external) links, only those within this repo. Both relative and absolute links are parsed successfully if they resolve.
ACKs for top commit:
maflcko:
re-utACK 4b7d984269
davidgumberg:
reACK 4b7d984269
Tree-SHA512: 9bc40d700b73499c046bb76157bc139f32ec3850f64ef813bbf7f18f9c01a253abe6a857d6f559890165f2bd26e7742c05d86232cd9b8efb33ff85d735f4f095
Patch Qts internal libpng to resolve the failure.
I would like to have this patched, so we can continue working on the
removal of `FORCE_USE_SYSTEM_CLANG`. Otherwise builds will be broken using
the default clang (18) on the current Ubuntu LTS (24.04).
This supports lcov 2.x in the sense that we are no-longer hardcoding
version specific options, and instead will use the `LCOV_OPTS` variable
(which is the more correct/flexible thing to do in any case). It's also
quite likely that devs are already having to pass extra options to lcov
2.x, given it's more stringent in terms of coverage generation and error
checking. See this thread for an example:
https://github.com/linux-test-project/lcov/issues/238.
Added an example to the developer notes.
Tested on one machine (LCOV 2.0, gcc 13.2) with:
```bash
./autogen.sh
./configure --enable-lcov CXXFLAGS="-fprofile-update=prefer-atomic" LCOV_OPTS="--rc branch_coverage=1 --ignore-errors mismatch"
make
make cov
<snip>
Processing file src/netaddress.cpp
lines=521 hit=480 functions=72 hit=72 branches=675 hit=499
Overall coverage rate:
lines......: 81.8% (79362 of 97002 lines)
functions......: 77.8% (10356 of 13310 functions)
branches......: 49.6% (130628 of 263196 branches)
```
and another machine (LCOV 2.1, Clang 18.1.3) with:
```bash
./autogen.sh
./configure --enable-lcov CC=clang CXX=clang++ LCOV_OPTS="--rc branch_coverage=1 --ignore-errors mismatch,inconsistent"
make
make cov
<snip>
Processing file src/util/strencodings.cpp
lines=315 hit=311 functions=38 hit=38 branches=425 hit=357
Overall coverage rate:
source files: 622
lines.......: 79.8% (70311 of 88132 lines)
functions...: 78.1% (13968 of 17881 functions)
branches....: 44.5% (157551 of 354317 branches)
Message summary:
101 warning messages:
count: 1
inconsistent: 100
3528 ignore messages:
inconsistent: 3528
```
5deb0b024e build, test, doc: Temporarily remove Android-related stuff (Hennadii Stepanov)
Pull request description:
Previously, our Android builds were geared towards generating APKs, which relied on Qt. However, after migrating to C++20, compiling for Android became unfeasible due to Qt 5.15's compatibility limitations with NDK only up to r25, which includes an outdated embedded libc++ (see https://github.com/bitcoin/bitcoin/issues/29360).
All removed stuff will be reinstated after migrating the build system to CMake and upgrading Qt to version 6.x.
This PR makes possible a clean migration to the CMake-based build system as it removes code, which is not used at this moment.
ACKs for top commit:
vasild:
ACK 5deb0b024e
fanquake:
ACK 5deb0b024e - given none of this is currently tested/wont compile. Can be revisted in future.
Tree-SHA512: 3bc2ccfe881e11cc1d78c27acd6f1d86cfba86821ef3bb5eca2e80d978fdfa13659ec82284dcaadc507e2394524dea91d4b8f81d0030c1cef9708df8be76bf07
7c8abf3c20 bench: bugfix, properly release wallet before erasing directory (furszy)
Pull request description:
Simple fix for #29816.
Since the wallet is appended to the global `WalletContext` during
creation, merely calling `reset()` on the benchmark shared_pointer
is insufficient to destruct the wallet. This no destruction of the
wallet object results in keeping the db connection open, which
was causes the `fs::remove_all()` failure on Windows.
ACKs for top commit:
maflcko:
utACK 7c8abf3c20
kevkevinpal:
utACK [7c8abf3](7c8abf3c20)
hebasto:
re-ACK 7c8abf3c20, I agree with changes since my recent [review](https://github.com/bitcoin/bitcoin/pull/30122#pullrequestreview-2061694682).
Tree-SHA512: 279df65bea8f7aa02af0a2efed62dca9bf9b29cb748eb369c602d223e08a8a907dea7b1bffbd3dab91b1656c1d91b18a9a0534bc3f153bd751414b0e6230b3a4
9ddf39dd87 fuzz: Handle missing BDBRO errors (Ava Chow)
Pull request description:
Adds error messages that were not being handled. Also removes error messages that no longer exist.
Fixes#30166
ACKs for top commit:
dergoegge:
reACK 9ddf39dd87
TheCharlatan:
ACK 9ddf39dd87
Tree-SHA512: 2597536a1e5d030653dfcb02fd892f7492f5a091def787f6cbd421b8bca9544847684a498e9458ea99ae7de5a8a6d91532ff904d1e39222d324939d31d2eb3f0
949abebea0 [fuzz] Avoid collecting initialization coverage (dergoegge)
Pull request description:
Our coverage reports include coverage of initialization code, which can be misleading when trying to evaluate the coverage a fuzz harness achieves through fuzzing alone.
This PR proposes to make fuzz coverage reports more accurate by resetting coverage counters after initialization code has been run. This makes it easier to evaluate which code was actually reached through fuzzing (e.g. to spot fuzz blockers).
ACKs for top commit:
maflcko:
utACK 949abebea0
brunoerg:
nice, utACK 949abebea0
Tree-SHA512: c8579bda4f3d71d199b9331fbe6316fce375a906743d0bc216bb94958dc03fdc9a951ea50cfeb487494a75668ae3c16471a82f7e5fdd912d781dc29d063e2c5b
