e280087946 qt: Use `QRegularExpression` in `AddressBookSortFilterProxyModel` class (Hennadii Stepanov)
5c5d8f2465 qt, test: Add tests for searching in `AddressBookPage` dialog (Hennadii Stepanov)
Pull request description:
This is a step in [migration](https://github.com/bitcoin/bitcoin/pull/24798) to Qt 6.
Related:
- bitcoin-core/gui#578
- bitcoin-core/gui#585
No behavior change. To ensure this, tests have been added.
ACKs for top commit:
hebasto:
> tACK [e280087](e280087946) on Ubuntu 21.10 Qt 5.15.2
promag:
Tested ACK e280087946 with Qt6 on macOS 12 M1.
w0xlt:
tACK e280087946 on Ubuntu 21.10 Qt 5.15.2
jarolrod:
Tested ACK e280087946 on M1 mac, x86 mac, x86 Linux with Qt5 and separately with Qt6
Tree-SHA512: 664baacc1504deb2f7fa651ea4a44f3942f5c9058befe4d2ce292beed032d4b1697710cfd10c0909602d8a4a6eeb680414e4a1f56d2038478c1ae2f34965d74f
31122aa979 refactor: Pass interfaces::Node references to OptionsModel constructor (Ryan Ofsky)
Pull request description:
Giving OptionsModel access to the node interface is needed as part of #602 to get bitcoind and bitcoin-qt to use the same settings instead of different settings.
It has been split off from #602 to simplify that PR. Previously these commits were part of bitcoin/bitcoin#15936 and also had some review discussion there.
ACKs for top commit:
promag:
Code review ACK 31122aa979.
furszy:
Code ACK 31122aa9
jarolrod:
ACK 31122aa979
Tree-SHA512: b8529322fd7ba97e19864129e6cf5f9acc58c124f2e5a7c50aca15772c8549de3c24e8b0c27e8cf2c06fd26529e9cdb898b0788a1de3cbfdfbdd3f85c9f0fe69
a17c5e96b6 Rename NetinfoRequestHandler::is_block_relay data member to is_tx_relay (Jon Atack)
f0bb7db34c Fix frequent -netinfo JSON errors from null getpeerinfo#relaytxes (Jon Atack)
Pull request description:
CLI -netinfo frequently returns "error: JSON value is not a boolean as expected" since the merge of #21160, which moved fRelayTxes (renamed to m_relay_txs in that pull) from CNodeStats to CNodeStateStats.
This change made getpeerinfo "relaytxes" an optional field that can return UniValue IsNull(). It is the only optional field consumed by -netinfo where the latter didn't already handle that case. See also https://github.com/bitcoin/bitcoin/pull/24691.
Also rename the NetinfoRequestHandler::is_block_relay data member to is_tx_relay and inverse its boolean logic. The naming is out of date and incorrect, as lack of request of tx relay does not imply block relay, and a preference for tx relay doesn't imply that block relay isn't happening. Thanks to Marco Falke and Martin Zumsande for their feedback on this.
(I may look at reducing the number of optional node stats fields via refactoring at the net processing level, but ongoing refactoring there may make that slow or complicated and this is a one-line fix that works now.)
ACKs for top commit:
mzumsande:
Code review ACK a17c5e96b6
Tree-SHA512: dc54ce80b78122874a6794555f99e5b328a1574b52bb3e7f974c699c2b759a60ea0807a6483c5bc0414a950d853c0eeeb13dcc1b790d3917c6ee4c9c99fe159f
908fb7e2ec test: Use permissions from git in `lint-files.py` (laanwj)
48d2e80a74 test: Don't use shell=True in `lint-files.py` (laanwj)
Pull request description:
Improvements to the `lint-files.py` script:
- Avoid use of `shell=True`.
- Check the permissions in git's metadata instead of in the filesystem. This stops the umask or filesystem from interfering. It's also more efficient as it only needs a single call to `git ls-files`.
(what triggered this change was `File "..." contains a shebang line, but has the file permission 775 instead of the expected executable permission 755.` errors running the script locally).
ACKs for top commit:
vincenzopalazzo:
re-tACK 908fb7e2ec
Tree-SHA512: 2eaf868c55a9c3508b12658a5b3ac429963fd0551e645332d0ac54be56fefccee95115e4667386df24b46b545593cb0d0bf8c6cecab73f9cb19d37ddf704c614
6fbb0edac2 Set effective_value when initializing a COutput (ishaanam)
Pull request description:
Previously in COutput, effective_value was initialized as the absolute value of the txout and the fee as 0. effective_value along with the fee was calculated outside of the COutput constructor and set after the object had been initialized.
These changes will allow either the fee or the feerate to be passed in a COutput constructor and the fee and effective_value are calculated and set in the constructor. As a result, AvailableCoins also needs to be passed the feerate when utxos are being spent. When balance is calculated or the coins are being listed and feerate is neither available nor required, AvailableCoinsListUnspent is used instead, which runs AvailableCoins while providing the default value for `feerate`. Unit tests for the calculation of effective value have also been added.
ACKs for top commit:
achow101:
re-ACK 6fbb0edac2
Xekyo:
re-ACK 6fbb0edac2
w0xlt:
Code Review ACK 6fbb0edac2
furszy:
Looks good, have been touching this area lately, code review ACK 6fbb0eda.
Tree-SHA512: 5943ee4f4b0c1dcfe146f2fc22853e607259d6d53156b80a8a8f4baa70760a8b25ab822777b7f5d21ecb02dac08bdee704a9a918d5660276d6994b19a284b256
baa3ddc49c doc: add release notes about `getreceivedbylabel` returning an error if the label is not in the address book. (furszy)
8897a21658 rpc: getreceivedbylabel, don't loop over the entire wallet txs map if no destinations were found for the input label. (furszy)
Pull request description:
Built on top of #23662, coming from comment https://github.com/bitcoin/bitcoin/pull/23662#pullrequestreview-971407999.
If `wallet.GetLabelAddresses()` returns an empty vector (the wallet does not have stored destinations with that label in the addressbook) or if none of the returned destinations are from the wallet, we can return the function right away.
Otherwise, we are walking through all the wallet txs + outputs for no reason (`output_scripts` is empty).
ACKs for top commit:
achow101:
ACK baa3ddc49c
theStack:
re-ACK baa3ddc49c
w0xlt:
ACK baa3ddc49c
Tree-SHA512: 00e10365b179bf008da2f3ef8fbb3ee04a330426374020e3f2d0151b16991baba4ef2b944e4659452f3e4d6cb20f128d0918ddf0453933a25a4d9fd8414a1911
71a8dbe5da refactor: Remove defunct attributes.h includes (Ben Woosley)
Pull request description:
Since the removal of NODISCARD in 81d5af42f4,
the only attributes.h def is LIFETIMEBOUND, and it's included in many more
places that it is used.
This removes all includes which do not have an associated use of LIFETIMEBOUND,
and adds it to the following files, due to their use of the same:
* src/validationinterface.h
* src/script/standard.h
See also #20499.
Top commit has no ACKs.
Tree-SHA512: f3e10a5cda5ab78371b77b702f4a241ff69d490a16cc6059f1a4202b97c584accdbc951cc7b6120eae94bee3b9249e9117b45cf6ed1a5228ca23b5638fcf7b7b
252f363f2f qt: Replace `QCoreApplication::quit()` with `QCoreApplication::exit(0)` (Hennadii Stepanov)
Pull request description:
### Qt 5:
- no behavior change.
See https://code.qt.io/cgit/qt/qtbase.git/tree/src/corelib/kernel/qcoreapplication.cpp?h=5.15#n2012:
```cpp
void QCoreApplication::quit()
{
exit(0);
}
```
### Qt 6:
- this change avoids sending a duplicated `QEvent::Quit`
We use `QEvent::Quit` to [handle](https://github.com/bitcoin-core/gui/pull/547) macOS dock menu events. Qt 6 uses `QEvent::Quit` more [widely](89f7a2759c). We do not want a duplicated `QEvent::Quit` which fires `Assert(node.args);` in the [`Shutdown()`](d1b3dfb275/src/init.cpp (L200)) function.
ACKs for top commit:
promag:
Code review ACK 252f363f2f.
Tree-SHA512: 6a04cbcf523c0375158a59b29afadf18da99738c8db8b8728f99319a8cdc10806d2f06dc5a7d3b8b0e1a5f1711be778a75d4ecdefef7cf66e26ae2848f7f57db
9ecb0a3550 doc: remove passing `--disable-external-signer` in OpenBSD build guide (Sebastian Falbesoner)
Pull request description:
Since we have a Boost.Process usage check in the build system (#24254, commit abc057c603), passing the option `--disable-external-signer` explicitly is not needed anymore on OpenBSD; the configure script will automatically detect that including `<boost/process.hpp>` leads to a compile error and disable external signer support accordingly:
```
$ ./configure MAKE=gmake
...
checking whether Boost.Process can be used... no
...
Options used to compile and link:
external signer = no
...
$ ./configure --enable-external-signer MAKE=gmake
...
checking whether Boost.Process can be used... no
configure: error: External signing is not supported for this Boost version
```
The PR basically reverts #22335 but keeps the part mentioning that external signer support is not available on OpenBSD. Also bumps the guide to version 7.1 (released [about a month ago](https://www.openbsd.org/71.html)), where I could verify that the instructions are still accurate.
ACKs for top commit:
fanquake:
ACK 9ecb0a3550
Tree-SHA512: a5f7e89a5a78f062a06e0047802c42ad49d89e0f0afb963886caa684966ea2e9c8a660320eedd98a5aa5eee0a9c2bb8bf7f5772338c4b49738a69c00e9367a15
a63b60f02b refactor: Add OptionsModel getOption/setOption methods (Ryan Ofsky)
Pull request description:
This is a trivial change which is needed as part of #602 to get bitcoind and bitcoin-qt to use the same settings instead of different settings. It is split off from #602 because it causes a lot of rebase conflicts (any time there is a GUI options change).
This PR is very small and easy to review ignoring whitespace: https://github.com/bitcoin-core/gui/pull/600/files?w=1
ACKs for top commit:
vasild:
ACK a63b60f02b
furszy:
Code review ACK a63b60f0
promag:
Code review ACK a63b60f02b.
Tree-SHA512: 1d99a1ce435b4055c1a38d2490702cf5b89bacc7d168c9968a60550bfd9f6dace649d5e98699de68d6305f02d5d1e3eb7e177ab578b98b996dd873b1df0ed236
Since the removal of NODISCARD in 81d5af42f4,
the only attributes def is LIFETIMEBOUND, and it's included in many more
places that it is used.
This removes all includes which do not have an associated use of LIFETIMEBOUND,
and adds it to the following files, due to their use of the same:
* src/validationinterface.h
* src/script/standard.h
Previously in COutput, effective_value was initialized as the absolute
value of the txout, and fee as 0. effective_value along with fee were
calculated outside of the COutput constructor and set after the
object had been initialized. These changes will allow either the fee
or the feerate to be passed in a COutput constructor. If either are
provided, fee and effective_value are calculated and set in the
constructor. As a result, AvailableCoins also needs to be passed the
feerate when utxos are being spent. When balance is calculated or the
coins are being listed and feerate is neither available nor required,
AvailableCoinsListUnspent is used instead, which runs AvailableCoins
while providing the default value for feerate. Unit tests for the
calculation of effective value have also been added.
If wallet.GetLabelAddresses() returns an empty vector (the wallet does not have addresses with that label in the addressbook) or if none of the returned destinations are from the wallet, we can return the function right away.
53494bc739 validation: Have ChainstateManager own m_chainparams (Carl Dong)
04c31c1295 Add ChainstateManager::m_adjusted_time_callback (Carl Dong)
dbe45c34f8 Add ChainstateManagerOpts, using as ::Options (Carl Dong)
Pull request description:
```
This decouples validation.cpp from netaddress.cpp (transitively,
timedata.cpp, and asmap.cpp).
This is important for libbitcoinkernel as:
- There is no reason for the consensus engine to be coupled with
netaddress, timedata, and asmap
- Users of libbitcoinkernel can now easily supply their own
std::function that provides the adjusted time.
See the src/Makefile.am changes for some satisfying removals.
```
Top commit has no ACKs.
Tree-SHA512: a093ec6ecacdc659d656574f05bd31ade6a6cdb64d5a97684f94ae7e55c0e360b78177553d4d1ef40280192674464d029a0d68e96caf8711d9274011172f1330
3f8def51d5 add 3 new test cases for SelectCoins() (akankshakashyap)
Pull request description:
Three new tests have been added.
1. More coins should be selected when effective fee < long term fee.
2. Less coin should be selected when effective fee > long term fee.
3. If a coin is preselected, it should be selected even if disadvantageous.
ACKs for top commit:
achow101:
ACK 3f8def51d5
brunoerg:
ACK 3f8def51d5
Tree-SHA512: 8db6dd942b02a38c99953b801605f98c4c17729768fdfcf7605c5bbdb17509500a39d0a78a4b19aab37812d2994ec7630d2b4e78d1d348f1c27b67588d74e155
We want m_chainparams to be alive for the duration of
ChainstateManager's lifetime since ChainstateManager's behaviour depends
on m_chainparams.
We could allow for a std::shared_ptr to be passed in as m_chainparams,
but that complicates things further. Given that CChainParams is not an
entity class or struct, we can just copy it and have ChainstateManager
own it.
This decouples validation.cpp from netaddress.cpp (transitively,
timedata.cpp, and asmap.cpp).
This is important for libbitcoinkernel as:
- There is no reason for the consensus engine to be coupled with
netaddress, timedata, and asmap
- Users of libbitcoinkernel can now easily supply their own
std::function that provides the adjusted time.
See the src/Makefile.am changes for some satisfying removals.
[META] Although it seems like we don't need it for just one option,
we're going to introduce another member to this struct *in the
next commit*. In future patchsets for libbitcoinkernel decoupling
it from ArgsManager, even more members will be added here.
and inverse its logic.
The naming is out of date and incorrect, as lack of request of tx relay does not
imply block relay, and a preference for tx relay doesn't imply that block relay
isn't happening.
Since the Boost.Process usage check was added to the build system
(commit abc057c603), passing the option
`--disable-external-signer` explicitly is not needed anymore on OpenBSD;
The configure script will automatically detect that including
<boost/process.hpp> leads to a compile error and disable external signer
support accordingly.
fa1b76aeb0 Do not call global Params() when chainman is in scope (MacroFake)
fa30234be8 Do not pass CChainParams& to PeerManager::make (MacroFake)
fafe5c0ca2 Do not pass CChainParams& to BlockAssembler constructor (MacroFake)
faf012b438 Do not pass Consensus::Params& to Chainstate helpers (MacroFake)
fa4ee53dca Do not pass time getter to Chainstate helpers (MacroFake)
Pull request description:
It seems confusing to pass chain params, consensus params, or a time function around when it is not needed.
Fix this by:
* Inlining the passed time getter function. I don't see a use case why this should be mockable.
* Using `chainman.GetConsensus()` or `chainman.GetParams()`, where possible.
ACKs for top commit:
promag:
Code review ACK fa1b76aeb0.
vincenzopalazzo:
ACK fa1b76aeb0
Tree-SHA512: 1abff5cba4b4871d97f17dbcdf67bc9255ff21fa4150a79a74e39b28f0610eab3e7dee24d56872dd6e111f003b55e288958cdd467e6218368d896f191e4ec9cd
"error: JSON value is not a boolean as expected"
due to fRelayTxes/m_relay_txs being moved in PR 21160 from CNodeStats to
CNodeStateStats, which made getpeerinfo#relaytxes an optional field that
can return UniValue IsNull().
Could be verified with
$ ./configure CC=clang CXX=clang++ CXXFLAGS='-Wthread-safety -Wthread-safety-negative'
$ make clean
$ make 2>&1 | grep m_tx_relay_mutex
Could be verified with
$ ./configure CC=clang CXX=clang++ CXXFLAGS='-Wthread-safety -Wthread-safety-negative'
$ make clean
$ make 2>&1 | grep m_most_recent_block_mutex
bcbf982553 qt, doc: Remove unneeded comments (Hennadii Stepanov)
9bd1565f65 qt: Revamp ClientModel code to handle {Block|Header}Tip core signals (Hennadii Stepanov)
48f6d39659 qt: Revamp ClientModel code to handle BannedListChanged core signal (Hennadii Stepanov)
36b12af7ee qt: Revamp ClientModel code to handle AlertChanged core signal (Hennadii Stepanov)
bfe5140c50 qt: Revamp ClientModel code to handle NetworkActiveChanged core signal (Hennadii Stepanov)
639563d7fe qt: Revamp ClientModel code to handle NumConnectionsChanged core signal (Hennadii Stepanov)
508e2dca5e qt: Revamp ClientModel code to handle ShowProgress core signal (Hennadii Stepanov)
Pull request description:
This PR:
- is a pure refactoring with no behavior change
- gets rid of `QMetaObject::invokeMethod()` "dynamic" calls, i.e., without compile-time checks of a called function name and its parameters
- replaces `std::bind`s with lambdas, making parameter permutation (including parameter omitting) explicit
- makes code simpler, more concise, and easier to reason about
Additionally, debug messages have been unified.
ACKs for top commit:
promag:
Code review ACK bcbf982553
w0xlt:
tACK bcbf982553 on Ubuntu 21.10, Qt 5.15.2.
Tree-SHA512: 35f62b84f916b3ad7442f0fea945d344b3c448878b33506ac7b81fdf5e49bd2a82e12a6927dc91f62c335487bf2305cc45e2f08985303eef31c3ed2dd39e1037
e3daecae03 scripted-diff: replace deprecated Q_OS_MAC with Q_OS_MACOS (João Barbosa)
Pull request description:
`Q_OS_MAC` is deprecated but it is also defined when Qt is configured with `-xplatform macx-ios-clang`, and currently it guards some features not available on iOS, like `QProcess`.
ACKs for top commit:
jarolrod:
tACK e3daecae03
hebasto:
ACK e3daecae03.
Tree-SHA512: 17b4b891c70f027f6a420be830e61bd87fde5297a4473a5b122e4e34bdf83141635bd5cf5143efe95a0dd6f8cf50bc67a2de6cbfed7956952369587c74ece225
facd1fb911 refactor: Use Span of std::byte in CExtKey::SetSeed (MarcoFalke)
fae1006019 util: Add ParseHex<std::byte>() helper (MarcoFalke)
fabdf81983 test: Add test for embedded null in hex string (MarcoFalke)
Pull request description:
This adds the hex->`std::byte` helper after the `std::byte`->hex helper was added in commit 9394964f6b
ACKs for top commit:
pk-b2:
ACK facd1fb911
laanwj:
Code review ACK facd1fb911
Tree-SHA512: e2329fbdea2e580bd1618caab31f5d0e59c245a028e1236662858e621929818870b76ab6834f7ac6a46d7874dfec63f498380ad99da6efe4218f720a60e859be
a4703ce9d7 doc: add release notes about removal of the `deprecatedrpc=exclude_coinbase` (Sebastian Falbesoner)
ef0aa74836 rpc: wallet: remove `-deprecatedrpc=exclude_coinbase` logic (Sebastian Falbesoner)
Pull request description:
Including coinbase transactions in `receivedby` RPCs and adding the `-deprecatedrpc=exclude_coinbase` was done in PR #14707 (released in v23.0). For the next release v24.0, this configuration option can be removed.
ACKs for top commit:
fanquake:
ACK a4703ce9d7
Tree-SHA512: 97cd4e78501e64f678c78d2ebb5be5376688c023e34fced71dd24e432d27aa31a74b5483545f49ba0bdf48656d8b8b7bee74e3db26cf6daf112613f1caa4dfa4
fafae678f6 build: Enable RPC_DOC_CHECK on --enable-debug (MacroFake)
Pull request description:
This probably makes no large difference, as the setting is already enabled by default in the functional tests. However, I think it is nice to also enable it in debug builds by default to catch issues while manually testing without the runtime flags specified.
See also https://github.com/bitcoin/bitcoin/issues/24709
ACKs for top commit:
vincenzopalazzo:
utACK fafae678f6
Tree-SHA512: cea3276fc9b5a3bc0f6d9819be9a50b19ecf762729d3e3975abdf00da06beaa3f664b18a826fbab1fedd9143bc0624a95a490bfe40c4b5b0a0f94dbc565ce738
1d4122dfef init: Allow -proxy="" setting values (Ryan Ofsky)
Pull request description:
This drops the `No proxy server specified. Use -proxy=<ip> or -proxy=<ip:port>` error when a empty `-proxy=` command line argument, `bitcoin.conf` value, or `settings.json` value is specified, and just makes bitcoin connect and listen normally in these cases.
The error was originally added in https://github.com/bitcoin/bitcoin/pull/20003 to prevent a bare `-proxy` command line argument with no assignment from clearing proxy settings. But it was implemented in an overbroad way breaking empty `-proxy=` assignments as well.
The motivation for this change is to prevent a GUI bug that happens with https://github.com/bitcoin/bitcoin/pull/15936, reported in https://github.com/bitcoin/bitcoin/pull/15936#pullrequestreview-937685759 by vasild, that happens after a proxy setting is enabled and disabled in the GUI. But this change also makes sense on its own to remove a potentially confusing error message.
ACKs for top commit:
hebasto:
re-ACK 1d4122dfef, only rebased since my recent [review](https://github.com/bitcoin/bitcoin/pull/24830#pullrequestreview-941255672).
Tree-SHA512: 753adfce199ed078a6cd9e0ea78e76c0b14070f8fcfe2a4632cd0c6dfe6b4e135ddffbe11a97e5e30520ea9e5bda00bad1493cbaef74cf425aa8613249167f53
055d94d1ab test: add coverage for unknown network in -onlynet (brunoerg)
Pull request description:
This PR adds test coverage for the following init error by passing an unknown network in -onlynet
0de36941ec/src/init.cpp (L1311)
ACKs for top commit:
MarcoFalke:
rACK 055d94d1ab
Tree-SHA512: 01bbb297afff371f6345889fa04117ff195b68f0bbf934878ba446049791fdbd7d2ce119ee4f9b3616cc0a81330d7055507dc81151acf68532c077f3575258e9
fa305fd92c Add mockable clock type and TicksSinceEpoch helper (MarcoFalke)
Pull request description:
This will be used primarily by the addr time refactor (https://github.com/bitcoin/bitcoin/pull/24697) to make addr relay time type safe. However, it can also be used in other places, and can be reviewed independently, so I split it up.
ACKs for top commit:
jonatack:
ACK fa305fd92c per `git range-diff 7b3343f fa20781 fa305fd`
ajtowns:
ACK fa305fd92c
Tree-SHA512: da00200126833c1f55b1b1e68f596eab5c9254e82b188ad17779c08ffd685e198a7c5270791b4b69a858dc6ba4e051fe0c8b445d203d356d0c884f6365ee1cfe
Will allow OptionsModel to read/write settings to the node settings.json
file and share settings with the node, instead of storing them
externally in QSettings.
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
fa9af21878 scripted-diff: Use getInt<T> over get_int/get_int64 (MacroFake)
Pull request description:
Seems better to see the return type directly and be able to modify it easier, as the return type is used for exceptions (in-range checking and parsing feedback).
ACKs for top commit:
fanquake:
ACK fa9af21878
Tree-SHA512: 284aa2527d0f663ca01550115025c9c64c787531d595f866c718f6ad09b9b0cac1e683a7d77f8009b75de990fd37166b44063ffa83fba8a04e9a31600b4c2725
7171ebc7cb index: Don't commit a best block before indexing it during sync (Martin Zumsande)
Pull request description:
This changes the periodic commit of the best block during the index sync phase to use the already indexed predecessor of the current block index, instead of committing the current one that will only be indexed (by calling `WriteBlock()`) after committing the best block.
The previous code would leave the index database in an inconsistent state until the block is actually indexed - if an unclean shutdown happened at just this point in time, the index could get corrupted because at next startup, we'd assume that we have already indexed this block.
ACKs for top commit:
ryanofsky:
Code review ACK 7171ebc7cb. Looks great! Just commit message changes since last review
Tree-SHA512: a008de511dd6a1731b7fdf6a90add48d1e53f7f7d6402672adb83e362677fc5b9f5cd021d3111728cb41d73f1b9c2140db79d7e183df0ab359cda8c01b0ef928
Committing a block prior to indexing would leave the index database
in an inconsistent state until it is indexed, which could corrupt the
index in case of a unclean shutdown. Thus commit its predecessor.
Co-authored-by: Luke Dashjr <luke-jr+git@utopios.org>
