backport: trivial 2024 10 23 pr10#6353
Merged
PastaPastaPasta merged 8 commits intodashpay:developfrom Oct 25, 2024
Merged
Conversation
…tions.py fa82a1e lint: Mention NONFATAL_UNREACHABLE in lint-assertions.py (MacroFake) Pull request description: Follow up to commit b1c5991. Also remove empty newline added in that commit. ACKs for top commit: fanquake: ACK fa82a1e Tree-SHA512: cf398eceb135672137183bfa19ee57a82553a3dbcbce74db954c6fcd79f9606092cc0d8217610fe6cd67b7ef2d4f01d90329f0f568516d9b14aa2cd0f0715478
6542842 Add clang lifetimebound section to developer notes (Jon Atack) e66b321 Add C++ functions and methods section to developer notes (Jon Atack) 5fca70f Link in developer notes style to internal interface exception (Jon Atack) fc4cb85 Prefer Python for scripts in developer notes (Jon Atack) 370120e Remove obsolete BDB ENABLE_WALLET section in developer notes (Jon Atack) Pull request description: A few updates noticed while working on a lifetimebound section. - Remove obsolete BDB ENABLE_WALLET section (only one file, src/wallet/bdb.h, still has a `db_cxx.h` BDB header) - Prefer Python for scripts in developer notes (and a few miscellaneous touch-ups) - In the code style section, add a link to the internal interface exception so that people are aware of it - Add a "C++ functions and methods" section - Add a Clang `lifetimebound` attribute section ACKs for top commit: laanwj: ACK 6542842 jarolrod: code review ACK bitcoin@6542842 Tree-SHA512: d2ae335cac899451d5c7bdc8e94fd82053a5f44ac1ba444bdde71abaaa24a519713c1078f3a8f07ec8466b181788a613fd3c68061e54b3fdc8cd6f3e3f9791ec
…block_mutex` with Mutex 83003ff refactor: replace RecursiveMutex `m_most_recent_block_mutex` with Mutex (Sebastian Falbesoner) 8edd0d3 refactor: reduce scope of lock `m_most_recent_block_mutex` (Sebastian Falbesoner) Pull request description: This PR is related to bitcoin#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: https://github.com/bitcoin/bitcoin/blob/b019cdc036343a437fd7ced85467bd95f48d84c4/src/net_processing.cpp#L1650-L1655 https://github.com/bitcoin/bitcoin/blob/b019cdc036343a437fd7ced85467bd95f48d84c4/src/net_processing.cpp#L1861-L1865 https://github.com/bitcoin/bitcoin/blob/b019cdc036343a437fd7ced85467bd95f48d84c4/src/net_processing.cpp#L3149-L3152 https://github.com/bitcoin/bitcoin/blob/b019cdc036343a437fd7ced85467bd95f48d84c4/src/net_processing.cpp#L3201-L3206 https://github.com/bitcoin/bitcoin/blob/b019cdc036343a437fd7ced85467bd95f48d84c4/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 83003ff with a small comment. hebasto: ACK 83003ff w0xlt: ACK bitcoin@83003ff Tree-SHA512: 3df290cafd2f6c4d40afb9f14e822a77d9c1828e66f5e2233f3ac1deccc2b0a8290bc5fb8eb992f49d39e887b50bc0e9aad63e05db2d870791a8d409fb95695f
82091da to
d12f63c
Compare
UdjinM6
requested changes
Oct 25, 2024
….py` f665c6e test: fix failing test interface_usdt_utxocache.py (Sebastian Falbesoner) Pull request description: The `from_node` argument doesn't exist anymore for `MiniWallet.create_self_transfer` since PR bitcoin#25435 (commit fa8421b), leading to an error on master: ``` $ sudo ./test/functional/interface_usdt_utxocache.py 2022-06-27T17:45:35.585000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_7s1djjo1 2022-06-27T17:45:36.515000Z TestFramework (INFO): testing the utxocache:uncache tracepoint API 2022-06-27T17:45:36.517000Z TestFramework (ERROR): Unexpected exception caught during testing Traceback (most recent call last): File "/home/honeybadger/bitcoin/test/functional/test_framework/test_framework.py", line 133, in main self.run_test() File "/home/honeybadger/bitcoin/./test/functional/interface_usdt_utxocache.py", line 149, in run_test self.test_uncache() File "/home/honeybadger/bitcoin/./test/functional/interface_usdt_utxocache.py", line 172, in test_uncache invalid_tx = self.wallet.create_self_transfer( TypeError: create_self_transfer() got an unexpected keyword argument 'from_node' 2022-06-27T17:45:36.568000Z TestFramework (INFO): Stopping nodes [...] ``` Fix this by removing the argument. (Unfortunately, the USDT tests don't seem to run on any CI target, I guess that's due to missing permissions to hook into the kernel.) ACKs for top commit: MarcoFalke: cr ACK f665c6e Tree-SHA512: 74f8e398739a25ab5518ff71b998d03d4e529a786ba5b424509de81a511ad3e2e1cd38a5b7bb9f1f5a21340391d6807f4951ff39fa3a2ad65a3b11b989eebea6
…an std::atomic_exchange 4de4221 build: Check for std::atomic::exchange rather than std::atomic_exchange (Andrew Chow) Pull request description: Our usage of std::atomic is with it's own exchange function, not std::atomic_exchange. So we should be looking specifically for that function. This removes the need for -latomic for riscv builds, which resolves a guix cross architecture reproducibility issue. ACKs for top commit: hebasto: ACK 4de4221 fanquake: ACK 4de4221 Tree-SHA512: dd8225fc9c6a335601f611700003d0249b9ef941efa502db39306129677929d013048e9221be1d6d7f0ea2d90313d4b87de239f441be21b25bea40a6c19a031e
817326a wallet: avoid rescans if under the snapshot (James O'Beirne) Pull request description: This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11) (parent PR: bitcoin#15606) --- Refuse to load a wallet if it requires a rescan lower than the height of assumed-valid blocks. Of course in live code right now, `BLOCK_ASSUMED_VALID` block index entries don't exist since they're a unique flag introduced by the use of UTXO snapshots, so this is prophylactic code exercised only by unittests. ACKs for top commit: achow101: ACK 817326a ryanofsky: Code review ACK 817326a. This seems like the simplest change we can make to avoid wallet problems when an assumeutxo snapshot is loaded. Tree-SHA512: cfa44b2eb33d1818d30df45210d0dde1e9b78cc9b7c88cb985054dc28427bba9e0905debe4196065d1d3a5ce7bca7e605e629d5ce5f0225b25395746e6d3d596
…sumedvalid 1c36baf wallet: have prune error take precedence over assumedvalid (James O'Beirne) Pull request description: Fixes bitcoin#23997 (comment). From Russ Yanofsky: > Agree with all of Marco's points here and think this should be updated > > If havePrune and hasAssumedValidChain are both true, better to show havePrune error message. Assumed-valid error message is vague and not very actionable. Would suggest "Error loading wallet. Wallet requires blocks to be downloaded, and software does not currently support loading wallets while blocks are being downloaded out of order though assumeutxo snapshots. Wallet should be able to load successfully after node sync reaches height {block_height}" ACKs for top commit: MarcoFalke: ACK 1c36baf aureleoules: ACK 1c36baf Tree-SHA512: bfb0024bb962525cbbd392ade3c0331a8b0525e7f2f2ab52b2dbb9b6dd6311070d85ecb762a7689db84a30991971865698ab6fec187206e6a92133790c5a91dc
fa6e6a3 doc: Remove confusing assert linter (MarcoFalke) Pull request description: The `assert()` documentation and linter are redundant and confusing: * The source code already refuses to compile with `assert()` disabled. * They violate the assumptions about `Assert()`, which *requires* side effects. * The existing linter doesn't enforce the guideline, only checking for `++` and `--` side effects. Fix all issues by removing the docs and the linter. See also bitcoin#26684 (comment) Going forward everyone is free to use whatever code in this regard they think is the easiest to read. Also, everyone is still free to share style-nits, if they think it is a good use of their time and of the pull request author. Finally, the author is still free to dismiss or ignore this style-nit, or any other style-nit. ACKs for top commit: hebasto: ACK fa6e6a3, I have reviewed the code and it looks OK. theStack: ACK fa6e6a3 Tree-SHA512: 686738d71e1316cc95e5d3f71869b55a02bfb137c795cc0875057f4410e564bc8eff03c985a2087b007fb08fc84551c7da1e8b30c7a9c3f2b14e5e44a5970236
d12f63c to
4101fea
Compare
UdjinM6
approved these changes
Oct 25, 2024
kwvg
approved these changes
Oct 25, 2024
knst
reviewed
Oct 25, 2024
| - Passing (non-)fundamental types in the [C++ Core | ||
| Guideline](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-conventional). | ||
|
|
||
| - Assertions should not have side-effects. |
Collaborator
There was a problem hiding this comment.
28304 -- yay! asserts everywhere! 🥳
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Batch of trivial backports
What was done?
See commits
How Has This Been Tested?
built locally; large combined merge passed tests locally
Breaking Changes
Should be none
Checklist: