Commit Graph

27113 Commits

Author SHA1 Message Date
pasta
40dc6d9e83
Merge #6256: backport: bitcoin#23590, 23061, 23694
7d170bd1cc Merge bitcoin/bitcoin#23694: doc: Add missing optional to MempoolEntryDescription (MarcoFalke)
ed7ad7af74 Merge bitcoin/bitcoin#23061: Fix (inverse) meaning of -persistmempool (merge-script)
f5f69ae982 Merge bitcoin/bitcoin#23590: Crash debug builds when mempool ConsensusScriptChecks fails (MarcoFalke)

Pull request description:

  more bitcoin backports

ACKs for top commit:
  UdjinM6:
    utACK 7d170bd1cc
  PastaPastaPasta:
    utACK 7d170bd1cc

Tree-SHA512: 0332461598c9aa17476b591e98a2b3a952fcea0b7abbda604668b13745bf73268e0faba25a9338eabb8d27fe594c26804f8cc653c30c4f53e6b88f0f7fced287
2024-10-21 09:30:48 -05:00
pasta
7701b8dcea
Merge #6319: backport: bitcoin#19668, #21598, #19979, #28774 - lock annotations
1db9e6ac76 Merge #19979: Replace LockAssertion with AssertLockHeld, remove LockAssertion (MarcoFalke)
d0a4198166 Merge #21598: refactor: Remove negative lock annotations from globals (MarcoFalke)
55114a682e Merge #19668: Do not hide compile-time thread safety warnings (MarcoFalke)
898282d620 Merge bitcoin/bitcoin#28774: wallet: avoid returning a reference to vMasterKey after releasing the mutex that guards it (Ava Chow)
6d452845dc fix: add missing lock annotation for ContextualCheckBlock and related TODO (Konstantin Akimov)
846ebab6e0 fix: fixes for orphange's locks and cs_main (Konstantin Akimov)
002580c42d fix: add ignoring safety-annotation for llmq/signing_shares due to template lambdas (Konstantin Akimov)
502d6ae8ef fix: add multiple missing annotation about locks for dash specific code (Konstantin Akimov)
a219a8be15 partial Merge #29040: refactor: Remove pre-C++20 code, fs::path cleanup (Konstantin Akimov)
042e8a4101 fix: drop unneded lock assert in net.cpp for m_nodes_mutex (Konstantin Akimov)

Pull request description:

  ## What was done?
  These 4 backports improve noticeable implementation of thread-safety analysis by moving many possible warnings to compilation level.
  There's a lot of fixes for annotations after that!

  ## How Has This Been Tested?
  Build with clang:
  ```
  CC=clang CXX=clang++ ./configure --prefix=$(pwd)/depends/x86_64-pc-linux-gnu --enable-suppress-external-warnings   --enable-debug  --enable-stacktraces --enable-werror --enable-crash-hooks --enable-maintainer-mode
  ```

  ## Breaking Changes
  n/a

  ## Checklist:
  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas
  - [x] I have added or updated relevant unit/integration/functional/e2e tests
  - [x] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone

ACKs for top commit:
  UdjinM6:
    utACK 1db9e6ac76
  PastaPastaPasta:
    utACK 1db9e6ac76

Tree-SHA512: 42a73e423f311689babc366b58a92f024556cf63af93357cfa0715c471efc1491d02ed9e7ba19b23efcdf423ab5bd051a24da2ad64c32e4b6db132a05f6669c1
2024-10-21 09:08:38 -05:00
MarcoFalke
7d170bd1cc
Merge bitcoin/bitcoin#23694: doc: Add missing optional to MempoolEntryDescription
fa1571b156dfa757d0ce0fddd53c25256196522b doc: Add missing optional to MempoolEntryDescription (MarcoFalke)

Pull request description:

  Needed for https://github.com/bitcoin/bitcoin/pull/23083.

  Can be reviewed with `--word-diff-regex=.`.

ACKs for top commit:
  josibake:
    ACK fa1571b156
  shaavan:
    ACK fa1571b156dfa757d0ce0fddd53c25256196522b

Tree-SHA512: b4370003d2aeadce438778e15bd9a0d6a7fef4711acbe8471a50a9d72bbf74e1705fecbaae6f7eb367ece7c795a816c4b8b6583ed6c8f91b35621ca30fd95c18
2024-10-17 20:25:29 +05:30
merge-script
ed7ad7af74
Merge bitcoin/bitcoin#23061: Fix (inverse) meaning of -persistmempool
faa9c19a4b27e7fabf7c5deae1b4c4ca612ed01a doc: Add 23061 release notes (MarcoFalke)
faff17bbde6dcb1482a6210bc48b3192603a446f Fix (inverse) meaning of -persistmempool (MarcoFalke)

Pull request description:

  Passing `-persistmempool` is currently treated as `-nopersistmempool`

ACKs for top commit:
  jnewbery:
    reACK faa9c19a4b27e7fabf7c5deae1b4c4ca612ed01a
  hebasto:
    ACK faa9c19a4b27e7fabf7c5deae1b4c4ca612ed01a, I have reviewed the code and it looks OK, I agree it can be merged.

Tree-SHA512: f34a89a07745dabe340eb845b2a348b79c093e9056f7a21c17e1ba2e278177c9b4cf30e8095791fd645a7f90eb34850b2eee0c869b4f6ec02bf749c73b0e52ee
2024-10-17 20:25:27 +05:30
MarcoFalke
f5f69ae982
Merge bitcoin/bitcoin#23590: Crash debug builds when mempool ConsensusScriptChecks fails
faad05c6d2ad1840c26f6fdea7d358bf9ba6efea Crash debug builds when mempool ConsensusScriptChecks fails (MarcoFalke)

Pull request description:

  Currently a bug in the function might sneak around our testing infrastructure.

  Fix that by turning bugs into crashes during tests.

ACKs for top commit:
  glozow:
    utACK faad05c6d2ad1840c26f6fdea7d358bf9ba6efea, there's something seriously wrong with the code if this returns false, good to throw in debug mode

Tree-SHA512: dfea1cd9ce3f1c303f49cca1417cd5c77c6ed12849aaff7b6ab1b6060f2f0c9cf5d4689017355d11f66639bab35823f65f848e6979042fa875181509dfd5d3d7
2024-10-17 20:19:46 +05:30
pasta
5903fb7898
Merge #6324: backport: merge bitcoin#28577, #28588, #28634, #28849, #28805, #28951, #29058, #29239, partial bitcoin#28331, #28452 (BIP324 backports: part 2)
5dd60c4875 merge bitcoin#29239: Make v2transport default for addnode RPC when enabled (Kittywhiskers Van Gogh)
b2ac426a24 merge bitcoin#29058: use v2transport for manual/addrfetch connections, add to -netinfo (Kittywhiskers Van Gogh)
92e862acb2 merge bitcoin#28951: damage ciphertext/aad in full byte range (Kittywhiskers Van Gogh)
4e96e261e0 merge bitcoin#28805: Make existing functional tests compatible with --v2transport (Kittywhiskers Van Gogh)
9371e2e9a4 merge bitcoin#28849: fix node index bug when comparing peerinfo (Kittywhiskers Van Gogh)
400c9ddd67 merge bitcoin#28634: add check for missing garbage terminator detection (Kittywhiskers Van Gogh)
65eb194d82 merge bitcoin#28588: add checks for v1 prefix matching / wrong network magic detection (Kittywhiskers Van Gogh)
6074974849 merge bitcoin#28577: raise V1_PREFIX_LEN from 12 to 16 (Kittywhiskers Van Gogh)
ff92d1adae partial bitcoin#28331: BIP324 integration (Kittywhiskers Van Gogh)
f9f8805c72 fix: drop `CConnman::mapNodesWithDataToSend`, use transport data (UdjinM6)
d39d8a4fa7 partial bitcoin#28452: Do not use std::vector = {} to release memory (Kittywhiskers Van Gogh)

Pull request description:

  ## Additional Information

  * Dependent on https://github.com/dashpay/dash/pull/6280

  * Dependent on https://github.com/dashpay/dash/pull/6276

  * Dependency for https://github.com/dashpay/dash/pull/6329

  * [bitcoin#28452](https://github.com/bitcoin/bitcoin/pull/28452) was backported as the behavior it introduces is required for backports like [bitcoin#28331](https://github.com/bitcoin/bitcoin/pull/28331). As it pre-dates earlier BIP324 backports, the `ClearShrink` usage from those backports have also been incorporated into this backport.

  * When backporting [bitcoin#28331](https://github.com/bitcoin/bitcoin/pull/28331), in TestFramework's `add_nodes()`, `extra_args[i]` is not converted to a `list` like it is done upstream and avoiding duplication of `-v2transport=1` is instead done by an additional check. This is done to prevent test failures in `feature_mnehf.py`.

  * The local variable `args` is removed in [bitcoin#28805](https://github.com/bitcoin/bitcoin/pull/28805) as it does nothing (the argument appending logic is removed as part of the backport) and upstream removes it anyways.

  Special thanks to UdjinM6 for significant contributions to this and dependent PRs! 🎉

  ## Breaking Changes

  None expected.

  ## Checklist

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

ACKs for top commit:
  UdjinM6:
    light ACK 5dd60c4875
  PastaPastaPasta:
    utACK 5dd60c4875

Tree-SHA512: 7f3d0e54e1c96fc99b2d6b1e64485110aa73aeec0e4e4245ed4e6dc0529a7608e9c39103af636d5945d289775c40d3d15d7d4a75bea82462194dbf355fca48dc
2024-10-16 11:48:52 -05:00
pasta
1fd0160914
Merge #6328: backport: bitcoin/bitcoin#30131, #23258, #30504 - fix bild for Ubuntu 24.10 + clang
e994691e2d Merge bitcoin/bitcoin#30504: doc: use proper doxygen formatting for CTxMemPool::cs (merge-script)
a3e6378108 Merge bitcoin/bitcoin#23258: doc: Fix outdated comments referring to ::ChainActive() (fanquake)
dcbf671551 Merge bitcoin/bitcoin#30131: wallet, tests: Avoid stringop-overflow warning in PollutePubKey (merge-script)

Pull request description:

  ## Issue being fixed or feature implemented
  It fixes build with clang 19.1.1 (default clang version on Ubuntu 24.10)

  ## What was done?
  Backport 30131, 30504 to fix compilation error and warning; 23258 to reduce conflicts.
  See commit messages for details

  ## How Has This Been Tested?
  Build and succeed
  ```
  CC=clang CXX=clang++ ./configure --prefix=$(pwd)/depends/x86_64-pc-linux-gnu --enable-suppress-external-warnings   --enable-debug  --enable-stacktraces --enable-werror --enable-crash-hooks --enable-maintainer-mode --enable-multiprocess
  ```

  ## Breaking Changes
  N/A

  ## Checklist:
  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas
  - [x] I have added or updated relevant unit/integration/functional/e2e tests
  - [x] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone

ACKs for top commit:
  PastaPastaPasta:
    utACK e994691e2d
  UdjinM6:
    utACK e994691e2d

Tree-SHA512: 82623030c164c0852d87e8497a59157630f87a385050b6c58d79bf5a8f32462fb26cb02e61b1062afdf9f835e10b9baf4590c326c0fb41bbdd79c82e9d105513
2024-10-16 11:41:00 -05:00
Kittywhiskers Van Gogh
5dd60c4875
merge bitcoin#29239: Make v2transport default for addnode RPC when enabled 2024-10-15 15:34:12 +00:00
Kittywhiskers Van Gogh
b2ac426a24
merge bitcoin#29058: use v2transport for manual/addrfetch connections, add to -netinfo 2024-10-15 15:34:12 +00:00
Kittywhiskers Van Gogh
92e862acb2
merge bitcoin#28951: damage ciphertext/aad in full byte range 2024-10-15 15:34:12 +00:00
Kittywhiskers Van Gogh
4e96e261e0
merge bitcoin#28805: Make existing functional tests compatible with --v2transport
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
2024-10-15 15:34:12 +00:00
Kittywhiskers Van Gogh
9371e2e9a4
merge bitcoin#28849: fix node index bug when comparing peerinfo 2024-10-15 15:34:12 +00:00
Kittywhiskers Van Gogh
400c9ddd67
merge bitcoin#28634: add check for missing garbage terminator detection 2024-10-15 15:34:12 +00:00
Kittywhiskers Van Gogh
65eb194d82
merge bitcoin#28588: add checks for v1 prefix matching / wrong network magic detection
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
2024-10-15 15:34:12 +00:00
Kittywhiskers Van Gogh
6074974849
merge bitcoin#28577: raise V1_PREFIX_LEN from 12 to 16 2024-10-15 15:34:11 +00:00
Kittywhiskers Van Gogh
ff92d1adae
partial bitcoin#28331: BIP324 integration
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

excludes:
- changes to `src/rpc/util.cpp`
2024-10-15 15:34:11 +00:00
UdjinM6
f9f8805c72
fix: drop CConnman::mapNodesWithDataToSend, use transport data 2024-10-15 15:33:34 +00:00
Kittywhiskers Van Gogh
d39d8a4fa7
partial bitcoin#28452: Do not use std::vector = {} to release memory
Does not include changes to `headerssync.cpp` as it does not exist yet
2024-10-15 15:33:34 +00:00
pasta
d62fa7c0d9
Merge #6322: backport: Merge bitcoin/bitcoin#25034: test: add missing stop_node calls to feature_coinstatsindex and feature_prune
766c2c2757 Merge bitcoin/bitcoin#25034: test: add missing stop_node calls to feature_coinstatsindex and feature_prune (MacroFake)

Pull request description:

  ## Issue being fixed or feature implemented
  #6316 follow-up

  ## What was done?

  ## How Has This Been Tested?

  ## Breaking Changes

  ## 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)_

ACKs for top commit:
  knst:
    ACK 766c2c2757
  kwvg:
    utACK 766c2c2757
  PastaPastaPasta:
    utACK 766c2c2757

Tree-SHA512: f2ea80f427ae7fbff0fec570e5a34c98da165dff50a1012398d60d6253b4a2defbe74a7c35ebe49d086724e590d5d684bd1ecd3cd988a5639cfa88606f4f9975
2024-10-15 09:31:41 -05:00
pasta
5318d68213
Merge #6302: feat: remove DIP0020 requirement for OP_ since it is activated long time ago
c8fd37d7ab docs: added a comment about removed SCRIPT_ENABLE_DIP0020_OPCODES (Konstantin Akimov)
61bc3002b0 feat: drop SCRIPT_ENABLE_DIP0020_OPCODES, make opcodes available from genesis block (Konstantin Akimov)
0e55abd0f2 feat: remove feature_dip0020_activationl.py functional test and related code (Konstantin Akimov)

Pull request description:

  ## Issue being fixed or feature implemented
  it's alternate solution for https://github.com/dashpay/dash/pull/6299 to fix a crash reported https://github.com/dashpay/dash/pull/6256

  ## What was done?
  Removed code related to DIP0020 activation for various OP codes. DIP0020 is activated long time ago and no any historical blocks are violating rules, removing it's backwards compatible.

  ## How Has This Been Tested?
  Run unit and functional tests. See also changes in data for unit tests and removed functional test.
  It also re-index mainnet and testnet successfully
  ```
  src/qt/dash-qt  -reindex -assumevalid=0
  src/qt/dash-qt -testnet -reindex -assumevalid=0
  ```

  Also extra test is done with bitcoin/bitcoin#23590 - no crash with it in `feature_dip0020_activation.py` [modified assuming it is always activated]

  ## Breaking Changes
  N/A

  ## Checklist:
  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas
  - [x] I have added or updated relevant unit/integration/functional/e2e tests
  - [x] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone

ACKs for top commit:
  UdjinM6:
    utACK c8fd37d7ab
  PastaPastaPasta:
    utACK c8fd37d7ab

Tree-SHA512: 05ddda4e8fb66305995e91c8a04fbda690aef8fb82acb23b7d62f302da60b5ec7e7a97bd988efd2523dbd9cafde9f4b65cae2db9e4b5257464ce1c8fcca6a40f
2024-10-15 09:17:19 -05:00
pasta
dc02da1b28
Merge #6283: fix: do not use extra_args for enforced DashTestFramework arguments
6d4a782756 refactor: make dash specific args `sporkkey` and `dip3params` resilient for dashd restart (Konstantin Akimov)
7eaa0cf9ca refactor: simplify extra arguments wallet_mnemonicbits.py since usehd=1 is default option (Konstantin Akimov)

Pull request description:

  ## Issue being fixed or feature implemented
  DashTestFramework requires 2 arguments for dashd which are lost every dashd restart: `sporkkey` and `dip3params`.
  Without this PR you need to pass this arguments manually every time when you restart dashd in functional tests.

  ## What was done?
  Make dash specific args `sporkkey` and `dip3params` resilient for dashd restart

  It makes workarounds such as [these](c28b05c5ca) no more needed:
  ```
  self.restart_node(1, self.extra_args[1] + ["-checkaddrman=1"])
  ```

  Also there's some cleanup for `wallet_mnemonicbits.py` to remove `usehd=1` which is not required, and it has been discovered during revising all node's restarts related code.

  ## How Has This Been Tested?
  Removed workaround from `rpc_net.py` and run functional tests

  ## Breaking Changes
  N/A

  ## Checklist:
  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas
  - [x] I have added or updated relevant unit/integration/functional/e2e tests
  - [x] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone

ACKs for top commit:
  UdjinM6:
    utACK 6d4a782756
  PastaPastaPasta:
    utACK 6d4a782756

Tree-SHA512: 638b2dfe45aa35d7a9c9b4e527c3211b47e8f2fc97caf130eae09ee348d539b4c73007be0e3949ac978e306d394d9ead1d63bda3f4b515335cc62c32d2635e62
2024-10-15 08:53:35 -05:00
merge-script
e994691e2d
Merge bitcoin/bitcoin#30504: doc: use proper doxygen formatting for CTxMemPool::cs
6a5e9e40e1dd3d397020703feb9aa0b6f4577c98 doc: use proper doxygen formatting for CTxMemPool::cs (Vasil Dimov)

Pull request description:

  Having `@par title` followed by an empty line renders improperly in Doxygen - it results in a paragraph with a title but without a body.

  https://www.doxygen.nl/manual/commands.html#cmdpar

  This also results in a compiler warning (or error) with Clang 19:

  ```
  ./txmempool.h:368:34: error: empty paragraph passed to '@par' command [-Werror,-Wdocumentation]
    368 |      * @par Consistency guarantees
        |        ~~~~~~~~~~~~~~~~~~~~~~~~~~^
  1 error generated.
  ```

ACKs for top commit:
  maflcko:
    review ACK 6a5e9e40e1dd3d397020703feb9aa0b6f4577c98
  tdb3:
    ACK 6a5e9e40e1dd3d397020703feb9aa0b6f4577c98

Tree-SHA512: 2c4c9e5fd4bd44754800a9bcfff74df101afc060b84451c45aa098e4ceb05a47f28a36f8473b31222552fad6339b752a148e6b1c7d41c2003f515b3eb4060902
2024-10-15 13:52:49 +07:00
fanquake
a3e6378108
Merge bitcoin/bitcoin#23258: doc: Fix outdated comments referring to ::ChainActive()
BACKPORT NOTE:
changes for TestLockPointValidity in src/validation.cpp are applied to the same function but in src/txmempool.cpp

a0efe529e4fd053b890450413b9ca5e1bcd8f2c2 Fix outdated comments referring to ::ChainActive() (Samuel Dobson)

Pull request description:

  After #21866 there are a few outdated comments referring to `::ChainActive()`, which should instead refer to `ChainstateManager::ActiveChain()`.

ACKs for top commit:
  jamesob:
    ACK a0efe529e4

Tree-SHA512: 80da19c105ed29ac247e6df4c8e916c3bf3f37230b63f07302114eef9c115add673e9649f0bbe237295be0c6da7b1030b5b93e14daf6768f17ce5de7cf2c9ff2
2024-10-15 13:52:49 +07:00
merge-script
dcbf671551
Merge bitcoin/bitcoin#30131: wallet, tests: Avoid stringop-overflow warning in PollutePubKey
2289d4524053ab71c0d9133987cb36412797c1a2 wallet, tests: Avoid stringop-overflow warning in PollutePubKey (Ava Chow)

Pull request description:

  Fixes  #30114

ACKs for top commit:
  maflcko:
    ACK 2289d4524053ab71c0d9133987cb36412797c1a2 with g++ 14.1.1 🦄
  theStack:
    utACK 2289d4524053ab71c0d9133987cb36412797c1a2
  laanwj:
    ACK 2289d4524053ab71c0d9133987cb36412797c1a2

Tree-SHA512: 173c3c299bdd890f73e8a67a37880dbf816265e8b3c8298557ef2fc4670f5447005c0d2d81726f9bc43f6a69d677365d90a604354b3cbab0e3c52c4526d0407e
2024-10-15 13:52:48 +07:00
MarcoFalke
1db9e6ac76
Merge #19979: Replace LockAssertion with AssertLockHeld, remove LockAssertion
0bd1184adf6610c0bd14f4e9a25c0a200e65ae25 Remove unused LockAssertion struct (Hennadii Stepanov)
ab2a44297fd0796bf5797ae2a477e8e56d9c3c12 Replace LockAssertion with a proper thread safety annotations (Hennadii Stepanov)
73f71e19965e07534eb47701f2b23c9ed59ef475 refactor: Use explicit function type instead of template (Hennadii Stepanov)

Pull request description:

  This PR replaces `LockAssertion` with `AssertLockHeld`, and removes `LockAssertion`.

  This PR is compared with alternatives in https://github.com/bitcoin-core/bitcoin-devwiki/wiki/AssertLockHeld-PRs

ACKs for top commit:
  MarcoFalke:
    ACK 0bd1184adf6610c0bd14f4e9a25c0a200e65ae25
  ajtowns:
    ACK 0bd1184adf6610c0bd14f4e9a25c0a200e65ae25
  vasild:
    ACK 0bd1184ad

Tree-SHA512: ef7780dd689faf0bb479fdb97c49bc652e2dd10c148234bb95502dfbb676442d8565ee37864d923ca21a25f9dc2a335bf46ee82c095e387b59a664ab05c0ae41
2024-10-12 20:06:29 +07:00
MarcoFalke
d0a4198166
Merge #21598: refactor: Remove negative lock annotations from globals
fa5eabe72117f6e3704858e8d5b2c57a120258ed refactor: Remove negative lock annotations from globals (MarcoFalke)

Pull request description:

  They only make sense for mutexes that are private members. Until cs_main is a private member the negative annotations should be replaced by excluded annotations, which are optional.

ACKs for top commit:
  sipa:
    utACK fa5eabe72117f6e3704858e8d5b2c57a120258ed
  ajtowns:
    ACK fa5eabe72117f6e3704858e8d5b2c57a120258ed
  hebasto:
    ACK fa5eabe72117f6e3704858e8d5b2c57a120258ed
  vasild:
    ACK fa5eabe72117f6e3704858e8d5b2c57a120258ed

Tree-SHA512: 06f8a200304f81533010efcc42d9f59b8c4d0ae355920c0a28efb6fa161a3e3e68f2dfffb0c009afd9c2501e6a293c6e5a419a64d718f1f4e79668ab2ab1fcdc
2024-10-12 20:06:28 +07:00
MarcoFalke
55114a682e
Merge #19668: Do not hide compile-time thread safety warnings
ea74e10acf17903e44c85e3678853414653dd4e1 doc: Add best practice for annotating/asserting locks (Hennadii Stepanov)
2ee7743fe723227f2ea1b031eddb14fc6863f4c8 sync.h: Make runtime lock checks require compile-time lock checks (Anthony Towns)
23d71d171e6e22ba5e4a909d597a54595b2a2c1f Do not hide compile-time thread safety warnings (Hennadii Stepanov)
3ddc150857178bfb1c854c05bf9b526777876f56 Add missed thread safety annotations (Hennadii Stepanov)
af9ea55a72c94678b343f5dd98dc78f3a3ac58cb Use LockAssertion utility class instead of AssertLockHeld() (Hennadii Stepanov)

Pull request description:

  On the way of transit from `RecursiveMutex` to `Mutex` (see #19303) it is crucial to have run-time `AssertLockHeld()` assertion that does _not_ hide compile-time Clang Thread Safety Analysis warnings.

  On master (65e4ecabd5b4252154640c7bac38c92a3f3a7018) using `AssertLockHeld()` could hide Clang Thread Safety Analysis warnings, e.g., with the following patch applied:
  ```diff
  --- a/src/txmempool.h
  +++ b/src/txmempool.h
  @@ -607,7 +607,7 @@ public:
       void addUnchecked(const CTxMemPoolEntry& entry, setEntries& setAncestors, bool validFeeEstimate = true) EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main);

       void removeRecursive(const CTransaction& tx, MemPoolRemovalReason reason) EXCLUSIVE_LOCKS_REQUIRED(cs);
  -    void removeForReorg(const CCoinsViewCache* pcoins, unsigned int nMemPoolHeight, int flags) EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main);
  +    void removeForReorg(const CCoinsViewCache* pcoins, unsigned int nMemPoolHeight, int flags) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
       void removeConflicts(const CTransaction& tx) EXCLUSIVE_LOCKS_REQUIRED(cs);
       void removeForBlock(const std::vector<CTransactionRef>& vtx, unsigned int nBlockHeight) EXCLUSIVE_LOCKS_REQUIRED(cs);

  ```
  Clang compiles the code without any thread safety warnings.

  See "Add missed thread safety annotations" commit for the actual thread safety warnings that are fixed in this PR.

ACKs for top commit:
  MarcoFalke:
    ACK ea74e10acf 🎙
  jnewbery:
    ACK ea74e10acf17903e44c85e3678853414653dd4e1
  ajtowns:
    ACK ea74e10acf17903e44c85e3678853414653dd4e1

Tree-SHA512: 8cba996e526751a1cb0e613c0cc1b10f027a3e9945fbfb4bd30f6355fd36b9f9c2e1e95ed3183fc254b42df7c30223278e18e5bdb5e1ef85db7fef067595d447
2024-10-12 20:06:28 +07:00
Ava Chow
898282d620
Merge bitcoin/bitcoin#28774: wallet: avoid returning a reference to vMasterKey after releasing the mutex that guards it
32a9f13cb805ecf6aebb5cf4e5d92b8a47c4548b wallet: avoid returning a reference to vMasterKey after releasing the mutex that guards it (Vasil Dimov)

Pull request description:

  `CWallet::GetEncryptionKey()` would return a reference to the internal
  `CWallet::vMasterKey`, guarded by `CWallet::cs_wallet`, which is unsafe.

  Returning a copy would be a shorter solution, but could have security
  implications of the master key remaining somewhere in the memory even
  after `CWallet::Lock()` (the current calls to
  `CWallet::GetEncryptionKey()` are safe, but that is not future proof).

  So, instead of `EncryptSecret(m_storage.GetEncryptionKey(), ...)`
  change the `GetEncryptionKey()` method to provide the encryption
  key to a given callback:
  `m_storage.WithEncryptionKey([](const CKeyingMaterial& k) { EncryptSecret(k, ...); })`

  This silences the following (clang 18):

  ```
  wallet/wallet.cpp:3520:12: error: returning variable 'vMasterKey' by reference requires holding mutex 'cs_wallet' [-Werror,-Wthread-safety-reference-return]
   3520 |     return vMasterKey;
        |            ^
  ```

  ---
  _Previously this PR modified both ArgsManager and wallet code. But the ArgsManager commit 856c88776f was merged in https://github.com/bitcoin/bitcoin/pull/29040 so now this only affects wallet code. The previous PR description was:_

  Avoid this unsafe pattern from `ArgsManager` and `CWallet`:

  ```cpp
  class A
  {
      Mutex mutex;
      Foo member GUARDED_BY(mutex);
      const Foo& Get()
      {
          LOCK(mutex);
          return member;
      } // callers of `Get()` will have access to `member` without owning the mutex.
  ```

ACKs for top commit:
  achow101:
    ACK 32a9f13cb805ecf6aebb5cf4e5d92b8a47c4548b
  ryanofsky:
    Code review ACK 32a9f13cb805ecf6aebb5cf4e5d92b8a47c4548b. This seems like a potentially real race condition, and the fix here is pretty simple.
  furszy:
    ACK 32a9f13c

Tree-SHA512: 133da84691642afc1a73cf14ad004a7266cb4be1a6a3ec634d131dca5dbcdef52522c1d5eb04f5b6c4e06e1fc3e6ac57315f8fe1e207b464ca025c2b4edefdc1
2024-10-12 20:06:28 +07:00
Konstantin Akimov
6d452845dc
fix: add missing lock annotation for ContextualCheckBlock and related TODO 2024-10-12 20:06:27 +07:00
Konstantin Akimov
846ebab6e0
fix: fixes for orphange's locks and cs_main 2024-10-12 20:06:27 +07:00
Konstantin Akimov
002580c42d
fix: add ignoring safety-annotation for llmq/signing_shares due to template lambdas 2024-10-12 20:06:27 +07:00
Konstantin Akimov
502d6ae8ef
fix: add multiple missing annotation about locks for dash specific code 2024-10-12 20:06:23 +07:00
Konstantin Akimov
a219a8be15
partial Merge #29040: refactor: Remove pre-C++20 code, fs::path cleanup
It fixes return reference to const variable under mutex which is not a good idea
2024-10-12 20:02:06 +07:00
Konstantin Akimov
042e8a4101
fix: drop unneded lock assert in net.cpp for m_nodes_mutex 2024-10-12 20:00:06 +07:00
Konstantin Akimov
6d4a782756
refactor: make dash specific args sporkkey and dip3params resilient for dashd restart
Co-Authored-By: UdjinM6 <UdjinM6@users.noreply.github.com>
2024-10-11 16:41:08 +07:00
Konstantin Akimov
7eaa0cf9ca
refactor: simplify extra arguments wallet_mnemonicbits.py since usehd=1 is default option 2024-10-11 16:38:53 +07:00
pasta
3f0c2ff324
Merge #6323: chore: bump dashbls subtree to 1.3.3
02260cba57 build: stop tracking cmake dependency relic_conf.h.in (UdjinM6)
02107450d0 Squashed 'src/dashbls/' changes from 795660db76..4e070243ae (Odysseas Gabrielides)
314102e054 Revert "build: stop tracking cmake dependency relic_conf.h.in" (Odysseas Gabrielides)

Pull request description:

  ## Issue being fixed or feature implemented
  Bumped blsdash to version 1.3.3

  ## What was done?

  ## How Has This Been Tested?

  ## Breaking Changes

  ## Checklist:
  - [x] 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)_

ACKs for top commit:
  kwvg:
    utACK 02260cba57
  UdjinM6:
    utACK 02260cba57

Tree-SHA512: 5f45919d5cc8e2c2eb37427c4e3b110bf34884472f4de1e3cd0e6b6bd7fc71d71e7d63192e6a9a637f7181f2e16641dec3abc15cd41e1e5b7322185a2bc7a58e
2024-10-10 12:47:59 -05:00
UdjinM6
02260cba57
build: stop tracking cmake dependency relic_conf.h.in
This reverts commit 0b3ab8d5e9.
2024-10-09 17:26:10 +03:00
Odysseas Gabrielides
c05fdde88d
Merge commit '02107450d0c68a372f720974c92fe16acf3961f9' into bump_blsdash_1.3.3 2024-10-09 17:25:19 +03:00
Odysseas Gabrielides
02107450d0 Squashed 'src/dashbls/' changes from 795660db76..4e070243ae
4e070243ae chore: bump version to 1.3.3 (#99)
d93956254e ci: disable Go bindings CI for macos for now (#98)
ae40c5c86d Merge pull request #97 from PastaPastaPasta/refac/pybind-bump-2.13.6
e835ece935 refactor: bump pybind version to 2.13.6
eda5d6a402 chore: change of gmp source (#95)
61f95aa80e chore: cleanup 6.2.1 left overs (#96)
adbd094409 Merge pull request #92 from kwvg/darwin_gmp
062ee6726b Merge pull request #90 from UdjinM6/fix_aarch_arch
3538d8b033 fix: aarch64 is not supported, should set ARCH to RELIC_NONE
e27a62f4a2 revert: disable gmp if targeting darwin on aarch64 when on 'auto'
bb2fe6ee55 build: enforce minimum version of libgmp based on arch and platform
9832b7a132 build: replace deprecated macros `AC_PROG_CC_C99` and `AM_PROG_CC_C_O`
b2428718b9 Merge pull request #91 from UdjinM6/fix_macos_test_build
3ffa7fa2b6 chore: bump version to 1.3.2 (#94)
0f4efc9327 Merge pull request #88 from HashEngineering/feat/support-android
a181889489 fix: rust bindings build for macos (#89)
738d187359 fix: detect gmp via brew earlier
ce4d6a47b6 fix: install libtool
4fa46ccaff fix: use macos-latest for test build
69bdc1aac7 Merge pull request #85 from kwvg/debug
39791d4e31 build: print build options after configure
73106a0121 build: use `-mbranch-protection=bti` on supporting `aarch64` compilers
6a3c28f6ca build: use stricter `-Werror` when testing compile flags
7a1b227637 build: rename {`NO`}`WARN_CFLAGS` to {`NO`}`WARN_FLAGS`, use with C{++}
28bea63838 build: set {`NO`}`WARN_CFLAGS` flags if not overridden and uniformly
32c2f0f5f8 trivial: rename `CORE_CXXFLAGS` to `CORE_FLAGS`, use with C{++}
b630c2c323 build: append `HARDENED_FLAGS` to `AM_CFLAGS`
e6008148e4 trivial: rename `HARDENED_CXXFLAGS` to `HARDENED_FLAGS`
af0e3daef5 build: subsume `PI{C,E}_FLAGS` into `HARDENED_CXXFLAGS`
9ff8618a1b build: expand `--disable-optimizations` to include `-O0` and `-fwrapv`
3036b83181 build: expand `--enable-debug` to include `-O0`, `-ftrapv` and dbg info
c90d43d43b build: add check to see if `CFLAGS` has been overridden
2d77f7ae49 build: remove vestigial `LIBTOOL_{CXX,CPP,LD}FLAGS`, `HARDENED_CPPFLAGS`
883a098868 build: autodetect i?86 and arm as 32-bit
deb3269820 build: don't specify exact `{CPU_}ARCH` if optimizations are disabled
720d49a44b trivial: fix indentation for `want_backend` check
f9328320af build: use `easy` backend if optimizations are disabled unless specified
3687cd59e0 build: define new flag `--enable-optimizations`
f82bfee5dd build: ensure help string format matches Autotool defaults
d68920063e build: define arguments as `--enable-[term]` instead of `--disable-[term]`
7f41e7dd16 fix: support android
1c2fc79c19 feat(rust): allow to move G1 and G2 elements between threads (#87)
3540b8bbed feat: debug with data hex (#86)

git-subtree-dir: src/dashbls
git-subtree-split: 4e070243aed142bc458472f8807ab77527dd879a
2024-10-09 17:25:18 +03:00
Odysseas Gabrielides
314102e054
Revert "build: stop tracking cmake dependency relic_conf.h.in"
This reverts commit a891c78654.
2024-10-09 17:24:41 +03:00
MacroFake
766c2c2757
Merge bitcoin/bitcoin#25034: test: add missing stop_node calls to feature_coinstatsindex and feature_prune
a3cd7dbfd8200c580aae9ea0f5473d58107dd582 test: stop node before calling assert_start_raises_init_error (Martin Zumsande)

Pull request description:

  In #24789, I forgot to stop the node before using `assert_start_raises_init_error` in `feature_coinstatsindex`. This resulted in a bitcoind process that is not being terminated after the test finishes.
  `feature_prune` has the same problem and also creates a zombie bitcoind process.

  Also adds an assert to `assert_start_raises_init_error` to make sure the node isn't already running to prevent this sort of mistake in the future.

Top commit has no ACKs.

Tree-SHA512: 902f683ebe7b19ca32ab83ca40d9698e9d91509b1d003f21a7221f79b647e05b6ef5c0c888fbb772cbca5e641d5c9437d522b6671f446c3ab321d79f7c6d0284
2024-10-09 13:44:32 +03:00
pasta
6157e67a55
Merge #6297: backport: merge bitcoin#23156, #23213, #23227, #23223, #23564, #23538, #23437, #23630, #23465, #23738, #17631, #22875 (auxiliary backports: part 18)
5aceee38fc merge bitcoin#22875: Fix Racy ParseOpCode function initialization (Kittywhiskers Van Gogh)
427d07f4db merge bitcoin#17631: Expose block filters over REST (Kittywhiskers Van Gogh)
d60f15ec33 merge bitcoin#23738: improve logging of ChainstateManager snapshot persistance (Kittywhiskers Van Gogh)
87257347c2 merge bitcoin#23465: Remove CTxMemPool params from ATMP (Kittywhiskers Van Gogh)
d2cbdc40d5 merge bitcoin#23630: Remove GetSpendHeight (Kittywhiskers Van Gogh)
8bdab4d4fe merge bitcoin#23437: AcceptToMemoryPool (Kittywhiskers Van Gogh)
1f4e8a0cf9 merge bitcoin#23538: Remove strtol in torcontrol (Kittywhiskers Van Gogh)
2318d9f996 merge bitcoin#23564: don't use deprecated brew package names (Kittywhiskers Van Gogh)
3b7a7394a9 merge bitcoin#23223: Disable lock contention logging in checkqueue_tests (Kittywhiskers Van Gogh)
b383609a72 merge bitcoin#23227: Avoid treating integer overflow as OP_0 (Kittywhiskers Van Gogh)
0188d32430 merge bitcoin#23213: Return error when header count is not integral (Kittywhiskers Van Gogh)
eb9e20890f merge bitcoin#23156: Remove unused ParsePrechecks and ParseDouble (Kittywhiskers Van Gogh)
18fff7e3d3 rpc: switch to taking an integer for `rate` in `quorum dkgsimerror` (Kittywhiskers Van Gogh)

Pull request description:

  ## Additional Information

  * Dependent on https://github.com/dashpay/dash/pull/6288

  * Dependent on https://github.com/dashpay/dash/pull/6296

  ## Breaking changes

  - `quorum dkgsimerror` will no longer accept a decimal value between 0 and 1 for the `rate` argument, it will now expect an integer between 0 to 100.

  ## Checklist

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

ACKs for top commit:
  PastaPastaPasta:
    utACK 5aceee38fc
  UdjinM6:
    utACK 5aceee38fc
  knst:
    utACK 5aceee38fc

Tree-SHA512: 8fc34b05a74f2ddbe84b2a7a54772e49941042c89bc74d71d33711e658754a3d086af11fb2437d2bb72ede0c611adc57b82193783e7b6f10fbd4ebab2a7fa7cb
2024-10-08 17:28:25 -05:00
pasta
f93c763ac6
Merge #6318: refactor: remove circular dependency governance/object over governance/classes
2e36832982 refactor: drop circular dependency governance/classes over governance/governance (Konstantin Akimov)
39f18ab154 refactor: move CGoveranceManager code from classes.cpp to governace.cpp (Konstantin Akimov)
350a5ca47c refactor: drop CSuperblock::GetGovernanceObject to simplify thread safety analysis over FindGovernanceObject (Konstantin Akimov)
5031f29441 refactor: add couple missing `const` for CGovernanceManager (Konstantin Akimov)
b240d08e09 refactor: move GetBestSuperblock to CGovernanceManager (Konstantin Akimov)
3641653174 refactor: move CSuperblockManager::IsValid to CGoveranceManager::IsValidSuperblock (Konstantin Akimov)
de8969f463 refactor: move ExecuteBestSuperblock to CGovernanceManager (Konstantin Akimov)
107d5b4941 refactor: move GetSuperblockPayments to CGovernanceManager (Konstantin Akimov)
7a470c441e refactor: move IsSuperblockTriggered to CGovernanceManager (Konstantin Akimov)
9638fdce6d refactor: pass mn_sync to CGovernanceManager ctor as a reference (UdjinM6)
7eb1634686 refactor: drop alias that is used only once (Konstantin Akimov)
1570a02c89 refactor: move ScopedLockBool from header to cpp file (Konstantin Akimov)
7aafb5a393 fix: add one more file to list of non-backported (flat-database.h) (Konstantin Akimov)
41f1a43236 fix: add missing const for member functions of CRateCheckBuffer (Konstantin Akimov)
982fc9a069 fix: avoid lock annotation for govman.cs in voteraw (Konstantin Akimov)

Pull request description:

  ## Issue being fixed or feature implemented
  This PR is preparation for bitcoin#19668, otherwise impossible to make lock annotations for CGovernanceManager properly.

  ## What was done?
  1. object mn_sync and peerman is pass to many methods of CGovernanceManager instead passing it to constructor.
  2. methods of class CSuperblockManager moved to CGovernanceManager where they belongs to.
  3. removed `CSuperblock::GetGovernanceObject` which makes a lot of mess with annotations of `govman.cs`

  And minor relevant improvements: moved ScopedLockBool from header to implementation, added multiple `const` for methods, added one more file `flat-database.h` to non-backported list

  ## How Has This Been Tested?
  Run unit and functional tests.

  ## Breaking Changes
  N/A

  ## Checklist:
  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas
  - [x] I have added or updated relevant unit/integration/functional/e2e tests
  - [x] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone

ACKs for top commit:
  UdjinM6:
    utACK 2e36832982
  PastaPastaPasta:
    utACK 2e36832982

Tree-SHA512: 59842c208f7ece46c9381fc3f9fc838d9ed1cf0fd2404eebf7fbd656c5df1fa5fd339410da83088089e2d954a017efb518cba290f6c5d45b5bcb91818041f931
2024-10-08 17:12:24 -05:00
Konstantin Akimov
2e36832982
refactor: drop circular dependency governance/classes over governance/governance 2024-10-09 00:13:15 +07:00
Konstantin Akimov
39f18ab154
refactor: move CGoveranceManager code from classes.cpp to governace.cpp
There's only code move, no changes. For review use `git show --color-moved=dimmed-zebra`
2024-10-09 00:13:15 +07:00
Konstantin Akimov
350a5ca47c
refactor: drop CSuperblock::GetGovernanceObject to simplify thread safety analysis over FindGovernanceObject 2024-10-09 00:13:15 +07:00
Konstantin Akimov
5031f29441
refactor: add couple missing const for CGovernanceManager 2024-10-09 00:13:14 +07:00
Konstantin Akimov
b240d08e09
refactor: move GetBestSuperblock to CGovernanceManager 2024-10-09 00:13:14 +07:00
Konstantin Akimov
3641653174
refactor: move CSuperblockManager::IsValid to CGoveranceManager::IsValidSuperblock 2024-10-09 00:13:13 +07:00