bfcd60f5d5 test: activate all index types in feature_init.py (Martin Zumsande)
0243907fae index: Don't commit without valid m_best_block_index (Martin Zumsande)
Pull request description:
When an index thread receives an interrupt during init before it got to index anything (so `m_best_block_index == nullptr` still), it will still try to commit previous "work" before stopping the thread. That means that `BaseIndex::CommitInternal()` calls `GetLocator(nullptr)`, which returns an locator to the tip ([code](06b6369766/src/chain.cpp (L31-L32))), and saves it to the index DB.
On the next startup, this locator will be read and it will be assumed that we have successfully synced the index to the tip, when in reality we have indexed nothing.
In the case of coinstatsindex, this would lead to a shutdown of bitcoind without any indication what went wrong. For the other indexes, there would be no immediate shutdown, but the index would be corrupt.
This PR fixes this by not committing when `m_best_block_index==nullptr`, and it also adds an error log message to the silent coinstatsindex shutdown path.
This is another small bug found by `feature_init.py` - the second commit enables blockfilterindex and coinstatsindex for this test, enabling coinstatsindex without the first commit would have led to frequent failures.
ACKs for top commit:
fjahr:
reACK bfcd60f5d5
shaavan:
reACK bfcd60f5d5
Tree-SHA512: 8e2bac0fc40cde209518a9e59b597ae0a5a875a2a90898673987c91733718d40e528dada942bf552b58bc021bf46e59da2d0cc5a61045f48f9bae2b1baf6033b
fa455975e5 util: Add missing unlinkat to syscall sandbox (MarcoFalke)
Pull request description:
This will be needed for g++-12 (after libstdc++6 12-20220206).
Steps to reproduce:
```
gdb --args ./src/bitcoind -sandbox=log-and-abort -regtest
./src/bitcoin-cli -regtest -named createwallet wallet_name=a descriptors=false
./src/bitcoin-cli -regtest stop
```
BT:
```
Thread 1 "b-shutoff" received signal SIGSYS, Bad system call.
0x00007ffff79564f7 in unlinkat () from /lib/x86_64-linux-gnu/libc.so.6
(gdb) bt
#0 0x00007ffff79564f7 in unlinkat () from /lib/x86_64-linux-gnu/libc.so.6
#1 0x00007ffff7cc7335 in ?? () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
#2 0x00007ffff7cc94e3 in std::filesystem::remove_all(std::filesystem::__cxx11::path const&) () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
#3 0x00005555559d4918 in wallet::BerkeleyEnvironment::Flush (this=0x7fffc4005160, fShutdown=<optimized out>) at /usr/include/c++/12/bits/fs_path.h:595
#4 0x000055555592c058 in wallet::StopWallets (context=...) at /usr/include/c++/12/bits/shared_ptr_base.h:1665
#5 0x00005555556617ca in Shutdown (node=...) at ./src/init.cpp:293
#6 0x000055555563ada6 in AppInit (argv=<optimized out>, argc=<optimized out>, node=...) at ./src/bitcoind.cpp:249
#7 main (argc=<optimized out>, argv=<optimized out>) at ./src/bitcoind.cpp:273
ACKs for top commit:
laanwj:
Code review ACK fa455975e5
Tree-SHA512: e80a38828f8656040954c9befa2d1c9d5170e204dc09c61031633349897f51ccd85cc5c99a089c4726d7f5237875cd9ed3fa8ef864cd6c1c8a2b8250b392d57f
f730bd7d58 scripted-diff: Rename ShowModalDialogAndDeleteOnClose (Hennadii Stepanov)
5d7666b151 qt: Revert 7fa91e8312 partially (Hennadii Stepanov)
89c277a6fc qt: Delay shutdown while a modal dialog is active (Hennadii Stepanov)
8c0eb80f41 qt: Disable tray icon menu when a modal dialog is active (Hennadii Stepanov)
92427354dd qt, refactor: Use local QAction instances for the tray icon menu (Hennadii Stepanov)
58e16035c1 qt, refactor: Drop BitcoinGUI::{send,receive}CoinsMenuAction members (Hennadii Stepanov)
fd667e73cd qt: Make show_hide_action dependent on the main window actual state (Hennadii Stepanov)
ee151d0327 qt: Drop BitcoinGUI::toggleHideAction member (Hennadii Stepanov)
78189daac8 qt, refactor: Fill up trayIconMenu before connections (Hennadii Stepanov)
66afa286e5 qt, refactor: Replace BitcoinGUI::trayIconActivated with a lambda (Hennadii Stepanov)
c3ca8364b2 qt, refactor: Replace BitcoinGUI::macosDockIconActivated with a lambda (Hennadii Stepanov)
Pull request description:
As pointed in bitcoin/bitcoin#23790 a regression in wallet unlock was introduced in bitcoin-core/gui#336 when a synchronous `AskPassphraseDialog` has been replaced with an asynchronous one.
This PR reverts a call back to a synchronous mode.
To make synchronous dialogs behave nice during shutdown some additional changes were made.
Please note that disabling the tray icon menu when a modal dialog is active is useful itself as on master (4ad59042b3) it is possible to switch to the "Receive" tab while the GUI is waiting for a password for the "Send" tab:
![Screenshot from 2021-12-17 18-59-51](https://user-images.githubusercontent.com/32963518/146580710-0a755f24-a166-414b-be60-7863232ac778.png)
This is confusing and must be avoided.
Fixesbitcoin/bitcoin#23790.
ACKs for top commit:
prayank23:
tACK f730bd7d58
Tree-SHA512: 2b68275754190e4a9831b96e882d3c5b005e03909aeb6f2c5846da07199bb3efbb74ce87a9d25bb139f643c43d377a2051b221d553281fa5aefdd3181a58077f
This comment described how the constructor of CFeeRate was previously
indirectly used to parse fee rate arguments from RPCs. The command line
input was actually in sat/vB but due to the use of AmountFromValue() it
got converted to BTC/vB. In the constructor this could be rectified by
creating a CFeeRate from that given value (in BTC/vB) and COIN as the
transaction size, turning the input effectively to sat/vB. Since this
usage pattern was removed from the codebase some months ago, the comment
is now obsolete.
Also:
• Use vsize and vbyte instead of size and byte
...to make sure that the linker errors that arise from coupling
regressions are caught by CI.
Adding to the "no wallet" ci job as suggested by MarcoFalke.
The bitcoin-chainstate executable serves to surface the dependencies
required by a program wishing to use Bitcoin Core's consensus engine as
it is right now.
More broadly, the _SOURCES list serves as a guiding "North Star" for the
libbitcoinkernel project: as we decouple more and more modules of the
codebase from our consensus engine, this _SOURCES list will grow shorter
and shorter. One day, only what is critical to our consensus engine will
remain. Right now, it's "the minimal list of files to link in to even
use our consensus engine".
[META] In a future commit the libbitcoinkernel library will be extracted
from bitcoin-chainstate, and the libbitcoinkernel library's
_SOURCES list will be the list that we aim to shrink.
34d0e07e92 Test that OP_1-OP_16 (but not lower/higher) start witness programs (Pieter Wuille)
Pull request description:
Cherry-picks one of the commits adding test coverage from #13062. As [pointed out by aj](https://github.com/bitcoin/bitcoin/pull/13062/files#r492723037):
> could move the test additions to the first commit, since they're testing things that are already true
Pull the additional test code into master earlier.
ACKs for top commit:
laanwj:
Code review ACK 34d0e07e92
Tree-SHA512: ff0ab2a54613ea6e8246b443363b362dd41b5e464faba4d11be6003aa6588a626cf56e142a3b94465cd37dd3ac4debea08455db96bade336171b6c30ea894950
fa6065661a refactor: Avoid unsigned integer overflow in core_write (MarcoFalke)
Pull request description:
Also, I find the new code a bit easier to understand.
ACKs for top commit:
shaavan:
Code Review ACK fa6065661a
Tree-SHA512: cd751e3b4dc97ef525eb8be8d0a49e9629389cb114df18d59a06e05388822af2939078e937f01494e6b317d601743b1a433ba47aa40c4dc602372d1f0fd0dc11
-BEGIN VERIFY SCRIPT-
sed -i 's/ShowModalDialogAndDeleteOnClose/ShowModalDialogAsynchronously/' -- $(git grep -l -e "ShowModalDialogAndDeleteOnClose")
-END VERIFY SCRIPT-
It is important to highlight that a modal dialog is showed
asynchronously as there are cases when the synchronous QDialog::exec()
is required.
The AskPassphraseDialog modal dialog must be synchronous here as
expected in the WalletModel::requestUnlock() function.
Fixed an introduced regression.
b75f4c89ec RPC: Return external_signer in getwalletinfo (Kristaps Kaupe)
Pull request description:
Add `external_signer` to the result object of `getwalletinfo` RPC which indicates whether `WALLET_FLAG_EXTERNAL_SIGNER` flag is set for the wallet.
ACKs for top commit:
S3RK:
utACK b75f4c89ec
achow101:
ACK b75f4c89ec
prayank23:
utACK b75f4c89ec
brunoerg:
utACK b75f4c89ec
Tree-SHA512: 066ccb97541fd4dc3d9728834645db714a3c8c93ccf29142811af4d79cfb9440a97bbb6c845434a909bc6e1775ef3737fcbb368c1f0582bc63973f6deb17a45f
ee822d85d6 util: use stronger-guarantee rename method (Vasil Dimov)
Pull request description:
Use std::filesystem::rename() instead of std::rename(). We rely on the
destination to be overwritten if it exists, but std::rename()'s behavior
is implementation-defined in this case.
This is a rebase of #20435 by vasild.
ACKs for top commit:
MarcoFalke:
review ACK ee822d85d6
hebasto:
Approach ACK ee822d85d6.
vasild:
ACK ee822d85d6
Tree-SHA512: 8f65f154d235c2704f18008d9d40ced3c5d84e4d55653aa70bde345066b6083c84667b5a2f4d69eeaad0bec6c607645e21ddd2bf85617bdec4a2e33752e2059a
In `PeerManagerImpl::RelayAddress()` we used just the hash of the
address that is being relayed to decide where to relay it to. Include
the port in that hash, so that e.g. `1.1.1.1:5555` and `1.1.1.1:6666`
get relayed to different peers. Those are two different, distinct
services after all.
This new constructor will be useful if we just want to hash a `CService`
object without the two `GetRand()` calls (in `RelayAddress()` in a
subsequent commit).
By default, for mainnet, the p2p listening port is 8333. Bitcoin Core
has a strong preference for only connecting to nodes that listen on that
port.
Remove that preference because connections over clearnet that involve
port 8333 make it easy to detect, analyze, block or divert Bitcoin p2p
traffic before the connection is even established (at TCP SYN time).
For further justification see the OP of:
https://github.com/bitcoin/bitcoin/pull/23306
0c49e52b22 build: remove unneeded getentropy detection (HAVE_GETENTROPY) (Sebastian Falbesoner)
5cd15ffdce random: use arc4random on OpenBSD (Sebastian Falbesoner)
Pull request description:
Inspired by a discussion on obtaining randomness on various OSes in a secp256k1 PR (https://github.com/bitcoin-core/secp256k1/pull/748#discussion_r524605472, see also https://bitcoincore.reviews/libsecp256k1-748), I think it makes sense to follow best practices and use `arc4random_buf` rather than `getentropy` on OpenBSD in our random module.
The [getentropy(2) man page](https://man.openbsd.org/getentropy.2) states:
```
getentropy() is not intended for regular code; please use the
arc4random(3) family of functions instead.
```
The [arc4random(3) man page](https://man.openbsd.org/arc4random.3) states:
```
Use of these functions is encouraged for almost all random number
consumption because the other interfaces are deficient in either quality,
portability, standardization, or availability.
```
On the linked PR discussion worries about using RC4 internally has been expressed (see https://security.stackexchange.com/questions/85601/is-arc4random-secure-enough/172905#172905), but this would only affect users of OpenBSD <5.5, using a version that was released more than 8 years ago.
ACKs for top commit:
laanwj:
Tested ACK 0c49e52b22
Tree-SHA512: b5ed3d0718962c5a3839db9a28f93d08a0ac93094cc664f83bc4cf1cfad25049e6240b7b81fe06b71e6a3a0ca24a2c337eab088abec5470ad014e10c04fdb216
Use std::filesystem::rename() instead of std::rename(). We rely on the
destination to be overwritten if it exists, but std::rename()'s behavior
is implementation-defined in this case.
fac9fe5d05 Fix unintended unsigned integer overflow in strencodings (MarcoFalke)
Pull request description:
This fixes two issues for strings that start with a colon and only have one colon:
* `fMultiColon` is incorrectly set to `true`
* There is an unsigned integer overflow `colon - 1` (`0 - 1`)
Neither issue matters, as the result is discarded. Though, it makes sense to still fix the issue for clarity and to avoid sanitizer issues in the function.
ACKs for top commit:
laanwj:
Code review ACK fac9fe5d05
shaavan:
Code Review ACK fac9fe5d05
Tree-SHA512: e71c21a0b617abf241e561ce6b90b963e2d5e2f77bd9547ce47209a1a94b454384391f86ef5d35fedd4f4df19add3896bb3d61fed396ebba8e864e3eeb75ed59
fa2f7d0059 fuzz: Avoid unsigned integer overflow in FormatParagraph (MarcoFalke)
Pull request description:
`FormatParagraph` is only ever called with compile time constant arguments, so I don't see the need for fuzzing it.
Though, keep it for now, but avoid the unsigned integer overflow with this patch.
ACKs for top commit:
laanwj:
Code review ACK fa2f7d0059
Tree-SHA512: 01fc64a9ef73c183921ca1b0cd8db9514c0a242e3acf215a3393f383ae129e01625ebb16eaf9cb86370eda62d0145c3dcf8f62e40edf5958abc1f777c5687280