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>
Wait until the node's process has fully stopped before starting a new instance.
Since the same code is used in tool_wallet.py, this consolidates the behavior
into a 'kill_process()' function.
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
For the coinbase vTxFees used a dummy value of -nFees. This
value was never returned by the RPC or used in a test.
Similarly the fist vTxSigOpsCost entry was calculated from
the dummy coinbase transaction.
Drop both and add code comments to prevent confusion.
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
Calls on the tx.calc_sha256 method can be confusing, as they return
the result (either txid or wtxid, depending on the with_witness
boolean parameter) as integer rather than as actual (w)txid. Use
.rehash() and .getwtxid() instead to improve readability and in some
cases avoid a conversion from string-txid to an integer.
In the functional test framework, we often treat hashes
as uint256 integers, which seems to be confusing and for no
good reason, as hashes are just sequences of bytes. This commit
gets rid of obvious internal instances of that where individual
functional tests are not affected. In the long-term, it might make
sense to store other hashes (mostly txids) as actual bytes to
avoid annoying conversions and improve code readability.
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.
for consistency with the other CLI commands (-netinfo, -addrinfo, -generate).
This can be considered a bugfix because IsArgSet() returns whether an arg has
been set even if it has been negated. After this change, we no longer treat
-nogetinfo and -getinfo=0 the same as -getinfo and -getinfo=1, and instead as if
-getinfo was not specified.
Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
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
GCC 14.2.1 will complain about a dangling reference after replacing Span
wiht std::span. This is a false-positive, because std::find does not
return a reference.
Remove the `&` to silence the warning. Also use ranges::find while
touching the line.
src/i2p.cpp:312:21: error: possibly dangling reference to a temporary [-Werror=dangling-reference]
312 | const auto& pos = std::find(kv.begin(), kv.end(), '=');
| ^~~
src/i2p.cpp:312:36: note: the temporary was destroyed at the end of the full expression ‘std::find<__gnu_cxx::__normal_iterator<const char*, span<const char> >, char>((& kv)->std::span<const char>::begin(), (& kv)->std::span<const char>::end(), '=')’
312 | const auto& pos = std::find(kv.begin(), kv.end(), '=');
| ~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors
Historically, the headers have been bumped some time after a file has
been touched. Do it now to avoid having to touch them again in the
future for that reason.
-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended 's;( 20[0-2][0-9])(-20[0-2][0-9])? The Bitcoin Core developers;\1-present The Bitcoin Core developers;g' $( git show --pretty="" --name-only HEAD~1 )
-END VERIFY SCRIPT-
This uses a macro, which can be a bit more brittle than an alias
template. However, class template argument deduction for alias templates
is only implemented in clang-19.
* The comment is wrong claiming that void* was returned when void was
returned in reality.
* The namespace is missing a name, leading to compile errors that are
suppressed with non-standard pragmas, and leading to compile errors in
future commits. Instead of using more non-standard suppressions, just
add the missing name.
* The SpanableYes/No types are missing begin/end iterators, which will
be needed when using std::span.
In theory this commit should only touch the span.h header, because
std::span can implicilty convert into Span in most places, if needed.
However, at least when using the clang compiler, there are some
false-positive lifetimebound warnings and some implicit conversions can
not be resolved.
Thus, this refactoring commit also changed the affected places to
replace Span with std::span.
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>
