[25.1] Final backports

fanquake commented at 9:09 am on September 15, 2023: member

Further backports for the 25.x branch. Currently:

fanquake added this to the milestone 25.1 on Sep 15, 2023

DrahtBot commented at 9:10 am on September 15, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	hebasto, dergoegge, willcl-ark

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

DrahtBot added the label Backport on Sep 15, 2023

DrahtBot added the label CI failed on Sep 15, 2023

MarcoFalke commented at 2:37 pm on September 21, 2023: member

unrelated: Looks like tsan seems to be starting to fail some time within the last two weeks.

Do not use std::vector = {} to release memory

Github-Pull: #28452
Rebased-From: 3fcd7fc7ff563bdc0e2bba66b4cbe72d898c876e

2c51a07c08

wallet: disallow migration of invalid or not-watched scripts

The legacy wallet allowed to import any raw script, without checking if
it was valid or not. Appending it to the watch-only set.

This causes a crash in the migration process because we are only
expecting to find valid scripts inside the legacy spkm.

These stored scripts internally map to `ISMINE_NO` (same as if they
weren't stored at all..).

So we need to check for these special case, and take into account that
the legacy spkm could be storing invalid not watched scripts.

Which, in code words, means IsMineInner() returning IsMineResult::INVALID
for them.

Github-Pull: #28125
Rebased-From: 1de8a2372ab39386e689b27d15c4d029be239319

0d2a33e05c

test: wallet, verify migration doesn't crash for an invalid script

The migration process must skip any invalid script inside the legacy
spkm and all the addressbook records linked to them.

These scripts are not being watched by the current wallet, nor should
be watched by the migrated one.

IsMine() returns ISMINE_NO for them.

Github-Pull: #28125
Rebased-From: 8e7e3e614955e60d3bf9e9a481ef8916bf9e22d9

c36770cefd

tx fees, policy: periodically flush fee estimates to fee_estimates.dat

This reduces chances of having old estimates in fee_estimates.dat.

Github-Pull: #27622
Rebased-From: 5b886f2b436eaa8c2b7de58dc4644dc6223040da

c4dd5989b3

tx fees, policy: do not read estimates of old fee_estimates.dat

Old fee estimates could cause transactions to become stuck in the
mempool. This commit prevents the node from using stale estimates
from an old file.

Github-Pull: #27622
Rebased-From: 3eb241a141defa564c94cb95c5bbaf4c5bd9682e

16bb9161fa

tx fees, policy: read stale fee estimates with a regtest-only option

If -acceptstalefeeestimates option is passed stale fee estimates can now
be read when operating in regtest environments.

Additionally, this commit updates all declarations of the CBlockPolicyEstimator
class to include a the second constructor variable.

Github-Pull: #27622
Rebased-From: cf219f29f3c5b41070eaab9a549a476f01990f3a

37764d3300

test: ensure old fee_estimate.dat not read on restart and flushed

This commit adds tests to ensure that old fee_estimates.dat files
are not read and that fee_estimates are periodically flushed to the
fee_estimates.dat file.

Additionaly it tests the -regtestonly option -acceptstalefeeestimates.

Github-Pull: #27622
Rebased-From: d2b39e09bc6a5982fc5cf4b538b7fdb0e3cae576

910c36253e

ci: Nuke Android APK task, Use credits for tsan

Github-Pull: #27834
Rebased-From: fa22538e481fa2c4f0b5d6f91166335e60b67fe9

5e51a9cc72

fanquake force-pushed on Oct 2, 2023

wallet: Check last block and conflict height are valid in MarkConflicted

MarkConflicted calculates conflict confirmations incorrectly when both
the last block processed height and the conflicting height are negative
(i.e. uninitialized). If either are negative, we should not be marking
conflicts and should exit early.

Github-Pull: #28542
Rebased-From: 4660fc82a1f5cf6eb6404d5268beef5919581661

d63478cb50

test: Test loading wallets with conflicts without a chain

Loading a wallet with conflicts without a chain (e.g. wallet tool and
migration) would previously result in an assertion due to -1 being both
a valid number of conflict confirmations, and the indicator that that
member has not been set yet.

Github-Pull: #28542
Rebased-From: 782701ce7d31919dba2241ee43b582d8ae5a2541

b3517cb1b5

fanquake commented at 12:40 pm on October 2, 2023: member

Added #28542. Slight diff in the tests to account for changes to send().

fanquake marked this as ready for review on Oct 2, 2023

fanquake commented at 4:06 pm on October 2, 2023: member

This will hopefully have 1 or 2 additional changes added (see milestone/labels), but marking as ready for review.

build, macos: Fix `qt` package build with new Xcode 15 linker

Github-Pull: #28543
Rebased-From: 79ef528511f0cbbe0a7097ef031f2964aaccfe5c

a6683945ca

dergoegge approved

dergoegge commented at 10:35 am on October 3, 2023: member

ACK a6683945ca38752875e82dddccba075195ec61a6

willcl-ark approved

willcl-ark commented at 12:16 pm on October 3, 2023: contributor

ACK a6683945ca

Verified the diffs between each patch and its Rebased-From patch

hebasto commented at 2:44 pm on October 3, 2023: member

Suggesting to add https://github.com/bitcoin-core/gui/pull/751

depends: fix unusable memory_resource in macos qt build

See https://codereview.qt-project.org/c/qt/qtbase/+/482392.

Github-Pull: #28571
Rebased-From: 848eec09363d1ba8198376eb9654b1a69e3541aa

e270f3f857

gui: macOS, do not process dock icon actions during shutdown

As the 'QMenuBar' is created without a parent window in MacOS, the
app crashes when the user presses the shutdown button and, right
after it, triggers any action in the menu bar.

This happens because the QMenuBar is manually deleted in the
BitcoinGUI destructor but the events attached to it children
actions are not disconnected, so QActions events such us the
'QMenu::aboutToShow' could try to access null pointers.

Instead of guarding every single QAction pointer inside the
QMenu::aboutToShow slot, or manually disconnecting all
registered events in the destructor, we can check if a
shutdown was requested and discard the event.

The 'node' field is a ref whose memory is held by the
main application class, so it is safe to use here. Events
are disconnected prior destructing the main application object.

Furthermore, the 'MacDockIconHandler::dockIconClicked' signal
can make the app crash during shutdown for the very same
reason. The 'show()' call triggers the 'QApplication::focusWindowChanged'
event, which is connected to the 'minimize_action' QAction,
which is also part of the app menu bar, which could no longer exist.

Github-Pull: gui#751
Rebased-From: e14cc8fc69cb3e3a98076fbb23a94eba7873368a

64ffa94231

gui: macOS, make appMenuBar part of the main app window

By moving the appMenuBar destruction responsibility to the QT
framework, we ensure the disconnection of the submenus signals
prior to the destruction of the main app window.

The standalone menu bar may have served a purpose in earlier
versions when it didn't contain actions that directly open
specific screens within the main application window. However,
at present, all the actions within the appMenuBar lead to the
opening of screens within the main app window. So, the absence
of a main app window makes these actions essentially pointless.

Github-Pull: gui#751
Rebased-From: bae209e3879fa099302d3b211362c49bbbfbdd14

f31899d19a

fanquake commented at 2:59 pm on October 3, 2023: member

Suggesting to add https://github.com/bitcoin-core/gui/pull/751

ok

hebasto commented at 3:22 pm on October 3, 2023: member

When backporting #28452 locally, I’ve got the following diff with this PR:

0--- a/src/net.cpp
1+++ b/src/net.cpp
2@@ -35,6 +35,7 @@
3 #include <util/threadinterrupt.h>
4 #include <util/trace.h>
5 #include <util/translation.h>
6+#include <util/vector.h>
7 
8 #ifdef WIN32
9 #include <string.h>

fanquake commented at 3:24 pm on October 3, 2023: member

I’ve got the following diff with this PR:

Yea, because it’s not used in that file in this branch.

fanquake referenced this in commit 97f756b12c on Oct 3, 2023

hebasto approved

hebasto commented at 4:01 pm on October 3, 2023: member

ACK f31899d19a0333e87c3690b3d3c2b574abb357fa, I’ve applied all backports locally and got a zero diff with this PR branch.

DrahtBot requested review from willcl-ark on Oct 3, 2023

DrahtBot requested review from dergoegge on Oct 3, 2023

http: refactor: use encapsulated HTTPRequestTracker

Introduces and uses a HTTPRequestTracker class to keep track of
how many HTTP requests are currently active, so we don't stop the
server before they're all handled.

This has two purposes:
1. In a next commit, allows us to untrack all requests associated
with a connection without running into lifetime issues of the
connection living longer than the request
(see https://github.com/bitcoin/bitcoin/pull/27909#discussion_r1265614783)

2. Improve encapsulation by making the mutex and cv internal members,
and exposing just the WaitUntilEmpty() method that can be safely
used.

Github-Pull: #28551
Rebased-From: 41f9027813f849a9fd6a1479bbb74b9037990c3c

ae86adabe4

http: log connection instead of request count

There is no significant benefit in logging the request count instead
of the connection count. Reduces amount of code and computational
complexity.

Github-Pull: #28551
Rebased-From: 084d0372311e658a486622f720d2b827d8416591

752a456fa8

http: bugfix: track closed connection

It is possible that the client disconnects before the request is
handled. In those cases, evhttp_request_set_on_complete_cb is never
called, which means that on shutdown the server we'll keep waiting
endlessly.

By adding evhttp_connection_set_closecb, libevent automatically
cleans up those dead connections at latest when we shutdown, and
depending on the libevent version already at the moment of remote
client disconnect. In both cases, the bug is fixed.

Github-Pull: #28551
Rebased-From: 68f23f57d77bc172ed39ecdd4d2d5cd5e13cf483

45a5fcb165

fanquake commented at 9:19 am on October 4, 2023: member

This is ready for final review.

hebasto approved

hebasto commented at 9:39 am on October 4, 2023: member

re-ACK 45a5fcb165081f38ef90944dd31c5e3107aa03dc, only #28551 has been backported with since my recent review.

dergoegge approved

dergoegge commented at 10:11 am on October 4, 2023: member

reACK 45a5fcb165081f38ef90944dd31c5e3107aa03dc

willcl-ark approved

willcl-ark commented at 10:16 am on October 4, 2023: contributor

reACK 45a5fcb165081f38ef90944dd31c5e3107aa03dc

Key	Value
Hostname	ubuntu
Arch	x86_64
Kernel	6.2.0-32-generic
System	Linux
CC	Ubuntu clang version 16.0.6
CXX	Ubuntu clang version 16.0.6
Configure	./configure –with-incompatible-bdb –without-miniupnpc –without-natpmp –disable-bench –without-gui –enable-debug CXX=/usr/lib/ccache/clang++ CC=/usr/lib/ccache/clang –disable-shared –with-pic –enable-benchmark=no –enable-module-recovery –disable-module-ecdh –no-create –no-recursion ./config.status ./config.status src/config/bitcoin-config.h

fanquake merged this on Oct 4, 2023

fanquake closed this on Oct 4, 2023

fanquake deleted the branch on Oct 4, 2023

[25.1] Final backports #28487

Reviews