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
672d49c863fd6b0f096f166aeb3504f51dfa7d19 scripted-diff: replace non-standard fixed width integer types (`u_int`...` -> `uint`...) (Sebastian Falbesoner)
Pull request description:
Fixed width integer types prefixed with `u_int` are not part of C++ (see https://en.cppreference.com/w/cpp/types/integer), so it's better to avoid and replace them with their standard-conforming counterparts. (For those interested in history, according to one theory those u_int... types have been introduced by BSD: https://stackoverflow.com/a/5163960, http://lists.freedesktop.org/archives/release-wranglers/2004-August/000923.html).
ACKs for top commit:
laanwj:
Code review ACK 672d49c863fd6b0f096f166aeb3504f51dfa7d19
fanquake:
ACK 672d49c863fd6b0f096f166aeb3504f51dfa7d19
Tree-SHA512: 68134a0adca0d5c87a7432367cb493491a67288d69a174be2181f8e26efa968d966b9eb1cde94813942405063ee3be2a3437cf2aa5f71375f59205cbdbf501bb
e5d183151709ab59d2fa6fe9e0243000e8d6abbe [netgroup] Use nStartByte as offset for the last byte of the group (dergoegge)
Pull request description:
This addresses my review [comments](https://github.com/bitcoin/bitcoin/pull/22910#discussion_r856095896) I left on #22910.
This has no effect on the current logic as `nStartByte` is only used for internal addresses which only ever add 10 whole bytes to the returned group. However to avoid future bugs, I think we should use `nStartByte` as offset for the last byte as well, in case we ever add a new address type that makes makes use of `nStartByte` and adds fractional bytes to the group.
ACKs for top commit:
jnewbery:
Code review ACK e5d183151709ab59d2fa6fe9e0243000e8d6abbe
theStack:
Concept and code-review ACK e5d183151709ab59d2fa6fe9e0243000e8d6abbe
Tree-SHA512: 4c08c7d6cb38b553e998798b3e3b790177aaa2141a48e277dfd538e01a7fccadf644329e93c5b0fb5e7e4037494c8dfe061b94eb52c6b31dc21bdf99eb0e311a
51708c4516cb9d52e84dc8850d93f556dda1a75b gui: peersWidget - ResizeToContents Age and IP/Netmask columns (randymcmillan)
209301a442512579d57f79c82417dc7c496248b6 gui: add Age column to peers tab (randymcmillan)
127de22c5fb396e1670d2a911faf7a9adc9241e2 gui: add FormatPeerAge() utility helper (Jon Atack)
Pull request description:
This change adds an "Age" column to the peers table view,
which displays the duration of each peer's connection.
ACKs for top commit:
jonatack:
re-ACK 51708c4516cb9d52e84dc8850d93f556dda1a75b
Jamewood:
> re-ACK 51708c4
shaavan:
reACK 51708c4516cb9d52e84dc8850d93f556dda1a75b
hebasto:
ACK 51708c4516cb9d52e84dc8850d93f556dda1a75b, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 27323f7080ec0d3fcdbf1b190fba1cd2d7406840ab6607c221cf8af950db9134e22721cc5a88f4fc4f390d8b05e98bc4b7521661a31fadad9e2c6c6390e71788
d2ba43fec82af84521f1dbe4475d01888ccf4d0d test: use MiniWallet for mempool_unbroadcast.py (Ayush Sharma)
Pull request description:
This PR enables one of the non-wallet functional tests (mempool_unbroadcast.py) to be run even with the Bitcoin Core wallet disabled by using the MiniWallet instead, as proposed in #20078 .
Top commit has no ACKs.
Tree-SHA512: e4c577899b66855dafca9dab875fa9b9c68b762a8cdb14f3a7547841c4f001e79d62641e6ae202fb56a3f28aeea1779143164c872507ff8da0bd9930a8ed182e
fad7ddf9e3710405d727f61d8200d5efed1e705b test: Run symlink regression tests on Windows (MarcoFalke)
Pull request description:
Seems odd to add tests, but not run them on the platform that needs them most.
ACKs for top commit:
laanwj:
Code review ACK fad7ddf9e3710405d727f61d8200d5efed1e705b
ryanofsky:
Code review ACK fad7ddf9e3710405d727f61d8200d5efed1e705b, just removing new test. Would be nice if the test could be added later, of course.
Tree-SHA512: 64b235967a38c2eb90657e8d7a0447bcc8ce81d1b75a275b6c48bd42efd9ea7e7939257e484f297ee84598def3738eaeb289561aeba1dd6a99b258d389995139
fa2bcc4e42e7fed61727b3de4019e9702d4090ce Run coin.IsSpent only once in a row (MarcoFalke)
Pull request description:
Follow-up to commit 64e4963c635ec3a73a5fa3f32f6ec08e70609f60 and https://github.com/bitcoin/bitcoin/pull/23976#discussion_r787758193
ACKs for top commit:
glozow:
utACK fa2bcc4e42e7fed61727b3de4019e9702d4090ce, agree the assertion is sufficient
theStack:
Code-review ACK fa2bcc4e42e7fed61727b3de4019e9702d4090ce
w0xlt:
crACK fa2bcc4e42
shaavan:
Code Review ACK fa2bcc4e42e7fed61727b3de4019e9702d4090ce
brunoerg:
crACK fa2bcc4e42e7fed61727b3de4019e9702d4090ce
Tree-SHA512: 3be9d6b313bf6bb835f031826c81777b4659118d839001d084e72462391cb64ba81d06a5e07fd21fcfb709a71b08892b23212a98604ce8481da489476b72f072
e177fcab3831b6d259da5164cabedcc9e78f6957 Replace `struct update_lock_points` with lambda (glozow)
c7cd98c7176800a51e6a6b3634a26b508aa33ff2 document and clean up MaybeUpdateMempoolForReorg (glozow)
Pull request description:
followup to #23683, addressing https://github.com/bitcoin/bitcoin/pull/23683#issuecomment-989741186
ACKs for top commit:
hebasto:
ACK e177fcab3831b6d259da5164cabedcc9e78f6957, I have reviewed the code and it looks OK, I agree it can be merged.
instagibbs:
ACK e177fcab3831b6d259da5164cabedcc9e78f6957
MarcoFalke:
Approach ACK e177fcab3831b6d259da5164cabedcc9e78f6957 😶
Tree-SHA512: 8c2709dd5cab73cde41f3e5655d5f237bacfb341f78eac026169be579528695ca628c8777b7d89760d8677a4e6786913293681cfe16ab702b30c909703e1824c
65efbba45d817261f590d043c69a9981e6b637bd rpcwallet: mention labels are deactivated for ranged descriptors (Antoine Poinsot)
Pull request description:
It was confusing when trying to use it as a blackbox. So mention it so next ones don't have to open the said box :)
See #23749 for context
ACKs for top commit:
Sjors:
utACK 65efbba45d817261f590d043c69a9981e6b637bd
achow101:
ACK 65efbba45d817261f590d043c69a9981e6b637bd
Tree-SHA512: d8a3d1f81c16d95855ac2b01e8fd20e83d6dac1721b3da464a9a890e46102992a6882918be87b2a28b929349ee7f1beb1af6c88b22f065fbbb6948275a6d2b8f
fa62207737657e76ba45d5bf826fc0ccac658df6 test: Return the largest utxo in MiniWallet.get_utxo (MarcoFalke)
Pull request description:
This is for consistency with the `send_self_transfer` method.
Also, remove the feature that the change of the last transfer can be retrieved via `get_utxo`. This can trivially and clearer be achieved by simply passing the txid of the transfer.
Also, this fixes the bug in `feature_txindex_compatibility` in current master after a silent merge conflict.
Fixes#23514
Top commit has no ACKs.
Tree-SHA512: edd066d372aaa72b4e0fc7526f84931c8d1f6d14f53678cb7832bc8e3d211f44b90ec9c59b7d915ef24acc63a36e7d66c8d3b7598355bd490ac637ed3bcc3dff
58765a450c40152db8160bca8a6b0f5b754c5858 qt: Use only Qt translation primitives in GUI code (W. J. van der Laan)
Pull request description:
Use `Object::tr`, `QT_TRANSLATE_NOOP`, and `QCoreApplication::translate` as appropriate instead of using `_()` which doesn't get picked up.
Replaces bitcoin/bitcoin#22764
Edit: I checked that the strings end up in the appropriate context in `bitcoin_en.ts` after `make translate`:
- "Settings file could not be read" "Settings file could not be written" end up in `bitcoin-core`
- "(press q to shutdown and continue later)" and "press q to shutdown" end up in `SplashScreen`
ACKs for top commit:
hebasto:
ACK 58765a450c40152db8160bca8a6b0f5b754c5858
Tree-SHA512: d0cc5901426c47d411d0fe75aac195b9684b51385f74c161da772dbf9f0977f7ad7a0976e17366f49b40718e9b6eb87c3e93306dc845f97adddbc47d00731742
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
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
We need to push DIP-0001's activation height ahead as post-DIP-0001
consensus rejects transactions greater than `MAX_STANDARD_TX_SIZE`
(`bad-txns-oversize`) and this test generates transactions nearly 9.5x
that.
We also need to push v20 activation ahead to avoid different subsidy
calculation logic (`bad-cb-amount`).
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
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
This was spotted when working on `feature_txindex_compatibility.py`,
attempting to load old databases would fail because `MigrateDBIfNeeded()`
would delete `DB_OLD_BEST_BLOCK`, write `EVODB_BEST_BLOCK`, commit it
but never flush it.
So when `MigrateDBIfNeeded2()` would read it again, it would note the lack
of `DB_OLD_BEST_BLOCK` and conclude it was a failed run and exit. This
is solved by actually flushing our new best block, which would prevent
`MigrateDBIfNeeded2` from raising an objection.
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
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
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
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
