Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

backport: trivial 2024 10 23 pr10 #6353

Merged
merged 8 commits into from
Oct 25, 2024
Merged

backport: trivial 2024 10 23 pr10 #6353

merged 8 commits into from
Oct 25, 2024

Conversation

PastaPastaPasta
Copy link
Member

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:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation
  • I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

@PastaPastaPasta PastaPastaPasta added this to the 22 milestone Oct 24, 2024
fanquake and others added 3 commits October 23, 2024 20:05
@fanquake @PastaPastaPasta
Merge bitcoin#24988: lint: Mention NONFATAL_UNREACHABLE in lint-asser…
f39fcd1 
…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
@PastaPastaPasta
Merge bitcoin#25092: doc: various developer notes updates
c91f010 
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
@PastaPastaPasta
Merge bitcoin#24062: refactor: replace RecursiveMutex `m_most_recent_…
de17997 
…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
UdjinM6
UdjinM6 requested changes Oct 25, 2024
View reviewed changes
depends/Makefile Outdated Show resolved Hide resolved
MacroFake and others added 5 commits October 25, 2024 09:13
@PastaPastaPasta
Merge bitcoin#25486: test: fix failing test `interface_usdt_utxocache…
1204dc0 
….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
@fanquake @PastaPastaPasta
Merge bitcoin#25599: build: Check for std::atomic::exchange rather th…
b66eebe 
…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
@achow101 @PastaPastaPasta
Merge bitcoin#23997: wallet: avoid rescans under assumed-valid blocks
e2e8598 
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
@fanquake @PastaPastaPasta
Merge bitcoin#26282: wallet: have prune error take precedence over as…
c59cb15 
…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
@fanquake @PastaPastaPasta
Merge bitcoin#28304: doc: Remove confusing assert linter
4101fea 
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
UdjinM6
UdjinM6 approved these changes Oct 25, 2024
View reviewed changes
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 4101fea

kwvg
kwvg approved these changes Oct 25, 2024
View reviewed changes
Copy link
Collaborator

@kwvg kwvg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 4101fea

@PastaPastaPasta PastaPastaPasta merged commit bef82c4 into dashpay:develop Oct 25, 2024
10 of 27 checks passed
knst
knst reviewed Oct 25, 2024
View reviewed changes
doc/developer-notes.md
@@ -717,12 +717,6 @@ Common misconceptions are clarified in those sections:
- Passing (non-)fundamental types in the [C++ Core
Guideline](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-conventional).

- Assertions should not have side-effects.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

28304 -- yay! asserts everywhere! 🥳

@PastaPastaPasta PastaPastaPasta deleted the develop-trivial-2024-10-23-pr10 branch October 25, 2024 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@knst knst knst left review comments

@UdjinM6 UdjinM6 UdjinM6 approved these changes

@kwvg kwvg kwvg approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
22
Development

Successfully merging this pull request may close these issues.
6 participants
@PastaPastaPasta @knst @UdjinM6 @kwvg @fanquake @achow101