02942056fd test: fix intermittent failure in p2p_orphan_handling.py (Martin Zumsande)
Pull request description:
If the mocktime is bumped before the node has successfully disconnected the peer, the requests for both parents could be spread over two GETDATAS: The first time `GetRequestsToSend` is invoked it would only request one tx from peer2, because the other one would only be available after peer1 was disconnected and its outstanding txrequest cleared.
So two GETDATAs would be sent, which would make the test fail.
Fixes#31700
ACKs for top commit:
maflcko:
lgtm ACK 02942056fd
instagibbs:
ACK 02942056fd
Tree-SHA512: 769200898345da197d86d673d9506f08f0a64b72a456e7e7c988ac37450d9c54ec65da1c8447c566c8578f7cfccdc5723ea680e636bfbe0b3d38265e5ef57774
9157d9e449 build: use make < 3.82 syntax for define directive (Sjors Provoost)
Pull request description:
From the GNU make 3.82 [release announcement](https://lists.gnu.org/archive/html/info-gnu/2010-07/msg00023.html) (2010):
> The 'define' make directive now allows a variable assignment operator
after the variable name, to allow for simple, conditional, or appending
multi-line variable assignment.
macOS ships with 3.81 (2006). This caused the multiprocess config options to be ignored.
Fixes#32068
ACKs for top commit:
ryanofsky:
Code review ACK 9157d9e449. This is a pretty unusual bug and I don't understand how it wasn't causing any errors with make 3.81, just causing the flags to be ignored.
Tree-SHA512: a07322e25ef18296264379a2704f31c654df196d3ea09fe712885c38813e54d758a2d603ee9f7a302da8011fba6d139aa30a356175ca99df728ade2572a87560
fa9cf38ab6 scripted-diff: test: Rename send_message to send_without_ping (MarcoFalke)
fa4356717d test: Prefer send_and_ping over send_message+sync_with_ping (MarcoFalke)
Pull request description:
`send_message` is problematic, because it is easy to forget a `sync_with_ping` (or other `wait_until`), leading to intermittent test failures. (Example: https://github.com/bitcoin/bitcoin/pull/31837#discussion_r1950370246)
There are more uses of `send_and_ping` in the codebase than `send_message`, so in most cases `send_and_ping` is needed anyway.
For the remaining cases, clearly document that no sync happens by renaming `send_message` to `send_without_ping`.
ACKs for top commit:
instagibbs:
ACK fa9cf38ab6
Tree-SHA512: 31caa6568d292ae3d3dda931a94aaa30cc1205ec2ef537a484393eb55687f86c212f1e751ac4a7636610bdf591502a50995dc63bf02f97be9fdc482072160b07
d5537c18a9 fuzz: make sure DecodeBase58(Check) is called with valid values more often (Lőrinc)
bad1433ef2 fuzz: Always restrict base conversion input lengths (Lőrinc)
Pull request description:
This is a follow-up to https://github.com/bitcoin/bitcoin/pull/30746, expanding coverage by:
* restricting every input for the base58 conversions, capping max sizes to `100` instead of `1000` or all available input (suggested by marcofleon in https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1963718683) since most actual usage has lengths of e.g. `21`, `34`, `78`.
* providing more valid values to the decoder (suggested by maflcko in https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1957847712) by randomly providing a random input or a valid encoded one; this also enables unifying the roundtrip tests to a single roundtrip per fuzz.
ACKs for top commit:
mzumsande:
Code Review / lightly tested ACK d5537c18a9
maflcko:
review ACK d5537c18a9🚛
Tree-SHA512: 50365654cdac8a38708a7475eaa43396642b7337e2ee8999374c3faafff4f05457abc1a54c701211e0ed24d36c12af77bcad17b49695699be42664f2be660659
2819c51482 test: Use rpc_deprecated only for testing deprecation (Fabian Jahr)
Pull request description:
The comment in `functional/rpc_deprecated.py` says "This test should be used to verify correct behaviour of deprecated RPC methods with and without the -deprecatedrpc flags." I think we can get rid of the "with" part since we can assume that every deprecated RPC is already tested in at least one other functional test. (I didn't look but I could verify in our coverage if someone has doubts about that.) In order for this test to continue working, the flag will need to be used there. Otherwise this seems to prescribe copy+pasting a basic test from another file and I don't see a good reason for that.
ACKs for top commit:
maflcko:
lgtm ACK 2819c51482
janb84:
re ACK [2819c51](2819c51482)
polespinasa:
reACK 2819c51482
Tree-SHA512: 96edfd07be863ad19f99feb27afbc2c3ad53560c93c93eac8de8d766ad8c46e5aa02fd013fec99af794cbe9adca8e459d5b80b454caea7e67b2388003e010bb6
From the GNU make 3.82 release announcement:
* The 'define' make directive now allows a variable assignment operator
after the variable name, to allow for simple, conditional, or appending
multi-line variable assignment.
macOS ships with 3.81. This caused the multiprocess config options
to be ignored.
Fixes#32068
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
send_message only drops the bytes in a buffer and a sync is needed to
avoid intermittent test issues. Change the name of the method to make
this more apparent during review.
-BEGIN VERIFY SCRIPT-
sed -i 's/send_message(/send_without_ping(/g' $( git grep -l 'send_message(' )
-END VERIFY SCRIPT-
36b6f36ac4 build: require sqlite when building the wallet (Sjors Provoost)
Pull request description:
Require that sqlite is available in order to compile the wallet. Removes instances of `USE_SQLITE` since it is no longer possible to not have sqlite available.
The `NO_SQLITE` option is dropped from depends.
This is another step towards dropping the legacy wallet, extracted from #31250.
ACKs for top commit:
m3dwards:
ACK 36b6f36ac4
davidgumberg:
crACK 36b6f36ac4
hebasto:
re-ACK 36b6f36ac4.
Tree-SHA512: 870a0135671c80c4f28602119eb8637a1ed43b51b1673bfe88425782fb62ec6ef0f3d6baf0d5984d6a243779b0f63423fd4c4dc324ef87bffba13d63e05ad793
80b5e7f2cb build: Remove manpages when making MacOS app (Ava Chow)
Pull request description:
When creating the MacOS app, the only file that should be in `Bitcoin-Qt.app/Contents/MacOS` is `Bitcoin-Qt`. Since #31844, there was also a `share/` containing the manpage for bitcoin-qt. This manpage is not useful to app users, and it is also causing code signing issues. Thus the directory should be removed when making the app.
Fixes https://github.com/bitcoin/bitcoin/issues/32052#issuecomment-2723007926
ACKs for top commit:
fanquake:
ACK 80b5e7f2cb
Tree-SHA512: fd15b7f99737484e40d31c583e01acc7470d038b0c584dfaefecc740811565ceee048913b6e5e37e7935b74d8100dc8323aed3f69d9a6baa5f434754009eb18c
5dfef6b9b3 depends: remove NO_HARDEN option (fanquake)
Pull request description:
This was only needed to work around a (Libtool related iirc) Windows issue, when hardening was disabled. I can no-longer recreate this failure, so it'd be good to remove this Windows carveout.
ACKs for top commit:
davidgumberg:
crACK 5dfef6b9b3
laanwj:
Code review ACK 5dfef6b9b3
Tree-SHA512: 38657f09c537ba02ecaf0676d47087a835283cabfc81ad9b2d5e68858dcd7a610b6a1df6730920d40b48be2bbc55a45d6b8aea4364884b5f1c1bd12126940f5b
If we bump the mocktime before the node has successfully disconnected
the peer, the requests for both parents could be spread over
two GETDATAS, which would make the test fail.
a24419f8be contrib: Fix `gen-bitcoin-conf.sh`. (David Gumberg)
Pull request description:
In #31118, the format of bitcoind's `--help` output changed slightly in a way that breaks `gen-bitcoin-conf.sh`, modify the script to accommodate the new format, by starting after the line that says "Options:" and stripping the `-help` options and descriptions from the script output.
Before this PR, all options above `-help` were excluded from the example bitcoin.conf.
ACKs for top commit:
mabu44:
Tested ACK a24419f8be
glozow:
ACK a24419f8be
rkrux:
tACK a24419f8be
BrandonOdiwuor:
crACK a24419f8be
Tree-SHA512: 2ef697166d0b37b61ec1a20e357b91d611c932a0e453c4669f74ab69e6310ea1776dce09c1b77e82746072265763cb0c750e6df4c8b4a7d39068fc03f97b221b
e5ff4e416e qa: use a clearer and documented amount error in malleated snapshot (Antoine Poinsot)
b34fdb5ade test: introduce output amount (de)compression routines (Sebastian Falbesoner)
a7911ed101 test: introduce VARINT (de)serialization routines (Sebastian Falbesoner)
Pull request description:
The `feature_assumeutxo.py` functional test checks various errors with malleated snapshots. Some of these cases are brittle or use confusing and undocumented values. Fix one of those by using a clear, documented and forward-compatible value.
I ran across those when working on an unrelated changeset which affected the snapshot. It took me a while to understand where the seemingly magic byte string was coming from, so i figured it was worth proposing this patch on its own for the sake of making the test more maintainable.
See commit messages for details.
ACKs for top commit:
janb84:
re ACK [e5ff4e4](e5ff4e416e)
theStack:
ACK e5ff4e416e
fjahr:
Code review ACK e5ff4e416e
i-am-yuvi:
tACK e5ff4e416e
Tree-SHA512: 60f022b7176836ce05e8f287b436329d7ca6460f3fcd95f78cd24e07a95a7d4d9cbbb68a117916a113fe451732b09a012d300fe860ff33d61823eca797ceddaf
11f8ab140f test: wallet, coverage for crash on dup block disconnection during unclean shutdown (Martin Zumsande)
9ef429b6ae wallet: fix crash on double block disconnection (furszy)
Pull request description:
The wallet crashes if it processes the same block disconnection event twice in a row due
to an incompatible coinbase transaction state.
This happens because `disconnectBlock` provides `TxStateInactive` without the "abandoned"
flag for coinbase transactions to `SyncTransaction`, while `AddToWallet()` internally modifies
it to retain the abandoned state.
The crash flow is as follows:
1) On the first disconnection, the transaction state transitions from "confirmed" to
"inactive," bypassing the state equality check since the provided state differs. Then,
`AddToWallet` internally updates the state to "inactive + abandoned"
2) On the second disconnection, as we provide only the "inactive" state
to `SyncTransaction()`, the state equality assertion fails and crashes the wallet.
Reviewers Note:
The crash can easily be replicated by cherry-picking the test commit in master.
ACKs for top commit:
mzumsande:
Code Review ACK 11f8ab140f
rkrux:
ACK 11f8ab140f
pinheadmz:
ACK 11f8ab140f
Tree-SHA512: 971069bca562f0afb06c34a2516842d01b5cbc2b18ed851392aa3caa3bb7488f4a84a5d017ea334e6361261d3c44aa597cc67a1d4fa16781f85e081f3d1f8771
de1ada079b doc: Adjust path in comment (Hennadii Stepanov)
Pull request description:
It was overlooked in bitcoin/bitcoin#31161.
ACKs for top commit:
l0rinc:
utACK de1ada079b
dergoegge:
ACK de1ada079b
Tree-SHA512: ea2b858f9c6520a71a3e14a3877ad38acfcddee6ee4aad5fa8fc1c08a0589d9d4df9b5fdbae834bd03bbb956f8b52fdd2b35144fe0bf943bc9455ed6c3fc1c80
893ca54585 contrib: Fix deterministic-unittest-coverage tool path (janb84)
Pull request description:
Fix for the tooling introduced/modified in #31901 but the tool path is broken due to silent merge conflict introduced by #31161.
The `deterministic-unittest-coverage` and `deterministic-fuzz-coverage` tools uses the `fuzz` and `test_bitcoind` binaries, for which the location was modified in #31161. This patch updates the location to align with that change.
ACKs for top commit:
maflcko:
lgtm ACK 893ca54585
hebasto:
ACK 893ca54585, I have reviewed the code and it looks OK.
Tree-SHA512: efd5a75b607a6a7889333674b9a1fccf30b6a80bb2aa941f9dcc10b2e0b6158d8c870d58aa76cef2a0280782570400730828d6a4aaf806b83959e9f4bc7313f4
fa21597064 ci: Revert "Temporary workaround for old CCACHE_DIR cirrus env" (MarcoFalke)
Pull request description:
Seems fine to revert this now. If this still happens it should be rare enough and trivial to fix via a new push (normal push, force-push, rebase, ...), or to just ignore the failure.
ACKs for top commit:
fanquake:
ACK fa21597064
Tree-SHA512: 125946dcd4025b04f11276e1df1e0f1b361fc2deed2e3919d680d58df97e6d82881b21c0c683142df6995226b4c12b2dde5415ad78474b8fcda115cc69fa0207
1ef22ce335 depends: patch around PlacementNew issue in capnp (fanquake)
Pull request description:
See #31772 and https://github.com/capnproto/capnproto/pull/2235.
Given there isn't agreement in #29796, pulled this out so it could be merged separately, and it's easier to run different test configurations externally.
Closes#31772.
ACKs for top commit:
ryanofsky:
Code review ACK 1ef22ce335. Confirmed patch is identical to one merged upstream. Only change since last review was tweaking the file paths and commit data in the patch.
TheCharlatan:
ACK 1ef22ce335
Tree-SHA512: 9c9ecf50c43cf74315f6659afab55aeafb436f70e83b328016ad574136dce46867220c6e1a85aefd8d3d3d027cd94cc807c79721a4983da9428de70f11224e52
In #31118, the format of bitcoind's `--help` output changed slightly in
a way that breaks `gen-bitcoin-conf.sh`, modify the script to accomodate
the new format, by starting after the line that says "Options:" and
strip the `-help` option and its description from the output.
18e83534ac wallet: Replace "non-0" with "non-zero" in translatable error message (Hennadii Stepanov)
Pull request description:
Transifex interprets the "-0" substring as a number in translatable strings. Since not all translations preserve "-0," this triggers a corresponding warning. While this warning could be disabled globally, it is more reasonable to adjust the original string instead.
ACKs for top commit:
davidgumberg:
ACK 18e83534ac
l0rinc:
ACK 18e83534ac
1440000bytes:
ACK 18e83534ac
BrandonOdiwuor:
Code Review ACK 18e83534ac
laanwj:
Code review ACK 18e83534ac
Tree-SHA512: 5c38cfc4b352dbbcc8de5fb907cf988a77a7ecded7a90fe0517bfb9e4cd5097bdeb1aa6edf5d9ca37de54d1d7939d5e49533ec93c403db90d9169ad7732e5124
cadbd4137d miner: have waitNext return after 20 min on testnet (Sjors Provoost)
d4020f502a Add waitNext() to BlockTemplate interface (Sjors Provoost)
Pull request description:
This PR introduces `waitNext()`. It waits for either the tip to update or for fees at the top of the mempool to rise sufficiently. It then returns a new template, with which the caller can rinse and repeat.
On testnet3 and testnet4 the difficulty drops after 20 minutes, so the second ensures that a new template is returned in that case.
Alternative approach to #31003, suggested in https://github.com/bitcoin/bitcoin/issues/31109#issuecomment-2451942362
ACKs for top commit:
ryanofsky:
Code review ACK cadbd4137d. Main change since last review is adding back a missing `m_interrupt` check in the waitNext loop. Also made various code cleanups in both commits.
ismaelsadeeq:
Code review ACK cadbd4137d
vasild:
ACK cadbd4137d
Tree-SHA512: c5a40053723c1c1674449ba1e4675718229a2022c8b0a4853b12a2c9180beb87536a1f99fde969a0ef099bca9ac69ca14ea4f399d277d2db7f556465ce47de95
Require that sqlite is available in order to compile the wallet. Removes
instances of USE_SQLITE since it is no longer possible to not have
sqlite available.
The NO_SQLITE option is dropped from depends.
Co-authored-by: Ava Chow <github@achow101.com>
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
This only existed to workaround a (iirc libtool related) windows issue
that only occured when compiling without hardening. We no-longer use
libtool, and I can no-longer create the failure.
e637dc2c01 refactor: Replace uint256 type with Wtxid in PackageMempoolAcceptResult struct (marcofleon)
a3baead7cb validation: use wtxid instead of txid in CheckEphemeralSpends (marcofleon)
Pull request description:
This PR addresses a small bug in [`AcceptMultipleTransactions`](45719390a1/src/validation.cpp (L1598)) where a txid was being inserted into a map that should only hold wtxids. `CheckEphemeralSpends` has an out parameter on failure that records that the child transaction did not spend the parent's dust. Instead of using the txid of this child, use its wtxid.
The second commit in this PR is a refactor of the `PackageMempoolAcceptResult` struct to use the `Wtxid` type instead of `uint256`. This helps to prevent errors like this in the future.
ACKs for top commit:
instagibbs:
ACK e637dc2c01
glozow:
ACK e637dc2c01, hooray for type safety
dergoegge:
Code review ACK e637dc2c01
Tree-SHA512: 17039efbb241b7741e2610be5a6d6f88f4c1cbe22d476931ec99e43f993d259a1a5e9334e1042651aff49edbdf7b9e1c1cd070a28dcba5724be6db842e4ad1e0
59c4930394 qa: Enable feature_init.py on Windows (Hodlinator)
Pull request description:
Windows has been skipped since feature_init.py was added in #23289. Possibly due to poorer support on older Python versions, or attempts to use `CTRL_C_EVENT` (which didn't work in my testing either) instead of `CTRL_BREAK_EVENT`.
ACKs for top commit:
maflcko:
lgtm ACK 59c4930394
BrandonOdiwuor:
Code Review ACK 59c4930394
hebasto:
ACK 59c4930394, I have reviewed the code and it looks OK.
Tree-SHA512: 4f3649b41bcba2e8d03b8dcb1a7a6882edafb2c456db4b0768fc86018e9e9ed7171cb3d3c99e74b4ef38a3fcf3ab5d2f1865bbd49d791f1ce0a246806634e1a7
568fcdddae scripted-diff: Adjust documentation per top-level target output location (Hennadii Stepanov)
026bb226e9 cmake: Set top-level target output locations (Hennadii Stepanov)
Pull request description:
This PR sets the target output locations to the `bin` and `lib` subdirectories within the build tree, creating a directory structure that mirrors that of the installed targets.
This approach is widely adopted by the large projects, such as [LLVM](e146c1867e/lldb/cmake/modules/LLDBStandalone.cmake (L128-L130)):
```cmake
set(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/bin)
set(CMAKE_LIBRARY_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/lib${LLVM_LIBDIR_SUFFIX})
set(CMAKE_ARCHIVE_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/lib${LLVM_LIBDIR_SUFFIX})
```
The `libsecp256k1` project has also recently [adopted](https://github.com/bitcoin-core/secp256k1/pull/1553) this approach.
With this PR, all binaries are conveniently located. For example, run:
```
$ ./build/bin/fuzz
```
instead of:
```
$ ./build/src/test/fuzz/fuzz
```
On Windows, all required DLLs are now located in the same directory as the executables, allowing to run `bitcoin-chainstate.exe` (which loads `bitcoinkernel.dll`) without the need to copy DLLs or modify the `PATH` variable.
The idea was briefly discussed among the build team during the recent CoreDev meeting.
---
**Warning**: This PR changes build locations of newly built executables like `bitcoind` and `test_bitcoin` from `src/` to `bin/` without deleting previously built executables. A clean build is recommended to avoid accidentally running old binaries.
ACKs for top commit:
theStack:
Light re-ACK 568fcdddae
ryanofsky:
Code review ACK 568fcdddae. Only change since last review was rebasing. I'm ok with this PR in its current form if other developers are happy with it. I just personally think it is inappropriate to \*silently\* break an everyday developer workflow like `git pull; make bitcoind`. I wouldn't have a problem with this PR if it triggered an explicit error, or if the problem was limited to less common workflows like changing cmake options in an existing build.
TheCharlatan:
Re-ACK 568fcdddae
theuni:
ACK 568fcdddae
Tree-SHA512: 1aa5ecd3cd49bd82f1dcc96c8e171d2d19c58aec8dade4bc329df89311f9e50cbf6cf021d004c58a0e1016c375b0fa348ccd52761bcdd179c2d1e61c105e3b9f
fac1dd9dff test: Fix authproxy named args debug logging (MarcoFalke)
Pull request description:
In Python the meaning of `args or argsn` is that `argsn` is fully ignored when `args` is a list with at least one element. However, the RPC server accepts mixed positional and named args in the same RPC.
Fix the debug log by always printing both. Also, add a new `_json_dumps` helper to avoid bloated code.
Can be tested via `--tracerpc` on a call that uses named args mixed with positional args.
ACKs for top commit:
i-am-yuvi:
Tested ACK fac1dd9dff
rkrux:
tACK fac1dd9dff
musaHaruna:
Tested ACK [fac1dd9](fac1dd9dff)
ryanofsky:
Code review ACK fac1dd9dff. Thanks for logging fix. This change should have been included in #19762
Tree-SHA512: ff63fbc2564b2c7589e9294baacf4c7a79f10d593776813392510702ca726e3893a29db3ba261f3aee1789a59bb215d7cb10fc85ca1a02632631d3722ddcdfc5
The translations for the following languages, which appear to be the
result of a mistake or an act of vandalism, have been discarded:
- Czech (cs)
- Danish (da)
- Dutch (nl)
Changes to the Thai (th) translation have been discarded due to multiple
unsolicited pronunciation notes.
c94195c077 doc: add note to windows build about stripping bin (fanquake)
Pull request description:
The Windows binaries are particularly big when they contain debug info, closing in on 500mb. Add a note to the Windows build instructions about using `--strip`.
I haven't tested this (the copying out to WSL). If we don't want to add this note, in favour of [user-presents or similar](https://github.com/bitcoin/bitcoin/issues/30593#issuecomment-2271304490), then we should just close#30593.
Fixes#30593.
ACKs for top commit:
hodlinator:
ACK c94195c077
hebasto:
ACK c94195c077.
Tree-SHA512: c55670486ef60c6bda720e65443e17747b840e220c5bf6d6c0b77590d95cd6c8f040bc0e67dfa8eb11451f4f2eac9faf25d74ea68251b881773836f4113e8595
d79dab0fa9 doc: warn against having qt6 installed on macOS (Sjors Provoost)
Pull request description:
Document #31009 in time for the v29 release.
ACKs for top commit:
achow101:
ACK d79dab0fa9
hebasto:
ACK d79dab0fa9.
Tree-SHA512: 4c6e557b6410c7fd766e1cdc356ae9f7410fbb4746732580e5bdf33ba43dca64e6f2fb66677d1e0c8fa71c19f212dc81ac73dc4277f2fd966bbd41c20d9291f8
611999e097 doc: link to benchcoin over bitcoinperf (fanquake)
Pull request description:
Seems like linking to https://github.com/bitcoin-dev-tools/benchcoin is now the best thing to do here. If not, we can just drop the other links.
ACKs for top commit:
l0rinc:
ACK 611999e097
laanwj:
ACK 611999e097
hebasto:
ACK 611999e097. I agree. I've had a great experience using it.
Tree-SHA512: 558060bec92099befaa047e9192e5172e6a0cdfc5530d1f8b4d64ac717ce999a993d39c5d108fa9df3e30b2fc089e31d720f344153381e7c53f0ed40938ae1e0
