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
bcbf982553aba8107fdb0db413d4b9fe8adc8f17 qt, doc: Remove unneeded comments (Hennadii Stepanov)
9bd1565f6501c81291b286cdfaecd0daf8981c75 qt: Revamp ClientModel code to handle {Block|Header}Tip core signals (Hennadii Stepanov)
48f6d39659e40f44907a7c09f839df988e6c6206 qt: Revamp ClientModel code to handle BannedListChanged core signal (Hennadii Stepanov)
36b12af7eeb571efccd972b2f732a81ae7310066 qt: Revamp ClientModel code to handle AlertChanged core signal (Hennadii Stepanov)
bfe5140c50d16cc995c7da458d38759b68e9cbe6 qt: Revamp ClientModel code to handle NetworkActiveChanged core signal (Hennadii Stepanov)
639563d7fea6b4d65840625dc466eede32d893cf qt: Revamp ClientModel code to handle NumConnectionsChanged core signal (Hennadii Stepanov)
508e2dca5e91c1ff921f01d260fc62f629f1dc9e 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 bcbf982553aba8107fdb0db413d4b9fe8adc8f17
w0xlt:
tACK bcbf982553 on Ubuntu 21.10, Qt 5.15.2.
Tree-SHA512: 35f62b84f916b3ad7442f0fea945d344b3c448878b33506ac7b81fdf5e49bd2a82e12a6927dc91f62c335487bf2305cc45e2f08985303eef31c3ed2dd39e1037
e3daecae0333e5474b54f64619ae6f512b683787 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 e3daecae0333e5474b54f64619ae6f512b683787
hebasto:
ACK e3daecae0333e5474b54f64619ae6f512b683787.
Tree-SHA512: 17b4b891c70f027f6a420be830e61bd87fde5297a4473a5b122e4e34bdf83141635bd5cf5143efe95a0dd6f8cf50bc67a2de6cbfed7956952369587c74ece225
facd1fb911abfc595a3484ee53397eff515d4c40 refactor: Use Span of std::byte in CExtKey::SetSeed (MarcoFalke)
fae1006019188700e0c497a63fc1550fe00ca8bb util: Add ParseHex<std::byte>() helper (MarcoFalke)
fabdf81983e2542d60542b80fb94ccb1acdd204a 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 9394964f6b9d1cf1220a4eca17ba18dc49ae876d
ACKs for top commit:
pk-b2:
ACK facd1fb911
laanwj:
Code review ACK facd1fb911abfc595a3484ee53397eff515d4c40
Tree-SHA512: e2329fbdea2e580bd1618caab31f5d0e59c245a028e1236662858e621929818870b76ab6834f7ac6a46d7874dfec63f498380ad99da6efe4218f720a60e859be
a4703ce9d79855ac0bd7dc07b71a51245f9aa5f8 doc: add release notes about removal of the `deprecatedrpc=exclude_coinbase` (Sebastian Falbesoner)
ef0aa74836c4339aa7f14fc1c9583d86dd5c388a 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 a4703ce9d79855ac0bd7dc07b71a51245f9aa5f8
Tree-SHA512: 97cd4e78501e64f678c78d2ebb5be5376688c023e34fced71dd24e432d27aa31a74b5483545f49ba0bdf48656d8b8b7bee74e3db26cf6daf112613f1caa4dfa4
fafae678f6cd8aaca2be8ece501b258160f7fbfa 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
1d4122dfefcb0a33c3d5bf7bbe2c7cd7e09d3764 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 1d4122dfefcb0a33c3d5bf7bbe2c7cd7e09d3764, only rebased since my recent [review](https://github.com/bitcoin/bitcoin/pull/24830#pullrequestreview-941255672).
Tree-SHA512: 753adfce199ed078a6cd9e0ea78e76c0b14070f8fcfe2a4632cd0c6dfe6b4e135ddffbe11a97e5e30520ea9e5bda00bad1493cbaef74cf425aa8613249167f53
fa305fd92c0a5a91831be3ccec0a5ef962a5fbcb 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 fa305fd92c0a5a91831be3ccec0a5ef962a5fbcb per `git range-diff 7b3343f fa20781 fa305fd`
ajtowns:
ACK fa305fd92c0a5a91831be3ccec0a5ef962a5fbcb
Tree-SHA512: da00200126833c1f55b1b1e68f596eab5c9254e82b188ad17779c08ffd685e198a7c5270791b4b69a858dc6ba4e051fe0c8b445d203d356d0c884f6365ee1cfe
fa9af218780b7960d756db80c57222e5bf2137b1 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 fa9af218780b7960d756db80c57222e5bf2137b1
Tree-SHA512: 284aa2527d0f663ca01550115025c9c64c787531d595f866c718f6ad09b9b0cac1e683a7d77f8009b75de990fd37166b44063ffa83fba8a04e9a31600b4c2725
7171ebc7cbd911fa7ccad732ea7f73bce30928ee 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 7171ebc7cbd911fa7ccad732ea7f73bce30928ee. 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>
bf6526f4a0ab30f77952ecdee90cd77dd9ba06f6 [test] Remove segwit argument from build_block_on_tip() (John Newbery)
c65bf50b44a38bc55224d8967e0df7af60ea4f1b Remove fUseWTXID parameter from CBlockHeaderAndShortTxIDs constructor (John Newbery)
Pull request description:
This implements two of the suggestions from code reviews of PR 20799:
- Remove fUseWTXID parameter from CBlockHeaderAndShortTxIDs constructor
- Remove segwit argument from build_block_on_tip()
ACKs for top commit:
dergoegge:
Code review ACK bf6526f4a0ab30f77952ecdee90cd77dd9ba06f6
naumenkogs:
ACK bf6526f4a0ab30f77952ecdee90cd77dd9ba06f6
Tree-SHA512: d553791d1364b9e655183755e829b195c9b47f59c62371dbae49d9c0f8d84fec58cf18f4dde89591672ef5658e18c9cf0206c2efd70606980f87e506bc3bd4e5
9db82f1bca0bb51c2180ca4a4ad63ba490e79da4 [net processing] Don't initialize TxRelay for non-tx-relay peers. (John Newbery)
b0a4ac9c26f60fd4993d89f45cafffaa389db2d4 [net processing] Add m_tx_relay_mutex to protect m_tx_relay ptr (John Newbery)
290a8dab0288344fa5731ec2ffd09478e9420a2f [net processing] Comment all TxRelay members (John Newbery)
42e3250497b03478d61cd6bfe6cd904de73d57b1 [net processing] [refactor] Move m_next_send_feefilter and m_fee_filter_sent (John Newbery)
Pull request description:
block-relay-only connections are additional outbound connections that bitcoind makes since v0.19. They participate in block relay, but do not propagate transactions or addresses. They were introduced in #15759.
When creating an outbound block-relay-only connection, since we know that we're never going to announce transactions over that connection, we can save on memory usage by not a `TxRelay` data structure for that connection. When receiving an inbound connection, we don't know whether the connection was opened by the peer as block-relay-only or not, and therefore we always construct a `TxRelay` data structure for inbound connections.
However, it is possible to tell whether an inbound connection will ever request that we start announcing transactions to it. The `fRelay` field in the `version` message may be set to `0` to indicate that the peer does not wish to receive transaction announcements. The peer may later request that we start announcing transactions to it by sending a `filterload` or `filterclear` message, **but only if we have offered `NODE_BLOOM` services to that peer**. `NODE_BLOOM` services are disabled by default, and it has been recommended for some time that users not enable `NODE_BLOOM` services on public connections, for privacy and anti-DoS reasons.
Therefore, if we have not offered `NODE_BLOOM` to the peer _and_ it has set `fRelay` to `0`, then we know that it will never request transaction announcements, and that we can save resources by not initializing the `TxRelay` data structure.
ACKs for top commit:
MarcoFalke:
review ACK 9db82f1bca0bb51c2180ca4a4ad63ba490e79da4 🖖
dergoegge:
Code review ACK 9db82f1bca0bb51c2180ca4a4ad63ba490e79da4
naumenkogs:
ACK 9db82f1bca0bb51c2180ca4a4ad63ba490e79da4
Tree-SHA512: 83a449a56cd6bf6ad05369f5ab91516e51b8c471c07ae38c886d51461e942d492ca34ae63d329c46e56d96d0baf59a3e34233e4289868f911db3b567072bdc41
ac6fbf2c83578129a0397d0d0dc9b1c6bdb30701 tidy: use modernize-use-default-member-init (fanquake)
7aa40f55636be565441a9e0af8de0a346bfa4da2 refactor: use C++11 default initializers (fanquake)
Pull request description:
Refactor and then enable [`modernize-use-default-member-init`](https://clang.llvm.org/extra/clang-tidy/checks/modernize-use-default-member-init.html) in our `clang-tidy` job.
Top commit has no ACKs.
Tree-SHA512: 536b406f20639f8c588fe9e96175ec60c7bb825506b2670b562370b2f572801c24203c483443be3c199e1b958c0765d4532e57c57a4e78689162a1dd422d844f
Delay initializing the TxRelay data structure for a peer until we receive
a version message from that peer. At that point we'll know whether it
will ever relay transactions. We only initialize the m_tx_relay
data structure if:
- this isn't an outbound block-relay-only connection; AND
- fRelay=true OR we're offering NODE_BLOOM to this peer
(NODE_BLOOM means that the peer may turn on tx relay later)
This fully comments all the TxRelay members. The only significant change
is to the comment for m_relay_txs. Previously the comment stated that
one of the purposes of the field was that "We don't relay tx invs before
receiving the peer's version message". However, even without the
m_relay_txs flag, we would not send transactions to the peer before
receiving the `version` message, since SendMessages() returns
immediately if fSuccessfullyConnected is not set to true, which only
happens once a `version` and `verack` message have been received.
Move m_next_send_feefilter and m_fee_filter_sent out of the `TxRelay`
data structure. All of the other members of `TxRelay` are related to
sending transactions _to_ the peer, whereas m_fee_filter_sent and
m_next_send_feefilter are both related to receiving transactions _from_
the peer. A node's tx relay behaviour is not always symmetrical (eg a
blocksonly node will ignore incoming transactions, but may still send
out its own transactions), so it doesn't make sense to group the
feefilter sending data with the TxRelay data in a single structure.
This does not change behaviour, since IsBlockOnlyConn() is always equal
to !peer.m_tx_relay. We still don't send feefilter messages to outbound
block-relay-only peers (tested in p2p_feefilter.py).
3a998d2e37bf76667b08cd947807ada1305520d7 Use steady_clock in ConnectAndCallRPC and inline time call in loop conditional (Jon Atack)
3799d2dcdd736dd24850b192e1b264bee1cd5e5a Fix -rpcwait with -netinfo printing negative time durations (Jon Atack)
Pull request description:
- Fix `bitcoin-cli -rpcwait -netinfo 1` returning negative time durations on its first invocation after node startup in the "send", "recv", and "age" columns (potentially the "txn" and "blk" columns also). To reproduce, start bitcoind on mainnet (for a longer startup time) and run `bitcoin-cli -rpcwait -netinfo <n>` where n is 1 or larger. The negative time durations are larger with a slower CPU speed or e.g. higher `checkblocks`/`checklevel` config option settings.
Examples:
```
<-> type net mping ping send recv txn blk hb addrp addrl age id
out manual onion -126 -126 -2 0
ms ms sec sec min min min
```
```
<-> type net mping ping send recv txn blk hb addrp addrl age id
out manual cjdns -64 -64 -1 0
ms ms sec sec min min min
```
```
<-> type net mping ping send recv txn blk hb addrp addrl age id
out manual ipv4 -89 -89 * . -1 0
ms ms sec sec min min min
```
```
<-> type net mping ping send recv txn blk hb addrp addrl age id
out manual ipv6 -133 * . -2 0
ms ms sec sec min min min
```
- Use `steady_clock` in ConnectAndCallRPC and inline the time call in the loop conditional to avoid unnecessary invocations and an unneeded local variable allocation.
ACKs for top commit:
MarcoFalke:
cr ACK 3a998d2e37bf76667b08cd947807ada1305520d7
Tree-SHA512: 141430d47189ad9f646ce8e51cb31c21b395f6294bb27ba9f7ae4c1e1505a63209a4a19662a0b462806437a9cfd07f1ea114e775adc2872d87397fe823f8b8dc
a55db4ea1cf10e0ab4a6eb5cd1dd3bd95626fba0 Add more proper thread safety annotations (Hennadii Stepanov)
8cfe93e3fcf263bf059f738d5e7d9c94901a7c5a Add proper thread safety annotation to `CWallet::GetTxConflicts()` (Hennadii Stepanov)
ca446f2c59720c1575aeeab9c9d636d98ce8528c Add proper thread safety annotation to `CachedTxGetAvailableCredit()` (Hennadii Stepanov)
Pull request description:
In non-test/benchmarking code, there are three cases of the `NO_THREAD_SAFETY_ANALYSIS` annotation which are accompanied with `TODO` comments.
This PR adds proper thread safety annotations instead of `NO_THREAD_SAFETY_ANALYSIS`.
ACKs for top commit:
laanwj:
Code review ACK a55db4ea1cf10e0ab4a6eb5cd1dd3bd95626fba0
Tree-SHA512: 806d72eebc1edf088bfa435c8cd11465be0de6789798dd92abd008425516768acb864a73d834a49d412bb10f7fccfb47473f998cb72739dab6caeef6bcfaf191
ada8358ef54aaa04c9182afe115d8046c801bdde Sanitize port in `addpeeraddress()` (amadeuszpawlik)
Pull request description:
In connection to #22087, it has been [pointed out](https://github.com/bitcoin/bitcoin/pull/22087#pullrequestreview-674786285) that `addpeeraddress` needs to get its port-value sanitized.
ACKs for top commit:
fanquake:
ACK ada8358ef54aaa04c9182afe115d8046c801bdde
Tree-SHA512: 48771cd4f6940aa7840fa23488565c09dea86bd5ec5a5a1fc0374afb4857aebcd2a1f51e2d4cb7348460e0ad9793dc5d2962df457084ed2b8d8142cae650003f
4f31c21b7f384e50e0af8275e831085d1d69b386 bench: Make all arguments -kebab-case (laanwj)
652b54e53220fedf2c342e428617bcbd2022964d bench: Add `--sanity-check` flag, use it in `make check` (laanwj)
Pull request description:
The benchmarks are run as part of `make check` for a crash-sanity check. The actual results are being ignored. So only run them for one iteration.
This makes the `bench_bitcoin` part take 2m00 instead of 5m20 here. Which is still too long (imo), but this needs to be solved in the `WalletLoading*` benchmarks which take that long per iteration.
Also change all `bench_bitcoin` arguments to kebab-case to be consistent with the other tools (in a separate commit).
ACKs for top commit:
jonatack:
ACK 4f31c21b7f384e50e0af8275e831085d1d69b386 on the sanity-check version per `git diff c52a71e 4f31c28` (modulo s/--sanity check/--sanity-check/ in src/bench/bench.cpp::L61)
hebasto:
ACK 4f31c21b7f384e50e0af8275e831085d1d69b386, tested on Ubuntu 22.04.
Tree-SHA512: 2661d130fd82e57c9041755190997a4af588fadddcdd05e04fd024f75da1202480e9feab5764566e8dfe7930e8ae0ec71e93f40ac373274953d274072723980d
Fixes negative time duration values in the "send", "recv",
and "age" columns (potentially the "txn" and "blk" columns also)
for the first run of -rpcwait -netinfo after bitcoind startup.
To reproduce, start bitcoind on mainnet and run
`bitcoin-cli -rpcwait -netinfo <n>` where n is 1 or larger.
The negative times will be larger/more apparent with a slower
CPU speed or e.g. higher checkblocks/checklevel config option
settings.
4c5ceb040cf50d24201903a9200fb23be88d96fb wallet: CreateTransaction(): return out-params as (optional) struct (Sebastian Falbesoner)
c9fdaa5e3ae09b45be6a5c2d4ee6b1e8cef9d8a8 wallet: CreateTransactionInternal(): return out-params as (optional) struct (Sebastian Falbesoner)
Pull request description:
The method `CWallet::CreateTransaction` currently returns several values in the form of out-parameters:
* the actual newly created transaction (`CTransactionRef& tx`)
* its required fee (`CAmount& nFeeRate`)
* the position of the change output (`int& nChangePosInOut`) -- as the name suggests, this is both an in- and out-param
By returning these values in an optional structure (which returns no value a.k.a. `std::nullopt` if an error occured), the interfaces is shorter, cleaner (requested change position is now in-param and can be passed by value) and callers don't have to create dummy variables for results that they are not interested in.
Note that the names of the replaced out-variables were kept in `CreateTransactionInternal` to keep the diff minimal. Also, the fee calculation data (`FeeCalculation& fee_calc_out`) would be another candidate to put into the structure, but `FeeCalculation` is currently an opaque data type in the wallet interface and I think it should stay that way.
As a potential follow-up, I think it would make sense to also do the same refactoring for `CWallet::FundTransaction`, which has a very similar parameter structure.
Suggested by laanwj in https://github.com/bitcoin/bitcoin/pull/20588#issuecomment-739838428.
ACKs for top commit:
achow101:
re-ACK 4c5ceb040cf50d24201903a9200fb23be88d96fb
Xekyo:
ACK 4c5ceb040cf50d24201903a9200fb23be88d96fb
w0xlt:
crACK 4c5ceb040c
Tree-SHA512: 27e5348bbf4f698713002d40c834dcda59c711c93207113e14522fc6d9ae7f4d8edf1ef6d214c5dd62bb52943d342878960ca333728828bf39b645a27d55d524
All uses of CBlockHeaderAndShortTxIDs in the product code are
constructed with fUseWTXID=true, so remove the parameter.
There is one use of the CBlockHeaderAndShortTxIDs constructor with
fUseWTXID=false in the unit tests. This is used to construct a
CBlockHeaderAndShortTxIDs for a block with only the coinbase
transaction, so setting fUseWTXID to true or false makes no difference.
Suggested in https://github.com/bitcoin/bitcoin/pull/20799#pullrequestreview-963480278
The benchmarks are run as part of `make check` for a minimum sanity
check. The actual results are being ignored. So only run them for one
iteration.
This makes the `bench_bitcoin` part take 2m00 instead of 5m20 here.
Which is still too long (imo), but this needs to be solved in the
`WalletLoading*` benchmarks which take that long per iteration.
83003ffe049a432f6fa4127e054f073127e70b90 refactor: replace RecursiveMutex `m_most_recent_block_mutex` with Mutex (Sebastian Falbesoner)
8edd0d31ac683378135a9839e5d4172b82f8f5b8 refactor: reduce scope of lock `m_most_recent_block_mutex` (Sebastian Falbesoner)
Pull request description:
This PR is related to #19303 and gets rid of the RecursiveMutex `m_most_recent_block_mutex`. All of the critical sections (5 in total) only directly access the guarded elements, i.e. it is not possible that within one section another one is called, and we can use a regular Mutex:
b019cdc036/src/net_processing.cpp (L1650-L1655)b019cdc036/src/net_processing.cpp (L1861-L1865)b019cdc036/src/net_processing.cpp (L3149-L3152)b019cdc036/src/net_processing.cpp (L3201-L3206)b019cdc036/src/net_processing.cpp (L4763-L4769)
The scope of the last critical section is reduced in the first commit, in order to avoid calling the non-trivial method `CConnman::PushMessage` while the lock is held.
ACKs for top commit:
furszy:
Code ACK 83003ffe with a small comment.
hebasto:
ACK 83003ffe049a432f6fa4127e054f073127e70b90
w0xlt:
ACK 83003ffe04
Tree-SHA512: 3df290cafd2f6c4d40afb9f14e822a77d9c1828e66f5e2233f3ac1deccc2b0a8290bc5fb8eb992f49d39e887b50bc0e9aad63e05db2d870791a8d409fb95695f
a01b92ad8672dcf4369ee9cf36a6b0157d73786c doc: add release notes about removal of the `deprecatedrpc=softforks` flag (Sebastian Falbesoner)
8c5533c7a953e79b423b465905dbfaa45ad60a49 rpc: remove deprecated "softforks" field from getblockchaininfo (Sebastian Falbesoner)
Pull request description:
Information on soft fork status has been moved from the `getblockchaininfo` RPC to the `getdeploymentinfo` RPC in #23508. The "softfork" result in `getblockchaininfo` was still available for 23.0 with the `-deprecatedrpc=softforks` configuration option, but this can be fully removed now for the next release (24.0).
ACKs for top commit:
shaavan:
ACK a01b92ad8672dcf4369ee9cf36a6b0157d73786c
ajtowns:
ACK a01b92ad8672dcf4369ee9cf36a6b0157d73786c
Tree-SHA512: 692d9d02fdf0b3c18376644a85b24b57efebf612738084c01ef47d47e41861e773688613a808e81f10ab6eec340de00eef96987a1e34d612aaf7f0a0b134d89e
2a22f034ca3298c9f86d1edd4283a0bea18dfbbe parsing external signer master fingerprint string as bytes instead of caring for lower/upper case in ExternalSigner::SignTransaction (avirgovi)
Pull request description:
Some external signers scripts may provide master fingerprint in uppercase format. In that case core will fail with `Signer fingerprint 00000000 does not match any of the inputs` as it only works with lowercase format. Even if the fingerprints match, yet one is lowercase the other uppercase.
ExternalSigner::SignTransaction is the only place where it is needed IMO, as changing it in other places may break the communication with the external signer (i.e. enumerating with lowercase may not find the device).
ACKs for top commit:
achow101:
ACK 2a22f034ca3298c9f86d1edd4283a0bea18dfbbe
theStack:
Code-review ACK 2a22f034ca3298c9f86d1edd4283a0bea18dfbbe
Sjors:
utACK 2a22f034ca3298c9f86d1edd4283a0bea18dfbbe
Tree-SHA512: f3d84b83fb0b5e06c405eaf9bf20a2fa864bf4172fd4de113b80b9b9a525a76f2f8cf63031b480358b3a7666023a2aef131fb89ff50448c66df3ed541da10f99
ba10b90915dae6a802ecb0f80f72a1a9ea5a4c67 Wallet: Ensure m_attaching_chain is set before registering for signals (Luke Dashjr)
Pull request description:
Avoids a race where chainStateFlushed could be called before rescanning began, yet rescan gets interrupted or fails
Followup for #24984 avoiding a race between registering and setting the flag.
ACKs for top commit:
mzumsande:
Code Review ACK ba10b90915dae6a802ecb0f80f72a1a9ea5a4c67
achow101:
ACK ba10b90915dae6a802ecb0f80f72a1a9ea5a4c67
Tree-SHA512: 1d2fa2db189d3e87f2d0863cf2ab62166094436483f0da16760b1083a4743bf08e476a3277e0d36564213d65dd6f0a1fc16a4bf68d3338c991a14d1de9fc0fee
f336ff7f213564909cf5f9742618cc6ec87600fd rpc: avoid expensive `IsMine` calls in `GetReceived` tally (Sebastian Falbesoner)
a7b65af2a4450663d4a20a617c59dac87b34fb36 rpc: avoid scriptPubKey<->CTxDestination conversions in `GetReceived` tally (Sebastian Falbesoner)
Pull request description:
The RPC calls `getreceivedbyaddress`/`getreceivedbylabel` both use the internal helper function `GetReceived` which was introduced in PR #17579 to deduplicate tallying code. For every wallet-related transaction output, the following unnecessary operations are currently performed in the tally loop, leading to a quite bad performance (as reported in #23645):
- converting from CScript -> TxDestination (`ExtractDestination(...)`), converting from TxDestination -> CScript (`CWallet::IsMine(const CTxDestination& dest)`); this can be avoided by directly using output scripts in the search set instead of addresses (first commit)
- checking if the iterated output script belongs to the wallet by calling `IsMine`; this can be avoided by only adding addresses to the search set which fulfil `IsMine` in the first place (second commit)
### Benchmark results
The functional test [wallet_pr23662.py](https://github.com/theStack/bitcoin/blob/pr23662_benchmarks/test/functional/wallet_pr23662.py) (not part of this PR) creates transactions with 15000 different addresses:
- 5000 outputs (500 txs with 10 outputs each) with label set, IsMine set (received)
- 5000 outputs (500 txs with 10 outputs each) with label set, IsMine not set (sent)
- 5000 outputs (500 txs with 10 outputs each) without label set, IsMine not set (sent)
Then, the time is measured for calling `getreceivedbyaddress` and `getreceivedbylabel`, the latter with two variants. Results on my machine:
| branch | `getreceivedbyaddress` (single) | `getreceivedbylabel` (single) | `getreceivedbylabel` (10000) |
|--------------------|---------------------------------|-------------------------------|------------------------------|
| master | 406.13ms | 425.33ms | 446.58ms |
| PR (first commit) | 367.18ms | 365.81ms | 426.33ms |
| PR (second commit) | 3.96ms | 4.83ms | 339.69ms |
Fixes#23645.
ACKs for top commit:
achow101:
ACK f336ff7f213564909cf5f9742618cc6ec87600fd
w0xlt:
ACK f336ff7f21
furszy:
Code ACK f336ff7f
Tree-SHA512: 9cbf402b9e269713bd3feda9e31719d9dca8a0dfd526de12fd3d561711589195d0c50143432c65dae279c4eab90a4fc3f99e29fbc0452fefe05113e92d129b8f
11e79084845a78e2421ea3abafe0de5a54ca2bde prevector: only allow trivially copyable types (Martin Leitner-Ankerl)
Pull request description:
prevector uses `memmove` to move around data, that means it can only be used with types that are trivially copyable. That implies that the types are trivially destructible, thus the checks for `is_trivially_destructible` are not needed.
ACKs for top commit:
w0xlt:
ACK 11e7908484
MarcoFalke:
review ACK 11e79084845a78e2421ea3abafe0de5a54ca2bde 🏯
ajtowns:
ACK 11e79084845a78e2421ea3abafe0de5a54ca2bde -- code review only
Tree-SHA512: cbb4d8bfa095100677874b552d92c324c7d6354fcf7adab2ed52f57bd1793762871798b5288064ed1af2d2903a0ec9dbfec48d99955fc428f18cc28d6840dccc
fae3200bbf2a0f4482ec6f5f2ada9534f6c82709 test: Slim down versionbits_tests.cpp (MacroFake)
Pull request description:
Seems confusing to spin up a full chainman that isn't even used.
Fix that by only spinning up logging. Also, remove the chainman include and comment.
ACKs for top commit:
fanquake:
ACK fae3200bbf2a0f4482ec6f5f2ada9534f6c82709
Tree-SHA512: 35261e9116c0c276f807453db3d635d83916ec2ffd99cf5641f8732736a30a542213096dcec550ef4522d97b3cafe384fdc6068138bc0b577c66fa61256719f8
