3fa4c54ac54b2d738e0c43b57b5c232ee02fe3b3 [net processing] Pass TxRelay to FindTxForGetData instead of Peer (dergoegge)
c85ee76a3647a42d3a59c28444c5ca52e7db7c5e [net processin] Don't take cs_main in FindTxForGetData (dergoegge)
Pull request description:
Addresses left over feedback from #26140.
* https://github.com/bitcoin/bitcoin/pull/26140#discussion_r1153498543
* https://github.com/bitcoin/bitcoin/pull/26140#discussion_r1153499627
`mapRelay` is only accessed from the message processing thread and does not need to be kept in sync with anything validation specific, it is therfore perfectly fine to have it guarded by `g_msgproc_mutex`.
ACKs for top commit:
jnewbery:
utACK 3fa4c54ac54b2d738e0c43b57b5c232ee02fe3b3
hebasto:
ACK 3fa4c54ac54b2d738e0c43b57b5c232ee02fe3b3, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 3ef84bfe4abfa8d991a7e65d9184221294d80e0df0bbb47f0270ab6ca1593266c98abf83c610f9f86b4d16c7a4b62bcf83f8856c68d3c2e10894bff6ed3e88cd
e414edd8fc88b55a91f446dcc4d55cc4cd6d32e7 qt: Update translation source file (Hennadii Stepanov)
b7800950913c47bc1a939bd7f99c6b0629aea530 qt: Adjust plural forms for translations (Hennadii Stepanov)
6ae8a24009cb006476b3be612a34e4deda30433a GUI: Send: Make feerates translatable (Luke Dashjr)
bd42f5e1cdb0cbd829c8fe1700d036b681f3930d Bugfix: GUI: Send/PSBT: Correct virtual size unit and make translatable (Luke Dashjr)
1b0407f5f1f5902f67b58c7ada9c4b0c45457244 Bugfix: GUI: transactiondesc: Translate outlier "own address" and "watch-only" (Luke Dashjr)
170f3126f220f9ffe3db471e0024a23dede3f90b GUI: Use translated external signer errors for messagebox text (Luke Dashjr)
96989599d6c7264022c188babdcd9cacac9ab69f GUI: Make messages for copying unsigned PSBTs translatable (Luke Dashjr)
08b8b287d3762dd6d4be09fad0b71c4cacfe6658 Bugfix: GUI: Debug info: Use correct "kB" case for small mempool sizes, and make translation-friendly (Luke Dashjr)
dacc322be10c6709338d2f0d7f5c4366609ad1a0 GUI: PSBTOperationsDialog: Support translating window title (Luke Dashjr)
5a4fe5527045ad3d7f1c834c3c6ea1d531c87879 GUI: Intro: Support translating caption of data directory chooser (Luke Dashjr)
3868ba3a27d594dd2968548fff3db457cbeb0080 GUI: Support translating peer network names (Luke Dashjr)
f1f981119884389f1f355bc11847a73812a2836e GUI: Support translating address type dropdown entries (Luke Dashjr)
Pull request description:
This PR updates the `src/qt/locale/bitcoin_en.xlf` translation source file according to [Release schedule for 25.0](https://github.com/bitcoin/bitcoin/issues/26549).
Some translation-related fixes have been picked from https://github.com/bitcoin-core/gui/pull/599 and https://github.com/bitcoin-core/gui/pull/716.
Note for reviewers: it is expected to get a zero diff after running `make -C src translate` locally.
ACKs for top commit:
jarolrod:
ACK e414edd8fc88b55a91f446dcc4d55cc4cd6d32e7
Tree-SHA512: 5b0c70db1e2f5871067e84f43ebea4ee4f0027fc5f2be49bbcb1d04e162ae76607b2b038e9d0622bcb5b3658d0ede8c10c4421ddaa3343f0e0be54315ca7a4f5
1869310f3cfa4ab26b5090d8a4002eefdc84870e refactor: remove unused param from legacy pubkey (Bushstar)
Pull request description:
Unused param present in legacy pubkey manager interface. This param will not be used and should be removed to prevent unintended usage.
ACKs for top commit:
Sjors:
ACK 1869310f3cfa4ab26b5090d8a4002eefdc84870e
furszy:
ACK 1869310f3cfa4ab26b5090d8a4002eefdc84870e
Tree-SHA512: 0fb41fc8f481f859262f2e8e9a93c990c1b4637e74fd9191ccc0b3c523d0e7d94217a3074bb357276e1941a10d29326f850f9b27eccc1eca57cf6b549353400c
Taking cs_main is no longer necessary since we moved
`m_recently_announced_invs` to `Peer` and `mapRelay` is actually only
accessed from the message processing thread.
faf8dc496e761a15956f8226d727f4bbab8dff82 fuzz: Remove legacy int parse fuzz tests (MarcoFalke)
Pull request description:
The fuzz tests checked that the result of the new function was equal to the legacy function. (Side note: The checks were incomplete, as evident by the follow-up fix in commit b5c9bb5cb9f4a8db57b33ef7399310c7d6de5822).
Given that they haven't found any issues in years (beside missing the above issue, that they couldn't catch), it seems time to remove them.
They may come in handy in the rare case that someone would want to modify `LocaleIndependentAtoi()` or `Parse*Int*()`, however that seems unlikely. Also, appropriate checks can be added then.
ACKs for top commit:
fanquake:
ACK faf8dc496e761a15956f8226d727f4bbab8dff82
dergoegge:
ACK faf8dc496e761a15956f8226d727f4bbab8dff82
Tree-SHA512: 4ec88b9fa8ba49a923b0604016f0f471b3c9b9e0ba6c5c3dc4e20503c6994789921e7221d9ec467a2a37a73f21a70ba51ba3370ed5ad311dee989e218290b29a
cd0c8eeb0940790b6ba83786d1c9e362d4dc4829 [net] Pass nRecvFloodSize to CNode (dergoegge)
860402ef2ed728ef096dda4e65e77d566782209f [net] Remove trivial GetConnectionType() getter (dergoegge)
b5a85b365a4abd98176b0935015dbb502cc3e6f6 [net] Delete CNetMessage copy constructor/assignment op (dergoegge)
Pull request description:
Follow-up PR for #27257
* Deletes the copy constructor/assignment operator of `CNetMessage`
* Removes trivial getter for the connection type
* Avoids passing `nRecvFloodSize` to CNode methods by passing it to `CNode` on creation
ACKs for top commit:
jnewbery:
utACK cd0c8eeb0940790b6ba83786d1c9e362d4dc4829
theStack:
ACK cd0c8eeb0940790b6ba83786d1c9e362d4dc4829
Tree-SHA512: 673a758668617f69fba77e61f0eaa1538da27a4849c82c98742436692baa2d7f001129af3e7a66b160e599d12109dac08137a146f10ff9b9ebdc5c2237311d41
9a1d73fdffa4f35e33bc187ac9b3afbba28b1950 Fix segfault when shutdown during wallet open (John Moffett)
Pull request description:
Fixes#689
## Summary
If you open a wallet and send a shutdown signal during that process, you'll get a segfault when the wallet finishes opening. That's because the `WalletController` object gets deleted manually in bitcoin.cpp during shutdown, but copies of the pointer (and pointers to child objects) are dangling in various places and are accessed in queued events after the deletion.
## Details
The issue in #689 is caused by the following sequence of events:
1. Wallet open modal dialog is shown and worker thread does the actual work.
2. Every 200ms, the main event loop checks to see if a shutdown has been requested, but only if a modal is not being shown.
3. Request a shutdown while the modal window is shown.
4. The wallet open process completes, the modal window is dismissed, and various `finish` signals are sent.
5. During handling of one of the `finish` signals, `qApp->processEvents()` is [called](e9262ea32a/src/qt/sendcoinsdialog.cpp (L603)), which causes the main event loop to detect the shutdown (now that the modal window has been dismissed). The `WalletController` and all the `WalletModel`s are [deleted](65de8eeeca/src/qt/bitcoin.cpp (L394-L401)).
6. Control returns to the `finish` method, which eventually tries to send a [signal](e9262ea32a/src/qt/sendcoinsdialog.cpp (L167)) from a wallet model, but it's been deleted already (and the signal is sent from a now-[dangling](d8bdee0fc8/src/qt/walletview.cpp (L65)) pointer).
The simplest fix for that is to change the `qApp->processEvents()` into a `QueuedConnection` call. (The `qApp->processEvents() was a [workaround](https://github.com/bitcoin/bitcoin/pull/593#issuecomment-3050699) to get the GUI to scroll to the last item in a list that just got added, and this is just a safer way of doing that).
However, once that segfault is fixed, another segfault occurs due to some queued wallet events happening after the wallet controller object is deleted here:
65de8eeeca/src/qt/bitcoin.cpp (L394-L401)
Since `m_wallet_controller` is a copy of that pointer in `bitcoingui.cpp`, it's now dangling and `if(null)` checks won't work correctly. For instance, this line:
65de8eeeca/src/qt/bitcoingui.cpp (L413)
sets up a `QueuedConnection` to `setCurrentWallet`, but by the time control reaches that method (one event cycle after shutdown deleted `m_wallet_controller` in `bitcoin.cpp`), the underlying objects have been destroyed (but the pointers are still dangling).
Ideally, we'd use a `QPointer` or `std::shared_ptr / std::weak_ptr`s for these, but the changes would be more involved.
This is a minimal fix for the issues. Just set `m_wallet_controller` to `nullptr` in `bitcoingui.cpp`, check its value in a couple places, and avoid a use of `qApp->processEvents`.
ACKs for top commit:
hebasto:
ACK 9a1d73fdffa4f35e33bc187ac9b3afbba28b1950, I have reviewed the code and it looks OK.
furszy:
ACK 9a1d73fdff
Tree-SHA512: a1b94676eb2fcb7606e68fab443b1565b4122aab93c35382b561842a049f4b43fecc459535370d67a64d6ebc4bcec0ebcda981fff633ebd41bdba6f7093ea540
8c47d599b87d6b2d43e7d37ce0aaf4f541535bb9 doc: improve -debuglogfile help to be a bit clearer (jonatack)
20d89d6802b67e2b8c864bedf23673d008e3faab bench: document expected results in logging benchmarks (jonatack)
d8deba8c36a42481b1c1e73009d7c9cc2cb25f70 bench: add LogPrintfCategory and LogPrintLevel benchmarks (Jon Atack)
102b2033493f0d61e9763d094cb8a0017f7e3a10 bench: order the logging benchmark code by output (Jon Atack)
4b3fdbf6fe20dc70c8dbdaacce161bc4e76b9c84 bench: update logging benchmark naming for clarity (Jon Atack)
4684aa8733e831d7858d43bec4271d5d026ba183 bench: allow logging benchmarks to be order-independent (Larry Ruane)
Pull request description:
Update our logging benchmarks for evaluating ongoing work like #25203 and refactoring proposals like #26619 and #26697.
- make the logging benchmarks order-independent (Larry Ruane)
- add missing benchmarks for the `LogPrintLevel` and `LogPrintfCategory` macros that our logging is migrating to; at some later point it should be feasible to drop some of the previous logging benchmarks
- update the logging benchmark naming to be clear which benchmark corresponds to which log macro, and update the ordering to be the same as the output
- add clarifying documentation to the logging benchmarks
- improve the `-debuglogfile` config option help to be clearer; can be tested by running `./src/bitcoind -help | grep -A4 '\-debuglogfile'`
Reviewers can run the logging benchmarks with:
```bash
./src/bench/bench_bitcoin -filter='LogP*.*'
```
ACKs for top commit:
LarryRuane:
ACK 8c47d599b87d6b2d43e7d37ce0aaf4f541535bb9
martinus:
code review & tested ACK 8c47d599b87d6b2d43e7d37ce0aaf4f541535bb9, here are my benchmark results:
achow101:
ACK 8c47d599b87d6b2d43e7d37ce0aaf4f541535bb9
Tree-SHA512: 705f8720c9ceaf14a1945039c7578a0c17a12215cbc44908099af4ac444561c3f95d833c5a91b325cdd4470737d8a01e2da64db2d542dd7c9a3747fbfdbf213e
3566aa7d495bb783bbd135726238d9f2a9e9f80e [net] Remove CNode friends (dergoegge)
3eac5e7cd1eda6ababb9af9cd72dae08d395993d [net] Add CNode helper for send byte accounting (dergoegge)
60441a3432df10f5d7c15c09c9569f27a793625b scripted-diff: [net] Rename CNode process queue members (dergoegge)
6693c499f7d48aaa1f527a1a860b943fc8442241 [net] Make cs_vProcessMsg a non-recursive mutex (dergoegge)
23d93526545c271b0eed2bd468681864c4213cce [net] Make CNode msg process queue members private (dergoegge)
897e342d6ec320c286753daef01d6eb9839e2c4d [net] Encapsulate CNode message polling (dergoegge)
cc5cdf877666c232a94f03faaf430cbeb6968372 [net] Deduplicate marking received message for processing (dergoegge)
ad44aa5c64d4ee5f31c867fda26350ab560575b7 [net] Add connection type getter to CNode (dergoegge)
Pull request description:
We should define clear interfaces between CNode, CConnman and PeerManager. This PR makes a small step in that direction by ending the friendship of CNode, CConnman and ConnmanTestMsg. CNode's message processing queue is made private in the process and its mutex is turned into a non-recursive mutex.
ACKs for top commit:
jnewbery:
utACK 3566aa7d495bb783bbd135726238d9f2a9e9f80e
vasild:
ACK 3566aa7d495bb783bbd135726238d9f2a9e9f80e
theStack:
re-ACK 3566aa7d495bb783bbd135726238d9f2a9e9f80e
brunoerg:
re-ACK 3566aa7d495bb783bbd135726238d9f2a9e9f80e
Tree-SHA512: 26b87da5054e32401b693b2904e9c5f40e35a53937c0b6cf44b8597034ad07bacf27d87cdffc54d3e7ccfebde4231ef30a38d326f88cc18133bbb34688ead567
f3221d373a8623fe4808e00c7a92fbb6e0d6419b test: add wallet too-long-mempool-chain error coverage (furszy)
acf0119d24c9f8fae825093249a46cd38e4a3a91 wallet: return error msg for too-long-mempool-chain failure (furszy)
Pull request description:
Fixes#23144.
We currently return a general "Insufficient funds" from Coin
Selection when we actually skipped unconfirmed UTXOs that
surpassed the mempool ancestors limit.
This PR make the error clearer by returning:
"Unconfirmed UTXOs are available, but spending them creates
a chain of transactions that will be rejected by the mempool"
Also, added an early return from Coin Selection if the sum of
the discarded coins decreases the available balance below the
target amount.
ACKs for top commit:
achow101:
ACK f3221d373a8623fe4808e00c7a92fbb6e0d6419b
S3RK:
Code review ACK f3221d373a8623fe4808e00c7a92fbb6e0d6419b
Xekyo:
ACK f3221d373a8623fe4808e00c7a92fbb6e0d6419b
Tree-SHA512: 13e5824b75ac302280ff894560a4ebf32a74f32fe49ef8281f2bc99c0104b92cef33d3b143c6e131f3a07eafe64533af7fc60abff585142c134b9d6e531a6a66
fa18504d5767a40dc9827fb081633219bf251001 rpc: Add submit option to generateblock (MarcoFalke)
fab9a08e145dc5a1d9576bf062473f1095b56a16 refactor: Replace block_hash with block_out (MarcoFalke)
Pull request description:
When submit is turned off, a block can be generated and returned as hex, to be used for further tests. For example, it can be submitted on a different node, on a different interface (like p2p), or just never submitted and be used for other testing purposes.
ACKs for top commit:
instagibbs:
ACK fa18504d5767a40dc9827fb081633219bf251001
TheCharlatan:
tACK fa18504d5767a40dc9827fb081633219bf251001
Tree-SHA512: 1b2ab6b71bb7e155c6482d75f5373f4e77de6446cb16bc2dfd19e7a4075b3a6ad87d7ad7a049a9eed934cb71574acfd27202f54c8bb3b03fac869f2e95db7ee5
faf3f12424fa8558e65fa3f1dd3aa1d0eea8604e refactor: Replace GetTimeMicros by SystemClock (MarcoFalke)
Pull request description:
It is unclear from the name that `GetTimeMicros` returns the system time. Also, it is not using the type-safe `std::chrono` types.
Fix both issues by replacing it with `SystemClock` in the only place it is used.
This refactor should not change behavior.
ACKs for top commit:
willcl-ark:
tACK faf3f1242
john-moffett:
ACK faf3f12424fa8558e65fa3f1dd3aa1d0eea8604e changes, but left a comment for the existing code.
Tree-SHA512: 069e6ef26467a469f128b98a4aeb334f141742befd7880cb3a7d280480e9f0684dc0686fa6a828cdcb3d11943ae5c7f8ad5d9d9dab4c668be85e5d28c78cd489
fae349076db03ddfbf23c5d828368d538b5d52d5 test: Remove unused Check* default constructors (MarcoFalke)
Pull request description:
They are no longer needed after the removal of `swap`, see https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1144532693
Also, flatten a redundant `if` check.
ACKs for top commit:
hebasto:
ACK fae349076db03ddfbf23c5d828368d538b5d52d5
Tree-SHA512: c0bc0c16b5df0f16fc25e18d2414a2a3c4769da1aa30d53f8d267bc2e97dd79a0296db94c1e49cd1ca89bd42275d8c462f7bf47f03f105dfe867ebea6563454b
