We preemptively perform a block mutation check before further processing
a block message (similar to early sanity checks on other messsage
types). The main reasons for this change are as follows:
- `CBlock::GetHash()` is a foot-gun without a prior mutation check, as
the hash returned only commits to the header but not to the actual
transactions (`CBlock::vtx`) contained in the block.
- We have observed attacks that abused mutated blocks in the past, which
could have been prevented by simply not processing mutated blocks
(e.g. https://github.com/bitcoin/bitcoin/pull/27608).
Github-Pull: #29412
Rebased-From: 49257c0304
This commits fixes a crash bug that can be caused with the following steps:
- change to the "Transactions" view
- right-click on an arbitrary transaction -> "Show transaction details"
- close the transaction detail window again
- select "Settings" -> "Mask values"
The problem is that the list of opened dialogs, tracked in the member
variable `m_opened_dialogs`, is only ever appended with newly opened
transaction detail dialog pointers, but never removed. This leads to
dangling pointers in the list, and if the "Mask values" menu item is
selected, a crash is caused in the course of trying to close the opened
transaction detail dialogs (see `closeOpenedDialogs()` method). Fix this
by removing the pointer from the list if the corresponding widget is
destroyed.
Github-Pull: https://github.com/bitcoin-core/gui/pull/774
Rebased-From: e26e665f9f
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: 68f23f57d7
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: 084d037231
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: 41f9027813
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: bae209e387
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: e14cc8fc69
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: 4660fc82a1
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: cf219f29f3
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: 3eb241a141
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: 1de8a2372a
e.g. sh(addr(ADDR)) or sh(raw(HEX)) are invalid descriptors.
Making sh and wsh top level functions to return addr/raw descriptors when
the subscript inference fails.
Github-Pull: #28067
Rebased-From: cc781a2180
addressbook records with no associated label could be
treated as change. And we don't want that for external
addresses.
Github-Pull: #28038
Rebased-From: a277f8357a
A single outbound slot is required, so if the first two slots
are taken by inbound in-flights, the node will reject additional
unless they are coming from outbound.
This means in the case where a fast sybil peer is attempting to
stall out a node, a single high bandwidth outbound peer can
mitigate the attack.
Github-Pull: #27626
Rebased-From: 03423f8bd1
This is a change in behavior so that if for some reason we request a block from a peer, we don't allow an unsolicited CMPCT_BLOCK announcement for that same block to cause a request for a full block from the uninvited peer (as some type of request is already outstanding from the original peer)
Github-Pull: #27626
Rebased-From: 13f9b20b4c
If transactions are being added to the mempool at a rate faster than 7tx/s
(INVENTORY_BROADCAST_PER_SECOND) then peers' inventory_to_send queue can
become relatively large. If this happens, increase the number of txids
we include in an INV message (normally capped at 35) by 5 for each 1000
txids in the queue.
This will tend to clear a temporary excess out reasonably quickly; an
excess of 4000 invs to send will be cleared down to 1000 in about 30
minutes, while an excess of 20000 invs would be cleared down to 1000 in
about 60 minutes.
Github-Pull: #27610
Rebased-From: 5b3406094f
We use CompareDepthAndScore to choose an order of txs to inv. Rather
than sorting txs that have been evicted from the mempool at the end
of the list, sort them at the beginning so they are removed from
the queue immediately.
Github-Pull: #27610
Rebased-From: 228e9201ef
0d6383fda0 Don't return OutputType::UNKNOWN in ParseOutputType (Pttn)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/issues/27472 by also handling at the relevant places the case where ParseOutputType returns `OutputType::UNKNOWN`, and not just when it returns `std::nullopt`.
ACKs for top commit:
achow101:
ACK 0d6383fda0
MarcoFalke:
lgtm ACK 0d6383fda0
furszy:
ACK 0d6383fda0
Tree-SHA512: 776793027b926283d3162e69fb9c8883c814b19bcce4574ccdf8e3140a1ec4ebc4aa8ccd1abae7ef3571f942d2e6c35305fd1244259540d90605106e01afc34c
`evhttp_uri_parse` can return a nullptr, for example when the URI
contains invalid characters (e.g. "%").
`GetQueryParameterFromUri` passes the output of `evhttp_uri_parse`
straight into `evhttp_uri_get_query`, which means that anyone calling
a REST endpoint in which query parameters are used (e.g. `rest_headers`)
can cause a segfault.
This bugfix is designed to be minimal and without additional behaviour change.
Follow-up work should be done to resolve this in a more general and robust way,
so not every endpoint has to handle it individually.
d52fa1b0a5 tests: Make sure that bumpfee feerate checks work when replacing outputs (Andrew Chow)
be177c15a4 bumpfee: Check the correct feerate when replacing outputs (Andrew Chow)
Pull request description:
When replacing the outputs of a transaction during `bumpfee`, it is possible to accidentally create a transaction that will not be accepted into the mempool as it does not meet the incremental relay fee requirements. This occurs because the size estimation used for checking the provided feerate does not account for the replaced outputs; it instead uses the original outputs. When the replaced outputs is significantly different from the original, there can be a large difference in estimated transaction sizes that can make a transaction miss the absolute fee requirements for the incremental relay fee. Unfortunately we do not currently inform the user when the bumped transaction fails to relay, so they could use `bumpfee` and think the transaction has been bumped when it actually has not.
This issue is resolved by replacing the outputs before doing the size estimation, and also updating the feerate checker to use the actual fee values when calculating the required minimum fee.
Also added a test for this scenario.
ACKs for top commit:
ishaanam:
reACK d52fa1b0a5
Xekyo:
reACK d52fa1b0a5
Tree-SHA512: d18301b587465322dd3fb1bb86496c3675265a56072047576e2baa5cf907dd3b54778f30721f662f0c235709a5568427c18542eb7efbfb6fdd9f481fe676c66b
4258c54f4e Merge bitcoin-core/secp256k1#1276: autotools: Don't regenerate Wycheproof header automatically
06c67dea9f autotools: Don't regenerate Wycheproof header automatically
3bab71cf05 Merge bitcoin-core/secp256k1#1268: release cleanup: bump version after 0.3.1
656c6ea8d8 release cleanup: bump version after 0.3.1
346a053d4c Merge bitcoin-core/secp256k1#1269: changelog: Fix link
6a37b2a5ea changelog: Fix link
ec98fcedd5 Merge bitcoin-core/secp256k1#1266: release: Prepare for 0.3.1
898e1c676e release: Prepare for 0.3.1
1d9a13fc26 changelog: Remove inconsistent newlines
0e091669a1 changelog: Catch up in preparation of 0.3.1
7b7503dac5 Merge bitcoin-core/secp256k1#1245: tests: Add Wycheproof ECDSA vectors
145078c418 Merge bitcoin-core/secp256k1#1118: Add x-only ecmult_const version with x specified as n/d
e5de454609 tests: Add Wycheproof ECDSA vectors
0f8642079b Add exhaustive tests for ecmult_const_xonly
4485926ace Add x-only ecmult_const version for x=n/d
a0f4644f7e Merge bitcoin-core/secp256k1#1252: Make position of * in pointer declarations in include/ consistent
4e682626a3 Merge bitcoin-core/secp256k1#1226: Add CMake instructions to release process
2d51a454fc Merge bitcoin-core/secp256k1#1257: ct: Use volatile "trick" in all fe/scalar cmov implementations
4a496a36fb ct: Use volatile "trick" in all fe/scalar cmov implementations
3d1f430f9f Make position of * in pointer declarations in include/ consistent
2bca0a5cbf Merge bitcoin-core/secp256k1#1241: build: Improve `SECP_TRY_APPEND_DEFAULT_CFLAGS` macro
afd8b23b27 Merge bitcoin-core/secp256k1#1244: Suppress `-Wunused-parameter` when building for coverage analysis
1d8f367515 Merge bitcoin-core/secp256k1#1250: No need to subtract 1 before doing a right shift
3e43041be6 No need to subtract 1 before doing a right shift
3addb4c1e8 build: Improve `SECP_TRY_APPEND_DEFAULT_CFLAGS` macro
0c07c82834 Add CMake instructions to release process
464a9115b4 Merge bitcoin-core/secp256k1#1242: Set ARM ASM symbol visibility to `hidden`
f16a709fd6 Merge bitcoin-core/secp256k1#1247: Apply Checks only in VERIFY mode.
70be3cade5 Merge bitcoin-core/secp256k1#1246: Typo
4ebd82852d Apply Checks only in VERIFY mode.
d1e7ca192d Typo
5bb03c2911 Replace `SECP256K1_ECMULT_TABLE_VERIFY` macro by a function
9c8c4f443c Merge bitcoin-core/secp256k1#1238: build: bump CMake minimum requirement to 3.13
0cf2fb91ef Merge bitcoin-core/secp256k1#1243: build: Ensure no optimization when building for coverage analysis
fd2a408647 Set ARM ASM symbol visibility to `hidden`
4429a8c218 Suppress `-Wunused-parameter` when building for coverage analysis
8e79c7ed11 build: Ensure no optimization when building for coverage analysis
96dd062511 build: bump CMake minimum requirement to 3.13
427bc3cdcf Merge bitcoin-core/secp256k1#1236: Update comment for secp256k1_modinv32_inv256
647f0a5cb1 Update comment for secp256k1_modinv32_inv256
5658209459 Merge bitcoin-core/secp256k1#1228: release cleanup: bump version after 0.3.0
28e63f7ea7 release cleanup: bump version after 0.3.0
git-subtree-dir: src/secp256k1
git-subtree-split: 4258c54f4ebfc09390168e8a43306c46b315134b
