backport: bitcoin#18160, #18454, #18487, #18524, #18532, #18546, #18551, #18561, #18563, #18587 #5479
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
knst
requested review from
PastaPastaPasta and
UdjinM6
and removed request for
UdjinM6
knst
force-pushed
the
bc-bp-v20-missing-9
branch
from
b79e955 to
1fd44d9
Compare
|
This pull request has conflicts, please rebase. |
knst
force-pushed
the
bc-bp-v20-missing-9
branch
from
1fd44d9 to
e36bd1a
Compare
Member
|
conflicts with #5492 |
PastaPastaPasta
requested changes
Jul 24, 2023
Member
PastaPastaPasta
left a comment
PastaPastaPasta
left a comment
There was a problem hiding this comment.
re-request when not blocked by other PR
|
This pull request has conflicts, please rebase. |
knst
force-pushed
the
bc-bp-v20-missing-9
branch
from
e36bd1a to
481e08a
Compare
knst
force-pushed
the
bc-bp-v20-missing-9
branch
from
481e08a to
0c129ee
Compare
PastaPastaPasta
approved these changes
Aug 3, 2023
Member
PastaPastaPasta
left a comment
PastaPastaPasta
left a comment
There was a problem hiding this comment.
utACK for merging via merge commit
…llBalanceChanged 0933a37 gui: Avoid Wallet::GetBalance in WalletModel::pollBalanceChanged (João Barbosa) Pull request description: Each 250ms the slot `WalletModel::pollBalanceChanged` is called which, at worst case, calls `Wallet::GetBalance`. This is a waste of resources since most of the time there aren't new transactions or new blocks. Fix this by early checking if cache is dirty or not. The actual balance computation can still hang the GUI thread but that is tracked in bitcoin#16874 and should be fixed with a solution similar to bitcoin#17135. ACKs for top commit: hebasto: ACK 0933a37, I have not tested the code, but I have reviewed it and it looks OK, I agree it can be merged. jonasschnelli: utACK 0933a37 instagibbs: ACK 0933a37 ryanofsky: Code review ACK 0933a37, but I would prefer (not strongly) for bitcoin#17905 to be merged first. This PR can be simpler if it is based on bitcoin#17905, so tryGetBalances can just be left alone instead of changing into to a more complicated tryGetBalancesIfNeeded function, and then getting changed back later when we want to optimize it out. jonatack: ACK 0933a37 based primarily on code review, despite a lot of manual testing with a large 177MB wallet. Tree-SHA512: 18db35bf33a7577666658c8cb0b57308c8474baa5ea95bf1468cd8531a69857d8915584f6ac505874717aa6aabeb1b506ac77630f8acdb6651afab89275e38a1
…Model::pollBalanceChanged d3a56be Revert "gui: Avoid Wallet::GetBalance in WalletModel::pollBalanceChanged" (Russell Yanofsky) bf0a510 gui: Avoid wallet tryGetBalances calls before TransactionChanged or BlockTip notifications (Russell Yanofsky) 2bc9b92 Cancel wallet balance timer when shutdown requested (Russell Yanofsky) 83f69fa Switch transaction table to use wallet height not node height (Russell Yanofsky) Pull request description: Main commit `gui: Avoid wallet tryGetBalances calls` is one-line change to `WalletModel::pollBalanceChanged` that returns early if there hasn't been a new `TransactionChanged` or `BlockTip` notification since the previous poll call. This is the same behavior that was implemented in bitcoin#18160, now implemented in a simpler way. The other commits are a straight revert of bitcoin#18160, and two tweaks to avoid relying on `WalletModel::m_client_model` lifetime which were causing travis failures with earlier versions of this PR. Motivation for this change is to be able to revert bitcoin#18160 and cut down on unnecessary cross-process calls that happen when bitcoin#18160 is combined with bitcoin#10102 This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10).# This is a combination of 2 commits.
…rface d6815a2 refactor: drop boost::signals2 in validationinterface (Russell Yanofsky) Pull request description: Stop using boost::signals2 internally in validationinterface. Replace with std::list and Add/Remove/Clear/Iterate helper functions. Motivation for change is to reduce dependencies and avoid issues happening with boost versions before 1.59: bitcoin#18517, bitcoin#18471 ACKs for top commit: MarcoFalke: ACK d6815a2 laanwj: ACK d6815a2 hebasto: re-ACK d6815a2 promag: ACK d6815a2. Tree-SHA512: 4fc0f14a8446e8616cc142af6c3d36815f3254525d30348ba8e4d4bc74c249a5a8c9bc119bdd1be7ebd7abe0b784bc0c5551a3e156a766890cb2fdd891a95919
…xecuted 2276339 Add test for UnregisterAllValidationInterfaces bug (Russell Yanofsky) 3c61abb Do not clear validationinterface entries being executed (Pieter Wuille) Pull request description: The previous code for MainSignalsInstance::Clear would decrement the reference count of every interface, including ones that were already Unregister()ed but still being executed. This fixes the issue pointed out here: https://github.com/bitcoin/bitcoin/pull/18524/files#r404395685 . It's not currently observable. ACKs for top commit: jonasschnelli: utACK 2276339 - reviewed code and test (thanks @ryanofsky for adding the test). MarcoFalke: ACK 2276339 🎎 ryanofsky: Code review ACK 2276339. No change to bugfix, just rebased and new test commit added since last review Tree-SHA512: c1d68e7c681a45c6cadc84e407c2266bcb4b12d34264e1232a61c4eadb74b551231c5a3b1d041de39f507aef4dfa7d4589b8bfe1833f069c739c6270d2a05dbe
7b8e157 rpc: Fix rpcRunLater race in walletpassphrase (João Barbosa) Pull request description: Release locks before calling `rpcRunLater`. Quick explanation: `rpcRunLater` leads to `event_free` which calls `event_del` which can wait for the event callback to finish if it's already running and that callback will try to lock wallet mutex - which is already locked in http thread. Fixes bitcoin#14995 , fixes bitcoin#18482. Best reviewed with whitespace changes hidden. ACKs for top commit: MarcoFalke: ACK 7b8e157, only tested that this avoids the node freezing. Did not look at how libevent works or how the deadlock happens or if this breaks other stuff. 📞 ryanofsky: Code review ACK 7b8e157. Just updated comment since last review Tree-SHA512: 17874a2fa7b0e164fb0d7ee4cb7d59650275b8c03476fb291d60af8b758495457660d3912623fb26259fefe84aeba21c0a9e0c6467982ba511f19344ed5413ab
added 5 commits
August 3, 2023 11:16
…ddress book [part 2] 7a2ecf1 Wallet: Change IsMine check in CWallet::DelAddressBook from assert to failure (Luke Dashjr) 2952c46 Wallet: Replace CAddressBookData.name with GetLabel() method (Luke Dashjr) d7092c3 QA: Test that change doesn't turn into non-change when spent in an avoid-reuse wallet (Luke Dashjr) Pull request description: Follow-up to bitcoin#18192, not strictly necessary for 0.20 ACKs for top commit: MarcoFalke: re-ACK 7a2ecf1, only change is adding an assert_equal in the test 🔰 jnewbery: utACK 7a2ecf1 Tree-SHA512: e0933ee40f705b751697dc27249e1868ed4874254b174ebdd0a7150125d8c818402e66df2371718c7eeb90e67ee2317215fb260aa9b9d7b9b45ee436de2988ff
… CRPCCommand tables fa1a922 rpc: Avoid initialization-order-fiasco on static CRPCCommand tables (MarcoFalke) Pull request description: Currently the fiasco is only theoretical because all content of the table are compile-time constants. However, the fiasco materializes should they ever become run-time constants (e.g. bitcoin#18531). ACKs for top commit: promag: ACK fa1a922. practicalswift: ACK fa1a922 -- fiasco bad :) Tree-SHA512: cccadb0ad56194599b74f04264d74c34fa865958580a850efc6474bbdc56f30cadce6b2e9a6ad5472ff46c3f4c793366acd8090fad409a45b25d961f2d89da19
13d2a33 Fix unregister_all_during_call cleanup (Russell Yanofsky) Pull request description: Use `TestingSetup` fixture to fix `unregister_all_during_call` test not calling `UnregisterBackgroundSignalScheduler`, which could trigger an assert in `RegisterBackgroundSignalScheduler` when called in later tests Failure reported by fanquake bitcoin#18551 (comment) ACKs for top commit: MarcoFalke: ACK 13d2a33 if appveyor unit tests pass Tree-SHA512: d2ec8ff14c54d97903af50031abfac1f38ec1c3aabc90371cfd5b79481fa69d3d77f339bfdf7d2178fd85e83402f72eda7cf4d339e5bbfa7e6e1a68836643b93
… shutdown before warmup finished faede1b test: Properly raise FailedToStartError when rpc shutdown before warmup finished (MarcoFalke) Pull request description: Should fix issues such as https://travis-ci.org/github/bitcoin/bitcoin/jobs/671910152#L7034 Top commit has no ACKs. Tree-SHA512: ac659f29c5ec91985c916b734e24911cbf4e2c5c4b1f1891a7e6c2d2511ec285167550fb03848eee4a7a3cbc9f8cdb0c766f4e881d9e44368c7415d007006368
NOTE: There is slight difference with original backport due to future changes in bitcoin#19272, bitcoin#19763 - otherwise functional test p2p_addr_relay.py fails fa1da3d test: Add basic addr relay test (MarcoFalke) fa1793c net: Pass connman const when relaying address (MarcoFalke) fa47a0b net: Make addr relay mockable (MarcoFalke) Pull request description: As usual: * Switch to std::chrono time to be type-safe and mockable * Add basic test that relies on mocktime to add code coverage ACKs for top commit: naumenkogs: utACK fa1da3d promag: ACK fa1da3d (fabe56e before bitcoin#18454 (comment)), fa5bf23 was dropped since last review. Tree-SHA512: 0552bf8fcbe375baa3cab62acd8c23b2994efa47daff818ad1116d0ffaa0b9e520dc1bca2bbc68369b25584e85e54861fe6fd0968de4f503b95439c099df9bd7 fixup - see bitcoin#19272, bitcoin#19763
PastaPastaPasta
force-pushed
the
bc-bp-v20-missing-9
branch
from
0c129ee to
2c9d41d
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
Next batch of backport from bitcoin v20
What was done?
NOTE: There is a slight difference with an original backport bitcoin#18454 due to
future changes in bitcoin#19272 and bitcoin#19763: otherwise p2p_addr_relay.py fails
NOTE: backport bitcoin#18587 reverts bitcoin#18160
How Has This Been Tested?
Run unit/functional tests
Breaking Changes
n/a
Checklist: